| Message ID | 20200626232641.4557-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status | expand |
@Ville @Imre, addressed your review comments, could you take a look? Manasi On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > Modify the helper to add a fixed delay or poll with timeout > based on platform specification to check for either Idle bit > set (DDI_BUF_CTL is idle for disable case) > > v3: > * Change the timeout to 16usecs (Ville) > v2: > * Use 2 separate functions or idle and active (Ville) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 884b507c5f55..052a74625a61 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > enum port port) > { > - i915_reg_t reg = DDI_BUF_CTL(port); > - int i; > - > - for (i = 0; i < 16; i++) { > - udelay(1); > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > - return; > + if (IS_BROXTON(dev_priv)) { > + udelay(16); > + return; > } > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > - port_name(port)); > + > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > + DDI_BUF_IS_IDLE), 16)) > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > + port_name(port)); > } > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > -- > 2.19.1 >
On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > Modify the helper to add a fixed delay or poll with timeout > based on platform specification to check for either Idle bit > set (DDI_BUF_CTL is idle for disable case) > > v3: > * Change the timeout to 16usecs (Ville) > v2: > * Use 2 separate functions or idle and active (Ville) Missing changelog? Did somehting change? > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 884b507c5f55..052a74625a61 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > enum port port) > { > - i915_reg_t reg = DDI_BUF_CTL(port); > - int i; > - > - for (i = 0; i < 16; i++) { > - udelay(1); > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > - return; > + if (IS_BROXTON(dev_priv)) { > + udelay(16); > + return; > } > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > - port_name(port)); > + > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > + DDI_BUF_IS_IDLE), 16)) > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > + port_name(port)); > } > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > -- > 2.19.1
On Tue, Jun 30, 2020 at 06:30:16PM +0300, Ville Syrjälä wrote: > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > > Modify the helper to add a fixed delay or poll with timeout > > based on platform specification to check for either Idle bit > > set (DDI_BUF_CTL is idle for disable case) > > > > v3: > > * Change the timeout to 16usecs (Ville) > > v2: > > * Use 2 separate functions or idle and active (Ville) > > Missing changelog? Did somehting change? > No its a v3 for thsi patch, but patch 2/2 is a v4 so published both patches with a v4, does this now look good with v3, timeout changed to 16 usecs? Manasi > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 884b507c5f55..052a74625a61 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > > enum port port) > > { > > - i915_reg_t reg = DDI_BUF_CTL(port); > > - int i; > > - > > - for (i = 0; i < 16; i++) { > > - udelay(1); > > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > > - return; > > + if (IS_BROXTON(dev_priv)) { > > + udelay(16); > > + return; > > } > > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > > - port_name(port)); > > + > > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > > + DDI_BUF_IS_IDLE), 16)) > > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > > + port_name(port)); > > } > > > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > > -- > > 2.19.1 > > -- > Ville Syrjälä > Intel
On Tue, Jun 30, 2020 at 01:49:22PM -0700, Manasi Navare wrote: > On Tue, Jun 30, 2020 at 06:30:16PM +0300, Ville Syrjälä wrote: > > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > > > Modify the helper to add a fixed delay or poll with timeout > > > based on platform specification to check for either Idle bit > > > set (DDI_BUF_CTL is idle for disable case) > > > > > > v3: > > > * Change the timeout to 16usecs (Ville) > > > v2: > > > * Use 2 separate functions or idle and active (Ville) > > > > Missing changelog? Did somehting change? > > > > No its a v3 for thsi patch, but patch 2/2 is a v4 so published > both patches with a v4, Doh. The changelog is backwards so didn't even notice. > > does this now look good with v3, timeout changed to 16 usecs? > > Manasi > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index 884b507c5f55..052a74625a61 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > > > enum port port) > > > { > > > - i915_reg_t reg = DDI_BUF_CTL(port); > > > - int i; > > > - > > > - for (i = 0; i < 16; i++) { > > > - udelay(1); > > > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > > > - return; > > > + if (IS_BROXTON(dev_priv)) { > > > + udelay(16); > > > + return; > > > } > > > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > > > - port_name(port)); > > > + > > > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > > > + DDI_BUF_IS_IDLE), 16)) > > > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > > > + port_name(port)); > > > } > > > > > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > > > -- > > > 2.19.1 > > > > -- > > Ville Syrjälä > > Intel
On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > Modify the helper to add a fixed delay or poll with timeout > based on platform specification to check for either Idle bit > set (DDI_BUF_CTL is idle for disable case) > > v3: > * Change the timeout to 16usecs (Ville) > v2: > * Use 2 separate functions or idle and active (Ville) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 884b507c5f55..052a74625a61 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > enum port port) > { > - i915_reg_t reg = DDI_BUF_CTL(port); > - int i; > - > - for (i = 0; i < 16; i++) { > - udelay(1); > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > - return; > + if (IS_BROXTON(dev_priv)) { > + udelay(16); > + return; > } > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > - port_name(port)); > + > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > + DDI_BUF_IS_IDLE), 16)) 16 is the BXT number. IIRC the spec said 8 usec for the other platforms. > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > + port_name(port)); > } > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > -- > 2.19.1
On Wed, Jul 01, 2020 at 12:03:30AM +0300, Ville Syrjälä wrote: > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > > Modify the helper to add a fixed delay or poll with timeout > > based on platform specification to check for either Idle bit > > set (DDI_BUF_CTL is idle for disable case) > > > > v3: > > * Change the timeout to 16usecs (Ville) > > v2: > > * Use 2 separate functions or idle and active (Ville) > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 884b507c5f55..052a74625a61 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > > enum port port) > > { > > - i915_reg_t reg = DDI_BUF_CTL(port); > > - int i; > > - > > - for (i = 0; i < 16; i++) { > > - udelay(1); > > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > > - return; > > + if (IS_BROXTON(dev_priv)) { > > + udelay(16); > > + return; > > } > > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > > - port_name(port)); > > + > > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > > + DDI_BUF_IS_IDLE), 16)) > > 16 is the BXT number. IIRC the spec said 8 usec for the other platforms. > Yes I see for HSW atleast yes it says 8usecs but i left it at 16 since thats what we always had and the only change was that BXT add a fixed delay But if you prefer i will change it to 8us timeout? Manasi > > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > > + port_name(port)); > > } > > > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > > -- > > 2.19.1 > > -- > Ville Syrjälä > Intel
On Tue, Jun 30, 2020 at 02:10:45PM -0700, Manasi Navare wrote: > On Wed, Jul 01, 2020 at 12:03:30AM +0300, Ville Syrjälä wrote: > > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote: > > > Modify the helper to add a fixed delay or poll with timeout > > > based on platform specification to check for either Idle bit > > > set (DDI_BUF_CTL is idle for disable case) > > > > > > v3: > > > * Change the timeout to 16usecs (Ville) > > > v2: > > > * Use 2 separate functions or idle and active (Ville) > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- > > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > > index 884b507c5f55..052a74625a61 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > > @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > > static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > > > enum port port) > > > { > > > - i915_reg_t reg = DDI_BUF_CTL(port); > > > - int i; > > > - > > > - for (i = 0; i < 16; i++) { > > > - udelay(1); > > > - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) > > > - return; > > > + if (IS_BROXTON(dev_priv)) { > > > + udelay(16); > > > + return; > > > } > > > - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", > > > - port_name(port)); > > > + > > > + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & > > > + DDI_BUF_IS_IDLE), 16)) > > > > 16 is the BXT number. IIRC the spec said 8 usec for the other platforms. > > > > Yes I see for HSW atleast yes it says 8usecs but i left it at 16 since thats > what we always had and the only change was that BXT add a fixed delay > But if you prefer i will change it to 8us timeout? My usual approach is to a) just use the spec value, b) if there's a sane reason for not using it then include a comment documenting the spec value. Often b) is just for "spec say a few microseconds, let's just wait a full millisecond to make it simple", or for "old platforms want timeout N, new ones want M, just go with the larger of the two for simplicity". Arguably the current code was trying to follow the latter approach, except if was supposed to since bxt wasn't supposed to poll at all. Anyways, since the current code already used 16 usec without any clarification I guess this is no worse than what we had. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Manasi > > > > + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", > > > + port_name(port)); > > > } > > > > > > static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll) > > > -- > > > 2.19.1 > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 884b507c5f55..052a74625a61 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -1184,16 +1184,15 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, enum port port) { - i915_reg_t reg = DDI_BUF_CTL(port); - int i; - - for (i = 0; i < 16; i++) { - udelay(1); - if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE) - return; + if (IS_BROXTON(dev_priv)) { + udelay(16); + return; } - drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n", - port_name(port)); + + if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) & + DDI_BUF_IS_IDLE), 16)) + drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n", + port_name(port)); } static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
Modify the helper to add a fixed delay or poll with timeout based on platform specification to check for either Idle bit set (DDI_BUF_CTL is idle for disable case) v3: * Change the timeout to 16usecs (Ville) v2: * Use 2 separate functions or idle and active (Ville) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)