lkml.org 
[lkml]   [2023]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 6/6] media: starfive: Add Starfive Camera Subsystem driver
    From


    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@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.
    >

    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]
    >

    \
     
     \ /
      Last update: 2023-03-27 01:14    [W:4.455 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site