[3/3] drm/todo: Add entry for fb funcs related cleanups
diff mbox series

Message ID 20191127180035.416209-3-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/3] drm/rockchip: Use drm_gem_fb_create_with_dirty
Related show

Commit Message

Daniel Vetter Nov. 27, 2019, 6 p.m. UTC
We're doing a great job for really simple drivers right now, but still
a lot of boilerplate for the bigger ones.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Thomas Zimmermann Nov. 29, 2019, 9:34 a.m. UTC | #1
Hi

Am 27.11.19 um 19:00 schrieb Daniel Vetter:
> We're doing a great job for really simple drivers right now, but still
> a lot of boilerplate for the bigger ones.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Just a small remark below, otherwise

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>


> ---
>  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 3ec509381fc5..2d85f37284a1 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
>  
>  Level: Intermediate
>  
> +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> +-----------------------------------------------------------------
> +
> +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
> +Various hold-ups:
> +
> +- Need to switch over to the generic dirty tracking code using
> +  drm_atomic_helper_dirtyfb first (e.g. qxl).
> +
> +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
> +  setup code can't be deleted.

From what I remember, almost all of the obvious, low-hanging fruits have
been picked here. The remaining fbdev users either have HW acceleration
(nouveau, gma500), or use the cfb drawing functions.

The TODO item should probably mention this, with some advise to do some
extra testing for compatibility or performance after moving to generic
fbdev.

Best regards
Thomas

> +
> +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> +  atomic drivers we could check for valid formats by calling
> +  drm_plane_check_pixel_format() against all planes, and pass if any plane
> +  supports the format. For non-atomic that's not possible since like the format
> +  list for the primary plane is fake and we'd therefor reject valid formats.
> +
> +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
> +  version of the varios drm_gem_fb_create functions. Maybe called
> +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
> +
> +Contact: Daniel Vetter
> +
> +Level: Intermediate
> +
>  Clean up mmap forwarding
>  ------------------------
>  
>
Daniel Vetter Nov. 29, 2019, 6:57 p.m. UTC | #2
On Fri, Nov 29, 2019 at 10:34:10AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 27.11.19 um 19:00 schrieb Daniel Vetter:
> > We're doing a great job for really simple drivers right now, but still
> > a lot of boilerplate for the bigger ones.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Just a small remark below, otherwise
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> 
> > ---
> >  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 3ec509381fc5..2d85f37284a1 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
> >  
> >  Level: Intermediate
> >  
> > +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> > +-----------------------------------------------------------------
> > +
> > +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
> > +Various hold-ups:
> > +
> > +- Need to switch over to the generic dirty tracking code using
> > +  drm_atomic_helper_dirtyfb first (e.g. qxl).
> > +
> > +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
> > +  setup code can't be deleted.
> 
> From what I remember, almost all of the obvious, low-hanging fruits have
> been picked here. The remaining fbdev users either have HW acceleration
> (nouveau, gma500), or use the cfb drawing functions.

I think a bunch of these (from you) aren't merged yet. I'll add a note
about sys vs cfb. About gma500/nouveau, I'm kinda tempted to just ditch
the acceleration ... but maybe someone cares, dunno.

> The TODO item should probably mention this, with some advise to do some
> extra testing for compatibility or performance after moving to generic
> fbdev.

Testing (at least on x86) won't catch the cfb/sysfb stuff, since it's
exactly the same asm instructions :-/ tbh I still don't know where this
actually makes a difference.
-Daniel

> 
> Best regards
> Thomas
> 
> > +
> > +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> > +  atomic drivers we could check for valid formats by calling
> > +  drm_plane_check_pixel_format() against all planes, and pass if any plane
> > +  supports the format. For non-atomic that's not possible since like the format
> > +  list for the primary plane is fake and we'd therefor reject valid formats.
> > +
> > +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
> > +  version of the varios drm_gem_fb_create functions. Maybe called
> > +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
> > +
> > +Contact: Daniel Vetter
> > +
> > +Level: Intermediate
> > +
> >  Clean up mmap forwarding
> >  ------------------------
> >  
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
Daniel Vetter Nov. 29, 2019, 7:05 p.m. UTC | #3
On Fri, Nov 29, 2019 at 07:57:39PM +0100, Daniel Vetter wrote:
> On Fri, Nov 29, 2019 at 10:34:10AM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 27.11.19 um 19:00 schrieb Daniel Vetter:
> > > We're doing a great job for really simple drivers right now, but still
> > > a lot of boilerplate for the bigger ones.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > 
> > Just a small remark below, otherwise
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > 
> > > ---
> > >  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > index 3ec509381fc5..2d85f37284a1 100644
> > > --- a/Documentation/gpu/todo.rst
> > > +++ b/Documentation/gpu/todo.rst
> > > @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
> > >  
> > >  Level: Intermediate
> > >  
> > > +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> > > +-----------------------------------------------------------------
> > > +
> > > +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
> > > +Various hold-ups:
> > > +
> > > +- Need to switch over to the generic dirty tracking code using
> > > +  drm_atomic_helper_dirtyfb first (e.g. qxl).
> > > +
> > > +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
> > > +  setup code can't be deleted.
> > 
> > From what I remember, almost all of the obvious, low-hanging fruits have
> > been picked here. The remaining fbdev users either have HW acceleration
> > (nouveau, gma500), or use the cfb drawing functions.
> 
> I think a bunch of these (from you) aren't merged yet. I'll add a note
> about sys vs cfb. About gma500/nouveau, I'm kinda tempted to just ditch
> the acceleration ... but maybe someone cares, dunno.

Correction, we already have a task for drm_fbdev_generic_setup, and that
mentions the sys/cfb issue already. So I'll leave this as-is.
-Daniel

> 
> > The TODO item should probably mention this, with some advise to do some
> > extra testing for compatibility or performance after moving to generic
> > fbdev.
> 
> Testing (at least on x86) won't catch the cfb/sysfb stuff, since it's
> exactly the same asm instructions :-/ tbh I still don't know where this
> actually makes a difference.
> -Daniel
> 
> > 
> > Best regards
> > Thomas
> > 
> > > +
> > > +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
> > > +  atomic drivers we could check for valid formats by calling
> > > +  drm_plane_check_pixel_format() against all planes, and pass if any plane
> > > +  supports the format. For non-atomic that's not possible since like the format
> > > +  list for the primary plane is fake and we'd therefor reject valid formats.
> > > +
> > > +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
> > > +  version of the varios drm_gem_fb_create functions. Maybe called
> > > +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
> > > +
> > > +Contact: Daniel Vetter
> > > +
> > > +Level: Intermediate
> > > +
> > >  Clean up mmap forwarding
> > >  ------------------------
> > >  
> > > 
> > 
> > -- 
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > (HRB 36809, AG Nürnberg)
> > Geschäftsführer: Felix Imendörffer
> > 
> 
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Thomas Zimmermann Dec. 2, 2019, 8:42 a.m. UTC | #4
(cc: SPARC64 maintainer)
(cc: RISC-V maintainers)

Hi Daniel

Am 29.11.19 um 20:05 schrieb Daniel Vetter:
> On Fri, Nov 29, 2019 at 07:57:39PM +0100, Daniel Vetter wrote:
>> On Fri, Nov 29, 2019 at 10:34:10AM +0100, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 27.11.19 um 19:00 schrieb Daniel Vetter:
>>>> We're doing a great job for really simple drivers right now, but still
>>>> a lot of boilerplate for the bigger ones.
>>>>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>
>>> Just a small remark below, otherwise
>>>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>
>>>
>>>> ---
>>>>  Documentation/gpu/todo.rst | 26 ++++++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
>>>> index 3ec509381fc5..2d85f37284a1 100644
>>>> --- a/Documentation/gpu/todo.rst
>>>> +++ b/Documentation/gpu/todo.rst
>>>> @@ -182,6 +182,32 @@ Contact: Maintainer of the driver you plan to convert
>>>>  
>>>>  Level: Intermediate
>>>>  
>>>> +drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
>>>> +-----------------------------------------------------------------
>>>> +
>>>> +A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
>>>> +Various hold-ups:
>>>> +
>>>> +- Need to switch over to the generic dirty tracking code using
>>>> +  drm_atomic_helper_dirtyfb first (e.g. qxl).
>>>> +
>>>> +- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
>>>> +  setup code can't be deleted.
>>>
>>> From what I remember, almost all of the obvious, low-hanging fruits have
>>> been picked here. The remaining fbdev users either have HW acceleration
>>> (nouveau, gma500), or use the cfb drawing functions.
>>
>> I think a bunch of these (from you) aren't merged yet. I'll add a note
>> about sys vs cfb. About gma500/nouveau, I'm kinda tempted to just ditch
>> the acceleration ... but maybe someone cares, dunno.
> 
> Correction, we already have a task for drm_fbdev_generic_setup, and that
> mentions the sys/cfb issue already. So I'll leave this as-is.

Maybe refer to the related TODO item.

> -Daniel
> 
>>
>>> The TODO item should probably mention this, with some advise to do some
>>> extra testing for compatibility or performance after moving to generic
>>> fbdev.
>>
>> Testing (at least on x86) won't catch the cfb/sysfb stuff, since it's
>> exactly the same asm instructions :-/ tbh I still don't know where this
>> actually makes a difference.

I briefly looked through the code of the CFB helpers. They use the
helpers at [1] for accessing the framebuffer. Those are special for
several architectures. [2]

SPARC64 expands to assembly instructions. [3] The rest appears to have
regular memory instructions in their implementations of __raw_readb().
Although not listed here, I found that RISC V has assembly code in
__raw_readb(). [4]

In the CFB code, there's also bit-shifting code that cares about
endianess. [5] In the end it seems to depends on FBINFO_FOREIGN_ENDIAN,
but the only user I could find was mb862xxfb. [6]

I don't know what the hand-crafted assembly instructions do in detail,
but for the moment we seem to be good without CFB code.

Best regards
Thomas


[1] https://elixir.bootlin.com/linux/v5.4/source/include/linux/fb.h#L564
[2] https://elixir.bootlin.com/linux/v5.4/source/include/linux/fb.h#L527
[3]
https://elixir.bootlin.com/linux/v5.4/source/arch/sparc/include/asm/io_64.h#L20
[4]
https://elixir.bootlin.com/linux/v5.4/source/arch/riscv/include/asm/io.h#L59
[5] https://elixir.bootlin.com/linux/v5.4/source/include/linux/fb.h#L578
[6]
https://elixir.bootlin.com/linux/v5.4/source/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c#L501

>> -Daniel
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>> +
>>>> +- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
>>>> +  atomic drivers we could check for valid formats by calling
>>>> +  drm_plane_check_pixel_format() against all planes, and pass if any plane
>>>> +  supports the format. For non-atomic that's not possible since like the format
>>>> +  list for the primary plane is fake and we'd therefor reject valid formats.
>>>> +
>>>> +- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
>>>> +  version of the varios drm_gem_fb_create functions. Maybe called
>>>> +  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
>>>> +
>>>> +Contact: Daniel Vetter
>>>> +
>>>> +Level: Intermediate
>>>> +
>>>>  Clean up mmap forwarding
>>>>  ------------------------
>>>>  
>>>>
>>>
>>> -- 
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Software Solutions Germany GmbH
>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> (HRB 36809, AG Nürnberg)
>>> Geschäftsführer: Felix Imendörffer
>>>
>>
>>
>>
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>

Patch
diff mbox series

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 3ec509381fc5..2d85f37284a1 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -182,6 +182,32 @@  Contact: Maintainer of the driver you plan to convert
 
 Level: Intermediate
 
+drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
+-----------------------------------------------------------------
+
+A lot more drivers could be switched over to the drm_gem_framebuffer helpers.
+Various hold-ups:
+
+- Need to switch over to the generic dirty tracking code using
+  drm_atomic_helper_dirtyfb first (e.g. qxl).
+
+- Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
+  setup code can't be deleted.
+
+- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
+  atomic drivers we could check for valid formats by calling
+  drm_plane_check_pixel_format() against all planes, and pass if any plane
+  supports the format. For non-atomic that's not possible since like the format
+  list for the primary plane is fake and we'd therefor reject valid formats.
+
+- Many drivers subclass drm_framebuffer, we'd need a embedding compatible
+  version of the varios drm_gem_fb_create functions. Maybe called
+  drm_gem_fb_create/_with_dirty/_with_funcs as needed.
+
+Contact: Daniel Vetter
+
+Level: Intermediate
+
 Clean up mmap forwarding
 ------------------------