| Message ID | 20240923031007.1058072-3-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Reduce SHPD_FILTER_CNT value | expand |
On Mon, Sep 23, 2024 at 08:40:07AM +0530, Suraj Kandpal wrote: > Reduce SHPD_CNT to 250us for display version 12 as it lines up > with DP1.4a(Table3-4) spec. > > --v2 > -Update commit message and comment [Matt] > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_hotplug_irq.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > index 2c4e946d5575..8427386132e6 100644 > --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > @@ -849,7 +849,11 @@ static void icp_hpd_irq_setup(struct drm_i915_private *dev_priv) > enabled_irqs = intel_hpd_enabled_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > hotplug_irqs = intel_hpd_hotplug_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > - if (INTEL_PCH_TYPE(dev_priv) <= PCH_TGP) > + /* > + * We reduce the value to 250us to be able to detect SHPD when an external display > + * is connected. This is also expected of us as stated in DP1.4a Table 3-4. > + */ > + if (INTEL_PCH_TYPE(dev_priv) < PCH_TGP) > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_500_ADJ); > else > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_250); IMO if we start reducing this for older platforms then we should just do it for all of them, instead of based on some random cutoff.
On Mon, Sep 23, 2024 at 08:38:35PM +0300, Ville Syrjälä wrote: > On Mon, Sep 23, 2024 at 08:40:07AM +0530, Suraj Kandpal wrote: > > Reduce SHPD_CNT to 250us for display version 12 as it lines up > > with DP1.4a(Table3-4) spec. > > > > --v2 > > -Update commit message and comment [Matt] > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hotplug_irq.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > index 2c4e946d5575..8427386132e6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > @@ -849,7 +849,11 @@ static void icp_hpd_irq_setup(struct drm_i915_private *dev_priv) > > enabled_irqs = intel_hpd_enabled_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > hotplug_irqs = intel_hpd_hotplug_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > > > - if (INTEL_PCH_TYPE(dev_priv) <= PCH_TGP) > > + /* > > + * We reduce the value to 250us to be able to detect SHPD when an external display > > + * is connected. This is also expected of us as stated in DP1.4a Table 3-4. > > + */ > > + if (INTEL_PCH_TYPE(dev_priv) < PCH_TGP) > > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_500_ADJ); > > else > > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_250); > > IMO if we start reducing this for older platforms then we > should just do it for all of them, instead of based on some > random cutoff. The other things we might want to think about are moving this into *_hpd_detection_setup() instead, and perhaps we should also start explicitly configuring SHPD_PULSE_CNT as well?
On Mon, Sep 23, 2024 at 08:38:35PM +0300, Ville Syrjälä wrote: > On Mon, Sep 23, 2024 at 08:40:07AM +0530, Suraj Kandpal wrote: > > Reduce SHPD_CNT to 250us for display version 12 as it lines up > > with DP1.4a(Table3-4) spec. > > > > --v2 > > -Update commit message and comment [Matt] > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hotplug_irq.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > index 2c4e946d5575..8427386132e6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > @@ -849,7 +849,11 @@ static void icp_hpd_irq_setup(struct drm_i915_private *dev_priv) > > enabled_irqs = intel_hpd_enabled_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > hotplug_irqs = intel_hpd_hotplug_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > > > - if (INTEL_PCH_TYPE(dev_priv) <= PCH_TGP) > > + /* > > + * We reduce the value to 250us to be able to detect SHPD when an external display > > + * is connected. This is also expected of us as stated in DP1.4a Table 3-4. > > + */ > > + if (INTEL_PCH_TYPE(dev_priv) < PCH_TGP) > > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_500_ADJ); > > else > > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_250); > > IMO if we start reducing this for older platforms then we > should just do it for all of them, instead of based on some > random cutoff. There is a note in the bspec (page 4342) that says "Program SHPD_FILTER_CNT with the "500 microseconds adjusted" value before enabling hotplug detection." for CNP and ICP which is where the cutoff originally came from. I'm not sure about CML (which uses CNP), but ICL at least supports DP1.4a, so I think the note in the bspec is probably just outdated and we'd still want 250 to ensure we align with the DP spec. So I'd suggest just dropping the condition in this function and using 250us here. We can probably leave things be in spt_hpd_irq_setup which gets used for CML/CNP. Matt > > -- > Ville Syrjälä > Intel
On Mon, Sep 23, 2024 at 11:09:19AM -0700, Matt Roper wrote: > On Mon, Sep 23, 2024 at 08:38:35PM +0300, Ville Syrjälä wrote: > > On Mon, Sep 23, 2024 at 08:40:07AM +0530, Suraj Kandpal wrote: > > > Reduce SHPD_CNT to 250us for display version 12 as it lines up > > > with DP1.4a(Table3-4) spec. > > > > > > --v2 > > > -Update commit message and comment [Matt] > > > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_hotplug_irq.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > > index 2c4e946d5575..8427386132e6 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c > > > @@ -849,7 +849,11 @@ static void icp_hpd_irq_setup(struct drm_i915_private *dev_priv) > > > enabled_irqs = intel_hpd_enabled_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > > hotplug_irqs = intel_hpd_hotplug_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); > > > > > > - if (INTEL_PCH_TYPE(dev_priv) <= PCH_TGP) > > > + /* > > > + * We reduce the value to 250us to be able to detect SHPD when an external display > > > + * is connected. This is also expected of us as stated in DP1.4a Table 3-4. > > > + */ > > > + if (INTEL_PCH_TYPE(dev_priv) < PCH_TGP) > > > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_500_ADJ); > > > else > > > intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_250); > > > > IMO if we start reducing this for older platforms then we > > should just do it for all of them, instead of based on some > > random cutoff. > > There is a note in the bspec (page 4342) that says > > "Program SHPD_FILTER_CNT with the "500 microseconds adjusted" > value before enabling hotplug detection." > > for CNP and ICP which is where the cutoff originally came from. I'm not > sure about CML (which uses CNP), but ICL at least supports DP1.4a, so I > think the note in the bspec is probably just outdated and we'd still > want 250 to ensure we align with the DP spec. The 250usec has been in the DP spec forever. DP 1.4a vs. older is a moot point. Also, this is about filtering out glitches generated by the sink, and anyone can plug any DP sink to any DP source. So if the 500us is a problem for TGP+ then it'll be a problem for LPT+ as well when using the same sink. For IBX/CPT it seems we can't adjust this to a more sensible value (the only choice seems to be 500us vs. 50us), so LPT+ is the hard cutoff unfortunately.
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c index 2c4e946d5575..8427386132e6 100644 --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c @@ -849,7 +849,11 @@ static void icp_hpd_irq_setup(struct drm_i915_private *dev_priv) enabled_irqs = intel_hpd_enabled_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); hotplug_irqs = intel_hpd_hotplug_irqs(dev_priv, dev_priv->display.hotplug.pch_hpd); - if (INTEL_PCH_TYPE(dev_priv) <= PCH_TGP) + /* + * We reduce the value to 250us to be able to detect SHPD when an external display + * is connected. This is also expected of us as stated in DP1.4a Table 3-4. + */ + if (INTEL_PCH_TYPE(dev_priv) < PCH_TGP) intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_500_ADJ); else intel_uncore_write(&dev_priv->uncore, SHPD_FILTER_CNT, SHPD_FILTER_CNT_250);
Reduce SHPD_CNT to 250us for display version 12 as it lines up with DP1.4a(Table3-4) spec. --v2 -Update commit message and comment [Matt] Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/intel_hotplug_irq.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)