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