Re: [PATCH] input/touchscreen/sur40: use COLORSPACE_RAW
On Wed, Jun 26, 2019 at 11:52:16AM +0200, Hans Verkuil wrote:
> This driver set the colorspace to SRGB, but that makes no sense for
> a touchscreen. Use RAW instead. This also ensures consistency with the
> v4l_pix_format_touch() call that's used in v4l2-ioctl.c.
>
> Signed-off-by: Hans Verkuil
> ---
> Dmitry, do you want to take this, or shall I? I have no preference.
Please take it.
Acked-by: Dmitry Torokhov
> ---
> diff --git a/drivers/input/touchscreen/sur40.c
> b/drivers/input/touchscreen/sur40.c
> index 00cb1ba2d364..3fd3e862269b 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -186,7 +186,7 @@ static const struct v4l2_pix_format sur40_pix_format[] = {
> .width = SENSOR_RES_X / 2,
> .height = SENSOR_RES_Y / 2,
> .field = V4L2_FIELD_NONE,
> - .colorspace = V4L2_COLORSPACE_SRGB,
> + .colorspace = V4L2_COLORSPACE_RAW,
> .bytesperline = SENSOR_RES_X / 2,
> .sizeimage = (SENSOR_RES_X/2) * (SENSOR_RES_Y/2),
> },
> @@ -195,7 +195,7 @@ static const struct v4l2_pix_format sur40_pix_format[] = {
> .width = SENSOR_RES_X / 2,
> .height = SENSOR_RES_Y / 2,
> .field = V4L2_FIELD_NONE,
> - .colorspace = V4L2_COLORSPACE_SRGB,
> + .colorspace = V4L2_COLORSPACE_RAW,
> .bytesperline = SENSOR_RES_X / 2,
> .sizeimage = (SENSOR_RES_X/2) * (SENSOR_RES_Y/2),
> }
--
Dmitry
[ANN] Media summit in Lisbon at September
Hi all, We are organizing a media mini-summit in Lisbon to happen in September, at the same week as the Linux Plumber Conference and the Kernel Summit. We're still discussing the details about that. In principle, it will be a free event for the ones registered to Linux Plumbers Conference, happening between Sept 9-11. They have a room available that we could use for the meeting on that period of time, but we need to adjust to avoid conflicts with other interesting micro-conferences that will happen in parallel (or eventually do it outside that period, but that would be harder to organize). I'll let you know more details once we got it. If you plan to attend, please let me know. It is open for all, but we'll have a limited number of seats. So, the earliest we get the number of interested people, the best. - At the last summit, we were supposed to do a Key Signing Party, but we end not doing it, due to lack of time. I suggest we add this again, but doing it earlier, in order to avoid getting out of time for doing it. From my side, I'd like to discuss what criteria we should adopt for the code that comes via /drivers/staging/media, in particular: how much time should we keep a code there that doesn't receive any patch addressing the drivers real issues (excluding codepatch, cleanups and kAPI changes)? What other themes should be discussed? Thanks, Mauro
cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Mon Jul 1 05:00:11 CEST 2019 media-tree git hash:f81cbfc4f82a75ca0a2dc181a9c93b88f0e6509d media_build git hash: 0f25e6fb13b6bc345218800ad9ac863deb2ee9c8 v4l-utils git hash: b16f9e945d74aa552abdd6f873821cb77faaf13a edid-decode git hash: 15df4aebf06da579241c58949493b866139d0e2b gcc version:i686-linux-gcc (GCC) 8.3.0 sparse repo:https://git.linuxtv.org/mchehab/sparse.git sparse version: 0.6.1-rc1 smatch repo:https://git.linuxtv.org/mchehab/smatch.git smatch version: 0.5.1 build-scripts repo: https://git.linuxtv.org/hverkuil/build-scripts.git build-scripts git hash: 47a36aaa2c369059ec7edef9ab87bbcdafb4bb79 host hardware: x86_64 host os:4.19.0-4-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK Check for strcpy/strncpy/strlcpy: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.63-i686: OK linux-3.16.63-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.136-i686: OK linux-3.18.136-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.167-i686: OK linux-4.4.167-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.162-i686: OK linux-4.9.162-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.105-i686: OK linux-4.14.105-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.20-i686: OK linux-4.18.20-x86_64: OK linux-4.19.28-i686: OK linux-4.19.28-x86_64: OK linux-4.20.15-i686: OK linux-4.20.15-x86_64: OK linux-5.0.15-i686: OK linux-5.0.15-x86_64: OK linux-5.1.1-i686: OK linux-5.1.1-x86_64: OK linux-5.2-rc1-i686: OK linux-5.2-rc1-x86_64: OK apps: OK spec-git: OK virtme: OK: Final Summary: 2165, Succeeded: 2165, Failed: 0, Warnings: 0 sparse: OK smatch: OK Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.log Detailed regression test results are available here: http://www.xs4all.nl/~hverkuil/logs/Monday-test-media.log http://www.xs4all.nl/~hverkuil/logs/Monday-test-media-dmesg.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Monday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [RFC,v3 5/9] media: platform: Add Mediatek ISP P1 V4L2 control
Hi Jungo,
On Tue, Jun 11, 2019 at 11:53:40AM +0800, Jungo Lin wrote:
> Reserved Mediatek ISP P1 V4L2 control number with 16.
> Moreover, add two V4L2 controls for ISP P1 user space
> usage.
>
> 1. V4L2_CID_MTK_GET_BIN_INFO
> - Provide the image output width & height in case
> camera binning mode is enabled.
Could you explain with a bit more details what these binned width and height
would mean? How would they relate to the video CAPTURE node width and height?
Isn't this something that should be rather exposed as an appropriate
selection rectangle, instead of custom controls?
>
> 2. V4L2_CID_MTK_RAW_PATH
> - Export the path control of the main stream to user space.
> One is pure raw and the other is processing raw.
> The default value is 0 which outputs the pure raw bayer image
> from sesnor, without image processing in ISP HW.
Is it just effectively a full processing bypass? The driver seems to only
update the related configuration when the streaming starts. Can it be
controlled per-frame?
Generally this sounds more like something that should be modelled using the
media topology, similar to the example below.
/\ /---\ /--\
||---| | | |
| Capture Subdev | | Processing Subdev |-o-| CAPTURE node |
||-\ | | | | |
\/ | \---/ | \--/
| |
\---/
Then the userspace can select whether it wants the image from the capture
interface directly or procesed by the ISP by configuring the media links
appropriately.
The current limitation of this model is that it can't be easily configured
per-frame, as media configurations are not included in the requests yet.
[snip]
> +static int handle_ctrl_get_bin_info(struct v4l2_ctrl *ctrl, int is_width)
> +{
> + struct mtk_cam_dev *cam_dev = ctrl->priv;
> + struct v4l2_format *fmt;
> +
> + fmt = &cam_dev->vdev_nodes[MTK_CAM_P1_MAIN_STREAM_OUT].vdev_fmt;
> +
> + dev_dbg(&cam_dev->pdev->dev, "Get bin info w*h:%d*%d is_width:%d",
> + fmt->fmt.pix_mp.width, fmt->fmt.pix_mp.height, is_width);
> +
> + if (is_width)
> + ctrl->val = fmt->fmt.pix_mp.width;
> + else
> + ctrl->val = fmt->fmt.pix_mp.height;
This seems to contradict to what the comment in the header says, because it just
always returns the video node format and doesn't seem to care about whether
binning is enabled or not.
> +
> + return 0;
> +}
> +
> +static int handle_ctrl_get_process_raw(struct v4l2_ctrl *ctrl)
> +{
> + struct mtk_cam_dev *cam_dev = ctrl->priv;
> + struct isp_p1_device *p1_dev = get_p1_device(&cam_dev->pdev->dev);
> +
> + ctrl->val = (p1_dev->isp_ctx.isp_raw_path == ISP_PROCESS_RAW_PATH);
> +
> + dev_dbg(&cam_dev->pdev->dev, "Get process raw:%d", ctrl->val);
> +
> + return 0;
> +}
> +
> +static int handle_ctrl_set_process_raw(struct v4l2_ctrl *ctrl)
> +{
> + struct mtk_cam_dev *cam_dev = ctrl->priv;
> + struct isp_p1_device *p1_dev = get_p1_device(&cam_dev->pdev->dev);
> +
> + p1_dev->isp_ctx.isp_raw_path = (ctrl->val) ?
> + ISP_PROCESS_RAW_PATH : ISP_PURE_RAW_PATH;
> + dev_dbg(&cam_dev->pdev->dev, "Set process raw:%d", ctrl->val);
> + return 0;
> +}
> +
> +static int mtk_cam_dev_g_ctrl(struct v4l2_ctrl *ctrl)
This is g_volatile_ctrl not, g_ctrl.
> +{
> + switch (ctrl->id) {
> + case V4L2_CID_MTK_PROCESSING_RAW:
> + handle_ctrl_get_process_raw(ctrl);
> + break;
No need to provide getters for non-volatile controls. The
framework manages them.
> + case V4L2_CID_MTK_GET_BIN_WIDTH:
> + handle_ctrl_get_bin_info(ctrl, 1);
> + break;
> + case V4L2_CID_MTK_GET_BIN_HEIGTH:
> + handle_ctrl_get_bin_info(ctrl, 0);
It's trivial to get the value, so there isn't much benefit in having a
function to do so, especially if one needs something like a is_width
argument that further complicates the code.
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int mtk_cam_dev_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + switch (ctrl->id) {
> + case V4L2_CID_MTK_PROCESSING_RAW:
> + return handle_ctrl_set_process_raw(ctrl);
Same as above. The operation is too trivial to deserve a function.
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct v4l2_ctrl_ops mtk_cam_dev_ctrl_ops = {
> + .g_volatile_ctrl = mtk_cam_dev_g_ctrl,
> + .s_ctrl = mtk_cam_dev_s_ctrl,
> +};
> +
> +struct v4l2_ctrl_config mtk_cam_controls[] = {
> + {
> + .ops = &mtk_cam_dev_ctrl_ops,
> + .id = V4L2_CID_MTK_PROCESSING_RAW,
> + .name = "MTK CAM PROCESSING RAW",
> + .type = V4L2_CTRL_TYPE_BOOLEAN,
> + .min = 0,
> + .max = 1,
> + .step = 1,
> +
Re: [PATCH] media: imx: mipi csi-2: Don't fail if initial state times-out
On Thu, 2019-06-27 at 15:12 -0700, Steve Longerbeam wrote: > > On 6/27/19 5:56 AM, Philipp Zabel wrote: > > Hi Fabio, > > > > On Thu, 2019-06-27 at 09:38 -0300, Fabio Estevam wrote: > > > Hi Philipp, > > > > > > On Thu, Jun 27, 2019 at 5:43 AM Philipp Zabel > > > wrote: > > > > > > > Are there any visual artifacts in the first frame(s) in this case? > > > > > > I do not observe visual artifacts when running gst-launch-1.0 v4l2src ! > > > kmssink > > > > > > > > So in my opinion the next version of this patch should make LP-11 > > > > > timeout a warning only, but keep the error return on clock lane > > > > > timeouts. > > > > > > > > I agree. > > > > > > Here is a reworked version of Ezequiel's patch as per the suggestions: > > > http://code.bulix.org/g5qap5-780475 > > > > > > Does this one look good? > > > > Limiting the change to wait_stopstate is fine, the actual message > > makes assumptions that could be misleading. How about: > > > > "Timeout waiting for LP-11 state on all active lanes. > > This is most likely caused by a bug in the sensor driver. > > Capture might fail or contain visual artifacts." > > Yes I agree that is more descriptive, if a bit wordy for a kernel error > message. Haha, yes, I remember that thought crossing my mind. > I think it could be reduced, something like: > > "LP-11 wait timeout on all lanes, likely a sensor driver bug, expect > capture failures." Much better, thanks. regards Philipp
Re: [PATCH v3] media: imx: mipi csi-2: Don't fail if initial state times-out
On Thu, 2019-06-27 at 19:29 -0300, Fabio Estevam wrote: > From: Ezequiel Garcia > > Not all sensors will be able to guarantee a proper initial state. > This may be either because the driver is not properly written, > or (probably unlikely) because the hardware won't support it. > > While the right solution in the former case is to fix the sensor > driver, the real world not always allows right solutions, due to lack > of available documentation and support on these sensors. > > Let's relax this requirement, and allow the driver to support stream start, > even if the sensor initial sequence wasn't the expected. > > Also improve the warning message to better explain the problem and provide > a hint that the sensor driver needs to be fixed. > > Signed-off-by: Ezequiel Garcia > Signed-off-by: Fabio Estevam Reviewed-by: Philipp Zabel thanks Philipp
