Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Thu, Jun 24, 2021 at 09:38:33AM -0700, Matthew Brost wrote:
> On Thu, Jun 10, 2021 at 05:27:48PM +0200, Daniel Vetter wrote:
> > On Wed, Jun 09, 2021 at 04:10:23PM -0700, Matthew Brost wrote:
> > > On Tue, Jun 08, 2021 at 10:46:15AM +0200, Daniel Vetter wrote:
> > > > On Tue, Jun 8, 2021 at 10:39 AM Tvrtko Ursulin
> > > > wrote:
> > > > >
> > > > >
> > > > > On 07/06/2021 18:31, Matthew Brost wrote:
> > > > > > On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> > > > > >>
> > > > > >> On 27/05/2021 15:35, Matthew Brost wrote:
> > > > > >>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 26/05/2021 19:10, Matthew Brost wrote:
> > > > >
> > > > > [snip]
> > > > >
> > > > > > +static int ct_send_nb(struct intel_guc_ct *ct,
> > > > > > + const u32 *action,
> > > > > > + u32 len,
> > > > > > + u32 flags)
> > > > > > +{
> > > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > > + unsigned long spin_flags;
> > > > > > + u32 fence;
> > > > > > + int ret;
> > > > > > +
> > > > > > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > > > > > +
> > > > > > + ret = ctb_has_room(ctb, len + 1);
> > > > > > + if (unlikely(ret))
> > > > > > + goto out;
> > > > > > +
> > > > > > + fence = ct_get_next_fence(ct);
> > > > > > + ret = ct_write(ct, action, len, fence, flags);
> > > > > > + if (unlikely(ret))
> > > > > > + goto out;
> > > > > > +
> > > > > > + intel_guc_notify(ct_to_guc(ct));
> > > > > > +
> > > > > > +out:
> > > > > > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > static int ct_send(struct intel_guc_ct *ct,
> > > > > > const u32 *action,
> > > > > > u32 len,
> > > > > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct
> > > > > > *ct,
> > > > > > u32 response_buf_size,
> > > > > > u32 *status)
> > > > > > {
> > > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > > struct ct_request request;
> > > > > > unsigned long flags;
> > > > > > u32 fence;
> > > > > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct
> > > > > > *ct,
> > > > > > GEM_BUG_ON(!len);
> > > > > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > > > > GEM_BUG_ON(!response_buf &&
> > > > > > response_buf_size);
> > > > > > + might_sleep();
> > > > >
> > > > > Sleep is just cond_resched below or there is more?
> > > > >
> > > > > >>>
> > > > > >>> Yes, the cond_resched.
> > > > > >>>
> > > > > > + /*
> > > > > > + * We use a lazy spin wait loop here as we believe
> > > > > > that if the CT
> > > > > > + * buffers are sized correctly the flow control
> > > > > > condition should be
> > > > > > + * rare.
> > > > > > + */
> > > > > > +retry:
> > > > > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > > > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > > > > + spin_unlock_irqrestore(&ct->ctbs.send.lock,
> > > > > > flags);
> > > > > > + cond_resched();
> > > > > > + goto retry;
> > > > > > + }
> > > > >
> > > > > If this patch is about adding a non-blocking send function,
> > > > > and below we can
> > > > > see that it creates a fork:
> > > > >
> > > > > intel_guc_ct_send:
> > > > > ...
> > > > > if (flags & INTEL_GUC_SEND_NB)
> > > > > return ct_send_nb(ct, action, len, flags);
> > > > >
> > > > > ret = ct_send(ct, action, len, response_buf,
> > > > > response_buf_size, &status);
> > > > >
> > > > > Then why is there a change in ct_send here, which is not the
> > > > > new
> > > > > non-blocking path?
> > > > >
> > > > > >>>
> > > > > >>> There is not a change to ct_send(), just to intel_guc_ct_send.
> > > > > >>
> > > > > >> I was doing by the diff which says:
> > > > > >>
> > > > > >> static int ct_send(struct intel_guc_ct *ct,
> > > > > >> cons
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Thu, Jun 10, 2021 at 05:27:48PM +0200, Daniel Vetter wrote:
> On Wed, Jun 09, 2021 at 04:10:23PM -0700, Matthew Brost wrote:
> > On Tue, Jun 08, 2021 at 10:46:15AM +0200, Daniel Vetter wrote:
> > > On Tue, Jun 8, 2021 at 10:39 AM Tvrtko Ursulin
> > > wrote:
> > > >
> > > >
> > > > On 07/06/2021 18:31, Matthew Brost wrote:
> > > > > On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> > > > >>
> > > > >> On 27/05/2021 15:35, Matthew Brost wrote:
> > > > >>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
> > > >
> > > > On 26/05/2021 19:10, Matthew Brost wrote:
> > > >
> > > > [snip]
> > > >
> > > > > +static int ct_send_nb(struct intel_guc_ct *ct,
> > > > > + const u32 *action,
> > > > > + u32 len,
> > > > > + u32 flags)
> > > > > +{
> > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > + unsigned long spin_flags;
> > > > > + u32 fence;
> > > > > + int ret;
> > > > > +
> > > > > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > > > > +
> > > > > + ret = ctb_has_room(ctb, len + 1);
> > > > > + if (unlikely(ret))
> > > > > + goto out;
> > > > > +
> > > > > + fence = ct_get_next_fence(ct);
> > > > > + ret = ct_write(ct, action, len, fence, flags);
> > > > > + if (unlikely(ret))
> > > > > + goto out;
> > > > > +
> > > > > + intel_guc_notify(ct_to_guc(ct));
> > > > > +
> > > > > +out:
> > > > > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > static int ct_send(struct intel_guc_ct *ct,
> > > > > const u32 *action,
> > > > > u32 len,
> > > > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct
> > > > > *ct,
> > > > > u32 response_buf_size,
> > > > > u32 *status)
> > > > > {
> > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > struct ct_request request;
> > > > > unsigned long flags;
> > > > > u32 fence;
> > > > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct
> > > > > *ct,
> > > > > GEM_BUG_ON(!len);
> > > > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > > > + might_sleep();
> > > >
> > > > Sleep is just cond_resched below or there is more?
> > > >
> > > > >>>
> > > > >>> Yes, the cond_resched.
> > > > >>>
> > > > > + /*
> > > > > + * We use a lazy spin wait loop here as we believe that
> > > > > if the CT
> > > > > + * buffers are sized correctly the flow control
> > > > > condition should be
> > > > > + * rare.
> > > > > + */
> > > > > +retry:
> > > > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > > > + spin_unlock_irqrestore(&ct->ctbs.send.lock,
> > > > > flags);
> > > > > + cond_resched();
> > > > > + goto retry;
> > > > > + }
> > > >
> > > > If this patch is about adding a non-blocking send function,
> > > > and below we can
> > > > see that it creates a fork:
> > > >
> > > > intel_guc_ct_send:
> > > > ...
> > > > if (flags & INTEL_GUC_SEND_NB)
> > > > return ct_send_nb(ct, action, len, flags);
> > > >
> > > > ret = ct_send(ct, action, len, response_buf,
> > > > response_buf_size, &status);
> > > >
> > > > Then why is there a change in ct_send here, which is not the
> > > > new
> > > > non-blocking path?
> > > >
> > > > >>>
> > > > >>> There is not a change to ct_send(), just to intel_guc_ct_send.
> > > > >>
> > > > >> I was doing by the diff which says:
> > > > >>
> > > > >> static int ct_send(struct intel_guc_ct *ct,
> > > > >> const u32 *action,
> > > > >> u32 len,
> > > > >> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > >> u32 response_buf_size,
> > > > >> u32 *status)
> > > > >> {
> > > > >> +struct intel_
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Wed, Jun 09, 2021 at 04:10:23PM -0700, Matthew Brost wrote:
> On Tue, Jun 08, 2021 at 10:46:15AM +0200, Daniel Vetter wrote:
> > On Tue, Jun 8, 2021 at 10:39 AM Tvrtko Ursulin
> > wrote:
> > >
> > >
> > > On 07/06/2021 18:31, Matthew Brost wrote:
> > > > On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> > > >>
> > > >> On 27/05/2021 15:35, Matthew Brost wrote:
> > > >>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 26/05/2021 19:10, Matthew Brost wrote:
> > >
> > > [snip]
> > >
> > > > +static int ct_send_nb(struct intel_guc_ct *ct,
> > > > + const u32 *action,
> > > > + u32 len,
> > > > + u32 flags)
> > > > +{
> > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > + unsigned long spin_flags;
> > > > + u32 fence;
> > > > + int ret;
> > > > +
> > > > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > > > +
> > > > + ret = ctb_has_room(ctb, len + 1);
> > > > + if (unlikely(ret))
> > > > + goto out;
> > > > +
> > > > + fence = ct_get_next_fence(ct);
> > > > + ret = ct_write(ct, action, len, fence, flags);
> > > > + if (unlikely(ret))
> > > > + goto out;
> > > > +
> > > > + intel_guc_notify(ct_to_guc(ct));
> > > > +
> > > > +out:
> > > > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > static int ct_send(struct intel_guc_ct *ct,
> > > > const u32 *action,
> > > > u32 len,
> > > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > u32 response_buf_size,
> > > > u32 *status)
> > > > {
> > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > struct ct_request request;
> > > > unsigned long flags;
> > > > u32 fence;
> > > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > GEM_BUG_ON(!len);
> > > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > > + might_sleep();
> > >
> > > Sleep is just cond_resched below or there is more?
> > >
> > > >>>
> > > >>> Yes, the cond_resched.
> > > >>>
> > > > + /*
> > > > + * We use a lazy spin wait loop here as we believe that
> > > > if the CT
> > > > + * buffers are sized correctly the flow control condition
> > > > should be
> > > > + * rare.
> > > > + */
> > > > +retry:
> > > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > > + spin_unlock_irqrestore(&ct->ctbs.send.lock,
> > > > flags);
> > > > + cond_resched();
> > > > + goto retry;
> > > > + }
> > >
> > > If this patch is about adding a non-blocking send function, and
> > > below we can
> > > see that it creates a fork:
> > >
> > > intel_guc_ct_send:
> > > ...
> > > if (flags & INTEL_GUC_SEND_NB)
> > > return ct_send_nb(ct, action, len, flags);
> > >
> > > ret = ct_send(ct, action, len, response_buf,
> > > response_buf_size, &status);
> > >
> > > Then why is there a change in ct_send here, which is not the new
> > > non-blocking path?
> > >
> > > >>>
> > > >>> There is not a change to ct_send(), just to intel_guc_ct_send.
> > > >>
> > > >> I was doing by the diff which says:
> > > >>
> > > >> static int ct_send(struct intel_guc_ct *ct,
> > > >> const u32 *action,
> > > >> u32 len,
> > > >> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > >> u32 response_buf_size,
> > > >> u32 *status)
> > > >> {
> > > >> +struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > >> struct ct_request request;
> > > >> unsigned long flags;
> > > >> u32 fence;
> > > >> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > >> GEM_BUG_ON(!len);
> > > >> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Wed, Jun 09, 2021 at 04:14:05PM +0200, Michal Wajdeczko wrote:
>
>
> On 07.06.2021 19:31, Matthew Brost wrote:
> > On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 27/05/2021 15:35, Matthew Brost wrote:
> >>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
>
> On 26/05/2021 19:10, Matthew Brost wrote:
>
> [snip]
>
> > +static int ct_send_nb(struct intel_guc_ct *ct,
> > + const u32 *action,
> > + u32 len,
> > + u32 flags)
> > +{
> > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > + unsigned long spin_flags;
> > + u32 fence;
> > + int ret;
> > +
> > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > +
> > + ret = ctb_has_room(ctb, len + 1);
> > + if (unlikely(ret))
> > + goto out;
> > +
> > + fence = ct_get_next_fence(ct);
> > + ret = ct_write(ct, action, len, fence, flags);
> > + if (unlikely(ret))
> > + goto out;
> > +
> > + intel_guc_notify(ct_to_guc(ct));
> > +
> > +out:
> > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > +
> > + return ret;
> > +}
> > +
> > static int ct_send(struct intel_guc_ct *ct,
> >const u32 *action,
> >u32 len,
> > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> >u32 response_buf_size,
> >u32 *status)
> > {
> > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > struct ct_request request;
> > unsigned long flags;
> > u32 fence;
> > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > GEM_BUG_ON(!len);
> > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > GEM_BUG_ON(!response_buf && response_buf_size);
> > + might_sleep();
>
> Sleep is just cond_resched below or there is more?
>
> >>>
> >>> Yes, the cond_resched.
> >>>
> > + /*
> > +* We use a lazy spin wait loop here as we believe that if the
> > CT
> > +* buffers are sized correctly the flow control condition
> > should be
> > +* rare.
> > +*/
> > +retry:
> > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> > + cond_resched();
> > + goto retry;
> > + }
>
> If this patch is about adding a non-blocking send function, and
> below we can
> see that it creates a fork:
>
> intel_guc_ct_send:
> ...
> if (flags & INTEL_GUC_SEND_NB)
> return ct_send_nb(ct, action, len, flags);
>
> ret = ct_send(ct, action, len, response_buf, response_buf_size,
> &status);
>
> Then why is there a change in ct_send here, which is not the new
> non-blocking path?
>
> >>>
> >>> There is not a change to ct_send(), just to intel_guc_ct_send.
> >>
> >> I was doing by the diff which says:
> >>
> >>static int ct_send(struct intel_guc_ct *ct,
> >> const u32 *action,
> >> u32 len,
> >> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> >> u32 response_buf_size,
> >> u32 *status)
> >>{
> >> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> >>struct ct_request request;
> >>unsigned long flags;
> >>u32 fence;
> >> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> >>GEM_BUG_ON(!len);
> >>GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> >>GEM_BUG_ON(!response_buf && response_buf_size);
> >> + might_sleep();
> >> + /*
> >> + * We use a lazy spin wait loop here as we believe that if the
> >> CT
> >> + * buffers are sized correctly the flow control condition
> >> should be
> >> + * rare.
> >> + */
> >> +retry:
> >>spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> >> + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> >> + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> >> + cond_resched();
> >> + goto retry;
> >> + }
> >>
> >> So it looks like a change
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Tue, Jun 08, 2021 at 10:46:15AM +0200, Daniel Vetter wrote:
> On Tue, Jun 8, 2021 at 10:39 AM Tvrtko Ursulin
> wrote:
> >
> >
> > On 07/06/2021 18:31, Matthew Brost wrote:
> > > On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> > >>
> > >> On 27/05/2021 15:35, Matthew Brost wrote:
> > >>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
> >
> > On 26/05/2021 19:10, Matthew Brost wrote:
> >
> > [snip]
> >
> > > +static int ct_send_nb(struct intel_guc_ct *ct,
> > > + const u32 *action,
> > > + u32 len,
> > > + u32 flags)
> > > +{
> > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > + unsigned long spin_flags;
> > > + u32 fence;
> > > + int ret;
> > > +
> > > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > > +
> > > + ret = ctb_has_room(ctb, len + 1);
> > > + if (unlikely(ret))
> > > + goto out;
> > > +
> > > + fence = ct_get_next_fence(ct);
> > > + ret = ct_write(ct, action, len, fence, flags);
> > > + if (unlikely(ret))
> > > + goto out;
> > > +
> > > + intel_guc_notify(ct_to_guc(ct));
> > > +
> > > +out:
> > > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int ct_send(struct intel_guc_ct *ct,
> > > const u32 *action,
> > > u32 len,
> > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > u32 response_buf_size,
> > > u32 *status)
> > > {
> > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > struct ct_request request;
> > > unsigned long flags;
> > > u32 fence;
> > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > GEM_BUG_ON(!len);
> > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > + might_sleep();
> >
> > Sleep is just cond_resched below or there is more?
> >
> > >>>
> > >>> Yes, the cond_resched.
> > >>>
> > > + /*
> > > + * We use a lazy spin wait loop here as we believe that if
> > > the CT
> > > + * buffers are sized correctly the flow control condition
> > > should be
> > > + * rare.
> > > + */
> > > +retry:
> > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> > > + cond_resched();
> > > + goto retry;
> > > + }
> >
> > If this patch is about adding a non-blocking send function, and
> > below we can
> > see that it creates a fork:
> >
> > intel_guc_ct_send:
> > ...
> > if (flags & INTEL_GUC_SEND_NB)
> > return ct_send_nb(ct, action, len, flags);
> >
> > ret = ct_send(ct, action, len, response_buf,
> > response_buf_size, &status);
> >
> > Then why is there a change in ct_send here, which is not the new
> > non-blocking path?
> >
> > >>>
> > >>> There is not a change to ct_send(), just to intel_guc_ct_send.
> > >>
> > >> I was doing by the diff which says:
> > >>
> > >> static int ct_send(struct intel_guc_ct *ct,
> > >> const u32 *action,
> > >> u32 len,
> > >> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > >> u32 response_buf_size,
> > >> u32 *status)
> > >> {
> > >> +struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > >> struct ct_request request;
> > >> unsigned long flags;
> > >> u32 fence;
> > >> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > >> GEM_BUG_ON(!len);
> > >> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > >> GEM_BUG_ON(!response_buf && response_buf_size);
> > >> +might_sleep();
> > >> +/*
> > >> + * We use a lazy spin wait loop here as we believe that if
> > >> the CT
> > >> + * buffers are sized correctly the flow control condition
> > >>
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Wed, Jun 09, 2021 at 03:58:38PM +0200, Michal Wajdeczko wrote:
>
>
> On 08.06.2021 10:39, Tvrtko Ursulin wrote:
> >
> > On 07/06/2021 18:31, Matthew Brost wrote:
> >> On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> >>>
> >>> On 27/05/2021 15:35, Matthew Brost wrote:
> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
> >
> > On 26/05/2021 19:10, Matthew Brost wrote:
> >
> > [snip]
> >
> >> +static int ct_send_nb(struct intel_guc_ct *ct,
> >> + const u32 *action,
> >> + u32 len,
> >> + u32 flags)
> >> +{
> >> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> >> + unsigned long spin_flags;
> >> + u32 fence;
> >> + int ret;
> >> +
> >> + spin_lock_irqsave(&ctb->lock, spin_flags);
> >> +
> >> + ret = ctb_has_room(ctb, len + 1);
> >> + if (unlikely(ret))
> >> + goto out;
> >> +
> >> + fence = ct_get_next_fence(ct);
> >> + ret = ct_write(ct, action, len, fence, flags);
> >> + if (unlikely(ret))
> >> + goto out;
> >> +
> >> + intel_guc_notify(ct_to_guc(ct));
> >> +
> >> +out:
> >> + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> static int ct_send(struct intel_guc_ct *ct,
> >> const u32 *action,
> >> u32 len,
> >> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> >> u32 response_buf_size,
> >> u32 *status)
> >> {
> >> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> >> struct ct_request request;
> >> unsigned long flags;
> >> u32 fence;
> >> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> >> GEM_BUG_ON(!len);
> >> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> >> GEM_BUG_ON(!response_buf && response_buf_size);
> >> + might_sleep();
> >
> > Sleep is just cond_resched below or there is more?
> >
>
> Yes, the cond_resched.
>
> >> + /*
> >> + * We use a lazy spin wait loop here as we believe that
> >> if the CT
> >> + * buffers are sized correctly the flow control condition
> >> should be
> >> + * rare.
> >> + */
> >> +retry:
> >> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> >> + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> >> + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> >> + cond_resched();
> >> + goto retry;
> >> + }
> >
> > If this patch is about adding a non-blocking send function, and
> > below we can
> > see that it creates a fork:
> >
> > intel_guc_ct_send:
> > ...
> > if (flags & INTEL_GUC_SEND_NB)
> > return ct_send_nb(ct, action, len, flags);
> >
> > ret = ct_send(ct, action, len, response_buf,
> > response_buf_size, &status);
> >
> > Then why is there a change in ct_send here, which is not the new
> > non-blocking path?
> >
>
> There is not a change to ct_send(), just to intel_guc_ct_send.
> >>>
> >>> I was doing by the diff which says:
> >>>
> >>> static int ct_send(struct intel_guc_ct *ct,
> >>> const u32 *action,
> >>> u32 len,
> >>> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> >>> u32 response_buf_size,
> >>> u32 *status)
> >>> {
> >>> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> >>> struct ct_request request;
> >>> unsigned long flags;
> >>> u32 fence;
> >>> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> >>> GEM_BUG_ON(!len);
> >>> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> >>> GEM_BUG_ON(!response_buf && response_buf_size);
> >>> + might_sleep();
> >>> + /*
> >>> + * We use a lazy spin wait loop here as we believe that if
> >>> the CT
> >>> + * buffers are sized correctly the flow control condition
> >>> should be
> >>> + * rare.
> >>> + */
> >>> +retry:
> >>> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> >>> + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> >>> + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> >>> + cond_resched();
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 07.06.2021 19:31, Matthew Brost wrote:
> On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
>>
>> On 27/05/2021 15:35, Matthew Brost wrote:
>>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
On 26/05/2021 19:10, Matthew Brost wrote:
[snip]
> +static int ct_send_nb(struct intel_guc_ct *ct,
> + const u32 *action,
> + u32 len,
> + u32 flags)
> +{
> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> + unsigned long spin_flags;
> + u32 fence;
> + int ret;
> +
> + spin_lock_irqsave(&ctb->lock, spin_flags);
> +
> + ret = ctb_has_room(ctb, len + 1);
> + if (unlikely(ret))
> + goto out;
> +
> + fence = ct_get_next_fence(ct);
> + ret = ct_write(ct, action, len, fence, flags);
> + if (unlikely(ret))
> + goto out;
> +
> + intel_guc_notify(ct_to_guc(ct));
> +
> +out:
> + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> +
> + return ret;
> +}
> +
> static int ct_send(struct intel_guc_ct *ct,
> const u32 *action,
> u32 len,
> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> u32 response_buf_size,
> u32 *status)
> {
> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> struct ct_request request;
> unsigned long flags;
> u32 fence;
> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> GEM_BUG_ON(!len);
> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> GEM_BUG_ON(!response_buf && response_buf_size);
> + might_sleep();
Sleep is just cond_resched below or there is more?
>>>
>>> Yes, the cond_resched.
>>>
> + /*
> + * We use a lazy spin wait loop here as we believe that if the
> CT
> + * buffers are sized correctly the flow control condition
> should be
> + * rare.
> + */
> +retry:
> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> + cond_resched();
> + goto retry;
> + }
If this patch is about adding a non-blocking send function, and below
we can
see that it creates a fork:
intel_guc_ct_send:
...
if (flags & INTEL_GUC_SEND_NB)
return ct_send_nb(ct, action, len, flags);
ret = ct_send(ct, action, len, response_buf, response_buf_size,
&status);
Then why is there a change in ct_send here, which is not the new
non-blocking path?
>>>
>>> There is not a change to ct_send(), just to intel_guc_ct_send.
>>
>> I was doing by the diff which says:
>>
>>static int ct_send(struct intel_guc_ct *ct,
>> const u32 *action,
>> u32 len,
>> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
>> u32 response_buf_size,
>> u32 *status)
>>{
>> +struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>> struct ct_request request;
>> unsigned long flags;
>> u32 fence;
>> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
>> GEM_BUG_ON(!len);
>> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>> GEM_BUG_ON(!response_buf && response_buf_size);
>> +might_sleep();
>> +/*
>> + * We use a lazy spin wait loop here as we believe that if the
>> CT
>> + * buffers are sized correctly the flow control condition
>> should be
>> + * rare.
>> + */
>> +retry:
>> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
>> +if (unlikely(!ctb_has_room(ctb, len + 1))) {
>> +spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
>> +cond_resched();
>> +goto retry;
>> +}
>>
>> So it looks like a change to ct_send to me. Is that wrong?
What about this part - is the patch changing the blocking ct_send or not,
and if it is why?
>>>
>>> Yes, ct_send() changes. Sorry for the confusion.
>>>
>>> This function needs to be updated to account for the H2G space and
>
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 08.06.2021 10:39, Tvrtko Ursulin wrote:
>
> On 07/06/2021 18:31, Matthew Brost wrote:
>> On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
>>>
>>> On 27/05/2021 15:35, Matthew Brost wrote:
On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
>
> On 26/05/2021 19:10, Matthew Brost wrote:
>
> [snip]
>
>> +static int ct_send_nb(struct intel_guc_ct *ct,
>> + const u32 *action,
>> + u32 len,
>> + u32 flags)
>> +{
>> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>> + unsigned long spin_flags;
>> + u32 fence;
>> + int ret;
>> +
>> + spin_lock_irqsave(&ctb->lock, spin_flags);
>> +
>> + ret = ctb_has_room(ctb, len + 1);
>> + if (unlikely(ret))
>> + goto out;
>> +
>> + fence = ct_get_next_fence(ct);
>> + ret = ct_write(ct, action, len, fence, flags);
>> + if (unlikely(ret))
>> + goto out;
>> +
>> + intel_guc_notify(ct_to_guc(ct));
>> +
>> +out:
>> + spin_unlock_irqrestore(&ctb->lock, spin_flags);
>> +
>> + return ret;
>> +}
>> +
>> static int ct_send(struct intel_guc_ct *ct,
>> const u32 *action,
>> u32 len,
>> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
>> u32 response_buf_size,
>> u32 *status)
>> {
>> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>> struct ct_request request;
>> unsigned long flags;
>> u32 fence;
>> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
>> GEM_BUG_ON(!len);
>> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>> GEM_BUG_ON(!response_buf && response_buf_size);
>> + might_sleep();
>
> Sleep is just cond_resched below or there is more?
>
Yes, the cond_resched.
>> + /*
>> + * We use a lazy spin wait loop here as we believe that
>> if the CT
>> + * buffers are sized correctly the flow control condition
>> should be
>> + * rare.
>> + */
>> +retry:
>> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
>> + if (unlikely(!ctb_has_room(ctb, len + 1))) {
>> + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
>> + cond_resched();
>> + goto retry;
>> + }
>
> If this patch is about adding a non-blocking send function, and
> below we can
> see that it creates a fork:
>
> intel_guc_ct_send:
> ...
> if (flags & INTEL_GUC_SEND_NB)
> return ct_send_nb(ct, action, len, flags);
>
> ret = ct_send(ct, action, len, response_buf,
> response_buf_size, &status);
>
> Then why is there a change in ct_send here, which is not the new
> non-blocking path?
>
There is not a change to ct_send(), just to intel_guc_ct_send.
>>>
>>> I was doing by the diff which says:
>>>
>>> static int ct_send(struct intel_guc_ct *ct,
>>> const u32 *action,
>>> u32 len,
>>> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
>>> u32 response_buf_size,
>>> u32 *status)
>>> {
>>> + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
>>> struct ct_request request;
>>> unsigned long flags;
>>> u32 fence;
>>> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
>>> GEM_BUG_ON(!len);
>>> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
>>> GEM_BUG_ON(!response_buf && response_buf_size);
>>> + might_sleep();
>>> + /*
>>> + * We use a lazy spin wait loop here as we believe that if
>>> the CT
>>> + * buffers are sized correctly the flow control condition
>>> should be
>>> + * rare.
>>> + */
>>> +retry:
>>> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
>>> + if (unlikely(!ctb_has_room(ctb, len + 1))) {
>>> + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
>>> + cond_resched();
>>> + goto retry;
>>> + }
>>>
>>> So it looks like a change to ct_send to me. Is that wrong?
>
> What about this part - is the patch changing the blocking ct_send
> or not,
> and if it is why?
>
Yes, ct_send() changes. Sorry for the confusion.
This functi
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Tue, Jun 8, 2021 at 10:39 AM Tvrtko Ursulin
wrote:
>
>
> On 07/06/2021 18:31, Matthew Brost wrote:
> > On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 27/05/2021 15:35, Matthew Brost wrote:
> >>> On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
>
> On 26/05/2021 19:10, Matthew Brost wrote:
>
> [snip]
>
> > +static int ct_send_nb(struct intel_guc_ct *ct,
> > + const u32 *action,
> > + u32 len,
> > + u32 flags)
> > +{
> > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > + unsigned long spin_flags;
> > + u32 fence;
> > + int ret;
> > +
> > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > +
> > + ret = ctb_has_room(ctb, len + 1);
> > + if (unlikely(ret))
> > + goto out;
> > +
> > + fence = ct_get_next_fence(ct);
> > + ret = ct_write(ct, action, len, fence, flags);
> > + if (unlikely(ret))
> > + goto out;
> > +
> > + intel_guc_notify(ct_to_guc(ct));
> > +
> > +out:
> > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > +
> > + return ret;
> > +}
> > +
> > static int ct_send(struct intel_guc_ct *ct,
> > const u32 *action,
> > u32 len,
> > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > u32 response_buf_size,
> > u32 *status)
> > {
> > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > struct ct_request request;
> > unsigned long flags;
> > u32 fence;
> > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > GEM_BUG_ON(!len);
> > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > GEM_BUG_ON(!response_buf && response_buf_size);
> > + might_sleep();
>
> Sleep is just cond_resched below or there is more?
>
> >>>
> >>> Yes, the cond_resched.
> >>>
> > + /*
> > + * We use a lazy spin wait loop here as we believe that if
> > the CT
> > + * buffers are sized correctly the flow control condition
> > should be
> > + * rare.
> > + */
> > +retry:
> > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> > + cond_resched();
> > + goto retry;
> > + }
>
> If this patch is about adding a non-blocking send function, and
> below we can
> see that it creates a fork:
>
> intel_guc_ct_send:
> ...
> if (flags & INTEL_GUC_SEND_NB)
> return ct_send_nb(ct, action, len, flags);
>
> ret = ct_send(ct, action, len, response_buf,
> response_buf_size, &status);
>
> Then why is there a change in ct_send here, which is not the new
> non-blocking path?
>
> >>>
> >>> There is not a change to ct_send(), just to intel_guc_ct_send.
> >>
> >> I was doing by the diff which says:
> >>
> >> static int ct_send(struct intel_guc_ct *ct,
> >> const u32 *action,
> >> u32 len,
> >> @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> >> u32 response_buf_size,
> >> u32 *status)
> >> {
> >> +struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> >> struct ct_request request;
> >> unsigned long flags;
> >> u32 fence;
> >> @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> >> GEM_BUG_ON(!len);
> >> GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> >> GEM_BUG_ON(!response_buf && response_buf_size);
> >> +might_sleep();
> >> +/*
> >> + * We use a lazy spin wait loop here as we believe that if
> >> the CT
> >> + * buffers are sized correctly the flow control condition
> >> should be
> >> + * rare.
> >> + */
> >> +retry:
> >> spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> >> +if (unlikely(!ctb_has_room(ctb, len + 1))) {
> >> +spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> >> +
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 07/06/2021 18:31, Matthew Brost wrote:
On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
On 27/05/2021 15:35, Matthew Brost wrote:
On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
On 26/05/2021 19:10, Matthew Brost wrote:
[snip]
+static int ct_send_nb(struct intel_guc_ct *ct,
+ const u32 *action,
+ u32 len,
+ u32 flags)
+{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
+ unsigned long spin_flags;
+ u32 fence;
+ int ret;
+
+ spin_lock_irqsave(&ctb->lock, spin_flags);
+
+ ret = ctb_has_room(ctb, len + 1);
+ if (unlikely(ret))
+ goto out;
+
+ fence = ct_get_next_fence(ct);
+ ret = ct_write(ct, action, len, fence, flags);
+ if (unlikely(ret))
+ goto out;
+
+ intel_guc_notify(ct_to_guc(ct));
+
+out:
+ spin_unlock_irqrestore(&ctb->lock, spin_flags);
+
+ return ret;
+}
+
static int ct_send(struct intel_guc_ct *ct,
const u32 *action,
u32 len,
@@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
u32 response_buf_size,
u32 *status)
{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct ct_request request;
unsigned long flags;
u32 fence;
@@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
+ might_sleep();
Sleep is just cond_resched below or there is more?
Yes, the cond_resched.
+ /*
+* We use a lazy spin wait loop here as we believe that if the CT
+* buffers are sized correctly the flow control condition should be
+* rare.
+*/
+retry:
spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+ if (unlikely(!ctb_has_room(ctb, len + 1))) {
+ spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+ cond_resched();
+ goto retry;
+ }
If this patch is about adding a non-blocking send function, and below we can
see that it creates a fork:
intel_guc_ct_send:
...
if (flags & INTEL_GUC_SEND_NB)
return ct_send_nb(ct, action, len, flags);
ret = ct_send(ct, action, len, response_buf, response_buf_size,
&status);
Then why is there a change in ct_send here, which is not the new
non-blocking path?
There is not a change to ct_send(), just to intel_guc_ct_send.
I was doing by the diff which says:
static int ct_send(struct intel_guc_ct *ct,
const u32 *action,
u32 len,
@@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
u32 response_buf_size,
u32 *status)
{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct ct_request request;
unsigned long flags;
u32 fence;
@@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
+ might_sleep();
+ /*
+* We use a lazy spin wait loop here as we believe that if the CT
+* buffers are sized correctly the flow control condition should be
+* rare.
+*/
+retry:
spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+ if (unlikely(!ctb_has_room(ctb, len + 1))) {
+ spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+ cond_resched();
+ goto retry;
+ }
So it looks like a change to ct_send to me. Is that wrong?
What about this part - is the patch changing the blocking ct_send or not,
and if it is why?
Yes, ct_send() changes. Sorry for the confusion.
This function needs to be updated to account for the H2G space and
backoff if no space is available.
Since this one is the sleeping path, it probably can and needs to be smarter
than having a cond_resched busy loop added. Like sleep and get woken up when
there is space. Otherwise it can degenerate to busy looping via contention
with the non-blocking path.
That screams over enginerring a simple problem to me. If the CT channel
is full we are really in trouble anyways - i.e. the performance is going
to terrible as we overwhelmed the GuC with traffic. That being said,
Performance of what would be terrible? Something relating to submitting
new jobs to the GPU I guess. Or something SRIOV related as you hint below.
But there is no real reason why CPU cycles/power should suffer if GuC is
busy.
Okay, if it can't happen in real world then it's possibly passable as a
design of a communication interface. But to me it leaves a bad taste and
a doubt that there is this other aspect of the real world. And that is
when the unexpected happens. Even the most trivial things like
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Thu, May 27, 2021 at 04:11:50PM +0100, Tvrtko Ursulin wrote:
>
> On 27/05/2021 15:35, Matthew Brost wrote:
> > On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 26/05/2021 19:10, Matthew Brost wrote:
> > >
> > > [snip]
> > >
> > > > > > > > +static int ct_send_nb(struct intel_guc_ct *ct,
> > > > > > > > + const u32 *action,
> > > > > > > > + u32 len,
> > > > > > > > + u32 flags)
> > > > > > > > +{
> > > > > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > > > > + unsigned long spin_flags;
> > > > > > > > + u32 fence;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > > > > > > > +
> > > > > > > > + ret = ctb_has_room(ctb, len + 1);
> > > > > > > > + if (unlikely(ret))
> > > > > > > > + goto out;
> > > > > > > > +
> > > > > > > > + fence = ct_get_next_fence(ct);
> > > > > > > > + ret = ct_write(ct, action, len, fence, flags);
> > > > > > > > + if (unlikely(ret))
> > > > > > > > + goto out;
> > > > > > > > +
> > > > > > > > + intel_guc_notify(ct_to_guc(ct));
> > > > > > > > +
> > > > > > > > +out:
> > > > > > > > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > > > > > > > +
> > > > > > > > + return ret;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static int ct_send(struct intel_guc_ct *ct,
> > > > > > > >const u32 *action,
> > > > > > > >u32 len,
> > > > > > > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > > > > >u32 response_buf_size,
> > > > > > > >u32 *status)
> > > > > > > > {
> > > > > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > > > > struct ct_request request;
> > > > > > > > unsigned long flags;
> > > > > > > > u32 fence;
> > > > > > > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > > > > > GEM_BUG_ON(!len);
> > > > > > > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > > > > > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > > > > > > + might_sleep();
> > > > > > >
> > > > > > > Sleep is just cond_resched below or there is more?
> > > > > > >
> > > > > >
> > > > > > Yes, the cond_resched.
> > > > > >
> > > > > > > > + /*
> > > > > > > > +* We use a lazy spin wait loop here as we believe that
> > > > > > > > if the CT
> > > > > > > > +* buffers are sized correctly the flow control
> > > > > > > > condition should be
> > > > > > > > +* rare.
> > > > > > > > +*/
> > > > > > > > +retry:
> > > > > > > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > > > > > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > > > > > > + spin_unlock_irqrestore(&ct->ctbs.send.lock,
> > > > > > > > flags);
> > > > > > > > + cond_resched();
> > > > > > > > + goto retry;
> > > > > > > > + }
> > > > > > >
> > > > > > > If this patch is about adding a non-blocking send function, and
> > > > > > > below we can
> > > > > > > see that it creates a fork:
> > > > > > >
> > > > > > > intel_guc_ct_send:
> > > > > > > ...
> > > > > > > if (flags & INTEL_GUC_SEND_NB)
> > > > > > > return ct_send_nb(ct, action, len, flags);
> > > > > > >
> > > > > > > ret = ct_send(ct, action, len, response_buf,
> > > > > > > response_buf_size, &status);
> > > > > > >
> > > > > > > Then why is there a change in ct_send here, which is not the new
> > > > > > > non-blocking path?
> > > > > > >
> > > > > >
> > > > > > There is not a change to ct_send(), just to intel_guc_ct_send.
> > > > >
> > > > > I was doing by the diff which says:
> > > > >
> > > > >static int ct_send(struct intel_guc_ct *ct,
> > > > > const u32 *action,
> > > > > u32 len,
> > > > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > > u32 response_buf_size,
> > > > > u32 *status)
> > > > >{
> > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > struct ct_request request;
> > > > > unsigned long flags;
> > > > > u32 fence;
> > > > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > > GEM_BUG_ON(!len);
> > > > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > > > + might_sleep();
> > > > > + /*
> > > > > + * We use a lazy spin wait loop here as we believe that if the
> > > > > CT
> > > > > + * buffers are sized correctly the flow control condition
> > > > > should be
> > > > > + * rare.
> > > > > + */
> > > > > +retry:
> > > > > spin_lock_irqsave(&ct->ctbs.send.
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 27/05/2021 15:35, Matthew Brost wrote:
On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
On 26/05/2021 19:10, Matthew Brost wrote:
[snip]
+static int ct_send_nb(struct intel_guc_ct *ct,
+ const u32 *action,
+ u32 len,
+ u32 flags)
+{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
+ unsigned long spin_flags;
+ u32 fence;
+ int ret;
+
+ spin_lock_irqsave(&ctb->lock, spin_flags);
+
+ ret = ctb_has_room(ctb, len + 1);
+ if (unlikely(ret))
+ goto out;
+
+ fence = ct_get_next_fence(ct);
+ ret = ct_write(ct, action, len, fence, flags);
+ if (unlikely(ret))
+ goto out;
+
+ intel_guc_notify(ct_to_guc(ct));
+
+out:
+ spin_unlock_irqrestore(&ctb->lock, spin_flags);
+
+ return ret;
+}
+
static int ct_send(struct intel_guc_ct *ct,
const u32 *action,
u32 len,
@@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
u32 response_buf_size,
u32 *status)
{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct ct_request request;
unsigned long flags;
u32 fence;
@@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
+ might_sleep();
Sleep is just cond_resched below or there is more?
Yes, the cond_resched.
+ /*
+* We use a lazy spin wait loop here as we believe that if the CT
+* buffers are sized correctly the flow control condition should be
+* rare.
+*/
+retry:
spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+ if (unlikely(!ctb_has_room(ctb, len + 1))) {
+ spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+ cond_resched();
+ goto retry;
+ }
If this patch is about adding a non-blocking send function, and below we can
see that it creates a fork:
intel_guc_ct_send:
...
if (flags & INTEL_GUC_SEND_NB)
return ct_send_nb(ct, action, len, flags);
ret = ct_send(ct, action, len, response_buf, response_buf_size,
&status);
Then why is there a change in ct_send here, which is not the new
non-blocking path?
There is not a change to ct_send(), just to intel_guc_ct_send.
I was doing by the diff which says:
static int ct_send(struct intel_guc_ct *ct,
const u32 *action,
u32 len,
@@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
u32 response_buf_size,
u32 *status)
{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct ct_request request;
unsigned long flags;
u32 fence;
@@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
+ might_sleep();
+ /*
+* We use a lazy spin wait loop here as we believe that if the CT
+* buffers are sized correctly the flow control condition should be
+* rare.
+*/
+retry:
spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+ if (unlikely(!ctb_has_room(ctb, len + 1))) {
+ spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+ cond_resched();
+ goto retry;
+ }
So it looks like a change to ct_send to me. Is that wrong?
What about this part - is the patch changing the blocking ct_send or not,
and if it is why?
Yes, ct_send() changes. Sorry for the confusion.
This function needs to be updated to account for the H2G space and
backoff if no space is available.
Since this one is the sleeping path, it probably can and needs to be
smarter than having a cond_resched busy loop added. Like sleep and get
woken up when there is space. Otherwise it can degenerate to busy
looping via contention with the non-blocking path.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Thu, May 27, 2021 at 11:02:24AM +0100, Tvrtko Ursulin wrote:
>
> On 26/05/2021 19:10, Matthew Brost wrote:
>
> [snip]
>
> > > > > > +static int ct_send_nb(struct intel_guc_ct *ct,
> > > > > > + const u32 *action,
> > > > > > + u32 len,
> > > > > > + u32 flags)
> > > > > > +{
> > > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > > + unsigned long spin_flags;
> > > > > > + u32 fence;
> > > > > > + int ret;
> > > > > > +
> > > > > > + spin_lock_irqsave(&ctb->lock, spin_flags);
> > > > > > +
> > > > > > + ret = ctb_has_room(ctb, len + 1);
> > > > > > + if (unlikely(ret))
> > > > > > + goto out;
> > > > > > +
> > > > > > + fence = ct_get_next_fence(ct);
> > > > > > + ret = ct_write(ct, action, len, fence, flags);
> > > > > > + if (unlikely(ret))
> > > > > > + goto out;
> > > > > > +
> > > > > > + intel_guc_notify(ct_to_guc(ct));
> > > > > > +
> > > > > > +out:
> > > > > > + spin_unlock_irqrestore(&ctb->lock, spin_flags);
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > static int ct_send(struct intel_guc_ct *ct,
> > > > > >const u32 *action,
> > > > > >u32 len,
> > > > > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > > >u32 response_buf_size,
> > > > > >u32 *status)
> > > > > > {
> > > > > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > > > > struct ct_request request;
> > > > > > unsigned long flags;
> > > > > > u32 fence;
> > > > > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > > > > GEM_BUG_ON(!len);
> > > > > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > > > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > > > > + might_sleep();
> > > > >
> > > > > Sleep is just cond_resched below or there is more?
> > > > >
> > > >
> > > > Yes, the cond_resched.
> > > >
> > > > > > + /*
> > > > > > +* We use a lazy spin wait loop here as we believe that if the
> > > > > > CT
> > > > > > +* buffers are sized correctly the flow control condition
> > > > > > should be
> > > > > > +* rare.
> > > > > > +*/
> > > > > > +retry:
> > > > > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > > > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > > > > + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> > > > > > + cond_resched();
> > > > > > + goto retry;
> > > > > > + }
> > > > >
> > > > > If this patch is about adding a non-blocking send function, and below
> > > > > we can
> > > > > see that it creates a fork:
> > > > >
> > > > > intel_guc_ct_send:
> > > > > ...
> > > > > if (flags & INTEL_GUC_SEND_NB)
> > > > > return ct_send_nb(ct, action, len, flags);
> > > > >
> > > > > ret = ct_send(ct, action, len, response_buf, response_buf_size,
> > > > > &status);
> > > > >
> > > > > Then why is there a change in ct_send here, which is not the new
> > > > > non-blocking path?
> > > > >
> > > >
> > > > There is not a change to ct_send(), just to intel_guc_ct_send.
> > >
> > > I was doing by the diff which says:
> > >
> > > static int ct_send(struct intel_guc_ct *ct,
> > > const u32 *action,
> > > u32 len,
> > > @@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
> > > u32 response_buf_size,
> > > u32 *status)
> > > {
> > > + struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > > struct ct_request request;
> > > unsigned long flags;
> > > u32 fence;
> > > @@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
> > > GEM_BUG_ON(!len);
> > > GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
> > > GEM_BUG_ON(!response_buf && response_buf_size);
> > > + might_sleep();
> > > + /*
> > > + * We use a lazy spin wait loop here as we believe that if the CT
> > > + * buffers are sized correctly the flow control condition should be
> > > + * rare.
> > > + */
> > > +retry:
> > > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > > + if (unlikely(!ctb_has_room(ctb, len + 1))) {
> > > + spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> > > + cond_resched();
> > > + goto retry;
> > > + }
> > >
> > > So it looks like a change to ct_send to me. Is that wrong?
>
> What about this part - is the patch changing the blocking ct_send or not,
> and if it is why?
>
Yes, ct_send() changes. Sorry for the confusion.
This function needs to be updated to account for the H2G space and
backoff if no space is available.
Matt
> Regards,
>
> Tvrtko
>
>
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > > As for why intel_guc_ct_send is updated and we
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 26/05/2021 19:10, Matthew Brost wrote:
[snip]
+static int ct_send_nb(struct intel_guc_ct *ct,
+ const u32 *action,
+ u32 len,
+ u32 flags)
+{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
+ unsigned long spin_flags;
+ u32 fence;
+ int ret;
+
+ spin_lock_irqsave(&ctb->lock, spin_flags);
+
+ ret = ctb_has_room(ctb, len + 1);
+ if (unlikely(ret))
+ goto out;
+
+ fence = ct_get_next_fence(ct);
+ ret = ct_write(ct, action, len, fence, flags);
+ if (unlikely(ret))
+ goto out;
+
+ intel_guc_notify(ct_to_guc(ct));
+
+out:
+ spin_unlock_irqrestore(&ctb->lock, spin_flags);
+
+ return ret;
+}
+
static int ct_send(struct intel_guc_ct *ct,
const u32 *action,
u32 len,
@@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
u32 response_buf_size,
u32 *status)
{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct ct_request request;
unsigned long flags;
u32 fence;
@@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
+ might_sleep();
Sleep is just cond_resched below or there is more?
Yes, the cond_resched.
+ /*
+* We use a lazy spin wait loop here as we believe that if the CT
+* buffers are sized correctly the flow control condition should be
+* rare.
+*/
+retry:
spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+ if (unlikely(!ctb_has_room(ctb, len + 1))) {
+ spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+ cond_resched();
+ goto retry;
+ }
If this patch is about adding a non-blocking send function, and below we can
see that it creates a fork:
intel_guc_ct_send:
...
if (flags & INTEL_GUC_SEND_NB)
return ct_send_nb(ct, action, len, flags);
ret = ct_send(ct, action, len, response_buf, response_buf_size,
&status);
Then why is there a change in ct_send here, which is not the new
non-blocking path?
There is not a change to ct_send(), just to intel_guc_ct_send.
I was doing by the diff which says:
static int ct_send(struct intel_guc_ct *ct,
const u32 *action,
u32 len,
@@ -473,6 +541,7 @@ static int ct_send(struct intel_guc_ct *ct,
u32 response_buf_size,
u32 *status)
{
+ struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct ct_request request;
unsigned long flags;
u32 fence;
@@ -482,8 +551,20 @@ static int ct_send(struct intel_guc_ct *ct,
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
+ might_sleep();
+ /*
+* We use a lazy spin wait loop here as we believe that if the CT
+* buffers are sized correctly the flow control condition should be
+* rare.
+*/
+retry:
spin_lock_irqsave(&ct->ctbs.send.lock, flags);
+ if (unlikely(!ctb_has_room(ctb, len + 1))) {
+ spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
+ cond_resched();
+ goto retry;
+ }
So it looks like a change to ct_send to me. Is that wrong?
What about this part - is the patch changing the blocking ct_send or
not, and if it is why?
Regards,
Tvrtko
Regards,
Tvrtko
As for why intel_guc_ct_send is updated and we don't just a new public
function, this was another reviewers suggestion. Again can't make
everyone happy.
fence = ct_get_next_fence(ct);
request.fence = fence;
@@ -495,7 +576,7 @@ static int ct_send(struct intel_guc_ct *ct,
list_add_tail(&request.link, &ct->requests.pending);
spin_unlock(&ct->requests.lock);
- err = ct_write(ct, action, len, fence);
+ err = ct_write(ct, action, len, fence, 0);
spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
@@ -537,7 +618,7 @@ static int ct_send(struct intel_guc_ct *ct,
* Command Transport (CT) buffer based GuC send function.
*/
int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
- u32 *response_buf, u32 response_buf_size)
+ u32 *response_buf, u32 response_buf_size, u32 flags)
{
u32 status = ~0; /* undefined */
int ret;
@@ -547,6 +628,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32
*action, u32 len,
return -ENODEV;
}
+ if (flags & INTEL_GUC_SEND_NB)
+ return ct_send_nb(ct, action, len, flags);
+
ret = ct_send(ct, action, len, response_buf, response_buf_size,
&status);
if (unlikely(ret < 0)) {
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Wed, May 26, 2021 at 09:57:10AM +0100, Tvrtko Ursulin wrote:
>
> On 25/05/2021 18:21, Matthew Brost wrote:
> > On Tue, May 25, 2021 at 10:21:00AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 06/05/2021 20:13, Matthew Brost wrote:
> > > > Add non blocking CTB send function, intel_guc_send_nb. In order to
> > > > support a non blocking CTB send function a spin lock is needed to
> > > > protect the CTB descriptors fields. Also the non blocking call must not
> > > > update the fence value as this value is owned by the blocking call
> > > > (intel_guc_send).
> > >
> > > Could the commit message say why the non-blocking send function is needed?
> > >
> >
> > Sure. Something like:
> >
> > 'CTBs will be used in the critical patch of GuC submission and there is
> > no need to wait for each CTB complete before moving on the i915'
>
> A bit more, like also mentioning the critical path is with interrupts
> disabled or so. And not just that there is no need to wait but waiting is not
> possible because this or that. So only choice is to do this busy loop send.
> It's a bit horrible so justification needs to be documented.
>
Don't I basically say all this? Anyways I'll scrub this comment.
> > > > The blocking CTB now must have a flow control mechanism to ensure the
> > > > buffer isn't overrun. A lazy spin wait is used as we believe the flow
> > > > control condition should be rare with properly sized buffer.
> > > >
> > > > The function, intel_guc_send_nb, is exported in this patch but unused.
> > > > Several patches later in the series make use of this function.
> > > >
> > > > Signed-off-by: John Harrison
> > > > Signed-off-by: Matthew Brost
> > > > ---
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96
> > > > +--
> > > >drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
> > > >3 files changed, 105 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > index c20f3839de12..4c0a367e41d8 100644
> > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > > @@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
> > > > intel_guc_log *log)
> > > >static
> > > >inline int intel_guc_send(struct intel_guc *guc, const u32 *action,
> > > > u32 len)
> > > >{
> > > > - return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
> > > > + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
> > > > +}
> > > > +
> > > > +#define INTEL_GUC_SEND_NB BIT(31)
> > > > +static
> > > > +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action,
> > > > u32 len)
> > > > +{
> > > > + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
> > > > +INTEL_GUC_SEND_NB);
> > > >}
> > > >static inline int
> > > > @@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc,
> > > > const u32 *action, u32 len,
> > > >u32 *response_buf, u32 response_buf_size)
> > > >{
> > > > return intel_guc_ct_send(&guc->ct, action, len,
> > > > -response_buf, response_buf_size);
> > > > +response_buf, response_buf_size, 0);
> > > >}
> > > >static inline void intel_guc_to_host_event_handler(struct intel_guc
> > > > *guc)
> > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > > index a76603537fa8..af7314d45a78 100644
> > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > > > @@ -3,6 +3,11 @@
> > > > * Copyright © 2016-2019 Intel Corporation
> > > > */
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > >#include "i915_drv.h"
> > > >#include "intel_guc_ct.h"
> > > >#include "gt/intel_gt.h"
> > > > @@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
> > > > if (unlikely(err))
> > > > goto err_deregister;
> > > > + ct->requests.last_fence = 1;
> > > > ct->enabled = true;
> > > > return 0;
> > > > @@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct
> > > > *ct)
> > > > return ++ct->requests.last_fence;
> > > >}
> > > > +static void write_barrier(struct intel_guc_ct *ct) {
> > > > + struct intel_guc *guc = ct_to_guc(ct);
> > > > + struct intel_gt *gt = guc_to_gt(guc);
> > > > +
> > > > + if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> > > > + GEM_BUG_ON(guc->send_regs.fw_domains);
> > > > + intel_uncore_write_fw(gt->uncore,
> > > > GEN11_SOFT_SCRATCH(0), 0);
> > >
> > > It's safe to write to this reg? Does it need a comment to explain it?
>
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 25/05/2021 18:21, Matthew Brost wrote:
On Tue, May 25, 2021 at 10:21:00AM +0100, Tvrtko Ursulin wrote:
On 06/05/2021 20:13, Matthew Brost wrote:
Add non blocking CTB send function, intel_guc_send_nb. In order to
support a non blocking CTB send function a spin lock is needed to
protect the CTB descriptors fields. Also the non blocking call must not
update the fence value as this value is owned by the blocking call
(intel_guc_send).
Could the commit message say why the non-blocking send function is needed?
Sure. Something like:
'CTBs will be used in the critical patch of GuC submission and there is
no need to wait for each CTB complete before moving on the i915'
A bit more, like also mentioning the critical path is with interrupts disabled
or so. And not just that there is no need to wait but waiting is not possible
because this or that. So only choice is to do this busy loop send. It's a bit
horrible so justification needs to be documented.
The blocking CTB now must have a flow control mechanism to ensure the
buffer isn't overrun. A lazy spin wait is used as we believe the flow
control condition should be rare with properly sized buffer.
The function, intel_guc_send_nb, is exported in this patch but unused.
Several patches later in the series make use of this function.
Signed-off-by: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96 +--
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
3 files changed, 105 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index c20f3839de12..4c0a367e41d8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
intel_guc_log *log)
static
inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
{
- return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
+ return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
+}
+
+#define INTEL_GUC_SEND_NB BIT(31)
+static
+inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len)
+{
+ return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
+INTEL_GUC_SEND_NB);
}
static inline int
@@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32
*action, u32 len,
u32 *response_buf, u32 response_buf_size)
{
return intel_guc_ct_send(&guc->ct, action, len,
-response_buf, response_buf_size);
+response_buf, response_buf_size, 0);
}
static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index a76603537fa8..af7314d45a78 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -3,6 +3,11 @@
* Copyright © 2016-2019 Intel Corporation
*/
+#include
+#include
+#include
+#include
+
#include "i915_drv.h"
#include "intel_guc_ct.h"
#include "gt/intel_gt.h"
@@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
if (unlikely(err))
goto err_deregister;
+ ct->requests.last_fence = 1;
ct->enabled = true;
return 0;
@@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
return ++ct->requests.last_fence;
}
+static void write_barrier(struct intel_guc_ct *ct) {
+ struct intel_guc *guc = ct_to_guc(ct);
+ struct intel_gt *gt = guc_to_gt(guc);
+
+ if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
+ GEM_BUG_ON(guc->send_regs.fw_domains);
+ intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
It's safe to write to this reg? Does it need a comment to explain it?
Yes, it is same. IMO 'SCRATCH' in the name is enough documentation.
Why would it be enough? It requires digging to figure it out since it appears
these are some sort of GuC special registers and not generic scratch:
commit 2d4ed3a988e6b1ff9729d0edd74bf4890571253e
Author: Michal Wajdeczko
Date: Mon May 27 18:36:05 2019 +
drm/i915/guc: New GuC scratch registers for Gen11
If it was a normal scratch then async trashing of those from a random driver
thread isn't per se safe if used from a GPU context running in parallel.
But then according to bspec they are called VF_SW_FLAG_ and not
GEN11_SOFT_SCRATCH so yeah.
+ } else {
+ wmb();
+ }
+}
+
static int ct_write(struct intel_guc_ct *ct,
const u32 *action,
u32 len /* in dwords */,
- u32 fence)
+ u32 fence, u32 flags)
{
struct i
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Mon, May 24, 2021 at 02:21:42PM +0200, Michal Wajdeczko wrote:
>
>
> On 06.05.2021 21:13, Matthew Brost wrote:
> > Add non blocking CTB send function, intel_guc_send_nb. In order to
> > support a non blocking CTB send function a spin lock is needed to
>
> spin lock was added in 16/97
>
> > protect the CTB descriptors fields. Also the non blocking call must not
> > update the fence value as this value is owned by the blocking call
> > (intel_guc_send).
>
> all H2G messages are using "fence", nb variant also needs to update it
>
> >
> > The blocking CTB now must have a flow control mechanism to ensure the
>
> s/blocking/non-blocking
>
Will fix the comments as these are a bit stale.
> > buffer isn't overrun. A lazy spin wait is used as we believe the flow
> > control condition should be rare with properly sized buffer.
>
> as this new nb function is still not used in this patch, then maybe
> better to move flow control to separate patch for easier review ?
>
You can't do non-blocking without flow control, it just doesn't work.
IMO that makes the review harder.
> >
> > The function, intel_guc_send_nb, is exported in this patch but unused.
> > Several patches later in the series make use of this function.
> >
> > Signed-off-by: John Harrison
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96 +--
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
> > 3 files changed, 105 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index c20f3839de12..4c0a367e41d8 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
> > intel_guc_log *log)
> > static
> > inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32
> > len)
> > {
> > - return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
> > + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
> > +}
> > +
> > +#define INTEL_GUC_SEND_NB BIT(31)
> > +static
> > +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32
> > len)
> > +{
> > + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
> > +INTEL_GUC_SEND_NB);
> > }
> >
> > static inline int
> > @@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const
> > u32 *action, u32 len,
> >u32 *response_buf, u32 response_buf_size)
> > {
> > return intel_guc_ct_send(&guc->ct, action, len,
> > -response_buf, response_buf_size);
> > +response_buf, response_buf_size, 0);
> > }
> >
> > static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > index a76603537fa8..af7314d45a78 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -3,6 +3,11 @@
> > * Copyright © 2016-2019 Intel Corporation
> > */
> >
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > #include "i915_drv.h"
> > #include "intel_guc_ct.h"
> > #include "gt/intel_gt.h"
> > @@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
> > if (unlikely(err))
> > goto err_deregister;
> >
> > + ct->requests.last_fence = 1;
>
> not needed
>
Yep.
> > ct->enabled = true;
> >
> > return 0;
> > @@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
> > return ++ct->requests.last_fence;
> > }
> >
> > +static void write_barrier(struct intel_guc_ct *ct) {
> > + struct intel_guc *guc = ct_to_guc(ct);
> > + struct intel_gt *gt = guc_to_gt(guc);
> > +
> > + if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> > + GEM_BUG_ON(guc->send_regs.fw_domains);
> > + intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
> > + } else {
> > + wmb();
> > + }
> > +}
>
> this chunk seems to be good candidate for separate patch that could be
> introduced earlier
>
Yes. Will include patches 3-20 post as this technically is required once
the mutex is removed.
> > +
> > static int ct_write(struct intel_guc_ct *ct,
> > const u32 *action,
> > u32 len /* in dwords */,
> > - u32 fence)
> > + u32 fence, u32 flags)
> > {
> > struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > struct guc_ct_buffer_desc *desc = ctb->desc;
> > @@ -393,9 +411,13 @@ static int ct_write(struct intel_guc_ct *ct,
> > FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
> > FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);
> >
> > - hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE,
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On Tue, May 25, 2021 at 10:21:00AM +0100, Tvrtko Ursulin wrote:
>
> On 06/05/2021 20:13, Matthew Brost wrote:
> > Add non blocking CTB send function, intel_guc_send_nb. In order to
> > support a non blocking CTB send function a spin lock is needed to
> > protect the CTB descriptors fields. Also the non blocking call must not
> > update the fence value as this value is owned by the blocking call
> > (intel_guc_send).
>
> Could the commit message say why the non-blocking send function is needed?
>
Sure. Something like:
'CTBs will be used in the critical patch of GuC submission and there is
no need to wait for each CTB complete before moving on the i915'
> >
> > The blocking CTB now must have a flow control mechanism to ensure the
> > buffer isn't overrun. A lazy spin wait is used as we believe the flow
> > control condition should be rare with properly sized buffer.
> >
> > The function, intel_guc_send_nb, is exported in this patch but unused.
> > Several patches later in the series make use of this function.
> >
> > Signed-off-by: John Harrison
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96 +--
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
> > 3 files changed, 105 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index c20f3839de12..4c0a367e41d8 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
> > intel_guc_log *log)
> > static
> > inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32
> > len)
> > {
> > - return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
> > + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
> > +}
> > +
> > +#define INTEL_GUC_SEND_NB BIT(31)
> > +static
> > +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32
> > len)
> > +{
> > + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
> > +INTEL_GUC_SEND_NB);
> > }
> > static inline int
> > @@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const
> > u32 *action, u32 len,
> >u32 *response_buf, u32 response_buf_size)
> > {
> > return intel_guc_ct_send(&guc->ct, action, len,
> > -response_buf, response_buf_size);
> > +response_buf, response_buf_size, 0);
> > }
> > static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > index a76603537fa8..af7314d45a78 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -3,6 +3,11 @@
> >* Copyright © 2016-2019 Intel Corporation
> >*/
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > #include "i915_drv.h"
> > #include "intel_guc_ct.h"
> > #include "gt/intel_gt.h"
> > @@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
> > if (unlikely(err))
> > goto err_deregister;
> > + ct->requests.last_fence = 1;
> > ct->enabled = true;
> > return 0;
> > @@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
> > return ++ct->requests.last_fence;
> > }
> > +static void write_barrier(struct intel_guc_ct *ct) {
> > + struct intel_guc *guc = ct_to_guc(ct);
> > + struct intel_gt *gt = guc_to_gt(guc);
> > +
> > + if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> > + GEM_BUG_ON(guc->send_regs.fw_domains);
> > + intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
>
> It's safe to write to this reg? Does it need a comment to explain it?
>
Yes, it is same. IMO 'SCRATCH' in the name is enough documentation.
> > + } else {
> > + wmb();
> > + }
> > +}
> > +
> > static int ct_write(struct intel_guc_ct *ct,
> > const u32 *action,
> > u32 len /* in dwords */,
> > - u32 fence)
> > + u32 fence, u32 flags)
> > {
> > struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> > struct guc_ct_buffer_desc *desc = ctb->desc;
> > @@ -393,9 +411,13 @@ static int ct_write(struct intel_guc_ct *ct,
> > FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
> > FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);
> > - hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
> > - FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
> > -GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
> > + hxg = (flags & INTEL_GUC_SEND_NB) ?
> > + (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
> > +FIELD_PREP(GUC_HXG_EVENT_MSG
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 06/05/2021 20:13, Matthew Brost wrote:
Add non blocking CTB send function, intel_guc_send_nb. In order to
support a non blocking CTB send function a spin lock is needed to
protect the CTB descriptors fields. Also the non blocking call must not
update the fence value as this value is owned by the blocking call
(intel_guc_send).
Could the commit message say why the non-blocking send function is needed?
The blocking CTB now must have a flow control mechanism to ensure the
buffer isn't overrun. A lazy spin wait is used as we believe the flow
control condition should be rare with properly sized buffer.
The function, intel_guc_send_nb, is exported in this patch but unused.
Several patches later in the series make use of this function.
Signed-off-by: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96 +--
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
3 files changed, 105 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index c20f3839de12..4c0a367e41d8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
intel_guc_log *log)
static
inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
{
- return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
+ return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
+}
+
+#define INTEL_GUC_SEND_NB BIT(31)
+static
+inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len)
+{
+ return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
+INTEL_GUC_SEND_NB);
}
static inline int
@@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32
*action, u32 len,
u32 *response_buf, u32 response_buf_size)
{
return intel_guc_ct_send(&guc->ct, action, len,
-response_buf, response_buf_size);
+response_buf, response_buf_size, 0);
}
static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index a76603537fa8..af7314d45a78 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -3,6 +3,11 @@
* Copyright © 2016-2019 Intel Corporation
*/
+#include
+#include
+#include
+#include
+
#include "i915_drv.h"
#include "intel_guc_ct.h"
#include "gt/intel_gt.h"
@@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
if (unlikely(err))
goto err_deregister;
+ ct->requests.last_fence = 1;
ct->enabled = true;
return 0;
@@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
return ++ct->requests.last_fence;
}
+static void write_barrier(struct intel_guc_ct *ct) {
+ struct intel_guc *guc = ct_to_guc(ct);
+ struct intel_gt *gt = guc_to_gt(guc);
+
+ if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
+ GEM_BUG_ON(guc->send_regs.fw_domains);
+ intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
It's safe to write to this reg? Does it need a comment to explain it?
+ } else {
+ wmb();
+ }
+}
+
static int ct_write(struct intel_guc_ct *ct,
const u32 *action,
u32 len /* in dwords */,
- u32 fence)
+ u32 fence, u32 flags)
{
struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct guc_ct_buffer_desc *desc = ctb->desc;
@@ -393,9 +411,13 @@ static int ct_write(struct intel_guc_ct *ct,
FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);
- hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
- FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
-GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
+ hxg = (flags & INTEL_GUC_SEND_NB) ?
+ (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
+FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
+ GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
+ (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
+FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
+ GUC_HXG_REQUEST_MSG_0_DATA0, action[0]));
CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n",
tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]);
@@ -412,6 +434,12 @@ static int ct_write(struct intel_guc_ct *ct,
}
GEM_BUG_ON(tail > size);
+ /*
+* make sure H2G buffer update and LRC tail
Re: [Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
On 06.05.2021 21:13, Matthew Brost wrote:
> Add non blocking CTB send function, intel_guc_send_nb. In order to
> support a non blocking CTB send function a spin lock is needed to
spin lock was added in 16/97
> protect the CTB descriptors fields. Also the non blocking call must not
> update the fence value as this value is owned by the blocking call
> (intel_guc_send).
all H2G messages are using "fence", nb variant also needs to update it
>
> The blocking CTB now must have a flow control mechanism to ensure the
s/blocking/non-blocking
> buffer isn't overrun. A lazy spin wait is used as we believe the flow
> control condition should be rare with properly sized buffer.
as this new nb function is still not used in this patch, then maybe
better to move flow control to separate patch for easier review ?
>
> The function, intel_guc_send_nb, is exported in this patch but unused.
> Several patches later in the series make use of this function.
>
> Signed-off-by: John Harrison
> Signed-off-by: Matthew Brost
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96 +--
> drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
> 3 files changed, 105 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index c20f3839de12..4c0a367e41d8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
> intel_guc_log *log)
> static
> inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
> {
> - return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
> + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
> +}
> +
> +#define INTEL_GUC_SEND_NBBIT(31)
> +static
> +inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32
> len)
> +{
> + return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
> + INTEL_GUC_SEND_NB);
> }
>
> static inline int
> @@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32
> *action, u32 len,
> u32 *response_buf, u32 response_buf_size)
> {
> return intel_guc_ct_send(&guc->ct, action, len,
> - response_buf, response_buf_size);
> + response_buf, response_buf_size, 0);
> }
>
> static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index a76603537fa8..af7314d45a78 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -3,6 +3,11 @@
> * Copyright © 2016-2019 Intel Corporation
> */
>
> +#include
> +#include
> +#include
> +#include
> +
> #include "i915_drv.h"
> #include "intel_guc_ct.h"
> #include "gt/intel_gt.h"
> @@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
> if (unlikely(err))
> goto err_deregister;
>
> + ct->requests.last_fence = 1;
not needed
> ct->enabled = true;
>
> return 0;
> @@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
> return ++ct->requests.last_fence;
> }
>
> +static void write_barrier(struct intel_guc_ct *ct) {
> + struct intel_guc *guc = ct_to_guc(ct);
> + struct intel_gt *gt = guc_to_gt(guc);
> +
> + if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
> + GEM_BUG_ON(guc->send_regs.fw_domains);
> + intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
> + } else {
> + wmb();
> + }
> +}
this chunk seems to be good candidate for separate patch that could be
introduced earlier
> +
> static int ct_write(struct intel_guc_ct *ct,
> const u32 *action,
> u32 len /* in dwords */,
> - u32 fence)
> + u32 fence, u32 flags)
> {
> struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> struct guc_ct_buffer_desc *desc = ctb->desc;
> @@ -393,9 +411,13 @@ static int ct_write(struct intel_guc_ct *ct,
>FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
>FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);
>
> - hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
> - FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
> - GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
> + hxg = (flags & INTEL_GUC_SEND_NB) ?
> + (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
> + FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
> + GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
> + (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
> + FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
> +
[Intel-gfx] [RFC PATCH 36/97] drm/i915/guc: Add non blocking CTB send function
Add non blocking CTB send function, intel_guc_send_nb. In order to
support a non blocking CTB send function a spin lock is needed to
protect the CTB descriptors fields. Also the non blocking call must not
update the fence value as this value is owned by the blocking call
(intel_guc_send).
The blocking CTB now must have a flow control mechanism to ensure the
buffer isn't overrun. A lazy spin wait is used as we believe the flow
control condition should be rare with properly sized buffer.
The function, intel_guc_send_nb, is exported in this patch but unused.
Several patches later in the series make use of this function.
Signed-off-by: John Harrison
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 12 ++-
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 96 +--
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 7 +-
3 files changed, 105 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index c20f3839de12..4c0a367e41d8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -75,7 +75,15 @@ static inline struct intel_guc *log_to_guc(struct
intel_guc_log *log)
static
inline int intel_guc_send(struct intel_guc *guc, const u32 *action, u32 len)
{
- return intel_guc_ct_send(&guc->ct, action, len, NULL, 0);
+ return intel_guc_ct_send(&guc->ct, action, len, NULL, 0, 0);
+}
+
+#define INTEL_GUC_SEND_NB BIT(31)
+static
+inline int intel_guc_send_nb(struct intel_guc *guc, const u32 *action, u32 len)
+{
+ return intel_guc_ct_send(&guc->ct, action, len, NULL, 0,
+INTEL_GUC_SEND_NB);
}
static inline int
@@ -83,7 +91,7 @@ intel_guc_send_and_receive(struct intel_guc *guc, const u32
*action, u32 len,
u32 *response_buf, u32 response_buf_size)
{
return intel_guc_ct_send(&guc->ct, action, len,
-response_buf, response_buf_size);
+response_buf, response_buf_size, 0);
}
static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
index a76603537fa8..af7314d45a78 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -3,6 +3,11 @@
* Copyright © 2016-2019 Intel Corporation
*/
+#include
+#include
+#include
+#include
+
#include "i915_drv.h"
#include "intel_guc_ct.h"
#include "gt/intel_gt.h"
@@ -308,6 +313,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
if (unlikely(err))
goto err_deregister;
+ ct->requests.last_fence = 1;
ct->enabled = true;
return 0;
@@ -343,10 +349,22 @@ static u32 ct_get_next_fence(struct intel_guc_ct *ct)
return ++ct->requests.last_fence;
}
+static void write_barrier(struct intel_guc_ct *ct) {
+ struct intel_guc *guc = ct_to_guc(ct);
+ struct intel_gt *gt = guc_to_gt(guc);
+
+ if (i915_gem_object_is_lmem(guc->ct.vma->obj)) {
+ GEM_BUG_ON(guc->send_regs.fw_domains);
+ intel_uncore_write_fw(gt->uncore, GEN11_SOFT_SCRATCH(0), 0);
+ } else {
+ wmb();
+ }
+}
+
static int ct_write(struct intel_guc_ct *ct,
const u32 *action,
u32 len /* in dwords */,
- u32 fence)
+ u32 fence, u32 flags)
{
struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
struct guc_ct_buffer_desc *desc = ctb->desc;
@@ -393,9 +411,13 @@ static int ct_write(struct intel_guc_ct *ct,
FIELD_PREP(GUC_CTB_MSG_0_NUM_DWORDS, len) |
FIELD_PREP(GUC_CTB_MSG_0_FENCE, fence);
- hxg = FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
- FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
-GUC_HXG_REQUEST_MSG_0_DATA0, action[0]);
+ hxg = (flags & INTEL_GUC_SEND_NB) ?
+ (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_EVENT) |
+FIELD_PREP(GUC_HXG_EVENT_MSG_0_ACTION |
+ GUC_HXG_EVENT_MSG_0_DATA0, action[0])) :
+ (FIELD_PREP(GUC_HXG_MSG_0_TYPE, GUC_HXG_TYPE_REQUEST) |
+FIELD_PREP(GUC_HXG_REQUEST_MSG_0_ACTION |
+ GUC_HXG_REQUEST_MSG_0_DATA0, action[0]));
CT_DEBUG(ct, "writing (tail %u) %*ph %*ph %*ph\n",
tail, 4, &header, 4, &hxg, 4 * (len - 1), &action[1]);
@@ -412,6 +434,12 @@ static int ct_write(struct intel_guc_ct *ct,
}
GEM_BUG_ON(tail > size);
+ /*
+* make sure H2G buffer update and LRC tail update (if this triggering a
+* submission) are visable before updating the descriptor tail
+*/
+ write_barrier(ct);
+
/* now update descriptor */
WRITE_ONCE(desc->tail,
