Hi Himanshu,

On 30/09/2025 06:03, Himanshu Dewangan wrote:
> Exynos MFC new driver upstream proposal
> +++++++++++++++++++++++++++++++++++++++
> 
> The s5p-mfc driver in the kernel is quite outdated and has not kept up with 
> the hardware advances of the MFC IP. 
> Going forward, we want to provide support for all the later versions of MFC 
> in open source (both mobile and
> ExynosAuto would be supported by Samsung)
> 
> We (Samsung) would like to propose a new driver for the latest generation of 
> Samsung MFC (Multi-Format Codec) hardware. 
> Although the kernel already contains a Samsung MFC driver 
> (drivers/media/platform/s5p-mfc/), the newer hardware 
> diverges significantly from the previous generations, in features supported, 
> register interface and in system integration. 
> This has necessitated a new driver, rather than updating the existing one. 

I did some (admittedly limited) comparison between the MFC hardware registers 
in patch 04/29
and what is in s5p-mfc and it did look like an incremental update, not a 
complete register redesign.

So I am not convinced from that that a new driver is needed.

In addition, a huge patch bomb like this is unreviewable and so I am just going 
to give a
few high-level remarks based on a quick scan of the patches.

1) Clearly you never ran v4l2-compliance with this new driver as it would fail 
immediately
   on the VIDIOC_QUERYCAP ioctl. It's a M2M device, so it should use either 
V4L2_CAP_VIDEO_M2M
   or V4L2_CAP_VIDEO_M2M_MPLANE, not a mix of output and capture caps. I 
suspect that
   v4l2-compliance will fail on many issues.

2) I noticed that there is no VB2_MMAP support. No idea why not, and all 
drivers with streaming I/O
   support must support VB2_MMAP.

3) Lots of controls are added, none of them documented or even defined in a 
public header.
   It's part of the public API, so that must all be present in 
include/uapi/linux. Just
   'git grep' for CID_MPEG_MFC51 to see how it should be done. I suspect that 
more than a few
   of these controls are either duplicated of already existing controls or 
should be promoted
   from MFC-specific controls to generic controls. But without documentation we 
can't tell.

4) Since this is a new M2M driver I would expect it to use the v4l2-mem2mem 
framework, instead
   it appears to be all homegrown. The only reason the existing s5p-mfc driver 
doesn't use that
   framework is because it predates it.

5) It clearly doesn't follow the stateful encoder/decoder interface spec as 
detailed in the
   V4L2 spec:

   
https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/dev-mem2mem.html

   Even s5p-mfc does at least attempt to follow that (I'm not sure how complete 
support is,
   given that it's an old driver, but it tries).

6) Minor issue: don't set the .wait_prepare/finish vb2_ops callbacks: these 
callbacks are
   slated for removal. Instead set the vb2_queue lock field to the mutex 
&dev->mfc_mutex.

There are probably many other things that need addressing, but I stopped there.

I would need convincing (and based on his reply, Krzysztof as well) that a new 
driver is the
right approach. If it is, then start small: just the decoder supporting just 
one codec.
Exclude all the fancy features, just start with the basics. The recently merged 
Qualcomm
iris driver can serve as inspiration: drivers/media/platform/qcom/iris/

I'll mark this series as Rejected in patchwork. It's always a very bad idea to 
try to upstream
a huge driver with zillions of features. If, like this one, it has serious 
design issues, then
you did a lot of work for nothing :-(

Apologies for the bad news,

Regards,

        Hans

> 
> 
> Current driver vs targetted driver comparison
> +++++++++++++++++++++++++++++++++++++++++++++
> 
> Target Feature 
> Current mainline : Simple V4L2/ VB2 interface based encoder/ decoder
> New proposed driver: Dual core support, NAL-q support, LLC, Coredump, 
> performance
> measurement, plugin driver architecture, hardware manager(meerkat), DRM/OTF,
> Resource manager, bandwidth compression, New Pixel formats (NV12N,  YV12, 
> P010,
> SBWC,  RGB family), 10 bit support, HDR support, Prio and RR Process 
> Scheduler support, 
> 
> Register map  
> Current mainline : Fixed register layout, compatible across s5p variants
> New proposed driver: Completely redesigned register set, incompatible with 
> old layout
> 
> Command model
> Current mainline : Mailbox-style command queue
> New proposed driver: Ring-buffer command queue with different signaling
> 
> Memory management
> Current mainline : CMA-backed contiguous buffers, ION legacy
> New proposed driver: dma-heap / scatter-gather buffers with strict alignment 
> rules
> 
> Firmware
> Current mainline : Supports up to Firmware v12, Legacy binary format, loaded 
> via shared mechanism     
> New proposed driver: New firmware format, different protocol for control 
> messages. Support Firmware v13 onwards
> 
> DT bindings   
> Current mainline : Single clock + reset, simple PM    
> New proposed driver: Multiple clocks, reset domains, runtime PM dependencies
> 
> Error handling        
> Current mainline : Simple IRQ error flags
> New proposed driver: Detailed error codes, recovery sequences required
> 
> Code impact
> Current mainline :~8k LOC, minimal conditionals
> New proposed driver: ~65KLOC with full features
> 
> 
> Plan for supporting latest MFC (Best balance between code clarity and 
> long-term kernel health, while still respecting ABI and legacy users.)
> 1     We propose a new driver (exynos-mfc) for the latest generation of 
> Samsung MFC (Multi-Format Codec) hardware. 
> 2     MFC FW V6 to V12 will be supported by existing mainline (s5p-mfc) and 
> later versions by Exynos-mfc driver
> 3     The existing MFC driver will remain available for older SoC’s and will 
> not be broken. The new driver will only bind
>       to new compatible strings in DT, avoiding regressions for legacy 
> hardware. Samsung will take responsibility for 
>       maintaining both drivers until the old one can be formally marked as 
> “legacy.”
> 4     Keep it buildable for old SoCs but clearly say new SoCs should use the 
> new driver.
> 5     VIDEO_SAMSUNG_S5P_MFC and VIDEO_EXYNOS_MFC will both be supported for 
> an agreed time
> 6     Long term Support for new driver  - commitment from Samsung
> 7     The hardware has diverged to the point where conditionally supporting 
> both in one codebase is worse for long-term kernel health
> 8     Splitting keeps the code clean, testable, and reviewable, while 
> ensuring legacy users aren’t broken due to user space ABI  guarantees.
>       This seems more clean and easier approach Both can coexist without code 
> spaghetti.
> 
> Features and description supported
> ++++++++++++++++++++++++++++++++++
> 
> 1     MFC driver registration Probe functionality and video nodes registration
> 2     MFC driver open & close support MFC firmware loading,  "ls" & "cat" on 
> the video nodes  and udev rules on the video nodes
> 3     H264 decoding   H264 decoding support - HW supported profiles & levels 
> (YUV420 Semi-Planar, 8-bit, min/max resolution)
> 4     HEVC decoding   HEVC decoding support - HW supported profiles & levels 
> (YUV420 Semi-Planar, 8-bit, min/max resolution)
> 5     VP8 decoding    VP8 decoding support - HW supported profiles & levels 
> (YUV420 Semi-Planar, 8-bit, min/max resolution)
> 6     VP9 decoding    VP9 decoding support - HW supported profiles & levels 
> (YUV420 Semi-Planar, 8-bit, min/max resolution)
> 7     AV1 decoding    AV1 decoding support - HW supported profiles & levels 
> (YUV420 Semi-Planar, 8-bit, min/max resolution)
> 8     H264 encoding   H264 encoding support - Basic encoding (YUV420 
> Semi-Planar, 8-bit, min/max resolution)
> 9     HEVC encoding   HEVC encoding support - Basic encoding (YUV420 
> Semi-Planar, 8-bit, min/max resolution)
> 10    VP8 encoding    VP8 encoding support - Basic encoding (YUV420 
> Semi-Planar, 8-bit, min/max resolution)
> 11    VP9 encoding    VP9 encoding support - Basic encoding (YUV420 
> Semi-Planar, 8-bit, min/max resolution)
> 12    Debug fs support                Debug fs to control the MFC 
> functionalities
> 13    Debug log support               Debug logs to get on the dmesg prompt 
> for debug purpose
> 14    Debug SFR dump support          MFC SFR dumps during failures
> 15    NAL Queue mode control          NAL Queue mode support
> 16    Batch mode control              Batch mode support
> 17    HW supported YUV formats        YUV 420, 422, 444 (planar, semi-planar) 
> formats support as per MFC HW specification
> 18    HW supported RGB formats        RGB format support as per MFC HW 
> specification
> 19    Multi instance decoding/encoding        Multi instance decoding/encoding
> 20    Suspend and resume              Suspend and resume support
> 21    Runtime suspend and resume      Runtime suspend and resume support
> 22    Priority decoding (decoders)    Priority decoding support
> 23    Frame delay configuration (decoders)    Frame delay configuration 
> support
> 24    Error handling and conceal control (decoders)   HW/Platform limitations 
> should be gracefully exited and,
>                                                       bit stream errors 
> should be reported or concealed with warning indication
> 25    Reference frame control (encoders)      Reference frame control support
> 26    SPS/PPS control (encoders)      SPS/PPS control support
> 27    Loop filter control (encoders)  Loop filter control support
> 28    B frame support (encoders)      B frame support
> 29    GOP control (encoders)          GOP control support
> 30    Frame rate control (encoders)   Frame rate control support
> 
> Features to be  supported in 2026
> +++++++++++++++++++++++++++++++++
> 
> 31    Bit rate control (encoders)             Bit rate control support
> 32    QP control (encoders)                   QP control support
> 33    I/IDR control (encoders)                I/IDR control support
> 34    Scalable control (encoders)             Scalable control support
> 35    ROI control (encoders)                  ROI control support
> 36    Multi slice control (encoders)          Multi slice control support
> 37    Profile/Level control (encoders)        Profile/Level control support
> 38    Padding control (encoders)              Padding control support
> 39    Aspect ratio control (encoders)         Aspect ratio control support
> 40    Buffer full handling (encoders)         Buffer full handling support
> 41    SBWC                                    SBWC bandwidth compression 
> support
> 42    10-bit support                          10-bit support as per MFC HW 
> specification
> 43    Secure signed FW support                Security signed FW support
> 44    Secure playback                         DRM content playback for 
> decoders
> 45    HDR Support (decoders)                  HDR metadata support
> 46    Dynamic resolution change               Dynamic resolution change 
> support
> 
> Nagaraju Siddineni (29):
>   dt-bindings: media: mfc: Add Exynos MFC devicetree binding
>   arm64: dts: mfc: Add MFC device tree for Auto V920 SoC
>   media: mfc: Add MFC driver data structures and debugging macros
>   media: mfc: Add full register map and bit definitions for MFC hardware
>   media: mfc: Add MFC driver header files and core utilities
>   media: mfc: Add MFC core hardware register and debugfs APIs
>   media: mfc: Add MFC core command, hwlock, ISR and run functionalities
>   media: mfc: Add Exynos‑MFC driver probe support
>   media: mfc: Add bus‑devfreq, QoS, multi‑view and control
>     infrastructure
>   media: mfc: Add buffer‑queue and IOVMM support
>   media: mfc: Add rate‑calculation framework and memory utilities
>   media: mfc: Introduce QoS support and instance context handling
>   media: mfc: Add decoder core sync functions
>   media: mfc: Add buffer‑control framework
>   media: mfc: Add decoder resource‑management (RM) support and
>     load‑balancing
>   media: mfc: Enhance HW‑lock handling, scheduling and error recovery
>   media: mfc: Add VB2 decoder support
>   media: mfc: Add V4L2 decoder driver
>   media: mfc: Add QoS, Butler workqueue, and priority‑based scheduling
>   media: mfc: Add H264 decoder support
>   media: mfc: Add multi‑codec support & QoS improvements
>   media: mfc: Add H.264 encoder support with buffer and QoS improvements
>   media: mfc: Add encoder parameters, ROI & QoS support
>   media: mfc: Add encoder VB2 support to driver
>   media: mfc: Add encoder v4l2 driver interface
>   media: mfc: Add full encoder support
>   media: mfc: Add H.264 encoder support
>   media: mfc: Add AVC, VP8, VP9, and HEVC encoding support
>   media: mfc: Hardware‑accelerated encoding support
> 
>  .../bindings/media/samsung,exynos-mfc.yaml    |   77 +
>  MAINTAINERS                                   |   10 +
>  .../dts/exynos/exynosautov920-evt2-mfc.dtsi   |  630 +++
>  .../arm64/boot/dts/exynos/exynosautov920.dtsi |    1 +
>  drivers/media/platform/samsung/Kconfig        |    7 +
>  drivers/media/platform/samsung/Makefile       |    1 +
>  .../media/platform/samsung/exynos-mfc/Kconfig |   34 +
>  .../platform/samsung/exynos-mfc/Makefile      |   26 +
>  .../samsung/exynos-mfc/base/mfc_buf.c         |  765 +++
>  .../samsung/exynos-mfc/base/mfc_buf.h         |   43 +
>  .../samsung/exynos-mfc/base/mfc_common.h      |  444 ++
>  .../samsung/exynos-mfc/base/mfc_data_struct.h | 2014 +++++++
>  .../samsung/exynos-mfc/base/mfc_debug.h       |  247 +
>  .../samsung/exynos-mfc/base/mfc_format.h      |  316 ++
>  .../samsung/exynos-mfc/base/mfc_macros.h      |   95 +
>  .../samsung/exynos-mfc/base/mfc_media.h       |  554 ++
>  .../samsung/exynos-mfc/base/mfc_mem.c         |  995 ++++
>  .../samsung/exynos-mfc/base/mfc_mem.h         |  155 +
>  .../samsung/exynos-mfc/base/mfc_qos.c         | 1070 ++++
>  .../samsung/exynos-mfc/base/mfc_qos.h         |   99 +
>  .../samsung/exynos-mfc/base/mfc_queue.c       |  966 ++++
>  .../samsung/exynos-mfc/base/mfc_queue.h       |  158 +
>  .../exynos-mfc/base/mfc_rate_calculate.c      |  640 +++
>  .../exynos-mfc/base/mfc_rate_calculate.h      |  106 +
>  .../samsung/exynos-mfc/base/mfc_regs.h        |   58 +
>  .../samsung/exynos-mfc/base/mfc_regs_mfc.h    | 1002 ++++
>  .../samsung/exynos-mfc/base/mfc_sched.h       |   30 +
>  .../samsung/exynos-mfc/base/mfc_utils.c       |  401 ++
>  .../samsung/exynos-mfc/base/mfc_utils.h       |  481 ++
>  .../media/platform/samsung/exynos-mfc/mfc.c   | 1366 +++++
>  .../platform/samsung/exynos-mfc/mfc_core.c    |  703 +++
>  .../samsung/exynos-mfc/mfc_core_buf_ctrl.c    |  543 ++
>  .../samsung/exynos-mfc/mfc_core_cmd.c         |  576 ++
>  .../samsung/exynos-mfc/mfc_core_cmd.h         |   41 +
>  .../samsung/exynos-mfc/mfc_core_enc_param.c   | 1463 ++++++
>  .../samsung/exynos-mfc/mfc_core_enc_param.h   |   23 +
>  .../samsung/exynos-mfc/mfc_core_hw_reg_api.c  |  122 +
>  .../samsung/exynos-mfc/mfc_core_hw_reg_api.h  |  144 +
>  .../samsung/exynos-mfc/mfc_core_hwlock.c      |  761 +++
>  .../samsung/exynos-mfc/mfc_core_hwlock.h      |   80 +
>  .../samsung/exynos-mfc/mfc_core_intlock.c     |   98 +
>  .../samsung/exynos-mfc/mfc_core_intlock.h     |   20 +
>  .../samsung/exynos-mfc/mfc_core_isr.c         | 2104 ++++++++
>  .../samsung/exynos-mfc/mfc_core_isr.h         |   25 +
>  .../samsung/exynos-mfc/mfc_core_ops.c         |  813 +++
>  .../samsung/exynos-mfc/mfc_core_ops.h         |   16 +
>  .../platform/samsung/exynos-mfc/mfc_core_pm.c |  209 +
>  .../platform/samsung/exynos-mfc/mfc_core_pm.h |   33 +
>  .../samsung/exynos-mfc/mfc_core_reg_api.c     |  597 +++
>  .../samsung/exynos-mfc/mfc_core_reg_api.h     |  380 ++
>  .../samsung/exynos-mfc/mfc_core_run.c         |  541 ++
>  .../samsung/exynos-mfc/mfc_core_run.h         |   35 +
>  .../samsung/exynos-mfc/mfc_core_sched_prio.c  |  431 ++
>  .../samsung/exynos-mfc/mfc_core_sync.c        |  423 ++
>  .../samsung/exynos-mfc/mfc_core_sync.h        |   34 +
>  .../samsung/exynos-mfc/mfc_ctx_ctrl.c         | 1469 ++++++
>  .../platform/samsung/exynos-mfc/mfc_debugfs.c |  248 +
>  .../platform/samsung/exynos-mfc/mfc_debugfs.h |   20 +
>  .../samsung/exynos-mfc/mfc_dec_v4l2.c         | 1739 +++++++
>  .../samsung/exynos-mfc/mfc_dec_v4l2.h         |   20 +
>  .../platform/samsung/exynos-mfc/mfc_dec_vb2.c |  394 ++
>  .../platform/samsung/exynos-mfc/mfc_dec_vb2.h |   19 +
>  .../samsung/exynos-mfc/mfc_enc_v4l2.c         | 4614 +++++++++++++++++
>  .../samsung/exynos-mfc/mfc_enc_v4l2.h         |   20 +
>  .../platform/samsung/exynos-mfc/mfc_enc_vb2.c |  443 ++
>  .../platform/samsung/exynos-mfc/mfc_enc_vb2.h |   19 +
>  .../platform/samsung/exynos-mfc/mfc_rm.c      | 2652 ++++++++++
>  .../platform/samsung/exynos-mfc/mfc_rm.h      |  110 +
>  68 files changed, 34773 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/samsung,exynos-mfc.yaml
>  create mode 100644 arch/arm64/boot/dts/exynos/exynosautov920-evt2-mfc.dtsi
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/Kconfig
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/Makefile
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_buf.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_buf.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_common.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_data_struct.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_debug.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_format.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_macros.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_media.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_mem.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_mem.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_qos.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_qos.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_queue.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_queue.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_rate_calculate.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_rate_calculate.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_regs.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/base/mfc_regs_mfc.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_sched.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_utils.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/base/mfc_utils.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_buf_ctrl.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_cmd.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_cmd.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_enc_param.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_enc_param.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_hw_reg_api.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_hw_reg_api.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_hwlock.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_hwlock.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_intlock.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_intlock.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_isr.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_isr.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_ops.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_ops.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_pm.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_pm.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_reg_api.c
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_reg_api.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_run.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_run.h
>  create mode 100644 
> drivers/media/platform/samsung/exynos-mfc/mfc_core_sched_prio.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_sync.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_core_sync.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_ctx_ctrl.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_debugfs.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_debugfs.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_dec_v4l2.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_dec_v4l2.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_dec_vb2.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_dec_vb2.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_enc_v4l2.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_enc_v4l2.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_enc_vb2.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_enc_vb2.h
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_rm.c
>  create mode 100644 drivers/media/platform/samsung/exynos-mfc/mfc_rm.h
> 

Reply via email to