On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> wrote:
> On Thu, 28 Feb 2019 11:24:53 +0100, Jani Nikula <jani.nikula@xxxxxxxxx>
> wrote:
>
>> On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> wrote:
>>> On Wed, 27 Feb 2019 18:02:38 +0100, Jani Nikula <jani.nikula@xxxxxxxxx>
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
