[PATCH] ARM: pxafb: Use the passed fb_var_screeninfo struct in pxafb_pan_display()

[Ville Syrjälä]

On Wed, Oct 28, 2009 at 10:13:17AM +0800, Eric Miao wrote:
> I see no problem with this patch, applied to 'devel'.

This patch looks broken to me. The only fields of of the fb_var_screeninfo
passed to pan_display that should be used are xoffset, yoffset and 
vmode & FB_VMODE_YWRAP. The rest are not checked by the common code at
all and depending on the hardware the pan operation may involve looking
at any number of fields from fb_var_screeninfo so if other fields are
taken from the passed fb_var_screeninfo the pan operation may end up doing
a lot more than panning (eg. it could change the video mode). Even worse
things could happen since the values are not checked at all.

To prevent driver authors' from shooting themselves in the foot it might
make sense to change the common code to make a copy of info->var and
merge those three fields from the passed fb_var_screeninfo into the copy,
which would then be passed to the driver.

> 
> On Wed, Oct 28, 2009 at 8:18 AM, Daniel Mack <daniel at caiaq.de> wrote:
> > Any idea what to do with this one?
> >
> > Daniel
> >
> >
> > On Thu, Oct 22, 2009 at 08:34:34AM +0200, Daniel Mack wrote:
> >> Hi Eric,
> >>
> >> this one has been living in our kernel tree for quite some time now but
> >> it seems it hasn't been merged yet. It's needed to make the pxafb driver
> >> work with DirectFB applications properly.
> >>
> >> Thanks,
> >> Daniel
> >>
> >>
> >> From 42e8a4226777b6cc82988061c80167813f403ec8 Mon Sep 17 00:00:00 2001
> >> From: Sven Neumann <s.neumann at raumfeld.com>
> >> Date: Wed, 6 May 2009 16:22:50 +0200
> >> Subject: [PATCH] ARM: pxafb: Use the passed fb_var_screeninfo struct in pxafb_pan_display()
> >>
> >> pxafb_pan_display() used to ignore the fb_var_screeninfo parameter. Now
> >> pass it to setup_base_frame() instead of pulling default values out of
> >> fb_info.
> >>
> >> Signed-off-by: Sven Neumann <s.neumann at raumfeld.com>
> >> Signed-off-by: Daniel Mack <daniel at caiaq.de>
> >> Cc: Eric Miao <eric.y.miao at gmail.com>
> >> ---
> >>  drivers/video/pxafb.c |   12 +++++++-----
> >>  1 files changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
> >> index 1820c4a..a20a7d4 100644
> >> --- a/drivers/video/pxafb.c
> >> +++ b/drivers/video/pxafb.c
> >> @@ -80,7 +80,8 @@
> >>  static int pxafb_activate_var(struct fb_var_screeninfo *var,
> >>                               struct pxafb_info *);
> >>  static void set_ctrlr_state(struct pxafb_info *fbi, u_int state);
> >> -static void setup_base_frame(struct pxafb_info *fbi, int branch);
> >> +static void setup_base_frame(struct pxafb_info *fbi,
> >> +                             struct fb_var_screeninfo *var, int branch);
> >>  static int setup_frame_dma(struct pxafb_info *fbi, int dma, int pal,
> >>                          unsigned long offset, size_t size);
> >>
> >> @@ -536,7 +537,7 @@ static int pxafb_pan_display(struct fb_var_screeninfo *var,
> >>       if (fbi->state != C_ENABLE)
> >>               return 0;
> >>
> >> -     setup_base_frame(fbi, 1);
> >> +     setup_base_frame(fbi, var, 1);
> >>
> >>       if (fbi->lccr0 & LCCR0_SDS)
> >>               lcd_writel(fbi, FBR1, fbi->fdadr[dma + 1] | 0x1);
> >> @@ -1052,9 +1053,10 @@ static int setup_frame_dma(struct pxafb_info *fbi, int dma, int pal,
> >>       return 0;
> >>  }
> >>
> >> -static void setup_base_frame(struct pxafb_info *fbi, int branch)
> >> +static void setup_base_frame(struct pxafb_info *fbi,
> >> +                             struct fb_var_screeninfo *var,
> >> +                             int branch)
> >>  {
> >> -     struct fb_var_screeninfo *var = &fbi->fb.var;
> >>       struct fb_fix_screeninfo *fix = &fbi->fb.fix;
> >>       int nbytes, dma, pal, bpp = var->bits_per_pixel;
> >>       unsigned long offset;
> >> @@ -1332,7 +1334,7 @@ static int pxafb_activate_var(struct fb_var_screeninfo *var,
> >>  #endif
> >>               setup_parallel_timing(fbi, var);
> >>
> >> -     setup_base_frame(fbi, 0);
> >> +     setup_base_frame(fbi, var, 0);
> >>
> >>       fbi->reg_lccr0 = fbi->lccr0 |
> >>               (LCCR0_LDM | LCCR0_SFM | LCCR0_IUM | LCCR0_EFM |
> >> --
> >> 1.6.5
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Ville Syrjälä
syrjala at sci.fi
http://www.sci.fi/~syrjala/