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...@amd.com wrote:
From: "Leo (Sunpeng) Li" <sunpeng...@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.


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to