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

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) {