On Mon, Apr 23, 2018 at 11:19:08AM +0530, Nautiyal, Ankit K wrote:
>
>
> On 4/20/2018 7:52 PM, Ville Syrjälä wrote:
> > On Fri, Apr 20, 2018 at 07:01:49PM +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, regardless 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.
> >> ---
> >> drivers/gpu/drm/drm_connector.c | 56 +++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 48 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >> index b3cde89..865ee354 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 drm_display_mode *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;
> >> +
> >> 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->head, 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,7 @@ 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, modelist;
> >> int mode_count = 0;
> >> int encoders_count = 0;
> >> int ret = 0;
> >> @@ -1605,23 +1625,37 @@ 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.head);
> > Why are we using a struct drm_display_mode to get a simple list_head?
>
> Yes you are right, we can use the simple list_head here. I was trying to
> use modelist as first mode, and goofed up.
> Its a mistake, I will correct it in next patchset.
>
> >
> >> +
> >> /* 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)) {
> >> + struct drm_display_mode *tmp_mode;
> >> +
> >> + tmp_mode = drm_mode_duplicate(dev, mode);
> > Duplicating every mode seems rather wasteful. I suppose we could
> > just add another list_head to struct drm_display_mode for this
> > purpose. An alternative could be to somehow mark each exposed mode
> > and just walk the original connector mode list.
>
> Agreed. Its indeed wasteful as we have to make the list in 2nd ioctl
> call again.
> As you have suggested, to mark the exposed modes, can we add a flag in
> drm_display_mode
> something like "to_be_exposed", which will be set in the first ioctl
> call while determining the space.
> 2nd time, we can skip the marking process, and iterate over the
> connector mode-list, making
> user-modes for only the modes that are marked?
No. You can't trust such a flag across two ioctl calls. It would have to
be protected by mode_config.mutex and as soon as you'd drop the lock
the flag would become invalid.
>
> >> + list_add_tail(&tmp_mode->head, &modelist.head);
> >> 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.head, 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 +1685,12 @@ 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.head, head) {
> >> + list_del(&mode->head);
> >> + drm_mode_destroy(dev, mode);
> >> + }
> >> + list_del(&modelist.head);
> >> +
> >> drm_connector_put(connector);
> >>
> >> return ret;
> >> --
> >> 2.7.4
>
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
