[PATCH 05/20] drm/meson: Use drm_fbdev_generic_setup()

[Noralf Trønnes]

(Cc: Daniel Vetter)


Den 12.09.2018 11.56, skrev Maxime Ripard:
> On Wed, Sep 12, 2018 at 11:48:34AM +0200, Neil Armstrong wrote:
>> Hi Noralf,
>>
>> On 08/09/2018 15:46, Noralf Trønnes wrote:
>>> The CMA helper is already using the drm_fb_helper_generic_probe part of
>>> the generic fbdev emulation. This patch makes full use of the generic
>>> fbdev emulation by using its drm_client callbacks. This means that
>>> drm_mode_config_funcs->output_poll_changed and drm_driver->lastclose are
>>> now handled by the emulation code. Additionally fbdev unregister happens
>>> automatically on drm_dev_unregister().
>>>
>>> The drm_fbdev_generic_setup() call is put after drm_dev_register() in the
>>> driver. This is done to highlight the fact that fbdev emulation is an
>>> internal client that makes use of the driver, it is not part of the
>>> driver as such. If fbdev setup fails, an error is printed, but the driver
>>> succeeds probing.
>> I can't really ack/nack this move, on one side it's great to have a
>> generic fbdev emulation instead of the old fbdev code, but on the
>> other side the Amlogic platform (like allwinner, samsung, rockchip,
>> ...)  still relies on an Fbdev variant of the libMali that uses
>> smem_start...
>>
>> I know it's dirty and fbdev based code should be legacy now, but I
>> consider it like an user-space breakage...
>>
>> I'll be happy if ARM provided it's Mali vendors a Fbdev libMali that
>> could use FBINFO_HIDE_SMEM_START and allocate BO's from the DRM
>> driver, it won't be the case and it will never be the case until the
>> Lima projects finalizes.
>>
>> So for me it's a no-go until Lima lands upstream in Linux *and* Mesa.
> My feelings exactly. If the choice is between reducing the DRM driver
> by a couple of dozens of lines or keeping the mali working, I'll pick
> the latter, every single day.

I don't know the reasoning for blocking smem_start other than what Daniel
wrote in these commit messages:

da6c7707caf3736c1cf968606bd97c07e79625d4
fbdev: Add FBINFO_HIDE_SMEM_START flag

   DRM drivers really, really, really don't want random userspace to
   share buffer behind it's back, bypassing the dma-buf buffer sharing
   machanism. For that reason we've ruthlessly rejected any IOCTL
   exposing the physical address of any graphics buffer.

   Unfortunately fbdev comes with that built-in. We could just set
   smem_start to 0, but that means we'd have to hand-roll our own fb_mmap
   implementation. For good reasons many drivers do that, but
   smem_start/length is still super convenient.

   Hence instead just stop the leak in the ioctl, to keep fb mmap working
   as-is. A second patch will set this flag for all drm drivers.


6be8f3bd2c78915a9f3a058a346ae93068d35c01
drm/fb: Stop leaking physical address

   For buffer sharing, use dma-buf instead. We can't set smem_start to 0
   unconditionally since that's used by the fbdev mmap default
   implementation. And we have plenty of userspace which would like to
   keep that working.

   This might break legit userspace - if it does we need to look at a
   case-by-cases basis how to handle that. Worst case I expect overrides
   for only specific drivers, since anything remotely modern should be
   using dma-buf/prime now (which is about 7 years old now for DRM
   drivers).

   This issue was uncovered because Noralf's rework to implement a
   generic fb_probe also implements it's own fb_mmap callback. Which
   means smem_start didn't have to be set anymore, which blew up some
   blob in userspace rather badly.


Noralf.