Re: [PATCH 1/2] drm/xe/guc: Prevent use of uninitialized mutex

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jul 02, 2024 at 06:54:05PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 02.07.2024 18:12, Rodrigo Vivi wrote:
> > On Mon, Jul 01, 2024 at 04:15:28PM -0700, Vinay Belgaumkar wrote:
> >> When skip_guc_pc is set and/or this is for a VF.
> >>
> >> Fixes: 3b1592fb7835 ("drm/xe/lnl: Apply Wa_22019338487")
> >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> >> index d88f5e960fbd..f7b468930697 100644
> >> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> >> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> >> @@ -26,6 +26,7 @@
> >>  #include "xe_mmio.h"
> >>  #include "xe_pcode.h"
> >>  #include "xe_pm.h"
> >> +#include "xe_sriov.h"
> >>  #include "xe_wa.h"
> >>  
> >>  #define MCHBAR_MIRROR_BASE_SNB	0x140000
> >> @@ -825,6 +826,9 @@ int xe_guc_pc_restore_stashed_freq(struct xe_guc_pc *pc)
> >>  {
> >>  	int ret = 0;
> >>  
> >> +	if (IS_SRIOV_VF(pc_to_xe(pc)) 
> > 
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > 
> > are all the other accesses protected?
> > likely because the freq sysfs is not available in VF mode?
> 
> in the VF mode we explicitly set info.skip_guc_pc = true and this flag
> is also used by xe_gt_freq_init() which setup sysfs
> 
> [1]
> https://gitlab.freedesktop.org/drm/xe/kernel/-/commit/65336c3fa2cf7f272067be9193303d1ab7c42190
> 
> > 
> > || pc_to_xe(pc)->info.skip_guc_pc)
> >> +		return 0;
> >> +
> > 
> > I don't like this skip_guc_pc... we might revisit later
> > to find a cleaner way...
> 
> +1
> 
> also IMO it is little confusing that xe_guc_pc is still doing some stuff
> if the skip_guc_pc flag is set

indeed, this is also annoying me...

> 
> maybe we should have xe_power component that will relay stuff to
> xe_guc_pc only if GuC PC is enabled and xe_guc_pc itself will be just
> implementing the GuC protocol details ?

well, we already have the xe_pm for power management..
but also xe_gt_freq, perhaps the right place for many of the
raw freq management that is done in here...

and xe_gt_idle for the raw rc6 things...

we need to adjust this layer and leave it really only for guc_pc
interaction.

> 
> > 
> > anyway, let's move with this patch. the rest is follow up.
> > 
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> >>  	mutex_lock(&pc->freq_lock);
> >>  	ret = pc_set_max_freq(pc, pc->stashed_max_freq);
> >>  	if (!ret)
> >> -- 
> >> 2.38.1
> >>