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@xxxxxxxxx>
> >> Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx> (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?
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
