Re: [PATCH v3 2/3] drm/i915: deprecate _SHIFT in favor of _MASK passed to accessors

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxx>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux