From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: yuji2.ishikawa@toshiba.co.jp, mchehab@kernel.org,
nobuhiro1.iwamatsu@toshiba.co.jp, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, rafael.j.wysocki@intel.com,
broonie@kernel.org, linux-media@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v5 4/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver v4l2 controls handler
Date: Thu, 26 Jan 2023 13:35:57 +0200 [thread overview]
Message-ID: <Y9JlnRKXofXl0s87@pendragon.ideasonboard.com> (raw)
In-Reply-To: <7d1a375a-c3d1-0f8e-4d69-10dedacf6974@xs4all.nl>
On Thu, Jan 26, 2023 at 09:39:59AM +0100, Hans Verkuil wrote:
> On 26/01/2023 01:38, yuji2.ishikawa@toshiba.co.jp 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.
I think controls are the wrong tool for this job though. The ISP
requires a large number of parameters, which would I think be better
suited passed as a parameters buffer like the ipu3 and rkisp1 driver do
for most of the data. Some parameters may still make sense as controls
(possibly mostly for the CSI2RX parameters), but I haven't checked that
in details.
> >> 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,
Laurent Pinchart
next prev parent reply other threads:[~2023-01-26 11:36 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-11 2:24 [PATCH v5 0/6] Add Toshiba Visconti Video Input Interface driver Yuji Ishikawa
2023-01-11 2:24 ` [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings Yuji Ishikawa
2023-01-11 9:19 ` Krzysztof Kozlowski
2023-01-11 12:48 ` yuji2.ishikawa
2023-01-17 15:26 ` Laurent Pinchart
2023-01-17 15:42 ` Krzysztof Kozlowski
2023-01-17 15:58 ` Laurent Pinchart
2023-01-17 17:01 ` Krzysztof Kozlowski
2023-01-22 19:25 ` Laurent Pinchart
2023-01-30 9:06 ` yuji2.ishikawa
2023-02-01 9:45 ` Laurent Pinchart
2023-02-01 11:24 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 2/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver Yuji Ishikawa
2023-01-11 15:30 ` kernel test robot
2023-01-11 22:55 ` kernel test robot
2023-01-17 10:01 ` Hans Verkuil
2023-01-25 12:12 ` yuji2.ishikawa
2023-01-17 22:39 ` Sakari Ailus
2023-02-01 2:02 ` yuji2.ishikawa
2023-02-01 9:41 ` Laurent Pinchart
2023-02-01 11:22 ` yuji2.ishikawa
2023-01-18 0:52 ` Laurent Pinchart
2023-02-02 4:37 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 3/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver user interace Yuji Ishikawa
2023-01-17 11:47 ` Hans Verkuil
2023-01-18 1:04 ` Laurent Pinchart
2023-01-25 10:20 ` yuji2.ishikawa
2023-01-26 20:49 ` Laurent Pinchart
2023-02-02 4:52 ` yuji2.ishikawa
2023-02-02 7:58 ` Laurent Pinchart
2023-01-26 1:25 ` yuji2.ishikawa
2023-01-26 8:01 ` Hans Verkuil
2023-01-27 12:47 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 4/6] media: platform: visconti: Add Toshiba Visconti Video Input Interface driver v4l2 controls handler Yuji Ishikawa
2023-01-17 11:19 ` Hans Verkuil
2023-01-26 0:38 ` yuji2.ishikawa
2023-01-26 8:39 ` Hans Verkuil
2023-01-26 11:35 ` Laurent Pinchart [this message]
2023-02-03 1:35 ` yuji2.ishikawa
2023-02-02 12:42 ` yuji2.ishikawa
2023-01-11 2:24 ` [PATCH v5 5/6] documentation: media: add documentation for Toshiba Visconti Video Input Interface driver Yuji Ishikawa
2023-01-11 2:24 ` [PATCH v5 6/6] MAINTAINERS: Add entries for Toshiba Visconti Video Input Interface Yuji Ishikawa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y9JlnRKXofXl0s87@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=hverkuil@xs4all.nl \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=nobuhiro1.iwamatsu@toshiba.co.jp \
--cc=rafael.j.wysocki@intel.com \
--cc=robh+dt@kernel.org \
--cc=yuji2.ishikawa@toshiba.co.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).