[PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[Ayan Kumar Halder]

If a plane supports a pixel format and the framebuffer does not pass any
modifiers, then drm_plane_check_pixel_format() should always return true
for the given format regardless of whether the plane supports any
modifiers or not.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
---
 drivers/gpu/drm/drm_plane.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 6d2a6e4..873c084 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -564,6 +564,9 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (!plane->modifier_count)
 		return 0;
 
+	if (modifier == DRM_FORMAT_MOD_NONE)
+		return 0;
+
 	for (i = 0; i < plane->modifier_count; i++) {
 		if (modifier == plane->modifiers[i])
 			break;
-- 
2.7.4

[PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[Ayan Kumar Halder]

If a plane supports a pixel format and the framebuffer does not pass any
modifiers, then drm_plane_check_pixel_format() should always return true
for the given format regardless of whether the plane supports any
modifiers or not.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
---
 drivers/gpu/drm/drm_plane.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 6d2a6e4..873c084 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -564,6 +564,9 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	if (!plane->modifier_count)
 		return 0;
 
+	if (modifier == DRM_FORMAT_MOD_NONE)
+		return 0;
+
 	for (i = 0; i < plane->modifier_count; i++) {
 		if (modifier == plane->modifiers[i])
 			break;
-- 
2.7.4

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

Re: [PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[Brian Starkey]

Hi Ayan,

On Fri, May 25, 2018 at 04:35:41PM +0100, Ayan Kumar Halder wrote:
>If a plane supports a pixel format and the framebuffer does not pass any
>modifiers, then drm_plane_check_pixel_format() should always return true
>for the given format regardless of whether the plane supports any
>modifiers or not.
>
>Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
>---
> drivers/gpu/drm/drm_plane.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>index 6d2a6e4..873c084 100644
>--- a/drivers/gpu/drm/drm_plane.c
>+++ b/drivers/gpu/drm/drm_plane.c
>@@ -564,6 +564,9 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
> 	if (!plane->modifier_count)
> 		return 0;
>
>+	if (modifier == DRM_FORMAT_MOD_NONE)
>+		return 0;
>+

There can be drivers which have formats which are only supported when
a modifier is used - so I don't think this check is right to have in
the core.

Drivers need to check if they support the format with no modifier
(same as DRM_FORMAT_MOD_LINEAR) in their format_mod_supported()
callback.

Thanks,
-Brian

> 	for (i = 0; i < plane->modifier_count; i++) {
> 		if (modifier == plane->modifiers[i])
> 			break;
>-- 
>2.7.4
>

Re: [PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[Ville Syrjälä]

On Fri, May 25, 2018 at 04:35:41PM +0100, Ayan Kumar Halder wrote:
> If a plane supports a pixel format and the framebuffer does not pass any
> modifiers, then drm_plane_check_pixel_format() should always return true
> for the given format regardless of whether the plane supports any
> modifiers or not.

If your plane supports DRM_FORMAT_MOD_LINEAR then you should add that
to the plane's modifier list.

> 
> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 6d2a6e4..873c084 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -564,6 +564,9 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
>  	if (!plane->modifier_count)
>  		return 0;
>  
> +	if (modifier == DRM_FORMAT_MOD_NONE)
> +		return 0;
> +
>  	for (i = 0; i < plane->modifier_count; i++) {
>  		if (modifier == plane->modifiers[i])
>  			break;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[Ville Syrjälä]

On Fri, May 25, 2018 at 04:48:59PM +0100, Brian Starkey wrote:
> Hi Ayan,
> 
> On Fri, May 25, 2018 at 04:35:41PM +0100, Ayan Kumar Halder wrote:
> >If a plane supports a pixel format and the framebuffer does not pass any
> >modifiers, then drm_plane_check_pixel_format() should always return true
> >for the given format regardless of whether the plane supports any
> >modifiers or not.
> >
> >Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> >---
> > drivers/gpu/drm/drm_plane.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >index 6d2a6e4..873c084 100644
> >--- a/drivers/gpu/drm/drm_plane.c
> >+++ b/drivers/gpu/drm/drm_plane.c
> >@@ -564,6 +564,9 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
> > 	if (!plane->modifier_count)
> > 		return 0;
> >
> >+	if (modifier == DRM_FORMAT_MOD_NONE)
> >+		return 0;
> >+
> 
> There can be drivers which have formats which are only supported when
> a modifier is used - so I don't think this check is right to have in
> the core.
> 
> Drivers need to check if they support the format with no modifier
> (same as DRM_FORMAT_MOD_LINEAR) in their format_mod_supported()
> callback.

BTW I tried to make the core hide the "did the request have modifiers
or not" difference as much as possible, and to validate that there is
at least one plane that can use the created fb:
https://patchwork.freedesktop.org/series/39814/

Not sure we ever figured out a good solution in the end, but at least
Eric's approach in https://patchwork.freedesktop.org/series/40133/
would avoid some of the problems I was having in my attempts. I guess 
I should try to revisit that patchset and see if I can make it less
objectionable this time around.

> 
> Thanks,
> -Brian
> 
> > 	for (i = 0; i < plane->modifier_count; i++) {
> > 		if (modifier == plane->modifiers[i])
> > 			break;
> >-- 
> >2.7.4
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: check_pixel_format() should return true if no modifiers are passed for a supported pixel format

[Ville Syrjälä]

On Fri, May 25, 2018 at 04:48:59PM +0100, Brian Starkey wrote:
> Hi Ayan,
> 
> On Fri, May 25, 2018 at 04:35:41PM +0100, Ayan Kumar Halder wrote:
> >If a plane supports a pixel format and the framebuffer does not pass any
> >modifiers, then drm_plane_check_pixel_format() should always return true
> >for the given format regardless of whether the plane supports any
> >modifiers or not.
> >
> >Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> >---
> > drivers/gpu/drm/drm_plane.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >index 6d2a6e4..873c084 100644
> >--- a/drivers/gpu/drm/drm_plane.c
> >+++ b/drivers/gpu/drm/drm_plane.c
> >@@ -564,6 +564,9 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
> > 	if (!plane->modifier_count)
> > 		return 0;
> >
> >+	if (modifier == DRM_FORMAT_MOD_NONE)
> >+		return 0;
> >+
> 
> There can be drivers which have formats which are only supported when
> a modifier is used - so I don't think this check is right to have in
> the core.
> 
> Drivers need to check if they support the format with no modifier
> (same as DRM_FORMAT_MOD_LINEAR) in their format_mod_supported()
> callback.

BTW I tried to make the core hide the "did the request have modifiers
or not" difference as much as possible, and to validate that there is
at least one plane that can use the created fb:
https://patchwork.freedesktop.org/series/39814/

Not sure we ever figured out a good solution in the end, but at least
Eric's approach in https://patchwork.freedesktop.org/series/40133/
would avoid some of the problems I was having in my attempts. I guess 
I should try to revisit that patchset and see if I can make it less
objectionable this time around.

> 
> Thanks,
> -Brian
> 
> > 	for (i = 0; i < plane->modifier_count; i++) {
> > 		if (modifier == plane->modifiers[i])
> > 			break;
> >-- 
> >2.7.4
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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