| Message ID | 20180319152828.41564-1-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3/19/2018 8:58 PM, Michal Wajdeczko wrote: > There is no need to mix parameter types in public CT functions > as we can always accept intel_guc_ct. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_guc_ct.c | 34 ++++++++++++++++++++++++---------- > drivers/gpu/drm/i915/intel_guc_ct.h | 6 ++---- > drivers/gpu/drm/i915/intel_uc.c | 4 ++-- > 3 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c > index 0a0d3d5..7dd7de0 100644 > --- a/drivers/gpu/drm/i915/intel_guc_ct.c > +++ b/drivers/gpu/drm/i915/intel_guc_ct.c > @@ -28,12 +28,21 @@ > > enum { CTB_OWNER_HOST = 0 }; > > +/** > + * intel_guc_ct_init_early - Initialize CT state without requiring device access > + * @ct: pointer to CT struct > + */ > void intel_guc_ct_init_early(struct intel_guc_ct *ct) > { > /* we're using static channel owners */ > ct->host_channel.owner = CTB_OWNER_HOST; > } > > +static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct) > +{ > + return container_of(ct, struct intel_guc, ct); > +} > + > static inline const char *guc_ct_buffer_type_to_str(u32 type) > { > switch (type) { > @@ -416,16 +425,19 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len) > } > > /** > - * Enable buffer based command transport > + * intel_guc_ct_enable - Enable buffer based command transport. > + * @ct: pointer to CT struct > + * > * Shall only be called for platforms with HAS_GUC_CT. > - * @guc: the guc > - * return: 0 on success > - * non-zero on failure > + * > + * Returns: > + * 0 on success, a negative errno code on failure. Should be * Return: 0 on sucess ... > */ > -int intel_guc_enable_ct(struct intel_guc *guc) > +int intel_guc_ct_enable(struct intel_guc_ct *ct) > { > + struct intel_guc *guc = ct_to_guc(ct); > struct drm_i915_private *dev_priv = guc_to_i915(guc); change to *i915 as part of this patch itself? :) similar for disable. Otherwise LGTM Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > - struct intel_guc_ct_channel *ctch = &guc->ct.host_channel; > + struct intel_guc_ct_channel *ctch = &ct->host_channel; > int err; > > GEM_BUG_ON(!HAS_GUC_CT(dev_priv)); > @@ -441,14 +453,16 @@ int intel_guc_enable_ct(struct intel_guc *guc) > } > > /** > - * Disable buffer based command transport. > + * intel_guc_ct_disable - Disable buffer based command transport. > + * @ct: pointer to CT struct > + * > * Shall only be called for platforms with HAS_GUC_CT. > - * @guc: the guc > */ > -void intel_guc_disable_ct(struct intel_guc *guc) > +void intel_guc_ct_disable(struct intel_guc_ct *ct) > { > + struct intel_guc *guc = ct_to_guc(ct); > struct drm_i915_private *dev_priv = guc_to_i915(guc); > - struct intel_guc_ct_channel *ctch = &guc->ct.host_channel; > + struct intel_guc_ct_channel *ctch = &ct->host_channel; > > GEM_BUG_ON(!HAS_GUC_CT(dev_priv)); > > diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h > index 6d97f36..595c8ad 100644 > --- a/drivers/gpu/drm/i915/intel_guc_ct.h > +++ b/drivers/gpu/drm/i915/intel_guc_ct.h > @@ -78,9 +78,7 @@ struct intel_guc_ct { > }; > > void intel_guc_ct_init_early(struct intel_guc_ct *ct); > - > -/* XXX: move to intel_uc.h ? don't fit there either */ > -int intel_guc_enable_ct(struct intel_guc *guc); > -void intel_guc_disable_ct(struct intel_guc *guc); > +int intel_guc_ct_enable(struct intel_guc_ct *ct); > +void intel_guc_ct_disable(struct intel_guc_ct *ct); > > #endif /* _INTEL_GUC_CT_H_ */ > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 34e847d..a45c2ce 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -232,7 +232,7 @@ static int guc_enable_communication(struct intel_guc *guc) > gen9_enable_guc_interrupts(dev_priv); > > if (HAS_GUC_CT(dev_priv)) > - return intel_guc_enable_ct(guc); > + return intel_guc_ct_enable(&guc->ct); > > guc->send = intel_guc_send_mmio; > return 0; > @@ -243,7 +243,7 @@ static void guc_disable_communication(struct intel_guc *guc) > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > if (HAS_GUC_CT(dev_priv)) > - intel_guc_disable_ct(guc); > + intel_guc_ct_disable(&guc->ct); > > gen9_disable_guc_interrupts(dev_priv); >
On Tue, 20 Mar 2018 08:24:14 +0100, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > > > On 3/19/2018 8:58 PM, Michal Wajdeczko wrote: >> There is no need to mix parameter types in public CT functions >> as we can always accept intel_guc_ct. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/intel_guc_ct.c | 34 >> ++++++++++++++++++++++++---------- >> drivers/gpu/drm/i915/intel_guc_ct.h | 6 ++---- >> drivers/gpu/drm/i915/intel_uc.c | 4 ++-- >> 3 files changed, 28 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c >> b/drivers/gpu/drm/i915/intel_guc_ct.c >> index 0a0d3d5..7dd7de0 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_ct.c >> +++ b/drivers/gpu/drm/i915/intel_guc_ct.c >> @@ -28,12 +28,21 @@ >> enum { CTB_OWNER_HOST = 0 }; >> +/** >> + * intel_guc_ct_init_early - Initialize CT state without requiring >> device access >> + * @ct: pointer to CT struct >> + */ >> void intel_guc_ct_init_early(struct intel_guc_ct *ct) >> { >> /* we're using static channel owners */ >> ct->host_channel.owner = CTB_OWNER_HOST; >> } >> +static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct) >> +{ >> + return container_of(ct, struct intel_guc, ct); >> +} >> + >> static inline const char *guc_ct_buffer_type_to_str(u32 type) >> { >> switch (type) { >> @@ -416,16 +425,19 @@ static int intel_guc_send_ct(struct intel_guc >> *guc, const u32 *action, u32 len) >> } >> /** >> - * Enable buffer based command transport >> + * intel_guc_ct_enable - Enable buffer based command transport. >> + * @ct: pointer to CT struct >> + * >> * Shall only be called for platforms with HAS_GUC_CT. >> - * @guc: the guc >> - * return: 0 on success >> - * non-zero on failure >> + * >> + * Returns: >> + * 0 on success, a negative errno code on failure. > Should be > * Return: 0 on sucess ... hmm, I'm not so sure: $ grep -r "\* Return: .*" drivers/gpu/drm/* | wc -l 153 $ grep -r "\* Returns:$" drivers/gpu/drm/* | wc -l 344 >> */ >> -int intel_guc_enable_ct(struct intel_guc *guc) >> +int intel_guc_ct_enable(struct intel_guc_ct *ct) >> { >> + struct intel_guc *guc = ct_to_guc(ct); >> struct drm_i915_private *dev_priv = guc_to_i915(guc); > change to *i915 as part of this patch itself? :) similar for disable. sure > Otherwise LGTM > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> thanks /m
On 3/20/2018 6:30 PM, Michal Wajdeczko wrote: > On Tue, 20 Mar 2018 08:24:14 +0100, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> >> >> On 3/19/2018 8:58 PM, Michal Wajdeczko wrote: >>> There is no need to mix parameter types in public CT functions >>> as we can always accept intel_guc_ct. >>> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> <snip> >>> /** >>> - * Enable buffer based command transport >>> + * intel_guc_ct_enable - Enable buffer based command transport. >>> + * @ct: pointer to CT struct >>> + * >>> * Shall only be called for platforms with HAS_GUC_CT. >>> - * @guc: the guc >>> - * return: 0 on success >>> - * non-zero on failure >>> + * >>> + * Returns: >>> + * 0 on success, a negative errno code on failure. >> Should be >> * Return: 0 on sucess ... > > hmm, I'm not so sure: > > $ grep -r "\* Return: .*" drivers/gpu/drm/* | wc -l > 153 > > $ grep -r "\* Returns:$" drivers/gpu/drm/* | wc -l > 344 > Hi Michal, kernel-doc rules recommend "Return:". Thanks, Sagar >>> */ >>> -int intel_guc_enable_ct(struct intel_guc *guc) >>> +int intel_guc_ct_enable(struct intel_guc_ct *ct) >>> { >>> + struct intel_guc *guc = ct_to_guc(ct); >>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >> change to *i915 as part of this patch itself? :) similar for disable. > > sure > >> Otherwise LGTM >> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > thanks > /m >
On Tue, 20 Mar 2018, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > On 3/20/2018 6:30 PM, Michal Wajdeczko wrote: >> On Tue, 20 Mar 2018 08:24:14 +0100, Sagar Arun Kamble >> <sagar.a.kamble@intel.com> wrote: >> >>> >>> >>> On 3/19/2018 8:58 PM, Michal Wajdeczko wrote: >>>> There is no need to mix parameter types in public CT functions >>>> as we can always accept intel_guc_ct. >>>> >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> > <snip> >>>> /** >>>> - * Enable buffer based command transport >>>> + * intel_guc_ct_enable - Enable buffer based command transport. >>>> + * @ct: pointer to CT struct >>>> + * >>>> * Shall only be called for platforms with HAS_GUC_CT. >>>> - * @guc: the guc >>>> - * return: 0 on success >>>> - * non-zero on failure >>>> + * >>>> + * Returns: >>>> + * 0 on success, a negative errno code on failure. >>> Should be >>> * Return: 0 on sucess ... >> >> hmm, I'm not so sure: >> >> $ grep -r "\* Return: .*" drivers/gpu/drm/* | wc -l >> 153 >> >> $ grep -r "\* Returns:$" drivers/gpu/drm/* | wc -l >> 344 >> > Hi Michal, > > kernel-doc rules recommend "Return:". Correct. For legacy reasons, a bunch of variants are recognized and canonicalized to "Return" in the output. I also recommend documenting the return values immediately following "Return: ", without \n, similar to the parameter documentation. BR, Jani. > > Thanks, > Sagar >>>> */ >>>> -int intel_guc_enable_ct(struct intel_guc *guc) >>>> +int intel_guc_ct_enable(struct intel_guc_ct *ct) >>>> { >>>> + struct intel_guc *guc = ct_to_guc(ct); >>>> struct drm_i915_private *dev_priv = guc_to_i915(guc); >>> change to *i915 as part of this patch itself? :) similar for disable. >> >> sure >> >>> Otherwise LGTM >>> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> >> thanks >> /m >>
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c b/drivers/gpu/drm/i915/intel_guc_ct.c index 0a0d3d5..7dd7de0 100644 --- a/drivers/gpu/drm/i915/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/intel_guc_ct.c @@ -28,12 +28,21 @@ enum { CTB_OWNER_HOST = 0 }; +/** + * intel_guc_ct_init_early - Initialize CT state without requiring device access + * @ct: pointer to CT struct + */ void intel_guc_ct_init_early(struct intel_guc_ct *ct) { /* we're using static channel owners */ ct->host_channel.owner = CTB_OWNER_HOST; } +static inline struct intel_guc *ct_to_guc(struct intel_guc_ct *ct) +{ + return container_of(ct, struct intel_guc, ct); +} + static inline const char *guc_ct_buffer_type_to_str(u32 type) { switch (type) { @@ -416,16 +425,19 @@ static int intel_guc_send_ct(struct intel_guc *guc, const u32 *action, u32 len) } /** - * Enable buffer based command transport + * intel_guc_ct_enable - Enable buffer based command transport. + * @ct: pointer to CT struct + * * Shall only be called for platforms with HAS_GUC_CT. - * @guc: the guc - * return: 0 on success - * non-zero on failure + * + * Returns: + * 0 on success, a negative errno code on failure. */ -int intel_guc_enable_ct(struct intel_guc *guc) +int intel_guc_ct_enable(struct intel_guc_ct *ct) { + struct intel_guc *guc = ct_to_guc(ct); struct drm_i915_private *dev_priv = guc_to_i915(guc); - struct intel_guc_ct_channel *ctch = &guc->ct.host_channel; + struct intel_guc_ct_channel *ctch = &ct->host_channel; int err; GEM_BUG_ON(!HAS_GUC_CT(dev_priv)); @@ -441,14 +453,16 @@ int intel_guc_enable_ct(struct intel_guc *guc) } /** - * Disable buffer based command transport. + * intel_guc_ct_disable - Disable buffer based command transport. + * @ct: pointer to CT struct + * * Shall only be called for platforms with HAS_GUC_CT. - * @guc: the guc */ -void intel_guc_disable_ct(struct intel_guc *guc) +void intel_guc_ct_disable(struct intel_guc_ct *ct) { + struct intel_guc *guc = ct_to_guc(ct); struct drm_i915_private *dev_priv = guc_to_i915(guc); - struct intel_guc_ct_channel *ctch = &guc->ct.host_channel; + struct intel_guc_ct_channel *ctch = &ct->host_channel; GEM_BUG_ON(!HAS_GUC_CT(dev_priv)); diff --git a/drivers/gpu/drm/i915/intel_guc_ct.h b/drivers/gpu/drm/i915/intel_guc_ct.h index 6d97f36..595c8ad 100644 --- a/drivers/gpu/drm/i915/intel_guc_ct.h +++ b/drivers/gpu/drm/i915/intel_guc_ct.h @@ -78,9 +78,7 @@ struct intel_guc_ct { }; void intel_guc_ct_init_early(struct intel_guc_ct *ct); - -/* XXX: move to intel_uc.h ? don't fit there either */ -int intel_guc_enable_ct(struct intel_guc *guc); -void intel_guc_disable_ct(struct intel_guc *guc); +int intel_guc_ct_enable(struct intel_guc_ct *ct); +void intel_guc_ct_disable(struct intel_guc_ct *ct); #endif /* _INTEL_GUC_CT_H_ */ diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 34e847d..a45c2ce 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -232,7 +232,7 @@ static int guc_enable_communication(struct intel_guc *guc) gen9_enable_guc_interrupts(dev_priv); if (HAS_GUC_CT(dev_priv)) - return intel_guc_enable_ct(guc); + return intel_guc_ct_enable(&guc->ct); guc->send = intel_guc_send_mmio; return 0; @@ -243,7 +243,7 @@ static void guc_disable_communication(struct intel_guc *guc) struct drm_i915_private *dev_priv = guc_to_i915(guc); if (HAS_GUC_CT(dev_priv)) - intel_guc_disable_ct(guc); + intel_guc_ct_disable(&guc->ct); gen9_disable_guc_interrupts(dev_priv);
There is no need to mix parameter types in public CT functions as we can always accept intel_guc_ct. Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_guc_ct.c | 34 ++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_guc_ct.h | 6 ++---- drivers/gpu/drm/i915/intel_uc.c | 4 ++-- 3 files changed, 28 insertions(+), 16 deletions(-)