On Fri, Sep 14, 2018 at 10:23 AM, Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>> >>>>> >>>> >>> >> > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
