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

[Nautiyal, Ankit K]

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 at intel.com>
>>>> Reviewed-by: Uma Shankar <uma.shankar at 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