| Message ID | 20170804162712.20468-2-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > --- > 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. > */ > #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). -Michel
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@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> --- >> 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. >> */ >> #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
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@intel.com> > > > Cc: Oscar Mateo <oscar.mateo@intel.com> > > > Cc: Michel Thierry <michel.thierry@intel.com> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > --- > > > 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
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. */ #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) +#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);
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@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Michel Thierry <michel.thierry@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- 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(-)