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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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ä
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
