Re: [RESEND-CI v4 11/15] drm/i915: prepare scaler for YCBCR420 modeset

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Wed, 2017-06-21 at 16:04 +0530, Shashank Sharma wrote:
> To get a YCBCR420 output from intel platforms, we need one
> scaler to scale down YCBCR444 samples to YCBCR420 samples.
> 
> This patch:
> - Does scaler allocation for HDMI ycbcr420 outputs.
> - Programs PIPE_MISC register for ycbcr420 output.
> - Adds a new scaler user "HDMI output" to plug-into existing
>   scaler framework. This output type is identified using bit
>   30 of the scaler users bitmap.
> 
> V2: rebase
> V3: rebase
> V4: rebase
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  6 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_hdmi.c    | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_panel.c   |  3 ++-
>  5 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 36d4e63..a8c9ac5 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -264,6 +264,12 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>  
>  			/* panel fitter case: assign as a crtc scaler */
>  			scaler_id = &scaler_state->scaler_id;
> +		} else if (i == SKL_HDMI_OUTPUT_INDEX) {
> +			name = "HDMI-OUTPUT";
> +			idx = intel_crtc->base.base.id;
> +
> +			/* hdmi output case: needs a pipe scaler */
> +			scaler_id = &scaler_state->scaler_id;
>  		} else {
>  			name = "PLANE";
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index da29536..983f581 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4626,6 +4626,11 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> +	if (crtc_state->hdmi_output == DRM_HDMI_OUTPUT_YCBCR420
> +		&& scaler_user == SKL_HDMI_OUTPUT_INDEX)

Is it really necessary to check for both? If it is, what's the point of creating
SKL_HDMI_OUTPUT_INDEX?


> +		/* YCBCR 444 -> 420 conversion needs a scaler */
> +		need_scaling = true;
> +
>  	/*
>  	 * if plane is being disabled or scaler is no more required or force detach
>  	 *  - free scaler binded to this plane/crtc
> @@ -4673,6 +4678,26 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  }
>  
>  /**
> + * skl_update_scaler_hdmi_output - Stages update to scaler state for HDMI.
> + * HDMI YCBCR 420 output needs a scaler, for downsampling.
> + *
> + * @state: crtc's scaler state
> + *
> + * Return
> + *     0 - scaler_usage updated successfully
> + *    error - requested scaling cannot be supported or other error condition
> + */
> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state)
> +{
> +	const struct drm_display_mode *mode = &state->base.adjusted_mode;
> +
> +	return skl_update_scaler(state, !state->base.active,
> +		SKL_HDMI_OUTPUT_INDEX, &state->scaler_state.scaler_id,
> +		state->pipe_src_w, state->pipe_src_h,
> +		mode->crtc_hdisplay, mode->crtc_vdisplay);
> +}
> +
> +/**
>   * skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
>   *
>   * @state: crtc's scaler state
> @@ -8058,6 +8083,7 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum drm_hdmi_output_type hdmi_out = intel_crtc->config->hdmi_output;
>  
>  	if (IS_BROADWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 9) {
>  		u32 val = 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 38fe56c..2206aa8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -471,7 +471,8 @@ struct intel_crtc_scaler_state {
>  	 *
>  	 *     If a bit is set, a user is using a scaler.
>  	 *     Here user can be a plane or crtc as defined below:
> -	 *       bits 0-30 - plane (bit position is index from drm_plane_index)
> +	 *       bits 0-29 - plane (bit position is index from drm_plane_index)
> +	 *       bit 30    - hdmi output
>  	 *       bit 31    - crtc
>  	 *
>  	 * Instead of creating a new index to cover planes and crtc, using
> @@ -484,6 +485,12 @@ struct intel_crtc_scaler_state {
>  	 * avilability.
>  	 */
>  #define SKL_CRTC_INDEX 31
> +
> +	/*
> +	 * HDMI YCBCR 420 output consume a scaler. So adding a user
> +	 * for HDMI output 420 requirement.
> +	 */
> +#define SKL_HDMI_OUTPUT_INDEX 30

I'm not convinced about this. The scaler is still used as a pipe scaler and the
patch even adds a call intel_pch_panel_fitting().

>  	unsigned scaler_users;
>  
>  	/* scaler used by crtc for panel fitting purpose */
> @@ -1481,6 +1488,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  				 struct intel_crtc_state *pipe_config);
>  
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
> +int skl_update_scaler_crtc_hdmi_output(struct intel_crtc_state *state);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
>  
>  static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 22da5cd..8d5aa1e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1415,6 +1415,7 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state,
>  	}
>  
>  	if (type == DRM_HDMI_OUTPUT_YCBCR420) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(conn_state->crtc);
>  
>  		/* YCBCR420 TMDS rate requirement is half the pixel clock */
>  		config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
> @@ -1422,6 +1423,15 @@ intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state,
>  		*clock_12bpc /= 2;
>  		*clock_8bpc /= 2;
>  
> +		/* YCBCR 420 output conversion needs a scaler */
> +		if (skl_update_scaler_crtc_hdmi_output(config)) {
> +			DRM_ERROR("Scaler allocation for output failed\n");
> +			return DRM_HDMI_OUTPUT_INVALID;
> +		}
> +
> +		/* Bind this scaler to pipe */
> +		intel_pch_panel_fitting(intel_crtc, config,
> +					DRM_MODE_SCALE_FULLSCREEN);
>  	}
>  
>  	/* Encoder is capable of this output, lets commit to CRTC */
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 96c2cbd..b6a32c4 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -110,7 +110,8 @@ intel_pch_panel_fitting(struct intel_crtc *intel_crtc,
>  
>  	/* Native modes don't need fitting */
>  	if (adjusted_mode->crtc_hdisplay == pipe_config->pipe_src_w &&
> -	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h)
> +	    adjusted_mode->crtc_vdisplay == pipe_config->pipe_src_h &&
> +	    pipe_config->hdmi_output != DRM_HDMI_OUTPUT_YCBCR420)

I don't like that the knowledge that YCBCR420 needs the scaler is spread
everywhere, including this function that should only deal with panel fitting. I
would rather add a force parameter to intel_pch_panel_fitting() and
skl_update_scaler() that would mean setup the scaler even if looks like it's not
necessary. That way the hdmi code would just call them with force enabled,
without sprinkling "hdmi_output == YCBCR420" everywhere.

Ander

>  		goto done;
>  
>  	switch (fitting_mode) {
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel