diff mbox series

drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

Message ID 20201023203957.3255-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported() | expand

Commit Message

Ville Syrjälä Oct. 23, 2020, 8:39 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The code responsible for creating the IN_FORMATS
blob is broken when the driver doesn't provide a
.format_mod_supported() hook. It just copies in
the format list, but leaves all the modifier information
zeroed. That would indicate (in a very silly way) that
there are in fact no supported format+modifier combinations.
That is utter nonsense.

Let's just not create the blob at all in that case. The
alternative would be to assume all format+mod combos will
work and populate it accordingly. But I'm not convinced we
can make that promise to userspace for all the drivers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Simon Ser Oct. 23, 2020, 10:03 p.m. UTC | #1 
On Friday, October 23, 2020 10:39 PM, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä ville.syrjala@linux.intel.com
>
> The code responsible for creating the IN_FORMATS
> blob is broken when the driver doesn't provide a
> .format_mod_supported() hook. It just copies in
> the format list, but leaves all the modifier information
> zeroed. That would indicate (in a very silly way) that
> there are in fact no supported format+modifier combinations.
> That is utter nonsense.

Should we WARN_ON when the driver enables allow_fb_modifiers but
doesn't populate format_mod_supported?
Ville Syrjälä Oct. 23, 2020, 10:52 p.m. UTC | #2 
On Fri, Oct 23, 2020 at 10:03:50PM +0000, Simon Ser wrote:
> On Friday, October 23, 2020 10:39 PM, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> 
> > From: Ville Syrjälä ville.syrjala@linux.intel.com
> >
> > The code responsible for creating the IN_FORMATS
> > blob is broken when the driver doesn't provide a
> > .format_mod_supported() hook. It just copies in
> > the format list, but leaves all the modifier information
> > zeroed. That would indicate (in a very silly way) that
> > there are in fact no supported format+modifier combinations.
> > That is utter nonsense.
> 
> Should we WARN_ON when the driver enables allow_fb_modifiers but
> doesn't populate format_mod_supported?

.format_mod_supported() was supposed to be optional IIRC.

But now that I look at it, it looks like only these
drivers are lacking .format_mod_supported(): exynos, mxsfb,
tiny/cirrus, tiny/gm12u320.

There is some other oddity going on with sun4i which sometimes
uses modifiers sometimes it doesn't. No idea what is going on there.
But it does have .format_mod_supported() at least.

So I guess if we can get exynos, mxsfb, and tiny/* to implement
the hook then we could make it mandatory.
Daniel Vetter Oct. 26, 2020, 9 a.m. UTC | #3 
On Sat, Oct 24, 2020 at 12:52 AM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Oct 23, 2020 at 10:03:50PM +0000, Simon Ser wrote:
> > On Friday, October 23, 2020 10:39 PM, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> >
> > > From: Ville Syrjälä ville.syrjala@linux.intel.com
> > >
> > > The code responsible for creating the IN_FORMATS
> > > blob is broken when the driver doesn't provide a
> > > .format_mod_supported() hook. It just copies in
> > > the format list, but leaves all the modifier information
> > > zeroed. That would indicate (in a very silly way) that
> > > there are in fact no supported format+modifier combinations.
> > > That is utter nonsense.
> >
> > Should we WARN_ON when the driver enables allow_fb_modifiers but
> > doesn't populate format_mod_supported?
>
> .format_mod_supported() was supposed to be optional IIRC.
>
> But now that I look at it, it looks like only these
> drivers are lacking .format_mod_supported(): exynos, mxsfb,
> tiny/cirrus, tiny/gm12u320.
>
> There is some other oddity going on with sun4i which sometimes
> uses modifiers sometimes it doesn't. No idea what is going on there.
> But it does have .format_mod_supported() at least.
>
> So I guess if we can get exynos, mxsfb, and tiny/* to implement
> the hook then we could make it mandatory.

I'd just delete it all, since it's obviously not tested for these
drivers. And then add a patch which warns if allow_fb_modifiers,
modifiers list passed y/n and .format_mod_support don't all agree.
Since a bunch of your don't even set allow_fb_modifiers but pass a
format list. So it's a complete mess :-/

Next step would then be to add some todo items, at least the
simple/tiny drivers should all be able to do this fairly easily, and
probably with linear only as the default.
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index e6231947f987..202a2b680947 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -124,10 +124,6 @@  static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
 
 	memcpy(formats_ptr(blob_data), plane->format_types, formats_size);
 
-	/* If we can't determine support, just bail */
-	if (!plane->funcs->format_mod_supported)
-		goto done;
-
 	mod = modifiers_ptr(blob_data);
 	for (i = 0; i < plane->modifier_count; i++) {
 		for (j = 0; j < plane->format_count; j++) {
@@ -145,7 +141,6 @@  static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane
 		mod++;
 	}
 
-done:
 	drm_object_attach_property(&plane->base, config->modifiers_property,
 				   blob->base.id);
 
@@ -281,7 +276,7 @@  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
 	}
 
-	if (config->allow_fb_modifiers)
+	if (config->allow_fb_modifiers && funcs->format_mod_supported)
 		create_in_format_blob(dev, plane);
 
 	return 0;