On Thu, 27 Sep 2018, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Thu, 27 Sep 2018, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>> Quoting Jani Nikula (2018-09-27 10:40:23)
>>> Slightly verbose, but does away with hand rolled shifts and provides
>>> static checking that the values fit the mask.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>> ---
>>> @@ -4650,11 +4650,11 @@ enum {
>>> #define _PP_ON_DELAYS 0x61208
>>> #define PP_ON_DELAYS(pps_idx) _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
>>> #define PANEL_PORT_SELECT_MASK GENMASK(31, 30)
>>> -#define PANEL_PORT_SELECT_LVDS (0 << 30)
>>> -#define PANEL_PORT_SELECT_DPA (1 << 30)
>>> -#define PANEL_PORT_SELECT_DPC (2 << 30)
>>> -#define PANEL_PORT_SELECT_DPD (3 << 30)
>>> -#define PANEL_PORT_SELECT_VLV(port) ((port) << 30)
>>> +#define PANEL_PORT_SELECT_LVDS FIELD_PREP(PANEL_PORT_SELECT_MASK, 0)
>>> +#define PANEL_PORT_SELECT_DPA FIELD_PREP(PANEL_PORT_SELECT_MASK, 1)
>>> +#define PANEL_PORT_SELECT_DPC FIELD_PREP(PANEL_PORT_SELECT_MASK, 2)
>>> +#define PANEL_PORT_SELECT_DPD FIELD_PREP(PANEL_PORT_SELECT_MASK, 3)
>>> +#define PANEL_PORT_SELECT_VLV(port) FIELD_PREP(PANEL_PORT_SELECT_MASK, port)
>>
>> Maybe verbose, but it reads far better as giving each field a distinct
>> name ties together all the individual options.
>>
>> Before seeing this I was sceptical about FIELD_PREP, no longer.
>>
>> Under this construct we aren't using masks per se, but giving a name to
>> a group of bits within the register (a field). So I think
>>
>> #define PANEL_PORT_SELECT GENMASK(31, 30)
>> #define PANEL_PORT_SELECT_LVDS FIELD_PREP(PANEL_PORT_SELECT, 0)
>
> Argh, I've screwed up. I must have failed to build after adding the last
> patch. You know, RFC and all. This results in:
>
> drivers/gpu/drm/i915/intel_display.c: In function ‘assert_panel_unlocked’:
> drivers/gpu/drm/i915/intel_display.c:1219:3: error: case label does not reduce to an integer constant
> case PANEL_PORT_SELECT_LVDS:
>
> Fail.
>
> However, taking just the mask&shift part from FIELD_PREP works:
>
> (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))
>
> But then we'll lose the static checks, unless we come up with something
> ingenious ourselves.
If my grep-fu serves me right, we have 186 macros defined in i915_reg.h
that are used as case labels, and almost all of them would be candidates
for FIELD_PREP().
Is an alternative FIELD_PREP() worth it without the static checks?
BR,
Jani.
>
> BR,
> Jani.
>
>
>>
>> is intuitive. Did you do a quick bloatometer to see if gcc code
>> generation is affected?
>> -Chris
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
