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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
