On Tue, 17 Mar 2020, Manasi Navare manasi.d.navare@intel.com wrote:

This patch adds a hook in drm_connector_helper_funcs to get the support of the driver for adaptive sync functionality.

This can be called in the connector probe helper function after the connector detect() and get_modes() hooks to also query the adaptive sync support of the driver.

I can obviously see that from the patch. But this does not explain at all *why* we need another hook to begin with, and why it neeeds to be called from ->fill_modes that is set to drm_helper_probe_single_connector_modes().

Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland harry.wentland@amd.com Cc: Nicholas Kazlauskas Nicholas.Kazlauskas@amd.com Signed-off-by: Manasi Navare manasi.d.navare@intel.com

---

drivers/gpu/drm/drm_probe_helper.c | 4 ++++ include/drm/drm_modeset_helper_vtables.h | 16 ++++++++++++++++ 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 576b4b7dcd89..4403817bfb02 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -482,6 +482,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,

count = (*connector_funcs->get_modes)(connector);

• /* Get the Adaptive Sync Support if helper exists */
• if (*connector_funcs->get_adaptive_sync_support)

• (**connector_funcs->get_adaptive_sync_support)(connector);
  

This is in the middle of a sequence figuring out the modes. First ->get_modes, then fallback to other mechanisms. Certainly we don't want to do something else in the middle.

/* * Fallback for when DDC probe failed in drm_get_edid() and thus skipped * override/firmware EDID. diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 7c20b1c8b6a7..0b203fdd25df 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1079,6 +1079,22 @@ struct drm_connector_helper_funcs { struct drm_writeback_job *job); void (*cleanup_writeback_job)(struct drm_writeback_connector *connector, struct drm_writeback_job *job);

• /**

• * @get_adaptive_sync_support:
  

• *
  

• * This hook is used by the probe helper to get the driver's support
  

• * for adaptive sync or variable refresh rate.
  

• * This is called from drm_helper_probe_single_connector_modes()
  

• * This is called after the @get_modes hook so that the connector modes
  

• * are already obtained and EDID is parsed to obtain the monitor
  

• * range descriptor information.
  

• *
  

• * This hook is optional and defined only for the drivers and on
  

• * connectors that advertise adaptive sync support.
  

• *
  

• */
  

• void (*get_adaptive_sync_support)(struct drm_connector *connector);

};

/**

On Thu, Mar 19, 2020 at 12:14:28PM +0200, Jani Nikula wrote:

On Tue, 17 Mar 2020, Manasi Navare manasi.d.navare@intel.com wrote:

...

This defines the get_vrr_support hook for intel DP connector VRR support is set to true based on the DPCD ignore MSA and EDID monitor range

Yeah... but what do you use it for?

Hi Jani,

My idea of adding the intel_dp->vrr_capable variable was to store the vrr capability in intel_dp for internal i915 use later when we decide on vrr crtc states etc and configure the pipe etc for VRR modes.

I added this in a hook that gets called in the connector probe function right after detect and get_modes() since thats when we will have parse the EDID monitor range and populated that in drm_display_info struct.

This hook is also needed for us to then set the vrr capable property for that connector.

But yes no that I rethink on of i actually need something in intel_dp I feel that I can just get away with crtc_state->vrr_capable that will be computed in atomic check based on the drm_display_info and dpcd read there and in this hook we can just set the vrr capable property.

But that would mean duplicating this conditional code in atomic check. What would be your suggestion? Cache it here in intel_dp and just use this to set crtc_state->VRR values in modeset or set it directly in atomic check?

In terms of reseting it, I can set this to 0 in intel_dp_detect(), if connector disconnected path where i reset the dsc_dpcd and dp_compliance variables?

Regards Manasi

...

Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland harry.wentland@amd.com Cc: Nicholas Kazlauskas Nicholas.Kazlauskas@amd.com Cc: Aditya Swarup aditya.swarup@intel.com Signed-off-by: Manasi Navare manasi.d.navare@intel.com

---

.../drm/i915/display/intel_display_types.h | 3 +++ drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 5e00e611f077..cd37ee6db1ff 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1353,6 +1353,9 @@ struct intel_dp {

/* Display stream compression testing */ bool force_dsc_en;

• /* DP Variable refresh rate/ Adaptive sync support */
• bool vrr_capable;

Only set, never read.

...

};

enum lspcon_vendor { diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 0a417cd2af2b..ccf5d868b5c1 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5860,6 +5860,24 @@ static int intel_dp_get_modes(struct drm_connector *connector) return 0; }

+static void intel_dp_get_vrr_support(struct drm_connector *connector) +{

• struct intel_dp *intel_dp = intel_attached_dp(to_intel_connector(connector));
• const struct drm_display_info *info = &connector->display_info;
• struct drm_i915_private *dev_priv = to_i915(connector->dev);
• /*

• * DP Sink is capable of Variable refresh video timings if
  

• * Ignore MSA bit is set in DPCD.
  

• * EDID monitor range also should be atleast 10 for reasonable
  

• * Adaptive sync/ VRR end user experience.
  

• */
  

• if (INTEL_GEN(dev_priv) >= 12 &&

•    drm_dp_sink_is_capable_without_timing_msa(intel_dp->dpcd) &&
  

•    info->monitor_range.max_vfreq - info->monitor_range.min_vfreq > 10)
  

• intel_dp->vrr_capable = true;
  

So for now this is just a cached value for i915 use. I don't know what you'll need it for, but you also don't explain why it needs to be *cached* instead of having a helper to tell you based on the above data. You only ever set ->vrr_capable to true, but you never reset it back to false e.g. when the display is changed on the connector.

Furthermore, because of the placing of the hook call in the previous patch, this will only use whatever details ->get_modes gives you, not the fallback data.

BR, Jani.

...

+}

static int intel_dp_connector_register(struct drm_connector *connector) { @@ -6756,6 +6774,7 @@ static const struct drm_connector_helper_funcs intel_dp_connector_helper_funcs = .get_modes = intel_dp_get_modes, .mode_valid = intel_dp_mode_valid, .atomic_check = intel_dp_connector_atomic_check,

• .get_adaptive_sync_support = intel_dp_get_vrr_support,

};

static const struct drm_encoder_funcs intel_dp_enc_funcs = {

-- Jani Nikula, Intel Open Source Graphics Center

On Thu, Mar 19, 2020 at 11:59:38AM +0200, Jani Nikula wrote:

On Tue, 17 Mar 2020, Manasi Navare manasi.d.navare@intel.com wrote:

...

DP sink device sets the Ignore MSA bit in its DP_DOWNSTREAM_PORT_COUNT register to indicate its ability to ignore the MSA video timing paramaters and its ability to support seamless video timing change over a range of timing exposed by DisplayID and EDID. This is required for the sink to indicate that it is Adaptive sync capable.

Cc: Jani Nikula jani.nikula@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Harry Wentland harry.wentland@amd.com Cc: Nicholas Kazlauskas Nicholas.Kazlauskas@amd.com Signed-off-by: Manasi Navare manasi.d.navare@intel.com

---

include/drm/drm_dp_helper.h | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index c6119e4c169a..ccd6e2e988b9 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1315,6 +1315,14 @@ drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) DP_ALTERNATE_SCRAMBLER_RESET_CAP; }

+/* Ignore MSA timing for Adaptive Sync support on DP 1.4 */ +static inline bool +drm_dp_sink_is_capable_without_timing_msa(const u8 dpcd[DP_RECEIVER_CAP_SIZE])

From the department of nitpicks, if you read the name of the function aloud, what does it actually mean?

Is sink capable of *what*?

As per the DP 1.4 spec, it says this indicates sink's ability to ignore MSA video timing parameters to support seamless video timing change over range of timing exposed in DisplayID and legacy EDID. This query should occur before enabling dynamic video timing change of incoming video stream without valid MSA video timing params.

May be i rename it as: drm_dp_sink_capable_video_without_timing_msa() ?

Manasi

BR, Jani.

...

+{

• return dpcd[DP_DOWN_STREAM_PORT_COUNT] &

• DP_MSA_TIMING_PAR_IGNORED;
  

+}

/*

• DisplayPort AUX channel

*/

-- Jani Nikula, Intel Open Source Graphics Center