[PATCH xserver 1/5] modesetting: Don't call xf86HandleColorMaps() at screen depth 30.

[Michel Dänzer]

On 2018-02-09 07:24 AM, Mario Kleiner wrote:
> On 02/08/2018 03:55 PM, Michel Dänzer wrote:
>> On 2018-02-08 12:14 PM, Mario Kleiner wrote:
>>> As it turns out, doing so will make any gamma table updates
>>> silently fail, because xf86HandleColorMaps() hooks the
>>> .LoadPalette function to xf86RandR12LoadPalette() if the
>>> .gamma_set function is supported by the ddx, as is in our
>>> case.
>>>
>>> Once xf86RandR12LoadPalette() has been called during server
>>> startup, all palette and/or gamma table updates go through
>>> xf86RandR12CrtcComputeGamma() to combine color palette
>>> updates with gamma table updates into a proper hardware lut
>>> for upload into the hw via the .gamma_set function/ioctl,
>>> passing in a palette with palette_red/green/blue_size == the
>>> size given by the visuals red/green/blueMask, which will
>>> be == 1024 for a depth 30 screen.
>>>
>>> That in turn is a problem, because the size of the hw lut
>>> crtc->gamma_size is fixed to 256 slots on all kms-drivers
>>> when using the legacy gamma_set ioctl, but
>>> xf86RandR12CrtcComputeGamma() can only handle palettes of a
>>> size <= the hw lut size. As the palette size of 1024 is greater
>>> than the hw lut size of 256, the code silently fails
>>> (gamma_slots == 0 in xf86RandR12CrtcComputeGamma()).
>>>
>>> Skipping xf86HandleColormaps() on a depth > 24 screen disables
>>> color palette handling, but keeps at least gamma table updates
>>> via xf86vidmode extension and RandR working.
>>
>> Sort of... It means xf86VidMode and RandR call directly into the driver
>> to set their LUTs, with no coordination between them, so whichever calls
>> into the driver last wins and clobbers the HW LUT.
> 
> Wouldn't that be desirable behavior in this case, assuming the client
> that calls last is the one that wants something more specific?

If it was that simple... :)

Imagine you use something like Night Light / redshift which uses
xf86VidMode, and some kind of per-monitor gamma calibration tool which
uses RandR. Surely you want to see the combination of the two
transformations, not only one or the other, depending on which one has
ended up calling into the driver last (which could result in
flip-flopping between the two transformations, depending on how often
each tool (re-)sets its LUT).


>> It would be better to fix xf86RandR12CrtcComputeGamma instead.
> 
> But how? The current code can upsample an input palette < hw lut by
> replicating input values onto multiple hw slots, which makes perfect
> sense. But downsampling a bigger input palette, e.g., by just picking
> every n'th slot, seems problematic to me. You lose information from the
> palette in a way that might not be at all what the application wanted if
> it went to the trouble of creating a custom palette, assuming any apps
> would do anything meaningful with palettes on a depth 30 screen in the
> first place.

You're right that the downsampling could theoretically be a problem. But
in practice, the LUTs should usually correspond to some kind of more or
less smooth curve, and I'm told display hardware tends to interpolate
between the LUT entries.


> Unless the application just used the default palette or a
> truecolor visual, like probably most do nowadays, which i assume would
> be an identity mapping, so it doesn't matter if a palette is applied at
> all?

No colormaps means e.g. the gamma sliders in some games don't work.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer