[PATCH] drm/i915: Reject NV12 planes with odd width/start position

[PATCH] drm/i915: Reject NV12 planes with odd width/start position

[Fritz Koenig]

Planes with an odd width will appear to have an incorrect
stride. When the start position is odd the controller
can lock up.

Signed-off-by: Fritz Koenig <frkoenig@google.com>
---

Hi,

This appears to be a limitation of the hardware that is not being
checked. Is this supported and am I not enabling it correctly?


 drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7481ce85746b..ca4553592ab9 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -188,6 +188,21 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
 	else
 		crtc_state->active_planes &= ~BIT(intel_plane->id);
 
+	/*
+	 * NV12 plane is not allowed to start from an odd position or
+	 * end on an odd position.
+	 */
+	if (state->fb && (DRM_FORMAT_NV12 == state->fb->format->format)) {
+		if ((intel_state->base.src_w >> 16) & 1) {
+			DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd width\n");
+			return -EINVAL;
+		}
+		if ((intel_state->base.src_x >> 16) & 1) {
+			DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd x pos\n");
+			return -EINVAL;
+		}
+	}
+
 	return intel_plane_atomic_calc_changes(old_crtc_state,
 					       &crtc_state->base,
 					       old_plane_state,
-- 
2.17.0.441.gb46fe60e1d-goog

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

✗ Fi.CI.BAT: failure for drm/i915: Reject NV12 planes with odd width/start position

[Patchwork]

== Series Details ==

Series: drm/i915: Reject NV12 planes with odd width/start position
URL   : https://patchwork.freedesktop.org/series/43403/
State : failure

== Summary ==

Applying: drm/i915: Reject NV12 planes with odd width/start position
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_atomic_plane.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_atomic_plane.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_atomic_plane.c
Patch failed at 0001 drm/i915: Reject NV12 planes with odd width/start position
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH] drm/i915: Reject NV12 planes with odd width/start position

[Ville Syrjälä]

On Thu, May 17, 2018 at 12:07:14PM -0700, Fritz Koenig wrote:
> Planes with an odd width will appear to have an incorrect
> stride. When the start position is odd the controller
> can lock up.

Just remove the strange NV12 check from the %2 checks in
intel_check_sprite_plane()?

> 
> Signed-off-by: Fritz Koenig <frkoenig@google.com>
> ---
> 
> Hi,
> 
> This appears to be a limitation of the hardware that is not being
> checked. Is this supported and am I not enabling it correctly?
> 
> 
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7481ce85746b..ca4553592ab9 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -188,6 +188,21 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>  	else
>  		crtc_state->active_planes &= ~BIT(intel_plane->id);
>  
> +	/*
> +	 * NV12 plane is not allowed to start from an odd position or
> +	 * end on an odd position.
> +	 */
> +	if (state->fb && (DRM_FORMAT_NV12 == state->fb->format->format)) {
> +		if ((intel_state->base.src_w >> 16) & 1) {
> +			DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd width\n");
> +			return -EINVAL;
> +		}
> +		if ((intel_state->base.src_x >> 16) & 1) {
> +			DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd x pos\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return intel_plane_atomic_calc_changes(old_crtc_state,
>  					       &crtc_state->base,
>  					       old_plane_state,
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Reject NV12 planes with odd width/start position

[Ville Syrjälä]

On Fri, May 18, 2018 at 03:25:26PM +0300, Ville Syrjälä wrote:
> On Thu, May 17, 2018 at 12:07:14PM -0700, Fritz Koenig wrote:
> > Planes with an odd width will appear to have an incorrect
> > stride. When the start position is odd the controller
> > can lock up.
> 
> Just remove the strange NV12 check from the %2 checks in
> intel_check_sprite_plane()?

Hmm. Actually that wouldn't help the "primary" plane. I guess we want to
put this check into skl_check_nv12_surface() until we have a better
place for it, or until someone fixes the initial phase stuff to actually
handle this correctly.

> 
> > 
> > Signed-off-by: Fritz Koenig <frkoenig@google.com>
> > ---
> > 
> > Hi,
> > 
> > This appears to be a limitation of the hardware that is not being
> > checked. Is this supported and am I not enabling it correctly?
> > 
> > 
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 7481ce85746b..ca4553592ab9 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -188,6 +188,21 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >  	else
> >  		crtc_state->active_planes &= ~BIT(intel_plane->id);
> >  
> > +	/*
> > +	 * NV12 plane is not allowed to start from an odd position or
> > +	 * end on an odd position.
> > +	 */
> > +	if (state->fb && (DRM_FORMAT_NV12 == state->fb->format->format)) {
> > +		if ((intel_state->base.src_w >> 16) & 1) {
> > +			DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd width\n");
> > +			return -EINVAL;
> > +		}
> > +		if ((intel_state->base.src_x >> 16) & 1) {
> > +			DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd x pos\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	return intel_plane_atomic_calc_changes(old_crtc_state,
> >  					       &crtc_state->base,
> >  					       old_plane_state,
> > -- 
> > 2.17.0.441.gb46fe60e1d-goog
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel

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

Re: [PATCH] drm/i915: Reject NV12 planes with odd width/start position

[Fritz Koenig]

On Fri, May 18, 2018 at 7:15 AM Ville Syrjälä
<ville.syrjala@linux.intel.com>
wrote:

> On Fri, May 18, 2018 at 03:25:26PM +0300, Ville Syrjälä wrote:
> > On Thu, May 17, 2018 at 12:07:14PM -0700, Fritz Koenig wrote:
> > > Planes with an odd width will appear to have an incorrect
> > > stride. When the start position is odd the controller
> > > can lock up.
> >
> > Just remove the strange NV12 check from the %2 checks in
> > intel_check_sprite_plane()?

> Hmm. Actually that wouldn't help the "primary" plane. I guess we want to
> put this check into skl_check_nv12_surface() until we have a better
> place for it, or until someone fixes the initial phase stuff to actually
> handle this correctly.


Ahh, yes, it looks like we are hitting this now because we are using the
NV12 plane
as the primary plane, which forgoes all the NV12 checks that occur in
intel_check_sprite_plane.  This patch is probably the wrong place to do it
then.

> >
> > >
> > > Signed-off-by: Fritz Koenig <frkoenig@google.com>
> > > ---
> > >
> > > Hi,
> > >
> > > This appears to be a limitation of the hardware that is not being
> > > checked. Is this supported and am I not enabling it correctly?
> > >
> > >
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > index 7481ce85746b..ca4553592ab9 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > > @@ -188,6 +188,21 @@ int intel_plane_atomic_check_with_state(const
struct intel_crtc_state *old_crtc_
> > >     else
> > >             crtc_state->active_planes &= ~BIT(intel_plane->id);
> > >
> > > +   /*
> > > +    * NV12 plane is not allowed to start from an odd position or
> > > +    * end on an odd position.
> > > +    */
> > > +   if (state->fb && (DRM_FORMAT_NV12 == state->fb->format->format)) {
> > > +           if ((intel_state->base.src_w >> 16) & 1) {
> > > +                   DRM_DEBUG_KMS("Invalid Source: Yuv format does
not support odd width\n");
> > > +                   return -EINVAL;
> > > +           }
> > > +           if ((intel_state->base.src_x >> 16) & 1) {
> > > +                   DRM_DEBUG_KMS("Invalid Source: Yuv format does
not support odd x pos\n");
> > > +                   return -EINVAL;
> > > +           }
> > > +   }
> > > +
> > >     return intel_plane_atomic_calc_changes(old_crtc_state,
> > >                                            &crtc_state->base,
> > >                                            old_plane_state,
> > > --
> > > 2.17.0.441.gb46fe60e1d-goog
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel

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

Re: [PATCH] drm/i915: Reject NV12 planes with odd width/start position

[Chris Wilson]

Quoting Fritz Koenig (2018-05-17 20:07:14)
> Planes with an odd width will appear to have an incorrect
> stride. When the start position is odd the controller
> can lock up.
> 
> Signed-off-by: Fritz Koenig <frkoenig@google.com>
> ---
> 
> Hi,
> 
> This appears to be a limitation of the hardware that is not being
> checked. Is this supported and am I not enabling it correctly?
> 
> 
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7481ce85746b..ca4553592ab9 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -188,6 +188,21 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>         else
>                 crtc_state->active_planes &= ~BIT(intel_plane->id);
>  
> +       /*
> +        * NV12 plane is not allowed to start from an odd position or
> +        * end on an odd position.
> +        */
> +       if (state->fb && (DRM_FORMAT_NV12 == state->fb->format->format)) {
> +               if ((intel_state->base.src_w >> 16) & 1) {
> +                       DRM_DEBUG_KMS("Invalid Source: Yuv format does not support odd width\n");

It may seem obvious from the message that src_w is odd, but it's always
nice to include as much information as possible for debugging; in this
case "%x", src_w.

if ((intel_state->base.src_w | intel_state->base.src_x) & BIT(16)) ?

I'd leave that question of style to Ville though.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx