On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> Hi,
>
> couple of minor comments inlined.
>
> On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki)
> wrote:
> > From: Ville Syrjälä <[email protected]>
> >
> > Separate the memory region from the framebuffer device a little bit.
> > It's now possible to select the memory region used by the framebuffer
> > device using the new mem_idx parameter of omapfb_plane_info. If the
> > mem_idx is specified it will be interpreted as an index into the
> > memory regions array, if it's not specified the framebuffer's index is
> > used instead. So by default each framebuffer keeps using it's own
> > memory region which preserves backwards compatibility.
> >
> > This allows cloning the same memory region to several overlays and yet
> > each overlay can be controlled independently since they can be
> > associated with separate framebuffer devices.
> >
> > Signed-off-by: Ville Syrjälä <[email protected]>
> > ---
> > Changes since v2:
> > * Removed the use_count and rely on just counting all active overlays. A
> > bit racy
> > but no chance of getting stuck in a state where memory allocation can't
> > be changed.
> > * s/source_idx/mem_idx as that seems to be a little more self explanatory
> > [...]
> >
> > drivers/video/omap2/omapfb/omapfb-ioctl.c | 163 ++++++++++++++++++++-----
> > drivers/video/omap2/omapfb/omapfb-main.c | 184
> > +++++++++++++++++++----------
> > drivers/video/omap2/omapfb/omapfb-sysfs.c | 60 ++++++++--
> > drivers/video/omap2/omapfb/omapfb.h | 38 ++++++-
> > include/linux/omapfb.h | 5 +-
> > 5 files changed, 339 insertions(+), 111 deletions(-)
> >
> > diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > index 1ffa760..cd00bdc 100644
> > --- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > +++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > [...]
> > static int omapfb_query_plane(struct fb_info *fbi, struct
> > omapfb_plane_info *pi)
> > {
> > struct omapfb_info *ofbi = FB2OFB(fbi);
> > + struct omap_overlay *ovl = ofbi->overlays[0];
> > + struct omap_overlay_info *ovli = &ovl->info;
> >
> > if (ofbi->num_overlays != 1) {
> > memset(pi, 0, sizeof(*pi));
> > } else {
> > - struct omap_overlay_info *ovli;
> > - struct omap_overlay *ovl;
> > -
> > - ovl = ofbi->overlays[0];
> > - ovli = &ovl->info;
> > -
>
> Is this really necessary?
Nope. Leftovers from some earlier stuff I think.
>
> > pi->pos_x = ovli->pos_x;
> > pi->pos_y = ovli->pos_y;
> > pi->enabled = ovli->enabled;
> > pi->channel_out = 0; /* xxx */
> > pi->mirror = 0;
> > + pi->mem_idx = get_mem_idx(ofbi);
> > pi->out_width = ovli->out_width;
> > pi->out_height = ovli->out_height;
> > }
> > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi,
> > struct omapfb_mem_info *mi)
> > struct omapfb_info *ofbi = FB2OFB(fbi);
> > struct omapfb2_device *fbdev = ofbi->fbdev;
> > struct omapfb2_mem_region *rg;
> > - int r, i;
> > + int r = 0;
> > size_t size;
> > + int i;
> >
> > if (mi->type > OMAPFB_MEMTYPE_MAX)
> > return -EINVAL;
> >
> > size = PAGE_ALIGN(mi->size);
> >
> > - rg = &ofbi->region;
> > + rg = ofbi->region;
> >
> > - for (i = 0; i < ofbi->num_overlays; i++) {
> > - if (ofbi->overlays[i]->info.enabled)
> > - return -EBUSY;
> > + /* FIXME probably should be a rwsem ... */
> > + mutex_lock(&rg->mtx);
> > + while (rg->ref) {
> > + mutex_unlock(&rg->mtx);
> > + schedule();
> > + mutex_lock(&rg->mtx);
> > + }
>
> Yes, rwsem would mean no unnecessary scheduling and also make things
> clearer.
Just tried it and seems to be mostly OK. We get lockdep checking as a
bonus. It didn't like setup_plane taking the same rwsem twice so I
added a check to see if the old and new regions are the same and just
lock once in that case. I thought rwsem was supposed to be OK with
read recursion but perhaps I was mitaken, or perhaps it's just lockdep
that's misbehaving.
>
> > [...]
> > static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info
> > *mi)
> > @@ -146,12 +242,15 @@ static int omapfb_query_mem(struct fb_info *fbi,
> > struct omapfb_mem_info *mi)
> > struct omapfb_info *ofbi = FB2OFB(fbi);
> > struct omapfb2_mem_region *rg;
> >
> > - rg = &ofbi->region;
> > memset(mi, 0, sizeof(*mi));
> >
> > + rg = omapfb_get_mem_region(ofbi->region);
>
> At some other users of region I haven't seen omapfb_get_mem_region,
> like store_mirror, store_overlays_rotate.
Right. I apparently just missed them. I now have a patch that tracks the
lock count too and added a few WARN_ON()s to some of the omapfb-main
functions that assume that the region is locked. That triggered a few
warning during boot which I already knew would not hold the lock since
they were only exectedu during init. But I fixed them anyway to take the
lock and now I can boot without warnings.
> It wouldn't have been nice to have this patch in smaller chunks. For example
> one for converting all region. to region-> and another one for adding the
> locking for it, then the rest.
I've now split it up into smalled chunks and eliminated all the
gratuitous changes. Assuming I can get my emmc working again and no issues
appear during some randr torture tests I will post the new patchset still today.
--
Ville Syrjälä
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
