All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v16 3/7] drm/i915: Introduce parameterized DBUF_CTL
Date: Wed, 29 Jan 2020 13:47:03 +0200	[thread overview]
Message-ID: <20200129114703.GT13686@intel.com> (raw)
In-Reply-To: <eae9425d255785ef5fb2bac5ed2bda0812df25f0.camel@intel.com>

On Wed, Jan 29, 2020 at 08:41:34AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2020-01-28 at 19:35 +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2020 at 10:44:52AM +0200, Stanislav Lisovskiy wrote:
> > > Now start using parameterized DBUF_CTL instead
> > > of hardcoded, this would allow shorter access
> > > functions when reading or storing entire state.
> > > 
> > > Tried to implement it in a MMIO_PIPE manner, however
> > > DBUF_CTL1 address is higher than DBUF_CTL2, which
> > > implies that we have to now subtract from base
> > > rather than add.
> > > 
> > > v2: - Removed unneeded DBUF_CTL_DIST and DBUF_CTL_ADDR
> > >       macros. Started to use _PICK construct as suggested
> > >       by Matt Roper.
> > > 
> > > v3: - DBUF_CTL_S* to _DBUF_CTL_S*, changed X to "slice"
> > >       in macro(Ville Syrjälä)
> > >     - Introduced enum for enumerating DBUF slices(Ville Syrjälä)
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power.c    | 30 +++++++++++--
> > > ------
> > >  .../drm/i915/display/intel_display_power.h    |  5 ++++
> > >  drivers/gpu/drm/i915/i915_reg.h               |  7 +++--
> > >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
> > >  4 files changed, 28 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 5e1c601f0f99..a59efb24be92 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -4418,9 +4418,11 @@ void icl_dbuf_slices_update(struct
> > > drm_i915_private *dev_priv,
> > >  		return;
> > >  
> > >  	if (req_slices > hw_enabled_slices)
> > > -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > > true);
> > > +		ret = intel_dbuf_slice_set(dev_priv,
> > > +					   _DBUF_CTL_S(DBUF_S2), true);
> > >  	else
> > > -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > > false);
> > > +		ret = intel_dbuf_slice_set(dev_priv,
> > > +					   _DBUF_CTL_S(DBUF_S2),
> > > false);
> > >  
> > >  	if (ret)
> > >  		dev_priv->enabled_dbuf_slices_num = req_slices;
> > > @@ -4428,14 +4430,16 @@ void icl_dbuf_slices_update(struct
> > > drm_i915_private *dev_priv,
> > >  
> > >  static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> > >  {
> > > -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) |
> > > DBUF_POWER_REQUEST);
> > > -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) |
> > > DBUF_POWER_REQUEST);
> > > -	POSTING_READ(DBUF_CTL_S2);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) |
> > > DBUF_POWER_REQUEST);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) |
> > > DBUF_POWER_REQUEST);
> > > +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> > >  
> > >  	udelay(10);
> > >  
> > > -	if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > > -	    !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > > +	if (!(I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > > +	    !(I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> > >  		DRM_ERROR("DBuf power enable timeout\n");
> > >  	else
> > >  		/*
> > > @@ -4447,14 +4451,16 @@ static void icl_dbuf_enable(struct
> > > drm_i915_private *dev_priv)
> > >  
> > >  static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> > >  {
> > > -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) &
> > > ~DBUF_POWER_REQUEST);
> > > -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) &
> > > ~DBUF_POWER_REQUEST);
> > > -	POSTING_READ(DBUF_CTL_S2);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) &
> > > ~DBUF_POWER_REQUEST);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) &
> > > ~DBUF_POWER_REQUEST);
> > > +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> > >  
> > >  	udelay(10);
> > >  
> > > -	if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > > -	    (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > > +	if ((I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > > +	    (I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> > >  		DRM_ERROR("DBuf power disable timeout!\n");
> > >  	else
> > >  		/*
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > index 2608a65af7fa..601e000ffd0d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > @@ -307,6 +307,11 @@ intel_display_power_put_async(struct
> > > drm_i915_private *i915,
> > >  }
> > >  #endif
> > >  
> > > +enum dbuf_slice {
> > > +	DBUF_S1,
> > > +	DBUF_S2,
> > > +};
> > > +
> > >  #define with_intel_display_power(i915, domain, wf) \
> > >  	for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
> > >  	     intel_display_power_put_async((i915), (domain), (wf)),
> > > (wf) = 0)
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index b93c4c18f05c..625be54d3eae 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7748,9 +7748,10 @@ enum {
> > >  #define DISP_ARB_CTL2	_MMIO(0x45004)
> > >  #define  DISP_DATA_PARTITION_5_6	(1 << 6)
> > >  #define  DISP_IPC_ENABLE		(1 << 3)
> > > -#define DBUF_CTL	_MMIO(0x45008)
> > > -#define DBUF_CTL_S1	_MMIO(0x45008)
> > > -#define DBUF_CTL_S2	_MMIO(0x44FE8)
> > > +#define DBUF_CTL_ADDR1			0x45008
> > > +#define DBUF_CTL_ADDR2			0x44FE8
> > > +#define _DBUF_CTL_S(X)			_MMIO(_PICK_EVEN(X,
> > > DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))
> > 
> > That's not at all what I meant. Also the 'X' is still there despite
> > what
> > the changelog says.
> > 
> > #define _DBUF_CTL_S1	0x45008
> > #define _DBUF_CTL_S2	0x44FE8
> > #define DBUF_CTL_S(slice)	_MMIO(_PICK_EVEN(slice, _DBUF_CTL_S1,
> > _DBUF_CTL_S2))
> 
> My idea was to still be able to use DBUF_CTL_S1 and DBUF_CTL_S2,  of

Just don't. Aliases are confusing.

> course this is a bit redundant, but though similar naming DBUF_CTL_S1
> and DBUF_CTL_S(0) might confuse somebody into using it. 
> For example in your case we can now only use DBUF_CTL_S() macro because
> _DBUF_CTL_S1/2 is not any longer using _MMIO.
> 
> But _now_ I get your point, I guess by "_" in the beginning, it is
> meant not to be used from outside. Some kind of like private class
> members in Python :)
> 
> I will change this once the rest of patches are reviewed, because that
> change anyway does not affect the actual functionality, but purely
> cosmetic.
> 
> Stan
> 
> > 
> > 
> > > +#define DBUF_CTL			_DBUF_CTL_S(0)
> > >  #define  DBUF_POWER_REQUEST		(1 << 31)
> > >  #define  DBUF_POWER_STATE		(1 << 30)
> > >  #define GEN7_MSG_CTL	_MMIO(0x45010)
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 04f94057d6b3..b8d78e26515c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3660,7 +3660,7 @@ u8 intel_enabled_dbuf_slices_num(struct
> > > drm_i915_private *dev_priv)
> > >  	 * only that 1 slice enabled until we have a proper way for on-
> > > demand
> > >  	 * toggling of the second slice.
> > >  	 */
> > > -	if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> > > +	if (0 && I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE)
> > >  		enabled_dbuf_slices_num++;
> > >  
> > >  	return enabled_dbuf_slices_num;
> > > -- 
> > > 2.24.1.485.gad05a3d8e5
> > 
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2020-01-29 11:47 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-24  8:44 [Intel-gfx] [PATCH v16 0/7] Enable second DBuf slice for ICL and TGL Stanislav Lisovskiy
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 1/7] drm/i915: Remove skl_ddl_allocation struct Stanislav Lisovskiy
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 2/7] drm/i915: Move dbuf slice update to proper place Stanislav Lisovskiy
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 3/7] drm/i915: Introduce parameterized DBUF_CTL Stanislav Lisovskiy
2020-01-28 17:35   ` Ville Syrjälä
2020-01-29  8:41     ` Lisovskiy, Stanislav
2020-01-29 11:47       ` Ville Syrjälä [this message]
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 4/7] drm/i915: Manipulate DBuf slices properly Stanislav Lisovskiy
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 5/7] drm/i915: Correctly map DBUF slices to pipes Stanislav Lisovskiy
2020-01-28 23:15   ` Matt Roper
2020-01-28 23:38     ` Matt Roper
2020-01-29  9:03     ` Lisovskiy, Stanislav
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 6/7] drm/i915: Protect intel_dbuf_slices_update with mutex Stanislav Lisovskiy
2020-01-28 23:33   ` Matt Roper
2020-01-29  9:22     ` Lisovskiy, Stanislav
2020-01-31 15:22     ` Ville Syrjälä
2020-01-24  8:44 ` [Intel-gfx] [PATCH v16 7/7] drm/i915: Update dbuf slices only with full modeset Stanislav Lisovskiy
2020-01-28 23:37   ` Matt Roper
2020-01-31 15:10     ` Ville Syrjälä
2020-01-24  9:52 ` [Intel-gfx] ✓ Fi.CI.BAT: success for Enable second DBuf slice for ICL and TGL (rev21) Patchwork
2020-01-26  9:19 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2020-01-27  7:48   ` Lisovskiy, Stanislav
2020-01-27 12:29     ` Peres, Martin
2020-01-27 13:01 ` [Intel-gfx] ✓ Fi.CI.IGT: success " Patchwork
2020-01-27 13:07 ` Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200129114703.GT13686@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=stanislav.lisovskiy@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.