Re: [Intel-gfx] [PATCH 01/16] drm/i915: Drop has_llc from device info
On Mon, 2022-05-09 at 15:52 +0100, Matthew Auld wrote:
> On Mon, 9 May 2022 at 15:05, Souza, Jose wrote:
> >
> > On Mon, 2022-05-09 at 12:09 +0100, Matthew Auld wrote:
> > > On Sat, 7 May 2022 at 14:29, José Roberto de Souza
> > > wrote:
> > > >
> > > > This feature is supported in graphics version 6 and newer in all
> > > > integrated GPUs not including VLC and CHV, so we can drop this flag
> > > > for a not so complicated macro check.
> > >
> > > s/VLC/VLV/ ?
> >
> > yep, thanks.
> >
> > >
> > > There are also some gen9/10 platforms that only have snooping.
> >
> > That is not reflected into current platform definition.
>
> See glk_info/bxt_info.
Oh okay, missed that.
In this case the macro gets too big and is better to keep the flag into device
info.
>
> > Can you point out the spec pages?
> >
> > >
> > > >
> > > > For this flag we were lucky as XE_HP_FEATURES was setting it to true
> > > > while DGFX_FEATURES was setting it to false and xehpsdv and DG2 were
> > > > using those macros in this givin order if it was the other way around,
> > > > some code paths would follow the HAS_LLC path while LLC is not
> > > > available in hardware and was not initialized in software.
> > > >
> > > > As a side effect of the of removal this flag, it will not be printed
> > > > in dmesg during driver load anymore and developers will have to rely
> > > > on to check the macro and compare with platform being used and IP
> > > > versions of it.
> > > >
> > > > Signed-off-by: José Roberto de Souza
> > > > ---
> > > > drivers/gpu/drm/i915/gt/intel_llc.c | 2 +-
> > > > drivers/gpu/drm/i915/i915_drv.h | 5 -
> > > > drivers/gpu/drm/i915/i915_pci.c | 4
> > > > drivers/gpu/drm/i915/intel_device_info.h | 1 -
> > > > 4 files changed, 5 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c
> > > > b/drivers/gpu/drm/i915/gt/intel_llc.c
> > > > index 40e2e28ee6c75..ffcff51ee6e47 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_llc.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_llc.c
> > > > @@ -52,7 +52,7 @@ static bool get_ia_constants(struct intel_llc *llc,
> > > > struct drm_i915_private *i915 = llc_to_gt(llc)->i915;
> > > > struct intel_rps *rps = _to_gt(llc)->rps;
> > > >
> > > > - if (!HAS_LLC(i915) || IS_DGFX(i915))
> > > > + if (!HAS_LLC(i915))
> > > > return false;
> > > >
> > > > if (rps->max_freq <= rps->min_freq)
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 6dfaf7fce9156..fd5269845e9ad 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1236,7 +1236,10 @@ IS_SUBPLATFORM(const struct drm_i915_private
> > > > *i915,
> > > > */
> > > > #define CMDPARSER_USES_GGTT(dev_priv) (GRAPHICS_VER(dev_priv) == 7)
> > > >
> > > > -#define HAS_LLC(dev_priv) (INTEL_INFO(dev_priv)->has_llc)
> > > > +#define HAS_LLC(dev_priv) (!IS_DGFX(dev_priv) &&
> > > > (GRAPHICS_VER(dev_priv) >= 8 || \
> > > > +
> > > > IS_HASWELL(dev_priv) || \
> > > > +
> > > > IS_IVYBRIDGE(dev_priv) || \
> > > > +
> > > > IS_SANDYBRIDGE(dev_priv)))
> > > > #define HAS_4TILE(dev_priv)(INTEL_INFO(dev_priv)->has_4tile)
> > > > #define HAS_SNOOP(dev_priv)(INTEL_INFO(dev_priv)->has_snoop)
> > > > #define HAS_EDRAM(dev_priv)((dev_priv)->edram_size_mb)
> > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > > > b/drivers/gpu/drm/i915/i915_pci.c
> > > > index 799573a5e5a6f..30a32a5d0e3c9 100644
> > > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > > @@ -404,7 +404,6 @@ static const struct intel_device_info ilk_m_info = {
> > > > .display.fbc_mask = BIT(INTEL_FBC_A), \
> > > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> > > > .has_coherent_ggtt = true, \
> > > > - .has_llc = 1, \
> > > > .has_rc6p = 1, \
> > > > .has_rps = true, \
> > > > .dma_mask_size = 40, \
> > > > @@ -454,7 +453,6 @@ static const struct intel_device_info
> > > > snb_m_gt2_info = {
> > > > .display.fbc_mask = BIT(INTEL_FBC_A), \
> > > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> > > > .has_coherent_ggtt = true, \
> > > > - .has_llc = 1, \
> > > > .has_rc6p = 1, \
> > > > .has_rps = true, \
> > > > .dma_mask_size = 40, \
> > > > @@ -878,7 +876,6 @@ static const struct intel_device_info rkl_info = {
> > > >
> > > > #define DGFX_FEATURES \
> > > > .memory_regions = REGION_SMEM | REGION_LMEM |
> > > > REGION_STOLEN_LMEM, \
> > > > - .has_llc = 0, \
> > > > .has_pxp = 0, \
> > > > .has_snoop = 1, \
> > > >
Re: [Intel-gfx] [PATCH 01/16] drm/i915: Drop has_llc from device info
On Mon, 9 May 2022 at 15:05, Souza, Jose wrote:
>
> On Mon, 2022-05-09 at 12:09 +0100, Matthew Auld wrote:
> > On Sat, 7 May 2022 at 14:29, José Roberto de Souza
> > wrote:
> > >
> > > This feature is supported in graphics version 6 and newer in all
> > > integrated GPUs not including VLC and CHV, so we can drop this flag
> > > for a not so complicated macro check.
> >
> > s/VLC/VLV/ ?
>
> yep, thanks.
>
> >
> > There are also some gen9/10 platforms that only have snooping.
>
> That is not reflected into current platform definition.
See glk_info/bxt_info.
> Can you point out the spec pages?
>
> >
> > >
> > > For this flag we were lucky as XE_HP_FEATURES was setting it to true
> > > while DGFX_FEATURES was setting it to false and xehpsdv and DG2 were
> > > using those macros in this givin order if it was the other way around,
> > > some code paths would follow the HAS_LLC path while LLC is not
> > > available in hardware and was not initialized in software.
> > >
> > > As a side effect of the of removal this flag, it will not be printed
> > > in dmesg during driver load anymore and developers will have to rely
> > > on to check the macro and compare with platform being used and IP
> > > versions of it.
> > >
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_llc.c | 2 +-
> > > drivers/gpu/drm/i915/i915_drv.h | 5 -
> > > drivers/gpu/drm/i915/i915_pci.c | 4
> > > drivers/gpu/drm/i915/intel_device_info.h | 1 -
> > > 4 files changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c
> > > b/drivers/gpu/drm/i915/gt/intel_llc.c
> > > index 40e2e28ee6c75..ffcff51ee6e47 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_llc.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_llc.c
> > > @@ -52,7 +52,7 @@ static bool get_ia_constants(struct intel_llc *llc,
> > > struct drm_i915_private *i915 = llc_to_gt(llc)->i915;
> > > struct intel_rps *rps = _to_gt(llc)->rps;
> > >
> > > - if (!HAS_LLC(i915) || IS_DGFX(i915))
> > > + if (!HAS_LLC(i915))
> > > return false;
> > >
> > > if (rps->max_freq <= rps->min_freq)
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 6dfaf7fce9156..fd5269845e9ad 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1236,7 +1236,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > > */
> > > #define CMDPARSER_USES_GGTT(dev_priv) (GRAPHICS_VER(dev_priv) == 7)
> > >
> > > -#define HAS_LLC(dev_priv) (INTEL_INFO(dev_priv)->has_llc)
> > > +#define HAS_LLC(dev_priv) (!IS_DGFX(dev_priv) &&
> > > (GRAPHICS_VER(dev_priv) >= 8 || \
> > > +
> > > IS_HASWELL(dev_priv) || \
> > > +
> > > IS_IVYBRIDGE(dev_priv) || \
> > > +
> > > IS_SANDYBRIDGE(dev_priv)))
> > > #define HAS_4TILE(dev_priv)(INTEL_INFO(dev_priv)->has_4tile)
> > > #define HAS_SNOOP(dev_priv)(INTEL_INFO(dev_priv)->has_snoop)
> > > #define HAS_EDRAM(dev_priv)((dev_priv)->edram_size_mb)
> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > > b/drivers/gpu/drm/i915/i915_pci.c
> > > index 799573a5e5a6f..30a32a5d0e3c9 100644
> > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > @@ -404,7 +404,6 @@ static const struct intel_device_info ilk_m_info = {
> > > .display.fbc_mask = BIT(INTEL_FBC_A), \
> > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> > > .has_coherent_ggtt = true, \
> > > - .has_llc = 1, \
> > > .has_rc6p = 1, \
> > > .has_rps = true, \
> > > .dma_mask_size = 40, \
> > > @@ -454,7 +453,6 @@ static const struct intel_device_info snb_m_gt2_info
> > > = {
> > > .display.fbc_mask = BIT(INTEL_FBC_A), \
> > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> > > .has_coherent_ggtt = true, \
> > > - .has_llc = 1, \
> > > .has_rc6p = 1, \
> > > .has_rps = true, \
> > > .dma_mask_size = 40, \
> > > @@ -878,7 +876,6 @@ static const struct intel_device_info rkl_info = {
> > >
> > > #define DGFX_FEATURES \
> > > .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM,
> > > \
> > > - .has_llc = 0, \
> > > .has_pxp = 0, \
> > > .has_snoop = 1, \
> > > .is_dgfx = 1, \
> > > @@ -985,7 +982,6 @@ static const struct intel_device_info adl_p_info = {
> > > .has_64bit_reloc = 1, \
> > > .has_flat_ccs = 1, \
> > > .has_global_mocs = 1, \
> > > - .has_llc = 1, \
> > > .has_logical_ring_contexts = 1, \
> > > .has_mslices = 1, \
> > > .has_rps = 1, \
> > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > >
Re: [Intel-gfx] [PATCH 01/16] drm/i915: Drop has_llc from device info
On Mon, 2022-05-09 at 12:09 +0100, Matthew Auld wrote:
> On Sat, 7 May 2022 at 14:29, José Roberto de Souza
> wrote:
> >
> > This feature is supported in graphics version 6 and newer in all
> > integrated GPUs not including VLC and CHV, so we can drop this flag
> > for a not so complicated macro check.
>
> s/VLC/VLV/ ?
yep, thanks.
>
> There are also some gen9/10 platforms that only have snooping.
That is not reflected into current platform definition.
Can you point out the spec pages?
>
> >
> > For this flag we were lucky as XE_HP_FEATURES was setting it to true
> > while DGFX_FEATURES was setting it to false and xehpsdv and DG2 were
> > using those macros in this givin order if it was the other way around,
> > some code paths would follow the HAS_LLC path while LLC is not
> > available in hardware and was not initialized in software.
> >
> > As a side effect of the of removal this flag, it will not be printed
> > in dmesg during driver load anymore and developers will have to rely
> > on to check the macro and compare with platform being used and IP
> > versions of it.
> >
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/gt/intel_llc.c | 2 +-
> > drivers/gpu/drm/i915/i915_drv.h | 5 -
> > drivers/gpu/drm/i915/i915_pci.c | 4
> > drivers/gpu/drm/i915/intel_device_info.h | 1 -
> > 4 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c
> > b/drivers/gpu/drm/i915/gt/intel_llc.c
> > index 40e2e28ee6c75..ffcff51ee6e47 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_llc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_llc.c
> > @@ -52,7 +52,7 @@ static bool get_ia_constants(struct intel_llc *llc,
> > struct drm_i915_private *i915 = llc_to_gt(llc)->i915;
> > struct intel_rps *rps = _to_gt(llc)->rps;
> >
> > - if (!HAS_LLC(i915) || IS_DGFX(i915))
> > + if (!HAS_LLC(i915))
> > return false;
> >
> > if (rps->max_freq <= rps->min_freq)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 6dfaf7fce9156..fd5269845e9ad 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1236,7 +1236,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > */
> > #define CMDPARSER_USES_GGTT(dev_priv) (GRAPHICS_VER(dev_priv) == 7)
> >
> > -#define HAS_LLC(dev_priv) (INTEL_INFO(dev_priv)->has_llc)
> > +#define HAS_LLC(dev_priv) (!IS_DGFX(dev_priv) &&
> > (GRAPHICS_VER(dev_priv) >= 8 || \
> > +
> > IS_HASWELL(dev_priv) || \
> > +
> > IS_IVYBRIDGE(dev_priv) || \
> > +
> > IS_SANDYBRIDGE(dev_priv)))
> > #define HAS_4TILE(dev_priv)(INTEL_INFO(dev_priv)->has_4tile)
> > #define HAS_SNOOP(dev_priv)(INTEL_INFO(dev_priv)->has_snoop)
> > #define HAS_EDRAM(dev_priv)((dev_priv)->edram_size_mb)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index 799573a5e5a6f..30a32a5d0e3c9 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -404,7 +404,6 @@ static const struct intel_device_info ilk_m_info = {
> > .display.fbc_mask = BIT(INTEL_FBC_A), \
> > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> > .has_coherent_ggtt = true, \
> > - .has_llc = 1, \
> > .has_rc6p = 1, \
> > .has_rps = true, \
> > .dma_mask_size = 40, \
> > @@ -454,7 +453,6 @@ static const struct intel_device_info snb_m_gt2_info = {
> > .display.fbc_mask = BIT(INTEL_FBC_A), \
> > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> > .has_coherent_ggtt = true, \
> > - .has_llc = 1, \
> > .has_rc6p = 1, \
> > .has_rps = true, \
> > .dma_mask_size = 40, \
> > @@ -878,7 +876,6 @@ static const struct intel_device_info rkl_info = {
> >
> > #define DGFX_FEATURES \
> > .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM, \
> > - .has_llc = 0, \
> > .has_pxp = 0, \
> > .has_snoop = 1, \
> > .is_dgfx = 1, \
> > @@ -985,7 +982,6 @@ static const struct intel_device_info adl_p_info = {
> > .has_64bit_reloc = 1, \
> > .has_flat_ccs = 1, \
> > .has_global_mocs = 1, \
> > - .has_llc = 1, \
> > .has_logical_ring_contexts = 1, \
> > .has_mslices = 1, \
> > .has_rps = 1, \
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > b/drivers/gpu/drm/i915/intel_device_info.h
> > index a2e53b8683285..e8d53c7a1bd83 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -149,7 +149,6 @@ enum intel_ppgtt_type {
> > func(has_heci_gscfi); \
> >
Re: [Intel-gfx] [PATCH 01/16] drm/i915: Drop has_llc from device info
On Sat, 7 May 2022 at 14:29, José Roberto de Souza wrote:
>
> This feature is supported in graphics version 6 and newer in all
> integrated GPUs not including VLC and CHV, so we can drop this flag
> for a not so complicated macro check.
s/VLC/VLV/ ?
There are also some gen9/10 platforms that only have snooping.
>
> For this flag we were lucky as XE_HP_FEATURES was setting it to true
> while DGFX_FEATURES was setting it to false and xehpsdv and DG2 were
> using those macros in this givin order if it was the other way around,
> some code paths would follow the HAS_LLC path while LLC is not
> available in hardware and was not initialized in software.
>
> As a side effect of the of removal this flag, it will not be printed
> in dmesg during driver load anymore and developers will have to rely
> on to check the macro and compare with platform being used and IP
> versions of it.
>
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/gt/intel_llc.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 5 -
> drivers/gpu/drm/i915/i915_pci.c | 4
> drivers/gpu/drm/i915/intel_device_info.h | 1 -
> 4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c
> b/drivers/gpu/drm/i915/gt/intel_llc.c
> index 40e2e28ee6c75..ffcff51ee6e47 100644
> --- a/drivers/gpu/drm/i915/gt/intel_llc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_llc.c
> @@ -52,7 +52,7 @@ static bool get_ia_constants(struct intel_llc *llc,
> struct drm_i915_private *i915 = llc_to_gt(llc)->i915;
> struct intel_rps *rps = _to_gt(llc)->rps;
>
> - if (!HAS_LLC(i915) || IS_DGFX(i915))
> + if (!HAS_LLC(i915))
> return false;
>
> if (rps->max_freq <= rps->min_freq)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6dfaf7fce9156..fd5269845e9ad 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1236,7 +1236,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> */
> #define CMDPARSER_USES_GGTT(dev_priv) (GRAPHICS_VER(dev_priv) == 7)
>
> -#define HAS_LLC(dev_priv) (INTEL_INFO(dev_priv)->has_llc)
> +#define HAS_LLC(dev_priv) (!IS_DGFX(dev_priv) &&
> (GRAPHICS_VER(dev_priv) >= 8 || \
> + IS_HASWELL(dev_priv)
> || \
> +
> IS_IVYBRIDGE(dev_priv) || \
> +
> IS_SANDYBRIDGE(dev_priv)))
> #define HAS_4TILE(dev_priv)(INTEL_INFO(dev_priv)->has_4tile)
> #define HAS_SNOOP(dev_priv)(INTEL_INFO(dev_priv)->has_snoop)
> #define HAS_EDRAM(dev_priv)((dev_priv)->edram_size_mb)
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 799573a5e5a6f..30a32a5d0e3c9 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -404,7 +404,6 @@ static const struct intel_device_info ilk_m_info = {
> .display.fbc_mask = BIT(INTEL_FBC_A), \
> .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> .has_coherent_ggtt = true, \
> - .has_llc = 1, \
> .has_rc6p = 1, \
> .has_rps = true, \
> .dma_mask_size = 40, \
> @@ -454,7 +453,6 @@ static const struct intel_device_info snb_m_gt2_info = {
> .display.fbc_mask = BIT(INTEL_FBC_A), \
> .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0), \
> .has_coherent_ggtt = true, \
> - .has_llc = 1, \
> .has_rc6p = 1, \
> .has_rps = true, \
> .dma_mask_size = 40, \
> @@ -878,7 +876,6 @@ static const struct intel_device_info rkl_info = {
>
> #define DGFX_FEATURES \
> .memory_regions = REGION_SMEM | REGION_LMEM | REGION_STOLEN_LMEM, \
> - .has_llc = 0, \
> .has_pxp = 0, \
> .has_snoop = 1, \
> .is_dgfx = 1, \
> @@ -985,7 +982,6 @@ static const struct intel_device_info adl_p_info = {
> .has_64bit_reloc = 1, \
> .has_flat_ccs = 1, \
> .has_global_mocs = 1, \
> - .has_llc = 1, \
> .has_logical_ring_contexts = 1, \
> .has_mslices = 1, \
> .has_rps = 1, \
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> b/drivers/gpu/drm/i915/intel_device_info.h
> index a2e53b8683285..e8d53c7a1bd83 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -149,7 +149,6 @@ enum intel_ppgtt_type {
> func(has_heci_gscfi); \
> func(has_guc_deprivilege); \
> func(has_l3_dpf); \
> - func(has_llc); \
> func(has_logical_ring_contexts); \
> func(has_mslices); \
> func(has_pooled_eu); \
> --
> 2.36.0
>
