[PATCH 1/5] drm: Don't leak fb when plane crtc coodinates are bad

[PATCH 1/5] drm: Don't leak fb when plane crtc coodinates are bad

[ville.syrjala]

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

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e7c8422..66233a8 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2342,7 +2342,8 @@ static int __setplane_internal(struct drm_plane *plane,
 	    crtc_y > INT_MAX - (int32_t) crtc_h) {
 		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
 			      crtc_w, crtc_h, crtc_x, crtc_y);
-		return -ERANGE;
+		ret = -ERANGE;
+		goto out;
 	}
 
 
-- 
2.4.9

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

[PATCH 2/5] drm: Swap w/h when converting the mode to src coordidates for a rotated primary plane

[ville.syrjala]

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

When converting the mode hdisplay/vdisplay to primary plane src
coordinates we need to take into account the current plane
rotation.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 87a2a44..0c6f621 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1790,8 +1790,13 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 	primary_state->crtc_w = set->mode->hdisplay;
 	primary_state->src_x = set->x << 16;
 	primary_state->src_y = set->y << 16;
-	primary_state->src_h = set->mode->vdisplay << 16;
-	primary_state->src_w = set->mode->hdisplay << 16;
+	if (primary_state->rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270))) {
+		primary_state->src_h = set->mode->hdisplay << 16;
+		primary_state->src_w = set->mode->vdisplay << 16;
+	} else {
+		primary_state->src_h = set->mode->vdisplay << 16;
+		primary_state->src_w = set->mode->hdisplay << 16;
+	}
 
 commit:
 	ret = update_output_state(state, set);
-- 
2.4.9

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

[PATCH 3/5] drm: Refactor plane src coordinate checks

[ville.syrjala]

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

Pull the plane src coordinate checks into a separate function so that we
can share them for the legacy and new stuff.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 59 +++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 66233a8..227613a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2286,6 +2286,32 @@ int drm_plane_check_pixel_format(const struct drm_plane *plane, u32 format)
 	return -EINVAL;
 }
 
+static int check_src_coords(uint32_t src_x, uint32_t src_y,
+			    uint32_t src_w, uint32_t src_h,
+			    const struct drm_framebuffer *fb)
+{
+	unsigned int fb_width, fb_height;
+
+	fb_width = fb->width << 16;
+	fb_height = fb->height << 16;
+
+	/* Make sure source coordinates are inside the fb. */
+	if (src_w > fb_width ||
+	    src_x > fb_width - src_w ||
+	    src_h > fb_height ||
+	    src_y > fb_height - src_h) {
+		DRM_DEBUG_KMS("Invalid source coordinates "
+			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
+			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
+			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
+			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
+			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
 /*
  * setplane_internal - setplane handler for internal callers
  *
@@ -2305,7 +2331,6 @@ static int __setplane_internal(struct drm_plane *plane,
 			       uint32_t src_w, uint32_t src_h)
 {
 	int ret = 0;
-	unsigned int fb_width, fb_height;
 
 	/* No fb means shut it down */
 	if (!fb) {
@@ -2346,24 +2371,9 @@ static int __setplane_internal(struct drm_plane *plane,
 		goto out;
 	}
 
-
-	fb_width = fb->width << 16;
-	fb_height = fb->height << 16;
-
-	/* Make sure source coordinates are inside the fb. */
-	if (src_w > fb_width ||
-	    src_x > fb_width - src_w ||
-	    src_h > fb_height ||
-	    src_y > fb_height - src_h) {
-		DRM_DEBUG_KMS("Invalid source coordinates "
-			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
-			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
-			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
-			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
-			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
-		ret = -ENOSPC;
+	ret = check_src_coords(src_x, src_y, src_w, src_h, fb);
+	if (ret)
 		goto out;
-	}
 
 	plane->old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
@@ -2557,17 +2567,8 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 	if (crtc->invert_dimensions)
 		swap(hdisplay, vdisplay);
 
-	if (hdisplay > fb->width ||
-	    vdisplay > fb->height ||
-	    x > fb->width - hdisplay ||
-	    y > fb->height - vdisplay) {
-		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n",
-			      fb->width, fb->height, hdisplay, vdisplay, x, y,
-			      crtc->invert_dimensions ? " (inverted)" : "");
-		return -ENOSPC;
-	}
-
-	return 0;
+	return check_src_coords(x << 16, y << 16,
+				hdisplay << 16, vdisplay << 16, fb);
 }
 EXPORT_SYMBOL(drm_crtc_check_viewport);
 
-- 
2.4.9

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

[PATCH 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[ville.syrjala]

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

On atomic drivers we can dig out the primary plane rotation from the
plane state instead of looking at the legacy crtc->invert_dimensions
flag. The flag is not set by anyone except omapdrm, and it would be
racy to set it the same way in the atomic helpers.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 227613a..ffaa3f5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2545,6 +2545,16 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
 }
 EXPORT_SYMBOL(drm_crtc_get_hv_timing);
 
+static bool invert_dimensions(const struct drm_crtc *crtc)
+{
+	if (crtc->state) {
+		return crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
+							 BIT(DRM_ROTATE_270));
+	} else {
+		return crtc->invert_dimensions;
+	}
+}
+
 /**
  * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
  *     CRTC viewport
@@ -2564,7 +2574,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 
 	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
 
-	if (crtc->invert_dimensions)
+	if (invert_dimensions(crtc))
 		swap(hdisplay, vdisplay);
 
 	return check_src_coords(x << 16, y << 16,
-- 
2.4.9

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

Re: [PATCH 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[Matt Roper]

On Thu, Oct 15, 2015 at 08:40:01PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On atomic drivers we can dig out the primary plane rotation from the
> plane state instead of looking at the legacy crtc->invert_dimensions
> flag. The flag is not set by anyone except omapdrm, and it would be
> racy to set it the same way in the atomic helpers.

Can't we just remove the invert_dimensions field completely now?  It's a
legacy-only field, but no legacy drivers actually set it, so it winds up
being completely unused.


Matt

> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 227613a..ffaa3f5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2545,6 +2545,16 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
>  }
>  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
>  
> +static bool invert_dimensions(const struct drm_crtc *crtc)
> +{
> +	if (crtc->state) {
> +		return crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
> +							 BIT(DRM_ROTATE_270));
> +	} else {
> +		return crtc->invert_dimensions;
> +	}
> +}
> +
>  /**
>   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
>   *     CRTC viewport
> @@ -2564,7 +2574,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>  
>  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
>  
> -	if (crtc->invert_dimensions)
> +	if (invert_dimensions(crtc))
>  		swap(hdisplay, vdisplay);
>  
>  	return check_src_coords(x << 16, y << 16,
> -- 
> 2.4.9
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[Ville Syrjälä]

On Thu, Oct 15, 2015 at 05:32:12PM -0700, Matt Roper wrote:
> On Thu, Oct 15, 2015 at 08:40:01PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On atomic drivers we can dig out the primary plane rotation from the
> > plane state instead of looking at the legacy crtc->invert_dimensions
> > flag. The flag is not set by anyone except omapdrm, and it would be
> > racy to set it the same way in the atomic helpers.
> 
> Can't we just remove the invert_dimensions field completely now?  It's a
> legacy-only field, but no legacy drivers actually set it, so it winds up
> being completely unused.

omap sets it.

> 
> 
> Matt
> 
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 227613a..ffaa3f5 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2545,6 +2545,16 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> >  }
> >  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> >  
> > +static bool invert_dimensions(const struct drm_crtc *crtc)
> > +{
> > +	if (crtc->state) {
> > +		return crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
> > +							 BIT(DRM_ROTATE_270));
> > +	} else {
> > +		return crtc->invert_dimensions;
> > +	}
> > +}
> > +
> >  /**
> >   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
> >   *     CRTC viewport
> > @@ -2564,7 +2574,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> >  
> >  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> >  
> > -	if (crtc->invert_dimensions)
> > +	if (invert_dimensions(crtc))
> >  		swap(hdisplay, vdisplay);
> >  
> >  	return check_src_coords(x << 16, y << 16,
> > -- 
> > 2.4.9
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[Daniel Vetter]

On Fri, Oct 16, 2015 at 11:38:18AM +0300, Ville Syrjälä wrote:
> On Thu, Oct 15, 2015 at 05:32:12PM -0700, Matt Roper wrote:
> > On Thu, Oct 15, 2015 at 08:40:01PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On atomic drivers we can dig out the primary plane rotation from the
> > > plane state instead of looking at the legacy crtc->invert_dimensions
> > > flag. The flag is not set by anyone except omapdrm, and it would be
> > > racy to set it the same way in the atomic helpers.
> > 
> > Can't we just remove the invert_dimensions field completely now?  It's a
> > legacy-only field, but no legacy drivers actually set it, so it winds up
> > being completely unused.
> 
> omap sets it.

Omap should be atomic now (maybe double-check with Laurent), so I think we
can indeed ditch it. Follow-up series perhaps?
-Daniel

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 227613a..ffaa3f5 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -2545,6 +2545,16 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> > >  }
> > >  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> > >  
> > > +static bool invert_dimensions(const struct drm_crtc *crtc)
> > > +{
> > > +	if (crtc->state) {
> > > +		return crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
> > > +							 BIT(DRM_ROTATE_270));
> > > +	} else {
> > > +		return crtc->invert_dimensions;
> > > +	}
> > > +}
> > > +
> > >  /**
> > >   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
> > >   *     CRTC viewport
> > > @@ -2564,7 +2574,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> > >  
> > >  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> > >  
> > > -	if (crtc->invert_dimensions)
> > > +	if (invert_dimensions(crtc))
> > >  		swap(hdisplay, vdisplay);
> > >  
> > >  	return check_src_coords(x << 16, y << 16,
> > > -- 
> > > 2.4.9
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[Ville Syrjälä]

On Fri, Oct 16, 2015 at 04:35:02PM +0200, Daniel Vetter wrote:
> On Fri, Oct 16, 2015 at 11:38:18AM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 15, 2015 at 05:32:12PM -0700, Matt Roper wrote:
> > > On Thu, Oct 15, 2015 at 08:40:01PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > On atomic drivers we can dig out the primary plane rotation from the
> > > > plane state instead of looking at the legacy crtc->invert_dimensions
> > > > flag. The flag is not set by anyone except omapdrm, and it would be
> > > > racy to set it the same way in the atomic helpers.
> > > 
> > > Can't we just remove the invert_dimensions field completely now?  It's a
> > > legacy-only field, but no legacy drivers actually set it, so it winds up
> > > being completely unused.
> > 
> > omap sets it.
> 
> Omap should be atomic now (maybe double-check with Laurent), so I think we
> can indeed ditch it. Follow-up series perhaps?

grep ATOMIC in omap didn't turn up anything, so I assumed no.

> -Daniel
> 
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > 
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > > index 227613a..ffaa3f5 100644
> > > > --- a/drivers/gpu/drm/drm_crtc.c
> > > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > @@ -2545,6 +2545,16 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> > > >  }
> > > >  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> > > >  
> > > > +static bool invert_dimensions(const struct drm_crtc *crtc)
> > > > +{
> > > > +	if (crtc->state) {
> > > > +		return crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
> > > > +							 BIT(DRM_ROTATE_270));
> > > > +	} else {
> > > > +		return crtc->invert_dimensions;
> > > > +	}
> > > > +}
> > > > +
> > > >  /**
> > > >   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
> > > >   *     CRTC viewport
> > > > @@ -2564,7 +2574,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> > > >  
> > > >  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> > > >  
> > > > -	if (crtc->invert_dimensions)
> > > > +	if (invert_dimensions(crtc))
> > > >  		swap(hdisplay, vdisplay);
> > > >  
> > > >  	return check_src_coords(x << 16, y << 16,
> > > > -- 
> > > > 2.4.9
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[Matt Roper]

On Fri, Oct 16, 2015 at 06:10:20PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 16, 2015 at 04:35:02PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 16, 2015 at 11:38:18AM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 15, 2015 at 05:32:12PM -0700, Matt Roper wrote:
> > > > On Thu, Oct 15, 2015 at 08:40:01PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > On atomic drivers we can dig out the primary plane rotation from the
> > > > > plane state instead of looking at the legacy crtc->invert_dimensions
> > > > > flag. The flag is not set by anyone except omapdrm, and it would be
> > > > > racy to set it the same way in the atomic helpers.
> > > > 
> > > > Can't we just remove the invert_dimensions field completely now?  It's a
> > > > legacy-only field, but no legacy drivers actually set it, so it winds up
> > > > being completely unused.
> > > 
> > > omap sets it.
> > 
> > Omap should be atomic now (maybe double-check with Laurent), so I think we
> > can indeed ditch it. Follow-up series perhaps?
> 
> grep ATOMIC in omap didn't turn up anything, so I assumed no.

omap might not be full atomic (just like i915 isn't full atomic), but
it's state-based now, so your new code below will cause the
invert_dimensions to be ignored by the DRM core.  And omap never uses
the field internally, aside from setting it once to reflect the state
values, so as far as I can see, the field is completely unused.


Matt

> 
> > -Daniel
> > 
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > 
> > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_crtc.c | 12 +++++++++++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > > > index 227613a..ffaa3f5 100644
> > > > > --- a/drivers/gpu/drm/drm_crtc.c
> > > > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > > > @@ -2545,6 +2545,16 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode,
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_crtc_get_hv_timing);
> > > > >  
> > > > > +static bool invert_dimensions(const struct drm_crtc *crtc)
> > > > > +{
> > > > > +	if (crtc->state) {
> > > > > +		return crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
> > > > > +							 BIT(DRM_ROTATE_270));
> > > > > +	} else {
> > > > > +		return crtc->invert_dimensions;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * drm_crtc_check_viewport - Checks that a framebuffer is big enough for the
> > > > >   *     CRTC viewport
> > > > > @@ -2564,7 +2574,7 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> > > > >  
> > > > >  	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
> > > > >  
> > > > > -	if (crtc->invert_dimensions)
> > > > > +	if (invert_dimensions(crtc))
> > > > >  		swap(hdisplay, vdisplay);
> > > > >  
> > > > >  	return check_src_coords(x << 16, y << 16,
> > > > > -- 
> > > > > 2.4.9
> > > > > 
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/5] drm: Check crtc viewport correctly with rotated primary plane on atomic drivers

[ville.syrjala]

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

On atomic drivers we can dig out the primary plane rotation from the
plane state instead of looking at the legacy crtc->invert_dimensions
flag. The flag is not set by anyone except omapdrm, and it would be
racy to set it the same way in the atomic helpers.

v2: Kill crtc->invert_dimensions totally since omap is state based
    already and no one else ever used it (Matt)

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c          | 5 +++--
 drivers/gpu/drm/omapdrm/omap_crtc.c | 3 ---
 include/drm/drm_crtc.h              | 5 -----
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 227613a..a7738be 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -677,7 +677,6 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 
 	crtc->dev = dev;
 	crtc->funcs = funcs;
-	crtc->invert_dimensions = false;
 
 	drm_modeset_lock_init(&crtc->mutex);
 	ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
@@ -2564,7 +2563,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
 
 	drm_crtc_get_hv_timing(mode, &hdisplay, &vdisplay);
 
-	if (crtc->invert_dimensions)
+	if (crtc->state &&
+	    crtc->primary->state->rotation & (BIT(DRM_ROTATE_90) |
+					      BIT(DRM_ROTATE_270)))
 		swap(hdisplay, vdisplay);
 
 	return check_src_coords(x << 16, y << 16,
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 9a4ba4f..ad09590 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -412,9 +412,6 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 		dispc_mgr_go(omap_crtc->channel);
 		omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
 	}
-
-	crtc->invert_dimensions = !!(crtc->primary->state->rotation &
-				    (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270)));
 }
 
 static int omap_crtc_atomic_set_property(struct drm_crtc *crtc,
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 317baf9..028e5d4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -407,9 +407,6 @@ struct drm_crtc_funcs {
  * @enabled: is this CRTC enabled?
  * @mode: current mode timings
  * @hwmode: mode timings as programmed to hw regs
- * @invert_dimensions: for purposes of error checking crtc vs fb sizes,
- *    invert the width/height of the crtc.  This is used if the driver
- *    is performing 90 or 270 degree rotated scanout
  * @x: x position on screen
  * @y: y position on screen
  * @funcs: CRTC control functions
@@ -458,8 +455,6 @@ struct drm_crtc {
 	 */
 	struct drm_display_mode hwmode;
 
-	bool invert_dimensions;
-
 	int x, y;
 	const struct drm_crtc_funcs *funcs;
 
-- 
2.4.9

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

[PATCH 5/5] drm: Check plane src coordinates correctly during page flip for atomic drivers

[ville.syrjala]

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

Instead of relying on the old crtc-{x,y,mode} gunk, dig out the primary
plane coordinates from the plane state when checking them against the
new framebuffer during page flip.

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ffaa3f5..52feffc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5193,7 +5193,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
+	if (crtc->state) {
+		const struct drm_plane_state *state = crtc->primary->state;
+
+		ret = check_src_coords(state->src_x, state->src_y,
+				       state->src_w, state->src_h, fb);
+	} else {
+		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
+	}
 	if (ret)
 		goto out;
 
-- 
2.4.9

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

Re: [PATCH 5/5] drm: Check plane src coordinates correctly during page flip for atomic drivers

[Matt Roper]

On Thu, Oct 15, 2015 at 08:40:02PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Instead of relying on the old crtc-{x,y,mode} gunk, dig out the primary
> plane coordinates from the plane state when checking them against the
> new framebuffer during page flip.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

For the series:

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I also confirmed that the i-g-t test I wrote here:
   http://lists.freedesktop.org/archives/intel-gfx/2015-October/077394.html
now passes with your patch series, so I believe Tvrtko's original bug
report should be fixed.


Matt

> ---
>  drivers/gpu/drm/drm_crtc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index ffaa3f5..52feffc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5193,7 +5193,14 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> +	if (crtc->state) {
> +		const struct drm_plane_state *state = crtc->primary->state;
> +
> +		ret = check_src_coords(state->src_x, state->src_y,
> +				       state->src_w, state->src_h, fb);
> +	} else {
> +		ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> +	}
>  	if (ret)
>  		goto out;
>  
> -- 
> 2.4.9
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm: Check plane src coordinates correctly during page flip for atomic drivers

[Tvrtko Ursulin]

On 16/10/15 17:27, Matt Roper wrote:
> On Thu, Oct 15, 2015 at 08:40:02PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Instead of relying on the old crtc-{x,y,mode} gunk, dig out the primary
>> plane coordinates from the plane state when checking them against the
>> new framebuffer during page flip.
>>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> For the series:
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> I also confirmed that the i-g-t test I wrote here:
>     http://lists.freedesktop.org/archives/intel-gfx/2015-October/077394.html
> now passes with your patch series, so I believe Tvrtko's original bug
> report should be fixed.

Oh I did not realize this series is about this, perhaps because I did 
not see 1/5 which maybe had some more obvious clues. :)

Great, that means if we decide to merge "drm/i915: Consider plane 
rotation when calculating stride in skl_do_mmio_flip" and 
"kms_rotation_crc: Exercise page flips with 90 degree rotation" we would 
have working rotated legacy page flip with a test case.

Regards,

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

Re: [PATCH 5/5] drm: Check plane src coordinates correctly during page flip for atomic drivers

[Daniel Vetter]

On Fri, Oct 16, 2015 at 05:40:59PM +0100, Tvrtko Ursulin wrote:
> 
> On 16/10/15 17:27, Matt Roper wrote:
> >On Thu, Oct 15, 2015 at 08:40:02PM +0300, ville.syrjala@linux.intel.com wrote:
> >>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >>Instead of relying on the old crtc-{x,y,mode} gunk, dig out the primary
> >>plane coordinates from the plane state when checking them against the
> >>new framebuffer during page flip.
> >>
> >>Cc: Matt Roper <matthew.d.roper@intel.com>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>Cc: Daniel Vetter <daniel@ffwll.ch>
> >>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >For the series:
> >
> >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Pulled all 5 into drm-misc, thanks.

> >I also confirmed that the i-g-t test I wrote here:
> >    http://lists.freedesktop.org/archives/intel-gfx/2015-October/077394.html
> >now passes with your patch series, so I believe Tvrtko's original bug
> >report should be fixed.
> 
> Oh I did not realize this series is about this, perhaps because I did not
> see 1/5 which maybe had some more obvious clues. :)
> 
> Great, that means if we decide to merge "drm/i915: Consider plane rotation
> when calculating stride in skl_do_mmio_flip" and "kms_rotation_crc: Exercise
> page flips with 90 degree rotation" we would have working rotated legacy
> page flip with a test case.

Matt, can you please push these igt patches and double-check that we have
subtests to catch the viewport confusion here?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm: Check plane src coordinates correctly during page flip for atomic drivers

[Matt Roper]

On Fri, Oct 16, 2015 at 07:17:31PM +0200, Daniel Vetter wrote:
> On Fri, Oct 16, 2015 at 05:40:59PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 16/10/15 17:27, Matt Roper wrote:
> > >On Thu, Oct 15, 2015 at 08:40:02PM +0300, ville.syrjala@linux.intel.com wrote:
> > >>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>
> > >>Instead of relying on the old crtc-{x,y,mode} gunk, dig out the primary
> > >>plane coordinates from the plane state when checking them against the
> > >>new framebuffer during page flip.
> > >>
> > >>Cc: Matt Roper <matthew.d.roper@intel.com>
> > >>Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > >>Cc: Daniel Vetter <daniel@ffwll.ch>
> > >>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > >For the series:
> > >
> > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Pulled all 5 into drm-misc, thanks.
> 
> > >I also confirmed that the i-g-t test I wrote here:
> > >    http://lists.freedesktop.org/archives/intel-gfx/2015-October/077394.html
> > >now passes with your patch series, so I believe Tvrtko's original bug
> > >report should be fixed.
> > 
> > Oh I did not realize this series is about this, perhaps because I did not
> > see 1/5 which maybe had some more obvious clues. :)
> > 
> > Great, that means if we decide to merge "drm/i915: Consider plane rotation
> > when calculating stride in skl_do_mmio_flip" and "kms_rotation_crc: Exercise
> > page flips with 90 degree rotation" we would have working rotated legacy
> > page flip with a test case.
> 
> Matt, can you please push these igt patches and double-check that we have
> subtests to catch the viewport confusion here?
> 
> Thanks, Daniel

Looks like Thomas already beat me to pushing the relevant i-g-t patches
(both the one I wrote and the one Tvrtko wrote), so I think we're
covered there.

Thanks.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx