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

[Tomi Valkeinen <tomi.valkeinen@nokia.com>]

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.

>  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.

>  #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?

 Tomi


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html