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

[Daniel Vetter]

On Wed, Sep 12, 2018 at 01:06:07PM +0200, Noralf Trønnes wrote:
> 
> Den 12.09.2018 12.57, skrev Noralf Trønnes:
> > (Cc: Daniel Vetter)
> > 
> 
> Somehow that CC was dropped somewhere after leaving email client.
> Trying once more.

Yeah I just made myself unpopular. If you want SMEM_START, then you need
to carry a local patch now. virt_to_pfn on the vmap should work. It's
about 2 lines, including the change to drop HIDE_SMEM_START.

And if consensus is that hiding SMEM_START is not a nice idea, then I'm
sure we can reintroduce it through some slightly unpretty backdoor, even
with Noralf's generic code. So not really a good reason to reject the
cleanup I think.
-Daniel


> > 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.
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch