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
