[PATCH xf86-video-amdgpu 1/5] Add functions for changing CRTC color management properties

[Michel Dänzer]

On 2018-04-10 08:02 PM, Leo Li wrote:
> On 2018-04-09 11:03 AM, Michel Dänzer wrote:
>> On 2018-03-26 10:00 PM, sunpeng.li at amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>>
>>> This change adds a few functions in preparation of enabling CRTC color
>>> managment via the randr interface.
>>>
>>> The driver-private CRTC object now contains a list of properties,
>>> mirroring the driver-private output object. The lifecycle of the CRTC
>>> properties will also mirror the output.
>>>
>>> Since color managment properties are all DRM blobs, we'll expose the
>>> ability to change the blob ID. The user can create blobs via libdrm
>>> (which can be done without ownership of DRM master), then set the ID via
>>> xrandr. The user will then have to ensure proper cleanup by subsequently
>>> releasing the blob.
>>
>> That sounds a bit clunky. :)
>>
>> When changing a blob ID, the change only takes effect on the next atomic
>> commit, doesn't it? How does the client trigger the atomic commit?
> 
> From the perspective of a client that wishes to change a property, the
> process between regular properties and blob properties should be
> essentially the same. Both will trigger an atomic commit when the DRM
> set property ioctl is called from our DDX driver.

That doesn't sound right — if every set property ioctl call implicitly
triggered an atomic commit, the KMS atomic API wouldn't be "atomic" at all.

Do you mean that the DDX driver triggers an atomic commit on each
property change? How is that done?


> The only difference is that DRM property blobs can be arbitrary in size,
> and needs to be passed by reference through its DRM-defined blob ID.
> Because of this, the client has to create the blob, save it's id, call
> libXrandr to change it, then destroy the blob after it's been committed.
> 
> The client has to call libXrandr due to DRM permissions. IIRC, there can
> be only one DRM master. And since xserver is DRM master, an external
> application cannot set DRM properties unless it goes through X. However,
> creating and destroying DRM property blobs and can be done by anyone.
> 
> Was this the source of the clunkiness? I've thought about having DDX
> create and destroy the blob instead, but that needs an interface for the
> client to get arbitrarily sized data to DDX. I'm not aware of any good
> ways to do so. Don't think the kernel can do this for us either. It does
> create the blob for legacy gamma, but that's because there's a dedicated
> ioctl for it.

Maybe there are better approaches than exposing these properties to
clients directly. See also my latest follow-up on patch 3.


>>> @@ -1604,6 +1623,18 @@ static void drmmode_output_dpms(xf86OutputPtr
>>> output, int mode)
>>>       }
>>>   }
>>>   +static Bool drmmode_crtc_property_ignore(drmModePropertyPtr prop)
>>> +{
>>> +    if (!prop)
>>> +        return TRUE;
>>> +    /* Ignore CRTC gamma lut sizes */
>>> +    if (!strcmp(prop->name, "GAMMA_LUT_SIZE") ||
>>> +        !strcmp(prop->name, "DEGAMMA_LUT_SIZE"))
>>> +        return TRUE;
>>
>> Without these properties, how can a client know the LUT sizes?
> 
> Good point, I originally thought the sizes are fixed and did not need
> exposing. But who knows if they may change, or even be different per asic.

AFAIK at least Intel hardware already has different sizes in different
hardware generations. We should avoid creating an API which couldn't
also work for the modesetting driver and generic clients.


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