Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
> 
> As per our last discussion, following points were discussed:
> 
> 1. To suppress the aspect-ratio info from getblob ioctl to a user that 
> does not support it:
> 
>      i. A new flag must be added to drm_blob_property to mark if the 
> blob has mode data.
> 
>      ii. This flag must be set when the user tries do an atomic modeset.
> 
>      iii. In the getblob ioctl, based on the above flag, it can be 
> determined that if the blob
> 
>      has mode data, and the aspect ratio info can be suppressed in 
> getblob ioctl, if user does not support it.
> 
> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass 
> on the file_priv which already has
> 
> the required information to the function drm_mode_convert_umode.
> 
> It will require addition of an new argument file_priv to several 
> functions, but that is right thing to do,
> 
> as file_priv is the correct structure for the capability information.
> 
> 3. Changing the usermode aspect ratio flag bits, without change in the
> picture_aspect_ratio would not matter, and does not need to be handled 
> in this patch.
> 
> 
> I just have one query here. We have agreed to modify 
> drm_mode_convert_umode, to filter out the aspect-ratio
> 
> info if user has not set aspect-ratio capability, but do we need a 
> similar change the drm_mode_convert_to_umode?

I think you maybe had those backwards.

drm_mode_convert_umode(), or more likely its relevant callers setcrtc
and setproperty, need to reject the mode if the client cap is not set.

drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc,
need to filter out the flags.

> 
> If we do, I am not sure if it would be possible to have the file_priv to 
> /drm_atomic_set_mode_for_crtc/, which calls
> 
> drm_mode_convert_to_umode.  The function /drm_atomic_set_mode_for_crtc/ 
> is used to set mode, originating by kernel,
> 
> and make a blob from the kernel mode, which it saves in crtc_state.
> 
> This function /: drm_atomic_set_mode_for_crtc, /is called by several 
> helper functions and driver functions, and passing
> 
> file_priv from all these functions does not seem to be plausible.

I don't think we need to plumb it quite that deep. Doing the
check in drm_atomic_crtc_set_property() should be sufficient.

> 
> Any suggestions on how to handle this situation?
> 
> 
> Regards,
> 
> Ankit
> 
> 
> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote:
> > Hi Ville,
> >
> > I still have some queries regarding the handling of aspect ratio flags 
> > in getblob ioctl.
> >
> > Please find below my responses inline.
> >
> >
> > On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
> >> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
> >>> Hi Ville,
> >>>
> >>> Appreciate your time and the suggestions.
> >>> Please find my response inline:
> >>>
> >>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
> >>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
> >>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
> >>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> >>>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>>>>>
> >>>>>>> If the user mode does not support aspect-ratio, and requests for
> >>>>>>> a modeset, then the flag bits representing aspect ratio in the
> >>>>>>> given user-mode must be rejected.
> >>>>>>> Similarly, while preparing a user-mode from kernel mode, the
> >>>>>>> aspect-ratio info must not be given, if aspect-ratio is not
> >>>>>>> supported by the user.
> >>>>>>>
> >>>>>>> This patch:
> >>>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state,
> >>>>>>> which is set only if the aspect-ratio is supported by the user.
> >>>>>>> 2. discards the aspect-ratio info from the user mode while
> >>>>>>> preparing kernel mode structure, during modeset, if the
> >>>>>>> user does not support aspect ratio.
> >>>>>>> 3. avoids setting the aspect-ratio info in user-mode, while
> >>>>>>> converting user-mode from the kernel-mode.
> >>>>>>>
> >>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>>>>>
> >>>>>>> V3: Addressed review comments from Ville:
> >>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
> >>>>>>> on the fly.
> >>>>>>> ---
> >>>>>>>     drivers/gpu/drm/drm_atomic.c | 61 
> >>>>>>> +++++++++++++++++++++++++++++++++++++++++---
> >>>>>>>     drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
> >>>>>>>     include/drm/drm_atomic.h     |  2 ++
> >>>>>>>     3 files changed, 79 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c 
> >>>>>>> b/drivers/gpu/drm/drm_atomic.c
> >>>>>>> index 69ff763..39313e2 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>>>>> @@ -316,6 +316,35 @@ static s32 __user 
> >>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state,
> >>>>>>>             return fence_ptr;
> >>>>>>>     }
> >>>>>>> +/**
> >>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
> >>>>>>> + * @state: the crtc state
> >>>>>>> + * @mode: kernel-internal mode, which is used to create a 
> >>>>>>> duplicate mode,
> >>>>>>> + * with updated picture aspect ratio.
> >>>>>>> + *
> >>>>>>> + * Allow the aspect ratio info in the kernel mode only if 
> >>>>>>> aspect ratio is
> >>>>>>> + * supported.
> >>>>>>> + *
> >>>>>>> + * RETURNS:
> >>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +struct drm_display_mode*
> >>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state 
> >>>>>>> *state,
> >>>>>>> +                    const struct drm_display_mode *mode)
> >>>>>>> +{
> >>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
> >>>>>>> +    struct drm_display_mode *ar_kmode;
> >>>>>>> +
> >>>>>>> +    ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
> >>>>>>> +    /*
> >>>>>>> +     * If aspect ratio is not supported, set the picture aspect 
> >>>>>>> ratio as
> >>>>>>> +     * NONE.
> >>>>>>> +     */
> >>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>>>>> +        ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>>>> +    return ar_kmode;
> >>>>>>> +}
> >>>>>>>         /**
> >>>>>>>      * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct 
> >>>>>>> drm_crtc_state *state,
> >>>>>>>         state->mode_blob = NULL;
> >>>>>>>             if (mode) {
> >>>>>>> -        drm_mode_convert_to_umode(&umode, mode);
> >>>>>>> +        struct drm_display_mode *ar_mode;
> >>>>>>> +
> >>>>>>> +        ar_mode = 
> >>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
> >>>>>>> +        drm_mode_convert_to_umode(&umode, ar_mode);
> >>>>>> This still looks sketchy.
> >>>>>>
> >>>>>> I believe there are just two places where we need to filter out the
> >>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
> >>>>>> ioctls.
> >>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
> >>>>> etc. apart from the mode blob.
> >>>>> Is there any way to check from blob id (or by any other means),
> >>>>> if the data is for the mode, or edid, or gamma etc.
> >>>> I think we'd have to somehow mark the mode blobs as special. Until we
> >>>> have more need for cleaning up blobs before returning them to 
> >>>> userspace
> >>>> I think a simple flag should do. If we come up with more uses like 
> >>>> this
> >>>> then it might make sense to introduce some kind of optional filter
> >>>> function for blobs.
> >>> If my understanding is correct,
> >>> 1. To be able to do this, we need to change in uapi.
> >> We should be fine with an internal flag. Obviously it won't be set
> >> for blobs freshly created by userspace, but that's fine because we
> >> do no other error checking for them either. The error checking will
> >> happen when the user tries to actually use the blob.
> >
> > I am not sure why getblob ioctl should even come into picture. 
> > (Perhaps I am missing something).
> > As per my understanding:
> > If a userspace needs to set a mode, it will just create a blob with 
> > drm_mode_mode_info,
> > and get the blob-id. This blob-id would be supplied to 
> > drm_mode_atomic_ioctl as a crtc
> > property mode_id.
> > At the kernel side,  the crtc property mode_id is set by looking-up 
> > for mode blob from blob id.
> > I am doing the change in the user mode aspect-ratio flags at this 
> > junction.
> >
> > As far as getblob ioctl is concerned, if the user does call getblob 
> > ioctl for getting mode blob from
> > the blob id, it will receive the same mode blob, with same aspect 
> > ratio info which the user had
> > created using create blob ioctl. If it does want to use the blob, it 
> > has to come through the
> > drm_mode_atomic_ioctl way, where aspect ratio flags are handled.
> >
> >>> 2. Whenever the userspace creates a blob for mode, the new flag/class
> >>> for blob type needs to be set.
> >>>
> >>> Do you think that it's worth the effort?
> >>>>> If we can check that, I can make sure that aspect-ratio flags are 
> >>>>> taken
> >>>>> care of, if the blob is for mode,
> >>>>> and the aspect ratio is not supported.
> >>>>>
> >>>>>> And for checking the user input I think we should probably just
> >>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
> >>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
> >>>>>> a non-NULL argument.
> >>>>>>
> >>>>>> I *think* those three places should be sufficient to cover 
> >>>>>> everything.
> >>>>> Agreed. Infact I tried to do that first, but the only problem we have
> >>>>> here, is that, the file_priv
> >>>>> which has the aspect ratio capability information is not supplied 
> >>>>> to the
> >>>>> drm_mode_convert_umode()
> >>>>> or the functions calling, drm_mode_conver_umode(). At best we have
> >>>>> drm_crtc_state.
> >>>> Simply passing the file_priv down would seem like the most obvious
> >>>> solution.
> >>> This means we have to make change in various drivers and we have to 
> >>> pull
> >>> the file_priv
> >>> parameter three levels down. Should we think of a better way for 
> >>> doing this?
> >> Coccinelle is pretty good at modifying function parameters.
> >
> > Thanks for pointing out Coccinelle, will defintely try that out.
> > But my question here is whether we really want to drag the file_priv
> > from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
> > drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
> > drm_mode_convert_umode.
> >
> > Instead, I tried to add a new field (allow_aspect_ratio) in 
> > drm_crtc_state which is
> > already passed from the drm_mode_atomic_ioctl all the way to
> > drm_mode_convert_umode.
> >
> >
> > -Regards
> > Ankit
> >>
> >>>>> I have added an aspect ratio allowed flag  in 
> >>>>> drm_crtc_state->state so
> >>>>> that its available in the
> >>>>> functions calling drm_mode_convert_umode.
> >>>>>
> >>>>> I am open to suggestion to avoid adding a new flag in 
> >>>>> drm_atomic_state,
> >>>>> if there is a better
> >>>>> way to let the drm_mode_convert_umode( ) know that user supports 
> >>>>> aspect
> >>>>> ratio capability.
> >>>>>>>             state->mode_blob =
> >>>>>>> drm_property_create_blob(state->crtc->dev,
> >>>>>>> sizeof(umode),
> >>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct 
> >>>>>>> drm_crtc_state *state,
> >>>>>>>             if (IS_ERR(state->mode_blob))
> >>>>>>>                 return PTR_ERR(state->mode_blob);
> >>>>>>>     -        drm_mode_copy(&state->mode, mode);
> >>>>>>> +        drm_mode_copy(&state->mode, ar_mode);
> >>>>>>>             state->enable = true;
> >>>>>>>             DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> >>>>>>> -                 mode->name, state);
> >>>>>>> +                 ar_mode->name, state);
> >>>>>>> +        drm_mode_destroy(state->crtc->dev, ar_mode);
> >>>>>>>         } else {
> >>>>>>>             memset(&state->mode, 0, sizeof(state->mode));
> >>>>>>>             state->enable = false;
> >>>>>>> @@ -436,6 +469,25 @@ 
> >>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
> >>>>>>>     }
> >>>>>>>         /**
> >>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
> >>>>>>> + * @state: the crtc state
> >>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to 
> >>>>>>> be updated.
> >>>>>>> + *
> >>>>>>> + * Allow the aspect ratio info in the userspace mode only if 
> >>>>>>> aspect ratio is
> >>>>>>> + * supported.
> >>>>>>> + */
> >>>>>>> +void
> >>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state 
> >>>>>>> *state,
> >>>>>>> +                    struct drm_mode_modeinfo *umode)
> >>>>>>> +{
> >>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
> >>>>>>> +
> >>>>>>> +    /* Reset the aspect ratio flags if aspect ratio is not 
> >>>>>>> supported */
> >>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>>>>> +        umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/**
> >>>>>>>      * drm_atomic_crtc_set_property - set property on CRTC
> >>>>>>>      * @crtc: the drm CRTC to set a property on
> >>>>>>>      * @state: the state object to update with the new property 
> >>>>>>> value
> >>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct 
> >>>>>>> drm_crtc *crtc,
> >>>>>>>         else if (property == config->prop_mode_id) {
> >>>>>>>             struct drm_property_blob *mode =
> >>>>>>>                 drm_property_lookup_blob(dev, val);
> >>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state,
> >>>>>>> +                    (struct drm_mode_modeinfo *)
> >>>>>>> +                    mode->data);
> >>>>>>>             ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >>>>>>>             drm_property_blob_put(mode);
> >>>>>>>             return ret;
> >>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c 
> >>>>>>> b/drivers/gpu/drm/drm_crtc.c
> >>>>>>> index f0556e6..a2d34fa 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
> >>>>>>>         drm_modeset_lock(&crtc->mutex, NULL);
> >>>>>>>         if (crtc->state) {
> >>>>>>>             if (crtc->state->enable) {
> >>>>>>> +            /*
> >>>>>>> +             * If the aspect-ratio is not required by the,
> >>>>>>> +             * userspace, do not set the aspect-ratio flags.
> >>>>>>> +             */
> >>>>>>> +            if (!file_priv->aspect_ratio_allowed)
> >>>>>>> + crtc->state->mode.picture_aspect_ratio =
> >>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
> >>>>>> These would still clobber the current crtc state. So definitely 
> >>>>>> don't
> >>>>>> want to do this.
> >>>>> Alright, so alternative way of doing this will be:
> >>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
> >>>>> mode structure,
> >>>>> but set the aspect ratio flags in the usermode to none, after 
> >>>>> calling the
> >>>>> drm_mode_convert_to_umode().
> >>>>>
> >>>>> Will take care of this in the next patchset.
> >>> I gave a thought about it again. As you have mentioned earlier that
> >>> get_crtc is one of the
> >>> place where the user mode aspect ratio flags must be filtered out, is
> >>> this what you have
> >>> in mind. I hope I did not misunderstand.
> >>>
> >>> Changing the usermode aspect ratio flag bits, without change in the
> >>> picture_aspect_ratio
> >>> kernel mode will not result in both structures out of sync?
> >> I don't think that should really matter. I suppose we might get one
> >> extra modeset when the user feeds that mode back via setcrtc/atomic, but
> >> meh. And actually the aspect ratio should only affect the infoframe, so
> >> we should be able to eventually eliminate that extra modeset and just
> >> update the infoframe instead.
> >>
> >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> >>>>>>>                 crtc_resp->mode_valid = 1;
> >>>>>>>     @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device 
> >>>>>>> *dev,
> >>>>>>>             crtc_resp->x = crtc->x;
> >>>>>>>             crtc_resp->y = crtc->y;
> >>>>>>>             if (crtc->enabled) {
> >>>>>>> +            /*
> >>>>>>> +             * If the aspect-ratio is not required by the,
> >>>>>>> +             * userspace, do not set the aspect-ratio flags.
> >>>>>>> +             */
> >>>>>>> +            if (!file_priv->aspect_ratio_allowed)
> >>>>>>> +                crtc->mode.picture_aspect_ratio =
> >>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
> >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
> >>>>>>>                 crtc_resp->mode_valid = 1;
> >>>>>>>     @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device 
> >>>>>>> *dev, void *data,
> >>>>>>>                 goto out;
> >>>>>>>             }
> >>>>>>>     +        /* If the user does not ask for aspect ratio, reset 
> >>>>>>> the aspect
> >>>>>>> +         * ratio bits in the usermode.
> >>>>>>> +         */
> >>>>>>> +        if (!file_priv->aspect_ratio_allowed)
> >>>>>>> +            crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>>>>>             ret = drm_mode_convert_umode(mode, &crtc_req->mode);
> >>>>>>>             if (ret) {
> >>>>>>>                 DRM_DEBUG_KMS("Invalid mode\n");
> >>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >>>>>>> index 1c27526..130dad9 100644
> >>>>>>> --- a/include/drm/drm_atomic.h
> >>>>>>> +++ b/include/drm/drm_atomic.h
> >>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
> >>>>>>>      * @ref: count of all references to this state (will not be 
> >>>>>>> freed until zero)
> >>>>>>>      * @dev: parent DRM device
> >>>>>>>      * @allow_modeset: allow full modeset
> >>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be 
> >>>>>>> passes to user
> >>>>>>>      * @legacy_cursor_update: hint to enforce legacy cursor 
> >>>>>>> IOCTL semantics
> >>>>>>>      * @async_update: hint for asynchronous plane update
> >>>>>>>      * @planes: pointer to array of structures with per-plane data
> >>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
> >>>>>>>             struct drm_device *dev;
> >>>>>>>         bool allow_modeset : 1;
> >>>>>>> +    bool allow_aspect_ratio : 1;
> >>>>>>>         bool legacy_cursor_update : 1;
> >>>>>>>         bool async_update : 1;
> >>>>>>>         struct __drm_planes_state *planes;
> >>>>>>> -- 
> >>>>>>> 2.7.4
> >>>>>>>
> >>>>>>> _______________________________________________
> >>>>>>> dri-devel mailing list
> >>>>>>> dri-devel@lists.freedesktop.org
> >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>> Regards,
> >>> Ankit
> >
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel