On 26/01/2023 01:38, yuji2.ishikawa@xxxxxxxxxxxxx wrote:
>>> +#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)
>>> +{
>>> + 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.
>
>> 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.
>
>> 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.
>
>> 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.
>
>>> + table_h_paddr =
>> (uintptr_t)viif_dev->table_paddr->dpc_table_h;
>>> + }
>>> + 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;
>>> + }
>>> + 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,
>>> + table_m_paddr,
>> table_l_paddr);
>>> + if (ret)
>>> + goto err;
>>> +
>>> + ret = hwd_viif_l1_set_dpc(viif_dev->hwd_res, &l1_dpc->param_h,
>> &l1_dpc->param_m,
>>> + &l1_dpc->param_l);
>>> +
>>> +err:
>>> + hwd_viif_isp_guard_end(viif_dev->hwd_res);
>>> + spin_unlock_irqrestore(&viif_dev->lock, irqflags);
>>> + return ret;
>>> +}
<snip>
>>> +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);
>>> + 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);
>>> + 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.
>
>>> + default:
>>> + pr_info("unknown_ctrl: id=%08X val=%d", ctrl->id, ctrl->val);
>>> + break;
>>> + }
>>> + return 0;
>>> +}
Regards,
Hans
