Re: [RFC 1/4] drm/i915: Add Display Gen info.

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 31/10/2018 08:13, Jani Nikula wrote:
> On Tue, 30 Oct 2018, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
>> On Tue, Oct 30, 2018 at 11:52:30AM +0200, Jani Nikula wrote:
>>> On Mon, 29 Oct 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>>>> +#define IS_DISPLAY_GEN2(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(2)))
>>>> +#define IS_DISPLAY_GEN3(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(3)))
>>>> +#define IS_DISPLAY_GEN4(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(4)))
>>>> +#define IS_DISPLAY_GEN5(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(5)))
>>>> +#define IS_DISPLAY_GEN6(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(6)))
>>>> +#define IS_DISPLAY_GEN7(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(7)))
>>>> +#define IS_DISPLAY_GEN8(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(8)))
>>>> +#define IS_DISPLAY_GEN9(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(9)))
>>>> +#define IS_DISPLAY_GEN10(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(10)))
>>>> +#define IS_DISPLAY_GEN11(dev_priv)	(!!((dev_priv)->info.display_gen_mask \
>>>> +					    & BIT(11)))
>>>
>>> I know this is the same pattern as in IS_GEN<N> above, but shouldn't the
>>> compiler end up with the same result if these were simply:
>>>
>>> #define IS_DISPLAY_GEN2(dev_priv) IS_DISPLAY_GEN(dev_priv, 2, 2)
>>
>>
>> humn... maybe this is too magic, but it works for me and I didn't add any additional
>> macro to the kernel to implement it :)
>>
>> [CI, DON'T TEST THIS] diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> [CI, DON'T TEST THIS] index 554627dc623c..02a8b51fd733 100644
>> [CI, DON'T TEST THIS] --- a/drivers/gpu/drm/i915/i915_drv.h
>> [CI, DON'T TEST THIS] +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2385,9 +2385,15 @@ intel_info(const struct drm_i915_private *dev_priv)
>>   	GENMASK((e) - 1, (s) - 1))
>>   
>>   /* Returns true if Gen is in inclusive range [Start, End] */
>> -#define IS_GEN(dev_priv, s, e) \
>> +#define _IS_GEN_ARG2(dev_priv, s, e) \
>>   	(!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e))))
>>   
>> +#define _IS_GEN_ARG1(dev_priv, g) \
>> +	(!!((dev_priv)->info.gen_mask & BIT((g) - 1)))
>> +
>> +#define IS_GEN(dev_priv, ...) \
>> +	CONCATENATE(_IS_GEN_ARG, COUNT_ARGS(__VA_ARGS__))((dev_priv), ##__VA_ARGS__)
>> +
>>   /*
>>    * Return true if revision is in range [since,until] inclusive.
>>    *
>>
>>
>> So we could use IS_GEN(dev_priv, 2) as well as IS_GEN(dev_priv, 2, 4), which IMO is very clear.
>> The same would apply for IS_DISPLAY_GEN() version. And if they generate the same code, we could
>> just change the expansion to repeat the argument.
> 
> I like this stuff.
> 
> So I'd prefer IS_GEN(dev_priv, 2) in favor of IS_GEN2(dev_priv)
> throughout.
> 
> As long as it doesn't increase the code size too much, but this being
> macro magic I don't think it should.
> 
> Care to cook up a patch against current tip to make IS_GEN() take 1 or 2
> args? If I read the above right, you'll get a build error for using
> IS_GEN(dev_priv, 1, 2, 3), is that correct?

I saw some mention somewhere on IS_GEN_RANGE, which looked clearer than 
IS_GEN(dev_priv, s, e). Presumably that did not go anywhere since now 
the proposal is the above? I have to say I am not sure it reads 
completely intuitive when seen near in code:

IS_GEN(dev_priv, 9)
IS_GEN(dev_priv, 8, 9)

Looks like a variable arg list and the difference in semantics does not 
come through. As such I am leaning towards thinking it is too much churn 
for unclear benefit. Or in other words I thought IS_GEN_RANGE was a 
better direction.

Regards,

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