Re: [PATCH 02/11] drm/i915: Register pipe level color properties

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Jul 23, 2014 at 11:34:56PM +0530, shashank.sharma@intel.com wrote:
> From: Shashank Sharma <shashank.sharma@intel.com>
> 
> In valleyview we have two pipe level color correction
> properties:
> 1. CSC correction (wide gamut)
> 2. Gamma correction
> 
> What this patch does:
> 1. This patch adds software infrastructure to register pipe level
>    color correction properties per CRTC. Adding a new function,
>    intel_attach_pipe_color_correction to register the pipe level
>    color correction properties with the given CRTC.
> 2. Adding a pointer in intel_crtc structure to store this property.
> 3. Adding structure gen6_pipe_color_corrections, which contains different
>    pipe level correction values for VLV.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
...snip...
> +struct drm_property *intel_clrmgr_register(struct drm_device *dev,
> +	struct drm_mode_object *obj, struct clrmgr_property *cp)
> +{
> +	struct drm_property *property;
> +
> +	/* Create drm property */
> +	switch (cp->type) {
> +	case DRM_MODE_PROP_BLOB:
> +		property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +			cp->name, cp->len);
> +		if (!property) {
> +			DRM_ERROR("Failed to create property %s\n", cp->name);
> +			goto error;
> +		}
> +		break;
> +
> +	case DRM_MODE_PROP_RANGE:
> +		property = drm_property_create_range(dev, DRM_MODE_PROP_RANGE,
> +			cp->name, cp->min, cp->max);
> +		if (!property) {
> +			DRM_ERROR("Failed to create property %s\n", cp->name);
> +			goto error;
> +		}
> +		break;
> +
> +	default:
> +		DRM_ERROR("Unsupported type for property %s\n", cp->name);
> +		goto error;
> +	}
> +	/* Attach property to object */
> +	drm_object_attach_property(obj, property, 0);
> +	DRM_DEBUG_DRIVER("Registered property %s\n", property->name);
> +	return property;
> +
> +error:
> +	DRM_ERROR("Failed to create property %s\n", cp->name);
> +	return NULL;
> +}

If I'm reading this right, you're creating a unique property for each
DRM object (crtc in this case) that you work with.  I.e., if you have
three pipes, then you wind up creating three CSC and three gamma
properties total.  This isn't really the way properties are meant to be
used...  A property describes a type of data that you want to store on a
per-object basis, but doesn't actually store anything itself.  You can
then attach that single property to multiple objects and all of those
objects will track their own value of the quantity described by the
property.  It's sort of like the difference between classes and objects
in OOP --- a DRM property is sort of like a class (just describes
something you plan to store) and attaching that property to a CRTC (or
plane, or connector) is like instantiating a new object of the class.

You should really only have two properties being created by the driver
here...one for CSC and one for Gamma.  Once you've created a property
once, you'll attach it to all of your CRTC's and they'll all manage
their own values.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795