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

[Jani Nikula <jani.nikula@intel.com>]

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