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