Re: [PATCH v4 24/34] omap3isp: Assume media_entity_pipeline_start may fail

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On Mon, Mar 05, 2012 at 12:26:44PM +0100, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Friday 02 March 2012 19:30:32 Sakari Ailus wrote:
> > Since media_entity_pipeline_start() now does link validation, it may
> > actually fail. Perform the error handling.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
> >
> > ---
> >  drivers/media/video/omap3isp/ispvideo.c |   53
> > +++++++++++++++++-------------- 1 files changed, 29 insertions(+), 24
> > deletions(-)
> > 
> > diff --git a/drivers/media/video/omap3isp/ispvideo.c
> > b/drivers/media/video/omap3isp/ispvideo.c index b0d541b..f2621bc 100644
> > --- a/drivers/media/video/omap3isp/ispvideo.c
> > +++ b/drivers/media/video/omap3isp/ispvideo.c
> > @@ -997,14 +997,16 @@ isp_video_streamon(struct file *file, void *fh, enum
> > v4l2_buf_type type) pipe->l3_ick =
> > clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
> >  	pipe->max_rate = pipe->l3_ick;
> > 
> > -	media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
> > +	ret = media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
> > +	if (ret < 0)
> > +		goto err_media_entity_pipeline_start;
> > 
> >  	/* Verify that the currently configured format matches the output of
> >  	 * the connected subdev.
> >  	 */
> >  	ret = isp_video_check_format(video, vfh);
> >  	if (ret < 0)
> > -		goto error;
> > +		goto err_isp_video_check_format;
> > 
> >  	video->bpl_padding = ret;
> >  	video->bpl_value = vfh->format.fmt.pix.bytesperline;
> > @@ -1021,7 +1023,7 @@ isp_video_streamon(struct file *file, void *fh, enum
> > v4l2_buf_type type) } else {
> >  		if (far_end == NULL) {
> >  			ret = -EPIPE;
> > -			goto error;
> > +			goto err_isp_video_check_format;
> >  		}
> > 
> >  		state = ISP_PIPELINE_STREAM_INPUT | ISP_PIPELINE_IDLE_INPUT;
> > @@ -1032,7 +1034,7 @@ isp_video_streamon(struct file *file, void *fh, enum
> > v4l2_buf_type type) /* Validate the pipeline and update its state. */
> >  	ret = isp_video_validate_pipeline(pipe);
> >  	if (ret < 0)
> > -		goto error;
> > +		goto err_isp_video_check_format;
> > 
> >  	pipe->error = false;
> > 
> > @@ -1054,7 +1056,7 @@ isp_video_streamon(struct file *file, void *fh, enum
> > v4l2_buf_type type)
> > 
> >  	ret = omap3isp_video_queue_streamon(&vfh->queue);
> >  	if (ret < 0)
> > -		goto error;
> > +		goto err_isp_video_check_format;
> > 
> >  	/* In sensor-to-memory mode, the stream can be started synchronously
> >  	 * to the stream on command. In memory-to-memory mode, it will be
> > @@ -1064,32 +1066,35 @@ isp_video_streamon(struct file *file, void *fh, enum
> > v4l2_buf_type type) ret = omap3isp_pipeline_set_stream(pipe,
> >  					      ISP_PIPELINE_STREAM_CONTINUOUS);
> >  		if (ret < 0)
> > -			goto error;
> > +			goto err_omap3isp_set_stream;
> >  		spin_lock_irqsave(&video->queue->irqlock, flags);
> >  		if (list_empty(&video->dmaqueue))
> >  			video->dmaqueue_flags |= ISP_VIDEO_DMAQUEUE_UNDERRUN;
> >  		spin_unlock_irqrestore(&video->queue->irqlock, flags);
> >  	}
> > 
> > -error:
> > -	if (ret < 0) {
> > -		omap3isp_video_queue_streamoff(&vfh->queue);
> > -		media_entity_pipeline_stop(&video->video.entity);
> > -		if (video->isp->pdata->set_constraints)
> > -			video->isp->pdata->set_constraints(video->isp, false);
> > -		/* The DMA queue must be emptied here, otherwise CCDC interrupts
> > -		 * that will get triggered the next time the CCDC is powered up
> > -		 * will try to access buffers that might have been freed but
> > -		 * still present in the DMA queue. This can easily get triggered
> > -		 * if the above omap3isp_pipeline_set_stream() call fails on a
> > -		 * system with a free-running sensor.
> > -		 */
> > -		INIT_LIST_HEAD(&video->dmaqueue);
> > -		video->queue = NULL;
> > -	}
> > +	video->streaming = 1;
> > +
> > +	mutex_unlock(&video->stream_lock);
> > +	return 0;
> > 
> > -	if (!ret)
> > -		video->streaming = 1;
> > +err_omap3isp_set_stream:
> > +	omap3isp_video_queue_streamoff(&vfh->queue);
> > +err_isp_video_check_format:
> > +	media_entity_pipeline_stop(&video->video.entity);
> > +err_media_entity_pipeline_start:
> 
> Could you please shorten the labels a bit (err_set_stream, err_check_format 
> and err_pipeline_start for instance) ?

Fixed.

> > +	if (video->isp->pdata->set_constraints)
> > +		video->isp->pdata->set_constraints(video->isp, false);
> > +	/* The DMA queue must be emptied here, otherwise CCDC
> > +	 * interrupts that will get triggered the next time the CCDC
> > +	 * is powered up will try to access buffers that might have
> > +	 * been freed but still present in the DMA queue. This can
> > +	 * easily get triggered if the above
> > +	 * omap3isp_pipeline_set_stream() call fails on a system with
> > +	 * a free-running sensor.
> 
> Please reindent the text to the 80 columns limits.

I can do the change albeit I think wrapping the lines at 72 characters
instead certainly doesn't harm readability.

> > +	 */
> > +	INIT_LIST_HEAD(&video->dmaqueue);
> > +	video->queue = NULL;
> > 
> >  	mutex_unlock(&video->stream_lock);
> >  	return ret;

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@xxxxxx	jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux