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@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?
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
