Re: [PATCH 05/20] drm/meson: Use drm_fbdev_generic_setup()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux