[PATCH 0/2] Adding rotattion to drm_plane_helper_check_update

[PATCH 0/2] Adding rotattion to drm_plane_helper_check_update

[Sonika Jindal]

This adds another parameter rotation to drm_plane_helper_check_update.
This will enable this function to do to size updations based upon the rotation
if any.
Updated the calls to this function in i915 and drm. Rockchip driver also needs
to be updated.

Sonika Jindal (2):
  drm: Adding rotation to drm_plane_helper_check_update
  drm/i915: Passing rotation to drm_plane_helper_check_update

 drivers/gpu/drm/drm_plane_helper.c   |   79 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |    6 ++-
 include/drm/drm_plane_helper.h       |    3 +-
 3 files changed, 81 insertions(+), 7 deletions(-)

-- 
1.7.10.4

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

[PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Sonika Jindal]

Taking rotation into account while checking the plane
and adjusting the sizes accordingly.

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
 include/drm/drm_plane_helper.h     |    3 +-
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index f24c4cf..4badd69 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 				    int max_scale,
 				    bool can_position,
 				    bool can_update_disabled,
-				    bool *visible)
+				    bool *visible,
+				    unsigned int rotation)
 {
 	int hscale, vscale;
+	int crtc_x, crtc_y;
+	unsigned int crtc_w, crtc_h;
+	uint32_t src_x, src_y, src_w, src_h;
 
 	if (!fb) {
 		*visible = false;
@@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	if (fb)
+		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
+				rotation);
+
 	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale);
 	if (hscale < 0 || vscale < 0) {
 		DRM_DEBUG_KMS("Invalid scaling of plane\n");
 		return -ERANGE;
@@ -182,6 +190,68 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	crtc_x = dest->x1;
+	crtc_y = dest->y1;
+	crtc_w = drm_rect_width(dest);
+	crtc_h = drm_rect_height(dest);
+
+	if (*visible) {
+		/* check again in case clipping clamped the results */
+		hscale = drm_rect_calc_hscale(src, dest,
+						DRM_PLANE_HELPER_NO_SCALING,
+						DRM_PLANE_HELPER_NO_SCALING);
+		if (hscale < 0) {
+			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
+			drm_rect_debug_print(src, true);
+			drm_rect_debug_print(dest, false);
+
+			return hscale;
+		}
+
+		vscale = drm_rect_calc_vscale(src, dest,
+						DRM_PLANE_HELPER_NO_SCALING,
+						DRM_PLANE_HELPER_NO_SCALING);
+		if (vscale < 0) {
+			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
+			drm_rect_debug_print(src, true);
+			drm_rect_debug_print(dest, false);
+
+			return vscale;
+		}
+
+		/* Make the source viewport size an exact multiple of the scaling factors. */
+		drm_rect_adjust_size(src,
+			drm_rect_width(dest) * hscale - drm_rect_width(src),
+			drm_rect_height(dest) * vscale - drm_rect_height(src));
+
+		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
+				    rotation);
+
+		/*
+		 * Hardware doesn't handle subpixel coordinates.
+		 * Adjust to (macro)pixel boundary, but be careful not to
+		 * increase the source viewport size, because that could
+		 * push the downscaling factor out of bounds.
+		 */
+		src_x = src->x1 >> 16;
+		src_w = drm_rect_width(src) >> 16;
+		src_y = src->y1 >> 16;
+		src_h = drm_rect_height(src) >> 16;
+	}
+
+	if (*visible) {
+		src->x1 = src_x;
+		src->x2 = src_x + src_w;
+		src->y1 = src_y;
+		src->y2 = src_y + src_h;
+	}
+
+	dest->x1 = crtc_x;
+	dest->x2 = crtc_x + crtc_w;
+	dest->y1 = crtc_y;
+	dest->y2 = crtc_y + crtc_h;
+
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_helper_check_update);
@@ -258,7 +328,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 					    &src, &dest, &clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, false, &visible);
+					    false, false, &visible,
+					    DRM_ROTATE_0);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index a185392..ef9eb04 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane,
 					 int max_scale,
 					 bool can_position,
 					 bool can_update_disabled,
-					 bool *visible);
+					 bool *visible,
+					 unsigned int rotation);
 extern int drm_primary_helper_update(struct drm_plane *plane,
 				     struct drm_crtc *crtc,
 				     struct drm_framebuffer *fb,
-- 
1.7.10.4

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

Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Ville Syrjälä]

On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> Taking rotation into account while checking the plane
> and adjusting the sizes accordingly.
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
>  include/drm/drm_plane_helper.h     |    3 +-
>  2 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index f24c4cf..4badd69 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>  				    int max_scale,
>  				    bool can_position,
>  				    bool can_update_disabled,
> -				    bool *visible)
> +				    bool *visible,
> +				    unsigned int rotation)
>  {
>  	int hscale, vscale;
> +	int crtc_x, crtc_y;
> +	unsigned int crtc_w, crtc_h;
> +	uint32_t src_x, src_y, src_w, src_h;
>  
>  	if (!fb) {
>  		*visible = false;
> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	if (fb)
> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> +				rotation);
> +
>  	/* Check scaling */
> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale);

This is an unrelated change. Relaxed scaling allows the the src/dest
rectangles to be reduced in size in order to keep the scaling ration
within the min/max range. I suppose we should switch to using it to
make the behaviour uniform across drivers, but definitely should be
done with a separate patch.

>  	if (hscale < 0 || vscale < 0) {
>  		DRM_DEBUG_KMS("Invalid scaling of plane\n");
>  		return -ERANGE;
> @@ -182,6 +190,68 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	crtc_x = dest->x1;
> +	crtc_y = dest->y1;
> +	crtc_w = drm_rect_width(dest);
> +	crtc_h = drm_rect_height(dest);

You don't adjust these in any way so they are not needed.

> +
> +	if (*visible) {
> +		/* check again in case clipping clamped the results */
> +		hscale = drm_rect_calc_hscale(src, dest,
> +						DRM_PLANE_HELPER_NO_SCALING,
> +						DRM_PLANE_HELPER_NO_SCALING);

First you allowed scaling and now you don't. What's up with that?

> +		if (hscale < 0) {
> +			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
> +			drm_rect_debug_print(src, true);
> +			drm_rect_debug_print(dest, false);
> +
> +			return hscale;
> +		}
> +
> +		vscale = drm_rect_calc_vscale(src, dest,
> +						DRM_PLANE_HELPER_NO_SCALING,
> +						DRM_PLANE_HELPER_NO_SCALING);
> +		if (vscale < 0) {
> +			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
> +			drm_rect_debug_print(src, true);
> +			drm_rect_debug_print(dest, false);
> +
> +			return vscale;
> +		}
> +
> +		/* Make the source viewport size an exact multiple of the scaling factors. */
> +		drm_rect_adjust_size(src,
> +			drm_rect_width(dest) * hscale - drm_rect_width(src),
> +			drm_rect_height(dest) * vscale - drm_rect_height(src));
> +
> +		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
> +				    rotation);
> +
> +		/*
> +		 * Hardware doesn't handle subpixel coordinates.

That's a purely hardware specific detail. It should not be part of the
helpers. And in any case the returned src coordinates must remain in
fixed point format.

> +		 * Adjust to (macro)pixel boundary, but be careful not to
> +		 * increase the source viewport size, because that could
> +		 * push the downscaling factor out of bounds.
> +		 */
> +		src_x = src->x1 >> 16;
> +		src_w = drm_rect_width(src) >> 16;
> +		src_y = src->y1 >> 16;
> +		src_h = drm_rect_height(src) >> 16;
> +	}
> +
> +	if (*visible) {
> +		src->x1 = src_x;
> +		src->x2 = src_x + src_w;
> +		src->y1 = src_y;
> +		src->y2 = src_y + src_h;
> +	}
> +
> +	dest->x1 = crtc_x;
> +	dest->x2 = crtc_x + crtc_w;
> +	dest->y1 = crtc_y;
> +	dest->y2 = crtc_y + crtc_h;
> +
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_helper_check_update);
> @@ -258,7 +328,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  					    &src, &dest, &clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> -					    false, false, &visible);
> +					    false, false, &visible,
> +					    DRM_ROTATE_0);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index a185392..ef9eb04 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane,
>  					 int max_scale,
>  					 bool can_position,
>  					 bool can_update_disabled,
> -					 bool *visible);
> +					 bool *visible,
> +					 unsigned int rotation);
>  extern int drm_primary_helper_update(struct drm_plane *plane,
>  				     struct drm_crtc *crtc,
>  				     struct drm_framebuffer *fb,
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[sonika]

On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
>> Taking rotation into account while checking the plane
>> and adjusting the sizes accordingly.
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
>>   include/drm/drm_plane_helper.h     |    3 +-
>>   2 files changed, 77 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
>> index f24c4cf..4badd69 100644
>> --- a/drivers/gpu/drm/drm_plane_helper.c
>> +++ b/drivers/gpu/drm/drm_plane_helper.c
>> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>>   				    int max_scale,
>>   				    bool can_position,
>>   				    bool can_update_disabled,
>> -				    bool *visible)
>> +				    bool *visible,
>> +				    unsigned int rotation)
>>   {
>>   	int hscale, vscale;
>> +	int crtc_x, crtc_y;
>> +	unsigned int crtc_w, crtc_h;
>> +	uint32_t src_x, src_y, src_w, src_h;
>>   
>>   	if (!fb) {
>>   		*visible = false;
>> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>>   		return -EINVAL;
>>   	}
>>   
>> +	if (fb)
>> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
>> +				rotation);
>> +
>>   	/* Check scaling */
>> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
>> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
>> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
>> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale);
> This is an unrelated change. Relaxed scaling allows the the src/dest
> rectangles to be reduced in size in order to keep the scaling ration
> within the min/max range. I suppose we should switch to using it to
> make the behaviour uniform across drivers, but definitely should be
> done with a separate patch.
Since, I added drm_rect_rotate before this, it changes the src sizes and 
it was giving me
Invalid scaling if we don't let the sizes to be changed using _relaxed 
functions. I am trying this
for 90/270 rotation. I can move it to a separate patch if required.
>>   	if (hscale < 0 || vscale < 0) {
>>   		DRM_DEBUG_KMS("Invalid scaling of plane\n");
>>   		return -ERANGE;
>> @@ -182,6 +190,68 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
>>   		return -EINVAL;
>>   	}
>>   
>> +	crtc_x = dest->x1;
>> +	crtc_y = dest->y1;
>> +	crtc_w = drm_rect_width(dest);
>> +	crtc_h = drm_rect_height(dest);
> You don't adjust these in any way so they are not needed.
>
>> +
>> +	if (*visible) {
>> +		/* check again in case clipping clamped the results */
>> +		hscale = drm_rect_calc_hscale(src, dest,
>> +						DRM_PLANE_HELPER_NO_SCALING,
>> +						DRM_PLANE_HELPER_NO_SCALING);
> First you allowed scaling and now you don't. What's up with that?
Hmm, so I can simply use min_scale and max_scale here.
>> +		if (hscale < 0) {
>> +			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
>> +			drm_rect_debug_print(src, true);
>> +			drm_rect_debug_print(dest, false);
>> +
>> +			return hscale;
>> +		}
>> +
>> +		vscale = drm_rect_calc_vscale(src, dest,
>> +						DRM_PLANE_HELPER_NO_SCALING,
>> +						DRM_PLANE_HELPER_NO_SCALING);
>> +		if (vscale < 0) {
>> +			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
>> +			drm_rect_debug_print(src, true);
>> +			drm_rect_debug_print(dest, false);
>> +
>> +			return vscale;
>> +		}
>> +
>> +		/* Make the source viewport size an exact multiple of the scaling factors. */
>> +		drm_rect_adjust_size(src,
>> +			drm_rect_width(dest) * hscale - drm_rect_width(src),
>> +			drm_rect_height(dest) * vscale - drm_rect_height(src));
>> +
>> +		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
>> +				    rotation);
>> +
>> +		/*
>> +		 * Hardware doesn't handle subpixel coordinates.
> That's a purely hardware specific detail. It should not be part of the
> helpers. And in any case the returned src coordinates must remain in
> fixed point format.
>
I was just trying to make it same as what we do in intel_check_sprite_plane.
So, looks like I can remove this one and the other part which you 
pointed above.
>> +		 * Adjust to (macro)pixel boundary, but be careful not to
>> +		 * increase the source viewport size, because that could
>> +		 * push the downscaling factor out of bounds.
>> +		 */
>> +		src_x = src->x1 >> 16;
>> +		src_w = drm_rect_width(src) >> 16;
>> +		src_y = src->y1 >> 16;
>> +		src_h = drm_rect_height(src) >> 16;
>> +	}
>> +
>> +	if (*visible) {
>> +		src->x1 = src_x;
>> +		src->x2 = src_x + src_w;
>> +		src->y1 = src_y;
>> +		src->y2 = src_y + src_h;
>> +	}
>> +
>> +	dest->x1 = crtc_x;
>> +	dest->x2 = crtc_x + crtc_w;
>> +	dest->y1 = crtc_y;
>> +	dest->y2 = crtc_y + crtc_h;
>> +
>> +
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL(drm_plane_helper_check_update);
>> @@ -258,7 +328,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>>   					    &src, &dest, &clip,
>>   					    DRM_PLANE_HELPER_NO_SCALING,
>>   					    DRM_PLANE_HELPER_NO_SCALING,
>> -					    false, false, &visible);
>> +					    false, false, &visible,
>> +					    DRM_ROTATE_0);
>>   	if (ret)
>>   		return ret;
>>   
>> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
>> index a185392..ef9eb04 100644
>> --- a/include/drm/drm_plane_helper.h
>> +++ b/include/drm/drm_plane_helper.h
>> @@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane,
>>   					 int max_scale,
>>   					 bool can_position,
>>   					 bool can_update_disabled,
>> -					 bool *visible);
>> +					 bool *visible,
>> +					 unsigned int rotation);
>>   extern int drm_primary_helper_update(struct drm_plane *plane,
>>   				     struct drm_crtc *crtc,
>>   				     struct drm_framebuffer *fb,
>> -- 
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Ville Syrjälä]

On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> 
> On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> >> Taking rotation into account while checking the plane
> >> and adjusting the sizes accordingly.
> >>
> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> >>   include/drm/drm_plane_helper.h     |    3 +-
> >>   2 files changed, 77 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> >> index f24c4cf..4badd69 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >>   				    int max_scale,
> >>   				    bool can_position,
> >>   				    bool can_update_disabled,
> >> -				    bool *visible)
> >> +				    bool *visible,
> >> +				    unsigned int rotation)
> >>   {
> >>   	int hscale, vscale;
> >> +	int crtc_x, crtc_y;
> >> +	unsigned int crtc_w, crtc_h;
> >> +	uint32_t src_x, src_y, src_w, src_h;
> >>   
> >>   	if (!fb) {
> >>   		*visible = false;
> >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> +	if (fb)
> >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> >> +				rotation);
> >> +
> >>   	/* Check scaling */
> >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale);
> > This is an unrelated change. Relaxed scaling allows the the src/dest
> > rectangles to be reduced in size in order to keep the scaling ration
> > within the min/max range. I suppose we should switch to using it to
> > make the behaviour uniform across drivers, but definitely should be
> > done with a separate patch.
> Since, I added drm_rect_rotate before this, it changes the src sizes and 
> it was giving me
> Invalid scaling if we don't let the sizes to be changed using _relaxed 
> functions. I am trying this
> for 90/270 rotation.

That would indicate a bug somewhere. Pontetially the bug could be in
whatever test you're using as well.

> I can move it to a separate patch if required.

We nee to figure out why you got the error first.

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

RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Jindal, Sonika]

We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed)
That's why I saw the need of this for primary plane as well. 
For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff
for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
With this change, the rotated plane is properly adjusted in the viewport.
So, I don't think it is a bug in test.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, January 14, 2015 2:58 PM
To: Jindal, Sonika
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> 
> On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> >> Taking rotation into account while checking the plane and adjusting 
> >> the sizes accordingly.
> >>
> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> >>   include/drm/drm_plane_helper.h     |    3 +-
> >>   2 files changed, 77 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> >> b/drivers/gpu/drm/drm_plane_helper.c
> >> index f24c4cf..4badd69 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >>   				    int max_scale,
> >>   				    bool can_position,
> >>   				    bool can_update_disabled,
> >> -				    bool *visible)
> >> +				    bool *visible,
> >> +				    unsigned int rotation)
> >>   {
> >>   	int hscale, vscale;
> >> +	int crtc_x, crtc_y;
> >> +	unsigned int crtc_w, crtc_h;
> >> +	uint32_t src_x, src_y, src_w, src_h;
> >>   
> >>   	if (!fb) {
> >>   		*visible = false;
> >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> +	if (fb)
> >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> >> +				rotation);
> >> +
> >>   	/* Check scaling */
> >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> >> +max_scale);
> > This is an unrelated change. Relaxed scaling allows the the src/dest 
> > rectangles to be reduced in size in order to keep the scaling ration 
> > within the min/max range. I suppose we should switch to using it to 
> > make the behaviour uniform across drivers, but definitely should be 
> > done with a separate patch.
> Since, I added drm_rect_rotate before this, it changes the src sizes 
> and it was giving me Invalid scaling if we don't let the sizes to be 
> changed using _relaxed functions. I am trying this for 90/270 
> rotation.

That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.

> I can move it to a separate patch if required.

We nee to figure out why you got the error first.

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

Re: [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Jindal, Sonika]

Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.
So, I think we _relaxed is the function which should be called to update the destination size appropriately.
Please correct me if I am wrong.

-----Original Message-----
From: Jindal, Sonika 
Sent: Wednesday, January 14, 2015 3:06 PM
To: 'Ville Syrjälä'
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
With this change, the rotated plane is properly adjusted in the viewport.
So, I don't think it is a bug in test.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
Sent: Wednesday, January 14, 2015 2:58 PM
To: Jindal, Sonika
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> 
> On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> >> Taking rotation into account while checking the plane and adjusting 
> >> the sizes accordingly.
> >>
> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> >>   include/drm/drm_plane_helper.h     |    3 +-
> >>   2 files changed, 77 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> >> b/drivers/gpu/drm/drm_plane_helper.c
> >> index f24c4cf..4badd69 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >>   				    int max_scale,
> >>   				    bool can_position,
> >>   				    bool can_update_disabled,
> >> -				    bool *visible)
> >> +				    bool *visible,
> >> +				    unsigned int rotation)
> >>   {
> >>   	int hscale, vscale;
> >> +	int crtc_x, crtc_y;
> >> +	unsigned int crtc_w, crtc_h;
> >> +	uint32_t src_x, src_y, src_w, src_h;
> >>   
> >>   	if (!fb) {
> >>   		*visible = false;
> >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> >>   		return -EINVAL;
> >>   	}
> >>   
> >> +	if (fb)
> >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> >> +				rotation);
> >> +
> >>   	/* Check scaling */
> >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> >> +max_scale);
> > This is an unrelated change. Relaxed scaling allows the the src/dest 
> > rectangles to be reduced in size in order to keep the scaling ration 
> > within the min/max range. I suppose we should switch to using it to 
> > make the behaviour uniform across drivers, but definitely should be 
> > done with a separate patch.
> Since, I added drm_rect_rotate before this, it changes the src sizes 
> and it was giving me Invalid scaling if we don't let the sizes to be 
> changed using _relaxed functions. I am trying this for 90/270 
> rotation.

That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.

> I can move it to a separate patch if required.

We nee to figure out why you got the error first.

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

Re: [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Ville Syrjälä]

On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote:
> Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.

If you want no scaling then with 90/270 rotation then your src->w should
be equal to dst->h. Then the calculated vscale will be 1.0. If it's not,
then your test is passing in bad coordinates.

> So, I think we _relaxed is the function which should be called to update the destination size appropriately.
> Please correct me if I am wrong.
> 
> -----Original Message-----
> From: Jindal, Sonika 
> Sent: Wednesday, January 14, 2015 3:06 PM
> To: 'Ville Syrjälä'
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
> 
> We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
> For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
> With this change, the rotated plane is properly adjusted in the viewport.
> So, I don't think it is a bug in test.
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Wednesday, January 14, 2015 2:58 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
> 
> On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> > 
> > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > >> Taking rotation into account while checking the plane and adjusting 
> > >> the sizes accordingly.
> > >>
> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > >> ---
> > >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> > >>   include/drm/drm_plane_helper.h     |    3 +-
> > >>   2 files changed, 77 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > >> b/drivers/gpu/drm/drm_plane_helper.c
> > >> index f24c4cf..4badd69 100644
> > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > >>   				    int max_scale,
> > >>   				    bool can_position,
> > >>   				    bool can_update_disabled,
> > >> -				    bool *visible)
> > >> +				    bool *visible,
> > >> +				    unsigned int rotation)
> > >>   {
> > >>   	int hscale, vscale;
> > >> +	int crtc_x, crtc_y;
> > >> +	unsigned int crtc_w, crtc_h;
> > >> +	uint32_t src_x, src_y, src_w, src_h;
> > >>   
> > >>   	if (!fb) {
> > >>   		*visible = false;
> > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > >>   		return -EINVAL;
> > >>   	}
> > >>   
> > >> +	if (fb)
> > >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > >> +				rotation);
> > >> +
> > >>   	/* Check scaling */
> > >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> > >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> > >> +max_scale);
> > > This is an unrelated change. Relaxed scaling allows the the src/dest 
> > > rectangles to be reduced in size in order to keep the scaling ration 
> > > within the min/max range. I suppose we should switch to using it to 
> > > make the behaviour uniform across drivers, but definitely should be 
> > > done with a separate patch.
> > Since, I added drm_rect_rotate before this, it changes the src sizes 
> > and it was giving me Invalid scaling if we don't let the sizes to be 
> > changed using _relaxed functions. I am trying this for 90/270 
> > rotation.
> 
> That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.
> 
> > I can move it to a separate patch if required.
> 
> We nee to figure out why you got the error first.
> 
> --
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Jindal, Sonika]

I think I am confused here..
Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well.
From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, January 14, 2015 5:14 PM
To: Jindal, Sonika
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote:
> Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.

If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates.

> So, I think we _relaxed is the function which should be called to update the destination size appropriately.
> Please correct me if I am wrong.
> 
> -----Original Message-----
> From: Jindal, Sonika
> Sent: Wednesday, January 14, 2015 3:06 PM
> To: 'Ville Syrjälä'
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> drm_plane_helper_check_update
> 
> We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
> For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
> With this change, the rotated plane is properly adjusted in the viewport.
> So, I don't think it is a bug in test.
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Wednesday, January 14, 2015 2:58 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> drm_plane_helper_check_update
> 
> On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> > 
> > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > >> Taking rotation into account while checking the plane and 
> > >> adjusting the sizes accordingly.
> > >>
> > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > >> ---
> > >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> > >>   include/drm/drm_plane_helper.h     |    3 +-
> > >>   2 files changed, 77 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > >> b/drivers/gpu/drm/drm_plane_helper.c
> > >> index f24c4cf..4badd69 100644
> > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > >>   				    int max_scale,
> > >>   				    bool can_position,
> > >>   				    bool can_update_disabled,
> > >> -				    bool *visible)
> > >> +				    bool *visible,
> > >> +				    unsigned int rotation)
> > >>   {
> > >>   	int hscale, vscale;
> > >> +	int crtc_x, crtc_y;
> > >> +	unsigned int crtc_w, crtc_h;
> > >> +	uint32_t src_x, src_y, src_w, src_h;
> > >>   
> > >>   	if (!fb) {
> > >>   		*visible = false;
> > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > >>   		return -EINVAL;
> > >>   	}
> > >>   
> > >> +	if (fb)
> > >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > >> +				rotation);
> > >> +
> > >>   	/* Check scaling */
> > >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> > >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> > >> +max_scale);
> > > This is an unrelated change. Relaxed scaling allows the the 
> > > src/dest rectangles to be reduced in size in order to keep the 
> > > scaling ration within the min/max range. I suppose we should 
> > > switch to using it to make the behaviour uniform across drivers, 
> > > but definitely should be done with a separate patch.
> > Since, I added drm_rect_rotate before this, it changes the src sizes 
> > and it was giving me Invalid scaling if we don't let the sizes to be 
> > changed using _relaxed functions. I am trying this for 90/270 
> > rotation.
> 
> That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.
> 
> > I can move it to a separate patch if required.
> 
> We nee to figure out why you got the error first.
> 
> --
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Ville Syrjälä]

On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote:
> I think I am confused here..
> Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well.

Well, userspace still has to know how the src and dst coordinates relate
to each other.

Src coordinates are based on the memory layout. So src x=0,y=0 is the
first pixel in memory. x=1,y=0 is the second pixel in memory etc.

Dst coordinates are based on the display pipe. So dst x=0,y=0 is the
first pixel that gets pushed out by the pipe for each frame. x=1,y=0
is the second pixel getting pushed out.

Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to
use a sprite plane to present the FB with 0 and 90 degree rotation, and
we want to position the sprite plane at coordinates 2,0 on the CRTC.
It should look something like this:

rotation=0
src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2)
dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2)
FB:            CRTC:
0,0 ----    0,0 --------
   |abcd|      |  abcd  |
   |efgh|      |  efgh  |
    ---- 4,2   |        |
               |        |
                -------- 8,4

rotation=90
src=0,0 -> 4,2 (w=4, h=2)
dst=2,0 -> 4,4 (w=2, h=4)
FB:            CRTC:
0,0 ----    0,0 --------
   |abcd|      |  dh    |
   |efgh|      |  cg    |
    ---- 4,2   |  bf    |
               |  ae    |
                -------- 8,4

> >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation.

Sounds a bit buggy then.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Wednesday, January 14, 2015 5:14 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
> 
> On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote:
> > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.
> 
> If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates.
> 
> > So, I think we _relaxed is the function which should be called to update the destination size appropriately.
> > Please correct me if I am wrong.
> > 
> > -----Original Message-----
> > From: Jindal, Sonika
> > Sent: Wednesday, January 14, 2015 3:06 PM
> > To: 'Ville Syrjälä'
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > drm_plane_helper_check_update
> > 
> > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
> > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
> > With this change, the rotated plane is properly adjusted in the viewport.
> > So, I don't think it is a bug in test.
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Wednesday, January 14, 2015 2:58 PM
> > To: Jindal, Sonika
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > drm_plane_helper_check_update
> > 
> > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> > > 
> > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > > >> Taking rotation into account while checking the plane and 
> > > >> adjusting the sizes accordingly.
> > > >>
> > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > >> ---
> > > >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> > > >>   include/drm/drm_plane_helper.h     |    3 +-
> > > >>   2 files changed, 77 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > > >> b/drivers/gpu/drm/drm_plane_helper.c
> > > >> index f24c4cf..4badd69 100644
> > > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > >>   				    int max_scale,
> > > >>   				    bool can_position,
> > > >>   				    bool can_update_disabled,
> > > >> -				    bool *visible)
> > > >> +				    bool *visible,
> > > >> +				    unsigned int rotation)
> > > >>   {
> > > >>   	int hscale, vscale;
> > > >> +	int crtc_x, crtc_y;
> > > >> +	unsigned int crtc_w, crtc_h;
> > > >> +	uint32_t src_x, src_y, src_w, src_h;
> > > >>   
> > > >>   	if (!fb) {
> > > >>   		*visible = false;
> > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > >>   		return -EINVAL;
> > > >>   	}
> > > >>   
> > > >> +	if (fb)
> > > >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > > >> +				rotation);
> > > >> +
> > > >>   	/* Check scaling */
> > > >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > > >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > > >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> > > >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> > > >> +max_scale);
> > > > This is an unrelated change. Relaxed scaling allows the the 
> > > > src/dest rectangles to be reduced in size in order to keep the 
> > > > scaling ration within the min/max range. I suppose we should 
> > > > switch to using it to make the behaviour uniform across drivers, 
> > > > but definitely should be done with a separate patch.
> > > Since, I added drm_rect_rotate before this, it changes the src sizes 
> > > and it was giving me Invalid scaling if we don't let the sizes to be 
> > > changed using _relaxed functions. I am trying this for 90/270 
> > > rotation.
> > 
> > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.
> > 
> > > I can move it to a separate patch if required.
> > 
> > We nee to figure out why you got the error first.
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

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

RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Jindal, Sonika]

So, if we do the source and dst adjustments in userspace, the logic in intel_check_sprite_plane will not hold good there.
That's how it gets right coordinates and w,h for sprite for 90 rotation.
I thought drm_rect_rotate and other functions are there to handle such scenarios.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, January 14, 2015 11:38 PM
To: Jindal, Sonika
Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote:
> I think I am confused here..
> Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well.

Well, userspace still has to know how the src and dst coordinates relate to each other.

Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc.

Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out.

Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC.
It should look something like this:

rotation=0
src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2)
dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2)
FB:            CRTC:
0,0 ----    0,0 --------
   |abcd|      |  abcd  |
   |efgh|      |  efgh  |
    ---- 4,2   |        |
               |        |
                -------- 8,4

rotation=90
src=0,0 -> 4,2 (w=4, h=2)
dst=2,0 -> 4,4 (w=2, h=4)
FB:            CRTC:
0,0 ----    0,0 --------
   |abcd|      |  dh    |
   |efgh|      |  cg    |
    ---- 4,2   |  bf    |
               |  ae    |
                -------- 8,4

> >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation.

Sounds a bit buggy then.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Wednesday, January 14, 2015 5:14 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> drm_plane_helper_check_update
> 
> On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote:
> > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.
> 
> If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates.
> 
> > So, I think we _relaxed is the function which should be called to update the destination size appropriately.
> > Please correct me if I am wrong.
> > 
> > -----Original Message-----
> > From: Jindal, Sonika
> > Sent: Wednesday, January 14, 2015 3:06 PM
> > To: 'Ville Syrjälä'
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > drm_plane_helper_check_update
> > 
> > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
> > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
> > With this change, the rotated plane is properly adjusted in the viewport.
> > So, I don't think it is a bug in test.
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Wednesday, January 14, 2015 2:58 PM
> > To: Jindal, Sonika
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > drm_plane_helper_check_update
> > 
> > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> > > 
> > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > > >> Taking rotation into account while checking the plane and 
> > > >> adjusting the sizes accordingly.
> > > >>
> > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > >> ---
> > > >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> > > >>   include/drm/drm_plane_helper.h     |    3 +-
> > > >>   2 files changed, 77 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > > >> b/drivers/gpu/drm/drm_plane_helper.c
> > > >> index f24c4cf..4badd69 100644
> > > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > >>   				    int max_scale,
> > > >>   				    bool can_position,
> > > >>   				    bool can_update_disabled,
> > > >> -				    bool *visible)
> > > >> +				    bool *visible,
> > > >> +				    unsigned int rotation)
> > > >>   {
> > > >>   	int hscale, vscale;
> > > >> +	int crtc_x, crtc_y;
> > > >> +	unsigned int crtc_w, crtc_h;
> > > >> +	uint32_t src_x, src_y, src_w, src_h;
> > > >>   
> > > >>   	if (!fb) {
> > > >>   		*visible = false;
> > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > >>   		return -EINVAL;
> > > >>   	}
> > > >>   
> > > >> +	if (fb)
> > > >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > > >> +				rotation);
> > > >> +
> > > >>   	/* Check scaling */
> > > >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > > >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > > >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> > > >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> > > >> +max_scale);
> > > > This is an unrelated change. Relaxed scaling allows the the 
> > > > src/dest rectangles to be reduced in size in order to keep the 
> > > > scaling ration within the min/max range. I suppose we should 
> > > > switch to using it to make the behaviour uniform across drivers, 
> > > > but definitely should be done with a separate patch.
> > > Since, I added drm_rect_rotate before this, it changes the src 
> > > sizes and it was giving me Invalid scaling if we don't let the 
> > > sizes to be changed using _relaxed functions. I am trying this for 
> > > 90/270 rotation.
> > 
> > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.
> > 
> > > I can move it to a separate patch if required.
> > 
> > We nee to figure out why you got the error first.
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

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

Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update

[Ville Syrjälä]

On Thu, Jan 15, 2015 at 02:04:02AM +0000, Jindal, Sonika wrote:
> So, if we do the source and dst adjustments in userspace, the logic in intel_check_sprite_plane will not hold good there.

I'm not sure what adjustments you mean. Src says exactly which part of
the FB you want to present to the user, rotation doesn't change that
fact. Dst says exactly where on the screen you want the image to appear,
again rotation doesn't change that fact.

> That's how it gets right coordinates and w,h for sprite for 90 rotation.
> I thought drm_rect_rotate and other functions are there to handle such scenarios.

They are there mostly to make sure clipping happens correctly. They
aren't there to magically fix userspace bugs.

The only magicy part is the relaxed scaling and the rounding of
coordinates based on hardware limits, and that stuff only exists so
that it's easier to write userspace code without having to know
the limitations of the hardware.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Wednesday, January 14, 2015 11:38 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to drm_plane_helper_check_update
> 
> On Wed, Jan 14, 2015 at 01:54:06PM +0000, Jindal, Sonika wrote:
> > I think I am confused here..
> > Since sprite plane handles this scaling and rotation in kernel, I thought it should be taken care in driver for primary as well.
> 
> Well, userspace still has to know how the src and dst coordinates relate to each other.
> 
> Src coordinates are based on the memory layout. So src x=0,y=0 is the first pixel in memory. x=1,y=0 is the second pixel in memory etc.
> 
> Dst coordinates are based on the display pipe. So dst x=0,y=0 is the first pixel that gets pushed out by the pipe for each frame. x=1,y=0 is the second pixel getting pushed out.
> 
> Let's say we have a 4x2 FB and a display mode of 8x4. Then we want to use a sprite plane to present the FB with 0 and 90 degree rotation, and we want to position the sprite plane at coordinates 2,0 on the CRTC.
> It should look something like this:
> 
> rotation=0
> src: x1=0,y1=0,x2=4,y2=2 (w=4, h=2)
> dst: x1=2,y1=0,x2=6,y2=2 (w=4, h=2)
> FB:            CRTC:
> 0,0 ----    0,0 --------
>    |abcd|      |  abcd  |
>    |efgh|      |  efgh  |
>     ---- 4,2   |        |
>                |        |
>                 -------- 8,4
> 
> rotation=90
> src=0,0 -> 4,2 (w=4, h=2)
> dst=2,0 -> 4,4 (w=2, h=4)
> FB:            CRTC:
> 0,0 ----    0,0 --------
>    |abcd|      |  dh    |
>    |efgh|      |  cg    |
>     ---- 4,2   |  bf    |
>                |  ae    |
>                 -------- 8,4
> 
> > >From the test, I don't really change the src sizes for primary as well as sprite to take care of 90/270 rotation.
> 
> Sounds a bit buggy then.
> 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Wednesday, January 14, 2015 5:14 PM
> > To: Jindal, Sonika
> > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > drm_plane_helper_check_update
> > 
> > On Wed, Jan 14, 2015 at 09:49:36AM +0000, Jindal, Sonika wrote:
> > > Since we do drm_rect_rotate with 90 rotation, the src->w changes to src->h.
> > > Now, when we call drm_rect_calc_hscale, the hscale calculated is lesser than the min_hscale (which is no_scaling by default), so it returns -ERANGE.
> > 
> > If you want no scaling then with 90/270 rotation then your src->w should be equal to dst->h. Then the calculated vscale will be 1.0. If it's not, then your test is passing in bad coordinates.
> > 
> > > So, I think we _relaxed is the function which should be called to update the destination size appropriately.
> > > Please correct me if I am wrong.
> > > 
> > > -----Original Message-----
> > > From: Jindal, Sonika
> > > Sent: Wednesday, January 14, 2015 3:06 PM
> > > To: 'Ville Syrjälä'
> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > > Subject: RE: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > > drm_plane_helper_check_update
> > > 
> > > We do exactly like this for sprite plane (ie, rotate the rect, then check scaling and adjust the size accordingly from drm_rect_calc_hscale_relaxed) That's why I saw the need of this for primary plane as well. 
> > > For sprite plane 90 rotation, intel_check_sprite_plane does the adjustments and the rotated sizes are fine. But since we don't do any of those stuff for primary the destination size doesn't come right, and I get a little corrupted output after rotation.
> > > With this change, the rotated plane is properly adjusted in the viewport.
> > > So, I don't think it is a bug in test.
> > > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Wednesday, January 14, 2015 2:58 PM
> > > To: Jindal, Sonika
> > > Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 1/2] drm: Adding rotation to 
> > > drm_plane_helper_check_update
> > > 
> > > On Wed, Jan 14, 2015 at 10:05:53AM +0530, sonika wrote:
> > > > 
> > > > On Tuesday 13 January 2015 07:02 PM, Ville Syrjälä wrote:
> > > > > On Tue, Jan 13, 2015 at 06:03:39PM +0530, Sonika Jindal wrote:
> > > > >> Taking rotation into account while checking the plane and 
> > > > >> adjusting the sizes accordingly.
> > > > >>
> > > > >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > > >> ---
> > > > >>   drivers/gpu/drm/drm_plane_helper.c |   79 ++++++++++++++++++++++++++++++++++--
> > > > >>   include/drm/drm_plane_helper.h     |    3 +-
> > > > >>   2 files changed, 77 insertions(+), 5 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> > > > >> b/drivers/gpu/drm/drm_plane_helper.c
> > > > >> index f24c4cf..4badd69 100644
> > > > >> --- a/drivers/gpu/drm/drm_plane_helper.c
> > > > >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > > >> @@ -138,9 +138,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > > >>   				    int max_scale,
> > > > >>   				    bool can_position,
> > > > >>   				    bool can_update_disabled,
> > > > >> -				    bool *visible)
> > > > >> +				    bool *visible,
> > > > >> +				    unsigned int rotation)
> > > > >>   {
> > > > >>   	int hscale, vscale;
> > > > >> +	int crtc_x, crtc_y;
> > > > >> +	unsigned int crtc_w, crtc_h;
> > > > >> +	uint32_t src_x, src_y, src_w, src_h;
> > > > >>   
> > > > >>   	if (!fb) {
> > > > >>   		*visible = false;
> > > > >> @@ -158,9 +162,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
> > > > >>   		return -EINVAL;
> > > > >>   	}
> > > > >>   
> > > > >> +	if (fb)
> > > > >> +		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
> > > > >> +				rotation);
> > > > >> +
> > > > >>   	/* Check scaling */
> > > > >> -	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
> > > > >> -	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
> > > > >> +	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
> > > > >> +	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, 
> > > > >> +max_scale);
> > > > > This is an unrelated change. Relaxed scaling allows the the 
> > > > > src/dest rectangles to be reduced in size in order to keep the 
> > > > > scaling ration within the min/max range. I suppose we should 
> > > > > switch to using it to make the behaviour uniform across drivers, 
> > > > > but definitely should be done with a separate patch.
> > > > Since, I added drm_rect_rotate before this, it changes the src 
> > > > sizes and it was giving me Invalid scaling if we don't let the 
> > > > sizes to be changed using _relaxed functions. I am trying this for 
> > > > 90/270 rotation.
> > > 
> > > That would indicate a bug somewhere. Pontetially the bug could be in whatever test you're using as well.
> > > 
> > > > I can move it to a separate patch if required.
> > > 
> > > We nee to figure out why you got the error first.
> > > 
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

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

[PATCH] drm: Adding rotation to drm_plane_helper_check_update

[Sonika Jindal]

Taking rotation into account while checking the plane
and adjusting the sizes accordingly.

v2: Adding parameter in the callers in the same patch(Matt)
    Removing unnecessary code and allowing scaling(Ville)

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c          |   44 ++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_display.c        |    6 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |    3 +-
 include/drm/drm_plane_helper.h              |    3 +-
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index f24c4cf..d6916af 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -138,7 +138,8 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 				    int max_scale,
 				    bool can_position,
 				    bool can_update_disabled,
-				    bool *visible)
+				    bool *visible,
+				    unsigned int rotation)
 {
 	int hscale, vscale;
 
@@ -158,9 +159,13 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	if (fb)
+		drm_rect_rotate(src, fb->width << 16, fb->height << 16,
+				rotation);
+
 	/* Check scaling */
-	hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
-	vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+	hscale = drm_rect_calc_hscale_relaxed(src, dest, min_scale, max_scale);
+	vscale = drm_rect_calc_vscale_relaxed(src, dest, min_scale, max_scale);
 	if (hscale < 0 || vscale < 0) {
 		DRM_DEBUG_KMS("Invalid scaling of plane\n");
 		return -ERANGE;
@@ -182,6 +187,36 @@ int drm_plane_helper_check_update(struct drm_plane *plane,
 		return -EINVAL;
 	}
 
+	if (*visible) {
+		/* check again in case clipping clamped the results */
+		hscale = drm_rect_calc_hscale(src, dest, min_scale, max_scale);
+		if (hscale < 0) {
+			DRM_DEBUG_KMS("Horizontal scaling factor out of limits\n");
+			drm_rect_debug_print(src, true);
+			drm_rect_debug_print(dest, false);
+
+			return hscale;
+		}
+
+		vscale = drm_rect_calc_vscale(src, dest, min_scale, max_scale);
+		if (vscale < 0) {
+			DRM_DEBUG_KMS("Vertical scaling factor out of limits\n");
+			drm_rect_debug_print(src, true);
+			drm_rect_debug_print(dest, false);
+
+			return vscale;
+		}
+
+		/* Make the source viewport size an exact multiple of the scaling factors. */
+		drm_rect_adjust_size(src,
+			drm_rect_width(dest) * hscale - drm_rect_width(src),
+			drm_rect_height(dest) * vscale - drm_rect_height(src));
+
+		drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16,
+				    rotation);
+
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_helper_check_update);
@@ -258,7 +293,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 					    &src, &dest, &clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, false, &visible);
+					    false, false, &visible,
+					    DRM_ROTATE_0);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f40288f..d19ed4b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12155,7 +12155,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, true, &state->visible);
+					    false, true, &state->visible,
+					    to_intel_plane(plane)->rotation);
 	if (ret)
 		return ret;
 
@@ -12429,7 +12430,8 @@ intel_check_cursor_plane(struct drm_plane *plane,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    true, true, &state->visible);
+					    true, true, &state->visible,
+					    to_intel_plane(plane)->rotation);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index e7ca25b..4813a53 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -551,7 +551,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
 					    &src, &dest, &clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    can_position, false, &visible);
+					    can_position, false, &visible,
+					    DRM_ROTATE_0);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index a185392..ef9eb04 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -84,7 +84,8 @@ extern int drm_plane_helper_check_update(struct drm_plane *plane,
 					 int max_scale,
 					 bool can_position,
 					 bool can_update_disabled,
-					 bool *visible);
+					 bool *visible,
+					 unsigned int rotation);
 extern int drm_primary_helper_update(struct drm_plane *plane,
 				     struct drm_crtc *crtc,
 				     struct drm_framebuffer *fb,
-- 
1.7.10.4

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

Re: [PATCH] drm: Adding rotation to drm_plane_helper_check_update

[Matt Roper]

On Wed, Jan 14, 2015 at 11:10:54AM +0530, Sonika Jindal wrote:
> Taking rotation into account while checking the plane
> and adjusting the sizes accordingly.
> 
> v2: Adding parameter in the callers in the same patch(Matt)
>     Removing unnecessary code and allowing scaling(Ville)
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
...
>  EXPORT_SYMBOL(drm_plane_helper_check_update);
> @@ -258,7 +293,8 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  					    &src, &dest, &clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> -					    false, false, &visible);
> +					    false, false, &visible,
> +					    DRM_ROTATE_0);

I think this needs to be BIT(DRM_ROTATE_0)?


...
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index e7ca25b..4813a53 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -551,7 +551,8 @@ static int vop_update_plane_event(struct drm_plane *plane,
>  					    &src, &dest, &clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
> -					    can_position, false, &visible);
> +					    can_position, false, &visible,
> +					    DRM_ROTATE_0);

Same here.


Matt

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

Re: [PATCH] drm: Adding rotation to drm_plane_helper_check_update

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5579
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  355/355              355/355
SNB                                  400/422              400/422
IVB                 -2              487/487              485/487
BYT                                  296/296              296/296
HSW                                  486/508              486/508
BDW                 -1              402/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*IVB  igt_kms_cursor_crc_cursor-256x256-offscreen      DMESG_WARN(1, M4)PASS(2, M34M21)      FAIL(1, M21)
*IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      DMESG_WARN(1, M4)PASS(2, M34M21)      FAIL(1, M21)
*BDW  igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible      PASS(5, M30M28)      DMESG_WARN(1, M30)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Passing rotation to drm_plane_helper_check_update

[Sonika Jindal]

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f40288f..d19ed4b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12155,7 +12155,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, true, &state->visible);
+					    false, true, &state->visible,
+					    to_intel_plane(plane)->rotation);
 	if (ret)
 		return ret;
 
@@ -12429,7 +12430,8 @@ intel_check_cursor_plane(struct drm_plane *plane,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    true, true, &state->visible);
+					    true, true, &state->visible,
+					    to_intel_plane(plane)->rotation);
 	if (ret)
 		return ret;
 
-- 
1.7.10.4

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

Re: [PATCH 2/2] drm/i915: Passing rotation to drm_plane_helper_check_update

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  354/354              354/354
ILK                                  354/354              354/354
SNB              +1-1              401/424              401/424
IVB                 -2              488/488              486/488
BYT                                  278/278              278/278
HSW                 -42              529/529              487/529
BDW                 -1              405/405              404/405
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt_kms_flip_flip-vs-dpms-off-vs-modeset-interruptible      NSPT(1, M35)PASS(11, M35M22)      PASS(1, M35)
*SNB  igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible      PASS(12, M35M22)      DMESG_WARN(1, M35)
*IVB  igt_kms_cursor_crc_cursor-256x256-offscreen      PASS(3, M21M34)      FAIL(1, M34)
*IVB  igt_kms_cursor_crc_cursor-64x64-offscreen      PASS(3, M21M34)      FAIL(1, M34)
 HSW  igt_kms_cursor_crc_cursor-size-change      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_kms_fence_pin_leak      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_kms_flip_event_leak      NSPT(6, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_kms_mmio_vs_cs_flip_setcrtc_vs_cs_flip      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_kms_mmio_vs_cs_flip_setplane_vs_cs_flip      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_lpsp_non-edp      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_cursor      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_cursor-dpms      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_dpms-mode-unset-non-lpsp      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_dpms-non-lpsp      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_drm-resources-equal      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_fences      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_fences-dpms      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_gem-execbuf      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_gem-mmap-cpu      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_gem-mmap-gtt      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_gem-pread      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_i2c      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_modeset-non-lpsp      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_modeset-non-lpsp-stress-no-wait      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_pci-d3-state      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_pm_rpm_rte      NSPT(7, M40M19)PASS(1, M20)      NSPT(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-bcs-early-read-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-bcs-early-read-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-bcs-overwrite-source-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-bcs-overwrite-source-interruptible      DMESG_WARN(6, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-rcs-early-read-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-rcs-early-read-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-rcs-gpu-read-after-write-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-rcs-overwrite-source-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gtt-rcs-overwrite-source-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-bcs-early-read-interruptible      DMESG_WARN(5, M40M19)PASS(2, M20M19)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-bcs-gpu-read-after-write-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-bcs-overwrite-source-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-bcs-overwrite-source-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-rcs-early-read-interruptible      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-rcs-gpu-read-after-write-interruptible      DMESG_WARN(6, M40M19)PASS(2, M20M19)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-rcs-overwrite-source-forked      DMESG_WARN(7, M40M19)PASS(1, M20)      DMESG_WARN(1, M19)
 HSW  igt_gem_concurrent_blit_gttX-rcs-overwrite-source-interruptible      DMESG_WARN(4, M40M19)PASS(2, M20M40)      DMESG_WARN(1, M19)
*BDW  igt_gem_concurrent_blit_gtt-bcs-gpu-read-after-write-interruptible      PASS(11, M30M28)      DMESG_WARN(1, M30)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/2] Adding rotattion to drm_plane_helper_check_update

[Matt Roper]

On Tue, Jan 13, 2015 at 06:03:38PM +0530, Sonika Jindal wrote:
> This adds another parameter rotation to drm_plane_helper_check_update.
> This will enable this function to do to size updations based upon the rotation
> if any.
> Updated the calls to this function in i915 and drm. Rockchip driver also needs
> to be updated.

It looks like you add the new function parameter in patch 1, but don't
add it to the i915 callsites until patch 2 (and don't add it to the
rockchip driver at all in this series).  That means that if someone is
bisecting the kernel and lands on this commit, some of the drivers will
fail to build, which isn't good.

The callsites need to be updated at the same time that you add the
parameter; fortunately there are only four of them (two in i915, one in
the generic plane helper, and one in rockchip).  For simplicity, you can
just pass the DRM_ROTATE_0 bit initially at the updated callsites when
you add the new parameter so that there's no change from the current
behavior for the existing callers.  Then you can come back in later in
driver-specific patch(es) and pass the real rotation value the driver is
using and adjust the caller's functionality as appropriate.


Matt

> 
> Sonika Jindal (2):
>   drm: Adding rotation to drm_plane_helper_check_update
>   drm/i915: Passing rotation to drm_plane_helper_check_update
> 
>  drivers/gpu/drm/drm_plane_helper.c   |   79 ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |    6 ++-
>  include/drm/drm_plane_helper.h       |    3 +-
>  3 files changed, 81 insertions(+), 7 deletions(-)
> 
> -- 
> 1.7.10.4
> 

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

Re: [PATCH 0/2] Adding rotattion to drm_plane_helper_check_update

[sonika]

On Tuesday 13 January 2015 11:42 PM, Matt Roper wrote:
> On Tue, Jan 13, 2015 at 06:03:38PM +0530, Sonika Jindal wrote:
>> This adds another parameter rotation to drm_plane_helper_check_update.
>> This will enable this function to do to size updations based upon the rotation
>> if any.
>> Updated the calls to this function in i915 and drm. Rockchip driver also needs
>> to be updated.
> It looks like you add the new function parameter in patch 1, but don't
> add it to the i915 callsites until patch 2 (and don't add it to the
> rockchip driver at all in this series).  That means that if someone is
> bisecting the kernel and lands on this commit, some of the drivers will
> fail to build, which isn't good.
>
> The callsites need to be updated at the same time that you add the
> parameter; fortunately there are only four of them (two in i915, one in
> the generic plane helper, and one in rockchip).  For simplicity, you can
> just pass the DRM_ROTATE_0 bit initially at the updated callsites when
> you add the new parameter so that there's no change from the current
> behavior for the existing callers.  Then you can come back in later in
> driver-specific patch(es) and pass the real rotation value the driver is
> using and adjust the caller's functionality as appropriate.
Sure, I will do this.

Sonika
>
> Matt
>
>> Sonika Jindal (2):
>>    drm: Adding rotation to drm_plane_helper_check_update
>>    drm/i915: Passing rotation to drm_plane_helper_check_update
>>
>>   drivers/gpu/drm/drm_plane_helper.c   |   79 ++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_display.c |    6 ++-
>>   include/drm/drm_plane_helper.h       |    3 +-
>>   3 files changed, 81 insertions(+), 7 deletions(-)
>>
>> -- 
>> 1.7.10.4
>>

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