| Message ID | 20230720210737.761400-3-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update AUX invalidation sequence | expand |
Hi Andy, On Thursday, 20 July 2023 23:07:30 CEST Andi Shyti wrote: > We always assumed that a device might either have AUX or FLAT > CCS, but this is an approximation that is not always true If there exists a device that can have CCSs that fall into either none or both of those categories then I think we should have that device or two listed here as an example, regardless of deducible from the change or not. Or if there are no such devices so far, but we are going to introduce some, then I think we should provide that information here. > as it > requires some further per device checks. > > Add the "has_aux_ccs" flag in the intel_device_info structure in > order to have a per device flag indicating of the AUX CCS. > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > Cc: <stable@vger.kernel.org> # v5.8+ > --- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_pci.c | 5 ++++- > drivers/gpu/drm/i915/intel_device_info.h | 1 + > 4 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/ i915/gt/gen8_engine_cs.c > index 563efee055602..0d4d5e0407a2d 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > @@ -267,7 +267,7 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > else if (engine->class == COMPUTE_CLASS) > flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS; > > - if (!HAS_FLAT_CCS(rq->engine->i915)) > + if (HAS_AUX_CCS(rq->engine->i915)) > count = 8 + 4; > else > count = 8; > @@ -307,7 +307,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) > if (mode & EMIT_INVALIDATE) { > cmd += 2; > > - if (!HAS_FLAT_CCS(rq->engine->i915) && > + if (HAS_AUX_CCS(rq->engine->i915) && > (rq->engine->class == VIDEO_DECODE_CLASS || > rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { > aux_inv = rq->engine->mask & > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/ i915_drv.h > index 682ef2b5c7d59..e9cc048b5727a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -848,6 +848,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > * stored in lmem to support the 3D and media compression formats. > */ > #define HAS_FLAT_CCS(i915) (INTEL_INFO(i915)->has_flat_ccs) > +#define HAS_AUX_CCS(i915) (INTEL_INFO(i915)->has_aux_ccs) > > #define HAS_GT_UC(i915) (INTEL_INFO(i915)->has_gt_uc) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/ i915_pci.c > index fcacdc21643cf..c9ff1d11a9fce 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -643,7 +643,8 @@ static const struct intel_device_info jsl_info = { > TGL_CACHELEVEL, \ > .has_global_mocs = 1, \ > .has_pxp = 1, \ > - .max_pat_index = 3 > + .max_pat_index = 3, \ > + .has_aux_ccs = 1 NIT: Can we please have the last element also followed by comma, like in other places (e.g. see below)? That will simplify future patches. Other than that, LGTM. Thanks, Janusz > > static const struct intel_device_info tgl_info = { > GEN12_FEATURES, > @@ -775,6 +776,7 @@ static const struct intel_device_info dg2_info = { > > static const struct intel_device_info ats_m_info = { > DG2_FEATURES, > + .has_aux_ccs = 1, > .require_force_probe = 1, > .tuning_thread_rr_after_dep = 1, > }; > @@ -827,6 +829,7 @@ static const struct intel_device_info mtl_info = { > .__runtime.media.ip.ver = 13, > PLATFORM(INTEL_METEORLAKE), > .extra_gt_list = xelpmp_extra_gt, > + .has_aux_ccs = 1, > .has_flat_ccs = 0, > .has_gmd_id = 1, > .has_guc_deprivilege = 1, > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/ i915/intel_device_info.h > index dbfe6443457b5..93485507506cc 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -151,6 +151,7 @@ enum intel_ppgtt_type { > func(has_reset_engine); \ > func(has_3d_pipeline); \ > func(has_4tile); \ > + func(has_aux_ccs); \ > func(has_flat_ccs); \ > func(has_global_mocs); \ > func(has_gmd_id); \ > --------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
On 20.07.2023 23:07, Andi Shyti wrote: > We always assumed that a device might either have AUX or FLAT > CCS, but this is an approximation that is not always true as it > requires some further per device checks. > > Add the "has_aux_ccs" flag in the intel_device_info structure in > order to have a per device flag indicating of the AUX CCS. As Matt mentioned in v6, aux_ccs is present also in older platforms. This is about presence/necessity (?) of aux_ccs table invalidation. Maybe has_aux_ccs_inv, dunno? Moreover you define flag per device, but this is rather per engine, theoretically could be also: MTL: .aux_ccs_inv_mask = BIT(RCS0) | BIT(BCS0) | ... Others: .aux_ccs_inv_mask = BIT(RCS0) | ... looks overkill, maybe helper function would be simpler, up to you. Regards Andrzej > > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > Cc: <stable@vger.kernel.org> # v5.8+ > --- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 4 ++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_pci.c | 5 ++++- > drivers/gpu/drm/i915/intel_device_info.h | 1 + > 4 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > index 563efee055602..0d4d5e0407a2d 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > @@ -267,7 +267,7 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > else if (engine->class == COMPUTE_CLASS) > flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS; > > - if (!HAS_FLAT_CCS(rq->engine->i915)) > + if (HAS_AUX_CCS(rq->engine->i915)) > count = 8 + 4; > else > count = 8; > @@ -307,7 +307,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) > if (mode & EMIT_INVALIDATE) { > cmd += 2; > > - if (!HAS_FLAT_CCS(rq->engine->i915) && > + if (HAS_AUX_CCS(rq->engine->i915) && > (rq->engine->class == VIDEO_DECODE_CLASS || > rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { > aux_inv = rq->engine->mask & > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 682ef2b5c7d59..e9cc048b5727a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -848,6 +848,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > * stored in lmem to support the 3D and media compression formats. > */ > #define HAS_FLAT_CCS(i915) (INTEL_INFO(i915)->has_flat_ccs) > +#define HAS_AUX_CCS(i915) (INTEL_INFO(i915)->has_aux_ccs) > > #define HAS_GT_UC(i915) (INTEL_INFO(i915)->has_gt_uc) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index fcacdc21643cf..c9ff1d11a9fce 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -643,7 +643,8 @@ static const struct intel_device_info jsl_info = { > TGL_CACHELEVEL, \ > .has_global_mocs = 1, \ > .has_pxp = 1, \ > - .max_pat_index = 3 > + .max_pat_index = 3, \ > + .has_aux_ccs = 1 > > static const struct intel_device_info tgl_info = { > GEN12_FEATURES, > @@ -775,6 +776,7 @@ static const struct intel_device_info dg2_info = { > > static const struct intel_device_info ats_m_info = { > DG2_FEATURES, > + .has_aux_ccs = 1, > .require_force_probe = 1, > .tuning_thread_rr_after_dep = 1, > }; > @@ -827,6 +829,7 @@ static const struct intel_device_info mtl_info = { > .__runtime.media.ip.ver = 13, > PLATFORM(INTEL_METEORLAKE), > .extra_gt_list = xelpmp_extra_gt, > + .has_aux_ccs = 1, > .has_flat_ccs = 0, > .has_gmd_id = 1, > .has_guc_deprivilege = 1, > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index dbfe6443457b5..93485507506cc 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -151,6 +151,7 @@ enum intel_ppgtt_type { > func(has_reset_engine); \ > func(has_3d_pipeline); \ > func(has_4tile); \ > + func(has_aux_ccs); \ > func(has_flat_ccs); \ > func(has_global_mocs); \ > func(has_gmd_id); \
Hi Andrzej, On Fri, Jul 21, 2023 at 11:41:22AM +0200, Andrzej Hajda wrote: > On 20.07.2023 23:07, Andi Shyti wrote: > > We always assumed that a device might either have AUX or FLAT > > CCS, but this is an approximation that is not always true as it > > requires some further per device checks. > > > > Add the "has_aux_ccs" flag in the intel_device_info structure in > > order to have a per device flag indicating of the AUX CCS. > > As Matt mentioned in v6, aux_ccs is present also in older platforms. > This is about presence/necessity (?) of aux_ccs table invalidation. > Maybe has_aux_ccs_inv, dunno? yes, true! It's aux_ccs_inv. > Moreover you define flag per device, but this is rather per engine, > theoretically could be also: > MTL: > .aux_ccs_inv_mask = BIT(RCS0) | BIT(BCS0) | ... > Others: > .aux_ccs_inv_mask = BIT(RCS0) | ... there is already some engine discrimination that is mandatory later in the series. Doing it here it's a bit replicated. > looks overkill, > maybe helper function would be simpler, up to you. Yes, a helper function that checks on the platform and returns true or false... it looks hardcoded to me, while the info structure is hardcoded by definition and looks easier to maintain by just toggling on/off a single flag in that structure. That's why I decided to place it there. But, because there are already two people suggesting it, then I will try it in v8. Thanks, Andi
Hi Janusz, On Fri, Jul 21, 2023 at 09:25:03AM +0000, Krzysztofik, Janusz wrote: > Hi Andy, > > On Thursday, 20 July 2023 23:07:30 CEST Andi Shyti wrote: > > We always assumed that a device might either have AUX or FLAT > > CCS, but this is an approximation that is not always true > > If there exists a device that can have CCSs that fall into either none or both > of those categories then I think we should have that device or two listed here > as an example, regardless of deducible from the change or not. Or if there > are no such devices so far, but we are going to introduce some, then I think > we should provide that information here. true! I will improve the commit log. [...] > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -643,7 +643,8 @@ static const struct intel_device_info jsl_info = { > > TGL_CACHELEVEL, \ > > .has_global_mocs = 1, \ > > .has_pxp = 1, \ > > - .max_pat_index = 3 > > + .max_pat_index = 3, \ > > + .has_aux_ccs = 1 > > NIT: Can we please have the last element also followed by comma, like in other > places (e.g. see below)? That will simplify future patches. > > Other than that, LGTM. As Andrzej and Matt suggested I will take another approach, i.e. adding a helper function that tells whether the aux invalidation is necessary or not. Thanks, Andi
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index 563efee055602..0d4d5e0407a2d 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -267,7 +267,7 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) else if (engine->class == COMPUTE_CLASS) flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS; - if (!HAS_FLAT_CCS(rq->engine->i915)) + if (HAS_AUX_CCS(rq->engine->i915)) count = 8 + 4; else count = 8; @@ -307,7 +307,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) if (mode & EMIT_INVALIDATE) { cmd += 2; - if (!HAS_FLAT_CCS(rq->engine->i915) && + if (HAS_AUX_CCS(rq->engine->i915) && (rq->engine->class == VIDEO_DECODE_CLASS || rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { aux_inv = rq->engine->mask & diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 682ef2b5c7d59..e9cc048b5727a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -848,6 +848,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, * stored in lmem to support the 3D and media compression formats. */ #define HAS_FLAT_CCS(i915) (INTEL_INFO(i915)->has_flat_ccs) +#define HAS_AUX_CCS(i915) (INTEL_INFO(i915)->has_aux_ccs) #define HAS_GT_UC(i915) (INTEL_INFO(i915)->has_gt_uc) diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index fcacdc21643cf..c9ff1d11a9fce 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -643,7 +643,8 @@ static const struct intel_device_info jsl_info = { TGL_CACHELEVEL, \ .has_global_mocs = 1, \ .has_pxp = 1, \ - .max_pat_index = 3 + .max_pat_index = 3, \ + .has_aux_ccs = 1 static const struct intel_device_info tgl_info = { GEN12_FEATURES, @@ -775,6 +776,7 @@ static const struct intel_device_info dg2_info = { static const struct intel_device_info ats_m_info = { DG2_FEATURES, + .has_aux_ccs = 1, .require_force_probe = 1, .tuning_thread_rr_after_dep = 1, }; @@ -827,6 +829,7 @@ static const struct intel_device_info mtl_info = { .__runtime.media.ip.ver = 13, PLATFORM(INTEL_METEORLAKE), .extra_gt_list = xelpmp_extra_gt, + .has_aux_ccs = 1, .has_flat_ccs = 0, .has_gmd_id = 1, .has_guc_deprivilege = 1, diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h index dbfe6443457b5..93485507506cc 100644 --- a/drivers/gpu/drm/i915/intel_device_info.h +++ b/drivers/gpu/drm/i915/intel_device_info.h @@ -151,6 +151,7 @@ enum intel_ppgtt_type { func(has_reset_engine); \ func(has_3d_pipeline); \ func(has_4tile); \ + func(has_aux_ccs); \ func(has_flat_ccs); \ func(has_global_mocs); \ func(has_gmd_id); \
We always assumed that a device might either have AUX or FLAT CCS, but this is an approximation that is not always true as it requires some further per device checks. Add the "has_aux_ccs" flag in the intel_device_info structure in order to have a per device flag indicating of the AUX CCS. Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> Cc: <stable@vger.kernel.org> # v5.8+ --- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 4 ++-- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_pci.c | 5 ++++- drivers/gpu/drm/i915/intel_device_info.h | 1 + 4 files changed, 8 insertions(+), 3 deletions(-)