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@xxxxxx> wrote:
> > On Fri, Mar 30, 2012 at 05:59:32PM -0500, Rob Clark wrote:
> >> From: Rob Clark <rob@xxxxxx>
> >>
> >> 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.
> > 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
I think you should rotate the planes according to the crtc rotation as
well. Otherwise the pipeline fb->plane->crtc->... doesn't make much
sense. If we would just deprecate the crtc='scanout engine' concept
this wouldn't be an issue, as you'd just have plane rotation all the
way.
I suppose another option would be to name your crtc rotation property to
something like "crtc fb rotation" to make it clear that it only applies
to the crtc scanout, and not any attached planes. Then we can still add a
"real" crtc rotation later. You could perhaps expose the rotation
capabilities of the actual display as "crtc rotation". OTOH maybe that'd
be equally illogical as rotation that doesn't rotate.
> >> /* 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).
I think that's typical. And some HW only does a subset of that (eg. just
one way 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 ;-)
My suggestion was rooted in the idea that it'd be easier to process
the crtc rotation when dealing with planes. But if you're not going
to do that, the extra information would be more or less useless.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
