[PATCH] drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()
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(-)
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;
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?
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.
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Simon Ser -
Ville Syrjala -
Ville Syrjälä