[PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Jean-Nicolas Graux]

Current way we find a waiting virtual channel for the next transfer
at the time one physical channel becomes free is not really fair.

More in details, in case there is more than one channel waiting at a time,
by just going through the arrays of memcpy and slave channels and stopping
as soon as state match waiting state, channels with high indexes can be
penalized.

Whenever dma engine is substantially overloaded so that we constantly
get several channels waiting, channels with highest indexes might not
be served for a substantial time which in the worse case, might hang
task that wait for dma transfer to complete.

This patch makes physical channel re-assignment more fair by storing
time in jiffies when a channel is put in waiting state. Whenever a
physical channel has to be re-assigned, this time is used to select
channel that is waiting for the longest time.

Signed-off-by: Jean-Nicolas Graux <jean-nicolas.graux@st.com>
---
 drivers/dma/amba-pl08x.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index fc8c2ba..4da830d 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -254,6 +254,7 @@ enum pl08x_dma_chan_state {
  * @slave: whether this channel is a device (slave) or for memcpy
  * @signal: the physical DMA request signal which this channel is using
  * @mux_use: count of descriptors using this DMA request signal setting
+ * @waiting_at: time in jiffies when this channel moved to waiting state
  */
 struct pl08x_dma_chan {
 	struct virt_dma_chan vc;
@@ -267,6 +268,7 @@ struct pl08x_dma_chan {
 	bool slave;
 	int signal;
 	unsigned mux_use;
+	unsigned long waiting_at;
 };
 
 /**
@@ -875,6 +877,7 @@ static void pl08x_phy_alloc_and_start(struct pl08x_dma_chan *plchan)
 	if (!ch) {
 		dev_dbg(&pl08x->adev->dev, "no physical channel available for xfer on %s\n", plchan->name);
 		plchan->state = PL08X_CHAN_WAITING;
+		plchan->waiting_at = jiffies;
 		return;
 	}
 
@@ -913,22 +916,25 @@ static void pl08x_phy_free(struct pl08x_dma_chan *plchan)
 {
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_dma_chan *p, *next;
-
+	unsigned long waiting_at;
  retry:
 	next = NULL;
+	waiting_at = jiffies;
 
 	/* Find a waiting virtual channel for the next transfer. */
 	list_for_each_entry(p, &pl08x->memcpy.channels, vc.chan.device_node)
-		if (p->state == PL08X_CHAN_WAITING) {
+		if (p->state == PL08X_CHAN_WAITING &&
+		    p->waiting_at <= waiting_at) {
 			next = p;
-			break;
+			waiting_at = p->waiting_at;
 		}
 
 	if (!next && pl08x->has_slave) {
 		list_for_each_entry(p, &pl08x->slave.channels, vc.chan.device_node)
-			if (p->state == PL08X_CHAN_WAITING) {
+			if (p->state == PL08X_CHAN_WAITING &&
+			    p->waiting_at <= waiting_at) {
 				next = p;
-				break;
+				waiting_at = p->waiting_at;
 			}
 	}
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Linus Walleij]

Hi Jean-Nicolas,

thanks for your patch!

You will have to resent the patch to the DMAengine list and the maintainer
(Vinod).

Out of curiosity: what platform are you using this on?

On Fri, Mar 1, 2019 at 5:18 PM Jean-Nicolas Graux
<jean-nicolas.graux@st.com> wrote:

> Current way we find a waiting virtual channel for the next transfer
> at the time one physical channel becomes free is not really fair.
>
> More in details, in case there is more than one channel waiting at a time,
> by just going through the arrays of memcpy and slave channels and stopping
> as soon as state match waiting state, channels with high indexes can be
> penalized.
>
> Whenever dma engine is substantially overloaded so that we constantly
> get several channels waiting, channels with highest indexes might not
> be served for a substantial time which in the worse case, might hang
> task that wait for dma transfer to complete.
>
> This patch makes physical channel re-assignment more fair by storing
> time in jiffies when a channel is put in waiting state. Whenever a
> physical channel has to be re-assigned, this time is used to select
> channel that is waiting for the longest time.
>
> Signed-off-by: Jean-Nicolas Graux <jean-nicolas.graux@st.com>

That's a neat trick, look like a good idea. Please add some
comment in the code in pl08x_phy_free() so it is clear
what is going on for people reading the code, with that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Jean Nicolas GRAUX]

Hello Linus,

You are welcome ;)

On 04/03/2019 11:00 AM, Linus Walleij wrote:
> Hi Jean-Nicolas,
>
> thanks for your patch!
>
> You will have to resent the patch to the DMAengine list and the maintainer
> (Vinod).

Sure I will. Sorry for that mistake.

>
> Out of curiosity: what platform are you using this on?

That's for the STMicro STA1295/STA1385 SoCs which make use of several 
ARM amba peripherals as well as nomadik gpio/pinctrl, etc ...
Those machines are unfortunately not available upstream. I would like to 
allocate time for that but as you know,
such activity require to dedicate substantial time & human resource. And 
we failed to find enough time for that up to now.

>
> On Fri, Mar 1, 2019 at 5:18 PM Jean-Nicolas Graux
> <jean-nicolas.graux@st.com> wrote:
>
>> Current way we find a waiting virtual channel for the next transfer
>> at the time one physical channel becomes free is not really fair.
>>
>> More in details, in case there is more than one channel waiting at a time,
>> by just going through the arrays of memcpy and slave channels and stopping
>> as soon as state match waiting state, channels with high indexes can be
>> penalized.
>>
>> Whenever dma engine is substantially overloaded so that we constantly
>> get several channels waiting, channels with highest indexes might not
>> be served for a substantial time which in the worse case, might hang
>> task that wait for dma transfer to complete.
>>
>> This patch makes physical channel re-assignment more fair by storing
>> time in jiffies when a channel is put in waiting state. Whenever a
>> physical channel has to be re-assigned, this time is used to select
>> channel that is waiting for the longest time.
>>
>> Signed-off-by: Jean-Nicolas Graux <jean-nicolas.graux@st.com>
> That's a neat trick, look like a good idea. Please add some
> comment in the code in pl08x_phy_free() so it is clear
> what is going on for people reading the code, with that:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I will.

Thanks for the review.

Regards. Jean-Nicolas.

>
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Linus Walleij]

On Mon, Mar 4, 2019 at 11:37 AM Jean Nicolas GRAUX
<jean-nicolas.graux@st.com> wrote:
> On 04/03/2019 11:00 AM, Linus Walleij wrote:

> > Out of curiosity: what platform are you using this on?
>
> That's for the STMicro STA1295/STA1385 SoCs which make use of several
> ARM amba peripherals as well as nomadik gpio/pinctrl, etc ...
> Those machines are unfortunately not available upstream. I would like to
> allocate time for that but as you know,
> such activity require to dedicate substantial time & human resource. And
> we failed to find enough time for that up to now.

That's OK, very nice to know that some of these drivers/IPs are in
active use. It looks like siblings to Cartesio, so I assume they are
closer to Nomadik than Ux500?

I am testing Nomadik a lot since the NHK15 development board has
very nice form factor, and I try to keep Ux500 running as well.

I recently submitted the Ux500 MCDE (multi-channel display engine)
graphics driver for review and I'm working on it, I don't know if this
series is using the old PL111-derivative (already supported by DRM),
MCDE or something else, would be nice to know so I know if I shall
put you in review for it :)
https://patchwork.freedesktop.org/patch/284491/

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Jean Nicolas GRAUX]

On 04/03/2019 1:05 PM, Linus Walleij wrote:
> On Mon, Mar 4, 2019 at 11:37 AM Jean Nicolas GRAUX
> <jean-nicolas.graux@st.com> wrote:
>> On 04/03/2019 11:00 AM, Linus Walleij wrote:
>>> Out of curiosity: what platform are you using this on?
>> That's for the STMicro STA1295/STA1385 SoCs which make use of several
>> ARM amba peripherals as well as nomadik gpio/pinctrl, etc ...
>> Those machines are unfortunately not available upstream. I would like to
>> allocate time for that but as you know,
>> such activity require to dedicate substantial time & human resource. And
>> we failed to find enough time for that up to now.
> That's OK, very nice to know that some of these drivers/IPs are in
> active use. It looks like siblings to Cartesio, so I assume they are
> closer to Nomadik than Ux500?

Yes, STA1295/STA1385 are SoCs for car radio & telematics market and it 
came after Cartesio SoC.

So while it share some IPs with the ux500 (like pl08x, pl18x, pl011, 
pl022, nmk-gpio, nmk-i2c, nmk-mtu, fsmc-nand, ...),
I guess you are right saying that it's closer to Nomadik architecture.

>
> I am testing Nomadik a lot since the NHK15 development board has
> very nice form factor, and I try to keep Ux500 running as well.

And honestly we benefit from your hard work. Thank you for that.

>
> I recently submitted the Ux500 MCDE (multi-channel display engine)
> graphics driver for review and I'm working on it, I don't know if this
> series is using the old PL111-derivative (already supported by DRM),
> MCDE or something else, would be nice to know so I know if I shall
> put you in review for it :)
> https://patchwork.freedesktop.org/patch/284491/

Unfortunately, STA SoCx do not rely on the MCDE engine but on STM LTDC 
that is also used by some STM32 SoC.
(devicetree/bindings/display/st,stm32-ltdc.txt).

I know almost nothing about display but as far as I remember, MCDE is 
known to be a quite complex peripheral!
Congratulations for taking up of such a challenging task.

Regards, Jean-Nicolas.

>
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Jean-Nicolas Graux]

Current way we find a waiting virtual channel for the next transfer
at the time one physical channel becomes free is not really fair.

More in details, in case there is more than one channel waiting at a time,
by just going through the arrays of memcpy and slave channels and stopping
as soon as state match waiting state, channels with high indexes can be
penalized.

Whenever dma engine is substantially overloaded so that we constantly
get several channels waiting, channels with highest indexes might not
be served for a substantial time which in the worse case, might hang
task that wait for dma transfer to complete.

This patch makes physical channel re-assignment more fair by storing
time in jiffies when a channel is put in waiting state. Whenever a
physical channel has to be re-assigned, this time is used to select
channel that is waiting for the longest time.

Signed-off-by: Jean-Nicolas Graux <jean-nicolas.graux@st.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Nicolas Guion <nicolas.guion@st.com>
---
 drivers/dma/amba-pl08x.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index fc8c2ba..8cfc753 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -254,6 +254,7 @@ enum pl08x_dma_chan_state {
  * @slave: whether this channel is a device (slave) or for memcpy
  * @signal: the physical DMA request signal which this channel is using
  * @mux_use: count of descriptors using this DMA request signal setting
+ * @waiting_at: time in jiffies when this channel moved to waiting state
  */
 struct pl08x_dma_chan {
 	struct virt_dma_chan vc;
@@ -267,6 +268,7 @@ struct pl08x_dma_chan {
 	bool slave;
 	int signal;
 	unsigned mux_use;
+	unsigned long waiting_at;
 };
 
 /**
@@ -875,6 +877,7 @@ static void pl08x_phy_alloc_and_start(struct pl08x_dma_chan *plchan)
 	if (!ch) {
 		dev_dbg(&pl08x->adev->dev, "no physical channel available for xfer on %s\n", plchan->name);
 		plchan->state = PL08X_CHAN_WAITING;
+		plchan->waiting_at = jiffies;
 		return;
 	}
 
@@ -913,22 +916,29 @@ static void pl08x_phy_free(struct pl08x_dma_chan *plchan)
 {
 	struct pl08x_driver_data *pl08x = plchan->host;
 	struct pl08x_dma_chan *p, *next;
-
+	unsigned long waiting_at;
  retry:
 	next = NULL;
+	waiting_at = jiffies;
 
-	/* Find a waiting virtual channel for the next transfer. */
+	/*
+	 * Find a waiting virtual channel for the next transfer.
+	 * To be fair, time when each channel reached waiting state is compared
+	 * to select channel that is waiting for the longest time.
+	 */
 	list_for_each_entry(p, &pl08x->memcpy.channels, vc.chan.device_node)
-		if (p->state == PL08X_CHAN_WAITING) {
+		if (p->state == PL08X_CHAN_WAITING &&
+		    p->waiting_at <= waiting_at) {
 			next = p;
-			break;
+			waiting_at = p->waiting_at;
 		}
 
 	if (!next && pl08x->has_slave) {
 		list_for_each_entry(p, &pl08x->slave.channels, vc.chan.device_node)
-			if (p->state == PL08X_CHAN_WAITING) {
+			if (p->state == PL08X_CHAN_WAITING &&
+			    p->waiting_at <= waiting_at) {
 				next = p;
-				break;
+				waiting_at = p->waiting_at;
 			}
 	}
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Vinod Koul]

On 04-03-19, 16:03, Jean-Nicolas Graux wrote:
> Current way we find a waiting virtual channel for the next transfer
> at the time one physical channel becomes free is not really fair.
> 
> More in details, in case there is more than one channel waiting at a time,
> by just going through the arrays of memcpy and slave channels and stopping
> as soon as state match waiting state, channels with high indexes can be
> penalized.
> 
> Whenever dma engine is substantially overloaded so that we constantly
> get several channels waiting, channels with highest indexes might not
> be served for a substantial time which in the worse case, might hang
> task that wait for dma transfer to complete.
> 
> This patch makes physical channel re-assignment more fair by storing
> time in jiffies when a channel is put in waiting state. Whenever a
> physical channel has to be re-assigned, this time is used to select
> channel that is waiting for the longest time.

Applied, thanks

-- 
~Vinod

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] dmaengine: pl08x: be fair when re-assigning physical channel

[Jean Nicolas GRAUX]

On 3/25/19 6:13 AM, Vinod Koul wrote:
> On 04-03-19, 16:03, Jean-Nicolas Graux wrote:
>> Current way we find a waiting virtual channel for the next transfer
>> at the time one physical channel becomes free is not really fair.
>>
>> More in details, in case there is more than one channel waiting at a time,
>> by just going through the arrays of memcpy and slave channels and stopping
>> as soon as state match waiting state, channels with high indexes can be
>> penalized.
>>
>> Whenever dma engine is substantially overloaded so that we constantly
>> get several channels waiting, channels with highest indexes might not
>> be served for a substantial time which in the worse case, might hang
>> task that wait for dma transfer to complete.
>>
>> This patch makes physical channel re-assignment more fair by storing
>> time in jiffies when a channel is put in waiting state. Whenever a
>> physical channel has to be re-assigned, this time is used to select
>> channel that is waiting for the longest time.
> Applied, thanks
>
Hello Vinod, You are welcome.
Regards. Jean-Nicolas.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel