On 5/7/2018 5:58 PM, Ville Syrjälä wrote:
On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:On 5/5/2018 1:49 AM, Ville Syrjälä wrote:On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:On Wed, May 02, 2018 at 12:20:20PM +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. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes. 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. V13: Minor modifications, as suggested by Ville. --- drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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 *export_list, + const struct drm_file *file_priv) { /* * If user-space hasn't configured the driver to expose the stereo 3D * modes, don't expose them. */ + 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) { + struct drm_display_mode *mode_itr; + + list_for_each_entry(mode_itr, export_list, export_head)By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided.I don't think that would work. If we just walk over all the modes then wouldn't we always find a duplicate when the same mode with two different aspect ratios is on the list? And then we'd export neither? Or maybe I misunderstood what you mean here. To me it seems like the correct option is to check against the export_list unconditionally, no matter whether the current mode has the aspect ratio set or not. If an identical mode is already on the list we don't export again, if it's not there then we export.Agreed. The current code does have a problem rightly pointed by Daniel Vetter and the main reason for that is - we are checking for duplicates only if the mode has some aspect-ratio. As you have suggested, If we can do away with the condition to check for duplicates for only "modes having aspect ratio", it will solve the problem. In that case, if the aspect-ratio cap is not set, we will be removing the duplicates for all modes, irrespective of having aspect-ratio information or not, but if aspect-ratio is allowed, we would do no such checking. Or do you suggest that we remove duplicates in any case, whether aspect-ratio cap is set by the user or not?If there are duplicates on the connector mode list we've done something wrong when populating the list. So no, we should not have to check for duplicates in that case.
Alright. So I'll just drop the condition to check the mode's aspect-ratio and so the export_list will be checked unconditionally, if aspect-ratio cap is not set. However if the aspect-ratio cap is set, we just move the mode to export-list without checking for its duplicates.
I will send the next-patch set with this change, along with the removal of helper functions as discussed earlier.
With that changed: Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>+ 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;} @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr; + LIST_HEAD(export_list);if (!drm_core_check_feature(dev, DRIVER_MODESET))return -EINVAL; @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,/* 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, &export_list, + file_priv)) { + list_add_tail(&mode->export_head, &export_list); 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 + * 'export_list'. When the ioctl is called first time to determine the, + * space, the export_list gets filled, to find the no.of modes. In the + * 2nd time, the user modes are filled, one by one from the export_list. */ 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, &export_list, export_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; diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio; + + /** + * @export_head: + * + * struct list_head for modes to be exposed to the userspace. + * This is to maintain a list of exposed modes while preparing + * user-mode's list in drm_mode_getconnector ioctl. The purpose of this + * list_head only lies in the ioctl function, and is not expected to be + * used outside the function. + * Once used, the stale pointers are not reset, but left as it is, to + * avoid overhead of protecting it by mode_config.mutex. + */ + struct list_head export_head; };/**-- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
