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

[Michal Wajdeczko]

On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula <jani.nikula at intel.com>  
wrote:

> On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko at intel.com> wrote:
>> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula at 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