On 2023/3/22 1:56, Laurent Pinchart wrote: > 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@xxxxxxxxxxxxxxxx> >> >> --- >> >> 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. > OK, will fix. Thank you for your comment. >> >> +{ >> >> + 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 ? used for debugging. It's not necessary. I will drop it. > >> >> + 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] >
