Re: [RFC 4/4] drm/i915/registers: define field values using FIELD_PREP()

From: Jani Nikula <jani.nikula@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [RFC 4/4] drm/i915/registers: define field values using FIELD_PREP()
Date: Thu, 27 Sep 2018 14:53:58 +0300	[thread overview]
Message-ID: <87efdfrwa1.fsf@intel.com> (raw)
In-Reply-To: <153804454783.11401.8959076690442256391@skylake-alporthouse-com>

On Thu, 27 Sep 2018, Chris Wilson <chris@chris-wilson.co.uk> 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@intel.com>
>> ---
>> @@ -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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx