[PATCH v5 7/9] drm: Expose modes with aspect ratio, only if requested
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon Feb 26 15:30:31 UTC 2018
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 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, 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 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).
>> ---
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180226/cfae331f/attachment-0001.html>
More information about the dri-devel
mailing list