Re: [PATCH 09/10] drm/modes: Provide global mode_valid hook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 14, 2017 at 08:32:57PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> Allow drivers to provide a device wide .mode_valid() hook in addition to
> the already existing crtc/encoder/bridge/connector hooks. This can be
> used to validate device/driver wide constraings without having to add
> those to the other hooks. And since we call this hook also for user
> modes later on in the modeset we don't have to worry about anything the
> hook has already rejected.
> 
> I also have some further ideas for this hook. Eg. we could replace the
> drm_mode_set_crtcinfo(HALVE_V) call in drm_mode_convert_umode()/etc.
> with a driver specific variant via this hook. At least on i915 we would
> like to pass CRTC_STEREO_DOUBLE to that function instead, and then
> we could safely use the crtc_ timings in all our .mode_valid() hooks,
> which would allow us to reuse those hooks for validating the
> adjusted_mode during a modeset.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_atomic.c       |  2 +-
>  drivers/gpu/drm/drm_crtc.c         |  2 +-
>  drivers/gpu/drm/drm_modes.c        | 48 +++++++++++++++++++++++++++-----------
>  drivers/gpu/drm/drm_probe_helper.c |  2 +-
>  include/drm/drm_mode_config.h      | 12 ++++++++++
>  include/drm/drm_modes.h            |  6 +++--
>  6 files changed, 53 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..1df2a50c25d5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -387,7 +387,7 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  
>  	if (blob) {
>  		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> -		    drm_mode_convert_umode(&state->mode,
> +		    drm_mode_convert_umode(state->crtc->dev, &state->mode,
>  		                           (const struct drm_mode_modeinfo *)
>  		                            blob->data))
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f0556e654116..7eaa3c87761e 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -610,7 +610,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> -		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
> +		ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
>  		if (ret) {
>  			DRM_DEBUG_KMS("Invalid mode\n");
>  			goto out;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 68a8ba4c2c38..4eb12ff4df17 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1023,17 +1023,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  }
>  EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo);
>  
> -/**
> - * drm_mode_validate_basic - make sure the mode is somewhat sane
> - * @mode: mode to check
> - *
> - * Check that the mode timings are at least somewhat reasonable.
> - * Any hardware specific limits are left up for each driver to check.
> - *
> - * Returns:
> - * The mode status
> - */
> -enum drm_mode_status
> +static enum drm_mode_status
>  drm_mode_validate_basic(const struct drm_display_mode *mode)
>  {
>  	if (mode->type & ~DRM_MODE_TYPE_ALL ||
> @@ -1060,7 +1050,35 @@ drm_mode_validate_basic(const struct drm_display_mode *mode)
>  
>  	return MODE_OK;
>  }
> -EXPORT_SYMBOL(drm_mode_validate_basic);
> +
> +/**
> + * drm_mode_validate_driver - make sure the mode is somewhat sane
> + * @dev: drm device
> + * @mode: mode to check
> + *
> + * First do basic validation on the mode, and then allow the driver
> + * to check for device/driver specific limitations via the optional
> + * &drm_mode_config_helper_funcs.mode_valid hook.
> + *
> + * Returns:
> + * The mode status
> + */
> +enum drm_mode_status
> +drm_mode_validate_driver(struct drm_device *dev,
> +			const struct drm_display_mode *mode)
> +{
> +	enum drm_mode_status status;
> +
> +	status = drm_mode_validate_basic(mode);
> +	if (status != MODE_OK)
> +		return status;
> +
> +	if (dev->mode_config.funcs->mode_valid)
> +		return dev->mode_config.funcs->mode_valid(dev, mode);
> +	else
> +		return MODE_OK;
> +}
> +EXPORT_SYMBOL(drm_mode_validate_driver);
>  
>  /**
>   * drm_mode_validate_size - make sure modes adhere to size constraints
> @@ -1562,6 +1580,7 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  
>  /**
>   * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode
> + * @dev: drm device
>   * @out: drm_display_mode to return to the user
>   * @in: drm_mode_modeinfo to use
>   *
> @@ -1571,7 +1590,8 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>   * Returns:
>   * Zero on success, negative errno on failure.
>   */
> -int drm_mode_convert_umode(struct drm_display_mode *out,
> +int drm_mode_convert_umode(struct drm_device *dev,
> +			   struct drm_display_mode *out,
>  			   const struct drm_mode_modeinfo *in)
>  {
>  	int ret = -EINVAL;
> @@ -1598,7 +1618,7 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
>  	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
>  	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
>  
> -	out->status = drm_mode_validate_basic(out);
> +	out->status = drm_mode_validate_driver(dev, out);
>  	if (out->status != MODE_OK)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 6dc2dde5b672..51303060898e 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -500,7 +500,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  
>  	list_for_each_entry(mode, &connector->modes, head) {
>  		if (mode->status == MODE_OK)
> -			mode->status = drm_mode_validate_basic(mode);
> +			mode->status = drm_mode_validate_driver(dev, mode);
>  
>  		if (mode->status == MODE_OK)
>  			mode->status = drm_mode_validate_size(mode, maxX, maxY);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 0b4ac2ebc610..e134a0682395 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -35,6 +35,7 @@ struct drm_device;
>  struct drm_atomic_state;
>  struct drm_mode_fb_cmd2;
>  struct drm_format_info;
> +struct drm_display_mode;
>  
>  /**
>   * struct drm_mode_config_funcs - basic driver provided mode setting functions
> @@ -101,6 +102,17 @@ struct drm_mode_config_funcs {
>  	void (*output_poll_changed)(struct drm_device *dev);
>  
>  	/**
> +	 * @mode_valid:
> +	 *
> +	 * Device specific validation of display modes. Can be used to reject
> +	 * modes that can never be supports. Only device wide constraints can

s/supports/supported/

> +	 * be checked here. crtc/encoder/bridge/connector specific constraints
> +	 * can be checked in the .mode_valid() hook for each specific object.

s/can/should/ I think, for my understanding of English at least.

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

Some igts that throw bs modes at the kernel would be nice, to complement
your work here (I didn't check for those patches, mail backlog and all).
-Daniel


> +	 */
> +	enum drm_mode_status (*mode_valid)(struct drm_device *dev,
> +					   const struct drm_display_mode *mode);
> +
> +	/**
>  	 * @atomic_check:
>  	 *
>  	 * This is the only hook to validate an atomic modeset update. This
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index 7ac61858b54e..f945afaf2313 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -444,7 +444,8 @@ struct drm_display_mode *drm_mode_create(struct drm_device *dev);
>  void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode);
>  void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
>  			       const struct drm_display_mode *in);
> -int drm_mode_convert_umode(struct drm_display_mode *out,
> +int drm_mode_convert_umode(struct drm_device *dev,
> +			   struct drm_display_mode *out,
>  			   const struct drm_mode_modeinfo *in);
>  void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
>  void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
> @@ -497,7 +498,8 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
>  					const struct drm_display_mode *mode2);
>  
>  /* for use by the crtc helper probe functions */
> -enum drm_mode_status drm_mode_validate_basic(const struct drm_display_mode *mode);
> +enum drm_mode_status drm_mode_validate_driver(struct drm_device *dev,
> +					      const struct drm_display_mode *mode);
>  enum drm_mode_status drm_mode_validate_size(const struct drm_display_mode *mode,
>  					    int maxX, int maxY);
>  enum drm_mode_status
> -- 
> 2.13.6
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux