>-----Original Message-----
>From: Ceraolo Spurio, Daniele
>Sent: Friday, June 22, 2018 10:26 AM
>To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; intel-
>gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Spotswood, John A <john.a.spotswood@xxxxxxxxx>; Mateo Lozano, Oscar
><oscar.mateo@xxxxxxxxx>
>Subject: Re: [PATCH] firmware/guc: Remove USES_GUC_SUBMISSION for
>suspend/resume
>
>Commit title is slightly misleading, as the USES_GUC_SUBMISSION is not removed
>from a suspend/resume path. the firmware tag is also confusing since this fixes
>an i915 bug. Maybe something like "drm/i915/guc: Remove
>USES_GUC_SUBMISSION for ads programming" would be clearer
Sure.
Makes sense.
>On 22/06/18 10:05, Anusha Srivatsa wrote:
>> In the guc_ctl_debug_flags, the ads struct is programmed only when
>> USES_GUC_SUBMISSION is satisfied. But, this has to be programmed for
>> all suspend/resume cases.
>> Remove the condition and program the ads struct for both huc loading
>> and guc submission.
>>
>> This issue was noticed when CI threw errors for enable_guc=2 (load
>> huc; disable submission)
>>
>
>Do we need a fixes: tag? Not sure we want this backported since GuC is off by
>default.
Hmm...so the patch - Load Guc,HuC on GLK is still not merged.
Maybe skip the fixes tag?
>> Credits to: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>> Cc: John Spotswood <john.a.spotswood@xxxxxxxxx>
>> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/intel_guc.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 1aff30b..b1d1a10 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -207,6 +207,7 @@ static u32 guc_ctl_debug_flags(struct intel_guc *guc)
>> {
>> u32 level = intel_guc_log_get_level(&guc->log);
>> u32 flags = 0;
>> + u32 ads = 0;
>>
>> if (!GUC_LOG_LEVEL_IS_ENABLED(level))
>> flags |= GUC_LOG_DEFAULT_DISABLED; @@ -217,12 +218,10
>@@ static
>> u32 guc_ctl_debug_flags(struct intel_guc *guc)
>> flags |= GUC_LOG_LEVEL_TO_VERBOSITY(level) <<
>> GUC_LOG_VERBOSITY_SHIFT;
>>
>> - if (USES_GUC_SUBMISSION(guc_to_i915(guc))) {
>> - u32 ads = intel_guc_ggtt_offset(guc, guc->ads_vma)
>> - >> PAGE_SHIFT;
>> + ads = intel_guc_ggtt_offset(guc, guc->ads_vma) <<
>
>You've flipped the shift here. With that fixed:
Oops...thanks for pointing it out.
>Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
And thanks for the review. Will re-spin the patch asap.
Anusha
>> + PAGE_SHIFT;
>>
>> - flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED;
>> - }
>> + flags |= ads << GUC_ADS_ADDR_SHIFT | GUC_ADS_ENABLED;
>>
>> return flags;
>> }
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
