[PATCH v5 8/9] drm: Add aspect ratio parsing in DRM layer

[Nautiyal, Ankit K]

On 2/23/2018 8:24 PM, Ville Syrjälä wrote:
> On Thu, Feb 15, 2018 at 05:51:01PM +0530, Nautiyal, Ankit K wrote:
>> From: "Sharma, Shashank" <shashank.sharma at intel.com>
>>
>> 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).
>>
>> Background:
>> This patch was once reviewed and merged, and later reverted due to
>> lack of DRM cap protection. This is a re-spin of this patch, this
>> time with DRM cap protection, to avoid aspect ratio information, when
>> the client doesn't request for it.
>>
>> Review link: https://pw-emeril.freedesktop.org/patch/104068/
>> Background discussion: https://patchwork.kernel.org/patch/9379057/
>>
>> 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> (V2)
>> Reviewed-by: Jose Abreu <Jose.Abreu at synopsys.com> (V4)
>>
>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Cc: Jim Bride <jim.bride at linux.intel.com>
>> Cc: Jose Abreu <Jose.Abreu at synopsys.com>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>
>> V3: modified the aspect-ratio check in drm_mode_equal as per new flags
>>      provided by Ville. https://patchwork.freedesktop.org/patch/188043/
>> V4: rebase
>> V5: rebase
>> ---
>>   drivers/gpu/drm/drm_modes.c | 33 ++++++++++++++++++++++++++++++++-
>>   1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index ca4c5cc..25140b9 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1050,7 +1050,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
>>   			      DRM_MODE_MATCH_TIMINGS |
>>   			      DRM_MODE_MATCH_CLOCK |
>>   			      DRM_MODE_MATCH_FLAGS |
>> -			      DRM_MODE_MATCH_3D_FLAGS);
>> +			      DRM_MODE_MATCH_3D_FLAGS|
>> +			      DRM_MODE_MATCH_ASPECT_RATIO);
> Hmm. I wonder if all the users are expecting this. Based on a cursory
> examination drm_fb_helper might want to ignore the aspect ratio since
> it's just looking to clone the same mode on multiple outputs. The other
> cases don't look to me like they should suffer badly from this.

Agreed.
Need to add a function drm_mode_equal_no_aspect_ratio() in drm_modes.c 
which will compare
every field, other than the aspect-ratio.
Next, in the drm_target_cloned( ), where same modes are to be cloned on 
multiple outputs, instead
of drm_mode_equal( ), drm_mode_equal_no_aspect_ratio( ) must be called.

Is this approach correct?

>
>>   }
>>   EXPORT_SYMBOL(drm_mode_equal);
>>   
>> @@ -1649,6 +1650,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;
> This looks redundant. The internal mode should not have the aspect ratio
> flags set. Or are we changing that?

Yes right, the aspect-ratio flags are never set in the internal-mode flags.
This line can be dropped.

>
>> +
>> +	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;
>>   }
>> @@ -1693,6 +1709,21 @@ int drm_mode_convert_umode(struct drm_device *dev,
>>   	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 */
> What the code is doing is obvious so this comment is redundant.
> The non-obvious part (ie. the "why?") is what the comment
> should contain.

Thanks for pointing that out, will take care of this in future patches.

Regards,
Ankit

>
>> +	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;
>> +	}
>> +
>>   	out->status = drm_mode_validate_driver(dev, out);
>>   	if (out->status != MODE_OK)
>>   		goto out;
>> -- 
>> 2.7.4

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180226/a38bb14a/attachment.html>