| Message ID | 20220505193524.276400-5-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [CI,1/7] drm/i915: Drop has_gt_uc from device info | expand |
On 05/05/2022 20:35, José Roberto de Souza wrote: > No need to have this parameter in intel_device_info struct > as all platforms with display version 9 or newer, haswell or broadwell > supports it. > > 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. > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > 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 5538564bc1d25..600d8cee272da 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) > > #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) > -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) > +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > + IS_BROADWELL(dev_priv) || \ > + IS_HASWELL(dev_priv)) This one is a bit borderline, not sure it passes Jani's criteria of simplicity, which I thought was a good one. And from the OCD angle it kind of sucks to expand the conditionals to all call sites (when it's even called from i915_irq.c, justifiably or not I don't know). What is the high level motivation for this work? Also, why is this in drm-intel-gt-next? :) Regards, Tvrtko > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) > #define HAS_PSR_HW_TRACKING(dev_priv) \ > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index 2dc0284629d30..a0693d9ff9cee 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -535,7 +535,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_ddi = 1, \ > .display.has_fpga_dbg = 1, \ > .display.has_dp_mst = 1, \ > .has_rc6p = 0 /* RC6p removed-by HSW */, \ > @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > .has_64bit_reloc = 1, \ > - .display.has_ddi = 1, \ > .display.has_fpga_dbg = 1, \ > .display.fbc_mask = BIT(INTEL_FBC_A), \ > .display.has_hdcp = 1, \ > @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { > .dbuf.size = 4096, \ > .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ > BIT(DBUF_S4), \ > - .display.has_ddi = 1, \ > .display.has_dmc = 1, \ > .display.has_dp_mst = 1, \ > .display.has_dsb = 1, \ > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index bef65e3f02c55..bc71ce48763ad 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -167,7 +167,6 @@ enum intel_ppgtt_type { > func(cursor_needs_physical); \ > func(has_cdclk_crawl); \ > func(has_dmc); \ > - func(has_ddi); \ > func(has_dp_mst); \ > func(has_dsb); \ > func(has_dsc); \
On Mon, 2022-05-09 at 14:32 +0100, Tvrtko Ursulin wrote: > On 05/05/2022 20:35, José Roberto de Souza wrote: > > No need to have this parameter in intel_device_info struct > > as all platforms with display version 9 or newer, haswell or broadwell > > supports it. > > > > 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. > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > 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 5538564bc1d25..600d8cee272da 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) > > > > #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) > > -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) > > +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > > + IS_BROADWELL(dev_priv) || \ > > + IS_HASWELL(dev_priv)) > > This one is a bit borderline, not sure it passes Jani's criteria of > simplicity, which I thought was a good one. And from the OCD angle it > kind of sucks to expand the conditionals to all call sites (when it's > even called from i915_irq.c, justifiably or not I don't know). This might increase code size but I don't believe it will case any performance impact even for interruption handling. > > What is the high level motivation for this work? Add new platforms definitions are becoming huge burden, 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. > > Also, why is this in drm-intel-gt-next? :) To reduce conflicts, moving just one of this patches around already causes conflicts. > > Regards, > > Tvrtko > > > > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > > #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) > > #define HAS_PSR_HW_TRACKING(dev_priv) \ > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > index 2dc0284629d30..a0693d9ff9cee 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -535,7 +535,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_ddi = 1, \ > > .display.has_fpga_dbg = 1, \ > > .display.has_dp_mst = 1, \ > > .has_rc6p = 0 /* RC6p removed-by HSW */, \ > > @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > > .has_64bit_reloc = 1, \ > > - .display.has_ddi = 1, \ > > .display.has_fpga_dbg = 1, \ > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > .display.has_hdcp = 1, \ > > @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { > > .dbuf.size = 4096, \ > > .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ > > BIT(DBUF_S4), \ > > - .display.has_ddi = 1, \ > > .display.has_dmc = 1, \ > > .display.has_dp_mst = 1, \ > > .display.has_dsb = 1, \ > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > index bef65e3f02c55..bc71ce48763ad 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > @@ -167,7 +167,6 @@ enum intel_ppgtt_type { > > func(cursor_needs_physical); \ > > func(has_cdclk_crawl); \ > > func(has_dmc); \ > > - func(has_ddi); \ > > func(has_dp_mst); \ > > func(has_dsb); \ > > func(has_dsc); \
On Mon, 09 May 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 05/05/2022 20:35, José Roberto de Souza wrote: >> No need to have this parameter in intel_device_info struct >> as all platforms with display version 9 or newer, haswell or broadwell >> supports it. >> >> 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. >> >> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >> 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 5538564bc1d25..600d8cee272da 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >> #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) >> >> #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) >> -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) >> +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ >> + IS_BROADWELL(dev_priv) || \ >> + IS_HASWELL(dev_priv)) > > This one is a bit borderline, not sure it passes Jani's criteria of > simplicity, which I thought was a good one. And from the OCD angle it > kind of sucks to expand the conditionals to all call sites (when it's > even called from i915_irq.c, justifiably or not I don't know). > > What is the high level motivation for this work? Yeah, just don't merge when there's open discussion. Get the acks. > Also, why is this in drm-intel-gt-next? :) Without the smiley, actually. *ALL* refactoring like this *MUST* go through drm-intel-next. This is now a source for conflicts for at least 4-6 weeks until we can merge drm-intel-gt-next -> drm-next -> drm-intel-next. BR, Jani. > > Regards, > > Tvrtko > > >> #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) >> #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) >> #define HAS_PSR_HW_TRACKING(dev_priv) \ >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >> index 2dc0284629d30..a0693d9ff9cee 100644 >> --- a/drivers/gpu/drm/i915/i915_pci.c >> +++ b/drivers/gpu/drm/i915/i915_pci.c >> @@ -535,7 +535,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_ddi = 1, \ >> .display.has_fpga_dbg = 1, \ >> .display.has_dp_mst = 1, \ >> .has_rc6p = 0 /* RC6p removed-by HSW */, \ >> @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { >> BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ >> BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ >> .has_64bit_reloc = 1, \ >> - .display.has_ddi = 1, \ >> .display.has_fpga_dbg = 1, \ >> .display.fbc_mask = BIT(INTEL_FBC_A), \ >> .display.has_hdcp = 1, \ >> @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { >> .dbuf.size = 4096, \ >> .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ >> BIT(DBUF_S4), \ >> - .display.has_ddi = 1, \ >> .display.has_dmc = 1, \ >> .display.has_dp_mst = 1, \ >> .display.has_dsb = 1, \ >> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >> index bef65e3f02c55..bc71ce48763ad 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.h >> +++ b/drivers/gpu/drm/i915/intel_device_info.h >> @@ -167,7 +167,6 @@ enum intel_ppgtt_type { >> func(cursor_needs_physical); \ >> func(has_cdclk_crawl); \ >> func(has_dmc); \ >> - func(has_ddi); \ >> func(has_dp_mst); \ >> func(has_dsb); \ >> func(has_dsc); \
On Mon, 2022-05-09 at 17:05 +0300, Jani Nikula wrote: > On Mon, 09 May 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > On 05/05/2022 20:35, José Roberto de Souza wrote: > > > No need to have this parameter in intel_device_info struct > > > as all platforms with display version 9 or newer, haswell or broadwell > > > supports it. > > > > > > 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. > > > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > 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 5538564bc1d25..600d8cee272da 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > > #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) > > > > > > #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) > > > -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) > > > +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > > > + IS_BROADWELL(dev_priv) || \ > > > + IS_HASWELL(dev_priv)) > > > > This one is a bit borderline, not sure it passes Jani's criteria of > > simplicity, which I thought was a good one. And from the OCD angle it > > kind of sucks to expand the conditionals to all call sites (when it's > > even called from i915_irq.c, justifiably or not I don't know). > > > > What is the high level motivation for this work? > > Yeah, just don't merge when there's open discussion. Get the acks. Sorry, I thought that for this ones we were good to go. > > > Also, why is this in drm-intel-gt-next? :) > > Without the smiley, actually. > > *ALL* refactoring like this *MUST* go through drm-intel-next. My understating was that if it was touching GT it should go to drm-intel-gt-next. > > This is now a source for conflicts for at least 4-6 weeks until we can > merge drm-intel-gt-next -> drm-next -> drm-intel-next. > > > BR, > Jani. > > > > > > Regards, > > > > Tvrtko > > > > > > > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > > > #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) > > > #define HAS_PSR_HW_TRACKING(dev_priv) \ > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > > index 2dc0284629d30..a0693d9ff9cee 100644 > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > @@ -535,7 +535,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_ddi = 1, \ > > > .display.has_fpga_dbg = 1, \ > > > .display.has_dp_mst = 1, \ > > > .has_rc6p = 0 /* RC6p removed-by HSW */, \ > > > @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { > > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > > > .has_64bit_reloc = 1, \ > > > - .display.has_ddi = 1, \ > > > .display.has_fpga_dbg = 1, \ > > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > > .display.has_hdcp = 1, \ > > > @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { > > > .dbuf.size = 4096, \ > > > .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ > > > BIT(DBUF_S4), \ > > > - .display.has_ddi = 1, \ > > > .display.has_dmc = 1, \ > > > .display.has_dp_mst = 1, \ > > > .display.has_dsb = 1, \ > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > > index bef65e3f02c55..bc71ce48763ad 100644 > > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > > @@ -167,7 +167,6 @@ enum intel_ppgtt_type { > > > func(cursor_needs_physical); \ > > > func(has_cdclk_crawl); \ > > > func(has_dmc); \ > > > - func(has_ddi); \ > > > func(has_dp_mst); \ > > > func(has_dsb); \ > > > func(has_dsc); \ >
On 09/05/2022 15:01, Souza, Jose wrote: > On Mon, 2022-05-09 at 14:32 +0100, Tvrtko Ursulin wrote: >> On 05/05/2022 20:35, José Roberto de Souza wrote: >>> No need to have this parameter in intel_device_info struct >>> as all platforms with display version 9 or newer, haswell or broadwell >>> supports it. >>> >>> 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. >>> >>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>> 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 5538564bc1d25..600d8cee272da 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >>> #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) >>> >>> #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) >>> -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) >>> +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ >>> + IS_BROADWELL(dev_priv) || \ >>> + IS_HASWELL(dev_priv)) >> >> This one is a bit borderline, not sure it passes Jani's criteria of >> simplicity, which I thought was a good one. And from the OCD angle it >> kind of sucks to expand the conditionals to all call sites (when it's >> even called from i915_irq.c, justifiably or not I don't know). > > This might increase code size but I don't believe it will case any performance impact even for interruption handling. Probably won't, but its IMO ugly and at some point a death of thousand cuts come to play ie. maybe you can't measure an effect of a single change, but over time pointless wastage of cycles accumulates. Not saying that I looked whether it applies to this concrete example, just a general principle - if the condition is not straightforward I would recommend looking at the number and context of callers. >> What is the high level motivation for this work? > > Add new platforms definitions are becoming huge burden, there is too many features to check if a new platform supports each one of it, what is leading > to platform definition errors. How does this change help with that? That work is always required, no? With flags it is at least mostly centralized in one file and with this series some parts become spread around so you have to not even know what feature supports what, but also where in code to look for places which need to be adjusted. (Example engine reset and further issues when/if other macros start getting out i915_drv.h.) > 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. Or can equally adjust the has flags assignments at a single file. To be clear I don't have a strong preference either way (in principle) at the moment, but think more consensus and discussion is needed here before changing it all. Regards, Tvrtko >> Also, why is this in drm-intel-gt-next? :) > > To reduce conflicts, moving just one of this patches around already causes conflicts. > >> >> Regards, >> >> Tvrtko >> >> >>> #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) >>> #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) >>> #define HAS_PSR_HW_TRACKING(dev_priv) \ >>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >>> index 2dc0284629d30..a0693d9ff9cee 100644 >>> --- a/drivers/gpu/drm/i915/i915_pci.c >>> +++ b/drivers/gpu/drm/i915/i915_pci.c >>> @@ -535,7 +535,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_ddi = 1, \ >>> .display.has_fpga_dbg = 1, \ >>> .display.has_dp_mst = 1, \ >>> .has_rc6p = 0 /* RC6p removed-by HSW */, \ >>> @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { >>> BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ >>> BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ >>> .has_64bit_reloc = 1, \ >>> - .display.has_ddi = 1, \ >>> .display.has_fpga_dbg = 1, \ >>> .display.fbc_mask = BIT(INTEL_FBC_A), \ >>> .display.has_hdcp = 1, \ >>> @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { >>> .dbuf.size = 4096, \ >>> .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ >>> BIT(DBUF_S4), \ >>> - .display.has_ddi = 1, \ >>> .display.has_dmc = 1, \ >>> .display.has_dp_mst = 1, \ >>> .display.has_dsb = 1, \ >>> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >>> index bef65e3f02c55..bc71ce48763ad 100644 >>> --- a/drivers/gpu/drm/i915/intel_device_info.h >>> +++ b/drivers/gpu/drm/i915/intel_device_info.h >>> @@ -167,7 +167,6 @@ enum intel_ppgtt_type { >>> func(cursor_needs_physical); \ >>> func(has_cdclk_crawl); \ >>> func(has_dmc); \ >>> - func(has_ddi); \ >>> func(has_dp_mst); \ >>> func(has_dsb); \ >>> func(has_dsc); \ >
On Mon, 2022-05-09 at 15:27 +0100, Tvrtko Ursulin wrote: > On 09/05/2022 15:01, Souza, Jose wrote: > > On Mon, 2022-05-09 at 14:32 +0100, Tvrtko Ursulin wrote: > > > On 05/05/2022 20:35, José Roberto de Souza wrote: > > > > No need to have this parameter in intel_device_info struct > > > > as all platforms with display version 9 or newer, haswell or broadwell > > > > supports it. > > > > > > > > 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. > > > > > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > 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 5538564bc1d25..600d8cee272da 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > > > > #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) > > > > > > > > #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) > > > > -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) > > > > +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ > > > > + IS_BROADWELL(dev_priv) || \ > > > > + IS_HASWELL(dev_priv)) > > > > > > This one is a bit borderline, not sure it passes Jani's criteria of > > > simplicity, which I thought was a good one. And from the OCD angle it > > > kind of sucks to expand the conditionals to all call sites (when it's > > > even called from i915_irq.c, justifiably or not I don't know). > > > > This might increase code size but I don't believe it will case any performance impact even for interruption handling. > > Probably won't, but its IMO ugly and at some point a death of thousand > cuts come to play ie. maybe you can't measure an effect of a single > change, but over time pointless wastage of cycles accumulates. Not > saying that I looked whether it applies to this concrete example, just a > general principle - if the condition is not straightforward I would > recommend looking at the number and context of callers. > > > > What is the high level motivation for this work? > > > > Add new platforms definitions are becoming huge burden, there is too many features to check if a new platform supports each one of it, what is leading > > to platform definition errors. > > How does this change help with that? That work is always required, no? > With flags it is at least mostly centralized in one file and with this > series some parts become spread around so you have to not even know what > feature supports what, but also where in code to look for places which > need to be adjusted. (Example engine reset and further issues when/if > other macros start getting out i915_drv.h.) There is already several features that don't have a device info flag. What helps is this case is define the HAS_XXX() using IP version. > > > 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. > > Or can equally adjust the has flags assignments at a single file. What happens it that a feature is disabled for a single platform and on the next one the information that from IP X and newer this feature is gone is lost in the source code. > > To be clear I don't have a strong preference either way (in principle) > at the moment, but think more consensus and discussion is needed here > before changing it all. Okay > > Regards, > > Tvrtko > > > > Also, why is this in drm-intel-gt-next? :) > > > > To reduce conflicts, moving just one of this patches around already causes conflicts. > > > > > > > > Regards, > > > > > > Tvrtko > > > > > > > > > > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) > > > > #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) > > > > #define HAS_PSR_HW_TRACKING(dev_priv) \ > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > > > index 2dc0284629d30..a0693d9ff9cee 100644 > > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > > @@ -535,7 +535,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_ddi = 1, \ > > > > .display.has_fpga_dbg = 1, \ > > > > .display.has_dp_mst = 1, \ > > > > .has_rc6p = 0 /* RC6p removed-by HSW */, \ > > > > @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { > > > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ > > > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ > > > > .has_64bit_reloc = 1, \ > > > > - .display.has_ddi = 1, \ > > > > .display.has_fpga_dbg = 1, \ > > > > .display.fbc_mask = BIT(INTEL_FBC_A), \ > > > > .display.has_hdcp = 1, \ > > > > @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { > > > > .dbuf.size = 4096, \ > > > > .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ > > > > BIT(DBUF_S4), \ > > > > - .display.has_ddi = 1, \ > > > > .display.has_dmc = 1, \ > > > > .display.has_dp_mst = 1, \ > > > > .display.has_dsb = 1, \ > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > > > > index bef65e3f02c55..bc71ce48763ad 100644 > > > > --- a/drivers/gpu/drm/i915/intel_device_info.h > > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h > > > > @@ -167,7 +167,6 @@ enum intel_ppgtt_type { > > > > func(cursor_needs_physical); \ > > > > func(has_cdclk_crawl); \ > > > > func(has_dmc); \ > > > > - func(has_ddi); \ > > > > func(has_dp_mst); \ > > > > func(has_dsb); \ > > > > func(has_dsc); \ > >
On Mon, 09 May 2022, "Souza, Jose" <jose.souza@intel.com> wrote: > On Mon, 2022-05-09 at 17:05 +0300, Jani Nikula wrote: >> On Mon, 09 May 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> > On 05/05/2022 20:35, José Roberto de Souza wrote: >> > > No need to have this parameter in intel_device_info struct >> > > as all platforms with display version 9 or newer, haswell or broadwell >> > > supports it. >> > > >> > > 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. >> > > >> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >> > > 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 5538564bc1d25..600d8cee272da 100644 >> > > --- a/drivers/gpu/drm/i915/i915_drv.h >> > > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > > @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >> > > #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) >> > > >> > > #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) >> > > -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) >> > > +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ >> > > + IS_BROADWELL(dev_priv) || \ >> > > + IS_HASWELL(dev_priv)) >> > >> > This one is a bit borderline, not sure it passes Jani's criteria of >> > simplicity, which I thought was a good one. And from the OCD angle it >> > kind of sucks to expand the conditionals to all call sites (when it's >> > even called from i915_irq.c, justifiably or not I don't know). >> > >> > What is the high level motivation for this work? >> >> Yeah, just don't merge when there's open discussion. Get the acks. > > Sorry, I thought that for this ones we were good to go. > >> >> > Also, why is this in drm-intel-gt-next? :) >> >> Without the smiley, actually. >> >> *ALL* refactoring like this *MUST* go through drm-intel-next. > > My understating was that if it was touching GT it should go to > drm-intel-gt-next. More like, if you change gt/gem features or functionality or refactor them in any significant way, they go via drm-intel-gt-next. Stuff that affects i915 core or display most likely should go via drm-intel-next. BR, Jani. > >> >> This is now a source for conflicts for at least 4-6 weeks until we can >> merge drm-intel-gt-next -> drm-next -> drm-intel-next. >> >> >> BR, >> Jani. >> >> >> > >> > Regards, >> > >> > Tvrtko >> > >> > >> > > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) >> > > #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) >> > > #define HAS_PSR_HW_TRACKING(dev_priv) \ >> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c >> > > index 2dc0284629d30..a0693d9ff9cee 100644 >> > > --- a/drivers/gpu/drm/i915/i915_pci.c >> > > +++ b/drivers/gpu/drm/i915/i915_pci.c >> > > @@ -535,7 +535,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_ddi = 1, \ >> > > .display.has_fpga_dbg = 1, \ >> > > .display.has_dp_mst = 1, \ >> > > .has_rc6p = 0 /* RC6p removed-by HSW */, \ >> > > @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { >> > > BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ >> > > BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ >> > > .has_64bit_reloc = 1, \ >> > > - .display.has_ddi = 1, \ >> > > .display.has_fpga_dbg = 1, \ >> > > .display.fbc_mask = BIT(INTEL_FBC_A), \ >> > > .display.has_hdcp = 1, \ >> > > @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { >> > > .dbuf.size = 4096, \ >> > > .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ >> > > BIT(DBUF_S4), \ >> > > - .display.has_ddi = 1, \ >> > > .display.has_dmc = 1, \ >> > > .display.has_dp_mst = 1, \ >> > > .display.has_dsb = 1, \ >> > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h >> > > index bef65e3f02c55..bc71ce48763ad 100644 >> > > --- a/drivers/gpu/drm/i915/intel_device_info.h >> > > +++ b/drivers/gpu/drm/i915/intel_device_info.h >> > > @@ -167,7 +167,6 @@ enum intel_ppgtt_type { >> > > func(cursor_needs_physical); \ >> > > func(has_cdclk_crawl); \ >> > > func(has_dmc); \ >> > > - func(has_ddi); \ >> > > func(has_dp_mst); \ >> > > func(has_dsb); \ >> > > func(has_dsc); \ >> >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5538564bc1d25..600d8cee272da 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1298,7 +1298,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define HAS_DP20(dev_priv) (IS_DG2(dev_priv)) #define HAS_CDCLK_CRAWL(dev_priv) (INTEL_INFO(dev_priv)->display.has_cdclk_crawl) -#define HAS_DDI(dev_priv) (INTEL_INFO(dev_priv)->display.has_ddi) +#define HAS_DDI(dev_priv) (DISPLAY_VER(dev_priv) >= 9 || \ + IS_BROADWELL(dev_priv) || \ + IS_HASWELL(dev_priv)) #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) (INTEL_INFO(dev_priv)->display.has_fpga_dbg) #define HAS_PSR(dev_priv) (INTEL_INFO(dev_priv)->display.has_psr) #define HAS_PSR_HW_TRACKING(dev_priv) \ diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index 2dc0284629d30..a0693d9ff9cee 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -535,7 +535,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_ddi = 1, \ .display.has_fpga_dbg = 1, \ .display.has_dp_mst = 1, \ .has_rc6p = 0 /* RC6p removed-by HSW */, \ @@ -683,7 +682,6 @@ static const struct intel_device_info skl_gt4_info = { BIT(TRANSCODER_C) | BIT(TRANSCODER_EDP) | \ BIT(TRANSCODER_DSI_A) | BIT(TRANSCODER_DSI_C), \ .has_64bit_reloc = 1, \ - .display.has_ddi = 1, \ .display.has_fpga_dbg = 1, \ .display.fbc_mask = BIT(INTEL_FBC_A), \ .display.has_hdcp = 1, \ @@ -932,7 +930,6 @@ static const struct intel_device_info adl_s_info = { .dbuf.size = 4096, \ .dbuf.slice_mask = BIT(DBUF_S1) | BIT(DBUF_S2) | BIT(DBUF_S3) | \ BIT(DBUF_S4), \ - .display.has_ddi = 1, \ .display.has_dmc = 1, \ .display.has_dp_mst = 1, \ .display.has_dsb = 1, \ diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index bef65e3f02c55..bc71ce48763ad 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -167,7 +167,6 @@ enum intel_ppgtt_type { func(cursor_needs_physical); \ func(has_cdclk_crawl); \ func(has_dmc); \ - func(has_ddi); \ func(has_dp_mst); \ func(has_dsb); \ func(has_dsc); \