[Intel-gfx] [PATCH v11 5/6] drm/i915/guc: Check the locking status of GuC WOPCM registers

[Yaodong Li]

On 03/01/2018 05:37 AM, Michal Wajdeczko wrote:
> On Thu, 01 Mar 2018 02:01:39 +0100, Jackie Li <yaodong.li at intel.com> 
> wrote:
>
>
>> +
>> +    err = write_and_verify(dev_priv, GUC_WOPCM_SIZE,
>> +                   dev_priv->wopcm.guc.size,
>
> you should use wopcm-> instead dev_priv->wopcm. (same below)
>
>> +                   GUC_WOPCM_SIZE_MASK | GUC_WOPCM_SIZE_LOCKED,
>> +                   GUC_WOPCM_SIZE_LOCKED);
>
> bikeshed: we should set BASE first, then SIZE ;)
I'd like to keep the sequence it as how the existing code is doing this.
Plus It is only called once during init, and now it works perfectly.:-)
>
>> +    if (err)
>> +        goto err_out;
>> +
>> +    huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
>> +    mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
>> +    err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
>> +                   dev_priv->wopcm.guc.base | huc_agent, mask,
>> +                   GUC_WOPCM_OFFSET_VALID);
>
> as the only supported HuC scenario for us is always with
> HUC_LOADING_AGENT_GUC, maybe we should always set this bit,
> but only add to mask for check conditionally? otherwise we
> couldn't run first without and later with HuC... just asking
>
I assume what you are asking is how to deal with the use case that
we first run without HuC firmware, then we unload the driver module
and come back with a HuC FW?

In this case, we need to also recalculate the GuC region and we need
also the reg values accordingly. Unfortunately, we cannot do it now once
the regs were locked.
>
> with function description and use wopcm pointer fixed,
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
Thanks again for the review.:-)
-Jackie