diff mbox series

[1/2] drm/i915/hotplug: Reduce SHPD_FLITER_CNT for DISPLAY_VER() == 12

Message ID 20240923031007.1058072-3-suraj.kandpal@intel.com (mailing list archive)
State New, archived
Headers show
Series Reduce SHPD_FILTER_CNT value | expand

Commit Message

Kandpal, Suraj Sept. 23, 2024, 3:10 a.m. UTC
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(-)

Comments

Ville Syrjala Sept. 23, 2024, 5:38 p.m. UTC | #1
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.
Ville Syrjala Sept. 23, 2024, 5:53 p.m. UTC | #2
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?
Matt Roper Sept. 23, 2024, 6:09 p.m. UTC | #3
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
Ville Syrjala Sept. 23, 2024, 6:28 p.m. UTC | #4
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.
Kandpal, Suraj Sept. 24, 2024, 5:17 a.m. UTC | #5
> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@intel.com>
> Sent: Monday, September 23, 2024 11:39 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org;
> Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH 1/2] drm/i915/hotplug: Reduce SHPD_FLITER_CNT for
> DISPLAY_VER() == 12
> 
> 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.
> 

Sure will do that

Regards,
Suraj Kandpal
> 
> Matt
> 
> >
> > --
> > Ville Syrjälä
> > Intel
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
diff mbox series

Patch

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);