[Intel-xe] [PATCH] drm/xe: Fix error handling in xe_gt_record_default_lrcs

[Matthew Brost]

On Wed, Apr 05, 2023 at 11:21:22AM -0700, Lucas De Marchi wrote:
> On Tue, Apr 04, 2023 at 07:59:19PM -0700, Matthew Brost wrote:
> > Kill engines if an error is encountered in xe_gt_record_default_lrcs so
> > the driver can free these resources on driver teardown.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > index bc821f431c45..3adde2205b39 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.c
> > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > @@ -301,8 +301,12 @@ int xe_gt_record_default_lrcs(struct xe_gt *gt)
> > 
> > 		gt->default_lrc[hwe->class] = default_lrc;
> > put_nop_e:
> > +		if (err)
> > +			xe_engine_kill(nop_e);
> > 		xe_engine_put(nop_e);
> > put_engine:
> > +		if (err)
> > +			xe_engine_kill(e);
> > 		xe_engine_put(e);
> 
> these error handling here mixed with normal flow seem a little bit hard
> to follow. I propose we change them as:
> 
> {
> 	struct xe_engine *e, *nop_e;
> 	struct xe_vm *vm;
> 
> 	for_each_hw_engine(hwe, gt, id) {
> 		void *default_lrc;
> 		
> 		e = nop_e = NULL;
> 
> 		vm = ...
> 		e = ...
> 		if (IS_ERR(e)) {
> 			...
> 			goto fail;
> 		}
> 
> 		...
> 
> 		xe_engine_put(nop_e);
>                 xe_engine_put(e);
>                 xe_vm_put(vm);
> 	}
> 
> 	return 0;
> 
> fail:
> 	if (nop_e) {
> 		xe_engine_kill(nop_e);
> 		xe_engine_put(nop_e);
> 	}
> 
> 	if (e) {
> 		xe_engine_kill(nop_e);
> 		xe_engine_put(nop_e);
> 	}
> 	
> 	xe_vm_put(vm);
> 
> 	/* Wait for engine kill */
> 	msleep(50);
> 
> 	return err;	
> }
> 

Sure.

> Although that msleep() seems arbitrary. Any alternative so we don't need
> to keep tweaking the value for slower platforms? What exactly are we
> waiting on?
>

It is for TDR to kick in and kill the outstanding jobs. Nothing easy
pops out off the top of my head.

Matt

> Lucas De Marchi
> 
> > put_vm:
> > 		xe_vm_put(vm);
> > @@ -310,6 +314,10 @@ int xe_gt_record_default_lrcs(struct xe_gt *gt)
> > 			break;
> > 	}
> > 
> > +	/* Wait for engine kill */
> > +	if (err)
> > +		msleep(50);
> > +
> > 	return err;
> > }
> > 
> > -- 
> > 2.34.1
> >