[PATCH] drm/omap: fix plane rotation

[PATCH] drm/omap: fix plane rotation

[Grazvydas Ignotas]

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 <robdclark@gmail.com>
Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 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

Re: [PATCH] drm/omap: fix plane rotation

[Rob Clark]

On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas <notasas@gmail.com> 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 <robdclark@gmail.com>
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
>  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
>

Re: [PATCH] drm/omap: fix plane rotation

[Grazvydas Ignotas]

On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas <notasas@gmail.com> 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 <robdclark@gmail.com>
>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>> ---
>>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omap: fix plane rotation

[Rob Clark]

On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark <robdclark@gmail.com> wrote:
>> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas <notasas@gmail.com> 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 <robdclark@gmail.com>
>>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>>> ---
>>>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omap: fix plane rotation

[Grazvydas Ignotas]

Gražvydas


On Mon, Apr 7, 2014 at 8:17 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark <robdclark@gmail.com> wrote:
>>> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas <notasas@gmail.com> 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 <robdclark@gmail.com>
>>>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>>>> ---
>>>>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omap: fix plane rotation

[Rob Clark]

On Mon, Apr 7, 2014 at 5:41 PM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> Gražvydas
>
>
> On Mon, Apr 7, 2014 at 8:17 PM, Rob Clark <robdclark@gmail.com> wrote:
>> On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>>> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark <robdclark@gmail.com> wrote:
>>>> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas <notasas@gmail.com> 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 <robdclark@gmail.com>
>>>>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>>>>> ---
>>>>>  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 <robdclark@gmail.com>


> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/omap: fix plane rotation

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 3251 bytes --]

On 08/04/14 01:12, Rob Clark wrote:
> On Mon, Apr 7, 2014 at 5:41 PM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>> Gražvydas
>>
>>
>> On Mon, Apr 7, 2014 at 8:17 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> On Mon, Apr 7, 2014 at 6:32 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>>>> On Sun, Apr 6, 2014 at 12:45 AM, Rob Clark <robdclark@gmail.com> wrote:
>>>>> On Sat, Apr 5, 2014 at 2:33 PM, Grazvydas Ignotas <notasas@gmail.com> 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 <robdclark@gmail.com>
>>>>>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>>>>>> ---
>>>>>>  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 <robdclark@gmail.com>

I can queue this up in my omapdrm fixes branch. I already have a few there.

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel