Re: [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4863 bytes --]

Hi Ville,

Thanks for the comments and suggestions.

Please find my response inline:


On 1/30/2018 12:28 AM, Ville Syrjälä wrote:
> On Fri, Jan 12, 2018 at 11:51:34AM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> We parse the EDID and add all the modes in the connector's
>> modelist. This adds CEA modes with aspect ratio information
>> too, regadless of if user space requested this information or
>> not.
>>
>> This patch prunes the modes with aspect-ratio information, from
>> a connector's modelist, if the user-space has not set the aspect
>> ratio DRM client cap.
>>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>> Cc: Jose Abreu <jose.abreu@synopsys.com>
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> V3: As suggested by Ville, modified the mechanism of pruning of
>> modes with aspect-ratio, if the aspect-ratio is not supported.
>> Instead of straight away pruning such a mode, the mode is
>> retained with aspect-ratio bits set to zero, provided it is
>> unique.
>> ---
>>   drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index b85a774..d968ec3 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1502,7 +1502,8 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne
>>   	return connector->encoder;
>>   }
>>   
>> -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>> +static bool drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode,
>> +					 const struct drm_display_mode *mode,
>>   					 const struct drm_file *file_priv)
>>   {
>>   	/*
>> @@ -1511,6 +1512,18 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>>   	 */
>>   	if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
>>   		return false;
>> +	/*
>> +	 * If user-space hasn't configured the driver to expose the modes
>> +	 * with aspect-ratio, don't expose them.
>> +	 */
>> +	if (!file_priv->aspect_ratio_allowed &&
>> +	    mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE &&
>> +	    drm_mode_match(mode, last_mode,
>> +			   DRM_MODE_MATCH_TIMINGS |
>> +			   DRM_MODE_MATCH_CLOCK |
>> +			   DRM_MODE_MATCH_FLAGS |
>> +			   DRM_MODE_MATCH_3D_FLAGS))
>> +		return false;
>>   
>>   	return true;
>>   }
>> @@ -1522,6 +1535,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   	struct drm_connector *connector;
>>   	struct drm_encoder *encoder;
>>   	struct drm_display_mode *mode;
>> +	struct drm_display_mode last_valid_mode;
> A pointer should be sufficient.

Thanks for pointing that out, will just save the address of last valid mode.

>
>>   	int mode_count = 0;
>>   	int encoders_count = 0;
>>   	int ret = 0;
>> @@ -1577,9 +1591,12 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   	out_resp->connection = connector->status;
>>   
>>   	/* delayed so we get modes regardless of pre-fill_modes state */
>> +	memset(&last_valid_mode, 0, sizeof(struct drm_display_mode));
>>   	list_for_each_entry(mode, &connector->modes, head)
>> -		if (drm_mode_expose_to_userspace(mode, file_priv))
>> +		if (drm_mode_expose_to_userspace(&last_valid_mode, mode, file_priv)) {
>>   			mode_count++;
>> +			drm_mode_copy(&last_valid_mode, mode);
>> +		}
>>   
>>   	/*
>>   	 * This ioctl is called twice, once to determine how much space is
>> @@ -1588,10 +1605,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>>   	if ((out_resp->count_modes >= mode_count) && mode_count) {
>>   		copied = 0;
>>   		mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
>> +		memset(&last_valid_mode, 0, sizeof(struct drm_display_mode));
>>   		list_for_each_entry(mode, &connector->modes, head) {
>> -			if (!drm_mode_expose_to_userspace(mode, file_priv))
>> +			if (!drm_mode_expose_to_userspace(&last_valid_mode,
>> +							  mode,
>> +							  file_priv))
>>   				continue;
>> -
>> +			if (!file_priv->aspect_ratio_allowed)
>> +				mode->picture_aspect_ratio =
>> +						HDMI_PICTURE_ASPECT_NONE;
> Here you're clobbering the internal mode structure. That's not
> acceptable.

I assumed, that the usermode, and the internal mode structure both 
should have aspect ratio
info as none, if the user does not support aspect ratio.
If that's not required, I can set the aspect ratio flags in usermode 
after the call
to drm_mode_convert_to_umode().

-Ankit


>
>> +			drm_mode_copy(&last_valid_mode, mode);
>>   			drm_mode_convert_to_umode(&u_mode, mode);
>>   			if (copy_to_user(mode_ptr + copied,
>>   					 &u_mode, sizeof(u_mode))) {
>> -- 
>> 2.7.4


[-- Attachment #1.2: Type: text/html, Size: 6241 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel