Re: [PATCH v3 2/4] media: synopsys: hdmirx: add HDMI audio capture support

[Igor Paunovic]

Hi Dmitry,

Thank you -- and thank you for reviewing the whole series and testing
2/4. It went through a lot of back-and-forth since the RFC, so having
it land with your Tested-by means a lot.

I'll prepare the follow-ups as separate patches once this series is in:

- The video_device lifetime issue on the V4L2 side (the devm-allocated
  struct with video_device_release_empty that the automated review
  flagged) -- as a standalone fix for the existing driver, since it's
  independent of the audio support.
- An atomicity fix for the GLOBAL_SWENABLE read-modify-write, now that
  the audio path adds a second writer to that register alongside the
  CEC code.

The repeated-suspend wedge I mentioned in the cover letter I'd rather
send only once I have a clean, reproducible trigger for it -- it's
still under investigation and I don't want to send a fix ahead of a
solid repro.

The dynamic codec registration you suggested (bind on the plug event,
read the actual wire sample rate and restrict the codec's rate) is the
direction I'd like to explore after these, once I've looked at what it
needs from hdmi_codec_ops.

Thanks again,
Igor


On Thu, Jul 23, 2026 at 10:56 PM Dmitry Osipenko
<dmitry.osipenko@xxxxxxxxxxxxx> wrote:
>
> On 7/21/26 08:31, Igor Paunovic wrote:
> > Hi Dmitry,
> >
> >> Re-arming will be best option if it works properly.
> >
> > Went back to it and made it work properly -- with one honest caveat
> > at the end.
> >
> > Plain cancel/re-arm could not have been enough as previously
> > written: hdmirx_resume() fully resets the controller via
> > reset_control_bulk_assert/deassert in hdmirx_enable(), wiping the
> > audio configuration (I2S_EN, AUDIO_ENABLE, FIFO thresholds), and
> > the worker's error path never re-programs those -- it only resyncs
> > the clock rate and reinits the FIFO. So v4 does:
> >
> > - suspend: cancel_delayed_work_sync() on the audio worker before
> > the clocks are gated (audio_streaming stays set);
> > - resume: if a capture stream is active, re-program the whole
> > audio path (clock from the last known sample rate, FIFO
> > thresholds, I2S_EN, AUDIO_ENABLE) and re-arm the worker.
> >
> > The programming is factored out of hw_params() into a shared
> > hdmirx_audio_setup() helper so resume and hw_params cannot drift.
> > I also checked that nothing in the post-resume hotplug re-init path
> > (plugout/plugin/submodule_init/controller_init) touches the audio
> > bits, so the restore survives the replug dance resume triggers.
> >
> > Measured on the Orange Pi 5 Plus (pm_test=devices, i.e. full device
> > suspend/resume callbacks, active capture stream with live audio
> > through the whole cycle): on resume the capture simply continues --
> > audio was flowing again the moment the display came back, no
> > userspace intervention, clean dmesg, no oops in any of the ~6
> > cycles exercised tonight.
> >
> > The caveat: a *second* suspend/resume cycle in the same boot leaves
> > the audio datapath silent (the I2S slave sees no bit clock, so
> > capture reads time out) even though the resume restore re-programs
> > everything, and a fresh stream open does not recover it -- only a
> > reboot does. Notably this happens with plain no-stream suspend
> > cycles as well, so it is not caused by (and would not be prevented
> > by) either the re-arm or the -EBUSY approach -- it looks like more
> > controller state lost on repeated reset that neither hw_params nor
> > resume currently covers, likely the same warm-up state machine I
> > described in the v3 testing note (audio needing a video STREAMON
> > after a replug). I am instrumenting the register state next to
> > root-cause it; I'd treat it as a follow-up rather than a v4
> > blocker, but happy to hear if you see it differently -- and curious
> > whether the 5B shows the same second-cycle behavior.
> >
> > v4 follows with the re-arm plus the dts and binding changes already
> > mentioned.
> >
> > One more small thing the audio bit exposes, for a possible
> > follow-up: hdmirx_cec_update_bits() is a non-atomic RMW (the read
> > and the write each take rst_lock separately), and with audio added
> > there are now two writers of GLOBAL_SWENABLE, so a CEC adapter
> > enable racing an audio enable/disable could in theory lose the
> > other side's bit. Mainline today has a single writer, so it is not
> > a bug there. Can send a small patch making it atomic if you think
> > it is worth it.
>
> Feel free to send the follow ups.
>
> --
> Best regards,
> Dmitry