| Message ID | 20230927210357.17461-3-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/4] drm/i915: Introduce intel_gt_mcr_lock_reset() | expand |
On Wed, Sep 27, 2023 at 11:03:56PM +0200, Nirmoy Das wrote: > During resume, the steer semaphore on GT1 was observed to be held. The > hardware team has confirmed the safety of clearing the steer semaphore > during driver load/resume, as no lock acquisitions can occur in this > process by other agents. I guess the question is whether we just want to handle the one case where we've already seen a BIOS snapshot screw up (i.e., specifically on GT1 during resume), or do we want to make this a general sanitization that we do on both GTs at both load and resume, just to be safe? Given that the hardware team has indicated no external agents would be expected to be using steering at the point the driver is loading/resuming, maybe it's best to always do the sanitization on platforms that have a hardware semaphore? Matt > > v2: reset on resume not in intel_gt_init(). > v3: do the reset on intel_gt_resume_early() > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index dab73980c9f1..59cebf205b72 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -13,6 +13,7 @@ > #include "intel_engine_pm.h" > #include "intel_gt.h" > #include "intel_gt_clock_utils.h" > +#include "intel_gt_mcr.h" > #include "intel_gt_pm.h" > #include "intel_gt_print.h" > #include "intel_gt_requests.h" > @@ -218,6 +219,17 @@ void intel_gt_pm_fini(struct intel_gt *gt) > > void intel_gt_resume_early(struct intel_gt *gt) > { > + /* > + * Reset the steer semaphore on GT1, as we have observed it > + * remaining held after a suspend operation. Confirmation > + * from the hardware team ensures the safety of resetting > + * the steer semaphore during driver load/resume, as there > + * are no lock acquisitions during this process by other > + * agents. > + */ > + if (MEDIA_VER(gt->i915) >= 13 && gt->type == GT_MEDIA) > + intel_gt_mcr_lock_reset(gt); > + > intel_uncore_resume_early(gt->uncore); > intel_gt_check_and_clear_faults(gt); > } > -- > 2.41.0 >
Hi Nirmoy, On Wed, Sep 27, 2023 at 11:03:56PM +0200, Nirmoy Das wrote: > During resume, the steer semaphore on GT1 was observed to be held. The > hardware team has confirmed the safety of clearing the steer semaphore > during driver load/resume, as no lock acquisitions can occur in this > process by other agents. > > v2: reset on resume not in intel_gt_init(). > v3: do the reset on intel_gt_resume_early() > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> In the previous version I added my r-b here. Please consider it for the next version: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Even though there are still some quesions coming from Matt. Thanks, Andi > --- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index dab73980c9f1..59cebf205b72 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -13,6 +13,7 @@ > #include "intel_engine_pm.h" > #include "intel_gt.h" > #include "intel_gt_clock_utils.h" > +#include "intel_gt_mcr.h" > #include "intel_gt_pm.h" > #include "intel_gt_print.h" > #include "intel_gt_requests.h" > @@ -218,6 +219,17 @@ void intel_gt_pm_fini(struct intel_gt *gt) > > void intel_gt_resume_early(struct intel_gt *gt) > { > + /* > + * Reset the steer semaphore on GT1, as we have observed it > + * remaining held after a suspend operation. Confirmation > + * from the hardware team ensures the safety of resetting > + * the steer semaphore during driver load/resume, as there > + * are no lock acquisitions during this process by other > + * agents. > + */ > + if (MEDIA_VER(gt->i915) >= 13 && gt->type == GT_MEDIA) > + intel_gt_mcr_lock_reset(gt); > + > intel_uncore_resume_early(gt->uncore); > intel_gt_check_and_clear_faults(gt); > } > -- > 2.41.0
On 9/28/2023 12:35 AM, Matt Roper wrote: > On Wed, Sep 27, 2023 at 11:03:56PM +0200, Nirmoy Das wrote: >> During resume, the steer semaphore on GT1 was observed to be held. The >> hardware team has confirmed the safety of clearing the steer semaphore >> during driver load/resume, as no lock acquisitions can occur in this >> process by other agents. > I guess the question is whether we just want to handle the one case > where we've already seen a BIOS snapshot screw up (i.e., specifically on > GT1 during resume), or do we want to make this a general sanitization > that we do on both GTs at both load and resume, just to be safe? Given > that the hardware team has indicated no external agents would be > expected to be using steering at the point the driver is > loading/resuming, maybe it's best to always do the sanitization on > platforms that have a hardware semaphore? Agree, will make it got all GT. Thanks, Nirmoy > > > Matt > >> v2: reset on resume not in intel_gt_init(). >> v3: do the reset on intel_gt_resume_early() >> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >> index dab73980c9f1..59cebf205b72 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >> @@ -13,6 +13,7 @@ >> #include "intel_engine_pm.h" >> #include "intel_gt.h" >> #include "intel_gt_clock_utils.h" >> +#include "intel_gt_mcr.h" >> #include "intel_gt_pm.h" >> #include "intel_gt_print.h" >> #include "intel_gt_requests.h" >> @@ -218,6 +219,17 @@ void intel_gt_pm_fini(struct intel_gt *gt) >> >> void intel_gt_resume_early(struct intel_gt *gt) >> { >> + /* >> + * Reset the steer semaphore on GT1, as we have observed it >> + * remaining held after a suspend operation. Confirmation >> + * from the hardware team ensures the safety of resetting >> + * the steer semaphore during driver load/resume, as there >> + * are no lock acquisitions during this process by other >> + * agents. >> + */ >> + if (MEDIA_VER(gt->i915) >= 13 && gt->type == GT_MEDIA) >> + intel_gt_mcr_lock_reset(gt); >> + >> intel_uncore_resume_early(gt->uncore); >> intel_gt_check_and_clear_faults(gt); >> } >> -- >> 2.41.0 >>
On 9/28/2023 9:19 AM, Andi Shyti wrote: > Hi Nirmoy, > > On Wed, Sep 27, 2023 at 11:03:56PM +0200, Nirmoy Das wrote: >> During resume, the steer semaphore on GT1 was observed to be held. The >> hardware team has confirmed the safety of clearing the steer semaphore >> during driver load/resume, as no lock acquisitions can occur in this >> process by other agents. >> >> v2: reset on resume not in intel_gt_init(). >> v3: do the reset on intel_gt_resume_early() >> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > In the previous version I added my r-b here. I moved code to different function so wanted to be sure :) > Please consider it > for the next version: > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > Even though there are still some quesions coming from Matt. I will make it generic for all GT in the next version. Thanks, Nirmoy > > Thanks, > Andi > >> --- >> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >> index dab73980c9f1..59cebf205b72 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c >> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c >> @@ -13,6 +13,7 @@ >> #include "intel_engine_pm.h" >> #include "intel_gt.h" >> #include "intel_gt_clock_utils.h" >> +#include "intel_gt_mcr.h" >> #include "intel_gt_pm.h" >> #include "intel_gt_print.h" >> #include "intel_gt_requests.h" >> @@ -218,6 +219,17 @@ void intel_gt_pm_fini(struct intel_gt *gt) >> >> void intel_gt_resume_early(struct intel_gt *gt) >> { >> + /* >> + * Reset the steer semaphore on GT1, as we have observed it >> + * remaining held after a suspend operation. Confirmation >> + * from the hardware team ensures the safety of resetting >> + * the steer semaphore during driver load/resume, as there >> + * are no lock acquisitions during this process by other >> + * agents. >> + */ >> + if (MEDIA_VER(gt->i915) >= 13 && gt->type == GT_MEDIA) >> + intel_gt_mcr_lock_reset(gt); >> + >> intel_uncore_resume_early(gt->uncore); >> intel_gt_check_and_clear_faults(gt); >> } >> -- >> 2.41.0
> > > During resume, the steer semaphore on GT1 was observed to be held. The > > > hardware team has confirmed the safety of clearing the steer semaphore > > > during driver load/resume, as no lock acquisitions can occur in this > > > process by other agents. > > > > > > v2: reset on resume not in intel_gt_init(). > > > v3: do the reset on intel_gt_resume_early() > > > > > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > > In the previous version I added my r-b here. > I moved code to different function so wanted to be sure :) > > Please consider it > > for the next version: > > > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > > > Even though there are still some quesions coming from Matt. > > I will make it generic for all GT in the next version. uuhh... yes, didn't think of it... thanks! Andi
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index dab73980c9f1..59cebf205b72 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -13,6 +13,7 @@ #include "intel_engine_pm.h" #include "intel_gt.h" #include "intel_gt_clock_utils.h" +#include "intel_gt_mcr.h" #include "intel_gt_pm.h" #include "intel_gt_print.h" #include "intel_gt_requests.h" @@ -218,6 +219,17 @@ void intel_gt_pm_fini(struct intel_gt *gt) void intel_gt_resume_early(struct intel_gt *gt) { + /* + * Reset the steer semaphore on GT1, as we have observed it + * remaining held after a suspend operation. Confirmation + * from the hardware team ensures the safety of resetting + * the steer semaphore during driver load/resume, as there + * are no lock acquisitions during this process by other + * agents. + */ + if (MEDIA_VER(gt->i915) >= 13 && gt->type == GT_MEDIA) + intel_gt_mcr_lock_reset(gt); + intel_uncore_resume_early(gt->uncore); intel_gt_check_and_clear_faults(gt); }
During resume, the steer semaphore on GT1 was observed to be held. The hardware team has confirmed the safety of clearing the steer semaphore during driver load/resume, as no lock acquisitions can occur in this process by other agents. v2: reset on resume not in intel_gt_init(). v3: do the reset on intel_gt_resume_early() Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)