| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1701259.X513TT2pbd@jernej-laptop>
Date: Thu, 28 Apr 2022 10:11:18 +0200
From: Jernej Škrabec <jernej.skrabec@...il.com>
To: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
Yong Deng <yong.deng@...ewell.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Chen-Yu Tsai <wens@...e.org>,
Samuel Holland <samuel@...lland.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v4 13/45] media: sun6i-csi: Introduce and use video helper functions
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@...tlin.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@...il.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");
> > >
> > > }
Powered by blists - more mailing lists