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 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 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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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