| Message ID | 1333148372-31870-1-git-send-email-rob.clark@linaro.org |
|---|---|
| State | New |
| Headers | show |
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. 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). > /* 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. > ret = -ENOSPC; > goto out; > }
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 ;-)) > 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 > >> /* 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). 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 ;-) BR, -R >> ret = -ENOSPC; >> goto out; >> } > > -- > Ville Syrjälä > syrjala@sci.fi > http://www.sci.fi/~syrjala/ > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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. > > 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.
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.
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. 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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
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
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); + /* 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)" : ""); ret = -ENOSPC; goto out; } @@ -3452,6 +3464,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, struct drm_framebuffer *fb; struct drm_pending_vblank_event *e = NULL; unsigned long flags; + int hdisplay, vdisplay; int ret = -EINVAL; if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS || @@ -3481,14 +3494,19 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; fb = obj_to_fb(obj); - if (crtc->mode.hdisplay > fb->width || - crtc->mode.vdisplay > fb->height || - crtc->x > fb->width - crtc->mode.hdisplay || - crtc->y > fb->height - crtc->mode.vdisplay) { - DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d.\n", - fb->width, fb->height, - crtc->mode.hdisplay, crtc->mode.vdisplay, - crtc->x, crtc->y); + hdisplay = crtc->mode.hdisplay; + vdisplay = crtc->mode.vdisplay; + + if (crtc->invert_dimensions) + swap(hdisplay, vdisplay); + + if (hdisplay > fb->width || + vdisplay > fb->height || + crtc->x > fb->width - hdisplay || + crtc->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->x, crtc->y, + crtc->invert_dimensions ? " (inverted)" : ""); ret = -ENOSPC; goto out; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index b5017c9..78dd5b5 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -364,6 +364,9 @@ struct drm_crtc_funcs { * @enabled: is this CRTC enabled? * @mode: current mode timings * @hwmode: mode timings as programmed to hw regs + * @invert_dimensions: for purposes of error checking crtc vs fb sizes, + * invert the width/height of the crtc. This is used if the driver + * is performing 90 or 270 degree rotated scanout * @x: x position on screen * @y: y position on screen * @funcs: CRTC control functions @@ -397,6 +400,8 @@ struct drm_crtc { */ struct drm_display_mode hwmode; + bool invert_dimensions; + int x, y; const struct drm_crtc_funcs *funcs; @@ -636,6 +641,9 @@ struct drm_plane_funcs { * @gamma_size: size of gamma table * @gamma_store: gamma correction table * @enabled: enabled flag + * @invert_dimensions: for purposes of error checking plane vs fb sizes, + * invert the width/height of the plane. This is used if the driver + * is performing 90 or 270 degree rotated scanout * @funcs: helper functions * @helper_private: storage for drver layer */ @@ -657,6 +665,7 @@ struct drm_plane { uint16_t *gamma_store; bool enabled; + bool invert_dimensions; const struct drm_plane_funcs *funcs; void *helper_private;
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(-)