[Intel-gfx] [PATCH] drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

[Intel-gfx] [PATCH] drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

[Ville Syrjala]

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;
-- 
2.26.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjala]

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;
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 4672 bytes --]

== Series Details ==

Series: drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()
URL   : https://patchwork.freedesktop.org/series/83019/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9191 -> Patchwork_18778
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18778 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18778, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18778:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-cml-s:           [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-cml-s/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-cml-s/igt@i915_selftest@live@execlists.html

  
Known issues
------------

  Here are the changes found in Patchwork_18778 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@execlists:
    - fi-skl-6600u:       [PASS][3] -> [INCOMPLETE][4] ([CI#80])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-skl-6600u/igt@i915_selftest@live@execlists.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-skl-6600u/igt@i915_selftest@live@execlists.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-threads:
    - fi-apl-guc:         [INCOMPLETE][5] ([i915#1635]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-apl-guc/igt@gem_close_race@basic-threads.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-apl-guc/igt@gem_close_race@basic-threads.html

  * igt@i915_pm_rpm@module-reload:
    - fi-byt-j1900:       [DMESG-WARN][7] ([i915#1982]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [DMESG-WARN][9] ([i915#165]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-kbl-7500u:       [DMESG-WARN][11] ([i915#2203]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - fi-icl-u2:          [DMESG-WARN][13] ([i915#1982]) -> [PASS][14] +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9191/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  
  [CI#80]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/80
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#165]: https://gitlab.freedesktop.org/drm/intel/issues/165
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2203]: https://gitlab.freedesktop.org/drm/intel/issues/2203


Participating hosts (46 -> 39)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_9191 -> Patchwork_18778

  CI-20190529: 20190529
  CI_DRM_9191: 4b693bbb9b41fda404b5cd081bf5cd8dba240468 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5823: 7dd2fe99bd9dde00456cc5abf7e5ef0c8d7d6118 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18778: d9e4406ae38a3735c63ef947b55686d1dd936100 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

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

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18778/index.html

[-- Attachment #1.2: Type: text/html, Size: 5523 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

[Simon Ser]

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?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Simon Ser]

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?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm: Don't create the IN_FORMATS blob when the driver does not provide .format_mod_supported()

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel