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

Thu Sep 13 14:55:14 UTC 2018

On Thu, Sep 13, 2018 at 04:26:53PM +0200, Neil Armstrong wrote:
> Hi Daniel,
> 
> On 13/09/2018 15:21, Daniel Vetter wrote:
> > 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.
> 
> Would it be totally unacceptable to add such 2 line :
> - enabled by a specific config depending on EXPERT and narrowed to specific platforms
> - printing a big fat pr_warning_once when used
> - with a big fat comment specifying when this code will be dropped and why we should not activate it

module_param_debug auto-taints your kernel, I'd just go with that. Plus
CONFIG_EXPERT (or CONFIG_BROKEN).

But yes, I think that's something I'll happily ack. Must be acked by Dave
(and perhaps a few others too), since defacto this means we now suddenly
care about closed source blobs. I'd say get someone from amd and a few of
the driver maintainers on board with this (nouveau, Eric, Rob, Lucas,
...), to make sure it really represents community consensus.

Cheers, Daniel

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