Re: [PATCH v2] drm/modes: Apply video parameters with only reflect option

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Dec 16, 2019 at 06:10:17PM +0100, Stephan Gerhold wrote:
> At the moment, video mode parameters like video=540x960,reflect_x,
> (without rotation set) are not taken into account when applying the
> mode in drm_client_modeset.c.

A rotation value without exactly one rotation angle is illegal.
IMO the code should not generate a value like that in the first
place.

> 
> One of the reasons for this is that the calculation that
> combines the panel_orientation with cmdline->rotation_reflection
> does not handle the case when cmdline->rotation_reflection does
> not have any rotation set.
> (i.e. cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK == 0)
> 
> Example:
>   *rotation = DRM_MODE_ROTATE_0 (no panel_orientation)
>   cmdline->rotation_reflection = DRM_MODE_REFLECT_X (video=MODE,reflect_x)
> 
> The current code does:
>   panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
>   cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
>   sum_rot = (panel_rot + cmdline_rot) % 4;
> 
> and therefore:
>   panel_rot = ilog2(DRM_MODE_ROTATE_0) = ilog2(1) = 0
>   cmdline_rot = ilog2(0) = -1
>   sum_rot = (0 + -1) % 4 = -1 % 4 = 3
>    ...
>   *rotation = DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X
> 
> So we incorrectly generate DRM_MODE_ROTATE_270 in this case.
> To prevent cmdline_rot from becoming -1, we need to treat
> the rotation as DRM_MODE_ROTATE_0.
> 
> On the other hand, there is no need to go through that calculation
> at all if no rotation is set in rotation_reflection.
> A simple XOR is enough to combine the reflections.
> 
> Finally, also allow DRM_MODE_ROTATE_0 in the if statement below.
> DRM_MODE_ROTATE_0 means "no rotation" and should therefore not
> require any special handling (e.g. specific tiling format).
> 
> This makes video parameters with only reflect option work correctly.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> v1: https://lists.freedesktop.org/archives/dri-devel/2019-December/248145.html
> 
> Changes in v2:
>   - Clarified commit message - parameters are parsed correctly,
>     but not taken into account when applying the mode.
> ---
>  drivers/gpu/drm/drm_client_modeset.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 895b73f23079..cfebce4f19a5 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -859,19 +859,23 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>  	 */
>  	cmdline = &connector->cmdline_mode;
>  	if (cmdline->specified && cmdline->rotation_reflection) {
> -		unsigned int cmdline_rest, panel_rest;
> -		unsigned int cmdline_rot, panel_rot;
> -		unsigned int sum_rot, sum_rest;
> +		if (cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK) {
> +			unsigned int cmdline_rest, panel_rest;
> +			unsigned int cmdline_rot, panel_rot;
> +			unsigned int sum_rot, sum_rest;
>  
> -		panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> -		cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> -		sum_rot = (panel_rot + cmdline_rot) % 4;
> +			panel_rot = ilog2(*rotation & DRM_MODE_ROTATE_MASK);
> +			cmdline_rot = ilog2(cmdline->rotation_reflection & DRM_MODE_ROTATE_MASK);
> +			sum_rot = (panel_rot + cmdline_rot) % 4;
>  
> -		panel_rest = *rotation & ~DRM_MODE_ROTATE_MASK;
> -		cmdline_rest = cmdline->rotation_reflection & ~DRM_MODE_ROTATE_MASK;
> -		sum_rest = panel_rest ^ cmdline_rest;
> +			panel_rest = *rotation & ~DRM_MODE_ROTATE_MASK;
> +			cmdline_rest = cmdline->rotation_reflection & ~DRM_MODE_ROTATE_MASK;
> +			sum_rest = panel_rest ^ cmdline_rest;
>  
> -		*rotation = (1 << sum_rot) | sum_rest;
> +			*rotation = (1 << sum_rot) | sum_rest;
> +		} else {
> +			*rotation ^= cmdline->rotation_reflection;
> +		}
>  	}
>  
>  	/*
> @@ -879,7 +883,8 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation)
>  	 * depending on the hardware this may require the framebuffer
>  	 * to be in a specific tiling format.
>  	 */
> -	if ((*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_180 ||
> +	if (((*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_0 &&
> +	     (*rotation & DRM_MODE_ROTATE_MASK) != DRM_MODE_ROTATE_180) ||
>  	    !plane->rotation_property)
>  		return false;
>  
> -- 
> 2.24.1
> 
> _______________________________________________
> 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