On Wed, 2020-06-17 at 15:40 -0700, Steve Longerbeam wrote:

Combine the rotate_irq() and norotate_irq() handlers into a single eof_irq() handler.

Signed-off-by: Steve Longerbeam slongerbeam@gmail.com

Reviewed-by: Philipp Zabel p.zabel@pengutronix.de

regards Philipp

---

drivers/gpu/ipu-v3/ipu-image-convert.c | 58 +++++++++----------------- 1 file changed, 20 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c index eeca50d9a1ee..f8b031ded3cf 100644 --- a/drivers/gpu/ipu-v3/ipu-image-convert.c +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c @@ -1709,9 +1709,10 @@ static irqreturn_t do_irq(struct ipu_image_convert_run *run) return IRQ_WAKE_THREAD; }

-static irqreturn_t norotate_irq(int irq, void *data) +static irqreturn_t eof_irq(int irq, void *data) { struct ipu_image_convert_chan *chan = data;

• struct ipu_image_convert_priv *priv = chan->priv; struct ipu_image_convert_ctx *ctx; struct ipu_image_convert_run *run; unsigned long flags;

@@ -1728,45 +1729,26 @@ static irqreturn_t norotate_irq(int irq, void *data)

ctx = run->ctx;

• if (ipu_rot_mode_is_irt(ctx->rot_mode)) {

• /* this is a rotation operation, just ignore */
  

• spin_unlock_irqrestore(&chan->irqlock, flags);
  

• return IRQ_HANDLED;
  

• }
• ret = do_irq(run);

-out:

• spin_unlock_irqrestore(&chan->irqlock, flags);
• return ret;

-}

-static irqreturn_t rotate_irq(int irq, void *data) -{

• struct ipu_image_convert_chan *chan = data;
• struct ipu_image_convert_priv *priv = chan->priv;
• struct ipu_image_convert_ctx *ctx;
• struct ipu_image_convert_run *run;
• unsigned long flags;
• irqreturn_t ret;
• spin_lock_irqsave(&chan->irqlock, flags);
• /* get current run and its context */
• run = chan->current_run;
• if (!run) {

• if (irq == chan->out_eof_irq) {

• if (ipu_rot_mode_is_irt(ctx->rot_mode)) {
  

• 	/* this is a rotation op, just ignore */
  

• 	ret = IRQ_HANDLED;
  

• 	goto out;
  

• }
  

• } else if (irq == chan->rot_out_eof_irq) {

• if (!ipu_rot_mode_is_irt(ctx->rot_mode)) {
  

• 	/* this was NOT a rotation op, shouldn't happen */
  

• 	dev_err(priv->ipu->dev,
  

• 		"Unexpected rotation interrupt\n");
  

• 	ret = IRQ_HANDLED;
  

• 	goto out;
  

• }
  

• } else {

• dev_err(priv->ipu->dev, "Received unknown irq %d\n", irq);
  

  ret = IRQ_NONE; goto out; }

• ctx = run->ctx;
• if (!ipu_rot_mode_is_irt(ctx->rot_mode)) {

• /* this was NOT a rotation operation, shouldn't happen */
  

• dev_err(priv->ipu->dev, "Unexpected rotation interrupt\n");
  

• spin_unlock_irqrestore(&chan->irqlock, flags);
  

• return IRQ_HANDLED;
  

• }
• ret = do_irq(run);

out: spin_unlock_irqrestore(&chan->irqlock, flags); @@ -1859,7 +1841,7 @@ static int get_ipu_resources(struct ipu_image_convert_chan *chan) chan->out_chan, IPU_IRQ_EOF);

• ret = request_threaded_irq(chan->out_eof_irq, norotate_irq, do_bh,

• ret = request_threaded_irq(chan->out_eof_irq, eof_irq, do_bh, 0, "ipu-ic", chan); if (ret < 0) { dev_err(priv->ipu->dev, "could not acquire irq %d\n",

@@ -1872,7 +1854,7 @@ static int get_ipu_resources(struct ipu_image_convert_chan *chan) chan->rotation_out_chan, IPU_IRQ_EOF);

• ret = request_threaded_irq(chan->rot_out_eof_irq, rotate_irq, do_bh,

• ret = request_threaded_irq(chan->rot_out_eof_irq, eof_irq, do_bh, 0, "ipu-ic", chan); if (ret < 0) { dev_err(priv->ipu->dev, "could not acquire irq %d\n",

Use a bit-mask of EOF irqs to determine when all required idmac channel EOFs have been received for a tile conversion, and only do tile completion processing after all EOFs have been received. Otherwise it was found that a conversion would stall after the completion of a tile and the start of the next tile, because the input/read idmac channel had not completed and entered idle state, thus locking up the channel when attempting to re-start it for the next tile.

Fixes: 0537db801bb01 ("gpu: ipu-v3: image-convert: reconfigure IC per tile") Signed-off-by: Steve Longerbeam slongerbeam@gmail.com --- Changes in v2: - need to clear eof_mask at completion of every tile, not just in convert_start(). --- drivers/gpu/ipu-v3/ipu-image-convert.c | 109 +++++++++++++++++++------ 1 file changed, 82 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c index f8b031ded3cf..aa1d4b6d278f 100644 --- a/drivers/gpu/ipu-v3/ipu-image-convert.c +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c @@ -137,6 +137,17 @@ struct ipu_image_convert_ctx; struct ipu_image_convert_chan; struct ipu_image_convert_priv;

+enum eof_irq_mask { + EOF_IRQ_IN = BIT(0), + EOF_IRQ_ROT_IN = BIT(1), + EOF_IRQ_OUT = BIT(2), + EOF_IRQ_ROT_OUT = BIT(3), +}; + +#define EOF_IRQ_COMPLETE (EOF_IRQ_IN | EOF_IRQ_OUT) +#define EOF_IRQ_ROT_COMPLETE (EOF_IRQ_IN | EOF_IRQ_OUT | \ + EOF_IRQ_ROT_IN | EOF_IRQ_ROT_OUT) + struct ipu_image_convert_ctx { struct ipu_image_convert_chan *chan;

@@ -173,6 +184,9 @@ struct ipu_image_convert_ctx { /* where to place converted tile in dest image */ unsigned int out_tile_map[MAX_TILES];

+ /* mask of completed EOF irqs at every tile conversion */ + enum eof_irq_mask eof_mask; + struct list_head list; };

@@ -189,6 +203,8 @@ struct ipu_image_convert_chan { struct ipuv3_channel *rotation_out_chan;

/* the IPU end-of-frame irqs */ + int in_eof_irq; + int rot_in_eof_irq; int out_eof_irq; int rot_out_eof_irq;

@@ -1380,6 +1396,9 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) dev_dbg(priv->ipu->dev, "%s: task %u: starting ctx %p run %p tile %u -> %u\n", __func__, chan->ic_task, ctx, run, tile, dst_tile);

+ /* clear EOF irq mask */ + ctx->eof_mask = 0; + if (ipu_rot_mode_is_irt(ctx->rot_mode)) { /* swap width/height for resizer */ dest_width = d_image->tile[dst_tile].height; @@ -1615,7 +1634,7 @@ static bool ic_settings_changed(struct ipu_image_convert_ctx *ctx) }

/* hold irqlock when calling */ -static irqreturn_t do_irq(struct ipu_image_convert_run *run) +static irqreturn_t do_tile_complete(struct ipu_image_convert_run *run) { struct ipu_image_convert_ctx *ctx = run->ctx; struct ipu_image_convert_chan *chan = ctx->chan; @@ -1700,6 +1719,7 @@ static irqreturn_t do_irq(struct ipu_image_convert_run *run) ctx->cur_buf_num ^= 1; }

+ ctx->eof_mask = 0; /* clear EOF irq mask for next tile */ ctx->next_tile++; return IRQ_HANDLED; done: @@ -1715,8 +1735,9 @@ static irqreturn_t eof_irq(int irq, void *data) struct ipu_image_convert_priv *priv = chan->priv; struct ipu_image_convert_ctx *ctx; struct ipu_image_convert_run *run; + irqreturn_t ret = IRQ_HANDLED; + bool tile_complete = false; unsigned long flags; - irqreturn_t ret;

spin_lock_irqsave(&chan->irqlock, flags);

@@ -1729,27 +1750,33 @@ static irqreturn_t eof_irq(int irq, void *data)

ctx = run->ctx;

- if (irq == chan->out_eof_irq) { - if (ipu_rot_mode_is_irt(ctx->rot_mode)) { - /* this is a rotation op, just ignore */ - ret = IRQ_HANDLED; - goto out; - } - } else if (irq == chan->rot_out_eof_irq) { + if (irq == chan->in_eof_irq) { + ctx->eof_mask |= EOF_IRQ_IN; + } else if (irq == chan->out_eof_irq) { + ctx->eof_mask |= EOF_IRQ_OUT; + } else if (irq == chan->rot_in_eof_irq || + irq == chan->rot_out_eof_irq) { if (!ipu_rot_mode_is_irt(ctx->rot_mode)) { /* this was NOT a rotation op, shouldn't happen */ dev_err(priv->ipu->dev, "Unexpected rotation interrupt\n"); - ret = IRQ_HANDLED; goto out; } + ctx->eof_mask |= (irq == chan->rot_in_eof_irq) ? + EOF_IRQ_ROT_IN : EOF_IRQ_ROT_OUT; } else { dev_err(priv->ipu->dev, "Received unknown irq %d\n", irq); ret = IRQ_NONE; goto out; }

- ret = do_irq(run); + if (ipu_rot_mode_is_irt(ctx->rot_mode)) + tile_complete = (ctx->eof_mask == EOF_IRQ_ROT_COMPLETE); + else + tile_complete = (ctx->eof_mask == EOF_IRQ_COMPLETE); + + if (tile_complete) + ret = do_tile_complete(run); out: spin_unlock_irqrestore(&chan->irqlock, flags); return ret; @@ -1783,6 +1810,10 @@ static void force_abort(struct ipu_image_convert_ctx *ctx)

static void release_ipu_resources(struct ipu_image_convert_chan *chan) { + if (chan->in_eof_irq >= 0) + free_irq(chan->in_eof_irq, chan); + if (chan->rot_in_eof_irq >= 0) + free_irq(chan->rot_in_eof_irq, chan); if (chan->out_eof_irq >= 0) free_irq(chan->out_eof_irq, chan); if (chan->rot_out_eof_irq >= 0) @@ -1801,7 +1832,27 @@ static void release_ipu_resources(struct ipu_image_convert_chan *chan)

chan->in_chan = chan->out_chan = chan->rotation_in_chan = chan->rotation_out_chan = NULL; - chan->out_eof_irq = chan->rot_out_eof_irq = -1; + chan->in_eof_irq = -1; + chan->rot_in_eof_irq = -1; + chan->out_eof_irq = -1; + chan->rot_out_eof_irq = -1; +} + +static int get_eof_irq(struct ipu_image_convert_chan *chan, + struct ipuv3_channel *channel) +{ + struct ipu_image_convert_priv *priv = chan->priv; + int ret, irq; + + irq = ipu_idmac_channel_irq(priv->ipu, channel, IPU_IRQ_EOF); + + ret = request_threaded_irq(irq, eof_irq, do_bh, 0, "ipu-ic", chan); + if (ret < 0) { + dev_err(priv->ipu->dev, "could not acquire irq %d\n", irq); + return ret; + } + + return irq; }

static int get_ipu_resources(struct ipu_image_convert_chan *chan) @@ -1837,31 +1888,33 @@ static int get_ipu_resources(struct ipu_image_convert_chan *chan) }

/* acquire the EOF interrupts */ - chan->out_eof_irq = ipu_idmac_channel_irq(priv->ipu, - chan->out_chan, - IPU_IRQ_EOF); + ret = get_eof_irq(chan, chan->in_chan); + if (ret < 0) { + chan->in_eof_irq = -1; + goto err; + } + chan->in_eof_irq = ret;

- ret = request_threaded_irq(chan->out_eof_irq, eof_irq, do_bh, - 0, "ipu-ic", chan); + ret = get_eof_irq(chan, chan->rotation_in_chan); if (ret < 0) { - dev_err(priv->ipu->dev, "could not acquire irq %d\n", - chan->out_eof_irq); - chan->out_eof_irq = -1; + chan->rot_in_eof_irq = -1; goto err; } + chan->rot_in_eof_irq = ret;

- chan->rot_out_eof_irq = ipu_idmac_channel_irq(priv->ipu, - chan->rotation_out_chan, - IPU_IRQ_EOF); + ret = get_eof_irq(chan, chan->out_chan); + if (ret < 0) { + chan->out_eof_irq = -1; + goto err; + } + chan->out_eof_irq = ret;

- ret = request_threaded_irq(chan->rot_out_eof_irq, eof_irq, do_bh, - 0, "ipu-ic", chan); + ret = get_eof_irq(chan, chan->rotation_out_chan); if (ret < 0) { - dev_err(priv->ipu->dev, "could not acquire irq %d\n", - chan->rot_out_eof_irq); chan->rot_out_eof_irq = -1; goto err; } + chan->rot_out_eof_irq = ret;

return 0; err: @@ -2440,6 +2493,8 @@ int ipu_image_convert_init(struct ipu_soc *ipu, struct device *dev) chan->ic_task = i; chan->priv = priv; chan->dma_ch = &image_convert_dma_chan[i]; + chan->in_eof_irq = -1; + chan->rot_in_eof_irq = -1; chan->out_eof_irq = -1; chan->rot_out_eof_irq = -1;

Hi Philipp,

On 6/26/20 2:38 AM, Philipp Zabel wrote:

Hi Steve,

On Thu, 2020-06-25 at 11:13 -0700, Steve Longerbeam wrote:

...

Use a bit-mask of EOF irqs to determine when all required idmac channel EOFs have been received for a tile conversion, and only do tile completion processing after all EOFs have been received. Otherwise it was found that a conversion would stall after the completion of a tile and the start of the next tile, because the input/read idmac channel had not completed and entered idle state, thus locking up the channel when attempting to re-start it for the next tile.

Do I understand correctly that there are cases where the output channel EOF IRQ has triggered and the next tile processing is kicked off before the input channel EOF IRQ triggers even without rotation?

Yes.

What is the cause of this? It would seem that the read channel EOF should occur before the write channel EOF, but there are cases seen where the opposite occurs. Maybe this has to do with idmac channel priorities, the IC PP read/write channels are set to the same priority (low), in which case the IPU should resort to round-robin when handling requests on those channels. Maybe the EOF irq is not signalled until after the IPU has updated CPMEM with status info after the transfers complete, and round-robin selects the write channel before the read channel for the CPMEM updates?

Do you have any way to reproduce this?

Yes, try a scaling only conversion, 1920x1080.422p -> 1024x768.422p. No rotation needed.

Steve

regards Philipp

...

Fixes: 0537db801bb01 ("gpu: ipu-v3: image-convert: reconfigure IC per tile") Signed-off-by: Steve Longerbeam slongerbeam@gmail.com

---

Changes in v2:

• need to clear eof_mask at completion of every tile, not just in convert_start().

---

drivers/gpu/ipu-v3/ipu-image-convert.c | 109 +++++++++++++++++++------ 1 file changed, 82 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c index f8b031ded3cf..aa1d4b6d278f 100644 --- a/drivers/gpu/ipu-v3/ipu-image-convert.c +++ b/drivers/gpu/ipu-v3/ipu-image-convert.c @@ -137,6 +137,17 @@ struct ipu_image_convert_ctx; struct ipu_image_convert_chan; struct ipu_image_convert_priv;

+enum eof_irq_mask {

• EOF_IRQ_IN = BIT(0),
• EOF_IRQ_ROT_IN = BIT(1),
• EOF_IRQ_OUT = BIT(2),
• EOF_IRQ_ROT_OUT = BIT(3),

+};

+#define EOF_IRQ_COMPLETE (EOF_IRQ_IN | EOF_IRQ_OUT) +#define EOF_IRQ_ROT_COMPLETE (EOF_IRQ_IN | EOF_IRQ_OUT | \

• 	      EOF_IRQ_ROT_IN | EOF_IRQ_ROT_OUT)
  

• struct ipu_image_convert_ctx { struct ipu_image_convert_chan *chan;

@@ -173,6 +184,9 @@ struct ipu_image_convert_ctx { /* where to place converted tile in dest image */ unsigned int out_tile_map[MAX_TILES];

• /* mask of completed EOF irqs at every tile conversion */
• enum eof_irq_mask eof_mask;
• struct list_head list; };

@@ -189,6 +203,8 @@ struct ipu_image_convert_chan { struct ipuv3_channel *rotation_out_chan;

/* the IPU end-of-frame irqs */

• int in_eof_irq;
• int rot_in_eof_irq; int out_eof_irq; int rot_out_eof_irq;

@@ -1380,6 +1396,9 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile) dev_dbg(priv->ipu->dev, "%s: task %u: starting ctx %p run %p tile %u -> %u\n", __func__, chan->ic_task, ctx, run, tile, dst_tile);

• /* clear EOF irq mask */
• ctx->eof_mask = 0;
• if (ipu_rot_mode_is_irt(ctx->rot_mode)) { /* swap width/height for resizer */ dest_width = d_image->tile[dst_tile].height;

@@ -1615,7 +1634,7 @@ static bool ic_settings_changed(struct ipu_image_convert_ctx *ctx) }

/* hold irqlock when calling */ -static irqreturn_t do_irq(struct ipu_image_convert_run *run) +static irqreturn_t do_tile_complete(struct ipu_image_convert_run *run) { struct ipu_image_convert_ctx *ctx = run->ctx; struct ipu_image_convert_chan *chan = ctx->chan; @@ -1700,6 +1719,7 @@ static irqreturn_t do_irq(struct ipu_image_convert_run *run) ctx->cur_buf_num ^= 1; }

• ctx->eof_mask = 0; /* clear EOF irq mask for next tile */ ctx->next_tile++; return IRQ_HANDLED; done:

@@ -1715,8 +1735,9 @@ static irqreturn_t eof_irq(int irq, void *data) struct ipu_image_convert_priv *priv = chan->priv; struct ipu_image_convert_ctx *ctx; struct ipu_image_convert_run *run;

• irqreturn_t ret = IRQ_HANDLED;
• bool tile_complete = false; unsigned long flags;

• irqreturn_t ret;

  spin_lock_irqsave(&chan->irqlock, flags);

@@ -1729,27 +1750,33 @@ static irqreturn_t eof_irq(int irq, void *data)

ctx = run->ctx;

• if (irq == chan->out_eof_irq) {

• if (ipu_rot_mode_is_irt(ctx->rot_mode)) {
  

• 	/* this is a rotation op, just ignore */
  

• 	ret = IRQ_HANDLED;
  

• 	goto out;
  

• }
  

• } else if (irq == chan->rot_out_eof_irq) {

• if (irq == chan->in_eof_irq) {

• ctx->eof_mask |= EOF_IRQ_IN;
  

• } else if (irq == chan->out_eof_irq) {

• ctx->eof_mask |= EOF_IRQ_OUT;
  

• } else if (irq == chan->rot_in_eof_irq ||

•    irq == chan->rot_out_eof_irq) {
  

  if (!ipu_rot_mode_is_irt(ctx->rot_mode)) { /* this was NOT a rotation op, shouldn't happen */ dev_err(priv->ipu->dev, "Unexpected rotation interrupt\n");

• 	ret = IRQ_HANDLED;
  goto out;
  

  }

• ctx->eof_mask |= (irq == chan->rot_in_eof_irq) ?
  

• 	EOF_IRQ_ROT_IN : EOF_IRQ_ROT_OUT;
  

  } else { dev_err(priv->ipu->dev, "Received unknown irq %d\n", irq); ret = IRQ_NONE; goto out; }

• ret = do_irq(run);

• if (ipu_rot_mode_is_irt(ctx->rot_mode))

• tile_complete =	(ctx->eof_mask == EOF_IRQ_ROT_COMPLETE);
  

• else

• tile_complete = (ctx->eof_mask == EOF_IRQ_COMPLETE);
  

• if (tile_complete)

• ret = do_tile_complete(run);
  

  out: spin_unlock_irqrestore(&chan->irqlock, flags); return ret;

@@ -1783,6 +1810,10 @@ static void force_abort(struct ipu_image_convert_ctx *ctx)

static void release_ipu_resources(struct ipu_image_convert_chan *chan) {

• if (chan->in_eof_irq >= 0)

• free_irq(chan->in_eof_irq, chan);
  

• if (chan->rot_in_eof_irq >= 0)

• free_irq(chan->rot_in_eof_irq, chan);
  

  if (chan->out_eof_irq >= 0) free_irq(chan->out_eof_irq, chan); if (chan->rot_out_eof_irq >= 0)

@@ -1801,7 +1832,27 @@ static void release_ipu_resources(struct ipu_image_convert_chan *chan)

chan->in_chan = chan->out_chan = chan->rotation_in_chan = chan->rotation_out_chan = NULL;

• chan->out_eof_irq = chan->rot_out_eof_irq = -1;

• chan->in_eof_irq = -1;
• chan->rot_in_eof_irq = -1;
• chan->out_eof_irq = -1;
• chan->rot_out_eof_irq = -1;

+}

+static int get_eof_irq(struct ipu_image_convert_chan *chan,

•        struct ipuv3_channel *channel)
  

+{

• struct ipu_image_convert_priv *priv = chan->priv;

• int ret, irq;

• irq = ipu_idmac_channel_irq(priv->ipu, channel, IPU_IRQ_EOF);

• ret = request_threaded_irq(irq, eof_irq, do_bh, 0, "ipu-ic", chan);

• if (ret < 0) {

• dev_err(priv->ipu->dev, "could not acquire irq %d\n", irq);
  

• return ret;
  

• }

• return irq; }

  static int get_ipu_resources(struct ipu_image_convert_chan *chan)

@@ -1837,31 +1888,33 @@ static int get_ipu_resources(struct ipu_image_convert_chan *chan) }

/* acquire the EOF interrupts */

• chan->out_eof_irq = ipu_idmac_channel_irq(priv->ipu,

• 				  chan->out_chan,
  

• 				  IPU_IRQ_EOF);
  

• ret = get_eof_irq(chan, chan->in_chan);
• if (ret < 0) {

• chan->in_eof_irq = -1;
  

• goto err;
  

• }
• chan->in_eof_irq = ret;

• ret = request_threaded_irq(chan->out_eof_irq, eof_irq, do_bh,

• 		   0, "ipu-ic", chan);
  

• ret = get_eof_irq(chan, chan->rotation_in_chan); if (ret < 0) {

• dev_err(priv->ipu->dev, "could not acquire irq %d\n",
  

• 	 chan->out_eof_irq);
  

• chan->out_eof_irq = -1;
  

• chan->rot_in_eof_irq = -1;
  

  goto err; }
• chan->rot_in_eof_irq = ret;

• chan->rot_out_eof_irq = ipu_idmac_channel_irq(priv->ipu,

• 				     chan->rotation_out_chan,
  

• 				     IPU_IRQ_EOF);
  

• ret = get_eof_irq(chan, chan->out_chan);
• if (ret < 0) {

• chan->out_eof_irq = -1;
  

• goto err;
  

• }
• chan->out_eof_irq = ret;

• ret = request_threaded_irq(chan->rot_out_eof_irq, eof_irq, do_bh,

• 		   0, "ipu-ic", chan);
  

• ret = get_eof_irq(chan, chan->rotation_out_chan); if (ret < 0) {

• dev_err(priv->ipu->dev, "could not acquire irq %d\n",
  

• 	chan->rot_out_eof_irq);
  

  chan->rot_out_eof_irq = -1; goto err; }

• chan->rot_out_eof_irq = ret;

  return 0; err:

@@ -2440,6 +2493,8 @@ int ipu_image_convert_init(struct ipu_soc *ipu, struct device *dev) chan->ic_task = i; chan->priv = priv; chan->dma_ch = &image_convert_dma_chan[i];

• chan->in_eof_irq = -1;
  

• chan->rot_in_eof_irq = -1;
  

  chan->out_eof_irq = -1; chan->rot_out_eof_irq = -1;