On Fri, Apr 27, 2018 at 05:44:54PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Cc: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> Cc: Jose Abreu <jose.abreu@xxxxxxxxxxxx>
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
>
> 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.
> 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.
> 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.
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.
> +
> 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'.
> + *
> + * struct list_head for modes to be exposed to the userspace.
> + *
> + */
> + struct list_head exp_head;
> +
> };
>
> /**
> --
> 2.7.4
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
