[Intel-gfx] [PATCH 10/47] drm/i915/guc: Add lrc descriptor context lookup array

[John Harrison]

On 6/25/2021 10:26, Matthew Brost wrote:
> On Fri, Jun 25, 2021 at 03:17:51PM +0200, Michal Wajdeczko wrote:
>> On 24.06.2021 09:04, Matthew Brost wrote:
>>> Add lrc descriptor context lookup array which can resolve the
>>> intel_context from the lrc descriptor index. In addition to lookup, it
>>> can determine in the lrc descriptor context is currently registered with
>>> the GuC by checking if an entry for a descriptor index is present.
>>> Future patches in the series will make use of this array.
>> s/lrc/LRC
>>
> I guess? lrc and LRC are used interchangeably throughout the current
> code base.
It is an abbreviation so LRC is technically the correct version for a 
comment. The fact that other existing comments are incorrect is not a 
valid reason to perpetuate a mistake :). Might as well fix it if you are 
going to repost the patch anyway for any other reason, but I would not 
call it a blocking issue.

Also, 'can determine in the' should be 'can determine if the'. Again, 
not exactly a blocking issue but should be fixed.

>>> Cc: John Harrison <john.c.harrison at intel.com>
>>> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  5 +++
>>>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 32 +++++++++++++++++--
>>>   2 files changed, 35 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> index b28fa54214f2..2313d9fc087b 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>> @@ -6,6 +6,8 @@
>>>   #ifndef _INTEL_GUC_H_
>>>   #define _INTEL_GUC_H_
>>>   
>>> +#include "linux/xarray.h"
>> #include <linux/xarray.h>
>>
> Yep.
>
>>> +
>>>   #include "intel_uncore.h"
>>>   #include "intel_guc_fw.h"
>>>   #include "intel_guc_fwif.h"
>>> @@ -46,6 +48,9 @@ struct intel_guc {
>>>   	struct i915_vma *lrc_desc_pool;
>>>   	void *lrc_desc_pool_vaddr;
>>>   
>>> +	/* guc_id to intel_context lookup */
>>> +	struct xarray context_lookup;
>>> +
>>>   	/* Control params for fw initialization */
>>>   	u32 params[GUC_CTL_MAX_DWORDS];
>> btw, IIRC there was idea to move most struct definitions to
>> intel_guc_types.h, is this still a plan ?
>>
> I don't ever recall discussing this but we can certainly do this. For
> what it is worth we do introduce intel_guc_submission_types.h a bit
> later. I'll make a note about intel_guc_types.h though.
>
> Matt
Yeah, my only recollection was about the submission types header. Are 
there sufficient non-submission fields in the GuC structure to warrant a 
general GuC types header?

With the commit message tweaks and #include fix mentioned above, it 
looks good to me.
Reviewed-by: John Harrison <John.C.Harrison at Intel.com>


>>>   
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> index a366890fb840..23a94a896a0b 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>> @@ -65,8 +65,6 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
>>>   	return rb_entry(rb, struct i915_priolist, node);
>>>   }
>>>   
>>> -/* Future patches will use this function */
>>> -__attribute__ ((unused))
>>>   static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
>>>   {
>>>   	struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;
>>> @@ -76,6 +74,15 @@ static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
>>>   	return &base[index];
>>>   }
>>>   
>>> +static inline struct intel_context *__get_context(struct intel_guc *guc, u32 id)
>>> +{
>>> +	struct intel_context *ce = xa_load(&guc->context_lookup, id);
>>> +
>>> +	GEM_BUG_ON(id >= GUC_MAX_LRC_DESCRIPTORS);
>>> +
>>> +	return ce;
>>> +}
>>> +
>>>   static int guc_lrc_desc_pool_create(struct intel_guc *guc)
>>>   {
>>>   	u32 size;
>>> @@ -96,6 +103,25 @@ static void guc_lrc_desc_pool_destroy(struct intel_guc *guc)
>>>   	i915_vma_unpin_and_release(&guc->lrc_desc_pool, I915_VMA_RELEASE_MAP);
>>>   }
>>>   
>>> +static inline void reset_lrc_desc(struct intel_guc *guc, u32 id)
>>> +{
>>> +	struct guc_lrc_desc *desc = __get_lrc_desc(guc, id);
>>> +
>>> +	memset(desc, 0, sizeof(*desc));
>>> +	xa_erase_irq(&guc->context_lookup, id);
>>> +}
>>> +
>>> +static inline bool lrc_desc_registered(struct intel_guc *guc, u32 id)
>>> +{
>>> +	return __get_context(guc, id);
>>> +}
>>> +
>>> +static inline void set_lrc_desc_registered(struct intel_guc *guc, u32 id,
>>> +					   struct intel_context *ce)
>>> +{
>>> +	xa_store_irq(&guc->context_lookup, id, ce, GFP_ATOMIC);
>>> +}
>>> +
>>>   static void guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>>>   {
>>>   	/* Leaving stub as this function will be used in future patches */
>>> @@ -400,6 +426,8 @@ int intel_guc_submission_init(struct intel_guc *guc)
>>>   	 */
>>>   	GEM_BUG_ON(!guc->lrc_desc_pool);
>>>   
>>> +	xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);
>>> +
>>>   	return 0;
>>>   }
>>>   
>>>