Re: [PATCH v5 4/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver v4l2 controls handler

On 26/01/2023 01:38, yuji2.ishikawa@toshiba.co.jp wrote:

quoted

quoted

+#define VISCONTI_VIIF_DPC_TABLE_SIZE 8192
+static int viif_l1_set_dpc(struct viif_device *viif_dev, struct viif_l1_dpc_config

*l1_dpc)

quoted

+{
+	uintptr_t table_h_paddr = 0;
+	uintptr_t table_m_paddr = 0;
+	uintptr_t table_l_paddr = 0;
+	unsigned long irqflags;
+	int ret;
+
+	if (l1_dpc->table_h_addr) {
+		if (copy_from_user(viif_dev->table_vaddr->dpc_table_h,
+				   u64_to_user_ptr(l1_dpc->table_h_addr),
+				   VISCONTI_VIIF_DPC_TABLE_SIZE))
+			return -EFAULT;

NACK!

I thought those addresses in a struct were iffy. This is not supported, it
basically bypasses the whole control framework.

I understand. 

quoted

The way to do this is to create separate array controls for these tables.
And table_h_addr becomes a simple 0 or 1 value, indicating whether to use
the table set by that control. For small arrays it is also an option to
embed them in the control structure.

As I wrote in reply for patch 2/6, I thought embedding is the only solution.
Thank you for giving another plan: adding controls for tables.
When I use individual controls for tables, are there some orderings between controls?
 -- such that control DPC_TABLE_{H,M,L} should be configured before SET_DPC

There is no ordering dependency. But you can cluster controls:

https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters

The idea is that userspace sets all the related controls with one VIDIOC_S_EXT_CTRLS
ioctl, and then for the clustered controls the s_ctrl callback is called only
once.

You can also check in try_ctrl if the controls in a cluster are sane. E.g.
if control A has value 1, and that requires that control B has a value >= 5,
then try_ctrl can verify that. Normally controls are independent from one
another, but clustering will link them together.

It's really what you want here. A good example is here: drivers/media/common/cx2341x.c
It's used by several PCI drivers that use this MPEG codec chipset, and it uses
clusters and also implements try_ctrl.

quoted

Are these l, h and m tables independent from one another? I.e. is it possible
to set l but not h and m? I suspect it is all or nothing, and in that case you
need only a single control to set all three tables (a two dimensional array).

These three tables can be setup individually.

quoted

Anyway, the same issue applies to all the controls were you pass addresses for
tables, that all needs to change.

All right. These controls must be fixed.

quoted

quoted

+		table_h_paddr =

(uintptr_t)viif_dev->table_paddr->dpc_table_h;

quoted

+	}
+	if (l1_dpc->table_m_addr) {
+		if (copy_from_user(viif_dev->table_vaddr->dpc_table_m,
+				   u64_to_user_ptr(l1_dpc->table_m_addr),
+				   VISCONTI_VIIF_DPC_TABLE_SIZE))
+			return -EFAULT;
+		table_m_paddr =

(uintptr_t)viif_dev->table_paddr->dpc_table_m;

quoted

+	}
+	if (l1_dpc->table_l_addr) {
+		if (copy_from_user(viif_dev->table_vaddr->dpc_table_l,
+				   u64_to_user_ptr(l1_dpc->table_l_addr),
+				   VISCONTI_VIIF_DPC_TABLE_SIZE))
+			return -EFAULT;
+		table_l_paddr = (uintptr_t)viif_dev->table_paddr->dpc_table_l;
+	}
+
+	spin_lock_irqsave(&viif_dev->lock, irqflags);
+	hwd_viif_isp_guard_start(viif_dev->hwd_res);
+	ret = hwd_viif_l1_set_dpc_table_transmission(viif_dev->hwd_res,

table_h_paddr,

quoted

+						     table_m_paddr,

table_l_paddr);

quoted

+	if (ret)
+		goto err;
+
+	ret = hwd_viif_l1_set_dpc(viif_dev->hwd_res, &l1_dpc->param_h,

&l1_dpc->param_m,

quoted

+				  &l1_dpc->param_l);
+
+err:
+	hwd_viif_isp_guard_end(viif_dev->hwd_res);
+	spin_unlock_irqrestore(&viif_dev->lock, irqflags);
+	return ret;
+}

<snip>

quoted

quoted

+static int visconti_viif_isp_get_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct viif_device *viif_dev = ctrl->priv;
+
+	pr_info("isp_get_ctrl: %s", ctrl->name);
+	if (pm_runtime_status_suspended(viif_dev->dev)) {
+		pr_info("warning: visconti viif HW is not powered");
+		return 0;
+	}
+
+	switch (ctrl->id) {
+	case V4L2_CID_VISCONTI_VIIF_CSI2RX_GET_CALIBRATION_STATUS:
+		return viif_csi2rx_get_calibration_status(viif_dev,

ctrl->p_new.p);

quoted

+	case V4L2_CID_VISCONTI_VIIF_CSI2RX_GET_ERR_STATUS:
+		return viif_csi2rx_get_err_status(viif_dev, ctrl->p_new.p);
+	case V4L2_CID_VISCONTI_VIIF_GET_LAST_CAPTURE_STATUS:
+		return viif_isp_get_last_capture_status(viif_dev,

ctrl->p_new.p);

quoted

+	case V4L2_CID_VISCONTI_VIIF_GET_REPORTED_ERRORS:
+		return viif_isp_get_reported_errors(viif_dev, ctrl->p_new.p);

My question for these four controls is: are these really volatile controls?
A volatile control means that the hardware can change the registers at any
time without telling the CPU about it via an interrupt or some similar
mechanism.

If there *is* such a mechanism, then it is not a volatile control, instead the
driver has to update the control value whenever the HW informs it about the
new value.

I can't tell, so that's why I ask here to double check.

I quickly checked HW and found ...

* CSI2RX_GET_CALIBRATION_STATUS: No interrupt mechanism

So that remains volatile.

* CSI2RX_GET_ERR_STATUS: An interrupt handler can be used
* GET_LAST_CAPTURE_STATUS: information can be updated at Vsync interrupt

For these two you can use v4l2_ctrl_s_ctrl to set the new value.
Note that this function takes a mutex, so you might not be able
to call it directly from the irq handler.

* GET_LAST_ERROR: An interrupt handler can be used

I'll try building control values while running interrupt services.
Do I have to do G_EXT_CTRLS followed by S_EXT_CTRLS if I want Read-To-Clear operation?
Currently, GET_LAST_ERROR control reports accumerated errors since last read.

Interesting use-case. I think this can stay a volatile control. Make sure
to document that reading this control will clear the values.

quoted

quoted

+	default:
+		pr_info("unknown_ctrl: id=%08X val=%d", ctrl->id, ctrl->val);
+		break;
+	}
+	return 0;
+}

Regards,

	Hans