-
Notifications
You must be signed in to change notification settings - Fork 7.8k
video: stm32: dcmipp: addition of (semi)planar format support #94081
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
video: stm32: dcmipp: addition of (semi)planar format support #94081
Conversation
80c3b9c
to
0108872
Compare
Added NV24/NV42 as well |
Mark the PR as Ready for review now that I have validated all 6 formats the DCMIPP can output (NV12/NV21/NV16/NV61/YUV420/YVU420) |
Tested OK on N6 for NV12 with #92884 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a minor comment on format documentation
include/zephyr/drivers/video.h
Outdated
/** | ||
* CbCr(UV) plane has same width and height as Y plane | ||
* | ||
* @code{.unparsed} | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | ... | | ||
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | | ||
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | | ||
* | ... | | ||
* @endcode | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is the same for NV16 and NV24. Should we add something to distinguish them ?
Idem for NV61 and NV42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Probably the way they are all represented is not the best one. Since everything is 8bits, there is no need to have Yyyyyyyy so just mentioning 8bit for each sample in the description and then adding number behind each component to indicate to which pixel it correspond could be better.
Something like:
| Y0 Y1 Y2 Y3|
| Y.. Y.. Y.. Y..|
| U0 V0 U1 V1 U2 V2 U3 V3 |
| U.. V.. U.. V.. U.. V.. U.. V.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something like what is described by Microsoft : https://learn.microsoft.com/en-us/windows/win32/medfound/recommended-8-bit-yuv-formats-for-video-rendering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe with a small change in numbering but this seems like adding some numbering helps.
| Y0 Y1 Y2 Y3|
| Y.. Y.. Y.. Y..|
| U0/1 V0/1 U2/3 V2/3 |
| U.. V.. U.. V.. U.. V.. U.. V.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see this coming.
A few suggestions for how to improve it just a bit, though all can be done in the future as well.
/* TODO - the HAL is missing a SetMemoryAddress for auxiliary addresses */ | ||
/* Update main buffer address */ | ||
if (pipe->id == DCMIPP_PIPE0) { | ||
WRITE_REG(dcmipp->hdcmipp.Instance->P0PPM0AR1, (uint32_t)plane); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also sys_write32()
available, which requires #include <zephyr/sys/sys_io.h>
if interested.
if (fmt->pixelformat == VIDEO_PIX_FMT_NV12 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_NV21 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_NV16 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_NV61 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_YUV420 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_YVU420) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to wrap this in a macro? For instance one locally defined in this driver, some VIDEO_IS_PLANAR(fmt->pixelformat)
for instance.
if (fmt->pixelformat == VIDEO_PIX_FMT_NV12 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_NV21 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_NV16 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_NV61 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_YUV420 || | ||
fmt->pixelformat == VIDEO_PIX_FMT_YVU420) { | ||
pipe_cfg.PixelPipePitch = fmt->width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood, bere this means that fmt->width
is being used as pitch for the first plane.
This seems to work for the particular formats used here, but if using a macro VIDEO_IS_PLANAR()
, maybe this would also need a macro a bit like this maybe:
#define VIDEO_Y_PLANE_PITCH(fmt) (VIDEO_IS_PLANAR((fmt)->pixelformat) ? (fmt)->width : 0)
|
||
#if defined(STM32_DCMIPP_HAS_PIXEL_PIPES) | ||
if (fmt->pixelformat == VIDEO_PIX_FMT_YUV420 || fmt->pixelformat == VIDEO_PIX_FMT_YVU420) { | ||
uint8_t *u_addr = pipe->next->buffer + fmt->width * fmt->height; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be difficult to guess that fmt->width
is actually the pitch of the Y plane.
How about a macro like this:
#define VIDEO_Y_PLANE_SIZE(fmt) (VIDEO_Y_PLANE_PITCH(fmt) * (fmt)->height)
The Y plane is present in all YUV planar formats so hopefully quite generic.
include/zephyr/drivers/video.h
Outdated
* @code{.unparsed} | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | ... | | ||
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | | ||
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | | ||
* | ... | | ||
* @endcode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If wanting a fully expanded version, how about something like this?
/**
* The CbCr (UV) plane has the same width and height as the Y plane.
*
* Where each letter (A, B, C...) below represents a logical group of pixels:
*
* @code{.unparsed}
* A A B B
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* A A B B
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* C C D D
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* C C D D
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
*
* | ... |
*
* A A B B
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
* C C D D
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
*
* | ... |
* @endcode
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative: describe the planes pixel packing first:
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* | ... |
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
* | ... |
And then give a diagram of how things are effectively mapped with a different diagram.
* Y0 Y1 Y2 Y3 ...
* Y6 Y7 Y8 Y9 ...
* ...
*
* U0/1/6/7 V0/1/6/7 U2/3/8/9 V2/3/8/9 ...
* ...
Or a diagram like this turned into ASCII:
Or even something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally wouldn't put the pixel packing description but it turns out that there are as well 10bit, 12bit variants for those formats, so, yes, ok to keep the pixel packing part.
Also having the following diagram which show how Y / U / V map together is nice:
- Y0 Y1 Y2 Y3 ...
- Y6 Y7 Y8 Y9 ...
- ...
- U0/1/6/7 V0/1/6/7 U2/3/8/9 V2/3/8/9 ...
- ...
Let me prepare the updated descriptions with that
include/zephyr/drivers/video.h
Outdated
/** | ||
* CrCb(VU) plane has same width as Y plane however half height | ||
* | ||
* @code{.unparsed} | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | ... | | ||
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | | ||
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | | ||
* | ... | | ||
* @endcode | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the discussion above:
/**
* The CrCb (VU) plane has same width and height as the Y plane
*
* Where each letter (A, B, C...) below represents a logical group of pixels:
*
* @code{.unparsed}
* A A B B
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* A A B B
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* C C D D
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* C C D D
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
*
* | ... |
*
* A A B B
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | ...
* C C D D
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | ...
*
* | ... |
* @endcode
*/
include/zephyr/drivers/video.h
Outdated
* CbCr(UV) plane has same width and height as Y plane | ||
* | ||
* @code{.unparsed} | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | ... | | ||
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | | ||
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | | ||
* | ... | | ||
* @endcode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the discussion above:
/**
* The CrCb (VU) plane has same width and height as the Y plane
*
* Where each number below represents a logical group of pixels:
*
* @code{.unparsed}
* A A B B
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* C C D D
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* E E F F
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* G G H H
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
*
* | ... |
*
* A A B B
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
* C C D D
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
* E E F F
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
* G G H H
* | Uuuuuuuu Vvvvvvvv | Uuuuuuuu Vvvvvvvv | ...
*
* | ... |
* @endcode
*/
include/zephyr/drivers/video.h
Outdated
* @code{.unparsed} | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | Yyyyyyyy Yyyyyyyy | Yyyyyyyy Yyyyyyyy | | ||
* | ... | | ||
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | | ||
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | | ||
* | ... | | ||
* @endcode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* The CrCb (VU) plane has same width and height as the Y plane
*
* Where each number below represents a logical group of pixels:
*
* @code{.unparsed}
* A A B B
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* C C D D
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* E E F F
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
* G G H H
* | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | Yyyyyyyy | ...
*
* | ... |
*
* A A B B
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | ...
* C C D D
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | ...
* E E F F
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | ...
* G G H H
* | Vvvvvvvv Uuuuuuuu | Vvvvvvvv Uuuuuuuu | ...
*
* | ... |
* @endcode
*/
Add description and fourcc define for some of the 2 and 3 planes pixel formats. Signed-off-by: Alain Volmat <[email protected]>
ISP is part of the pixel pipes hence it doesn't make any sense to try to call ISP external handlers if the DCMIPP doesn't have pixel pipes available. Signed-off-by: Alain Volmat <[email protected]>
Add support for NV12/NV21, NV16/NV61 and YUV420/YVU420 (semi)planar formats which can be output by the main zephyrproject-rtos#1 pipe. Signed-off-by: Alain Volmat <[email protected]>
0108872
to
85c17b2
Compare
@ngphibang @josuah , I've only updated the video.h header with the formats description for now. |
|
The STM32 DCMIPP pipe 1 (main) is able to generate semi planar and planar formats such as NV12/NV16 or YUV420.
This PR adds definition of those formats as well as their description in the API header and add their support within the dcmipp driver.