Re: [PATCH 01/15] drm/i915/guc: Add support for data reporting in GuC responses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux