Re: [PATCH v2 4/5] media: rcar-vin: Simplify the shutdown process

[Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>]

Hi,

On 22/01/2025 18:53, Niklas Söderlund wrote:
> When shutting down capture extra care was needed to try and complete a
> buffer that was involved in the emulated support for SEQ_{TB,BT}. This
> was needed as a buffer might be queued once to the driver, but two times
> to the hardware using different offsets.
>
> As support for SEQ_{TB,BT} is now removed this shutdown process can be
> greatly simplified. And in addition the state keeping of the VIN device
> can be reduced to a single boolean value instead of keeping track of
> this SEQ_{TB,BT} stopping dance.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
>   .../platform/renesas/rcar-vin/rcar-core.c     |  4 +-
>   .../platform/renesas/rcar-vin/rcar-dma.c      | 75 ++++++-------------
>   .../platform/renesas/rcar-vin/rcar-vin.h      | 18 +----
>   3 files changed, 26 insertions(+), 71 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>

 Tomi

> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index b8e35ef4d9d8..cfbc9ec27706 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -1080,7 +1080,7 @@ static int __maybe_unused rvin_suspend(struct device *dev)
>   {
>   	struct rvin_dev *vin = dev_get_drvdata(dev);
>   
> -	if (vin->state != RUNNING)
> +	if (!vin->running)
>   		return 0;
>   
>   	rvin_stop_streaming(vin);
> @@ -1092,7 +1092,7 @@ static int __maybe_unused rvin_resume(struct device *dev)
>   {
>   	struct rvin_dev *vin = dev_get_drvdata(dev);
>   
> -	if (vin->state != RUNNING)
> +	if (!vin->running)
>   		return 0;
>   
>   	/*
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index ba55ccf648de..3eb6b5fcac89 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -1022,8 +1022,7 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>   	if (WARN_ON(vin->buf_hw[slot].buffer))
>   		return;
>   
> -	if ((vin->state != STOPPED && vin->state != RUNNING) ||
> -	    list_empty(&vin->buf_list)) {
> +	if (list_empty(&vin->buf_list)) {
>   		vin->buf_hw[slot].buffer = NULL;
>   		phys_addr = vin->scratch_phys;
>   	} else {
> @@ -1064,8 +1063,6 @@ static int rvin_capture_start(struct rvin_dev *vin)
>   	/* Continuous Frame Capture Mode */
>   	rvin_write(vin, VNFC_C_FRAME, VNFC_REG);
>   
> -	vin->state = RUNNING;
> -
>   	return 0;
>   }
>   
> @@ -1106,9 +1103,9 @@ static irqreturn_t rvin_irq(int irq, void *data)
>   	if (!(int_status & VNINTS_FIS))
>   		goto done;
>   
> -	/* Nothing to do if capture status is 'STOPPED' */
> -	if (vin->state == STOPPED) {
> -		vin_dbg(vin, "IRQ while state stopped\n");
> +	/* Nothing to do if not running. */
> +	if (!vin->running) {
> +		vin_dbg(vin, "IRQ while not running, ignoring\n");
>   		goto done;
>   	}
>   
> @@ -1389,6 +1386,8 @@ int rvin_start_streaming(struct rvin_dev *vin)
>   	if (ret)
>   		rvin_set_stream(vin, 0);
>   
> +	vin->running = true;
> +
>   	spin_unlock_irqrestore(&vin->qlock, flags);
>   
>   	return ret;
> @@ -1421,44 +1420,21 @@ static int rvin_start_streaming_vq(struct vb2_queue *vq, unsigned int count)
>   
>   void rvin_stop_streaming(struct rvin_dev *vin)
>   {
> -	unsigned int i, retries;
>   	unsigned long flags;
> -	bool buffersFreed;
>   
>   	spin_lock_irqsave(&vin->qlock, flags);
>   
> -	if (vin->state == STOPPED) {
> +	if (!vin->running) {
>   		spin_unlock_irqrestore(&vin->qlock, flags);
>   		return;
>   	}
>   
> -	vin->state = STOPPING;
> -
> -	/* Wait until only scratch buffer is used, max 3 interrupts. */
> -	retries = 0;
> -	while (retries++ < RVIN_RETRIES) {
> -		buffersFreed = true;
> -		for (i = 0; i < HW_BUFFER_NUM; i++)
> -			if (vin->buf_hw[i].buffer)
> -				buffersFreed = false;
> -
> -		if (buffersFreed)
> -			break;
> -
> -		spin_unlock_irqrestore(&vin->qlock, flags);
> -		msleep(RVIN_TIMEOUT_MS);
> -		spin_lock_irqsave(&vin->qlock, flags);
> -	}
> -
>   	/* Wait for streaming to stop */
> -	retries = 0;
> -	while (retries++ < RVIN_RETRIES) {
> -
> +	for (unsigned int i = 0; i < RVIN_RETRIES; i++) {
>   		rvin_capture_stop(vin);
>   
>   		/* Check if HW is stopped */
>   		if (!rvin_capture_active(vin)) {
> -			vin->state = STOPPED;
>   			break;
>   		}
>   
> @@ -1467,32 +1443,25 @@ void rvin_stop_streaming(struct rvin_dev *vin)
>   		spin_lock_irqsave(&vin->qlock, flags);
>   	}
>   
> -	if (!buffersFreed || vin->state != STOPPED) {
> -		/*
> -		 * If this happens something have gone horribly wrong.
> -		 * Set state to stopped to prevent the interrupt handler
> -		 * to make things worse...
> -		 */
> -		vin_err(vin, "Failed stop HW, something is seriously broken\n");
> -		vin->state = STOPPED;
> -	}
> +	if (rvin_capture_active(vin))
> +		vin_err(vin, "Hardware did not stop\n");
> +
> +	vin->running = false;
>   
>   	spin_unlock_irqrestore(&vin->qlock, flags);
>   
> -	/* If something went wrong, free buffers with an error. */
> -	if (!buffersFreed) {
> -		return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
> -		for (i = 0; i < HW_BUFFER_NUM; i++) {
> -			if (vin->buf_hw[i].buffer)
> -				vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
> -						VB2_BUF_STATE_ERROR);
> -		}
> -	}
> -
>   	rvin_set_stream(vin, 0);
>   
>   	/* disable interrupts */
>   	rvin_disable_interrupts(vin);
> +
> +	/* Return unprocessed buffers from hardware. */
> +	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {
> +		if (vin->buf_hw[i].buffer)
> +			vb2_buffer_done(&vin->buf_hw[i].buffer->vb2_buf,
> +					VB2_BUF_STATE_ERROR);
> +	}
> +
>   }
>   
>   static void rvin_stop_streaming_vq(struct vb2_queue *vq)
> @@ -1538,8 +1507,6 @@ int rvin_dma_register(struct rvin_dev *vin, int irq)
>   
>   	spin_lock_init(&vin->qlock);
>   
> -	vin->state = STOPPED;
> -
>   	for (i = 0; i < HW_BUFFER_NUM; i++)
>   		vin->buf_hw[i].buffer = NULL;
>   
> @@ -1642,7 +1609,7 @@ void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha)
>   
>   	vin->alpha = alpha;
>   
> -	if (vin->state == STOPPED)
> +	if (!vin->running)
>   		goto out;
>   
>   	switch (vin->format.pixelformat) {
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> index f13ef379d095..934474d2334a 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-vin.h
> @@ -61,18 +61,6 @@ enum rvin_isp_id {
>   	(((unsigned int)RVIN_CSI_MAX) > ((unsigned int)RVIN_ISP_MAX) ? \
>   	 (unsigned int)RVIN_CSI_MAX : (unsigned int)RVIN_ISP_MAX)
>   
> -/**
> - * enum rvin_dma_state - DMA states
> - * @STOPPED:   No operation in progress
> - * @RUNNING:   Operation in progress have buffers
> - * @STOPPING:  Stopping operation
> - */
> -enum rvin_dma_state {
> -	STOPPED = 0,
> -	RUNNING,
> -	STOPPING,
> -};
> -
>   /**
>    * struct rvin_video_format - Data format stored in memory
>    * @fourcc:	Pixelformat
> @@ -173,11 +161,11 @@ struct rvin_info {
>    * @scratch:		cpu address for scratch buffer
>    * @scratch_phys:	physical address of the scratch buffer
>    *
> - * @qlock:		protects @buf_hw, @buf_list, @sequence and @state
> + * @qlock:		Protects @buf_hw, @buf_list, @sequence and @running
>    * @buf_hw:		Keeps track of buffers given to HW slot
>    * @buf_list:		list of queued buffers
>    * @sequence:		V4L2 buffers sequence number
> - * @state:		keeps track of operation state
> + * @running:		Keeps track of if the VIN is running
>    *
>    * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
>    * @chsel:		Cached value of the current CSI-2 channel selection
> @@ -220,7 +208,7 @@ struct rvin_dev {
>   	} buf_hw[HW_BUFFER_NUM];
>   	struct list_head buf_list;
>   	unsigned int sequence;
> -	enum rvin_dma_state state;
> +	bool running;
>   
>   	bool is_csi;
>   	unsigned int chsel;