[Intel-gfx] [PATCH 06/11] drm/i915: Enable big joiner support in enable and disable sequences.

[Maarten Lankhorst]

Op 28-11-2019 om 20:43 schreef Ville Syrjälä:
> On Thu, Nov 14, 2019 at 05:05:17PM +0100, Maarten Lankhorst wrote:
>> Make vdsc work when no output is enabled. The big joiner needs VDSC
>> on the slave, so enable it and set the appropriate bits.
>> Also update timestamping constants, because slave crtc's are not
>> updated in drm_atomic_helper_update_legacy_modeset_state().
>>
>> This should be enough to bring up CRTC's in a big joiner configuration,
>> without any plane configuration on the second pipe yet.
>>
>> HOWEVER, we still bring up the crtc's in the wrong order. We need to
>> make sure that the master crtc is brought up after the slave crtc.
>> This is done correctly later in this series.
>>
>> The next steps are to enable planes correctly, and make sure we enable
>> and update both master and slave in the correct order.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_ddi.c      |  48 ++-
>>  drivers/gpu/drm/i915/display/intel_display.c  | 399 ++++++++++++------
>>  .../drm/i915/display/intel_display_types.h    |  22 +
>>  drivers/gpu/drm/i915/display/intel_dp.c       |  21 +-
>>  drivers/gpu/drm/i915/display/intel_vdsc.c     | 122 ++++--
>>  drivers/gpu/drm/i915/display/intel_vdsc.h     |   2 +
>>  6 files changed, 408 insertions(+), 206 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
>> index 8f817de34460..1215f619da36 100644
>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>> @@ -2218,13 +2218,6 @@ static void intel_ddi_get_power_domains(struct intel_encoder *encoder,
>>  	    intel_phy_is_tc(dev_priv, phy))
>>  		intel_display_power_get(dev_priv,
>>  					intel_ddi_main_link_aux_domain(dig_port));
>> -
>> -	/*
>> -	 * VDSC power is needed when DSC is enabled
>> -	 */
>> -	if (crtc_state->dsc.compression_enable)
>> -		intel_display_power_get(dev_priv,
>> -					intel_dsc_power_domain(crtc_state));
>>  }
>>  
>>  void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state)
>> @@ -3557,7 +3550,8 @@ static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>  
>>  	/* 7.l Configure and enable FEC if needed */
>>  	intel_ddi_enable_fec(encoder, crtc_state);
>> -	intel_dsc_enable(encoder, crtc_state);
>> +	if (!crtc_state->bigjoiner)
>> +		intel_dsc_enable(encoder, crtc_state);
>>  }
>>  
>>  static void hsw_ddi_pre_enable_dp(struct intel_encoder *encoder,
>> @@ -3629,7 +3623,8 @@ static void hsw_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>  	if (!is_mst)
>>  		intel_ddi_enable_pipe_clock(crtc_state);
>>  
>> -	intel_dsc_enable(encoder, crtc_state);
>> +	if (!crtc_state->bigjoiner)
>> +		intel_dsc_enable(encoder, crtc_state);
>>  }
>>  
>>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>> @@ -4252,19 +4247,18 @@ void intel_ddi_compute_min_voltage_level(struct drm_i915_private *dev_priv,
>>  		crtc_state->min_voltage_level = 2;
>>  }
>>  
>> -void intel_ddi_get_config(struct intel_encoder *encoder,
>> -			  struct intel_crtc_state *pipe_config)
>> +static void intel_ddi_read_func_ctl(struct intel_encoder *encoder,
>> +				    struct intel_crtc_state *pipe_config)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->uapi.crtc);
>>  	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
>>  	u32 temp, flags = 0;
>>  
>> -	/* XXX: DSI transcoder paranoia */
>> -	if (WARN_ON(transcoder_is_dsi(cpu_transcoder)))
>> +	temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
>> +	if (!(temp & TRANS_DDI_FUNC_ENABLE))
>>  		return;
>>  
>> -	temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
>>  	if (temp & TRANS_DDI_PHSYNC)
>>  		flags |= DRM_MODE_FLAG_PHSYNC;
>>  	else
>> @@ -4350,6 +4344,29 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>>  	default:
>>  		break;
>>  	}
>> +}
>> +
>> +void intel_ddi_get_config(struct intel_encoder *encoder,
>> +			  struct intel_crtc_state *pipe_config)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
>> +
>> +	/* XXX: DSI transcoder paranoia */
>> +	if (WARN_ON(transcoder_is_dsi(cpu_transcoder)))
>> +		return;
>> +
>> +	intel_ddi_read_func_ctl(encoder, pipe_config);
>> +	if (pipe_config->bigjoiner_slave) {
>> +		/* read out pipe settings from master */
>> +		enum transcoder save = pipe_config->cpu_transcoder;
>> +
>> +		 /* Our own transcoder needs to be disabled when reading it in intel_ddi_read_func_ctl() */
>> +		WARN_ON(pipe_config->output_types);
>> +		pipe_config->cpu_transcoder = (enum transcoder)pipe_config->bigjoiner_linked_crtc->pipe;
> That's pretty horrible.
>
> I don't understand the enable/disable sequence enough to know what
> we need to configure/enable where exactly. Bspec just says:
> "2. Follow the steps to Enable Planes, Pipe, and Transcoder for pipe C, but
>     do not configure or enable transcoder C.
>  3. Follow the steps to Enable Planes, Pipe, and Transcoder for pipe B and
>     transcoder B."
>
> So we need to enable transcoder for pipe C but not enable transcoder C?
> I have no idea what actually means.

Correct, bspec is slightly unclear here.

First we have to bring up the transcoder for pipe B, up to and including enabling FEC.

After this, we enable pipe C. None of the transcoder registers for pipe C are ever
touched. On pipe C, only the PIPE_* registers are touched, after that we set
DSS_CTL1/2 to the correct values for bigjoiner slave mode.

After we set up pipe C, we bring up pipe B, continuing from the step after enabling FEC.

When immplementing this I was getting a consistent FIFO underrun, but that was because
we missed the FEC overhead and it wasn't tested on real hw before. It was fixed in
commit cffb4c3ea372 "drm/i915/dp: Fix dsc bpp calculations, v5."

After fixing that, the FIFO underrun disappeared and presumably it works on real
hardware now, wasn't able to verify on a real DSC monitor. Only on a DSC emulator. :)

~Maarten