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@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.
>
> 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(-)
>
> 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
--
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
