* [PATCH v3 0/3] drm/i915: introduce macros to define register contents
@ 2019-02-27 17:02 Jani Nikula
2019-02-27 17:02 ` [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() " Jani Nikula
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Jani Nikula @ 2019-02-27 17:02 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
v3 of [1] with naming hopefully settled (fingers crossed), and some more
compile time checks, documentation and other polish added.
The naming scheme of the local wrappers/copies of bit fiddling macros is
to just use REG_ prefix for the regular kernel ones, with hopefully
minimal confusion. So we end up with:
REG_BIT()
REG_GENMASK()
REG_FIELD_PREP()
REG_FIELD_GET()
We can also use the same macros in i915_reg.h and rest of the driver,
with no mixed use for register contents. Indeed some of the regular
kernel macros lead to non-u32 types being used.
BR,
Jani.
[1] http://mid.mail-archive.com/cover.1547726792.git.jani.nikula@intel.com
Jani Nikula (3):
drm/i915: introduce REG_BIT() and REG_GENMASK() to define register
contents
drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
drm/i915: use REG_FIELD_PREP() to define register bitfield values
drivers/gpu/drm/i915/i915_reg.h | 182 +++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_dp.c | 40 +++----
drivers/gpu/drm/i915/intel_lvds.c | 40 +++----
3 files changed, 151 insertions(+), 111 deletions(-)
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
@ 2019-02-27 17:02 ` Jani Nikula
2019-02-27 20:50 ` Chris Wilson
2019-02-27 23:06 ` Michal Wajdeczko
2019-02-27 17:02 ` [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors Jani Nikula
` (5 subsequent siblings)
6 siblings, 2 replies; 22+ messages in thread
From: Jani Nikula @ 2019-02-27 17:02 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
Introduce REG_BIT(n) to define register bits and REG_GENMASK(h, l) to
define register bitfield masks.
We define the above as wrappers to BIT() and GENMASK() respectively to
force u32 type to go with our register size, and to add compile time
checks on the bit numbers.
The intention is that these are easier to get right and review against
the spec than hand rolled masks.
Convert power sequencer registers as an example.
v3:
- rename macros to REG_BIT() and REG_GENMASK() to avoid underscore
prefix and to be in line with kernel macros (Chris)
- add compile time checks (Mika)
v2:
- rename macros to just _BIT() and _MASK() to reduce verbosity
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 | 94 +++++++++++++++++++++------------
1 file changed, 61 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c9b482bc6433..e847a18067bc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -25,6 +25,8 @@
#ifndef _I915_REG_H_
#define _I915_REG_H_
+#include <linux/bits.h>
+
/**
* DOC: The i915 register macro definition style guide
*
@@ -59,15 +61,13 @@
* significant to least significant bit. Indent the register content macros
* using two extra spaces between ``#define`` and the macro name.
*
- * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. 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.
+ * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Use
+ * ``REG_GENMASK()`` to define _MASK. 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 bits using ``(1 << N)`` instead of ``BIT(N)``. We may change this in
- * the future, but this is the prevailing style. Do **not** add ``_BIT`` suffix
- * to the name.
+ * Define bits using ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
*
* Group the register and its contents together without blank lines, separate
* from other registers and their contents with one blank line.
@@ -105,8 +105,8 @@
* #define _FOO_A 0xf000
* #define _FOO_B 0xf001
* #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
- * #define FOO_ENABLE (1 << 31)
- * #define FOO_MODE_MASK (0xf << 16)
+ * #define FOO_ENABLE REG_BIT(31)
+ * #define FOO_MODE_MASK REG_GENMASK(19, 16)
* #define FOO_MODE_SHIFT 16
* #define FOO_MODE_BAR (0 << 16)
* #define FOO_MODE_BAZ (1 << 16)
@@ -116,6 +116,34 @@
* #define GEN8_BAR _MMIO(0xb888)
*/
+/**
+ * REG_BIT() - Prepare a u32 bit value
+ * @__n: 0-based bit number
+ *
+ * Local wrapper for BIT() to force u32, with compile time checks.
+ *
+ * @return: Value with bit @__n set.
+ */
+#define REG_BIT(__n) \
+ ((u32)(BIT(__n) + \
+ BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) && \
+ ((__n) < 0 || (__n) > 31))))
+
+/**
+ * REG_GENMASK() - Prepare a continuous u32 bitmask
+ * @__high: 0-based high bit
+ * @__low: 0-based low bit
+ *
+ * Local wrapper for GENMASK() to force u32, with compile time checks.
+ *
+ * @return: Continuous bitmask from @__high to @__low, inclusive.
+ */
+#define REG_GENMASK(__high, __low) \
+ ((u32)(GENMASK(__high, __low) + \
+ BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
+ __builtin_constant_p(__low) && \
+ ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
+
typedef struct {
u32 reg;
} i915_reg_t;
@@ -4691,18 +4719,18 @@ enum {
#define _PP_STATUS 0x61200
#define PP_STATUS(pps_idx) _MMIO_PPS(pps_idx, _PP_STATUS)
-#define PP_ON (1 << 31)
+#define PP_ON REG_BIT(31)
#define _PP_CONTROL_1 0xc7204
#define _PP_CONTROL_2 0xc7304
#define ICP_PP_CONTROL(x) _MMIO(((x) == 1) ? _PP_CONTROL_1 : \
_PP_CONTROL_2)
-#define POWER_CYCLE_DELAY_MASK (0x1f << 4)
+#define POWER_CYCLE_DELAY_MASK REG_GENMASK(8, 4)
#define POWER_CYCLE_DELAY_SHIFT 4
-#define VDD_OVERRIDE_FORCE (1 << 3)
-#define BACKLIGHT_ENABLE (1 << 2)
-#define PWR_DOWN_ON_RESET (1 << 1)
-#define PWR_STATE_TARGET (1 << 0)
+#define VDD_OVERRIDE_FORCE REG_BIT(3)
+#define BACKLIGHT_ENABLE REG_BIT(2)
+#define PWR_DOWN_ON_RESET REG_BIT(1)
+#define PWR_STATE_TARGET REG_BIT(0)
/*
* Indicates that all dependencies of the panel are on:
*
@@ -4710,14 +4738,14 @@ enum {
* - pipe enabled
* - LVDS/DVOB/DVOC on
*/
-#define PP_READY (1 << 30)
+#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_MASK (3 << 28)
#define PP_SEQUENCE_SHIFT 28
-#define PP_CYCLE_DELAY_ACTIVE (1 << 27)
-#define PP_SEQUENCE_STATE_MASK 0x0000000f
+#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)
@@ -4730,41 +4758,41 @@ enum {
#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_MASK (0xffff << 16)
-#define BXT_POWER_CYCLE_DELAY_MASK 0x1f0
+#define BXT_POWER_CYCLE_DELAY_MASK REG_GENMASK(8, 4)
#define BXT_POWER_CYCLE_DELAY_SHIFT 4
-#define EDP_FORCE_VDD (1 << 3)
-#define EDP_BLC_ENABLE (1 << 2)
-#define PANEL_POWER_RESET (1 << 1)
-#define PANEL_POWER_ON (1 << 0)
+#define EDP_FORCE_VDD REG_BIT(3)
+#define EDP_BLC_ENABLE REG_BIT(2)
+#define PANEL_POWER_RESET REG_BIT(1)
+#define PANEL_POWER_ON REG_BIT(0)
#define _PP_ON_DELAYS 0x61208
#define PP_ON_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
#define PANEL_PORT_SELECT_SHIFT 30
-#define PANEL_PORT_SELECT_MASK (3 << 30)
+#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_POWER_UP_DELAY_MASK 0x1fff0000
+#define PANEL_POWER_UP_DELAY_MASK REG_GENMASK(28, 16)
#define PANEL_POWER_UP_DELAY_SHIFT 16
-#define PANEL_LIGHT_ON_DELAY_MASK 0x1fff
+#define PANEL_LIGHT_ON_DELAY_MASK REG_GENMASK(12, 0)
#define PANEL_LIGHT_ON_DELAY_SHIFT 0
#define _PP_OFF_DELAYS 0x6120C
#define PP_OFF_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_OFF_DELAYS)
-#define PANEL_POWER_DOWN_DELAY_MASK 0x1fff0000
+#define PANEL_POWER_DOWN_DELAY_MASK REG_GENMASK(28, 16)
#define PANEL_POWER_DOWN_DELAY_SHIFT 16
-#define PANEL_LIGHT_OFF_DELAY_MASK 0x1fff
+#define PANEL_LIGHT_OFF_DELAY_MASK REG_GENMASK(12, 0)
#define PANEL_LIGHT_OFF_DELAY_SHIFT 0
#define _PP_DIVISOR 0x61210
#define PP_DIVISOR(pps_idx) _MMIO_PPS(pps_idx, _PP_DIVISOR)
-#define PP_REFERENCE_DIVIDER_MASK 0xffffff00
+#define PP_REFERENCE_DIVIDER_MASK REG_GENMASK(31, 8)
#define PP_REFERENCE_DIVIDER_SHIFT 8
-#define PANEL_POWER_CYCLE_DELAY_MASK 0x1f
+#define PANEL_POWER_CYCLE_DELAY_MASK REG_GENMASK(4, 0)
#define PANEL_POWER_CYCLE_DELAY_SHIFT 0
/* Panel fitting */
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
2019-02-27 17:02 ` [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() " Jani Nikula
@ 2019-02-27 17:02 ` Jani Nikula
2019-02-27 20:58 ` Chris Wilson
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Jani Nikula @ 2019-02-27 17:02 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
bitfield.h defines FIELD_GET() and FIELD_PREP() macros to access
bitfields using the mask alone, with no need for separate shift. Indeed,
the shift is redundant.
We define REG_FIELD_GET() and REG_FIELD_PREP() wrappers for the above,
in part to force u32 and for consistency with REG_BIT() and
REG_GENMASK(), but also as we'll need to redefine REG_FIELD_PREP() in
follow-up work to make it produce integer constant expressions.
For the most part, REG_FIELD_GET() is shorter than masking followed by
shift, and arguably has more clarity.
REG_FIELD_PREP() can get more verbose than simply shifting in place, but
it does provide masking to ensure we don't overflow the mask, something
we usually don't bother with currently.
Convert power sequencer registers as an example.
v2:
- Add the REG_FIELD_GET() and REG_FIELD_PREP() wrappers to use them
consistently from the start.
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 | 45 ++++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_dp.c | 40 +++++++++++----------------
drivers/gpu/drm/i915/intel_lvds.c | 40 +++++++++++++--------------
3 files changed, 64 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e847a18067bc..1bd75770483a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -25,6 +25,7 @@
#ifndef _I915_REG_H_
#define _I915_REG_H_
+#include <linux/bitfield.h>
#include <linux/bits.h>
/**
@@ -61,11 +62,11 @@
* significant to least significant bit. Indent the register content macros
* using two extra spaces between ``#define`` and the macro name.
*
- * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Use
- * ``REG_GENMASK()`` to define _MASK. 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 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 bits using ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
*
@@ -107,7 +108,6 @@
* #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_SHIFT 16
* #define FOO_MODE_BAR (0 << 16)
* #define FOO_MODE_BAZ (1 << 16)
* #define FOO_MODE_QUX_SNB (2 << 16)
@@ -144,6 +144,30 @@
__builtin_constant_p(__low) && \
((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
+/**
+ * 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().
+ *
+ * @return: @__val masked and shifted into the field defined by @__mask.
+ */
+#define REG_FIELD_PREP(__mask, __val) ((u32)FIELD_PREP(__mask, __val))
+
+/**
+ * REG_FIELD_GET() - Extract a u32 bitfield value
+ * @__mask: shifted mask defining the field's length and position
+ * @__val: value to extract the bitfield value from
+ *
+ * Local wrapper for FIELD_GET() to force u32 and for consistency with
+ * REG_FIELD_PREP(), REG_BIT() and REG_GENMASK().
+ *
+ * @return: Masked and shifted value of the field defined by @__mask in @__val.
+ */
+#define REG_FIELD_GET(__mask, __val) ((u32)FIELD_GET(__mask, __val))
+
typedef struct {
u32 reg;
} i915_reg_t;
@@ -4726,7 +4750,6 @@ enum {
#define ICP_PP_CONTROL(x) _MMIO(((x) == 1) ? _PP_CONTROL_1 : \
_PP_CONTROL_2)
#define POWER_CYCLE_DELAY_MASK REG_GENMASK(8, 4)
-#define POWER_CYCLE_DELAY_SHIFT 4
#define VDD_OVERRIDE_FORCE REG_BIT(3)
#define BACKLIGHT_ENABLE REG_BIT(2)
#define PWR_DOWN_ON_RESET REG_BIT(1)
@@ -4743,7 +4766,6 @@ enum {
#define PP_SEQUENCE_NONE (0 << 28)
#define PP_SEQUENCE_POWER_UP (1 << 28)
#define PP_SEQUENCE_POWER_DOWN (2 << 28)
-#define PP_SEQUENCE_SHIFT 28
#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)
@@ -4769,7 +4791,6 @@ enum {
#define _PP_ON_DELAYS 0x61208
#define PP_ON_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
-#define PANEL_PORT_SELECT_SHIFT 30
#define PANEL_PORT_SELECT_MASK REG_GENMASK(31, 30)
#define PANEL_PORT_SELECT_LVDS (0 << 30)
#define PANEL_PORT_SELECT_DPA (1 << 30)
@@ -4777,23 +4798,17 @@ enum {
#define PANEL_PORT_SELECT_DPD (3 << 30)
#define PANEL_PORT_SELECT_VLV(port) ((port) << 30)
#define PANEL_POWER_UP_DELAY_MASK REG_GENMASK(28, 16)
-#define PANEL_POWER_UP_DELAY_SHIFT 16
#define PANEL_LIGHT_ON_DELAY_MASK REG_GENMASK(12, 0)
-#define PANEL_LIGHT_ON_DELAY_SHIFT 0
#define _PP_OFF_DELAYS 0x6120C
#define PP_OFF_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_OFF_DELAYS)
#define PANEL_POWER_DOWN_DELAY_MASK REG_GENMASK(28, 16)
-#define PANEL_POWER_DOWN_DELAY_SHIFT 16
#define PANEL_LIGHT_OFF_DELAY_MASK REG_GENMASK(12, 0)
-#define PANEL_LIGHT_OFF_DELAY_SHIFT 0
#define _PP_DIVISOR 0x61210
#define PP_DIVISOR(pps_idx) _MMIO_PPS(pps_idx, _PP_DIVISOR)
#define PP_REFERENCE_DIVIDER_MASK REG_GENMASK(31, 8)
-#define PP_REFERENCE_DIVIDER_SHIFT 8
#define PANEL_POWER_CYCLE_DELAY_MASK REG_GENMASK(4, 0)
-#define PANEL_POWER_CYCLE_DELAY_SHIFT 0
/* Panel fitting */
#define PFIT_CONTROL _MMIO(DISPLAY_MMIO_BASE(dev_priv) + 0x61230)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e1a051c0fbfe..d1d282bc4814 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6438,25 +6438,16 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
}
/* Pull timing values out of registers */
- seq->t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
- PANEL_POWER_UP_DELAY_SHIFT;
-
- seq->t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
- PANEL_LIGHT_ON_DELAY_SHIFT;
-
- seq->t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
- PANEL_LIGHT_OFF_DELAY_SHIFT;
-
- seq->t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
- PANEL_POWER_DOWN_DELAY_SHIFT;
+ seq->t1_t3 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, pp_on);
+ seq->t8 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, pp_on);
+ seq->t9 = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, pp_off);
+ seq->t10 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, pp_off);
if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
HAS_PCH_ICP(dev_priv)) {
- seq->t11_t12 = ((pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
- BXT_POWER_CYCLE_DELAY_SHIFT) * 1000;
+ seq->t11_t12 = REG_FIELD_GET(BXT_POWER_CYCLE_DELAY_MASK, pp_ctl) * 1000;
} else {
- seq->t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
- PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
+ seq->t11_t12 = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, pp_div) * 1000;
}
}
@@ -6616,22 +6607,23 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
I915_WRITE(regs.pp_ctrl, pp);
}
- pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
- (seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
- pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
- (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
+ pp_on = REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, seq->t1_t3) |
+ REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, seq->t8);
+ pp_off = REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, seq->t9) |
+ REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, seq->t10);
/* Compute the divisor for the pp clock, simply match the Bspec
* formula. */
if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
HAS_PCH_ICP(dev_priv)) {
pp_div = I915_READ(regs.pp_ctrl);
pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
- pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
- << BXT_POWER_CYCLE_DELAY_SHIFT);
+ pp_div |= REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK,
+ DIV_ROUND_UP(seq->t11_t12, 1000));
} else {
- pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
- pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
- << PANEL_POWER_CYCLE_DELAY_SHIFT);
+ pp_div = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK,
+ (100 * div) / 2 - 1);
+ pp_div |= REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
+ DIV_ROUND_UP(seq->t11_t12, 1000));
}
/* Haswell doesn't have any port selection bits for the panel
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index b4aa49768e90..646fc29e159a 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -152,24 +152,17 @@ static void intel_lvds_pps_get_hw_state(struct drm_i915_private *dev_priv,
pps->powerdown_on_reset = I915_READ(PP_CONTROL(0)) & PANEL_POWER_RESET;
val = I915_READ(PP_ON_DELAYS(0));
- pps->port = (val & PANEL_PORT_SELECT_MASK) >>
- PANEL_PORT_SELECT_SHIFT;
- pps->t1_t2 = (val & PANEL_POWER_UP_DELAY_MASK) >>
- PANEL_POWER_UP_DELAY_SHIFT;
- pps->t5 = (val & PANEL_LIGHT_ON_DELAY_MASK) >>
- PANEL_LIGHT_ON_DELAY_SHIFT;
+ pps->port = REG_FIELD_GET(PANEL_PORT_SELECT_MASK, val);
+ pps->t1_t2 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, val);
+ pps->t5 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, val);
val = I915_READ(PP_OFF_DELAYS(0));
- pps->t3 = (val & PANEL_POWER_DOWN_DELAY_MASK) >>
- PANEL_POWER_DOWN_DELAY_SHIFT;
- pps->tx = (val & PANEL_LIGHT_OFF_DELAY_MASK) >>
- PANEL_LIGHT_OFF_DELAY_SHIFT;
+ pps->t3 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, val);
+ pps->tx = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, val);
val = I915_READ(PP_DIVISOR(0));
- pps->divider = (val & PP_REFERENCE_DIVIDER_MASK) >>
- PP_REFERENCE_DIVIDER_SHIFT;
- val = (val & PANEL_POWER_CYCLE_DELAY_MASK) >>
- PANEL_POWER_CYCLE_DELAY_SHIFT;
+ pps->divider = REG_FIELD_GET(PP_REFERENCE_DIVIDER_MASK, val);
+ val = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, val);
/*
* Remove the BSpec specified +1 (100ms) offset that accounts for a
* too short power-cycle delay due to the asynchronous programming of
@@ -209,15 +202,18 @@ static void intel_lvds_pps_init_hw(struct drm_i915_private *dev_priv,
val |= PANEL_POWER_RESET;
I915_WRITE(PP_CONTROL(0), val);
- I915_WRITE(PP_ON_DELAYS(0), (pps->port << PANEL_PORT_SELECT_SHIFT) |
- (pps->t1_t2 << PANEL_POWER_UP_DELAY_SHIFT) |
- (pps->t5 << PANEL_LIGHT_ON_DELAY_SHIFT));
- I915_WRITE(PP_OFF_DELAYS(0), (pps->t3 << PANEL_POWER_DOWN_DELAY_SHIFT) |
- (pps->tx << PANEL_LIGHT_OFF_DELAY_SHIFT));
+ val = REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, pps->port) |
+ REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, pps->t1_t2) |
+ REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, pps->t5);
+ I915_WRITE(PP_ON_DELAYS(0), val);
- val = pps->divider << PP_REFERENCE_DIVIDER_SHIFT;
- val |= (DIV_ROUND_UP(pps->t4, 1000) + 1) <<
- PANEL_POWER_CYCLE_DELAY_SHIFT;
+ val = REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, pps->t3) |
+ REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, pps->tx);
+ I915_WRITE(PP_OFF_DELAYS(0), val);
+
+ val = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, pps->divider) |
+ REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
+ DIV_ROUND_UP(pps->t4, 1000) + 1);
I915_WRITE(PP_DIVISOR(0), val);
}
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
2019-02-27 17:02 ` [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() " Jani Nikula
2019-02-27 17:02 ` [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors Jani Nikula
@ 2019-02-27 17:02 ` Jani Nikula
2019-02-27 21:07 ` Chris Wilson
` (3 more replies)
2019-02-27 17:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: introduce macros to define register contents (rev3) Patchwork
` (3 subsequent siblings)
6 siblings, 4 replies; 22+ messages in thread
From: Jani Nikula @ 2019-02-27 17:02 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
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))
+
/**
* 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)
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: introduce macros to define register contents (rev3)
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
` (2 preceding siblings ...)
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
@ 2019-02-27 17:44 ` Patchwork
2019-02-27 17:46 ` ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-02-27 17:44 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: drm/i915: introduce macros to define register contents (rev3)
URL : https://patchwork.freedesktop.org/series/50513/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
918ed22b15ea drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
-:91: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__n' - possible side-effects?
#91: FILE: drivers/gpu/drm/i915/i915_reg.h:127:
+#define REG_BIT(__n) \
+ ((u32)(BIT(__n) + \
+ BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) && \
+ ((__n) < 0 || (__n) > 31))))
-:105: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__high' - possible side-effects?
#105: FILE: drivers/gpu/drm/i915/i915_reg.h:141:
+#define REG_GENMASK(__high, __low) \
+ ((u32)(GENMASK(__high, __low) + \
+ BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
+ __builtin_constant_p(__low) && \
+ ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
-:105: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__low' - possible side-effects?
#105: FILE: drivers/gpu/drm/i915/i915_reg.h:141:
+#define REG_GENMASK(__high, __low) \
+ ((u32)(GENMASK(__high, __low) + \
+ BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
+ __builtin_constant_p(__low) && \
+ ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
total: 0 errors, 0 warnings, 3 checks, 169 lines checked
1e626607f4fd drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
81637c300f08 drm/i915: use REG_FIELD_PREP() to define register bitfield values
-:73: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__x' - possible side-effects?
#73: FILE: drivers/gpu/drm/i915/i915_reg.h:150:
+#define IS_POWER_OF_2(__x) ((__x) && (((__x) & ((__x) - 1)) == 0))
-:88: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__mask' - possible side-effects?
#88: FILE: drivers/gpu/drm/i915/i915_reg.h:162:
+#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))))
-:88: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__val' - possible side-effects?
#88: FILE: drivers/gpu/drm/i915/i915_reg.h:162:
+#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))))
-:93: WARNING:LONG_LINE: line over 100 characters
#93: FILE: drivers/gpu/drm/i915/i915_reg.h:167:
+ BUILD_BUG_ON_ZERO(__builtin_choose_expr(__builtin_constant_p(__val), (~((__mask) >> __bf_shf(__mask)) & (__val)), 0))))
total: 0 errors, 1 warnings, 3 checks, 114 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: introduce macros to define register contents (rev3)
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
` (3 preceding siblings ...)
2019-02-27 17:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: introduce macros to define register contents (rev3) Patchwork
@ 2019-02-27 17:46 ` Patchwork
2019-02-27 18:20 ` ✓ Fi.CI.BAT: success " Patchwork
2019-02-27 20:13 ` ✓ Fi.CI.IGT: " Patchwork
6 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-02-27 17:46 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: drm/i915: introduce macros to define register contents (rev3)
URL : https://patchwork.freedesktop.org/series/50513/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
Okay!
Commit: drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
Okay!
Commit: drm/i915: use REG_FIELD_PREP() to define register bitfield values
+drivers/gpu/drm/i915/intel_display.c:1165:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/intel_display.c:1168:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/intel_display.c:1171:22: error: Expected constant expression in case statement
+drivers/gpu/drm/i915/intel_display.c:1174:22: error: Expected constant expression in case statement
-drivers/gpu/drm/i915/intel_display.c:2404:13: warning: call with no type!
-./include/linux/reservation.h:220:20: warning: dereference of noderef expression
-./include/linux/reservation.h:220:20: warning: dereference of noderef expression
-./include/linux/reservation.h:220:20: warning: dereference of noderef expression
-./include/linux/reservation.h:220:45: warning: dereference of noderef expression
-./include/linux/reservation.h:220:45: warning: dereference of noderef expression
-./include/linux/reservation.h:220:45: warning: dereference of noderef expression
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: introduce macros to define register contents (rev3)
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
` (4 preceding siblings ...)
2019-02-27 17:46 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2019-02-27 18:20 ` Patchwork
2019-02-27 20:13 ` ✓ Fi.CI.IGT: " Patchwork
6 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-02-27 18:20 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: drm/i915: introduce macros to define register contents (rev3)
URL : https://patchwork.freedesktop.org/series/50513/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5666 -> Patchwork_12317
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/50513/revisions/3/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_12317:
### IGT changes ###
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_selftest@live_gtt:
- {fi-icl-y}: PASS -> INCOMPLETE
Known issues
------------
Here are the changes found in Patchwork_12317 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_contexts:
- fi-icl-u2: NOTRUN -> DMESG-FAIL [fdo#108569]
* igt@kms_busy@basic-flip-a:
- fi-kbl-7567u: PASS -> SKIP [fdo#109271] / [fdo#109278] +2
* igt@kms_busy@basic-flip-b:
- fi-gdg-551: PASS -> FAIL [fdo#103182]
* igt@kms_psr@primary_mmap_gtt:
- fi-blb-e6850: NOTRUN -> SKIP [fdo#109271] +27
#### Possible fixes ####
* igt@i915_selftest@live_requests:
- fi-icl-u2: INCOMPLETE [fdo#109644] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS
* igt@prime_vgem@basic-fence-flip:
- fi-gdg-551: FAIL [fdo#103182] -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109644]: https://bugs.freedesktop.org/show_bug.cgi?id=109644
Participating hosts (44 -> 38)
------------------------------
Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510
Build changes
-------------
* Linux: CI_DRM_5666 -> Patchwork_12317
CI_DRM_5666: 358ab8acaabef3cef2a7ce9e5dd7c4196a0c30fc @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4860: b79007f9c575a538a63ce9301a890ed9e1a45f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12317: 81637c300f08431f584763347f3e167fe89dfc44 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
81637c300f08 drm/i915: use REG_FIELD_PREP() to define register bitfield values
1e626607f4fd drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
918ed22b15ea drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12317/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: introduce macros to define register contents (rev3)
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
` (5 preceding siblings ...)
2019-02-27 18:20 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-02-27 20:13 ` Patchwork
6 siblings, 0 replies; 22+ messages in thread
From: Patchwork @ 2019-02-27 20:13 UTC (permalink / raw)
To: intel-gfx
== Series Details ==
Series: drm/i915: introduce macros to define register contents (rev3)
URL : https://patchwork.freedesktop.org/series/50513/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5666_full -> Patchwork_12317_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_12317_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_bad_reloc@negative-reloc-bsd2:
- shard-iclb: NOTRUN -> SKIP [fdo#109276] +7
* igt@gem_exec_params@no-vebox:
- shard-iclb: NOTRUN -> SKIP [fdo#109283]
* igt@gem_exec_suspend@basic-s3:
- shard-skl: PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]
* igt@gem_pwrite@huge-gtt-forwards:
- shard-iclb: NOTRUN -> SKIP [fdo#109290]
* igt@gem_unref_active_buffers:
- shard-apl: PASS -> INCOMPLETE [fdo#103927]
* igt@i915_pm_rpm@cursor-dpms:
- shard-iclb: PASS -> DMESG-WARN [fdo#107724] +4
* igt@i915_pm_rpm@modeset-lpsp-stress:
- shard-iclb: PASS -> INCOMPLETE [fdo#108840]
* igt@i915_query@query-topology-known-pci-ids:
- shard-iclb: NOTRUN -> SKIP [fdo#109303]
* igt@kms_busy@extended-modeset-hang-newfb-render-b:
- shard-skl: PASS -> DMESG-WARN [fdo#107956]
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
- shard-hsw: PASS -> DMESG-WARN [fdo#107956]
* igt@kms_busy@extended-modeset-hang-oldfb-render-d:
- shard-skl: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +5
* igt@kms_busy@extended-modeset-hang-oldfb-render-e:
- shard-kbl: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +4
* igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
- shard-iclb: NOTRUN -> DMESG-WARN [fdo#107956]
- shard-apl: PASS -> DMESG-WARN [fdo#107956]
* igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
- shard-iclb: NOTRUN -> FAIL [fdo#107725] +1
* igt@kms_ccs@pipe-c-missing-ccs-buffer:
- shard-apl: NOTRUN -> SKIP [fdo#109271] +21
* igt@kms_chamelium@hdmi-edid-read:
- shard-iclb: NOTRUN -> SKIP [fdo#109284] +1
* igt@kms_color@pipe-b-degamma:
- shard-apl: PASS -> FAIL [fdo#104782]
* igt@kms_concurrent@pipe-f:
- shard-iclb: NOTRUN -> SKIP [fdo#109278] +1
* igt@kms_cursor_crc@cursor-128x128-dpms:
- shard-iclb: NOTRUN -> FAIL [fdo#103232]
* igt@kms_cursor_crc@cursor-256x85-sliding:
- shard-apl: NOTRUN -> FAIL [fdo#103232] +1
* igt@kms_cursor_crc@cursor-64x64-suspend:
- shard-skl: PASS -> FAIL [fdo#103191] / [fdo#103232]
* igt@kms_cursor_legacy@cursora-vs-flipb-legacy:
- shard-iclb: NOTRUN -> SKIP [fdo#109274] +1
* igt@kms_fbcon_fbt@fbc:
- shard-iclb: PASS -> DMESG-WARN [fdo#109593]
* igt@kms_fbcon_fbt@fbc-suspend:
- shard-iclb: PASS -> INCOMPLETE [fdo#107713]
* igt@kms_flip@2x-plain-flip-ts-check-interruptible:
- shard-glk: PASS -> FAIL [fdo#100368]
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-skl: PASS -> FAIL [fdo#105363]
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
- shard-glk: PASS -> FAIL [fdo#103167] +1
* igt@kms_frontbuffer_tracking@fbc-badstride:
- shard-skl: PASS -> FAIL [fdo#105682] +1
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-mmap-wc:
- shard-iclb: PASS -> FAIL [fdo#103167] +3
* igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-mmap-gtt:
- shard-iclb: NOTRUN -> SKIP [fdo#109280] +8
* igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-wc:
- shard-kbl: NOTRUN -> SKIP [fdo#109271] +59
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-pgflip-blt:
- shard-skl: PASS -> FAIL [fdo#103167] +1
* igt@kms_frontbuffer_tracking@psr-2p-scndscrn-cur-indfb-move:
- shard-skl: NOTRUN -> SKIP [fdo#109271] +68
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
- shard-skl: PASS -> FAIL [fdo#103166] +1
* igt@kms_plane@plane-position-covered-pipe-b-planes:
- shard-iclb: PASS -> FAIL [fdo#103166] +1
* igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
- shard-skl: NOTRUN -> FAIL [fdo#108145] +2
* igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
- shard-skl: PASS -> FAIL [fdo#107815] / [fdo#108145]
* igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
- shard-kbl: NOTRUN -> FAIL [fdo#108145] / [fdo#108590]
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
- shard-kbl: NOTRUN -> FAIL [fdo#108145]
* igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
- shard-apl: PASS -> FAIL [fdo#103166]
* igt@kms_psr@psr2_primary_mmap_cpu:
- shard-iclb: NOTRUN -> SKIP [fdo#109441] +1
* igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-snb: PASS -> SKIP [fdo#109271]
* igt@perf@short-reads:
- shard-skl: NOTRUN -> FAIL [fdo#103183]
* igt@prime_udl:
- shard-iclb: NOTRUN -> SKIP [fdo#109291] +1
* igt@runner@aborted:
- shard-iclb: NOTRUN -> FAIL [fdo#109593]
#### Possible fixes ####
* igt@gem_ctx_isolation@rcs0-s3:
- shard-apl: INCOMPLETE [fdo#103927] -> PASS
* igt@gem_exec_suspend@basic-s3:
- shard-iclb: INCOMPLETE [fdo#107713] -> PASS
* igt@i915_pm_rpm@basic-rte:
- shard-iclb: DMESG-WARN [fdo#107724] -> PASS +2
* igt@kms_cursor_crc@cursor-128x42-random:
- shard-apl: FAIL [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-64x21-offscreen:
- shard-skl: FAIL [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-size-change:
- shard-glk: FAIL [fdo#103232] -> PASS
* igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
- shard-glk: FAIL [fdo#104873] -> PASS
* igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-ytiled:
- shard-iclb: FAIL [fdo#103184] -> PASS
* igt@kms_flip@flip-vs-suspend:
- shard-skl: INCOMPLETE [fdo#107773] / [fdo#109507] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
- shard-apl: FAIL [fdo#103167] -> PASS +1
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
- shard-glk: FAIL [fdo#103167] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-rte:
- shard-apl: FAIL [fdo#103167] / [fdo#105682] -> PASS
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-blt:
- shard-skl: FAIL [fdo#105682] -> PASS +2
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-indfb-draw-mmap-cpu:
- shard-skl: FAIL [fdo#103167] -> PASS +2
* igt@kms_frontbuffer_tracking@fbcpsr-tilingchange:
- shard-iclb: FAIL -> PASS +7
* igt@kms_plane@plane-panning-bottom-right-pipe-a-planes:
- shard-skl: FAIL [fdo#103166] -> PASS
* igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
- shard-glk: FAIL [fdo#103166] -> PASS +1
* igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
- shard-iclb: FAIL [fdo#103166] -> PASS +1
- shard-apl: FAIL [fdo#103166] -> PASS
* igt@kms_psr@sprite_blt:
- shard-iclb: FAIL [fdo#107383] -> PASS
* igt@kms_rotation_crc@multiplane-rotation:
- shard-kbl: DMESG-FAIL [fdo#105763] -> PASS
* igt@kms_setmode@basic:
- shard-glk: FAIL [fdo#99912] -> PASS
* igt@tools_test@tools_test:
- shard-hsw: SKIP [fdo#109271] -> PASS
- shard-kbl: SKIP [fdo#109271] -> PASS
#### Warnings ####
* igt@kms_cursor_crc@cursor-256x256-suspend:
- shard-iclb: INCOMPLETE [fdo#107713] -> FAIL [fdo#103232]
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103183]: https://bugs.freedesktop.org/show_bug.cgi?id=103183
[fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
[fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
[fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
[fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
[fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
[fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
[fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
[fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
[fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
[fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
[fdo#109290]: https://bugs.freedesktop.org/show_bug.cgi?id=109290
[fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
[fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
[fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (7 -> 7)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_5666 -> Patchwork_12317
CI_DRM_5666: 358ab8acaabef3cef2a7ce9e5dd7c4196a0c30fc @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4860: b79007f9c575a538a63ce9301a890ed9e1a45f35 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_12317: 81637c300f08431f584763347f3e167fe89dfc44 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12317/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-27 17:02 ` [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() " Jani Nikula
@ 2019-02-27 20:50 ` Chris Wilson
2019-02-27 21:13 ` Ville Syrjälä
2019-02-27 23:06 ` Michal Wajdeczko
1 sibling, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2019-02-27 20:50 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
Quoting Jani Nikula (2019-02-27 17:02:36)
> Introduce REG_BIT(n) to define register bits and REG_GENMASK(h, l) to
> define register bitfield masks.
>
> We define the above as wrappers to BIT() and GENMASK() respectively to
> force u32 type to go with our register size, and to add compile time
> checks on the bit numbers.
>
> The intention is that these are easier to get right and review against
> the spec than hand rolled masks.
>
> Convert power sequencer registers as an example.
>
> v3:
> - rename macros to REG_BIT() and REG_GENMASK() to avoid underscore
> prefix and to be in line with kernel macros (Chris)
> - add compile time checks (Mika)
>
> v2:
> - rename macros to just _BIT() and _MASK() to reduce verbosity
>
> 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 | 94 +++++++++++++++++++++------------
> 1 file changed, 61 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c9b482bc6433..e847a18067bc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -25,6 +25,8 @@
> #ifndef _I915_REG_H_
> #define _I915_REG_H_
>
> +#include <linux/bits.h>
> +
> /**
> * DOC: The i915 register macro definition style guide
> *
> @@ -59,15 +61,13 @@
> * significant to least significant bit. Indent the register content macros
> * using two extra spaces between ``#define`` and the macro name.
> *
> - * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. 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.
> + * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Use
> + * ``REG_GENMASK()`` to define _MASK. 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 bits using ``(1 << N)`` instead of ``BIT(N)``. We may change this in
> - * the future, but this is the prevailing style. Do **not** add ``_BIT`` suffix
> - * to the name.
> + * Define bits using ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
> *
> * Group the register and its contents together without blank lines, separate
> * from other registers and their contents with one blank line.
> @@ -105,8 +105,8 @@
> * #define _FOO_A 0xf000
> * #define _FOO_B 0xf001
> * #define FOO(pipe) _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
> - * #define FOO_ENABLE (1 << 31)
> - * #define FOO_MODE_MASK (0xf << 16)
> + * #define FOO_ENABLE REG_BIT(31)
> + * #define FOO_MODE_MASK REG_GENMASK(19, 16)
> * #define FOO_MODE_SHIFT 16
> * #define FOO_MODE_BAR (0 << 16)
> * #define FOO_MODE_BAZ (1 << 16)
> @@ -116,6 +116,34 @@
> * #define GEN8_BAR _MMIO(0xb888)
> */
>
> +/**
> + * REG_BIT() - Prepare a u32 bit value
> + * @__n: 0-based bit number
> + *
> + * Local wrapper for BIT() to force u32, with compile time checks.
> + *
> + * @return: Value with bit @__n set.
> + */
> +#define REG_BIT(__n) \
> + ((u32)(BIT(__n) + \
> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) && \
> + ((__n) < 0 || (__n) > 31))))
> +
> +/**
> + * REG_GENMASK() - Prepare a continuous u32 bitmask
> + * @__high: 0-based high bit
> + * @__low: 0-based low bit
> + *
> + * Local wrapper for GENMASK() to force u32, with compile time checks.
> + *
> + * @return: Continuous bitmask from @__high to @__low, inclusive.
> + */
> +#define REG_GENMASK(__high, __low) \
> + ((u32)(GENMASK(__high, __low) + \
> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
> + __builtin_constant_p(__low) && \
> + ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
> +
> typedef struct {
> u32 reg;
> } i915_reg_t;
> @@ -4691,18 +4719,18 @@ enum {
>
> #define _PP_STATUS 0x61200
> #define PP_STATUS(pps_idx) _MMIO_PPS(pps_idx, _PP_STATUS)
> -#define PP_ON (1 << 31)
> +#define PP_ON REG_BIT(31)
Ok.
>
> #define _PP_CONTROL_1 0xc7204
> #define _PP_CONTROL_2 0xc7304
> #define ICP_PP_CONTROL(x) _MMIO(((x) == 1) ? _PP_CONTROL_1 : \
> _PP_CONTROL_2)
> -#define POWER_CYCLE_DELAY_MASK (0x1f << 4)
> +#define POWER_CYCLE_DELAY_MASK REG_GENMASK(8, 4)
Ok.
> #define POWER_CYCLE_DELAY_SHIFT 4
> -#define VDD_OVERRIDE_FORCE (1 << 3)
> -#define BACKLIGHT_ENABLE (1 << 2)
> -#define PWR_DOWN_ON_RESET (1 << 1)
> -#define PWR_STATE_TARGET (1 << 0)
> +#define VDD_OVERRIDE_FORCE REG_BIT(3)
> +#define BACKLIGHT_ENABLE REG_BIT(2)
> +#define PWR_DOWN_ON_RESET REG_BIT(1)
> +#define PWR_STATE_TARGET REG_BIT(0)
Ok.
> /*
> * Indicates that all dependencies of the panel are on:
> *
> @@ -4710,14 +4738,14 @@ enum {
> * - pipe enabled
> * - LVDS/DVOB/DVOC on
> */
> -#define PP_READY (1 << 30)
> +#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_MASK (3 << 28)
> #define PP_SEQUENCE_SHIFT 28
> -#define PP_CYCLE_DELAY_ACTIVE (1 << 27)
> -#define PP_SEQUENCE_STATE_MASK 0x0000000f
> +#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)
Ok.
> @@ -4730,41 +4758,41 @@ enum {
>
> #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_MASK (0xffff << 16)
> -#define BXT_POWER_CYCLE_DELAY_MASK 0x1f0
> +#define BXT_POWER_CYCLE_DELAY_MASK REG_GENMASK(8, 4)
Score one for consistency.
> #define BXT_POWER_CYCLE_DELAY_SHIFT 4
> -#define EDP_FORCE_VDD (1 << 3)
> -#define EDP_BLC_ENABLE (1 << 2)
> -#define PANEL_POWER_RESET (1 << 1)
> -#define PANEL_POWER_ON (1 << 0)
> +#define EDP_FORCE_VDD REG_BIT(3)
> +#define EDP_BLC_ENABLE REG_BIT(2)
> +#define PANEL_POWER_RESET REG_BIT(1)
> +#define PANEL_POWER_ON REG_BIT(0)
Ok.
> #define _PP_ON_DELAYS 0x61208
> #define PP_ON_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
> #define PANEL_PORT_SELECT_SHIFT 30
> -#define PANEL_PORT_SELECT_MASK (3 << 30)
> +#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_POWER_UP_DELAY_MASK 0x1fff0000
> +#define PANEL_POWER_UP_DELAY_MASK REG_GENMASK(28, 16)
Ok.
> #define PANEL_POWER_UP_DELAY_SHIFT 16
> -#define PANEL_LIGHT_ON_DELAY_MASK 0x1fff
> +#define PANEL_LIGHT_ON_DELAY_MASK REG_GENMASK(12, 0)
Ok.
> #define PANEL_LIGHT_ON_DELAY_SHIFT 0
>
> #define _PP_OFF_DELAYS 0x6120C
> #define PP_OFF_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_OFF_DELAYS)
> -#define PANEL_POWER_DOWN_DELAY_MASK 0x1fff0000
> +#define PANEL_POWER_DOWN_DELAY_MASK REG_GENMASK(28, 16)
> #define PANEL_POWER_DOWN_DELAY_SHIFT 16
> -#define PANEL_LIGHT_OFF_DELAY_MASK 0x1fff
> +#define PANEL_LIGHT_OFF_DELAY_MASK REG_GENMASK(12, 0)
> #define PANEL_LIGHT_OFF_DELAY_SHIFT 0
Ok.
> #define _PP_DIVISOR 0x61210
> #define PP_DIVISOR(pps_idx) _MMIO_PPS(pps_idx, _PP_DIVISOR)
> -#define PP_REFERENCE_DIVIDER_MASK 0xffffff00
> +#define PP_REFERENCE_DIVIDER_MASK REG_GENMASK(31, 8)
Ok.
> #define PP_REFERENCE_DIVIDER_SHIFT 8
> -#define PANEL_POWER_CYCLE_DELAY_MASK 0x1f
> +#define PANEL_POWER_CYCLE_DELAY_MASK REG_GENMASK(4, 0)
Ok.
I'll get used to the hi,lo convention eventually.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors
2019-02-27 17:02 ` [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors Jani Nikula
@ 2019-02-27 20:58 ` Chris Wilson
0 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-02-27 20:58 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
Quoting Jani Nikula (2019-02-27 17:02:37)
> bitfield.h defines FIELD_GET() and FIELD_PREP() macros to access
> bitfields using the mask alone, with no need for separate shift. Indeed,
> the shift is redundant.
>
> We define REG_FIELD_GET() and REG_FIELD_PREP() wrappers for the above,
> in part to force u32 and for consistency with REG_BIT() and
> REG_GENMASK(), but also as we'll need to redefine REG_FIELD_PREP() in
> follow-up work to make it produce integer constant expressions.
>
> For the most part, REG_FIELD_GET() is shorter than masking followed by
> shift, and arguably has more clarity.
>
> REG_FIELD_PREP() can get more verbose than simply shifting in place, but
> it does provide masking to ensure we don't overflow the mask, something
> we usually don't bother with currently.
>
> Convert power sequencer registers as an example.
>
> v2:
> - Add the REG_FIELD_GET() and REG_FIELD_PREP() wrappers to use them
> consistently from the start.
>
> 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 | 45 ++++++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_dp.c | 40 +++++++++++----------------
> drivers/gpu/drm/i915/intel_lvds.c | 40 +++++++++++++--------------
> 3 files changed, 64 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e847a18067bc..1bd75770483a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -25,6 +25,7 @@
> #ifndef _I915_REG_H_
> #define _I915_REG_H_
>
> +#include <linux/bitfield.h>
> #include <linux/bits.h>
>
> /**
> @@ -61,11 +62,11 @@
> * significant to least significant bit. Indent the register content macros
> * using two extra spaces between ``#define`` and the macro name.
> *
> - * For bit fields, define a ``_MASK`` and a ``_SHIFT`` macro. Use
> - * ``REG_GENMASK()`` to define _MASK. 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 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 bits using ``REG_BIT(N)``. Do **not** add ``_BIT`` suffix to the name.
> *
> @@ -107,7 +108,6 @@
> * #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_SHIFT 16
> * #define FOO_MODE_BAR (0 << 16)
> * #define FOO_MODE_BAZ (1 << 16)
> * #define FOO_MODE_QUX_SNB (2 << 16)
> @@ -144,6 +144,30 @@
> __builtin_constant_p(__low) && \
> ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
>
> +/**
> + * 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().
> + *
> + * @return: @__val masked and shifted into the field defined by @__mask.
> + */
> +#define REG_FIELD_PREP(__mask, __val) ((u32)FIELD_PREP(__mask, __val))
> +
> +/**
> + * REG_FIELD_GET() - Extract a u32 bitfield value
> + * @__mask: shifted mask defining the field's length and position
> + * @__val: value to extract the bitfield value from
> + *
> + * Local wrapper for FIELD_GET() to force u32 and for consistency with
> + * REG_FIELD_PREP(), REG_BIT() and REG_GENMASK().
> + *
> + * @return: Masked and shifted value of the field defined by @__mask in @__val.
> + */
> +#define REG_FIELD_GET(__mask, __val) ((u32)FIELD_GET(__mask, __val))
I think I prefer the REG_FIELD variants already. Let's see how they work
in practice.
> typedef struct {
> u32 reg;
> } i915_reg_t;
> @@ -4726,7 +4750,6 @@ enum {
> #define ICP_PP_CONTROL(x) _MMIO(((x) == 1) ? _PP_CONTROL_1 : \
> _PP_CONTROL_2)
> #define POWER_CYCLE_DELAY_MASK REG_GENMASK(8, 4)
> -#define POWER_CYCLE_DELAY_SHIFT 4
> #define VDD_OVERRIDE_FORCE REG_BIT(3)
> #define BACKLIGHT_ENABLE REG_BIT(2)
> #define PWR_DOWN_ON_RESET REG_BIT(1)
> @@ -4743,7 +4766,6 @@ enum {
> #define PP_SEQUENCE_NONE (0 << 28)
> #define PP_SEQUENCE_POWER_UP (1 << 28)
> #define PP_SEQUENCE_POWER_DOWN (2 << 28)
> -#define PP_SEQUENCE_SHIFT 28
> #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)
> @@ -4769,7 +4791,6 @@ enum {
>
> #define _PP_ON_DELAYS 0x61208
> #define PP_ON_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
> -#define PANEL_PORT_SELECT_SHIFT 30
> #define PANEL_PORT_SELECT_MASK REG_GENMASK(31, 30)
> #define PANEL_PORT_SELECT_LVDS (0 << 30)
> #define PANEL_PORT_SELECT_DPA (1 << 30)
> @@ -4777,23 +4798,17 @@ enum {
> #define PANEL_PORT_SELECT_DPD (3 << 30)
> #define PANEL_PORT_SELECT_VLV(port) ((port) << 30)
> #define PANEL_POWER_UP_DELAY_MASK REG_GENMASK(28, 16)
> -#define PANEL_POWER_UP_DELAY_SHIFT 16
> #define PANEL_LIGHT_ON_DELAY_MASK REG_GENMASK(12, 0)
> -#define PANEL_LIGHT_ON_DELAY_SHIFT 0
>
> #define _PP_OFF_DELAYS 0x6120C
> #define PP_OFF_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_OFF_DELAYS)
> #define PANEL_POWER_DOWN_DELAY_MASK REG_GENMASK(28, 16)
> -#define PANEL_POWER_DOWN_DELAY_SHIFT 16
> #define PANEL_LIGHT_OFF_DELAY_MASK REG_GENMASK(12, 0)
> -#define PANEL_LIGHT_OFF_DELAY_SHIFT 0
>
> #define _PP_DIVISOR 0x61210
> #define PP_DIVISOR(pps_idx) _MMIO_PPS(pps_idx, _PP_DIVISOR)
> #define PP_REFERENCE_DIVIDER_MASK REG_GENMASK(31, 8)
> -#define PP_REFERENCE_DIVIDER_SHIFT 8
> #define PANEL_POWER_CYCLE_DELAY_MASK REG_GENMASK(4, 0)
> -#define PANEL_POWER_CYCLE_DELAY_SHIFT 0
>
> /* Panel fitting */
> #define PFIT_CONTROL _MMIO(DISPLAY_MMIO_BASE(dev_priv) + 0x61230)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e1a051c0fbfe..d1d282bc4814 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -6438,25 +6438,16 @@ intel_pps_readout_hw_state(struct intel_dp *intel_dp, struct edp_power_seq *seq)
> }
>
> /* Pull timing values out of registers */
> - seq->t1_t3 = (pp_on & PANEL_POWER_UP_DELAY_MASK) >>
> - PANEL_POWER_UP_DELAY_SHIFT;
> -
> - seq->t8 = (pp_on & PANEL_LIGHT_ON_DELAY_MASK) >>
> - PANEL_LIGHT_ON_DELAY_SHIFT;
> -
> - seq->t9 = (pp_off & PANEL_LIGHT_OFF_DELAY_MASK) >>
> - PANEL_LIGHT_OFF_DELAY_SHIFT;
> -
> - seq->t10 = (pp_off & PANEL_POWER_DOWN_DELAY_MASK) >>
> - PANEL_POWER_DOWN_DELAY_SHIFT;
> + seq->t1_t3 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, pp_on);
> + seq->t8 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, pp_on);
> + seq->t9 = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, pp_off);
> + seq->t10 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, pp_off);
That I like.
> if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> HAS_PCH_ICP(dev_priv)) {
> - seq->t11_t12 = ((pp_ctl & BXT_POWER_CYCLE_DELAY_MASK) >>
> - BXT_POWER_CYCLE_DELAY_SHIFT) * 1000;
> + seq->t11_t12 = REG_FIELD_GET(BXT_POWER_CYCLE_DELAY_MASK, pp_ctl) * 1000;
> } else {
> - seq->t11_t12 = ((pp_div & PANEL_POWER_CYCLE_DELAY_MASK) >>
> - PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
> + seq->t11_t12 = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, pp_div) * 1000;
> }
A casualty of verbosity.
> }
>
> @@ -6616,22 +6607,23 @@ intel_dp_init_panel_power_sequencer_registers(struct intel_dp *intel_dp,
> I915_WRITE(regs.pp_ctrl, pp);
> }
>
> - pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) |
> - (seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT);
> - pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) |
> - (seq->t10 << PANEL_POWER_DOWN_DELAY_SHIFT);
> + pp_on = REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, seq->t1_t3) |
> + REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, seq->t8);
> + pp_off = REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, seq->t9) |
> + REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, seq->t10);
Re-reading a few times and it is growing on me. Yes, it's on the
verbose, just wait until we say REG16_FIELD_PREP.
> /* Compute the divisor for the pp clock, simply match the Bspec
> * formula. */
> if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv) ||
> HAS_PCH_ICP(dev_priv)) {
> pp_div = I915_READ(regs.pp_ctrl);
> pp_div &= ~BXT_POWER_CYCLE_DELAY_MASK;
> - pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> - << BXT_POWER_CYCLE_DELAY_SHIFT);
> + pp_div |= REG_FIELD_PREP(BXT_POWER_CYCLE_DELAY_MASK,
> + DIV_ROUND_UP(seq->t11_t12, 1000));
> } else {
> - pp_div = ((100 * div)/2 - 1) << PP_REFERENCE_DIVIDER_SHIFT;
> - pp_div |= (DIV_ROUND_UP(seq->t11_t12, 1000)
> - << PANEL_POWER_CYCLE_DELAY_SHIFT);
> + pp_div = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK,
> + (100 * div) / 2 - 1);
> + pp_div |= REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
> + DIV_ROUND_UP(seq->t11_t12, 1000));
> }
>
> /* Haswell doesn't have any port selection bits for the panel
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index b4aa49768e90..646fc29e159a 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -152,24 +152,17 @@ static void intel_lvds_pps_get_hw_state(struct drm_i915_private *dev_priv,
> pps->powerdown_on_reset = I915_READ(PP_CONTROL(0)) & PANEL_POWER_RESET;
>
> val = I915_READ(PP_ON_DELAYS(0));
> - pps->port = (val & PANEL_PORT_SELECT_MASK) >>
> - PANEL_PORT_SELECT_SHIFT;
> - pps->t1_t2 = (val & PANEL_POWER_UP_DELAY_MASK) >>
> - PANEL_POWER_UP_DELAY_SHIFT;
> - pps->t5 = (val & PANEL_LIGHT_ON_DELAY_MASK) >>
> - PANEL_LIGHT_ON_DELAY_SHIFT;
> + pps->port = REG_FIELD_GET(PANEL_PORT_SELECT_MASK, val);
> + pps->t1_t2 = REG_FIELD_GET(PANEL_POWER_UP_DELAY_MASK, val);
> + pps->t5 = REG_FIELD_GET(PANEL_LIGHT_ON_DELAY_MASK, val);
>
> val = I915_READ(PP_OFF_DELAYS(0));
> - pps->t3 = (val & PANEL_POWER_DOWN_DELAY_MASK) >>
> - PANEL_POWER_DOWN_DELAY_SHIFT;
> - pps->tx = (val & PANEL_LIGHT_OFF_DELAY_MASK) >>
> - PANEL_LIGHT_OFF_DELAY_SHIFT;
> + pps->t3 = REG_FIELD_GET(PANEL_POWER_DOWN_DELAY_MASK, val);
> + pps->tx = REG_FIELD_GET(PANEL_LIGHT_OFF_DELAY_MASK, val);
>
> val = I915_READ(PP_DIVISOR(0));
> - pps->divider = (val & PP_REFERENCE_DIVIDER_MASK) >>
> - PP_REFERENCE_DIVIDER_SHIFT;
> - val = (val & PANEL_POWER_CYCLE_DELAY_MASK) >>
> - PANEL_POWER_CYCLE_DELAY_SHIFT;
> + pps->divider = REG_FIELD_GET(PP_REFERENCE_DIVIDER_MASK, val);
> + val = REG_FIELD_GET(PANEL_POWER_CYCLE_DELAY_MASK, val);
> /*
> * Remove the BSpec specified +1 (100ms) offset that accounts for a
> * too short power-cycle delay due to the asynchronous programming of
> @@ -209,15 +202,18 @@ static void intel_lvds_pps_init_hw(struct drm_i915_private *dev_priv,
> val |= PANEL_POWER_RESET;
> I915_WRITE(PP_CONTROL(0), val);
>
> - I915_WRITE(PP_ON_DELAYS(0), (pps->port << PANEL_PORT_SELECT_SHIFT) |
> - (pps->t1_t2 << PANEL_POWER_UP_DELAY_SHIFT) |
> - (pps->t5 << PANEL_LIGHT_ON_DELAY_SHIFT));
> - I915_WRITE(PP_OFF_DELAYS(0), (pps->t3 << PANEL_POWER_DOWN_DELAY_SHIFT) |
> - (pps->tx << PANEL_LIGHT_OFF_DELAY_SHIFT));
> + val = REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, pps->port) |
> + REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, pps->t1_t2) |
> + REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, pps->t5);
> + I915_WRITE(PP_ON_DELAYS(0), val);
That wouldn't have been so bad as
I915_WRITE(PP_ON_DELAYS(0),
REG_FIELD_PREP(PANEL_PORT_SELECT_MASK, pps->port) |
REG_FIELD_PREP(PANEL_POWER_UP_DELAY_MASK, pps->t1_t2) |
REG_FIELD_PREP(PANEL_LIGHT_ON_DELAY_MASK, pps->t5));
?
>
> - val = pps->divider << PP_REFERENCE_DIVIDER_SHIFT;
> - val |= (DIV_ROUND_UP(pps->t4, 1000) + 1) <<
> - PANEL_POWER_CYCLE_DELAY_SHIFT;
> + val = REG_FIELD_PREP(PANEL_POWER_DOWN_DELAY_MASK, pps->t3) |
> + REG_FIELD_PREP(PANEL_LIGHT_OFF_DELAY_MASK, pps->tx);
> + I915_WRITE(PP_OFF_DELAYS(0), val);
> +
> + val = REG_FIELD_PREP(PP_REFERENCE_DIVIDER_MASK, pps->divider) |
> + REG_FIELD_PREP(PANEL_POWER_CYCLE_DELAY_MASK,
> + DIV_ROUND_UP(pps->t4, 1000) + 1);
> I915_WRITE(PP_DIVISOR(0), val);
> }
REG_FIELD_GET is enough to justify the changes, and REG_FIELD_PREP takes
a bit more getting used, but it is growing on me.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
@ 2019-02-27 21:07 ` Chris Wilson
2019-02-27 21:11 ` Ville Syrjälä
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Chris Wilson @ 2019-02-27 21:07 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
2019-02-27 21:07 ` Chris Wilson
@ 2019-02-27 21:11 ` Ville Syrjälä
2019-02-28 0:17 ` Michal Wajdeczko
2019-02-28 4:49 ` kbuild test robot
3 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2019-02-27 21:11 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-27 20:50 ` Chris Wilson
@ 2019-02-27 21:13 ` Ville Syrjälä
0 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2019-02-27 21:13 UTC (permalink / raw)
To: Chris Wilson; +Cc: Jani Nikula, intel-gfx
On Wed, Feb 27, 2019 at 08:50:31PM +0000, Chris Wilson wrote:
> Quoting Jani Nikula (2019-02-27 17:02:36)
<snip>
> > #define PP_REFERENCE_DIVIDER_SHIFT 8
> > -#define PANEL_POWER_CYCLE_DELAY_MASK 0x1f
> > +#define PANEL_POWER_CYCLE_DELAY_MASK REG_GENMASK(4, 0)
>
> Ok.
>
> I'll get used to the hi,lo convention eventually.
The nice thing is that it matches the spec.
The hard part is running out of fingers for wide bitfields :P
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-27 17:02 ` [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() " Jani Nikula
2019-02-27 20:50 ` Chris Wilson
@ 2019-02-27 23:06 ` Michal Wajdeczko
2019-02-28 10:07 ` Jani Nikula
1 sibling, 1 reply; 22+ messages in thread
From: Michal Wajdeczko @ 2019-02-27 23:06 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@intel.com>
wrote:
> @@ -116,6 +116,34 @@
> * #define GEN8_BAR _MMIO(0xb888)
> */
> +/**
> + * REG_BIT() - Prepare a u32 bit value
> + * @__n: 0-based bit number
> + *
> + * Local wrapper for BIT() to force u32, with compile time checks.
> + *
> + * @return: Value with bit @__n set.
> + */
> +#define REG_BIT(__n) \
> + ((u32)(BIT(__n) + \
> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) && \
> + ((__n) < 0 || (__n) > 31))))
Maybe to simplify the code we can define this macro using macro below:
#define REG_BIT(__n) REG_GENMASK(__n, __n)
> +
> +/**
> + * REG_GENMASK() - Prepare a continuous u32 bitmask
> + * @__high: 0-based high bit
> + * @__low: 0-based low bit
> + *
> + * Local wrapper for GENMASK() to force u32, with compile time checks.
> + *
> + * @return: Continuous bitmask from @__high to @__low, inclusive.
> + */
> +#define REG_GENMASK(__high, __low) \
> + ((u32)(GENMASK(__high, __low) + \
> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
> + __builtin_constant_p(__low) && \
> + ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
> +
nit: Since we are defining new set of macros, do we really have to follow
naming of the underlying macros? maybe we can can have clear new names:
REG_BIT(n)
REG_BITS(hi,low)
Thanks,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
2019-02-27 21:07 ` Chris Wilson
2019-02-27 21:11 ` Ville Syrjälä
@ 2019-02-28 0:17 ` Michal Wajdeczko
2019-02-28 10:24 ` Jani Nikula
2019-02-28 4:49 ` kbuild test robot
3 siblings, 1 reply; 22+ messages in thread
From: Michal Wajdeczko @ 2019-02-28 0:17 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
` (2 preceding siblings ...)
2019-02-28 0:17 ` Michal Wajdeczko
@ 2019-02-28 4:49 ` kbuild test robot
3 siblings, 0 replies; 22+ messages in thread
From: kbuild test robot @ 2019-02-28 4:49 UTC (permalink / raw)
Cc: Jani Nikula, intel-gfx, kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5549 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67280 bytes --]
[-- Attachment #3: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-27 23:06 ` Michal Wajdeczko
@ 2019-02-28 10:07 ` Jani Nikula
2019-02-28 10:12 ` Chris Wilson
0 siblings, 1 reply; 22+ messages in thread
From: Jani Nikula @ 2019-02-28 10:07 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@intel.com>
> wrote:
>
>> @@ -116,6 +116,34 @@
>> * #define GEN8_BAR _MMIO(0xb888)
>> */
>> +/**
>> + * REG_BIT() - Prepare a u32 bit value
>> + * @__n: 0-based bit number
>> + *
>> + * Local wrapper for BIT() to force u32, with compile time checks.
>> + *
>> + * @return: Value with bit @__n set.
>> + */
>> +#define REG_BIT(__n) \
>> + ((u32)(BIT(__n) + \
>> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) && \
>> + ((__n) < 0 || (__n) > 31))))
>
> Maybe to simplify the code we can define this macro using macro below:
>
> #define REG_BIT(__n) REG_GENMASK(__n, __n)
I don't want to limit the macro to constant expressions (even if that's
the most common use for us), and in non-constant expressions the simple
shift becomes unnecessarily complicated with GENMASK. Plus there's the
double evaluation of __n.
>
>> +
>> +/**
>> + * REG_GENMASK() - Prepare a continuous u32 bitmask
>> + * @__high: 0-based high bit
>> + * @__low: 0-based low bit
>> + *
>> + * Local wrapper for GENMASK() to force u32, with compile time checks.
>> + *
>> + * @return: Continuous bitmask from @__high to @__low, inclusive.
>> + */
>> +#define REG_GENMASK(__high, __low) \
>> + ((u32)(GENMASK(__high, __low) + \
>> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
>> + __builtin_constant_p(__low) && \
>> + ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
>> +
>
> nit: Since we are defining new set of macros, do we really have to follow
> naming of the underlying macros? maybe we can can have clear new names:
>
> REG_BIT(n)
> REG_BITS(hi,low)
We've pretty much been having this conversation ever since the first RFC
was posted. It could be BITS, MASK, GENMASK, FIELD (except that clashes
with REG_FIELD from regmap.h), BITFIELD, whatnot. And next thing you
know, we look at REG_FIELD_PREP and REG_FIELD_GET and wonder if we
should have our own names for them too. REG_BITS_PREP? REG_BITS_VALUE?
REG_BITS_GET?
We *might* be able to come up with internally consistent naming
everyone's happy with, and have those names grow on people, but based on
the discussion so far I'm not optimistic.
So basically I gave up on that, and with the current proposal, the names
are the same as the widely used kernel macros, with REG_ prefix
added. If you know what they do, you know what these do. It's still
consistent, just in a different way.
Also, I pretty much expect our code outside of i915_reg.h to use a mix
of our versions and the underlying ones. And I'm not sure I want to
start policing people to use our versions exclusively. If the names
differ only with the REG_ part, I think the mix will be much easier to
live with.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-28 10:07 ` Jani Nikula
@ 2019-02-28 10:12 ` Chris Wilson
2019-02-28 11:12 ` Michal Wajdeczko
0 siblings, 1 reply; 22+ messages in thread
From: Chris Wilson @ 2019-02-28 10:12 UTC (permalink / raw)
To: Jani Nikula, Michal Wajdeczko, intel-gfx
Quoting Jani Nikula (2019-02-28 10:07:06)
> On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> > On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@intel.com>
> > wrote:
> >
> >> @@ -116,6 +116,34 @@
> >> * #define GEN8_BAR _MMIO(0xb888)
> >> */
> >> +/**
> >> + * REG_BIT() - Prepare a u32 bit value
> >> + * @__n: 0-based bit number
> >> + *
> >> + * Local wrapper for BIT() to force u32, with compile time checks.
> >> + *
> >> + * @return: Value with bit @__n set.
> >> + */
> >> +#define REG_BIT(__n) \
> >> + ((u32)(BIT(__n) + \
> >> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) && \
> >> + ((__n) < 0 || (__n) > 31))))
> >
> > Maybe to simplify the code we can define this macro using macro below:
> >
> > #define REG_BIT(__n) REG_GENMASK(__n, __n)
>
> I don't want to limit the macro to constant expressions (even if that's
> the most common use for us), and in non-constant expressions the simple
> shift becomes unnecessarily complicated with GENMASK. Plus there's the
> double evaluation of __n.
>
> >
> >> +
> >> +/**
> >> + * REG_GENMASK() - Prepare a continuous u32 bitmask
> >> + * @__high: 0-based high bit
> >> + * @__low: 0-based low bit
> >> + *
> >> + * Local wrapper for GENMASK() to force u32, with compile time checks.
> >> + *
> >> + * @return: Continuous bitmask from @__high to @__low, inclusive.
> >> + */
> >> +#define REG_GENMASK(__high, __low) \
> >> + ((u32)(GENMASK(__high, __low) + \
> >> + BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) && \
> >> + __builtin_constant_p(__low) && \
> >> + ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
> >> +
> >
> > nit: Since we are defining new set of macros, do we really have to follow
> > naming of the underlying macros? maybe we can can have clear new names:
> >
> > REG_BIT(n)
> > REG_BITS(hi,low)
>
> We've pretty much been having this conversation ever since the first RFC
> was posted. It could be BITS, MASK, GENMASK, FIELD (except that clashes
> with REG_FIELD from regmap.h), BITFIELD, whatnot. And next thing you
> know, we look at REG_FIELD_PREP and REG_FIELD_GET and wonder if we
> should have our own names for them too. REG_BITS_PREP? REG_BITS_VALUE?
> REG_BITS_GET?
>
> We *might* be able to come up with internally consistent naming
> everyone's happy with, and have those names grow on people, but based on
> the discussion so far I'm not optimistic.
>
> So basically I gave up on that, and with the current proposal, the names
> are the same as the widely used kernel macros, with REG_ prefix
> added. If you know what they do, you know what these do. It's still
> consistent, just in a different way.
>
> Also, I pretty much expect our code outside of i915_reg.h to use a mix
> of our versions and the underlying ones. And I'm not sure I want to
> start policing people to use our versions exclusively. If the names
> differ only with the REG_ part, I think the mix will be much easier to
> live with.
+1
An alternative naming scheme might be:
BIT_U32
GENMASK_U32
FIELD_GET_U32
FIELD_PREP_U32
(which extend naturally to 16b registers etc) and perhaps more obvious
what the nature of the change over the common macros. The U is in all
likelihood redundant, so even
BIT32
GENMASK32
FIELD32_GET
FIELD32_PREP
?
But I think the central tenant is that we want to use the common naming
scheme so that we can trivially go from GENMASK to REG_GENMASK/GENMASK32
and that other people reading our code already know the language (plus
we want these to be part of include/linux and out of our hair!)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-28 0:17 ` Michal Wajdeczko
@ 2019-02-28 10:24 ` Jani Nikula
2019-02-28 11:38 ` Michal Wajdeczko
0 siblings, 1 reply; 22+ messages in thread
From: Jani Nikula @ 2019-02-28 10:24 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
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
>
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents
2019-02-28 10:12 ` Chris Wilson
@ 2019-02-28 11:12 ` Michal Wajdeczko
0 siblings, 0 replies; 22+ messages in thread
From: Michal Wajdeczko @ 2019-02-28 11:12 UTC (permalink / raw)
To: Jani Nikula, intel-gfx, Chris Wilson
On Thu, 28 Feb 2019 11:12:43 +0100, Chris Wilson
<chris@chris-wilson.co.uk> wrote:
> But I think the central tenant is that we want to use the common naming
> scheme so that we can trivially go from GENMASK to REG_GENMASK/GENMASK32
> and that other people reading our code already know the language (plus
> we want these to be part of include/linux and out of our hair!)
That's valid point.
Thanks,
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-28 10:24 ` Jani Nikula
@ 2019-02-28 11:38 ` Michal Wajdeczko
2019-02-28 13:48 ` Jani Nikula
0 siblings, 1 reply; 22+ messages in thread
From: Michal Wajdeczko @ 2019-02-28 11:38 UTC (permalink / raw)
To: intel-gfx, Jani Nikula
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values
2019-02-28 11:38 ` Michal Wajdeczko
@ 2019-02-28 13:48 ` Jani Nikula
0 siblings, 0 replies; 22+ messages in thread
From: Jani Nikula @ 2019-02-28 13:48 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
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.
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-02-28 13:46 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 17:02 [PATCH v3 0/3] drm/i915: introduce macros to define register contents Jani Nikula
2019-02-27 17:02 ` [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() " Jani Nikula
2019-02-27 20:50 ` Chris Wilson
2019-02-27 21:13 ` Ville Syrjälä
2019-02-27 23:06 ` Michal Wajdeczko
2019-02-28 10:07 ` Jani Nikula
2019-02-28 10:12 ` Chris Wilson
2019-02-28 11:12 ` Michal Wajdeczko
2019-02-27 17:02 ` [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors Jani Nikula
2019-02-27 20:58 ` Chris Wilson
2019-02-27 17:02 ` [PATCH v3 3/3] drm/i915: use REG_FIELD_PREP() to define register bitfield values Jani Nikula
2019-02-27 21:07 ` Chris Wilson
2019-02-27 21:11 ` Ville Syrjälä
2019-02-28 0:17 ` Michal Wajdeczko
2019-02-28 10:24 ` Jani Nikula
2019-02-28 11:38 ` Michal Wajdeczko
2019-02-28 13:48 ` Jani Nikula
2019-02-28 4:49 ` kbuild test robot
2019-02-27 17:44 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: introduce macros to define register contents (rev3) Patchwork
2019-02-27 17:46 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-02-27 18:20 ` ✓ Fi.CI.BAT: success " Patchwork
2019-02-27 20:13 ` ✓ Fi.CI.IGT: " Patchwork
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.