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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@sci.fi> wrote:
> > > 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.
> > 
> > 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.

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.

-- 
Ville Syrjälä
Intel OTC