Re: [PATCH v1 10/10] uapi: linux: add mediatek isp_7x camsys user api

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Nov 06, 2024 at 03:04:37PM +0200, Laurent Pinchart wrote:
> On Mon, Oct 14, 2024 at 05:56:40AM +0000, CK Hu (胡俊光) wrote:
> > Hi, Shu-hsiang:
> > 
> > On Wed, 2024-10-09 at 19:15 +0800, Shu-hsiang Yang wrote:
> > > Add UAPI for MediaTek ISP platform, providing user-space
> > > interfaces for the new camsys driver.
> > > 
> > > Signed-off-by: Shu-hsiang Yang <Shu-hsiang.Yang@mediatek.com>
> > > ---
> > 
> > [snip]
> > 
> > > +
> > > +/* MTK ISP camsys controls */
> > > +#define V4L2_CID_MTK_CAM_USED_ENGINE_LIMIT	(V4L2_CID_USER_MTK_CAM_BASE + 1)
> > > +#define V4L2_CID_MTK_CAM_BIN_LIMIT		(V4L2_CID_USER_MTK_CAM_BASE + 2)
> > > +#define V4L2_CID_MTK_CAM_FRZ_LIMIT		(V4L2_CID_USER_MTK_CAM_BASE + 3)
> > > +#define V4L2_CID_MTK_CAM_RESOURCE_PLAN_POLICY	(V4L2_CID_USER_MTK_CAM_BASE + 4)
> > > +#define V4L2_CID_MTK_CAM_USED_ENGINE		(V4L2_CID_USER_MTK_CAM_BASE + 5)
> > > +#define V4L2_CID_MTK_CAM_BIN			(V4L2_CID_USER_MTK_CAM_BASE + 6)
> > > +#define V4L2_CID_MTK_CAM_FRZ			(V4L2_CID_USER_MTK_CAM_BASE + 7)
> > > +#define V4L2_CID_MTK_CAM_USED_ENGINE_TRY	(V4L2_CID_USER_MTK_CAM_BASE + 8)
> > > +#define V4L2_CID_MTK_CAM_BIN_TRY		(V4L2_CID_USER_MTK_CAM_BASE + 9)
> > > +#define V4L2_CID_MTK_CAM_FRZ_TRY		(V4L2_CID_USER_MTK_CAM_BASE + 10)
> > > +#define V4L2_CID_MTK_CAM_PIXEL_RATE		(V4L2_CID_USER_MTK_CAM_BASE + 11)
> > > +#define V4L2_CID_MTK_CAM_FEATURE		(V4L2_CID_USER_MTK_CAM_BASE + 12)
> > > +#define V4L2_CID_MTK_CAM_SYNC_ID		(V4L2_CID_USER_MTK_CAM_BASE + 13)
> > > +#define V4L2_CID_MTK_CAM_RAW_PATH_SELECT	(V4L2_CID_USER_MTK_CAM_BASE + 14)
> > > +#define V4L2_CID_MTK_CAM_HSF_EN			(V4L2_CID_USER_MTK_CAM_BASE + 15)
> > > +#define V4L2_CID_MTK_CAM_PDE_INFO		(V4L2_CID_USER_MTK_CAM_BASE + 16)
> > > +#define V4L2_CID_MTK_CAM_MSTREAM_EXPOSURE	(V4L2_CID_USER_MTK_CAM_BASE + 17)
> > > +#define V4L2_CID_MTK_CAM_RAW_RESOURCE_CALC	(V4L2_CID_USER_MTK_CAM_BASE + 18)
> > > +#define V4L2_CID_MTK_CAM_TG_FLASH_CFG		(V4L2_CID_USER_MTK_CAM_BASE + 19)
> > > +#define V4L2_CID_MTK_CAM_RAW_RESOURCE_UPDATE	(V4L2_CID_USER_MTK_CAM_BASE + 20)
> > > +#define V4L2_CID_MTK_CAM_CAMSYS_HW_MODE		(V4L2_CID_USER_MTK_CAM_BASE + 21)
> > > +
> > 
> > Please give introduction of how to use these user space interface.
> 
> I'm very, very *not* thrilled by all this. It looks like a big pile of
> hacks really. Every single parameter used by those controls needs to be
> clearly documented, including explaining how they are used, in order for
> us to review the API. I suspect that many of the parameters should
> instead be handled through the ISP parameters buffers, or be controlled
> from standard V4L2 APIs.

While on the topic of documentation, we also need a high-level
architecture document. This patch series adds 30000 lines of code, it's
very hard for reviewers to understand the architecture of the driver
just by looking at the code. At the very least, a high-level description
of the hardware (at least from the view point of the interface the
firmware exposes) is needed.

-- 
Regards,

Laurent Pinchart