Re: [PATCH] Add support for SMSC UFX6000/7000 USB display adapters

[Florian Tobias Schandinat <FlorianSchandinat@gmx.de>]

Hi Bernie,

On 08/16/2011 04:53 PM, Bernie Thompson wrote:
> Hi Florian,
> 
> On Tue, Aug 16, 2011 at 3:17 AM, Florian Tobias Schandinat
> <FlorianSchandinat@gmx.de> wrote:
>>> + * TODO: Propose standard fb.h ioctl for reporting damage,
>>> + * using _IOWR() and one of the existing area structs from fb.h
>>> + * Consider these ioctls deprecated, but they're still used by the
>>> + * DisplayLink X server as yet - need both to be modified in tandem
>>> + * when new ioctl(s) are ready.
>>> + */
>>> +#define UFX_IOCTL_RETURN_EDID        (0xAD)
>>> +#define UFX_IOCTL_REPORT_DAMAGE      (0xAA)
>>
>> Why don't we solve this now? So what do you need?
> 
> It would be wonderful to solve this now.  This capability is needed
> for any device which doesn't have a directly mmapable hardware
> framebuffer (e.g. any USB graphics device, but also remote
> framebuffers, devices like eink controllers, etc.)
> 
> If we define the standard interface, we'll also modify udlfb (the
> DisplayLink driver) to match, so we'll have several clients of the
> interface in the tree from day one.
> 
> And we also have a patch to the xorg fbdev driver to be a client of
> this interface -- to pass X DAMAGE protocol notifications to the
> kernel framebuffer, allowing all these devices to work with common
> clients (whereas they can't today, without fb_defio or some other
> trick to detect framebuffer writes).
> 
> Here is the xorg fbdev patch that can be updated to the new interface:
> http://git.plugable.com/gitphp/index.php?p=xf86-video-fbdev&a=commitdiff&hÛfe3f839ee00135a7cccf13db5926fb1019abde
> 
>> An IOCTL, a struct like
>> struct dloarea {
>>        int x, y;
>>        int w, h;
>> };
>> and I don't think any of the existing structs in fb.h would fit. I'd just prefer
>> to have those int's replaced by something of known size, probably __u32 to make
>> it consistent with fb_var_screeninfo?
> 
> Yes. that'd be perfect.   Could be as simple as
> 
> struct fb_rect {
> 	__u32_t x;
> 	__u32_t y;
> 	__u32_t width;
> 	__u32_t height;
> };
> #define FBIOPUT_DAMAGE _IOW('F', 0x1A, struct fb_rect)
> 
> (with the 1A ordinal being out of date now).  But anything similarly
> simple is fine.

Yes, I think we can add this to fb.h without any problems.

>> Maybe this should be also reported by the capability field that Laurent's API
>> extension will likely add.
> 
> Definitely.  In addition to the above, the only other thing needed is
> capability negotiation between the fbdev client (say, xorg) and the
> fbdev driver.  The negotiation has to be two-step:
> 
> The fbdev client tells the driver that it's damage-aware (A)
> The fbdev driver returns to the client whether it requires damage (B)
> 
> If A and B, then the client will send damage and the driver will rely
> on it (and not use fb_defio, timers, or other backup)
> If A but not B, then the client should avoid unncessary overhead of
> sending damage notifications that the driver isn't listening to.
> If not A, then the driver must use fb_defio, timers, or some of the
> other (less-efficient) backup mechanisms to make the mmap client's
> framebuffer writes get noticed.
> 
> Timing: When the driver doesn't have a damage-aware client, they often
> fall back to using fb_defio.  fb_defio requires special setup
> (framebuffer mmap with pages set to fault on writes), that has
> complexities that are best avoided if possible.  So we want step (A)
> to happen before mmap, at least, and we should define the API to make
> it happen at the earliest point we can define.
> 
> You can see the xorg driver patch above, and the smsc and displaylink
> drivers doing this today, but in not the prettiest way (as soon as the
> driver gets the first damage notification, it tries to stop using
> fb_defio).  But an explicit capability negotiation, defined to happen
> at the earliest point, would be cleaner.

What's the problem with the way you do it now? (aside that you should IMHO reset
the flag whether the client does use damage or not on set_par/mode change)
The extension I proposed and is part of Laurent's patches is a capability
reporting from the driver to the client, that would solve (B). Sure we could
think of anything to make the other way (A) possible but I doubt that it would
be worth it as I think you have to keep defio support around anyway for
applications that do not support damage reporting. I think implementing (A)
would just needlessly add complexity to the drivers as it would be some sort of
alternative mmap IOCTL. Or we could try to find yet another flag inside var that
is unused for it but that is difficult.

> All the pieces are close. It would be great to get this standardized!
> Thanks for stepping in to help lead this.

Sure, all the nice features will just be used if they are easy usable from user
space, so providing an API instead of requiring user space to know individual
drivers is a good thing.


Best regards,

Florian Tobias Schandinat