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 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