| Message ID | 20180908134648.2582-6-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/cma-helper drivers: Use drm_fbdev_generic_setup() | expand |
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. Neil > > Cc: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/meson/meson_drv.c | 19 ++----------------- > drivers/gpu/drm/meson/meson_drv.h | 1 - > 2 files changed, 2 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c > index d3443125e661..348b5a198b9d 100644 > --- a/drivers/gpu/drm/meson/meson_drv.c > +++ b/drivers/gpu/drm/meson/meson_drv.c > @@ -68,15 +68,7 @@ > * - Powering Up HDMI controller and PHY > */ > > -static void meson_fb_output_poll_changed(struct drm_device *dev) > -{ > - struct meson_drm *priv = dev->dev_private; > - > - drm_fbdev_cma_hotplug_event(priv->fbdev); > -} > - > static const struct drm_mode_config_funcs meson_mode_config_funcs = { > - .output_poll_changed = meson_fb_output_poll_changed, > .atomic_check = drm_atomic_helper_check, > .atomic_commit = drm_atomic_helper_commit, > .fb_create = drm_gem_fb_create, > @@ -282,13 +274,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > > drm_mode_config_reset(drm); > > - priv->fbdev = drm_fbdev_cma_init(drm, 32, > - drm->mode_config.num_connector); > - if (IS_ERR(priv->fbdev)) { > - ret = PTR_ERR(priv->fbdev); > - goto free_drm; > - } > - > drm_kms_helper_poll_init(drm); > > platform_set_drvdata(pdev, priv); > @@ -297,6 +282,8 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > if (ret) > goto free_drm; > > + drm_fbdev_generic_setup(drm, 32); > + > return 0; > > free_drm: > @@ -313,11 +300,9 @@ static int meson_drv_bind(struct device *dev) > static void meson_drv_unbind(struct device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > - struct meson_drm *priv = drm->dev_private; > > drm_dev_unregister(drm); > drm_kms_helper_poll_fini(drm); > - drm_fbdev_cma_fini(priv->fbdev); > drm_mode_config_cleanup(drm); > drm_dev_put(drm); > > diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h > index 8450d6ac8c9b..aab96260da9f 100644 > --- a/drivers/gpu/drm/meson/meson_drv.h > +++ b/drivers/gpu/drm/meson/meson_drv.h > @@ -33,7 +33,6 @@ struct meson_drm { > > struct drm_device *drm; > struct drm_crtc *crtc; > - struct drm_fbdev_cma *fbdev; > struct drm_plane *primary_plane; > > /* Components Data */ >
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. Maxime
(Cc: Daniel Vetter) 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.
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. > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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. 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
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@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >> >
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). 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. 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@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>> > >> > > >
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 ? @Noaralf, do you have a branch somewhere I can base a work on ? 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@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>> >>>> >>> >> >
On Fri, Sep 14, 2018 at 10:23 AM, Neil Armstrong <narmstrong@baylibre.com> wrote: > 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 ? smem_start is gone already, for everyone. If you want to delay this, then either you need to revert my commits, or get your hack in quickly. -Daniel > @Noaralf, do you have a branch somewhere I can base a work on ? > > 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@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>> >>>>> >>>> >>> >> >
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@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>>
Hi Noralf, Daniel, On 14/09/2018 18:33, Noralf Trønnes wrote: > > 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 Thanks, I'll try to drop something, but not immediately. Neil > > 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@lists.freedesktop.org >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>>> >
Hi Noralf, On 17/09/2018 09:53, Neil Armstrong wrote: > Hi Noralf, Daniel, > > On 14/09/2018 18:33, Noralf Trønnes wrote: >> >> 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 Did you have time reviewing the v2 of "drm/fb_helper: Allow leaking fbdev smem_start" ? I'll be happy acking this patch once the smem_start stuff gets sorted out. Thanks, Neil [...]
Hi Noralf, Le 08/09/2018 15:46, Noralf Trønnes a écrit : > 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. > > Cc: Neil Armstrong <narmstrong@baylibre.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- > drivers/gpu/drm/meson/meson_drv.c | 19 ++----------------- > drivers/gpu/drm/meson/meson_drv.h | 1 - > 2 files changed, 2 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c > index d3443125e661..348b5a198b9d 100644 > --- a/drivers/gpu/drm/meson/meson_drv.c > +++ b/drivers/gpu/drm/meson/meson_drv.c > @@ -68,15 +68,7 @@ > * - Powering Up HDMI controller and PHY > */ > > -static void meson_fb_output_poll_changed(struct drm_device *dev) > -{ > - struct meson_drm *priv = dev->dev_private; > - > - drm_fbdev_cma_hotplug_event(priv->fbdev); > -} > - > static const struct drm_mode_config_funcs meson_mode_config_funcs = { > - .output_poll_changed = meson_fb_output_poll_changed, > .atomic_check = drm_atomic_helper_check, > .atomic_commit = drm_atomic_helper_commit, > .fb_create = drm_gem_fb_create, > @@ -282,13 +274,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > > drm_mode_config_reset(drm); > > - priv->fbdev = drm_fbdev_cma_init(drm, 32, > - drm->mode_config.num_connector); > - if (IS_ERR(priv->fbdev)) { > - ret = PTR_ERR(priv->fbdev); > - goto free_drm; > - } > - > drm_kms_helper_poll_init(drm); > > platform_set_drvdata(pdev, priv); > @@ -297,6 +282,8 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) > if (ret) > goto free_drm; > > + drm_fbdev_generic_setup(drm, 32); > + > return 0; > > free_drm: > @@ -313,11 +300,9 @@ static int meson_drv_bind(struct device *dev) > static void meson_drv_unbind(struct device *dev) > { > struct drm_device *drm = dev_get_drvdata(dev); > - struct meson_drm *priv = drm->dev_private; > > drm_dev_unregister(drm); > drm_kms_helper_poll_fini(drm); > - drm_fbdev_cma_fini(priv->fbdev); > drm_mode_config_cleanup(drm); > drm_dev_put(drm); > > diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h > index 8450d6ac8c9b..aab96260da9f 100644 > --- a/drivers/gpu/drm/meson/meson_drv.h > +++ b/drivers/gpu/drm/meson/meson_drv.h > @@ -33,7 +33,6 @@ struct meson_drm { > > struct drm_device *drm; > struct drm_crtc *crtc; > - struct drm_fbdev_cma *fbdev; > struct drm_plane *primary_plane; > > /* Components Data */ > A little bit late to get this in 4.20 (or 5.0) and get rid of the old fbdev code, but at least we have a (dirty) workaround in place for the Mali fbdev blob... Acked-by: Neil Armstrong <narmstrong@baylibre.com> Thanks for pushing the fbdev emulation on the right path ! Neil
Den 03.10.2018 21.24, skrev Neil Armstrong: > Hi Noralf, > > Le 08/09/2018 15:46, Noralf Trønnes a écrit : >> 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. >> >> Cc: Neil Armstrong <narmstrong@baylibre.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- >> drivers/gpu/drm/meson/meson_drv.c | 19 ++----------------- >> drivers/gpu/drm/meson/meson_drv.h | 1 - >> 2 files changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c >> index d3443125e661..348b5a198b9d 100644 >> --- a/drivers/gpu/drm/meson/meson_drv.c >> +++ b/drivers/gpu/drm/meson/meson_drv.c >> @@ -68,15 +68,7 @@ >> * - Powering Up HDMI controller and PHY >> */ >> >> -static void meson_fb_output_poll_changed(struct drm_device *dev) >> -{ >> - struct meson_drm *priv = dev->dev_private; >> - >> - drm_fbdev_cma_hotplug_event(priv->fbdev); >> -} >> - >> static const struct drm_mode_config_funcs meson_mode_config_funcs = { >> - .output_poll_changed = meson_fb_output_poll_changed, >> .atomic_check = drm_atomic_helper_check, >> .atomic_commit = drm_atomic_helper_commit, >> .fb_create = drm_gem_fb_create, >> @@ -282,13 +274,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) >> >> drm_mode_config_reset(drm); >> >> - priv->fbdev = drm_fbdev_cma_init(drm, 32, >> - drm->mode_config.num_connector); >> - if (IS_ERR(priv->fbdev)) { >> - ret = PTR_ERR(priv->fbdev); >> - goto free_drm; >> - } >> - >> drm_kms_helper_poll_init(drm); >> >> platform_set_drvdata(pdev, priv); >> @@ -297,6 +282,8 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) >> if (ret) >> goto free_drm; >> >> + drm_fbdev_generic_setup(drm, 32); >> + >> return 0; >> >> free_drm: >> @@ -313,11 +300,9 @@ static int meson_drv_bind(struct device *dev) >> static void meson_drv_unbind(struct device *dev) >> { >> struct drm_device *drm = dev_get_drvdata(dev); >> - struct meson_drm *priv = drm->dev_private; >> >> drm_dev_unregister(drm); >> drm_kms_helper_poll_fini(drm); >> - drm_fbdev_cma_fini(priv->fbdev); >> drm_mode_config_cleanup(drm); >> drm_dev_put(drm); >> >> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h >> index 8450d6ac8c9b..aab96260da9f 100644 >> --- a/drivers/gpu/drm/meson/meson_drv.h >> +++ b/drivers/gpu/drm/meson/meson_drv.h >> @@ -33,7 +33,6 @@ struct meson_drm { >> >> struct drm_device *drm; >> struct drm_crtc *crtc; >> - struct drm_fbdev_cma *fbdev; >> struct drm_plane *primary_plane; >> >> /* Components Data */ >> > A little bit late to get this in 4.20 (or 5.0) and get rid of the old fbdev code, > but at least we have a (dirty) workaround in place for the Mali fbdev blob... Glad you found a solution for that! > Acked-by: Neil Armstrong <narmstrong@baylibre.com> > > Thanks for pushing the fbdev emulation on the right path ! Applied to drm-misc-next, thanks. Noralf. > Neil
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c index d3443125e661..348b5a198b9d 100644 --- a/drivers/gpu/drm/meson/meson_drv.c +++ b/drivers/gpu/drm/meson/meson_drv.c @@ -68,15 +68,7 @@ * - Powering Up HDMI controller and PHY */ -static void meson_fb_output_poll_changed(struct drm_device *dev) -{ - struct meson_drm *priv = dev->dev_private; - - drm_fbdev_cma_hotplug_event(priv->fbdev); -} - static const struct drm_mode_config_funcs meson_mode_config_funcs = { - .output_poll_changed = meson_fb_output_poll_changed, .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, .fb_create = drm_gem_fb_create, @@ -282,13 +274,6 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) drm_mode_config_reset(drm); - priv->fbdev = drm_fbdev_cma_init(drm, 32, - drm->mode_config.num_connector); - if (IS_ERR(priv->fbdev)) { - ret = PTR_ERR(priv->fbdev); - goto free_drm; - } - drm_kms_helper_poll_init(drm); platform_set_drvdata(pdev, priv); @@ -297,6 +282,8 @@ static int meson_drv_bind_master(struct device *dev, bool has_components) if (ret) goto free_drm; + drm_fbdev_generic_setup(drm, 32); + return 0; free_drm: @@ -313,11 +300,9 @@ static int meson_drv_bind(struct device *dev) static void meson_drv_unbind(struct device *dev) { struct drm_device *drm = dev_get_drvdata(dev); - struct meson_drm *priv = drm->dev_private; drm_dev_unregister(drm); drm_kms_helper_poll_fini(drm); - drm_fbdev_cma_fini(priv->fbdev); drm_mode_config_cleanup(drm); drm_dev_put(drm); diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h index 8450d6ac8c9b..aab96260da9f 100644 --- a/drivers/gpu/drm/meson/meson_drv.h +++ b/drivers/gpu/drm/meson/meson_drv.h @@ -33,7 +33,6 @@ struct meson_drm { struct drm_device *drm; struct drm_crtc *crtc; - struct drm_fbdev_cma *fbdev; struct drm_plane *primary_plane; /* Components Data */
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. Cc: Neil Armstrong <narmstrong@baylibre.com> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- drivers/gpu/drm/meson/meson_drv.c | 19 ++----------------- drivers/gpu/drm/meson/meson_drv.h | 1 - 2 files changed, 2 insertions(+), 18 deletions(-)