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. > + > + 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. > + /* 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. With that fixed, and the IS_ENABLED in the if Reviewed-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> Tested-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> Thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
