Re: [PATCH v4 13/45] media: sun6i-csi: Introduce and use video helper functions

[Paul Kocialkowski]

Hi Jernej,

On Wed 27 Apr 22, 20:50, Jernej Škrabec wrote:
> Dne petek, 15. april 2022 ob 17:27:39 CEST je Paul Kocialkowski napisal(a):
> > Introduce some helpers for buffer and general video configuration.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> > ---
> >  .../platform/sunxi/sun6i-csi/sun6i_video.c    | 46 +++++++++++--------
> >  1 file changed, 28 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c index
> > e6c85fcc65bb..e47eeb27dc4e 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> > @@ -92,6 +92,29 @@ static bool sun6i_video_format_check(u32 format)
> >  	return false;
> >  }
> > 
> > +/* Video */
> > +
> > +static void sun6i_video_buffer_configure(struct sun6i_csi_device *csi_dev,
> > +					 struct sun6i_csi_buffer 
> *csi_buffer)
> > +{
> > +	csi_buffer->queued_to_csi = true;
> > +	sun6i_csi_update_buf_addr(csi_dev, csi_buffer->dma_addr);
> > +}
> > +
> > +static void sun6i_video_configure(struct sun6i_csi_device *csi_dev)
> > +{
> > +	struct sun6i_video *video = &csi_dev->video;
> > +	struct sun6i_csi_config config = { 0 };
> > +
> > +	config.pixelformat = video->format.fmt.pix.pixelformat;
> > +	config.code = video->mbus_code;
> > +	config.field = video->format.fmt.pix.field;
> > +	config.width = video->format.fmt.pix.width;
> > +	config.height = video->format.fmt.pix.height;
> > +
> > +	sun6i_csi_update_config(csi_dev, &config);
> > +}
> > +
> >  /* Queue */
> > 
> >  static int sun6i_video_queue_setup(struct vb2_queue *queue,
> > @@ -160,7 +183,6 @@ static int sun6i_video_start_streaming(struct vb2_queue
> > *queue, struct video_device *video_dev = &video->video_dev;
> >  	struct sun6i_csi_buffer *buf;
> >  	struct sun6i_csi_buffer *next_buf;
> > -	struct sun6i_csi_config config;
> >  	struct v4l2_subdev *subdev;
> >  	unsigned long flags;
> >  	int ret;
> > @@ -182,22 +204,13 @@ static int sun6i_video_start_streaming(struct
> > vb2_queue *queue, goto error_media_pipeline;
> >  	}
> > 
> > -	config.pixelformat = video->format.fmt.pix.pixelformat;
> > -	config.code = video->mbus_code;
> > -	config.field = video->format.fmt.pix.field;
> > -	config.width = video->format.fmt.pix.width;
> > -	config.height = video->format.fmt.pix.height;
> > -
> > -	ret = sun6i_csi_update_config(csi_dev, &config);
> > -	if (ret < 0)
> > -		goto error_media_pipeline;
> > +	sun6i_video_configure(csi_dev);
> 
> What happened to that error handling? New helper function ignores return value 
> of sun6i_csi_update_config(). Why?

Ah that's a good point, the error value is still being returned by
sun6i_csi_update_config so it should be kept around at this stage.

Note that this is a transitional commit and sun6i_video_configure
(which gets renamed to sun6i_csi_capture_configure) is eventually
reworked to only configure registers (no checks) and returns void.

If you think it's important to keep it in the meantime I can do that.

Paul

> Best regards,
> Jernej
> 
> > 
> >  	spin_lock_irqsave(&video->dma_queue_lock, flags);
> > 
> >  	buf = list_first_entry(&video->dma_queue,
> >  			       struct sun6i_csi_buffer, list);
> > -	buf->queued_to_csi = true;
> > -	sun6i_csi_update_buf_addr(csi_dev, buf->dma_addr);
> > +	sun6i_video_buffer_configure(csi_dev, buf);
> > 
> >  	sun6i_csi_set_stream(csi_dev, true);
> > 
> > @@ -219,8 +232,7 @@ static int sun6i_video_start_streaming(struct vb2_queue
> > *queue, * would also drop frame when lacking of queued buffer.
> >  	 */
> >  	next_buf = list_next_entry(buf, list);
> > -	next_buf->queued_to_csi = true;
> > -	sun6i_csi_update_buf_addr(csi_dev, next_buf->dma_addr);
> > +	sun6i_video_buffer_configure(csi_dev, next_buf);
> > 
> >  	spin_unlock_irqrestore(&video->dma_queue_lock, flags);
> > 
> > @@ -294,8 +306,7 @@ void sun6i_video_frame_done(struct sun6i_csi_device
> > *csi_dev) * for next ISR call.
> >  	 */
> >  	if (!next_buf->queued_to_csi) {
> > -		next_buf->queued_to_csi = true;
> > -		sun6i_csi_update_buf_addr(csi_dev, next_buf->dma_addr);
> > +		sun6i_video_buffer_configure(csi_dev, next_buf);
> >  		dev_dbg(csi_dev->dev, "Frame dropped!\n");
> >  		goto complete;
> >  	}
> > @@ -309,8 +320,7 @@ void sun6i_video_frame_done(struct sun6i_csi_device
> > *csi_dev) /* Prepare buffer for next frame but one.  */
> >  	if (!list_is_last(&next_buf->list, &video->dma_queue)) {
> >  		next_buf = list_next_entry(next_buf, list);
> > -		next_buf->queued_to_csi = true;
> > -		sun6i_csi_update_buf_addr(csi_dev, next_buf->dma_addr);
> > +		sun6i_video_buffer_configure(csi_dev, next_buf);
> >  	} else {
> >  		dev_dbg(csi_dev->dev, "Next frame will be dropped!\n");
> >  	}
> 
> 
> 
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature