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

[Imre Deak <imre.deak@nokia.com>]

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?

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

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

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.

Otherwise it looks ok to me.

--Imre