Hi Niklas,
On Fri, Oct 16, 2020 at 01:14:07AM +0200, Niklas Söderlund wrote:
> To support suspend and resume the ability to start and stop the hardware
> needs to be available internally in the driver. Currently this code is
> in the start and stop callbacks of the vb2_ops struct. In these
> callbacks the code is intertwined with buffer allocation and freeing.
>
> Prepare for suspend and resume support by braking out the hardware
breaking
> start/stop code into new methods.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
I very much like this, it really simplifies the code
Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
Thanks
j
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 78 +++++++++++++---------
> drivers/media/platform/rcar-vin/rcar-vin.h | 3 +
> 2 files changed, 48 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f65deac4c2dbed54..5a5f0e5007478c8d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1050,16 +1050,20 @@ static irqreturn_t rvin_irq(int irq, void *data)
> return IRQ_RETVAL(handled);
> }
>
> -/* Need to hold qlock before calling */
> static void return_unused_buffers(struct rvin_dev *vin,
> enum vb2_buffer_state state)
> {
> struct rvin_buffer *buf, *node;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&vin->qlock, flags);
>
> list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
> vb2_buffer_done(&buf->vb.vb2_buf, state);
> list_del(&buf->list);
> }
> +
> + spin_unlock_irqrestore(&vin->qlock, flags);
> }
>
> static int rvin_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> @@ -1243,53 +1247,55 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> return ret;
> }
>
> -static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> +int rvin_start_streaming(struct rvin_dev *vin)
> {
> - struct rvin_dev *vin = vb2_get_drv_priv(vq);
> unsigned long flags;
> int ret;
>
> - /* Allocate scratch buffer. */
> - vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
> - &vin->scratch_phys, GFP_KERNEL);
> - if (!vin->scratch) {
> - spin_lock_irqsave(&vin->qlock, flags);
> - return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> - spin_unlock_irqrestore(&vin->qlock, flags);
> - vin_err(vin, "Failed to allocate scratch buffer\n");
> - return -ENOMEM;
> - }
> -
> ret = rvin_set_stream(vin, 1);
> - if (ret) {
> - spin_lock_irqsave(&vin->qlock, flags);
> - return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> - spin_unlock_irqrestore(&vin->qlock, flags);
> - goto out;
> - }
> + if (ret)
> + return ret;
>
> spin_lock_irqsave(&vin->qlock, flags);
>
> vin->sequence = 0;
>
> ret = rvin_capture_start(vin);
> - if (ret) {
> - return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> + if (ret)
> rvin_set_stream(vin, 0);
> - }
>
> spin_unlock_irqrestore(&vin->qlock, flags);
> -out:
> - if (ret)
> - dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> - vin->scratch_phys);
>
> return ret;
> }
>
> -static void rvin_stop_streaming(struct vb2_queue *vq)
> +static int rvin_start_streaming_vq(struct vb2_queue *vq, unsigned int count)
> {
> struct rvin_dev *vin = vb2_get_drv_priv(vq);
> + int ret = -ENOMEM;
> +
> + /* Allocate scratch buffer. */
> + vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
> + &vin->scratch_phys, GFP_KERNEL);
> + if (!vin->scratch)
> + goto err_scratch;
> +
> + ret = rvin_start_streaming(vin);
> + if (ret)
> + goto err_start;
> +
> + return 0;
> +err_start:
> + dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> + vin->scratch_phys);
> +err_scratch:
> + return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> +
> + return ret;
> +}
> +
> +void rvin_stop_streaming(struct rvin_dev *vin)
> +{
> unsigned int i, retries;
> unsigned long flags;
> bool buffersFreed;
> @@ -1341,27 +1347,33 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
> vin->state = STOPPED;
> }
>
> - /* Return all unused buffers. */
> - return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
> -
> spin_unlock_irqrestore(&vin->qlock, flags);
>
> rvin_set_stream(vin, 0);
>
> /* disable interrupts */
> rvin_disable_interrupts(vin);
> +}
> +
> +static void rvin_stop_streaming_vq(struct vb2_queue *vq)
> +{
> + struct rvin_dev *vin = vb2_get_drv_priv(vq);
> +
> + rvin_stop_streaming(vin);
>
> /* Free scratch buffer. */
> dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> vin->scratch_phys);
> +
> + return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
> }
>
> static const struct vb2_ops rvin_qops = {
> .queue_setup = rvin_queue_setup,
> .buf_prepare = rvin_buffer_prepare,
> .buf_queue = rvin_buffer_queue,
> - .start_streaming = rvin_start_streaming,
> - .stop_streaming = rvin_stop_streaming,
> + .start_streaming = rvin_start_streaming_vq,
> + .stop_streaming = rvin_stop_streaming_vq,
> .wait_prepare = vb2_ops_wait_prepare,
> .wait_finish = vb2_ops_wait_finish,
> };
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 2fef23470e3ddfe3..4ec8584709c847a9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -299,4 +299,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin);
> int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
>
> +int rvin_start_streaming(struct rvin_dev *vin);
> +void rvin_stop_streaming(struct rvin_dev *vin);
> +
> #endif
> --
> 2.28.0
>
