diff mbox series

[RFC,18/97] drm/i915/guc: Don't receive all G2H messages in irq handler

Message ID 20210506191451.77768-19-matthew.brost@intel.com (mailing list archive)
State New, archived
Headers show
Series Basic GuC submission support in the i915 | expand

Commit Message

Matthew Brost May 6, 2021, 7:13 p.m. UTC 
From: Michal Wajdeczko <michal.wajdeczko@intel.com>

In irq handler try to receive just single G2H message, let other
messages to be received from tasklet.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 67 ++++++++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  3 +
 2 files changed, 50 insertions(+), 20 deletions(-)

Comments

Matthew Brost May 25, 2021, 6:15 p.m. UTC | #1 
On Thu, May 06, 2021 at 12:13:32PM -0700, Matthew Brost wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> In irq handler try to receive just single G2H message, let other
> messages to be received from tasklet.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 67 ++++++++++++++++-------
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  3 +
>  2 files changed, 50 insertions(+), 20 deletions(-)
> 
> 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 cb58fa7f970c..d630ec32decf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -81,6 +81,7 @@ enum { CTB_SEND = 0, CTB_RECV = 1 };
>  
>  enum { CTB_OWNER_HOST = 0 };
>  
> +static void ct_receive_tasklet_func(unsigned long data);
>  static void ct_incoming_request_worker_func(struct work_struct *w);
>  
>  /**
> @@ -95,6 +96,7 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>  	INIT_LIST_HEAD(&ct->requests.pending);
>  	INIT_LIST_HEAD(&ct->requests.incoming);
>  	INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);
> +	tasklet_init(&ct->receive_tasklet, ct_receive_tasklet_func, (unsigned long)ct);

This function is deprecated. tasklet_setup should be used.
I can fix this up when I post the CTB patches.

>  }
>  
>  static inline const char *guc_ct_buffer_type_to_str(u32 type)
> @@ -244,6 +246,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)
>  {
>  	GEM_BUG_ON(ct->enabled);
>  
> +	tasklet_kill(&ct->receive_tasklet);
>  	i915_vma_unpin_and_release(&ct->vma, I915_VMA_RELEASE_MAP);
>  	memset(ct, 0, sizeof(*ct));
>  }
> @@ -629,7 +632,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
>  	CT_DEBUG(ct, "received %*ph\n", 4 * len, data);
>  
>  	desc->head = head * 4;
> -	return 0;
> +	return available - len;
>  
>  corrupted:
>  	CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n",
> @@ -665,10 +668,10 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>  	u32 status;
>  	u32 datalen;
>  	struct ct_request *req;
> +	unsigned long flags;
>  	bool found = false;
>  
>  	GEM_BUG_ON(!ct_header_is_response(header));
> -	GEM_BUG_ON(!in_irq());
>  
>  	/* Response payload shall at least include fence and status */
>  	if (unlikely(len < 2)) {
> @@ -688,7 +691,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>  
>  	CT_DEBUG(ct, "response fence %u status %#x\n", fence, status);
>  
> -	spin_lock(&ct->requests.lock);
> +	spin_lock_irqsave(&ct->requests.lock, flags);
>  	list_for_each_entry(req, &ct->requests.pending, link) {
>  		if (unlikely(fence != req->fence)) {
>  			CT_DEBUG(ct, "request %u awaits response\n",
> @@ -707,7 +710,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>  		found = true;
>  		break;
>  	}
> -	spin_unlock(&ct->requests.lock);
> +	spin_unlock_irqrestore(&ct->requests.lock, flags);
>  
>  	if (!found)
>  		CT_ERROR(ct, "Unsolicited response %*ph\n", msgsize, msg);
> @@ -821,31 +824,55 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
>  	return 0;
>  }
>  
> +static int ct_receive(struct intel_guc_ct *ct)
> +{
> +	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
> +	ret = ct_read(ct, msg);
> +	spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ct_header_is_response(msg[0]))
> +		ct_handle_response(ct, msg);
> +	else
> +		ct_handle_request(ct, msg);
> +
> +	return ret;
> +}
> +
> +static void ct_try_receive_message(struct intel_guc_ct *ct)
> +{
> +	int ret;
> +
> +	if (GEM_WARN_ON(!ct->enabled))
> +		return;
> +
> +	ret = ct_receive(ct);
> +	if (ret > 0)
> +		tasklet_hi_schedule(&ct->receive_tasklet);
> +}
> +
> +static void ct_receive_tasklet_func(unsigned long data)
> +{

As mentioned above tasklet_init is deprecated. The callback now accepts
the tasklet structure and ct can be looked up via container_of macro.

Everything else looks good.

With that and changing to the new tasklet API:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> +	struct intel_guc_ct *ct = (struct intel_guc_ct *)data;
> +
> +	ct_try_receive_message(ct);
> +}
> +
>  /*
>   * When we're communicating with the GuC over CT, GuC uses events
>   * to notify us about new messages being posted on the RECV buffer.
>   */
>  void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
>  {
> -	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
> -	unsigned long flags;
> -	int err = 0;
> -
>  	if (unlikely(!ct->enabled)) {
>  		WARN(1, "Unexpected GuC event received while CT disabled!\n");
>  		return;
>  	}
>  
> -	do {
> -		spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
> -		err = ct_read(ct, msg);
> -		spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
> -		if (err)
> -			break;
> -
> -		if (ct_header_is_response(msg[0]))
> -			err = ct_handle_response(ct, msg);
> -		else
> -			err = ct_handle_request(ct, msg);
> -	} while (!err);
> +	ct_try_receive_message(ct);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> index bc52dc479a14..cb222f202301 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> @@ -6,6 +6,7 @@
>  #ifndef _INTEL_GUC_CT_H_
>  #define _INTEL_GUC_CT_H_
>  
> +#include <linux/interrupt.h>
>  #include <linux/spinlock.h>
>  #include <linux/workqueue.h>
>  
> @@ -55,6 +56,8 @@ struct intel_guc_ct {
>  		struct intel_guc_ct_buffer recv;
>  	} ctbs;
>  
> +	struct tasklet_struct receive_tasklet;
> +
>  	struct {
>  		u32 last_fence; /* last fence used to send request */
>  
> -- 
> 2.28.0
>
Michal Wajdeczko May 25, 2021, 7:43 p.m. UTC | #2 
On 25.05.2021 20:15, Matthew Brost wrote:
> On Thu, May 06, 2021 at 12:13:32PM -0700, Matthew Brost wrote:
>> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> In irq handler try to receive just single G2H message, let other
>> messages to be received from tasklet.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 67 ++++++++++++++++-------
>>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  3 +
>>  2 files changed, 50 insertions(+), 20 deletions(-)
>>
>> 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 cb58fa7f970c..d630ec32decf 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
>> @@ -81,6 +81,7 @@ enum { CTB_SEND = 0, CTB_RECV = 1 };
>>  
>>  enum { CTB_OWNER_HOST = 0 };
>>  
>> +static void ct_receive_tasklet_func(unsigned long data);
>>  static void ct_incoming_request_worker_func(struct work_struct *w);
>>  
>>  /**
>> @@ -95,6 +96,7 @@ void intel_guc_ct_init_early(struct intel_guc_ct *ct)
>>  	INIT_LIST_HEAD(&ct->requests.pending);
>>  	INIT_LIST_HEAD(&ct->requests.incoming);
>>  	INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);
>> +	tasklet_init(&ct->receive_tasklet, ct_receive_tasklet_func, (unsigned long)ct);
> 
> This function is deprecated. tasklet_setup should be used.
> I can fix this up when I post the CTB patches.

I didn't notice that, but yes, passing struct to callback will be
cleaner, so please fix it

Thanks,
Michal

> 
>>  }
>>  
>>  static inline const char *guc_ct_buffer_type_to_str(u32 type)
>> @@ -244,6 +246,7 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct)
>>  {
>>  	GEM_BUG_ON(ct->enabled);
>>  
>> +	tasklet_kill(&ct->receive_tasklet);
>>  	i915_vma_unpin_and_release(&ct->vma, I915_VMA_RELEASE_MAP);
>>  	memset(ct, 0, sizeof(*ct));
>>  }
>> @@ -629,7 +632,7 @@ static int ct_read(struct intel_guc_ct *ct, u32 *data)
>>  	CT_DEBUG(ct, "received %*ph\n", 4 * len, data);
>>  
>>  	desc->head = head * 4;
>> -	return 0;
>> +	return available - len;
>>  
>>  corrupted:
>>  	CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n",
>> @@ -665,10 +668,10 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>  	u32 status;
>>  	u32 datalen;
>>  	struct ct_request *req;
>> +	unsigned long flags;
>>  	bool found = false;
>>  
>>  	GEM_BUG_ON(!ct_header_is_response(header));
>> -	GEM_BUG_ON(!in_irq());
>>  
>>  	/* Response payload shall at least include fence and status */
>>  	if (unlikely(len < 2)) {
>> @@ -688,7 +691,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>  
>>  	CT_DEBUG(ct, "response fence %u status %#x\n", fence, status);
>>  
>> -	spin_lock(&ct->requests.lock);
>> +	spin_lock_irqsave(&ct->requests.lock, flags);
>>  	list_for_each_entry(req, &ct->requests.pending, link) {
>>  		if (unlikely(fence != req->fence)) {
>>  			CT_DEBUG(ct, "request %u awaits response\n",
>> @@ -707,7 +710,7 @@ static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
>>  		found = true;
>>  		break;
>>  	}
>> -	spin_unlock(&ct->requests.lock);
>> +	spin_unlock_irqrestore(&ct->requests.lock, flags);
>>  
>>  	if (!found)
>>  		CT_ERROR(ct, "Unsolicited response %*ph\n", msgsize, msg);
>> @@ -821,31 +824,55 @@ static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
>>  	return 0;
>>  }
>>  
>> +static int ct_receive(struct intel_guc_ct *ct)
>> +{
>> +	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
>> +	ret = ct_read(ct, msg);
>> +	spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ct_header_is_response(msg[0]))
>> +		ct_handle_response(ct, msg);
>> +	else
>> +		ct_handle_request(ct, msg);
>> +
>> +	return ret;
>> +}
>> +
>> +static void ct_try_receive_message(struct intel_guc_ct *ct)
>> +{
>> +	int ret;
>> +
>> +	if (GEM_WARN_ON(!ct->enabled))
>> +		return;
>> +
>> +	ret = ct_receive(ct);
>> +	if (ret > 0)
>> +		tasklet_hi_schedule(&ct->receive_tasklet);
>> +}
>> +
>> +static void ct_receive_tasklet_func(unsigned long data)
>> +{
> 
> As mentioned above tasklet_init is deprecated. The callback now accepts
> the tasklet structure and ct can be looked up via container_of macro.
> 
> Everything else looks good.
> 
> With that and changing to the new tasklet API:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
>> +	struct intel_guc_ct *ct = (struct intel_guc_ct *)data;
>> +
>> +	ct_try_receive_message(ct);
>> +}
>> +
>>  /*
>>   * When we're communicating with the GuC over CT, GuC uses events
>>   * to notify us about new messages being posted on the RECV buffer.
>>   */
>>  void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
>>  {
>> -	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
>> -	unsigned long flags;
>> -	int err = 0;
>> -
>>  	if (unlikely(!ct->enabled)) {
>>  		WARN(1, "Unexpected GuC event received while CT disabled!\n");
>>  		return;
>>  	}
>>  
>> -	do {
>> -		spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
>> -		err = ct_read(ct, msg);
>> -		spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
>> -		if (err)
>> -			break;
>> -
>> -		if (ct_header_is_response(msg[0]))
>> -			err = ct_handle_response(ct, msg);
>> -		else
>> -			err = ct_handle_request(ct, msg);
>> -	} while (!err);
>> +	ct_try_receive_message(ct);
>>  }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> index bc52dc479a14..cb222f202301 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> @@ -6,6 +6,7 @@
>>  #ifndef _INTEL_GUC_CT_H_
>>  #define _INTEL_GUC_CT_H_
>>  
>> +#include <linux/interrupt.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/workqueue.h>
>>  
>> @@ -55,6 +56,8 @@ struct intel_guc_ct {
>>  		struct intel_guc_ct_buffer recv;
>>  	} ctbs;
>>  
>> +	struct tasklet_struct receive_tasklet;
>> +
>>  	struct {
>>  		u32 last_fence; /* last fence used to send request */
>>  
>> -- 
>> 2.28.0
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
diff mbox series

Patch

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 cb58fa7f970c..d630ec32decf 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
@@ -81,6 +81,7 @@  enum { CTB_SEND = 0, CTB_RECV = 1 };
 
 enum { CTB_OWNER_HOST = 0 };
 
+static void ct_receive_tasklet_func(unsigned long data);
 static void ct_incoming_request_worker_func(struct work_struct *w);
 
 /**
@@ -95,6 +96,7 @@  void intel_guc_ct_init_early(struct intel_guc_ct *ct)
 	INIT_LIST_HEAD(&ct->requests.pending);
 	INIT_LIST_HEAD(&ct->requests.incoming);
 	INIT_WORK(&ct->requests.worker, ct_incoming_request_worker_func);
+	tasklet_init(&ct->receive_tasklet, ct_receive_tasklet_func, (unsigned long)ct);
 }
 
 static inline const char *guc_ct_buffer_type_to_str(u32 type)
@@ -244,6 +246,7 @@  void intel_guc_ct_fini(struct intel_guc_ct *ct)
 {
 	GEM_BUG_ON(ct->enabled);
 
+	tasklet_kill(&ct->receive_tasklet);
 	i915_vma_unpin_and_release(&ct->vma, I915_VMA_RELEASE_MAP);
 	memset(ct, 0, sizeof(*ct));
 }
@@ -629,7 +632,7 @@  static int ct_read(struct intel_guc_ct *ct, u32 *data)
 	CT_DEBUG(ct, "received %*ph\n", 4 * len, data);
 
 	desc->head = head * 4;
-	return 0;
+	return available - len;
 
 corrupted:
 	CT_ERROR(ct, "Corrupted descriptor addr=%#x head=%u tail=%u size=%u\n",
@@ -665,10 +668,10 @@  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 	u32 status;
 	u32 datalen;
 	struct ct_request *req;
+	unsigned long flags;
 	bool found = false;
 
 	GEM_BUG_ON(!ct_header_is_response(header));
-	GEM_BUG_ON(!in_irq());
 
 	/* Response payload shall at least include fence and status */
 	if (unlikely(len < 2)) {
@@ -688,7 +691,7 @@  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 
 	CT_DEBUG(ct, "response fence %u status %#x\n", fence, status);
 
-	spin_lock(&ct->requests.lock);
+	spin_lock_irqsave(&ct->requests.lock, flags);
 	list_for_each_entry(req, &ct->requests.pending, link) {
 		if (unlikely(fence != req->fence)) {
 			CT_DEBUG(ct, "request %u awaits response\n",
@@ -707,7 +710,7 @@  static int ct_handle_response(struct intel_guc_ct *ct, const u32 *msg)
 		found = true;
 		break;
 	}
-	spin_unlock(&ct->requests.lock);
+	spin_unlock_irqrestore(&ct->requests.lock, flags);
 
 	if (!found)
 		CT_ERROR(ct, "Unsolicited response %*ph\n", msgsize, msg);
@@ -821,31 +824,55 @@  static int ct_handle_request(struct intel_guc_ct *ct, const u32 *msg)
 	return 0;
 }
 
+static int ct_receive(struct intel_guc_ct *ct)
+{
+	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
+	ret = ct_read(ct, msg);
+	spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
+	if (ret < 0)
+		return ret;
+
+	if (ct_header_is_response(msg[0]))
+		ct_handle_response(ct, msg);
+	else
+		ct_handle_request(ct, msg);
+
+	return ret;
+}
+
+static void ct_try_receive_message(struct intel_guc_ct *ct)
+{
+	int ret;
+
+	if (GEM_WARN_ON(!ct->enabled))
+		return;
+
+	ret = ct_receive(ct);
+	if (ret > 0)
+		tasklet_hi_schedule(&ct->receive_tasklet);
+}
+
+static void ct_receive_tasklet_func(unsigned long data)
+{
+	struct intel_guc_ct *ct = (struct intel_guc_ct *)data;
+
+	ct_try_receive_message(ct);
+}
+
 /*
  * When we're communicating with the GuC over CT, GuC uses events
  * to notify us about new messages being posted on the RECV buffer.
  */
 void intel_guc_ct_event_handler(struct intel_guc_ct *ct)
 {
-	u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra dw for the header */
-	unsigned long flags;
-	int err = 0;
-
 	if (unlikely(!ct->enabled)) {
 		WARN(1, "Unexpected GuC event received while CT disabled!\n");
 		return;
 	}
 
-	do {
-		spin_lock_irqsave(&ct->ctbs.recv.lock, flags);
-		err = ct_read(ct, msg);
-		spin_unlock_irqrestore(&ct->ctbs.recv.lock, flags);
-		if (err)
-			break;
-
-		if (ct_header_is_response(msg[0]))
-			err = ct_handle_response(ct, msg);
-		else
-			err = ct_handle_request(ct, msg);
-	} while (!err);
+	ct_try_receive_message(ct);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index bc52dc479a14..cb222f202301 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -6,6 +6,7 @@ 
 #ifndef _INTEL_GUC_CT_H_
 #define _INTEL_GUC_CT_H_
 
+#include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
@@ -55,6 +56,8 @@  struct intel_guc_ct {
 		struct intel_guc_ct_buffer recv;
 	} ctbs;
 
+	struct tasklet_struct receive_tasklet;
+
 	struct {
 		u32 last_fence; /* last fence used to send request */