diff mbox

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

Message ID 1515738096-16892-7-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nautiyal, Ankit K Jan. 12, 2018, 6:21 a.m. UTC 
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(-)

Comments

Ville Syrjälä Jan. 29, 2018, 6:58 p.m. UTC | #1 
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.

>  	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.

> +			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
Nautiyal, Ankit K Jan. 31, 2018, 8 a.m. UTC | #2 
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
diff mbox

Patch

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;
 	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;
+			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))) {