On Sun, 21 March 2010 Jaya Kumar <jayakumar.lkml@xxxxxxxxx> wrote:
> > Add framebuffer support to PicoLCD device with use of deferred-io.
> >
> > Only changed areas of framebuffer get sent to device in order to
> > save USB bandwidth and especially resources on PicoLCD device or
> > allow higher refresh rate for a small area.
>
> Interesting work. One minor comment, defio doesn't currently guarantee
> that it is "changed areas". Just that it is "written" pages which
> typically equates to "changed" but does not guarantee this.
When copying from framebuffer to shadow buffer I check if any given tile
has changed and only send that tile to the device if it has effectively
changed. So this check is done independently of defio.
(though I have to force-fully transfer the whole framebuffer at probe
or after reset - which is missing)
> > Signed-off-by: Bruno Prémont <bonbons@xxxxxxxxxxxxxxxxx>
> > ---
> > drivers/hid/Kconfig | 7 +-
> > drivers/hid/hid-picolcd.c | 454 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 460 insertions(+), 1 deletions(-)
>
> It is customary for framebuffer drivers to live in drivers/video. This
> is the first one I've reviewed that is outside of it. Is there a good
> reason for this one to be outside of it? If so, could you put it in
> the comments.
I've kept all the pieces of PicoLCD driver together under hid,
as it's a HID based device. Framebuffer is just one of its features and
keeping pieces together makes it easier to handle.
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 7097f0a..a474bcd 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -230,6 +230,11 @@ config HID_PETALYNX
> > config HID_PICOLCD
> > tristate "PicoLCD (graphic version)"
> > depends on USB_HID
> > + select FB_DEFERRED_IO if FB
> > + select FB_SYS_FILLRECT if FB
> > + select FB_SYS_COPYAREA if FB
> > + select FB_SYS_IMAGEBLIT if FB
> > + select FB_SYS_FOPS if FB
>
> I think all of that "if FB" stuff looks odd, it would disappear if it
> were in the right Kconfig.
In the initial patch I did select FB as well though for v2 I decided
to make all the dependencies of non-input features optional. For the FB
case the various helper entries are needed, for the rest it's sufficient
to have #ifdef's for their device class.
Another option would be to distribute the Kconfig entries over the
various device class's Kconfig files though I don't think that would
make things easier to understand...
> > +/* Framebuffer visual structures */
> > +static const struct fb_fix_screeninfo picolcdfb_fix = {
> > + .id = PICOLCDFB_NAME,
> > + .type = FB_TYPE_PACKED_PIXELS,
> > + .visual = FB_VISUAL_MONO01,
>
> Interesting choice. Out of curiosity, which fb client application are
> you testing/using this with?
For now I've just been testing with manual read/write to /dev/fb. I've
not yet played with fb applications.
I've had a look at fb applications, seems that none wants to play with
individual bits, they all bail out requesting 8 or more bpp (some
fail politely, others just exit/segfault and don't undo changes they did)
So I guess I will have to add support for translating 8bpp to device's
1bpp to be able to use any existing fb application and switching between
1bpp and 8bpp framebuffer...
> > + /*
> > + * Translate the XBM format screen_base into the format needed by the
> > + * PicoLCD. See display layout above.
> > + * Do this one tile after the other and push those tiles that changed.
> > + */
>
> I think screen_base is in standard fb format, which you've specified
> as MONO01 above. When you say, XBM format, in the above comment is it
> exactly the same?
It should be the same (from what I read MONO01 versus MONO10 is just
whether 0 or 1 is "black" or "white") and in combination with
PACKED_PIXELS it makes 8 pixels per bytes.
Thanks for the review,
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
