On Thu, 27 Jan 2022, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Thu, Jan 27, 2022 at 01:17:21PM +0200, Jani Nikula wrote:
>> On Thu, 27 Jan 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > Use REG_GENMASK() & co. for the M/N register values. There are
>> > also a lot of weird unused defines (eg. *_OFFSET) we can just
>> > throw out.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_display.c | 10 ++++-----
>> > drivers/gpu/drm/i915/i915_reg.h | 22 +++-----------------
>> > 2 files changed, 8 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> > index f76faa195cb9..d91164d1eb92 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -3865,11 +3865,11 @@ static void intel_get_m_n(struct drm_i915_private *i915,
>> > i915_reg_t data_m_reg, i915_reg_t data_n_reg,
>> > i915_reg_t link_m_reg, i915_reg_t link_n_reg)
>> > {
>> > - m_n->link_m = intel_de_read(i915, link_m_reg);
>> > - m_n->link_n = intel_de_read(i915, link_n_reg);
>> > - m_n->gmch_m = intel_de_read(i915, data_m_reg) & ~TU_SIZE_MASK;
>> > - m_n->gmch_n = intel_de_read(i915, data_n_reg);
>> > - m_n->tu = ((intel_de_read(i915, data_m_reg) & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
>> > + m_n->link_m = intel_de_read(i915, link_m_reg) & DATA_LINK_M_N_MASK;
>> > + m_n->link_n = intel_de_read(i915, link_n_reg) & DATA_LINK_M_N_MASK;
>> > + m_n->gmch_m = intel_de_read(i915, data_m_reg) & DATA_LINK_M_N_MASK;
>> > + m_n->gmch_n = intel_de_read(i915, data_n_reg) & DATA_LINK_M_N_MASK;
>> > + m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(i915, data_m_reg)) + 1;
>>
>> The commit message might mention we throw some bits away while reading.
>
> Right, forgot to note that.
>
>>
>> A follow-up could perhasps axe the double read of the data_m_reg, but
>> *shrug*.
>
> I was going back and forth between keeping the double read vs. reading
> each just once here vs. reading straight in the callers and just passing
> the values through (would avoid needing i915_reg_t for the prototype
> in the headrer in a later patch). Somehow none of them tasted quit right
> so opted to keep the current thing for now.
Yeah, not a big deal. Also now that i915_reg_t is in i915_reg_defs.h I
think the overhead for that is neglible.
BR,
Jani.
>
>>
>> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>
>>
>> > }
>> >
>> > static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc,
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index 2e4dd9db63fe..ec48406eb37a 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -5209,16 +5209,14 @@ enum {
>> > #define _PIPEB_DATA_M_G4X 0x71050
>> >
>> > /* Transfer unit size for display port - 1, default is 0x3f (for TU size 64) */
>> > -#define TU_SIZE(x) (((x) - 1) << 25) /* default size 64 */
>> > -#define TU_SIZE_SHIFT 25
>> > -#define TU_SIZE_MASK (0x3f << 25)
>> > +#define TU_SIZE_MASK REG_GENMASK(30, 25)
>> > +#define TU_SIZE(x) REG_FIELD_PREP(TU_SIZE_MASK, (x) - 1) /* default size 64 */
>> >
>> > -#define DATA_LINK_M_N_MASK (0xffffff)
>> > +#define DATA_LINK_M_N_MASK REG_GENMASK(23, 0)
>> > #define DATA_LINK_N_MAX (0x800000)
>> >
>> > #define _PIPEA_DATA_N_G4X 0x70054
>> > #define _PIPEB_DATA_N_G4X 0x71054
>> > -#define PIPE_GMCH_DATA_N_MASK (0xffffff)
>> >
>> > /*
>> > * Computing Link M and N values for the Display Port link
>> > @@ -5233,11 +5231,8 @@ enum {
>> >
>> > #define _PIPEA_LINK_M_G4X 0x70060
>> > #define _PIPEB_LINK_M_G4X 0x71060
>> > -#define PIPEA_DP_LINK_M_MASK (0xffffff)
>> > -
>> > #define _PIPEA_LINK_N_G4X 0x70064
>> > #define _PIPEB_LINK_N_G4X 0x71064
>> > -#define PIPEA_DP_LINK_N_MASK (0xffffff)
>> >
>> > #define PIPE_DATA_M_G4X(pipe) _MMIO_PIPE(pipe, _PIPEA_DATA_M_G4X, _PIPEB_DATA_M_G4X)
>> > #define PIPE_DATA_N_G4X(pipe) _MMIO_PIPE(pipe, _PIPEA_DATA_N_G4X, _PIPEB_DATA_N_G4X)
>> > @@ -6840,24 +6835,13 @@ enum {
>> >
>> >
>> > #define _PIPEA_DATA_M1 0x60030
>> > -#define PIPE_DATA_M1_OFFSET 0
>> > #define _PIPEA_DATA_N1 0x60034
>> > -#define PIPE_DATA_N1_OFFSET 0
>> > -
>> > #define _PIPEA_DATA_M2 0x60038
>> > -#define PIPE_DATA_M2_OFFSET 0
>> > #define _PIPEA_DATA_N2 0x6003c
>> > -#define PIPE_DATA_N2_OFFSET 0
>> > -
>> > #define _PIPEA_LINK_M1 0x60040
>> > -#define PIPE_LINK_M1_OFFSET 0
>> > #define _PIPEA_LINK_N1 0x60044
>> > -#define PIPE_LINK_N1_OFFSET 0
>> > -
>> > #define _PIPEA_LINK_M2 0x60048
>> > -#define PIPE_LINK_M2_OFFSET 0
>> > #define _PIPEA_LINK_N2 0x6004c
>> > -#define PIPE_LINK_N2_OFFSET 0
>> >
>> > /* PIPEB timing regs are same start from 0x61000 */
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
