diff mbox series

[v7,09/15] drm/i915: Add support for starting FRL training for HDMI2.1 via PCON

Message ID 20201218103723.30844-10-ankit.k.nautiyal@intel.com (mailing list archive)
State New, archived
Headers show
Series Add support for DP-HDMI2.1 PCON | expand

Commit Message

Nautiyal, Ankit K Dec. 18, 2020, 10:37 a.m. UTC 
This patch adds functions to start FRL training for an HDMI2.1 sink,
connected via a PCON as a DP branch device.
This patch also adds a new structure for storing frl training related
data, when FRL training is completed.

v2: As suggested by Uma Shankar:
-renamed couple of variables for better clarity
-tweaked the macros used for correct semantics for true/false
-fixed other styling issues.

v3: Completed the TODO for condition for going to FRL mode.
Modified the condition to determine the required FRL b/w
based only on the Pcon and Sink's max FRL values.
Moved the frl structure initialization to intel_dp_init_connector().

v4: Fixed typo in initialization of frl structure.

v5: Always use FRL if its possible, instead of enabling only for
higher modes as done in v3.

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Reviewed-by: Uma Shankar <uma.shankar@intel.com> (v2)
---
 .../drm/i915/display/intel_display_types.h    |   7 +
 drivers/gpu/drm/i915/display/intel_dp.c       | 151 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.h       |   2 +
 3 files changed, 160 insertions(+)

Comments

Ville Syrjala Feb. 1, 2021, 8:38 p.m. UTC | #1 
On Fri, Dec 18, 2020 at 04:07:17PM +0530, Ankit Nautiyal wrote:
> This patch adds functions to start FRL training for an HDMI2.1 sink,
> connected via a PCON as a DP branch device.
> This patch also adds a new structure for storing frl training related
> data, when FRL training is completed.
> 
> v2: As suggested by Uma Shankar:
> -renamed couple of variables for better clarity
> -tweaked the macros used for correct semantics for true/false
> -fixed other styling issues.
> 
> v3: Completed the TODO for condition for going to FRL mode.
> Modified the condition to determine the required FRL b/w
> based only on the Pcon and Sink's max FRL values.
> Moved the frl structure initialization to intel_dp_init_connector().
> 
> v4: Fixed typo in initialization of frl structure.
> 
> v5: Always use FRL if its possible, instead of enabling only for
> higher modes as done in v3.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Reviewed-by: Uma Shankar <uma.shankar@intel.com> (v2)
> ---
>  .../drm/i915/display/intel_display_types.h    |   7 +
>  drivers/gpu/drm/i915/display/intel_dp.c       | 151 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.h       |   2 +
>  3 files changed, 160 insertions(+)
<snip>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0596d6c24e73..43027a6d5e5e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3891,6 +3891,8 @@ static void intel_disable_dp(struct intel_atomic_state *state,
>  	intel_edp_backlight_off(old_conn_state);
>  	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
>  	intel_edp_panel_off(intel_dp);
> +	intel_dp->frl.is_trained = false;
> +	intel_dp->frl.trained_rate_gbps = 0;

This stuff looks rather misplaced (or missing from elsewhere). This code
doesn't even get executed on modern platforms.

<snip>
> +static int intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)
> +{
> +#define PCON_EXTENDED_TRAIN_MODE (1 > 0)
> +#define PCON_CONCURRENT_MODE (1 > 0)
> +#define PCON_SEQUENTIAL_MODE !PCON_CONCURRENT_MODE
> +#define PCON_NORMAL_TRAIN_MODE !PCON_EXTENDED_TRAIN_MODE

All of that looks like nonsense. What is it supposed to do?

> +#define TIMEOUT_FRL_READY_MS 500
> +#define TIMEOUT_HDMI_LINK_ACTIVE_MS 1000
> +
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	int max_frl_bw, max_pcon_frl_bw, max_edid_frl_bw, ret;
> +	u8 max_frl_bw_mask = 0, frl_trained_mask;
> +	bool is_active;
> +
> +	ret = drm_dp_pcon_reset_frl_config(&intel_dp->aux);
> +	if (ret < 0)
> +		return ret;
> +
> +	max_pcon_frl_bw = intel_dp->dfp.pcon_max_frl_bw;
> +	drm_dbg(&i915->drm, "PCON max rate = %d Gbps\n", max_pcon_frl_bw);
> +
> +	max_edid_frl_bw = intel_dp_hdmi_sink_max_frl(intel_dp);
> +	drm_dbg(&i915->drm, "Sink max rate from EDID = %d Gbps\n", max_edid_frl_bw);
> +
> +	max_frl_bw = min(max_edid_frl_bw, max_pcon_frl_bw);
> +
> +	if (max_frl_bw <= 0)
> +		return -EINVAL;
> +
> +	ret = drm_dp_pcon_frl_prepare(&intel_dp->aux, false);
> +	if (ret < 0)
> +		return ret;
> +	/* Wait for PCON to be FRL Ready */
> +	wait_for(is_active = drm_dp_pcon_is_frl_ready(&intel_dp->aux) == true, TIMEOUT_FRL_READY_MS);
> +
> +	if (!is_active)
> +		return -ETIMEDOUT;
> +
> +	max_frl_bw_mask = intel_dp_pcon_set_frl_mask(max_frl_bw);
> +	ret = drm_dp_pcon_frl_configure_1(&intel_dp->aux, max_frl_bw, PCON_SEQUENTIAL_MODE);
> +	if (ret < 0)
> +		return ret;
> +	ret = drm_dp_pcon_frl_configure_2(&intel_dp->aux, max_frl_bw_mask, PCON_NORMAL_TRAIN_MODE);
> +	if (ret < 0)
> +		return ret;
> +	ret = drm_dp_pcon_frl_enable(&intel_dp->aux);
> +	if (ret < 0)
> +		return ret;
> +	/*
> +	 * Wait for FRL to be completed
> +	 * Check if the HDMI Link is up and active.
> +	 */
> +	wait_for(is_active = drm_dp_pcon_hdmi_link_active(&intel_dp->aux) == true, TIMEOUT_HDMI_LINK_ACTIVE_MS);
> +
> +	if (!is_active)
> +		return -ETIMEDOUT;
> +
> +	/* Verify HDMI Link configuration shows FRL Mode */
> +	if (DP_PCON_HDMI_MODE_FRL != drm_dp_pcon_hdmi_link_mode(&intel_dp->aux, &frl_trained_mask)) {
> +		drm_dbg(&i915->drm, "HDMI couldn't be trained in FRL Mode\n");
> +		return -EINVAL;
> +	}
> +	drm_dbg(&i915->drm, "MAX_FRL_MASK = %u, FRL_TRAINED_MASK = %u\n", max_frl_bw_mask, frl_trained_mask);
> +
> +	intel_dp->frl.trained_rate_gbps = intel_dp_pcon_get_frl_mask(frl_trained_mask);
> +	intel_dp->frl.is_trained = true;
> +	drm_dbg(&i915->drm, "FRL trained with : %d Gbps\n", intel_dp->frl.trained_rate_gbps);
> +
> +	return 0;
> +}
> +
> +static bool intel_dp_is_hdmi_2_1_sink(struct intel_dp *intel_dp)
> +{
> +	if (drm_dp_is_branch(intel_dp->dpcd) &&
> +	    intel_dp->has_hdmi_sink &&
> +	    intel_dp_hdmi_sink_max_frl(intel_dp) > 0)
> +		return true;
> +
> +	return false;
> +}
> +
> +void intel_dp_check_frl_training(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	/* Always go for FRL training if supported */
> +	if (!intel_dp_is_hdmi_2_1_sink(intel_dp) ||
> +	    intel_dp->frl.is_trained)
> +		return;
> +
> +	if (intel_dp_pcon_start_frl_training(intel_dp) < 0) {
> +		int ret, mode;
> +
> +		drm_dbg(&dev_priv->drm, "Couldnt set FRL mode, continuing with TMDS mode\n");
> +		ret = drm_dp_pcon_reset_frl_config(&intel_dp->aux);
> +		mode = drm_dp_pcon_hdmi_link_mode(&intel_dp->aux, NULL);
> +
> +		if (ret < 0 || mode != DP_PCON_HDMI_MODE_TMDS)
> +			drm_dbg(&dev_priv->drm, "Issue with PCON, cannot set TMDS mode\n");
> +	} else {
> +		drm_dbg(&dev_priv->drm, "FRL training Completed\n");
> +	}
> +}
> +
>  static void
>  g4x_set_link_train(struct intel_dp *intel_dp,
>  		   const struct intel_crtc_state *crtc_state,
> @@ -8210,6 +8358,9 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  			       (temp & ~0xf) | 0xd);
>  	}
>  
> +	intel_dp->frl.is_trained = false;
> +	intel_dp->frl.trained_rate_gbps = 0;
> +
>  	return true;
>  
>  fail:
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index b871a09b6901..58b76a20459c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -144,4 +144,6 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
>  void intel_dp_sync_state(struct intel_encoder *encoder,
>  			 const struct intel_crtc_state *crtc_state);
>  
> +void intel_dp_check_frl_training(struct intel_dp *intel_dp);
> +
>  #endif /* __INTEL_DP_H__ */
> -- 
> 2.17.1
Nautiyal, Ankit K Feb. 2, 2021, 6:39 a.m. UTC | #2 
Hi Ville,

Please find my responses inline.

On 2/2/2021 2:08 AM, Ville Syrjälä wrote:
> On Fri, Dec 18, 2020 at 04:07:17PM +0530, Ankit Nautiyal wrote:
>> This patch adds functions to start FRL training for an HDMI2.1 sink,
>> connected via a PCON as a DP branch device.
>> This patch also adds a new structure for storing frl training related
>> data, when FRL training is completed.
>>
>> v2: As suggested by Uma Shankar:
>> -renamed couple of variables for better clarity
>> -tweaked the macros used for correct semantics for true/false
>> -fixed other styling issues.
>>
>> v3: Completed the TODO for condition for going to FRL mode.
>> Modified the condition to determine the required FRL b/w
>> based only on the Pcon and Sink's max FRL values.
>> Moved the frl structure initialization to intel_dp_init_connector().
>>
>> v4: Fixed typo in initialization of frl structure.
>>
>> v5: Always use FRL if its possible, instead of enabling only for
>> higher modes as done in v3.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Reviewed-by: Uma Shankar <uma.shankar@intel.com> (v2)
>> ---
>>   .../drm/i915/display/intel_display_types.h    |   7 +
>>   drivers/gpu/drm/i915/display/intel_dp.c       | 151 ++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dp.h       |   2 +
>>   3 files changed, 160 insertions(+)
> <snip>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 0596d6c24e73..43027a6d5e5e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -3891,6 +3891,8 @@ static void intel_disable_dp(struct intel_atomic_state *state,
>>   	intel_edp_backlight_off(old_conn_state);
>>   	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
>>   	intel_edp_panel_off(intel_dp);
>> +	intel_dp->frl.is_trained = false;
>> +	intel_dp->frl.trained_rate_gbps = 0;
> This stuff looks rather misplaced (or missing from elsewhere). This code
> doesn't even get executed on modern platforms.

I think these two lines should have been added to 
intel_ddi_post_disable_dp() for TGL+

My intention was to reset these before disabling DP. In hindsight, since 
we are initializing (resetting) these in dp_init_connector, this doesnt 
seem to be required.

I will send a patch to remove these two lines from here.


>
> <snip>
>> +static int intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)
>> +{
>> +#define PCON_EXTENDED_TRAIN_MODE (1 > 0)
>> +#define PCON_CONCURRENT_MODE (1 > 0)
>> +#define PCON_SEQUENTIAL_MODE !PCON_CONCURRENT_MODE
>> +#define PCON_NORMAL_TRAIN_MODE !PCON_EXTENDED_TRAIN_MODE
> All of that looks like nonsense. What is it supposed to do?

When asking an HDMI2.1 PCON to initiate FRL training there are 2 options:

Sequential/Concurrent mode: Sequential mode attempts the FRL training 
after DP Link training is completed. Concurrent mode tries to do the FRL 
training, during DP link training.

Normal train Mode/ Extended mode: Normal train mode, PCON FW trains FRL 
from Max to min BW, set by source in BW Mask. It aborts on first 
successful training. In Extended mode, PCON FW trains for all BW set by 
source in BW mask.

For Concurrent and Extended mode we need to set some extra bits in PCON 
FRL config DPCDs

The intention was to go with sequential and Normal training mode, so no 
need to set above bits.

Do you think, some documentation will make this clear?

Thanks & Regards,

Ankit

>
>> +#define TIMEOUT_FRL_READY_MS 500
>> +#define TIMEOUT_HDMI_LINK_ACTIVE_MS 1000
>> +
>> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> +	int max_frl_bw, max_pcon_frl_bw, max_edid_frl_bw, ret;
>> +	u8 max_frl_bw_mask = 0, frl_trained_mask;
>> +	bool is_active;
>> +
>> +	ret = drm_dp_pcon_reset_frl_config(&intel_dp->aux);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	max_pcon_frl_bw = intel_dp->dfp.pcon_max_frl_bw;
>> +	drm_dbg(&i915->drm, "PCON max rate = %d Gbps\n", max_pcon_frl_bw);
>> +
>> +	max_edid_frl_bw = intel_dp_hdmi_sink_max_frl(intel_dp);
>> +	drm_dbg(&i915->drm, "Sink max rate from EDID = %d Gbps\n", max_edid_frl_bw);
>> +
>> +	max_frl_bw = min(max_edid_frl_bw, max_pcon_frl_bw);
>> +
>> +	if (max_frl_bw <= 0)
>> +		return -EINVAL;
>> +
>> +	ret = drm_dp_pcon_frl_prepare(&intel_dp->aux, false);
>> +	if (ret < 0)
>> +		return ret;
>> +	/* Wait for PCON to be FRL Ready */
>> +	wait_for(is_active = drm_dp_pcon_is_frl_ready(&intel_dp->aux) == true, TIMEOUT_FRL_READY_MS);
>> +
>> +	if (!is_active)
>> +		return -ETIMEDOUT;
>> +
>> +	max_frl_bw_mask = intel_dp_pcon_set_frl_mask(max_frl_bw);
>> +	ret = drm_dp_pcon_frl_configure_1(&intel_dp->aux, max_frl_bw, PCON_SEQUENTIAL_MODE);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = drm_dp_pcon_frl_configure_2(&intel_dp->aux, max_frl_bw_mask, PCON_NORMAL_TRAIN_MODE);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = drm_dp_pcon_frl_enable(&intel_dp->aux);
>> +	if (ret < 0)
>> +		return ret;
>> +	/*
>> +	 * Wait for FRL to be completed
>> +	 * Check if the HDMI Link is up and active.
>> +	 */
>> +	wait_for(is_active = drm_dp_pcon_hdmi_link_active(&intel_dp->aux) == true, TIMEOUT_HDMI_LINK_ACTIVE_MS);
>> +
>> +	if (!is_active)
>> +		return -ETIMEDOUT;
>> +
>> +	/* Verify HDMI Link configuration shows FRL Mode */
>> +	if (DP_PCON_HDMI_MODE_FRL != drm_dp_pcon_hdmi_link_mode(&intel_dp->aux, &frl_trained_mask)) {
>> +		drm_dbg(&i915->drm, "HDMI couldn't be trained in FRL Mode\n");
>> +		return -EINVAL;
>> +	}
>> +	drm_dbg(&i915->drm, "MAX_FRL_MASK = %u, FRL_TRAINED_MASK = %u\n", max_frl_bw_mask, frl_trained_mask);
>> +
>> +	intel_dp->frl.trained_rate_gbps = intel_dp_pcon_get_frl_mask(frl_trained_mask);
>> +	intel_dp->frl.is_trained = true;
>> +	drm_dbg(&i915->drm, "FRL trained with : %d Gbps\n", intel_dp->frl.trained_rate_gbps);
>> +
>> +	return 0;
>> +}
>> +
>> +static bool intel_dp_is_hdmi_2_1_sink(struct intel_dp *intel_dp)
>> +{
>> +	if (drm_dp_is_branch(intel_dp->dpcd) &&
>> +	    intel_dp->has_hdmi_sink &&
>> +	    intel_dp_hdmi_sink_max_frl(intel_dp) > 0)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +void intel_dp_check_frl_training(struct intel_dp *intel_dp)
>> +{
>> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> +
>> +	/* Always go for FRL training if supported */
>> +	if (!intel_dp_is_hdmi_2_1_sink(intel_dp) ||
>> +	    intel_dp->frl.is_trained)
>> +		return;
>> +
>> +	if (intel_dp_pcon_start_frl_training(intel_dp) < 0) {
>> +		int ret, mode;
>> +
>> +		drm_dbg(&dev_priv->drm, "Couldnt set FRL mode, continuing with TMDS mode\n");
>> +		ret = drm_dp_pcon_reset_frl_config(&intel_dp->aux);
>> +		mode = drm_dp_pcon_hdmi_link_mode(&intel_dp->aux, NULL);
>> +
>> +		if (ret < 0 || mode != DP_PCON_HDMI_MODE_TMDS)
>> +			drm_dbg(&dev_priv->drm, "Issue with PCON, cannot set TMDS mode\n");
>> +	} else {
>> +		drm_dbg(&dev_priv->drm, "FRL training Completed\n");
>> +	}
>> +}
>> +
>>   static void
>>   g4x_set_link_train(struct intel_dp *intel_dp,
>>   		   const struct intel_crtc_state *crtc_state,
>> @@ -8210,6 +8358,9 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>>   			       (temp & ~0xf) | 0xd);
>>   	}
>>   
>> +	intel_dp->frl.is_trained = false;
>> +	intel_dp->frl.trained_rate_gbps = 0;
>> +
>>   	return true;
>>   
>>   fail:
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
>> index b871a09b6901..58b76a20459c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
>> @@ -144,4 +144,6 @@ bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
>>   void intel_dp_sync_state(struct intel_encoder *encoder,
>>   			 const struct intel_crtc_state *crtc_state);
>>   
>> +void intel_dp_check_frl_training(struct intel_dp *intel_dp);
>> +
>>   #endif /* __INTEL_DP_H__ */
>> -- 
>> 2.17.1
Ville Syrjala Feb. 2, 2021, 6:47 a.m. UTC | #3 
On Tue, Feb 02, 2021 at 12:09:47PM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
> 
> Please find my responses inline.
> 
> On 2/2/2021 2:08 AM, Ville Syrjälä wrote:
> > On Fri, Dec 18, 2020 at 04:07:17PM +0530, Ankit Nautiyal wrote:
> >> This patch adds functions to start FRL training for an HDMI2.1 sink,
> >> connected via a PCON as a DP branch device.
> >> This patch also adds a new structure for storing frl training related
> >> data, when FRL training is completed.
> >>
> >> v2: As suggested by Uma Shankar:
> >> -renamed couple of variables for better clarity
> >> -tweaked the macros used for correct semantics for true/false
> >> -fixed other styling issues.
> >>
> >> v3: Completed the TODO for condition for going to FRL mode.
> >> Modified the condition to determine the required FRL b/w
> >> based only on the Pcon and Sink's max FRL values.
> >> Moved the frl structure initialization to intel_dp_init_connector().
> >>
> >> v4: Fixed typo in initialization of frl structure.
> >>
> >> v5: Always use FRL if its possible, instead of enabling only for
> >> higher modes as done in v3.
> >>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> Reviewed-by: Uma Shankar <uma.shankar@intel.com> (v2)
> >> ---
> >>   .../drm/i915/display/intel_display_types.h    |   7 +
> >>   drivers/gpu/drm/i915/display/intel_dp.c       | 151 ++++++++++++++++++
> >>   drivers/gpu/drm/i915/display/intel_dp.h       |   2 +
> >>   3 files changed, 160 insertions(+)
> > <snip>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index 0596d6c24e73..43027a6d5e5e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> @@ -3891,6 +3891,8 @@ static void intel_disable_dp(struct intel_atomic_state *state,
> >>   	intel_edp_backlight_off(old_conn_state);
> >>   	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
> >>   	intel_edp_panel_off(intel_dp);
> >> +	intel_dp->frl.is_trained = false;
> >> +	intel_dp->frl.trained_rate_gbps = 0;
> > This stuff looks rather misplaced (or missing from elsewhere). This code
> > doesn't even get executed on modern platforms.
> 
> I think these two lines should have been added to 
> intel_ddi_post_disable_dp() for TGL+
> 
> My intention was to reset these before disabling DP. In hindsight, since 
> we are initializing (resetting) these in dp_init_connector, this doesnt 
> seem to be required.
> 
> I will send a patch to remove these two lines from here.
> 
> 
> >
> > <snip>
> >> +static int intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)
> >> +{
> >> +#define PCON_EXTENDED_TRAIN_MODE (1 > 0)
> >> +#define PCON_CONCURRENT_MODE (1 > 0)
> >> +#define PCON_SEQUENTIAL_MODE !PCON_CONCURRENT_MODE
> >> +#define PCON_NORMAL_TRAIN_MODE !PCON_EXTENDED_TRAIN_MODE
> > All of that looks like nonsense. What is it supposed to do?
> 
> When asking an HDMI2.1 PCON to initiate FRL training there are 2 options:
> 
> Sequential/Concurrent mode: Sequential mode attempts the FRL training 
> after DP Link training is completed. Concurrent mode tries to do the FRL 
> training, during DP link training.
> 
> Normal train Mode/ Extended mode: Normal train mode, PCON FW trains FRL 
> from Max to min BW, set by source in BW Mask. It aborts on first 
> successful training. In Extended mode, PCON FW trains for all BW set by 
> source in BW mask.
> 
> For Concurrent and Extended mode we need to set some extra bits in PCON 
> FRL config DPCDs
> 
> The intention was to go with sequential and Normal training mode, so no 
> need to set above bits.
> 
> Do you think, some documentation will make this clear?

I'm asking why does the code do

#define PCON_EXTENDED_TRAIN_MODE true
#define PCON_CONCURRENT_MODE true
#define PCON_SEQUENTIAL_MODE false
#define PCON_NORMAL_TRAIN_MODE false

but in a very convoluted way?
Nautiyal, Ankit K Feb. 2, 2021, 8:11 a.m. UTC | #4 
On 2/2/2021 12:17 PM, Ville Syrjälä wrote:
> On Tue, Feb 02, 2021 at 12:09:47PM +0530, Nautiyal, Ankit K wrote:
>> Hi Ville,
>>
>> Please find my responses inline.
>>
>> On 2/2/2021 2:08 AM, Ville Syrjälä wrote:
>>> On Fri, Dec 18, 2020 at 04:07:17PM +0530, Ankit Nautiyal wrote:
>>>> This patch adds functions to start FRL training for an HDMI2.1 sink,
>>>> connected via a PCON as a DP branch device.
>>>> This patch also adds a new structure for storing frl training related
>>>> data, when FRL training is completed.
>>>>
>>>> v2: As suggested by Uma Shankar:
>>>> -renamed couple of variables for better clarity
>>>> -tweaked the macros used for correct semantics for true/false
>>>> -fixed other styling issues.
>>>>
>>>> v3: Completed the TODO for condition for going to FRL mode.
>>>> Modified the condition to determine the required FRL b/w
>>>> based only on the Pcon and Sink's max FRL values.
>>>> Moved the frl structure initialization to intel_dp_init_connector().
>>>>
>>>> v4: Fixed typo in initialization of frl structure.
>>>>
>>>> v5: Always use FRL if its possible, instead of enabling only for
>>>> higher modes as done in v3.
>>>>
>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>> Reviewed-by: Uma Shankar <uma.shankar@intel.com> (v2)
>>>> ---
>>>>    .../drm/i915/display/intel_display_types.h    |   7 +
>>>>    drivers/gpu/drm/i915/display/intel_dp.c       | 151 ++++++++++++++++++
>>>>    drivers/gpu/drm/i915/display/intel_dp.h       |   2 +
>>>>    3 files changed, 160 insertions(+)
>>> <snip>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> index 0596d6c24e73..43027a6d5e5e 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>>> @@ -3891,6 +3891,8 @@ static void intel_disable_dp(struct intel_atomic_state *state,
>>>>    	intel_edp_backlight_off(old_conn_state);
>>>>    	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
>>>>    	intel_edp_panel_off(intel_dp);
>>>> +	intel_dp->frl.is_trained = false;
>>>> +	intel_dp->frl.trained_rate_gbps = 0;
>>> This stuff looks rather misplaced (or missing from elsewhere). This code
>>> doesn't even get executed on modern platforms.
>> I think these two lines should have been added to
>> intel_ddi_post_disable_dp() for TGL+
>>
>> My intention was to reset these before disabling DP. In hindsight, since
>> we are initializing (resetting) these in dp_init_connector, this doesnt
>> seem to be required.
>>
>> I will send a patch to remove these two lines from here.
>>
>>
>>> <snip>
>>>> +static int intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)
>>>> +{
>>>> +#define PCON_EXTENDED_TRAIN_MODE (1 > 0)
>>>> +#define PCON_CONCURRENT_MODE (1 > 0)
>>>> +#define PCON_SEQUENTIAL_MODE !PCON_CONCURRENT_MODE
>>>> +#define PCON_NORMAL_TRAIN_MODE !PCON_EXTENDED_TRAIN_MODE
>>> All of that looks like nonsense. What is it supposed to do?
>> When asking an HDMI2.1 PCON to initiate FRL training there are 2 options:
>>
>> Sequential/Concurrent mode: Sequential mode attempts the FRL training
>> after DP Link training is completed. Concurrent mode tries to do the FRL
>> training, during DP link training.
>>
>> Normal train Mode/ Extended mode: Normal train mode, PCON FW trains FRL
>> from Max to min BW, set by source in BW Mask. It aborts on first
>> successful training. In Extended mode, PCON FW trains for all BW set by
>> source in BW mask.
>>
>> For Concurrent and Extended mode we need to set some extra bits in PCON
>> FRL config DPCDs
>>
>> The intention was to go with sequential and Normal training mode, so no
>> need to set above bits.
>>
>> Do you think, some documentation will make this clear?
> I'm asking why does the code do
>
> #define PCON_EXTENDED_TRAIN_MODE true
> #define PCON_CONCURRENT_MODE true
> #define PCON_SEQUENTIAL_MODE false
> #define PCON_NORMAL_TRAIN_MODE false
>
> but in a very convoluted way?

Yes this doesnt seem to be very intuitive, I had earlier used simpler in 
initial versions.

As discussed offline, I will avoid all this and perhaps add enums for 
this in drm_dp_helper.

Thanks & Regards,

Ankit
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index c88d2b918d9f..daecff9783ea 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1339,6 +1339,11 @@  struct intel_dp_compliance {
 	u8 test_lane_count;
 };
 
+struct intel_dp_pcon_frl {
+	bool is_trained;
+	int trained_rate_gbps;
+};
+
 struct intel_dp {
 	i915_reg_t output_reg;
 	u32 DP;
@@ -1461,6 +1466,8 @@  struct intel_dp {
 
 	bool hobl_failed;
 	bool hobl_active;
+
+	struct intel_dp_pcon_frl frl;
 };
 
 enum lspcon_vendor {
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 0596d6c24e73..43027a6d5e5e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3891,6 +3891,8 @@  static void intel_disable_dp(struct intel_atomic_state *state,
 	intel_edp_backlight_off(old_conn_state);
 	intel_dp_set_power(intel_dp, DP_SET_POWER_D3);
 	intel_edp_panel_off(intel_dp);
+	intel_dp->frl.is_trained = false;
+	intel_dp->frl.trained_rate_gbps = 0;
 }
 
 static void g4x_disable_dp(struct intel_atomic_state *state,
@@ -3986,6 +3988,152 @@  cpt_set_link_train(struct intel_dp *intel_dp,
 	intel_de_posting_read(dev_priv, intel_dp->output_reg);
 }
 
+static int intel_dp_pcon_get_frl_mask(u8 frl_bw_mask)
+{
+	int bw_gbps[] = {9, 18, 24, 32, 40, 48};
+	int i;
+
+	for (i = ARRAY_SIZE(bw_gbps) - 1; i >= 0; i--) {
+		if (frl_bw_mask & (1 << i))
+			return bw_gbps[i];
+	}
+	return 0;
+}
+
+static int intel_dp_pcon_set_frl_mask(int max_frl)
+{
+
+	switch (max_frl) {
+	case 48:
+		return DP_PCON_FRL_BW_MASK_48GBPS;
+	case 40:
+		return DP_PCON_FRL_BW_MASK_40GBPS;
+	case 32:
+		return DP_PCON_FRL_BW_MASK_32GBPS;
+	case 24:
+		return DP_PCON_FRL_BW_MASK_24GBPS;
+	case 18:
+		return DP_PCON_FRL_BW_MASK_18GBPS;
+	case 9:
+		return DP_PCON_FRL_BW_MASK_9GBPS;
+	}
+
+	return 0;
+}
+
+static int intel_dp_hdmi_sink_max_frl(struct intel_dp *intel_dp)
+{
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
+
+	return (connector->display_info.hdmi.max_frl_rate_per_lane *
+		connector->display_info.hdmi.max_lanes);
+}
+
+static int intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)
+{
+#define PCON_EXTENDED_TRAIN_MODE (1 > 0)
+#define PCON_CONCURRENT_MODE (1 > 0)
+#define PCON_SEQUENTIAL_MODE !PCON_CONCURRENT_MODE
+#define PCON_NORMAL_TRAIN_MODE !PCON_EXTENDED_TRAIN_MODE
+#define TIMEOUT_FRL_READY_MS 500
+#define TIMEOUT_HDMI_LINK_ACTIVE_MS 1000
+
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	int max_frl_bw, max_pcon_frl_bw, max_edid_frl_bw, ret;
+	u8 max_frl_bw_mask = 0, frl_trained_mask;
+	bool is_active;
+
+	ret = drm_dp_pcon_reset_frl_config(&intel_dp->aux);
+	if (ret < 0)
+		return ret;
+
+	max_pcon_frl_bw = intel_dp->dfp.pcon_max_frl_bw;
+	drm_dbg(&i915->drm, "PCON max rate = %d Gbps\n", max_pcon_frl_bw);
+
+	max_edid_frl_bw = intel_dp_hdmi_sink_max_frl(intel_dp);
+	drm_dbg(&i915->drm, "Sink max rate from EDID = %d Gbps\n", max_edid_frl_bw);
+
+	max_frl_bw = min(max_edid_frl_bw, max_pcon_frl_bw);
+
+	if (max_frl_bw <= 0)
+		return -EINVAL;
+
+	ret = drm_dp_pcon_frl_prepare(&intel_dp->aux, false);
+	if (ret < 0)
+		return ret;
+	/* Wait for PCON to be FRL Ready */
+	wait_for(is_active = drm_dp_pcon_is_frl_ready(&intel_dp->aux) == true, TIMEOUT_FRL_READY_MS);
+
+	if (!is_active)
+		return -ETIMEDOUT;
+
+	max_frl_bw_mask = intel_dp_pcon_set_frl_mask(max_frl_bw);
+	ret = drm_dp_pcon_frl_configure_1(&intel_dp->aux, max_frl_bw, PCON_SEQUENTIAL_MODE);
+	if (ret < 0)
+		return ret;
+	ret = drm_dp_pcon_frl_configure_2(&intel_dp->aux, max_frl_bw_mask, PCON_NORMAL_TRAIN_MODE);
+	if (ret < 0)
+		return ret;
+	ret = drm_dp_pcon_frl_enable(&intel_dp->aux);
+	if (ret < 0)
+		return ret;
+	/*
+	 * Wait for FRL to be completed
+	 * Check if the HDMI Link is up and active.
+	 */
+	wait_for(is_active = drm_dp_pcon_hdmi_link_active(&intel_dp->aux) == true, TIMEOUT_HDMI_LINK_ACTIVE_MS);
+
+	if (!is_active)
+		return -ETIMEDOUT;
+
+	/* Verify HDMI Link configuration shows FRL Mode */
+	if (DP_PCON_HDMI_MODE_FRL != drm_dp_pcon_hdmi_link_mode(&intel_dp->aux, &frl_trained_mask)) {
+		drm_dbg(&i915->drm, "HDMI couldn't be trained in FRL Mode\n");
+		return -EINVAL;
+	}
+	drm_dbg(&i915->drm, "MAX_FRL_MASK = %u, FRL_TRAINED_MASK = %u\n", max_frl_bw_mask, frl_trained_mask);
+
+	intel_dp->frl.trained_rate_gbps = intel_dp_pcon_get_frl_mask(frl_trained_mask);
+	intel_dp->frl.is_trained = true;
+	drm_dbg(&i915->drm, "FRL trained with : %d Gbps\n", intel_dp->frl.trained_rate_gbps);
+
+	return 0;
+}
+
+static bool intel_dp_is_hdmi_2_1_sink(struct intel_dp *intel_dp)
+{
+	if (drm_dp_is_branch(intel_dp->dpcd) &&
+	    intel_dp->has_hdmi_sink &&
+	    intel_dp_hdmi_sink_max_frl(intel_dp) > 0)
+		return true;
+
+	return false;
+}
+
+void intel_dp_check_frl_training(struct intel_dp *intel_dp)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+
+	/* Always go for FRL training if supported */
+	if (!intel_dp_is_hdmi_2_1_sink(intel_dp) ||
+	    intel_dp->frl.is_trained)
+		return;
+
+	if (intel_dp_pcon_start_frl_training(intel_dp) < 0) {
+		int ret, mode;
+
+		drm_dbg(&dev_priv->drm, "Couldnt set FRL mode, continuing with TMDS mode\n");
+		ret = drm_dp_pcon_reset_frl_config(&intel_dp->aux);
+		mode = drm_dp_pcon_hdmi_link_mode(&intel_dp->aux, NULL);
+
+		if (ret < 0 || mode != DP_PCON_HDMI_MODE_TMDS)
+			drm_dbg(&dev_priv->drm, "Issue with PCON, cannot set TMDS mode\n");
+	} else {
+		drm_dbg(&dev_priv->drm, "FRL training Completed\n");
+	}
+}
+
 static void
 g4x_set_link_train(struct intel_dp *intel_dp,
 		   const struct intel_crtc_state *crtc_state,
@@ -8210,6 +8358,9 @@  intel_dp_init_connector(struct intel_digital_port *dig_port,
 			       (temp & ~0xf) | 0xd);
 	}
 
+	intel_dp->frl.is_trained = false;
+	intel_dp->frl.trained_rate_gbps = 0;
+
 	return true;
 
 fail:
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index b871a09b6901..58b76a20459c 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -144,4 +144,6 @@  bool intel_dp_initial_fastset_check(struct intel_encoder *encoder,
 void intel_dp_sync_state(struct intel_encoder *encoder,
 			 const struct intel_crtc_state *crtc_state);
 
+void intel_dp_check_frl_training(struct intel_dp *intel_dp);
+
 #endif /* __INTEL_DP_H__ */