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.
> 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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
