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?