[PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Jan 31 13:09:11 UTC 2018
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 at 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 at 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 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.
>
> 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.
>
> >
> >> 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 at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list