Re: [PATCH v2 5/5] drm/tegra: plane: Support 180° rotation

[Dmitry Osipenko <digetx@gmail.com>]

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.