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

[Rob Clark <rob.clark@linaro.org>]

On Wed, Apr 4, 2012 at 3:19 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> 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@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@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.
>>
>> 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


ok, I think we are saying the same thing.  This is what I meant by
doing the x/y swapping in userspace.

So I think I leave the invert_coords for crtc, and drop for plane

BR,
-R

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