| Message ID | 1396722831-22761-1-git-send-email-notasas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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,
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(+)