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 >> >> 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 >> Cc: Shashank Sharma >> Cc: Jose Abreu >> >> Signed-off-by: Ankit Nautiyal >> >> 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