[1/2] drm/i915: Introduce INTEL_GEN_RANGE macro
diff mbox

Message ID 1462544186-34818-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin May 6, 2016, 2:16 p.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

To be used for more efficient Gen range checking.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

Chris Wilson May 6, 2016, 2:33 p.m. UTC | #1
On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> To be used for more efficient Gen range checking.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 15fcbcece13c..935e381407ba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table {
>  })
>  #define INTEL_INFO(p) 	(&__I915__(p)->info)
>  #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
>  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>  #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d5a7cfec589b..2c3681757aba 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
>  
>  	/* FIXME: We lack the proper locking here, so only run this on the
>  	 * platforms that need. */
> -	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
> +	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
>  		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
>  	cache->fb.pixel_format = fb->pixel_format;
>  	cache->fb.stride = fb->pitches[0];
> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine)
>  			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
>  
>  	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
> -	if (IS_GEN7(dev))
> +	if (IS_GEN7(dev_priv))
>  		I915_WRITE(GFX_MODE_GEN7,
>  			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
>  			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
>  
> -	if (IS_GEN6(dev)) {
> +	if (IS_GEN6(dev_priv)) {

This chunk shouldn't be in this patch.

Couldn't improve upon INTEL_GEN_RANGE.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Dave Gordon May 6, 2016, 7:11 p.m. UTC | #2
On 06/05/16 15:33, Chris Wilson wrote:
> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> To be used for more efficient Gen range checking.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |  1 +
>>   drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 15fcbcece13c..935e381407ba 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table {
>>   })
>>   #define INTEL_INFO(p) 	(&__I915__(p)->info)
>>   #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
>>   #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>>   #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index d5a7cfec589b..2c3681757aba 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
>>
>>   	/* FIXME: We lack the proper locking here, so only run this on the
>>   	 * platforms that need. */
>> -	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
>> +	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
>>   		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
>>   	cache->fb.pixel_format = fb->pixel_format;
>>   	cache->fb.stride = fb->pitches[0];
>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine)
>>   			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
>>
>>   	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
>> -	if (IS_GEN7(dev))
>> +	if (IS_GEN7(dev_priv))
>>   		I915_WRITE(GFX_MODE_GEN7,
>>   			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
>>   			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
>>
>> -	if (IS_GEN6(dev)) {
>> +	if (IS_GEN6(dev_priv)) {
>
> This chunk shouldn't be in this patch.
>
> Couldn't improve upon INTEL_GEN_RANGE.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris

INTEL_GEN_IN_RANGE() ?

Perhaps emphasises that we're using INclusive ranges?

(whereas the open-coded version might use inclusive or exclusive or 
semi-inclusive at either end, as in the example above.)

.Dave.
Jani Nikula May 9, 2016, 12:32 p.m. UTC | #3
On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote:
> On 06/05/16 15:33, Chris Wilson wrote:
>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> To be used for more efficient Gen range checking.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h         |  1 +
>>>   drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
>>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
>>>   3 files changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 15fcbcece13c..935e381407ba 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table {
>>>   })
>>>   #define INTEL_INFO(p) 	(&__I915__(p)->info)
>>>   #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
>>>   #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>>>   #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>>> index d5a7cfec589b..2c3681757aba 100644
>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
>>>
>>>   	/* FIXME: We lack the proper locking here, so only run this on the
>>>   	 * platforms that need. */
>>> -	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
>>> +	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
>>>   		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
>>>   	cache->fb.pixel_format = fb->pixel_format;
>>>   	cache->fb.stride = fb->pitches[0];
>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine)
>>>   			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
>>>
>>>   	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
>>> -	if (IS_GEN7(dev))
>>> +	if (IS_GEN7(dev_priv))
>>>   		I915_WRITE(GFX_MODE_GEN7,
>>>   			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
>>>   			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
>>>
>>> -	if (IS_GEN6(dev)) {
>>> +	if (IS_GEN6(dev_priv)) {
>>
>> This chunk shouldn't be in this patch.
>>
>> Couldn't improve upon INTEL_GEN_RANGE.
>>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> -Chris
>
> INTEL_GEN_IN_RANGE() ?
>
> Perhaps emphasises that we're using INclusive ranges?

Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and
IS_BXT_REVID(). Might as well make this shorter to write.

BR,
Jani.

>
> (whereas the open-coded version might use inclusive or exclusive or 
> semi-inclusive at either end, as in the example above.)
>
> .Dave.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Dave Gordon May 9, 2016, 2:26 p.m. UTC | #4
On 09/05/16 13:32, Jani Nikula wrote:
> On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote:
>> On 06/05/16 15:33, Chris Wilson wrote:
>>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> To be used for more efficient Gen range checking.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h         |  1 +
>>>>    drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
>>>>    drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
>>>>    3 files changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 15fcbcece13c..935e381407ba 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table {
>>>>    })
>>>>    #define INTEL_INFO(p) 	(&__I915__(p)->info)
>>>>    #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
>>>>    #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>>>>    #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>>>> index d5a7cfec589b..2c3681757aba 100644
>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
>>>>
>>>>    	/* FIXME: We lack the proper locking here, so only run this on the
>>>>    	 * platforms that need. */
>>>> -	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
>>>> +	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
>>>>    		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
>>>>    	cache->fb.pixel_format = fb->pixel_format;
>>>>    	cache->fb.stride = fb->pitches[0];
>>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine)
>>>>    			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
>>>>
>>>>    	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
>>>> -	if (IS_GEN7(dev))
>>>> +	if (IS_GEN7(dev_priv))
>>>>    		I915_WRITE(GFX_MODE_GEN7,
>>>>    			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
>>>>    			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
>>>>
>>>> -	if (IS_GEN6(dev)) {
>>>> +	if (IS_GEN6(dev_priv)) {
>>>
>>> This chunk shouldn't be in this patch.
>>>
>>> Couldn't improve upon INTEL_GEN_RANGE.
>>>
>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> -Chris
>>
>> INTEL_GEN_IN_RANGE() ?
>>
>> Perhaps emphasises that we're using INclusive ranges?
>
> Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and
> IS_BXT_REVID(). Might as well make this shorter to write.
>
> BR,
> Jani.

We need a notation that will look good (and be efficient) for "before 
GEN x" and "from GEN y on" (or "up to and including GEN p" and "after 
GEN q") -- these make up the vast majority of tests on the GEN number.

Also, we need real clarity on inclusion/exclusion. since..until notation 
is perhaps ambiguous - in particular, is "until" included? English usage 
is not clear here, it's often but not always exclusive!

.Dave.
Jani Nikula May 9, 2016, 2:40 p.m. UTC | #5
On Mon, 09 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote:
> On 09/05/16 13:32, Jani Nikula wrote:
>> On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote:
>>> On 06/05/16 15:33, Chris Wilson wrote:
>>>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> To be used for more efficient Gen range checking.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_drv.h         |  1 +
>>>>>    drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
>>>>>    drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
>>>>>    3 files changed, 8 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index 15fcbcece13c..935e381407ba 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table {
>>>>>    })
>>>>>    #define INTEL_INFO(p) 	(&__I915__(p)->info)
>>>>>    #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>>>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
>>>>>    #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>>>>>    #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>>>>> index d5a7cfec589b..2c3681757aba 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>>>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
>>>>>
>>>>>    	/* FIXME: We lack the proper locking here, so only run this on the
>>>>>    	 * platforms that need. */
>>>>> -	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
>>>>> +	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
>>>>>    		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
>>>>>    	cache->fb.pixel_format = fb->pixel_format;
>>>>>    	cache->fb.stride = fb->pitches[0];
>>>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine)
>>>>>    			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
>>>>>
>>>>>    	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
>>>>> -	if (IS_GEN7(dev))
>>>>> +	if (IS_GEN7(dev_priv))
>>>>>    		I915_WRITE(GFX_MODE_GEN7,
>>>>>    			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
>>>>>    			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
>>>>>
>>>>> -	if (IS_GEN6(dev)) {
>>>>> +	if (IS_GEN6(dev_priv)) {
>>>>
>>>> This chunk shouldn't be in this patch.
>>>>
>>>> Couldn't improve upon INTEL_GEN_RANGE.
>>>>
>>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> -Chris
>>>
>>> INTEL_GEN_IN_RANGE() ?
>>>
>>> Perhaps emphasises that we're using INclusive ranges?
>>
>> Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and
>> IS_BXT_REVID(). Might as well make this shorter to write.
>>
>> BR,
>> Jani.
>
> We need a notation that will look good (and be efficient) for "before 
> GEN x" and "from GEN y on" (or "up to and including GEN p" and "after 
> GEN q") -- these make up the vast majority of tests on the GEN number.
>
> Also, we need real clarity on inclusion/exclusion. since..until notation 
> is perhaps ambiguous - in particular, is "until" included? English usage 
> is not clear here, it's often but not always exclusive!

If you want real clarity, you'll ditch this patch and use

	if (INTEL_GEN(p) >= 5 && INTEL_GEN(p) <= 7)
        	etc...

and be done with it. There's too much churn going on with all the
related macros anyway, so I'm not at all convinced about the patches
anyway.

J.



>
> .Dave.
>
Chris Wilson May 9, 2016, 3:23 p.m. UTC | #6
On Mon, May 09, 2016 at 05:40:39PM +0300, Jani Nikula wrote:
> On Mon, 09 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote:
> > On 09/05/16 13:32, Jani Nikula wrote:
> >> On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote:
> >>> On 06/05/16 15:33, Chris Wilson wrote:
> >>>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote:
> >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>
> >>>>> To be used for more efficient Gen range checking.
> >>>>>
> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> ---
> >>>>>    drivers/gpu/drm/i915/i915_drv.h         |  1 +
> >>>>>    drivers/gpu/drm/i915/intel_fbc.c        |  2 +-
> >>>>>    drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------
> >>>>>    3 files changed, 8 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>> index 15fcbcece13c..935e381407ba 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table {
> >>>>>    })
> >>>>>    #define INTEL_INFO(p) 	(&__I915__(p)->info)
> >>>>>    #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
> >>>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
> >>>>>    #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
> >>>>>    #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >>>>> index d5a7cfec589b..2c3681757aba 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >>>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
> >>>>>
> >>>>>    	/* FIXME: We lack the proper locking here, so only run this on the
> >>>>>    	 * platforms that need. */
> >>>>> -	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
> >>>>> +	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
> >>>>>    		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
> >>>>>    	cache->fb.pixel_format = fb->pixel_format;
> >>>>>    	cache->fb.stride = fb->pitches[0];
> >>>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine)
> >>>>>    			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
> >>>>>
> >>>>>    	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
> >>>>> -	if (IS_GEN7(dev))
> >>>>> +	if (IS_GEN7(dev_priv))
> >>>>>    		I915_WRITE(GFX_MODE_GEN7,
> >>>>>    			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
> >>>>>    			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
> >>>>>
> >>>>> -	if (IS_GEN6(dev)) {
> >>>>> +	if (IS_GEN6(dev_priv)) {
> >>>>
> >>>> This chunk shouldn't be in this patch.
> >>>>
> >>>> Couldn't improve upon INTEL_GEN_RANGE.
> >>>>
> >>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>> -Chris
> >>>
> >>> INTEL_GEN_IN_RANGE() ?
> >>>
> >>> Perhaps emphasises that we're using INclusive ranges?
> >>
> >> Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and
> >> IS_BXT_REVID(). Might as well make this shorter to write.
> >>
> >> BR,
> >> Jani.
> >
> > We need a notation that will look good (and be efficient) for "before 
> > GEN x" and "from GEN y on" (or "up to and including GEN p" and "after 
> > GEN q") -- these make up the vast majority of tests on the GEN number.
> >
> > Also, we need real clarity on inclusion/exclusion. since..until notation 
> > is perhaps ambiguous - in particular, is "until" included? English usage 
> > is not clear here, it's often but not always exclusive!
> 
> If you want real clarity, you'll ditch this patch and use
> 
> 	if (INTEL_GEN(p) >= 5 && INTEL_GEN(p) <= 7)
>         	etc...
> 
> and be done with it. There's too much churn going on with all the
> related macros anyway, so I'm not at all convinced about the patches
> anyway.

IS_GEN(p, start, end) using inclusive ranges is fine.

This is a small patch that generates a remarkable amount of object code
reduction.
-Chris
Jani Nikula May 10, 2016, 7:54 a.m. UTC | #7
On Mon, 09 May 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> IS_GEN(p, start, end) using inclusive ranges is fine.
>
> This is a small patch that generates a remarkable amount of object code
> reduction.

Ok.

I guess 0 is okay for start, but how about a "GEN_FOREVER" style macro
for replacing ->gen >= 5 with IS_GEN(dev_priv, 5, GEN_FOREVER)? This
would be in line with the IS_{SKL,BXT}_REVID checks.


BR,
Jani.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 15fcbcece13c..935e381407ba 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2518,6 +2518,7 @@  struct drm_i915_cmd_table {
 })
 #define INTEL_INFO(p) 	(&__I915__(p)->info)
 #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
+#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s))
 #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
 #define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d5a7cfec589b..2c3681757aba 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -740,7 +740,7 @@  static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
 
 	/* FIXME: We lack the proper locking here, so only run this on the
 	 * platforms that need. */
-	if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7)
+	if (INTEL_GEN_RANGE(dev_priv, 5, 6))
 		cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj);
 	cache->fb.pixel_format = fb->pixel_format;
 	cache->fb.stride = fb->pitches[0];
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 710cd598c701..c3dbb74d048b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -508,7 +508,7 @@  static void intel_ring_setup_status_page(struct intel_engine_cs *engine)
 	 * arises: do we still need this and if so how should we go about
 	 * invalidating the TLB?
 	 */
-	if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) {
+	if (INTEL_GEN_RANGE(dev_priv, 6, 7)) {
 		i915_reg_t reg = RING_INSTPM(engine->mmio_base);
 
 		/* ring should be idle before issuing a sync flush*/
@@ -1222,7 +1222,7 @@  static int init_render_ring(struct intel_engine_cs *engine)
 		return ret;
 
 	/* WaTimedSingleVertexDispatch:cl,bw,ctg,elk,ilk,snb */
-	if (INTEL_INFO(dev)->gen >= 4 && INTEL_INFO(dev)->gen < 7)
+	if (INTEL_GEN_RANGE(dev_priv, 4, 6))
 		I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
 
 	/* We need to disable the AsyncFlip performance optimisations in order
@@ -1231,7 +1231,7 @@  static int init_render_ring(struct intel_engine_cs *engine)
 	 *
 	 * WaDisableAsyncFlipPerfMode:snb,ivb,hsw,vlv
 	 */
-	if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8)
+	if (INTEL_GEN_RANGE(dev_priv, 6, 7))
 		I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(ASYNC_FLIP_PERF_DISABLE));
 
 	/* Required for the hardware to program scanline values for waiting */
@@ -1241,12 +1241,12 @@  static int init_render_ring(struct intel_engine_cs *engine)
 			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT));
 
 	/* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */
-	if (IS_GEN7(dev))
+	if (IS_GEN7(dev_priv))
 		I915_WRITE(GFX_MODE_GEN7,
 			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) |
 			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
 
-	if (IS_GEN6(dev)) {
+	if (IS_GEN6(dev_priv)) {
 		/* From the Sandybridge PRM, volume 1 part 3, page 24:
 		 * "If this bit is set, STCunit will have LRA as replacement
 		 *  policy. [...] This bit must be reset.  LRA replacement
@@ -1256,7 +1256,7 @@  static int init_render_ring(struct intel_engine_cs *engine)
 			   _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB));
 	}
 
-	if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8)
+	if (INTEL_GEN_RANGE(dev_priv, 6, 7))
 		I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
 
 	if (HAS_L3_DPF(dev))