[PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon May 7 12:52:49 UTC 2018
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 at 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, 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 at linux.intel.com>
>>>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>>>> Cc: Jose Abreu <jose.abreu at synopsys.com>
>>>>>
>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at 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.
>>>>> 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 at ffwll.ch>
>>>>
>>>>> + 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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>> --
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> http://blog.ffwll.ch
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list