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

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

On Thu, Mar 18, 2010 at 09:52:39AM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> On Wed, Mar 17, 2010 at 09:14:25PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > 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:
> > > [...]
> > > > @@ -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.
> 
> Ah ok, so it's not so obvious change. Nested read locks could really lead
> to a deadlock I think. A read lock will block if there is a write waiter
> in the queue to avoid write starvation..

Yes but I think in out case it should be fine because if we hit this:

 t  thread 1     thread 2
 |
 |  down_read(0)
 |               down_write(1)
 v  down_read(1)

then thread 2 will eventually do a up_write() without taking any
other region rwsem, and thread 1 can then continue.

The other locks we have to worry about are the fb_info mutex which
should always be taken before any region lock, and mmap_sem which in
mmap() is taken before the region lock. I'm hoping there are no
mmap_sem users lurking in the driver that already hold the region
rwsem.

-- 
Ville Syrjälä