[PATCH RFC] drm: support for rotated scanout

[Ville Syrjälä]

On Tue, Apr 03, 2012 at 04:02:54PM -0500, Rob Clark wrote:
> 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.

If you fb dimensions are 1080x1920 and you want to show all of it, you
always set the src coords to 1080x1920+0+0 regardless of rotation.

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

Nope.

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

Nope. The plane's crtc coords should be in the same orientation as the
crtc itself.

Perhaps an example will illustrate my point a bit better. Let's say
you have a 800x600 fb. You only want to show the center 640x480 area
90 degrees rotated and unscaled in the middle of a 1024x768 display
(display mode hdisplay=1024 vdisplay=768).

To achieve that you would set the plane's coordinates like so:
src 640x480+80+60
crtc 480x640+272+64

-- 
Ville Syrjälä
Intel OTC