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

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

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

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

> 	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

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