[PATCH RFC] drm: support for rotated scanout

[Rob Clark]

On Mon, Apr 2, 2012 at 1:39 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Mon, Apr 02, 2012 at 08:26:14PM +0300, Ville Syrjälä wrote:
>> On Mon, Apr 02, 2012 at 10:31:47AM -0500, Rob Clark wrote:
>> > 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 ;-))
>>
>> Not sure if you got my point, which was that w/ plane rotation the
>> src coordinate check should be correct as is. Instead you should
>> apply the rotation when you clip/process the plane's crtc coordinates.
>>
>> Since we don't clip the crtc coordinates in the common code (to allow
>> partially/fully offscreen planes), all the work ends up happening in
>> driver specific code.
>
> What I write doesn't actually match what I meant to write. I didn't
> mean that you'd rotate the crtc coordinates prior to clipping.
> What I meant is that you (probably) rotate the src coordinates in
> the driver in order to do clipping and scaling factor calculations.

Do you mean that userspace should rotate/swap the src coordinates
before calling setplane ioctl?  What I'm perhaps misunderstanding
about what you are getting too is that if the fb is created w/
unrotated coordinates (for ex, 1080x1920 instead of 1920x1080), and
you don't fix up the src coordinates somewhere (either userspace in
the core drm coordinate checking code), then you have a problem, and
the setplane never even reaches the driver's cb fxn.

The fb should of course not be created w/ bogus dimension, because you
might be scanning out one part with one crtc or plane non-rotated, and
other crtc/plane rotated.

> But in any case the bounds checking in the core code is OK, as the
> src coordinates are specified in the orienation of the fb memory
> layout.

Ok, that seems to sound like you are advocating doing the x/y swapping
in userspace for overlays..  which seems ok.

but, fwiw, if we did ever start checking the plane's dst coordinates
vs. the crtc, we would have to do the same w/h swap that we need to do
now for crtc.


BR,
-R


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