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

[sonika <sonika.jindal@intel.com>]

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