On Sat, Mar 31, 2012 at 4:09 PM, Ville Syrj?l? <syrjala at sci.fi> wrote:
> On Fri, Mar 30, 2012 at 05:59:32PM -0500, Rob Clark wrote:
>> From: Rob Clark <rob at 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 at sci.fi
> http://www.sci.fi/~syrjala/
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
