[Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
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
Cc: Joonas Lahtinen
Cc: Michal Wajdeczko
Cc: Mika Kuoppala
Signed-off-by: Jani Nikula
---
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_ENABLEREG_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_ACTIVEREG_BIT(27)
#define PP_SEQUENCE_STATE_MASK REG_GENMASK(3, 0)
-#d
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
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 > Cc: Joonas Lahtinen > Cc: Michal Wajdeczko > Cc: Mika Kuoppala > Signed-off-by: Jani Nikula > --- > 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_ENABLEREG_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() - Ex
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
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. -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula wrote: @@ -108,9 +108,9 @@ * #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A, _FOO_B) * #define FOO_ENABLEREG_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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
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 1150i915_reg_t pp_reg;
ea0760cf Jesse Barnes 2011-01-04 1151u32 val;
10ed55e4 Ville Syrjälä 2018-05-23 1152enum pipe panel_pipe =
INVALID_PIPE;
0de3b485 Thomas Jarosch 2011-08-25 1153bool locked = true;
ea0760cf Jesse Barnes 2011-01-04 1154
4f8036a2 Tvrtko Ursulin 2016-10-13 1155if (WARN_ON(HAS_DDI(dev_priv)))
bedd4dba Jani Nikula2014-08-22 1156return;
bedd4dba Jani Nikula2014-08-22 1157
4f8036a2 Tvrtko Ursulin 2016-10-13 1158if (HAS_PCH_SPLIT(dev_priv)) {
bedd4dba Jani Nikula2014-08-22 1159u32 port_sel;
bedd4dba Jani Nikula2014-08-22 1160
44cb734c Imre Deak 2016-08-10 1161pp_reg = PP_CONTROL(0);
44cb734c Imre Deak 2016-08-10 1162port_sel =
I915_READ(PP_ON_DELAYS(0)) & PANEL_PORT_SELECT_MASK;
bedd4dba Jani Nikula2014-08-22 1163
4c23dea4 Ville Syrjälä 2018-05-18 1164switch (port_sel) {
4c23dea4 Ville Syrjälä 2018-05-18 @1165case
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 1167break;
4c23dea4 Ville Syrjälä 2018-05-18 1168case
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 1170break;
4c23dea4 Ville Syrjälä 2018-05-18 1171case
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 1173break;
4c23dea4 Ville Syrjälä 2018-05-18 1174case
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 1176break;
4c23dea4 Ville Syrjälä 2018-05-18 1177default:
4c23dea4 Ville Syrjälä 2018-05-18 1178
MISSING_CASE(port_sel);
4c23dea4 Ville Syrjälä 2018-05-18 1179break;
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 Nikula2014-08-22 1182/* presumably write
lock depends on pipe, not port select */
44cb734c Imre Deak 2016-08-10 1183pp_reg =
PP_CONTROL(pipe);
bedd4dba Jani Nikula2014-08-22 1184panel_pipe = pipe;
ea0760cf Jesse Barnes 2011-01-04 1185} else {
f0d2b758 Ville Syrjälä 2018-05-18 1186u32 port_sel;
f0d2b758 Ville Syrjälä 2018-05-18 1187
44cb734c Imre Deak 2016-08-10 1188pp_reg = PP_CONTROL(0);
f0d2b758 Ville Syrjälä 2018-05-18 1189port_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 1191WARN_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 1195val = I915_READ(pp_reg);
ea0760cf Jesse Bar
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
On Thu, 28 Feb 2019, Michal Wajdeczko wrote:
> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula
> wrote:
>
>> @@ -108,9 +108,9 @@
>> * #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
>> * #define FOO_ENABLEREG_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
>
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula
wrote:
On Thu, 28 Feb 2019, Michal Wajdeczko wrote:
On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula
wrote:
@@ -108,9 +108,9 @@
* #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A,
_FOO_B)
* #define FOO_ENABLEREG_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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
On Thu, 28 Feb 2019, Michal Wajdeczko wrote:
> On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula
> wrote:
>
>> On Thu, 28 Feb 2019, Michal Wajdeczko wrote:
>>> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula
>>> wrote:
>>>
@@ -108,9 +108,9 @@
* #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A,
_FOO_B)
* #define FOO_ENABLEREG_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.
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
