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
