From: "Bruno Prémont" <bonbons@linux-vserver.org>
To: Jaya Kumar <jayakumar.lkml@gmail.com>
Cc: Jiri Kosina <jkosina@suse.cz>,
linux-input@vger.kernel.org, linux-usb@vger.kernel.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
"Rick L. Vinyard Jr." <rvinyard@cs.nmsu.edu>,
Nicu Pavel <npavel@ituner.com>, Oliver Neukum <oliver@neukum.org>
Subject: Re: [PATCH v2 2/6] hid: add framebuffer support to PicoLCD device
Date: Sun, 21 Mar 2010 16:11:28 +0000 [thread overview]
Message-ID: <20100321171128.324a606f@neptune.home> (raw)
In-Reply-To: <45a44e481003210024l2c66938fp9436d34f6473f811@mail.gmail.com>
On Sun, 21 March 2010 Jaya Kumar <jayakumar.lkml@gmail.com> 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@linux-vserver.org>
> > ---
> > 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
next prev parent reply other threads:[~2010-03-21 16:11 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-20 16:00 [PATCH v2 0/6] hid: new driver for PicoLCD device Bruno Prémont
[not found] ` <20100320170014.440959a8-hY15tx4IgV39zxVx7UNMDg@public.gmane.org>
2010-03-20 16:02 ` [PATCH v2 1/6] " Bruno Prémont
2010-03-21 3:46 ` Dmitry Torokhov
[not found] ` <20100321034600.GE29360-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
2010-03-21 16:37 ` Bruno Prémont
2010-03-22 4:35 ` Dmitry Torokhov
[not found] ` <20100322043508.GC31621-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
2010-03-22 11:38 ` Bruno Prémont
2010-03-22 8:54 ` Jiri Kosina
2010-03-20 16:04 ` [PATCH v2 2/6] hid: add framebuffer support to " Bruno Prémont
2010-03-21 3:25 ` Dmitry Torokhov
2010-03-21 7:24 ` Jaya Kumar
2010-03-21 16:11 ` Bruno Prémont [this message]
2010-03-22 8:56 ` Jiri Kosina
2010-03-20 16:06 ` [PATCH v2 3/6] hid: add backlight " Bruno Prémont
2010-03-22 8:59 ` Jiri Kosina
2010-03-22 11:01 ` Bruno Prémont
2010-03-20 16:08 ` [PATCH v2 4/6] hid: add lcd " Bruno Prémont
2010-03-20 16:10 ` [PATCH v2 5/6] hid: add GPO (leds) " Bruno Prémont
2010-03-20 16:11 ` [PATCH v2 6/6] hid: add experimental access to PicoLCD device's Bruno Prémont
2010-03-21 3:08 ` Dmitry Torokhov
[not found] ` <20100321030802.GB29360-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
2010-03-21 10:29 ` Bruno Prémont
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100321171128.324a606f@neptune.home \
--to=bonbons@linux-vserver.org \
--cc=jayakumar.lkml@gmail.com \
--cc=jkosina@suse.cz \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=npavel@ituner.com \
--cc=oliver@neukum.org \
--cc=rvinyard@cs.nmsu.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).