| Message ID | 20220415152811.636419-14-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 and A31 ISP / CSI Rework | expand |
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@bootlin.com> > --- > .../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? 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"); > }
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@bootlin.com> > > --- > > .../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"); > > } > > > >
Dne četrtek, 28. april 2022 ob 10:04:37 CEST je Paul Kocialkowski napisal(a): > 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@bootlin.com> > > > --- > > > > > > .../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. If it's only transitional, then it's fine. Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej > > 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"); > > > > > > }
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); 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"); }
Introduce some helpers for buffer and general video configuration. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- .../platform/sunxi/sun6i-csi/sun6i_video.c | 46 +++++++++++-------- 1 file changed, 28 insertions(+), 18 deletions(-)