| Message ID | 1518697262-3001-8-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 15, 2018 at 05:51:00PM +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. > > 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). > --- > drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 16b9c38..49778e8 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1507,8 +1507,10 @@ 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 *last_mode, > + const struct drm_display_mode *mode, I would put the 'mode' argument first since that's the "main" object we're operating on. > + const struct drm_file *file_priv) > { > /* > * If user-space hasn't configured the driver to expose the stereo 3D > @@ -1516,6 +1518,23 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, > */ > 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. But in case of a unique > + * mode, let the mode be passed, so that it can be enumerated with > + * aspect ratio bits erased. Might want to note that we expect the list to be sorted such that modes that have different aspect ratio but are otherwise identical are back to back. > + */ > + if (!file_priv->aspect_ratio_allowed && > + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) { > + if (last_mode && !drm_mode_match(mode, last_mode, > + DRM_MODE_MATCH_TIMINGS | > + DRM_MODE_MATCH_CLOCK | > + DRM_MODE_MATCH_FLAGS | > + DRM_MODE_MATCH_3D_FLAGS)) > + return true; > + else > + return false; How does that even work? It'll return false as long as last_mode==NULL, and last_mode never becomes non-NULL unless we return true. To me it looks like the correct logic would be: if (last_mode && drm_mode_match(...)) return false; > + } > > return true; > } > @@ -1527,6 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > struct drm_connector *connector; > struct drm_encoder *encoder; > struct drm_display_mode *mode; > + struct drm_display_mode *last_valid_mode; > int mode_count = 0; > int encoders_count = 0; > int ret = 0; > @@ -1582,9 +1602,13 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->connection = connector->status; > > /* delayed so we get modes regardless of pre-fill_modes state */ > + last_valid_mode = NULL; > list_for_each_entry(mode, &connector->modes, head) > - if (drm_mode_expose_to_userspace(mode, file_priv)) > + if (drm_mode_expose_to_userspace(last_valid_mode, mode, > + file_priv)) { > mode_count++; > + last_valid_mode = mode; > + } > > /* > * This ioctl is called twice, once to determine how much space is > @@ -1593,11 +1617,21 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if ((out_resp->count_modes >= mode_count) && mode_count) { > copied = 0; > mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; > + last_valid_mode = NULL; > list_for_each_entry(mode, &connector->modes, head) { > - if (!drm_mode_expose_to_userspace(mode, file_priv)) > + if (!drm_mode_expose_to_userspace(last_valid_mode, > + mode, > + file_priv)) > continue; > - > drm_mode_convert_to_umode(&u_mode, mode); > + > + /* > + * Reset the aspect-ratio flag bits from the user mode, > + * if the user mode does have aspect ratio capability. > + */ > + if (!file_priv->aspect_ratio_allowed) > + u_mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); Useless parens. Could use the same helper I suggested we add for getcrtc() and getblob(). > + > if (copy_to_user(mode_ptr + copied, > &u_mode, sizeof(u_mode))) { > ret = -EFAULT; > @@ -1605,6 +1639,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > > goto out; > } > + last_valid_mode = mode; > copied++; > } > } > -- > 2.7.4
On 2/23/2018 8:06 PM, Ville Syrjälä wrote: > On Thu, Feb 15, 2018 at 05:51:00PM +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. >> >> 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). >> --- >> drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 40 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >> index 16b9c38..49778e8 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -1507,8 +1507,10 @@ 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 *last_mode, >> + const struct drm_display_mode *mode, > I would put the 'mode' argument first since that's the "main" object > we're operating on. Yes that makes more sense. Will make the mode as first argument in the next revision. > >> + const struct drm_file *file_priv) >> { >> /* >> * If user-space hasn't configured the driver to expose the stereo 3D >> @@ -1516,6 +1518,23 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, >> */ >> 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. But in case of a unique >> + * mode, let the mode be passed, so that it can be enumerated with >> + * aspect ratio bits erased. > Might want to note that we expect the list to be sorted such that modes > that have different aspect ratio but are otherwise identical are back to > back. Indeed. The whole idea, is based on the fact that the list of modes are sorted. Will put a note here. >> + */ >> + if (!file_priv->aspect_ratio_allowed && >> + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) { >> + if (last_mode && !drm_mode_match(mode, last_mode, >> + DRM_MODE_MATCH_TIMINGS | >> + DRM_MODE_MATCH_CLOCK | >> + DRM_MODE_MATCH_FLAGS | >> + DRM_MODE_MATCH_3D_FLAGS)) >> + return true; >> + else >> + return false; > How does that even work? It'll return false as long as > last_mode==NULL, and last_mode never becomes non-NULL unless > we return true. > > To me it looks like the correct logic would be: > if (last_mode && drm_mode_match(...)) > return false; This is a mistake, thanks for pointing it out. While testing with aspect-ratio cap not set, for all the available displays, the first mode in the list, never had a mode with aspect ratio. (or in other words, first mode was always with aspect ratio NONE) This resulted in early exit from the condition and returning true, making last_mode as the first mode. Long story short, couldn't catch the problem while testing as first mode was always with aspect ratio NONE :) Your suggested code is indeed the correct logic, which I intended. >> + } >> >> return true; >> } >> @@ -1527,6 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> struct drm_connector *connector; >> struct drm_encoder *encoder; >> struct drm_display_mode *mode; >> + struct drm_display_mode *last_valid_mode; >> int mode_count = 0; >> int encoders_count = 0; >> int ret = 0; >> @@ -1582,9 +1602,13 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> out_resp->connection = connector->status; >> >> /* delayed so we get modes regardless of pre-fill_modes state */ >> + last_valid_mode = NULL; >> list_for_each_entry(mode, &connector->modes, head) >> - if (drm_mode_expose_to_userspace(mode, file_priv)) >> + if (drm_mode_expose_to_userspace(last_valid_mode, mode, >> + file_priv)) { >> mode_count++; >> + last_valid_mode = mode; >> + } >> >> /* >> * This ioctl is called twice, once to determine how much space is >> @@ -1593,11 +1617,21 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> if ((out_resp->count_modes >= mode_count) && mode_count) { >> copied = 0; >> mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; >> + last_valid_mode = NULL; >> list_for_each_entry(mode, &connector->modes, head) { >> - if (!drm_mode_expose_to_userspace(mode, file_priv)) >> + if (!drm_mode_expose_to_userspace(last_valid_mode, >> + mode, >> + file_priv)) >> continue; >> - >> drm_mode_convert_to_umode(&u_mode, mode); >> + >> + /* >> + * Reset the aspect-ratio flag bits from the user mode, >> + * if the user mode does have aspect ratio capability. >> + */ >> + if (!file_priv->aspect_ratio_allowed) >> + u_mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > Useless parens. Could use the same helper I suggested we add for > getcrtc() and getblob(). Yes, the agreed helper function will be used here. Regards, Ankit > >> + >> if (copy_to_user(mode_ptr + copied, >> &u_mode, sizeof(u_mode))) { >> ret = -EFAULT; >> @@ -1605,6 +1639,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> >> goto out; >> } >> + last_valid_mode = mode; >> copied++; >> } >> } >> -- >> 2.7.4
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 16b9c38..49778e8 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1507,8 +1507,10 @@ 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 *last_mode, + const struct drm_display_mode *mode, + const struct drm_file *file_priv) { /* * If user-space hasn't configured the driver to expose the stereo 3D @@ -1516,6 +1518,23 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, */ 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. But in case of a unique + * mode, let the mode be passed, so that it can be enumerated with + * aspect ratio bits erased. + */ + if (!file_priv->aspect_ratio_allowed && + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) { + if (last_mode && !drm_mode_match(mode, last_mode, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_CLOCK | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS)) + return true; + else + return false; + } return true; } @@ -1527,6 +1546,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_connector *connector; struct drm_encoder *encoder; struct drm_display_mode *mode; + struct drm_display_mode *last_valid_mode; int mode_count = 0; int encoders_count = 0; int ret = 0; @@ -1582,9 +1602,13 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, out_resp->connection = connector->status; /* delayed so we get modes regardless of pre-fill_modes state */ + last_valid_mode = NULL; list_for_each_entry(mode, &connector->modes, head) - if (drm_mode_expose_to_userspace(mode, file_priv)) + if (drm_mode_expose_to_userspace(last_valid_mode, mode, + file_priv)) { mode_count++; + last_valid_mode = mode; + } /* * This ioctl is called twice, once to determine how much space is @@ -1593,11 +1617,21 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; + last_valid_mode = NULL; list_for_each_entry(mode, &connector->modes, head) { - if (!drm_mode_expose_to_userspace(mode, file_priv)) + if (!drm_mode_expose_to_userspace(last_valid_mode, + mode, + file_priv)) continue; - drm_mode_convert_to_umode(&u_mode, mode); + + /* + * Reset the aspect-ratio flag bits from the user mode, + * if the user mode does have aspect ratio capability. + */ + if (!file_priv->aspect_ratio_allowed) + u_mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); + if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT; @@ -1605,6 +1639,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, goto out; } + last_valid_mode = mode; copied++; } }