[libcamera-devel,03/10] libcamera: ipu3: Fold mediaBusToFormat() into only caller
diff mbox series

Message ID 20200602013909.3170593-4-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: ipu3: Allow zero-copy RAW stream
Related show

Commit Message

Niklas Söderlund June 2, 2020, 1:39 a.m. UTC 
Make the code easier to read and refactor.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
 1 file changed, 19 insertions(+), 19 deletions(-)

Comments

Jacopo Mondi June 2, 2020, 9:04 a.m. UTC | #1 
Hi Niklas,

On Tue, Jun 02, 2020 at 03:39:02AM +0200, Niklas Söderlund wrote:
> Make the code easier to read and refactor.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 6df1e29281941ebf..f7363244e1d2d0ff 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -133,8 +133,6 @@ public:
>  	int start();
>  	int stop();
>
> -	static V4L2PixelFormat mediaBusToFormat(unsigned int code);
> -
>  	V4L2VideoDevice *output_;
>  	V4L2Subdevice *csi2_;
>  	CameraSensor *sensor_;
> @@ -1504,7 +1502,25 @@ int CIO2Device::configure(const Size &size,
>  	if (ret)
>  		return ret;
>
> -	outputFormat->fourcc = mediaBusToFormat(sensorFormat.mbus_code);
> +	V4L2PixelFormat v4l2Format;
> +	switch (sensorFormat.mbus_code) {
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> +		break;
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> +		break;
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> +		break;
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	outputFormat->fourcc = v4l2Format;

Nit: you could assign outputFormat->fourcc directly and avoid the
temporary variable

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	outputFormat->size = sensorFormat.size;
>  	outputFormat->planesCount = 1;
>
> @@ -1578,22 +1594,6 @@ int CIO2Device::stop()
>  	return output_->streamOff();
>  }
>
> -V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> -{
> -	switch (code) {
> -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> -	default:
> -		return {};
> -	}
> -}
> -
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>
>  } /* namespace libcamera */
> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 4, 2020, 3:07 a.m. UTC | #2 
Hi Niklas,

Thank you for the patch.

On Tue, Jun 02, 2020 at 03:39:02AM +0200, Niklas Söderlund wrote:
> Make the code easier to read and refactor.

I'm not necessarily opposed to this, but given that mapping between
media bus formats and V4L2 pixel formats on video nodes is an operation
that most pipeline handlers need to perform, and that it is
device-specific, would it make sense to try and keep it in separate
functions that would have the same name in all pipeline handlers ? We
could even create an static const std::map for that, and remove the
function.

If you think the code is better with this patch, I'm fine with it.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 6df1e29281941ebf..f7363244e1d2d0ff 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -133,8 +133,6 @@ public:
>  	int start();
>  	int stop();
>  
> -	static V4L2PixelFormat mediaBusToFormat(unsigned int code);
> -
>  	V4L2VideoDevice *output_;
>  	V4L2Subdevice *csi2_;
>  	CameraSensor *sensor_;
> @@ -1504,7 +1502,25 @@ int CIO2Device::configure(const Size &size,
>  	if (ret)
>  		return ret;
>  
> -	outputFormat->fourcc = mediaBusToFormat(sensorFormat.mbus_code);
> +	V4L2PixelFormat v4l2Format;
> +	switch (sensorFormat.mbus_code) {
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> +		break;
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> +		break;
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> +		break;
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	outputFormat->fourcc = v4l2Format;
>  	outputFormat->size = sensorFormat.size;
>  	outputFormat->planesCount = 1;
>  
> @@ -1578,22 +1594,6 @@ int CIO2Device::stop()
>  	return output_->streamOff();
>  }
>  
> -V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> -{
> -	switch (code) {
> -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> -	default:
> -		return {};
> -	}
> -}
> -
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>  
>  } /* namespace libcamera */
Niklas Söderlund June 6, 2020, 10:53 a.m. UTC | #3 
Hi Laurent,

Thanks for your feedback.

On 2020-06-04 06:07:02 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Jun 02, 2020 at 03:39:02AM +0200, Niklas Söderlund wrote:
> > Make the code easier to read and refactor.
> 
> I'm not necessarily opposed to this, but given that mapping between
> media bus formats and V4L2 pixel formats on video nodes is an operation
> that most pipeline handlers need to perform, and that it is
> device-specific, would it make sense to try and keep it in separate
> functions that would have the same name in all pipeline handlers ? We
> could even create an static const std::map for that, and remove the
> function.

I'm not opposed to move code into helpers and creating infrastructure 
that can be shared between different pipelines. If we go that route I 
think we should define a base class that pipelines then may implement 
instead of depending on soft convections around naming which will be 
hard to enforce.

As we are still in early stages of identifying what and how to share 
infrastructure between pipelines I think it's better we make each 
pipeline as neat as possible now and later break out common things when 
we know how we wish it to look. In my experience preemptively trying to 
anticipate where things could be shared and build for a imaginary 
infrastructure makes it harder identify a good design once work to 
actually share code starts :-) But I might be wrong.

I will keep this patch as is for v2 unless anyone strongly oppose it.

> 
> If you think the code is better with this patch, I'm fine with it.
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 6df1e29281941ebf..f7363244e1d2d0ff 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -133,8 +133,6 @@ public:
> >  	int start();
> >  	int stop();
> >  
> > -	static V4L2PixelFormat mediaBusToFormat(unsigned int code);
> > -
> >  	V4L2VideoDevice *output_;
> >  	V4L2Subdevice *csi2_;
> >  	CameraSensor *sensor_;
> > @@ -1504,7 +1502,25 @@ int CIO2Device::configure(const Size &size,
> >  	if (ret)
> >  		return ret;
> >  
> > -	outputFormat->fourcc = mediaBusToFormat(sensorFormat.mbus_code);
> > +	V4L2PixelFormat v4l2Format;
> > +	switch (sensorFormat.mbus_code) {
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> > +		break;
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> > +		break;
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> > +		break;
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	outputFormat->fourcc = v4l2Format;
> >  	outputFormat->size = sensorFormat.size;
> >  	outputFormat->planesCount = 1;
> >  
> > @@ -1578,22 +1594,6 @@ int CIO2Device::stop()
> >  	return output_->streamOff();
> >  }
> >  
> > -V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> > -{
> > -	switch (code) {
> > -	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> > -	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> > -	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> > -	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > -		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> > -	default:
> > -		return {};
> > -	}
> > -}
> > -
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> >  
> >  } /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 6df1e29281941ebf..f7363244e1d2d0ff 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -133,8 +133,6 @@  public:
 	int start();
 	int stop();
 
-	static V4L2PixelFormat mediaBusToFormat(unsigned int code);
-
 	V4L2VideoDevice *output_;
 	V4L2Subdevice *csi2_;
 	CameraSensor *sensor_;
@@ -1504,7 +1502,25 @@  int CIO2Device::configure(const Size &size,
 	if (ret)
 		return ret;
 
-	outputFormat->fourcc = mediaBusToFormat(sensorFormat.mbus_code);
+	V4L2PixelFormat v4l2Format;
+	switch (sensorFormat.mbus_code) {
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
+		break;
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
+		break;
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
+		break;
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+		v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	outputFormat->fourcc = v4l2Format;
 	outputFormat->size = sensorFormat.size;
 	outputFormat->planesCount = 1;
 
@@ -1578,22 +1594,6 @@  int CIO2Device::stop()
 	return output_->streamOff();
 }
 
-V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
-{
-	switch (code) {
-	case MEDIA_BUS_FMT_SBGGR10_1X10:
-		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
-	case MEDIA_BUS_FMT_SGBRG10_1X10:
-		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
-	case MEDIA_BUS_FMT_SGRBG10_1X10:
-		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
-	default:
-		return {};
-	}
-}
-
 REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
 
 } /* namespace libcamera */