[Intel-gfx] [PATCH v4 2/9] drm/i915: Add plumbing for digital connector state, v2.

Daniel Vetter daniel at ffwll.ch
Wed Apr 19 15:48:26 UTC 2017


On Wed, Apr 12, 2017 at 12:50:00PM +0200, Maarten Lankhorst wrote:
> Some atomic properties are common between the various kinds of
> connectors, for example a lot of them use panel fitting mode.
> It makes sense to put a lot of it in a common place, so each
> connector can use it while they're being converted.
> 
> Implement the properties required for the connectors:
> - scaling mode property
> - force audio property
> - broadcast rgb
> - aspect ratio
> 
> While at it, make clear that intel_digital_connector_atomic_get_property
> is a hack that has to be removed when all connector properties
> are converted to atomic.
> 
> Changes since v1:
> - Scaling mode and aspect ratio are partly handled in core now.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  | 142 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  14 +++-
>  drivers/gpu/drm/i915/intel_dp.c      |   2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  27 ++++++-
>  drivers/gpu/drm/i915/intel_dsi.c     |   2 +-
>  drivers/gpu/drm/i915/intel_dvo.c     |   2 +-
>  drivers/gpu/drm/i915/intel_lvds.c    |   2 +-
>  drivers/gpu/drm/i915/intel_panel.c   |   4 +-
>  8 files changed, 180 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 50fb1f76cc5f..eb72166675f0 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c

Refactoring idea for the future: intel_atomic.c makes 0 sense, just makes
sure we have lots of functionality spread all over imo. I think following
the split-up model I've done in the drm core would be a lot better, with
intel_connector/encoder/crtc/plane.c for the shared helper functions. And
then maybe separate files for the per-platform stuff, e.g. i9xx_crtc.c,
pch_crtc.c and so on.

> @@ -36,7 +36,7 @@
>  #include "intel_drv.h"
>  
>  /**
> - * intel_connector_atomic_get_property - fetch connector property value
> + * intel_connector_atomic_get_property - fetch legacy connector property value
>   * @connector: connector to fetch property for
>   * @state: state containing the property value
>   * @property: property to look up
> @@ -45,12 +45,14 @@
>   * The DRM core does not store shadow copies of properties for
>   * atomic-capable drivers.  This entrypoint is used to fetch
>   * the current value of a driver-specific connector property.
> + *
> + * This is a intermediary solution until all connectors are
> + * converted to support full atomic properties.
>   */
> -int
> -intel_connector_atomic_get_property(struct drm_connector *connector,
> -				    const struct drm_connector_state *state,
> -				    struct drm_property *property,
> -				    uint64_t *val)
> +int intel_connector_atomic_get_property(struct drm_connector *connector,
> +					const struct drm_connector_state *state,
> +					struct drm_property *property,
> +					uint64_t *val)
>  {
>  	int i;
>  
> @@ -73,7 +75,133 @@ intel_connector_atomic_get_property(struct drm_connector *connector,
>  	return -EINVAL;
>  }
>  
> -/*
> +/**
> + * intel_digital_connector_atomic_get_property - hook for connector->atomic_get_property.
> + * @connector: Connector to get the property for.
> + * @state: Connector state to retrieve the property from.
> + * @property: Property to retrieve.
> + * @val: Return value for the property.
> + *
> + * Returns the atomic property value for a digital connector.
> + */
> +int intel_digital_connector_atomic_get_property(struct drm_connector *connector,
> +						const struct drm_connector_state *state,
> +						struct drm_property *property,
> +						uint64_t *val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_digital_connector_state *intel_conn_state =
> +		to_intel_digital_connector_state(state);
> +
> +	if (property == dev_priv->force_audio_property)
> +		*val = intel_conn_state->force_audio;
> +	else if (property == dev_priv->broadcast_rgb_property)
> +		*val = intel_conn_state->broadcast_rgb;
> +	else {
> +		DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_digital_connector_atomic_set_property - hook for connector->atomic_set_property.
> + * @connector: Connector to set the property for.
> + * @state: Connector state to set the property on.
> + * @property: Property to set.
> + * @val: New value for the property.
> + *
> + * Sets the atomic property value for a digital connector.
> + */
> +int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> +						struct drm_connector_state *state,
> +						struct drm_property *property,
> +						uint64_t val)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_digital_connector_state *intel_conn_state =
> +		to_intel_digital_connector_state(state);
> +
> +	if (property == dev_priv->force_audio_property) {
> +		intel_conn_state->force_audio = val;
> +		return 0;
> +	}
> +
> +	if (property == dev_priv->broadcast_rgb_property) {
> +		intel_conn_state->broadcast_rgb = val;
> +		return 0;
> +	}
> +
> +	DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
> +	return -EINVAL;
> +}
> +
> +int intel_digital_connector_atomic_check(struct drm_connector *conn,
> +					 struct drm_connector_state *new_state)
> +{
> +	struct intel_digital_connector_state *new_conn_state =
> +		to_intel_digital_connector_state(new_state);
> +	struct drm_connector_state *old_state =
> +		drm_atomic_get_old_connector_state(new_state->state, conn);
> +	struct intel_digital_connector_state *old_conn_state =
> +		to_intel_digital_connector_state(old_state);
> +	struct intel_connector *intel_connector = to_intel_connector(conn);
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (intel_connector->panel.allowed_fitting_mode_mask &&
> +	    (BIT(new_state->scaling_mode) & ~intel_connector->panel.allowed_fitting_mode_mask)) {
> +		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] Panel fitting mode %u not allowed.\n",
> +				 conn->base.id, conn->name, new_state->scaling_mode);
> +		return -EINVAL;
> +	}

Range-checking like this should be done by correctling registering the
proeprty ... Separate patch to fix that (if it's indeed broken)?

> +
> +	if (!new_state->crtc)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);
> +
> +	/* Invisible to fastset since it's a pure connector property */
> +	if (new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode)
> +		crtc_state->connectors_changed = true;

Hm, I thought we put all the pfit register settings into the crtc state,
so scaling_mode changes should also be possible to handle by fastset?

Looks all reasonable otherwise.
-Daniel

> +
> +	/*
> +	 * These properties are handled by fastset, and might not end
> +	 * up in a modeset.
> +	 */
> +	if (new_conn_state->force_audio != old_conn_state->force_audio ||
> +	    new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb ||
> +	    new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio)
> +		crtc_state->mode_changed = true;
> +
> +	return 0;
> +}
> +
> +/**
> + * intel_digital_connector_duplicate_state - duplicate connector state
> + * @connector: digital connector
> + *
> + * Allocates and returns a copy of the connector state (both common and
> + * digital connector specific) for the specified connector.
> + *
> + * Returns: The newly allocated connector state, or NULL on failure.
> + */
> +struct drm_connector_state *
> +intel_digital_connector_duplicate_state(struct drm_connector *connector)
> +{
> +	struct intel_digital_connector_state *state;
> +
> +	state = kmemdup(connector->state, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return NULL;
> +
> +	__drm_atomic_helper_connector_duplicate_state(connector, &state->base);
> +	return &state->base;
> +}
> +
> +/**
>   * intel_crtc_duplicate_state - duplicate crtc state
>   * @crtc: drm crtc
>   *
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 48a546210d8b..5eb4ddb04797 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5956,11 +5956,21 @@ static void intel_connector_verify_state(struct intel_connector *connector)
>  
>  int intel_connector_init(struct intel_connector *connector)
>  {
> -	drm_atomic_helper_connector_reset(&connector->base);
> +	struct intel_digital_connector_state *conn_state;
>  
> -	if (!connector->base.state)
> +	/*
> +	 * Allocate enough memory to hold intel_digital_connector_state,
> +	 * This might be a few bytes too many, but for connectors that don't
> +	 * need it we'll free the state and allocate a smaller one on the first
> +	 * succesful commit anyway.
> +	 */
> +	conn_state = kzalloc(sizeof(*conn_state), GFP_KERNEL);
> +	if (!conn_state)
>  		return -ENOMEM;
>  
> +	__drm_atomic_helper_connector_reset(&connector->base,
> +					    &conn_state->base);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 944b1449b7f9..0afd8f3037f0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5896,7 +5896,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  			      pipe_name(pipe));
>  	}
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode, 0);
>  	intel_connector->panel.backlight.power = intel_edp_backlight_power;
>  	intel_panel_setup_backlight(connector, pipe);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f78d7c5f3805..4df9e4531edc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -262,6 +262,7 @@ struct intel_panel {
>  	struct drm_display_mode *fixed_mode;
>  	struct drm_display_mode *downclock_mode;
>  	int fitting_mode;
> +	unsigned allowed_fitting_mode_mask;
>  
>  	/* backlight */
>  	struct {
> @@ -323,6 +324,15 @@ struct intel_connector {
>  	struct intel_dp *mst_port;
>  };
>  
> +struct intel_digital_connector_state {
> +	struct drm_connector_state base;
> +
> +	enum hdmi_force_audio force_audio;
> +	int broadcast_rgb;
> +};
> +
> +#define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base)
> +
>  struct dpll {
>  	/* given values */
>  	int n;
> @@ -1668,7 +1678,8 @@ void intel_overlay_reset(struct drm_i915_private *dev_priv);
>  /* intel_panel.c */
>  int intel_panel_init(struct intel_panel *panel,
>  		     struct drm_display_mode *fixed_mode,
> -		     struct drm_display_mode *downclock_mode);
> +		     struct drm_display_mode *downclock_mode,
> +		     unsigned allowed_fitting_mode_mask);
>  void intel_panel_fini(struct intel_panel *panel);
>  void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
>  			    struct drm_display_mode *adjusted_mode);
> @@ -1879,6 +1890,20 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
>  					const struct drm_connector_state *state,
>  					struct drm_property *property,
>  					uint64_t *val);
> +
> +int intel_digital_connector_atomic_get_property(struct drm_connector *connector,
> +						const struct drm_connector_state *state,
> +						struct drm_property *property,
> +						uint64_t *val);
> +int intel_digital_connector_atomic_set_property(struct drm_connector *connector,
> +						struct drm_connector_state *state,
> +						struct drm_property *property,
> +						uint64_t val);
> +int intel_digital_connector_atomic_check(struct drm_connector *conn,
> +					 struct drm_connector_state *new_state);
> +struct drm_connector_state *
> +intel_digital_connector_duplicate_state(struct drm_connector *connector);
> +
>  struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>  void intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			       struct drm_crtc_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 3ffe8b1f1d48..e787142025ac 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -1853,7 +1853,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
>  	connector->display_info.width_mm = fixed_mode->width_mm;
>  	connector->display_info.height_mm = fixed_mode->height_mm;
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, NULL, 0);
>  	intel_panel_setup_backlight(connector, INVALID_PIPE);
>  
>  	intel_dsi_add_properties(intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index c1544a53095d..0b723157c38b 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -554,7 +554,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
>  			 */
>  			intel_panel_init(&intel_connector->panel,
>  					 intel_dvo_get_current_mode(connector),
> -					 NULL);
> +					 NULL, 0);
>  			intel_dvo->panel_wants_dither = true;
>  		}
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 8b942ef2b3ec..919d84290774 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1181,7 +1181,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>  out:
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode, 0);
>  	intel_panel_setup_backlight(connector, INVALID_PIPE);
>  
>  	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c527401f..c6ce15550e5e 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1801,12 +1801,14 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
>  
>  int intel_panel_init(struct intel_panel *panel,
>  		     struct drm_display_mode *fixed_mode,
> -		     struct drm_display_mode *downclock_mode)
> +		     struct drm_display_mode *downclock_mode,
> +		     unsigned allowed_fitting_mode_mask)
>  {
>  	intel_panel_init_backlight_funcs(panel);
>  
>  	panel->fixed_mode = fixed_mode;
>  	panel->downclock_mode = downclock_mode;
> +	panel->allowed_fitting_mode_mask = allowed_fitting_mode_mask;
>  
>  	return 0;
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the Intel-gfx mailing list