s/ce/cn/ when grabbing guc_state.lock before calling clr_context_registered.
Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 1f9d4fde421f..9b7b4f4e0d91 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1937,9 +1937,9 @@ static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) list_del_init(&cn->guc_id.link); ce->guc_id = cn->guc_id;
- spin_lock(&ce->guc_state.lock); + spin_lock(&cn->guc_state.lock); clr_context_registered(cn); - spin_unlock(&ce->guc_state.lock); + spin_unlock(&cn->guc_state.lock);
set_context_guc_id_invalid(cn);
On 12/9/2021 10:48 AM, Matthew Brost wrote:
s/ce/cn/ when grabbing guc_state.lock before calling clr_context_registered.
Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com
I'm assuming we didn't see any splat from the lockdep assert in clr_context_registered in our CI runs because we never hit this case as it requires 64k+ contexts. Maybe we can add a selftest to purposely exercise this path? Not a blocker for merging this fix.
Daniele
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 1f9d4fde421f..9b7b4f4e0d91 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1937,9 +1937,9 @@ static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) list_del_init(&cn->guc_id.link); ce->guc_id = cn->guc_id;
spin_lock(&ce->guc_state.lock);
spin_lock(&cn->guc_state.lock);
spin_unlock(&ce->guc_state.lock);
spin_unlock(&cn->guc_state.lock);set_context_guc_id_invalid(cn);
On 09/12/2021 19:14, Daniele Ceraolo Spurio wrote:
On 12/9/2021 10:48 AM, Matthew Brost wrote:
s/ce/cn/ when grabbing guc_state.lock before calling clr_context_registered.
Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
I think Cc: stable is not needed here:
$ git tag --contains 0f7976506de61 drm-intel-fixes-2021-11-18 drm-intel-gt-next-2021-10-08 drm-intel-gt-next-2021-10-21 drm-intel-gt-next-2021-11-22 drm-intel-next-2021-10-15 drm-intel-next-fixes-2021-11-09 v5.16-rc1 v5.16-rc2 v5.16-rc3 v5.16-rc4
So still can hit 5.16 via fixes. Rodrigo, did I get this right and you will be able to pick it up next week or so?
Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com
I'm assuming we didn't see any splat from the lockdep assert in clr_context_registered in our CI runs because we never hit this case as it requires 64k+ contexts. Maybe we can add a selftest to purposely exercise this path? Not a blocker for merging this fix.
Was the bug found by inspection or reported?
Given the buggy function is called steal_guc_id, so if the implication is there is no testing for guc id stealing, then it indeed please add some coverage ASAP.
Regards,
Tvrtko
Daniele
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 1f9d4fde421f..9b7b4f4e0d91 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1937,9 +1937,9 @@ static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) list_del_init(&cn->guc_id.link); ce->guc_id = cn->guc_id; - spin_lock(&ce->guc_state.lock); + spin_lock(&cn->guc_state.lock); clr_context_registered(cn); - spin_unlock(&ce->guc_state.lock); + spin_unlock(&cn->guc_state.lock); set_context_guc_id_invalid(cn);
On Fri, 10 Dec 2021, Tvrtko Ursulin tvrtko.ursulin@linux.intel.com wrote:
On 09/12/2021 19:14, Daniele Ceraolo Spurio wrote:
On 12/9/2021 10:48 AM, Matthew Brost wrote:
s/ce/cn/ when grabbing guc_state.lock before calling clr_context_registered.
Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
I think Cc: stable is not needed here:
$ git tag --contains 0f7976506de61 drm-intel-fixes-2021-11-18 drm-intel-gt-next-2021-10-08 drm-intel-gt-next-2021-10-21 drm-intel-gt-next-2021-11-22 drm-intel-next-2021-10-15 drm-intel-next-fixes-2021-11-09 v5.16-rc1 v5.16-rc2 v5.16-rc3 v5.16-rc4
'dim fixes 0f7976506de61' concurs.
BR, Jani.
So still can hit 5.16 via fixes. Rodrigo, did I get this right and you will be able to pick it up next week or so?
Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com
I'm assuming we didn't see any splat from the lockdep assert in clr_context_registered in our CI runs because we never hit this case as it requires 64k+ contexts. Maybe we can add a selftest to purposely exercise this path? Not a blocker for merging this fix.
Was the bug found by inspection or reported?
Given the buggy function is called steal_guc_id, so if the implication is there is no testing for guc id stealing, then it indeed please add some coverage ASAP.
Regards,
Tvrtko
Daniele
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 1f9d4fde421f..9b7b4f4e0d91 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1937,9 +1937,9 @@ static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) list_del_init(&cn->guc_id.link); ce->guc_id = cn->guc_id; - spin_lock(&ce->guc_state.lock); + spin_lock(&cn->guc_state.lock); clr_context_registered(cn); - spin_unlock(&ce->guc_state.lock); + spin_unlock(&cn->guc_state.lock); set_context_guc_id_invalid(cn);
On Fri, Dec 10, 2021 at 08:41:22AM +0000, Tvrtko Ursulin wrote:
On 09/12/2021 19:14, Daniele Ceraolo Spurio wrote:
On 12/9/2021 10:48 AM, Matthew Brost wrote:
s/ce/cn/ when grabbing guc_state.lock before calling clr_context_registered.
Fixes: 0f7976506de61 ("drm/i915/guc: Rework and simplify locking") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
I think Cc: stable is not needed here:
$ git tag --contains 0f7976506de61 drm-intel-fixes-2021-11-18 drm-intel-gt-next-2021-10-08 drm-intel-gt-next-2021-10-21 drm-intel-gt-next-2021-11-22 drm-intel-next-2021-10-15 drm-intel-next-fixes-2021-11-09 v5.16-rc1 v5.16-rc2 v5.16-rc3 v5.16-rc4
So still can hit 5.16 via fixes. Rodrigo, did I get this right and you will be able to pick it up next week or so?
Will remove.
Reviewed-by: Daniele Ceraolo Spurio daniele.ceraolospurio@intel.com
I'm assuming we didn't see any splat from the lockdep assert in clr_context_registered in our CI runs because we never hit this case as it requires 64k+ contexts. Maybe we can add a selftest to purposely exercise this path? Not a blocker for merging this fix.
Was the bug found by inspection or reported?
Internal testing.
Given the buggy function is called steal_guc_id, so if the implication is there is no testing for guc id stealing, then it indeed please add some coverage ASAP.
Will do. I'll aim to get something out next week.
Matt
Regards,
Tvrtko
Daniele
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 1f9d4fde421f..9b7b4f4e0d91 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -1937,9 +1937,9 @@ static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce) list_del_init(&cn->guc_id.link); ce->guc_id = cn->guc_id; - spin_lock(&ce->guc_state.lock); + spin_lock(&cn->guc_state.lock); clr_context_registered(cn); - spin_unlock(&ce->guc_state.lock); + spin_unlock(&cn->guc_state.lock); set_context_guc_id_invalid(cn);
dri-devel@lists.freedesktop.org
-
Daniele Ceraolo Spurio -
Jani Nikula -
Matthew Brost -
Tvrtko Ursulin