[PATCH] drm/xe: Initialize freq_lock even with skip_guc_pc

[Matt Roper]

On Fri, Jan 12, 2024 at 01:16:38PM -0800, Belgaumkar, Vinay wrote:
> 
> On 1/12/2024 1:10 PM, Rodrigo Vivi wrote:
> > On Fri, Jan 12, 2024 at 10:29:50AM -0800, Matt Roper wrote:
> > > gucpc isn't initialized when skip_guc_pc is used, but some of the sysfs
> > > frequency entries still try to grab the uninitialized frequency mutex
> > > before realizing that they need to bail out.  Tweak the init/fini code
> > > flows so that the mutex is always initialized properly.
> > hmmm... good catch.
> > But I honestly don't believe we should be calling/running any function
> > that is trying to get the freq_lock. If guc pc is skip, why would we try
> > to communicate with guc to get or set the freq?
> > 
> > The shared buffers are not set and all. We need a deeper decoupling here.
> > Vinay?
> 
> Yeah, the frequency related sysfs are definitely not being created when
> skip_guc_pc is set. @Matt, do we know which sysfs entries are causing this?

Looks like I was running a build from a couple days ago before commit
"drm/xe: Check skip_guc_pc before setting SLPC flag" landed.  Without
that patch, the frequency sysfs entries still get created, even when
skip_guc_pc is set.  Now that it's in place, this change probably isn't
needed anymore.


Matt

> 
> Thanks,
> 
> Vinay.
> 
> > 
> > > Fixes: 975e4a3795d4 ("drm/xe: Manually setup C6 when skip_guc_pc is set")
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_guc_pc.c | 9 +++++----
> > >   1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > index f71085228cb3..67ce24fae79d 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > @@ -964,11 +964,12 @@ void xe_guc_pc_fini(struct xe_guc_pc *pc)
> > >   	if (xe->info.skip_guc_pc) {
> > >   		xe_gt_idle_disable_c6(pc_to_gt(pc));
> > > -		return;
> > > +		goto out;
> > >   	}
> > >   	XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> > >   	XE_WARN_ON(xe_guc_pc_stop(pc));
> > > +out:
> > >   	mutex_destroy(&pc->freq_lock);
> > >   }
> > > @@ -984,11 +985,11 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
> > >   	struct xe_bo *bo;
> > >   	u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data));
> > > -	if (xe->info.skip_guc_pc)
> > > -		return 0;
> > > -
> > >   	mutex_init(&pc->freq_lock);
> > > +	if (xe->info.skip_guc_pc)
> > > +		return 0;
> > > +
> > >   	bo = xe_managed_bo_create_pin_map(xe, tile, size,
> > >   					  XE_BO_CREATE_VRAM_IF_DGFX(tile) |
> > >   					  XE_BO_CREATE_GGTT_BIT);
> > > -- 
> > > 2.43.0
> > > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation