| Message ID | 1524831296-11198-9-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 27, 2018 at 05:44:54PM +0530, Nautiyal, Ankit K wrote: > From: Ankit Nautiyal <ankit.k.nautiyal@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@linux.intel.com> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Cc: Jose Abreu <jose.abreu@synopsys.com> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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. > 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
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@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@linux.intel.com> >> Cc: Shashank Sharma <shashank.sharma@intel.com> >> Cc: Jose Abreu <jose.abreu@synopsys.com> >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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
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; + 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; 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); + 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: + * + * struct list_head for modes to be exposed to the userspace. + * + */ + struct list_head exp_head; + }; /**