[PATCH xf86-video-amdgpu 02/13] Push color properties to kernel DRM on CRTC init

[Michel Dänzer]

On 2018-05-03 08:31 PM, sunpeng.li at amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li at 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.li at 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.


> +/**
> + * 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).


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


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