diff mbox series

[v3,1/3] drm/i915/display: Program PIPE_MBUS_DBOX_CTL with adl-p values

Message ID 20220328191617.122838-1-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/3] drm/i915/display: Program PIPE_MBUS_DBOX_CTL with adl-p values | expand

Commit Message

Souza, Jose March 28, 2022, 7:16 p.m. UTC 
From: Caz Yokoyama <caz.yokoyama@intel.com>

B credits set by IFWI do not match with specification default, so here
programming the right value.

Also while at it, taking the oportunity to do a read-modify-write to
not overwrite all other bits in this register that specification don't
ask us to change.

BSpec: 49213
BSpec: 50343
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Sripada, Radhakrishna March 29, 2022, 5:59 p.m. UTC | #1 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of José
> Roberto de Souza
> Sent: Tuesday, March 29, 2022 12:46 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>
> Subject: [Intel-gfx] [PATCH v3 1/3] drm/i915/display: Program
> PIPE_MBUS_DBOX_CTL with adl-p values
> 
> From: Caz Yokoyama <caz.yokoyama@intel.com>
> 
> B credits set by IFWI do not match with specification default, so here
> programming the right value.
> 
> Also while at it, taking the oportunity to do a read-modify-write to
> not overwrite all other bits in this register that specification don't
> ask us to change.
> 
> BSpec: 49213
> BSpec: 50343
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>


> Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 3d2ff258f0a94..078ada041e1cd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1830,13 +1830,19 @@ static void icl_pipe_mbus_enable(struct intel_crtc
> *crtc, bool joined_mbus)
>  	enum pipe pipe = crtc->pipe;
>  	u32 val;
> 
> +	val = intel_de_read(dev_priv, PIPE_MBUS_DBOX_CTL(pipe));
> +	val &= ~MBUS_DBOX_A_CREDIT_MASK;
>  	/* Wa_22010947358:adl-p */
>  	if (IS_ALDERLAKE_P(dev_priv))
> -		val = joined_mbus ? MBUS_DBOX_A_CREDIT(6) :
> MBUS_DBOX_A_CREDIT(4);
> +		val |= joined_mbus ? MBUS_DBOX_A_CREDIT(6) :
> MBUS_DBOX_A_CREDIT(4);
>  	else
> -		val = MBUS_DBOX_A_CREDIT(2);
> +		val |= MBUS_DBOX_A_CREDIT(2);
> 
> -	if (DISPLAY_VER(dev_priv) >= 12) {
> +	val &= ~(MBUS_DBOX_BW_CREDIT_MASK |
> MBUS_DBOX_B_CREDIT_MASK);
> +	if (IS_ALDERLAKE_P(dev_priv)) {
> +		val |= MBUS_DBOX_BW_CREDIT(2);
> +		val |= MBUS_DBOX_B_CREDIT(8);
> +	} else if (DISPLAY_VER(dev_priv) >= 12) {
>  		val |= MBUS_DBOX_BW_CREDIT(2);
>  		val |= MBUS_DBOX_B_CREDIT(12);
>  	} else {
> --
> 2.35.1
Ville Syrjälä March 29, 2022, 6:14 p.m. UTC | #2 
On Mon, Mar 28, 2022 at 12:16:15PM -0700, José Roberto de Souza wrote:
> From: Caz Yokoyama <caz.yokoyama@intel.com>
> 
> B credits set by IFWI do not match with specification default, so here
> programming the right value.
> 
> Also while at it, taking the oportunity to do a read-modify-write to
> not overwrite all other bits in this register that specification don't
> ask us to change.

RMWs considered harmful. This is a double buffered register and in the
future we may have to program it via DSB to update it atomically with
the rest of the registers (eg. if we want to avoid the modeset for the
mbus joining change). And when that happens the RMW will have to be
removed again since the DSB can't even read registers. So IMO better
to not even start down this path.

> 
> BSpec: 49213
> BSpec: 50343
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3d2ff258f0a94..078ada041e1cd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1830,13 +1830,19 @@ static void icl_pipe_mbus_enable(struct intel_crtc *crtc, bool joined_mbus)
>  	enum pipe pipe = crtc->pipe;
>  	u32 val;
>  
> +	val = intel_de_read(dev_priv, PIPE_MBUS_DBOX_CTL(pipe));
> +	val &= ~MBUS_DBOX_A_CREDIT_MASK;
>  	/* Wa_22010947358:adl-p */
>  	if (IS_ALDERLAKE_P(dev_priv))
> -		val = joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
> +		val |= joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
>  	else
> -		val = MBUS_DBOX_A_CREDIT(2);
> +		val |= MBUS_DBOX_A_CREDIT(2);
>  
> -	if (DISPLAY_VER(dev_priv) >= 12) {
> +	val &= ~(MBUS_DBOX_BW_CREDIT_MASK | MBUS_DBOX_B_CREDIT_MASK);
> +	if (IS_ALDERLAKE_P(dev_priv)) {
> +		val |= MBUS_DBOX_BW_CREDIT(2);
> +		val |= MBUS_DBOX_B_CREDIT(8);
> +	} else if (DISPLAY_VER(dev_priv) >= 12) {
>  		val |= MBUS_DBOX_BW_CREDIT(2);
>  		val |= MBUS_DBOX_B_CREDIT(12);
>  	} else {
> -- 
> 2.35.1
Souza, Jose March 29, 2022, 7:20 p.m. UTC | #3 
On Tue, 2022-03-29 at 21:14 +0300, Ville Syrjälä wrote:
> On Mon, Mar 28, 2022 at 12:16:15PM -0700, José Roberto de Souza wrote:
> > From: Caz Yokoyama <caz.yokoyama@intel.com>
> > 
> > B credits set by IFWI do not match with specification default, so here
> > programming the right value.
> > 
> > Also while at it, taking the oportunity to do a read-modify-write to
> > not overwrite all other bits in this register that specification don't
> > ask us to change.
> 
> RMWs considered harmful. This is a double buffered register and in the
> future we may have to program it via DSB to update it atomically with
> the rest of the registers (eg. if we want to avoid the modeset for the
> mbus joining change). And when that happens the RMW will have to be
> removed again since the DSB can't even read registers. So IMO better
> to not even start down this path.

Okay but right now it is not harmful as affected pipes would be disabled at this point.
Without the RMW will need to set the default value for 3 other registers in PIPE_MBUS_DBOX_CTL offset.

I'm good with any, option. 

> 
> > 
> > BSpec: 49213
> > BSpec: 50343
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 3d2ff258f0a94..078ada041e1cd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1830,13 +1830,19 @@ static void icl_pipe_mbus_enable(struct intel_crtc *crtc, bool joined_mbus)
> >  	enum pipe pipe = crtc->pipe;
> >  	u32 val;
> >  
> > +	val = intel_de_read(dev_priv, PIPE_MBUS_DBOX_CTL(pipe));
> > +	val &= ~MBUS_DBOX_A_CREDIT_MASK;
> >  	/* Wa_22010947358:adl-p */
> >  	if (IS_ALDERLAKE_P(dev_priv))
> > -		val = joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
> > +		val |= joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
> >  	else
> > -		val = MBUS_DBOX_A_CREDIT(2);
> > +		val |= MBUS_DBOX_A_CREDIT(2);
> >  
> > -	if (DISPLAY_VER(dev_priv) >= 12) {
> > +	val &= ~(MBUS_DBOX_BW_CREDIT_MASK | MBUS_DBOX_B_CREDIT_MASK);
> > +	if (IS_ALDERLAKE_P(dev_priv)) {
> > +		val |= MBUS_DBOX_BW_CREDIT(2);
> > +		val |= MBUS_DBOX_B_CREDIT(8);
> > +	} else if (DISPLAY_VER(dev_priv) >= 12) {
> >  		val |= MBUS_DBOX_BW_CREDIT(2);
> >  		val |= MBUS_DBOX_B_CREDIT(12);
> >  	} else {
> > -- 
> > 2.35.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3d2ff258f0a94..078ada041e1cd 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1830,13 +1830,19 @@  static void icl_pipe_mbus_enable(struct intel_crtc *crtc, bool joined_mbus)
 	enum pipe pipe = crtc->pipe;
 	u32 val;
 
+	val = intel_de_read(dev_priv, PIPE_MBUS_DBOX_CTL(pipe));
+	val &= ~MBUS_DBOX_A_CREDIT_MASK;
 	/* Wa_22010947358:adl-p */
 	if (IS_ALDERLAKE_P(dev_priv))
-		val = joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
+		val |= joined_mbus ? MBUS_DBOX_A_CREDIT(6) : MBUS_DBOX_A_CREDIT(4);
 	else
-		val = MBUS_DBOX_A_CREDIT(2);
+		val |= MBUS_DBOX_A_CREDIT(2);
 
-	if (DISPLAY_VER(dev_priv) >= 12) {
+	val &= ~(MBUS_DBOX_BW_CREDIT_MASK | MBUS_DBOX_B_CREDIT_MASK);
+	if (IS_ALDERLAKE_P(dev_priv)) {
+		val |= MBUS_DBOX_BW_CREDIT(2);
+		val |= MBUS_DBOX_B_CREDIT(8);
+	} else if (DISPLAY_VER(dev_priv) >= 12) {
 		val |= MBUS_DBOX_BW_CREDIT(2);
 		val |= MBUS_DBOX_B_CREDIT(12);
 	} else {