Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

["Ville Syrjälä" <ville.syrjala@nokia.com>]

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ä <ville.syrjala@nokia.com>
> > 
> > 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ä <ville.syrjala@nokia.com>
> > ---
> > 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ä