Hi Alex, On Sunday, June 23, 2024 5:33:28 A.M. EDT you wrote: > Hi Detlev, > > Am 20.06.24 um 16:19 schrieb Detlev Casanova: > > This driver supports the second generation of the Rockchip Video > > decoder, also known as vdpu34x. > > It is currently only used on the RK3588(s) SoC. > > > > There are 2 decoders on the RK3588 SoC that can work in pair to decode > > 8K video at 30 FPS but currently, only using one core at a time is > > supported. > > > > Scheduling requests between the two cores will be implemented later. > > > > The core supports H264, HEVC, VP9 and AVS2 decoding but this driver > > currently only supports H264. > > > > The driver is based on rkvdec and they may share some code in the > > future. > > The decision to make a different driver is mainly because rkvdec2 has > > more features and can work with multiple cores. > > > > The registers are mapped in a struct in RAM using bitfields. It is IO > > copied to the HW when all values are configured. > > The decision to use such a struct instead of writing buffers one by one > > > > is based on the following reasons: > > - Rockchip cores are known to misbehave when registers are not written > > > > in address order, > > > > - Those cores also need the software to write all registers, even if > > > > they are written their default values or are not related to the task > > (this core will not start decoding some H264 frames if some VP9 > > registers are not written to 0) > > > > - In the future, to support multiple cores, the scheduler could be > > > > optimized by storing the precomputed registers values and copy them > > to the HW as soos as a core becomes available. > > > > This makes the code more readable and may bring performance improvements > > in future features. > > > > Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx> > > --- > > > > drivers/staging/media/Kconfig | 1 + > > drivers/staging/media/Makefile | 1 + > > drivers/staging/media/rkvdec2/Kconfig | 15 + > > drivers/staging/media/rkvdec2/Makefile | 3 + > > drivers/staging/media/rkvdec2/TODO | 9 + > > drivers/staging/media/rkvdec2/rkvdec2-h264.c | 739 +++++++++++ > > drivers/staging/media/rkvdec2/rkvdec2-regs.h | 345 +++++ > > drivers/staging/media/rkvdec2/rkvdec2.c | 1253 ++++++++++++++++++ > > drivers/staging/media/rkvdec2/rkvdec2.h | 130 ++ > > 9 files changed, 2496 insertions(+) > > create mode 100644 drivers/staging/media/rkvdec2/Kconfig > > create mode 100644 drivers/staging/media/rkvdec2/Makefile > > create mode 100644 drivers/staging/media/rkvdec2/TODO > > create mode 100644 drivers/staging/media/rkvdec2/rkvdec2-h264.c > > create mode 100644 drivers/staging/media/rkvdec2/rkvdec2-regs.h > > create mode 100644 drivers/staging/media/rkvdec2/rkvdec2.c > > create mode 100644 drivers/staging/media/rkvdec2/rkvdec2.h > > ... > > > +static inline void rkvdec2_memcpy_toio(void __iomem *dst, void *src, > > size_t len) +{ > > +#ifdef CONFIG_ARM64 > > + __iowrite32_copy(dst, src, len); > > +#elif defined(CONFIG_ARM) > > I guess that can get an "#else" since memcpy_toio exists for all archs. > > > + memcpy_toio(dst, src, len); > > +#endif > > +} > > + > > ... > > > + /* Set timeout threshold */ > > + if (pixels < RKVDEC2_1080P_PIXELS) > > + regs->common.timeout_threshold = RKVDEC2_TIMEOUT_1080p; > > + else if (pixels < RKVDEC2_4K_PIXELS) > > + regs->common.timeout_threshold = RKVDEC2_TIMEOUT_4K; > > + else if (pixels < RKVDEC2_8K_PIXELS) > > + regs->common.timeout_threshold = RKVDEC2_TIMEOUT_8K; > > + > > Did you test if it works with anything > 8K? If so, you propably want to > make the check above > > + else > + regs->common.timeout_threshold = RKVDEC2_TIMEOUT_8K; > > Otherwise the timeout may not be set/contain invalid values from any former > stream. That's right, but it would be set to 0 because of the memset. RKVDEC2_TIMEOUT_8K might not be enough for bigger frame sizes, so I'll set it to the maximum value (0xffffffff) when frames are bigger than 8K and also adapt the watchdog time: RKVDEC2_TIMEOUT_8K is around 100 ms, but 0xffffffff is arnoud 5.3 seconds (reg032/axi_clock_freq) I'll do more tests with this as well. > ... > > > + > > +static const struct rkvdec2_coded_fmt_desc rkvdec2_coded_fmts[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_H264_SLICE, > > + .frmsize = { > > + .min_width = 16, > > + .max_width = 65520, > > + .step_width = 16, > > + .min_height = 16, > > + .max_height = 65520, > > + .step_height = 16, > > + }, > > + .ctrls = &rkvdec2_h264_ctrls, > > + .ops = &rkvdec2_h264_fmt_ops, > > + .num_decoded_fmts = ARRAY_SIZE(rkvdec2_h264_decoded_fmts), > > + .decoded_fmts = rkvdec2_h264_decoded_fmts, > > + .subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, > > + }, > > +}; > > + > > Note, that this is also given to userspace (VIDIOC_ENUM_FRAMESIZES) and > this is already incorrect in the old rkvdec driver (and hantro): From > userspace perspective we do not have a restriction in > step_width/step_width, as we are aligning any given width/height to HW > requirements in the driver - what we should give to userspace is > fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; fsize->stepwise.min_height = > 1; fsize->stepwise.min_width = 1; fsize->stepwise.max_height = 65520; > fsize->stepwise.max_width = 65520; Is fsize->stepwise.min_height = 1; and fsize->stepwise.min_width = 1 correct ? Or do you mean fsize->stepwise.step_height = 1; and fsize->stepwise.setp_width = 1 ? It would give this instead: .frmsize = { .min_width = 16, .max_width = 65520, .step_width = 1, .min_height = 16, .max_height = 65520, .step_height = 1, }, and .vidioc_enum_framesizes sets fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS; > I guess this new driver should be an > opportunity to fix that and distinguish between internal and external > frame size requirements and the .vidioc_enum_framesizes callback should > adapted accordingly. Regards, Alex Detlev.
Attachment:
signature.asc
Description: This is a digitally signed message part.