[Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver

[Sharma, Shashank]

Thanks a lot for your time and the detailed explanation, Matt. 
I will re-design the patches according to your / Daniel's suggestions. 

Regards
Shashank
-----Original Message-----
From: Roper, Matthew D 
Sent: Saturday, July 26, 2014 7:29 AM
To: Sharma, Shashank
Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org; Vetter, Daniel
Subject: Re: [Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver

On Fri, Jul 25, 2014 at 10:06:27AM +0530, Sharma, Shashank wrote:
> 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 ?

Yep, that's what I meant.

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

The property stuff can be pretty confusing at first glance since it doesn't quite work like a lot of people intuitively expect it to --- when you set a property value, that value gets stored with the object itself, not with the property.  I think it's actually a little bit easier to understand if you dig through the DRM object data structures.
Note that drm_crtc/drm_plane/drm_connector are all derived from drm_mode_object (using C-style inheritance).  And drm_mode_object contains a field

        struct drm_object_properties *properties;

The definition of drm_object_properties is:

        struct drm_object_properties {
                int count;
                uint32_t ids[DRM_OBJECT_MAX_PROPERTY];
                uint64_t values[DRM_OBJECT_MAX_PROPERTY];
        };

so you can see that each object (crtc, plane, connector) has a table of property ID's and values inside of it.

When you call one of the drm_property_create functions, the important thing that happens is an ID number gets assigned to the new property you created.  Then you can go attach that single property to as many other objects (planes, crtc's, connectors) as you want.  The
drm_object_attach_property() function just adds a new entry to the ids[] and values[] arrays shown above for the specific object.

So for example, let's assume that you modified your patch to only create a single gamma property, as we described.  And for the purposes of this example, let's pretend that when your code actually runs, gamma happens to get assigned an ID number 7 by the DRM core code.  You would then proceed to attach that gamma property to both of your BayTrail CRTC's.
After doing so, there would be some values i and j such that
        crtc1->base.properties->ids[i] == 7
        crtc2->base.properties->ids[j] == 7 In this case, your two gamma values for the two CRTC's would be stored internally in
        crtc1->base.properties->values[i]
                and
        crtc2->base.properties->values[j] and can be updated independently.

(Note that there are some patches on the dri-devel mailing list from Rob Clark that refactor some of the internal structures described above in preparation for atomic modeset, but the general idea remains the same.)

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

Right, the values that you apply get stored in each CRTC itself as

        crtc->base.properties->values[i]

If you need to get the value back again later in your driver code, you can retrieve the value for a property associated with a specific CRTC with the drm_object_property_get_value() function.

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

True, not all hardware may support all properties.  I don't personally know enough about color correction on non-Intel platforms to know whether your specific properties here make sense for general standardization or not.  For items that do apply to all hardware, see drm_mode_create_standard_plane_properties and drm_mode_create_standard_connector_properties for examples of how/where we create the properties and stick them in the drm_device.  If a property is general enough to make sense cross-driver, but may not apply to all platforms supported by a driver or all crtc's/planes, then you can add some additional logic to decide whether you want to attach it to a given crtc or not.

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

The i915 driver already sort of has a pattern that gets used when you have a generic driver entrypoint that needs to eventually dispatch to platform-specific logic.  If you look in intel_display.c, there's a function intel_init_display() that looks at your device ID and then sets up some platform-specific function pointers.  So you might add a new dev_priv->display.set_gamma field and hook up appropriate vlv_set_gamma(), hsw_set_gamma(), i9xx_set_gamma(), etc. implementations for whatever platforms you're exposing the property on.  Then your main
intel_crtc->set_property() function that I gave pseudocode for above could become more like:

         if (property == dev_priv->gamma_property) {
                 dev_priv->display.set_gamma(crtc);
         } else if (property == dev_priv->csc_property) {
                 dev_priv->display.set_csc(crtc);
         } ...

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

I don't see that as too much of a problem since, as I showed above, you're probably not actually going to put the entire implementation directly in the main set_plane handler, but rather call off to platform-specific calls.  So there might be a lot of properties as time goes on, but the code still stays fairly compact and easy to read.  The main difference from your initial code is that the platform-specific functions are following the same model that's already used in the driver for other things like crtc enable/disable, setplane, modeset, etc.
rather than using a new layer of indirection that works a bit differently.

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

As I noted above, the platform-specific dev_priv->display dispatch table helps with this.  It should provide the same form of separation / abstraction that you were shooting for with your color manager code, just in a manner that's a little bit more in line with how other parts of i915 already work.

I think the above explanations should also cover the points you list below, but if you still have concerns or questions about any of them, feel free to ask.


Matt

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

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