[Intel-gfx] [bug report] drm/i915/guc: Add support for data reporting in GuC responses

[Michal Wajdeczko]

Hello Dan,

On 18.06.2021 09:38, Dan Carpenter wrote:
> Hello Michal Wajdeczko,
> 
> The patch b839a869dfc9: "drm/i915/guc: Add support for data reporting
> in GuC responses" from Mar 26, 2018, leads to the following static
> checker warning:
> 
> 	drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c:307 intel_guc_ct_enable()
> 	error: passing non negative 4095 to ERR_PTR

Thanks for catching that!

Please note that data returned by these CTB (de)registration actions is
defined as MBZ, so this should never happen.

Anyway, fix posted [1] but since we are in the middle of GuC update [2]
we would rather wait and merge rebased version.

Thanks,
Michal

[1] https://patchwork.freedesktop.org/patch/440046/
[2] https://patchwork.freedesktop.org/series/91106/

> 
> drivers/gpu/drm/i915/gt/uc/.intel_guc.c
>    405          intel_guc_notify(guc);
>    406  
>    407          /*
>    408           * No GuC command should ever take longer than 10ms.
>    409           * Fast commands should still complete in 10us.
>    410           */
>    411          ret = __intel_wait_for_register_fw(uncore,
>    412                                             guc_send_reg(guc, 0),
>    413                                             INTEL_GUC_MSG_TYPE_MASK,
>    414                                             INTEL_GUC_MSG_TYPE_RESPONSE <<
>    415                                             INTEL_GUC_MSG_TYPE_SHIFT,
>    416                                             10, 10, &status);
>    417          /* If GuC explicitly returned an error, convert it to -EIO */
>    418          if (!ret && !INTEL_GUC_MSG_IS_RESPONSE_SUCCESS(status))
>    419                  ret = -EIO;
> 
> If __intel_wait_for_register_fw() fails then either "ret" is set or
> "status" status has a code and "ret" becomes -EIO.
> 
>    420  
>    421          if (ret) {
>    422                  DRM_ERROR("MMIO: GuC action %#x failed with error %d %#x\n",
>    423                            action[0], ret, status);
>    424                  goto out;
> 
> So if there is any errors we return here.
> 
>    425          }
>    426  
>    427          if (response_buf) {
>    428                  int count = min(response_buf_size, guc->send_regs.count - 1);
>    429  
>    430                  for (i = 0; i < count; i++)
>    431                          response_buf[i] = intel_uncore_read(uncore,
>    432                                                              guc_send_reg(guc, i + 1));
>    433          }
>    434  
>    435          /* Use data from the GuC response as our return value */
>    436          ret = INTEL_GUC_MSG_TO_DATA(status);
> 
> But this is setting "ret" to something positive in the 0xffff range.
> The caller treats it as an error code.
> 
>    437  
>    438  out:
>    439          intel_uncore_forcewake_put(uncore, guc->send_regs.fw_domains);
>    440          mutex_unlock(&guc->send_mutex);
>    441  
>    442          return ret;
>    443  }
> 
> regards,
> dan carpenter
>