Re: [Intel-gfx] [PATCH 13/47] drm/i915/guc: Implement GuC context operations for new inteface
On Fri, Jun 25, 2021 at 03:25:13PM +0200, Michal Wajdeczko wrote:
>
>
> On 24.06.2021 09:04, Matthew Brost wrote:
> > Implement GuC context operations which includes GuC specific operations
> > alloc, pin, unpin, and destroy.
> >
> > Signed-off-by: John Harrison
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gt/intel_context.c | 5 +
> > drivers/gpu/drm/i915/gt/intel_context_types.h | 22 +-
> > drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 -
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 34 +
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 +
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 664 --
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/i915_request.c | 1 +
> > 8 files changed, 677 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
> > b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 4033184f13b9..2b68af16222c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -383,6 +383,11 @@ intel_context_init(struct intel_context *ce, struct
> > intel_engine_cs *engine)
> >
> > mutex_init(&ce->pin_mutex);
> >
> > + spin_lock_init(&ce->guc_state.lock);
> > +
> > + ce->guc_id = GUC_INVALID_LRC_ID;
> > + INIT_LIST_HEAD(&ce->guc_id_link);
> > +
> > i915_active_init(&ce->active,
> > __intel_context_active, __intel_context_retire, 0);
> > }
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > index bb6fef7eae52..ce7c69b34cd1 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > @@ -95,6 +95,7 @@ struct intel_context {
> > #define CONTEXT_BANNED 6
> > #define CONTEXT_FORCE_SINGLE_SUBMISSION7
> > #define CONTEXT_NOPREEMPT 8
> > +#define CONTEXT_LRCA_DIRTY 9
> >
> > struct {
> > u64 timeout_us;
> > @@ -137,14 +138,29 @@ struct intel_context {
> >
> > u8 wa_bb_page; /* if set, page num reserved for context workarounds */
> >
> > + struct {
> > + /** lock: protects everything in guc_state */
> > + spinlock_t lock;
> > + /**
> > +* sched_state: scheduling state of this context using GuC
> > +* submission
> > +*/
> > + u8 sched_state;
> > + } guc_state;
> > +
> > /* GuC scheduling state that does not require a lock. */
> > atomic_t guc_sched_state_no_lock;
> >
> > + /* GuC lrc descriptor ID */
> > + u16 guc_id;
> > +
> > + /* GuC lrc descriptor reference count */
> > + atomic_t guc_id_ref;
> > +
> > /*
> > -* GuC lrc descriptor ID - Not assigned in this patch but future patches
> > -* in the series will.
> > +* GuC ID link - in list when unpinned but guc_id still valid in GuC
> > */
> > - u16 guc_id;
> > + struct list_head guc_id_link;
>
> some fields are being added with kerneldoc, some without
> what's the rule ?
>
Yea, idk. I think we need to scrub all the structures in the driver and
add kernel doc everywhere. IMO not a blocker too as I think all the
structures are going to be reworked with OO concepts after the GuC code
lands before moving to DRM scheduler. That would be logical time to
update all the kernel doc too.
> > };
> >
> > #endif /* __INTEL_CONTEXT_TYPES__ */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> > b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> > index 41e5350a7a05..49d4857ad9b7 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> > @@ -87,7 +87,6 @@
> > #define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)
> >
> > #define MAX_CONTEXT_HW_ID (1 << 21) /* exclusive */
> > -#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
> > #define GEN11_MAX_CONTEXT_HW_ID(1 << 11) /* exclusive */
> > /* in Gen12 ID 0x7FF is reserved to indicate idle */
> > #define GEN12_MAX_CONTEXT_HW_ID(GEN11_MAX_CONTEXT_HW_ID - 1)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 9ba8219475b2..d44316dc914b 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -44,6 +44,14 @@ struct intel_guc {
> > void (*disable)(struct intel_guc *guc);
> > } interrupts;
> >
> > + /*
> > +* contexts_lock protects the pool of free guc ids and a linked list of
> > +* guc ids available to be stolen
> > +*/
> > + spinlock_t contexts_lock;
> > + struct ida guc_ids;
> > + struct list_head guc_id_list;
> > +
> > bool submission_selected;
> >
> > struct i915_vma *ads_vma;
> > @@ -102,6 +110,29 @@ intel_guc_send_and_receive(struct intel_guc *guc,
> > const u32 *action, u32 len,
> >
Re: [Intel-gfx] [PATCH 13/47] drm/i915/guc: Implement GuC context operations for new inteface
On 24.06.2021 09:04, Matthew Brost wrote:
> Implement GuC context operations which includes GuC specific operations
> alloc, pin, unpin, and destroy.
>
> Signed-off-by: John Harrison
> Signed-off-by: Matthew Brost
> ---
> drivers/gpu/drm/i915/gt/intel_context.c | 5 +
> drivers/gpu/drm/i915/gt/intel_context_types.h | 22 +-
> drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 -
> drivers/gpu/drm/i915/gt/uc/intel_guc.h| 34 +
> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 +
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 664 --
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/i915_request.c | 1 +
> 8 files changed, 677 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
> b/drivers/gpu/drm/i915/gt/intel_context.c
> index 4033184f13b9..2b68af16222c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -383,6 +383,11 @@ intel_context_init(struct intel_context *ce, struct
> intel_engine_cs *engine)
>
> mutex_init(&ce->pin_mutex);
>
> + spin_lock_init(&ce->guc_state.lock);
> +
> + ce->guc_id = GUC_INVALID_LRC_ID;
> + INIT_LIST_HEAD(&ce->guc_id_link);
> +
> i915_active_init(&ce->active,
>__intel_context_active, __intel_context_retire, 0);
> }
> diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
> b/drivers/gpu/drm/i915/gt/intel_context_types.h
> index bb6fef7eae52..ce7c69b34cd1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -95,6 +95,7 @@ struct intel_context {
> #define CONTEXT_BANNED 6
> #define CONTEXT_FORCE_SINGLE_SUBMISSION 7
> #define CONTEXT_NOPREEMPT8
> +#define CONTEXT_LRCA_DIRTY 9
>
> struct {
> u64 timeout_us;
> @@ -137,14 +138,29 @@ struct intel_context {
>
> u8 wa_bb_page; /* if set, page num reserved for context workarounds */
>
> + struct {
> + /** lock: protects everything in guc_state */
> + spinlock_t lock;
> + /**
> + * sched_state: scheduling state of this context using GuC
> + * submission
> + */
> + u8 sched_state;
> + } guc_state;
> +
> /* GuC scheduling state that does not require a lock. */
> atomic_t guc_sched_state_no_lock;
>
> + /* GuC lrc descriptor ID */
> + u16 guc_id;
> +
> + /* GuC lrc descriptor reference count */
> + atomic_t guc_id_ref;
> +
> /*
> - * GuC lrc descriptor ID - Not assigned in this patch but future patches
> - * in the series will.
> + * GuC ID link - in list when unpinned but guc_id still valid in GuC
>*/
> - u16 guc_id;
> + struct list_head guc_id_link;
some fields are being added with kerneldoc, some without
what's the rule ?
> };
>
> #endif /* __INTEL_CONTEXT_TYPES__ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> index 41e5350a7a05..49d4857ad9b7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> @@ -87,7 +87,6 @@
> #define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)
>
> #define MAX_CONTEXT_HW_ID(1 << 21) /* exclusive */
> -#define MAX_GUC_CONTEXT_HW_ID(1 << 20) /* exclusive */
> #define GEN11_MAX_CONTEXT_HW_ID (1 << 11) /* exclusive */
> /* in Gen12 ID 0x7FF is reserved to indicate idle */
> #define GEN12_MAX_CONTEXT_HW_ID (GEN11_MAX_CONTEXT_HW_ID - 1)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 9ba8219475b2..d44316dc914b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -44,6 +44,14 @@ struct intel_guc {
> void (*disable)(struct intel_guc *guc);
> } interrupts;
>
> + /*
> + * contexts_lock protects the pool of free guc ids and a linked list of
> + * guc ids available to be stolen
> + */
> + spinlock_t contexts_lock;
> + struct ida guc_ids;
> + struct list_head guc_id_list;
> +
> bool submission_selected;
>
> struct i915_vma *ads_vma;
> @@ -102,6 +110,29 @@ intel_guc_send_and_receive(struct intel_guc *guc, const
> u32 *action, u32 len,
>response_buf, response_buf_size, 0);
> }
>
> +static inline int intel_guc_send_busy_loop(struct intel_guc* guc,
> +const u32 *action,
> +u32 len,
> +bool loop)
> +{
> + int err;
> +
> + /* No sleeping with spin locks, just busy loop */
> + might_sleep_if(loop && (!in_atomic() && !irqs_disabled()));
> +
> +retry:
> + err = intel_guc_send_nb(guc, action, len);
>
[Intel-gfx] [PATCH 13/47] drm/i915/guc: Implement GuC context operations for new inteface
Implement GuC context operations which includes GuC specific operations
alloc, pin, unpin, and destroy.
Signed-off-by: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_context.c | 5 +
drivers/gpu/drm/i915/gt/intel_context_types.h | 22 +-
drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 -
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 34 +
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 664 --
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/i915_request.c | 1 +
8 files changed, 677 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 4033184f13b9..2b68af16222c 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -383,6 +383,11 @@ intel_context_init(struct intel_context *ce, struct
intel_engine_cs *engine)
mutex_init(&ce->pin_mutex);
+ spin_lock_init(&ce->guc_state.lock);
+
+ ce->guc_id = GUC_INVALID_LRC_ID;
+ INIT_LIST_HEAD(&ce->guc_id_link);
+
i915_active_init(&ce->active,
__intel_context_active, __intel_context_retire, 0);
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index bb6fef7eae52..ce7c69b34cd1 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -95,6 +95,7 @@ struct intel_context {
#define CONTEXT_BANNED 6
#define CONTEXT_FORCE_SINGLE_SUBMISSION7
#define CONTEXT_NOPREEMPT 8
+#define CONTEXT_LRCA_DIRTY 9
struct {
u64 timeout_us;
@@ -137,14 +138,29 @@ struct intel_context {
u8 wa_bb_page; /* if set, page num reserved for context workarounds */
+ struct {
+ /** lock: protects everything in guc_state */
+ spinlock_t lock;
+ /**
+* sched_state: scheduling state of this context using GuC
+* submission
+*/
+ u8 sched_state;
+ } guc_state;
+
/* GuC scheduling state that does not require a lock. */
atomic_t guc_sched_state_no_lock;
+ /* GuC lrc descriptor ID */
+ u16 guc_id;
+
+ /* GuC lrc descriptor reference count */
+ atomic_t guc_id_ref;
+
/*
-* GuC lrc descriptor ID - Not assigned in this patch but future patches
-* in the series will.
+* GuC ID link - in list when unpinned but guc_id still valid in GuC
*/
- u16 guc_id;
+ struct list_head guc_id_link;
};
#endif /* __INTEL_CONTEXT_TYPES__ */
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
index 41e5350a7a05..49d4857ad9b7 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
@@ -87,7 +87,6 @@
#define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)
#define MAX_CONTEXT_HW_ID (1 << 21) /* exclusive */
-#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
#define GEN11_MAX_CONTEXT_HW_ID(1 << 11) /* exclusive */
/* in Gen12 ID 0x7FF is reserved to indicate idle */
#define GEN12_MAX_CONTEXT_HW_ID(GEN11_MAX_CONTEXT_HW_ID - 1)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 9ba8219475b2..d44316dc914b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -44,6 +44,14 @@ struct intel_guc {
void (*disable)(struct intel_guc *guc);
} interrupts;
+ /*
+* contexts_lock protects the pool of free guc ids and a linked list of
+* guc ids available to be stolen
+*/
+ spinlock_t contexts_lock;
+ struct ida guc_ids;
+ struct list_head guc_id_list;
+
bool submission_selected;
struct i915_vma *ads_vma;
@@ -102,6 +110,29 @@ intel_guc_send_and_receive(struct intel_guc *guc, const
u32 *action, u32 len,
response_buf, response_buf_size, 0);
}
+static inline int intel_guc_send_busy_loop(struct intel_guc* guc,
+ const u32 *action,
+ u32 len,
+ bool loop)
+{
+ int err;
+
+ /* No sleeping with spin locks, just busy loop */
+ might_sleep_if(loop && (!in_atomic() && !irqs_disabled()));
+
+retry:
+ err = intel_guc_send_nb(guc, action, len);
+ if (unlikely(err == -EBUSY && loop)) {
+ if (likely(!in_atomic() && !irqs_disabled()))
+ cond_resched();
+ else
+ cpu_relax();
+ goto retry;
+ }
+
+ return err;
+}
+
static in
