Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2019-03-25 Thread Tvrtko Ursulin


On 13/11/2018 22:28, Jani Nikula wrote:

On Tue, 13 Nov 2018, Tvrtko Ursulin  wrote:

On 13/11/2018 11:40, Jani Nikula wrote:

On Mon, 12 Nov 2018, Tvrtko Ursulin  wrote:

From: Tvrtko Ursulin 

Introduce subplatform mask to eliminate throughout the code devid checking
sprinkle, mostly courtesy of IS_*_UL[TX] macros.

Subplatform mask initialization is done at runtime device info init.


I kind of like the concept, and I like the centralization of devid
checks in one function, but I've always wanted to take this to one step
further: only specify device ids in i915_pciids.h, and *nowhere* else.

It's perhaps too much duplication to create a device info for all these
variants, but I think it would be possible to make the subplatform info
table driven using macros defined in i915_pciids.h.


It would be much nicer, but how would you do it? Perhaps my imagination
is just strong enough today.


So here's an idea.



Simply by splitting the id's into subplatform parts, for instance where
we have today:

#define INTEL_BDW_GT1_IDS(info)  \
  INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
  INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
  INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
  INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
  INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
  INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

We'd split to:

#define INTEL_BDW_GT1_ULT_IDS(info)  \
  INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
  INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \

#define INTEL_BDW_GT1_ULX_IDS(info)  \
  INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \


So far so good.



#define INTEL_BDW_GT1_IDS(info)  \
  INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
  INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
  INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */


Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
to the above...



Then in i915_pci.c, instead of:

...
INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
...

We'd have:

...
INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
...


...so you don't need to make this change at all. But that's a minor
detail.


And a separate table to map the id's to subplatform values.

Hmm, but we would probably need to extrac the id's from the
INTEL_BDW1_GT_IDS like macros so they can be used in this second site
without the info parameter. Something like the trick for device info
flags, but can it be made to generate a macro? I think not..


Are we shy of macro magic? Pfft.

#undef INTEL_VGA_DEVICE
#define INTEL_VGA_DEVICE(id, info) (id)

static const u32 bdw_ult_ids[] = {
INTEL_BDW_GT1_ULT_IDS(0),
};

static const u32 bdw_ulx_ids[] = {
INTEL_BDW_GT1_ULX_IDS(0),
};

#undef INTEL_VGA_DEVICE

Now you can add another mapping on top with pointers to similar arrays
as above and corresponding subplatform bits. Just need to order the code
to not clobber the real INTEL_VGA_DEVICE needs.

We don't need to split the ult/ulx tables by platform either if we only
care about the subplatform ult/ulx here, just need to remember add all
ult/ulx in corresponding arrays.


I eventually remember this thread/idea and have incorporated it into the 
latest series. Only on trybot for now, but you can have a peek if you 
want to see if it matches your expectations.


Regards,

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

Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-15 Thread Tvrtko Ursulin


On 13/11/2018 22:28, Jani Nikula wrote:

On Tue, 13 Nov 2018, Tvrtko Ursulin  wrote:

On 13/11/2018 11:40, Jani Nikula wrote:

On Mon, 12 Nov 2018, Tvrtko Ursulin  wrote:

From: Tvrtko Ursulin 

Introduce subplatform mask to eliminate throughout the code devid checking
sprinkle, mostly courtesy of IS_*_UL[TX] macros.

Subplatform mask initialization is done at runtime device info init.


I kind of like the concept, and I like the centralization of devid
checks in one function, but I've always wanted to take this to one step
further: only specify device ids in i915_pciids.h, and *nowhere* else.

It's perhaps too much duplication to create a device info for all these
variants, but I think it would be possible to make the subplatform info
table driven using macros defined in i915_pciids.h.


It would be much nicer, but how would you do it? Perhaps my imagination
is just strong enough today.


So here's an idea.



Simply by splitting the id's into subplatform parts, for instance where
we have today:

#define INTEL_BDW_GT1_IDS(info)  \
  INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
  INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
  INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
  INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
  INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
  INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

We'd split to:

#define INTEL_BDW_GT1_ULT_IDS(info)  \
  INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
  INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \

#define INTEL_BDW_GT1_ULX_IDS(info)  \
  INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \


So far so good.



#define INTEL_BDW_GT1_IDS(info)  \
  INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
  INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
  INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */


Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
to the above...



Then in i915_pci.c, instead of:

...
INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
...

We'd have:

...
INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
...


...so you don't need to make this change at all. But that's a minor
detail.


Indeed, makes the change less intrusive.


And a separate table to map the id's to subplatform values.

Hmm, but we would probably need to extrac the id's from the
INTEL_BDW1_GT_IDS like macros so they can be used in this second site
without the info parameter. Something like the trick for device info
flags, but can it be made to generate a macro? I think not..


Are we shy of macro magic? Pfft.

#undef INTEL_VGA_DEVICE
#define INTEL_VGA_DEVICE(id, info) (id)

static const u32 bdw_ult_ids[] = {
INTEL_BDW_GT1_ULT_IDS(0),
};

static const u32 bdw_ulx_ids[] = {
INTEL_BDW_GT1_ULX_IDS(0),
};

#undef INTEL_VGA_DEVICE

Now you can add another mapping on top with pointers to similar arrays
as above and corresponding subplatform bits. Just need to order the code
to not clobber the real INTEL_VGA_DEVICE needs.

We don't need to split the ult/ulx tables by platform either if we only
care about the subplatform ult/ulx here, just need to remember add all
ult/ulx in corresponding arrays.


Nice and simple, thank you!

I am marking this for when I get round updating the series.

Regards,

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


Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-13 Thread Jani Nikula
On Tue, 13 Nov 2018, Tvrtko Ursulin  wrote:
> On 13/11/2018 11:40, Jani Nikula wrote:
>> On Mon, 12 Nov 2018, Tvrtko Ursulin  wrote:
>>> From: Tvrtko Ursulin 
>>>
>>> Introduce subplatform mask to eliminate throughout the code devid checking
>>> sprinkle, mostly courtesy of IS_*_UL[TX] macros.
>>>
>>> Subplatform mask initialization is done at runtime device info init.
>> 
>> I kind of like the concept, and I like the centralization of devid
>> checks in one function, but I've always wanted to take this to one step
>> further: only specify device ids in i915_pciids.h, and *nowhere* else.
>> 
>> It's perhaps too much duplication to create a device info for all these
>> variants, but I think it would be possible to make the subplatform info
>> table driven using macros defined in i915_pciids.h.
>
> It would be much nicer, but how would you do it? Perhaps my imagination 
> is just strong enough today.

So here's an idea.

>
> Simply by splitting the id's into subplatform parts, for instance where 
> we have today:
>
> #define INTEL_BDW_GT1_IDS(info)  \
>  INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>  INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>  INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>  INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
>  INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>  INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */
>
> We'd split to:
>
> #define INTEL_BDW_GT1_ULT_IDS(info)  \
>  INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
>  INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
>
> #define INTEL_BDW_GT1_ULX_IDS(info)  \
>  INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \

So far so good.

>
> #define INTEL_BDW_GT1_IDS(info)  \
>  INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
>  INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
>  INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

Now include INTEL_BDW_GT1_ULT_IDS(info) and INTEL_BDW_GT1_ULX_IDS(info)
to the above...

>
> Then in i915_pci.c, instead of:
>
>   ...
>   INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>   ...
>
> We'd have:
>
>   ...
>   INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
>   INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
>   INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
>   ...

...so you don't need to make this change at all. But that's a minor
detail.

> And a separate table to map the id's to subplatform values.
>
> Hmm, but we would probably need to extrac the id's from the 
> INTEL_BDW1_GT_IDS like macros so they can be used in this second site 
> without the info parameter. Something like the trick for device info 
> flags, but can it be made to generate a macro? I think not..

Are we shy of macro magic? Pfft.

#undef INTEL_VGA_DEVICE
#define INTEL_VGA_DEVICE(id, info) (id)

static const u32 bdw_ult_ids[] = {
INTEL_BDW_GT1_ULT_IDS(0),
};

static const u32 bdw_ulx_ids[] = {
INTEL_BDW_GT1_ULX_IDS(0),
};

#undef INTEL_VGA_DEVICE

Now you can add another mapping on top with pointers to similar arrays
as above and corresponding subplatform bits. Just need to order the code
to not clobber the real INTEL_VGA_DEVICE needs.

We don't need to split the ult/ulx tables by platform either if we only
care about the subplatform ult/ulx here, just need to remember add all
ult/ulx in corresponding arrays.


BR,
Jani.

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


Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-13 Thread Tvrtko Ursulin


On 13/11/2018 11:40, Jani Nikula wrote:

On Mon, 12 Nov 2018, Tvrtko Ursulin  wrote:

From: Tvrtko Ursulin 

Introduce subplatform mask to eliminate throughout the code devid checking
sprinkle, mostly courtesy of IS_*_UL[TX] macros.

Subplatform mask initialization is done at runtime device info init.


I kind of like the concept, and I like the centralization of devid
checks in one function, but I've always wanted to take this to one step
further: only specify device ids in i915_pciids.h, and *nowhere* else.

It's perhaps too much duplication to create a device info for all these
variants, but I think it would be possible to make the subplatform info
table driven using macros defined in i915_pciids.h.


It would be much nicer, but how would you do it? Perhaps my imagination 
is just strong enough today.


Simply by splitting the id's into subplatform parts, for instance where 
we have today:


#define INTEL_BDW_GT1_IDS(info)  \
INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \
INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \
INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

We'd split to:

#define INTEL_BDW_GT1_ULT_IDS(info)  \
INTEL_VGA_DEVICE(0x1602, info), /* GT1 ULT */ \
INTEL_VGA_DEVICE(0x1606, info), /* GT1 ULT */ \

#define INTEL_BDW_GT1_ULX_IDS(info)  \
INTEL_VGA_DEVICE(0x160E, info), /* GT1 ULX */ \

#define INTEL_BDW_GT1_IDS(info)  \
INTEL_VGA_DEVICE(0x160B, info), /* GT1 Iris */ \
INTEL_VGA_DEVICE(0x160A, info), /* GT1 Server */ \
INTEL_VGA_DEVICE(0x160D, info)  /* GT1 Workstation */

Then in i915_pci.c, instead of:

...
INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
...

We'd have:

...
INTEL_BDW_GT1_ULT_IDS(&intel_broadwell_gt1_info),
INTEL_BDW_GT1_ULX_IDS(&intel_broadwell_gt1_info),
INTEL_BDW_GT1_IDS(&intel_broadwell_gt1_info),
...

And a separate table to map the id's to subplatform values.

Hmm, but we would probably need to extrac the id's from the 
INTEL_BDW1_GT_IDS like macros so they can be used in this second site 
without the info parameter. Something like the trick for device info 
flags, but can it be made to generate a macro? I think not..


Regards,

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


Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-13 Thread Jani Nikula
On Mon, 12 Nov 2018, Tvrtko Ursulin  wrote:
> From: Tvrtko Ursulin 
>
> Introduce subplatform mask to eliminate throughout the code devid checking
> sprinkle, mostly courtesy of IS_*_UL[TX] macros.
>
> Subplatform mask initialization is done at runtime device info init.

I kind of like the concept, and I like the centralization of devid
checks in one function, but I've always wanted to take this to one step
further: only specify device ids in i915_pciids.h, and *nowhere* else.

It's perhaps too much duplication to create a device info for all these
variants, but I think it would be possible to make the subplatform info
table driven using macros defined in i915_pciids.h.

I think Rodrigo had patches to define CNL port F in terms of num_ports,
but perhaps the subplatform approach works better for that.

BR,
Jani.


>
> v2: Fixed IS_SUBPLATFORM. Updated commit msg.
> v3: Chris was right, there is an ordering problem.
> v4: Drop mask sharing, rename title, rebase.
>
> Signed-off-by: Tvrtko Ursulin 
> Suggested-by: Chris Wilson 
> Cc: Chris Wilson 
> Cc: Jani Nikula 
> ---
>  drivers/gpu/drm/i915/i915_drv.c  |  2 +
>  drivers/gpu/drm/i915/i915_drv.h  | 59 +
>  drivers/gpu/drm/i915/intel_device_info.c | 65 
>  drivers/gpu/drm/i915/intel_device_info.h | 18 +++
>  4 files changed, 108 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4f5ddc3d2f4d..14c199438978 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1688,6 +1688,8 @@ i915_driver_create(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>   runtime_info->gen_mask = BIT(INTEL_GEN(i915) - 1);
>   runtime_info->platform_mask = BIT(device_info->platform);
>  
> + intel_device_info_subplatform_init(i915);
> +
>   return i915;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 283592dd7023..4ec4a6308fe4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2377,6 +2377,10 @@ static inline unsigned int i915_sg_segment_size(void)
>  
>  #define IS_PLATFORM(dev_priv, p) \
>   ((dev_priv)->runtime_info.platform_mask & BIT(p))
> +#define IS_SUBPLATFORM(dev_priv, p, s) \
> + (IS_PLATFORM(dev_priv, p) && \
> +  ((dev_priv)->runtime_info.subplatform_mask & \
> +   BIT(INTEL_SUBPLATFORM_##s)))
>  
>  #define IS_I830(dev_priv)IS_PLATFORM(dev_priv, INTEL_I830)
>  #define IS_I845G(dev_priv)   IS_PLATFORM(dev_priv, INTEL_I845G)
> @@ -2391,11 +2395,15 @@ static inline unsigned int i915_sg_segment_size(void)
>  #define IS_G45(dev_priv) IS_PLATFORM(dev_priv, INTEL_G45)
>  #define IS_GM45(dev_priv)IS_PLATFORM(dev_priv, INTEL_GM45)
>  #define IS_G4X(dev_priv) (IS_G45(dev_priv) || IS_GM45(dev_priv))
> -#define IS_PINEVIEW_G(dev_priv)  (INTEL_DEVID(dev_priv) == 0xa001)
> -#define IS_PINEVIEW_M(dev_priv)  (INTEL_DEVID(dev_priv) == 0xa011)
> +#define IS_PINEVIEW_G(dev_priv)  \
> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_G)
> +#define IS_PINEVIEW_M(dev_priv)  \
> + IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_M)
>  #define IS_PINEVIEW(dev_priv)IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
>  #define IS_G33(dev_priv) IS_PLATFORM(dev_priv, INTEL_G33)
> -#define IS_IRONLAKE_M(dev_priv)  (INTEL_DEVID(dev_priv) == 0x0046)
> +#define IS_IRONLAKE(dev_priv)IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
> +#define IS_IRONLAKE_M(dev_priv)  \
> + IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, IRONLAKE_M)
>  #define IS_IVYBRIDGE(dev_priv)   IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
>  #define IS_IVB_GT1(dev_priv) (IS_IVYBRIDGE(dev_priv) && \
>INTEL_INFO(dev_priv)->gt == 1)
> @@ -2413,40 +2421,20 @@ static inline unsigned int i915_sg_segment_size(void)
>  #define IS_MOBILE(dev_priv)  (INTEL_INFO(dev_priv)->is_mobile)
>  #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \
>   (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00)
> -#define IS_BDW_ULT(dev_priv) (IS_BROADWELL(dev_priv) && \
> -  ((INTEL_DEVID(dev_priv) & 0xf) == 0x6 ||   
> \
> -  (INTEL_DEVID(dev_priv) & 0xf) == 0xb ||
> \
> -  (INTEL_DEVID(dev_priv) & 0xf) == 0xe))
> -/* ULX machines are also considered ULT. */
> -#define IS_BDW_ULX(dev_priv) (IS_BROADWELL(dev_priv) && \
> -  (INTEL_DEVID(dev_priv) & 0xf) == 0xe)
> +#define IS_BDW_ULT(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULT)
> +#define IS_BDW_ULX(dev_priv) IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULX)
>  #define IS_BDW_GT3(dev_priv) (IS_BROADWELL(dev_priv) && \
>INTEL_INFO(dev_priv)->gt == 3)
> -#define IS_HSW_ULT(dev_priv) (IS_HASWELL(dev_priv) && \
> -  (IN

Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-13 Thread Tvrtko Ursulin


On 12/11/2018 17:29, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-11-12 17:12:41)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
b/drivers/gpu/drm/i915/intel_device_info.c
index 00758d11047b..b9d08428f35b 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -118,6 +118,8 @@ intel_device_info_dump_runtime(const struct 
intel_runtime_device_info *info,
 drm_printf(p, "CS timestamp frequency: %u kHz\n",
info->cs_timestamp_frequency_khz);
  
+   drm_printf(p, "Subplatform mask: %x\n", info->subplatform_mask);


Any chance for a magic decoder ring? Quick identification of ult/ulx I
think would be very nice.


Yes I wasn't happy with not doing that myself. So I think it is 
definitely needed.


Regards,

Tvrtko

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


Re: [Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-12 Thread Chris Wilson
Quoting Tvrtko Ursulin (2018-11-12 17:12:41)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c 
> b/drivers/gpu/drm/i915/intel_device_info.c
> index 00758d11047b..b9d08428f35b 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -118,6 +118,8 @@ intel_device_info_dump_runtime(const struct 
> intel_runtime_device_info *info,
> drm_printf(p, "CS timestamp frequency: %u kHz\n",
>info->cs_timestamp_frequency_khz);
>  
> +   drm_printf(p, "Subplatform mask: %x\n", info->subplatform_mask);

Any chance for a magic decoder ring? Quick identification of ult/ulx I
think would be very nice.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [RFC 6/7] drm/i915: Introduce subplatform concept

2018-11-12 Thread Tvrtko Ursulin
From: Tvrtko Ursulin 

Introduce subplatform mask to eliminate throughout the code devid checking
sprinkle, mostly courtesy of IS_*_UL[TX] macros.

Subplatform mask initialization is done at runtime device info init.

v2: Fixed IS_SUBPLATFORM. Updated commit msg.
v3: Chris was right, there is an ordering problem.
v4: Drop mask sharing, rename title, rebase.

Signed-off-by: Tvrtko Ursulin 
Suggested-by: Chris Wilson 
Cc: Chris Wilson 
Cc: Jani Nikula 
---
 drivers/gpu/drm/i915/i915_drv.c  |  2 +
 drivers/gpu/drm/i915/i915_drv.h  | 59 +
 drivers/gpu/drm/i915/intel_device_info.c | 65 
 drivers/gpu/drm/i915/intel_device_info.h | 18 +++
 4 files changed, 108 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4f5ddc3d2f4d..14c199438978 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1688,6 +1688,8 @@ i915_driver_create(struct pci_dev *pdev, const struct 
pci_device_id *ent)
runtime_info->gen_mask = BIT(INTEL_GEN(i915) - 1);
runtime_info->platform_mask = BIT(device_info->platform);
 
+   intel_device_info_subplatform_init(i915);
+
return i915;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 283592dd7023..4ec4a6308fe4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2377,6 +2377,10 @@ static inline unsigned int i915_sg_segment_size(void)
 
 #define IS_PLATFORM(dev_priv, p) \
((dev_priv)->runtime_info.platform_mask & BIT(p))
+#define IS_SUBPLATFORM(dev_priv, p, s) \
+   (IS_PLATFORM(dev_priv, p) && \
+((dev_priv)->runtime_info.subplatform_mask & \
+ BIT(INTEL_SUBPLATFORM_##s)))
 
 #define IS_I830(dev_priv)  IS_PLATFORM(dev_priv, INTEL_I830)
 #define IS_I845G(dev_priv) IS_PLATFORM(dev_priv, INTEL_I845G)
@@ -2391,11 +2395,15 @@ static inline unsigned int i915_sg_segment_size(void)
 #define IS_G45(dev_priv)   IS_PLATFORM(dev_priv, INTEL_G45)
 #define IS_GM45(dev_priv)  IS_PLATFORM(dev_priv, INTEL_GM45)
 #define IS_G4X(dev_priv)   (IS_G45(dev_priv) || IS_GM45(dev_priv))
-#define IS_PINEVIEW_G(dev_priv)(INTEL_DEVID(dev_priv) == 0xa001)
-#define IS_PINEVIEW_M(dev_priv)(INTEL_DEVID(dev_priv) == 0xa011)
+#define IS_PINEVIEW_G(dev_priv)\
+   IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_G)
+#define IS_PINEVIEW_M(dev_priv)\
+   IS_SUBPLATFORM(dev_priv, INTEL_PINEVIEW, PINEVIEW_M)
 #define IS_PINEVIEW(dev_priv)  IS_PLATFORM(dev_priv, INTEL_PINEVIEW)
 #define IS_G33(dev_priv)   IS_PLATFORM(dev_priv, INTEL_G33)
-#define IS_IRONLAKE_M(dev_priv)(INTEL_DEVID(dev_priv) == 0x0046)
+#define IS_IRONLAKE(dev_priv)  IS_PLATFORM(dev_priv, INTEL_IRONLAKE)
+#define IS_IRONLAKE_M(dev_priv)\
+   IS_SUBPLATFORM(dev_priv, INTEL_IRONLAKE, IRONLAKE_M)
 #define IS_IVYBRIDGE(dev_priv) IS_PLATFORM(dev_priv, INTEL_IVYBRIDGE)
 #define IS_IVB_GT1(dev_priv)   (IS_IVYBRIDGE(dev_priv) && \
 INTEL_INFO(dev_priv)->gt == 1)
@@ -2413,40 +2421,20 @@ static inline unsigned int i915_sg_segment_size(void)
 #define IS_MOBILE(dev_priv)(INTEL_INFO(dev_priv)->is_mobile)
 #define IS_HSW_EARLY_SDV(dev_priv) (IS_HASWELL(dev_priv) && \
(INTEL_DEVID(dev_priv) & 0xFF00) == 0x0C00)
-#define IS_BDW_ULT(dev_priv)   (IS_BROADWELL(dev_priv) && \
-((INTEL_DEVID(dev_priv) & 0xf) == 0x6 ||   
\
-(INTEL_DEVID(dev_priv) & 0xf) == 0xb ||
\
-(INTEL_DEVID(dev_priv) & 0xf) == 0xe))
-/* ULX machines are also considered ULT. */
-#define IS_BDW_ULX(dev_priv)   (IS_BROADWELL(dev_priv) && \
-(INTEL_DEVID(dev_priv) & 0xf) == 0xe)
+#define IS_BDW_ULT(dev_priv)   IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULT)
+#define IS_BDW_ULX(dev_priv)   IS_SUBPLATFORM(dev_priv, INTEL_BROADWELL, ULX)
 #define IS_BDW_GT3(dev_priv)   (IS_BROADWELL(dev_priv) && \
 INTEL_INFO(dev_priv)->gt == 3)
-#define IS_HSW_ULT(dev_priv)   (IS_HASWELL(dev_priv) && \
-(INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00)
+#define IS_HSW_ULT(dev_priv)   IS_SUBPLATFORM(dev_priv, INTEL_HASWELL, ULT)
 #define IS_HSW_GT3(dev_priv)   (IS_HASWELL(dev_priv) && \
 INTEL_INFO(dev_priv)->gt == 3)
 /* ULX machines are also considered ULT. */
-#define IS_HSW_ULX(dev_priv)   (INTEL_DEVID(dev_priv) == 0x0A0E || \
-INTEL_DEVID(dev_priv) == 0x0A1E)
-#define IS_SKL_ULT(dev_priv)   (INTEL_DEVID(dev_priv) == 0x1906 || \
-INTEL_DEVID(dev_priv) == 0x1913 || \
-INTEL_DEVID(dev_priv) == 0x1916 || \
-INTEL_DEVID(dev_priv) == 0x1921 || \
-