Re: [PATCH v3 3/3] drm/i915/gt: document TLB cache invalidation functions

[Randy Dunlap <rdunlap@xxxxxxxxxxxxx>]

Hi Mauro,

On 8/4/22 00:37, Mauro Carvalho Chehab wrote:
> Add a description for the TLB cache invalidation algorithm and for
> the related kAPI functions.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> ---
> 
> To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH v3 0/3] at: https://lore.kernel.org/all/cover.1659598090.git.mchehab@xxxxxxxxxx/
> 
>  Documentation/gpu/i915.rst          |  7 ++
>  drivers/gpu/drm/i915/gt/intel_tlb.c | 25 ++++++++
>  drivers/gpu/drm/i915/gt/intel_tlb.h | 99 +++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
> 

> diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.c b/drivers/gpu/drm/i915/gt/intel_tlb.c
> index af8cae979489..16b918ffe824 100644
> --- a/drivers/gpu/drm/i915/gt/intel_tlb.c
> +++ b/drivers/gpu/drm/i915/gt/intel_tlb.c
> @@ -145,6 +145,18 @@ static void mmio_invalidate_full(struct intel_gt *gt)
>  	intel_uncore_forcewake_put_delayed(uncore, FORCEWAKE_ALL);
>  }
>  
> +/**
> + * intel_gt_invalidate_tlb_full - do full TLB cache invalidation
> + * @gt: GT structure

In multiple places (here and below) it would be nice to know what a
GT structure is. I looked thru multiple C and header files yesterday
and didn't find any comments about it.

Just saying that @gt is a GT structure isn't very helpful, other
than making kernel-doc shut up.

> + * @seqno: sequence number
> + *
> + * Do a full TLB cache invalidation if the @seqno is bigger than the last
> + * full TLB cache invalidation.
> + *
> + * Note:
> + * The TLB cache invalidation logic depends on GEN-specific registers.
> + * It currently supports MMIO-based TLB flush for GEN8 to GEN12.
> + */
>  void intel_gt_invalidate_tlb_full(struct intel_gt *gt, u32 seqno)
>  {
>  	intel_wakeref_t wakeref;
> @@ -171,12 +183,25 @@ void intel_gt_invalidate_tlb_full(struct intel_gt *gt, u32 seqno)
>  	}
>  }
>  
> +/**
> + * intel_gt_init_tlb - initialize TLB-specific vars
> + * @gt: GT structure
> + *
> + * TLB cache invalidation logic internally uses some resources that require
> + * initialization. Should be called before doing any TLB cache invalidation.
> + */
>  void intel_gt_init_tlb(struct intel_gt *gt)
>  {
>  	mutex_init(&gt->tlb.invalidate_lock);
>  	seqcount_mutex_init(&gt->tlb.seqno, &gt->tlb.invalidate_lock);
>  }
>  
> +/**
> + * intel_gt_fini_tlb - free TLB-specific vars
> + * @gt: GT structure
> + *
> + * Frees any resources needed by TLB cache invalidation logic.
> + */
>  void intel_gt_fini_tlb(struct intel_gt *gt)
>  {
>  	mutex_destroy(&gt->tlb.invalidate_lock);
> diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.h b/drivers/gpu/drm/i915/gt/intel_tlb.h
> index 46ce25bf5afe..2838c051f872 100644
> --- a/drivers/gpu/drm/i915/gt/intel_tlb.h
> +++ b/drivers/gpu/drm/i915/gt/intel_tlb.h
> @@ -11,16 +11,115 @@
>  
>  #include "intel_gt_types.h"
>  
> +/**
> + * DOC: TLB cache invalidation logic
> + *
...
> +
>  void intel_gt_invalidate_tlb_full(struct intel_gt *gt, u32 seqno);
>  
>  void intel_gt_init_tlb(struct intel_gt *gt);
>  void intel_gt_fini_tlb(struct intel_gt *gt);
>  
> +/**
> + * intel_gt_tlb_seqno - Returns the current TLB invlidation sequence number
> + * @gt: GT structure
> + *
> + * There's no need to lock while calling it, as seqprop_sequence is thread-safe
> + */
>  static inline u32 intel_gt_tlb_seqno(const struct intel_gt *gt)
>  {
>  	return seqprop_sequence(&gt->tlb.seqno);
>  }
>  
> +/**
> + * intel_gt_next_invalidate_tlb_full - Returns the next TLB full invalidation
> + *	sequence number
> + * @gt: GT structure
> + *
> + * There's no need to lock while calling it, as seqprop_sequence is thread-safe
> + */
>  static inline u32 intel_gt_next_invalidate_tlb_full(const struct intel_gt *gt)
>  {
>  	return intel_gt_tlb_seqno(gt) | 1;

thanks.

-- 
~Randy