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

[Leo Li <sunpeng.li-5C7GfCeVMHo@public.gmane.org>]

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