On Tue, Feb 13, 2018 at 09:53:53PM +0530, Nautiyal, Ankit K wrote: > Hi Ville, > > Thanks yet again to look into this. > > I am still skeptical about rejecting the mode, if aspect ratio cap is > not set. > Perhaps I am not aware with the userspace expectations. > > Please find my response inline: > > On 2/13/2018 6:48 PM, Ville Syrjälä wrote: > > 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. > I guess, rejecting the mode, altogether can break the existing user-spaces. > Current user-spaces, oblivious of the aspect-ratio capability in drm, > will not set the aspect-ratio capability. > Some of them, might have some garbage value in the aspect ratio bits of > the user mode. If we reject such > modes, the user-spaces that were earlier working will break. > (But if we are sure, that the aspect ratio flag bits will all be reset, > then it makes sense to reject the mode.) We already reject all unspecified flags. > > Instead of rejecting such modes, if we just only ignore the aspect ratio > bits in such cases, and carry on with the > modeset for the given user mode, the behaviour would be intact, just as > prior to the aspect ratio changes. > > > drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc, > > need to filter out the flags. > > Yes right. I'll be filtering out the flags in the getblob and getcrtc > functions. > > Thanks & Regards, > Ankit > >> 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