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

[Leo Li]

On 2018-04-11 04:39 AM, Michel Dänzer wrote:
> 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?
> 

Yes,

In drmmode_display.c: drmmode_output_set_property(), a call is made to
drmModeConnectorSetProperty() [libdrm].

And if drmmode_crtc_set_property() is considered, then a call can be
made to drmModeObjectSetProperty() [libdrm].

Both of these functions trigger an ioctl that eventually calls
drm_mode_obj_set_property_ioctl() within the kernel. It will trigger an
atomic commit, if the kernel driver supports it.

To be "truly atomic", the set of properties that DDX wish to change must
be compiled together using drmModeAtomicAddProperty(), then committed
all at once via drmModeAtomicCommit(). Intel's IGT test suite has a good
example of this, within igt_atomic_commit() here:
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_kms.c#n2997
The for-loops will add the properties, which are the committed at the end.

> 
>> 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.
> 

I'll reply to this in patch 3 as well.

Leo

> 
>>>> @@ -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.
> 
>