[PATCH] drm: Fix page flip ioctl format check

[PATCH] drm: Fix page flip ioctl format check

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Revert back to comparing fb->format->format instead fb->format for the
page flip ioctl. This check was originally only here to disallow pixel
format changes, but when we changed it to do the pointer comparison
we potentially started to reject some (but definitely not all) modifier
changes as well. In fact the current behaviour depends on whether the
driver overrides the format info for a specific format+modifier combo.
Eg. on i915 this now rejects compression vs. no compression changes but
does not reject any other tiling changes. That's just inconsistent
nonsense.

The main reason we have to go back to the old behaviour is to fix page
flipping with Xorg. At some point Xorg got its atomic rights taken away
and since then we can't page flip between compressed and non-compressed
fbs on i915. Currently we get no page flipping for any games pretty much
since Mesa likes to use compressed buffers. Not sure how compositors are
working around this (don't use one myself). I guess they must be doing
something to get non-compressed buffers instead. Either that or
somehow no one noticed the tearing from the blit fallback.

Looking back at the original discussion on this change we pretty much
just did it in the name of skipping a few extra pointer dereferences.
However, I've decided not to revert the whole thing in case someone
has since started to depend on these changes. None of the other checks
are relevant for i915 anyways.

Cc: stable@vger.kernel.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d6ad60ab0d38..f2ca5315f23b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (old_fb->format != fb->format) {
+	if (old_fb->format->format != fb->format->format) {
 		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
 		ret = -EINVAL;
 		goto out;
-- 
2.24.1

[Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Revert back to comparing fb->format->format instead fb->format for the
page flip ioctl. This check was originally only here to disallow pixel
format changes, but when we changed it to do the pointer comparison
we potentially started to reject some (but definitely not all) modifier
changes as well. In fact the current behaviour depends on whether the
driver overrides the format info for a specific format+modifier combo.
Eg. on i915 this now rejects compression vs. no compression changes but
does not reject any other tiling changes. That's just inconsistent
nonsense.

The main reason we have to go back to the old behaviour is to fix page
flipping with Xorg. At some point Xorg got its atomic rights taken away
and since then we can't page flip between compressed and non-compressed
fbs on i915. Currently we get no page flipping for any games pretty much
since Mesa likes to use compressed buffers. Not sure how compositors are
working around this (don't use one myself). I guess they must be doing
something to get non-compressed buffers instead. Either that or
somehow no one noticed the tearing from the blit fallback.

Looking back at the original discussion on this change we pretty much
just did it in the name of skipping a few extra pointer dereferences.
However, I've decided not to revert the whole thing in case someone
has since started to depend on these changes. None of the other checks
are relevant for i915 anyways.

Cc: stable@vger.kernel.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d6ad60ab0d38..f2ca5315f23b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (old_fb->format != fb->format) {
+	if (old_fb->format->format != fb->format->format) {
 		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
 		ret = -EINVAL;
 		goto out;
-- 
2.24.1

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

[PATCH] drm: Fix page flip ioctl format check

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Revert back to comparing fb->format->format instead fb->format for the
page flip ioctl. This check was originally only here to disallow pixel
format changes, but when we changed it to do the pointer comparison
we potentially started to reject some (but definitely not all) modifier
changes as well. In fact the current behaviour depends on whether the
driver overrides the format info for a specific format+modifier combo.
Eg. on i915 this now rejects compression vs. no compression changes but
does not reject any other tiling changes. That's just inconsistent
nonsense.

The main reason we have to go back to the old behaviour is to fix page
flipping with Xorg. At some point Xorg got its atomic rights taken away
and since then we can't page flip between compressed and non-compressed
fbs on i915. Currently we get no page flipping for any games pretty much
since Mesa likes to use compressed buffers. Not sure how compositors are
working around this (don't use one myself). I guess they must be doing
something to get non-compressed buffers instead. Either that or
somehow no one noticed the tearing from the blit fallback.

Looking back at the original discussion on this change we pretty much
just did it in the name of skipping a few extra pointer dereferences.
However, I've decided not to revert the whole thing in case someone
has since started to depend on these changes. None of the other checks
are relevant for i915 anyways.

Cc: stable@vger.kernel.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index d6ad60ab0d38..f2ca5315f23b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (ret)
 		goto out;
 
-	if (old_fb->format != fb->format) {
+	if (old_fb->format->format != fb->format->format) {
 		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
 		ret = -EINVAL;
 		goto out;
-- 
2.24.1

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Laurent Pinchart]

Hi Ville,

Thank you for the patch.

On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel
> format changes, but when we changed it to do the pointer comparison
> we potentially started to reject some (but definitely not all) modifier
> changes as well. In fact the current behaviour depends on whether the
> driver overrides the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent
> nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away
> and since then we can't page flip between compressed and non-compressed
> fbs on i915. Currently we get no page flipping for any games pretty much
> since Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or
> somehow no one noticed the tearing from the blit fallback.
> 
> Looking back at the original discussion on this change we pretty much
> just did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone
> has since started to depend on these changes. None of the other checks
> are relevant for i915 anyways.

Do display controller usually support changing modifiers for page flips
? I understand from the information about that i915 does, but is that
usual ? Could there be drivers that really on this check to reject
modifier changes, and that aren't prepared to handle them if they are
not rejected by the core ? I'm not opposed to this change, but I'd like
to carefully consider the fallout.

> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;

-- 
Regards,

Laurent Pinchart

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Laurent Pinchart]

Hi Ville,

Thank you for the patch.

On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel
> format changes, but when we changed it to do the pointer comparison
> we potentially started to reject some (but definitely not all) modifier
> changes as well. In fact the current behaviour depends on whether the
> driver overrides the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent
> nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away
> and since then we can't page flip between compressed and non-compressed
> fbs on i915. Currently we get no page flipping for any games pretty much
> since Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or
> somehow no one noticed the tearing from the blit fallback.
> 
> Looking back at the original discussion on this change we pretty much
> just did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone
> has since started to depend on these changes. None of the other checks
> are relevant for i915 anyways.

Do display controller usually support changing modifiers for page flips
? I understand from the information about that i915 does, but is that
usual ? Could there be drivers that really on this check to reject
modifier changes, and that aren't prepared to handle them if they are
not rejected by the core ? I'm not opposed to this change, but I'd like
to carefully consider the fallout.

> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;

-- 
Regards,

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Laurent Pinchart]

Hi Ville,

Thank you for the patch.

On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel
> format changes, but when we changed it to do the pointer comparison
> we potentially started to reject some (but definitely not all) modifier
> changes as well. In fact the current behaviour depends on whether the
> driver overrides the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent
> nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away
> and since then we can't page flip between compressed and non-compressed
> fbs on i915. Currently we get no page flipping for any games pretty much
> since Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or
> somehow no one noticed the tearing from the blit fallback.
> 
> Looking back at the original discussion on this change we pretty much
> just did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone
> has since started to depend on these changes. None of the other checks
> are relevant for i915 anyways.

Do display controller usually support changing modifiers for page flips
? I understand from the information about that i915 does, but is that
usual ? Could there be drivers that really on this check to reject
modifier changes, and that aren't prepared to handle them if they are
not rejected by the core ? I'm not opposed to this change, but I'd like
to carefully consider the fallout.

> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;

-- 
Regards,

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Thu, Apr 16, 2020 at 08:08:14PM +0300, Laurent Pinchart wrote:
> Hi Ville,
> 
> Thank you for the patch.
> 
> On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Revert back to comparing fb->format->format instead fb->format for the
> > page flip ioctl. This check was originally only here to disallow pixel
> > format changes, but when we changed it to do the pointer comparison
> > we potentially started to reject some (but definitely not all) modifier
> > changes as well. In fact the current behaviour depends on whether the
> > driver overrides the format info for a specific format+modifier combo.
> > Eg. on i915 this now rejects compression vs. no compression changes but
> > does not reject any other tiling changes. That's just inconsistent
> > nonsense.
> > 
> > The main reason we have to go back to the old behaviour is to fix page
> > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > and since then we can't page flip between compressed and non-compressed
> > fbs on i915. Currently we get no page flipping for any games pretty much
> > since Mesa likes to use compressed buffers. Not sure how compositors are
> > working around this (don't use one myself). I guess they must be doing
> > something to get non-compressed buffers instead. Either that or
> > somehow no one noticed the tearing from the blit fallback.
> > 
> > Looking back at the original discussion on this change we pretty much
> > just did it in the name of skipping a few extra pointer dereferences.
> > However, I've decided not to revert the whole thing in case someone
> > has since started to depend on these changes. None of the other checks
> > are relevant for i915 anyways.
> 
> Do display controller usually support changing modifiers for page flips
> ? I understand from the information about that i915 does, but is that
> usual ? Could there be drivers that really on this check to reject
> modifier changes, and that aren't prepared to handle them if they are
> not rejected by the core ? I'm not opposed to this change, but I'd like
> to carefully consider the fallout.

After a bit of grepping I can't actually see any other driver providing
a .get_format_info() hook. So looks like there is no change in behaviour
for any other driver. Based on that we could even do a full revert, but
meh.

> 
> > Cc: stable@vger.kernel.org
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f2ca5315f23b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (old_fb->format != fb->format) {
> > +	if (old_fb->format->format != fb->format->format) {
> >  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >  		ret = -EINVAL;
> >  		goto out;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Thu, Apr 16, 2020 at 08:08:14PM +0300, Laurent Pinchart wrote:
> Hi Ville,
> 
> Thank you for the patch.
> 
> On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Revert back to comparing fb->format->format instead fb->format for the
> > page flip ioctl. This check was originally only here to disallow pixel
> > format changes, but when we changed it to do the pointer comparison
> > we potentially started to reject some (but definitely not all) modifier
> > changes as well. In fact the current behaviour depends on whether the
> > driver overrides the format info for a specific format+modifier combo.
> > Eg. on i915 this now rejects compression vs. no compression changes but
> > does not reject any other tiling changes. That's just inconsistent
> > nonsense.
> > 
> > The main reason we have to go back to the old behaviour is to fix page
> > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > and since then we can't page flip between compressed and non-compressed
> > fbs on i915. Currently we get no page flipping for any games pretty much
> > since Mesa likes to use compressed buffers. Not sure how compositors are
> > working around this (don't use one myself). I guess they must be doing
> > something to get non-compressed buffers instead. Either that or
> > somehow no one noticed the tearing from the blit fallback.
> > 
> > Looking back at the original discussion on this change we pretty much
> > just did it in the name of skipping a few extra pointer dereferences.
> > However, I've decided not to revert the whole thing in case someone
> > has since started to depend on these changes. None of the other checks
> > are relevant for i915 anyways.
> 
> Do display controller usually support changing modifiers for page flips
> ? I understand from the information about that i915 does, but is that
> usual ? Could there be drivers that really on this check to reject
> modifier changes, and that aren't prepared to handle them if they are
> not rejected by the core ? I'm not opposed to this change, but I'd like
> to carefully consider the fallout.

After a bit of grepping I can't actually see any other driver providing
a .get_format_info() hook. So looks like there is no change in behaviour
for any other driver. Based on that we could even do a full revert, but
meh.

> 
> > Cc: stable@vger.kernel.org
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f2ca5315f23b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (old_fb->format != fb->format) {
> > +	if (old_fb->format->format != fb->format->format) {
> >  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >  		ret = -EINVAL;
> >  		goto out;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Thu, Apr 16, 2020 at 08:08:14PM +0300, Laurent Pinchart wrote:
> Hi Ville,
> 
> Thank you for the patch.
> 
> On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Revert back to comparing fb->format->format instead fb->format for the
> > page flip ioctl. This check was originally only here to disallow pixel
> > format changes, but when we changed it to do the pointer comparison
> > we potentially started to reject some (but definitely not all) modifier
> > changes as well. In fact the current behaviour depends on whether the
> > driver overrides the format info for a specific format+modifier combo.
> > Eg. on i915 this now rejects compression vs. no compression changes but
> > does not reject any other tiling changes. That's just inconsistent
> > nonsense.
> > 
> > The main reason we have to go back to the old behaviour is to fix page
> > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > and since then we can't page flip between compressed and non-compressed
> > fbs on i915. Currently we get no page flipping for any games pretty much
> > since Mesa likes to use compressed buffers. Not sure how compositors are
> > working around this (don't use one myself). I guess they must be doing
> > something to get non-compressed buffers instead. Either that or
> > somehow no one noticed the tearing from the blit fallback.
> > 
> > Looking back at the original discussion on this change we pretty much
> > just did it in the name of skipping a few extra pointer dereferences.
> > However, I've decided not to revert the whole thing in case someone
> > has since started to depend on these changes. None of the other checks
> > are relevant for i915 anyways.
> 
> Do display controller usually support changing modifiers for page flips
> ? I understand from the information about that i915 does, but is that
> usual ? Could there be drivers that really on this check to reject
> modifier changes, and that aren't prepared to handle them if they are
> not rejected by the core ? I'm not opposed to this change, but I'd like
> to carefully consider the fallout.

After a bit of grepping I can't actually see any other driver providing
a .get_format_info() hook. So looks like there is no change in behaviour
for any other driver. Based on that we could even do a full revert, but
meh.

> 
> > Cc: stable@vger.kernel.org
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f2ca5315f23b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (old_fb->format != fb->format) {
> > +	if (old_fb->format->format != fb->format->format) {
> >  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >  		ret = -EINVAL;
> >  		goto out;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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

[Intel-gfx] ✗ Fi.CI.DOCS: warning for drm: Fix page flip ioctl format check

[Patchwork]

== Series Details ==

Series: drm: Fix page flip ioctl format check
URL   : https://patchwork.freedesktop.org/series/76040/
State : warning

== Summary ==

$ make htmldocs 2>&1 > /dev/null | grep i915
/home/cidrm/kernel/Documentation/gpu/i915.rst:610: WARNING: duplicate label gpu/i915:layout, other instance in /home/cidrm/kernel/Documentation/gpu/i915.rst

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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm: Fix page flip ioctl format check

[Patchwork]

== Series Details ==

Series: drm: Fix page flip ioctl format check
URL   : https://patchwork.freedesktop.org/series/76040/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8311 -> Patchwork_17331
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Warnings ####

  * igt@amdgpu/amd_prime@i915-to-amd:
    - fi-bwr-2160:        [FAIL][1] -> [SKIP][2] ([fdo#109271])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/fi-bwr-2160/igt@amdgpu/amd_prime@i915-to-amd.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/fi-bwr-2160/igt@amdgpu/amd_prime@i915-to-amd.html

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-x1275:       [SKIP][3] ([fdo#109271]) -> [FAIL][4] ([i915#62] / [i915#95])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (51 -> 45)
------------------------------

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


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8311 -> Patchwork_17331

  CI-20190529: 20190529
  CI_DRM_8311: 19367bb5e65eaf0719597b3ff244fd1c2ea12bda @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5593: 1c658f5e46598ae93345177d4981ef54704daec6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17331: a4e87db6ffcae3e28a42d5072f14a1a1ce5733df @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a4e87db6ffca drm: Fix page flip ioctl format check

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [PATCH] drm: Fix page flip ioctl format check

[Kadiyala, Kishore]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Thursday, April 16, 2020 10:34 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; stable@vger.kernel.org
> Subject: [PATCH] drm: Fix page flip ioctl format check
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel format
> changes, but when we changed it to do the pointer comparison we
> potentially started to reject some (but definitely not all) modifier changes as
> well. In fact the current behaviour depends on whether the driver overrides
> the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away and
> since then we can't page flip between compressed and non-compressed fbs
> on i915. Currently we get no page flipping for any games pretty much since
> Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or somehow
> no one noticed the tearing from the blit fallback.
> 
> Looking back at the original discussion on this change we pretty much just
> did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone has
> since started to depend on these changes. None of the other checks are
> relevant for i915 anyways.
> 
> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just
> 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've validated this patch on Ubuntu1910 (having X Server v1.20.5 ) with compression enabled .
Feel free to add my Acked-by/ Tested-by: Kishore Kadiyala <kishore.kadiyala@intel.com>

Regards,
Kishore
 
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device
> *dev,
>  	if (ret)
>  		goto out;
> 
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame
> buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> --
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Kadiyala, Kishore]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Thursday, April 16, 2020 10:34 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; stable@vger.kernel.org
> Subject: [PATCH] drm: Fix page flip ioctl format check
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel format
> changes, but when we changed it to do the pointer comparison we
> potentially started to reject some (but definitely not all) modifier changes as
> well. In fact the current behaviour depends on whether the driver overrides
> the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away and
> since then we can't page flip between compressed and non-compressed fbs
> on i915. Currently we get no page flipping for any games pretty much since
> Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or somehow
> no one noticed the tearing from the blit fallback.
> 
> Looking back at the original discussion on this change we pretty much just
> did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone has
> since started to depend on these changes. None of the other checks are
> relevant for i915 anyways.
> 
> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just
> 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've validated this patch on Ubuntu1910 (having X Server v1.20.5 ) with compression enabled .
Feel free to add my Acked-by/ Tested-by: Kishore Kadiyala <kishore.kadiyala@intel.com>

Regards,
Kishore
 
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device
> *dev,
>  	if (ret)
>  		goto out;
> 
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame
> buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> --
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [PATCH] drm: Fix page flip ioctl format check

[Kadiyala, Kishore]

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Thursday, April 16, 2020 10:34 PM
> To: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; stable@vger.kernel.org
> Subject: [PATCH] drm: Fix page flip ioctl format check
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel format
> changes, but when we changed it to do the pointer comparison we
> potentially started to reject some (but definitely not all) modifier changes as
> well. In fact the current behaviour depends on whether the driver overrides
> the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away and
> since then we can't page flip between compressed and non-compressed fbs
> on i915. Currently we get no page flipping for any games pretty much since
> Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or somehow
> no one noticed the tearing from the blit fallback.
> 
> Looking back at the original discussion on this change we pretty much just
> did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone has
> since started to depend on these changes. None of the other checks are
> relevant for i915 anyways.
> 
> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just
> 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've validated this patch on Ubuntu1910 (having X Server v1.20.5 ) with compression enabled .
Feel free to add my Acked-by/ Tested-by: Kishore Kadiyala <kishore.kadiyala@intel.com>

Regards,
Kishore
 
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device
> *dev,
>  	if (ret)
>  		goto out;
> 
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame
> buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> --
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel
> format changes, but when we changed it to do the pointer comparison
> we potentially started to reject some (but definitely not all) modifier
> changes as well. In fact the current behaviour depends on whether the
> driver overrides the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent
> nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away
> and since then we can't page flip between compressed and non-compressed
> fbs on i915. Currently we get no page flipping for any games pretty much
> since Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or
> somehow no one noticed the tearing from the blit fallback.

Mesa only uses compressed buffers if you enable modifiers, and there's a
_loooooooooooot_ more that needs to be fixed in Xorg to enable that for
real. Like real atomic support. Without modifiers all you get is X tiling,
and that works just fine.

Which would also fix this issue here you're papering over.

So if this is the entire reason for this, I'm inclined to not do this.
Current Xorg is toast wrt modifiers, that's not news.
-Daniel

> 
> Looking back at the original discussion on this change we pretty much
> just did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone
> has since started to depend on these changes. None of the other checks
> are relevant for i915 anyways.
> 
> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel
> format changes, but when we changed it to do the pointer comparison
> we potentially started to reject some (but definitely not all) modifier
> changes as well. In fact the current behaviour depends on whether the
> driver overrides the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent
> nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away
> and since then we can't page flip between compressed and non-compressed
> fbs on i915. Currently we get no page flipping for any games pretty much
> since Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or
> somehow no one noticed the tearing from the blit fallback.

Mesa only uses compressed buffers if you enable modifiers, and there's a
_loooooooooooot_ more that needs to be fixed in Xorg to enable that for
real. Like real atomic support. Without modifiers all you get is X tiling,
and that works just fine.

Which would also fix this issue here you're papering over.

So if this is the entire reason for this, I'm inclined to not do this.
Current Xorg is toast wrt modifiers, that's not news.
-Daniel

> 
> Looking back at the original discussion on this change we pretty much
> just did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone
> has since started to depend on these changes. None of the other checks
> are relevant for i915 anyways.
> 
> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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: Fix page flip ioctl format check

[Daniel Vetter]

On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Revert back to comparing fb->format->format instead fb->format for the
> page flip ioctl. This check was originally only here to disallow pixel
> format changes, but when we changed it to do the pointer comparison
> we potentially started to reject some (but definitely not all) modifier
> changes as well. In fact the current behaviour depends on whether the
> driver overrides the format info for a specific format+modifier combo.
> Eg. on i915 this now rejects compression vs. no compression changes but
> does not reject any other tiling changes. That's just inconsistent
> nonsense.
> 
> The main reason we have to go back to the old behaviour is to fix page
> flipping with Xorg. At some point Xorg got its atomic rights taken away
> and since then we can't page flip between compressed and non-compressed
> fbs on i915. Currently we get no page flipping for any games pretty much
> since Mesa likes to use compressed buffers. Not sure how compositors are
> working around this (don't use one myself). I guess they must be doing
> something to get non-compressed buffers instead. Either that or
> somehow no one noticed the tearing from the blit fallback.

Mesa only uses compressed buffers if you enable modifiers, and there's a
_loooooooooooot_ more that needs to be fixed in Xorg to enable that for
real. Like real atomic support. Without modifiers all you get is X tiling,
and that works just fine.

Which would also fix this issue here you're papering over.

So if this is the entire reason for this, I'm inclined to not do this.
Current Xorg is toast wrt modifiers, that's not news.
-Daniel

> 
> Looking back at the original discussion on this change we pretty much
> just did it in the name of skipping a few extra pointer dereferences.
> However, I've decided not to revert the whole thing in case someone
> has since started to depend on these changes. None of the other checks
> are relevant for i915 anyways.
> 
> Cc: stable@vger.kernel.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index d6ad60ab0d38..f2ca5315f23b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (ret)
>  		goto out;
>  
> -	if (old_fb->format != fb->format) {
> +	if (old_fb->format->format != fb->format->format) {
>  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
>  		ret = -EINVAL;
>  		goto out;
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Revert back to comparing fb->format->format instead fb->format for the
> > page flip ioctl. This check was originally only here to disallow pixel
> > format changes, but when we changed it to do the pointer comparison
> > we potentially started to reject some (but definitely not all) modifier
> > changes as well. In fact the current behaviour depends on whether the
> > driver overrides the format info for a specific format+modifier combo.
> > Eg. on i915 this now rejects compression vs. no compression changes but
> > does not reject any other tiling changes. That's just inconsistent
> > nonsense.
> > 
> > The main reason we have to go back to the old behaviour is to fix page
> > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > and since then we can't page flip between compressed and non-compressed
> > fbs on i915. Currently we get no page flipping for any games pretty much
> > since Mesa likes to use compressed buffers. Not sure how compositors are
> > working around this (don't use one myself). I guess they must be doing
> > something to get non-compressed buffers instead. Either that or
> > somehow no one noticed the tearing from the blit fallback.
> 
> Mesa only uses compressed buffers if you enable modifiers, and there's a
> _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> real. Like real atomic support.

Why would you need atomic for modifiers? Xorg doesn't even have
any sensible framework for atomic and I suspect it never will.

> Without modifiers all you get is X tiling,
> and that works just fine.
> 
> Which would also fix this issue here you're papering over.
> 
> So if this is the entire reason for this, I'm inclined to not do this.
> Current Xorg is toast wrt modifiers, that's not news.

Works just fine. Also pretty sure modifiers are even enabled by
default now in modesetting.

And as stated the current check doesn't have consistent behaviour
anyway. You can still flip between different modifiers as long a the
driver doesn't override .get_format_info() for one of them. The *only*
case where that happens is CCS on i915. There is no valid reason to
special case that one.

> -Daniel
> 
> > 
> > Looking back at the original discussion on this change we pretty much
> > just did it in the name of skipping a few extra pointer dereferences.
> > However, I've decided not to revert the whole thing in case someone
> > has since started to depend on these changes. None of the other checks
> > are relevant for i915 anyways.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f2ca5315f23b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (old_fb->format != fb->format) {
> > +	if (old_fb->format->format != fb->format->format) {
> >  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >  		ret = -EINVAL;
> >  		goto out;
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Revert back to comparing fb->format->format instead fb->format for the
> > page flip ioctl. This check was originally only here to disallow pixel
> > format changes, but when we changed it to do the pointer comparison
> > we potentially started to reject some (but definitely not all) modifier
> > changes as well. In fact the current behaviour depends on whether the
> > driver overrides the format info for a specific format+modifier combo.
> > Eg. on i915 this now rejects compression vs. no compression changes but
> > does not reject any other tiling changes. That's just inconsistent
> > nonsense.
> > 
> > The main reason we have to go back to the old behaviour is to fix page
> > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > and since then we can't page flip between compressed and non-compressed
> > fbs on i915. Currently we get no page flipping for any games pretty much
> > since Mesa likes to use compressed buffers. Not sure how compositors are
> > working around this (don't use one myself). I guess they must be doing
> > something to get non-compressed buffers instead. Either that or
> > somehow no one noticed the tearing from the blit fallback.
> 
> Mesa only uses compressed buffers if you enable modifiers, and there's a
> _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> real. Like real atomic support.

Why would you need atomic for modifiers? Xorg doesn't even have
any sensible framework for atomic and I suspect it never will.

> Without modifiers all you get is X tiling,
> and that works just fine.
> 
> Which would also fix this issue here you're papering over.
> 
> So if this is the entire reason for this, I'm inclined to not do this.
> Current Xorg is toast wrt modifiers, that's not news.

Works just fine. Also pretty sure modifiers are even enabled by
default now in modesetting.

And as stated the current check doesn't have consistent behaviour
anyway. You can still flip between different modifiers as long a the
driver doesn't override .get_format_info() for one of them. The *only*
case where that happens is CCS on i915. There is no valid reason to
special case that one.

> -Daniel
> 
> > 
> > Looking back at the original discussion on this change we pretty much
> > just did it in the name of skipping a few extra pointer dereferences.
> > However, I've decided not to revert the whole thing in case someone
> > has since started to depend on these changes. None of the other checks
> > are relevant for i915 anyways.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f2ca5315f23b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (old_fb->format != fb->format) {
> > +	if (old_fb->format->format != fb->format->format) {
> >  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >  		ret = -EINVAL;
> >  		goto out;
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Revert back to comparing fb->format->format instead fb->format for the
> > page flip ioctl. This check was originally only here to disallow pixel
> > format changes, but when we changed it to do the pointer comparison
> > we potentially started to reject some (but definitely not all) modifier
> > changes as well. In fact the current behaviour depends on whether the
> > driver overrides the format info for a specific format+modifier combo.
> > Eg. on i915 this now rejects compression vs. no compression changes but
> > does not reject any other tiling changes. That's just inconsistent
> > nonsense.
> > 
> > The main reason we have to go back to the old behaviour is to fix page
> > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > and since then we can't page flip between compressed and non-compressed
> > fbs on i915. Currently we get no page flipping for any games pretty much
> > since Mesa likes to use compressed buffers. Not sure how compositors are
> > working around this (don't use one myself). I guess they must be doing
> > something to get non-compressed buffers instead. Either that or
> > somehow no one noticed the tearing from the blit fallback.
> 
> Mesa only uses compressed buffers if you enable modifiers, and there's a
> _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> real. Like real atomic support.

Why would you need atomic for modifiers? Xorg doesn't even have
any sensible framework for atomic and I suspect it never will.

> Without modifiers all you get is X tiling,
> and that works just fine.
> 
> Which would also fix this issue here you're papering over.
> 
> So if this is the entire reason for this, I'm inclined to not do this.
> Current Xorg is toast wrt modifiers, that's not news.

Works just fine. Also pretty sure modifiers are even enabled by
default now in modesetting.

And as stated the current check doesn't have consistent behaviour
anyway. You can still flip between different modifiers as long a the
driver doesn't override .get_format_info() for one of them. The *only*
case where that happens is CCS on i915. There is no valid reason to
special case that one.

> -Daniel
> 
> > 
> > Looking back at the original discussion on this change we pretty much
> > just did it in the name of skipping a few extra pointer dereferences.
> > However, I've decided not to revert the whole thing in case someone
> > has since started to depend on these changes. None of the other checks
> > are relevant for i915 anyways.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index d6ad60ab0d38..f2ca5315f23b 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (ret)
> >  		goto out;
> >  
> > -	if (old_fb->format != fb->format) {
> > +	if (old_fb->format->format != fb->format->format) {
> >  		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> >  		ret = -EINVAL;
> >  		goto out;
> > -- 
> > 2.24.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Revert back to comparing fb->format->format instead fb->format for the
> > > page flip ioctl. This check was originally only here to disallow pixel
> > > format changes, but when we changed it to do the pointer comparison
> > > we potentially started to reject some (but definitely not all) modifier
> > > changes as well. In fact the current behaviour depends on whether the
> > > driver overrides the format info for a specific format+modifier combo.
> > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > does not reject any other tiling changes. That's just inconsistent
> > > nonsense.
> > >
> > > The main reason we have to go back to the old behaviour is to fix page
> > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > and since then we can't page flip between compressed and non-compressed
> > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > working around this (don't use one myself). I guess they must be doing
> > > something to get non-compressed buffers instead. Either that or
> > > somehow no one noticed the tearing from the blit fallback.
> >
> > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > real. Like real atomic support.
>
> Why would you need atomic for modifiers? Xorg doesn't even have
> any sensible framework for atomic and I suspect it never will.

Frankly if no one cares about atomic in X I don't think we should do
work-arounds for lack of atomic in X.

> > Without modifiers all you get is X tiling,
> > and that works just fine.
> >
> > Which would also fix this issue here you're papering over.
> >
> > So if this is the entire reason for this, I'm inclined to not do this.
> > Current Xorg is toast wrt modifiers, that's not news.
>
> Works just fine. Also pretty sure modifiers are even enabled by
> default now in modesetting.

Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
it works. Otherwise good chances for some oddball black screens on
configurations that worked before. Which is why all non-atomic
compositors reverted modifiers by default again.

> And as stated the current check doesn't have consistent behaviour
> anyway. You can still flip between different modifiers as long a the
> driver doesn't override .get_format_info() for one of them. The *only*
> case where that happens is CCS on i915. There is no valid reason to
> special case that one.

The thing is, you need atomic to make CCS work reliably enough for
compositors and distros to dare enabling it by default. CCS flipping
works with atomic. I really see no point in baking this in with as
uapi. Just fix Xorg.
-Daniel

>
> > -Daniel
> >
> > >
> > > Looking back at the original discussion on this change we pretty much
> > > just did it in the name of skipping a few extra pointer dereferences.
> > > However, I've decided not to revert the whole thing in case someone
> > > has since started to depend on these changes. None of the other checks
> > > are relevant for i915 anyways.
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >     if (ret)
> > >             goto out;
> > >
> > > -   if (old_fb->format != fb->format) {
> > > +   if (old_fb->format->format != fb->format->format) {
> > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > >             ret = -EINVAL;
> > >             goto out;
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Revert back to comparing fb->format->format instead fb->format for the
> > > page flip ioctl. This check was originally only here to disallow pixel
> > > format changes, but when we changed it to do the pointer comparison
> > > we potentially started to reject some (but definitely not all) modifier
> > > changes as well. In fact the current behaviour depends on whether the
> > > driver overrides the format info for a specific format+modifier combo.
> > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > does not reject any other tiling changes. That's just inconsistent
> > > nonsense.
> > >
> > > The main reason we have to go back to the old behaviour is to fix page
> > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > and since then we can't page flip between compressed and non-compressed
> > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > working around this (don't use one myself). I guess they must be doing
> > > something to get non-compressed buffers instead. Either that or
> > > somehow no one noticed the tearing from the blit fallback.
> >
> > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > real. Like real atomic support.
>
> Why would you need atomic for modifiers? Xorg doesn't even have
> any sensible framework for atomic and I suspect it never will.

Frankly if no one cares about atomic in X I don't think we should do
work-arounds for lack of atomic in X.

> > Without modifiers all you get is X tiling,
> > and that works just fine.
> >
> > Which would also fix this issue here you're papering over.
> >
> > So if this is the entire reason for this, I'm inclined to not do this.
> > Current Xorg is toast wrt modifiers, that's not news.
>
> Works just fine. Also pretty sure modifiers are even enabled by
> default now in modesetting.

Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
it works. Otherwise good chances for some oddball black screens on
configurations that worked before. Which is why all non-atomic
compositors reverted modifiers by default again.

> And as stated the current check doesn't have consistent behaviour
> anyway. You can still flip between different modifiers as long a the
> driver doesn't override .get_format_info() for one of them. The *only*
> case where that happens is CCS on i915. There is no valid reason to
> special case that one.

The thing is, you need atomic to make CCS work reliably enough for
compositors and distros to dare enabling it by default. CCS flipping
works with atomic. I really see no point in baking this in with as
uapi. Just fix Xorg.
-Daniel

>
> > -Daniel
> >
> > >
> > > Looking back at the original discussion on this change we pretty much
> > > just did it in the name of skipping a few extra pointer dereferences.
> > > However, I've decided not to revert the whole thing in case someone
> > > has since started to depend on these changes. None of the other checks
> > > are relevant for i915 anyways.
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >     if (ret)
> > >             goto out;
> > >
> > > -   if (old_fb->format != fb->format) {
> > > +   if (old_fb->format->format != fb->format->format) {
> > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > >             ret = -EINVAL;
> > >             goto out;
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Revert back to comparing fb->format->format instead fb->format for the
> > > page flip ioctl. This check was originally only here to disallow pixel
> > > format changes, but when we changed it to do the pointer comparison
> > > we potentially started to reject some (but definitely not all) modifier
> > > changes as well. In fact the current behaviour depends on whether the
> > > driver overrides the format info for a specific format+modifier combo.
> > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > does not reject any other tiling changes. That's just inconsistent
> > > nonsense.
> > >
> > > The main reason we have to go back to the old behaviour is to fix page
> > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > and since then we can't page flip between compressed and non-compressed
> > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > working around this (don't use one myself). I guess they must be doing
> > > something to get non-compressed buffers instead. Either that or
> > > somehow no one noticed the tearing from the blit fallback.
> >
> > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > real. Like real atomic support.
>
> Why would you need atomic for modifiers? Xorg doesn't even have
> any sensible framework for atomic and I suspect it never will.

Frankly if no one cares about atomic in X I don't think we should do
work-arounds for lack of atomic in X.

> > Without modifiers all you get is X tiling,
> > and that works just fine.
> >
> > Which would also fix this issue here you're papering over.
> >
> > So if this is the entire reason for this, I'm inclined to not do this.
> > Current Xorg is toast wrt modifiers, that's not news.
>
> Works just fine. Also pretty sure modifiers are even enabled by
> default now in modesetting.

Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
it works. Otherwise good chances for some oddball black screens on
configurations that worked before. Which is why all non-atomic
compositors reverted modifiers by default again.

> And as stated the current check doesn't have consistent behaviour
> anyway. You can still flip between different modifiers as long a the
> driver doesn't override .get_format_info() for one of them. The *only*
> case where that happens is CCS on i915. There is no valid reason to
> special case that one.

The thing is, you need atomic to make CCS work reliably enough for
compositors and distros to dare enabling it by default. CCS flipping
works with atomic. I really see no point in baking this in with as
uapi. Just fix Xorg.
-Daniel

>
> > -Daniel
> >
> > >
> > > Looking back at the original discussion on this change we pretty much
> > > just did it in the name of skipping a few extra pointer dereferences.
> > > However, I've decided not to revert the whole thing in case someone
> > > has since started to depend on these changes. None of the other checks
> > > are relevant for i915 anyways.
> > >
> > > Cc: stable@vger.kernel.org
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >     if (ret)
> > >             goto out;
> > >
> > > -   if (old_fb->format != fb->format) {
> > > +   if (old_fb->format->format != fb->format->format) {
> > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > >             ret = -EINVAL;
> > >             goto out;
> > > --
> > > 2.24.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > format changes, but when we changed it to do the pointer comparison
> > > > we potentially started to reject some (but definitely not all) modifier
> > > > changes as well. In fact the current behaviour depends on whether the
> > > > driver overrides the format info for a specific format+modifier combo.
> > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > does not reject any other tiling changes. That's just inconsistent
> > > > nonsense.
> > > >
> > > > The main reason we have to go back to the old behaviour is to fix page
> > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > and since then we can't page flip between compressed and non-compressed
> > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > working around this (don't use one myself). I guess they must be doing
> > > > something to get non-compressed buffers instead. Either that or
> > > > somehow no one noticed the tearing from the blit fallback.
> > >
> > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > real. Like real atomic support.
> >
> > Why would you need atomic for modifiers? Xorg doesn't even have
> > any sensible framework for atomic and I suspect it never will.
> 
> Frankly if no one cares about atomic in X I don't think we should do
> work-arounds for lack of atomic in X.
> 
> > > Without modifiers all you get is X tiling,
> > > and that works just fine.
> > >
> > > Which would also fix this issue here you're papering over.
> > >
> > > So if this is the entire reason for this, I'm inclined to not do this.
> > > Current Xorg is toast wrt modifiers, that's not news.
> >
> > Works just fine. Also pretty sure modifiers are even enabled by
> > default now in modesetting.
> 
> Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> it works. Otherwise good chances for some oddball black screens on
> configurations that worked before. Which is why all non-atomic
> compositors reverted modifiers by default again.

Y alone is hard to scanout also, and yet we do nothing to reject that.
It's just an inconsistent mess.

If we really want to keep this check then we should rewrite it
to be explicit:

if (old_fb->format->format != new_fb->format->format ||
    is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
    return -EINVAL;

Now it's just a random thing that may even stop doing what it's
currently doing if anyone touches their .get_format_info()
implementation.

> 
> > And as stated the current check doesn't have consistent behaviour
> > anyway. You can still flip between different modifiers as long a the
> > driver doesn't override .get_format_info() for one of them. The *only*
> > case where that happens is CCS on i915. There is no valid reason to
> > special case that one.
> 
> The thing is, you need atomic to make CCS work reliably enough for
> compositors and distros to dare enabling it by default.

If it's not enabled by default then there is no harm in letting people
explicitly enable it and get better performance.

> CCS flipping
> works with atomic. I really see no point in baking this in with as
> uapi.

It's just going back to the original intention of the check.
Heck, the debug message doesn't even match what it's doing now.

> Just fix Xorg.

Be serious. No one is going to rewrite all the randr code to be atomic.

> -Daniel
> 
> >
> > > -Daniel
> > >
> > > >
> > > > Looking back at the original discussion on this change we pretty much
> > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > However, I've decided not to revert the whole thing in case someone
> > > > has since started to depend on these changes. None of the other checks
> > > > are relevant for i915 anyways.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > >     if (ret)
> > > >             goto out;
> > > >
> > > > -   if (old_fb->format != fb->format) {
> > > > +   if (old_fb->format->format != fb->format->format) {
> > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > >             ret = -EINVAL;
> > > >             goto out;
> > > > --
> > > > 2.24.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > format changes, but when we changed it to do the pointer comparison
> > > > we potentially started to reject some (but definitely not all) modifier
> > > > changes as well. In fact the current behaviour depends on whether the
> > > > driver overrides the format info for a specific format+modifier combo.
> > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > does not reject any other tiling changes. That's just inconsistent
> > > > nonsense.
> > > >
> > > > The main reason we have to go back to the old behaviour is to fix page
> > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > and since then we can't page flip between compressed and non-compressed
> > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > working around this (don't use one myself). I guess they must be doing
> > > > something to get non-compressed buffers instead. Either that or
> > > > somehow no one noticed the tearing from the blit fallback.
> > >
> > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > real. Like real atomic support.
> >
> > Why would you need atomic for modifiers? Xorg doesn't even have
> > any sensible framework for atomic and I suspect it never will.
> 
> Frankly if no one cares about atomic in X I don't think we should do
> work-arounds for lack of atomic in X.
> 
> > > Without modifiers all you get is X tiling,
> > > and that works just fine.
> > >
> > > Which would also fix this issue here you're papering over.
> > >
> > > So if this is the entire reason for this, I'm inclined to not do this.
> > > Current Xorg is toast wrt modifiers, that's not news.
> >
> > Works just fine. Also pretty sure modifiers are even enabled by
> > default now in modesetting.
> 
> Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> it works. Otherwise good chances for some oddball black screens on
> configurations that worked before. Which is why all non-atomic
> compositors reverted modifiers by default again.

Y alone is hard to scanout also, and yet we do nothing to reject that.
It's just an inconsistent mess.

If we really want to keep this check then we should rewrite it
to be explicit:

if (old_fb->format->format != new_fb->format->format ||
    is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
    return -EINVAL;

Now it's just a random thing that may even stop doing what it's
currently doing if anyone touches their .get_format_info()
implementation.

> 
> > And as stated the current check doesn't have consistent behaviour
> > anyway. You can still flip between different modifiers as long a the
> > driver doesn't override .get_format_info() for one of them. The *only*
> > case where that happens is CCS on i915. There is no valid reason to
> > special case that one.
> 
> The thing is, you need atomic to make CCS work reliably enough for
> compositors and distros to dare enabling it by default.

If it's not enabled by default then there is no harm in letting people
explicitly enable it and get better performance.

> CCS flipping
> works with atomic. I really see no point in baking this in with as
> uapi.

It's just going back to the original intention of the check.
Heck, the debug message doesn't even match what it's doing now.

> Just fix Xorg.

Be serious. No one is going to rewrite all the randr code to be atomic.

> -Daniel
> 
> >
> > > -Daniel
> > >
> > > >
> > > > Looking back at the original discussion on this change we pretty much
> > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > However, I've decided not to revert the whole thing in case someone
> > > > has since started to depend on these changes. None of the other checks
> > > > are relevant for i915 anyways.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > >     if (ret)
> > > >             goto out;
> > > >
> > > > -   if (old_fb->format != fb->format) {
> > > > +   if (old_fb->format->format != fb->format->format) {
> > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > >             ret = -EINVAL;
> > > >             goto out;
> > > > --
> > > > 2.24.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > format changes, but when we changed it to do the pointer comparison
> > > > we potentially started to reject some (but definitely not all) modifier
> > > > changes as well. In fact the current behaviour depends on whether the
> > > > driver overrides the format info for a specific format+modifier combo.
> > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > does not reject any other tiling changes. That's just inconsistent
> > > > nonsense.
> > > >
> > > > The main reason we have to go back to the old behaviour is to fix page
> > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > and since then we can't page flip between compressed and non-compressed
> > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > working around this (don't use one myself). I guess they must be doing
> > > > something to get non-compressed buffers instead. Either that or
> > > > somehow no one noticed the tearing from the blit fallback.
> > >
> > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > real. Like real atomic support.
> >
> > Why would you need atomic for modifiers? Xorg doesn't even have
> > any sensible framework for atomic and I suspect it never will.
> 
> Frankly if no one cares about atomic in X I don't think we should do
> work-arounds for lack of atomic in X.
> 
> > > Without modifiers all you get is X tiling,
> > > and that works just fine.
> > >
> > > Which would also fix this issue here you're papering over.
> > >
> > > So if this is the entire reason for this, I'm inclined to not do this.
> > > Current Xorg is toast wrt modifiers, that's not news.
> >
> > Works just fine. Also pretty sure modifiers are even enabled by
> > default now in modesetting.
> 
> Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> it works. Otherwise good chances for some oddball black screens on
> configurations that worked before. Which is why all non-atomic
> compositors reverted modifiers by default again.

Y alone is hard to scanout also, and yet we do nothing to reject that.
It's just an inconsistent mess.

If we really want to keep this check then we should rewrite it
to be explicit:

if (old_fb->format->format != new_fb->format->format ||
    is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
    return -EINVAL;

Now it's just a random thing that may even stop doing what it's
currently doing if anyone touches their .get_format_info()
implementation.

> 
> > And as stated the current check doesn't have consistent behaviour
> > anyway. You can still flip between different modifiers as long a the
> > driver doesn't override .get_format_info() for one of them. The *only*
> > case where that happens is CCS on i915. There is no valid reason to
> > special case that one.
> 
> The thing is, you need atomic to make CCS work reliably enough for
> compositors and distros to dare enabling it by default.

If it's not enabled by default then there is no harm in letting people
explicitly enable it and get better performance.

> CCS flipping
> works with atomic. I really see no point in baking this in with as
> uapi.

It's just going back to the original intention of the check.
Heck, the debug message doesn't even match what it's doing now.

> Just fix Xorg.

Be serious. No one is going to rewrite all the randr code to be atomic.

> -Daniel
> 
> >
> > > -Daniel
> > >
> > > >
> > > > Looking back at the original discussion on this change we pretty much
> > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > However, I've decided not to revert the whole thing in case someone
> > > > has since started to depend on these changes. None of the other checks
> > > > are relevant for i915 anyways.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > >     if (ret)
> > > >             goto out;
> > > >
> > > > -   if (old_fb->format != fb->format) {
> > > > +   if (old_fb->format->format != fb->format->format) {
> > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > >             ret = -EINVAL;
> > > >             goto out;
> > > > --
> > > > 2.24.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Rodrigo Vivi]

On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > format changes, but when we changed it to do the pointer comparison
> > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > nonsense.
> > > > >
> > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > and since then we can't page flip between compressed and non-compressed
> > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > working around this (don't use one myself). I guess they must be doing
> > > > > something to get non-compressed buffers instead. Either that or
> > > > > somehow no one noticed the tearing from the blit fallback.
> > > >
> > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > real. Like real atomic support.
> > >
> > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > any sensible framework for atomic and I suspect it never will.
> > 
> > Frankly if no one cares about atomic in X I don't think we should do
> > work-arounds for lack of atomic in X.
> > 
> > > > Without modifiers all you get is X tiling,
> > > > and that works just fine.
> > > >
> > > > Which would also fix this issue here you're papering over.
> > > >
> > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > Current Xorg is toast wrt modifiers, that's not news.
> > >
> > > Works just fine. Also pretty sure modifiers are even enabled by
> > > default now in modesetting.
> > 
> > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > it works. Otherwise good chances for some oddball black screens on
> > configurations that worked before. Which is why all non-atomic
> > compositors reverted modifiers by default again.
> 
> Y alone is hard to scanout also, and yet we do nothing to reject that.
> It's just an inconsistent mess.
> 
> If we really want to keep this check then we should rewrite it
> to be explicit:
> 
> if (old_fb->format->format != new_fb->format->format ||
>     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
>     return -EINVAL;
> 
> Now it's just a random thing that may even stop doing what it's
> currently doing if anyone touches their .get_format_info()
> implementation.
> 
> > 
> > > And as stated the current check doesn't have consistent behaviour
> > > anyway. You can still flip between different modifiers as long a the
> > > driver doesn't override .get_format_info() for one of them. The *only*
> > > case where that happens is CCS on i915. There is no valid reason to
> > > special case that one.
> > 
> > The thing is, you need atomic to make CCS work reliably enough for
> > compositors and distros to dare enabling it by default.
> 
> If it's not enabled by default then there is no harm in letting people
> explicitly enable it and get better performance.
> 
> > CCS flipping
> > works with atomic. I really see no point in baking this in with as
> > uapi.
> 
> It's just going back to the original intention of the check.
> Heck, the debug message doesn't even match what it's doing now.
> 
> > Just fix Xorg.
> 
> Be serious. No one is going to rewrite all the randr code to be atomic.

I fully understand Daniel's concern here, but I also believe this won't be
done so soon at least. Meanwhile would it be acceptable to have a comment
with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
?

> 
> > -Daniel
> > 
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > Looking back at the original discussion on this change we pretty much
> > > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > > However, I've decided not to revert the whole thing in case someone
> > > > > has since started to depend on these changes. None of the other checks
> > > > > are relevant for i915 anyways.
> > > > >
> > > > > Cc: stable@vger.kernel.org
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > >     if (ret)
> > > > >             goto out;
> > > > >
> > > > > -   if (old_fb->format != fb->format) {
> > > > > +   if (old_fb->format->format != fb->format->format) {
> > > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > >             ret = -EINVAL;
> > > > >             goto out;
> > > > > --
> > > > > 2.24.1
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> 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: Fix page flip ioctl format check

[Rodrigo Vivi]

On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > format changes, but when we changed it to do the pointer comparison
> > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > nonsense.
> > > > >
> > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > and since then we can't page flip between compressed and non-compressed
> > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > working around this (don't use one myself). I guess they must be doing
> > > > > something to get non-compressed buffers instead. Either that or
> > > > > somehow no one noticed the tearing from the blit fallback.
> > > >
> > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > real. Like real atomic support.
> > >
> > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > any sensible framework for atomic and I suspect it never will.
> > 
> > Frankly if no one cares about atomic in X I don't think we should do
> > work-arounds for lack of atomic in X.
> > 
> > > > Without modifiers all you get is X tiling,
> > > > and that works just fine.
> > > >
> > > > Which would also fix this issue here you're papering over.
> > > >
> > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > Current Xorg is toast wrt modifiers, that's not news.
> > >
> > > Works just fine. Also pretty sure modifiers are even enabled by
> > > default now in modesetting.
> > 
> > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > it works. Otherwise good chances for some oddball black screens on
> > configurations that worked before. Which is why all non-atomic
> > compositors reverted modifiers by default again.
> 
> Y alone is hard to scanout also, and yet we do nothing to reject that.
> It's just an inconsistent mess.
> 
> If we really want to keep this check then we should rewrite it
> to be explicit:
> 
> if (old_fb->format->format != new_fb->format->format ||
>     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
>     return -EINVAL;
> 
> Now it's just a random thing that may even stop doing what it's
> currently doing if anyone touches their .get_format_info()
> implementation.
> 
> > 
> > > And as stated the current check doesn't have consistent behaviour
> > > anyway. You can still flip between different modifiers as long a the
> > > driver doesn't override .get_format_info() for one of them. The *only*
> > > case where that happens is CCS on i915. There is no valid reason to
> > > special case that one.
> > 
> > The thing is, you need atomic to make CCS work reliably enough for
> > compositors and distros to dare enabling it by default.
> 
> If it's not enabled by default then there is no harm in letting people
> explicitly enable it and get better performance.
> 
> > CCS flipping
> > works with atomic. I really see no point in baking this in with as
> > uapi.
> 
> It's just going back to the original intention of the check.
> Heck, the debug message doesn't even match what it's doing now.
> 
> > Just fix Xorg.
> 
> Be serious. No one is going to rewrite all the randr code to be atomic.

I fully understand Daniel's concern here, but I also believe this won't be
done so soon at least. Meanwhile would it be acceptable to have a comment
with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
?

> 
> > -Daniel
> > 
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > Looking back at the original discussion on this change we pretty much
> > > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > > However, I've decided not to revert the whole thing in case someone
> > > > > has since started to depend on these changes. None of the other checks
> > > > > are relevant for i915 anyways.
> > > > >
> > > > > Cc: stable@vger.kernel.org
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > >     if (ret)
> > > > >             goto out;
> > > > >
> > > > > -   if (old_fb->format != fb->format) {
> > > > > +   if (old_fb->format->format != fb->format->format) {
> > > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > >             ret = -EINVAL;
> > > > >             goto out;
> > > > > --
> > > > > 2.24.1
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Fix page flip ioctl format check

[Rodrigo Vivi]

On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > format changes, but when we changed it to do the pointer comparison
> > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > nonsense.
> > > > >
> > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > and since then we can't page flip between compressed and non-compressed
> > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > working around this (don't use one myself). I guess they must be doing
> > > > > something to get non-compressed buffers instead. Either that or
> > > > > somehow no one noticed the tearing from the blit fallback.
> > > >
> > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > real. Like real atomic support.
> > >
> > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > any sensible framework for atomic and I suspect it never will.
> > 
> > Frankly if no one cares about atomic in X I don't think we should do
> > work-arounds for lack of atomic in X.
> > 
> > > > Without modifiers all you get is X tiling,
> > > > and that works just fine.
> > > >
> > > > Which would also fix this issue here you're papering over.
> > > >
> > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > Current Xorg is toast wrt modifiers, that's not news.
> > >
> > > Works just fine. Also pretty sure modifiers are even enabled by
> > > default now in modesetting.
> > 
> > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > it works. Otherwise good chances for some oddball black screens on
> > configurations that worked before. Which is why all non-atomic
> > compositors reverted modifiers by default again.
> 
> Y alone is hard to scanout also, and yet we do nothing to reject that.
> It's just an inconsistent mess.
> 
> If we really want to keep this check then we should rewrite it
> to be explicit:
> 
> if (old_fb->format->format != new_fb->format->format ||
>     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
>     return -EINVAL;
> 
> Now it's just a random thing that may even stop doing what it's
> currently doing if anyone touches their .get_format_info()
> implementation.
> 
> > 
> > > And as stated the current check doesn't have consistent behaviour
> > > anyway. You can still flip between different modifiers as long a the
> > > driver doesn't override .get_format_info() for one of them. The *only*
> > > case where that happens is CCS on i915. There is no valid reason to
> > > special case that one.
> > 
> > The thing is, you need atomic to make CCS work reliably enough for
> > compositors and distros to dare enabling it by default.
> 
> If it's not enabled by default then there is no harm in letting people
> explicitly enable it and get better performance.
> 
> > CCS flipping
> > works with atomic. I really see no point in baking this in with as
> > uapi.
> 
> It's just going back to the original intention of the check.
> Heck, the debug message doesn't even match what it's doing now.
> 
> > Just fix Xorg.
> 
> Be serious. No one is going to rewrite all the randr code to be atomic.

I fully understand Daniel's concern here, but I also believe this won't be
done so soon at least. Meanwhile would it be acceptable to have a comment
with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
?

> 
> > -Daniel
> > 
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > Looking back at the original discussion on this change we pretty much
> > > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > > However, I've decided not to revert the whole thing in case someone
> > > > > has since started to depend on these changes. None of the other checks
> > > > > are relevant for i915 anyways.
> > > > >
> > > > > Cc: stable@vger.kernel.org
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > >     if (ret)
> > > > >             goto out;
> > > > >
> > > > > -   if (old_fb->format != fb->format) {
> > > > > +   if (old_fb->format->format != fb->format->format) {
> > > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > >             ret = -EINVAL;
> > > > >             goto out;
> > > > > --
> > > > > 2.24.1
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>
> On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > >
> > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > nonsense.
> > > > > >
> > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > >
> > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > real. Like real atomic support.
> > > >
> > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > any sensible framework for atomic and I suspect it never will.
> > >
> > > Frankly if no one cares about atomic in X I don't think we should do
> > > work-arounds for lack of atomic in X.
> > >
> > > > > Without modifiers all you get is X tiling,
> > > > > and that works just fine.
> > > > >
> > > > > Which would also fix this issue here you're papering over.
> > > > >
> > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > >
> > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > default now in modesetting.
> > >
> > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > it works. Otherwise good chances for some oddball black screens on
> > > configurations that worked before. Which is why all non-atomic
> > > compositors reverted modifiers by default again.
> >
> > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > It's just an inconsistent mess.
> >
> > If we really want to keep this check then we should rewrite it
> > to be explicit:
> >
> > if (old_fb->format->format != new_fb->format->format ||
> >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> >     return -EINVAL;
> >
> > Now it's just a random thing that may even stop doing what it's
> > currently doing if anyone touches their .get_format_info()
> > implementation.
> >
> > >
> > > > And as stated the current check doesn't have consistent behaviour
> > > > anyway. You can still flip between different modifiers as long a the
> > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > case where that happens is CCS on i915. There is no valid reason to
> > > > special case that one.
> > >
> > > The thing is, you need atomic to make CCS work reliably enough for
> > > compositors and distros to dare enabling it by default.
> >
> > If it's not enabled by default then there is no harm in letting people
> > explicitly enable it and get better performance.
> >
> > > CCS flipping
> > > works with atomic. I really see no point in baking this in with as
> > > uapi.
> >
> > It's just going back to the original intention of the check.
> > Heck, the debug message doesn't even match what it's doing now.
> >
> > > Just fix Xorg.
> >
> > Be serious. No one is going to rewrite all the randr code to be atomic.
>
> I fully understand Daniel's concern here, but I also believe this won't be
> done so soon at least. Meanwhile would it be acceptable to have a comment
> with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> ?

Here's a few numbers:

- skl shipped in Aug 2015, so about 5 years. Since then would we like
to have modifiers enabled for intel, because it costs us quite a bit
of performance. This isn't new at all.
- the last Xorg release is from May 2018, so two years. Meanwhile even
patches to fix some of the atomic mixups in -modesetting landed, but
they never shipped so not useful.
- I spent a few hours (which really is nothing) reading Xorg code
yesterday, and I concur with Daniel Stone's napkin estimate that this
will take about half to one year to fix properly. It's not happening,
no one is working on that.

Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
see why the kernel should bend over for that.

Once that has changed (I'm not betting on that) and there's clear
effort behind modifiers for Xorg-modesetting I guess we can look into
stop-gap measures, but meanwhile the best imo is to not disturb the
dead.

Cheers, Daniel

> >
> > > -Daniel
> > >
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Looking back at the original discussion on this change we pretty much
> > > > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > > > However, I've decided not to revert the whole thing in case someone
> > > > > > has since started to depend on these changes. None of the other checks
> > > > > > are relevant for i915 anyways.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > > >     if (ret)
> > > > > >             goto out;
> > > > > >
> > > > > > -   if (old_fb->format != fb->format) {
> > > > > > +   if (old_fb->format->format != fb->format->format) {
> > > > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > > >             ret = -EINVAL;
> > > > > >             goto out;
> > > > > > --
> > > > > > 2.24.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>
> On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > >
> > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > nonsense.
> > > > > >
> > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > >
> > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > real. Like real atomic support.
> > > >
> > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > any sensible framework for atomic and I suspect it never will.
> > >
> > > Frankly if no one cares about atomic in X I don't think we should do
> > > work-arounds for lack of atomic in X.
> > >
> > > > > Without modifiers all you get is X tiling,
> > > > > and that works just fine.
> > > > >
> > > > > Which would also fix this issue here you're papering over.
> > > > >
> > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > >
> > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > default now in modesetting.
> > >
> > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > it works. Otherwise good chances for some oddball black screens on
> > > configurations that worked before. Which is why all non-atomic
> > > compositors reverted modifiers by default again.
> >
> > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > It's just an inconsistent mess.
> >
> > If we really want to keep this check then we should rewrite it
> > to be explicit:
> >
> > if (old_fb->format->format != new_fb->format->format ||
> >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> >     return -EINVAL;
> >
> > Now it's just a random thing that may even stop doing what it's
> > currently doing if anyone touches their .get_format_info()
> > implementation.
> >
> > >
> > > > And as stated the current check doesn't have consistent behaviour
> > > > anyway. You can still flip between different modifiers as long a the
> > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > case where that happens is CCS on i915. There is no valid reason to
> > > > special case that one.
> > >
> > > The thing is, you need atomic to make CCS work reliably enough for
> > > compositors and distros to dare enabling it by default.
> >
> > If it's not enabled by default then there is no harm in letting people
> > explicitly enable it and get better performance.
> >
> > > CCS flipping
> > > works with atomic. I really see no point in baking this in with as
> > > uapi.
> >
> > It's just going back to the original intention of the check.
> > Heck, the debug message doesn't even match what it's doing now.
> >
> > > Just fix Xorg.
> >
> > Be serious. No one is going to rewrite all the randr code to be atomic.
>
> I fully understand Daniel's concern here, but I also believe this won't be
> done so soon at least. Meanwhile would it be acceptable to have a comment
> with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> ?

Here's a few numbers:

- skl shipped in Aug 2015, so about 5 years. Since then would we like
to have modifiers enabled for intel, because it costs us quite a bit
of performance. This isn't new at all.
- the last Xorg release is from May 2018, so two years. Meanwhile even
patches to fix some of the atomic mixups in -modesetting landed, but
they never shipped so not useful.
- I spent a few hours (which really is nothing) reading Xorg code
yesterday, and I concur with Daniel Stone's napkin estimate that this
will take about half to one year to fix properly. It's not happening,
no one is working on that.

Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
see why the kernel should bend over for that.

Once that has changed (I'm not betting on that) and there's clear
effort behind modifiers for Xorg-modesetting I guess we can look into
stop-gap measures, but meanwhile the best imo is to not disturb the
dead.

Cheers, Daniel

> >
> > > -Daniel
> > >
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Looking back at the original discussion on this change we pretty much
> > > > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > > > However, I've decided not to revert the whole thing in case someone
> > > > > > has since started to depend on these changes. None of the other checks
> > > > > > are relevant for i915 anyways.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > > >     if (ret)
> > > > > >             goto out;
> > > > > >
> > > > > > -   if (old_fb->format != fb->format) {
> > > > > > +   if (old_fb->format->format != fb->format->format) {
> > > > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > > >             ret = -EINVAL;
> > > > > >             goto out;
> > > > > > --
> > > > > > 2.24.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>
> On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > >
> > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > nonsense.
> > > > > >
> > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > >
> > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > real. Like real atomic support.
> > > >
> > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > any sensible framework for atomic and I suspect it never will.
> > >
> > > Frankly if no one cares about atomic in X I don't think we should do
> > > work-arounds for lack of atomic in X.
> > >
> > > > > Without modifiers all you get is X tiling,
> > > > > and that works just fine.
> > > > >
> > > > > Which would also fix this issue here you're papering over.
> > > > >
> > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > >
> > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > default now in modesetting.
> > >
> > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > it works. Otherwise good chances for some oddball black screens on
> > > configurations that worked before. Which is why all non-atomic
> > > compositors reverted modifiers by default again.
> >
> > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > It's just an inconsistent mess.
> >
> > If we really want to keep this check then we should rewrite it
> > to be explicit:
> >
> > if (old_fb->format->format != new_fb->format->format ||
> >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> >     return -EINVAL;
> >
> > Now it's just a random thing that may even stop doing what it's
> > currently doing if anyone touches their .get_format_info()
> > implementation.
> >
> > >
> > > > And as stated the current check doesn't have consistent behaviour
> > > > anyway. You can still flip between different modifiers as long a the
> > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > case where that happens is CCS on i915. There is no valid reason to
> > > > special case that one.
> > >
> > > The thing is, you need atomic to make CCS work reliably enough for
> > > compositors and distros to dare enabling it by default.
> >
> > If it's not enabled by default then there is no harm in letting people
> > explicitly enable it and get better performance.
> >
> > > CCS flipping
> > > works with atomic. I really see no point in baking this in with as
> > > uapi.
> >
> > It's just going back to the original intention of the check.
> > Heck, the debug message doesn't even match what it's doing now.
> >
> > > Just fix Xorg.
> >
> > Be serious. No one is going to rewrite all the randr code to be atomic.
>
> I fully understand Daniel's concern here, but I also believe this won't be
> done so soon at least. Meanwhile would it be acceptable to have a comment
> with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> ?

Here's a few numbers:

- skl shipped in Aug 2015, so about 5 years. Since then would we like
to have modifiers enabled for intel, because it costs us quite a bit
of performance. This isn't new at all.
- the last Xorg release is from May 2018, so two years. Meanwhile even
patches to fix some of the atomic mixups in -modesetting landed, but
they never shipped so not useful.
- I spent a few hours (which really is nothing) reading Xorg code
yesterday, and I concur with Daniel Stone's napkin estimate that this
will take about half to one year to fix properly. It's not happening,
no one is working on that.

Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
see why the kernel should bend over for that.

Once that has changed (I'm not betting on that) and there's clear
effort behind modifiers for Xorg-modesetting I guess we can look into
stop-gap measures, but meanwhile the best imo is to not disturb the
dead.

Cheers, Daniel

> >
> > > -Daniel
> > >
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > > Looking back at the original discussion on this change we pretty much
> > > > > > just did it in the name of skipping a few extra pointer dereferences.
> > > > > > However, I've decided not to revert the whole thing in case someone
> > > > > > has since started to depend on these changes. None of the other checks
> > > > > > are relevant for i915 anyways.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Fixes: dbd4d5761e1f ("drm: Replace 'format->format' comparisons to just 'format' comparisons")
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_plane.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > > > > index d6ad60ab0d38..f2ca5315f23b 100644
> > > > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > > > @@ -1153,7 +1153,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > > > > >     if (ret)
> > > > > >             goto out;
> > > > > >
> > > > > > -   if (old_fb->format != fb->format) {
> > > > > > +   if (old_fb->format->format != fb->format->format) {
> > > > > >             DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > > > >             ret = -EINVAL;
> > > > > >             goto out;
> > > > > > --
> > > > > > 2.24.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > <ville.syrjala@linux.intel.com> wrote:
> > > > >
> > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > >
> > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > nonsense.
> > > > > > >
> > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > >
> > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > real. Like real atomic support.
> > > > >
> > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > any sensible framework for atomic and I suspect it never will.
> > > >
> > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > work-arounds for lack of atomic in X.
> > > >
> > > > > > Without modifiers all you get is X tiling,
> > > > > > and that works just fine.
> > > > > >
> > > > > > Which would also fix this issue here you're papering over.
> > > > > >
> > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > >
> > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > default now in modesetting.
> > > >
> > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > it works. Otherwise good chances for some oddball black screens on
> > > > configurations that worked before. Which is why all non-atomic
> > > > compositors reverted modifiers by default again.
> > >
> > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > It's just an inconsistent mess.
> > >
> > > If we really want to keep this check then we should rewrite it
> > > to be explicit:
> > >
> > > if (old_fb->format->format != new_fb->format->format ||
> > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > >     return -EINVAL;
> > >
> > > Now it's just a random thing that may even stop doing what it's
> > > currently doing if anyone touches their .get_format_info()
> > > implementation.
> > >
> > > >
> > > > > And as stated the current check doesn't have consistent behaviour
> > > > > anyway. You can still flip between different modifiers as long a the
> > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > special case that one.
> > > >
> > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > compositors and distros to dare enabling it by default.
> > >
> > > If it's not enabled by default then there is no harm in letting people
> > > explicitly enable it and get better performance.
> > >
> > > > CCS flipping
> > > > works with atomic. I really see no point in baking this in with as
> > > > uapi.
> > >
> > > It's just going back to the original intention of the check.
> > > Heck, the debug message doesn't even match what it's doing now.
> > >
> > > > Just fix Xorg.
> > >
> > > Be serious. No one is going to rewrite all the randr code to be atomic.
> >
> > I fully understand Daniel's concern here, but I also believe this won't be
> > done so soon at least. Meanwhile would it be acceptable to have a comment
> > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > ?
> 
> Here's a few numbers:
> 
> - skl shipped in Aug 2015, so about 5 years. Since then would we like
> to have modifiers enabled for intel, because it costs us quite a bit
> of performance. This isn't new at all.
> - the last Xorg release is from May 2018, so two years. Meanwhile even
> patches to fix some of the atomic mixups in -modesetting landed, but
> they never shipped so not useful.
> - I spent a few hours (which really is nothing) reading Xorg code
> yesterday, and I concur with Daniel Stone's napkin estimate that this
> will take about half to one year to fix properly. It's not happening,
> no one is working on that.
> 
> Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> see why the kernel should bend over for that.
> 
> Once that has changed (I'm not betting on that) and there's clear
> effort behind modifiers for Xorg-modesetting I guess we can look into
> stop-gap measures, but meanwhile the best imo is to not disturb the
> dead.

The alternative interpretation is that the current kernel code is
just nonsense, and since no one is depending on the current nonsense
behaviour we can safely change it it back to make sense.

Would allow people to at least test modifier plumbing via dri3/etc.
Also those of us who know what they're doing and want to actually
play games on Intel GPUs can flip it on for a a bit extra performance.
In the meantime I'll just have to keep carrying this patch in my own
kernels.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > <ville.syrjala@linux.intel.com> wrote:
> > > > >
> > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > >
> > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > nonsense.
> > > > > > >
> > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > >
> > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > real. Like real atomic support.
> > > > >
> > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > any sensible framework for atomic and I suspect it never will.
> > > >
> > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > work-arounds for lack of atomic in X.
> > > >
> > > > > > Without modifiers all you get is X tiling,
> > > > > > and that works just fine.
> > > > > >
> > > > > > Which would also fix this issue here you're papering over.
> > > > > >
> > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > >
> > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > default now in modesetting.
> > > >
> > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > it works. Otherwise good chances for some oddball black screens on
> > > > configurations that worked before. Which is why all non-atomic
> > > > compositors reverted modifiers by default again.
> > >
> > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > It's just an inconsistent mess.
> > >
> > > If we really want to keep this check then we should rewrite it
> > > to be explicit:
> > >
> > > if (old_fb->format->format != new_fb->format->format ||
> > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > >     return -EINVAL;
> > >
> > > Now it's just a random thing that may even stop doing what it's
> > > currently doing if anyone touches their .get_format_info()
> > > implementation.
> > >
> > > >
> > > > > And as stated the current check doesn't have consistent behaviour
> > > > > anyway. You can still flip between different modifiers as long a the
> > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > special case that one.
> > > >
> > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > compositors and distros to dare enabling it by default.
> > >
> > > If it's not enabled by default then there is no harm in letting people
> > > explicitly enable it and get better performance.
> > >
> > > > CCS flipping
> > > > works with atomic. I really see no point in baking this in with as
> > > > uapi.
> > >
> > > It's just going back to the original intention of the check.
> > > Heck, the debug message doesn't even match what it's doing now.
> > >
> > > > Just fix Xorg.
> > >
> > > Be serious. No one is going to rewrite all the randr code to be atomic.
> >
> > I fully understand Daniel's concern here, but I also believe this won't be
> > done so soon at least. Meanwhile would it be acceptable to have a comment
> > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > ?
> 
> Here's a few numbers:
> 
> - skl shipped in Aug 2015, so about 5 years. Since then would we like
> to have modifiers enabled for intel, because it costs us quite a bit
> of performance. This isn't new at all.
> - the last Xorg release is from May 2018, so two years. Meanwhile even
> patches to fix some of the atomic mixups in -modesetting landed, but
> they never shipped so not useful.
> - I spent a few hours (which really is nothing) reading Xorg code
> yesterday, and I concur with Daniel Stone's napkin estimate that this
> will take about half to one year to fix properly. It's not happening,
> no one is working on that.
> 
> Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> see why the kernel should bend over for that.
> 
> Once that has changed (I'm not betting on that) and there's clear
> effort behind modifiers for Xorg-modesetting I guess we can look into
> stop-gap measures, but meanwhile the best imo is to not disturb the
> dead.

The alternative interpretation is that the current kernel code is
just nonsense, and since no one is depending on the current nonsense
behaviour we can safely change it it back to make sense.

Would allow people to at least test modifier plumbing via dri3/etc.
Also those of us who know what they're doing and want to actually
play games on Intel GPUs can flip it on for a a bit extra performance.
In the meantime I'll just have to keep carrying this patch in my own
kernels.

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > <ville.syrjala@linux.intel.com> wrote:
> > > > >
> > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > >
> > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > nonsense.
> > > > > > >
> > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > >
> > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > real. Like real atomic support.
> > > > >
> > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > any sensible framework for atomic and I suspect it never will.
> > > >
> > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > work-arounds for lack of atomic in X.
> > > >
> > > > > > Without modifiers all you get is X tiling,
> > > > > > and that works just fine.
> > > > > >
> > > > > > Which would also fix this issue here you're papering over.
> > > > > >
> > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > >
> > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > default now in modesetting.
> > > >
> > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > it works. Otherwise good chances for some oddball black screens on
> > > > configurations that worked before. Which is why all non-atomic
> > > > compositors reverted modifiers by default again.
> > >
> > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > It's just an inconsistent mess.
> > >
> > > If we really want to keep this check then we should rewrite it
> > > to be explicit:
> > >
> > > if (old_fb->format->format != new_fb->format->format ||
> > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > >     return -EINVAL;
> > >
> > > Now it's just a random thing that may even stop doing what it's
> > > currently doing if anyone touches their .get_format_info()
> > > implementation.
> > >
> > > >
> > > > > And as stated the current check doesn't have consistent behaviour
> > > > > anyway. You can still flip between different modifiers as long a the
> > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > special case that one.
> > > >
> > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > compositors and distros to dare enabling it by default.
> > >
> > > If it's not enabled by default then there is no harm in letting people
> > > explicitly enable it and get better performance.
> > >
> > > > CCS flipping
> > > > works with atomic. I really see no point in baking this in with as
> > > > uapi.
> > >
> > > It's just going back to the original intention of the check.
> > > Heck, the debug message doesn't even match what it's doing now.
> > >
> > > > Just fix Xorg.
> > >
> > > Be serious. No one is going to rewrite all the randr code to be atomic.
> >
> > I fully understand Daniel's concern here, but I also believe this won't be
> > done so soon at least. Meanwhile would it be acceptable to have a comment
> > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > ?
> 
> Here's a few numbers:
> 
> - skl shipped in Aug 2015, so about 5 years. Since then would we like
> to have modifiers enabled for intel, because it costs us quite a bit
> of performance. This isn't new at all.
> - the last Xorg release is from May 2018, so two years. Meanwhile even
> patches to fix some of the atomic mixups in -modesetting landed, but
> they never shipped so not useful.
> - I spent a few hours (which really is nothing) reading Xorg code
> yesterday, and I concur with Daniel Stone's napkin estimate that this
> will take about half to one year to fix properly. It's not happening,
> no one is working on that.
> 
> Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> see why the kernel should bend over for that.
> 
> Once that has changed (I'm not betting on that) and there's clear
> effort behind modifiers for Xorg-modesetting I guess we can look into
> stop-gap measures, but meanwhile the best imo is to not disturb the
> dead.

The alternative interpretation is that the current kernel code is
just nonsense, and since no one is depending on the current nonsense
behaviour we can safely change it it back to make sense.

Would allow people to at least test modifier plumbing via dri3/etc.
Also those of us who know what they're doing and want to actually
play games on Intel GPUs can flip it on for a a bit extra performance.
In the meantime I'll just have to keep carrying this patch in my own
kernels.

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Mon, May 11, 2020 at 2:37 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> > On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > >
> > > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > >
> > > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > > nonsense.
> > > > > > > >
> > > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > > >
> > > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > > real. Like real atomic support.
> > > > > >
> > > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > > any sensible framework for atomic and I suspect it never will.
> > > > >
> > > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > > work-arounds for lack of atomic in X.
> > > > >
> > > > > > > Without modifiers all you get is X tiling,
> > > > > > > and that works just fine.
> > > > > > >
> > > > > > > Which would also fix this issue here you're papering over.
> > > > > > >
> > > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > > >
> > > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > > default now in modesetting.
> > > > >
> > > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > > it works. Otherwise good chances for some oddball black screens on
> > > > > configurations that worked before. Which is why all non-atomic
> > > > > compositors reverted modifiers by default again.
> > > >
> > > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > > It's just an inconsistent mess.
> > > >
> > > > If we really want to keep this check then we should rewrite it
> > > > to be explicit:
> > > >
> > > > if (old_fb->format->format != new_fb->format->format ||
> > > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > > >     return -EINVAL;
> > > >
> > > > Now it's just a random thing that may even stop doing what it's
> > > > currently doing if anyone touches their .get_format_info()
> > > > implementation.
> > > >
> > > > >
> > > > > > And as stated the current check doesn't have consistent behaviour
> > > > > > anyway. You can still flip between different modifiers as long a the
> > > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > > special case that one.
> > > > >
> > > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > > compositors and distros to dare enabling it by default.
> > > >
> > > > If it's not enabled by default then there is no harm in letting people
> > > > explicitly enable it and get better performance.
> > > >
> > > > > CCS flipping
> > > > > works with atomic. I really see no point in baking this in with as
> > > > > uapi.
> > > >
> > > > It's just going back to the original intention of the check.
> > > > Heck, the debug message doesn't even match what it's doing now.
> > > >
> > > > > Just fix Xorg.
> > > >
> > > > Be serious. No one is going to rewrite all the randr code to be atomic.
> > >
> > > I fully understand Daniel's concern here, but I also believe this won't be
> > > done so soon at least. Meanwhile would it be acceptable to have a comment
> > > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > > ?
> >
> > Here's a few numbers:
> >
> > - skl shipped in Aug 2015, so about 5 years. Since then would we like
> > to have modifiers enabled for intel, because it costs us quite a bit
> > of performance. This isn't new at all.
> > - the last Xorg release is from May 2018, so two years. Meanwhile even
> > patches to fix some of the atomic mixups in -modesetting landed, but
> > they never shipped so not useful.
> > - I spent a few hours (which really is nothing) reading Xorg code
> > yesterday, and I concur with Daniel Stone's napkin estimate that this
> > will take about half to one year to fix properly. It's not happening,
> > no one is working on that.
> >
> > Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> > see why the kernel should bend over for that.
> >
> > Once that has changed (I'm not betting on that) and there's clear
> > effort behind modifiers for Xorg-modesetting I guess we can look into
> > stop-gap measures, but meanwhile the best imo is to not disturb the
> > dead.
>
> The alternative interpretation is that the current kernel code is
> just nonsense, and since no one is depending on the current nonsense
> behaviour we can safely change it it back to make sense.
>
> Would allow people to at least test modifier plumbing via dri3/etc.
> Also those of us who know what they're doing and want to actually
> play games on Intel GPUs can flip it on for a a bit extra performance.
> In the meantime I'll just have to keep carrying this patch in my own
> kernels.

You can also carry a one-liner for -modesetting to re-enable atomic on
master (it's fixed up there, simply never released, why we've had to
take it away). And then you can also play with modifiers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Mon, May 11, 2020 at 2:37 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> > On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > >
> > > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > >
> > > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > > nonsense.
> > > > > > > >
> > > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > > >
> > > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > > real. Like real atomic support.
> > > > > >
> > > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > > any sensible framework for atomic and I suspect it never will.
> > > > >
> > > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > > work-arounds for lack of atomic in X.
> > > > >
> > > > > > > Without modifiers all you get is X tiling,
> > > > > > > and that works just fine.
> > > > > > >
> > > > > > > Which would also fix this issue here you're papering over.
> > > > > > >
> > > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > > >
> > > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > > default now in modesetting.
> > > > >
> > > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > > it works. Otherwise good chances for some oddball black screens on
> > > > > configurations that worked before. Which is why all non-atomic
> > > > > compositors reverted modifiers by default again.
> > > >
> > > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > > It's just an inconsistent mess.
> > > >
> > > > If we really want to keep this check then we should rewrite it
> > > > to be explicit:
> > > >
> > > > if (old_fb->format->format != new_fb->format->format ||
> > > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > > >     return -EINVAL;
> > > >
> > > > Now it's just a random thing that may even stop doing what it's
> > > > currently doing if anyone touches their .get_format_info()
> > > > implementation.
> > > >
> > > > >
> > > > > > And as stated the current check doesn't have consistent behaviour
> > > > > > anyway. You can still flip between different modifiers as long a the
> > > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > > special case that one.
> > > > >
> > > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > > compositors and distros to dare enabling it by default.
> > > >
> > > > If it's not enabled by default then there is no harm in letting people
> > > > explicitly enable it and get better performance.
> > > >
> > > > > CCS flipping
> > > > > works with atomic. I really see no point in baking this in with as
> > > > > uapi.
> > > >
> > > > It's just going back to the original intention of the check.
> > > > Heck, the debug message doesn't even match what it's doing now.
> > > >
> > > > > Just fix Xorg.
> > > >
> > > > Be serious. No one is going to rewrite all the randr code to be atomic.
> > >
> > > I fully understand Daniel's concern here, but I also believe this won't be
> > > done so soon at least. Meanwhile would it be acceptable to have a comment
> > > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > > ?
> >
> > Here's a few numbers:
> >
> > - skl shipped in Aug 2015, so about 5 years. Since then would we like
> > to have modifiers enabled for intel, because it costs us quite a bit
> > of performance. This isn't new at all.
> > - the last Xorg release is from May 2018, so two years. Meanwhile even
> > patches to fix some of the atomic mixups in -modesetting landed, but
> > they never shipped so not useful.
> > - I spent a few hours (which really is nothing) reading Xorg code
> > yesterday, and I concur with Daniel Stone's napkin estimate that this
> > will take about half to one year to fix properly. It's not happening,
> > no one is working on that.
> >
> > Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> > see why the kernel should bend over for that.
> >
> > Once that has changed (I'm not betting on that) and there's clear
> > effort behind modifiers for Xorg-modesetting I guess we can look into
> > stop-gap measures, but meanwhile the best imo is to not disturb the
> > dead.
>
> The alternative interpretation is that the current kernel code is
> just nonsense, and since no one is depending on the current nonsense
> behaviour we can safely change it it back to make sense.
>
> Would allow people to at least test modifier plumbing via dri3/etc.
> Also those of us who know what they're doing and want to actually
> play games on Intel GPUs can flip it on for a a bit extra performance.
> In the meantime I'll just have to keep carrying this patch in my own
> kernels.

You can also carry a one-liner for -modesetting to re-enable atomic on
master (it's fixed up there, simply never released, why we've had to
take it away). And then you can also play with modifiers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Fix page flip ioctl format check

[Daniel Vetter]

On Mon, May 11, 2020 at 2:37 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> > On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > >
> > > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > >
> > > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > >
> > > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > > nonsense.
> > > > > > > >
> > > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > > >
> > > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > > real. Like real atomic support.
> > > > > >
> > > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > > any sensible framework for atomic and I suspect it never will.
> > > > >
> > > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > > work-arounds for lack of atomic in X.
> > > > >
> > > > > > > Without modifiers all you get is X tiling,
> > > > > > > and that works just fine.
> > > > > > >
> > > > > > > Which would also fix this issue here you're papering over.
> > > > > > >
> > > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > > >
> > > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > > default now in modesetting.
> > > > >
> > > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > > it works. Otherwise good chances for some oddball black screens on
> > > > > configurations that worked before. Which is why all non-atomic
> > > > > compositors reverted modifiers by default again.
> > > >
> > > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > > It's just an inconsistent mess.
> > > >
> > > > If we really want to keep this check then we should rewrite it
> > > > to be explicit:
> > > >
> > > > if (old_fb->format->format != new_fb->format->format ||
> > > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > > >     return -EINVAL;
> > > >
> > > > Now it's just a random thing that may even stop doing what it's
> > > > currently doing if anyone touches their .get_format_info()
> > > > implementation.
> > > >
> > > > >
> > > > > > And as stated the current check doesn't have consistent behaviour
> > > > > > anyway. You can still flip between different modifiers as long a the
> > > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > > special case that one.
> > > > >
> > > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > > compositors and distros to dare enabling it by default.
> > > >
> > > > If it's not enabled by default then there is no harm in letting people
> > > > explicitly enable it and get better performance.
> > > >
> > > > > CCS flipping
> > > > > works with atomic. I really see no point in baking this in with as
> > > > > uapi.
> > > >
> > > > It's just going back to the original intention of the check.
> > > > Heck, the debug message doesn't even match what it's doing now.
> > > >
> > > > > Just fix Xorg.
> > > >
> > > > Be serious. No one is going to rewrite all the randr code to be atomic.
> > >
> > > I fully understand Daniel's concern here, but I also believe this won't be
> > > done so soon at least. Meanwhile would it be acceptable to have a comment
> > > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > > ?
> >
> > Here's a few numbers:
> >
> > - skl shipped in Aug 2015, so about 5 years. Since then would we like
> > to have modifiers enabled for intel, because it costs us quite a bit
> > of performance. This isn't new at all.
> > - the last Xorg release is from May 2018, so two years. Meanwhile even
> > patches to fix some of the atomic mixups in -modesetting landed, but
> > they never shipped so not useful.
> > - I spent a few hours (which really is nothing) reading Xorg code
> > yesterday, and I concur with Daniel Stone's napkin estimate that this
> > will take about half to one year to fix properly. It's not happening,
> > no one is working on that.
> >
> > Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> > see why the kernel should bend over for that.
> >
> > Once that has changed (I'm not betting on that) and there's clear
> > effort behind modifiers for Xorg-modesetting I guess we can look into
> > stop-gap measures, but meanwhile the best imo is to not disturb the
> > dead.
>
> The alternative interpretation is that the current kernel code is
> just nonsense, and since no one is depending on the current nonsense
> behaviour we can safely change it it back to make sense.
>
> Would allow people to at least test modifier plumbing via dri3/etc.
> Also those of us who know what they're doing and want to actually
> play games on Intel GPUs can flip it on for a a bit extra performance.
> In the meantime I'll just have to keep carrying this patch in my own
> kernels.

You can also carry a one-liner for -modesetting to re-enable atomic on
master (it's fixed up there, simply never released, why we've had to
take it away). And then you can also play with modifiers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Mon, May 11, 2020 at 02:41:13PM +0200, Daniel Vetter wrote:
> On Mon, May 11, 2020 at 2:37 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> > > On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > >
> > > > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > >
> > > > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > > > nonsense.
> > > > > > > > >
> > > > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > > > >
> > > > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > > > real. Like real atomic support.
> > > > > > >
> > > > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > > > any sensible framework for atomic and I suspect it never will.
> > > > > >
> > > > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > > > work-arounds for lack of atomic in X.
> > > > > >
> > > > > > > > Without modifiers all you get is X tiling,
> > > > > > > > and that works just fine.
> > > > > > > >
> > > > > > > > Which would also fix this issue here you're papering over.
> > > > > > > >
> > > > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > > > >
> > > > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > > > default now in modesetting.
> > > > > >
> > > > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > > > it works. Otherwise good chances for some oddball black screens on
> > > > > > configurations that worked before. Which is why all non-atomic
> > > > > > compositors reverted modifiers by default again.
> > > > >
> > > > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > > > It's just an inconsistent mess.
> > > > >
> > > > > If we really want to keep this check then we should rewrite it
> > > > > to be explicit:
> > > > >
> > > > > if (old_fb->format->format != new_fb->format->format ||
> > > > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > > > >     return -EINVAL;
> > > > >
> > > > > Now it's just a random thing that may even stop doing what it's
> > > > > currently doing if anyone touches their .get_format_info()
> > > > > implementation.
> > > > >
> > > > > >
> > > > > > > And as stated the current check doesn't have consistent behaviour
> > > > > > > anyway. You can still flip between different modifiers as long a the
> > > > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > > > special case that one.
> > > > > >
> > > > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > > > compositors and distros to dare enabling it by default.
> > > > >
> > > > > If it's not enabled by default then there is no harm in letting people
> > > > > explicitly enable it and get better performance.
> > > > >
> > > > > > CCS flipping
> > > > > > works with atomic. I really see no point in baking this in with as
> > > > > > uapi.
> > > > >
> > > > > It's just going back to the original intention of the check.
> > > > > Heck, the debug message doesn't even match what it's doing now.
> > > > >
> > > > > > Just fix Xorg.
> > > > >
> > > > > Be serious. No one is going to rewrite all the randr code to be atomic.
> > > >
> > > > I fully understand Daniel's concern here, but I also believe this won't be
> > > > done so soon at least. Meanwhile would it be acceptable to have a comment
> > > > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > > > ?
> > >
> > > Here's a few numbers:
> > >
> > > - skl shipped in Aug 2015, so about 5 years. Since then would we like
> > > to have modifiers enabled for intel, because it costs us quite a bit
> > > of performance. This isn't new at all.
> > > - the last Xorg release is from May 2018, so two years. Meanwhile even
> > > patches to fix some of the atomic mixups in -modesetting landed, but
> > > they never shipped so not useful.
> > > - I spent a few hours (which really is nothing) reading Xorg code
> > > yesterday, and I concur with Daniel Stone's napkin estimate that this
> > > will take about half to one year to fix properly. It's not happening,
> > > no one is working on that.
> > >
> > > Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> > > see why the kernel should bend over for that.
> > >
> > > Once that has changed (I'm not betting on that) and there's clear
> > > effort behind modifiers for Xorg-modesetting I guess we can look into
> > > stop-gap measures, but meanwhile the best imo is to not disturb the
> > > dead.
> >
> > The alternative interpretation is that the current kernel code is
> > just nonsense, and since no one is depending on the current nonsense
> > behaviour we can safely change it it back to make sense.
> >
> > Would allow people to at least test modifier plumbing via dri3/etc.
> > Also those of us who know what they're doing and want to actually
> > play games on Intel GPUs can flip it on for a a bit extra performance.
> > In the meantime I'll just have to keep carrying this patch in my own
> > kernels.
> 
> You can also carry a one-liner for -modesetting to re-enable atomic on
> master (it's fixed up there, simply never released, why we've had to
> take it away). And then you can also play with modifiers.

Nah. I prefer to carry the obviously corect fix rather than something
that may or may not have unknown issues.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Mon, May 11, 2020 at 02:41:13PM +0200, Daniel Vetter wrote:
> On Mon, May 11, 2020 at 2:37 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> > > On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > >
> > > > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > >
> > > > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > > > nonsense.
> > > > > > > > >
> > > > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > > > >
> > > > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > > > real. Like real atomic support.
> > > > > > >
> > > > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > > > any sensible framework for atomic and I suspect it never will.
> > > > > >
> > > > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > > > work-arounds for lack of atomic in X.
> > > > > >
> > > > > > > > Without modifiers all you get is X tiling,
> > > > > > > > and that works just fine.
> > > > > > > >
> > > > > > > > Which would also fix this issue here you're papering over.
> > > > > > > >
> > > > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > > > >
> > > > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > > > default now in modesetting.
> > > > > >
> > > > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > > > it works. Otherwise good chances for some oddball black screens on
> > > > > > configurations that worked before. Which is why all non-atomic
> > > > > > compositors reverted modifiers by default again.
> > > > >
> > > > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > > > It's just an inconsistent mess.
> > > > >
> > > > > If we really want to keep this check then we should rewrite it
> > > > > to be explicit:
> > > > >
> > > > > if (old_fb->format->format != new_fb->format->format ||
> > > > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > > > >     return -EINVAL;
> > > > >
> > > > > Now it's just a random thing that may even stop doing what it's
> > > > > currently doing if anyone touches their .get_format_info()
> > > > > implementation.
> > > > >
> > > > > >
> > > > > > > And as stated the current check doesn't have consistent behaviour
> > > > > > > anyway. You can still flip between different modifiers as long a the
> > > > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > > > special case that one.
> > > > > >
> > > > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > > > compositors and distros to dare enabling it by default.
> > > > >
> > > > > If it's not enabled by default then there is no harm in letting people
> > > > > explicitly enable it and get better performance.
> > > > >
> > > > > > CCS flipping
> > > > > > works with atomic. I really see no point in baking this in with as
> > > > > > uapi.
> > > > >
> > > > > It's just going back to the original intention of the check.
> > > > > Heck, the debug message doesn't even match what it's doing now.
> > > > >
> > > > > > Just fix Xorg.
> > > > >
> > > > > Be serious. No one is going to rewrite all the randr code to be atomic.
> > > >
> > > > I fully understand Daniel's concern here, but I also believe this won't be
> > > > done so soon at least. Meanwhile would it be acceptable to have a comment
> > > > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > > > ?
> > >
> > > Here's a few numbers:
> > >
> > > - skl shipped in Aug 2015, so about 5 years. Since then would we like
> > > to have modifiers enabled for intel, because it costs us quite a bit
> > > of performance. This isn't new at all.
> > > - the last Xorg release is from May 2018, so two years. Meanwhile even
> > > patches to fix some of the atomic mixups in -modesetting landed, but
> > > they never shipped so not useful.
> > > - I spent a few hours (which really is nothing) reading Xorg code
> > > yesterday, and I concur with Daniel Stone's napkin estimate that this
> > > will take about half to one year to fix properly. It's not happening,
> > > no one is working on that.
> > >
> > > Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> > > see why the kernel should bend over for that.
> > >
> > > Once that has changed (I'm not betting on that) and there's clear
> > > effort behind modifiers for Xorg-modesetting I guess we can look into
> > > stop-gap measures, but meanwhile the best imo is to not disturb the
> > > dead.
> >
> > The alternative interpretation is that the current kernel code is
> > just nonsense, and since no one is depending on the current nonsense
> > behaviour we can safely change it it back to make sense.
> >
> > Would allow people to at least test modifier plumbing via dri3/etc.
> > Also those of us who know what they're doing and want to actually
> > play games on Intel GPUs can flip it on for a a bit extra performance.
> > In the meantime I'll just have to keep carrying this patch in my own
> > kernels.
> 
> You can also carry a one-liner for -modesetting to re-enable atomic on
> master (it's fixed up there, simply never released, why we've had to
> take it away). And then you can also play with modifiers.

Nah. I prefer to carry the obviously corect fix rather than something
that may or may not have unknown issues.

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

Re: [PATCH] drm: Fix page flip ioctl format check

[Ville Syrjälä]

On Mon, May 11, 2020 at 02:41:13PM +0200, Daniel Vetter wrote:
> On Mon, May 11, 2020 at 2:37 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Sat, May 09, 2020 at 12:13:02PM +0200, Daniel Vetter wrote:
> > > On Fri, May 8, 2020 at 7:09 PM Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > >
> > > > On Fri, Apr 17, 2020 at 09:28:34PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Apr 17, 2020 at 08:10:26PM +0200, Daniel Vetter wrote:
> > > > > > On Fri, Apr 17, 2020 at 5:43 PM Ville Syrjälä
> > > > > > <ville.syrjala@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Fri, Apr 17, 2020 at 05:23:10PM +0200, Daniel Vetter wrote:
> > > > > > > > On Thu, Apr 16, 2020 at 08:04:20PM +0300, Ville Syrjala wrote:
> > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > >
> > > > > > > > > Revert back to comparing fb->format->format instead fb->format for the
> > > > > > > > > page flip ioctl. This check was originally only here to disallow pixel
> > > > > > > > > format changes, but when we changed it to do the pointer comparison
> > > > > > > > > we potentially started to reject some (but definitely not all) modifier
> > > > > > > > > changes as well. In fact the current behaviour depends on whether the
> > > > > > > > > driver overrides the format info for a specific format+modifier combo.
> > > > > > > > > Eg. on i915 this now rejects compression vs. no compression changes but
> > > > > > > > > does not reject any other tiling changes. That's just inconsistent
> > > > > > > > > nonsense.
> > > > > > > > >
> > > > > > > > > The main reason we have to go back to the old behaviour is to fix page
> > > > > > > > > flipping with Xorg. At some point Xorg got its atomic rights taken away
> > > > > > > > > and since then we can't page flip between compressed and non-compressed
> > > > > > > > > fbs on i915. Currently we get no page flipping for any games pretty much
> > > > > > > > > since Mesa likes to use compressed buffers. Not sure how compositors are
> > > > > > > > > working around this (don't use one myself). I guess they must be doing
> > > > > > > > > something to get non-compressed buffers instead. Either that or
> > > > > > > > > somehow no one noticed the tearing from the blit fallback.
> > > > > > > >
> > > > > > > > Mesa only uses compressed buffers if you enable modifiers, and there's a
> > > > > > > > _loooooooooooot_ more that needs to be fixed in Xorg to enable that for
> > > > > > > > real. Like real atomic support.
> > > > > > >
> > > > > > > Why would you need atomic for modifiers? Xorg doesn't even have
> > > > > > > any sensible framework for atomic and I suspect it never will.
> > > > > >
> > > > > > Frankly if no one cares about atomic in X I don't think we should do
> > > > > > work-arounds for lack of atomic in X.
> > > > > >
> > > > > > > > Without modifiers all you get is X tiling,
> > > > > > > > and that works just fine.
> > > > > > > >
> > > > > > > > Which would also fix this issue here you're papering over.
> > > > > > > >
> > > > > > > > So if this is the entire reason for this, I'm inclined to not do this.
> > > > > > > > Current Xorg is toast wrt modifiers, that's not news.
> > > > > > >
> > > > > > > Works just fine. Also pretty sure modifiers are even enabled by
> > > > > > > default now in modesetting.
> > > > > >
> > > > > > Y/CSS is harder to scan out, you need to verify with TEST_ONLY whether
> > > > > > it works. Otherwise good chances for some oddball black screens on
> > > > > > configurations that worked before. Which is why all non-atomic
> > > > > > compositors reverted modifiers by default again.
> > > > >
> > > > > Y alone is hard to scanout also, and yet we do nothing to reject that.
> > > > > It's just an inconsistent mess.
> > > > >
> > > > > If we really want to keep this check then we should rewrite it
> > > > > to be explicit:
> > > > >
> > > > > if (old_fb->format->format != new_fb->format->format ||
> > > > >     is_ccs(old_fb->modifier) != is_ccs(new_fb->modifier))
> > > > >     return -EINVAL;
> > > > >
> > > > > Now it's just a random thing that may even stop doing what it's
> > > > > currently doing if anyone touches their .get_format_info()
> > > > > implementation.
> > > > >
> > > > > >
> > > > > > > And as stated the current check doesn't have consistent behaviour
> > > > > > > anyway. You can still flip between different modifiers as long a the
> > > > > > > driver doesn't override .get_format_info() for one of them. The *only*
> > > > > > > case where that happens is CCS on i915. There is no valid reason to
> > > > > > > special case that one.
> > > > > >
> > > > > > The thing is, you need atomic to make CCS work reliably enough for
> > > > > > compositors and distros to dare enabling it by default.
> > > > >
> > > > > If it's not enabled by default then there is no harm in letting people
> > > > > explicitly enable it and get better performance.
> > > > >
> > > > > > CCS flipping
> > > > > > works with atomic. I really see no point in baking this in with as
> > > > > > uapi.
> > > > >
> > > > > It's just going back to the original intention of the check.
> > > > > Heck, the debug message doesn't even match what it's doing now.
> > > > >
> > > > > > Just fix Xorg.
> > > > >
> > > > > Be serious. No one is going to rewrite all the randr code to be atomic.
> > > >
> > > > I fully understand Daniel's concern here, but I also believe this won't be
> > > > done so soon at least. Meanwhile would it be acceptable to have a comment
> > > > with the code /* XXX: Xorg blah... */ or /* FIXME: After Xorg blah.. */
> > > > ?
> > >
> > > Here's a few numbers:
> > >
> > > - skl shipped in Aug 2015, so about 5 years. Since then would we like
> > > to have modifiers enabled for intel, because it costs us quite a bit
> > > of performance. This isn't new at all.
> > > - the last Xorg release is from May 2018, so two years. Meanwhile even
> > > patches to fix some of the atomic mixups in -modesetting landed, but
> > > they never shipped so not useful.
> > > - I spent a few hours (which really is nothing) reading Xorg code
> > > yesterday, and I concur with Daniel Stone's napkin estimate that this
> > > will take about half to one year to fix properly. It's not happening,
> > > no one is working on that.
> > >
> > > Conclusion: No one cares about modifiers on Xorg-modesetting. I don't
> > > see why the kernel should bend over for that.
> > >
> > > Once that has changed (I'm not betting on that) and there's clear
> > > effort behind modifiers for Xorg-modesetting I guess we can look into
> > > stop-gap measures, but meanwhile the best imo is to not disturb the
> > > dead.
> >
> > The alternative interpretation is that the current kernel code is
> > just nonsense, and since no one is depending on the current nonsense
> > behaviour we can safely change it it back to make sense.
> >
> > Would allow people to at least test modifier plumbing via dri3/etc.
> > Also those of us who know what they're doing and want to actually
> > play games on Intel GPUs can flip it on for a a bit extra performance.
> > In the meantime I'll just have to keep carrying this patch in my own
> > kernels.
> 
> You can also carry a one-liner for -modesetting to re-enable atomic on
> master (it's fixed up there, simply never released, why we've had to
> take it away). And then you can also play with modifiers.

Nah. I prefer to carry the obviously corect fix rather than something
that may or may not have unknown issues.

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

[Intel-gfx] ✓ Fi.CI.IGT: success for drm: Fix page flip ioctl format check

[Patchwork]

== Series Details ==

Series: drm: Fix page flip ioctl format check
URL   : https://patchwork.freedesktop.org/series/76040/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8311_full -> Patchwork_17331_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@kms_flip@flip-vs-suspend@a-edp1}:
    - shard-skl:          [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-skl2/igt@kms_flip@flip-vs-suspend@a-edp1.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-skl1/igt@kms_flip@flip-vs-suspend@a-edp1.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_params@invalid-bsd-ring:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#109276])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-iclb2/igt@gem_exec_params@invalid-bsd-ring.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-iclb8/igt@gem_exec_params@invalid-bsd-ring.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-apl:          [PASS][5] -> [DMESG-WARN][6] ([i915#183])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-apl2/igt@gem_tiled_swapping@non-threaded.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-apl7/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-skl:          [PASS][7] -> [INCOMPLETE][8] ([i915#151] / [i915#69])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-skl5/igt@i915_pm_rpm@system-suspend-execbuf.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-skl3/igt@i915_pm_rpm@system-suspend-execbuf.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x256-offscreen:
    - shard-skl:          [PASS][9] -> [FAIL][10] ([i915#54]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-skl7/igt@kms_cursor_crc@pipe-a-cursor-256x256-offscreen.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-256x256-offscreen.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([i915#54] / [i915#93] / [i915#95])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-kbl6/igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-kbl2/igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen.html
    - shard-apl:          [PASS][13] -> [FAIL][14] ([i915#54] / [i915#95])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-apl4/igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-apl6/igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen.html

  * igt@kms_hdr@bpc-switch-dpms:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([i915#1188])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-skl1/igt@kms_hdr@bpc-switch-dpms.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-skl2/igt@kms_hdr@bpc-switch-dpms.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-kbl:          [PASS][17] -> [DMESG-WARN][18] ([i915#180]) +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-kbl1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-apl:          [PASS][19] -> [DMESG-WARN][20] ([i915#180])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-apl7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-apl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][21] -> [FAIL][22] ([fdo#108145] / [i915#265]) +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109441]) +5 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-iclb8/igt@kms_psr@psr2_cursor_mmap_cpu.html

  * igt@perf@short-reads:
    - shard-apl:          [PASS][25] -> [FAIL][26] ([i915#51])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-apl1/igt@perf@short-reads.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-apl6/igt@perf@short-reads.html

  
#### Possible fixes ####

  * {igt@gem_ctx_isolation@preservation-s3@vcs0}:
    - shard-apl:          [DMESG-WARN][27] ([i915#180]) -> [PASS][28] +1 similar issue
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-apl1/igt@gem_ctx_isolation@preservation-s3@vcs0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-apl2/igt@gem_ctx_isolation@preservation-s3@vcs0.html

  * igt@gem_ctx_persistence@processes:
    - shard-tglb:         [FAIL][29] ([i915#1528]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-tglb1/igt@gem_ctx_persistence@processes.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-tglb8/igt@gem_ctx_persistence@processes.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [SKIP][31] ([fdo#109349]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-iclb8/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * {igt@kms_flip@flip-vs-expired-vblank-interruptible@b-edp1}:
    - shard-skl:          [FAIL][33] ([i915#46]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-skl5/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-edp1.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-skl6/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-edp1.html

  * {igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a2}:
    - shard-glk:          [FAIL][35] ([i915#79]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-glk8/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a2.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-glk4/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a2.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-iclb:         [INCOMPLETE][37] ([i915#1185] / [i915#250]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-iclb3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-iclb5/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-kbl:          [FAIL][39] ([fdo#108145] / [i915#265] / [i915#93] / [i915#95]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-kbl3/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-kbl3/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_psr@psr2_primary_render:
    - shard-iclb:         [SKIP][41] ([fdo#109441]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-iclb3/igt@kms_psr@psr2_primary_render.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-iclb2/igt@kms_psr@psr2_primary_render.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][43] ([i915#31] / [i915#93] / [i915#95]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-kbl4/igt@kms_setmode@basic.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-kbl7/igt@kms_setmode@basic.html

  
#### Warnings ####

  * igt@i915_pm_dc@dc6-dpms:
    - shard-tglb:         [SKIP][45] ([i915#468]) -> [FAIL][46] ([i915#454])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-tglb2/igt@i915_pm_dc@dc6-dpms.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-tglb6/igt@i915_pm_dc@dc6-dpms.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][47] ([fdo#109642] / [fdo#111068]) -> [FAIL][48] ([i915#608])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-iclb4/igt@kms_psr2_su@page_flip.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_setmode@basic:
    - shard-apl:          [FAIL][49] ([i915#31]) -> [FAIL][50] ([i915#31] / [i915#95])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-apl8/igt@kms_setmode@basic.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-apl1/igt@kms_setmode@basic.html

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-tglb:         [INCOMPLETE][51] ([i915#456]) -> [INCOMPLETE][52] ([i915#402] / [i915#456])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8311/shard-tglb7/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/shard-tglb7/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#1185]: https://gitlab.freedesktop.org/drm/intel/issues/1185
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#151]: https://gitlab.freedesktop.org/drm/intel/issues/151
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#183]: https://gitlab.freedesktop.org/drm/intel/issues/183
  [i915#250]: https://gitlab.freedesktop.org/drm/intel/issues/250
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#456]: https://gitlab.freedesktop.org/drm/intel/issues/456
  [i915#46]: https://gitlab.freedesktop.org/drm/intel/issues/46
  [i915#468]: https://gitlab.freedesktop.org/drm/intel/issues/468
  [i915#51]: https://gitlab.freedesktop.org/drm/intel/issues/51
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#608]: https://gitlab.freedesktop.org/drm/intel/issues/608
  [i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8311 -> Patchwork_17331

  CI-20190529: 20190529
  CI_DRM_8311: 19367bb5e65eaf0719597b3ff244fd1c2ea12bda @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5593: 1c658f5e46598ae93345177d4981ef54704daec6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17331: a4e87db6ffcae3e28a42d5072f14a1a1ce5733df @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17331/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx