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

[Michel Dänzer]

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?


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


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


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