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