diff mbox series

[v3,3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values

Message ID 3f787052750840935689ac2fa8f3fa44cfbf1119.1551286447.git.jani.nikula@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: introduce macros to define register contents | expand

Commit Message

Jani Nikula Feb. 27, 2019, 5:02 p.m. UTC 
Slightly verbose, but does away with hand rolled shifts. Ties the field
values with the mask defining the field.

Unfortunately we have to make a local copy of FIELD_PREP() to evaluate
to a integer constant expression. But with this, we can ensure the mask
is non-zero, power of 2, fits u32, and the value fits the mask (when the
value is a constant expression).

Convert power sequencer registers as an example.

v3:
- rename the macro to REG_FIELD_PREP to avoid underscore prefix and to
  be in line with kernel macros (Chris)
- rename power of 2 check macro (Chris)

v2:
 - add build-time checks with BUILD_BUG_ON_ZERO()
 - rename to just _FIELD() due to regmap.h REG_FIELD() clash

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 69 +++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 30 deletions(-)

Comments

Chris Wilson Feb. 27, 2019, 9:07 p.m. UTC | #1 
Quoting Jani Nikula (2019-02-27 17:02:38)
> Slightly verbose, but does away with hand rolled shifts. Ties the field
> values with the mask defining the field.
> 
> Unfortunately we have to make a local copy of FIELD_PREP() to evaluate
> to a integer constant expression. But with this, we can ensure the mask
> is non-zero, power of 2, fits u32, and the value fits the mask (when the
> value is a constant expression).
> 
> Convert power sequencer registers as an example.
> 
> v3:
> - rename the macro to REG_FIELD_PREP to avoid underscore prefix and to
>   be in line with kernel macros (Chris)
> - rename power of 2 check macro (Chris)
> 
> v2:
>  - add build-time checks with BUILD_BUG_ON_ZERO()
>  - rename to just _FIELD() due to regmap.h REG_FIELD() clash
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 69 +++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1bd75770483a..70b7c5f0777b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -62,11 +62,11 @@
>   * significant to least significant bit. Indent the register content macros
>   * using two extra spaces between ``#define`` and the macro name.
>   *
> - * Define bit fields using ``REG_GENMASK(h, l)``. Define bit field contents so
> - * that they are already shifted in place, and can be directly OR'd. For
> - * convenience, function-like macros may be used to define bit fields, but do
> - * note that the macros may be needed to read as well as write the register
> - * contents.
> + * Define bit fields using ``REG_GENMASK(h, l)``. Define bit field contents
> + * using ``REG_FIELD_PREP(mask, value)``. This will define the values already
> + * shifted in place, so they can be directly OR'd together. For convenience,
> + * function-like macros may be used to define bit fields, but do note that the
> + * macros may be needed to read as well as write the register contents.
>   *
>   * Define bits using ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
>   *
> @@ -108,9 +108,9 @@
>   *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
>   *  #define   FOO_ENABLE                REG_BIT(31)
>   *  #define   FOO_MODE_MASK             REG_GENMASK(19, 16)
> - *  #define   FOO_MODE_BAR              (0 << 16)
> - *  #define   FOO_MODE_BAZ              (1 << 16)
> - *  #define   FOO_MODE_QUX_SNB          (2 << 16)
> + *  #define   FOO_MODE_BAR              REG_FIELD_PREP(FOO_MODE_MASK, 0)
> + *  #define   FOO_MODE_BAZ              REG_FIELD_PREP(FOO_MODE_MASK, 1)
> + *  #define   FOO_MODE_QUX_SNB          REG_FIELD_PREP(FOO_MODE_MASK, 2)
>   *
>   *  #define BAR                         _MMIO(0xb000)
>   *  #define GEN8_BAR                    _MMIO(0xb888)
> @@ -144,17 +144,27 @@
>                                  __builtin_constant_p(__low) &&         \
>                                  ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
>  
> +/*
> + * Local integer constant expression version of is_power_of_2().
> + */
> +#define IS_POWER_OF_2(__x)             ((__x) && (((__x) & ((__x) - 1)) == 0))

There's definitely something to be said about linux/build_bug.h not providing
this.

>  /**
>   * REG_FIELD_PREP() - Prepare a u32 bitfield value
>   * @__mask: shifted mask defining the field's length and position
>   * @__val: value to put in the field
>  
> - * Local wrapper for FIELD_PREP() to force u32 and for consistency with
> - * REG_FIELD_GET(), REG_BIT() and REG_GENMASK().
> + * Local copy of FIELD_PREP() to generate an integer constant expression, force
> + * u32 and for consistency with REG_FIELD_GET(), REG_BIT() and REG_GENMASK().
>   *
>   * @return: @__val masked and shifted into the field defined by @__mask.
>   */
> -#define REG_FIELD_PREP(__mask, __val)  ((u32)FIELD_PREP(__mask, __val))
> +#define REG_FIELD_PREP(__mask, __val)                                          \
> +       ((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +     \
> +              BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) +               \
> +              BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) +         \
> +              BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \

Take 0xff0 and add 0x10 to make 0x100!

So each field must be a conjoint set of bits.

> +              BUILD_BUG_ON_ZERO(__builtin_choose_expr(__builtin_constant_p(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))

If val is constant, check that val is within the range of mask;
otherwise skip the test rather than do it at runtime.

Hmm. I'd like a debug option to always check. That'll probably take just
a bit more ifdeffry, but if it catches just one bug, worth it?

>   * REG_FIELD_GET() - Extract a u32 bitfield value
> @@ -4763,27 +4773,26 @@ enum {
>   */
>  #define   PP_READY                     REG_BIT(30)
>  #define   PP_SEQUENCE_MASK             REG_GENMASK(29, 28)
> -#define   PP_SEQUENCE_NONE             (0 << 28)
> -#define   PP_SEQUENCE_POWER_UP         (1 << 28)
> -#define   PP_SEQUENCE_POWER_DOWN       (2 << 28)
> +#define   PP_SEQUENCE_NONE             REG_FIELD_PREP(PP_SEQUENCE_MASK, 0)
> +#define   PP_SEQUENCE_POWER_UP         REG_FIELD_PREP(PP_SEQUENCE_MASK, 1)
> +#define   PP_SEQUENCE_POWER_DOWN       REG_FIELD_PREP(PP_SEQUENCE_MASK, 2)

Ok.

>  #define   PP_CYCLE_DELAY_ACTIVE                REG_BIT(27)
>  #define   PP_SEQUENCE_STATE_MASK       REG_GENMASK(3, 0)
> -#define   PP_SEQUENCE_STATE_OFF_IDLE   (0x0 << 0)
> -#define   PP_SEQUENCE_STATE_OFF_S0_1   (0x1 << 0)
> -#define   PP_SEQUENCE_STATE_OFF_S0_2   (0x2 << 0)
> -#define   PP_SEQUENCE_STATE_OFF_S0_3   (0x3 << 0)
> -#define   PP_SEQUENCE_STATE_ON_IDLE    (0x8 << 0)
> -#define   PP_SEQUENCE_STATE_ON_S1_0    (0x9 << 0)
> -#define   PP_SEQUENCE_STATE_ON_S1_2    (0xa << 0)
> -#define   PP_SEQUENCE_STATE_ON_S1_3    (0xb << 0)
> -#define   PP_SEQUENCE_STATE_RESET      (0xf << 0)

> +#define   PP_SEQUENCE_STATE_OFF_IDLE   REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x0)
> +#define   PP_SEQUENCE_STATE_OFF_S0_1   REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x1)
> +#define   PP_SEQUENCE_STATE_OFF_S0_2   REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x2)
> +#define   PP_SEQUENCE_STATE_OFF_S0_3   REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x3)
Ok.

> +#define   PP_SEQUENCE_STATE_ON_IDLE    REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x8)
> +#define   PP_SEQUENCE_STATE_ON_S1_0    REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x9)
> +#define   PP_SEQUENCE_STATE_ON_S1_2    REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0xa)
> +#define   PP_SEQUENCE_STATE_ON_S1_3    REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0xb)

I feel there's a connection here...

> +#define   PP_SEQUENCE_STATE_RESET      REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0xf)

Ok.

>  #define _PP_CONTROL                    0x61204
>  #define PP_CONTROL(pps_idx)            _MMIO_PPS(pps_idx, _PP_CONTROL)
>  #define  PANEL_UNLOCK_MASK             REG_GENMASK(31, 16)
> -#define  PANEL_UNLOCK_REGS             (0xabcd << 16)
> +#define  PANEL_UNLOCK_REGS             REG_FIELD_PREP(PANEL_UNLOCK_MASK, 0xabcd)

Ok.

>  #define  BXT_POWER_CYCLE_DELAY_MASK    REG_GENMASK(8, 4)
> -#define  BXT_POWER_CYCLE_DELAY_SHIFT   4
>  #define  EDP_FORCE_VDD                 REG_BIT(3)
>  #define  EDP_BLC_ENABLE                        REG_BIT(2)
>  #define  PANEL_POWER_RESET             REG_BIT(1)
> @@ -4792,11 +4801,11 @@ enum {
>  #define _PP_ON_DELAYS                  0x61208
>  #define PP_ON_DELAYS(pps_idx)          _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
>  #define  PANEL_PORT_SELECT_MASK                REG_GENMASK(31, 30)
> -#define  PANEL_PORT_SELECT_LVDS                (0 << 30)
> -#define  PANEL_PORT_SELECT_DPA         (1 << 30)
> -#define  PANEL_PORT_SELECT_DPC         (2 << 30)
> -#define  PANEL_PORT_SELECT_DPD         (3 << 30)
> -#define  PANEL_PORT_SELECT_VLV(port)   ((port) << 30)
> +#define  PANEL_PORT_SELECT_LVDS                REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 0)
> +#define  PANEL_PORT_SELECT_DPA         REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 1)
> +#define  PANEL_PORT_SELECT_DPC         REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 2)
> +#define  PANEL_PORT_SELECT_DPD         REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 3)
> +#define  PANEL_PORT_SELECT_VLV(port)   REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, port)
>  #define  PANEL_POWER_UP_DELAY_MASK     REG_GENMASK(28, 16)
>  #define  PANEL_LIGHT_ON_DELAY_MASK     REG_GENMASK(12, 0)

Ok.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä Feb. 27, 2019, 9:11 p.m. UTC | #2 
On Wed, Feb 27, 2019 at 07:02:38PM +0200, Jani Nikula wrote:
> Slightly verbose, but does away with hand rolled shifts. Ties the field
> values with the mask defining the field.
> 
> Unfortunately we have to make a local copy of FIELD_PREP() to evaluate
> to a integer constant expression. But with this, we can ensure the mask
> is non-zero, power of 2, fits u32, and the value fits the mask (when the
> value is a constant expression).

I might like a debug knob to make that into a runtime check for
non-const expressions. But that can be considered later.
Michal Wajdeczko Feb. 28, 2019, 12:17 a.m. UTC | #3 
On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula@intel.com>  
wrote:

> @@ -108,9 +108,9 @@
>   *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
>   *  #define   FOO_ENABLE                REG_BIT(31)
>   *  #define   FOO_MODE_MASK             REG_GENMASK(19, 16)
> - *  #define   FOO_MODE_BAR              (0 << 16)
> - *  #define   FOO_MODE_BAZ              (1 << 16)
> - *  #define   FOO_MODE_QUX_SNB          (2 << 16)
> + *  #define   FOO_MODE_BAR              REG_FIELD_PREP(FOO_MODE_MASK, 0)
> + *  #define   FOO_MODE_BAZ              REG_FIELD_PREP(FOO_MODE_MASK, 1)
> + *  #define   FOO_MODE_QUX_SNB          REG_FIELD_PREP(FOO_MODE_MASK, 2)

hmm, shouldn't we define these values as:

#define   FOO_MODE_BAR              (0)
#define   FOO_MODE_BAZ              (1)
#define   FOO_MODE_QUX_SNB          (2)

to allow using them natively with REG_FIELD_GET/PREPARE() ?
maybe we should also consider dropping _MASK suffix?

MMIO_WRITE(...,
            REG_FIELD_PREPARE(FOO_ENABLE, true) |
            REG_FIELD_PREPARE(FOO_MODE, FOO_MODE_BAR))

mode = REG_FIELD_GET(FOO_MODE, MMIO_READ(...));
enabled = REG_FIELD_GET(FOO_ENABLE, MMIO_READ(...));

Thanks,
Michal
kernel test robot Feb. 28, 2019, 4:49 a.m. UTC | #4 
Hi Jani,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20190227]
[cannot apply to v5.0-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jani-Nikula/drm-i915-introduce-macros-to-define-register-contents/20190228-093058
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/intel_display.c:1165:22: sparse: error: Expected constant expression in case statement
   drivers/gpu/drm/i915/intel_display.c:1168:22: sparse: error: Expected constant expression in case statement
   drivers/gpu/drm/i915/intel_display.c:1171:22: sparse: error: Expected constant expression in case statement
   drivers/gpu/drm/i915/intel_display.c:1174:22: sparse: error: Expected constant expression in case statement

vim +1165 drivers/gpu/drm/i915/intel_display.c

040484af Jesse Barnes   2011-01-03  1147  
4f8036a2 Tvrtko Ursulin 2016-10-13  1148  void assert_panel_unlocked(struct drm_i915_private *dev_priv, enum pipe pipe)
ea0760cf Jesse Barnes   2011-01-04  1149  {
f0f59a00 Ville Syrjälä  2015-11-18  1150  	i915_reg_t pp_reg;
ea0760cf Jesse Barnes   2011-01-04  1151  	u32 val;
10ed55e4 Ville Syrjälä  2018-05-23  1152  	enum pipe panel_pipe = INVALID_PIPE;
0de3b485 Thomas Jarosch 2011-08-25  1153  	bool locked = true;
ea0760cf Jesse Barnes   2011-01-04  1154  
4f8036a2 Tvrtko Ursulin 2016-10-13  1155  	if (WARN_ON(HAS_DDI(dev_priv)))
bedd4dba Jani Nikula    2014-08-22  1156  		return;
bedd4dba Jani Nikula    2014-08-22  1157  
4f8036a2 Tvrtko Ursulin 2016-10-13  1158  	if (HAS_PCH_SPLIT(dev_priv)) {
bedd4dba Jani Nikula    2014-08-22  1159  		u32 port_sel;
bedd4dba Jani Nikula    2014-08-22  1160  
44cb734c Imre Deak      2016-08-10  1161  		pp_reg = PP_CONTROL(0);
44cb734c Imre Deak      2016-08-10  1162  		port_sel = I915_READ(PP_ON_DELAYS(0)) & PANEL_PORT_SELECT_MASK;
bedd4dba Jani Nikula    2014-08-22  1163  
4c23dea4 Ville Syrjälä  2018-05-18  1164  		switch (port_sel) {
4c23dea4 Ville Syrjälä  2018-05-18 @1165  		case PANEL_PORT_SELECT_LVDS:
a44628b9 Ville Syrjälä  2018-05-14  1166  			intel_lvds_port_enabled(dev_priv, PCH_LVDS, &panel_pipe);
4c23dea4 Ville Syrjälä  2018-05-18  1167  			break;
4c23dea4 Ville Syrjälä  2018-05-18  1168  		case PANEL_PORT_SELECT_DPA:
4c23dea4 Ville Syrjälä  2018-05-18  1169  			intel_dp_port_enabled(dev_priv, DP_A, PORT_A, &panel_pipe);
4c23dea4 Ville Syrjälä  2018-05-18  1170  			break;
4c23dea4 Ville Syrjälä  2018-05-18  1171  		case PANEL_PORT_SELECT_DPC:
4c23dea4 Ville Syrjälä  2018-05-18  1172  			intel_dp_port_enabled(dev_priv, PCH_DP_C, PORT_C, &panel_pipe);
4c23dea4 Ville Syrjälä  2018-05-18  1173  			break;
4c23dea4 Ville Syrjälä  2018-05-18  1174  		case PANEL_PORT_SELECT_DPD:
4c23dea4 Ville Syrjälä  2018-05-18  1175  			intel_dp_port_enabled(dev_priv, PCH_DP_D, PORT_D, &panel_pipe);
4c23dea4 Ville Syrjälä  2018-05-18  1176  			break;
4c23dea4 Ville Syrjälä  2018-05-18  1177  		default:
4c23dea4 Ville Syrjälä  2018-05-18  1178  			MISSING_CASE(port_sel);
4c23dea4 Ville Syrjälä  2018-05-18  1179  			break;
4c23dea4 Ville Syrjälä  2018-05-18  1180  		}
4f8036a2 Tvrtko Ursulin 2016-10-13  1181  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
bedd4dba Jani Nikula    2014-08-22  1182  		/* presumably write lock depends on pipe, not port select */
44cb734c Imre Deak      2016-08-10  1183  		pp_reg = PP_CONTROL(pipe);
bedd4dba Jani Nikula    2014-08-22  1184  		panel_pipe = pipe;
ea0760cf Jesse Barnes   2011-01-04  1185  	} else {
f0d2b758 Ville Syrjälä  2018-05-18  1186  		u32 port_sel;
f0d2b758 Ville Syrjälä  2018-05-18  1187  
44cb734c Imre Deak      2016-08-10  1188  		pp_reg = PP_CONTROL(0);
f0d2b758 Ville Syrjälä  2018-05-18  1189  		port_sel = I915_READ(PP_ON_DELAYS(0)) & PANEL_PORT_SELECT_MASK;
f0d2b758 Ville Syrjälä  2018-05-18  1190  
f0d2b758 Ville Syrjälä  2018-05-18  1191  		WARN_ON(port_sel != PANEL_PORT_SELECT_LVDS);
a44628b9 Ville Syrjälä  2018-05-14  1192  		intel_lvds_port_enabled(dev_priv, LVDS, &panel_pipe);
ea0760cf Jesse Barnes   2011-01-04  1193  	}
ea0760cf Jesse Barnes   2011-01-04  1194  
ea0760cf Jesse Barnes   2011-01-04  1195  	val = I915_READ(pp_reg);
ea0760cf Jesse Barnes   2011-01-04  1196  	if (!(val & PANEL_POWER_ON) ||
ec49ba2d Jani Nikula    2014-08-21  1197  	    ((val & PANEL_UNLOCK_MASK) == PANEL_UNLOCK_REGS))
ea0760cf Jesse Barnes   2011-01-04  1198  		locked = false;
ea0760cf Jesse Barnes   2011-01-04  1199  
e2c719b7 Rob Clark      2014-12-15  1200  	I915_STATE_WARN(panel_pipe == pipe && locked,
ea0760cf Jesse Barnes   2011-01-04  1201  	     "panel assertion failure, pipe %c regs locked\n",
9db4a9c7 Jesse Barnes   2011-02-07  1202  	     pipe_name(pipe));
ea0760cf Jesse Barnes   2011-01-04  1203  }
ea0760cf Jesse Barnes   2011-01-04  1204  

:::::: The code at line 1165 was first introduced by commit
:::::: 4c23dea48b0d8d9da2ac0911f8cee105f4394281 drm/i915: Implement the missing bits of assert_panel_unlocked()

:::::: TO: Ville Syrjälä <ville.syrjala@linux.intel.com>
:::::: CC: Ville Syrjälä <ville.syrjala@linux.intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Jani Nikula Feb. 28, 2019, 10:24 a.m. UTC | #5 
On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula@intel.com>  
> wrote:
>
>> @@ -108,9 +108,9 @@
>>   *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
>>   *  #define   FOO_ENABLE                REG_BIT(31)
>>   *  #define   FOO_MODE_MASK             REG_GENMASK(19, 16)
>> - *  #define   FOO_MODE_BAR              (0 << 16)
>> - *  #define   FOO_MODE_BAZ              (1 << 16)
>> - *  #define   FOO_MODE_QUX_SNB          (2 << 16)
>> + *  #define   FOO_MODE_BAR              REG_FIELD_PREP(FOO_MODE_MASK, 0)
>> + *  #define   FOO_MODE_BAZ              REG_FIELD_PREP(FOO_MODE_MASK, 1)
>> + *  #define   FOO_MODE_QUX_SNB          REG_FIELD_PREP(FOO_MODE_MASK, 2)
>
> hmm, shouldn't we define these values as:
>
> #define   FOO_MODE_BAR              (0)
> #define   FOO_MODE_BAZ              (1)
> #define   FOO_MODE_QUX_SNB          (2)
>
> to allow using them natively with REG_FIELD_GET/PREPARE() ?
> maybe we should also consider dropping _MASK suffix?
>
> MMIO_WRITE(...,
>             REG_FIELD_PREPARE(FOO_ENABLE, true) |
>             REG_FIELD_PREPARE(FOO_MODE, FOO_MODE_BAR))
>
> mode = REG_FIELD_GET(FOO_MODE, MMIO_READ(...));
> enabled = REG_FIELD_GET(FOO_ENABLE, MMIO_READ(...));

I would have to agree with you *if* we were writing all this from
scratch. But almost all of the existing bitfield values are defined
shifted in place, so you can OR them in place directly. I want to keep
it that way instead of creating a mix. And we have about 1k macros with
_MASK suffix too.

So, yeah, it's going to be slightly problematic to REG_FIELD_GET() a
field and compare it against a defined value for that field. I expect us
to keep using things like:

	if ((val & FOO_MODE_MASK) == FOO_MODE_BAR)

Indeed, one of the reasons for the local integer constant expression
version of REG_FIELD_PREP() is to allow it in case labels:

	switch (val & FOO_MODE_MASK) {
        case FOO_MODE_BAR: /* defined using REG_FIELD_PREP() */
        	/* ... */
        }

I don't want to have to start changing these common existing conventions
throughout the driver. With the proposed approach, we can define the
registers in the new style and not change anything. We can drop _SHIFT
case by case if we move to REG_FIELD_PREP/GET usage.


BR,
Jani.


>
> Thanks,
> Michal
>
Michal Wajdeczko Feb. 28, 2019, 11:38 a.m. UTC | #6 
On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula <jani.nikula@intel.com>  
wrote:

> On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
>> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula@intel.com>
>> wrote:
>>
>>> @@ -108,9 +108,9 @@
>>>   *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A,  
>>> _FOO_B)
>>>   *  #define   FOO_ENABLE                REG_BIT(31)
>>>   *  #define   FOO_MODE_MASK             REG_GENMASK(19, 16)
>>> - *  #define   FOO_MODE_BAR              (0 << 16)
>>> - *  #define   FOO_MODE_BAZ              (1 << 16)
>>> - *  #define   FOO_MODE_QUX_SNB          (2 << 16)
>>> + *  #define   FOO_MODE_BAR              REG_FIELD_PREP(FOO_MODE_MASK,  
>>> 0)
>>> + *  #define   FOO_MODE_BAZ              REG_FIELD_PREP(FOO_MODE_MASK,  
>>> 1)
>>> + *  #define   FOO_MODE_QUX_SNB          REG_FIELD_PREP(FOO_MODE_MASK,  
>>> 2)
>>
>> hmm, shouldn't we define these values as:
>>
>> #define   FOO_MODE_BAR              (0)
>> #define   FOO_MODE_BAZ              (1)
>> #define   FOO_MODE_QUX_SNB          (2)
>>
>> to allow using them natively with REG_FIELD_GET/PREPARE() ?
>> maybe we should also consider dropping _MASK suffix?
>>
>> MMIO_WRITE(...,
>>             REG_FIELD_PREPARE(FOO_ENABLE, true) |
>>             REG_FIELD_PREPARE(FOO_MODE, FOO_MODE_BAR))
>>
>> mode = REG_FIELD_GET(FOO_MODE, MMIO_READ(...));
>> enabled = REG_FIELD_GET(FOO_ENABLE, MMIO_READ(...));
>
> I would have to agree with you *if* we were writing all this from
> scratch. But almost all of the existing bitfield values are defined
> shifted in place, so you can OR them in place directly. I want to keep
> it that way instead of creating a mix. And we have about 1k macros with
> _MASK suffix too.

But since this is 'documentation' part, maybe we should push into preferred
usage model, leaving behind existing code (and let it upgrade case by case)

>
> So, yeah, it's going to be slightly problematic to REG_FIELD_GET() a
> field and compare it against a defined value for that field. I expect us
> to keep using things like:
>
> 	if ((val & FOO_MODE_MASK) == FOO_MODE_BAR)

Hmm, if you still want to use it this way, what's the purpose of having
pair of REG_FIELD_GET macro?

>
> Indeed, one of the reasons for the local integer constant expression
> version of REG_FIELD_PREP() is to allow it in case labels:
>
> 	switch (val & FOO_MODE_MASK) {
>         case FOO_MODE_BAR: /* defined using REG_FIELD_PREP() */
>         	/* ... */
>         }
>
> I don't want to have to start changing these common existing conventions
> throughout the driver. With the proposed approach, we can define the
> registers in the new style and not change anything. We can drop _SHIFT
> case by case if we move to REG_FIELD_PREP/GET usage.
>

But actually maybe better option would be to let old definitions and code
intact, and then later change both places at once, to follow new rules:

	mode = REG_FIELD_GET(FOO_MODE_MASK, val);
	switch (mode) {
		case FOO_MODE_BAR: /* defined like 0 based enums */
		/* ... */
	}

otherwise, regardless of having new style _PREP/_GET macros, we still
be using our old convention based on _SHIFT'ed values.

As Chris replied earlier, we must take into account "that other people
reading our code already know the language" and with keeping register
values shifted, we may break style expected by _PREP/_GET.

Thanks,
Michal
Jani Nikula Feb. 28, 2019, 1:48 p.m. UTC | #7 
On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula <jani.nikula@intel.com>  
> wrote:
>
>> On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
>>> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula@intel.com>
>>> wrote:
>>>
>>>> @@ -108,9 +108,9 @@
>>>>   *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A,  
>>>> _FOO_B)
>>>>   *  #define   FOO_ENABLE                REG_BIT(31)
>>>>   *  #define   FOO_MODE_MASK             REG_GENMASK(19, 16)
>>>> - *  #define   FOO_MODE_BAR              (0 << 16)
>>>> - *  #define   FOO_MODE_BAZ              (1 << 16)
>>>> - *  #define   FOO_MODE_QUX_SNB          (2 << 16)
>>>> + *  #define   FOO_MODE_BAR              REG_FIELD_PREP(FOO_MODE_MASK,  
>>>> 0)
>>>> + *  #define   FOO_MODE_BAZ              REG_FIELD_PREP(FOO_MODE_MASK,  
>>>> 1)
>>>> + *  #define   FOO_MODE_QUX_SNB          REG_FIELD_PREP(FOO_MODE_MASK,  
>>>> 2)
>>>
>>> hmm, shouldn't we define these values as:
>>>
>>> #define   FOO_MODE_BAR              (0)
>>> #define   FOO_MODE_BAZ              (1)
>>> #define   FOO_MODE_QUX_SNB          (2)
>>>
>>> to allow using them natively with REG_FIELD_GET/PREPARE() ?
>>> maybe we should also consider dropping _MASK suffix?
>>>
>>> MMIO_WRITE(...,
>>>             REG_FIELD_PREPARE(FOO_ENABLE, true) |
>>>             REG_FIELD_PREPARE(FOO_MODE, FOO_MODE_BAR))
>>>
>>> mode = REG_FIELD_GET(FOO_MODE, MMIO_READ(...));
>>> enabled = REG_FIELD_GET(FOO_ENABLE, MMIO_READ(...));
>>
>> I would have to agree with you *if* we were writing all this from
>> scratch. But almost all of the existing bitfield values are defined
>> shifted in place, so you can OR them in place directly. I want to keep
>> it that way instead of creating a mix. And we have about 1k macros with
>> _MASK suffix too.
>
> But since this is 'documentation' part, maybe we should push into preferred
> usage model, leaving behind existing code (and let it upgrade case by case)

I'm actually not even sure I agree with the usage model. Contrast:

I915_WRITE(..., FOO_MODE_BAR | FOO_SOMETHING_ELSE);

with

I915_WRITE(..., REG_FIELD_PREP(FOO_MODE_MASK, FOO_MODE_BAR) |
	   FOO_SOMETHING_ELSE);

When possible, I think I still prefer having the verbose part in
i915_reg.h in favor of keeping the code readable *and* uniform
regardless of whether the fields were defined using REG_FIELD_PREP() or
manual shifts.

I can also imagine doing:

#define FOO_SIZE_MASK		REG_GENMASK(7, 0)
#define FOO_SIZE(size)		REG_FIELD_PREP(FOO_SIZE_MASK, size)

and using:

I915_WRITE(..., FOO_SIZE(42))

instead of:

I915_WRITE(..., REG_FIELD_PREP(FOO_SIZE_MASK, size))

>
>>
>> So, yeah, it's going to be slightly problematic to REG_FIELD_GET() a
>> field and compare it against a defined value for that field. I expect us
>> to keep using things like:
>>
>> 	if ((val & FOO_MODE_MASK) == FOO_MODE_BAR)
>
> Hmm, if you still want to use it this way, what's the purpose of having
> pair of REG_FIELD_GET macro?

I don't think we read register fields to compare them against the macros
all that much. I think it's more common to read the fields to extract
some value (say, pixels, timing), or to compare against a value stored
in state.

>
>>
>> Indeed, one of the reasons for the local integer constant expression
>> version of REG_FIELD_PREP() is to allow it in case labels:
>>
>> 	switch (val & FOO_MODE_MASK) {
>>         case FOO_MODE_BAR: /* defined using REG_FIELD_PREP() */
>>         	/* ... */
>>         }
>>
>> I don't want to have to start changing these common existing conventions
>> throughout the driver. With the proposed approach, we can define the
>> registers in the new style and not change anything. We can drop _SHIFT
>> case by case if we move to REG_FIELD_PREP/GET usage.
>>
>
> But actually maybe better option would be to let old definitions and code
> intact, and then later change both places at once, to follow new rules:
>
> 	mode = REG_FIELD_GET(FOO_MODE_MASK, val);
> 	switch (mode) {
> 		case FOO_MODE_BAR: /* defined like 0 based enums */
> 		/* ... */
> 	}
>
> otherwise, regardless of having new style _PREP/_GET macros, we still
> be using our old convention based on _SHIFT'ed values.
>
> As Chris replied earlier, we must take into account "that other people
> reading our code already know the language" and with keeping register
> values shifted, we may break style expected by _PREP/_GET.

So imagine the mixed use, one set of registers converted, some others
not, and you have code with:

I915_WRITE(..., REG_FIELD_PREP(FOO_MODE_MASK, FOO_MODE_BAR) |
	   FOO_SOMETHING_ELSE);
I915_WRITE(..., BAR_MODE_BAZ | BAR_SOMETHING_ELSE);

And you're wondering why the inconsistency.


BR,
Jani.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1bd75770483a..70b7c5f0777b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -62,11 +62,11 @@ 
  * significant to least significant bit. Indent the register content macros
  * using two extra spaces between ``#define`` and the macro name.
  *
- * Define bit fields using ``REG_GENMASK(h, l)``. Define bit field contents so
- * that they are already shifted in place, and can be directly OR'd. For
- * convenience, function-like macros may be used to define bit fields, but do
- * note that the macros may be needed to read as well as write the register
- * contents.
+ * Define bit fields using ``REG_GENMASK(h, l)``. Define bit field contents
+ * using ``REG_FIELD_PREP(mask, value)``. This will define the values already
+ * shifted in place, so they can be directly OR'd together. For convenience,
+ * function-like macros may be used to define bit fields, but do note that the
+ * macros may be needed to read as well as write the register contents.
  *
  * Define bits using ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
  *
@@ -108,9 +108,9 @@ 
  *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
  *  #define   FOO_ENABLE                REG_BIT(31)
  *  #define   FOO_MODE_MASK             REG_GENMASK(19, 16)
- *  #define   FOO_MODE_BAR              (0 << 16)
- *  #define   FOO_MODE_BAZ              (1 << 16)
- *  #define   FOO_MODE_QUX_SNB          (2 << 16)
+ *  #define   FOO_MODE_BAR              REG_FIELD_PREP(FOO_MODE_MASK, 0)
+ *  #define   FOO_MODE_BAZ              REG_FIELD_PREP(FOO_MODE_MASK, 1)
+ *  #define   FOO_MODE_QUX_SNB          REG_FIELD_PREP(FOO_MODE_MASK, 2)
  *
  *  #define BAR                         _MMIO(0xb000)
  *  #define GEN8_BAR                    _MMIO(0xb888)
@@ -144,17 +144,27 @@ 
 				 __builtin_constant_p(__low) &&		\
 				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
 
+/*
+ * Local integer constant expression version of is_power_of_2().
+ */
+#define IS_POWER_OF_2(__x)		((__x) && (((__x) & ((__x) - 1)) == 0))
+
 /**
  * REG_FIELD_PREP() - Prepare a u32 bitfield value
  * @__mask: shifted mask defining the field's length and position
  * @__val: value to put in the field
 
- * Local wrapper for FIELD_PREP() to force u32 and for consistency with
- * REG_FIELD_GET(), REG_BIT() and REG_GENMASK().
+ * Local copy of FIELD_PREP() to generate an integer constant expression, force
+ * u32 and for consistency with REG_FIELD_GET(), REG_BIT() and REG_GENMASK().
  *
  * @return: @__val masked and shifted into the field defined by @__mask.
  */
-#define REG_FIELD_PREP(__mask, __val)	((u32)FIELD_PREP(__mask, __val))
+#define REG_FIELD_PREP(__mask, __val)						\
+	((u32)((((typeof(__mask))(__val) << __bf_shf(__mask)) & (__mask)) +	\
+	       BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) +		\
+	       BUILD_BUG_ON_ZERO((__mask) == 0 || (__mask) > U32_MAX) +		\
+	       BUILD_BUG_ON_ZERO(!IS_POWER_OF_2((__mask) + (1ULL << __bf_shf(__mask)))) + \
+	       BUILD_BUG_ON_ZERO(__builtin_choose_expr(__builtin_constant_p(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
 
 /**
  * REG_FIELD_GET() - Extract a u32 bitfield value
@@ -4763,27 +4773,26 @@  enum {
  */
 #define   PP_READY			REG_BIT(30)
 #define   PP_SEQUENCE_MASK		REG_GENMASK(29, 28)
-#define   PP_SEQUENCE_NONE		(0 << 28)
-#define   PP_SEQUENCE_POWER_UP		(1 << 28)
-#define   PP_SEQUENCE_POWER_DOWN	(2 << 28)
+#define   PP_SEQUENCE_NONE		REG_FIELD_PREP(PP_SEQUENCE_MASK, 0)
+#define   PP_SEQUENCE_POWER_UP		REG_FIELD_PREP(PP_SEQUENCE_MASK, 1)
+#define   PP_SEQUENCE_POWER_DOWN	REG_FIELD_PREP(PP_SEQUENCE_MASK, 2)
 #define   PP_CYCLE_DELAY_ACTIVE		REG_BIT(27)
 #define   PP_SEQUENCE_STATE_MASK	REG_GENMASK(3, 0)
-#define   PP_SEQUENCE_STATE_OFF_IDLE	(0x0 << 0)
-#define   PP_SEQUENCE_STATE_OFF_S0_1	(0x1 << 0)
-#define   PP_SEQUENCE_STATE_OFF_S0_2	(0x2 << 0)
-#define   PP_SEQUENCE_STATE_OFF_S0_3	(0x3 << 0)
-#define   PP_SEQUENCE_STATE_ON_IDLE	(0x8 << 0)
-#define   PP_SEQUENCE_STATE_ON_S1_0	(0x9 << 0)
-#define   PP_SEQUENCE_STATE_ON_S1_2	(0xa << 0)
-#define   PP_SEQUENCE_STATE_ON_S1_3	(0xb << 0)
-#define   PP_SEQUENCE_STATE_RESET	(0xf << 0)
+#define   PP_SEQUENCE_STATE_OFF_IDLE	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x0)
+#define   PP_SEQUENCE_STATE_OFF_S0_1	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x1)
+#define   PP_SEQUENCE_STATE_OFF_S0_2	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x2)
+#define   PP_SEQUENCE_STATE_OFF_S0_3	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x3)
+#define   PP_SEQUENCE_STATE_ON_IDLE	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x8)
+#define   PP_SEQUENCE_STATE_ON_S1_0	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0x9)
+#define   PP_SEQUENCE_STATE_ON_S1_2	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0xa)
+#define   PP_SEQUENCE_STATE_ON_S1_3	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0xb)
+#define   PP_SEQUENCE_STATE_RESET	REG_FIELD_PREP(PP_SEQUENCE_STATE_MASK, 0xf)
 
 #define _PP_CONTROL			0x61204
 #define PP_CONTROL(pps_idx)		_MMIO_PPS(pps_idx, _PP_CONTROL)
 #define  PANEL_UNLOCK_MASK		REG_GENMASK(31, 16)
-#define  PANEL_UNLOCK_REGS		(0xabcd << 16)
+#define  PANEL_UNLOCK_REGS		REG_FIELD_PREP(PANEL_UNLOCK_MASK, 0xabcd)
 #define  BXT_POWER_CYCLE_DELAY_MASK	REG_GENMASK(8, 4)
-#define  BXT_POWER_CYCLE_DELAY_SHIFT	4
 #define  EDP_FORCE_VDD			REG_BIT(3)
 #define  EDP_BLC_ENABLE			REG_BIT(2)
 #define  PANEL_POWER_RESET		REG_BIT(1)
@@ -4792,11 +4801,11 @@  enum {
 #define _PP_ON_DELAYS			0x61208
 #define PP_ON_DELAYS(pps_idx)		_MMIO_PPS(pps_idx, _PP_ON_DELAYS)
 #define  PANEL_PORT_SELECT_MASK		REG_GENMASK(31, 30)
-#define  PANEL_PORT_SELECT_LVDS		(0 << 30)
-#define  PANEL_PORT_SELECT_DPA		(1 << 30)
-#define  PANEL_PORT_SELECT_DPC		(2 << 30)
-#define  PANEL_PORT_SELECT_DPD		(3 << 30)
-#define  PANEL_PORT_SELECT_VLV(port)	((port) << 30)
+#define  PANEL_PORT_SELECT_LVDS		REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 0)
+#define  PANEL_PORT_SELECT_DPA		REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 1)
+#define  PANEL_PORT_SELECT_DPC		REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 2)
+#define  PANEL_PORT_SELECT_DPD		REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, 3)
+#define  PANEL_PORT_SELECT_VLV(port)	REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, port)
 #define  PANEL_POWER_UP_DELAY_MASK	REG_GENMASK(28, 16)
 #define  PANEL_LIGHT_ON_DELAY_MASK	REG_GENMASK(12, 0)