| Message ID | 20220507132850.10272-16-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] drm/i915: Drop has_llc from device info | expand |
Quoting José Roberto de Souza (2022-05-07 16:28:50) > No need to have this parameter in intel_device_info struct > as this feature is supported by Broadwell, Haswell all platforms with > display version 9 or newer. This is opposite of the direction we want to move to. We want to embrace the has_xyz flags, instead of the macro trickery. > 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. This is not a very good rationale. If the platform has something, but it becomes disabled in runtime, then we should add an another print after the runtime sanitization has been done. Regards, Joonas > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 +++- > drivers/gpu/drm/i915/i915_pci.c | 3 --- > drivers/gpu/drm/i915/intel_device_info.h | 1 - > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4b1025dbaab2a..4a1edf48d37b9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1306,7 +1306,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > IS_BROADWELL(dev_priv) || \ > IS_HASWELL(dev_priv)) > #define HAS_DP_MST(dev_priv) (HAS_DDI(dev_priv)) > -#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > +#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > + IS_BROADWELL(dev_priv) || \ > + IS_HASWELL(dev_priv)) > #define HAS_PSR(dev_priv) (DISPLAY_VER(dev_priv) >= 9) > #define HAS_PSR2_SEL_FETCH(dev_priv) (DISPLAY_VER(dev_priv) >= 12) > #define HAS_TRANSCODER(dev_priv, trans) ((INTEL_INFO(dev_priv)->display.cpu_transcoder_mask & BIT(trans)) != 0) > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index 5a42acb162a15..6a5b70b3ea2d7 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -523,7 +523,6 @@ static const struct intel_device_info vlv_info = { > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), \ > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP), \ > - .display.has_fpga_dbg = 1, \ > HSW_PIPE_OFFSETS > > #define HSW_PLATFORM \ > @@ -657,7 +656,6 @@ static const struct intel_device_info skl_gt4_info = { > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > - .display.has_fpga_dbg = 1, \ > .display.fbc_mask = BIT(INTEL_FBC_A), \ > .display.has_hdcp = 1, \ > .display.has_dmc = 1, \ > @@ -894,7 +892,6 @@ static const struct intel_device_info adl_s_info = { > .display.has_dmc = 1, \ > .display.has_dsc = 1, \ > .display.fbc_mask = BIT(INTEL_FBC_A), \ > - .display.has_fpga_dbg = 1, \ > .display.has_hdcp = 1, \ > .display.has_hotplug = 1, \ > .display.ver = 13, \ > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 7581ef4a68f94..e61a334b611ac 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -157,7 +157,6 @@ enum intel_ppgtt_type { > func(has_cdclk_crawl); \ > func(has_dmc); \ > func(has_dsc); \ > - func(has_fpga_dbg); \ > func(has_gmch); \ > func(has_hdcp); \ > func(has_hotplug); \ > -- > 2.36.0 >
On Mon, 2022-05-09 at 15:38 +0300, Joonas Lahtinen wrote: > Quoting José Roberto de Souza (2022-05-07 16:28:50) > > No need to have this parameter in intel_device_info struct > > as this feature is supported by Broadwell, Haswell all platforms with > > display version 9 or newer. > > This is opposite of the direction we want to move to. > > We want to embrace the has_xyz flags, instead of the macro trickery. This ever growing flag definition is causing problems when defining new platforms. There is too many features to check if a new platform supports each one of it, what is leading to platform definition errors. Also usually when a feature is dropped a HSD will be filed, so the person taking care of that can just adjust the macro upper platform or IP bound and disable it for good. > > > 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. > > This is not a very good rationale. If the platform has something, but it > becomes disabled in runtime, then we should add an another print after > the runtime sanitization has been done. In my opinion this flags should only change in runtime if the feature is fused off like is done for has_dsc and has_dmc. > > Regards, Joonas > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 4 +++- > > drivers/gpu/drm/i915/i915_pci.c | 3 --- > > drivers/gpu/drm/i915/intel_device_info.h | 1 - > > 3 files changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 4b1025dbaab2a..4a1edf48d37b9 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1306,7 +1306,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > IS_BROADWELL(dev_priv) || \ > > IS_HASWELL(dev_priv)) > > #define HAS_DP_MST(dev_priv) (HAS_DDI(dev_priv)) > > -#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > > +#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > > + IS_BROADWELL(dev_priv) || \ > > + IS_HASWELL(dev_priv)) > > #define HAS_PSR(dev_priv) (DISPLAY_VER(dev_priv) >= 9) > > #define HAS_PSR2_SEL_FETCH(dev_priv) (DISPLAY_VER(dev_priv) >= 12) > > #define HAS_TRANSCODER(dev_priv, trans) ((INTEL_INFO(dev_priv)->display.cpu_transcoder_mask & BIT(trans)) != 0) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > index 5a42acb162a15..6a5b70b3ea2d7 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -523,7 +523,6 @@ static const struct intel_device_info vlv_info = { > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), \ > > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP), \ > > - .display.has_fpga_dbg = 1, \ > > HSW_PIPE_OFFSETS > > > > #define HSW_PLATFORM \ > > @@ -657,7 +656,6 @@ static const struct intel_device_info skl_gt4_info = { > > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > > - .display.has_fpga_dbg = 1, \ > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > .display.has_hdcp = 1, \ > > .display.has_dmc = 1, \ > > @@ -894,7 +892,6 @@ static const struct intel_device_info adl_s_info = { > > .display.has_dmc = 1, \ > > .display.has_dsc = 1, \ > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > - .display.has_fpga_dbg = 1, \ > > .display.has_hdcp = 1, \ > > .display.has_hotplug = 1, \ > > .display.ver = 13, \ > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > index 7581ef4a68f94..e61a334b611ac 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -157,7 +157,6 @@ enum intel_ppgtt_type { > > func(has_cdclk_crawl); \ > > func(has_dmc); \ > > func(has_dsc); \ > > - func(has_fpga_dbg); \ > > func(has_gmch); \ > > func(has_hdcp); \ > > func(has_hotplug); \ > > -- > > 2.36.0 > >
Quoting Souza, Jose (2022-05-09 17:19:28) > On Mon, 2022-05-09 at 15:38 +0300, Joonas Lahtinen wrote: > > Quoting José Roberto de Souza (2022-05-07 16:28:50) > > > No need to have this parameter in intel_device_info struct > > > as this feature is supported by Broadwell, Haswell all platforms with > > > display version 9 or newer. > > > > This is opposite of the direction we want to move to. > > > > We want to embrace the has_xyz flags, instead of the macro trickery. > > This ever growing flag definition is causing problems when defining new platforms. The ever growing macros that change between kernel versions are much more painful than easily printable list per SKU. Just to make it clear, a strict NACK from me for merging any patches into this direction. Regards, Joonas > > There is too many features to check if a new platform supports each one of it, what is leading to platform definition errors. > > Also usually when a feature is dropped a HSD will be filed, so the person taking care of that can just adjust the macro upper platform or IP bound and > disable it for good. > > > > > > 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. > > > > This is not a very good rationale. If the platform has something, but it > > becomes disabled in runtime, then we should add an another print after > > the runtime sanitization has been done. > > In my opinion this flags should only change in runtime if the feature is fused off like is done for has_dsc and has_dmc. > > > > > Regards, Joonas > > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 4 +++- > > > drivers/gpu/drm/i915/i915_pci.c | 3 --- > > > drivers/gpu/drm/i915/intel_device_info.h | 1 - > > > 3 files changed, 3 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > index 4b1025dbaab2a..4a1edf48d37b9 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1306,7 +1306,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > > IS_BROADWELL(dev_priv) || \ > > > IS_HASWELL(dev_priv)) > > > #define HAS_DP_MST(dev_priv) (HAS_DDI(dev_priv)) > > > -#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > > > +#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > > > + IS_BROADWELL(dev_priv) || \ > > > + IS_HASWELL(dev_priv)) > > > #define HAS_PSR(dev_priv) (DISPLAY_VER(dev_priv) >= 9) > > > #define HAS_PSR2_SEL_FETCH(dev_priv) (DISPLAY_VER(dev_priv) >= 12) > > > #define HAS_TRANSCODER(dev_priv, trans) ((INTEL_INFO(dev_priv)->display.cpu_transcoder_mask & BIT(trans)) != 0) > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > > index 5a42acb162a15..6a5b70b3ea2d7 100644 > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > @@ -523,7 +523,6 @@ static const struct intel_device_info vlv_info = { > > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), \ > > > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ > > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP), \ > > > - .display.has_fpga_dbg = 1, \ > > > HSW_PIPE_OFFSETS > > > > > > #define HSW_PLATFORM \ > > > @@ -657,7 +656,6 @@ static const struct intel_device_info skl_gt4_info = { > > > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ > > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > > > - .display.has_fpga_dbg = 1, \ > > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > > .display.has_hdcp = 1, \ > > > .display.has_dmc = 1, \ > > > @@ -894,7 +892,6 @@ static const struct intel_device_info adl_s_info = { > > > .display.has_dmc = 1, \ > > > .display.has_dsc = 1, \ > > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > > - .display.has_fpga_dbg = 1, \ > > > .display.has_hdcp = 1, \ > > > .display.has_hotplug = 1, \ > > > .display.ver = 13, \ > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > > index 7581ef4a68f94..e61a334b611ac 100644 > > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > > @@ -157,7 +157,6 @@ enum intel_ppgtt_type { > > > func(has_cdclk_crawl); \ > > > func(has_dmc); \ > > > func(has_dsc); \ > > > - func(has_fpga_dbg); \ > > > func(has_gmch); \ > > > func(has_hdcp); \ > > > func(has_hotplug); \ > > > -- > > > 2.36.0 > > > >
On Tue, 10 May 2022, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > Quoting Souza, Jose (2022-05-09 17:19:28) >> On Mon, 2022-05-09 at 15:38 +0300, Joonas Lahtinen wrote: >> > Quoting José Roberto de Souza (2022-05-07 16:28:50) >> > > No need to have this parameter in intel_device_info struct >> > > as this feature is supported by Broadwell, Haswell all platforms with >> > > display version 9 or newer. >> > >> > This is opposite of the direction we want to move to. >> > >> > We want to embrace the has_xyz flags, instead of the macro trickery. >> >> This ever growing flag definition is causing problems when defining new platforms. > > The ever growing macros that change between kernel versions are much > more painful than easily printable list per SKU. > > Just to make it clear, a strict NACK from me for merging any patches > into this direction. I was hoping to start a discussion to reach consensus on this with my mail [1], adding all maintainers in Cc, but merging started before the discussion really even started. Obviously no further patches on this are to be merged, and the question now is really what to do with the ones that were. Revert? BR, Jani. [1] https://lore.kernel.org/r/87sfpol0kz.fsf@intel.com > > Regards, Joonas > >> >> There is too many features to check if a new platform supports each one of it, what is leading to platform definition errors. >> >> Also usually when a feature is dropped a HSD will be filed, so the person taking care of that can just adjust the macro upper platform or IP bound and >> disable it for good. >> >> > >> > > 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. >> > >> > This is not a very good rationale. If the platform has something, but it >> > becomes disabled in runtime, then we should add an another print after >> > the runtime sanitization has been done. >> >> In my opinion this flags should only change in runtime if the feature is fused off like is done for has_dsc and has_dmc. >> >> > >> > Regards, Joonas >> > >> > > >> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >> > > --- >> > > drivers/gpu/drm/i915/i915_drv.h | 4 +++- >> > > drivers/gpu/drm/i915/i915_pci.c | 3 --- >> > > drivers/gpu/drm/i915/intel_device_info.h | 1 - >> > > 3 files changed, 3 insertions(+), 5 deletions(-) >> > > >> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> > > index 4b1025dbaab2a..4a1edf48d37b9 100644 >> > > --- a/drivers/gpu/drm/i915/i915_drv.h >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > > @@ -1306,7 +1306,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >> > > IS_BROADWELL(dev_priv) || \ >> > > IS_HASWELL(dev_priv)) >> > > #define HAS_DP_MST(dev_priv) (HAS_DDI(dev_priv)) >> > > -#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) >> > > +#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ >> > > + IS_BROADWELL(dev_priv) || \ >> > > + IS_HASWELL(dev_priv)) >> > > #define HAS_PSR(dev_priv) (DISPLAY_VER(dev_priv) >= 9) >> > > #define HAS_PSR2_SEL_FETCH(dev_priv) (DISPLAY_VER(dev_priv) >= 12) >> > > #define HAS_TRANSCODER(dev_priv, trans) ((INTEL_INFO(dev_priv)->display.cpu_transcoder_mask & BIT(trans)) != 0) >> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >> > > index 5a42acb162a15..6a5b70b3ea2d7 100644 >> > > --- a/drivers/gpu/drm/i915/i915_pci.c >> > > +++ b/drivers/gpu/drm/i915/i915_pci.c >> > > @@ -523,7 +523,6 @@ static const struct intel_device_info vlv_info = { >> > > .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), \ >> > > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ >> > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP), \ >> > > - .display.has_fpga_dbg = 1, \ >> > > HSW_PIPE_OFFSETS >> > > >> > > #define HSW_PLATFORM \ >> > > @@ -657,7 +656,6 @@ static const struct intel_device_info skl_gt4_info = { >> > > .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ >> > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ >> > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ >> > > - .display.has_fpga_dbg = 1, \ >> > > .display.fbc_mask = BIT(INTEL_FBC_A), \ >> > > .display.has_hdcp = 1, \ >> > > .display.has_dmc = 1, \ >> > > @@ -894,7 +892,6 @@ static const struct intel_device_info adl_s_info = { >> > > .display.has_dmc = 1, \ >> > > .display.has_dsc = 1, \ >> > > .display.fbc_mask = BIT(INTEL_FBC_A), \ >> > > - .display.has_fpga_dbg = 1, \ >> > > .display.has_hdcp = 1, \ >> > > .display.has_hotplug = 1, \ >> > > .display.ver = 13, \ >> > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >> > > index 7581ef4a68f94..e61a334b611ac 100644 >> > > --- a/drivers/gpu/drm/i915/intel_device_info.h >> > > +++ b/drivers/gpu/drm/i915/intel_device_info.h >> > > @@ -157,7 +157,6 @@ enum intel_ppgtt_type { >> > > func(has_cdclk_crawl); \ >> > > func(has_dmc); \ >> > > func(has_dsc); \ >> > > - func(has_fpga_dbg); \ >> > > func(has_gmch); \ >> > > func(has_hdcp); \ >> > > func(has_hotplug); \ >> > > -- >> > > 2.36.0 >> > > >>
On 10/05/2022 08:41, Jani Nikula wrote: > On Tue, 10 May 2022, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: >> Quoting Souza, Jose (2022-05-09 17:19:28) >>> On Mon, 2022-05-09 at 15:38 +0300, Joonas Lahtinen wrote: >>>> Quoting José Roberto de Souza (2022-05-07 16:28:50) >>>>> No need to have this parameter in intel_device_info struct >>>>> as this feature is supported by Broadwell, Haswell all platforms with >>>>> display version 9 or newer. >>>> >>>> This is opposite of the direction we want to move to. >>>> >>>> We want to embrace the has_xyz flags, instead of the macro trickery. >>> >>> This ever growing flag definition is causing problems when defining new platforms. >> >> The ever growing macros that change between kernel versions are much >> more painful than easily printable list per SKU. >> >> Just to make it clear, a strict NACK from me for merging any patches >> into this direction. > > I was hoping to start a discussion to reach consensus on this with my > mail [1], adding all maintainers in Cc, but merging started before the > discussion really even started. > > Obviously no further patches on this are to be merged, and the question > now is really what to do with the ones that were. Revert? From the process standpoint strictly yes, but in practice I think there is no rush. The ones which got merged I definitely do not like are: Rc6 - because it creates an inconsistency where rc6p remains a device info flag. DDI - because it is not 100% trivial and used from i915_irq.c. But a) I am not sure it is truly on an irq path, and b) it is display code so not my call anyway. (Affects the DP MST one as well by inheritance.) The others are at best lukewarm to me - primarily because I am not convinced there is a benefit to it all. One day the need might come to move them back if some platforms drops support or something, which would be more churn. And it is handy to see a consolidated description of a platform in dmesg when looking at bugs. So just not sure it's an improvement. Give there is much more conversions proposed I guess it may make sense to revert until overall consensus is achieved, since it may be odd to have a handful if we decide to stop there. Regards, Tvrtko
On Wed, 2022-05-11 at 08:39 +0100, Tvrtko Ursulin wrote: > On 10/05/2022 08:41, Jani Nikula wrote: > > On Tue, 10 May 2022, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > > > Quoting Souza, Jose (2022-05-09 17:19:28) > > > > On Mon, 2022-05-09 at 15:38 +0300, Joonas Lahtinen wrote: > > > > > Quoting José Roberto de Souza (2022-05-07 16:28:50) > > > > > > No need to have this parameter in intel_device_info struct > > > > > > as this feature is supported by Broadwell, Haswell all platforms with > > > > > > display version 9 or newer. > > > > > > > > > > This is opposite of the direction we want to move to. > > > > > > > > > > We want to embrace the has_xyz flags, instead of the macro trickery. > > > > > > > > This ever growing flag definition is causing problems when defining new platforms. > > > > > > The ever growing macros that change between kernel versions are much > > > more painful than easily printable list per SKU. > > > > > > Just to make it clear, a strict NACK from me for merging any patches > > > into this direction. > > > > I was hoping to start a discussion to reach consensus on this with my > > mail [1], adding all maintainers in Cc, but merging started before the > > discussion really even started. > > > > Obviously no further patches on this are to be merged, and the question > > now is really what to do with the ones that were. Revert? > > From the process standpoint strictly yes, but in practice I think there > is no rush. > > The ones which got merged I definitely do not like are: > > Rc6 - because it creates an inconsistency where rc6p remains a device > info flag. > > DDI - because it is not 100% trivial and used from i915_irq.c. But a) I > am not sure it is truly on an irq path, and b) it is display code so not > my call anyway. (Affects the DP MST one as well by inheritance.) > > The others are at best lukewarm to me - primarily because I am not > convinced there is a benefit to it all. One day the need might come to > move them back if some platforms drops support or something, which would > be more churn. And it is handy to see a consolidated description of a > platform in dmesg when looking at bugs. So just not sure it's an > improvement. If platform feature list print is a must, we could print those features converted to platform and IP checks in intel_device_info_print_runtime(). > > Give there is much more conversions proposed I guess it may make sense > to revert until overall consensus is achieved, since it may be odd to > have a handful if we decide to stop there. > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4b1025dbaab2a..4a1edf48d37b9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1306,7 +1306,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, IS_BROADWELL(dev_priv) || \ IS_HASWELL(dev_priv)) #define HAS_DP_MST(dev_priv) (HAS_DDI(dev_priv)) -#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) +#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ + IS_BROADWELL(dev_priv) || \ + IS_HASWELL(dev_priv)) #define HAS_PSR(dev_priv) (DISPLAY_VER(dev_priv) >= 9) #define HAS_PSR2_SEL_FETCH(dev_priv) (DISPLAY_VER(dev_priv) >= 12) #define HAS_TRANSCODER(dev_priv, trans) ((INTEL_INFO(dev_priv)->display.cpu_transcoder_mask & BIT(trans)) != 0) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 5a42acb162a15..6a5b70b3ea2d7 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -523,7 +523,6 @@ static const struct intel_device_info vlv_info = { .platform_engine_mask = BIT(RCS0) | BIT(VCS0) | BIT(BCS0) | BIT(VECS0), \ .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP), \ - .display.has_fpga_dbg = 1, \ HSW_PIPE_OFFSETS #define HSW_PLATFORM \ @@ -657,7 +656,6 @@ static const struct intel_device_info skl_gt4_info = { .display.cpu_transcoder_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) | \ BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ - .display.has_fpga_dbg = 1, \ .display.fbc_mask = BIT(INTEL_FBC_A), \ .display.has_hdcp = 1, \ .display.has_dmc = 1, \ @@ -894,7 +892,6 @@ static const struct intel_device_info adl_s_info = { .display.has_dmc = 1, \ .display.has_dsc = 1, \ .display.fbc_mask = BIT(INTEL_FBC_A), \ - .display.has_fpga_dbg = 1, \ .display.has_hdcp = 1, \ .display.has_hotplug = 1, \ .display.ver = 13, \ diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index 7581ef4a68f94..e61a334b611ac 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -157,7 +157,6 @@ enum intel_ppgtt_type { func(has_cdclk_crawl); \ func(has_dmc); \ func(has_dsc); \ - func(has_fpga_dbg); \ func(has_gmch); \ func(has_hdcp); \ func(has_hotplug); \
No need to have this parameter in intel_device_info struct as this feature is supported by Broadwell, Haswell all platforms with display version 9 or newer. 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 <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 +++- drivers/gpu/drm/i915/i915_pci.c | 3 --- drivers/gpu/drm/i915/intel_device_info.h | 1 - 3 files changed, 3 insertions(+), 5 deletions(-)