[v2,2/8] media: qcom: camss: Add subdev notify support

Message ID 20240320141136.26827-3-quic_depengs@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Hans Verkuil
Headers
Series media: qcom: camss: Add sm8550 support |

Commit Message

Depeng Shao March 20, 2024, 2:11 p.m. UTC
  From: Yongsheng Li <quic_yon@quicinc.com>

The buf done irq and register update register are moved
to CSID in SM8550, so but the write master configuration
in VFE, in case adapt existing code logic. So add buf
done and register related subdev event, and use the notify
interface in the v4l2_device structure to communicate
between CSID and VFE driver.

Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
---
 .../media/platform/qcom/camss/camss-csid.h    |  7 +++
 .../media/platform/qcom/camss/camss-csiphy.h  |  2 +
 drivers/media/platform/qcom/camss/camss-vfe.h |  2 +
 drivers/media/platform/qcom/camss/camss.c     | 50 +++++++++++++++++++
 drivers/media/platform/qcom/camss/camss.h     |  7 +++
 5 files changed, 68 insertions(+)
  

Comments

Bryan O'Donoghue March 20, 2024, 4:08 p.m. UTC | #1
On 20/03/2024 14:11, Depeng Shao wrote:
> From: Yongsheng Li <quic_yon@quicinc.com>
> 
> The buf done irq and register update register are moved
> to CSID in SM8550, so but the write master configuration
> in VFE, in case adapt existing code logic. So add buf
> done and register related subdev event, and use the notify
> interface in the v4l2_device structure to communicate
> between CSID and VFE driver.


Shouldn't it be possible to just have a function to write internally ?

You know the indexes of the CSID -> VFE connection.

The subdev notify is I think not the right fit for this purpose within 
our driver.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> index fddccb69da13..4a9e5a2d1f92 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.h
> +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> @@ -147,6 +147,13 @@ struct csid_hw_ops {
>   	 * @csid: CSID device
>   	 */
>   	void (*subdev_init)(struct csid_device *csid);
> +
> +	/*
> +	 * event - receive event from parent v4l2 device
> +	 * @csid: CSID device
> +	 */
> +	void (*event)(struct csid_device *csid,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct csid_device {
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
> index c9b7fe82b1f0..ffe1b95eea98 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.h
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
> @@ -61,6 +61,8 @@ struct csiphy_hw_ops {
>   	void (*lanes_disable)(struct csiphy_device *csiphy,
>   			      struct csiphy_config *cfg);
>   	irqreturn_t (*isr)(int irq, void *dev);
> +	void (*event)(struct csiphy_device *csiphy,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct csiphy_device {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
> index 0572c9b08e11..9919fe0ff101 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.h
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.h
> @@ -115,6 +115,8 @@ struct vfe_hw_ops {
>   	int (*vfe_halt)(struct vfe_device *vfe);
>   	void (*violation_read)(struct vfe_device *vfe);
>   	void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
> +	void (*event)(struct vfe_device *vfe,
> +			unsigned int evt_type, void *arg);
>   };
>   
>   struct vfe_isr_ops {
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 1923615f0eea..b57cd25bf6c7 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1904,6 +1904,55 @@ static void camss_genpd_cleanup(struct camss *camss)
>   	dev_pm_domain_detach(camss->genpd, true);
>   }
>   
> +static void camss_v4l2_subdev_notify(struct v4l2_subdev *sd,
> +			unsigned int cmd, void *arg)
> +{
> +	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
> +	struct camss *camss = to_camss(v4l2_dev);
> +	struct vfe_device *vfe;
> +	struct vfe_line *vfe_line;
> +	struct csid_device *csid;
> +	int evt_data = *(int *)arg;
> +
> +	if (camss->res->version != CAMSS_8550)
> +		return;
> +
> +	switch (cmd) {
> +	case NOTIFY_BUF_DONE:
> +		csid = v4l2_get_subdevdata(sd);
> +		vfe = &(camss->vfe[csid->id]);
> +		if (vfe->ops->event)
> +			vfe->ops->event(vfe,
> +				NOTIFY_BUF_DONE, (void *)&evt_data);
> +		break;
> +
> +	case NOTIFY_RUP:
> +		vfe_line = v4l2_get_subdevdata(sd);
> +		vfe = to_vfe(vfe_line);
> +		csid = &(camss->csid[vfe->id]);
> +
> +		if (csid->ops->event)
> +			csid->ops->event(csid,
> +				NOTIFY_RUP, (void *)&evt_data);
> +		break;
> +
> +	case NOTIFY_RUP_CLEAR:
> +		vfe_line = v4l2_get_subdevdata(sd);
> +		vfe = to_vfe(vfe_line);
> +		csid = &(camss->csid[vfe->id]);
> +
> +		if (csid->ops->event)
> +			csid->ops->event(csid,
> +				NOTIFY_RUP_CLEAR, (void *)&evt_data);
> +
> +		break;
> +
> +	default:
> +		dev_err(camss->dev, "Not supported evt type\n");
> +		break;
> +	}
> +}

I'm really not sure I see a good reason for this.

Why can't we just define calls between vfe and csid similar to

drivers/media/platform/qcom/camss/camss-csid.c:		ret = vfe_get(vfe);

---
bod
  
Gjorgji Rosikopulos March 20, 2024, 4:41 p.m. UTC | #2
Hi Bryan,

On 3/20/2024 6:08 PM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> From: Yongsheng Li <quic_yon@quicinc.com>
>>
>> The buf done irq and register update register are moved
>> to CSID in SM8550, so but the write master configuration
>> in VFE, in case adapt existing code logic. So add buf
>> done and register related subdev event, and use the notify
>> interface in the v4l2_device structure to communicate
>> between CSID and VFE driver.
> 
> 
> Shouldn't it be possible to just have a function to write internally ?
> 
> You know the indexes of the CSID -> VFE connection.
> 
> The subdev notify is I think not the right fit for this purpose within
> our driver.
> 

<snip>

> 
> I'm really not sure I see a good reason for this.
> 
> Why can't we just define calls between vfe and csid similar to
> 
> drivers/media/platform/qcom/camss/camss-csid.c:        ret = vfe_get(vfe);


Maybe we need to rethink and redesign this part of the driver.

In the initial version when this driver was introduced the CSID was
independent device from hw perspective,
and represented as separate sub-device.

With the newer architectures CSID was part of IFE hw (handled by the VFE
sub-device in this driver)
and vfe_get was introduced, but i believe it was not an issue because
the CISD still was kind of independent.

In the patch series:
"https://lore.kernel.org/lkml/20240319173935.481-4-quic_grosikop@quicinc.com/T/"

We try to decouple CSID from VFE and remove direct dependency
introducing parent_dev_ops,
where depending of the topology and parent device (VFE in this case or
other parent in future chipsets which contain CSID)
can reuse the code.

I am not sure if introducing parent_dev_ops is right way to go but we
can discuss further and see how to extend the driver
in proper way.

Just to add i am not saying that adding direct calls to VFE is not
proper but just close the door for other sub-device contain CSID to use
the code. :-)


Regards,
~Gjorgji

> 
> ---
> bod
>
  

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
index fddccb69da13..4a9e5a2d1f92 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.h
+++ b/drivers/media/platform/qcom/camss/camss-csid.h
@@ -147,6 +147,13 @@  struct csid_hw_ops {
 	 * @csid: CSID device
 	 */
 	void (*subdev_init)(struct csid_device *csid);
+
+	/*
+	 * event - receive event from parent v4l2 device
+	 * @csid: CSID device
+	 */
+	void (*event)(struct csid_device *csid,
+			unsigned int evt_type, void *arg);
 };
 
 struct csid_device {
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.h b/drivers/media/platform/qcom/camss/camss-csiphy.h
index c9b7fe82b1f0..ffe1b95eea98 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy.h
+++ b/drivers/media/platform/qcom/camss/camss-csiphy.h
@@ -61,6 +61,8 @@  struct csiphy_hw_ops {
 	void (*lanes_disable)(struct csiphy_device *csiphy,
 			      struct csiphy_config *cfg);
 	irqreturn_t (*isr)(int irq, void *dev);
+	void (*event)(struct csiphy_device *csiphy,
+			unsigned int evt_type, void *arg);
 };
 
 struct csiphy_device {
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.h b/drivers/media/platform/qcom/camss/camss-vfe.h
index 0572c9b08e11..9919fe0ff101 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.h
+++ b/drivers/media/platform/qcom/camss/camss-vfe.h
@@ -115,6 +115,8 @@  struct vfe_hw_ops {
 	int (*vfe_halt)(struct vfe_device *vfe);
 	void (*violation_read)(struct vfe_device *vfe);
 	void (*vfe_wm_stop)(struct vfe_device *vfe, u8 wm);
+	void (*event)(struct vfe_device *vfe,
+			unsigned int evt_type, void *arg);
 };
 
 struct vfe_isr_ops {
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1923615f0eea..b57cd25bf6c7 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1904,6 +1904,55 @@  static void camss_genpd_cleanup(struct camss *camss)
 	dev_pm_domain_detach(camss->genpd, true);
 }
 
+static void camss_v4l2_subdev_notify(struct v4l2_subdev *sd,
+			unsigned int cmd, void *arg)
+{
+	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
+	struct camss *camss = to_camss(v4l2_dev);
+	struct vfe_device *vfe;
+	struct vfe_line *vfe_line;
+	struct csid_device *csid;
+	int evt_data = *(int *)arg;
+
+	if (camss->res->version != CAMSS_8550)
+		return;
+
+	switch (cmd) {
+	case NOTIFY_BUF_DONE:
+		csid = v4l2_get_subdevdata(sd);
+		vfe = &(camss->vfe[csid->id]);
+		if (vfe->ops->event)
+			vfe->ops->event(vfe,
+				NOTIFY_BUF_DONE, (void *)&evt_data);
+		break;
+
+	case NOTIFY_RUP:
+		vfe_line = v4l2_get_subdevdata(sd);
+		vfe = to_vfe(vfe_line);
+		csid = &(camss->csid[vfe->id]);
+
+		if (csid->ops->event)
+			csid->ops->event(csid,
+				NOTIFY_RUP, (void *)&evt_data);
+		break;
+
+	case NOTIFY_RUP_CLEAR:
+		vfe_line = v4l2_get_subdevdata(sd);
+		vfe = to_vfe(vfe_line);
+		csid = &(camss->csid[vfe->id]);
+
+		if (csid->ops->event)
+			csid->ops->event(csid,
+				NOTIFY_RUP_CLEAR, (void *)&evt_data);
+
+		break;
+
+	default:
+		dev_err(camss->dev, "Not supported evt type\n");
+		break;
+	}
+}
+
 /*
  * camss_probe - Probe CAMSS platform device
  * @pdev: Pointer to CAMSS platform device
@@ -1974,6 +2023,7 @@  static int camss_probe(struct platform_device *pdev)
 	media_device_init(&camss->media_dev);
 
 	camss->v4l2_dev.mdev = &camss->media_dev;
+	camss->v4l2_dev.notify = camss_v4l2_subdev_notify;
 	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index 2f63206a8463..f1fe68dedd9e 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -86,6 +86,13 @@  enum icc_count {
 	ICC_SM8250_COUNT = 4,
 };
 
+enum subdev_notify_evt {
+	NOTIFY_BUF_DONE = 0,
+	NOTIFY_RUP,
+	NOTIFY_RUP_CLEAR,
+	NOTIFY_MAX,
+};
+
 struct camss_resources {
 	enum camss_version version;
 	const char *pd_name;