[PATCH RFC] drm: support for rotated scanout

[Rob Clark]

On Sat, Mar 31, 2012 at 4:09 PM, Ville Syrjälä <syrjala at sci.fi> wrote:
> On Fri, Mar 30, 2012 at 05:59:32PM -0500, Rob Clark wrote:
>> From: Rob Clark <rob at 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.

fwiw, it might be a bit odd that in one case I swapped fb dimensions,
and in the other crtc dimensions..  although it was just laziness
(there were fewer param's to swap that 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).

fwiw, in my case planes and CRTCs are rotated independently.. ie.
rotating the CRTC doesn't automatically rotate the planes.  But I
include invert_dimensions flag in both drm_crtc and drm_plane so
drivers for hw that works differently can do whatever is appropriate

>
>>       /* 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.
>

I was trying to avoid putting full matrix transformation in the kernel ;-)

but at least the hw I'm familiar with is only doing simple isometric
transformations in scanout (ie. multiples of 90 degress and/or
horiz/vert mirroring).  Anything more complex would require a shadow
buffer and gpu, which is (I think) better to do in userspace.  But I
don't know if this is sufficient for other drivers or not.. part of
the reason for the RFC ;-)

BR,
-R

>>                       ret = -ENOSPC;
>>                       goto out;
>>               }
>
> --
> Ville Syrjälä
> syrjala at sci.fi
> http://www.sci.fi/~syrjala/
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel