Re: [PATCH 00/21] OMAPDSS: DISPC changes for writeback pipeline

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

On Fri, 2012-09-14 at 15:43 +0530, Archit Taneja wrote:
> On Friday 14 September 2012 01:57 PM, Tomi Valkeinen wrote:

> I initially kept all of this the same, but I changed my mind at some 
> point, not totally sure why. Even if we stick to the dispc_ovl_* names, 
> we would still need to create q common function which dispc_ovl_setup() 
> and dispc_wb_setup() could call. I called this dispc_plane_setup(), and 
> then it felt weird to call everything else ovl specifuic, hence renamed 
> all of them to dispc_plane_*.
> 
> Could you suggest a better name than dispc_plane_setup?

Well... dispc_ovl_setup_common?

The function is also quite big, with huge number of arguments. Makes me
wonder if we could split it up to some sensible parts. Would it be
possible to have functions to setup, say, input related parameters
(base-address, pix format, etc.), output related parameters (ovl
position, ...).

Well, it could just make it more confusing, as some things are shared
between input and output, like scaling related things. But just an idea.

> 
> >
> >> functions. The next few patches change how overlay caps are used within the
> >> dispc functions, this helps reusing more functions between overlays and
> >
> > I dislike this a bit, I think dispc driver should know what HW it has,
> > you shouldn't need to pass caps to it. So I'd prefer the dispc driver to
> > to have this information in dispc_features. I believe all OVL_CAPS
> > should be there, and then exported to other drivers via some means. I
> > guess this means could for now be just initializing ovl->caps with data
> > from dispc.c.
> 
> Currently, we pass the plane id to these low level functions, it 
> extracts out the ovl struct usingthe plane id, and checks the ovl caps.
> 
> What I'm doing now is just passing the caps directly to these low level 
> functions. So that I don't need to have complicated checks in every 
> function to extract caps between overlays or writeback.

Yep, I see. It's ok.

My main dislike is the use of omap_dss_get_overlay() in dispc.c. I'd
like dispc.c to be self-contained, so what I mean is that instead of
initializing the caps in dss_features.c, and calling the above function
in dispc.c, we should have a dispc.c internal table for dispc's HW,
which would contain the caps and other necessary information.

But that's not really related to this series.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]