Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation
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 wrote:
>>>
>>> Combining horizontal and vertical reflections gives us 180 degrees of
>>> rotation.
>>>
>>> Signed-off-by: Dmitry Osipenko
>>> ---
>>> 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(>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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation
16.06.2020 00:47, Emil Velikov пишет:
> Hi all,
>
> Perhaps a silly question:
>
> On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko wrote:
>>
>> Combining horizontal and vertical reflections gives us 180 degrees of
>> rotation.
>>
>> Signed-off-by: Dmitry Osipenko
>> ---
>> 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(>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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation
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
>> ---
>> 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 = _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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation
Hi all,
Perhaps a silly question:
On Mon, 15 Jun 2020 at 08:28, Dmitry Osipenko wrote:
>
> Combining horizontal and vertical reflections gives us 180 degrees of
> rotation.
>
> Signed-off-by: Dmitry Osipenko
> ---
> 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(>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation
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
> ---
> 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 = _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(>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(>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(>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
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
