[Intel-gfx] [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
> Current DRM layer functions don't parse aspect ratio information
> while converting a user mode->kernel mode or vice versa. This
> causes modeset to pick mode with wrong aspect ratio, eventually
> causing failures in HDMI compliance test cases, due to wrong VIC.
> 
> This patch adds aspect ratio information in DRM's mode conversion
> and mode comparision functions, to make sure kernel picks mode
> with right aspect ratio (as per the VIC).
> 
> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> Signed-off-by: Lin, Jia <lin.a.jia at intel.com>
> Signed-off-by: Akashdeep Sharma <akashdeep.sharma at intel.com>
> Reviewed-by: Jim Bride <jim.bride at linux.intel.com>
> Reviewed-by: Jose Abreu <Jose.Abreu at synopsys.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Emil Velikov <emil.l.velikov at gmail.com>
> 
> V2: Addressed review comments from Sean:
> - Fix spellings/typo
> - No need to handle aspect ratio none
> - Add a break, for default case too
> V3: Rebase
> V4: Added r-b from Jose
> ---
>  drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 53f07ac..fde927a 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  	    mode1->vsync_end == mode2->vsync_end &&
>  	    mode1->vtotal == mode2->vtotal &&
>  	    mode1->vscan == mode2->vscan &&
> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
>  	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
>  	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
>  		return true;
> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  	out->vrefresh = in->vrefresh;
>  	out->flags = in->flags;
>  	out->type = in->type;
> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> +
> +	switch (in->picture_aspect_ratio) {
> +	case HDMI_PICTURE_ASPECT_4_3:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> +		break;
> +	case HDMI_PICTURE_ASPECT_16_9:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> +		break;
> +	case HDMI_PICTURE_ASPECT_RESERVED:
> +	default:
> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> +		break;
> +	}
> +
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  }
> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  
> +	/* Clearing picture aspect ratio bits from out flags */
> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> +
> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> +		break;
> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> +		break;
> +	default:
> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +		break;
> +	}

Hmm. So now we have the mode aspect ratio infromation duplicated
in two different places. Not sure that won't lead to some confusion.
Although we do want the user to be able to override via the property I
suppose, so we'd have to change that (+ the inforframe code) to
look at the mode flags instead if we would eliminate
'picture_aspect_ratio'.

And that brings me to the other point. At least i915 will simply
override the mode's aspect ration with the property. So this looks like
a big no-op for now on i915. It's looking like we might need a new
propetty value to differentiate between "auto" and "none" for real.

> +
>  	out->status = drm_mode_validate_basic(out);
>  	if (out->status != MODE_OK)
>  		goto out;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC