| Message ID | 20200522181435.3174615-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote: > Older kernels do not support all 'reported' formats. Skip them on those > kernels. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Marking this as RFC, as I don't actually yet know what the correct > kernel version is to condition against. I think it will be v5.9 if we're lucky :-) > Currently, the QCam application can not stream the VIMC Camera for users > of QT Version less that 5.14 as that is when the BGR888 format becomes a > 'preferred' native format. > > Users of QT < 5.14 will find that qcam will chose a 'preferred' format > which is reported as supported by the kernel, when in fact it isn't. > > Ensure that those formats are not reported as supported by the vimc > pipeline handler on older kernels. > > src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index 68d65bc785b0..78fb0ece21f8 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > const StreamRoles &roles) > { > CameraConfiguration *config = new VimcCameraConfiguration(); > + VimcCameraData *data = cameraData(camera); > > if (roles.empty()) > return config; > @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > std::map<PixelFormat, std::vector<SizeRange>> formats; > > for (const auto &pixelformat : pixelformats) { > + /* > + * \todo: Update this when the kernel correctly supports other > + * reported formats. > + */ > + if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) { I wouldn't hardcode any kernel version until we know when the issue will be fixed. I would remove the unsupported formats from the pixelformats map now, and deal with versioning later. > + if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > + LOG(VIMC, Info) > + << "Skipping unsupported pixel format" > + << pixelformat.first.toString(); > + continue; > + } > + } > + You also need to patch the VimcCameraConfiguration::validate() and PipelineHandlerVimc::configure() functions, as they both use the pixelformats map. One option would be to use StreamConfiguration::formats().pixelformats() in both locations, so only one location would need to deal with kernel versions. > /* The scaler hardcodes a x3 scale-up ratio. */ > std::vector<SizeRange> sizes{ > SizeRange{ { 48, 48 }, { 4096, 2160 } }
Hi Laurent, On 22/05/2020 21:37, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote: >> Older kernels do not support all 'reported' formats. Skip them on those >> kernels. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> Marking this as RFC, as I don't actually yet know what the correct >> kernel version is to condition against. > > I think it will be v5.9 if we're lucky :-) I think some progress has been made for 5.7, but I don't know the current state. Kaaira might know more here. >> Currently, the QCam application can not stream the VIMC Camera for users >> of QT Version less that 5.14 as that is when the BGR888 format becomes a >> 'preferred' native format. >> >> Users of QT < 5.14 will find that qcam will chose a 'preferred' format >> which is reported as supported by the kernel, when in fact it isn't. >> >> Ensure that those formats are not reported as supported by the vimc >> pipeline handler on older kernels. >> >> src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp >> index 68d65bc785b0..78fb0ece21f8 100644 >> --- a/src/libcamera/pipeline/vimc/vimc.cpp >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp >> @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, >> const StreamRoles &roles) >> { >> CameraConfiguration *config = new VimcCameraConfiguration(); >> + VimcCameraData *data = cameraData(camera); >> >> if (roles.empty()) >> return config; >> @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, >> std::map<PixelFormat, std::vector<SizeRange>> formats; >> >> for (const auto &pixelformat : pixelformats) { >> + /* >> + * \todo: Update this when the kernel correctly supports other >> + * reported formats. >> + */ >> + if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) { > > I wouldn't hardcode any kernel version until we know when the issue will > be fixed. I would remove the unsupported formats from the pixelformats > map now, and deal with versioning later. Ok, well that will depend upon the current mainline state. >> + if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { >> + LOG(VIMC, Info) >> + << "Skipping unsupported pixel format" >> + << pixelformat.first.toString(); >> + continue; >> + } >> + } >> + > > You also need to patch the VimcCameraConfiguration::validate() and I started at validate in fact, but couldn't (quickly) get the CameraData > PipelineHandlerVimc::configure() functions, as they both use the > pixelformats map. One option would be to use > StreamConfiguration::formats().pixelformats() in both locations, so only > one location would need to deal with kernel versions. That makes sense in validate(), but not configure() (which should have already been validated()). (Context, this is based upon a branch with your patch applied to provide the PixelFormat -> MBUS code mapping) > >> /* The scaler hardcodes a x3 scale-up ratio. */ >> std::vector<SizeRange> sizes{ >> SizeRange{ { 48, 48 }, { 4096, 2160 } } >
Hi Kieran, On Tue, May 26, 2020 at 01:04:31PM +0100, Kieran Bingham wrote: > On 22/05/2020 21:37, Laurent Pinchart wrote: > > On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote: > >> Older kernels do not support all 'reported' formats. Skip them on those > >> kernels. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> Marking this as RFC, as I don't actually yet know what the correct > >> kernel version is to condition against. > > > > I think it will be v5.9 if we're lucky :-) > > I think some progress has been made for 5.7, but I don't know the > current state. Kaaira might know more here. > > >> Currently, the QCam application can not stream the VIMC Camera for users > >> of QT Version less that 5.14 as that is when the BGR888 format becomes a > >> 'preferred' native format. > >> > >> Users of QT < 5.14 will find that qcam will chose a 'preferred' format > >> which is reported as supported by the kernel, when in fact it isn't. > >> > >> Ensure that those formats are not reported as supported by the vimc > >> pipeline handler on older kernels. > >> > >> src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > >> index 68d65bc785b0..78fb0ece21f8 100644 > >> --- a/src/libcamera/pipeline/vimc/vimc.cpp > >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp > >> @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > >> const StreamRoles &roles) > >> { > >> CameraConfiguration *config = new VimcCameraConfiguration(); > >> + VimcCameraData *data = cameraData(camera); > >> > >> if (roles.empty()) > >> return config; > >> @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > >> std::map<PixelFormat, std::vector<SizeRange>> formats; > >> > >> for (const auto &pixelformat : pixelformats) { > >> + /* > >> + * \todo: Update this when the kernel correctly supports other > >> + * reported formats. > >> + */ > >> + if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) { > > > > I wouldn't hardcode any kernel version until we know when the issue will > > be fixed. I would remove the unsupported formats from the pixelformats > > map now, and deal with versioning later. > > Ok, well that will depend upon the current mainline state. Sure. If v5.7 already supports multiple formats then we should deal with it now, but otherwise we can defer the version handling until we know in which kernel version it will be available. > >> + if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > >> + LOG(VIMC, Info) > >> + << "Skipping unsupported pixel format" > >> + << pixelformat.first.toString(); > >> + continue; > >> + } > >> + } > >> + > > > > You also need to patch the VimcCameraConfiguration::validate() and > > I started at validate in fact, but couldn't (quickly) get the CameraData > > > PipelineHandlerVimc::configure() functions, as they both use the > > pixelformats map. One option would be to use > > StreamConfiguration::formats().pixelformats() in both locations, so only > > one location would need to deal with kernel versions. > > That makes sense in validate(), but not configure() (which should have > already been validated()). You're right, my bad. Only validate() needs to be addressed in addition to generateConfiguration(). > (Context, this is based upon a branch with your patch applied to provide > the PixelFormat -> MBUS code mapping) > > >> /* The scaler hardcodes a x3 scale-up ratio. */ > >> std::vector<SizeRange> sizes{ > >> SizeRange{ { 48, 48 }, { 4096, 2160 } }
On Tue, May 26, 2020 at 01:04:31PM +0100, Kieran Bingham wrote: > Hi Laurent, > > On 22/05/2020 21:37, Laurent Pinchart wrote: > > Hi Kieran, > > > > Thank you for the patch. > > > > On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote: > >> Older kernels do not support all 'reported' formats. Skip them on those > >> kernels. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> Marking this as RFC, as I don't actually yet know what the correct > >> kernel version is to condition against. > > > > I think it will be v5.9 if we're lucky :-) > > > I think some progress has been made for 5.7, but I don't know the > current state. Kaaira might know more here. Hi! Support for other formats has been added to vimc, but it is a bit buggy right now as it swaps red and blue channels in the final image. I have sent a patch to fix that, as soon as it gets accepted, libcamera can support them as well for newer kernel versions. > > > >> Currently, the QCam application can not stream the VIMC Camera for users > >> of QT Version less that 5.14 as that is when the BGR888 format becomes a > >> 'preferred' native format. > >> > >> Users of QT < 5.14 will find that qcam will chose a 'preferred' format > >> which is reported as supported by the kernel, when in fact it isn't. > >> > >> Ensure that those formats are not reported as supported by the vimc > >> pipeline handler on older kernels. > >> > >> src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > >> index 68d65bc785b0..78fb0ece21f8 100644 > >> --- a/src/libcamera/pipeline/vimc/vimc.cpp > >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp > >> @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > >> const StreamRoles &roles) > >> { > >> CameraConfiguration *config = new VimcCameraConfiguration(); > >> + VimcCameraData *data = cameraData(camera); > >> > >> if (roles.empty()) > >> return config; > >> @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > >> std::map<PixelFormat, std::vector<SizeRange>> formats; > >> > >> for (const auto &pixelformat : pixelformats) { > >> + /* > >> + * \todo: Update this when the kernel correctly supports other > >> + * reported formats. > >> + */ > >> + if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) { > > > > I wouldn't hardcode any kernel version until we know when the issue will > > be fixed. I would remove the unsupported formats from the pixelformats > > map now, and deal with versioning later. > > Ok, well that will depend upon the current mainline state. > > > >> + if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { > >> + LOG(VIMC, Info) > >> + << "Skipping unsupported pixel format" > >> + << pixelformat.first.toString(); > >> + continue; > >> + } > >> + } > >> + > > > > You also need to patch the VimcCameraConfiguration::validate() and > > I started at validate in fact, but couldn't (quickly) get the CameraData > > > PipelineHandlerVimc::configure() functions, as they both use the > > pixelformats map. One option would be to use > > StreamConfiguration::formats().pixelformats() in both locations, so only > > one location would need to deal with kernel versions. > > That makes sense in validate(), but not configure() (which should have > already been validated()). > > (Context, this is based upon a branch with your patch applied to provide > the PixelFormat -> MBUS code mapping) > > > > >> /* The scaler hardcodes a x3 scale-up ratio. */ > >> std::vector<SizeRange> sizes{ > >> SizeRange{ { 48, 48 }, { 4096, 2160 } } > > > > -- > Regards > -- > Kieran
Hi Kaaira, On 26/05/2020 14:25, Kaaira Gupta wrote: > On Tue, May 26, 2020 at 01:04:31PM +0100, Kieran Bingham wrote: >> Hi Laurent, >> >> On 22/05/2020 21:37, Laurent Pinchart wrote: >>> Hi Kieran, >>> >>> Thank you for the patch. >>> >>> On Fri, May 22, 2020 at 07:14:35PM +0100, Kieran Bingham wrote: >>>> Older kernels do not support all 'reported' formats. Skip them on those >>>> kernels. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> Marking this as RFC, as I don't actually yet know what the correct >>>> kernel version is to condition against. >>> >>> I think it will be v5.9 if we're lucky :-) >> >> >> I think some progress has been made for 5.7, but I don't know the >> current state. Kaaira might know more here. > > Hi! Support for other formats has been added to vimc, but it is a bit > buggy right now as it swaps red and blue channels in the final image. I > have sent a patch to fix that, as soon as it gets accepted, libcamera > can support them as well for newer kernel versions. I've just managed to test my jpeg encoder with BGR888 and RGB888 and both of those came out with good images. Then I ran QCam using the virtme configuration you sent me - and indeed I see the colour inversions. But I suspect that could be due to qcam now. On mainline (v5.7-rc) I can successfully capture BGR888 and RGB888 (RG24|BG24) but I can not configure a BGRA8888 BA24 pipeline yet. >>>> Currently, the QCam application can not stream the VIMC Camera for users >>>> of QT Version less that 5.14 as that is when the BGR888 format becomes a >>>> 'preferred' native format. >>>> >>>> Users of QT < 5.14 will find that qcam will chose a 'preferred' format >>>> which is reported as supported by the kernel, when in fact it isn't. >>>> >>>> Ensure that those formats are not reported as supported by the vimc >>>> pipeline handler on older kernels. >>>> >>>> src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp >>>> index 68d65bc785b0..78fb0ece21f8 100644 >>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp >>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp >>>> @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, >>>> const StreamRoles &roles) >>>> { >>>> CameraConfiguration *config = new VimcCameraConfiguration(); >>>> + VimcCameraData *data = cameraData(camera); >>>> >>>> if (roles.empty()) >>>> return config; >>>> @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, >>>> std::map<PixelFormat, std::vector<SizeRange>> formats; >>>> >>>> for (const auto &pixelformat : pixelformats) { >>>> + /* >>>> + * \todo: Update this when the kernel correctly supports other >>>> + * reported formats. >>>> + */ >>>> + if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) { >>> >>> I wouldn't hardcode any kernel version until we know when the issue will >>> be fixed. I would remove the unsupported formats from the pixelformats >>> map now, and deal with versioning later. >> >> Ok, well that will depend upon the current mainline state. >> >> >>>> + if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { >>>> + LOG(VIMC, Info) >>>> + << "Skipping unsupported pixel format" >>>> + << pixelformat.first.toString(); >>>> + continue; >>>> + } >>>> + } >>>> + >>> >>> You also need to patch the VimcCameraConfiguration::validate() and >> >> I started at validate in fact, but couldn't (quickly) get the CameraData >> >>> PipelineHandlerVimc::configure() functions, as they both use the >>> pixelformats map. One option would be to use >>> StreamConfiguration::formats().pixelformats() in both locations, so only >>> one location would need to deal with kernel versions. >> >> That makes sense in validate(), but not configure() (which should have >> already been validated()). >> >> (Context, this is based upon a branch with your patch applied to provide >> the PixelFormat -> MBUS code mapping) >> >>> >>>> /* The scaler hardcodes a x3 scale-up ratio. */ >>>> std::vector<SizeRange> sizes{ >>>> SizeRange{ { 48, 48 }, { 4096, 2160 } } >>> >> >> -- >> Regards >> -- >> Kieran
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index 68d65bc785b0..78fb0ece21f8 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -171,6 +171,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, const StreamRoles &roles) { CameraConfiguration *config = new VimcCameraConfiguration(); + VimcCameraData *data = cameraData(camera); if (roles.empty()) return config; @@ -178,6 +179,19 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, std::map<PixelFormat, std::vector<SizeRange>> formats; for (const auto &pixelformat : pixelformats) { + /* + * \todo: Update this when the kernel correctly supports other + * reported formats. + */ + if (data->media_->version() <= KERNEL_VERSION(5, 7, 0)) { + if (pixelformat.first != PixelFormat(DRM_FORMAT_BGR888)) { + LOG(VIMC, Info) + << "Skipping unsupported pixel format" + << pixelformat.first.toString(); + continue; + } + } + /* The scaler hardcodes a x3 scale-up ratio. */ std::vector<SizeRange> sizes{ SizeRange{ { 48, 48 }, { 4096, 2160 } }
Older kernels do not support all 'reported' formats. Skip them on those kernels. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Marking this as RFC, as I don't actually yet know what the correct kernel version is to condition against. Currently, the QCam application can not stream the VIMC Camera for users of QT Version less that 5.14 as that is when the BGR888 format becomes a 'preferred' native format. Users of QT < 5.14 will find that qcam will chose a 'preferred' format which is reported as supported by the kernel, when in fact it isn't. Ensure that those formats are not reported as supported by the vimc pipeline handler on older kernels. src/libcamera/pipeline/vimc/vimc.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+)