Re: [PATCH v10 37/40] drm/i915: Commit CP without modeset

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jan 31, 2019 at 12:29:53PM +0530, Ramalingam C wrote:
> Commits the content protection change of a connector,
> without crtc modeset. This improves the user experience.
> 
> Originally proposed by Sean Paul at v3 of HDCP1.4 framework
> https://patchwork.freedesktop.org/patch/191759/. For some
> reason this was dropped, but needed for the proper functionality
> of HDCP encryption.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Suggested-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  7 -------
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>  drivers/gpu/drm/i915/intel_hdcp.c    | 32 ++++++++++++++++++++++++++++----
>  4 files changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ca705546a0ab..9cb03c1b0abc 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3495,11 +3495,6 @@ static void intel_enable_ddi(struct intel_encoder *encoder,
>  		intel_enable_ddi_hdmi(encoder, crtc_state, conn_state);
>  	else
>  		intel_enable_ddi_dp(encoder, crtc_state, conn_state);
> -
> -	/* Enable hdcp if it's desired */
> -	if (conn_state->content_protection ==
> -	    DRM_MODE_CONTENT_PROTECTION_DESIRED)
> -		intel_hdcp_enable(to_intel_connector(conn_state->connector));
>  }
>  
>  static void intel_disable_ddi_dp(struct intel_encoder *encoder,
> @@ -3542,8 +3537,6 @@ static void intel_disable_ddi(struct intel_encoder *encoder,
>  			      const struct intel_crtc_state *old_crtc_state,
>  			      const struct drm_connector_state *old_conn_state)
>  {
> -	intel_hdcp_disable(to_intel_connector(old_conn_state->connector));
> -
>  	if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI))
>  		intel_disable_ddi_hdmi(encoder, old_crtc_state, old_conn_state);
>  	else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a025efb1d7c6..9b964dabb57c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13034,6 +13034,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	struct drm_connector *connector;
>  	struct drm_crtc *crtc;
>  	struct intel_crtc *intel_crtc;
>  	u64 put_domains[I915_MAX_PIPES] = {};
> @@ -13128,9 +13130,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> +					   new_conn_state, i)
> +		intel_hdcp_atomic_pre_commit(connector, old_conn_state,
> +					     new_conn_state);
> +
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display.update_crtcs(state);
>  
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i)
> +		intel_hdcp_atomic_commit(connector, new_conn_state);

So this is kinda awkward, having these one-use callbacks for hdcp.
"changing connector state without a full modeset" is a generic problem,
and Hans has added a new intel_encoder->update_pipe callback for this
stuff. The way to use it:

- Keep the enable/disable calls where they currently are, we still need
  them for full modesets.

- Add a new update_pipe callback, which is for the !needs_modeset case for
  both hdmi and dp, and adjust hdcp state there.

- Adjust compute_config to only flag update_pipe, not a full modeset in
  compute_config (in atomic_check).

This should fit a lot neater into the overall infrastructure for fastset
and all that stuff.

For validation the problem is that we're now adding two paths, so need to
make sure the content protection test copes. We need to make sure that
we're enabling content protection both with a modeset and without,
probably the simplest trick to get there is to add a new subtest which
does a dpms off->on once content protection is on, and checks that it
stays on.

Also we need CI results without this patch so I can start merging. Rough
merge plan:
- needs ack to merge component.c through drm-intel
- merge all the i915 patches
- topic branch for mei, shared with mei subsystem
- make sure that CI has mei enabled and that tests aren't on fire
- merge this one here

Cheers, Daniel

> +
>  	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
>  	 * already, but still need the state for the delayed optimization. To
>  	 * fix this:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1901d12bacc4..e6b6e920ab01 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2129,6 +2129,11 @@ static inline void intel_backlight_device_unregister(struct intel_connector *con
>  void intel_hdcp_atomic_check(struct drm_connector *connector,
>  			     struct drm_connector_state *old_state,
>  			     struct drm_connector_state *new_state);
> +void intel_hdcp_atomic_pre_commit(struct drm_connector *connector,
> +				  struct drm_connector_state *old_state,
> +				  struct drm_connector_state *new_state);
> +void intel_hdcp_atomic_commit(struct drm_connector *connector,
> +			      struct drm_connector_state *new_state);
>  int intel_hdcp_init(struct intel_connector *connector,
>  		    const struct intel_hdcp_shim *hdcp_shim,
>  		    bool hdcp2_supported);
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index f38feeadb363..ef127dc7a241 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -1876,7 +1876,6 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  {
>  	u64 old_cp = old_state->content_protection;
>  	u64 new_cp = new_state->content_protection;
> -	struct drm_crtc_state *crtc_state;
>  
>  	if (!new_state->crtc) {
>  		/*
> @@ -1897,10 +1896,35 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  	    (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
>  	     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
>  		return;
> +}
> +
> +void intel_hdcp_atomic_pre_commit(struct drm_connector *connector,
> +				  struct drm_connector_state *old_state,
> +				  struct drm_connector_state *new_state)
> +{
> +	u64 old_cp = old_state->content_protection;
> +	u64 new_cp = new_state->content_protection;
> +
> +	/*
> +	 * Disable HDCP if the connector is becoming disabled, or if requested
> +	 * via the property.
> +	 */
> +	if ((!new_state->crtc &&
> +	     old_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) ||
> +	    (new_state->crtc &&
> +	     old_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED &&
> +	     new_cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
> +		intel_hdcp_disable(to_intel_connector(connector));
> +}
> +
> +void intel_hdcp_atomic_commit(struct drm_connector *connector,
> +			      struct drm_connector_state *new_state)
> +{
> +	u64 new_cp = new_state->content_protection;
>  
> -	crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
> -						   new_state->crtc);
> -	crtc_state->mode_changed = true;
> +	/* Enable hdcp if it's desired */
> +	if (new_state->crtc && new_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED)
> +		intel_hdcp_enable(to_intel_connector(connector));
>  }
>  
>  /* Handles the CP_IRQ raised from the DP HDCP sink */
> -- 
> 2.7.4
> 

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