Hi Maxime, Daniel, On 22/09/2018 10:56, Daniel Vetter wrote: > On Fri, Sep 21, 2018 at 04:07:40PM +0200, Maxime Ripard wrote: >> Hi, >> >> On Thu, Sep 20, 2018 at 03:04:12PM +0200, Neil Armstrong wrote: >>> Since "drm/fb: Stop leaking physical address", the default behaviour of >>> the DRM fbdev emulation is to set the smem_base to 0 and pass the new >>> FBINFO_HIDE_SMEM_START flag. >>> >>> The main reason is to avoid leaking physical addresse to user-space, and >>> it follows a general move over the kernel code to avoid user-space to >>> manipulate physical addresses and then use some other mechanisms like >>> dma-buf to transfer physical buffer handles over multiple subsystems. >>> >>> But, a lot of devices depends on closed sources binaries to enable >>> OpenGL hardware acceleration that uses this smem_start value to >>> pass physical addresses to out-of-tree modules in order to render >>> into these physical adresses. These should use dma-buf buffers allocated >>> from the DRM display device instead and stop relying on fbdev overallocation >>> to gather DMA memory (some HW vendors delivers GBM and Wayland capable >>> binaries, but older unsupported devices won't have these new binaries >>> and are doomed until an Open Source solution like Lima finalizes). >>> >>> Since these devices heavily depends on this kind of software and because >>> the smem_start population was available for years, it's a breakage to >>> stop leaking smem_start without any alternative solutions. >>> >>> This patch adds a Kconfig depending on the EXPERT config and an unsafe >>> kernel module parameter tainting the kernel when enabled. >>> >>> A clear comment and Kconfig help text was added to clarify why and when >>> this patch should be reverted, but in the meantime it's a necessary >>> feature to keep. >>> >>> Cc: Dave Airlie <airlied@xxxxxxxxx> >>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> >>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx> >>> Cc: Noralf Trønnes <noralf@xxxxxxxxxxx> >>> Cc: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> >>> Cc: Eric Anholt <eric@xxxxxxxxxx> >>> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx> >>> Cc: Rob Clark <robdclark@xxxxxxxxx> >>> Cc: Ben Skeggs <skeggsb@xxxxxxxxx> >>> Cc: Christian König <christian.koenig@xxxxxxx> >>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx> >>> --- >>> drivers/gpu/drm/Kconfig | 15 +++++++++++++++ >>> drivers/gpu/drm/drm_fb_helper.c | 33 +++++++++++++++++++++++++++++++-- >>> 2 files changed, 46 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>> index 8871b3f..b07c298 100644 >>> --- a/drivers/gpu/drm/Kconfig >>> +++ b/drivers/gpu/drm/Kconfig >>> @@ -110,6 +110,21 @@ config DRM_FBDEV_OVERALLOC >>> is 100. Typical values for double buffering will be 200, >>> triple buffering 300. >>> >>> +config DRM_FBDEV_LEAK_PHYS_SMEM >>> + bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)" >>> + depends on DRM_FBDEV_EMULATION && EXPERT >>> + default n >>> + help >>> + In order to keep user-space compatibility, we want in certain >>> + use-cases to keep leaking the fbdev physical address to the >>> + user-space program handling the fbdev buffer. >>> + This option is a shame and should be removed as soon as possible >>> + and be considered as a broken and legacy behaviour from a modern >>> + fbdev device driver. > > s/shame/not supported by upstream developers/ Ack > > And maybe add "Please send any bug reports when using this to your > proprietary software vendor that requires this." It's (somehow) on the following line : >>> + If in doubt, say "N" or spread the word to your closed source >>> + library vendor. I will make it even scarier ! > > I'd also be in favour of just calling this the ARM Mali blob on > $affected_devices. Makes it easier for people to find the magic knob > they're looking for, if they're unlucky enough to have such a soc. I'm not sure it's only for Mali... Anyway I'll add "Mali" to help people findind it. > >>> + >>> + If in doubt, say "N" or spread the word to your closed source >>> + library vendor. >>> + >>> config DRM_LOAD_EDID_FIRMWARE >>> bool "Allow to specify an EDID data set instead of probing for it" >>> depends on DRM >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >>> index bf2190c..a354944 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -56,6 +56,25 @@ MODULE_PARM_DESC(drm_fbdev_overalloc, >>> "Overallocation of the fbdev buffer (%) [default=" >>> __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]"); >>> >>> +/* >>> + * In order to keep user-space compatibility, we want in certain use-cases >>> + * to keep leaking the fbdev physical address to the user-space program >>> + * handling the fbdev buffer. >>> + * This is a bad habit essentially kept into closed source opengl driver >>> + * that should really be moved into open-source upstream projects instead >>> + * of using legacy physical addresses in user space to communicate with >>> + * other out-of-tree kernel modules. >>> + * >>> + * This module_param *should* be removed as soon as possible and be >>> + * considered as a broken and legacy behaviour from a modern fbdev device. >>> + */ >>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) >>> +static bool drm_leak_fbdev_smem = false; >>> +module_param_unsafe(drm_leak_fbdev_smem, bool, 0600); >>> +MODULE_PARM_DESC(fbdev_emulation, >>> + "Allow unsafe leaking fbdev physical smem address [default=false]"); >>> +#endif >>> + >>> static LIST_HEAD(kernel_fb_helper_list); >>> static DEFINE_MUTEX(kernel_fb_helper_lock); >>> >>> @@ -2673,8 +2692,12 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper, >>> >>> info = fb_helper->fbdev; >>> info->var.pixclock = 0; >>> - /* don't leak any physical addresses to userspace */ >>> - info->flags |= FBINFO_HIDE_SMEM_START; >>> + /* Shamelessly allow physical address leaking to userspace */ >>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) >>> + if (!drm_leak_fbdev_smem) >>> +#endif >> >> You can use the IS_ENABLED directy within the if statement. It would >> make the code a bit nicer. It would, but 01.org send me ugly mails with undeclared errors... >> >>> + /* don't leak any physical addresses to userspace */ >>> + info->flags |= FBINFO_HIDE_SMEM_START; >>> >>> /* Need to drop locks to avoid recursive deadlock in >>> * register_framebuffer. This is ok because the only thing left to do is >>> @@ -3083,6 +3106,12 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >>> fbi->fbops = &drm_fbdev_fb_ops; >>> fbi->screen_size = fb->height * fb->pitches[0]; >>> fbi->fix.smem_len = fbi->screen_size; >>> + /* Shamelessly leak the physical address to user-space */ >>> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) >>> + if (drm_leak_fbdev_smem) >>> + fbi->fix.smem_start = >>> + page_to_phys(virt_to_page(fbi->screen_buffer)); >>> +#endif >> >> fbi->screen_buffer is only set.. >> >>> fbi->screen_buffer = buffer->vaddr; >> >> .. at the next line. Moving that line before the if statement works. Oops, will fix... >> >> With that fixed, and the IS_ENABLED in the if >> >> Reviewed-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> >> Tested-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> > > I think all the help text is sufficiently to scare people away from using > this by accident. > > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> Thanks, Neil >> >> Thanks! >> Maxime >> >> -- >> Maxime Ripard, Bootlin >> Embedded Linux and Kernel engineering >> https://bootlin.com > > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
