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 02:50:45PM +0100, Valkeinen Tomi (Nokia-D/Helsinki) wrote:
> Hi,
> 
> On Fri, 2010-03-05 at 14:26 +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.
> 
> Couple of comments inline. Mostly about making the patch a bit simpler.
> 
> > 
> > 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(-)
> 
> <snip>
> 
> > +static void omapfb_calc_addr(const struct omapfb_info *ofbi,
> > +                            const struct fb_var_screeninfo *var,
> > +                            const struct fb_fix_screeninfo *fix,
> > +                            int rotation, u32 *paddr, void __iomem **vaddr)
> > +{
> > +       u32 data_start_p;
> > +       void __iomem *data_start_v;
> > +       int offset;
> > +
> > +       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
> > +               data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> > +               data_start_v = NULL;
> > +       } else {
> > +               data_start_p = omapfb_get_region_paddr(ofbi);
> > +               data_start_v = omapfb_get_region_vaddr(ofbi);
> > +       }
> > +
> > +       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
> > +               offset = calc_rotation_offset_vrfb(var, fix, rotation);
> > +       else
> > +               offset = calc_rotation_offset_dma(var, fix, rotation);
> > +
> > +       data_start_p += offset;
> > +       data_start_v += offset;
> > +
> > +       if (offset)
> > +               DBG("offset %d, %d = %d\n",
> > +                   var->xoffset, var->yoffset, offset);
> > +
> > +       DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> > +
> > +       *paddr = data_start_p;
> > +       *vaddr = data_start_v;
> > +}
> > 
> >  /* setup overlay according to the fb */
> > -static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> > +int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> >                 u16 posx, u16 posy, u16 outw, u16 outh)
> >  {
> >         int r = 0;
> > @@ -831,9 +863,8 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> >         struct fb_var_screeninfo *var = &fbi->var;
> >         struct fb_fix_screeninfo *fix = &fbi->fix;
> >         enum omap_color_mode mode = 0;
> > -       int offset;
> > -       u32 data_start_p;
> > -       void __iomem *data_start_v;
> > +       u32 data_start_p = 0;
> > +       void __iomem *data_start_v = NULL;
> >         struct omap_overlay_info info;
> >         int xres, yres;
> >         int screen_width;
> > @@ -860,28 +891,9 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> >                 yres = var->yres;
> >         }
> > 
> > -
> > -       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
> > -               data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> > -               data_start_v = NULL;
> > -       } else {
> > -               data_start_p = omapfb_get_region_paddr(ofbi);
> > -               data_start_v = omapfb_get_region_vaddr(ofbi);
> > -       }
> > -
> > -       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
> > -               offset = calc_rotation_offset_vrfb(var, fix, rotation);
> > -       else
> > -               offset = calc_rotation_offset_dma(var, fix, rotation);
> > -
> > -       data_start_p += offset;
> > -       data_start_v += offset;
> > -
> > -       if (offset)
> > -               DBG("offset %d, %d = %d\n",
> > -                               var->xoffset, var->yoffset, offset);
> > -
> > -       DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> > +       if (ofbi->region->size)
> > +               omapfb_calc_addr(ofbi, var, fix, rotation,
> > +                                &data_start_p, &data_start_v);
> > 
> >         r = fb_mode_to_dss_mode(var, &mode);
> >         if (r) {
> 
> Moving this code to omapfb_calc_addr() could perhaps be a separate
> patch? I don't see other changes in there.

Ah right. It was leftover from an earlier patch with different logic in
the SETUP_PLANE ioctl. But it's still probable easier on the eyes to
have it in a separate function. I'll split the patch up.

> 
> >  static struct device_attribute omapfb_attrs[] = {
> > diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
> > index cd54fdb..ce15533 100644
> > --- a/drivers/video/omap2/omapfb/omapfb.h
> > +++ b/drivers/video/omap2/omapfb/omapfb.h
> 
> >  static inline int omapfb_overlay_enable(struct omap_overlay *ovl,
> > -               int enable)
> > +                                       int enable)
> >  {
> >         struct omap_overlay_info info;
> > +       int r;
> > 
> >         ovl->get_overlay_info(ovl, &info);
> > +       if (info.enabled = enable)
> > +               return 0;
> >         info.enabled = enable;
> > -       return ovl->set_overlay_info(ovl, &info);
> > +       r = ovl->set_overlay_info(ovl, &info);
> > +       if (r)
> > +               return r;
> > +
> > +       return 0;
> > +}
> 
> There are perhaps more changes here than needed. int r is not needed,
> and the last lines do not need to be changed. And is if (info.enabled =
> enable) required by this patch, or is it just optimization? If
> optimization, it could be in separate patch.

That too was leftover from an earlier version which require more logic
here. I'll split it up as well.

> >  #endif
> > diff --git a/include/linux/omapfb.h b/include/linux/omapfb.h
> > index 9bdd914..5de473c 100644
> > --- a/include/linux/omapfb.h
> > +++ b/include/linux/omapfb.h
> > @@ -85,6 +85,9 @@
> >  #define OMAPFB_MEMTYPE_SRAM            1
> >  #define OMAPFB_MEMTYPE_MAX             1
> > 
> > +#define OMAPFB_MEM_IDX_ENABLED 0x8
> > +#define OMAPFB_MEM_IDX_MASK    0x7
> 
> Should enabled be 0x80, and mask 0x7f? There are never that many mem
> indexes, but is there any point in limiting the value?

Hmm. Right. The original clone_idx enable on rover was 0x4 for some
reason. My idea clearly was to move it to the msb but apparently my
brain skipped a beat there and I forgot to add the 0s. Will fix.

-- 
Ville Syrjälä