diff mbox series

drm/simple-kms: Drop drm_simple_kms_format_mod_supported.

Message ID 20210124204748.25234-1-mario.kleiner.de@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/simple-kms: Drop drm_simple_kms_format_mod_supported. | expand

Commit Message

Mario Kleiner Jan. 24, 2021, 8:47 p.m. UTC
The check was introduced to make sure that only the
DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.

However, if .format_mod_supported is not hooked up to
drm_simple_kms_format_mod_supported then the core will
simply validate modifiers against the format_modifiers
list passed into drm_simple_display_pipe_init() or
drm_universal_plane_init() and perform the same validation
as drm_simple_kms_format_mod_supported() would have done.

Additionally, if a kms driver / plane does not support
modifiers, it will not reject fb updates with no modifiers/
DRM_FORMAT_MOD_INVALID. This is important, because some
simple drm drivers, e.g., pl111, pass NULL as format_modifiers
list, so modifier support is disabled for these drivers,
userspace would fall back to drmAddFB() without modifiers,
and ergo the current drm_simple_kms_format_mod_supported()
function would reject valid modifier-less fb's.

So this should fix display on non-tinydrm drivers like
pl111, and probably also for non-atomic clients?

The Mesa vc4 gallium driver mentions pl111 as one possible
display driver in render_only mode, so i assume this matters
for some SoC's?

The background for the patch that introduced this was to
fix atomic modesetting in the X-Servers modesetting-ddx,
but with atomic modesetting and format modifiers disabled
in modesetting-ddx (and also current kernels when interacting
with modesetting-ddx), i assume this should fix some panels.

Note that i don't have any of the hw required for testing
this, this is purely based on looking at the code, so this
patch is only compile-tested.

For more reference, this fix was motivated by some discussions
around broken page-flipping on VideoCore6 / RaspberryPi 4
with current Raspbian OS, so the experts may want to weigh
in on that Mesa bug report as well, under the following link:

https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601

Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only DRM_FORMAT_MOD_LINEAR.")
Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Eric Anholt <eric@anholt.net>
Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------
 1 file changed, 8 deletions(-)

Comments

Ville Syrjälä Jan. 25, 2021, 12:13 p.m. UTC | #1
On Sun, Jan 24, 2021 at 09:47:48PM +0100, Mario Kleiner wrote:
> The check was introduced to make sure that only the
> DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.
> 
> However, if .format_mod_supported is not hooked up to
> drm_simple_kms_format_mod_supported then the core will
> simply validate modifiers against the format_modifiers
> list passed into drm_simple_display_pipe_init() or
> drm_universal_plane_init() and perform the same validation
> as drm_simple_kms_format_mod_supported() would have done.
> 
> Additionally, if a kms driver / plane does not support
> modifiers, it will not reject fb updates with no modifiers/
> DRM_FORMAT_MOD_INVALID. This is important, because some
> simple drm drivers, e.g., pl111, pass NULL as format_modifiers
> list, so modifier support is disabled for these drivers,
> userspace would fall back to drmAddFB() without modifiers,
> and ergo the current drm_simple_kms_format_mod_supported()
> function would reject valid modifier-less fb's.
> 
> So this should fix display on non-tinydrm drivers like
> pl111, and probably also for non-atomic clients?
> 
> The Mesa vc4 gallium driver mentions pl111 as one possible
> display driver in render_only mode, so i assume this matters
> for some SoC's?
> 
> The background for the patch that introduced this was to
> fix atomic modesetting in the X-Servers modesetting-ddx,
> but with atomic modesetting and format modifiers disabled
> in modesetting-ddx (and also current kernels when interacting
> with modesetting-ddx), i assume this should fix some panels.
> 
> Note that i don't have any of the hw required for testing
> this, this is purely based on looking at the code, so this
> patch is only compile-tested.
> 
> For more reference, this fix was motivated by some discussions
> around broken page-flipping on VideoCore6 / RaspberryPi 4
> with current Raspbian OS, so the experts may want to weigh
> in on that Mesa bug report as well, under the following link:
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601
> 
> Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only DRM_FORMAT_MOD_LINEAR.")
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 743e57c1b44f..5f3e30553172 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -229,13 +229,6 @@ static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane,
>  	pipe->funcs->cleanup_fb(pipe, state);
>  }
>  
> -static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
> -						uint32_t format,
> -						uint64_t modifier)
> -{
> -	return modifier == DRM_FORMAT_MOD_LINEAR;
> -}
> -
>  static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
>  	.prepare_fb = drm_simple_kms_plane_prepare_fb,
>  	.cleanup_fb = drm_simple_kms_plane_cleanup_fb,
> @@ -250,7 +243,6 @@ static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
>  	.reset			= drm_atomic_helper_plane_reset,
>  	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> -	.format_mod_supported   = drm_simple_kms_format_mod_supported,

This will now cause also this driver to report a totally borked
IN_FORMATS blob.

Would need this
https://patchwork.freedesktop.org/series/83018/

but IIRC the bikeshed around that kinda suggested we should just
implement .format_mod_support() always. Can't quite recall the
details anymore.
Mario Kleiner Jan. 25, 2021, 3:32 p.m. UTC | #2
On Mon, Jan 25, 2021 at 1:13 PM Ville Syrjälä <ville.syrjala@linux.intel.com>
wrote:

> On Sun, Jan 24, 2021 at 09:47:48PM +0100, Mario Kleiner wrote:
> > The check was introduced to make sure that only the
> > DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.
> >
> > However, if .format_mod_supported is not hooked up to
> > drm_simple_kms_format_mod_supported then the core will
> > simply validate modifiers against the format_modifiers
> > list passed into drm_simple_display_pipe_init() or
> > drm_universal_plane_init() and perform the same validation
> > as drm_simple_kms_format_mod_supported() would have done.
> >
> > Additionally, if a kms driver / plane does not support
> > modifiers, it will not reject fb updates with no modifiers/
> > DRM_FORMAT_MOD_INVALID. This is important, because some
> > simple drm drivers, e.g., pl111, pass NULL as format_modifiers
> > list, so modifier support is disabled for these drivers,
> > userspace would fall back to drmAddFB() without modifiers,
> > and ergo the current drm_simple_kms_format_mod_supported()
> > function would reject valid modifier-less fb's.
> >
> > So this should fix display on non-tinydrm drivers like
> > pl111, and probably also for non-atomic clients?
> >
> > The Mesa vc4 gallium driver mentions pl111 as one possible
> > display driver in render_only mode, so i assume this matters
> > for some SoC's?
> >
> > The background for the patch that introduced this was to
> > fix atomic modesetting in the X-Servers modesetting-ddx,
> > but with atomic modesetting and format modifiers disabled
> > in modesetting-ddx (and also current kernels when interacting
> > with modesetting-ddx), i assume this should fix some panels.
> >
> > Note that i don't have any of the hw required for testing
> > this, this is purely based on looking at the code, so this
> > patch is only compile-tested.
> >
> > For more reference, this fix was motivated by some discussions
> > around broken page-flipping on VideoCore6 / RaspberryPi 4
> > with current Raspbian OS, so the experts may want to weigh
> > in on that Mesa bug report as well, under the following link:
> >
> > https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601
> >
> > Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only
> DRM_FORMAT_MOD_LINEAR.")
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------
> >  1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index 743e57c1b44f..5f3e30553172 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -229,13 +229,6 @@ static void drm_simple_kms_plane_cleanup_fb(struct
> drm_plane *plane,
> >       pipe->funcs->cleanup_fb(pipe, state);
> >  }
> >
> > -static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
> > -                                             uint32_t format,
> > -                                             uint64_t modifier)
> > -{
> > -     return modifier == DRM_FORMAT_MOD_LINEAR;
> > -}
> > -
> >  static const struct drm_plane_helper_funcs
> drm_simple_kms_plane_helper_funcs = {
> >       .prepare_fb = drm_simple_kms_plane_prepare_fb,
> >       .cleanup_fb = drm_simple_kms_plane_cleanup_fb,
> > @@ -250,7 +243,6 @@ static const struct drm_plane_funcs
> drm_simple_kms_plane_funcs = {
> >       .reset                  = drm_atomic_helper_plane_reset,
> >       .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >       .atomic_destroy_state   = drm_atomic_helper_plane_destroy_state,
> > -     .format_mod_supported   = drm_simple_kms_format_mod_supported,
>
> This will now cause also this driver to report a totally borked
> IN_FORMATS blob.
>
> Would need this
> https://patchwork.freedesktop.org/series/83018/
>
>
The slight problem with that (see my comments in the linked Mesa bug
report), is that at least one common userspace driver - modesetting-ddx -
treat a lack of an IN_FORMATS blob not as "don't use modifiers for
drm_framebuffers", but as "everything goes" -- Use every modifier and
tiling format that the graphics driver exposes also for scanout buffers.
I'm arguing in that bug report that modesetting-ddx shouldn't use atomic or
modifiers at all, given how broken that driver is atm. in that area, so i'm
not sure if my argument here is valid. Just saying that doing the "every
modifier is valid for every format" in absence of  format_mod_supported()
would probably be less harmful to some existing userspace. Ofc. then
there's a reason why atomic gets rejected by the kernel for current
modesetting-ddx...

I'm not sure if I'm arguing pro or contra your patch here btw. Just
pointing out one possible victim if it were applied.

but IIRC the bikeshed around that kinda suggested we should just
> implement .format_mod_support() always. Can't quite recall the
> details anymore.
>
>
I see. But if .format_mod_supported() is always implemented, then we'd need
to handle the case modifier == DRM_FORMAT_MOD_INVALID in the core or in
each format_mod_supported() implementation, because currently iff this is
hooked up, it gets always used, even if the user-space does not use
modifiers. The X-Servers modesetting-ddx, e.g., does not use atomic or
modifiers by default, and the linked Mesa bug report shows why - or at
least why it shouldn't atm. I think none of the X drivers does.

The softer alternative solution instead of my patch would be to also accept
modifier == DRM_FORMAT_MOD_INVALID as valid for simple kms drivers.
-mario
Ville Syrjälä Jan. 25, 2021, 4:34 p.m. UTC | #3
On Mon, Jan 25, 2021 at 04:32:48PM +0100, Mario Kleiner wrote:
> On Mon, Jan 25, 2021 at 1:13 PM Ville Syrjälä <ville.syrjala@linux.intel.com>
> wrote:
> 
> > On Sun, Jan 24, 2021 at 09:47:48PM +0100, Mario Kleiner wrote:
> > > The check was introduced to make sure that only the
> > > DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.
> > >
> > > However, if .format_mod_supported is not hooked up to
> > > drm_simple_kms_format_mod_supported then the core will
> > > simply validate modifiers against the format_modifiers
> > > list passed into drm_simple_display_pipe_init() or
> > > drm_universal_plane_init() and perform the same validation
> > > as drm_simple_kms_format_mod_supported() would have done.
> > >
> > > Additionally, if a kms driver / plane does not support
> > > modifiers, it will not reject fb updates with no modifiers/
> > > DRM_FORMAT_MOD_INVALID. This is important, because some
> > > simple drm drivers, e.g., pl111, pass NULL as format_modifiers
> > > list, so modifier support is disabled for these drivers,
> > > userspace would fall back to drmAddFB() without modifiers,
> > > and ergo the current drm_simple_kms_format_mod_supported()
> > > function would reject valid modifier-less fb's.
> > >
> > > So this should fix display on non-tinydrm drivers like
> > > pl111, and probably also for non-atomic clients?
> > >
> > > The Mesa vc4 gallium driver mentions pl111 as one possible
> > > display driver in render_only mode, so i assume this matters
> > > for some SoC's?
> > >
> > > The background for the patch that introduced this was to
> > > fix atomic modesetting in the X-Servers modesetting-ddx,
> > > but with atomic modesetting and format modifiers disabled
> > > in modesetting-ddx (and also current kernels when interacting
> > > with modesetting-ddx), i assume this should fix some panels.
> > >
> > > Note that i don't have any of the hw required for testing
> > > this, this is purely based on looking at the code, so this
> > > patch is only compile-tested.
> > >
> > > For more reference, this fix was motivated by some discussions
> > > around broken page-flipping on VideoCore6 / RaspberryPi 4
> > > with current Raspbian OS, so the experts may want to weigh
> > > in on that Mesa bug report as well, under the following link:
> > >
> > > https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601
> > >
> > > Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only
> > DRM_FORMAT_MOD_LINEAR.")
> > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Noralf Trønnes <noralf@tronnes.org>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------
> > >  1 file changed, 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> > b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > index 743e57c1b44f..5f3e30553172 100644
> > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > @@ -229,13 +229,6 @@ static void drm_simple_kms_plane_cleanup_fb(struct
> > drm_plane *plane,
> > >       pipe->funcs->cleanup_fb(pipe, state);
> > >  }
> > >
> > > -static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
> > > -                                             uint32_t format,
> > > -                                             uint64_t modifier)
> > > -{
> > > -     return modifier == DRM_FORMAT_MOD_LINEAR;
> > > -}
> > > -
> > >  static const struct drm_plane_helper_funcs
> > drm_simple_kms_plane_helper_funcs = {
> > >       .prepare_fb = drm_simple_kms_plane_prepare_fb,
> > >       .cleanup_fb = drm_simple_kms_plane_cleanup_fb,
> > > @@ -250,7 +243,6 @@ static const struct drm_plane_funcs
> > drm_simple_kms_plane_funcs = {
> > >       .reset                  = drm_atomic_helper_plane_reset,
> > >       .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >       .atomic_destroy_state   = drm_atomic_helper_plane_destroy_state,
> > > -     .format_mod_supported   = drm_simple_kms_format_mod_supported,
> >
> > This will now cause also this driver to report a totally borked
> > IN_FORMATS blob.
> >
> > Would need this
> > https://patchwork.freedesktop.org/series/83018/
> >
> >
> The slight problem with that (see my comments in the linked Mesa bug
> report), is that at least one common userspace driver - modesetting-ddx -
> treat a lack of an IN_FORMATS blob not as "don't use modifiers for
> drm_framebuffers", but as "everything goes" -- Use every modifier and
> tiling format that the graphics driver exposes also for scanout buffers.
> I'm arguing in that bug report that modesetting-ddx shouldn't use atomic or
> modifiers at all, given how broken that driver is atm. in that area, so i'm
> not sure if my argument here is valid. Just saying that doing the "every
> modifier is valid for every format" in absence of  format_mod_supported()
> would probably be less harmful to some existing userspace. Ofc. then
> there's a reason why atomic gets rejected by the kernel for current
> modesetting-ddx...
> 
> I'm not sure if I'm arguing pro or contra your patch here btw. Just
> pointing out one possible victim if it were applied.

I have no idea how anything would get broken by it.

Currently:
- the broken IN_FORMAT blob says nothing is supported at all,
  so if someone consults it they won't be able to find a working
  pixel format
- if they ignore the broken IN_FORMAT blob then it doesn't matter
  if it's present or not.

I guess the only way somehting might be affected is if they just use
the presence of the IN_FORMATS blob as a hint for something but never
actually look at the contents.

> 
> but IIRC the bikeshed around that kinda suggested we should just
> > implement .format_mod_support() always. Can't quite recall the
> > details anymore.
> >
> >
> I see. But if .format_mod_supported() is always implemented, then we'd need
> to handle the case modifier == DRM_FORMAT_MOD_INVALID in the core or in
> each format_mod_supported() implementation, because currently iff this is
> hooked up, it gets always used, even if the user-space does not use
> modifiers. The X-Servers modesetting-ddx, e.g., does not use atomic or
> modifiers by default, and the linked Mesa bug report shows why - or at
> least why it shouldn't atm. I think none of the X drivers does.
> 
> The softer alternative solution instead of my patch would be to also accept
> modifier == DRM_FORMAT_MOD_INVALID as valid for simple kms drivers.

DRM_FORMAT_MOD_INVALID is not a valid modifier.
What kind of broken code is using it?
Mario Kleiner Jan. 25, 2021, 6:19 p.m. UTC | #4
On Mon, Jan 25, 2021 at 5:34 PM Ville Syrjälä <ville.syrjala@linux.intel.com>
wrote:

> On Mon, Jan 25, 2021 at 04:32:48PM +0100, Mario Kleiner wrote:
> > On Mon, Jan 25, 2021 at 1:13 PM Ville Syrjälä <
> ville.syrjala@linux.intel.com>
> > wrote:
> >
> > > On Sun, Jan 24, 2021 at 09:47:48PM +0100, Mario Kleiner wrote:
> > > > The check was introduced to make sure that only the
> > > > DRM_FORMAT_MOD_LINEAR modifier is accepted by tinydrm.
> > > >
> > > > However, if .format_mod_supported is not hooked up to
> > > > drm_simple_kms_format_mod_supported then the core will
> > > > simply validate modifiers against the format_modifiers
> > > > list passed into drm_simple_display_pipe_init() or
> > > > drm_universal_plane_init() and perform the same validation
> > > > as drm_simple_kms_format_mod_supported() would have done.
> > > >
> > > > Additionally, if a kms driver / plane does not support
> > > > modifiers, it will not reject fb updates with no modifiers/
> > > > DRM_FORMAT_MOD_INVALID. This is important, because some
> > > > simple drm drivers, e.g., pl111, pass NULL as format_modifiers
> > > > list, so modifier support is disabled for these drivers,
> > > > userspace would fall back to drmAddFB() without modifiers,
> > > > and ergo the current drm_simple_kms_format_mod_supported()
> > > > function would reject valid modifier-less fb's.
> > > >
> > > > So this should fix display on non-tinydrm drivers like
> > > > pl111, and probably also for non-atomic clients?
> > > >
> > > > The Mesa vc4 gallium driver mentions pl111 as one possible
> > > > display driver in render_only mode, so i assume this matters
> > > > for some SoC's?
> > > >
> > > > The background for the patch that introduced this was to
> > > > fix atomic modesetting in the X-Servers modesetting-ddx,
> > > > but with atomic modesetting and format modifiers disabled
> > > > in modesetting-ddx (and also current kernels when interacting
> > > > with modesetting-ddx), i assume this should fix some panels.
> > > >
> > > > Note that i don't have any of the hw required for testing
> > > > this, this is purely based on looking at the code, so this
> > > > patch is only compile-tested.
> > > >
> > > > For more reference, this fix was motivated by some discussions
> > > > around broken page-flipping on VideoCore6 / RaspberryPi 4
> > > > with current Raspbian OS, so the experts may want to weigh
> > > > in on that Mesa bug report as well, under the following link:
> > > >
> > > > https://gitlab.freedesktop.org/mesa/mesa/-/issues/3601
> > > >
> > > > Fixes: dff906c3f91c ("drm/tinydrm: Advertise that we can do only
> > > DRM_FORMAT_MOD_LINEAR.")
> > > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > > > Cc: Eric Anholt <eric@anholt.net>
> > > > Cc: Noralf Trønnes <noralf@tronnes.org>
> > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_simple_kms_helper.c | 8 --------
> > > >  1 file changed, 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > index 743e57c1b44f..5f3e30553172 100644
> > > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > > > @@ -229,13 +229,6 @@ static void
> drm_simple_kms_plane_cleanup_fb(struct
> > > drm_plane *plane,
> > > >       pipe->funcs->cleanup_fb(pipe, state);
> > > >  }
> > > >
> > > > -static bool drm_simple_kms_format_mod_supported(struct drm_plane
> *plane,
> > > > -                                             uint32_t format,
> > > > -                                             uint64_t modifier)
> > > > -{
> > > > -     return modifier == DRM_FORMAT_MOD_LINEAR;
> > > > -}
> > > > -
> > > >  static const struct drm_plane_helper_funcs
> > > drm_simple_kms_plane_helper_funcs = {
> > > >       .prepare_fb = drm_simple_kms_plane_prepare_fb,
> > > >       .cleanup_fb = drm_simple_kms_plane_cleanup_fb,
> > > > @@ -250,7 +243,6 @@ static const struct drm_plane_funcs
> > > drm_simple_kms_plane_funcs = {
> > > >       .reset                  = drm_atomic_helper_plane_reset,
> > > >       .atomic_duplicate_state =
> drm_atomic_helper_plane_duplicate_state,
> > > >       .atomic_destroy_state   =
> drm_atomic_helper_plane_destroy_state,
> > > > -     .format_mod_supported   = drm_simple_kms_format_mod_supported,
> > >
> > > This will now cause also this driver to report a totally borked
> > > IN_FORMATS blob.
> > >
> > > Would need this
> > > https://patchwork.freedesktop.org/series/83018/
> > >
> > >
> > The slight problem with that (see my comments in the linked Mesa bug
> > report), is that at least one common userspace driver - modesetting-ddx -
> > treat a lack of an IN_FORMATS blob not as "don't use modifiers for
> > drm_framebuffers", but as "everything goes" -- Use every modifier and
> > tiling format that the graphics driver exposes also for scanout buffers.
> > I'm arguing in that bug report that modesetting-ddx shouldn't use atomic
> or
> > modifiers at all, given how broken that driver is atm. in that area, so
> i'm
> > not sure if my argument here is valid. Just saying that doing the "every
> > modifier is valid for every format" in absence of  format_mod_supported()
> > would probably be less harmful to some existing userspace. Ofc. then
> > there's a reason why atomic gets rejected by the kernel for current
> > modesetting-ddx...
> >
> > I'm not sure if I'm arguing pro or contra your patch here btw. Just
> > pointing out one possible victim if it were applied.
>
> I have no idea how anything would get broken by it.
>
> Currently:
> - the broken IN_FORMAT blob says nothing is supported at all,
>   so if someone consults it they won't be able to find a working
>   pixel format
> - if they ignore the broken IN_FORMAT blob then it doesn't matter
>   if it's present or not.
>
>
Yes. What modesetting-ddx does iff xorg.conf Option "Debug"
"dmabuf_capable" is set, is it tries to get the list of display capable
modifiers. If the list is empty (due to broken IN_FORMAT blob, because
.format_mod_supported == NULL for a kms driver in the current kernel), or
because the IN_FORMAT blob doesn't exist (if your patch is applied and
.format_mod_supported == NULL) or if it can't query in the first place
(because use of atomic uapi is disabled or rejected by the kernel - as is
the case for current kernels on modesetting-ddx), then it will
unfortunately not decide not to use modifiers, but instead will consider
all dmabuf modifiers exposed by the render driver as scanout capable, also
tiled formats and pass those back to Mesa's DRI3/Present backend or
Vulkan/WSI-X11 where things can go sideways, at least as so far shown for
v3d / Broadcom VideoCore6 / RPi-4.

Anyway, yes it probably could only break a broken userspace driver more.
But that is what modesetting-ddx is atm. iff one decided to enable the
dmabuf_capable option, which is what Raspbian currently does, for an
underwhelming RPi4 out of the box visual experience.

Looking at your patch i'd suggest to also reset

config->allow_fb_modifiers = false;

if .format_mod_supported == NULL, so clients that check for the
DRM_CAP_ADDFB2_MODIFIERS capability will more likely stay away from
modifiers. At least modesetting-ddx would fall back to drmModeAddFB instead
of drmModeAddFBWithModifiers, which would be better, but probably still
broken. It's complicated...

I guess the only way somehting might be affected is if they just use
> the presence of the IN_FORMATS blob as a hint for something but never
> actually look at the contents.
>

Or at DRM_CAP_ADDFB2_MODIFIERS query result, see above.


>
> >
> > but IIRC the bikeshed around that kinda suggested we should just
> > > implement .format_mod_support() always. Can't quite recall the
> > > details anymore.
> > >
> > >
> > I see. But if .format_mod_supported() is always implemented, then we'd
> need
> > to handle the case modifier == DRM_FORMAT_MOD_INVALID in the core or in
> > each format_mod_supported() implementation, because currently iff this is
> > hooked up, it gets always used, even if the user-space does not use
> > modifiers. The X-Servers modesetting-ddx, e.g., does not use atomic or
> > modifiers by default, and the linked Mesa bug report shows why - or at
> > least why it shouldn't atm. I think none of the X drivers does.
> >
> > The softer alternative solution instead of my patch would be to also
> accept
> > modifier == DRM_FORMAT_MOD_INVALID as valid for simple kms drivers.
>
> DRM_FORMAT_MOD_INVALID is not a valid modifier.
> What kind of broken code is using it?
>
>
Oh sorry, i meant DRM_FORMAT_MOD_NONE (==0) ie. what is in fb->modifier if
userspace doesn't assign a modifier, e.g., drmModeAddFB().

But now after looking again, i just realized that DRM_FORMAT_MOD_NONE is
identical to DRM_FORMAT_MOD_LINEAR, ie. both are zero, so i guess my point
is moot.

That actually means this whole patch of mine is not needed and I was
hunting a ghost :/. It also means that simple drm drivers should probably
work with RPi if they revert that dmabuf_capable patch on Raspbian's
modesetting-ddx with no need for extra kernel fixes, so this is not wasted.

Thanks for helping me think!
-mario
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 743e57c1b44f..5f3e30553172 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -229,13 +229,6 @@  static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane,
 	pipe->funcs->cleanup_fb(pipe, state);
 }
 
-static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
-						uint32_t format,
-						uint64_t modifier)
-{
-	return modifier == DRM_FORMAT_MOD_LINEAR;
-}
-
 static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
 	.prepare_fb = drm_simple_kms_plane_prepare_fb,
 	.cleanup_fb = drm_simple_kms_plane_cleanup_fb,
@@ -250,7 +243,6 @@  static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
 	.reset			= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
-	.format_mod_supported   = drm_simple_kms_format_mod_supported,
 };
 
 /**