Re: [PATCH 3/3] drm/todo: Add entry for fb funcs related cleanups

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5224 bytes --]

(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
> 

-- 
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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel