[PATCH 1/3] drm/dp: DRM DP helper for reading Ignore MSA from DPCD

[PATCH 1/3] drm/dp: DRM DP helper for reading Ignore MSA from DPCD

[Manasi Navare]

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])
+{
+	return dpcd[DP_DOWN_STREAM_PORT_COUNT] &
+		DP_MSA_TIMING_PAR_IGNORED;
+}
+
 /*
  * DisplayPort AUX channel
  */
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: Create a drm_connector_helper_funcs hook for Adaptive Sync support

[Manasi Navare]

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.

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);
+
 	/*
 	 * 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);
 };
 
 /**
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Create a drm_connector_helper_funcs hook for Adaptive Sync support

[Harry Wentland]

On 2020-03-18 2:35 a.m., Manasi Navare 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.
> 
> 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>

Patches 1 and 2 are
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  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);
> +
>  	/*
>  	 * 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);
>  };
>  
>  /**
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Create a drm_connector_helper_funcs hook for Adaptive Sync support

[Jani Nikula]

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);
>  };
>  
>  /**

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Create a drm_connector_helper_funcs hook for Adaptive Sync support

[Manasi Navare]

On Thu, Mar 19, 2020 at 12:07:37PM +0200, Jani Nikula wrote:
> 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().
>

This needs to be called after get_modes that ends up populating the
monitor range after parsing the EDID.

I could have just modified the get_modes hook in the driver to set the
vrr capabilities but that doesnt go with the definition of get_modes() hook
which is purely to obtain modes from edid.

So i added a separate hook which will always return the vrr capabilities if this
hook exists in the driver everytime a connector probe happens.

Would you suggest this elsewhere or some other design?
I am open to better placement of this hook or reusing some other hook or
other suggestions for getting the vrr capabilities in the driver

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>
> > 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);
> >  };
> >  
> >  /**
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/i915/dp: intel_dp connector hook for VRR support

[Manasi Navare]

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

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;
 };
 
 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;
+}
+
 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 = {
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/i915/dp: intel_dp connector hook for VRR support

[Jani Nikula]

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?

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

Re: [PATCH 3/3] drm/i915/dp: intel_dp connector hook for VRR support

[Manasi Navare]

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

Re: [PATCH 1/3] drm/dp: DRM DP helper for reading Ignore MSA from DPCD

[Jani Nikula]

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*?

BR,
Jani.


> +{
> +	return dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> +		DP_MSA_TIMING_PAR_IGNORED;
> +}
> +
>  /*
>   * DisplayPort AUX channel
>   */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/dp: DRM DP helper for reading Ignore MSA from DPCD

[Manasi Navare]

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