On Fri, Aug 04, 2017 at 02:29:33PM -0700, Daniele Ceraolo Spurio wrote:
>
>
> On 04/08/17 13:40, Michel Thierry wrote:
> > On 8/4/2017 9:26 AM, Michal Wajdeczko wrote:
> > > GuC may return additional data in the command status response.
> > > Format and meaning of this data is action specific.
> > > We will use this non-negative data as a new success return value.
> > >
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > > Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_guc_ct.c | 14 +++++++-------
> > > drivers/gpu/drm/i915/intel_guc_fwif.h | 6 ++++++
> > > drivers/gpu/drm/i915/intel_uc.c | 5 ++++-
> > > 3 files changed, 17 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c
> > > b/drivers/gpu/drm/i915/intel_guc_ct.c
> > > index c4cbec1..1249868 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_ct.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc_ct.c
> > > @@ -387,9 +387,9 @@ static int ctch_send(struct intel_guc *guc,
> > > err = wait_for_response(desc, fence, status);
> > > if (unlikely(err))
> > > return err;
> > > - if (*status != INTEL_GUC_STATUS_SUCCESS)
> > > + if (INTEL_GUC_RECV_TO_STATUS(*status) != INTEL_GUC_STATUS_SUCCESS)
> > > return -EIO;
> > > - return 0;
> > > + return INTEL_GUC_RECV_TO_DATA(*status);
> > > }
> > > /*
> > > @@ -399,18 +399,18 @@ static int intel_guc_send_ct(struct intel_guc
> > > *guc, const u32 *action, u32 len)
> > > {
> > > struct intel_guc_ct_channel *ctch = &guc->ct.host_channel;
> > > u32 status = ~0; /* undefined */
> > > - int err;
> > > + int ret;
> > > mutex_lock(&guc->send_mutex);
> > > - err = ctch_send(guc, ctch, action, len, &status);
> > > - if (unlikely(err)) {
> > > + ret = ctch_send(guc, ctch, action, len, &status);
> > > + if (unlikely(ret < 0)) {
> > > DRM_ERROR("CT: send action %#X failed; err=%d status=%#X\n",
> > > - action[0], err, status);
> > > + action[0], ret, status);
> > > }
> > > mutex_unlock(&guc->send_mutex);
> > > - return err;
> > > + return ret;
> > > }
> > > /**
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > b/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > index 5fa2860..98c0560 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> > > @@ -567,10 +567,16 @@ enum intel_guc_action {
> > > * command in SS0. The response is distinguishable from a command
> > > * by the fact that all the MASK bits are set. The remaining bits
> > > * give more detail.
> > > + * Bits [16:27] are reserved for optional data reporting.
>
> mmm, from the information I can find the optional data reporting bits are
> only [16:22], while [23:27] should be MBZ. Are you extending the range to
> cope with future changes on the GuC side or am I missing something? If it is
> the first case, my personal preference would be to stick with whatever is in
> the GuC API to avoid confusion. Since you're adding all the defines below it
> should be trivial to extend it if we ever need to.
It's the former case. But by looking the same information, only [15:0] bits are
declared for success/failure code, and [27:23] are MBZ for specific action.
So current proposal is still in line with that spec.
Michal
>
> > > */
> > > #define INTEL_GUC_RECV_MASK ((u32)0xF0000000)
> > > #define INTEL_GUC_RECV_IS_RESPONSE(x) ((u32)(x) >=
> > > INTEL_GUC_RECV_MASK)
> > > #define INTEL_GUC_RECV_STATUS(x) (INTEL_GUC_RECV_MASK | (x))
> > > +#define INTEL_GUC_RECV_DATA_SHIFT 16
> > > +#define INTEL_GUC_RECV_DATA_MASK (0xFFF << INTEL_GUC_RECV_DATA_SHIFT)
> > > +#define INTEL_GUC_RECV_TO_STATUS(x) ((x) & ~
> > > INTEL_GUC_RECV_DATA_MASK)
> >
> > checkpatch should have complained about the blank space after '~'.
> >
> > > +#define INTEL_GUC_RECV_TO_DATA(x) (((x) &
> > > INTEL_GUC_RECV_DATA_MASK) >> \
> > > + INTEL_GUC_RECV_DATA_SHIFT)
> > > /* GUC will return status back to SOFT_SCRATCH_O_REG */
> > > enum intel_guc_status {
> > > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > > b/drivers/gpu/drm/i915/intel_uc.c
> > > index 27e072c..ff25477 100644
> > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > @@ -502,7 +502,7 @@ int intel_guc_send_mmio(struct intel_guc *guc,
> > > const u32 *action, u32 len)
> > > INTEL_GUC_RECV_MASK,
> > > INTEL_GUC_RECV_MASK,
> > > 10, 10, &status);
> > > - if (status != INTEL_GUC_STATUS_SUCCESS) {
> > > + if (INTEL_GUC_RECV_TO_STATUS(status) != INTEL_GUC_STATUS_SUCCESS) {
> > > /*
> > > * Either the GuC explicitly returned an error (which
> > > * we convert to -EIO here) or no response at all was
> > > @@ -514,6 +514,9 @@ int intel_guc_send_mmio(struct intel_guc *guc,
> > > const u32 *action, u32 len)
> > > DRM_WARN("INTEL_GUC_SEND: Action 0x%X failed;"
> > > " ret=%d status=0x%08X response=0x%08X\n",
> > > action[0], ret, status, I915_READ(SOFT_SCRATCH(15)));
> > > + } else {
> > > + /* Use data encoded in status dword as return value */
> > > + ret = INTEL_GUC_RECV_TO_DATA(status);
> > > }
> > > intel_uncore_forcewake_put(dev_priv, guc->send_regs.fw_domains);
> > >
> >
> > Other than the blank space after that '~', it looks good to me.
> >
> > Just a note, for other people reading this; there are 3 cases in which
> > intel_guc_send return value is only checked for truthiness (i.e.
> > __guc_allocate_doorbell, __guc_deallocate_doorbell and
> > intel_guc_sample_forcewake callers are checking for 'if (err)').
> >
> > I know none of the existing H2G commands will return any extra data, so
> > intel_guc_send should be returning only negative numbers or zero (for
> > now).
> >
>
> I'd add a note in the commit message about the fact that we currently don't
> expect any command to return data in the status dword.
>
> -Daniele
>
> > -Michel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
