Re: [PATCH RFC] drm: support for rotated scanout

["Ville Syrjälä" <syrjala@sci.fi>]

On Fri, Mar 30, 2012 at 05:59:32PM -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> For drivers that can support rotated scanout, the extra parameter
> checking in drm-core, while nice, tends to get confused.  To solve
> this drivers can set the crtc or plane invert_dimensions field so
> that the dimension checking takes into account the rotation that
> the driver is performing.
> ---
> Note: RFC still mainly because I've only tested the CRTC rotation so
> far.. still need to write some test code for plane rotation.
> 
>  drivers/gpu/drm/drm_crtc.c |   50 +++++++++++++++++++++++++++++--------------
>  include/drm/drm_crtc.h     |    9 ++++++++
>  2 files changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0dff444..261c9bd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -375,6 +375,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  
>  	crtc->dev = dev;
>  	crtc->funcs = funcs;
> +	crtc->invert_dimensions = false;
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  
> @@ -609,6 +610,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	plane->base.properties = &plane->properties;
>  	plane->dev = dev;
>  	plane->funcs = funcs;
> +	plane->invert_dimensions = false;
>  	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
>  				      GFP_KERNEL);
>  	if (!plane->format_types) {
> @@ -1758,6 +1760,9 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	fb_width = fb->width << 16;
>  	fb_height = fb->height << 16;
>  
> +	if (plane->invert_dimensions)
> +		swap(fb_width, fb_height);
> +

In my opinion the only sane way to specify this stuff is that
the source coordinates are not transformed in any way.

So you fetch the data from the fb based on the source coordinates, then
apply all plane transformations (if any), and then apply all CRTC
transformations (if any).

>  	/* Make sure source coordinates are inside the fb. */
>  	if (plane_req->src_w > fb_width ||
>  	    plane_req->src_x > fb_width - plane_req->src_w ||
> @@ -1856,6 +1861,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
>  
>  	if (crtc_req->mode_valid) {
> +		int hdisplay, vdisplay;
>  		/* If we have a mode we need a framebuffer. */
>  		/* If we pass -1, set the mode with the currently bound fb */
>  		if (crtc_req->fb_id == -1) {
> @@ -1891,14 +1897,20 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  
>  		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
>  
> -		if (mode->hdisplay > fb->width ||
> -		    mode->vdisplay > fb->height ||
> -		    crtc_req->x > fb->width - mode->hdisplay ||
> -		    crtc_req->y > fb->height - mode->vdisplay) {
> -			DRM_DEBUG_KMS("Invalid CRTC viewport %ux%u+%u+%u for fb size %ux%u.\n",
> -				      mode->hdisplay, mode->vdisplay,
> -				      crtc_req->x, crtc_req->y,
> -				      fb->width, fb->height);
> +		hdisplay = mode->hdisplay;
> +		vdisplay = mode->vdisplay;
> +
> +		if (crtc->invert_dimensions)
> +			swap(hdisplay, vdisplay);
> +
> +		if (hdisplay > fb->width ||
> +		    vdisplay > fb->height ||
> +		    crtc_req->x > fb->width - hdisplay ||
> +		    crtc_req->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, crtc_req->x, crtc_req->y,
> +				      crtc->invert_dimensions ? " (inverted)" : "");

I would perhaps just stick more details about the transformations into
drm_crtc, but we will anyway require a better mode setting API. So
perhaps this is an OK intermediate solution.

>  			ret = -ENOSPC;
>  			goto out;
>  		}

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/