[PATCH v12 08/10] drm: Expose modes with aspect ratio, only if requested
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon Apr 30 06:52:56 UTC 2018
On 4/27/2018 7:35 PM, Ville Syrjälä wrote:
> On Fri, Apr 27, 2018 at 05:44:54PM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal at 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
>> whether 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. However if such a mode is unique in the list, it is
>> kept in the list, with aspect-ratio flags reset.
>>
>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>> Cc: Jose Abreu <jose.abreu at synopsys.com>
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at 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.
>> V4: rebase
>> V5: Addressed review comments from Ville:
>> -used a pointer to store last valid mode.
>> -avoided, modifying of picture_aspect_ratio in kernel mode,
>> instead only flags bits of user mode are reset (if aspect-ratio
>> is not supported).
>> V6: As suggested by Ville, corrected the mode pruning logic and
>> elaborated the mode pruning logic and the assumptions taken.
>> V7: rebase
>> V8: rebase
>> V9: rebase
>> V10: rebase
>> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
>> logic to correctly identify and prune modes with aspect-ratio,
>> if aspect-ratio cap is not set.
>> V12: As suggested by Ville, added another list_head in
>> drm_mode_display to traverse the list of exposed modes and
>> avoided duplication of modes.
>> ---
>> drivers/gpu/drm/drm_connector.c | 52 ++++++++++++++++++++++++++++++++++-------
>> include/drm/drm_modes.h | 9 +++++++
>> 2 files changed, 53 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index b3cde89..e12964b 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1531,15 +1531,35 @@ 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,
>> - const struct drm_file *file_priv)
>> +static bool
>> +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
>> + const struct list_head *modelist,
>> + const struct drm_file *file_priv)
>> {
>> /*
>> * If user-space hasn't configured the driver to expose the stereo 3D
>> * modes, don't expose them.
>> */
>> + struct drm_display_mode *mode_itr;
>> +
> This is clearly misplaced. It should be before the comment. Hmm,
> atually better move it into the aspect ratio if block since it's
> not needed elsewhere.
That makes sense, will move it to the aspect-ratio block.
>
>> 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. However if such a mode
>> + * is unique, let it be exposed, but reset the aspect-ratio flags
>> + * while preparing the list of user-modes.
>> + */
>> + if (!file_priv->aspect_ratio_allowed &&
>> + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
>> + list_for_each_entry(mode_itr, modelist, exp_head)
>> + if (drm_mode_match(mode_itr, mode,
>> + DRM_MODE_MATCH_TIMINGS |
>> + DRM_MODE_MATCH_CLOCK |
>> + DRM_MODE_MATCH_FLAGS |
>> + DRM_MODE_MATCH_3D_FLAGS))
>> + return false;
>> + }
>>
>> return true;
>> }
>> @@ -1550,7 +1570,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>> struct drm_mode_get_connector *out_resp = data;
>> struct drm_connector *connector;
>> struct drm_encoder *encoder;
>> - struct drm_display_mode *mode;
>> + struct drm_display_mode *mode, *tmp;
>> + struct list_head modelist;
> LIST_HEAD_INIT(modelist);
>
> And maybe call it export_list or something like that.
Alright. Will change the name to export_list.
I hope you mean LIST_HEAD(export_list);
>> int mode_count = 0;
>> int encoders_count = 0;
>> int ret = 0;
>> @@ -1605,23 +1626,34 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>> out_resp->subpixel = connector->display_info.subpixel_order;
>> out_resp->connection = connector->status;
>>
>> + INIT_LIST_HEAD(&modelist);
>> +
>> /* delayed so we get modes regardless of pre-fill_modes state */
>> list_for_each_entry(mode, &connector->modes, head)
>> - if (drm_mode_expose_to_userspace(mode, file_priv))
>> + if (drm_mode_expose_to_userspace(mode, &modelist,
>> + file_priv)) {
>> + list_add_tail(&mode->exp_head, &modelist);
>> mode_count++;
>> + }
>>
>> /*
>> * This ioctl is called twice, once to determine how much space is
>> * needed, and the 2nd time to fill it.
>> + * The modes that need to be exposed to the user are maintained in the
>> + * 'modelist'. When the ioctl is called first time to determine the,
>> + * space, the modelist gets filled, to find the no. of modes. In the
>> + * 2nd time, the user modes are filled, one by one from the modelist.
>> */
>> if ((out_resp->count_modes >= mode_count) && mode_count) {
>> copied = 0;
>> mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
>> - list_for_each_entry(mode, &connector->modes, head) {
>> - if (!drm_mode_expose_to_userspace(mode, file_priv))
>> - continue;
>> -
>> + list_for_each_entry(mode, &modelist, exp_head) {
>> drm_mode_convert_to_umode(&u_mode, mode);
>> + /*
>> + * Reset aspect ratio flags of user-mode, if modes with
>> + * aspect-ratio are not supported.
>> + */
>> + drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode);
>> if (copy_to_user(mode_ptr + copied,
>> &u_mode, sizeof(u_mode))) {
>> ret = -EFAULT;
>> @@ -1651,6 +1683,10 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>
>> out:
>> + list_for_each_entry_safe(mode, tmp, &modelist, exp_head)
>> + list_del(&mode->exp_head);
>> + list_del(&modelist);
> That last list_del() doesn't make sense. Also you can't touch the
> mode->exp_head without locking. We should document that it's
> protected by mode_config.mutex.
Oh yes, list_del () is not required, the deletion of the last mode, will
automatically
set the next and prev of the list_head to itself.
> Since we don't actually care about exp_head outside of this function
> I think we can just leave the stale pointers in place. No one else is
> supposed to look at them, a fact we should probaly also include in the
> documentation.
Ok, so I will let the stale pointers in place, and explain the reasoning
as comments.
>> +
>> drm_connector_put(connector);
>>
>> return ret;
>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>> index e0b060d..1888ec7 100644
>> --- a/include/drm/drm_modes.h
>> +++ b/include/drm/drm_modes.h
>> @@ -411,6 +411,15 @@ struct drm_display_mode {
>> * Field for setting the HDMI picture aspect ratio of a mode.
>> */
>> enum hdmi_picture_aspect picture_aspect_ratio;
>> +
>> + /**
>> + * @exp_head:
> I think we can afford to write 'export_head'.
Yeah right, shortening export_head to exp_head didn't do much :)
Will change it to export_head.
Regards,
Ankit
>> + *
>> + * struct list_head for modes to be exposed to the userspace.
>> + *
>> + */
>> + struct list_head exp_head;
>> +
>> };
>>
>> /**
>> --
>> 2.7.4
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180430/0681a808/attachment.html>
More information about the dri-devel
mailing list