[PATCH] drm/omap: fix plane rotation
On 08/04/14 01:12, Rob Clark wrote:
> On Mon, Apr 7, 2014 at 5:41 PM, Grazvydas Ignotas
> wrote:
>> Gra?vydas
>>
>>
>> On Mon, Apr 7, 2014 at 8:17 PM, Rob Clark wrote:
>>> On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas
>>> wrote:
On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark wrote:
> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas
> wrote:
>> Plane rotation with omapdrm is currently broken.
>> It seems omap_plane_mode_set() expects width and height in screen
>> coordinates, so pass it like that.
>>
>> Cc: Rob Clark
>> Signed-off-by: Grazvydas Ignotas
>> ---
>> drivers/gpu/drm/omapdrm/omap_plane.c |8
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>> b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index 370580c..5611f15 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane
>> *plane,
>>
>> drm_framebuffer_reference(fb);
>>
>> + /* omap_plane_mode_set() takes adjusted src */
>> + switch (omap_plane->win.rotation & 0xf) {
>> + case BIT(DRM_ROTATE_90):
>> + case BIT(DRM_ROTATE_270):
>> + swap(src_w, src_h);
>> + break;
>> + }
>> +
>
> hmm, I think that the better thing would be to do this in
> omap_framebuffer_update_scanout(). In fact we do already swap
> src_w/src_h there. Only we don't swap the width/height parameters we
> pass down to omapdss. Not quite sure if something changed in omapdss
> with regards to rotation_type, but keeping it with the rest of the
> rotation related maths in _update_scanout() seems cleaner.
But then it has to know somehow if apply was triggered from
omap_crtc_mode_set() or omap_plane_update(). The problem is
omap_crtc_mode_set() passes rotated width/height (and crtc rotation
works correctly), but omap_plane_update() uses unrotated width/height
(so plane rotation is currently broken).
>>>
>>>
>>> Something still seems a bit suspicious.. drm core is not swapping
>>> width/height for crtc (other than for validating the configuration...
>>> which might also be needed for planes). I guess it is getting
>>> already-rotated width/height from userspace. So probably we want to
>>> do the same for planes, so it might be a good idea to move
>>> crtc->invert_dimensions into plane.
>>
>> OK I guess I said it wrong..
>> The display I have is 1080x1920 portrait panel.
>> For omap_crtc_mode_set(), hdisplay is always 1080 and vdisplay, is
>> 1920, regardless of rotation. That is passed over to
>> omap_plane_mode_set() and everything works.
>
> ahh, ok, that makes much more sense.. (sorry, it's been a long time
> since I looked at rotation)
>
> Reviewed-by: Rob Clark
I can queue this up in my omapdrm fixes branch. I already have a few there.
Tomi
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL:
[PATCH] drm/omap: fix plane rotation
Gra?vydas
On Mon, Apr 7, 2014 at 8:17 PM, Rob Clark wrote:
> On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas
> wrote:
>> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark wrote:
>>> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas
>>> wrote:
Plane rotation with omapdrm is currently broken.
It seems omap_plane_mode_set() expects width and height in screen
coordinates, so pass it like that.
Cc: Rob Clark
Signed-off-by: Grazvydas Ignotas
---
drivers/gpu/drm/omapdrm/omap_plane.c |8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
b/drivers/gpu/drm/omapdrm/omap_plane.c
index 370580c..5611f15 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane *plane,
drm_framebuffer_reference(fb);
+ /* omap_plane_mode_set() takes adjusted src */
+ switch (omap_plane->win.rotation & 0xf) {
+ case BIT(DRM_ROTATE_90):
+ case BIT(DRM_ROTATE_270):
+ swap(src_w, src_h);
+ break;
+ }
+
>>>
>>> hmm, I think that the better thing would be to do this in
>>> omap_framebuffer_update_scanout(). In fact we do already swap
>>> src_w/src_h there. Only we don't swap the width/height parameters we
>>> pass down to omapdss. Not quite sure if something changed in omapdss
>>> with regards to rotation_type, but keeping it with the rest of the
>>> rotation related maths in _update_scanout() seems cleaner.
>>
>> But then it has to know somehow if apply was triggered from
>> omap_crtc_mode_set() or omap_plane_update(). The problem is
>> omap_crtc_mode_set() passes rotated width/height (and crtc rotation
>> works correctly), but omap_plane_update() uses unrotated width/height
>> (so plane rotation is currently broken).
>
>
> Something still seems a bit suspicious.. drm core is not swapping
> width/height for crtc (other than for validating the configuration...
> which might also be needed for planes). I guess it is getting
> already-rotated width/height from userspace. So probably we want to
> do the same for planes, so it might be a good idea to move
> crtc->invert_dimensions into plane.
OK I guess I said it wrong..
The display I have is 1080x1920 portrait panel.
For omap_crtc_mode_set(), hdisplay is always 1080 and vdisplay, is
1920, regardless of rotation. That is passed over to
omap_plane_mode_set() and everything works.
But in omap_plane_update(), src_w and src_h are passed instead, which
correspond to framebuffer size and not display? At least
drmModeSetPlane(), where those values originate, seems to expect
framebuffer dimensions, but passing 1920, 1080 there (with 90 deg.
rotation set) results in corruption on screen caused by bad base
address for hardware overlay without my patch.
Gra?vydas
[PATCH] drm/omap: fix plane rotation
On Mon, Apr 7, 2014 at 5:41 PM, Grazvydas Ignotas wrote:
> Gra?vydas
>
>
> On Mon, Apr 7, 2014 at 8:17 PM, Rob Clark wrote:
>> On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas
>> wrote:
>>> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark wrote:
On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas
wrote:
> Plane rotation with omapdrm is currently broken.
> It seems omap_plane_mode_set() expects width and height in screen
> coordinates, so pass it like that.
>
> Cc: Rob Clark
> Signed-off-by: Grazvydas Ignotas
> ---
> drivers/gpu/drm/omapdrm/omap_plane.c |8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 370580c..5611f15 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane *plane,
>
> drm_framebuffer_reference(fb);
>
> + /* omap_plane_mode_set() takes adjusted src */
> + switch (omap_plane->win.rotation & 0xf) {
> + case BIT(DRM_ROTATE_90):
> + case BIT(DRM_ROTATE_270):
> + swap(src_w, src_h);
> + break;
> + }
> +
hmm, I think that the better thing would be to do this in
omap_framebuffer_update_scanout(). In fact we do already swap
src_w/src_h there. Only we don't swap the width/height parameters we
pass down to omapdss. Not quite sure if something changed in omapdss
with regards to rotation_type, but keeping it with the rest of the
rotation related maths in _update_scanout() seems cleaner.
>>>
>>> But then it has to know somehow if apply was triggered from
>>> omap_crtc_mode_set() or omap_plane_update(). The problem is
>>> omap_crtc_mode_set() passes rotated width/height (and crtc rotation
>>> works correctly), but omap_plane_update() uses unrotated width/height
>>> (so plane rotation is currently broken).
>>
>>
>> Something still seems a bit suspicious.. drm core is not swapping
>> width/height for crtc (other than for validating the configuration...
>> which might also be needed for planes). I guess it is getting
>> already-rotated width/height from userspace. So probably we want to
>> do the same for planes, so it might be a good idea to move
>> crtc->invert_dimensions into plane.
>
> OK I guess I said it wrong..
> The display I have is 1080x1920 portrait panel.
> For omap_crtc_mode_set(), hdisplay is always 1080 and vdisplay, is
> 1920, regardless of rotation. That is passed over to
> omap_plane_mode_set() and everything works.
ahh, ok, that makes much more sense.. (sorry, it's been a long time
since I looked at rotation)
Reviewed-by: Rob Clark
> But in omap_plane_update(), src_w and src_h are passed instead, which
> correspond to framebuffer size and not display? At least
> drmModeSetPlane(), where those values originate, seems to expect
> framebuffer dimensions, but passing 1920, 1080 there (with 90 deg.
> rotation set) results in corruption on screen caused by bad base
> address for hardware overlay without my patch.
>
>
> Gra?vydas
[PATCH] drm/omap: fix plane rotation
On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark wrote:
> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas
> wrote:
>> Plane rotation with omapdrm is currently broken.
>> It seems omap_plane_mode_set() expects width and height in screen
>> coordinates, so pass it like that.
>>
>> Cc: Rob Clark
>> Signed-off-by: Grazvydas Ignotas
>> ---
>> drivers/gpu/drm/omapdrm/omap_plane.c |8
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>> b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index 370580c..5611f15 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane *plane,
>>
>> drm_framebuffer_reference(fb);
>>
>> + /* omap_plane_mode_set() takes adjusted src */
>> + switch (omap_plane->win.rotation & 0xf) {
>> + case BIT(DRM_ROTATE_90):
>> + case BIT(DRM_ROTATE_270):
>> + swap(src_w, src_h);
>> + break;
>> + }
>> +
>
> hmm, I think that the better thing would be to do this in
> omap_framebuffer_update_scanout(). In fact we do already swap
> src_w/src_h there. Only we don't swap the width/height parameters we
> pass down to omapdss. Not quite sure if something changed in omapdss
> with regards to rotation_type, but keeping it with the rest of the
> rotation related maths in _update_scanout() seems cleaner.
But then it has to know somehow if apply was triggered from
omap_crtc_mode_set() or omap_plane_update(). The problem is
omap_crtc_mode_set() passes rotated width/height (and crtc rotation
works correctly), but omap_plane_update() uses unrotated width/height
(so plane rotation is currently broken).
Gra?vydas
[PATCH] drm/omap: fix plane rotation
On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas wrote:
> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark wrote:
>> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas
>> wrote:
>>> Plane rotation with omapdrm is currently broken.
>>> It seems omap_plane_mode_set() expects width and height in screen
>>> coordinates, so pass it like that.
>>>
>>> Cc: Rob Clark
>>> Signed-off-by: Grazvydas Ignotas
>>> ---
>>> drivers/gpu/drm/omapdrm/omap_plane.c |8
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>>> b/drivers/gpu/drm/omapdrm/omap_plane.c
>>> index 370580c..5611f15 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>>> @@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane *plane,
>>>
>>> drm_framebuffer_reference(fb);
>>>
>>> + /* omap_plane_mode_set() takes adjusted src */
>>> + switch (omap_plane->win.rotation & 0xf) {
>>> + case BIT(DRM_ROTATE_90):
>>> + case BIT(DRM_ROTATE_270):
>>> + swap(src_w, src_h);
>>> + break;
>>> + }
>>> +
>>
>> hmm, I think that the better thing would be to do this in
>> omap_framebuffer_update_scanout(). In fact we do already swap
>> src_w/src_h there. Only we don't swap the width/height parameters we
>> pass down to omapdss. Not quite sure if something changed in omapdss
>> with regards to rotation_type, but keeping it with the rest of the
>> rotation related maths in _update_scanout() seems cleaner.
>
> But then it has to know somehow if apply was triggered from
> omap_crtc_mode_set() or omap_plane_update(). The problem is
> omap_crtc_mode_set() passes rotated width/height (and crtc rotation
> works correctly), but omap_plane_update() uses unrotated width/height
> (so plane rotation is currently broken).
Something still seems a bit suspicious.. drm core is not swapping
width/height for crtc (other than for validating the configuration...
which might also be needed for planes). I guess it is getting
already-rotated width/height from userspace. So probably we want to
do the same for planes, so it might be a good idea to move
crtc->invert_dimensions into plane.
BR,
-R.
>
> Gra?vydas
[PATCH] drm/omap: fix plane rotation
Plane rotation with omapdrm is currently broken.
It seems omap_plane_mode_set() expects width and height in screen
coordinates, so pass it like that.
Cc: Rob Clark
Signed-off-by: Grazvydas Ignotas
---
drivers/gpu/drm/omapdrm/omap_plane.c |8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
b/drivers/gpu/drm/omapdrm/omap_plane.c
index 370580c..5611f15 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane *plane,
drm_framebuffer_reference(fb);
+ /* omap_plane_mode_set() takes adjusted src */
+ switch (omap_plane->win.rotation & 0xf) {
+ case BIT(DRM_ROTATE_90):
+ case BIT(DRM_ROTATE_270):
+ swap(src_w, src_h);
+ break;
+ }
+
return omap_plane_mode_set(plane, crtc, fb,
crtc_x, crtc_y, crtc_w, crtc_h,
src_x, src_y, src_w, src_h,
--
1.7.9.5
[PATCH] drm/omap: fix plane rotation
On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas wrote:
> Plane rotation with omapdrm is currently broken.
> It seems omap_plane_mode_set() expects width and height in screen
> coordinates, so pass it like that.
>
> Cc: Rob Clark
> Signed-off-by: Grazvydas Ignotas
> ---
> drivers/gpu/drm/omapdrm/omap_plane.c |8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 370580c..5611f15 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -253,6 +253,14 @@ static int omap_plane_update(struct drm_plane *plane,
>
> drm_framebuffer_reference(fb);
>
> + /* omap_plane_mode_set() takes adjusted src */
> + switch (omap_plane->win.rotation & 0xf) {
> + case BIT(DRM_ROTATE_90):
> + case BIT(DRM_ROTATE_270):
> + swap(src_w, src_h);
> + break;
> + }
> +
hmm, I think that the better thing would be to do this in
omap_framebuffer_update_scanout(). In fact we do already swap
src_w/src_h there. Only we don't swap the width/height parameters we
pass down to omapdss. Not quite sure if something changed in omapdss
with regards to rotation_type, but keeping it with the rest of the
rotation related maths in _update_scanout() seems cleaner.
BR,
-R
> return omap_plane_mode_set(plane, crtc, fb,
> crtc_x, crtc_y, crtc_w, crtc_h,
> src_x, src_y, src_w, src_h,
> --
> 1.7.9.5
>
