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 ;-))
> 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@xxxxxx
> http://www.sci.fi/~syrjala/
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
