On 3/9/2018 6:40 PM, Ville Syrjälä wrote:
> On Fri, Mar 09, 2018 at 11:07:14AM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> If the user-space does not support aspect-ratio, then getblob called
>> with the blob id of a user-mode, should clear the aspect-ratio
>> information in the blob data.
>>
>> Currently for a given blob id, there is no way to determine if the
>> blob stores user-mode or not. This can only be ascertained when the
>> blob is used for an atomic modeset call.
>>
>> This patch:
>> -adds a new field 'is_video_mode' in drm_property_blob to
>>   differentiate between the video mode blobs and the other blobs.
>> -sets the field 'is_video_mode' when the blob is used for modeset.
>> -removes the aspect-ratio info from the user-mode data if aspect-ratio
>>   is not supported by the user, while returning the blob to the user,
>>   in getblob ioctl.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> V6: As suggested by Ville:
>>      -added helper functions for determining if aspect-ratio is
>>       expected in user-mode and for allowing/disallowing the
>>       aspect-ratio, if its not expected.
>>      -avoided clobbering of blob-data, instead cleared the aspect-ratio
>>       in the user-mode only, so that another client with aspect-ratio
>>       cap, can still get the aspect-ratio information from getblob.
>> V7: Fixed warning [Wint-to-pointer-cast] for 32 bit platforms.
>> ---
>>   drivers/gpu/drm/drm_atomic.c   |  1 +
>>   drivers/gpu/drm/drm_modes.c    | 44 ++++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_property.c |  5 +++++
>>   include/drm/drm_modes.h        |  4 ++++
>>   include/drm/drm_property.h     |  2 ++
>>   5 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 34b7d42..2b1c88a 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -464,6 +464,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   	else if (property == config->prop_mode_id) {
>>   		struct drm_property_blob *mode =
>>   			drm_property_lookup_blob(dev, val);
>> +		mode->is_video_mode = true;
>>   		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>   		drm_property_blob_put(mode);
>>   		return ret;
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index a48672c..4430294 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -1761,3 +1761,47 @@ bool drm_mode_is_420(const struct drm_display_info *display,
>>   		drm_mode_is_420_also(display, mode);
>>   }
>>   EXPORT_SYMBOL(drm_mode_is_420);
>> +
>> +/**
>> + * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
>> + * is expected from the user-mode.
>> + *
>> + * If the user has set aspect-ratio cap, then the flag of the user-mode is
>> + * allowed to contain aspect-ratio value.
>> + * If the user does not set aspect-ratio cap, then the only value allowed in the
>> + * flags bits is aspect-ratio NONE.
>> + *
>> + * @file_priv: file private structure to get the user capabilities
>> + * @flags: 32 bit flag that contains the aspect-ratio information.
>> + *
>> + * Returns:
>> + * true if the aspect-ratio info is allowed in the user-mode flags.
>> + * false, otherwise.
>> + */
>> +bool
>> +drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv, uint32_t flags)
> I think we should pass the entire umode to these functions. That way the
> compiler will make sure we're passing in the correct thing instead of
> some other mode flags or random integers.

Agreed, that makes sense.
Will change parameter to the umode.
>
>> +{
>> +	return file_priv->aspect_ratio_allowed ||
>> +	       (flags & DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE;
>> +}
>> +EXPORT_SYMBOL(drm_mode_aspect_ratio_allowed);
>> +
>> +/**
>> + * drm_mode_handle_aspect_ratio - handles the aspect-ratio bits in the user-mode
>> + * flags.
>> + *
>> + * Checks if the aspect-ratio information is allowed. Resets the aspect-ratio
>> + * bits in the user-mode flag, if aspect-ratio info is not allowed.
>> + *
>> + * @file_priv: file private structure to get the user capabilities.
>> + * @flags: 32 bit flag that is to be modified, in case the aspect
>> + * ratio info is not allowed.
>> + *
>> + */
>> +void
>> +drm_mode_handle_aspect_ratio(const struct drm_file *file_priv, uint32_t *flags)
>> +{
>> +	if (!drm_mode_aspect_ratio_allowed(file_priv, *flags))
>> +		*flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
>> +}
>> +EXPORT_SYMBOL(drm_mode_handle_aspect_ratio);
> The function name could be more descriptive.
> drm_mode_filter_aspect_ratio_flags() or someting similar perhaps?

Point taken. I was a little worried about the length of the function name.
But having a meaningful name is important.
Will change this also, in the next patch set.

> Otherwise I think this is looking good.
>
> IIRC I did have a few more patches in my last inforame series to make
> sure we don't send invalid aspect ratio values in the infoframe. Unless
> something has changed I believe we still need those as well. Or am I
> wrong?

You are right, There are two patches:
https://patchwork.freedesktop.org/patch/188049/
and https://patchwork.freedesktop.org/patch/188051/
from the info-frame clean up series, both of which have already received 
r-b.

These patches need a little change, as they reject picture aspect ratio  
 > 16:9
Since we are adding support of 64:27 and 256:135, then we need to update 
the reject condition.
I can make the required changes and add these two patches as part of 
this series.

What do you suggest? Is it fine to make these changes?

Regards,
Ankit
>
>> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
>> index c37ac41..0a9c879 100644
>> --- a/drivers/gpu/drm/drm_property.c
>> +++ b/drivers/gpu/drm/drm_property.c
>> @@ -757,6 +757,11 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
>>   			ret = -EFAULT;
>>   			goto unref;
>>   		}
>> +		if (blob->is_video_mode) {
>> +			struct drm_mode_modeinfo __user *mode =
>> +				u64_to_user_ptr(out_resp->data);
>> +			drm_mode_handle_aspect_ratio(file_priv, &mode->flags);
>> +		}
>>   	}
>>   	out_resp->length = blob->length;
>>   unref:
>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>> index 2f78b7e..51d1188 100644
>> --- a/include/drm/drm_modes.h
>> +++ b/include/drm/drm_modes.h
>> @@ -461,6 +461,10 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
>>   			  const struct drm_display_mode *mode);
>>   bool drm_mode_is_420(const struct drm_display_info *display,
>>   		     const struct drm_display_mode *mode);
>> +bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
>> +				   uint32_t flags);
>> +void drm_mode_handle_aspect_ratio(const struct drm_file *file_priv,
>> +				  uint32_t *flags);
>>   
>>   struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
>>   				      int hdisplay, int vdisplay, int vrefresh,
>> diff --git a/include/drm/drm_property.h b/include/drm/drm_property.h
>> index 8a522b4..95e6e32 100644
>> --- a/include/drm/drm_property.h
>> +++ b/include/drm/drm_property.h
>> @@ -194,6 +194,7 @@ struct drm_property {
>>    * @head_global: entry on the global blob list in
>>    * 	&drm_mode_config.property_blob_list.
>>    * @head_file: entry on the per-file blob list in &drm_file.blobs list.
>> + * @is_video_mode: flag to mark the blobs that contain drm_mode_modeinfo.
>>    * @length: size of the blob in bytes, invariant over the lifetime of the object
>>    * @data: actual data, embedded at the end of this structure
>>    *
>> @@ -208,6 +209,7 @@ struct drm_property_blob {
>>   	struct drm_device *dev;
>>   	struct list_head head_global;
>>   	struct list_head head_file;
>> +	bool is_video_mode;
>>   	size_t length;
>>   	unsigned char data[];
>>   };
>> -- 
>> 2.7.4