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

[Leo Li]

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.

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.

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

> 
> 
>> @@ -1618,6 +1649,163 @@ static Bool drmmode_property_ignore(drmModePropertyPtr prop)
>>   	return FALSE;
>>   }
>>   
>> +/**
>> +* Configure and change the given output property through randr. Currently
> 
> "RandR"
> 
>> +* ignores DRM_MODE_PROP_ENU property types. Used as part of create_resources.
> 
> DRM_MODE_PROP_ENUM is missing the final M.
> 
>> +*
>> +* Return: 0 on success, X-defined error codes on failure.
>> +*/
>> +static int __rr_configure_and_change_property(xf86OutputPtr output,
>> +					      drmmode_prop_ptr pmode_prop)
> 
> No leading underscores in function names please. >
> 
>> +	}
>> +	else if (mode_prop->flags & DRM_MODE_PROP_RANGE) {
> 
> The else should be on the same line as }.
> 
> 
>> +static void drmmode_crtc_create_resources(xf86CrtcPtr crtc,
>> +					  xf86OutputPtr output)
>> +{
>> +	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(crtc->scrn);
>> +	int i, j;
>> +
>> +	/* 'p' prefix for driver private objects */
>> +	drmmode_crtc_private_ptr pmode_crtc = crtc->driver_private;
> 
> Existing code refers to this as drmmode_crtc, please stick to that.
> 
> 
>> +	drmModeCrtcPtr mode_crtc = pmode_crtc->mode_crtc;
>> +
>> +	drmmode_prop_ptr pmode_prop;
>> +	drmModePropertyPtr mode_prop;
>> +
>> +	/* Get list of DRM CRTC properties, and their values */
>> +	drmModeObjectPropertiesPtr mode_props;
> 
> All local variable declarations should be in a single block, with no
> blank lines between them, and generally sorted from longer lines to
> shorter ones.
> 
> 
>> +	mode_props = drmModeObjectGetProperties(pAMDGPUEnt->fd,
>> +						mode_crtc->crtc_id,
>> +						DRM_MODE_OBJECT_CRTC);
>> +	if (!mode_props)
>> +		goto err_allocs;
>> +
>> +	/* Allocate, then populate the driver-private CRTC property list */
>> +	pmode_crtc->props = calloc(mode_props->count_props + 1,
>> +				     sizeof(drmmode_prop_rec));
> 
> Continuation lines should be aligned to opening parens. Any editor which
> supports EditorConfig should do this automagically.
> 
> 
>> +	if (!pmode_crtc->props)
>> +		goto err_allocs;
>> +
>> +	pmode_crtc->num_props = 0;
>> +
>> +	/* Filter through drm crtc properties for relevant ones, and save
>> +	 * them */
> 
> 	/* Multi-line comments should be formatted like this.
> 	 * Comment end marker on a separate line:
> 	 */
> 
> 
>> +err_allocs:
>> +	xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
>> +		   "Memory error creating resources for CRTC %d\n",
>> +		   mode_crtc->crtc_id);
>> +	drmModeFreeObjectProperties(mode_props);
>> +	return;
>> +}
> 
> Remove the superfluous return statement at the end of a function
> returning void.

Will fix all style problems.

Leo

> 
>