Re: [PATCH v2 6/6] media: starfive: Add Starfive Camera Subsystem driver

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

Hi Jack,

On Tue, Mar 21, 2023 at 08:56:34PM +0800, Jack Zhu wrote:
> On 2023/3/12 20:43, Laurent Pinchart wrote:
> > Hi Jack,
> > 
> > Thank you for the patch.
> > 
> > I'll do a partial review here as the patch is huge and I'm lacking time
> > at the moment.
> 
> Thank you for the review and your time.
> 
> > On Fri, Mar 10, 2023 at 08:05:53PM +0800, Jack Zhu wrote:
> >> Add the driver for Starfive Camera Subsystem found on
> >> Starfive JH7110 SoC. It is used for handing image sensor
> >> data.
> >> 
> >> Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com>
> >> ---
> >>  drivers/media/platform/Kconfig                |    1 +
> >>  drivers/media/platform/Makefile               |    1 +
> >>  drivers/media/platform/starfive/Kconfig       |   18 +
> >>  drivers/media/platform/starfive/Makefile      |   14 +
> >>  drivers/media/platform/starfive/stf_camss.c   |  728 +++++++++
> >>  drivers/media/platform/starfive/stf_camss.h   |  104 ++
> >>  drivers/media/platform/starfive/stf_common.h  |  149 ++
> >>  drivers/media/platform/starfive/stf_isp.c     | 1079 ++++++++++++++
> >>  drivers/media/platform/starfive/stf_isp.h     |  183 +++
> >>  .../media/platform/starfive/stf_isp_hw_ops.c  | 1286 ++++++++++++++++
> >>  drivers/media/platform/starfive/stf_video.c   | 1286 ++++++++++++++++
> >>  drivers/media/platform/starfive/stf_video.h   |   89 ++
> >>  drivers/media/platform/starfive/stf_vin.c     | 1314 +++++++++++++++++
> >>  drivers/media/platform/starfive/stf_vin.h     |  194 +++
> >>  .../media/platform/starfive/stf_vin_hw_ops.c  |  357 +++++
> >>  include/uapi/linux/stf_isp_ioctl.h            |  127 ++
> >>  16 files changed, 6930 insertions(+)
> >>  create mode 100644 drivers/media/platform/starfive/Kconfig
> >>  create mode 100644 drivers/media/platform/starfive/Makefile
> >>  create mode 100644 drivers/media/platform/starfive/stf_camss.c
> >>  create mode 100644 drivers/media/platform/starfive/stf_camss.h
> >>  create mode 100644 drivers/media/platform/starfive/stf_common.h
> >>  create mode 100644 drivers/media/platform/starfive/stf_isp.c
> >>  create mode 100644 drivers/media/platform/starfive/stf_isp.h
> >>  create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
> >>  create mode 100644 drivers/media/platform/starfive/stf_video.c
> >>  create mode 100644 drivers/media/platform/starfive/stf_video.h
> >>  create mode 100644 drivers/media/platform/starfive/stf_vin.c
> >>  create mode 100644 drivers/media/platform/starfive/stf_vin.h
> >>  create mode 100644 drivers/media/platform/starfive/stf_vin_hw_ops.c
> >>  create mode 100644 include/uapi/linux/stf_isp_ioctl.h

[snip]

> >> diff --git a/drivers/media/platform/starfive/stf_camss.c b/drivers/media/platform/starfive/stf_camss.c
> >> new file mode 100644
> >> index 000000000000..525f2d80c5eb
> >> --- /dev/null
> >> +++ b/drivers/media/platform/starfive/stf_camss.c
> >> @@ -0,0 +1,728 @@

[snip]

> >> +/*
> >> + * stfcamss_find_sensor - Find a linked media entity which represents a sensor
> >> + * @entity: Media entity to start searching from
> >> + *
> >> + * Return a pointer to sensor media entity or NULL if not found
> >> + */
> >> +struct media_entity *stfcamss_find_sensor(struct media_entity *entity)
> > 
> > From a camss point of view, the source is the csi2rx. You shouldn't
> > need to access the sensor directly, only the csi2rx. If you think you
> > have a need to access the sensor, please explain why and we can discuss
> > it.
> 
> need to use format and bpp of sensor to configure isp HW.

You can obtain the same information from the ISP sink pad:


+----------+       +------------+       +-----------+
|          |       |            |       |           | 
| Sensor [0| ----> |0] csi2rx [1| ----> |0]   ISP   |
|          |       |            |       |           |
+----------+       +------------+       +-----------+

(I'm not entirely sure if the csi2rx and ISP pad numbers are correct,
but that's the idea.)

The csi2rx can't modify the format (size and bpp), so the format on the
sensor's pad 0, csi2rx pad 0, csi2rx pad 1 and ISP pad 0 must be
identical.

In isp_sensor_fmt_to_index(), the ISP driver doesn't need to get the
format from the sensor, it can use the format on ISP pad 0 instead.

In video_enum_framesizes(), the ISP driver shouldn't look at the format
on the ISP input at all, it must enumerate all sizes that the video node
supports, regardless of what is connected to its input.

video_enum_frameintervals() should be dropped, the ISP itself has no
notion of frame interval. Userspace can query and configure the frame
rate from the sensor subdev directly.

In video_pipeline_s_fmt(), the ISP driver shouldn't look at the format
on the ISP input at all either. It must accept any format supported by
the ISP. It's only when starting streaming that the pipeline is
validated to make sure the formats configured on all subdevs match. I
recommend reading https://git.ideasonboard.org/doc/mc-v4l2.git for an
overview of how Media Controller-based drivers should behave. The
documentation describes how the API is meant to be used from userspace,
but the operating principles apply to driver development too.

video_subscribe_event() and video_unsubscribe_event() should also be
dropped, events from the sensor can be accessed by userspace on the
sensor subdev directly.

vin_set_stream() should call .s_stream() on the csi2rx, not the sensor.
The csi2rx .s_stream() handler will forward the call to the sensor.

> >> +{
> >> +	struct media_pad *pad;
> >> +
> >> +	while (1) {
> >> +		if (!entity->pads)
> >> +			return NULL;
> >> +
> >> +		pad = &entity->pads[0];
> >> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> >> +			return NULL;
> >> +
> >> +		pad = media_pad_remote_pad_first(pad);
> >> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> >> +			return NULL;
> >> +
> >> +		entity = pad->entity;
> >> +
> >> +		if (entity->function == MEDIA_ENT_F_CAM_SENSOR)
> >> +			return entity;
> >> +	}
> >> +}

[snip]

> >> diff --git a/include/uapi/linux/stf_isp_ioctl.h b/include/uapi/linux/stf_isp_ioctl.h
> >> new file mode 100644
> >> index 000000000000..3f302ef235d2
> >> --- /dev/null
> >> +++ b/include/uapi/linux/stf_isp_ioctl.h
> >> @@ -0,0 +1,127 @@

[snip]

> >> +enum _STF_ISP_IOCTL {
> > 
> > Device-specific ioctls are allowed, but they must all be clearly
> > documented.
> 
> OK, I will add annotations for these ioctls.
> 
> > 
> >> +	STF_ISP_IOCTL_LOAD_FW = BASE_VIDIOC_PRIVATE + 1,
> > 
> > Why can't you use the Linux kernel firmware API ?
> 
> The ioctl is used for loading config file, it is different from
> the Linux kernel firmware API. I will rename it.

Could you explain what the config file is used for ?

> >> +	STF_ISP_IOCTL_DMABUF_ALLOC,
> >> +	STF_ISP_IOCTL_DMABUF_FREE,
> >> +	STF_ISP_IOCTL_GET_HW_VER,
> > 
> > Not used, drop them.
> 
> OK, will drop them.
> 
> > 
> >> +	STF_ISP_IOCTL_REG,
> > 
> > Setting registers from userspace isn't allowed. No exception.
> 
> OK, will fix.
> 
> > 
> >> +	STF_ISP_IOCTL_SHADOW_LOCK,
> >> +	STF_ISP_IOCTL_SHADOW_UNLOCK,
> >> +	STF_ISP_IOCTL_SHADOW_UNLOCK_N_TRIGGER,
> >> +	STF_ISP_IOCTL_SET_USER_CONFIG_ISP,
> > 
> > I'm not sure what these ioctls do exactly as documentation is missing,
> > but I don't think they are the right API. Please describe the problem
> > you're trying to solve, and we'll find a good API.
> 
> These were used for debugging, I will drop them. Thanks.
> 
> >> +	STF_ISP_IOCTL_MAX
> >> +};

[snip]

-- 
Regards,

Laurent Pinchart