| Message ID | 20200614200121.14147-6-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 180 degrees rotation support for NVIDIA Tegra DRM | expand |
On Sun, Jun 14, 2020 at 11:01:21PM +0300, Dmitry Osipenko wrote: > Combining horizontal and vertical reflections gives us 180 degrees of > rotation. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/drm/tegra/dc.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index f31bca27cde4..ddd9b88f8fce 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c > @@ -608,6 +608,7 @@ static int tegra_plane_atomic_check(struct drm_plane *plane, > { > struct tegra_plane_state *plane_state = to_tegra_plane_state(state); > unsigned int rotation = DRM_MODE_ROTATE_0 | > + DRM_MODE_ROTATE_180 | Leave this out ... > DRM_MODE_REFLECT_X | > DRM_MODE_REFLECT_Y; > struct tegra_bo_tiling *tiling = &plane_state->tiling; > @@ -659,6 +660,14 @@ static int tegra_plane_atomic_check(struct drm_plane *plane, > else > plane_state->reflect_y = false; > > + if (tegra_fb_is_bottom_up(state->fb)) > + plane_state->reflect_y = true; > + > + if (rotation & DRM_MODE_ROTATE_180) { > + plane_state->reflect_x = !plane_state->reflect_x; > + plane_state->reflect_y = !plane_state->reflect_y; > + } ... and drm_rotation_simplify() will do this for you. Though the bottim_up() thing will need a slightly different tweak I guess. I'd write this as somehting like: rotation = state->rotation; if (bottom_up()) rotation ^= DRM_MODE_REFLECT_Y; rotation = drm_rotation_simplify(rotation, DRM_MODE_ROTATE_0 | DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y; Also note my use of XOR for the bottom_up() handling. I suspect the current code is already broken if you combine bottom_up() and REFLECT_Y since it just uses an OR instead of an XOR. That's assuming my hucnh what bottom_up() is supposed to do is correct. > + > /* > * Tegra doesn't support different strides for U and V planes so we > * error out if the user tries to display a framebuffer with such a > @@ -720,7 +729,7 @@ static void tegra_plane_atomic_update(struct drm_plane *plane, > window.dst.h = drm_rect_height(&plane->state->dst); > window.bits_per_pixel = fb->format->cpp[0] * 8; > window.reflect_x = state->reflect_x; > - window.reflect_y = tegra_fb_is_bottom_up(fb) || state->reflect_y; > + window.reflect_y = state->reflect_y; > > /* copy from state */ > window.zpos = plane->state->normalized_zpos; > @@ -806,6 +815,7 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm, > err = drm_plane_create_rotation_property(&plane->base, > DRM_MODE_ROTATE_0, > DRM_MODE_ROTATE_0 | > + DRM_MODE_ROTATE_180 | > DRM_MODE_REFLECT_X | > DRM_MODE_REFLECT_Y); > if (err < 0) > @@ -1094,6 +1104,7 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm, > err = drm_plane_create_rotation_property(&plane->base, > DRM_MODE_ROTATE_0, > DRM_MODE_ROTATE_0 | > + DRM_MODE_ROTATE_180 | > DRM_MODE_REFLECT_X | > DRM_MODE_REFLECT_Y); > if (err < 0) > -- > 2.26.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
15.06.2020 19:57, Ville Syrjälä пишет: > On Sun, Jun 14, 2020 at 11:01:21PM +0300, Dmitry Osipenko wrote: >> Combining horizontal and vertical reflections gives us 180 degrees of >> rotation. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/gpu/drm/tegra/dc.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c >> index f31bca27cde4..ddd9b88f8fce 100644 >> --- a/drivers/gpu/drm/tegra/dc.c >> +++ b/drivers/gpu/drm/tegra/dc.c >> @@ -608,6 +608,7 @@ static int tegra_plane_atomic_check(struct drm_plane *plane, >> { >> struct tegra_plane_state *plane_state = to_tegra_plane_state(state); >> unsigned int rotation = DRM_MODE_ROTATE_0 | >> + DRM_MODE_ROTATE_180 | > > Leave this out ... > >> DRM_MODE_REFLECT_X | >> DRM_MODE_REFLECT_Y; >> struct tegra_bo_tiling *tiling = &plane_state->tiling; >> @@ -659,6 +660,14 @@ static int tegra_plane_atomic_check(struct drm_plane *plane, >> else >> plane_state->reflect_y = false; >> >> + if (tegra_fb_is_bottom_up(state->fb)) >> + plane_state->reflect_y = true; >> + >> + if (rotation & DRM_MODE_ROTATE_180) { >> + plane_state->reflect_x = !plane_state->reflect_x; >> + plane_state->reflect_y = !plane_state->reflect_y; >> + } > > ... and drm_rotation_simplify() will do this for you. Hello Ville, Indeed, thank you for the suggestion! > Though the bottim_up() thing will need a slightly different tweak I > guess. > > I'd write this as somehting like: > rotation = state->rotation; > if (bottom_up()) > rotation ^= DRM_MODE_REFLECT_Y; > rotation = drm_rotation_simplify(rotation, > DRM_MODE_ROTATE_0 | > DRM_MODE_REFLECT_X | > DRM_MODE_REFLECT_Y; > > Also note my use of XOR for the bottom_up() handling. I suspect > the current code is already broken if you combine bottom_up() > and REFLECT_Y since it just uses an OR instead of an XOR. That's > assuming my hucnh what bottom_up() is supposed to do is correct. The bottom_up() is a legacy way of specifying reflect_y flag of the modern DRM's rotation property. It's was used by older userspace before Tegra DRM driver got support for the rotation property and we keep it today in order to maintain backwards compatibility with older userspace. Although, maybe it's about time to retire it since I don't think that such old userspace exists anymore. The legacy bottom_up() duplicates the modern reflect_y flag, so these are not mutually exclusive. Combining with yours suggestion above, we can write it in this way: /* * Older userspace used custom BO flag in order to specify * the Y reflection, while modern userspace uses the generic * DRM rotation property in order to achieve the same result. * The legacy BO flag amends the modern rotation property * when both are set. */ if (tegra_fb_is_bottom_up(state->fb)) rotation = drm_rotation_simplify(state->rotation | DRM_MODE_REFLECT_Y, rotation); else rotation = drm_rotation_simplify(state->rotation, rotation); Thank you very much for taking a look at this patch! I'll prepare v3 in accordance to yours comments.
Hi all, Perhaps a silly question: On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko <digetx@gmail.com> wrote: > > Combining horizontal and vertical reflections gives us 180 degrees of > rotation. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/gpu/drm/tegra/dc.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c > index f31bca27cde4..ddd9b88f8fce 100644 > --- a/drivers/gpu/drm/tegra/dc.c > +++ b/drivers/gpu/drm/tegra/dc.c > + if (rotation & DRM_MODE_ROTATE_180) { > + plane_state->reflect_x = !plane_state->reflect_x; > + plane_state->reflect_y = !plane_state->reflect_y; > + } > + As mentioned by Ville the above is already handled by drm_rotation_simplify() ... although it makes me wonder: > err = drm_plane_create_rotation_property(&plane->base, > DRM_MODE_ROTATE_0, > DRM_MODE_ROTATE_0 | > + DRM_MODE_ROTATE_180 | > DRM_MODE_REFLECT_X | > DRM_MODE_REFLECT_Y); Would it make sense for drm_plane_create_rotation_property() itself, to add DRM_MODE_ROTATE_180, when both reflections are supported? -Emil
16.06.2020 00:47, Emil Velikov пишет: > Hi all, > > Perhaps a silly question: > > On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko <digetx@gmail.com> wrote: >> >> Combining horizontal and vertical reflections gives us 180 degrees of >> rotation. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/gpu/drm/tegra/dc.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c >> index f31bca27cde4..ddd9b88f8fce 100644 >> --- a/drivers/gpu/drm/tegra/dc.c >> +++ b/drivers/gpu/drm/tegra/dc.c > >> + if (rotation & DRM_MODE_ROTATE_180) { >> + plane_state->reflect_x = !plane_state->reflect_x; >> + plane_state->reflect_y = !plane_state->reflect_y; >> + } >> + > As mentioned by Ville the above is already handled by > drm_rotation_simplify() ... although it makes me wonder: > > >> err = drm_plane_create_rotation_property(&plane->base, >> DRM_MODE_ROTATE_0, >> DRM_MODE_ROTATE_0 | >> + DRM_MODE_ROTATE_180 | >> DRM_MODE_REFLECT_X | >> DRM_MODE_REFLECT_Y); > > Would it make sense for drm_plane_create_rotation_property() itself, > to add DRM_MODE_ROTATE_180, when both reflections are supported? Hello Emil, That's a good point! All DRM_MODE_ROTATE_180 should be removed because Tegra can't do 180° + reflected-x. The DRM core takes care of 180° rotation when both x/y reflections are supported.
16.06.2020 14:25, Dmitry Osipenko пишет: > 16.06.2020 00:47, Emil Velikov пишет: >> Hi all, >> >> Perhaps a silly question: >> >> On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko <digetx@gmail.com> wrote: >>> >>> Combining horizontal and vertical reflections gives us 180 degrees of >>> rotation. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/gpu/drm/tegra/dc.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c >>> index f31bca27cde4..ddd9b88f8fce 100644 >>> --- a/drivers/gpu/drm/tegra/dc.c >>> +++ b/drivers/gpu/drm/tegra/dc.c >> >>> + if (rotation & DRM_MODE_ROTATE_180) { >>> + plane_state->reflect_x = !plane_state->reflect_x; >>> + plane_state->reflect_y = !plane_state->reflect_y; >>> + } >>> + >> As mentioned by Ville the above is already handled by >> drm_rotation_simplify() ... although it makes me wonder: >> >> >>> err = drm_plane_create_rotation_property(&plane->base, >>> DRM_MODE_ROTATE_0, >>> DRM_MODE_ROTATE_0 | >>> + DRM_MODE_ROTATE_180 | >>> DRM_MODE_REFLECT_X | >>> DRM_MODE_REFLECT_Y); >> >> Would it make sense for drm_plane_create_rotation_property() itself, >> to add DRM_MODE_ROTATE_180, when both reflections are supported? > > Hello Emil, > > That's a good point! All DRM_MODE_ROTATE_180 should be removed because > Tegra can't do 180° + reflected-x. The DRM core takes care of 180° > rotation when both x/y reflections are supported. > I just found out that I forgot to drop the WIP patches which added transparent rotation support while was checking whether these plane DRM_MODE_ROTATE_180 could be dropped and it's actually need to be set for the planes, otherwise 180 rotation support is filtered out by the atomic check.
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index f31bca27cde4..ddd9b88f8fce 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -608,6 +608,7 @@ static int tegra_plane_atomic_check(struct drm_plane *plane, { struct tegra_plane_state *plane_state = to_tegra_plane_state(state); unsigned int rotation = DRM_MODE_ROTATE_0 | + DRM_MODE_ROTATE_180 | DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y; struct tegra_bo_tiling *tiling = &plane_state->tiling; @@ -659,6 +660,14 @@ static int tegra_plane_atomic_check(struct drm_plane *plane, else plane_state->reflect_y = false; + if (tegra_fb_is_bottom_up(state->fb)) + plane_state->reflect_y = true; + + if (rotation & DRM_MODE_ROTATE_180) { + plane_state->reflect_x = !plane_state->reflect_x; + plane_state->reflect_y = !plane_state->reflect_y; + } + /* * Tegra doesn't support different strides for U and V planes so we * error out if the user tries to display a framebuffer with such a @@ -720,7 +729,7 @@ static void tegra_plane_atomic_update(struct drm_plane *plane, window.dst.h = drm_rect_height(&plane->state->dst); window.bits_per_pixel = fb->format->cpp[0] * 8; window.reflect_x = state->reflect_x; - window.reflect_y = tegra_fb_is_bottom_up(fb) || state->reflect_y; + window.reflect_y = state->reflect_y; /* copy from state */ window.zpos = plane->state->normalized_zpos; @@ -806,6 +815,7 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm, err = drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0, DRM_MODE_ROTATE_0 | + DRM_MODE_ROTATE_180 | DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y); if (err < 0) @@ -1094,6 +1104,7 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm, err = drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0, DRM_MODE_ROTATE_0 | + DRM_MODE_ROTATE_180 | DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y); if (err < 0)
Combining horizontal and vertical reflections gives us 180 degrees of rotation. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/gpu/drm/tegra/dc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)