On 2018-05-16 01:08 PM, Michel Dänzer wrote:
On 2018-05-03 08:31 PM, sunpeng...@amd.com wrote:
From: "Leo (Sunpeng) Li" <sunpeng...@amd.com>

Push staged values on the driver-private CRTC, to kernel DRM when it's
initialized. This is to flush out any previous state that hardware was
in, and set them to their default values.

Signed-off-by: Leo (Sunpeng) Li <sunpeng...@amd.com>
---
  src/drmmode_display.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 136 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 0ffc6ad..85de01e 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -824,6 +824,133 @@ err_allocs:
        return 0;
  }
+/**
+ * Query DRM for the property ID - as recognized by DRM - for the specified
+ * color management property, on the specified CRTC.
+ *
+ * @crtc: The CRTC to query DRM properties on.
+ * @prop_id: Color management property enum.
+ *
+ * Return the DRM property ID, if the property exists. 0 otherwise.
+ */
+static uint32_t get_drm_cm_prop_id(xf86CrtcPtr crtc,
+                                  enum drmmode_cm_prop prop_id)
+{
+       AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+       drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+       drmModeObjectPropertiesPtr drm_props;
+       int i;
+
+       drm_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
+                                              drmmode_crtc->mode_crtc->crtc_id,
+                                              DRM_MODE_OBJECT_CRTC);
+       if (!drm_props)
+               goto err_allocs;
+
+       for (i = 0; i < drm_props->count_props; i++) {
+               drmModePropertyPtr drm_prop;
+
+               drm_prop = drmModeGetProperty(pAMDGPUEnt->fd,
+                                             drm_props->props[i]);
+               if (!drm_prop)
+                       goto err_allocs;
+
+               if (get_cm_enum_from_str(drm_prop->name) == prop_id){
+                       drmModeFreeProperty(drm_prop);
+                       return drm_props->props[i];
+               }
+
+               drmModeFreeProperty(drm_prop);
+       }
+
+err_allocs:
+       drmModeFreeObjectProperties(drm_props);
+       return 0;
+}

It seems a bit heavy to call drmModeObjectGetProperties and
drmModeGetProperty (which both in turn call into the kernel) every time
here. Assuming the property IDs don't change at runtime, they could be
cached.


Hmm, good point. I took a look at the DRM property code, and indeed the
ID's don't change.


+/**
+ * Push staged color management properties on the CRTC to DRM.
+ *
+ * @crtc: The CRTC containing staged properties
+ * @cm_prop_index: The color property to push
+ *
+ * Return 0 on success, X-defined error codes on failure.
+ */
+static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,
+                                    enum drmmode_cm_prop cm_prop_index)
+{
+       AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
+       drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+       size_t expected_bytes = 0;
+       uint32_t created_blob_id = 0;
+       void *blob_data = NULL;
+       uint32_t drm_prop_id;
+       Bool free_blob_data = FALSE;

free_blob_data is always FALSE. If that's intended, it shouldn't be
added (in this patch yet).


Must have missed this while preparing the patches, will move to patch 6.


@@ -1458,6 +1586,14 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_res
        drmmode_crtc->ctm->matrix[0] = drmmode_crtc->ctm->matrix[4] =
                drmmode_crtc->ctm->matrix[8] = (uint64_t)1 << 32;
+ /* Push properties to initialize them */
+       for (i = 0; i < CM_NUM_PROPS; i++) {
+               if (i == CM_DEGAMMA_LUT_SIZE || i == CM_GAMMA_LUT_SIZE)
+                       continue;
+               if (drmmode_crtc_push_cm_prop(crtc, i))
+                       return 0;

Per my follow-up to the cover letter, I don't think this should be
necessary here anyway, but FWIW:

Returning 0 early here breaks the pAMDGPUEnt->assigned_crtcs related
logic in this function and drmmode_pre_init.


I originally thought it'd make sense if DDX pushes the properties on
init, to maintain consistency between what DDX initially reports, and
what DRM is using. It would also reset color properties if X was restarted.

The other way around could also work, which I assume is what you're
suggesting? i.e. pull all color properties from DRM into the
driver-private crtc on crtc init, instead of pushing it to kernel?

Leo


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

Reply via email to