Re: [PATCH 8/9] media: camss: vfe: add the gen4 block and SM8750 camera subsystem

[Shuai Liu]

Hi Qianhui,

The gen4 write master programming itself matches the gen3 conventions
(the FRAME_INCR >> 8 formula, the 256-byte unit addressing, RDI_WM()
offsets), so only three things stand out:

1. The interconnect table does not match the binding

The binding in patch 1 and the DT in patch 9 declare four interconnect
paths for sm8750-camss:

  ahb, hf_mnoc, sf_mnoc, sf_icp_mnoc

but icc_res_sm8750[] only contains two entries (ahb, hf_mnoc), and
camss requests paths by the names in this table, so the sf_mnoc and
sf_icp_mnoc paths never receive any bandwidth vote.  For reference,
sm8650 keeps the binding and the driver in sync: both declare exactly
ahb and hf_mnoc.

Please either add the missing entries with real bandwidth numbers, or
reduce the binding and DTS to the two paths the driver actually votes
on.  As posted, the extra DT paths are dead weight and the bandwidth
they are supposed to guarantee is never requested.

2. The commit message miscounts the instances

The message says "two full and two lite CSIDs" and "two full and one
lite VFEs", but the binding, the DTS and this patch's own resource
tables all describe three full CSIDs (csid0/1/2) plus two lite
(csid_lite0/1), and three full VFEs (vfe0/1/2) plus two lite
(vfe_lite0/1).  Patch 1/9's message has the same numbers.  Please fix
the commit messages.

3. vfe_prepare_buffer() rejects formats at buffer-queue time

The 256-byte alignment of pix->plane_fmt[0].bytesperline * pix->height
is a property of the negotiated format, not of a particular buffer.
Format negotiation does not guarantee it: camss_format aligns
bytesperline per format (5 bytes for RAW10) and height is not aligned
at all, so plenty of resolutions pass try_fmt/s_fmt and then fail every
QBUF with -EINVAL and no explanation.

Suggest enforcing the constraint where the format is chosen: either
round bytesperline up to a 256-byte alignment for gen4 (which makes the
payload 256-aligned for any height), or reject unaligned formats in
try_fmt/s_fmt with a clear message, and keep only the per-buffer
address alignment check in prepare_buffer().

Thanks,
Shuai Liu

(This review was prepared with the assistance of an AI coding tool;
all findings were verified against the patched source.)