[PATCH v5 5/9] drm: Handle aspect-ratio info in getblob

[Nautiyal, Ankit K]

Hi Ville,

Thanks for your time for the review and the suggestions.

Please find my comments inline:


On 2/23/2018 7:52 PM, Ville Syrjälä wrote:
> On Thu, Feb 15, 2018 at 05:50:58PM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal at 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 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 at intel.com>
>> ---
>>   drivers/gpu/drm/drm_atomic.c   | 1 +
>>   drivers/gpu/drm/drm_property.c | 6 ++++++
>>   include/drm/drm_property.h     | 2 ++
>>   3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 46733d5..86b483e 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_property.c b/drivers/gpu/drm/drm_property.c
>> index bae50e6..639787c 100644
>> --- a/drivers/gpu/drm/drm_property.c
>> +++ b/drivers/gpu/drm/drm_property.c
>> @@ -746,6 +746,12 @@ int drm_mode_getblob_ioctl(struct drm_device *dev,
>>   	if (!blob)
>>   		return -ENOENT;
>>   
>> +	if (blob->is_video_mode && !file_priv->aspect_ratio_allowed) {
>> +		struct drm_mode_modeinfo *mode =
>> +			(struct drm_mode_modeinfo *) blob->data;
>> +		mode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> This is still clobbering the internal state. If another client with
> the aspect ratio cap comes in later and asks for the same blob we
> can no longer tell it what the aspect ratio was. So I think we have
> to make a temporary copy of the mode here.
>
> Also pointless parens.

Point taken. In that case, will it be alright to just clear the 
aspect-ratio bits in out_resp->data,
that is being sent to the user?
That way, the blob will retain the aspect ratio information.

Also, as per your comments in subsequent patches of the series, I would 
use a helper function
to check, if aspect-ratio info is allowed and for clearing the bits if 
aspect-ratio info is not
allowed.


Regards,
Ankit

>
>> +	}
>> +
>>   	if (out_resp->length == blob->length) {
>>   		if (copy_to_user(u64_to_user_ptr(out_resp->data),
>>   				 blob->data,
>> 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

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