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@xxxxxxxxx>
> >>>>>>>
> >>>>>>> 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@xxxxxxxxx>
> >>>>>>>
> >>>>>>> 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@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>> Regards,
> >>> Ankit
> >
>
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
