Re: [PATCH 00/11]: Color manager framework for I915 driver

["Sharma, Shashank" <shashank.sharma@intel.com>]

Thanks for your time, and review comments Matt.

I appreciate the suggestions by you, Daniel.
But I need a few more details, and I have a few concerns with the design 
you suggest, please find my comments inline.

Regards
Shashank
On 7/25/2014 6:13 AM, Matt Roper wrote:
> On Thu, Jul 24, 2014 at 04:08:41AM +0000, Sharma, Shashank wrote:
>> Hi Daniel,
>> Thanks for your time and comments.
>>
>> The current design is exactly same as we discussed previously in the mail chain, color manager is just the framework which does this job:
>> 1.  Create a DRM property for requesting object.
>> 2.   Attach the DRM property with the object.
>
> I didn't see Daniel's response when I sent my other message, but I had a
> lot of the same thoughts that he brought up.  I think my previous email
> explains one of the concerns here --- properties don't hold values, so
> you only need to create a property once total (well, technically once
> per DRM device), not once per object.
>
> Once you stop creating duplicate properties, you don't really need the
> color manager framework at all.  Just find an appropriate driver init
> function and create each property once, storing the property pointer
> somewhere in dev_priv (or, if these properties can become cross-driver
> standard properties, they'd be created once by the DRM core and stored
> in drm_device).
>
Matt, do you suggest to create one DRM property named CSC for all pipes 
? And one drm property named Gamma for all pipes ?
Can you please elaborate a bit more in this part: "Create a DRM property 
and attach to ech CRTC, managing their own values."

In this design the current design, I have few concerns here, on your 
suggestion:
1. If I enable gamma correction on one pipe (pipe A, driving DSI 
display) but don't apply gamma correction on other (pipe B, driving 
HDMI), how to maintain the state of each, without adding additional 
complexity in the driver. I have to create some additional data 
structure and attach to dev_priv.

2. The previously applied values are to be stored somewhere, to be 
re-stored in case of suspend/resume.

Plus, If I create a core DRM property for each of the color corrections, 
not all HWs running DRM driver will have properties like CSC, Gamma, Hue 
and Saturation, Contrast, Brightness. It would be a waste for them to 
have this.
>> There is no other job done here in the framework, no parsing and nothing else.
>> The color manager data structures also just add and array of DRM properties for an object (CRTC/PIPE) and total no of DRM properties.
>> So there is nothing which is not required.
>>
>> Typical sequence of how it works:
>> 	1. intel CRTC init calls color-manager init for that CRTC, gets a color pointer, which has space to save DRM properties.
>> 	2. intel CRTC init calls attach color properties, which will register the DRM property, add into the color pointer, and return.
>
> CRTC init can just attach the (already created as described above)
> properties to the new CRTC being created.  No special color manager
> interface calls needed.
>
>> 	3. A CRTC set property checks if this is color property, calls color-manager-set-property.
>> 	4. Color manager set-property calls core set property function, which takes care of calling platform specific set_propety function.
>
> This level of indirection seems unnecessary.
> intel_{crtc,plane}->set_property() can just point at functions that just
> do:
>
>          if (property == dev_priv->foo_property) {
>                  // do foo stuff;
>          } else if (property == dev_priv->bar_property) {
>                  // do bar stuff;
>          } else if (property == dev_priv->baz_property) {
>                  // do baz stuff;
>          } ...
>
> The properties you're adding now as part of the "color manager" will
> likely be joined by other, unrelated propeties in the future.  There's
> no need to isolate "color manager" properties behind another level of
> function pointer abstraction.
>
The main problems I see here is:
1. drm property doesn't hold a space to have a .set_property callback, 
so we have to hardcode this every time, based on the platform and 
property. For example, Gamma correction method in VLV and big core is 
entirely different. Some platfroms might not even support 
gamma_corerction, but some might support advance gamma_corerction.

2. If I have 10 properties, I keep on populating dev_priv with 10 
pointers, for each property, and end up writing a lot of if (property == 
dev_priv->some_property), do something.
Seeing the increase of no of DRM properties, this doesn't look good.

3. every time there is a new platform or there is a new color property 
added, I have to modify these places intel_crtc_set_property, 
intel_plane_set_property, and  add/remove one if condition.

Now, if we use color-manager, it would provide solutions for them in 
this way (also explains the need of color-manager data structures):

1. add a container data structure name color_property, which just holds 
a drm_property*, a bool status (to show if this current property is 
enabled/ adjusted /corrected or not), and the .set_property function to 
route it to appropriate set_property method.
struct clrmgr_property {
	bool enabled;
	struct drm_property *property;
	.set_handler();
}

2. Add only one pointer in dev_priv OR intel_crtc OR intel_plane
named "color_status", which is an array of all registered color 
properties with this drm_object, and a simple count of how many of them.
intel_crtc->color_status = color_manager_init();
struct clrmgr_status {
	uint count;
	struct color_property cp[max_allowed];
}

3. In case of a new platform / SOC, with different color correction 
properties, all I have to do is add an array for this platform in 
color_manager.c, like gen6_color_properties.
The color_manager_regsiter_pipe/plane function is written in such a way 
that, it will take care of everything automatically. No need to modify 
code anywhere else.

struct clrmgr_property gen_x_color_properties = {
	{"csc", no_of_values, .set_prop},
	{"gamma", no_of_values, .set_prop},
	{"X", no_of_values, .set_prop},
	{"Y", no_of_values, .set_prop}
}

struct clrmgr_property gen_y_color_properties = {
	{"csc", no_of_values, .set_prop},
	{"A", no_of_values, .set_prop},
	{"B", no_of_values, .set_prop}
}

color_property_regsiter_function
{
	/ * take out color properties as per gen (Even this can be 	
	automated using proper indexing), and create
	drm properties in a loop.*/
	while (arra_size(gen_x_color_prop)) {
		/* extract_color_properties_from_array and create DRM 	
		properties one by one */
	}
}

Now, a simple crtc_set_property/plane_set_property function will do 
this: crtc_set_property:
	struct clrmgr_status status = intel_crtc->_color_status;
	while (count < status->no_of_prop) {
		if (prop == status->cp[count]->property)
			.set_handler();
	}

>> 	5. Color manager exit gets call from CRTC/plane destroy, and frees the DRM property per CRTC/plane, plus color pointer.
>
> As with init, these can just be moved to the proper crtc/plane tear down
> functions; no need to pull them out into separate color manager
> functions.
>
>> Can you please point out, which of the above steps is not falling in line for you?
>
> I think Daniel's big point is that the i915 driver has (or will
> eventually have) lots of crtc and plane properties that do various
> things.  You're pulling some of those properties out into a separate
> framework that you call "color manager" that simply adds indirection.
> But that extra indirection doesn't really add any value; the DRM core,
> with its property support, is already all the framework we need.
>
>
> Matt
>
Yes, in fact I a agree with Daniel and you, on this point. And ideally I 
feel that all the properties(color, non-color) should follow this same 
approach to register/unregister, and we will all be spared from changing 
the code every time, and with a lot of un-necessary pointers in dev_private.
But I thought it was too ambitious to expect a change in all the 
drm_properies, so I though of starting from similar properties, from a 
domain, which is color-correction. If you guys like the approach, we can 
think of registering all the properties in this way.

Please let me know about this approach.