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

Fri Sep 14 08:51:42 UTC 2018

On Fri, Sep 14, 2018 at 10:23 AM, Neil Armstrong
<narmstrong at baylibre.com> wrote:
> Hi Daniel,
>
> On 13/09/2018 16:55, Daniel Vetter wrote:
>> 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).
>
> OK, I think you mean module_param_unsafe, but I see the point.
>
>>
>> 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.
>
> I'll drop something, but I'm afraid a kernel won't have this hack, shouldn't this serie be delayed for a release ?

smem_start is gone already, for everyone. If you want to delay this,
then either you need to revert my commits, or get your hack in
quickly.
-Daniel

> @Noaralf, do you have a branch somewhere I can base a work on ?
>
> Neil
>
>>
>> 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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch