Prev: KVM: SVM: Coding style cleanup
Next: Email Back Shortly
From: Bruno Prémont on 25 Feb 2010 06:40
On Thu, 25 February 2010 Jiri Kosina <jkosina(a)suse.cz> wrote:
> On Wed, 24 Feb 2010, Bruno Prémont wrote:
>
> > +config HID_PICOLCD
> > + tristate "Minibox PicoLCD (graphic version)"
> > + depends on FB
> > + select FB_DEFERRED_IO
> > + select FB_SYS_FILLRECT
> > + select FB_SYS_COPYAREA
> > + select FB_SYS_IMAGEBLIT
> > + select FB_SYS_FOPS
> > + select BACKLIGHT_LCD_SUPPORT
> > + select BACKLIGHT_CLASS_DEVICE
> > + select LCD_CLASS_DEVICE
>
> I guess you need dependency on USB_HID as well, right?

Yep, will add

> > --- a/drivers/hid/hid-picolcd.c
> > +++ b/drivers/hid/hid-picolcd.c
> > @@ -0,0 +1,1075 @@
> > +/***************************************************************************
> > + * Copyright (C) 2010 by Bruno Prémont <bonbons(a)linux-vserver.org> *
> > + * *
> > + * Based on Logitech G13 driver (v0.4) *
> > + * Copyright (C) 2009 by Rick L. Vinyard, Jr. <rvinyard(a)cs.nmsu.edu> *
> > + * *
> > + * This program is free software: you can redistribute it and/or modify *
> > + * it under the terms of the GNU General Public License as published by *
> > + * the Free Software Foundation, either version 2 of the License. *
>
> I support removing the 'or any later' clause, but I think your
> version has the grammar wrong :)

Oops, will fix

> > +/* Update fb_vbitmap from the screen_base and send changed tiles to device */
> > +static void picolcd_fb_update(struct picolcd_data *data)
> > +{
> > + int chip, tile;
> > +
> > + spin_lock(&data->lock);
> > + if (!(data->status & PICOLCD_READY_FB)) {
> > + spin_unlock(&data->lock);
> > + picolcd_fb_reset(data->hdev, 0);
> > + } else
> > + spin_unlock(&data->lock);
>
> Please put the brackets to the else branch as well.

Ok, will add the brackets while switching from spin_(un)lock to
spin_(un)lock_irq{save|restore}.

> Also, it'd be great if the framebuffer part would get Ack from
> someone more familiar with framebuffer code than me.

I would appreciate such one as well, especially regarding the
deferredio part and the more advanced fb use. For now I only tested
read/write from /dev/fbx.


For the two sysfs attributes I currently use, the 'reset' one shall
probably be moved to debugfs (I would like to place it under
/sys/kernel/debug/hid/$device/ next to rdesc and event).

By the way, I'm wondering why event does not list any of the reports
coming from my device though as I understand the code it should be
doing that before my raw_event function gets called...

The one for deferredio refresh rate should ideally go below fb device
(and I guess that might also be of use for other users of deferredio)

Thanks for the review,
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Kosina on 25 Feb 2010 10:20
On Thu, 25 Feb 2010, Bruno Pr�mont wrote:

> For the two sysfs attributes I currently use, the 'reset' one shall
> probably be moved to debugfs (I would like to place it under
> /sys/kernel/debug/hid/$device/ next to rdesc and event).

Yes, that would make sense.

( ... which reminds me to finally to the Documentation/ABI part in sync
with respect to current HID code again ... )

> By the way, I'm wondering why event does not list any of the reports
> coming from my device though as I understand the code it should be doing
> that before my raw_event function gets called...

Sorry, what 'event' do you mean in 'event does not list any of the
reports'?

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Rick L. Vinyard, Jr. on 25 Feb 2010 10:40
Jiri Kosina wrote:
> On Wed, 24 Feb 2010, Rick L. Vinyard, Jr. wrote:
>
>> The key difference is the replacement of spin_lock() with spin_trylock()
>> such that if the non-interrupt code has already obtained the lock, the
>> interrupt will not deadlock but instead take the else path and schedule
>> a
>> framebuffer update at the next interval.
>
> Why is _irqsave() and/or deferred work not enough? The aproach with
> _trylock() seems to be overly complicated for no good reason (I personally
> become very suspicious every time I see code that is using _trylock()).
>

I was concerned about _irqsave() because the lock is split across two
functions to protect the urb after it is handed off to the usb subsystem
with usb_submit_urb(). It's locked in g13_fb_send() and unlocked in the
urb completion callback.

As for deferred work, the g13_fb_send() is the I/O portion of the deferred
framebuffer callback. I was concerned that without a lock one deferred
update could hand the urb off to the usb subsystem and a second could try
to access it before it was handed back to the driver.

In this case the _trylock() would fail and in the else patch we would
defer yet again until the next update cycle.

I took this approach because usb_interrupt_msg() couldn't be used from an
interrupt context, such as a resume hook because eventually down the chain
it does a wait_for_completion_timeout().

It has the added benefit of reusing the urb instead of creating a new one
for each framebuffer sent out, but that wasn't a reason... just a side
effect.

The downside is that I had to manage the urb.

One thing I could do is forget about directly calling g13_fb_send() from
any context and instead use the deferred framebuffer workqueue.

That's probably a simpler approach anyway.

> [ by the way, Rick, are you planning to resubmit the G13 driver with
> incorporated feedback from the last review round? ]
>

Yes. I just wanted to get the details of suspend/resume worked out before
resubmitting.

--

Rick

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Bruno Prémont on 25 Feb 2010 10:40
On Thu, 25 February 2010 Jiri Kosina <jkosina(a)suse.cz> wrote:
>
> On Thu, 25 Feb 2010, Bruno Prémont wrote:
>
> > For the two sysfs attributes I currently use, the 'reset' one shall
> > probably be moved to debugfs (I would like to place it under
> > /sys/kernel/debug/hid/$device/ next to rdesc and event).
>
> Yes, that would make sense.

So I will move it.

> ( ... which reminds me to finally to the Documentation/ABI part in
> sync with respect to current HID code again ... )
>
> > By the way, I'm wondering why event does not list any of the
> > reports coming from my device though as I understand the code it
> > should be doing that before my raw_event function gets called...
>
> Sorry, what 'event' do you mean in 'event does not list any of the
> reports'?

Sorry, one 's' that got eaten between my brain and mail client. I
mean events file in debugfs.
For USB HID keyboard each key press generates "report dump" under
/sys/kernel/debug/hid/$device/events but for my PicoLCD I don't get
any entry in there. I'm wondering why.

Thanks,
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Rick L. Vinyard, Jr. on 25 Feb 2010 13:00

Bruno Prémont wrote:
> The one for deferredio refresh rate should ideally go below fb device
> (and I guess that might also be of use for other users of deferredio)
>

I'm testing a patch now. I didn't expect anyone else would really need it,
but it does help with throttling bus traffic from userspace.

If it's in fbsysfs.c we can both eliminate it from our drivers.

--

Rick

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: KVM: SVM: Coding style cleanup
Next: Email Back Shortly