[PATCH] drm/i915: use variadic macros and arrays to choose port/pipe based registers

[PATCH] drm/i915: use variadic macros and arrays to choose port/pipe based registers

[Jani Nikula]

This allows the use of more than 3 ports/pipes/whatever without tricks,
even if the register offsets are not evenly spaced.

There's the risk of out of bounds access if we're not careful; currently
that would "just" lead to the wrong register offset being used. It might
be possible to add build bug ons for build time constant indexing.

We already have ports defined up to E, not sure if we might have bugs
related to them and the current _PORT3() macro.

   text	   data	    bss	    dec	    hex	filename
1239868	  46199	   4096	1290163	 13afb3	drivers/gpu/drm/i915/i915.ko
1238828	  46199	   4096	1289123	 13aba3	drivers/gpu/drm/i915/i915.ko

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 672cb102f477..c6435a447300 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
 }
 
+#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
+
 #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
 #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
 #define _PLANE(plane, a, b) _PIPE(plane, a, b)
@@ -56,14 +58,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
 #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
 #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
-#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
-			       (pipe) == PIPE_B ? (b) : (c))
+#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__)
 #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c))
-#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
-			       (port) == PORT_B ? (b) : (c))
+#define _PORT3(port, ...) _PICK(port, __VA_ARGS__)
 #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c))
-#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \
-			     (phy) == DPIO_PHY1 ? (b) : (c))
+#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
 #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
 
 #define _MASKED_FIELD(mask, value) ({					   \
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use variadic macros and arrays to choose port/pipe based registers

[Chris Wilson]

On Fri, Jan 27, 2017 at 05:57:06PM +0200, Jani Nikula wrote:
> This allows the use of more than 3 ports/pipes/whatever without tricks,
> even if the register offsets are not evenly spaced.
> 
> There's the risk of out of bounds access if we're not careful; currently
> that would "just" lead to the wrong register offset being used. It might
> be possible to add build bug ons for build time constant indexing.
> 
> We already have ports defined up to E, not sure if we might have bugs
> related to them and the current _PORT3() macro.
> 
>    text	   data	    bss	    dec	    hex	filename
> 1239868	  46199	   4096	1290163	 13afb3	drivers/gpu/drm/i915/i915.ko
> 1238828	  46199	   4096	1289123	 13aba3	drivers/gpu/drm/i915/i915.ko
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 672cb102f477..c6435a447300 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>  }
>  
> +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])

Very neat. The danger is that for a variable index, the compiler will
plonk the array on the stack, for each invocation. Though for a constant
the compiler will see through it and generate the right constant.

https://godbolt.org/g/YCK1od

Given that it looks like the compiler will get smarter, this looks like
a much less error prone way of writing these.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use variadic macros and arrays to choose port/pipe based registers

[Ville Syrjälä]

On Fri, Jan 27, 2017 at 05:57:06PM +0200, Jani Nikula wrote:
> This allows the use of more than 3 ports/pipes/whatever without tricks,
> even if the register offsets are not evenly spaced.
> 
> There's the risk of out of bounds access if we're not careful; currently
> that would "just" lead to the wrong register offset being used. It might
> be possible to add build bug ons for build time constant indexing.
> 
> We already have ports defined up to E, not sure if we might have bugs
> related to them and the current _PORT3() macro.
> 
>    text	   data	    bss	    dec	    hex	filename
> 1239868	  46199	   4096	1290163	 13afb3	drivers/gpu/drm/i915/i915.ko
> 1238828	  46199	   4096	1289123	 13aba3	drivers/gpu/drm/i915/i915.ko

Fits in well with all the other preprocessor horrors we have ;)
I'm convinced.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 672cb102f477..c6435a447300 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>  }
>  
> +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> +
>  #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
>  #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
>  #define _PLANE(plane, a, b) _PIPE(plane, a, b)
> @@ -56,14 +58,11 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
>  #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
>  #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
> -#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
> -			       (pipe) == PIPE_B ? (b) : (c))
> +#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__)
>  #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c))
> -#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
> -			       (port) == PORT_B ? (b) : (c))
> +#define _PORT3(port, ...) _PICK(port, __VA_ARGS__)
>  #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c))
> -#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \
> -			     (phy) == DPIO_PHY1 ? (b) : (c))
> +#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
>  #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
>  
>  #define _MASKED_FIELD(mask, value) ({					   \
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: use variadic macros and arrays to choose port/pipe based registers

[Patchwork]

== Series Details ==

Series: drm/i915: use variadic macros and arrays to choose port/pipe based registers
URL   : https://patchwork.freedesktop.org/series/18691/
State : success

== Summary ==

Series 18691v1 drm/i915: use variadic macros and arrays to choose port/pipe based registers
https://patchwork.freedesktop.org/api/1.0/series/18691/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:78   pass:65   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:223  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:221  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

dc47d8d6ed4ea669aeccad104fde843fe039b9a5 drm-tip: 2017y-01m-30d-09h-12m-47s UTC integration manifest
39ff6fc1 drm/i915: use variadic macros and arrays to choose port/pipe based registers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3635/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use variadic macros and arrays to choose port/pipe based registers

[Jani Nikula]

On Fri, 27 Jan 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Jan 27, 2017 at 05:57:06PM +0200, Jani Nikula wrote:
>> This allows the use of more than 3 ports/pipes/whatever without tricks,
>> even if the register offsets are not evenly spaced.
>> 
>> There's the risk of out of bounds access if we're not careful; currently
>> that would "just" lead to the wrong register offset being used. It might
>> be possible to add build bug ons for build time constant indexing.
>> 
>> We already have ports defined up to E, not sure if we might have bugs
>> related to them and the current _PORT3() macro.
>> 
>>    text	   data	    bss	    dec	    hex	filename
>> 1239868	  46199	   4096	1290163	 13afb3	drivers/gpu/drm/i915/i915.ko
>> 1238828	  46199	   4096	1289123	 13aba3	drivers/gpu/drm/i915/i915.ko
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 672cb102f477..c6435a447300 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -48,6 +48,8 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>>  	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>>  }
>>  
>> +#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
>
> Very neat. The danger is that for a variable index, the compiler will
> plonk the array on the stack, for each invocation. Though for a constant
> the compiler will see through it and generate the right constant.
>
> https://godbolt.org/g/YCK1od
>
> Given that it looks like the compiler will get smarter, this looks like
> a much less error prone way of writing these.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks for the reviews, pushed to drm-intel-next-queued with Daniel's
IRC ack added on top.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx