Re: [PATCH] media: exynos4-is: Fix recursive locking in isp_video_release()
On 10/18/19 12:20, Seung-Woo Kim wrote: >>From isp_video_release(), &isp->video_lock is held and subsequent > vb2_fop_release() tries to lock vdev->lock which is same with the > previous one. Replace vb2_fop_release() with _vb2_fop_release() to > fix the recursive locking. > > Fixes: 1380f5754cb0 ("[media] videobuf2: Add missing lock held on > vb2_fop_release") > Signed-off-by: Seung-Woo Kim Reviewed-by: Sylwester Nawrocki
Re: [PATCH 4/7] media: ov9650: add a sanity check
On 8/22/19 21:39, Mauro Carvalho Chehab wrote: > As pointed by cppcheck: > > [drivers/media/i2c/ov9650.c:706]: (error) Shifting by a negative value > is undefined behaviour > [drivers/media/i2c/ov9650.c:707]: (error) Shifting by a negative value > is undefined behaviour > [drivers/media/i2c/ov9650.c:721]: (error) Shifting by a negative value > is undefined behaviour > > Prevent mangling with gains with invalid values > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/media/i2c/ov9650.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > index 8b56011446a9..cb49fda902fd 100644 > --- a/drivers/media/i2c/ov9650.c > +++ b/drivers/media/i2c/ov9650.c > @@ -703,6 +703,11 @@ static int ov965x_set_gain(struct ov965x *ov965x, int > auto_gain) > for (m = 6; m >= 0; m--) > if (gain >= (1 << m) * 16) > break; > + > + /* Sanity check: don't adjust the gain with a negative val */ s/val/value ? > + if (m < 0) > + return -EINVAL; This will never happen as min value of V4L2_CID_GAIN control is 16 (gain is always >= 16 and m is always >= 0). But if it suppresses the warning I'm fine with it. Reviewed-by: Sylwester Nawrocki
Re: [PATCH v2 5/7] media: use the BIT() macro
On 8/23/19 11:47, Mauro Carvalho Chehab wrote: > There are lots of places where we're doing 1 << 31. That's bad, > as, depending on the architecture, this has an undefined behavior. > > The BIT() macro is already prepared to handle this, so, let's > just switch all "1 << number" macros by BIT(number) at the header files > with has 1 << 31. > > Signed-off-by: Mauro Carvalho Chehab > v2: > As suggested by Laurent: > - Don't touch multi-bit masks > - remove explicit casts For: drivers/media/platform/exynos4-is/fimc-lite-reg.h drivers/media/platform/exynos4-is/fimc-reg.h drivers/media/platform/s3c-camif/camif-regs.h Reviewed-by: Sylwester Nawrocki
Re: [PATCH v6 2/2] media: v4l2: Get rid of ->vidioc_enum_fmt_vid_{cap,out}_mplane
On 6/4/19 09:06, Boris Brezillon wrote: > Support for multiplanar and singleplanar formats is mutually exclusive, > at least in practice. In our attempt to unify support for support for > mplane and !mplane in v4l, let's get rid of the > ->vidioc_enum_fmt_{vid,out}_cap_mplane() hooks and call > ->vidioc_enum_fmt_{vid,out}_cap() instead. > > Signed-off-by: Boris Brezillon For: drivers/media/platform/exynos-gsc drivers/media/platform/exynos4-is drivers/media/platform/s5p-mfc Reviewed-by: Sylwester Nawrocki -- Regards, Sylwester
Re: [PATCH v6 1/2] media: v4l2: Make sure all drivers set _MPLANE caps in vdev->device_caps
On 6/4/19 09:06, Boris Brezillon wrote: > This is needed if we want the core to be able to check _MPLANE support > without having to call the ->vdioc_querycap() hook. > > Signed-off-by: Boris Brezillon For: drivers/media/platform/exynos-gsc drivers/media/platform/exynos4-is drivers/media/platform/s5p-mfc Reviewed-by: Sylwester Nawrocki -- Thanks, Sylwester
Re: [PATCH v3 1/2] media: exynos4-is: Properly report _MPLANE caps
Hi Laurent, On 4/19/19 00:35, Laurent Pinchart wrote: > As this driver is quite old I assume there's little interest in > investing in it, but is there any chance Samsung would be interested in > contributing to libcamera for newer generations ? :-) I'm not aware of such plans at the moment, I assume the prerequisite would be mainline drivers for the camera subsystem which in turn requires whole SoC support in the mainline tree. And there have been quite a few newer Exynos SoC released for which I haven't seen efforts of adding mainline support. Of course things may change any time and I may not know everything so nothing is impossible. -- Regards, Sylwester
Re: [PATCH v4 1/2] media: v4l2: Make sure all drivers set _MPLANE caps in vdev->device_caps
On 4/16/19 14:03, Boris Brezillon wrote: > This is needed if we want the core to be able to check _MPLANE support > without having to call the ->vdioc_querycap() hook. > > Signed-off-by: Boris Brezillon > --- > Changes in v4: > - Add a hack in fimc-lite and fimc-isp-video ->querycap() > implementation to avoid reporting _MPLANE caps as userspace is not > ready for that > diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c > b/drivers/media/platform/exynos4-is/fimc-isp-video.c > index bb35a2017f21..0fb474b608ba 100644 > --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c > +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c > @@ -349,7 +349,15 @@ static int isp_video_querycap(struct file *file, void > *priv, > { > struct fimc_isp *isp = video_drvdata(file); > > - __fimc_vidioc_querycap(&isp->pdev->dev, cap, V4L2_CAP_STREAMING); > + __fimc_vidioc_querycap(&isp->pdev->dev, cap); > + > + /* > + * FIXME: Userspace does not expect V4L2_CAP_VIDEO_CAPTURE_MPLANE to > + * be set when calling ioctl(QUERYCAP) but we need to set this bit > + * in vdev->device_caps to let the v4l2 core do some consistency check. > + * Let's clear it here until we find a better solution. > + */ > + cap->device_caps &= ~V4L2_CAP_VIDEO_CAPTURE_MPLANE; If the common convention is that we set these caps then I don't see a reason why we couldn't drop this hack, and in fimc-lite as well. The chances that leaving these caps raised are not greater than zero on systems where this driver could be used I'd say. Otherwise the patches look good to me. Thanks, Sylwester
Re: [PATCH v3 1/2] media: exynos4-is: Properly report _MPLANE caps
On 4/12/19 12:32, Hans Verkuil wrote: > On 4/12/19 12:16 PM, Sylwester Nawrocki wrote: >> On 4/12/19 11:20, Boris Brezillon wrote: >>> The fimc-isp-video.c and fimc-lite.c were missing the >>> V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. >> >> Th omission was intentional, as these video nodes cannot be used >> by "standard" V4L2 capture applications. Some sub-devices need to >> be configured first in order to use these video nodes. It was >> agreed to not set these capabilities in the driver, instead user >> space library, e.g. libv4l2 with driver-specific plugin handling >> the media controller and v4l2 subdev calls would set the capability >> for the applications. Has something changed? > > I remember that this was discussed, but nobody actually followed that > guideline. Just do: > > git grep -l V4L2_CAP_VIDEO_CAPTURE drivers/media/platform > > It really doesn't make sense to leave it out since userspace has to know if > it is single or multiplanar, and if you don't set this capability, then how > can it tell? Presumably you can't by only querying the video device, this would require MC and driver-specific user space. > I always thought we need a separate CAP or some other way of signaling this, > but the few times I proposed something it was always shot down. > > So to be consistent with the other MC drivers you should set this MPLANE cap > as well in the fimc driver. I'm not against the $subject patch, rather the opposite. Especially if it is a part of reasonable API updates. But removing the capabilities was actually a requirement to get the driver merged, not a guideline. And was rather painful then, now the driver is probably not in use any more so I don't really mind. -- Regards, Sylwester
Re: [PATCH v3 1/2] media: exynos4-is: Properly report _MPLANE caps
Hi, On 4/12/19 11:20, Boris Brezillon wrote: > The fimc-isp-video.c and fimc-lite.c were missing the > V4L2_CAP_VIDEO_CAPTURE_MPLANE flag when reporting device caps. Th omission was intentional, as these video nodes cannot be used by "standard" V4L2 capture applications. Some sub-devices need to be configured first in order to use these video nodes. It was agreed to not set these capabilities in the driver, instead user space library, e.g. libv4l2 with driver-specific plugin handling the media controller and v4l2 subdev calls would set the capability for the applications. Has something changed? -- Thanks, Sylwester
Re: [PATCH] media: s5p-jpeg: Correct step and max values for V4L2_CID_JPEG_RESTART_INTERVAL
On 1/9/19 19:00, Paweł Chmiel wrote: > This commit corrects max and step values for v4l2 control for > V4L2_CID_JPEG_RESTART_INTERVAL. Max should be 0x and step should be 1. > It was found by using v4l2-compliance tool and checking result of > VIDIOC_QUERY_EXT_CTRL/QUERYMENU test. > Previously it was complaining that step was bigger than difference > between max and min. > > Fixes: 15f4bc3b1f42 ("[media] s5p-jpeg: Add JPEG controls support") > Signed-off-by: Paweł Chmiel Reviewed-by: Sylwester Nawrocki
Re: [PATCH v2] media: Use of_node_name_eq for node name comparisons
On 12/6/18 20:35, Rob Herring wrote: > Convert string compares of DT node names to use of_node_name_eq helper > instead. This removes direct access to the node name pointer. > Reviewed-by: Laurent Pinchart > Reviewed-by: Benoit Parrot > Signed-off-by: Rob Herring > --- > v2: > - Also convert tabs to spaces between the 'if' and '(' Reviewed-by: Sylwester Nawrocki
Re: [PATCH] MAINTAINERS fixups
On 11/08/2018 02:23 PM, Hans Verkuil wrote: > Update file paths in MAINTAINERS. > > Signed-off-by: Hans Verkuil > Reported-by: Joe Perches Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
Hi Hans, On 11/05/2018 02:12 PM, Hans Verkuil wrote: > Thank you for the review. One question: have you also tested this with at > least > one of the affected drivers? > > I'd like to have at least one Tested-by line. I just tested it now - video playback on Exynos4210 Trats2 so it covers the s5p-mfc and exynos4-is (fimc-m2m) drivers. Well done, I couldn't see any breakage. You can add "Tested-by: Sylwester Nawrocki " to patches: 1, 2, 3, 7, 8, 10. -- Regards, Sylwester
Re: [RFC PATCH 10/11] v4l2-ioctl: remove unused vidioc_g/s_crop
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > Now that all drivers have dropped vidioc_g/s_crop we can remove > support for them in the V4L2 core. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 08/11] exynos4-is: convert g/s_crop to g/s_selection
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > Replace g/s_crop by g/s_selection and set the V4L2_FL_QUIRK_INVERTED_CROP > flag since this is one of the old drivers that predates the selection > API. Those old drivers allowed g_crop when it really shouldn't have since > g_crop returns a compose rectangle instead of a crop rectangle for the > CAPTURE stream, and vice versa for the OUTPUT stream. > > Also drop the now unused vidioc_cropcap. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 09/11] s5p-g2d: convert g/s_crop to g/s_selection
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > Replace g/s_crop by g/s_selection and set the V4L2_FL_QUIRK_INVERTED_CROP > flag since this is one of the old drivers that predates the selection > API. Those old drivers allowed g_crop when it really shouldn't have since > g_crop returns a compose rectangle instead of a crop rectangle for the > CAPTURE stream, and vice versa for the OUTPUT stream. > > Also drop the now unused vidioc_cropcap. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 07/11] s5p_mfc_dec.c: convert g_crop to g_selection
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > The g_crop really implemented composition for the CAPTURE stream. > > Replace g_crop by g_selection and set the V4L2_FL_QUIRK_INVERTED_CROP > flag since this is one of the old drivers that predates the selection > API. Those old drivers allowed g_crop when it really shouldn't have > since g_crop returns a compose rectangle instead of a crop rectangle. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 06/11] exynos-gsc: replace v4l2_crop by v4l2_selection
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > Replace the use of struct v4l2_crop by struct v4l2_selection. > Also drop the unused gsc_g_crop function. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 03/11] v4l2-ioctl: add QUIRK_INVERTED_CROP
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > Some old Samsung drivers use the legacy crop API incorrectly: > the crop and compose targets are swapped. Normally VIDIOC_G_CROP > will return the CROP rectangle of a CAPTURE stream and the COMPOSE > rectangle of an OUTPUT stream. > > The Samsung drivers do the opposite. Note that these drivers predate > the selection API. > > If this 'QUIRK' flag is set, then the v4l2-ioctl core will swap > the CROP and COMPOSE targets as well. > > That way backwards compatibility is ensured and we can convert the > Samsung drivers to the selection API. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 01/11] v4l2-ioctl: don't use CROP/COMPOSE_ACTIVE
On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > Drop the deprecated _ACTIVE part. > > Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
Hi Hans, On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: > > From: Hans Verkuil > > This patch series converts the last remaining drivers that use g/s_crop and > cropcap to g/s_selection. Thank you for this clean up! I remember attempting conversion of those remaining drivers to selection API long time ago but I didn't have a good idea then how to address that crop and compose target inversion mess so I abandoned that efforts then. -- Regards, Sylwester
Re: [RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sakari, On 09/16/2018 12:52 AM, Sakari Ailus wrote: > v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as > entities) to what is fairly certainly known to be unique in the system, > even if there were more devices of the same kind. > > These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set > the name to the name of the driver or the module while omitting the > device's I²C address and bus, leaving the devices with a static name and > effectively limiting the number of such devices in a media device to 1. > > Address this by using the name set by the V4L2 framework. > > Signed-off-by: Sakari Ailus > Reviewed-by: Akinobu Mita (ov9650) I'm not against this patch but please don't expect an ack from me as this patch breaks existing user space code, scripts using media-ctl, etc. will need to be updated after kernel upgrade. I'm mostly concerned about ov9650 as other drivers are likely only used by Samsung or are obsoleted. -- Thanks, Sylwester
Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
On 09/20/2018 06:49 PM, Grant Grundler wrote: > On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote: >> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen >> wrote: >>> +/* Digital gain control */ >>> +#define IMX208_DGTL_GAIN_MIN 0 >>> +#define IMX208_DGTL_GAIN_MAX 4096 >>> +#define IMX208_DGTL_GAIN_DEFAULT 0x100 >>> +#define IMX208_DGTL_GAIN_STEP 1 >>> +/* Initialize control handlers */ >>> +static int imx208_init_controls(struct imx208 *imx208) >>> +{ >> [snip] >>> + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, >>> V4L2_CID_DIGITAL_GAIN, >>> + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX, >>> + IMX208_DGTL_GAIN_STEP, >>> + IMX208_DGTL_GAIN_DEFAULT); >> >> We have a problem here. The sensor supports only a discrete range of >> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is >> fixed point). This makes it possible for the userspace to set values >> that are not allowed by the sensor specification and also leaves no >> way to enumerate the supported values. The driver could always adjust the value in set_ctrl callback so invalid settings are not allowed. I'm not sure if it's best approach but I once did something similar for the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits for fractional part. The driver reports values multiplied by 16. See ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1. Total Gain to Control Bit Correlation" in the OV9650 datasheet for details. The integer menu control just seemed not suitable for 2^10 values. Now the gain control has range 16...1984 out of which only 1024 values are valid. It might not be best approach for a GUI but at least the driver exposes mapping of all valid values, which could be enumerated with VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific user space code. >> I can see two solutions here: >> >> 1) Define the control range from 0 to 4 and treat it as an exponent of >> 2, so that the value for the sensor becomes (1 << val) * 256. >> (Suggested by Sakari offline.) >> >> This approach has the problem of losing the original unit (and scale) >> of the value. > > Exactly - will users be confused by this? If we have to explain it, > probably not the best choice. > >> >> 2) Use an integer menu control, which reports only the supported >> discrete values - {1, 2, 4, 8, 16}. >> >> With this approach, userspace can enumerate the real gain values, but >> we would either need to introduce a new control (e.g. >> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and >> register V4L2_CID_DIGITAL_GAIN as an integer menu. >> >> Any opinions or better ideas? > > My $0.02: leave the user UI alone - let users specify/select anything > in the range the normal API or UI allows. But have sensor specific > code map all values in that range to values the sensor supports. Users > will notice how it works when they play with it. One can "adjust" the > mapping so it "feels right". -- Regards, Sylwester
Re: [RFC 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sakari, On Thu, 13 Sep 2018 at 11:21, Sakari Ailus wrote: [...] > diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c > b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > index ce196b60f917..64212551524e 100644 > --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c > +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c > @@ -1683,7 +1683,7 @@ static int s5c73m3_probe(struct i2c_client *client, > v4l2_subdev_init(sd, &s5c73m3_subdev_ops); > sd->owner = client->dev.driver->owner; > v4l2_set_subdevdata(sd, state); > - strlcpy(sd->name, "S5C73M3", sizeof(sd->name)); > + v4l2_i2c_subdev_set_name(sd, client, NULL, NULL); > > sd->internal_ops = &s5c73m3_internal_ops; > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > @@ -1698,7 +1698,7 @@ static int s5c73m3_probe(struct i2c_client *client, > return ret; > > v4l2_i2c_subdev_init(oif_sd, client, &oif_subdev_ops); > - strcpy(oif_sd->name, "S5C73M3-OIF"); > + v4l2_i2c_subdev_set_name(sd, client, NULL, "-OIF"); I would suggest to change the "OIF-" prefix to lower case, to avoid something like "s5c73m3-OIF". IIRC client->name is derived from DT compatible string, which is in lower case. Otherwise looks OK to me. -- Thanks, Sylwester
Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort
On 07/06/2018 03:43 PM, Ezequiel Garcia wrote: Could you elaborate how the core ensures DMA operation is not in progress after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied? Well, .streamoff is handled by v4l2_m2m_streamoff, which guarantees that no job is running by calling v4l2_m2m_cancel_job. The call chain goes like this: vidioc_streamoff v4l2_m2m_ioctl_streamoff v4l2_m2m_streamoff v4l2_m2m_cancel_job wait_event(m2m_ctx->finished, ...); The wait_event() wakes up by v4l2_m2m_job_finish(), which is called by g2d_isr after marking the buffers as done. The reason why I haven't elaborated this in the commit log is because it's documented in .job_abort declaration. Indeed, you are right, job_abort implementation can be safely removed in this case. As it is it doesn't help to handle cases when the HW gets stuck and refuses to generate an interrupt. The rcar_jpu seems to be addressing such situation properly though. diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c index 66aa8cf1d048..e98708883413 100644 --- a/drivers/media/platform/s5p-g2d/g2d.c +++ b/drivers/media/platform/s5p-g2d/g2d.c @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file, void *prv, const struct v4l2_crop *c static void job_abort(void *prv) { - struct g2d_ctx *ctx = prv; - struct g2d_dev *dev = ctx->dev; - - if (dev->curr == NULL) /* No job currently running */ - return; - - wait_event_timeout(dev->irq_queue, - dev->curr == NULL, - msecs_to_jiffies(G2D_TIMEOUT)); I think after this patch there will be a potential race condition possible, we could have the hardware DMA and CPU writing to same buffer with sequence like: ... QBUF STREAMON STREAMOFF DQBUF CPU accessing the buffer <-- at this point G2D DMA could still be writing to an already dequeued buffer. Image processing can take few miliseconds, it should be fairly easy to trigger such a condition. I don't think this is the case, as I've explained above. This commit merely removes a redundant wait, as job_abort simply waits the interrupt handler to complete, and that is the purpose of v4l2_m2m_job_finish. It only makes sense to implement job_abort if you can actually stop the current DMA. If you can only wait for it to complete, then it's not needed. Agreed. The intention of this series is simply to make job_abort optional, and remove those drivers that implement job_abort as a wait-for- current-job. Sure, thanks for your effort. -- Regards, Sylwester
Re: [PATCH v2 2/3] s5p-g2d: Remove unrequired wait in .job_abort
Hi, On 07/04/2018 10:04 AM, Hans Verkuil wrote: > On 18/06/18 06:38, Ezequiel Garcia wrote: >> As per the documentation, job_abort is not required >> to wait until the current job finishes. It is redundant >> to do so, as the core will perform the wait operation. Could you elaborate how the core ensures DMA operation is not in progress after VIDIOC_STREAMOFF, VIDIOC_DQBUF with this patch applied? >> Remove the wait infrastructure completely. > > Sylwester, can you review this? Thanks for forwarding Hans! >> Cc: Kyungmin Park >> Cc: Kamil Debski >> Cc: Andrzej Hajda >> Signed-off-by: Ezequiel Garcia >> --- >> drivers/media/platform/s5p-g2d/g2d.c | 11 --- >> drivers/media/platform/s5p-g2d/g2d.h | 1 - >> 2 files changed, 12 deletions(-) >> >> diff --git a/drivers/media/platform/s5p-g2d/g2d.c >> b/drivers/media/platform/s5p-g2d/g2d.c >> index 66aa8cf1d048..e98708883413 100644 >> --- a/drivers/media/platform/s5p-g2d/g2d.c >> +++ b/drivers/media/platform/s5p-g2d/g2d.c >> @@ -483,15 +483,6 @@ static int vidioc_s_crop(struct file *file, void *prv, >> const struct v4l2_crop *c >> >> static void job_abort(void *prv) >> { >> -struct g2d_ctx *ctx = prv; >> -struct g2d_dev *dev = ctx->dev; >> - >> -if (dev->curr == NULL) /* No job currently running */ >> -return; >> - >> -wait_event_timeout(dev->irq_queue, >> - dev->curr == NULL, >> - msecs_to_jiffies(G2D_TIMEOUT)); I think after this patch there will be a potential race condition possible, we could have the hardware DMA and CPU writing to same buffer with sequence like: ... QBUF STREAMON STREAMOFF DQBUF CPU accessing the buffer <-- at this point G2D DMA could still be writing to an already dequeued buffer. Image processing can take few miliseconds, it should be fairly easy to trigger such a condition. Not saying about DMA being still in progress after device file handle is closed, but that's something that deserves a separate patch. It seems there is a bug in the driver as there is no call to v4l2_m2m_ctx_release()in the device fops release() callback. I think we could remove the s5p-g2d driver altogether as the functionality is covered by the exynos DRM IPP driver (drivers/gpu/drm/exynos). >> } >> >> static void device_run(void *prv) >> @@ -563,7 +554,6 @@ static irqreturn_t g2d_isr(int irq, void *prv) >> v4l2_m2m_job_finish(dev->m2m_dev, ctx->fh.m2m_ctx); >> >> dev->curr = NULL; >> -wake_up(&dev->irq_queue); >> return IRQ_HANDLED; >> } -- Regards, Sylwester
Re: [PATCH] media: s3c-camif: ignore -ENOIOCTLCMD from v4l2_subdev_call for s_power
On 06/10/2018 05:42 PM, Akinobu Mita wrote: When the subdevice doesn't provide s_power core ops callback, the v4l2_subdev_call for s_power returns -ENOIOCTLCMD. If the subdevice doesn't have the special handling for its power saving mode, the s_power isn't required. So -ENOIOCTLCMD from the v4l2_subdev_call should be ignored. Signed-off-by: Akinobu Mita Acked-by: Sylwester Nawrocki
[PATCH] s5p-mfc: Fix buffer look up in s5p_mfc_handle_frame_{new, copy_time} functions
Look up of buffers in s5p_mfc_handle_frame_new, s5p_mfc_handle_frame_copy_time functions is not working properly for DMA addresses above 2 GiB. As a result flags and timestamp of returned buffers are not set correctly and it breaks operation of GStreamer/OMX plugins which rely on the CAPTURE buffer queue flags. Due to improper return type of the get_dec_y_adr, get_dspl_y_adr callbacks and sign bit extension these callbacks return incorrect address values, e.g. 0xfefc instead of 0xfefc. Then the statement: "if (vb2_dma_contig_plane_dma_addr(&dst_buf->b->vb2_buf, 0) == dec_y_addr)" is always false, which breaks looking up capture queue buffers. To ensure proper matching by address u32 type is used for the DMA addresses. This should work on all related SoCs, since the MFC DMA address width is not larger than 32-bit. Changes done in this patch are minimal as there is a larger patch series pending refactoring the whole driver. Signed-off-by: Sylwester Nawrocki --- drivers/media/platform/s5p-mfc/s5p_mfc.c | 23 --- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c index a80251ed3143..780548dd650e 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c @@ -254,24 +254,24 @@ static void s5p_mfc_handle_frame_all_extracted(struct s5p_mfc_ctx *ctx) static void s5p_mfc_handle_frame_copy_time(struct s5p_mfc_ctx *ctx) { struct s5p_mfc_dev *dev = ctx->dev; - struct s5p_mfc_buf *dst_buf, *src_buf; - size_t dec_y_addr; + struct s5p_mfc_buf *dst_buf, *src_buf; + u32 dec_y_addr; unsigned int frame_type; /* Make sure we actually have a new frame before continuing. */ frame_type = s5p_mfc_hw_call(dev->mfc_ops, get_dec_frame_type, dev); if (frame_type == S5P_FIMV_DECODE_FRAME_SKIPPED) return; - dec_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dec_y_adr, dev); + dec_y_addr = (u32)s5p_mfc_hw_call(dev->mfc_ops, get_dec_y_adr, dev); /* Copy timestamp / timecode from decoded src to dst and set appropriate flags. */ src_buf = list_entry(ctx->src_queue.next, struct s5p_mfc_buf, list); list_for_each_entry(dst_buf, &ctx->dst_queue, list) { - if (vb2_dma_contig_plane_dma_addr(&dst_buf->b->vb2_buf, 0) - == dec_y_addr) { - dst_buf->b->timecode = - src_buf->b->timecode; + u32 addr = (u32)vb2_dma_contig_plane_dma_addr(&dst_buf->b->vb2_buf, 0); + + if (addr == dec_y_addr) { + dst_buf->b->timecode = src_buf->b->timecode; dst_buf->b->vb2_buf.timestamp = src_buf->b->vb2_buf.timestamp; dst_buf->b->flags &= @@ -307,10 +307,10 @@ static void s5p_mfc_handle_frame_new(struct s5p_mfc_ctx *ctx, unsigned int err) { struct s5p_mfc_dev *dev = ctx->dev; struct s5p_mfc_buf *dst_buf; - size_t dspl_y_addr; + u32 dspl_y_addr; unsigned int frame_type; - dspl_y_addr = s5p_mfc_hw_call(dev->mfc_ops, get_dspl_y_adr, dev); + dspl_y_addr = (u32)s5p_mfc_hw_call(dev->mfc_ops, get_dspl_y_adr, dev); if (IS_MFCV6_PLUS(dev)) frame_type = s5p_mfc_hw_call(dev->mfc_ops, get_disp_frame_type, ctx); @@ -329,9 +329,10 @@ static void s5p_mfc_handle_frame_new(struct s5p_mfc_ctx *ctx, unsigned int err) /* The MFC returns address of the buffer, now we have to * check which videobuf does it correspond to */ list_for_each_entry(dst_buf, &ctx->dst_queue, list) { + u32 addr = (u32)vb2_dma_contig_plane_dma_addr(&dst_buf->b->vb2_buf, 0); + /* Check if this is the buffer we're looking for */ - if (vb2_dma_contig_plane_dma_addr(&dst_buf->b->vb2_buf, 0) - == dspl_y_addr) { + if (addr == dspl_y_addr) { list_del(&dst_buf->list); ctx->dst_queue_cnt--; dst_buf->b->sequence = ctx->sequence; -- 2.14.2
Re: [PATCH 3/7] s5p-mfc: fix two sparse warnings
On 05/14/2018 03:13 PM, Hans Verkuil wrote: > From: Hans Verkuil > > media-git/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c: In function > 'vidioc_querycap': > media-git/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:1317:2: warning: > 'strncpy' output may be truncated copying 31 bytes from a string of length 31 > [-Wstringop-truncation] > strncpy(cap->card, dev->vfd_enc->name, sizeof(cap->card) - 1); > ^ > media-git/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c: In function > 'vidioc_querycap': > media-git/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:275:2: warning: > 'strncpy' output may be truncated copying 31 bytes from a string of length 31 > [-Wstringop-truncation] > strncpy(cap->card, dev->vfd_dec->name, sizeof(cap->card) - 1); > ^~~~~ > > Signed-off-by: Hans Verkuil Acked-by: Sylwester Nawrocki
Re: [PATCH 26/61] media: platform: exynos4-is: simplify getting .drvdata
On 04/19/2018 04:05 PM, Wolfram Sang wrote: > We should get drvdata from struct device directly. Going via > platform_device is an unneeded step back and forth. > > Signed-off-by: Wolfram Sang Acked-by: Sylwester Nawrocki
Re: [PATCH 27/61] media: platform: s5p-mfc: simplify getting .drvdata
On 04/19/2018 04:05 PM, Wolfram Sang wrote: > We should get drvdata from struct device directly. Going via > platform_device is an unneeded step back and forth. > > Signed-off-by: Wolfram Sang Acked-by: Sylwester Nawrocki
[PATCH] exynos4-is: Prevent NULL pointer dereference in __isp_video_try_fmt()
This patch fixes potential NULL pointer dereference as indicated by the following static checker warning: drivers/media/platform/exynos4-is/fimc-isp-video.c:408 isp_video_try_fmt_mplane() error: NULL dereference inside function '__isp_video_try_fmt(isp, &f->fmt.pix_mp, (0))()'. Fixes: 34947b8aebe3: ("[media] exynos4-is: Add the FIMC-IS ISP capture DMA driver") Reported-by: Dan Carpenter Signed-off-by: Sylwester Nawrocki --- drivers/media/platform/exynos4-is/fimc-isp-video.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c index 55ba696b8cf4..a920164f53f1 100644 --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c @@ -384,12 +384,17 @@ static void __isp_video_try_fmt(struct fimc_isp *isp, struct v4l2_pix_format_mplane *pixm, const struct fimc_fmt **fmt) { - *fmt = fimc_isp_find_format(&pixm->pixelformat, NULL, 2); + const struct fimc_fmt *__fmt; + + __fmt = fimc_isp_find_format(&pixm->pixelformat, NULL, 2); + + if (fmt) + *fmt = __fmt; pixm->colorspace = V4L2_COLORSPACE_SRGB; pixm->field = V4L2_FIELD_NONE; - pixm->num_planes = (*fmt)->memplanes; - pixm->pixelformat = (*fmt)->fourcc; + pixm->num_planes = __fmt->memplanes; + pixm->pixelformat = __fmt->fourcc; /* * TODO: double check with the docmentation these width/height * constraints are correct. -- 2.14.2
Re: [bug report] [media] exynos4-is: Add the FIMC-IS ISP capture DMA driver
On 05/03/2018 01:43 PM, Dan Carpenter wrote: > [ This code is five years old now. It's so weird to me that the warning > is showing up in my new warnings pile. Perhaps this wasn't included > in my allmodconfig before? - dan ] Might be, the bug found is real. The module is normally disabled anyway for other reasons. > Hello Sylwester Nawrocki, > > The patch 34947b8aebe3: "[media] exynos4-is: Add the FIMC-IS ISP > capture DMA driver" from Dec 20, 2013, leads to the following static > checker warning: > > drivers/media/platform/exynos4-is/fimc-isp-video.c:408 > isp_video_try_fmt_mplane() > error: NULL dereference inside function '__isp_video_try_fmt(isp, > &f->fmt.pix_mp, (0))()'. > > drivers/media/platform/exynos4-is/fimc-isp-video.c >383 static void __isp_video_try_fmt(struct fimc_isp *isp, >384 struct v4l2_pix_format_mplane *pixm, >385 const struct fimc_fmt **fmt) >386 { >387 *fmt = fimc_isp_find_format(&pixm->pixelformat, NULL, 2); > > Unchecked dereference. We're not allowed to pass a NULL "fmt". > >388 >389 pixm->colorspace = V4L2_COLORSPACE_SRGB; >390 pixm->field = V4L2_FIELD_NONE; >391 pixm->num_planes = (*fmt)->memplanes; >392 pixm->pixelformat = (*fmt)->fourcc; >393 /* >394 * TODO: double check with the docmentation these width/height >395 * constraints are correct. >396 */ >397 v4l_bound_align_image(&pixm->width, FIMC_ISP_SOURCE_WIDTH_MIN, >398FIMC_ISP_SOURCE_WIDTH_MAX, 3, >399&pixm->height, > FIMC_ISP_SOURCE_HEIGHT_MIN, >400FIMC_ISP_SOURCE_HEIGHT_MAX, 0, 0); >401 } >402 >403 static int isp_video_try_fmt_mplane(struct file *file, void *fh, >404 struct v4l2_format *f) >405 { >406 struct fimc_isp *isp = video_drvdata(file); >407 >408 __isp_video_try_fmt(isp, &f->fmt.pix_mp, NULL); > > And yet here we are. > >409 return 0; >410 } We may need something like: 8< diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c index 55ba696b8cf4..a920164f53f1 100644 --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c @@ -384,12 +384,17 @@ static void __isp_video_try_fmt(struct fimc_isp *isp, struct v4l2_pix_format_mplane *pixm, const struct fimc_fmt **fmt) { - *fmt = fimc_isp_find_format(&pixm->pixelformat, NULL, 2); + const struct fimc_fmt *__fmt; + + __fmt = fimc_isp_find_format(&pixm->pixelformat, NULL, 2); + + if (fmt) + *fmt = __fmt; pixm->colorspace = V4L2_COLORSPACE_SRGB; pixm->field = V4L2_FIELD_NONE; - pixm->num_planes = (*fmt)->memplanes; - pixm->pixelformat = (*fmt)->fourcc; + pixm->num_planes = __fmt->memplanes; + pixm->pixelformat = __fmt->fourcc; /* * TODO: double check with the docmentation these width/height * constraints are correct. 8< I will post the patch to clear the warning. -- Thanks, Sylwester
Re: [git:media_tree/master] media: include/media: fix missing | operator when setting cfg
Hi Mauro, On 05/05/2018 03:01 PM, Mauro Carvalho Chehab wrote: > This is an automatic generated email to let you know that the following patch > were queued: > > Subject: media: include/media: fix missing | operator when setting cfg I intended to submit this patch in a pull request, with subject line fixed as mentioned in comments to the submitted match: https://patchwork.linuxtv.org/patch/48784 It might not be a big deal but it's not the first time patch gets applied with review comments ignored. Then it looks like I acked something that I actually didn't. -- Regards, Sylwester
Re: [PATCH] [media] include/media: fix missing | operator when setting cfg
On 04/18/2018 05:24 PM, Colin Ian King wrote: > Oops, shall I re-send? There is no need to, thanks.
Re: [PATCH] [media] include/media: fix missing | operator when setting cfg
On 04/18/2018 05:20 PM, Sylwester Nawrocki wrote: > On 04/18/2018 05:06 PM, Colin King wrote: >> From: Colin Ian King >> >> The value from a readl is being masked with ITE_REG_CIOCAN_MASK however >> this is not being used and cfg is being re-assigned. I believe the >> assignment operator should actually be instead the |= operator. >> >> Detected by CoverityScan, CID#1467987 ("Unused value") >> >> Signed-off-by: Colin Ian King > Thanks for the patch. > > Acked-by: Sylwester Nawrocki I forgot to mention that the subject should rather looks something like: "exynos4-is: fimc-lite: : fix missing | operator when setting cfg" -- Regards, Sylwester
Re: [PATCH] [media] include/media: fix missing | operator when setting cfg
On 04/18/2018 05:06 PM, Colin King wrote: > From: Colin Ian King > > The value from a readl is being masked with ITE_REG_CIOCAN_MASK however > this is not being used and cfg is being re-assigned. I believe the > assignment operator should actually be instead the |= operator. > > Detected by CoverityScan, CID#1467987 ("Unused value") > > Signed-off-by: Colin Ian King Thanks for the patch. Acked-by: Sylwester Nawrocki
Re: [PATCH 07/16] media: exymos4-is: allow compile test for EXYNOS FIMC-LITE
On 04/05/2018 07:54 PM, Mauro Carvalho Chehab wrote: > There's nothing that prevents building this driver with > COMPILE_TEST. So, enable it. > > While here, make the Kconfig dependency cleaner by removing > the unneeded if block. > > Signed-off-by: Mauro Carvalho Chehab With s/exymos4-is/exynos4-is in the subject Acked-by: Sylwester Nawrocki -- Regards, Sylwester
[GIT PULL] HEVC V4L2 controls and s5p-mfc update
Hi Mauro, The following changes since commit 3f127ce11353fd1071cae9b65bc13add6aec6b90: media: em28xx-cards: fix em28xx_duplicate_dev() (2018-03-08 06:06:51 -0500) are available in the git repository at: git://linuxtv.org/snawrocki/samsung.git tags/v4.17-media-samsung for you to fetch changes up to 8e8ae9fe34ac0611494b0b540476c4b9b52eac5b: s5p-mfc: Amend initial min, max values of HEVC hierarchical coding QP controls (2018-03-19 17:44:24 +0100) Support for MFC v10.10 in the s5p-mfc driver and addition of related HEVC V4L2 controls. Marek Szyprowski (1): media: s5p-mfc: Use real device for request_firmware() call Smitha T Murthy (12): videodev2.h: Add v4l2 definition for HEVC v4l2-ioctl: add HEVC format description v4l2: Documentation of HEVC compressed format v4l2: Add v4l2 control IDs for HEVC encoder v4l2: Documentation for HEVC CIDs s5p-mfc: Rename IS_MFCV8 macro s5p-mfc: Adding initial support for MFC v10.10 s5p-mfc: Use min scratch buffer size as provided by F/W s5p-mfc: Support MFCv10.10 buffer requirements s5p-mfc: Add support for HEVC decoder s5p-mfc: Add VP9 decoder support s5p-mfc: Add support for HEVC encoder Sylwester Nawrocki (2): s5p-mfc: Ensure HEVC QP controls range is properly updated s5p-mfc: Amend initial min, max values of HEVC hierarchical coding QP controls Documentation/devicetree/bindings/media/s5p-mfc.txt | 1 + Documentation/media/uapi/v4l/extended-controls.rst | 410 ++ Documentation/media/uapi/v4l/pixfmt-compressed.rst | 5 + drivers/media/platform/s5p-mfc/regs-mfc-v10.h | 87 +++ drivers/media/platform/s5p-mfc/regs-mfc-v8.h| 2 + drivers/media/platform/s5p-mfc/s5p_mfc.c| 28 + drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c | 9 + drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 68 ++- drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 8 +- drivers/media/platform/s5p-mfc/s5p_mfc_dec.c| 48 +- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 597 - drivers/media/platform/s5p-mfc/s5p_mfc_opr.h| 14 + drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 397 -- drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h | 15 + drivers/media/v4l2-core/v4l2-ctrls.c| 119 drivers/media/v4l2-core/v4l2-ioctl.c| 1 + include/uapi/linux/v4l2-controls.h | 93 +++- include/uapi/linux/videodev2.h | 1 + 18 files changed, 1824 insertions(+), 79 deletions(-) create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v10.h -- Regards, Sylwester
[PATCH] s5p-mfc: Amend initial min, max values of HEVC hierarchical coding QP controls
Valid range for those controls is specified in documentation as [0, 51], so initialize the controls to such range rather than [INT_MIN, INT_MAX]. Signed-off-by: Sylwester Nawrocki --- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c index 810dabe2f1b9..7382b41f4f6d 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c @@ -856,56 +856,56 @@ static struct mfc_control controls[] = { { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, { .id = V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP, .type = V4L2_CTRL_TYPE_INTEGER, - .minimum = INT_MIN, - .maximum = INT_MAX, + .minimum = 0, + .maximum = 51, .step = 1, .default_value = 0, }, -- 2.14.2
[PATCH] s5p-mfc: Ensure HEVC QP controls range is properly updated
When value of V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP or V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP controls is changed we should update range of a set of HEVC quantization parameter v4l2 controls as specified in the HEVC controls documentation. Signed-off-by: Sylwester Nawrocki --- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 40 1 file changed, 40 insertions(+) diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c index 6c80ebc5dbcc..810dabe2f1b9 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c @@ -1773,6 +1773,42 @@ static inline int vui_sar_idc(enum v4l2_mpeg_video_h264_vui_sar_idc sar) return t[sar]; } +/* + * Update range of all HEVC quantization parameter controls that depend on the + * V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP, V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP controls. + */ +static void __enc_update_hevc_qp_ctrls_range(struct s5p_mfc_ctx *ctx, +int min, int max) +{ + static const int __hevc_qp_ctrls[] = { + V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP, + V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP, + V4L2_CID_MPEG_VIDEO_HEVC_B_FRAME_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP, + V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP, + }; + struct v4l2_ctrl *ctrl = NULL; + int i, j; + + for (i = 0; i < ARRAY_SIZE(__hevc_qp_ctrls); i++) { + for (j = 0; j < ARRAY_SIZE(ctx->ctrls); j++) { + if (ctx->ctrls[j]->id == __hevc_qp_ctrls[i]) { + ctrl = ctx->ctrls[j]; + break; + } + } + if (WARN_ON(!ctrl)) + break; + + __v4l2_ctrl_modify_range(ctrl, min, max, ctrl->step, min); + } +} + static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl) { struct s5p_mfc_ctx *ctx = ctrl_to_ctx(ctrl); @@ -2038,9 +2074,13 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl) break; case V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP: p->codec.hevc.rc_min_qp = ctrl->val; + __enc_update_hevc_qp_ctrls_range(ctx, ctrl->val, +p->codec.hevc.rc_max_qp); break; case V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP: p->codec.hevc.rc_max_qp = ctrl->val; + __enc_update_hevc_qp_ctrls_range(ctx, p->codec.hevc.rc_min_qp, +ctrl->val); break; case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL: p->codec.hevc.level_v4l2 = ctrl->val; -- 2.14.2
Re: [GIT PULL] HEVC V4L2 controls and s5p-mfc update
On 03/12/2018 05:49 PM, Hans Verkuil wrote: > On 03/12/2018 09:35 AM, Sylwester Nawrocki wrote: >> On 03/12/2018 05:19 PM, Hans Verkuil wrote: >>> Does this include this __v4l2_ctrl_modify_range() request: >>> >>> https://patchwork.kernel.org/patch/10196605/ >>> >>> I haven't seen anything for that. I'd like to see that implemented before >>> this >>> is merged, otherwise this typically will never happen. >> Not yet, I assumed it will come afterwards. >> I will write the patch myself until end of this week, unless Smitha submits >> it earlier. >> > OK, in that case I am OK with this pull request. Nice to see HEVC support in! Hm, I have found some issues in the documentation after those patches. I will try to fix it and also post the control range update patch. I have marked the pull request as obsoleted, please ignore it. -- Regards, Sylwester
Re: [GIT PULL] HEVC V4L2 controls and s5p-mfc update
Hi Hans, On 03/12/2018 05:19 PM, Hans Verkuil wrote: > Does this include this __v4l2_ctrl_modify_range() request: > > https://patchwork.kernel.org/patch/10196605/ > > I haven't seen anything for that. I'd like to see that implemented before this > is merged, otherwise this typically will never happen. Not yet, I assumed it will come afterwards. I will write the patch myself until end of this week, unless Smitha submits it earlier. -- Regards, Sylwester
[GIT PULL] HEVC V4L2 controls and s5p-mfc update
Hi Mauro, The following changes since commit 3f127ce11353fd1071cae9b65bc13add6aec6b90: media: em28xx-cards: fix em28xx_duplicate_dev() (2018-03-08 06:06:51 -0500) are available in the git repository at: git://linuxtv.org/snawrocki/samsung.git tags/for-v4.17/media/samsung for you to fetch changes up to 4d26b437a1d98495454119082d50a68eadb2da4a: s5p-mfc: Add support for HEVC encoder (2018-03-12 16:15:28 +0100) Support for MFC v10.10 in the s5p-mfc driver and addition of related HEVC V4L2 controls. Smitha T Murthy (12): videodev2.h: Add v4l2 definition for HEVC v4l2-ioctl: add HEVC format description v4l2: Documentation of HEVC compressed format v4l2: Add v4l2 control IDs for HEVC encoder v4l2: Documentation for HEVC CIDs s5p-mfc: Rename IS_MFCV8 macro s5p-mfc: Adding initial support for MFC v10.10 s5p-mfc: Use min scratch buffer size as provided by F/W s5p-mfc: Support MFCv10.10 buffer requirements s5p-mfc: Add support for HEVC decoder s5p-mfc: Add VP9 decoder support s5p-mfc: Add support for HEVC encoder Documentation/devicetree/bindings/media/s5p-mfc.txt | 1 + Documentation/media/uapi/v4l/extended-controls.rst | 410 +++ Documentation/media/uapi/v4l/pixfmt-compressed.rst | 5 + drivers/media/platform/s5p-mfc/regs-mfc-v10.h | 87 drivers/media/platform/s5p-mfc/regs-mfc-v8.h| 2 + drivers/media/platform/s5p-mfc/s5p_mfc.c| 28 ++ drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c | 9 + drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 68 ++- drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 6 +- drivers/media/platform/s5p-mfc/s5p_mfc_dec.c| 48 +- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 557 - drivers/media/platform/s5p-mfc/s5p_mfc_opr.h| 14 + drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 397 +-- drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h | 15 + drivers/media/v4l2-core/v4l2-ctrls.c| 119 + drivers/media/v4l2-core/v4l2-ioctl.c| 1 + include/uapi/linux/v4l2-controls.h | 93 +++- include/uapi/linux/videodev2.h | 1 + 18 files changed, 1783 insertions(+), 78 deletions(-) create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v10.h -- Regards, Sylwester
Re: [PATCH v3 2/3] media: MAINTAINERS: add entry for ov9650 driver
On 01/21/2018 04:14 PM, Akinobu Mita wrote: > This adds an entry to the MAINTAINERS file for ov9650 driver. The > following persons are added in this entry. > > * Sakari as a person who looks after media sensor driver patches > * Sylwester as a module author > * Myself as a person who has the hardware and can test the patches > > Cc: Sylwester Nawrocki > Cc: Jacopo Mondi > Cc: H. Nikolaus Schaller > Cc: Hugues Fruchet > Cc: Sakari Ailus > Cc: Mauro Carvalho Chehab > Cc: Rob Herring > Signed-off-by: Akinobu Mita Feel free to add my: Acked-by: Sylwester Nawrocki > --- > MAINTAINERS | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index e358141..8924e39 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -10052,6 +10052,15 @@ S: Maintained > F: drivers/media/i2c/ov7670.c > F: Documentation/devicetree/bindings/media/i2c/ov7670.txt > > +OMNIVISION OV9650 SENSOR DRIVER > +M: Sakari Ailus > +R: Akinobu Mita > +R: Sylwester Nawrocki > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: drivers/media/i2c/ov9650.c Regards, Sylwester
Re: [PATCH] [media] s3c-camif: array underflow in __camif_subdev_try_format()
On 01/22/2018 11:37 AM, Dan Carpenter wrote: > The while loop is a post op, "while (i-- >= 0)" so the last iteration > will read camif_mbus_formats[-1] and then the loop will exit with "i" > set to -2 and so we do: "mf->code = camif_mbus_formats[-2];". > > I've changed it to a pre-op, I've added a check to ensure we found the > right format and I've removed the "mf->code = camif_mbus_formats[i];" > because that is a no-op anyway. > > Fixes: babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series > camera interface") > Signed-off-by: Dan Carpenter > > diff --git a/drivers/media/platform/s3c-camif/camif-capture.c > b/drivers/media/platform/s3c-camif/camif-capture.c > index 437395a61065..012f4b389c55 100644 > --- a/drivers/media/platform/s3c-camif/camif-capture.c > +++ b/drivers/media/platform/s3c-camif/camif-capture.c > @@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct > camif_dev *camif, > /* FIXME: constraints against codec or preview path ? */ > pix_lim = &variant->vp_pix_limits[VP_CODEC]; > > - while (i-- >= 0) > + while (--i >= 0) > if (camif_mbus_formats[i] == mf->code) > break; > - > - mf->code = camif_mbus_formats[i]; > + if (i < 0) > + return; Thanks for the patch Dan. mf->width needs to be aligned by this try_format function so we shouldn't return here. Also it needs to be ensured mf->code is set to one of the supported values when this function returns. Sorry, the current code really doesn't give a clue what was intended. There is already queued a patch from Arnd [1] addressing the issues you have found. > if (pad == CAMIF_SD_PAD_SINK) { > v4l_bound_align_image(&mf->width, 8, CAMIF_MAX_PIX_WIDTH, > [1] https://patchwork.linuxtv.org/patch/46508 -- Regards, Sylwester
Re: [PATCH v2 2/2] media: ov9650: add device tree binding
On 01/08/2018 10:35 AM, Sakari Ailus wrote: > I was going to say you're missing the MAINTAINERS entry for this newly > added file but then I noticed that the entire driver is missing an entry. > Still this file should have a MAINTAINERS entry added for it independently > of that, in the same patch. > > Cc Sylwester. I don't the hardware and I can't test the patches so Mita-san if you wish so please add yourself as a maintainer of whole driver. -- Regards, Sylwester
Re: [PATCH v2 1/2] media: ov9650: support device tree probing
nt ret; > > - if (!pdata) { > - dev_err(&client->dev, "platform data not specified\n"); > - return -EINVAL; > - } > - > - if (pdata->mclk_frequency == 0) { > - dev_err(&client->dev, "MCLK frequency not specified\n"); > - return -EINVAL; > - } > - > ov965x = devm_kzalloc(&client->dev, sizeof(*ov965x), GFP_KERNEL); > if (!ov965x) > return -ENOMEM; > > - mutex_init(&ov965x->lock); I would leave mutex initialization as first thing after the private data structure allocation, is there a need to move it further? > ov965x->client = client; > - ov965x->mclk_frequency = pdata->mclk_frequency; > + > + if (pdata) { > + if (pdata->mclk_frequency == 0) { > + dev_err(&client->dev, "MCLK frequency not specified\n"); > + return -EINVAL; > + } > + ov965x->mclk_frequency = pdata->mclk_frequency; > + > + ret = ov965x_configure_gpios_pdata(ov965x, pdata); > + if (ret < 0) > + return ret; > + } else if (dev_fwnode(&client->dev)) { > + ov965x->clk = devm_clk_get(&ov965x->client->dev, NULL); > + if (IS_ERR(ov965x->clk)) > + return PTR_ERR(ov965x->clk); > + ov965x->mclk_frequency = clk_get_rate(ov965x->clk); > + > + ret = ov965x_configure_gpios(ov965x); > + if (ret < 0) > + return ret; > + } else { > + dev_err(&client->dev, > + "Neither platform data nor device property > specified\n"); > + > + return -EINVAL; > + } > + > + mutex_init(&ov965x->lock); > > sd = &ov965x->sd; > v4l2_i2c_subdev_init(sd, client, &ov965x_subdev_ops); > @@ -1502,10 +1550,6 @@ static int ov965x_probe(struct i2c_client *client, > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | >V4L2_SUBDEV_FL_HAS_EVENTS; > > - ret = ov965x_configure_gpios(ov965x, pdata); > - if (ret < 0) > - goto err_mutex; > - > ov965x->pad.flags = MEDIA_PAD_FL_SOURCE; > sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > ret = media_entity_pads_init(&sd->entity, 1, &ov965x->pad); > @@ -1561,9 +1605,19 @@ static const struct i2c_device_id ov965x_id[] = { > }; > MODULE_DEVICE_TABLE(i2c, ov965x_id); > > +#if IS_ENABLED(CONFIG_OF) Is there any advantage in using IS_ENABLED() rather than just #ifdef CONFIG_OF ? of_match_ptr() is defined with just #idef CONFIG_OF/ #else/#endif. I would use simply #ifdef CONFIG_OF here. Otherwise looks good. Reviewed-by: Sylwester Nawrocki -- Regards, Sylwester
[GIT PULL] Samsung SoC related updates
Hi Mauro, The following changes since commit 0ca4e3130402caea8731a7b54afde56a6edb17c9: media: pxa_camera: rename the soc_camera_ prefix to pxa_camera_ (2017-12-14 12:40:01 -0500) are available in the git repository at: git://linuxtv.org/snawrocki/samsung.git for-v4.16/media/next for you to fetch changes up to 8d10c3a3fa56badd9d8691b59a88e7f00fdeaa7b: s5p-jpeg: Fix off-by-one problem (2017-12-15 17:33:50 +0100) Arnd Bergmann (1): exynos4-is: properly initialize frame format Flavio Ceolin (1): s5p-jpeg: Fix off-by-one problem Marek Szyprowski (3): exynos-gsc: Drop obsolete capabilities exynos4-is: Drop obsolete capabilities exynos4-is: Remove dependency on obsolete SoC support Shuah Khan (2): s5p-mfc: Remove firmware buf null check in s5p_mfc_load_firmware() s5p-mfc: Fix lock contention - request_firmware() once Simon Shields (1): exynos4-is: Check pipe is valid before calling subdev Sylwester Nawrocki (1): s5p-mfc: Fix encoder menu controls initialization drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +--- drivers/media/platform/exynos4-is/Kconfig | 2 +- drivers/media/platform/exynos4-is/fimc-core.c | 2 +- drivers/media/platform/exynos4-is/fimc-isp.c| 14 +++--- drivers/media/platform/exynos4-is/fimc-lite.c | 2 +- drivers/media/platform/exynos4-is/fimc-m2m.c| 10 +- drivers/media/platform/s5p-jpeg/jpeg-core.c | 2 +- drivers/media/platform/s5p-mfc/s5p_mfc.c| 6 ++ drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 3 +++ drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 10 +- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 2 +- include/media/drv-intf/exynos-fimc.h| 3 ++- 12 files changed, 30 insertions(+), 30 deletions(-) -- Regards, Sylwester
[PATCH] s5p-mfc: Fix encoder menu controls initialization
This patch fixes the menu_skip_mask field initialization and addresses a following issue found by the SVACE static analysis: * NO_EFFECT.SELF: assignment to self in expression 'cfg.menu_skip_mask = cfg.menu_skip_mask' No effect at drivers/media/platform/s5p-mfc/s5p_mfc_enc.c:2083 Signed-off-by: Sylwester Nawrocki --- drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c index 2a5fd7c42cd5..0d5d465561be 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c @@ -2080,7 +2080,7 @@ int s5p_mfc_enc_ctrls_setup(struct s5p_mfc_ctx *ctx) if (cfg.type == V4L2_CTRL_TYPE_MENU) { cfg.step = 0; - cfg.menu_skip_mask = cfg.menu_skip_mask; + cfg.menu_skip_mask = controls[i].menu_skip_mask; cfg.qmenu = mfc51_get_menu(cfg.id); } else { cfg.step = controls[i].step; -- 2.14.2
Re: [Patch v6 10/12] [media] v4l2: Add v4l2 control IDs for HEVC encoder
On 12/12/2017 03:34 AM, Smitha T Murthy wrote: >> s/Lay/Layer here and below >> > Ok I will change it. While it's fine to make such change for controls up to V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP... >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_QP:return "HEVC >>> Hierarchical Lay 1 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_QP:return "HEVC >>> Hierarchical Lay 2 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_QP:return "HEVC >>> Hierarchical Lay 3 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_QP:return "HEVC >>> Hierarchical Lay 4 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_QP:return "HEVC >>> Hierarchical Lay 5 QP"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_QP:return "HEVC >>> Hierarchical Lay 6 QP"; ...for the controls below we may need to replace "Lay" with "L." to make sure the length of the string don't exceed 31 characters (32 with terminating NULL). The names below seem to be 1 character too long and will be truncated when running VIDIOC_QUERY_CTRL ioctl. >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L0_BR:return "HEVC >>> Hierarchical Lay 0 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L1_BR:return "HEVC >>> Hierarchical Lay 1 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L2_BR:return "HEVC >>> Hierarchical Lay 2 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L3_BR:return "HEVC >>> Hierarchical Lay 3 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L4_BR:return "HEVC >>> Hierarchical Lay 4 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L5_BR:return "HEVC >>> Hierarchical Lay 5 Bit Rate"; >>> + case V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR:return "HEVC >>> Hierarchical Lay 6 Bit Rate"; -- Regards, Sylwester
Re: [PATCH 12/22] media: s3c-camif: add missing description at s3c_camif_find_format()
On 11/29/2017 08:08 PM, Mauro Carvalho Chehab wrote: > Fix this warning: > drivers/media/platform/s3c-camif/camif-core.c:112: warning: No > description found for parameter 'vp' > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Sylwester Nawrocki
Re: [PATCH 4/7] media: exynos4-is: Remove dependency on obsolete SoC support
On 10/04/2017 08:38 AM, Marek Szyprowski wrote: > Support for Exynos4212 SoCs has been removed by commit bca9085e0ae9 ("ARM: > dts: exynos: remove Exynos4212 support (dead code)"), so there is no need > to keep remaining dead code related to this SoC version. > > Signed-off-by: Marek Szyprowski Acked-by: Sylwester Nawrocki
[GIT PULL] Exynos/S5P updates for 4.15
Hi Mauro, The following changes since commit 8382e556b1a2f30c4bf866f021b33577a64f9ebf: Simplify major/minor non-dynamic logic (2017-10-11 15:32:11 -0400) are available in the git repository at: git://linuxtv.org/snawrocki/samsung.git for-v4.15/media/next for you to fetch changes up to e5fa99e5df93e815920e87e907e5cb61e765505b: s5p-mfc: Adjust a null pointer check in four functions (2017-10-13 13:17:49 +0200) Hoegeun Kwon (2): exynos-gsc: Add compatible for Exynos 5250 and 5420 SoC version exynos-gsc: Add hardware rotation limits Markus Elfring (3): s5p-mfc: Delete an error message for a failed memory allocation s5p-mfc: Improve a size determination in s5p_mfc_alloc_memdev() s5p-mfc: Adjust a null pointer check in four functions .../devicetree/bindings/media/exynos5-gsc.txt | 9 +- drivers/media/platform/exynos-gsc/gsc-core.c | 127 - drivers/media/platform/s5p-mfc/s5p_mfc.c | 14 +- 3 files changed, 134 insertions(+), 16 deletions(-) -- Regards, Sylwester
Re: [PATCH v15 01/32] v4l: async: Remove re-probing support
Hi Sakari, On 10/09/2017 04:18 PM, Sakari Ailus wrote: > Sure, how about this at the end of the current commit message: > > If there is a need to support removing the clock provider in the future, > this should be implemented in the clock framework instead, not in V4L2. I find it a little bit misleading, there is already support for removing the clock provider, only any clock references for consumers became then stale. Perhaps: "If there is a need to support the clock provider unregister/register cycle while keeping the clock references in the consumers in the future, this should be implemented in the clock framework instead, not in V4L2." ? That said, I doubt this issue is going to be entirely solved solely in the clock framework, as it is a more general problem of resource dependencies. It could be related to other resources, like regulator or GPIO. It has been discussed for a long time now and it will likely take time until a general solution is available. -- Thanks, Sylwester
Re: [RESEND PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
On 09/28/2017 02:53 PM, Mauro Carvalho Chehab wrote: > (Resending for Mauro, while dropping the non-list recipients. The original > likely had too many recipients.) > > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. > > At drivers, this makes even clearer about the match criteria. > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Sylwester Nawrocki
Re: [PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure
On 09/27/2017 11:46 PM, Mauro Carvalho Chehab wrote: > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME > match criteria requires just a device name. > > So, it doesn't make sense to enclose those into structs, > as the criteria can go directly into the union. > > That makes easier to document it, as we don't need to document > weird senseless structs. > > At drivers, this makes even clearer about the match criteria. > > Signed-off-by: Mauro Carvalho Chehab Acked-by: Sylwester Nawrocki
Re: as3645a flash userland interface
On 09/14/2017 01:53 PM, Pavel Machek wrote: On Thu 2017-09-14 13:01:19, Sylwester Nawrocki wrote: On 09/14/2017 12:07 PM, Pavel Machek wrote: Isn't the V4L2 subdev/Media Controller API supposed to provide means for associating flash LEDs with camera sensors? You seem to be insisting on using the sysfs leds interface for that, which is not a primary interface for camera flash AFAICT. a) subdev/media controller API currently does not provide such means. Yes, but it should, that's what it was designed for AFAIK. b) if we have /sys/class/leds interface to userland, it should be useful. At the same time we shouldn't overcomplicate it with the camera functionality. I'm advocating adding label = "main_camera" into the .dts. That's all. c) having flashlight application going through media controller API is a bad joke. It doesn't have to, maybe I misunderstood what you exactly ask for. Nevertheless what's missing is some user visible name/label for each flash LED, right? Currently enumerating flash LEDs can be done by looking at the function part of /sys/class/leds/:: path. Could additional information be appended to the part, so user can identify which LED is which? E.g. "flash(rear)", "flash(front)", etc. This could be achieved by simply adding label property in DT. Or is the list of supported strings already standardized? label = "flash_main_camera" would work for me, yes. And yes, I'd prefer to do this before 4.14 release, so that userland-visible interface does not change. Looking at existing label entries in DT (git grep label.*led -- arch/arm/boot/dts | sed 's\^.*label\label\' | sort | uniq) I can't see why something like this couldn't be added. Or label = "as3645a::flash_main_camera" so we abide the LED device naming rules described in Documentation/leds/leds-class.txt. -- Regards, Sylwester
Re: as3645a flash userland interface
On 09/14/2017 12:07 PM, Pavel Machek wrote: Isn't the V4L2 subdev/Media Controller API supposed to provide means for associating flash LEDs with camera sensors? You seem to be insisting on using the sysfs leds interface for that, which is not a primary interface for camera flash AFAICT. > a) subdev/media controller API currently does not provide such means. Yes, but it should, that's what it was designed for AFAIK. b) if we have /sys/class/leds interface to userland, it should be useful. At the same time we shouldn't overcomplicate it with the camera functionality. c) having flashlight application going through media controller API is a bad joke. It doesn't have to, maybe I misunderstood what you exactly ask for. Nevertheless what's missing is some user visible name/label for each flash LED, right? Currently enumerating flash LEDs can be done by looking at the function part of /sys/class/leds/:: path. Could additional information be appended to the part, so user can identify which LED is which? E.g. "flash(rear)", "flash(front)", etc. This could be achieved by simply adding label property in DT. Or is the list of supported strings already standardized? -- Regards, Sylwester
Re: as3645a flash userland interface
Hi, On 09/13/2017 07:53 PM, Jacek Anaszewski wrote: On 09/12/2017 11:55 PM, Pavel Machek wrote: On Tue 2017-09-12 20:53:33, Jacek Anaszewski wrote: On 09/12/2017 12:36 PM, Pavel Machek wrote: What directory are the flash controls in? /sys/class/leds/led-controller:flash ? Could we arrange for something less generic, like /sys/class/leds/main-camera:flash ? I'd rather avoid overcomplicating this. LED class device name pattern is well defined to devicename:colour:function (see Documentation/leds/leds-class.txt, "LED Device Naming" section). In this case "flash" in place of the "function" segment makes the things clear enough I suppose. It does not. Phones usually have two cameras, front and back, and these days both cameras have their flash. And poor userspace flashlight application can not know if as3645 drivers front LED or back LED. Thus, I'd set devicename to front-camera or main-camera -- because that's what it is associated with. Userspace does not care what hardware drives the LED, but needs to know if it is front or back camera. The name of a LED flash class device isn't fixed and is derived from DT label property. Name in the example of some DT bindings will not force people to apply similar pattern for the other drivers and even for the related one. No worry about having to keep anything forever basing on that. Isn't the V4L2 subdev/Media Controller API supposed to provide means for associating flash LEDs with camera sensors? You seem to be insisting on using the sysfs leds interface for that, which is not a primary interface for camera flash AFAICT. -- Regards, Sylwester
Re: [PATCH] [media] s3c-camif: fix out-of-bounds array access
On 09/13/2017 04:03 PM, Arnd Bergmann wrote: On Wed, Sep 13, 2017 at 11:25 AM, Sylwester Nawrocki wrote: On 09/12/2017 10:09 PM, Arnd Bergmann wrote: { const struct s3c_camif_variant *variant = camif->variant; const struct vp_pix_limits *pix_lim; - int i = ARRAY_SIZE(camif_mbus_formats); /* FIXME: constraints against codec or preview path ? */ pix_lim = &variant->vp_pix_limits[VP_CODEC]; - while (i-- >= 0) - if (camif_mbus_formats[i] == mf->code) - break; - - mf->code = camif_mbus_formats[i]; Interesting finding... the function needs to ensure mf->code is set to one of supported values by the driver, so instead of removing how about changing the above line to: if (i < 0) mf->code = camif_mbus_formats[0]; ? That would still have one of the two out-of-bounds accesses;-) Ah, indeed :/ maybe this for (i = 0; i < ARRAY_SIZE(camif_mbus_formats); i++) if (camif_mbus_formats[i] == mf->code) break; if (i == ARRAY_SIZE(camif_mbus_formats)) mf->code = camif_mbus_formats[0]; Yes, it should work that way. -- Thanks, Sylwester
Re: [PATCH] s5p-cec: add NACK detection support
On 08/31/2017 06:56 PM, Hans Verkuil wrote: The s5p-cec driver returned CEC_TX_STATUS_ERROR for the NACK condition. Some digging into the datasheet uncovered the S5P_CEC_TX_STAT1 register where bit 0 indicates if the transmit was nacked or not. Use this to return the correct CEC_TX_STATUS_NACK status to userspace. This was the only driver that couldn't tell a NACK from another error, and that was very unusual. And a potential problem for applications as well. Tested with my Odroid-U3. Signed-off-by: Hans Verkuil Acked-by: Sylwester Nawrocki
Re: [PATCH v4 0/4] Exynos-gsc: Support the hardware rotation limits
On 09/13/2017 01:41 PM, Hoegeun Kwon wrote: Hoegeun Kwon (4): [media] exynos-gsc: Add compatible for Exynos 5250 and 5420 specific version ARM: dts: exynos: Add clean name of compatible. drm/exynos/gsc: Add hardware rotation limits [media] exynos-gsc: Add hardware rotation limits Thanks Hoegeung, I have applied patches 1 nad 4 from this series. -- Regards, Sylwester
Re: [PATCH v4 4/4] [media] exynos-gsc: Add hardware rotation limits
On 09/13/2017 01:41 PM, Hoegeun Kwon wrote: @@ -1004,11 +1088,33 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv) .num_clocks = 1, }; +static struct gsc_driverdata gsc_v_5250_drvdata = { + .variant = { + [0] = &gsc_v_5250_variant, + [1] = &gsc_v_5250_variant, + [2] = &gsc_v_5250_variant, + [3] = &gsc_v_5250_variant, + }, + .num_entities = 4, + .clk_names = { "gscl" }, + .num_clocks = 1, +}; + +static struct gsc_driverdata gsc_v_5420_drvdata = { + .variant = { + [0] = &gsc_v_5420_variant, + [1] = &gsc_v_5420_variant, + }, + .num_entities = 4, Shouldn't num_enities be 2 here? You don't need to resend, I can amend that when applying. + .clk_names = { "gscl" }, + .num_clocks = 1, +}; + -- Regards, Sylwester
Re: [PATCH] [media] s3c-camif: fix out-of-bounds array access
On 09/12/2017 10:09 PM, Arnd Bergmann wrote: > While experimenting with older compiler versions, I ran > into a warning that no longer shows up on gcc-4.8 or newer: > > drivers/media/platform/s3c-camif/camif-capture.c: In function > '__camif_subdev_try_format': > drivers/media/platform/s3c-camif/camif-capture.c:1265:25: error: array > subscript is below array bounds > > This is an off-by-one bug, leading to an access before the start of the > array, while newer compilers silently assume this undefined behavior > cannot happen and leave the loop at index 0 if no other entry matches. > > Since the code is not only wrong, but also has no effect besides the > out-of-bounds access, this patch just removes it. > > I found an existing gcc bug for it and added a reduced version > of the function there. > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69249#c3 > Fixes: babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series > camera interface") > Signed-off-by: Arnd Bergmann > --- > drivers/media/platform/s3c-camif/camif-capture.c | 7 --- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/media/platform/s3c-camif/camif-capture.c > b/drivers/media/platform/s3c-camif/camif-capture.c > index 25c7a7d42292..c6921f6a5a6a 100644 > --- a/drivers/media/platform/s3c-camif/camif-capture.c > +++ b/drivers/media/platform/s3c-camif/camif-capture.c > @@ -1256,17 +1256,10 @@ static void __camif_subdev_try_format(struct > camif_dev *camif, > { > const struct s3c_camif_variant *variant = camif->variant; > const struct vp_pix_limits *pix_lim; > - int i = ARRAY_SIZE(camif_mbus_formats); > > /* FIXME: constraints against codec or preview path ? */ > pix_lim = &variant->vp_pix_limits[VP_CODEC]; > > - while (i-- >= 0) > - if (camif_mbus_formats[i] == mf->code) > - break; > - > - mf->code = camif_mbus_formats[i]; Interesting finding... the function needs to ensure mf->code is set to one of supported values by the driver, so instead of removing how about changing the above line to: if (i < 0) mf->code = camif_mbus_formats[0]; ? -- Thanks, Sylwester
Re: [PATCH v3 4/6] [media] exynos-gsc: Add hardware rotation limits
Hi Hoegeun, On 09/13/2017 04:33 AM, Hoegeun Kwon wrote: >>> @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq, >>> void *priv) >>> static const struct of_device_id exynos_gsc_match[] = { >>>{ >>> -.compatible = "samsung,exynos5-gsc", >>> -.data = &gsc_v_100_drvdata, >> Can you keep the "samsung,exynos5-gsc" entry with the gsc_v_5250_variant >> data, so that it can work with "samsung,exynos5-gsc" compatible in DT >> on both exynos5250 and exynos5420 SoCs? >> > Thank you for your question. > > Exynos 5250 and 5420 have different hardware rotation limits. > Exynos 5250 is '.real_rot_en_w/h = 2016' and 5420 is '.real_rot_en_w/h = > 2048'. > > So my opinion they must have different compatible. I think there is some misunderstanding, mu suggestion was to keep the "samsung,exynos5-gsc" compatible entry in addition to the new introduced ones: "samsung,exynos5250-gsc" and "samsung,exynos5420-gsc". That's in order to make your changes possibly backward compatible, in theory the updated driver should still work without changes in dts. -- Regards, Sylwester
Re: [media] s5p-mfc: Adjust a null pointer check in four functions
On 09/12/2017 05:00 PM, SF Markus Elfring wrote: * Do you care to preserve an information like the author date? In this case not, but actually the Date line is not an issue. Thanks for your information. It seems then that you quoted a line too much. Anyway the patch is malformed, … > I have got doubts for this view because the file was automatically generated by the command “git format-patch” also for the discussed three update steps. Generating patch is only part of the story, it seems the patch is not sent properly and tags which should be in SMTP header end up in the message body. I think there would not be such issues if you have used git format-patch + git send-email. I normally do amend things like this while applying, I will do that this time as well. It's already too much time wasted for such a dubious patch. -- Thanks, Sylwester
Re: [media] s5p-mfc: Adjust a null pointer check in four functions
On 09/11/2017 09:21 PM, SF Markus Elfring wrote: Date: Fri, 8 Sep 2017 22:37:00 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Can you resend with that 4 lines removed? * Do you care to preserve an information like the author date? In this case not, but actually the Date line is not an issue. Anyway the patch is malformed, please try to save your posted patch and apply with git am and see how finally the commit message looks like. * Would you like to support special characters in the commit message? I can't see any need for special characters in the patch itself. Please submit the patch in a way that it can be applied properly with patchwork client (or git am). Are you using git send-email for sending patches? Not so far. I would suggest switching to git send-email, then issues like above could be easily avoided. -- Regards, Sylwester
Re: [PATCH v3 4/6] [media] exynos-gsc: Add hardware rotation limits
On 09/08/2017 08:02 AM, Hoegeun Kwon wrote: The hardware rotation limits of gsc depends on SOC (Exynos 5250/5420/5433). Distinguish them and add them to the driver data. Signed-off-by: Hoegeun Kwon --- drivers/media/platform/exynos-gsc/gsc-core.c | 96 1 file changed, 83 insertions(+), 13 deletions(-) diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c index 4380150..8f8636e 100644 --- a/drivers/media/platform/exynos-gsc/gsc-core.c +++ b/drivers/media/platform/exynos-gsc/gsc-core.c @@ -943,7 +943,37 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv) return IRQ_HANDLED; } -static struct gsc_pix_max gsc_v_100_max = { +static struct gsc_pix_max gsc_v_5250_max = { + .org_scaler_bypass_w= 8192, + .org_scaler_bypass_h= 8192, + .org_scaler_input_w = 4800, + .org_scaler_input_h = 3344, + .real_rot_dis_w = 4800, + .real_rot_dis_h = 3344, + .real_rot_en_w = 2016, + .real_rot_en_h = 2016, + .target_rot_dis_w = 4800, + .target_rot_dis_h = 3344, + .target_rot_en_w= 2016, + .target_rot_en_h= 2016, +}; + +static struct gsc_pix_max gsc_v_5420_max = { + .org_scaler_bypass_w= 8192, + .org_scaler_bypass_h= 8192, + .org_scaler_input_w = 4800, + .org_scaler_input_h = 3344, + .real_rot_dis_w = 4800, + .real_rot_dis_h = 3344, + .real_rot_en_w = 2048, + .real_rot_en_h = 2048, + .target_rot_dis_w = 4800, + .target_rot_dis_h = 3344, + .target_rot_en_w= 2016, + .target_rot_en_h= 2016, +}; + +static struct gsc_pix_max gsc_v_5433_max = { .org_scaler_bypass_w= 8192, .org_scaler_bypass_h= 8192, .org_scaler_input_w = 4800, @@ -979,8 +1009,8 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv) .target_h = 2, /* yuv420 : 2, others : 1 */ }; -static struct gsc_variant gsc_v_100_variant = { - .pix_max= &gsc_v_100_max, +static struct gsc_variant gsc_v_5250_variant = { + .pix_max= &gsc_v_5250_max, .pix_min= &gsc_v_100_min, .pix_align = &gsc_v_100_align, .in_buf_cnt = 32, @@ -992,12 +1022,48 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv) .local_sc_down = 2, }; -static struct gsc_driverdata gsc_v_100_drvdata = { +static struct gsc_variant gsc_v_5420_variant = { + .pix_max= &gsc_v_5420_max, + .pix_min= &gsc_v_100_min, + .pix_align = &gsc_v_100_align, + .in_buf_cnt = 32, + .out_buf_cnt= 32, + .sc_up_max = 8, + .sc_down_max= 16, + .poly_sc_down_max = 4, + .pre_sc_down_max= 4, + .local_sc_down = 2, +}; + +static struct gsc_variant gsc_v_5433_variant = { + .pix_max= &gsc_v_5433_max, + .pix_min= &gsc_v_100_min, + .pix_align = &gsc_v_100_align, + .in_buf_cnt = 32, + .out_buf_cnt= 32, + .sc_up_max = 8, + .sc_down_max= 16, + .poly_sc_down_max = 4, + .pre_sc_down_max= 4, + .local_sc_down = 2, +}; + +static struct gsc_driverdata gsc_v_5250_drvdata = { .variant = { - [0] = &gsc_v_100_variant, - [1] = &gsc_v_100_variant, - [2] = &gsc_v_100_variant, - [3] = &gsc_v_100_variant, + [0] = &gsc_v_5250_variant, + [1] = &gsc_v_5250_variant, + [2] = &gsc_v_5250_variant, + [3] = &gsc_v_5250_variant, + }, + .num_entities = 4, + .clk_names = { "gscl" }, + .num_clocks = 1, +}; + +static struct gsc_driverdata gsc_v_5420_drvdata = { + .variant = { + [0] = &gsc_v_5420_variant, + [1] = &gsc_v_5420_variant, }, .num_entities = 4, .clk_names = { "gscl" }, @@ -1006,9 +1072,9 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv) static struct gsc_driverdata gsc_5433_drvdata = { .variant = { - [0] = &gsc_v_100_variant, - [1] = &gsc_v_100_variant, - [2] = &gsc_v_100_variant, + [0] = &gsc_v_5433_variant, + [1] = &gsc_v_5433_variant, + [2] = &gsc_v_5433_variant, }, .num_entities = 3, .clk_names = { "pclk", "aclk", "aclk_xiu", "aclk_gsclbend" }, @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv) static const struct of_device_id exynos_gsc_match[] = { { -
Re: [PATCH 3/3] [media] s5p-mfc: Adjust a null pointer check in four functions
On 09/08/2017 10:53 PM, SF Markus Elfring wrote: From: Markus Elfring Date: Fri, 8 Sep 2017 22:37:00 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Can you resend with that 4 lines removed? Are you using git send-email for sending patches? -- Thanks, Sylwester
Re: [PATCH 2/3] [media] s5p-mfc: Improve a size determination in s5p_mfc_alloc_memdev()
On 09/08/2017 10:52 PM, SF Markus Elfring wrote: Replace the specification of a data structure by a pointer dereference as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring Acked-by: Sylwester Nawrocki
Re: [PATCH 1/3] [media] s5p-mfc: Delete an error message for a failed memory allocation in s5p_mfc_probe()
On 09/08/2017 10:51 PM, SF Markus Elfring wrote: Omit an extra message for a memory allocation failure in this function. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring Could you make the commit summary shorter, to keep it below 70 characters [1]? With that changed feel free to add Acked-by: Sylwester Nawrocki -- Regards, Sylwester [1] Documentation/process/submitting-patches.rst
Re: [PATCH 3/5] s5p-cec: use CEC_CAP_DEFAULTS
On 08/04/2017 12:41 PM, Hans Verkuil wrote: Use the new CEC_CAP_DEFAULTS define in the s5p-cec driver. Signed-off-by: Hans Verkuil Acked-by: Sylwester Nawrocki
Re: [PATCH 1/5] media/cec.h: add CEC_CAP_DEFAULTS
On 08/04/2017 12:41 PM, Hans Verkuil wrote: The CEC_CAP_LOG_ADDRS, CEC_CAP_TRANSMIT, CEC_CAP_PASSTHROUGH and CEC_CAP_RC capabilities are normally always present. Add a CEC_CAP_DEFAULTS define that ORs these four caps to simplify drivers. Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
[GIT PULL] s5p-jpeg fixes for v4.14-rc1
Hi Mauro, The following changes since commit ec0c3ec497cabbf3bfa03a9eb5edcc252190a4e0: media: ddbridge: split code into multiple files (2017-08-09 12:17:01 -0400) are available in the git repository at: git://linuxtv.org/snawrocki/samsung.git for-v4.14/media/next-2 for you to fetch changes up to a1c60f2c60228a6c0d31e95ae4e65e6afd4655df: s5p-jpeg: directly use parsed subsampling on exynos5433 (2017-08-11 19:13:06 +0200) Andrzej Pietrasiewicz (5): s5p-jpeg: don't overwrite result's "size" member s5p-jpeg: set w/h when encoding s5p-jpeg: disable encoder/decoder in exynos4-like hardware after use s5p-jpeg: fix number of components macro s5p-jpeg: directly use parsed subsampling on exynos5433 Tony K Nadackal (2): s5p-jpeg: Fix crash in jpeg isr due to multiple interrupts. s5p-jpeg: Clear JPEG_CODEC_ON bits in sw reset function drivers/media/platform/s5p-jpeg/jpeg-core.c | 18 ++ drivers/media/platform/s5p-jpeg/jpeg-core.h | 1 + drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c | 9 - drivers/media/platform/s5p-jpeg/jpeg-regs.h | 2 +- 4 files changed, 24 insertions(+), 6 deletions(-) -- Regards, Sylwester
Re: [PATCH 4/5] media: platform: s5p-jpeg: fix number of components macro
On 08/08/2017 01:27 PM, Andrzej Pietrasiewicz wrote: The value to be processed must be first masked and then shifted, not the other way round. Signed-off-by: Andrzej Pietrasiewicz --- drivers/media/platform/s5p-jpeg/jpeg-regs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/s5p-jpeg/jpeg-regs.h b/drivers/media/platform/s5p-jpeg/jpeg-regs.h index 1870400..df790b1 100644 --- a/drivers/media/platform/s5p-jpeg/jpeg-regs.h +++ b/drivers/media/platform/s5p-jpeg/jpeg-regs.h @@ -371,7 +371,7 @@ #define EXYNOS4_NF_SHIFT 16 #define EXYNOS4_NF_MASK 0xff #define EXYNOS4_NF(x) \ - (((x) << EXYNOS4_NF_SHIFT) & EXYNOS4_NF_MASK) + (((x) & EXYNOS4_NF_MASK) << EXYNOS4_NF_SHIFT) I'm going to add below tag when applying this patch. Fixes: 6c96dbbc2aa9f5b4a ("[media] s5p-jpeg: add support for 5433") -- Regards, Sylwester
Re: [PATCH 5/6] [media] exynos4-is: constify video_subdev structures
On 08/08/2017 12:58 PM, Julia Lawall wrote: The v4l2_subdev_ops structures are only passed as the second argument of v4l2_subdev_init, which is const, so the v4l2_subdev_ops structures can be const as well. Done with the help of Coccinelle. Signed-off-by: Julia Lawall Reviewed-by: Sylwester Nawrocki
Re: [PATCH 03/12] [media] s5p-g2d: constify v4l2_m2m_ops structures
On 08/06/2017 10:25 AM, Julia Lawall wrote: The v4l2_m2m_ops structures are only passed as the only argument to v4l2_m2m_init, which is declared as const. Thus the v4l2_m2m_ops structures themselves can be const. Done with the help of Coccinelle. Signed-off-by: Julia Lawall Reviewed-by: Sylwester Nawrocki
Re: [PATCH 11/12] [media] exynos4-is: constify v4l2_m2m_ops structures
On 08/06/2017 10:25 AM, Julia Lawall wrote: The v4l2_m2m_ops structures are only passed as the only argument to v4l2_m2m_init, which is declared as const. Thus the v4l2_m2m_ops structures themselves can be const. Done with the help of Coccinelle. Signed-off-by: Julia Lawall Reviewed-by: Sylwester Nawrocki
Re: [PATCH 06/12] [media] exynos-gsc: constify v4l2_m2m_ops structures
On 08/06/2017 10:25 AM, Julia Lawall wrote: The v4l2_m2m_ops structures are only passed as the only argument to v4l2_m2m_init, which is declared as const. Thus the v4l2_m2m_ops structures themselves can be const. Done with the help of Coccinelle. Signed-off-by: Julia Lawall Reviewed-by: Sylwester Nawrocki
Re: [PATCHv2 5/5] media-device: remove driver_version
On 07/21/2017 12:57 PM, Hans Verkuil wrote: From: Hans Verkuil Since the driver_version field in struct media_device is no longer used, just remove it. Signed-off-by: Hans Verkuil --- drivers/media/media-device.c | 3 --- include/media/media-device.h | 2 -- 2 files changed, 5 deletions(-) diff --git a/include/media/media-device.h b/include/media/media-device.h index 6896266031b9..7d268802cc2e 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -68,7 +68,6 @@ struct media_device_ops { * @serial: Device serial number (optional) * @bus_info: Unique and stable device location identifier * @hw_revision: Hardware device revision - * @driver_version: Device driver version * @topology_version: Monotonic counter for storing the version of the graph *topology. Should be incremented each time the topology changes. * @id: Unique ID used on the last registered graph object @@ -134,7 +133,6 @@ struct media_device { char serial[40]; char bus_info[32]; u32 hw_revision; - u32 driver_version; It seems we still have such paragraph in include/media/media-device.h: * - &media_entity.driver_version is formatted with the KERNEL_VERSION() *macro. The version minor must be incremented when new features are added *to the userspace API without breaking binary compatibility. The version *major must be incremented when binary compatibility is broken. Shouldn't this also be removed? -- Regards, Sylwester
Re: [PATCHv2 5/5] media-device: remove driver_version
On 07/21/2017 12:57 PM, Hans Verkuil wrote: From: Hans Verkuil Since the driver_version field in struct media_device is no longer used, just remove it. Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [PATCHv2 2/5] s3c-camif: don't set driver_version
On 07/21/2017 12:57 PM, Hans Verkuil wrote: From: Hans Verkuil This field will be removed as it is not needed anymore. Signed-off-by: Hans Verkuil Reviewed-by: Sylwester Nawrocki
Re: [PATCH v2] media: Convert to using %pOF instead of full_name
On 07/21/2017 09:28 PM, Rob Herring wrote: Now that we have a custom printf format specifier, convert users of full_name to use %pOF instead. This is preparation to remove storing of the full path string for each node. Signed-off-by: Rob Herring Acked-by: Niklas Söderlund Acked-by: Laurent Pinchart Reviewed-by: Matthias Brugger Acked-by: Nicolas Ferre Acked-by: Lad, Prabhakar --- v2: - Fix missing to_of_node() in xilinx-vipp.c - Drop v4l2-async.c changes. Doing as revert instead. - Add acks Reviewed-by: Sylwester Nawrocki
[GIT PULL FOR 4.14] Samsung SoC related updates
Hi Mauro, The following changes since commit 6538b02d210f52ef2a2e67d59fcb58be98451fbd: media: Make parameter of media_entity_remote_pad() const (2017-07-20 16:54:04 -0400) are available in the git repository at: git://linuxtv.org/snawrocki/samsung.git for-v4.14/media/next for you to fetch changes up to c7782331ca78c9b84485051365c1aaceac6c634c: exynos4-is: fimc-is-i2c: constify dev_pm_ops structures. (2017-07-21 13:22:40 +0200) Arvind Yadav (1): exynos4-is: fimc-is-i2c: constify dev_pm_ops structures. Gustavo A. R. Silva (1): s5k5baf: remove unnecessary static in s5k5baf_get_selection() Thierry Escande (3): s5p-jpeg: Handle parsing error in s5p_jpeg_parse_hdr() s5p-jpeg: Don't use temporary structure in s5p_jpeg_buf_queue s5p-jpeg: Split s5p_jpeg_parse_hdr() Tony K Nadackal (3): s5p-jpeg: Call jpeg_bound_align_image after qbuf s5p-jpeg: Correct WARN_ON statement for checking subsampling s5p-jpeg: Decode 4:1:1 chroma subsampling format henryhsu (2): s5p-jpeg: Add support for resolution change event s5p-jpeg: Add stream error handling for Exynos5420 drivers/media/i2c/s5k5baf.c | 2 +- .../media/platform/exynos4-is/fimc-is-i2c.c | 2 +- drivers/media/platform/s5p-jpeg/jpeg-core.c | 186 drivers/media/platform/s5p-jpeg/jpeg-core.h | 7 + 4 files changed, 150 insertions(+), 47 deletions(-) -- Thanks, Sylwester
Re: [PATCH] media: Convert to using %pOF instead of full_name
On 07/19/2017 06:02 PM, Rob Herring wrote: diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 851f128eba22..0a385d1ff28c 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -47,9 +47,7 @@ static bool match_fwnode(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode)) return sd->fwnode == asd->match.fwnode.fwnode; - return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)), - of_node_full_name( - to_of_node(asd->match.fwnode.fwnode))); + return to_of_node(sd->fwnode) == to_of_node(asd->match.fwnode.fwnode); >> I'm afraid this will not work, please see commit d2180e0cf77dc7a7049671d5d57d "[media] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay" > Maybe I'm missing something, but how does that work exactly? Before the overlay is applied, the remote endpoint node (and its parent) can't be resolved. In the commit example, the endpoint in the media_bridge would also have to be part of the overlay. If you apply and un-apply the overlay, then the of_node (and fw_node) in the overlay is once again invalid. IOW, you should be in the same state as before the overlay was applied. The node is still around because of paranoia that actually freeing nodes would break things. It seems this paranoia is real, so i think we need to do something to prevent this from spreading. Furthermore, it does not appear that any media driver supports overlays and we have no general way to apply them in mainline yet (other than an in kernel API). So really this scenario is not one we have to support yet. Indeed, the motivation of the above mentioned commit was some out of tree driver. I don't know was the exact use case, assuming that the endpoint in the bridge node was also part of the overlay the bridge driver must have not been rescanning device tree after overlay un-apply and apply. Currently there is no other way to do this than to unbind and bind. So the bridge driver must have been referencing an already invalid node as you point out. I haven't been following DT overlays very closely, as Frank explains your change seems to be actually an improvement of current code. -- Thanks, Sylwester
Re: [PATCH] media: Convert to using %pOF instead of full_name
On 07/18/2017 11:43 PM, Rob Herring wrote: > Now that we have a custom printf format specifier, convert users of > full_name to use %pOF instead. This is preparation to remove storing > of the full path string for each node. > > Signed-off-by: Rob Herring > --- > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 3 +- > drivers/media/i2c/s5k5baf.c| 7 ++-- > drivers/media/platform/am437x/am437x-vpfe.c| 4 +- > drivers/media/platform/atmel/atmel-isc.c | 4 +- > drivers/media/platform/davinci/vpif_capture.c | 16 > drivers/media/platform/exynos4-is/fimc-is.c| 8 ++-- > drivers/media/platform/exynos4-is/fimc-lite.c | 3 +- > drivers/media/platform/exynos4-is/media-dev.c | 8 ++-- > drivers/media/platform/exynos4-is/mipi-csis.c | 4 +- > drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 6 +-- > drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 8 ++-- > drivers/media/platform/omap3isp/isp.c | 8 ++-- > drivers/media/platform/pxa_camera.c| 2 +- > drivers/media/platform/rcar-vin/rcar-core.c| 4 +- > drivers/media/platform/soc_camera/soc_camera.c | 6 +-- > drivers/media/platform/xilinx/xilinx-vipp.c| 52 > +- > drivers/media/v4l2-core/v4l2-async.c | 4 +- > drivers/media/v4l2-core/v4l2-clk.c | 3 +- > include/media/v4l2-clk.h | 4 +- > 19 files changed, 71 insertions(+), 83 deletions(-) > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c > b/drivers/media/platform/xilinx/xilinx-vipp.c > index ac4704388920..9233ad0b1b6b 100644 > --- a/drivers/media/platform/xilinx/xilinx-vipp.c > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c > @@ -144,9 +144,8 @@ static int xvip_graph_build_one(struct > xvip_composite_device *xdev, > remote = ent->entity; > > if (link.remote_port >= remote->num_pads) { > - dev_err(xdev->dev, "invalid port number %u on %s\n", > - link.remote_port, > - to_of_node(link.remote_node)->full_name); > + dev_err(xdev->dev, "invalid port number %u on %pOF\n", > + link.remote_port, link.remote_node); Shouldn't there be to_of_node(link.remote_node) instead of link.remote_node ? > v4l2_fwnode_put_link(&link); > ret = -EINVAL; > break; > @@ -242,17 +241,17 @@ static int xvip_graph_build_dma(struct > xvip_composite_device *xdev) > ent = xvip_graph_find_entity(xdev, >to_of_node(link.remote_node)); > if (ent == NULL) { > - dev_err(xdev->dev, "no entity found for %s\n", > - to_of_node(link.remote_node)->full_name); > + dev_err(xdev->dev, "no entity found for %pOF\n", > + to_of_node(link.remote_node)); > v4l2_fwnode_put_link(&link); > ret = -ENODEV; > break; > } > diff --git a/drivers/media/v4l2-core/v4l2-async.c > b/drivers/media/v4l2-core/v4l2-async.c > index 851f128eba22..0a385d1ff28c 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -47,9 +47,7 @@ static bool match_fwnode(struct v4l2_subdev *sd, struct > v4l2_async_subdev *asd) > if (!is_of_node(sd->fwnode) || !is_of_node(asd->match.fwnode.fwnode)) > return sd->fwnode == asd->match.fwnode.fwnode; > > - return !of_node_cmp(of_node_full_name(to_of_node(sd->fwnode)), > - of_node_full_name( > - to_of_node(asd->match.fwnode.fwnode))); > + return to_of_node(sd->fwnode) == to_of_node(asd->match.fwnode.fwnode); I'm afraid this will not work, please see commit d2180e0cf77dc7a7049671d5d57d "[media] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay" -- Regards, Sylwester
Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera
Hi Hugues, On 07/03/2017 11:16 AM, Hugues Fruchet wrote: > This patchset enables OV9655 camera support. > > OV9655 support has been tested using STM32F4DIS-CAM extension board > plugged on connector P1 of STM32F746G-DISCO board. > Due to lack of OV9650/52 hardware support, the modified related code > could not have been checked for non-regression. > > First patches upgrade current support of OV9650/52 to prepare then > introduction of OV9655 variant patch. > Because of OV9655 register set slightly different from OV9650/9652, > not all of the driver features are supported (controls). Supported > resolutions are limited to VGA, QVGA, QQVGA. > Supported format is limited to RGB565. > Controls are limited to color bar test pattern for test purpose. I appreciate your efforts towards making a common driver but IMO it would be better to create a separate driver for the OV9655 sensor. The original driver is 1576 lines of code, your patch set adds half of that (816). There are significant differences in the feature set of both sensors, there are differences in the register layout. I would go for a separate driver, we would then have code easier to follow and wouldn't need to worry about possible regressions. I'm afraid I have lost the camera module and won't be able to test the patch set against regressions. IMHO from maintenance POV it's better to make a separate driver. In the end of the day we wouldn't be adding much more code than it is being done now. > .../devicetree/bindings/media/i2c/ov965x.txt | 45 ++ > drivers/media/i2c/Kconfig | 6 +- > drivers/media/i2c/ov9650.c | 816 > + > 3 files changed, 736 insertions(+), 131 deletions(-) > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov965x.txt -- Thanks, Sylwester
Re: [PATCH v2 3/7] [media] ov9650: add device tree support
On 07/03/2017 11:16 AM, Hugues Fruchet wrote: > Allows use of device tree configuration data. > If no device tree data is there, configuration is taken from platform data. > In order to keep GPIOs configuration compatible between both way of doing, > GPIOs are switched to descriptor-based interface. > > Signed-off-by: H. Nikolaus Schaller > Signed-off-by: Hugues Fruchet > --- > drivers/media/i2c/Kconfig | 2 +- > drivers/media/i2c/ov9650.c | 77 > ++ > 2 files changed, 59 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 121b3b5..168115c 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -615,7 +615,7 @@ config VIDEO_OV7670 > > config VIDEO_OV9650 > tristate "OmniVision OV9650/OV9652 sensor support" > - depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on GPIOLIB && I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > ---help--- > This is a V4L2 sensor-level driver for the Omnivision > OV9650 and OV9652 camera sensors. > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > index 1e4e99e..7e9a902 100644 > --- a/drivers/media/i2c/ov9650.c > +++ b/drivers/media/i2c/ov9650.c > @@ -11,12 +11,14 @@ >* it under the terms of the GNU General Public License version 2 as >* published by the Free Software Foundation. >*/ > +#include > #include > #include > #include > #include > #include > #include > +#include > #include > #include > #include > @@ -249,9 +251,10 @@ struct ov965x { > struct v4l2_subdev sd; > struct media_pad pad; > enum v4l2_mbus_type bus_type; > - int gpios[NUM_GPIOS]; > + struct gpio_desc *gpios[NUM_GPIOS]; > /* External master clock frequency */ > unsigned long mclk_frequency; > + struct clk *clk; > > /* Protects the struct fields below */ > struct mutex lock; > @@ -511,10 +514,10 @@ static int ov965x_set_color_matrix(struct ov965x > *ov965x) > return 0; > } > > -static void ov965x_gpio_set(int gpio, int val) > +static void ov965x_gpio_set(struct gpio_desc *gpio, int val) > { > - if (gpio_is_valid(gpio)) > - gpio_set_value(gpio, val); > + if (gpio) > + gpiod_set_value_cansleep(gpio, val); > } > > static void __ov965x_set_power(struct ov965x *ov965x, int on) > @@ -1406,24 +1409,28 @@ static int ov965x_configure_gpios(struct ov965x > *ov965x, > const struct ov9650_platform_data *pdata) > { > int ret, i; > + int gpios[NUM_GPIOS]; > > - ov965x->gpios[GPIO_PWDN] = pdata->gpio_pwdn; > - ov965x->gpios[GPIO_RST] = pdata->gpio_reset; > + gpios[GPIO_PWDN] = pdata->gpio_pwdn; > + gpios[GPIO_RST] = pdata->gpio_reset; > > - for (i = 0; i < ARRAY_SIZE(ov965x->gpios); i++) { > - int gpio = ov965x->gpios[i]; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { > + int gpio = gpios[i]; > > if (!gpio_is_valid(gpio)) > continue; > ret = devm_gpio_request_one(&ov965x->client->dev, gpio, > - GPIOF_OUT_INIT_HIGH, "OV965X"); > - if (ret < 0) > + GPIOF_OUT_INIT_HIGH, DRIVER_NAME); > + if (ret < 0) { > + dev_err(&ov965x->client->dev, > + "Failed to request gpio%d (%d)\n", gpio, ret); > return ret; > + } > v4l2_dbg(1, debug, &ov965x->sd, "set gpio %d to 1\n", gpio); > > gpio_set_value(gpio, 1); > gpio_export(gpio, 0); > - ov965x->gpios[i] = gpio; > + ov965x->gpios[i] = gpio_to_desc(gpio); > } > > return 0; > @@ -1469,14 +1476,10 @@ static int ov965x_probe(struct i2c_client *client, > struct v4l2_subdev *sd; > struct ov965x *ov965x; > int ret; > + struct device_node *np = client->dev.of_node; > > - if (pdata == NULL) { > - dev_err(&client->dev, "platform data not specified\n"); > - return -EINVAL; > - } > - > - if (pdata->mclk_frequency == 0) { > - dev_err(&client->dev, "MCLK frequency not specified\n"); > + if (!pdata && !np) { > + dev_err(&client->dev, "Platform data or device tree data must > be provided\n"); > return -EINVAL; > } > > @@ -1486,7 +1489,35 @@ static int ov965x_probe(struct i2c_client *client, > > mutex_init(&ov965x->lock); > ov965x->client = client; > - ov965x->mclk_frequency = pdata->mclk_frequency; > + mutex_init(&ov965x->lock); Are you initializing the mutex twice? > + if (np) { > + /* Device tree */ > + ov965x->gpios[GPIO_RST] = > + devm_gpiod_get_optional(&c
Re: [PATCH v2 2/7] [media] ov9650: switch i2c device id to lower case
On 07/03/2017 11:16 AM, Hugues Fruchet wrote: > Switch i2c device id to lower case as it is s/i2c/I2C ? > done for other omnivision cameras. s/omnivision/Omnivision This is required for properly matching driver with device on DT platforms, right? It might be worth to mention that so it is clear why we break any non-dt platform that could be already using this driver. There seem to be none in the mainline kernel tree though. > Signed-off-by: Hugues Fruchet Reviewed-by: Sylwester Nawrocki > Signed-off-by: Hugues Fruchet > --- > drivers/media/i2c/ov9650.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > index 2de2fbb..1e4e99e 100644 > --- a/drivers/media/i2c/ov9650.c > +++ b/drivers/media/i2c/ov9650.c > @@ -1545,8 +1545,8 @@ static int ov965x_remove(struct i2c_client *client) > } > > static const struct i2c_device_id ov965x_id[] = { > - { "OV9650", 0 }, > - { "OV9652", 0 }, > + { "ov9650", 0 }, > + { "ov9652", 0 }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(i2c, ov965x_id);
Re: [PATCH v2 0/7] [PATCH v2 0/7] Add support of OV9655 camera
Hi Hugues, On 07/06/2017 09:51 AM, Hugues FRUCHET wrote: Hi Sylwester, Do you have the possibility to check for non-regression of this patchset on 9650/52 camera ? I will try to test your patch set once I find the camera module for my Micro2440SDK board. I've spent already a day on setting up everything and fixing multiple regressions in the kernel. I will likely try your patch series in coming week. -- Thanks, Sylwester
Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX
On 07/06/2017 12:24 PM, Jose Abreu wrote: >>> +- edid-phandle: phandle to the EDID handler block. >> >> Could you make this property optional and when it is missing assume that >> device >> corresponding to the parent node of this node handles EDID? This way we could >> avoid having property pointing to the parent node. > > Hmm, this is for the CEC notifier. Do you mean I should grab the > parent device for the notifier? This property is already optional > if cec is not enabled though. Yes, device associated with the parent node. Something like: - edid-phandle - phandle to the EDID handler block; if this property is not specified it is assumed that EDID is handled by device described by parent node of the HDMI RX node Not sure if it is any better than always requiring edid-phandle property, even when it is pointing to the parent node. We would need a DT maintainer's opinion on that. -- Thanks, Sylwester
Re: [PATCH v6 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX
On 07/04/2017 04:11 PM, Jose Abreu wrote: > Document the bindings for the Synopsys Designware HDMI RX. > > Signed-off-by: Jose Abreu > --- > .../devicetree/bindings/media/snps,dw-hdmi-rx.txt | 70 > ++ > 1 file changed, 70 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt Could you make the DT binding documentation patch first patch in the series? Now checkpatch will complain about undocumented compatible string when the driver patches are applied alone. > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt >b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt > new file mode 100644 > index 000..449b8a2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt > @@ -0,0 +1,70 @@ > +Synopsys DesignWare HDMI RX Decoder > +=== > + > +This document defines device tree properties for the Synopsys DesignWare HDMI > +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding > +specification by itself but is meant to be referenced by platform-specific > +device tree bindings. > + > +When referenced from platform device tree bindings the properties defined in > +this document are defined as follows. It would be good to make it clear which properties are required and which are optional. And also to mention the properties below belong to the HDMI RX node. > +- compatible: Shall be "snps,dw-hdmi-rx". > + > +- reg: Memory mapped base address and length of the DWC HDMI RX registers. > + > +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt. s/5v/HDMI 5V ? > + > +- clocks: Phandle to the config clock block. > + > +- clock-names: Shall be "cfg". > + > +- edid-phandle: phandle to the EDID handler block. Could you make this property optional and when it is missing assume that device corresponding to the parent node of this node handles EDID? This way we could avoid having property pointing to the parent node. > +- #address-cells: Shall be 1. > + > +- #size-cells: Shall be 0. > + > +You also have to create a subnode for phy driver. Phy properties are as > follows. s/phy driver. Phy/the PHY device. PHY ? Might be also worth to make it explicit these are all required properties. > +- compatible: Shall be "snps,dw-hdmi-phy-e405". > + > +- reg: Shall be JTAG address of phy. s/phy/the PHY ? > +- clocks: Phandle for cfg clock. > + > +- clock-names:Shall be "cfg". > + > +A sample binding is now provided. The compatible string is for a SoC which > has > +has a Synopsys DesignWare HDMI RX decoder inside. > + > +Example: > + > +dw_hdmi_soc: dw-hdmi-soc@0 { > + compatible = "snps,dw-hdmi-soc"; Perhaps just make it compatible = "..."; ? > + reg = <0x11c00 0x1000>; /* EDIDs */ This is not relevant and undocumented, will likely be part of documentation of other binding thus I'd suggest dropping this reg property. > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + hdmi-rx@0 { > + compatible = "snps,dw-hdmi-rx"; > + reg = <0x0 0x1>; > + interrupts = <1 2>; > + edid-phandle = <&dw_hdmi_soc>; > + > + clocks = <&dw_hdmi_refclk>; > + clock-names = "cfg"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + hdmi-phy@fc { > + compatible = "snps,dw-hdmi-phy-e405"; > + reg = <0xfc>; > + > + clocks = <&dw_hdmi_refclk>; > + clock-names = "cfg"; > + }; > + }; > +}; Otherwise looks good. I'll likely not have comments to the other patches. -- Regards, Sylwester
Re: [PATCH v2 3/3] [media] intel-ipu3: cio2: Add new MIPI-CSI2 driver
Hi, On 06/28/2017 03:31 PM, Sakari Ailus wrote: IMO VB2/V4L2 could better support conversion between single and multi-planar buffer types so that the applications could just use any and drivers could manage with one. I don't have a strong opinion either way, but IMO this could be well addressed later on by improving the framework when (or if) the support for formats such as NV12 is added. We had already conversion between single and multi-planar buffer types in the kernel. But for some reasons it got removed. [1] The conversion is supposed to be done in libv4l2, which is not mandatory so it cannot be used to ensure backward compatibility while moving driver from one API to the other. [1] commit 1d0c86cad38678fa42f6d048a7b9e4057c8c16fc [media] media: v4l: remove single to multiplane conversion Regards, Sylwester
Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module
On 06/28/2017 11:12 AM, H. Nikolaus Schaller wrote: >> Am 28.06.2017 um 00:57 schrieb Sylwester Nawrocki : >> On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote: >>>> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki : >>>> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote: >>>>>> What I am missing to support the GTA04 camera is the control of the >>>>>> optional "vana-supply". >>>>>> So the driver does not power up the camera module when needed and >>>>>> therefore probing fails. >>>>>> >>>>>> - vana-supply: a regulator to power up the camera module. >>>>>> >>>>>> Driver code is not complex to add: >>>> >>>>> Yes, I saw it in your code, but as I don't have any programmable power >>>>> supply on my setup, I have not pushed this commit. >>>> >>>> Since you are about to add voltage supplies to the DT binding I'd suggest >>>> to include all three voltage supplies of the sensor chip. Looking at the >>>> OV9650 >>>> and the OV9655 datasheet there are following names used for the voltage >>>> supply >>>> pins: >>>> >>>> AVDD - Analog power supply, >>>> DVDD - Power supply for digital core logic, >>>> DOVDD - Digital power supply for I/O. >>> >>> The latter two are usually not independently switchable from the SoC power >>> the module is connected to. >>> >>> And sometimes DVDD and DOVDD are connected together. >>> >>> So the driver can't make much use of knowing or requesting them because the >>> 1.8V supply is always active, even during suspend. >>> >>>> >>>> I doubt the sensor can work without any of these voltage supplies, thus >>>> regulator_get_optional() should not be used. I would just use the regulator >>>> bulk API to handle all three power supplies. >>> >>> The digital part works with AVDD turned off. So the LDO supplying AVDD >>> should >>> be switchable to save power (&vaux3 on the GTA04 device).> >>> But not all designs can switch it off. Hence the idea to define it as an >>> /optional/ regulator. If it is not defined by DT, the driver simply assumes >>> it is always powered on. >> >> I didn't say we can't define regulator supply properties as optional in the >> DT >> binding. If we define them as such and any of these *-supply properties is >> missing in DT with regulator_get() the regulator core will use dummy >> regulator >> for that particular voltage supply. While with regulator_get_optional() >> -ENODEV is returned when the regulator cannot be found. > > Ah, ok. I see. > > I had thought that it is the right thing to do like devm_gpiod_get_optional(). > > That one it is described as: > > "* This is equivalent to gpiod_get(), except that when no GPIO was assigned to > * the requested function it will return NULL. This is convenient for drivers > * that need to handle optional GPIOs." > > Seems to be inconsistent definition of what "optional" means. Indeed, this commit explains it further: commit de1dd9fd2156874b45803299b3b27e65d5defdd9 regulator: core: Provide hints to the core about optional supplies > So we indeed should use devm_regulator_get() in this case. Thanks for > > pointing out! >>> So in summary we only need AVDD switched for the GTA04 - but it does not >>> matter if the others are optional properties. We would not use them. >>> >>> It does matter if they are mandatory because it adds DT complexity (size >>> and processing) without added function. >> >> We should not be defining DT binding only with selected use cases/board >> designs in mind. IMO all three voltage supplies should be listed in the >> binding, presumably all can be made optional, with an assumption that when >> the property is missing selected pin is hooked up to a fixed regulator. > > Ok, then it should just be defined in the bindings but not used by > the driver? Yes, I think so. So we have a possibly complete binding right from the beginning. I someone needs handling other supplies than AVDD they could update the driver in future. Regards, Sylwester
Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
Hi Jose, On 06/28/2017 11:25 AM, Jose Abreu wrote: On 27-06-2017 21:34, Sylwester Nawrocki wrote: On 06/27/2017 10:43 AM, Jose Abreu wrote: On 25-06-2017 22:13, Sylwester Nawrocki wrote: On 06/20/2017 07:26 PM, Jose Abreu wrote: This is an initial submission for the Synopsys Designware HDMI RX Controller Driver. This driver interacts with a phy driver so that a communication between them is created and a video pipeline is configured. Signed-off-by: Jose Abreu The modules are installed but I think I don't have udev :/ I'm running this on an embedded platform called ARC AXS and I'm using buildroot with minimal options. OK, then let's keep this request_module. +static int dw_hdmi_v4l2_notify_complete(struct v4l2_async_notifier *notifier) +{ + struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier); + int ret; + + ret = v4l2_device_register_subdev_nodes(&dw_dev->v4l2_dev); There shouldn't be multiple struct v4l2_device instances, instead we should have only one created by the main driver. AFAIU, in your case it would be driver associated with the dw-hdmi-soc DT node. And normally such a top level driver creates subdev device nodes when its all required sub-devices are available. I think this patch could be useful for you: https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.linuxtv. org_patch_41834&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19I Jiuxx_p_Rzo2g-uHDKw&m=uNHRQkbP_-z8v5d30KFx9pcPEUhlr4ciWY3ZDAVExTA&s=dB9wpgeP 7AJg1eDRty0-RKhq3DY-7J5srIzyVoJey5I&e= With that the dw-hdmi-soc driver would have it's v4l2-async notifier's notify_complete() callback called only when both the hdmi-rx and the hdmi-phy subdevs are registered. Yeah, I saw the patches. I just implemented this way because they are not merged yet, right? >> I think these patches will be merged in v4.14-rc1, so together with your driver. You could apply them locally and indicate that your series depends on them in the cover letter. Ok, will apply them locally and re-test. Thanks. + if (ret) { + dev_err(dw_dev->dev, "failed to register subdev nodes\n"); + return ret; + } + + return 0; +} +static int dw_hdmi_rx_probe(struct platform_device *pdev) +{ + /* V4L2 initialization */ + sd = &dw_dev->sd; + v4l2_subdev_init(sd, &dw_hdmi_sd_ops); + strlcpy(sd->name, dev_name(dev), sizeof(sd->name)); + sd->dev = dev; + sd->internal_ops = &dw_hdmi_internal_ops; + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS; >>>> Don't you also need V4L2_SUBDEV_FL_HAS_DEVNODE flag set? Ouch. Yes I need otherwise the subdev will not be associated with the v4l2_device. >> This flag indicates that the v4l2 subdev device node (/dev/v4l-subdev?) should be created for this subdevice. Ok, will add for controller driver only then: I think for phy this should not be added because controller is responsible to manage phy entirely so creating a /dev/ which can be seen by userspace can lead to confusion, maybe? Right, there should be no need for the PHY. It's up to you to set it or not for the RX controller subdev. I just got confused because in your dw_hdmi_sd_ops data structure there are ops which would suggest that device node is used. Regards, Sylwester
Re: [PATCH 2/3] media: ti-vpe: cal: use of_graph_get_remote_endpoint()
On 06/28/2017 02:33 AM, Kuninori Morimoto wrote: From: Kuninori Morimoto Now, we can use of_graph_get_remote_endpoint(). Let's use it. Signed-off-by: Kuninori Morimoto Reviewed-by: Sylwester Nawrocki
Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module
On 06/27/2017 07:48 AM, H. Nikolaus Schaller wrote: >> Am 26.06.2017 um 22:04 schrieb Sylwester Nawrocki : >> >> On 06/26/2017 12:35 PM, Hugues FRUCHET wrote: >>>> What I am missing to support the GTA04 camera is the control of the >>>> optional "vana-supply". >>>> So the driver does not power up the camera module when needed and >>>> therefore probing fails. >>>> >>>> - vana-supply: a regulator to power up the camera module. >>>> >>>> Driver code is not complex to add: >> >>> Yes, I saw it in your code, but as I don't have any programmable power >>> supply on my setup, I have not pushed this commit. >> >> Since you are about to add voltage supplies to the DT binding I'd suggest >> to include all three voltage supplies of the sensor chip. Looking at the >> OV9650 >> and the OV9655 datasheet there are following names used for the voltage >> supply >> pins: >> >> AVDD - Analog power supply, >> DVDD - Power supply for digital core logic, >> DOVDD - Digital power supply for I/O. > > The latter two are usually not independently switchable from the SoC power > the module is connected to. > > And sometimes DVDD and DOVDD are connected together. > > So the driver can't make much use of knowing or requesting them because the > 1.8V supply is always active, even during suspend. > >> >> I doubt the sensor can work without any of these voltage supplies, thus >> regulator_get_optional() should not be used. I would just use the regulator >> bulk API to handle all three power supplies. > > The digital part works with AVDD turned off. So the LDO supplying AVDD should > be switchable to save power (&vaux3 on the GTA04 device).> > But not all designs can switch it off. Hence the idea to define it as an > /optional/ regulator. If it is not defined by DT, the driver simply assumes > it is always powered on. I didn't say we can't define regulator supply properties as optional in the DT binding. If we define them as such and any of these *-supply properties is missing in DT with regulator_get() the regulator core will use dummy regulator for that particular voltage supply. While with regulator_get_optional() -ENODEV is returned when the regulator cannot be found. > So in summary we only need AVDD switched for the GTA04 - but it does not > matter if the others are optional properties. We would not use them. > > It does matter if they are mandatory because it adds DT complexity (size > and processing) without added function. We should not be defining DT binding only with selected use cases/board designs in mind. IMO all three voltage supplies should be listed in the binding, presumably all can be made optional, with an assumption that when the property is missing selected pin is hooked up to a fixed regulator. -- Thanks, Sylwester
Re: [PATCH v4 2/4] [media] platform: Add Synopsys Designware HDMI RX Controller Driver
Hi Jose, On 06/27/2017 10:43 AM, Jose Abreu wrote: > On 25-06-2017 22:13, Sylwester Nawrocki wrote: >> On 06/20/2017 07:26 PM, Jose Abreu wrote: >>> This is an initial submission for the Synopsys Designware HDMI RX >>> Controller Driver. This driver interacts with a phy driver so that >>> a communication between them is created and a video pipeline is >>> configured. >>> Signed-off-by: Jose Abreu >>> +static int dw_hdmi_phy_init(struct dw_hdmi_dev *dw_dev) >>> +{ >>> + struct dw_phy_pdata *phy = &dw_dev->phy_config; >>> + const struct of_device_id *of_id; >>> + struct of_dev_auxdata lookup; >> struct of_dev_auxdata lookup = { }; >> >> You could initialize to 0 here and... >> >>> + struct device_node *child; >>> + const char *drvname; >>> + int ret; >>> + >>> + child = dw_hdmi_get_phy_of_node(dw_dev, &of_id); >>> + if (!child || !of_id || !of_id->data) { >>> + dev_err(dw_dev->dev, "no supported phy found in DT\n"); >>> + return -EINVAL; >>> + } >>> + >>> + drvname = of_id->data; >>> + phy->funcs = &dw_hdmi_phy_funcs; >>> + phy->funcs_arg = dw_dev; >>> + >>> + lookup.compatible = (char *)of_id->compatible; >>> + lookup.phys_addr = 0x0; >>> + lookup.name = NULL; >> >> ...drop these two assignments. > > Ok. > >>> + lookup.platform_data = phy; >>> + >>> + request_module(drvname); >> >> I'd say this request_module() is not needed when you use the v4l2-async >> subnotifiers and the modules are properly installed in the file system. >> I might be missing something though. > > Hmm, well I didn't actually test without request_module but I > think its needed, otherwise I would have to do: > > modprobe phy_module > modprobe controller_module > > With request_module I just have to do: > > modprobe controller_module If you are sure you need it I'm not against that. But assuming you have udev in your system it should also work like this, without request_module(): 1. modprobe controller_module -> phy device is created in the kernel, uevent sent 2. udev receives uevent, finds matching module and does modprobe phy_module Remaining part is as before: phy_module registers the driver which gets matched with phy device; probe() is called which registers v4l2 subdev which then is registered to v4l2_device through the v4l2-async mechanism. All this assumes udev is running and modules are installed in /lib/modules/$(uname -r). E.g. there should be your module alias as shown by modinfo phy_module in /lib/modules/$(uname -r)/modules.alias. >>> + ret = of_platform_populate(dw_dev->of_node, NULL, &lookup, dw_dev->dev); >>> + if (ret) { >>> + dev_err(dw_dev->dev, "failed to populate phy driver\n"); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static void dw_hdmi_phy_exit(struct dw_hdmi_dev *dw_dev) >>> +{ >>> + of_platform_depopulate(dw_dev->dev); >>> +} >>> +static int dw_hdmi_v4l2_notify_complete(struct v4l2_async_notifier >>> *notifier) >>> +{ >>> + struct dw_hdmi_dev *dw_dev = notifier_to_dw_dev(notifier); >>> + int ret; >>> + >>> + ret = v4l2_device_register_subdev_nodes(&dw_dev->v4l2_dev); >> >> There shouldn't be multiple struct v4l2_device instances, instead we should >> have only one created by the main driver. AFAIU, in your case it would be >> driver associated with the dw-hdmi-soc DT node. And normally such a top >> level >> driver creates subdev device nodes when its all required sub-devices are >> available. >> >> I think this patch could be useful for you: >> https://patchwork.linuxtv.org/patch/41834 >> >> With that the dw-hdmi-soc driver would have it's v4l2-async notifier's >> notify_complete() callback called only when both the hdmi-rx and the >> hdmi-phy subdevs are registered. > > Yeah, I saw the patches. I just implemented this way because they > are not merged yet, right? I think these patches will be merged in v4.14-rc1, so together with your driver. You could apply them locally and indicate that your series depends on them in the cover letter. >>> + if (ret) { >>> + dev_err(dw_dev->dev, "failed to register subdev nodes\n"); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> +static int dw_hdmi_rx_probe(struct platform_device *pdev) >>> +{ >>> + /* V4L2 initialization */ >>> + sd = &dw_dev->sd; >>> + v4l2_subdev_init(sd, &dw_hdmi_sd_ops); >>> + strlcpy(sd->name, dev_name(dev), sizeof(sd->name)); >>> + sd->dev = dev; >>> + sd->internal_ops = &dw_hdmi_internal_ops; >>> + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS; >> >> Don't you also need V4L2_SUBDEV_FL_HAS_DEVNODE flag set? > > Ouch. Yes I need otherwise the subdev will not be associated with > the v4l2_device. This flag indicates that the v4l2 subdev device node (/dev/v4l-subdev?) should be created for this subdevice. --- Regards, Sylwester
Re: [PATCH v1 1/6] DT bindings: add bindings for ov965x camera module
On 06/26/2017 12:35 PM, Hugues FRUCHET wrote: >> What I am missing to support the GTA04 camera is the control of the optional >> "vana-supply". >> So the driver does not power up the camera module when needed and therefore >> probing fails. >> >> - vana-supply: a regulator to power up the camera module. >> >> Driver code is not complex to add: > Yes, I saw it in your code, but as I don't have any programmable power > supply on my setup, I have not pushed this commit. Since you are about to add voltage supplies to the DT binding I'd suggest to include all three voltage supplies of the sensor chip. Looking at the OV9650 and the OV9655 datasheet there are following names used for the voltage supply pins: AVDD - Analog power supply, DVDD - Power supply for digital core logic, DOVDD - Digital power supply for I/O. I doubt the sensor can work without any of these voltage supplies, thus regulator_get_optional() should not be used. I would just use the regulator bulk API to handle all three power supplies. -- Regards, Sylwester