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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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

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 ?

> 
> 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
>>