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

Mon Sep 17 07:53:06 UTC 2018

Hi Noralf, Daniel,

On 14/09/2018 18:33, Noralf Trønnes wrote:
> 
> Den 14.09.2018 10.23, skrev Neil Armstrong:
>> 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 ?
> 
> It's not this series that drops smem_start support.
> It happened in commit 894a677f4b3e:
> drm/cma-helper: Use the generic fbdev emulation
> 
> This series only deals with the fb_helper callbacks.
> 
>> @Noaralf, do you have a branch somewhere I can base a work on ?
> 
> I haven't got a git repo, but you can apply the patches from patchwork:
> 
> [01/20] drm/fb-helper: Improve error reporting in setup
> curl https://patchwork.freedesktop.org/patch/247860/mbox/ | git am
> 
> [05/20] drm/meson: Use drm_fbdev_generic_setup()
> curl https://patchwork.freedesktop.org/patch/247868/mbox/ | git am

Thanks,

I'll try to drop something, but not immediately.

Neil

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