from:"Javier Martinez Canillas"

[PATCH v2 1/2] media: ov2680: don't register the v4l2 subdevice before checking chip ID

2018-09-01 Thread Javier Martinez Canillas

The driver registers the v4l2 subdevice before attempting to power on the
chip and checking its ID. This means that a media device driver that it's
waiting for this subdevice to be bound, will prematurely expose its media
device node to userspace because if something goes wrong the media entity
will be cleaned up again on the ov2680 probe function.

This also simplifies the probe function error path since no initialization
is made before attempting to enable the resources or checking the chip ID.

Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
Signed-off-by: Javier Martinez Canillas 

---

Changes in v2:
- Just move ov2680_check_id() before ov2680_v4l2_init() - Suggested by Sakari.

 drivers/media/i2c/ov2680.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index f753a1c333ef..3ccd584568fb 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -1088,26 +1088,20 @@ static int ov2680_probe(struct i2c_client *client)
 
mutex_init(>lock);
 
-   ret = ov2680_v4l2_init(sensor);
+   ret = ov2680_check_id(sensor);
if (ret < 0)
goto lock_destroy;
 
-   ret = ov2680_check_id(sensor);
+   ret = ov2680_v4l2_init(sensor);
if (ret < 0)
-   goto error_cleanup;
+   goto lock_destroy;
 
dev_info(dev, "ov2680 init correctly\n");
 
return 0;
 
-error_cleanup:
-   dev_err(dev, "ov2680 init fail: %d\n", ret);
-
-   media_entity_cleanup(>sd.entity);
-   v4l2_async_unregister_subdev(>sd);
-   v4l2_ctrl_handler_free(>ctrls.handler);
-
 lock_destroy:
+   dev_err(dev, "ov2680 init fail: %d\n", ret);
mutex_destroy(>lock);
 
return ret;
-- 
2.17.1

Re: [PATCH] media: ov2680: register the v4l2 subdev async at the end of probe

2018-09-01 Thread Javier Martinez Canillas

Hi Sakari,

Thanks for the feedback.

On 09/01/2018 01:46 PM, Sakari Ailus wrote:
> Hi Javier,
> 
> On Fri, Aug 31, 2018 at 05:19:06PM +0200, Javier Martinez Canillas wrote:
>> The driver registers the subdev async in the middle of the probe function
>> but this has to be done at the very end of the probe function to prevent
>> registering a device whose probe function could fail (i.e: the clock and
>> regulators enable can fail, the I2C transfers could return errors, etc).
>>
>> It could also lead to a media device driver that is waiting to bound the
>> v4l2 subdevice to incorrectly expose its media device to userspace, since
>> the subdev is registered but later its media entity is cleaned up on error.
>>
>> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
>> Signed-off-by: Javier Martinez Canillas 
>>
>> ---
>>
>>  drivers/media/i2c/ov2680.c | 9 -
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
>> index f753a1c333ef..2ef920a17278 100644
>> --- a/drivers/media/i2c/ov2680.c
>> +++ b/drivers/media/i2c/ov2680.c
>> @@ -983,10 +983,6 @@ static int ov2680_v4l2_init(struct ov2680_dev *sensor)
>>  
>>  sensor->sd.ctrl_handler = hdl;
>>  
>> -ret = v4l2_async_register_subdev(>sd);
>> -if (ret < 0)
>> -goto cleanup_entity;
>> -
>>  return 0;
>>  
>>  cleanup_entity:
>> @@ -1096,6 +1092,10 @@ static int ov2680_probe(struct i2c_client *client)
>>  if (ret < 0)
>>  goto error_cleanup;
> 
> How about instead moving ov2680_check_id() call earlier in probe()? That
> would seem to be a better fix: the driver should check the device is around
> before registering anything.
>

Yes, that would work too. We can't move it too early though since it has to
be after the DT was parsed, the regulator, clocks and gpio looked up, etc.

But moving ov2680_check_id() before ov2680_v4l2_init() would work indeed,
in that case ov2680_v4l2_init() should be renamed to ov2680_v4l2_register()
I think to make it clear that the function not only does initialization but
also register the v4l2 subdevice.

I'll post a v2 doing what you suggest.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"

2018-08-01 Thread Javier Martinez Canillas

Hi Marco,

On 08/01/2018 05:49 PM, Marco Felsch wrote:
> Hi Javier,
> 
> On 18-07-31 15:30, Marco Felsch wrote:
>> Hi Javier,
>>
>> On 18-07-31 14:52, Javier Martinez Canillas wrote:
>>> Hi Marco,
>>>
>>> On 07/31/2018 02:36 PM, Marco Felsch wrote:
>>>
>>> [snip]
>>>
>>>>>
>>>>> Yes, another thing that patch 19/22 should take into account is DTs that
>>>>> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
>>>>> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
>>>>>
>>>>> In other words, it should work both when input connectors are defined in
>>>>> the DT and when these are not defined and only an output port is defined.
>>>>
>>>> Yes, it would be a approach to map the output port dynamicaly to the
>>>> highest port number. I tried to keep things easy by a static mapping.
>>>> Maybe a follow up patch can change this behaviour.
>>>>
>>>> Anyway, input connectors aren't required. There must be at least one
>>>> port child node with a correct port-number in the DT.
>>>>
>>>
>>> Yes, that was my point. But your patch uses the port child reg property as
>>> the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.
>>>
>>> If there's only one port child (for the output) then the DT binding says
>>> that the reg property isn't required, so this will be 0 and your patch will
>>> wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
>>> should be the first one in your enum tvp5150_ports and not the last one.
>>
>> Yes, now I got you. I implemted this in such a way in my first apporach.
>> But at the moment I don't know why I changed this. Maybe to keep the
>> decoder->input number in sync with the em28xx devices, which will set the
>> port by the s_routing() callback.
>>
>> Let me check this.
> 
> I checked it again. Your're right, it should be doable but IMHO it isn't
> the right solution. I checked some drivers which use of_graph and all of
> them put the output at the end. So the tvp5150 will be the only one
> which maps the out put to the first pad and it isn't intuitive.
> 

Ah, I didn't notice that the tvp5150 was the exception. I just mentioned due
DT maintainers always ask for driver changes to be DTB backward compatible.

> I discused it with a colleague. We think a better solution would be to fix
> the v4l2-core parser code to allow a independent dt-port<->pad mapping.
> Since now the pad's correspond to the port number. This mapping should
> be done by a driver callback, so each driver can do it's own custom
> mapping.
>

That sounds good to me.
 
> Regards,
> Marco
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"

2018-07-31 Thread Javier Martinez Canillas

Hi Marco,

On 07/31/2018 02:36 PM, Marco Felsch wrote:

[snip]

>>
>> Yes, another thing that patch 19/22 should take into account is DTs that
>> don't have input connectors defined. So probably TVP5150_PORT_YOUT should
>> be 0 instead of TVP5150_PORT_NUM - 1 as is the case in the current patch.
>>
>> In other words, it should work both when input connectors are defined in
>> the DT and when these are not defined and only an output port is defined.
> 
> Yes, it would be a approach to map the output port dynamicaly to the
> highest port number. I tried to keep things easy by a static mapping.
> Maybe a follow up patch can change this behaviour.
> 
> Anyway, input connectors aren't required. There must be at least one
> port child node with a correct port-number in the DT.
>

Yes, that was my point. But your patch uses the port child reg property as
the index for the struct device_node *endpoints[TVP5150_PORT_NUM] array.

If there's only one port child (for the output) then the DT binding says
that the reg property isn't required, so this will be 0 and your patch will
wrongly map it to TVP5150_PORT_AIP1A. That's why I said that the output port
should be the first one in your enum tvp5150_ports and not the last one.

> Regards,
> Marco
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"

2018-07-31 Thread Javier Martinez Canillas

Hi Mauro,

On 07/31/2018 12:06 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 31 Jul 2018 10:52:56 +0200
> Javier Martinez Canillas  escreveu:
> 
>> Hello Mauro,
>>
>> On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
>>> Em Thu, 28 Jun 2018 18:20:50 +0200
>>> Marco Felsch  escreveu:
>>>   
>>>> From: Javier Martinez Canillas 
>>>>
>>>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>>>> added input signals support for the tvp5150, but the approach was found
>>>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>>>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>>>
>>>> This left the driver with an undocumented (and wrong) DT parsing logic,
>>>> so lets get rid of this code as well until the input connectors support
>>>> is implemented properly.
>>>>
>>>> It's a partial revert due other patches added on top of mentioned commit
>>>> not allowing the commit to be reverted cleanly anymore. But all the code
>>>> related to the DT parsing logic and input entities creation are removed.
>>>>
>>>> Suggested-by: Laurent Pinchart 
>>>> Signed-off-by: Javier Martinez Canillas 
>>>> Acked-by: Laurent Pinchart 
>>>> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
>>>> Signed-off-by: Marco Felsch 
>>>> ---  
>>>
>>> ...
>>>   
>>>> -static int tvp5150_registered(struct v4l2_subdev *sd)
>>>> -{
>>>> -#ifdef CONFIG_MEDIA_CONTROLLER
>>>> -  struct tvp5150 *decoder = to_tvp5150(sd);
>>>> -  int ret = 0;
>>>> -  int i;
>>>> -
>>>> -  for (i = 0; i < TVP5150_INPUT_NUM; i++) {
>>>> -  struct media_entity *input = >input_ent[i];
>>>> -  struct media_pad *pad = >input_pad[i];
>>>> -
>>>> -  if (!input->name)
>>>> -  continue;
>>>> -
>>>> -  decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
>>>> -
>>>> -  ret = media_entity_pads_init(input, 1, pad);
>>>> -  if (ret < 0)
>>>> -  return ret;
>>>> -
>>>> -  ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
>>>> -  if (ret < 0)
>>>> -  return ret;
>>>> -
>>>> -  ret = media_create_pad_link(input, 0, >entity,
>>>> -  DEMOD_PAD_IF_INPUT, 0);
>>>> -  if (ret < 0) {
>>>> -  media_device_unregister_entity(input);
>>>> -  return ret;
>>>> -  }
>>>> -  }
>>>> -#endif  
>>>
>>> Hmm... I suspect that reverting this part may cause problems for drivers
>>> like em28xx when compiled with MC, as they rely that the supported demods
>>> will have 3 pads (DEMOD_NUM_PADS).
>>>  
>>
>> I don't see how this change could affect em28xx and other drivers. The 
>> function
>> tvp5150_registered() being removed here, only register the media entity and 
>> add
>> a link if input->name was set. This is set in tvp5150_parse_dt() and only if 
>> a
>> input connector as defined in the Device Tree file.
>>
>> In other words, all the code removed by this patch is DT-only and isn't used 
>> by
>> any media driver that makes use of the tvp5151.
>>
>> As mentioned in the commit message, this code has never been used (besides 
>> from
>> my testings) and should had been removed when the DT binding was reverted, 
>> but
>> for some reasons the first patch landed and the second didn't at the time.
> 
> Short answer: 
> 
> Yeah, you're right. Yet, patch 19/22 will cause regressions.
>
> Long answer:
> 
> That's easy enough to test.
> 
> Without this patch, a em28xx-based board (Terratec Grabster AV350) reports:
> 
> $ ./mc_nextgen_test -D
> digraph board {
>   rankdir=TB
>   colorscheme=x11
>   labelloc="t"
>   label="Grabster AV 350
>  driver:em28xx, bus: usb-:00:14.0-2
> "
>   intf_devnode_7 [label="intf_devnode_7\nvideo\n/dev/video0", shape=box, 
> style=filled, fillcolor=yellow]
>   intf_devnode_10 [label="intf_devnode_10\nvbi\n/dev/vbi0", shape=box, 
> style=filled, fillcolo

Re: [PATCH 18/22] partial revert of "[media] tvp5150: add HW input connectors support"

2018-07-31 Thread Javier Martinez Canillas

Hello Mauro,

On 07/30/2018 08:18 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 18:20:50 +0200
> Marco Felsch  escreveu:
> 
>> From: Javier Martinez Canillas 
>>
>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>> added input signals support for the tvp5150, but the approach was found
>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>
>> This left the driver with an undocumented (and wrong) DT parsing logic,
>> so lets get rid of this code as well until the input connectors support
>> is implemented properly.
>>
>> It's a partial revert due other patches added on top of mentioned commit
>> not allowing the commit to be reverted cleanly anymore. But all the code
>> related to the DT parsing logic and input entities creation are removed.
>>
>> Suggested-by: Laurent Pinchart 
>> Signed-off-by: Javier Martinez Canillas 
>> Acked-by: Laurent Pinchart 
>> [m.fel...@pengutronix.de: rm TVP5150_INPUT_NUM define]
>> Signed-off-by: Marco Felsch 
>> ---
> 
> ...
> 
>> -static int tvp5150_registered(struct v4l2_subdev *sd)
>> -{
>> -#ifdef CONFIG_MEDIA_CONTROLLER
>> -struct tvp5150 *decoder = to_tvp5150(sd);
>> -int ret = 0;
>> -int i;
>> -
>> -for (i = 0; i < TVP5150_INPUT_NUM; i++) {
>> -struct media_entity *input = >input_ent[i];
>> -struct media_pad *pad = >input_pad[i];
>> -
>> -if (!input->name)
>> -continue;
>> -
>> -decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
>> -
>> -ret = media_entity_pads_init(input, 1, pad);
>> -if (ret < 0)
>> -return ret;
>> -
>> -ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
>> -if (ret < 0)
>> -return ret;
>> -
>> -ret = media_create_pad_link(input, 0, >entity,
>> -DEMOD_PAD_IF_INPUT, 0);
>> -if (ret < 0) {
>> -media_device_unregister_entity(input);
>> -return ret;
>> -}
>> -}
>> -#endif
> 
> Hmm... I suspect that reverting this part may cause problems for drivers
> like em28xx when compiled with MC, as they rely that the supported demods
> will have 3 pads (DEMOD_NUM_PADS).
>

I don't see how this change could affect em28xx and other drivers. The function
tvp5150_registered() being removed here, only register the media entity and add
a link if input->name was set. This is set in tvp5150_parse_dt() and only if a
input connector as defined in the Device Tree file.

In other words, all the code removed by this patch is DT-only and isn't used by
any media driver that makes use of the tvp5151.

As mentioned in the commit message, this code has never been used (besides from
my testings) and should had been removed when the DT binding was reverted, but
for some reasons the first patch landed and the second didn't at the time.

> Thanks,
> Mauro
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: Software-only image processing for Intel "complex" cameras

2018-07-27 Thread Javier Martinez Canillas

Hi Mauro,

On Mon, Jun 25, 2018 at 3:10 PM, Mauro Carvalho Chehab
 wrote:
> Em Mon, 25 Jun 2018 09:48:56 +
> "Zheng, Jian Xu"  escreveu:
>
>> Hi Mauro,
>>
>> > -Original Message-
>> > From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
>> > ow...@vger.kernel.org] On Behalf Of Mauro Carvalho Chehab
>> > Sent: Friday, June 22, 2018 5:41 AM
>> > To: mario.limoncie...@dell.com
>> > Cc: pa...@ucw.cz; nico...@ndufresne.ca; linux-media@vger.kernel.org;
>> > sakari.ai...@linux.intel.com; niklas.soderl...@ragnatech.se; Hu, Jerry W
>> > ; Zheng, Jian Xu 
>> > Subject: Re: Software-only image processing for Intel "complex" cameras
>> >
>> > Em Fri, 22 Jun 2018 06:08:50 +0900
>> > Mauro Carvalho Chehab  escreveu:
>> >
>> > > Em Thu, 21 Jun 2018 18:58:37 +
>> > >  escreveu:
>> > >
>> > Jerry/Jian,
>> >
>> > Could you please shed a light about how a Dell 5285 hardware would be
>> > detected by the IPU3 driver and what MC graph is created by the current
>> > driver?
>>
>> Sure, Mauro. I need to check the information on the Dell 5285.
>> IPU3 driver are detected by PCI vendor id and device id.
>>
>> IPU3 CIO2 MC graph is:
>> Sensor A -> IPU3 CSI2 0(subdev) -> IPU3 CIO2 0 (video node)
>> Sensor B -> IPU3 CSI2 1(subdev) -> IPU3 CIO2 1 (video node)
>

I don't think your questions below were answered. I'll try to answer
since I've been looking at this, but Sakari or others can correct me
if I got something wrong.

> How does it detect what driver should be loaded for Sensor A and B?
>

It depends on the firmware interface used to describe the hardware
topology, for ACPI the sensors are described in a DSDT table and match
using an entry in the struct acpi_device_id table in the camera
driver. For DT the sensors are described in a DT node with a
compatible string that matches an entry in the struct of_device_id
table, or an entry in the struct i2c_device_id table as a fallback.

> Does the ACPI table identifies the sensors? If so, how the driver
> associates an specific PCI vendor ID with the corresponding sensor
> settings?
>

Camera sensors are usually not PCI devices, but I2C devices. And yes,
these are described in an ACPI table (DSDT) as mentioned before. For
example in a laptop I've the following on an disassembled DSDT table:

Device (CAM0)
{
Name (_ADR, Zero)  // _ADR: Address
Name (_HID, "OVTI5648")  // _HID: Hardware ID
Name (_CID, "OVTI5648")  // _CID: Compatible ID
Name (_DDN, "OV5648-CRDD")  // _DDN: DOS Device Name
Name (_UID, Zero)  // _UID: Unique ID
Name (_DEP, Package (0x02)  // _DEP: Dependencies
{
PMI0,
I2C4
})

Now how the drivers get loaded / registered and how the media entities
are associated is somewhat orthogonal (although the information to do
both is usually encoded in the same place).

In the case of OF, the relation between the bridge device and the
sensors is encoded in the DT using the video interface / graph binding
[0, 1]. For ACPI, the same information is encoded in an ACPI table
using a _DSD extension [2], which is quite similar to the DT binding.

Since both hardware descriptions use the same properties (port,
endpoint, etc) and semantics, drivers can use the V4L2 fwnode kAPI to
parse this information regardless of the underlying firmware interface
used to encoded this relationship. This allows the same camera drivers
to be used with either DT or ACPI.

So a bridge driver will register a v4l2 async notifier on probe and
camera drivers will register the subdevices using the v4l2 async
interface. That way the v4l2 async layer can bind the two and call the
bridge device .bound (each time that a pending device is registered)
and .complete (when there are no pending devices remaining to be
registered) callbacks.

A problem I found though is that [2] is Linux specific, so the
firmware for a machine meant to be used with Windows (i.e: Microsoft
Surface 2-in-1 computers) will have a DSDT table that's not compatible
with the v4l2 fwnode layer. One option is to override [4] the DSDT
table with one that contains the information expected by Linux, for
example by having the table in the initramfs [5]. The problem is that
it seems the distros (at least in the Fedora case) don't want to get
into the business of shipping machine specific ACPI tables.

Another option could be to have a shim layer in the kernel that
translates the information in the DSDT as used by Windows to what's
expected by Linux. This is an option that I haven't investigated yet,
but wanted to know your opinion if this could be a feasible approach.
Or if you have any other suggestions on how we should proceed with
this.

[0]: 
https://www.kernel.org/doc/Documentation/devicetree/bindings/media/video-interfaces.txt
[1]: https://www.kernel.org/doc/Documentation/devicetree/bindings/graph.txt
[2]: https://www.kernel.org/doc/Documentation/acpi/dsd/graph.txt
[3]:

Re: Devices with a front and back webcam represented as a single UVC device

2018-07-24 Thread Javier Martinez Canillas

Hi Laurent,

On Thu, Jul 12, 2018 at 3:01 PM, Laurent Pinchart
 wrote:

[snip]

>>
>> Laurent, thank you for your input on this. I thought it was a bit weird that
>> the cam on my HP X2 only had what appears to be the debug controls, so I
>> opened it up and as I suspect (after your analysis) it is using a USB
>> module for the front camera, but the back camera is a sensor directly
>> hooked with its CSI/MIPI bus to the PCB, so very likely it is using the
>> ATOMISP stuff.
>>
>> So I think that we can consider this "solved" for my 2-in-1.
>
> Great, I'll add you to the list of potential testers for an ATOMISP solution
> :-)
>

The ATOMISP driver was removed from staging by commit 51b8dc5163d
("media: staging: atomisp: Remove driver"). Do you mean that there's a
plan to bring that driver back?

Best regards,
Javier

Re: [PATCH 00/21] TVP5150 fixes and new features

2018-06-28 Thread Javier Martinez Canillas

Hi Marco,

On 06/28/2018 06:20 PM, Marco Felsch wrote:
> First some fixes were made which may possibly interesting for other
> kernel versions.
> 
> Then I picked most of the patches from Philipp [1] and ported them
> to the recent media_tree master branch [3].
> 
> But the main purpose of this series is to convert the proprietary
> connector DT property into the generic input port property. I picked commit
> ('partial revert of "[media] tvp5150: add HW input connectors support"')
> to have a clean working base and used the results of the discussion [2].
> 

Thanks for working on this! I've felt guilty that I never re-worked my
patch-set after the discussion from [2].

I'll try to review these patches next week.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

2018-06-12 Thread Javier Martinez Canillas

Hi Steve,

On 06/11/2018 11:06 PM, Steve Longerbeam wrote:

[snip]

>>
>> I've been made aware [1] that recently V4L2_FIELD_ALTERNATE has been
>> clarified [2] to specify that v4l2_mbus_fmt.height should contain the
>> number of lines per field, not per frame:
> 
> Yep! That was nagging at me as well. I noticed at least one other
> platform (omap3isp) that doubles the source pad frame height

Coincidentally I noticed this problem when testing on a board with an
omap3isp. This is the pipeline setup I've been using for a long time: 

$ media-ctl -l '"tvp5150 1-005c":1->"OMAP3 ISP CCDC":0[1]'
$ media-ctl -l '"OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
$ media-ctl -V '"OMAP3 ISP CCDC":0 [UYVY2X8 720x240 field:alternate]'
$ media-ctl -V '"OMAP3 ISP CCDC":1 [UYVY 720x480 field:interlaced-tb]'

> when the sensor reports ALTERNATE field mode, to capture a
> whole frame. Makes sense. I think the crop height will need to

As you said, the ISP doubles the source pad height, and so the sink
pad is meant to have half of the frame height and this should match
the camera sensor height. But since the tvp5150 had the full frame
height (720x480) in its source pad, this didn't match the CCDC sink
pads height which lead to .link_validate callback to return -EPIPE:

ioctl(3, VIDIOC_STREAMON, 0xbeabea18)   = -1 EPIPE (Broken pipe)

After the revert, link validation / STREAMON works correctly and the
following is what the relevant media entities look like in the graph:

- entity 15: OMAP3 ISP CCDC (3 pads, 9 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev2
pad0: Sink
[fmt:UYVY2X8/720x240 field:alternate]
<- "OMAP3 ISP CCP2":1 []
<- "OMAP3 ISP CSI2a":1 []
<- "tvp5150 1-005c":1 [ENABLED]
pad1: Source
[fmt:UYVY/720x480 field:interlaced-tb
 crop.bounds:(0,0)/720x240
 crop:(0,0)/720x240]
-> "OMAP3 ISP CCDC output":0 [ENABLED]
-> "OMAP3 ISP resizer":0 []

- entity 81: tvp5150 1-005c (4 pads, 1 link)
 type V4L2 subdev subtype Decoder flags 0
 device node name /dev/v4l-subdev8
pad0: Sink
pad1: Source
[fmt:UYVY2X8/720x240 field:alternate
 crop.bounds:(0,0)/720x480
 crop:(0,0)/720x480]
-> "OMAP3 ISP CCDC":0 [ENABLED]

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [ANN v2] Complex Camera Workshop - Tokyo - Jun, 19

2018-06-06 Thread Javier Martinez Canillas

gt;> =
>>
>> The 3A algorithm handing is highly dependent on the hardware. The
>> idea here is to allow libv4l to have a set of 3A algorithms that
>> will be specific to certain mc-based hardware.
>>
>> One requirement, if we want vendor stacks to use our solution, is that
>> it should allow allow external closed-source algorithms to run as well.
>>
>> The 3A library API must be standardized, to allow the closed-source
>> vendor implementation to be replaced by an open-source implementation
>> should someone have the time and energy (and qualifications) to write
>> one.
>>
>> Sandboxed execution of the 3A library must be possible as closed-source
>> can't always be blindly trusted. This includes the ability to wrap the
>> library in a daemon should the platform's multimedia stack wishes
>> and to avoid any direct access to the kernel devices by the 3A library
>> itself (all accesses should be marshaled by the camera stack).
>>
>> Please note that this daemon is *not* a camera daemon that would
>> communicates with the V4L2 driver through a custom back channel.
>>
>> The decision to run the 3A library in a sandboxed process or to call
>> it directly from the camera stack should be left to the camera stack
>> and to the platform integrator, and should not be visible by the 3A
>> library.
>>
>> The 3A library must be usable on major Linux-based camera stacks (the
>> Android and Chrome OS camera HALs are certainly important targets,
>> more can be added) unmodified, which will allow usage of the vendor
>> binary provided for Chrome OS or Android on regular Linux systems.
>
> This is quite an interesting idea and it would be really useful if it
> could be done. I'm kind of worried, though, about Android in
> particular, since the execution environment in Android differs
> significantly from a regular Linux distributions (including Chrome OS,
> which is not so far from such), namely:
> - different libc (bionic) and dynamic linker - I guess this could be
> solved by static linking?
> - dedicated toolchains - perhaps not much of a problem if the per-arch
> ABI is the same?
>
>>
>> It would make sense to design a modular camera stack, and try to make
>> most components as platform-independent as possible. This should include:
>>
>> - the kernel drivers (V4L2-compliant and usable without any closed-source
>>   userspace component);
>> - the 3A library
>> - any other component that could be shared (for instance a possible
>>   request API library).
>>
>> The rest of the code will mostly be glue around those components to
>> integrate them in a particular camera stack, and should be as
>> platform-agnostic as possible.
>>
>> In the case of the Android camera HAL, ideally it would be a glue that
>> could be used with different camera vendors (probably with some kind of
>> vendor-specific configuration, or possibly with a separate vendor-specific
>> component to handle pipeline configuration).
>>
>> 4 Complex camera workshop
>> =
>>
>> The workshop will be happening in Tokyo, Japan, at Jun, 19, at the
>> google offices. The location is:
>>
>> 〒106-6126 Tokyo, Minato, Roppongi, 6 Chome−10−1 Roppongi Hills Mori Tower 44F
>
> Nearest station exits:
> - Hibiya line Roppongi station exit 1c (recommended)
> - Oedo line Roppongi station exit 3 (and few minutes walk)
>
>>
>> 4.1 Physical Attendees
>> ==
>>
>> Tomasz Figa 
>> Mauro Carvalho Chehab 
>> Kieran Bingham 
>> Laurent Pinchart 
>> Niklas Söderlund 
>> Zheng, Jian Xu Zheng 
>>
>> Anywone else?
>
> Looking at latest reply in this thread:
>
> jacopo mondi 
>
> Anyone else, please tell me beforehand (at least 1-2 days before), as
> I need to take care of building access, since it's a multi-tenant
> office building. I'll contact each attendee separately with further
> details by email.
>
>>
>> 4.2. Attendees Via Google Hangouts
>> ==
>>
>> Hans Verkuil  - Via Google Hangouts - maybe only on 
>> afternoon
>> Javier Martinez Canillas  - Via Google Hangouts - only 
>> on reasonable TZ-compatible-hours
>
> What time zone would that be? I guess we could try to tweak the agenda
> to take this into account.
>

Wim, Nicolas and myself are in CEST (UTC +2). The best time for Wim to
do the PipeWire presentation would be 10:30 am CEST.

Best regards,
Javier

Re: [ANN] Meeting to discuss improvements to support MC-based cameras on generic apps

2018-06-01 Thread Javier Martinez Canillas

On Thu, May 31, 2018 at 4:19 PM, Hans Verkuil  wrote:
> On 05/31/18 15:58, Hans Verkuil wrote:
>> On 05/31/18 15:22, Mauro Carvalho Chehab wrote:
>>> Em Mon, 28 May 2018 10:43:51 -0300
>>> Mauro Carvalho Chehab  escreveu:
>>>
 Em Thu, 17 May 2018 16:07:08 -0300
 Mauro Carvalho Chehab  escreveu:

> Hi all,
>
> The goal of this e-mail is to schedule a meeting in order to discuss
> improvements at the media subsystem in order to support complex camera
> hardware by usual apps.
>
> The main focus here is to allow supporting devices with MC-based
> hardware connected to a camera.
>
> In short, my proposal is to meet with the interested parties on solving
> this issue during the Open Source Summit in Japan, e. g. between
> June, 19-22, in Tokyo.

 Let's schedule the meeting to happen in Tokyo, Japan at June, 19.

 Location yet to be defined, but it will either be together with
 OSS Japan or at Google. I'll confirm the address tomorrow.
>>>
>>> More details about the meeting:
>>>
>>> Date: June, 19
>>> Site: Google
>>> Address: 〒106-6126 Tokyo, Minato, Roppongi, 6 Chome−10−1 Roppongi Hills 
>>> Mori Tower 44F
>>>
>>> Please confirm who will be attending the meeting.
>>
>> I plan to attend the meeting via Google Hangouts.
>
> Well, the afternoon part of the meeting at least :-)
>

I also plan to attend via Google Hangouts. At least the hours that are
reasonable on my timezone.

Best regards,
Javier

Re: [RESEND PATCH] partial revert of "[media] tvp5150: add HW input connectors support"

2017-12-14 Thread Javier Martinez Canillas

Hello Mauro,

On 12/14/2017 06:02 PM, Mauro Carvalho Chehab wrote:
> Em Wed,  6 Dec 2017 01:33:05 +0100
> Javier Martinez Canillas <javi...@redhat.com> escreveu:
> 
>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>> added input signals support for the tvp5150, but the approach was found
>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>
>> This left the driver with an undocumented (and wrong) DT parsing logic,
>> so lets get rid of this code as well until the input connectors support
>> is implemented properly.
>>
>> It's a partial revert due other patches added on top of mentioned commit
>> not allowing the commit to be reverted cleanly anymore. But all the code
>> related to the DT parsing logic and input entities creation are removed.
>>
>> Suggested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>> Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>
>> Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> 
>>
>> ---
>>
>> This patch was posted about a year ago but was never merged:
>>
>> https://patchwork.kernel.org/patch/9472623/
> 
> It was a RFT, on that time.
> 

Yes, sorry if it sounded as if I was complaining. I was just mentioning and
part of the patch falling through the cracks is that I also forgot about it.

> I guess I told that before. Maybe not. Anyway, reverting it doesn't seem 
> to be the proper fix, as it will break support for existing devices, by
> removing functionality from tvp5150 driver. You should remind that, since
> the code was added, someone could be already using it, as all it is

I'm not sure about this. What I'm removing is basically dead code (unless
someone is using an undocumented Device Tree binding), since the DT binding
got already removed by commit 31e717dba1e1 ("[media] Revert "[media] tvp5150:
document input connectors DT bindings").

> needed is to have some dtb. Also, it gets rid of a lot of good work for
> no good reason. Reinserting them later while preserving the code
> copyrights could be painful.
>

I would normally agree with you, although I think that in this particular case
is better to just revert this (unused) code for the reasons I mentioned above.

But don't really have a strong opinion on this, so I'm OK with either approach.

> IMHO, the best here is to move ahead, agreeing with a DT structure
> that represents the connectors and then change the driver to
> implement it, if needed.
>

There was some agreement on the DT binding, it's just that I never found time
to implement the logic in the driver. Let's see if I can get some during the
winter holidays and finally fix this.

> Thanks,
> Mauro
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

[RESEND PATCH] partial revert of "[media] tvp5150: add HW input connectors support"

2017-12-05 Thread Javier Martinez Canillas

Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
added input signals support for the tvp5150, but the approach was found
to be incorrect so the corresponding DT binding commit 82c2ffeb217a
("[media] tvp5150: document input connectors DT bindings") was reverted.

This left the driver with an undocumented (and wrong) DT parsing logic,
so lets get rid of this code as well until the input connectors support
is implemented properly.

It's a partial revert due other patches added on top of mentioned commit
not allowing the commit to be reverted cleanly anymore. But all the code
related to the DT parsing logic and input entities creation are removed.

Suggested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>
Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

---

This patch was posted about a year ago but was never merged:

https://patchwork.kernel.org/patch/9472623/

Best regards,
Javier

 drivers/media/i2c/tvp5150.c | 142 
 1 file changed, 142 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 7b79a7498751..a7c2c0c09a8d 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -43,8 +43,6 @@ struct tvp5150 {
struct v4l2_subdev sd;
 #ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pads[DEMOD_NUM_PADS];
-   struct media_entity input_ent[TVP5150_INPUT_NUM];
-   struct media_pad input_pad[TVP5150_INPUT_NUM];
 #endif
struct v4l2_ctrl_handler hdl;
struct v4l2_rect rect;
@@ -1015,40 +1013,6 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev 
*sd,
return 0;
 }
 
-/
-   Media entity ops
- /
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-static int tvp5150_link_setup(struct media_entity *entity,
- const struct media_pad *local,
- const struct media_pad *remote, u32 flags)
-{
-   struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
-   struct tvp5150 *decoder = to_tvp5150(sd);
-   int i;
-
-   for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-   if (remote->entity == >input_ent[i])
-   break;
-   }
-
-   /* Do nothing for entities that are not input connectors */
-   if (i == TVP5150_INPUT_NUM)
-   return 0;
-
-   decoder->input = i;
-
-   tvp5150_selmux(sd);
-
-   return 0;
-}
-
-static const struct media_entity_operations tvp5150_sd_media_ops = {
-   .link_setup = tvp5150_link_setup,
-};
-#endif
-
 /
I2C Command
  /
@@ -1186,42 +1150,6 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, 
struct v4l2_tuner *vt)
return 0;
 }
 
-static int tvp5150_registered(struct v4l2_subdev *sd)
-{
-#ifdef CONFIG_MEDIA_CONTROLLER
-   struct tvp5150 *decoder = to_tvp5150(sd);
-   int ret = 0;
-   int i;
-
-   for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-   struct media_entity *input = >input_ent[i];
-   struct media_pad *pad = >input_pad[i];
-
-   if (!input->name)
-   continue;
-
-   decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
-
-   ret = media_entity_pads_init(input, 1, pad);
-   if (ret < 0)
-   return ret;
-
-   ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
-   if (ret < 0)
-   return ret;
-
-   ret = media_create_pad_link(input, 0, >entity,
-   DEMOD_PAD_IF_INPUT, 0);
-   if (ret < 0) {
-   media_device_unregister_entity(input);
-   return ret;
-   }
-   }
-#endif
-
-   return 0;
-}
-
 /* --- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1272,11 +1200,6 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
.pad = _pad_ops,
 };
 
-static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
-   .registered = tvp5150_registered,
-};
-
-
 /
I2C Client & Driver
  /
@@ -1358,12 +1281,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, 
struct device_node *np)
 {
struct v4l2_fwnode_endpoint bus_cfg;

Re: [RFT PATCH] [media] partial revert of "[media] tvp5150: add HW input connectors support"

2017-08-17 Thread Javier Martinez Canillas

Hello Philipp,

On Thu, Aug 17, 2017 at 3:05 PM, Philipp Zabel <p.za...@pengutronix.de> wrote:
> Hi,
>
> On Tue, 2016-12-13 at 12:39 -0300, Javier Martinez Canillas wrote:
>> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
>> added input signals support for the tvp5150, but the approach was found
>> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
>> ("[media] tvp5150: document input connectors DT bindings") was reverted.
>>
>> This left the driver with an undocumented (and wrong) DT parsing logic,
>> so lets get rid of this code as well until the input connectors support
>> is implemented properly.
>>
>> It's a partial revert due other patches added on top of mentioned commit
>> not allowing the commit to be reverted cleanly anymore. But all the code
>> related to the DT parsing logic and input entities creation are removed.
>>
>> > Suggested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>> > Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>
> what is the state of this patch? Was it forgotten or was the revert
> deemed unnecessary?
>

I think that was just forgotten. That code still needs to be reverted
since the DT patch was also reverted.

Albeit the code is harmless since should be a no-op if a connectors DT
node isn't found.

> regards
> Philipp
>

Best regards,
Javier

Re: [PATCH 2/3] leds: as3645a: Add LED flash class driver

2017-08-16 Thread Javier Martinez Canillas

Hello Sakari,

I haven't looked at the driver, but just have a comment about the I2C subsystem.

On Wed, Aug 16, 2017 at 2:55 PM, Sakari Ailus
 wrote:

[snip]

> +
> +static const struct of_device_id as3645a_of_table[] = {
> +   { .compatible = "ams,as3645a" },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, as3645a_of_table);
> +
> +SIMPLE_DEV_PM_OPS(as3645a_pm_ops, as3645a_resume, as3645a_suspend);
> +
> +static struct i2c_driver as3645a_i2c_driver = {
> +   .driver = {
> +   .of_match_table = as3645a_of_table,
> +   .name = AS_NAME,
> +   .pm   = _pm_ops,
> +   },
> +   .probe_new  = as3645a_probe,
> +   .remove = as3645a_remove,
> +};
> +
> +module_i2c_driver(as3645a_i2c_driver);
> +

The I2C core is still broken w.r.t reporting a proper MODALIAS for OF
registered devices, it will report a MODALIAS=i2c:as3645a in this
case. So if you build this as a module, autoload won't work.

In theory this will be fixed soon, but for now you should add a
i2c_device_id table and export it with MODULE_DEVICE_TABLE(i2c,...) if
you care about module autoloading.

Best regards,
Javier

Re: [PATCH] media: i2c: adv748x: Export I2C device table entries as module aliases

2017-08-09 Thread Javier Martinez Canillas

On 08/09/2017 01:05 PM, Kieran Bingham wrote:
> On 09/08/17 11:58, Javier Martinez Canillas wrote:
>> Hi Kieran,
>>
>> On 08/09/2017 12:29 PM, Kieran Bingham wrote:
>>> Hi Javier,
>>>
>>> Thankyou for the patch
>>
>> You are welcome.
>>  
>>> On 09/08/17 10:37, Javier Martinez Canillas wrote:
>>>> The I2C core always reports a MODALIAS of the form i2c: even if the
>>>> device was registered via OF, and the driver is only exporting the OF ID
>>>> table entries as module aliases.
>>>>
>>>> So if the driver is built as module, autoload won't work since udev/kmod
>>>> won't be able to match the registered OF device with its driver module.
>>>
>>> Good catch, and perhaps I should have known better :D
>>>
>>> I've only worked on this driver as a built-in so far :-) #BadExcuses
>>>
>>
>> A better excuse I think is that after all these years, one would had thought
>> that the I2C OF modalias issue would had been finally fixed, but not yet :)
> 
> Quite! Let's try to bubble that back up the todo list.
> Now - where did I put my free time. I'm sure I left it around here somewhere 
> :-)
> 

We are getting there though. I'm just waiting for the patches in this [0] series
to land and then I'll be able to post the I2C core uevent modalias patch.

I've asked Wolfram if he can at least pick the driver patches [1], but he didn't
answer me yet...

[0]: https://www.spinics.net/lists/arm-kernel/msg588431.html
[1]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1457427.html

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] media: i2c: adv748x: Export I2C device table entries as module aliases

2017-08-09 Thread Javier Martinez Canillas

Hi Kieran,

On 08/09/2017 12:29 PM, Kieran Bingham wrote:
> Hi Javier,
> 
> Thankyou for the patch
>

You are welcome.
 
> On 09/08/17 10:37, Javier Martinez Canillas wrote:
>> The I2C core always reports a MODALIAS of the form i2c: even if the
>> device was registered via OF, and the driver is only exporting the OF ID
>> table entries as module aliases.
>>
>> So if the driver is built as module, autoload won't work since udev/kmod
>> won't be able to match the registered OF device with its driver module.
> 
> Good catch, and perhaps I should have known better :D
> 
> I've only worked on this driver as a built-in so far :-) #BadExcuses
>

A better excuse I think is that after all these years, one would had thought
that the I2C OF modalias issue would had been finally fixed, but not yet :)
 
>> Before this patch:
>>
>> $ modinfo drivers/media/i2c/adv748x/adv748x.ko | grep alias
>> alias:  of:N*T*Cadi,adv7482C*
>> alias:  of:N*T*Cadi,adv7482
>> alias:  of:N*T*Cadi,adv7481C*
>> alias:  of:N*T*Cadi,adv7481
>>
>> After this patch:
>>
>> modinfo drivers/media/i2c/adv748x/adv748x.ko | grep alias
>> alias:  of:N*T*Cadi,adv7482C*
>> alias:  of:N*T*Cadi,adv7482
>> alias:  of:N*T*Cadi,adv7481C*
>> alias:  of:N*T*Cadi,adv7481
>> alias:  i2c:adv7482
>> alias:  i2c:adv7481
>>
>> Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> 

Thanks!

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

[PATCH] media: i2c: adv748x: Export I2C device table entries as module aliases

2017-08-09 Thread Javier Martinez Canillas

The I2C core always reports a MODALIAS of the form i2c: even if the
device was registered via OF, and the driver is only exporting the OF ID
table entries as module aliases.

So if the driver is built as module, autoload won't work since udev/kmod
won't be able to match the registered OF device with its driver module.

Before this patch:

$ modinfo drivers/media/i2c/adv748x/adv748x.ko | grep alias
alias:  of:N*T*Cadi,adv7482C*
alias:  of:N*T*Cadi,adv7482
alias:  of:N*T*Cadi,adv7481C*
alias:  of:N*T*Cadi,adv7481

After this patch:

modinfo drivers/media/i2c/adv748x/adv748x.ko | grep alias
alias:  of:N*T*Cadi,adv7482C*
alias:  of:N*T*Cadi,adv7482
alias:  of:N*T*Cadi,adv7481C*
alias:  of:N*T*Cadi,adv7481
alias:  i2c:adv7482
alias:  i2c:adv7481

Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>
---

 drivers/media/i2c/adv748x/adv748x-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c 
b/drivers/media/i2c/adv748x/adv748x-core.c
index aeb6ae80cb18..5ee14f2c2747 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -807,6 +807,7 @@ static const struct i2c_device_id adv748x_id[] = {
{ "adv7482", 0 },
{ },
 };
+MODULE_DEVICE_TABLE(i2c, adv748x_id);
 
 static const struct of_device_id adv748x_of_table[] = {
{ .compatible = "adi,adv7481", },
-- 
2.13.3

Re: [PATCH] media: vimc: cleanup a few warnings

2017-07-14 Thread Javier Martinez Canillas

On Thu, Jul 13, 2017 at 5:47 PM, Javier Martinez Canillas
<jav...@dowhile0.org> wrote:
> On Thu, Jul 13, 2017 at 5:38 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:

[snip]

>>
>> Shouldn't these be set to the corresponding driver structs' id_table
>> fields? Or do I miss something...?
>>
>
> Agreed, the real problem is that the .id_table is not set for these
> drivers. The match only works because the platform subsystem fallbacks
> to the driver's name if an .id_table isn't defined:
>

I just posted a patch fixing the build warning in the driver as
suggested by Sakari:

https://patchwork.linuxtv.org/patch/42480/

Best regards,
Javier

[PATCH] [media] vimc: set id_table for platform drivers

2017-07-14 Thread Javier Martinez Canillas

The vimc platform drivers define a platform device ID table but these
are not set to the .id_table field in the platform driver structure.

So the platform device ID table is only used to fill the aliases in
the module but are not used for matching (works because the platform
subsystem fallbacks to the driver's name if no .id_table is set).

But this also means that the platform device ID table isn't used if
the driver is built-in, which leads to the following build warning:

This causes the following build warnings when the driver is built-in:

drivers/media/platform/vimc//vimc-capture.c:528:40: warning: 
‘vimc_cap_driver_ids’ defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_cap_driver_ids[] = {
^~~
drivers/media/platform/vimc//vimc-debayer.c:588:40: warning: 
‘vimc_deb_driver_ids’ defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_deb_driver_ids[] = {
^~~
drivers/media/platform/vimc//vimc-scaler.c:442:40: warning: 
‘vimc_sca_driver_ids’ defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_sca_driver_ids[] = {
^~~
drivers/media/platform/vimc//vimc-sensor.c:376:40: warning: 
‘vimc_sen_driver_ids’ defined but not used [-Wunused-const-variable=]
 static const struct platform_device_id vimc_sen_driver_ids[] = {
^~~

Reported-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
Suggested-by: Sakari Ailus <sakari.ai...@iki.fi>
Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>

---

 drivers/media/platform/vimc/vimc-capture.c | 15 ---
 drivers/media/platform/vimc/vimc-debayer.c | 15 ---
 drivers/media/platform/vimc/vimc-scaler.c  | 15 ---
 drivers/media/platform/vimc/vimc-sensor.c  | 15 ---
 4 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c 
b/drivers/media/platform/vimc/vimc-capture.c
index 14cb32e21130..88a1e5670c72 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -517,21 +517,22 @@ static int vimc_cap_remove(struct platform_device *pdev)
return 0;
 }
 
+static const struct platform_device_id vimc_cap_driver_ids[] = {
+   {
+   .name   = VIMC_CAP_DRV_NAME,
+   },
+   { }
+};
+
 static struct platform_driver vimc_cap_pdrv = {
.probe  = vimc_cap_probe,
.remove = vimc_cap_remove,
+   .id_table   = vimc_cap_driver_ids,
.driver = {
.name   = VIMC_CAP_DRV_NAME,
},
 };
 
-static const struct platform_device_id vimc_cap_driver_ids[] = {
-   {
-   .name   = VIMC_CAP_DRV_NAME,
-   },
-   { }
-};
-
 module_platform_driver(vimc_cap_pdrv);
 
 MODULE_DEVICE_TABLE(platform, vimc_cap_driver_ids);
diff --git a/drivers/media/platform/vimc/vimc-debayer.c 
b/drivers/media/platform/vimc/vimc-debayer.c
index 35b15bd4d61d..033a131f67af 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -577,21 +577,22 @@ static int vimc_deb_remove(struct platform_device *pdev)
return 0;
 }
 
+static const struct platform_device_id vimc_deb_driver_ids[] = {
+   {
+   .name   = VIMC_DEB_DRV_NAME,
+   },
+   { }
+};
+
 static struct platform_driver vimc_deb_pdrv = {
.probe  = vimc_deb_probe,
.remove = vimc_deb_remove,
+   .id_table   = vimc_deb_driver_ids,
.driver = {
.name   = VIMC_DEB_DRV_NAME,
},
 };
 
-static const struct platform_device_id vimc_deb_driver_ids[] = {
-   {
-   .name   = VIMC_DEB_DRV_NAME,
-   },
-   { }
-};
-
 module_platform_driver(vimc_deb_pdrv);
 
 MODULE_DEVICE_TABLE(platform, vimc_deb_driver_ids);
diff --git a/drivers/media/platform/vimc/vimc-scaler.c 
b/drivers/media/platform/vimc/vimc-scaler.c
index fe77505d2679..0a3e086e12f3 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -431,21 +431,22 @@ static int vimc_sca_remove(struct platform_device *pdev)
return 0;
 }
 
+static const struct platform_device_id vimc_sca_driver_ids[] = {
+   {
+   .name   = VIMC_SCA_DRV_NAME,
+   },
+   { }
+};
+
 static struct platform_driver vimc_sca_pdrv = {
.probe  = vimc_sca_probe,
.remove = vimc_sca_remove,
+   .id_table   = vimc_sca_driver_ids,
.driver = {
.name   = VIMC_SCA_DRV_NAME,
},
 };
 
-static const struct platform_device_id vimc_sca_driver_ids[] = {
-   {
-

Re: [PATCH] media: vimc: cleanup a few warnings

2017-07-13 Thread Javier Martinez Canillas

On Thu, Jul 13, 2017 at 5:38 PM, Sakari Ailus  wrote:

[snip]

>>
>> -static const struct platform_device_id vimc_sen_driver_ids[] = {
>> +static const __maybe_unused
>> +struct platform_device_id vimc_sen_driver_ids[] = {
>>   {
>>   .name   = VIMC_SEN_DRV_NAME,
>>   },
>
> Shouldn't these be set to the corresponding driver structs' id_table
> fields? Or do I miss something...?
>

Agreed, the real problem is that the .id_table is not set for these
drivers. The match only works because the platform subsystem fallbacks
to the driver's name if an .id_table isn't defined:

http://elixir.free-electrons.com/linux/latest/source/drivers/base/platform.c#L964

Best regards,
Javier

Re: [RFC 00/10] V4L2 explicit synchronization support

2017-04-06 Thread Javier Martinez Canillas

Hello Gustavo,

On 04/06/2017 10:08 AM, Gustavo Padovan wrote:
> Hi Javier,
> 
> 2017-04-05 Javier Martinez Canillas <jav...@osg.samsung.com>:
> 
>> Hello Gustavo,
>>
>> On 04/05/2017 11:09 AM, Gustavo Padovan wrote:
>>> 2017-04-03 Javier Martinez Canillas <jav...@osg.samsung.com>:
>>>
>>>> Hello Mauro and Gustavo,
>>>>
>>>> On 04/03/2017 07:16 AM, Mauro Carvalho Chehab wrote:
>>>>> Hi Gustavo,
>>>>>
>>>>> Em Mon, 13 Mar 2017 16:20:25 -0300
>>>>> Gustavo Padovan <gust...@padovan.org> escreveu:
>>>>>
>>>>>> From: Gustavo Padovan <gustavo.pado...@collabora.com>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> This RFC adds support for Explicit Synchronization of shared buffers in 
>>>>>> V4L2.
>>>>>> It uses the Sync File Framework[1] as vector to communicate the fences
>>>>>> between kernel and userspace.
>>>>>
>>>>> Thanks for your work!
>>>>>
>>>>> I looked on your patchset, and I didn't notice anything really weird
>>>>> there. So, instead on reviewing patch per patch, I prefer to discuss
>>>>> about the requirements and API, as depending on it, the code base will
>>>>> change a lot.
>>>>>
>>>>
>>>> Agree that's better to first set on an uAPI and then implement based on 
>>>> that.
>>>>  
>>>>> I'd like to do some tests with it on devices with mem2mem drivers.
>>>>> My plan is to use an Exynos board for such thing, but I guess that
>>>>> the DRM driver for it currently doesn't. I'm seeing internally if someone
>>>>> could be sure that Exynos driver upstream will become ready for such
>>>>> tests.
>>>>>
>>>>
>>>> Not sure if you should try to do testing before agreeing on an uAPI and
>>>> implementation.
>>>>
>>>>> Javier wrote some patches last year meant to implement implicit
>>>>> fences support. What we noticed is that, while his mechanism worked
>>>>> fine for pure capture and pure output devices, when we added a mem2mem
>>>>> device, on a DMABUF+fences pipeline, e. g.:
>>>>>
>>>>>   sensor -> [m2m] -> DRM
>>>>>
>>>>> End everything using fences/DMABUF, the fences mechanism caused dead
>>>>> locks on existing userspace apps.
>>>>>
>>>>> A m2m device has both capture and output devnodes. Both should be
>>>>> queued/dequeued. The capture queue is synchronized internally at the
>>>>> driver with the output buffer[1].
>>>>>
>>>>> [1] The names here are counter-intuitive: "capture" is a devnode
>>>>> where userspace receives a video stream; "output" is a devnode where
>>>>> userspace feeds a video stream.
>>>>>
>>>>> The problem is that adding implicit fences changed the behavior of
>>>>> the ioctls, causing gstreamer to wait forever for buffers to be ready.
>>>>>
>>>>
>>>> The problem was related to trying to make user-space unaware of the 
>>>> implicit
>>>> fences support, and so it tried to QBUF a buffer that had already a pending
>>>> fence. A workaround was to block the second QBUF ioctl if the buffer had a
>>>> pending fence, but this caused the mentioned deadlock since GStreamer 
>>>> wasn't
>>>> expecting the QBUF ioctl to block.
>>>>
>>>>> I suspect that, even with explicit fences, the behavior of Q/DQ
>>>>> will be incompatible with the current behavior (or will require some
>>>>> dirty hacks to make it identical). 
>>>
>>> For QBUF the only difference is that we set flags for fences and pass
>>> and receives in and out fences. For DQBUF the behavior is exactly the
>>> same. What incompatibles or hacks do you see?
>>>
>>> I had the expectation that the flags would be for userspace to learn
>>> about any different behavior.
>>>
>>>>>
>>>>> So, IMHO, the best would be to use a new set of ioctls, when fences are
>>>>> used (like VIDIOC_QFENCE/DQFENCE).
>>>>>
>>>>
>>>> For explicit you can check if there's an input-fence so is different than
>

Re: [RFC 00/10] V4L2 explicit synchronization support

2017-04-05 Thread Javier Martinez Canillas

Hello Gustavo,

On 04/05/2017 11:09 AM, Gustavo Padovan wrote:
> 2017-04-03 Javier Martinez Canillas <jav...@osg.samsung.com>:
> 
>> Hello Mauro and Gustavo,
>>
>> On 04/03/2017 07:16 AM, Mauro Carvalho Chehab wrote:
>>> Hi Gustavo,
>>>
>>> Em Mon, 13 Mar 2017 16:20:25 -0300
>>> Gustavo Padovan <gust...@padovan.org> escreveu:
>>>
>>>> From: Gustavo Padovan <gustavo.pado...@collabora.com>
>>>>
>>>> Hi,
>>>>
>>>> This RFC adds support for Explicit Synchronization of shared buffers in 
>>>> V4L2.
>>>> It uses the Sync File Framework[1] as vector to communicate the fences
>>>> between kernel and userspace.
>>>
>>> Thanks for your work!
>>>
>>> I looked on your patchset, and I didn't notice anything really weird
>>> there. So, instead on reviewing patch per patch, I prefer to discuss
>>> about the requirements and API, as depending on it, the code base will
>>> change a lot.
>>>
>>
>> Agree that's better to first set on an uAPI and then implement based on that.
>>  
>>> I'd like to do some tests with it on devices with mem2mem drivers.
>>> My plan is to use an Exynos board for such thing, but I guess that
>>> the DRM driver for it currently doesn't. I'm seeing internally if someone
>>> could be sure that Exynos driver upstream will become ready for such
>>> tests.
>>>
>>
>> Not sure if you should try to do testing before agreeing on an uAPI and
>> implementation.
>>
>>> Javier wrote some patches last year meant to implement implicit
>>> fences support. What we noticed is that, while his mechanism worked
>>> fine for pure capture and pure output devices, when we added a mem2mem
>>> device, on a DMABUF+fences pipeline, e. g.:
>>>
>>> sensor -> [m2m] -> DRM
>>>
>>> End everything using fences/DMABUF, the fences mechanism caused dead
>>> locks on existing userspace apps.
>>>
>>> A m2m device has both capture and output devnodes. Both should be
>>> queued/dequeued. The capture queue is synchronized internally at the
>>> driver with the output buffer[1].
>>>
>>> [1] The names here are counter-intuitive: "capture" is a devnode
>>> where userspace receives a video stream; "output" is a devnode where
>>> userspace feeds a video stream.
>>>
>>> The problem is that adding implicit fences changed the behavior of
>>> the ioctls, causing gstreamer to wait forever for buffers to be ready.
>>>
>>
>> The problem was related to trying to make user-space unaware of the implicit
>> fences support, and so it tried to QBUF a buffer that had already a pending
>> fence. A workaround was to block the second QBUF ioctl if the buffer had a
>> pending fence, but this caused the mentioned deadlock since GStreamer wasn't
>> expecting the QBUF ioctl to block.
>>
>>> I suspect that, even with explicit fences, the behavior of Q/DQ
>>> will be incompatible with the current behavior (or will require some
>>> dirty hacks to make it identical). 
> 
> For QBUF the only difference is that we set flags for fences and pass
> and receives in and out fences. For DQBUF the behavior is exactly the
> same. What incompatibles or hacks do you see?
> 
> I had the expectation that the flags would be for userspace to learn
> about any different behavior.
> 
>>>
>>> So, IMHO, the best would be to use a new set of ioctls, when fences are
>>> used (like VIDIOC_QFENCE/DQFENCE).
>>>
>>
>> For explicit you can check if there's an input-fence so is different than
>> implicit, but still I agree that it would be better to have specific ioctls.
> 
> I'm pretty new to v4l2 so I don't know all use cases yet, but what I
> thought was to just add extra flags to QBUF to mark when using fences
> instead of having userspace  to setup completely new ioctls for fences.
> The burden for userspace should be smaller with flags.
>

Yes, you are right. So I guess its better indeed to just extend the current
ioctls as you propose and only move to new ones if really needed.

>>
>>>>
>>>> I'm sending this to start the discussion on the best approach to implement
>>>> Explicit Synchronization, please check the TODO/OPEN section below.
>>>>
>>>> Explicit Synchronization allows us to control the synchronization of
>>>> shared buffers from userspace by passing fences to the kernel and/o

Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

2017-04-05 Thread Javier Martinez Canillas

Hello Philipp,

On Wed, Apr 5, 2017 at 5:34 AM, Philipp Zabel  wrote:
> On Wed, 2017-04-05 at 09:21 +0100, Russell King - ARM Linux wrote:

[snip]

> I think the output part is accurate, as the audio pad is an artifact of
> an unrelated change. I'm not so sure about the VBI pad, but I think that
> shouldn't exist either. The input pad, on the other hand, not having any
> of graph representation in the device tree seems a bit strange. There

Agreed, there should be a OF graph representation (and also a MC
representation) of the input PADs.

The tvp5150 driver currently has hardcoded as input TVP5150_COMPOSITE1
(AIP1B), so it won't work for a board that has the composite connector
wired to TVP5150_COMPOSITE0 (AIP1A) neither will work for S-Video
(AIP1A + AIP1B).

> was a custom binding for the inputs, that got quickly reverted:
> https://patchwork.kernel.org/patch/8395521/
>

Yes, that was my first attempt to have input connector support for
tvp5150. The patches were merged without a detailed review of the DT
bindings and latter were found to be wrong. Instead of having a driver
specific DT binding, a generic binding that could be used by any
device should be defined.

The latest proposal was [0] but that was also found to be inadequate.
After a lot of discussion on #v4l, the best approach we could come
with was something like like [1]. But I was busy with other stuff and
never found time to do the driver changes.

By the way, only the DT bindings portion from the first approach got
reverted but the patch reverting the driver changes never made to
mainline. Could you please test if [2] doesn't cause any issues to you
so the left over can be finally removed?

> regards
> Philipp
>

[0]: https://lkml.org/lkml/2016/4/12/983
[1]: https://hastebin.com/kadagidino.diff
[2]: https://patchwork.kernel.org/patch/9472623/

Best regards,
Javier

Re: [RFC 00/10] V4L2 explicit synchronization support

2017-04-03 Thread Javier Martinez Canillas

o which buffer the fence will be attached thus
>> the BUF_QUEUED event tells which buffer is associated to the fence received 
>> in
>> QBUF by userspace.
>>
>> Patches 8 and 9 add more fence infrastructure to support out-fences and 
>> finally
>> patch 10 adds support to out-fences.
>>
>> TODO/OPEN:
>> --
>>
>> * For this first implementation we will keep the ordering of the buffers 
>> queued
>> in videobuf2, that means we will only enqueue buffer whose fence was 
>> signalled
>> if that buffer is the first one in the queue. Otherwise it has to wait until 
>> it
>> is the first one. This is not implmented yet. Later we could create a flag to
>> allow unordered queing in the drivers from vb2 if needed.
> 
> The V4L2 spec doesn't warrant that the buffers will be dequeued at the
> queue order.
> 
> In practice, however, most drivers will not reorder. Yet, mem2mem codec 
> drivers may reorder the buffers at the output, as the luminance information
> (Y) usually comes first on JPEG/MPEG-like formats.
> 
>> * Should we have out-fences per-buffer or per-plane? or both? In this RFC, 
>> for
>> simplicity they are per-buffer, but Mauro and Javier raised the option of
>> doing per-plane fences. That could benefit mem2mem and V4L2 <-> GPU operation
>> at least on cases when we have Capture hw that releases the Y frame before 
>> the
>> other frames for example. When using V4L2 per-plane out-fences to communicate
>> with KMS they would need to be merged together as currently the DRM Plane
>> interface only supports one fence per DRM Plane.
> 
> That's another advantage of using a new set of ioctls for queues: with that,
> queuing/dequeing per plane will be easier. On codec drivers, doing it per
> plane could bring performance improvements.
>

You don't really need to Q/DQ on a per plane basis AFAICT. Since on QBUF you
can get a set of out-fences that can be passed to the other driver and so it
should be able to wait per fence.

>> In-fences should be per-buffer as the DRM only has per-buffer fences, but

I'm not that familiar with DRM, but I thought DRM fences was also per plane
and not per buffer.

How this works without fences? For V4L2 there's a dma-buf fd per plane and
so I was expecting the DRM API to also import a dma-buf fd per DRM plane.

I only have access to an Exynos board whose display controller supports
single plane formats, so I don't know how this works for multi planar.

>> in case of mem2mem operations per-plane fences might be useful?
>>
>> So should we have both ways, per-plane and per-buffer, or just one of them
>> for now?
>
> The API should be flexible enough to support both usecases. We could
> implement just per-buffer in the beginning, but, on such case, we
> should deploy an API that will allow to later add per-plane fences
> without breaking userspace.
>
> So, I prefer that, on multiplane fences, we have one fence per plane,
> even if, at the first implementation, all fences will be released
> at the same time, when the buffer is fully filled. That would allow
> us to later improve it, without changing userspace.

It's true that vb2 can't currently signal fences per plane since the interface
between vb2 and drivers is per vb2_buffer. But the uAPI shouldn't be restricted
by this implementation detail (that can be changed) and should support per plane
fences IMHO.

That's for example the case with the V4L2 dma-buf API. There is a dma-buf fd per
plane, and internally for vb2 single planar buffers use the dma-buf associated
with plane 0.

Now when mentioning this, I noticed that in your implementation the fences are
not associated with a dma-buf. I thought the idea was for the fences to be
associated with a dma-buf's reservation object. If we do that, then fences will
be per fence since the dma-buf/reservation objet are also per fence in v4l2/vb2.

> 
>> * other open topics are how to deal with hw-fences and Request API.
> 
> Let's not mix issues here. Request API is already complex enough
> without explicit fences. The same is true for explicit fences: it is 
> also complex to add support for it with multi-planes and a not
> ordered DQ. 
> 
> So, my suggestion here is to not delay Request API due to fences,
> nor to delay fences due to Request API, letting them to be merged
> on different Kernel releases. When both features got added, someone
> can work on a patchset that would allow using Request API with fences.
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC 03/10] [media] vb2: add in-fence support to QBUF

2017-04-03 Thread Javier Martinez Canillas

Hello Gustavo,

On 03/13/2017 04:20 PM, Gustavo Padovan wrote:

[snip]

>  
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
> + struct dma_fence *fence = NULL;
>   int ret;
>  
>   if (vb2_fileio_is_active(q)) {
> @@ -565,7 +567,17 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>   }
>  
>   ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> - return ret ? ret : vb2_core_qbuf(q, b->index, b);
> +
> + if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
> + if (b->memory != VB2_MEMORY_DMABUF)
> + return -EINVAL;
> +

I wonder if is correct to check this. Only one side of the pipeline uses
V4L2_MEMORY_DMABUF while the other uses V4L2_MEMORY_MMAP + VIDIOC_EXPBUF.

So that means that fences can only be used in one direction?

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 00/15] Exynos MFC v6+ - remove the need for the reserved memory

2017-03-15 Thread Javier Martinez Canillas

Hello Marian,

On Wed, Mar 15, 2017 at 9:00 AM, Marian Mihailescu
 wrote:
> Thanks for the quick reply.
>
> Decoding works without issues for me too.
> I did not change the CMA size or used s5p_mfc.mem parameter. However, 
> according to the Marek, the default 8M should be enough for 3 instances of 
> H264 encoders/decoders. My test was encoding a 30 seconds 720p clip, so I 
> thought memory should not be a big issue; also it’s working w/o these patches 
> applied, so I think CMA size is enough.
> Nevertheless, I will try setting them, but I would feel better if someone 
> else would try encoding too.
>

Ok, I thought it may be that because Shuah and I needed to increase
the CMA size in order to decode a H.264 1080p video.

But if decoding is working correctly for you and only fails on
encoding, then I'm afraid that I won't be able to help you since as
mentioned I didn't test that.

> Cheers,
> Marian
>

Best regards,
Javier

Re: [PATCH v2 00/15] Exynos MFC v6+ - remove the need for the reserved memory

2017-03-15 Thread Javier Martinez Canillas

Hello Marian,

On Wed, Mar 15, 2017 at 8:36 AM, Marian Mihailescu
 wrote:
> Hi,
>
> After testing these patches, encoding using MFC fails when requesting
> buffers for capture (it works for output) with ENOMEM (it complains it
> cannot allocate memory on bank1).
> Did anyone else test encoding?
>

I've not tested encoding, but I did test decoding and it works for me
with Shuah's patch to increase the CMA memory [0]. Did you test with
that one as well?

Also you could try using the 5p_mfc.mem kernel param as explained in
the commit message of "media: s5p-mfc: Add support for probe-time
preallocated block based allocator".

[0]: https://patchwork.kernel.org/patch/9596737/

> Thanks,
> Marian
>

Best regards,
Javier

Re: [PATCH] media: mfc: Fix race between interrupt routine and device functions

2017-03-06 Thread Javier Martinez Canillas

Hello Marek,

On 02/23/2017 08:43 AM, Marek Szyprowski wrote:
> Interrupt routine must wake process waiting for given interrupt AFTER
> updating driver's internal structures and contexts. Doing it in-between
> is a serious bug. This patch moves all calls to the wake() function to
> the end of the interrupt processing block to avoid potential and real
> races, especially on multi-core platforms. This also fixes following issue
> reported from clock core (clocks were disabled in interrupt after being
> unprepared from the other place in the driver, the stack trace however
> points to the different place than s5p_mfc driver because of the race):
> 
> WARNING: CPU: 1 PID: 18 at drivers/clk/clk.c:544 clk_core_unprepare+0xc8/0x108
> Modules linked in:
> CPU: 1 PID: 18 Comm: kworker/1:0 Not tainted 
> 4.10.0-next-20170223-00070-g04e18bc99ab9-dirty #2154
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: pm pm_runtime_work
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x74/0x94)
> [] (dump_stack) from [] (__warn+0xd4/0x100)
> [] (__warn) from [] (warn_slowpath_null+0x20/0x28)
> [] (warn_slowpath_null) from [] 
> (clk_core_unprepare+0xc8/0x108)
> [] (clk_core_unprepare) from [] (clk_unprepare+0x24/0x2c)
> [] (clk_unprepare) from [] 
> (exynos_sysmmu_suspend+0x48/0x60)
> [] (exynos_sysmmu_suspend) from [] 
> (pm_generic_runtime_suspend+0x2c/0x38)
> [] (pm_generic_runtime_suspend) from [] 
> (genpd_runtime_suspend+0x94/0x220)
> [] (genpd_runtime_suspend) from [] 
> (__rpm_callback+0x134/0x208)
> [] (__rpm_callback) from [] (rpm_callback+0x20/0x80)
> [] (rpm_callback) from [] (rpm_suspend+0xdc/0x458)
> [] (rpm_suspend) from [] (pm_runtime_work+0x80/0x90)
> [] (pm_runtime_work) from [] 
> (process_one_work+0x120/0x318)
> [] (process_one_work) from [] (worker_thread+0x2c/0x4ac)
> [] (worker_thread) from [] (kthread+0xfc/0x134)
> [] (kthread) from [] (ret_from_fork+0x14/0x3c)
> ---[ end trace 1ead49a7bb83f0d8 ]---
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> Fixes: af93574678108 ("[media] MFC: Add MFC 5.1 V4L2 driver")
> CC: sta...@vger.kernel.org # v4.5+
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] [media] exynos4-is: Add missing 'of_node_put'

2017-02-24 Thread Javier Martinez Canillas

Hello Christophe,

On 01/23/2017 06:16 PM, Christophe JAILLET wrote:
> It is likely that a "of_node_put(ep)" is missing here.
> There is one in the previous error handling code, and one a few lines
> below in the normal case as well.
> 
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c 
> b/drivers/media/platform/exynos4-is/media-dev.c
> index e3a8709138fa..da5b76c1df98 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -402,8 +402,10 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>   return ret;
>   }
>  
> - if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS)
> + if (WARN_ON(endpoint.base.port == 0) || index >= FIMC_MAX_SENSORS) {
> + of_node_put(ep);
>   return -EINVAL;
> + }
>  
>   pd->mux_id = (endpoint.base.port - 1) & 0x1;
>  
> 

Thanks for the patch, but Krzysztof sent the exact same patch before [0]. There
was feedback from Sylwester at the time that you can also look at [0]. Could you
please take that into account and post a patch according to what he suggested?

[0]: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415207.html

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] media: s5p-mfc: fix MMAP of mfc buffer during reqbufs

2017-02-24 Thread Javier Martinez Canillas

Hello Pankaj,

On 12/16/2016 08:48 AM, Pankaj Dubey wrote:
> From: Smitha T Murthy <smith...@samsung.com>
> 
> It has been observed on ARM64 based Exynos SoC, if IOMMU is not enabled
> and we try to use reserved memory for MFC, reqbufs fails with below
> mentioned error
> ---
> V4L2 Codec decoding example application
> Kamil Debski <k.deb...@samsung.com>
> Copyright 2012 Samsung Electronics Co., Ltd.
> 
> Opening MFC.
> (mfc.c:mfc_open:58): MFC Info (/dev/video0): driver="s5p-mfc" \
> bus_info="platform:12c3.mfc0" card="s5p-mfc-dec" fd=0x4[
> 42.339165] Remapping memory failed, error: -6
> 
> MFC Open Success.
> (main.c:main:711): Successfully opened all necessary files and devices
> (mfc.c:mfc_dec_setup_output:103): Setup MFC decoding OUTPUT buffer \
> size=4194304 (requested=4194304)
> (mfc.c:mfc_dec_setup_output:120): Number of MFC OUTPUT buffers is 2 \
> (requested 2)
> 
> [App] Out buf phy : 0x, virt : 0x
> Output Length is = 0x30
> Error (mfc.c:mfc_dec_setup_output:145): Failed to MMAP MFC OUTPUT buffer
> -
> This is because the device requesting for memory is mfc0.left not the parent 
> mfc0.
> Hence setting of alloc_devs need to be done only if IOMMU is enabled
> and in that case both the left and right device is treated as mfc0 only.
> 

I see, so likely you were facing the issue described in patch 1/2 after this
patch since the driver doesn't set alloc_devs when IOMMU is disabled, right?

In any case, I guess these patches have been superseded by Marek's series[0]
so they are no longer needed?

[0]: https://www.spinics.net/lists/linux-media/msg56.html

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 1/2] media: s5p-mfc: convert drivers to use the new vb2_queue dev field

2017-02-24 Thread Javier Martinez Canillas

Hello Pankaj,

On 12/16/2016 08:48 AM, Pankaj Dubey wrote:
> From: Smitha T Murthy <smith...@samsung.com>
> 
> commit 2548fee63d9e ("[media] media/platform: convert drivers to use the new
> vb2_queue dev field") has missed to set dev pointer of vb2_queue which will be
> used in reqbufs of mfc driver. Without this change following error is 
> observed:
> 
> ---
> V4L2 Codec decoding example application
> Kamil Debski <k.deb...@samsung.com>
> Copyright 2012 Samsung Electronics Co., Ltd.
> 
> Opening MFC.
> (mfc.c:mfc_open:58): MFC Info (/dev/video0): driver="s5p-mfc" \
> bus_info="platform:12c3.mfc0" card="s5p-mfc-dec" fd=0x4[
> 42.339165] Remapping memory failed, error: -6
> 
> MFC Open Success.
> (main.c:main:711): Successfully opened all necessary files and devices
> (mfc.c:mfc_dec_setup_output:103): Setup MFC decoding OUTPUT buffer \
> size=4194304 (requested=4194304)
> (mfc.c:mfc_dec_setup_output:120): Number of MFC OUTPUT buffers is 2 \
> (requested 2)
> 
> [App] Out buf phy : 0x, virt : 0x
> Output Length is = 0x30
> Error (mfc.c:mfc_dec_setup_output:145): Failed to MMAP MFC OUTPUT buffer
> ---
> 

On which kernel version did you face this issue?

> Signed-off-by: Smitha T Murthy <smith...@samsung.com>
> [pankaj.dubey: debugging issue and formatting commit message]
> Signed-off-by: Pankaj Dubey <pankaj.du...@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 0a5b8f5..6ea8246 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -838,6 +838,7 @@ static int s5p_mfc_open(struct file *file)
>   q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>   q->drv_priv = >fh;
>   q->lock = >mfc_mutex;
> + q->dev = >plat_dev->dev;
>   if (vdev == dev->vfd_dec) {
>   q->io_modes = VB2_MMAP;
>   q->ops = get_dec_queue_ops();
> @@ -861,6 +862,7 @@ static int s5p_mfc_open(struct file *file)
>   q->io_modes = VB2_MMAP;
>   q->drv_priv = >fh;
>   q->lock = >mfc_mutex;
> + q->dev = >plat_dev->dev;
>   if (vdev == dev->vfd_dec) {
>   q->io_modes = VB2_MMAP;
>   q->ops = get_dec_queue_ops();
>

Please correct me if I'm wrong, but AFAIU this shouldn't be needed in
the s5p-mfc driver since the videobuf2 core either uses the vb2_queue
.dev field or the vb2_queue .alloc_devs. And the latter is set in the
s5p_mfc_queue_setup() function, so the .dev field shouldn't be used.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[PATCH 1/3] [media] soc-camera: ov5642: Add OF device ID table

2017-02-22 Thread Javier Martinez Canillas

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/i2c/soc_camera/ov5642.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/soc_camera/ov5642.c 
b/drivers/media/i2c/soc_camera/ov5642.c
index 3d185bd622a3..1926f382dfce 100644
--- a/drivers/media/i2c/soc_camera/ov5642.c
+++ b/drivers/media/i2c/soc_camera/ov5642.c
@@ -1063,9 +1063,18 @@ static const struct i2c_device_id ov5642_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ov5642_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov5642_of_match[] = {
+   { .compatible = "ovti,ov5642" },
+   { },
+};
+MODULE_DEVICE_TABLE(of, ov5642_of_match);
+#endif
+
 static struct i2c_driver ov5642_i2c_driver = {
.driver = {
.name = "ov5642",
+   .of_match_table = of_match_ptr(ov5642_of_match),
},
.probe  = ov5642_probe,
.remove = ov5642_remove,
-- 
2.9.3

[PATCH 2/3] [media] tc358743: Add OF device ID table

2017-02-22 Thread Javier Martinez Canillas

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/i2c/tc358743.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index f569a05fe105..76baf7a7bd57 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1951,9 +1951,18 @@ static struct i2c_device_id tc358743_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, tc358743_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id tc358743_of_match[] = {
+   { .compatible = "toshiba,tc358743" },
+   {},
+};
+MODULE_DEVICE_TABLE(of, tc358743_of_match);
+#endif
+
 static struct i2c_driver tc358743_driver = {
.driver = {
.name = "tc358743",
+   .of_match_table = of_match_ptr(tc358743_of_match),
},
.probe = tc358743_probe,
.remove = tc358743_remove,
-- 
2.9.3

[PATCH 3/3] [media] si4713: Add OF device ID table

2017-02-22 Thread Javier Martinez Canillas

The driver doesn't have a struct of_device_id table but supported devices
are registered via Device Trees. This is working on the assumption that a
I2C device registered via OF will always match a legacy I2C device ID and
that the MODALIAS reported will always be of the form i2c:.

But this could change in the future so the correct approach is to have an
OF device ID table if the devices are registered via OF.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/radio/si4713/si4713.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/radio/si4713/si4713.c 
b/drivers/media/radio/si4713/si4713.c
index 60f026a58076..f4a53f1e856e 100644
--- a/drivers/media/radio/si4713/si4713.c
+++ b/drivers/media/radio/si4713/si4713.c
@@ -1656,9 +1656,18 @@ static const struct i2c_device_id si4713_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, si4713_id);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id si4713_of_match[] = {
+   { .compatible = "silabs,si4713" },
+   { },
+};
+MODULE_DEVICE_TABLE(of, si4713_of_match);
+#endif
+
 static struct i2c_driver si4713_i2c_driver = {
.driver = {
.name   = "si4713",
+   .of_match_table = of_match_ptr(si4713_of_match),
},
.probe  = si4713_probe,
.remove = si4713_remove,
-- 
2.9.3

[PATCH] [media] et8ek8: Export OF device ID as module aliases

2017-02-20 Thread Javier Martinez Canillas

The I2C core always reports a MODALIAS of the form i2c: even if the
device was registered via OF, this means that exporting the OF device ID
table device aliases in the module is not needed. But in order to change
how the core reports modaliases to user-space, it's better to export it.

Before this patch:

$ modinfo drivers/media/i2c/et8ek8/et8ek8.ko | grep alias
alias:  i2c:et8ek8

After this patch:

$ modinfo drivers/media/i2c/et8ek8/et8ek8.ko | grep alias
alias:  i2c:et8ek8
alias:  of:N*T*Ctoshiba,et8ek8C*
alias:  of:N*T*Ctoshiba,et8ek8

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/i2c/et8ek8/et8ek8_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c 
b/drivers/media/i2c/et8ek8/et8ek8_driver.c
index bec4a563a09c..f39f5179dd95 100644
--- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
+++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
@@ -1485,6 +1485,7 @@ static const struct of_device_id et8ek8_of_table[] = {
{ .compatible = "toshiba,et8ek8" },
{ },
 };
+MODULE_DEVICE_TABLE(of, et8ek8_of_table);
 
 static const struct i2c_device_id et8ek8_id_table[] = {
{ ET8EK8_NAME, 0 },
-- 
2.9.3

Re: [PATCH v2 08/15] media: s5p-mfc: Move firmware allocation to DMA configure function

2017-02-20 Thread Javier Martinez Canillas

Hello Marek,

On 02/20/2017 10:38 AM, Marek Szyprowski wrote:
> To complete DMA memory configuration for MFC device, allocation of the
> firmware buffer is needed, because some parameters are dependant on its base
> address. Till now, this has been handled in the s5p_mfc_alloc_firmware()
> function. This patch moves that logic to s5p_mfc_configure_dma_memory() to
> keep DMA memory related operations in a single place. This way
> s5p_mfc_alloc_firmware() is simplified and does what it name says. The
> other consequence of this change is moving s5p_mfc_alloc_firmware() call
> from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory().
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 14/15] media: s5p-mfc: Use preallocated block allocator always for MFC v6+

2017-02-20 Thread Javier Martinez Canillas

Hello Marek,

On 02/20/2017 10:23 AM, Marek Szyprowski wrote:
> Hi Javier,
> 
> On 2017-02-17 22:13, Javier Martinez Canillas wrote:
>> On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
>>> It turned out that all versions of MFC v6+ hardware doesn't have a strict
>>> requirement for ALL buffers to be allocated on higher addresses than the
>>> firmware base like it was documented for MFC v5. This requirement is true
>>> only for the device and per-context buffers. All video data buffers can be
>>> allocated anywhere for all MFC v6+ versions. Basing on this fact, the
>>> special DMA configuration based on two reserved memory regions is not
>>> really needed for MFC v6+ devices, because the memory requirements for the
>>> firmware, device and per-context buffers can be fulfilled by the simple
>>> probe-time pre-allocated block allocator instroduced in previous patch.
>> s/instroduced/introduced
>>
>>> This patch enables support for such pre-allocated block based allocator
>>> always for MFC v6+ devices. Due to the limitations of the memory management
>>> subsystem the largest supported size of the pre-allocated buffer when no
>>> CMA (Contiguous Memory Allocator) is enabled is 4MiB.
>>>
>>> This patch also removes the requirement to provide two reserved memory
>>> regions for MFC v6+ devices in device tree. Now the driver is fully
>>> functional without them.
>>>
>> Great!
>>
>>> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
>>> ---
>> The patch looks good to me though and it works on my tests,
>> I've just one comment below.
>>
>> Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>> Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>>
>>>   Documentation/devicetree/bindings/media/s5p-mfc.txt | 2 +-
>>>   drivers/media/platform/s5p-mfc/s5p_mfc.c| 9 ++---
>>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt 
>>> b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> index 2c901286d818..d3404b5d4d17 100644
>>> --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>>> @@ -28,7 +28,7 @@ Optional properties:
>>> - memory-region : from reserved memory binding: phandles to two reserved
>>>   memory regions, first is for "left" mfc memory bus interfaces,
>>>   second if for the "right" mfc memory bus, used when no SYSMMU
>>> -support is available
>>> +support is available; used only by MFC v5 present in Exynos4 SoCs
>>> Obsolete properties:
>>> - samsung,mfc-r, samsung,mfc-l : support removed, please use 
>>> memory-region
>>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
>>> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>>> index 8fc6fe4ba087..36f0aec2a1b3 100644
>>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>>> @@ -1178,9 +1178,12 @@ static void s5p_mfc_unconfigure_2port_memory(struct 
>>> s5p_mfc_dev *mfc_dev)
>>>   static int s5p_mfc_configure_common_memory(struct s5p_mfc_dev *mfc_dev)
>>>   {
>>>   struct device *dev = _dev->plat_dev->dev;
>>> -unsigned long mem_size = SZ_8M;
>>> +unsigned long mem_size = SZ_4M;
>>>   unsigned int bitmap_size;
>>>   +if (IS_ENABLED(CONFIG_DMA_CMA) || exynos_is_iommu_available(dev))
>>> +mem_size = SZ_8M;
>>> +
>>>   if (mfc_mem_size)
>>>   mem_size = memparse(mfc_mem_size, NULL);
>>>
>> The driver allows the user to set mem_size > SZ_4M using the s5p_mfc.mem
>> parameter even when CMA ins't enabled and IOMMU isn't available. Maybe it
>> should fail early instead and notify the user what's missing?
> 
> It will notify user that driver failed to preallocate memory. 4M is the upper
> limit for standard kernel configuration, but afair there were some kconfig
> knobs to force kernel to use larger buckets for buddy allocator (what changes
> the limit). Frankly I would leave it as is.
> 
> If user wants to specify s5p-mfc.mem, he already has some knowledge how to
> configure the kernel. I don't think that driver should check all possible
> scenarios of failing and give detailed explanation what was configured
> wrong. You can also enable CMA and configure the 8MiB of the default global
> area. In such configuration preallocation of mfc firmware buffer will also
> fail. Should the driver care about it? IMO it is enough to tell user that
> preallocating of given megabytes failed.
> 

Right, it makes sense to leave it at is then indeed and let the users figure
out why the allocation failed with their specific kernel configuration.

> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 00/15] Exynos MFC v6+ - remove the need for the reserved memory

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Dear All,
> 
> This patchset is a result of my work on enabling full support for MFC device
> (multimedia codec) on Exynos 5433 on ARM64 architecture. Initially I thought
> that to let it working on ARM64 architecture with IOMMU, I would need to
> solve the issue related to the fact that s5p-mfc driver was depending on the
> first-fit allocation method in the DMA-mapping / IOMMU glue code (ARM64 use
> different algorithm). It turned out, that there is a much simpler way.
> 

A nice side effect of these patches is that I don't see anymore a page fault
error with CMA when sharing dma-buf between s5p-fmc and exynos-gsc drivers.

Following GST pipeline used to lead to a system crash, but it's working now:

$ gst-launch-1.0 filesrc location=test.mp4 ! qtdemux ! h264parse ! \
v4l2video2dec capture-io-mode=dmabuf ! v4l2video0convert \
output-io-mode=dmabuf-import capture-io-mode=dmabuf ! kmssink

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 15/15] ARM: dts: exynos: Remove MFC reserved buffers

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> During my research I found that some of the requirements for the memory
> buffers for MFC v6+ devices were blindly copied from the previous (v5)
> version and simply turned out to be excessive. The relaxed requirements
> are applied by the recent patches to the MFC driver and the driver is
> now fully functional even without the reserved memory blocks for all
> v6+ variants. This patch removes those reserved memory nodes from all
> boards having MFC v6+ hardware block.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 14/15] media: s5p-mfc: Use preallocated block allocator always for MFC v6+

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> It turned out that all versions of MFC v6+ hardware doesn't have a strict
> requirement for ALL buffers to be allocated on higher addresses than the
> firmware base like it was documented for MFC v5. This requirement is true
> only for the device and per-context buffers. All video data buffers can be
> allocated anywhere for all MFC v6+ versions. Basing on this fact, the
> special DMA configuration based on two reserved memory regions is not
> really needed for MFC v6+ devices, because the memory requirements for the
> firmware, device and per-context buffers can be fulfilled by the simple
> probe-time pre-allocated block allocator instroduced in previous patch.

s/instroduced/introduced

> 
> This patch enables support for such pre-allocated block based allocator
> always for MFC v6+ devices. Due to the limitations of the memory management
> subsystem the largest supported size of the pre-allocated buffer when no
> CMA (Contiguous Memory Allocator) is enabled is 4MiB.
> 
> This patch also removes the requirement to provide two reserved memory
> regions for MFC v6+ devices in device tree. Now the driver is fully
> functional without them.
>

Great!

> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

The patch looks good to me though and it works on my tests,
I've just one comment below.

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

>  Documentation/devicetree/bindings/media/s5p-mfc.txt | 2 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc.c| 9 ++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt 
> b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> index 2c901286d818..d3404b5d4d17 100644
> --- a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> @@ -28,7 +28,7 @@ Optional properties:
>- memory-region : from reserved memory binding: phandles to two reserved
>   memory regions, first is for "left" mfc memory bus interfaces,
>   second if for the "right" mfc memory bus, used when no SYSMMU
> - support is available
> + support is available; used only by MFC v5 present in Exynos4 SoCs
>  
>  Obsolete properties:
>- samsung,mfc-r, samsung,mfc-l : support removed, please use memory-region
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 8fc6fe4ba087..36f0aec2a1b3 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1178,9 +1178,12 @@ static void s5p_mfc_unconfigure_2port_memory(struct 
> s5p_mfc_dev *mfc_dev)
>  static int s5p_mfc_configure_common_memory(struct s5p_mfc_dev *mfc_dev)
>  {
>   struct device *dev = _dev->plat_dev->dev;
> - unsigned long mem_size = SZ_8M;
> + unsigned long mem_size = SZ_4M;
>   unsigned int bitmap_size;
>  
> + if (IS_ENABLED(CONFIG_DMA_CMA) || exynos_is_iommu_available(dev))
> + mem_size = SZ_8M;
> +
>   if (mfc_mem_size)
>   mem_size = memparse(mfc_mem_size, NULL);
> 

The driver allows the user to set mem_size > SZ_4M using the s5p_mfc.mem
parameter even when CMA ins't enabled and IOMMU isn't available. Maybe it
should fail early instead and notify the user what's missing?

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 13/15] media: s5p-mfc: Remove special configuration of IOMMU domain

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> The main reason for using special configuration of IOMMU domain was the
> problem with MFC firmware, which failed to operate properly when placed
> at 0 DMA address. Instead of adding custom code for configuring each
> variant of IOMMU domain and architecture specific glue code, simply use
> what arch code provides and if the DMA base address equals zero, skip
> first 128 KiB to keep required alignment. This patch also make the driver
> operational on ARM64 architecture, because it no longer depends on ARM
> specific DMA-mapping and IOMMU glue code functions.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 12/15] media: s5p-mfc: Add support for probe-time preallocated block based allocator

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> Current MFC driver depends on the fact that when IOMMU is available, the
> DMA-mapping framework and its IOMMU glue will use first-fit allocator.
> This was true for ARM architecture, but its not for ARM64 arch. However, in
> case of MFC v6+ hardware and latest firmware, it turned out that there is
> no strict requirement for ALL buffers to be allocated on higher addresses
> than the firmware base. This requirement is true only for the device and
> per-context buffers. All video data buffers can be allocated anywhere for
> all MFC v6+ versions.
> 
> Such relaxed requirements for the memory buffers can be easily fulfilled
> by allocating firmware, device and per-context buffers from the probe-time
> preallocated larger buffer. This patch adds support for it. This way the
> driver finally works fine on ARM64 architecture. The size of the
> preallocated buffer is 8 MiB, what is enough for three instances H264
> decoders or encoders (other codecs have smaller memory requirements).
> If one needs more for particular use case, one can use "mem" module
> parameter to force larger (or smaller) buffer (for example by adding
> "s5p_mfc.mem=16M" to kernel command line).
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,

-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 11/15] media: s5p-mfc: Split variant DMA memory configuration into separate functions

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> Move code for DMA memory configuration with IOMMU into separate function
> to make it easier to compare what is being done in each case.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c | 102 
> ++-
>  1 file changed, 61 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index 92a88c20b26d..a18740c81c55 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1107,41 +1107,13 @@ static struct device *s5p_mfc_alloc_memdev(struct 
> device *dev,
>   return NULL;
>  }
>  
> -static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)
> +static int s5p_mfc_configure_2port_memory(struct s5p_mfc_dev *mfc_dev)
>  {
>   struct device *dev = _dev->plat_dev->dev;
>   void *bank2_virt;
>   dma_addr_t bank2_dma_addr;
>   unsigned long align_size = 1 << MFC_BASE_ALIGN_ORDER;
> - struct s5p_mfc_priv_buf *fw_buf = _dev->fw_buf;
> -
> - /*
> -  * When IOMMU is available, we cannot use the default configuration,
> -  * because of MFC firmware requirements: address space limited to
> -  * 256M and non-zero default start address.
> -  * This is still simplified, not optimal configuration, but for now
> -  * IOMMU core doesn't allow to configure device's IOMMUs channel
> -  * separately.
> -  */
> - if (exynos_is_iommu_available(dev)) {
> - int ret = exynos_configure_iommu(dev, S5P_MFC_IOMMU_DMA_BASE,
> -  S5P_MFC_IOMMU_DMA_SIZE);
> - if (ret)
> - return ret;
> -
> - mfc_dev->mem_dev[BANK1_CTX] = mfc_dev->mem_dev[BANK2_CTX] = dev;
> - ret = s5p_mfc_alloc_firmware(mfc_dev);
> - if (ret) {
> - exynos_unconfigure_iommu(dev);
> - return ret;
> - }
> -
> - mfc_dev->dma_base[BANK1_CTX] = mfc_dev->fw_buf.dma;
> - mfc_dev->dma_base[BANK2_CTX] = mfc_dev->fw_buf.dma;
> - vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
> -
> - return 0;
> - }
> + int ret;

This should be declared in patch 8/15.

>  
>   /*
>* Create and initialize virtual devices for accessing
> @@ -1188,26 +1160,74 @@ static int s5p_mfc_configure_dma_memory(struct 
> s5p_mfc_dev *mfc_dev)
>   DMA_BIT_MASK(32));
>   vb2_dma_contig_set_max_seg_size(mfc_dev->mem_dev[BANK2_CTX],
>   DMA_BIT_MASK(32));
> -

This seems to be an unrelated change.

The rest looks good to me.

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 10/15] media: s5p-mfc: Reduce firmware buffer size for MFC v6+ variants

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> Firmware for MFC v6+ variants is not larger than 400 KiB, so there is no
> need to allocate a full 1 MiB buffer for it. Reduce it to 512 KiB to keep
> proper alignment of allocated buffer.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 09/15] media: s5p-mfc: Allocate firmware with internal private buffer alloc function

2017-02-17 Thread Javier Martinez Canillas

Hell Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> Once firmware buffer has been converted to use s5p_mfc_priv_buf structure,
> it is possible to allocate it with existing s5p_mfc_alloc_priv_buf()
> function. This change will help to reduce code variants in the next
> patches.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 08/15] media: s5p-mfc: Move firmware allocation to DMA configure function

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> To complete DMA memory configuration for MFC device, allocation of the
> firmware buffer is needed, because some parameters are dependant on its base
> address. Till now, this has been handled in the s5p_mfc_alloc_firmware()
> function. This patch moves that logic to s5p_mfc_configure_dma_memory() to
> keep DMA memory related operations in a single place. This way
> s5p_mfc_alloc_firmware() is simplified and does what it name says. The
> other consequence of this change is moving s5p_mfc_alloc_firmware() call
> from the s5p_mfc_probe() function to the s5p_mfc_configure_dma_memory().
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c  | 58 
> +--
>  drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c | 31 --
>  2 files changed, 45 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index bc1aeb25ebeb..92a88c20b26d 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1110,6 +1110,10 @@ static struct device *s5p_mfc_alloc_memdev(struct 
> device *dev,
>  static int s5p_mfc_configure_dma_memory(struct s5p_mfc_dev *mfc_dev)
>  {
>   struct device *dev = _dev->plat_dev->dev;
> + void *bank2_virt;
> + dma_addr_t bank2_dma_addr;
> + unsigned long align_size = 1 << MFC_BASE_ALIGN_ORDER;
> + struct s5p_mfc_priv_buf *fw_buf = _dev->fw_buf;
>  
>   /*
>* When IOMMU is available, we cannot use the default configuration,
> @@ -1122,14 +1126,21 @@ static int s5p_mfc_configure_dma_memory(struct 
> s5p_mfc_dev *mfc_dev)
>   if (exynos_is_iommu_available(dev)) {
>   int ret = exynos_configure_iommu(dev, S5P_MFC_IOMMU_DMA_BASE,
>S5P_MFC_IOMMU_DMA_SIZE);
> - if (ret == 0) {
> - mfc_dev->mem_dev[BANK1_CTX] =
> - mfc_dev->mem_dev[BANK2_CTX] = dev;
> - vb2_dma_contig_set_max_seg_size(dev,
> - DMA_BIT_MASK(32));
> + if (ret)
> + return ret;
> +
> + mfc_dev->mem_dev[BANK1_CTX] = mfc_dev->mem_dev[BANK2_CTX] = dev;
> + ret = s5p_mfc_alloc_firmware(mfc_dev);
> + if (ret) {
> + exynos_unconfigure_iommu(dev);
> + return ret;
>   }
>  
> - return ret;
> + mfc_dev->dma_base[BANK1_CTX] = mfc_dev->fw_buf.dma;
> + mfc_dev->dma_base[BANK2_CTX] = mfc_dev->fw_buf.dma;

I guess you meant to use fw_buf->dma here? Since otherwise the fw_buf
variable won't be used.

> + vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
> +
> + return 0;
>   }
>  
>   /*
> @@ -1147,6 +1158,32 @@ static int s5p_mfc_configure_dma_memory(struct 
> s5p_mfc_dev *mfc_dev)
>   return -ENODEV;
>   }
>  
> + /* Allocate memory for firmware and initialize both banks addresses */
> + ret = s5p_mfc_alloc_firmware(mfc_dev);
> + if (ret)

Shouldn't you have to unregister both banks devices here in the error path?

Also, ret is not declared so this patch will cause a compile error, breaking
git bisect-ability.

> + return ret;
> +
> + mfc_dev->dma_base[BANK1_CTX] = mfc_dev->fw_buf.dma;

Same comment than before, probably you wanted to use fw_buf->dma here?

The rest of the patch looks good to me. 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 07/15] media: s5p-mfc: Put firmware to private buffer structure

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:52 AM, Marek Szyprowski wrote:
> Use s5p_mfc_priv_buf structure for keeping the firmware image. This will
> help handling of firmware buffer allocation in the next patches.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 06/15] media: s5p-mfc: Move setting DMA max segmetn size to DMA configure function

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Setting DMA max segment size to 32 bit mask is a part of DMA memory
> configuration, so move those calls to s5p_mfc_configure_dma_memory()
> function.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 05/15] media: s5p-mfc: Simplify alloc/release private buffer functions

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Change parameters for s5p_mfc_alloc_priv_buf() and s5p_mfc_release_priv_buf()
> functions. Instead of DMA device pointer and a base, provide common MFC
> device structure and memory bank context identifier.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 01/15] media: s5p-mfc: Remove unused structures and dead code

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Remove unused structures, definitions and functions that are no longer
> called from the driver code.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Also on an Exynos5422 Odroid XU4 and Exyos5800 Peach Pi boards:

Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 02/15] media: s5p-mfc: Use generic of_device_get_match_data helper

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Replace custom code with generic helper to retrieve driver data.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 04/15] media: s5p-mfc: Replace bank1/bank2 entries with an array

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Internal MFC driver device structure contains two entries for keeping
> addresses of the DMA memory banks. Replace them with the dma_base[] array
> and use defines for accessing particular banks. This will help to simplify
> code in the next patches.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 03/15] media: s5p-mfc: Replace mem_dev_* entries with an array

2017-02-17 Thread Javier Martinez Canillas

Hello Marek,

On 02/14/2017 04:51 AM, Marek Szyprowski wrote:
> Internal MFC driver device structure contains two pointers to devices used
> for DMA memory allocation: mem_dev_l and mem_dev_r. Replace them with the
> mem_dev[] array and use defines for accessing particular banks. This will
> help to simplify code in the next patches.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] media: s5p-mfc: Fix initialization of internal structures

2017-02-14 Thread Javier Martinez Canillas

Hello Marek,

On 02/03/2017 11:05 AM, Marek Szyprowski wrote:
> Initialize members of the internal device and context structures as early
> as possible to avoid access to uninitialized objects on initialization
> failures. If loading firmware or creating of the hardware instance fails,
> driver will access device or context queue in error handling path, which
> might not be initialized yet, what causes kernel panic. Fix this by moving
> initialization of all static members as early as possible.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Also tested on an Exynos5422 Odroid XU4 and Exynos5800 Peach Pi:

Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 0/2] [media] exynos-gsc: Fix support for NV21 and NV61 formats

2017-02-13 Thread Javier Martinez Canillas

Hello Sylwester,

On 02/13/2017 11:34 AM, Sylwester Nawrocki wrote:
> Hi Javier,
> 
> On 02/13/2017 01:53 PM, Javier Martinez Canillas wrote:
>> Any comments on this series?
> 
> The patches look good to me, I will Ack the patches in case
> Mauro wants to apply them directly.  Alternatively I will
> add them to my tree for v4.12 after the merge window.
> 

Great, thanks a lot for your help!

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 0/2] [media] exynos-gsc: Fix support for NV21 and NV61 formats

2017-02-13 Thread Javier Martinez Canillas

Hello,

On Wed, Feb 1, 2017 at 5:05 PM, Javier Martinez Canillas
<jav...@osg.samsung.com> wrote:
> Hello,
>
> Commit 652bb68018a5 ("[media] exynos-gsc: do proper bytesperline and
> sizeimage calculation") fixed corrupted frames for most exynos-gsc
> formats, but even after that patch two issues were still remaining:
>
> 1) Frames were still not correct for NV21 and NV61 formats.
> 2) Y42B format didn't work when used as output (only as input).
>
> This patch series fixes (1).
>
> Best regards,
> Javier
>
>
> Thibault Saunier (2):
>   [media] exynos-gsc: Do not swap cb/cr for semi planar formats
>   [media] exynos-gsc: Add support for NV{16,21,61}M pixel formats
>
>  drivers/media/platform/exynos-gsc/gsc-core.c | 29 
> ++--
>  1 file changed, 27 insertions(+), 2 deletions(-)

Any comments on this series?

Best regards,
Javier

Re: [PATCH] [media] exynos-gsc: Avoid spamming the log on VIDIOC_TRY_FMT

2017-02-02 Thread Javier Martinez Canillas

Hello Shuah,

On 02/01/2017 11:25 PM, Shuah Khan wrote:
> On 01/24/2017 02:42 PM, Javier Martinez Canillas wrote:
>> There isn't an ioctl to enum the supported field orders, so a user-space
>> application can call VIDIOC_TRY_FMT using different field orders to know
>> if one is supported. For example, GStreamer does this so during playback
>> dozens of the following messages appear in the kernel log buffer:
>>
>> [ 442.143393] Not supported field order(4)
>>
>> Instead of printing this as an error, just keep it as debug information.
>>
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>>
>> ---
>>
>>  drivers/media/platform/exynos-gsc/gsc-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
>> b/drivers/media/platform/exynos-gsc/gsc-core.c
>> index 8524fe15fa80..678b600f0500 100644
>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>> @@ -408,7 +408,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct 
>> v4l2_format *f)
>>  if (pix_mp->field == V4L2_FIELD_ANY)
>>  pix_mp->field = V4L2_FIELD_NONE;
>>  else if (pix_mp->field != V4L2_FIELD_NONE) {
>> -pr_err("Not supported field order(%d)\n", pix_mp->field);
>> +pr_debug("Not supported field order(%d)\n", pix_mp->field);
> 
> It make sense to leave it as an error, but print only once perhaps.
> The down side to making this debug is that it becomes harder to
> figure out when we run into this case.
>

I disagree. User-space trying different field orders doesn't sound like an
error to me since as mentioned there isn't an ioctl to enum supported ones.

In fact other drivers don't print anything in their try_fmt handler for the
same case, for example [0] and [1].

> thanks,
> -- Shuah
> 

[0]: 
http://lxr.free-electrons.com/source/drivers/media/platform/mx2_emmaprp.c#L496
[1]: 
http://lxr.free-electrons.com/source/drivers/media/platform/exynos4-is/fimc-m2m.c#L297

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] [media] exynos-gsc: Add support for NV{16,21,61}M pixel formats

2017-02-01 Thread Javier Martinez Canillas

From: Thibault Saunier <thibault.saun...@osg.samsung.com>

Those are useful formats that should be handled.

Signed-off-by: Thibault Saunier <thibault.saun...@osg.samsung.com>
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/media/platform/exynos-gsc/gsc-core.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index a846659ae5c1..eff636d4502b 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -112,6 +112,15 @@ static const struct gsc_fmt gsc_formats[] = {
.num_planes = 1,
.num_comp   = 2,
}, {
+   .name   = "YUV 4:2:2 non-contig, Y/CbCr",
+   .pixelformat= V4L2_PIX_FMT_NV16M,
+   .depth  = { 8, 8 },
+   .color  = GSC_YUV422,
+   .yorder = GSC_LSB_Y,
+   .corder = GSC_CBCR,
+   .num_planes = 2,
+   .num_comp   = 2,
+   }, {
.name   = "YUV 4:2:2 planar, Y/CrCb",
.pixelformat= V4L2_PIX_FMT_NV61,
.depth  = { 16 },
@@ -121,6 +130,15 @@ static const struct gsc_fmt gsc_formats[] = {
.num_planes = 1,
.num_comp   = 2,
}, {
+   .name   = "YUV 4:2:2 non-contig, Y/CrCb",
+   .pixelformat= V4L2_PIX_FMT_NV61M,
+   .depth  = { 8, 8 },
+   .color  = GSC_YUV422,
+   .yorder = GSC_LSB_Y,
+   .corder = GSC_CRCB,
+   .num_planes = 2,
+   .num_comp   = 2,
+   }, {
.name   = "YUV 4:2:0 planar, YCbCr",
.pixelformat= V4L2_PIX_FMT_YUV420,
.depth  = { 12 },
@@ -158,6 +176,15 @@ static const struct gsc_fmt gsc_formats[] = {
.num_planes = 1,
.num_comp   = 2,
}, {
+   .name   = "YUV 4:2:0 non-contig. 2p, Y/CrCb",
+   .pixelformat= V4L2_PIX_FMT_NV21M,
+   .depth  = { 8, 4 },
+   .color  = GSC_YUV420,
+   .yorder = GSC_LSB_Y,
+   .corder = GSC_CRCB,
+   .num_planes = 2,
+   .num_comp   = 2,
+   }, {
.name   = "YUV 4:2:0 non-contig. 2p, Y/CbCr",
.pixelformat= V4L2_PIX_FMT_NV12M,
.depth  = { 8, 4 },
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] [media] exynos-gsc: Fix support for NV21 and NV61 formats

2017-02-01 Thread Javier Martinez Canillas

Hello,

Commit 652bb68018a5 ("[media] exynos-gsc: do proper bytesperline and
sizeimage calculation") fixed corrupted frames for most exynos-gsc
formats, but even after that patch two issues were still remaining:

1) Frames were still not correct for NV21 and NV61 formats.
2) Y42B format didn't work when used as output (only as input).

This patch series fixes (1).

Best regards,
Javier


Thibault Saunier (2):
  [media] exynos-gsc: Do not swap cb/cr for semi planar formats
  [media] exynos-gsc: Add support for NV{16,21,61}M pixel formats

 drivers/media/platform/exynos-gsc/gsc-core.c | 29 ++--
 1 file changed, 27 insertions(+), 2 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] [media] exynos-gsc: Do not swap cb/cr for semi planar formats

2017-02-01 Thread Javier Martinez Canillas

From: Thibault Saunier <thibault.saun...@osg.samsung.com>

In the case of semi planar formats cb and cr are in the same plane
in memory, meaning that will be set to 'cb' whatever the format is,
and whatever the (packed) order of those components are.

Suggested-by: Nicolas Dufresne <nicolas.dufre...@collabora.com>
Signed-off-by: Thibault Saunier <thibault.saun...@osg.samsung.com>
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 40aff08dd51d..a846659ae5c1 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -861,9 +861,7 @@ int gsc_prepare_addr(struct gsc_ctx *ctx, struct vb2_buffer 
*vb,
 
if ((frame->fmt->pixelformat == V4L2_PIX_FMT_VYUY) ||
(frame->fmt->pixelformat == V4L2_PIX_FMT_YVYU) ||
-   (frame->fmt->pixelformat == V4L2_PIX_FMT_NV61) ||
(frame->fmt->pixelformat == V4L2_PIX_FMT_YVU420) ||
-   (frame->fmt->pixelformat == V4L2_PIX_FMT_NV21) ||
(frame->fmt->pixelformat == V4L2_PIX_FMT_YVU420M))
swap(addr->cb, addr->cr);
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] exynos-gsc: Avoid spamming the log on VIDIOC_TRY_FMT

2017-01-24 Thread Javier Martinez Canillas

There isn't an ioctl to enum the supported field orders, so a user-space
application can call VIDIOC_TRY_FMT using different field orders to know
if one is supported. For example, GStreamer does this so during playback
dozens of the following messages appear in the kernel log buffer:

[ 442.143393] Not supported field order(4)

Instead of printing this as an error, just keep it as debug information.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/media/platform/exynos-gsc/gsc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 8524fe15fa80..678b600f0500 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -408,7 +408,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct 
v4l2_format *f)
if (pix_mp->field == V4L2_FIELD_ANY)
pix_mp->field = V4L2_FIELD_NONE;
else if (pix_mp->field != V4L2_FIELD_NONE) {
-   pr_err("Not supported field order(%d)\n", pix_mp->field);
+   pr_debug("Not supported field order(%d)\n", pix_mp->field);
return -EINVAL;
}
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

2017-01-24 Thread Javier Martinez Canillas

Hello Steve,

On Fri, Jan 6, 2017 at 11:11 PM, Steve Longerbeam  wrote:
> From: Philipp Zabel 

[snip]

>
> +config VIDEO_MULTIPLEXER
> +   tristate "Video Multiplexer"
> +   depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER

The driver can be build as a module...

> +
> +static const struct of_device_id vidsw_dt_ids[] = {
> +   { .compatible = "video-multiplexer", },
> +   { /* sentinel */ }
> +};
> +

... so you need a MODULE_DEVICE_TABLE(of, vidsw_dt_ids) here or
otherwise module autoloading won't work.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-20 Thread Javier Martinez Canillas

Hello Marek,

On 01/20/2017 05:08 AM, Marek Szyprowski wrote:

[snip]

>>
>> This seems to be caused by some needed clocks to access the power domains
>> to be gated, since I don't get these erros when passing clk_ignore_unused
>> as parameter in the kernel command line.
> 
> I think that those issues were fixes by the following patch:
> https://patchwork.kernel.org/patch/9484607/
> It still didn't reach mainline, but I hope it will go as a fix to v4.10.
> 

Argh, I missed on a first read that the patch you mentioned already marks
CLK_ACLK432_SCALER as CLK_IS_CRITICAL. So please also ignore my clk patch.

Sorry for the noise and the mess with these patches. Thought I also tested
on linux-next to see if the issue was solved there, but it seems I didn't.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-20 Thread Javier Martinez Canillas

Hello,

On 01/20/2017 07:06 AM, Javier Martinez Canillas wrote:
> Hello Marek,
> 
> On 01/20/2017 05:37 AM, Marek Szyprowski wrote:
> 
> [snip]
>>
>> Please send this patch instead of adding more clocks to the power domains.
>> This way we will avoid adding more dependencies to userspace (DT ABI).
>> Likely those clocks are also needed to access any device in that power
>> domains.
>>
>> Later, once the runtime PM for clocks get merged, a patch for exynos542x
>> clocks driver can be made to replace IS_CRITICAL with proper runtime
>> handling.
>>
> 
> Ok, I thought that we didn't want to mark more clocks as CLK_IGNORE_UNUSED
> or CLK_IS_CRITICAL but I agree this can be done as a temporary workaround
> until a proper runtime PM based solution gets merged.
> 

I posted following patch [0] for clk-exynos5420, so $SUBJECT can be dropped.

[0]: https://patchwork.kernel.org/patch/9527957/

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] [media] exynos-gsc: Only reset the GSC HW on probe() if !CONFIG_PM

2017-01-20 Thread Javier Martinez Canillas

Hello,

On 01/19/2017 07:36 PM, Javier Martinez Canillas wrote:
> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
> CONFIG_PM is unset") removed the implicit dependency that the driver
> had with CONFIG_PM, since it relied on the config option to be enabled.
> 
> In order to work with !CONFIG_PM, the GSC reset logic that happens in
> the runtime resume callback had to be executed on the probe function.
> 
> But there's no need to do this if CONFIG_PM is enabled, since in this
> case the runtime PM resume callback will be called by VIDIOC_STREAMON
> ioctl, so the resume handler will call the GSC HW reset function.
> 
> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 

Please ignore this patch as suggested by Marek in other thread.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-20 Thread Javier Martinez Canillas

Hello Marek,

On 01/20/2017 05:37 AM, Marek Szyprowski wrote:

[snip]

>>   I'll post a proper patch for the exynos5800.dtsi, to override the
>> clocks in the gsc_pd device node.
>>
>> I also see that the two power domains that fail to be disabled msc_pd
>> (power-domain@10044120) and isp_pd (power-domain@10044020) don't have
>> clocks defined in the exynos54xx.dtsi. I'll add clocks for those too.
> 
> Please send this patch instead of adding more clocks to the power domains.
> This way we will avoid adding more dependencies to userspace (DT ABI).
> Likely those clocks are also needed to access any device in that power
> domains.
> 
> Later, once the runtime PM for clocks get merged, a patch for exynos542x
> clocks driver can be made to replace IS_CRITICAL with proper runtime
> handling.
> 

Ok, I thought that we didn't want to mark more clocks as CLK_IGNORE_UNUSED
or CLK_IS_CRITICAL but I agree this can be done as a temporary workaround
until a proper runtime PM based solution gets merged.

> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-20 Thread Javier Martinez Canillas

Hello Marek,

On 01/20/2017 05:08 AM, Marek Szyprowski wrote:
> Hi Javier,
> 

[snip]

>> Ok, I misunderstood the relationship between runtime PM and the power domains
>> then. I thought the power domains were only powered on when the runtime PM
>> framework resumed an associated device (i.e: pm_runtime_get_sync() is 
>> called).
> 
> Power domains are implemented transparently for the drivers. Even when driver
> doesn't support runtime pm, but its device is in the power domain, the core 
> will
> ensure that the domain will be turned on all the time the driver is bound to 
> the
> device.
>

I got it now, thank a lot for your explanation.
 
>> But even if this isn't the case, shouldn't the reset in probe only be needed
>> if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message).
> 
> This looks like an over-engineering. I don't like polluting driver code with
> conditional statements like IS_ENABLED(CONFIG_*). It should not hurt to reset
> the device in driver probe, especially just in case the device was left in
> some partially configured/working state by bootloader or previous kernel run
> (if started from kexec). Adding this conditional code to avoid some issues
> with power domain or clocks configuration also suggests that one should
> instead solve the problem elsewhere. Driver should be able to access device
> registers in its probe() in any case without the additional hacks.
>

Fair enough, I already posted the patch but I'll ask it to be dropped.

[snip]

>>
>> This seems to be caused by some needed clocks to access the power domains
>> to be gated, since I don't get these erros when passing clk_ignore_unused
>> as parameter in the kernel command line.
> 
> I think that those issues were fixes by the following patch:
> https://patchwork.kernel.org/patch/9484607/
> It still didn't reach mainline, but I hope it will go as a fix to v4.10.
>

That won't be enough since the CLK_ACLK432_SCALER needs to be ungated for
Exynos5422/5800 as mentioned on another mail in this thread.

> Please test if this solves your issue. Please not that adding more clocks
> to the power domain drivers will solve only the problem with turning domain
> on/off, but the are more cases where those clocks should be turned on (like
> IOMMU integration), so marking them as critical solves that problem too.
>

Ok, I understand your point.
 
> Best regards

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

2017-01-19 Thread Javier Martinez Canillas

Commit a006c04e6218 ("[media] exynos-gsc: Fixup clock management at
->remove()") changed the driver's .remove function logic to fist do
a pm_runtime_get_sync() to make sure the device is powered before
attempting to gate the gsc clock.

But the commit also removed a pm_runtime_disable() call that leads
to an unbalanced pm_runtime_enable() error if the driver is removed
and re-probed:

exynos-gsc 13e0.video-scaler: Unbalanced pm_runtime_enable!
exynos-gsc 13e1.video-scaler: Unbalanced pm_runtime_enable!

Fixes: a006c04e6218 ("[media] exynos-gsc: Fixup clock management at ->remove()")
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Acked-by: Marek Szyprowski <m.szyprow...@samsung.com>

---

Changes in v2:
  - Added Marek Szyprowski's Acked-by tag.

 drivers/media/platform/exynos-gsc/gsc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index cbf75b6194b4..83272f10722d 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1118,6 +1118,7 @@ static int gsc_remove(struct platform_device *pdev)
clk_disable_unprepare(gsc->clock[i]);
 
pm_runtime_put_noidle(>dev);
+   pm_runtime_disable(>dev);
 
dev_dbg(>dev, "%s driver unloaded\n", pdev->name);
return 0;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] [media] exynos-gsc: Only reset the GSC HW on probe() if !CONFIG_PM

2017-01-19 Thread Javier Martinez Canillas

Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
CONFIG_PM is unset") removed the implicit dependency that the driver
had with CONFIG_PM, since it relied on the config option to be enabled.

In order to work with !CONFIG_PM, the GSC reset logic that happens in
the runtime resume callback had to be executed on the probe function.

But there's no need to do this if CONFIG_PM is enabled, since in this
case the runtime PM resume callback will be called by VIDIOC_STREAMON
ioctl, so the resume handler will call the GSC HW reset function.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

I-ve only tested with CONFIG_PM enabled since my Exynos5422 Odroid
XU4 board fails to boot when the config option is disabled.

Best regards,
Javier

Changes in v2:
 - Remove the Fixes tag and reword the commit message after feedback
   from Marek Szyprowski.

 drivers/media/platform/exynos-gsc/gsc-core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 83272f10722d..42e1e09ea915 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1083,8 +1083,10 @@ static int gsc_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, gsc);
 
-   gsc_hw_set_sw_reset(gsc);
-   gsc_wait_reset(gsc);
+   if (!IS_ENABLED(CONFIG_PM)) {
+   gsc_hw_set_sw_reset(gsc);
+   gsc_wait_reset(gsc);
+   }
 
vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-19 Thread Javier Martinez Canillas

Hello Marek,

On 01/19/2017 11:56 AM, Javier Martinez Canillas wrote:
> On 01/19/2017 11:17 AM, Marek Szyprowski wrote:

[snip]

> 
> Also when removing the exynos_gsc driver, I get the same error:
> 
> # rmmod s5p_mfc
> [  106.405972] s5p-mfc 1100.codec: Removing 1100.codec
> # rmmod exynos_gsc
> [  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 
> 0x00048e14
> [  227.015116] pgd = ed5dc000
> [  227.017213] [00048e14] *pgd=b17c6835
> [  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
> ...
> [  227.241585] [] (gsc_wait_reset [exynos_gsc]) from [] 
> (gsc_runtime_resume+0x9c/0xec [exynos_gsc])
> [  227.252331] [] (gsc_runtime_resume [exynos_gsc]) from 
> [] (genpd_runtime_resume+0x120/0x1d4)
> [  227.262294] [] (genpd_runtime_resume) from [] 
> (__rpm_callback+0xc8/0x218)
> 
> # cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
> domain  status  slaves
> /device runtime status
> --
> power-domain@100440C0   on  
> /devices/platform/soc/1445.mixeractive
> /devices/platform/soc/1453.hdmi active
> power-domain@10044120   on  
> power-domain@10044060   off-0   
> power-domain@10044020   on  
> power-domain@10044000   on  
> /devices/platform/soc/13e0.video-scaler suspended
> /devices/platform/soc/13e1.video-scaler resuming
> 
> This seems to be caused by some needed clocks to access the power domains
> to be gated, since I don't get these erros when passing clk_ignore_unused
> as parameter in the kernel command line.
>

I found the issue. The problem was that Exynos5422 needs not only the
CLK_ACLK_ 300_GSCL but also CLK_ACLK432_SCALER to be ungated in order
to access the GSCL IP block.

The Exynos5422 manual shows in Figure 7-14 that ACLK_432_SCLAER is needed
for the internal buses.

Exynos5420 only needs CLK_ACLK_ 300_GSCL to be ungated.

With following hack, the issue goes away for the gsc_pd in the Odroid XU4:

diff --git a/drivers/clk/samsung/clk-exynos5420.c 
b/drivers/clk/samsung/clk-exynos5420.c
index 8c8b495cbf0d..9876ec28b94c 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -586,7 +586,7 @@ static const struct samsung_gate_clock 
exynos5800_gate_clks[] __initconst = {
GATE(CLK_ACLK550_CAM, "aclk550_cam", "mout_user_aclk550_cam",
GATE_BUS_TOP, 24, 0, 0),
GATE(CLK_ACLK432_SCALER, "aclk432_scaler", "mout_user_aclk432_scaler",
-   GATE_BUS_TOP, 27, 0, 0),
+   GATE_BUS_TOP, 27, CLK_IS_CRITICAL, 0),
 };

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain  status  slaves
/device runtime status
--
power-domain@100440C0   on  
/devices/platform/soc/1445.mixeractive
/devices/platform/soc/1453.hdmi active
power-domain@10044120   on  
power-domain@10044060   off-0   
/devices/platform/soc/1100.codecsuspended
power-domain@10044020   on  
power-domain@10044000   off-0   
/devices/platform/soc/13e0.video-scaler suspended
/devices/platform/soc/13e1.video-scaler suspended

# rmmod s5p_mfc
[   82.885227] s5p-mfc 1100.codec: Removing 1100.codec
# rmmod exynos_gsc

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain  status  slaves
/device runtime status
--
power-domain@100440C0   on  
/devices/platform/soc/1445.mixeractive
/devices/platform/soc/1453.hdmi active
power-domain@10044120   on  
power-domain@10044060   off-0   
power-domain@10044020   on  
power-domain@10044000   off-0
 
I'll post a proper patch for the exynos5800.dtsi, to override the
clocks in the gsc_pd device node.

I also see that the two power domains that fail to be disabled msc_pd
(power-domain@10044120) and isp_pd (power-domain@10044020) don't have
clocks defined in the exynos54xx.dtsi. I'll add clocks for those too.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-19 Thread Javier Martinez Canillas

Hello Marek,

Thanks a lot for your feedback.

On 01/19/2017 11:17 AM, Marek Szyprowski wrote:
> Hi Javier,
> 
> On 2017-01-18 01:30, Javier Martinez Canillas wrote:
>> Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
>> CONFIG_PM is unset") removed the implicit dependency that the driver
>> had with CONFIG_PM, since it relied on the config option to be enabled.
>>
>> In order to work with !CONFIG_PM, the GSC reset logic that happens in
>> the runtime resume callback had to be executed on the probe function.
>>
>> The problem is that if CONFIG_PM is enabled, the power domain for the
>> GSC could be disabled and so an attempt to write to the GSC_SW_RESET
>> register leads to an unhandled fault / imprecise external abort error:
> 
> Driver core ensures that driver's probe() is called with respective power
> domain turned on, so this is not the right reason for the proposed change.
>

Ok, I misunderstood the relationship between runtime PM and the power domains
then. I thought the power domains were only powered on when the runtime PM
framework resumed an associated device (i.e: pm_runtime_get_sync() is called).

But even if this isn't the case, shouldn't the reset in probe only be needed
if CONFIG_PM isn't enabled? (IOW, $SUBJECT but with another commit message).

>> [   10.178825] Unhandled fault: imprecise external abort (0x1406) at 
>> 0x
>> [   10.186982] pgd = ed728000
>> [   10.190847] [] *pgd=
>> [   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
>> [   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> [   10.237134] task: ed49e400 task.stack: ed724000
>> [   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
>> [   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
>> [   10.256139] pc : []lr : []psr: 60070013
>> [   10.256139] sp : ed725d30  ip :   fp : 0001
>> [   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
>> [   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : 8ecd  r4 : c0c03900
>> [   10.285664] r3 :   r2 : 0001  r1 : ed7d8b98  r0 : ed7d8810
>>
>> So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
>> runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
>> making the reset in probe unneeded.
>>
>> Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when 
>> CONFIG_PM is unset")
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> Frankly, I don't get why this change is needed.
>

Yes, it seems $SUBJECT is just papering over the real issue. There's
something really wrong with the Exynos power domains, I see that PDs
can't be disabled by the genpd framework, exynos_pd_power_off() fail:

# dmesg | grep power-domain
[4.893318] Power domain power-domain@10044020 disable failed
[4.893342] Power domain power-domain@10044120 disable failed
[4.893711] Power domain power-domain@10044000 disable failed
[   12.690052] Power domain power-domain@10044000 disable failed
[   12.703963] Power domain power-domain@10044000 disable failed

So PD are kept on even when unused / attached devices are suspended.
Only the mfc_pd (power-domain@10044060) is correctly turned off.

# cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
domain  status  slaves
/device runtime status
--
power-domain@100440C0   on  
/devices/platform/soc/1445.mixeractive
/devices/platform/soc/1453.hdmi active
power-domain@10044120   on  
power-domain@10044060   off-0   
/devices/platform/soc/1100.codecsuspended
power-domain@10044020   on  
power-domain@10044000   on  
/devices/platform/soc/13e0.video-scaler suspended
/devices/platform/soc/13e1.video-scaler suspended

Also when removing the exynos_gsc driver, I get the same error:

# rmmod s5p_mfc
[  106.405972] s5p-mfc 1100.codec: Removing 1100.codec
# rmmod exynos_gsc
[  227.008559] Unhandled fault: imprecise external abort (0x1c06) at 0x00048e14
[  227.015116] pgd = ed5dc000
[  227.017213] [00048e14] *pgd=b17c6835
[  227.020889] Internal error: : 1c06 [#1] PREEMPT SMP ARM
...
[  227.241585] [] (gsc_wait_reset [exynos_gsc]) from [] 
(gsc_runtime_resume+0x9c/0xec [exynos_gsc])
[  227.252331] [] (gsc_runtime_resume [exynos_gsc]) from [] 
(genpd_runtime_resume+0x120/0x1d4)
[  227.262294] [] (genpd_runtime_resume) from [] 
(__rpm_callback+0xc8/0x218)

# cat /sys/kernel/de

Re: [PATCH 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

2017-01-19 Thread Javier Martinez Canillas

Hello Marek,

On 01/19/2017 11:12 AM, Marek Szyprowski wrote:
> Hi Javier,
> 
> On 2017-01-18 01:30, Javier Martinez Canillas wrote:
>> Commit a006c04e6218 ("[media] exynos-gsc: Fixup clock management at
>> ->remove()") changed the driver's .remove function logic to fist do
>> a pm_runtime_get_sync() to make sure the device is powered before
>> attempting to gate the gsc clock.
>>
>> But the commit also removed a pm_runtime_disable() call that leads
>> to an unbalanced pm_runtime_enable() error if the driver is removed
>> and re-probed:
>>
>> exynos-gsc 13e0.video-scaler: Unbalanced pm_runtime_enable!
>> exynos-gsc 13e1.video-scaler: Unbalanced pm_runtime_enable!
>>
>> Fixes: a006c04e6218 ("[media] exynos-gsc: Fixup clock management at 
>> ->remove()")
>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> I must have mixed something during the rebase of the Ulf's patch, because
> the original one kept pm_runtime_disable in the right place:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317678.html
> 
> I'm really sorry.
> 

Ah, I see. No worries.

> Acked-by: Marek Szyprowski <m.szyprow...@samsung.com>
> 

Thanks a lot for your review.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] [media] exynos-gsc: Fix imprecise external abort due disabled power domain

2017-01-17 Thread Javier Martinez Canillas

Commit 15f90ab57acc ("[media] exynos-gsc: Make driver functional when
CONFIG_PM is unset") removed the implicit dependency that the driver
had with CONFIG_PM, since it relied on the config option to be enabled.

In order to work with !CONFIG_PM, the GSC reset logic that happens in
the runtime resume callback had to be executed on the probe function.

The problem is that if CONFIG_PM is enabled, the power domain for the
GSC could be disabled and so an attempt to write to the GSC_SW_RESET
register leads to an unhandled fault / imprecise external abort error:

[   10.178825] Unhandled fault: imprecise external abort (0x1406) at 0x
[   10.186982] pgd = ed728000
[   10.190847] [] *pgd=
[   10.195553] Internal error: : 1406 [#1] PREEMPT SMP ARM
[   10.229761] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   10.237134] task: ed49e400 task.stack: ed724000
[   10.242934] PC is at gsc_wait_reset+0x5c/0x6c [exynos_gsc]
[   10.249710] LR is at gsc_probe+0x300/0x33c [exynos_gsc]
[   10.256139] pc : []lr : []psr: 60070013
[   10.256139] sp : ed725d30  ip :   fp : 0001
[   10.271492] r10: eea74800  r9 : ecd6a2c0  r8 : ed7d8854
[   10.277912] r7 : ed7d8c08  r6 : ed7d8810  r5 : 8ecd  r4 : c0c03900
[   10.285664] r3 :   r2 : 0001  r1 : ed7d8b98  r0 : ed7d8810

So only do a GSC reset if CONFIG_PM is disabled, since if is enabled the
runtime PM resume callback will be called by the VIDIOC_STREAMON ioctl,
making the reset in probe unneeded.

Fixes: 15f90ab57acc ("[media] exynos-gsc: Make driver functional when CONFIG_PM 
is unset")
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

I-ve only tested with CONFIG_PM enabled since my Exynos5422 Odroid
XU4 board fails to boot when the config option is disabled.

Best regards,
Javier

 drivers/media/platform/exynos-gsc/gsc-core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index 83272f10722d..42e1e09ea915 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1083,8 +1083,10 @@ static int gsc_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, gsc);
 
-   gsc_hw_set_sw_reset(gsc);
-   gsc_wait_reset(gsc);
+   if (!IS_ENABLED(CONFIG_PM)) {
+   gsc_hw_set_sw_reset(gsc);
+   gsc_wait_reset(gsc);
+   }
 
vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] [media] exynos-gsc: Fix unbalanced pm_runtime_enable() error

2017-01-17 Thread Javier Martinez Canillas

Commit a006c04e6218 ("[media] exynos-gsc: Fixup clock management at
->remove()") changed the driver's .remove function logic to fist do
a pm_runtime_get_sync() to make sure the device is powered before
attempting to gate the gsc clock.

But the commit also removed a pm_runtime_disable() call that leads
to an unbalanced pm_runtime_enable() error if the driver is removed
and re-probed:

exynos-gsc 13e0.video-scaler: Unbalanced pm_runtime_enable!
exynos-gsc 13e1.video-scaler: Unbalanced pm_runtime_enable!

Fixes: a006c04e6218 ("[media] exynos-gsc: Fixup clock management at ->remove()")
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
b/drivers/media/platform/exynos-gsc/gsc-core.c
index cbf75b6194b4..83272f10722d 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1118,6 +1118,7 @@ static int gsc_remove(struct platform_device *pdev)
clk_disable_unprepare(gsc->clock[i]);
 
pm_runtime_put_noidle(>dev);
+   pm_runtime_disable(>dev);
 
dev_dbg(>dev, "%s driver unloaded\n", pdev->name);
return 0;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] omap3isp: prevent releasing MC too early

2016-12-15 Thread Javier Martinez Canillas

Hello Mauro,

On 12/15/2016 09:37 AM, Mauro Carvalho Chehab wrote:

[snip]

> 
> What happens is that omap3isp driver calls media_device_unregister()
> too early. Right now, it is called at omap3isp_video_device_release(),
> with happens when a driver unbind is ordered by userspace, and not after
> the last usage of all /dev/video?? devices.
> 
> There are two possible fixes:
> 
> 1) at omap3isp_video_device_release(), streamoff all streams and mark
> that the media device will be gone.
> 
> 2) instead of using video_device_release_empty for the video->video.release,
> create a omap3isp_video_device_release() that will call
> media_device_unregister() when destroying the last /dev/video?? devnode.
>

There's also option (3), to have a proper refcounting to make sure that
the media device node is not freed until all references to it are gone.

I understand that's what Sakari's RFC patches do. I'll try to make some
time tomorrow to test and review his patches.
 
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFT PATCH] [media] partial revert of "[media] tvp5150: add HW input connectors support"

2016-12-13 Thread Javier Martinez Canillas

Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
added input signals support for the tvp5150, but the approach was found
to be incorrect so the corresponding DT binding commit 82c2ffeb217a
("[media] tvp5150: document input connectors DT bindings") was reverted.

This left the driver with an undocumented (and wrong) DT parsing logic,
so lets get rid of this code as well until the input connectors support
is implemented properly.

It's a partial revert due other patches added on top of mentioned commit
not allowing the commit to be reverted cleanly anymore. But all the code
related to the DT parsing logic and input entities creation are removed.

Suggested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

Hello Laurent,

I've tested this patch on top of media/master on my IGEPv2 + tvp5150
with the following:

$ media-ctl -r -l '"tvp5150 1-005c":1->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP 
CCDC":1->"OMAP3 ISP CCDC output":0[1]'
$ media-ctl -v --set-format '"OMAP3 ISP CCDC":0 [UYVY2X8 720x240 
field:alternate]'
$ media-ctl -v --set-format '"OMAP3 ISP CCDC":1 [UYVY2X8 720x240 
field:interlaced-tb]'
$ yavta -f UYVY -s 720x480 -n 1 --field interlaced-tb --capture=1 -F /dev/video2
$ raw2rgbpnm -f UYVY -s 720x480 frame-00.bin frame.pnm

I've also tested the other composite input with the following change:

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 5fe5faefe212..973be68ff78c 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1371,7 +1371,7 @@ static int tvp5150_probe(struct i2c_client *c,
return res;

core->norm = V4L2_STD_ALL;  /* Default is autodetect */
-   core->input = TVP5150_COMPOSITE1;
+   core->input = TVP5150_COMPOSITE0;
core->enable = true;

v4l2_ctrl_handler_init(>hdl, 5);

But as mentioned, it also worked for me without the revert so please let
me know if the driver works with your omap3 board.

Best regards,
Javier

 drivers/media/i2c/tvp5150.c | 142 
 1 file changed, 142 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 48646a7f3fb0..5fe5faefe212 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -42,8 +42,6 @@ struct tvp5150 {
struct v4l2_subdev sd;
 #ifdef CONFIG_MEDIA_CONTROLLER
struct media_pad pads[DEMOD_NUM_PADS];
-   struct media_entity input_ent[TVP5150_INPUT_NUM];
-   struct media_pad input_pad[TVP5150_INPUT_NUM];
 #endif
struct v4l2_ctrl_handler hdl;
struct v4l2_rect rect;
@@ -1018,40 +1016,6 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev 
*sd,
 }
 
 /
-   Media entity ops
- /
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-static int tvp5150_link_setup(struct media_entity *entity,
- const struct media_pad *local,
- const struct media_pad *remote, u32 flags)
-{
-   struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
-   struct tvp5150 *decoder = to_tvp5150(sd);
-   int i;
-
-   for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-   if (remote->entity == >input_ent[i])
-   break;
-   }
-
-   /* Do nothing for entities that are not input connectors */
-   if (i == TVP5150_INPUT_NUM)
-   return 0;
-
-   decoder->input = i;
-
-   tvp5150_selmux(sd);
-
-   return 0;
-}
-
-static const struct media_entity_operations tvp5150_sd_media_ops = {
-   .link_setup = tvp5150_link_setup,
-};
-#endif
-
-/
I2C Command
  /
 
@@ -1188,42 +1152,6 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, 
struct v4l2_tuner *vt)
return 0;
 }
 
-static int tvp5150_registered(struct v4l2_subdev *sd)
-{
-#ifdef CONFIG_MEDIA_CONTROLLER
-   struct tvp5150 *decoder = to_tvp5150(sd);
-   int ret = 0;
-   int i;
-
-   for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-   struct media_entity *input = >input_ent[i];
-   struct media_pad *pad = >input_pad[i];
-
-   if (!input->name)
-   continue;
-
-   decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
-
-   ret = media_entity_pads_init(input, 1, pad);
-   if (ret < 0)
-   return ret;
-
-   ret = media_device_register_entity(sd->v4l2_d

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

2016-12-12 Thread Javier Martinez Canillas

Hello Mauro,

On 12/12/2016 06:51 AM, Mauro Carvalho Chehab wrote:
> Hi Laurent,
> 
> Em Fri,  9 Dec 2016 13:47:13 +0200
> Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:
> 
>> Hello,
>>
>> This patch series fixes a regression reported by Devin Heitmueller that
>> affects a large number of em28xx. The problem was introduced by
>>
>> commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
>> Author: Mauro Carvalho Chehab <mche...@osg.samsung.com>
>> Date:   Tue Jan 26 06:59:39 2016 -0200
>>
>> [media] em28xx: fix implementation of s_stream
>>
>> that started calling s_stream(1) in the em28xx driver when enabling the
>> stream, resulting in the tvp5150 s_stream() operation writing several
>> registers with values fit for other platforms (namely OMAP3, possibly others)
>> but not for em28xx.
>>
>> The series starts with two unrelated drive-by cleanups and an unrelated bug
>> fix. It then continues with a patch to remove an unneeded and armful call to
>> tvp5150_reset() when getting the format from the subdevice (4/6), an update 
>> of
>> an invalid comment and the addition of macros for register bits in order to
>> make the code more readable (5/6) and actually allow following the incorrect
>> code flow, and finally a rework of the s_stream() operation to fix the
>> problem.
>>
>> Compared to v1,
>>
>> - Patch 4/5 now calls tvp5150_reset() at probe time
>> - Patch 5/6 is fixed with an extra ~ removed
>>
>> I haven't been able to test this with an em28xx device as I don't own any 
>> that
>> contains a tvp5150, but Mauro reported that the series fixes the issue with
>> his device.
>>
>> I also haven't been able to test anything on an OMAP3 platform, as the 
>> tvp5150
>> driver go broken on DT-based systems by
> 
> I applied today patches 1 to 3, as I don't see any risk of regressions
> there. Stable was c/c on patch 3.
> 
> I want to do more tests on patches 4-6, with both progressive video and RF. 
> It would also be nice if someone could test it on OMAP3, to be sure that no
> regressions happened there.
>

I've tested patches 4-6 on a IGEPv2 and video capture is still working for both
composite input AIP1A (after changing the hardcoded selected input) and AIP1B.

The patches also look good to me, so please feel free to add my Reviewed-by and
Tested-by tags on these.

I wasn't able to test S-Video since my S-Video source broke (an old DVD player)
but this never worked for me anyways with this board.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/5] davinci: VPIF: add DT support

2016-12-07 Thread Javier Martinez Canillas

Hello Kevin,

On Wed, Dec 7, 2016 at 3:30 PM, Kevin Hilman  wrote:
> Prepare the groundwork for adding DT support for davinci VPIF drivers.
> This series does some fixups/cleanups and then adds the DT binding and
> DT compatible string matching for DT probing.
>
> The controversial part from previous versions around async subdev
> parsing, and specifically hard-coding the input/output routing of
> subdevs, has been left out of this series.  That part can be done as a
> follow-on step after agreement has been reached on the path forward.

I had a similar need for another board (OMAP3 IGEPv2), that has a
TVP5151 video decoder (that also supports 2 composite or 1 s-video
signal) attached to the OMAP3 ISP.

I posted some RFC patches [0] to define the input signals in the DT,
and AFAICT Laurent and Hans were not against the approach but just had
some comments on the DT binding.

Basically they wanted the ports to be directly in the tvp5150 node
instead of under a connectors sub-node [1] and to just be called just
a (input / output) port instead of a connector [2].

Unfortunately I was busy with other tasks so I couldn't res-pin the
patches, but I think you could have something similar in the DT
binding for your case and it shouldn't be hard to parse the ports /
endpoints in the driver to get that information from DT and setup the
input and output pins.

> With this version, platforms can still use the VPIF capture/display
> drivers, but must provide platform_data for the subdevs and subdev
> routing.
>

I guess DT backward compatibility isn't a big issue on this platform,
since support for the platform is quite recently and after all someone
who wants to use the vpif with current DT will need platform data and
pdata-quirks anyways. So I agree with you that the input / output
signals lookup from DT could be done as a follow-up.

[0]: https://lkml.org/lkml/2016/4/12/983
[1]: https://lkml.org/lkml/2016/4/27/678
[2]: https://lkml.org/lkml/2016/11/11/346

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay

2016-12-05 Thread Javier Martinez Canillas

Hello Javi,

On 12/05/2016 07:09 AM, Javi Merino wrote:
> In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
> part of a devicetree overlay, for example:
> 
> _bridge {
>   ...
>   my_port: port@0 {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   reg = <0>;
>   ep: endpoint@0 {
>   remote-endpoint = <>;
>   };
>   };
> };
> 
> / {
>   fragment@0 {
>   target = <>;
>   __overlay__ {
>   my_cam {
>   compatible = "foo,bar";
>   port {
>   camera0: endpoint {
>   remote-endpoint = <_port>;
>   ...
>   };
>   };
>   };
>   };
>   };
> };
> 
> Each time the overlay is applied, its of_node pointer will be
> different.  We are not interested in matching the pointer, what we
> want to match is that the path is the one we are expecting.  Change to
> use of_node_cmp() so that we continue matching after the overlay has
> been removed and reapplied.
> 
> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> Signed-off-by: Javi Merino <javi.mer...@kernel.org>
> ---

I already reviewed v1 but you didn't carry the tag. So again:

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] Samsung fixes for 4.8

2016-11-28 Thread Javier Martinez Canillas

Hello Mauro,

On 11/16/2016 12:19 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 16 Nov 2016 16:08:19 +0100
> Sylwester Nawrocki <s.nawro...@samsung.com> escreveu:
> 
>> On 11/16/2016 03:46 PM, Mauro Carvalho Chehab wrote:
>>>>>> Marek Szyprowski (1):  
>>>>>>>>>   s5p-mfc: fix failure path of s5p_mfc_alloc_memdev()
>>>>>
>>>>> Mauro, this patch seems to had slipped through the cracks, I can't see it
>>>>> in neither media fixes nor the master branch. Could you please check it?  
>>>
>>> The patch seems to be on my tree:  
>>

This patch is indeed in your tree as commit:

https://git.linuxtv.org/media_tree.git/commit/?id=3467c9a7e7f9

and also in present in the media/v4.9-2 tag.

But the patch never made to mainline. In fact, I don't see any of the
patches in the media/v4.9-2 tag to be merged in v4.9-rc7.

>> Oops, sorry, I didn't check it properly. Would be nice to see that
>> patch also in linux-next.
> 
> I'll likely add all patches at linux-next today, after handling more
> patches.
> 
> Thanks,
> Mauro

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

2016-11-23 Thread Javier Martinez Canillas

Hello Javi,

On 11/23/2016 07:09 AM, Javi Merino wrote:
> In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> a devicetree overlay, its of_node pointer will be different each time
> the overlay is applied.  We are not interested in matching the
> pointer, what we want to match is that the path is the one we are
> expecting.  Change to use of_node_cmp() so that we continue matching
> after the overlay has been removed and reapplied.
>

I'm still not that familiar with DT overlays (and I guess others aren't
either) so I think that including an example of a base tree and overlay
DTS where this is an issue, could make things more clear in the commit.

IIUC, it should be something like this?

-- base tree --

 {
camera: camera@10 {
reg = <0x10>;
port {
cam_ep: endpoint {
...
};
};
};
};

_bridge {
...
ports {
port@0 {
reg = <0>;
ep: endpoint {
remote-endpoint = <_ep>;
};
};
};
};

-- overlay --

/plugin/;
/ {
...
fragment@0 {
target = <>;
__overlay__ {
compatible = "foo,bar";
...
port {
cam_ep: endpoint {
...
};
};
};
}
}

> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> Cc: Javier Martinez Canillas <jav...@osg.samsung.com>
> Cc: Sakari Ailus <sakari.ai...@linux.intel.com>
> Signed-off-by: Javi Merino <javi.mer...@kernel.org>
> ---
> Hi,
> 
> I feel it is a bit of a hack, but I couldn't think of anything better.
> I'm ccing devicetree@ and Pantelis because there may be a simpler
> solution.
>

I also couldn't think a better way to do this, since IIUC the node's name is
the only thing that doesn't change, and is available at the time the bridge
driver calls v4l2_async_notifier_register() when parsing the base tree.

>  drivers/media/v4l2-core/v4l2-async.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index 5bada20..d33a17c 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
>  
>  static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
>  {
> - return sd->of_node == asd->match.of.node;
> + return !of_node_cmp(of_node_full_name(sd->of_node),
> +         of_node_full_name(asd->match.of.node));
>  }
>  
>  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev 
> *asd)
> 

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 1/2] [media] tvp5150: Add input connectors DT bindings

2016-11-11 Thread Javier Martinez Canillas

[adding Kevin as cc since he was interested in this feature as well]

Hello Hans,

On 11/11/2016 01:11 PM, Hans Verkuil wrote:
> This old mail came up in a discussion today so let me comment on this:
>

Thanks a lot for the feedback.
 
> On 04/27/2016 09:12 PM, Javier Martinez Canillas wrote:
>> Hello Laurent,
>>
>> Thanks a lot for your feedback.
>>
>> On 04/27/2016 10:29 AM, Laurent Pinchart wrote:
>>> Hi Javier,
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday 12 Apr 2016 18:42:52 Javier Martinez Canillas wrote:
>>>> The tvp5150 and tvp5151 decoders support different video input source
>>>> connections to their AIP1A and AIP1B pins. Either two Composite or a
>>>> S-Video input signals are supported.
>>>>
>>>> The possible configurations are as follows:
>>>>
>>>> - Analog Composite signal connected to AIP1A.
>>>> - Analog Composite signal connected to AIP1B.
>>>> - Analog S-Video Y (luminance) and C (chrominance)
>>>>   signals connected to AIP1A and AIP1B respectively.
>>>>
>>>> This patch extends the Device Tree binding documentation to describe
>>>> how the input connectors for these devices should be defined in a DT.
>>>>
>>>> Suggested-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>>> Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
>>>>
>>>> ---
>>>> Hello,
>>>>
>>>> The DT binding assumes that there is a 1:1 map between physical connectors
>>>> and connections, so there will be a connector described in the DT for each
>>>> connection.
>>>>
>>>> There is also the question about how the DT bindings will be extended to
>>>> support other attributes (color/position/group) using the properties API.
>>>
>>> I foresee lots of bikeshedding on that particular topic, but I don't think 
>>> it 
>>> will be a blocker. We need a volunteer to quickstart a discussion on the 
>>> devicetree (or possible devicetree-spec) mailing list :-)
>>>
>>
>> Yes, I plan to extend this binding once we have the properties API in 
>> mainline
>> but that can be done as a follow-up since it should just be more properties 
>> on
>> top of compatible, label and port that will be supported in the meantime.
>>  
>>>> But I believe that can be done as a follow-up, once the properties API is
>>>> in mainline.
>>>>
>>>> Best regards,
>>>> Javier
>>>>
>>>> Changes in v2:
>>>> - Remove from the changelog a mention of devices that multiplex the
>>>>   physical RCA connectors to be used for the S-Video Y and C signals
>>>>   since it's a special case and it doesn't really work on the IGEPv2.
>>>>
>>>>  .../devicetree/bindings/media/i2c/tvp5150.txt  | 59 
>>>> +++
>>>>  1 file changed, 59 insertions(+)
>>>> :
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>>> b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt index
>>>> 8c0fc1a26bf0..df555650b0b4 100644
>>>> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
>>>> @@ -26,8 +26,46 @@ Required Endpoint Properties for parallel
>>>> synchronization: If none of hsync-active, vsync-active and
>>>> field-even-active is specified, the endpoint is assumed to use embedded
>>>> BT.656 synchronization.
>>>>
>>>> +-Optional nodes:
>>>> +- connectors: The list of tvp5150 input connectors available on a given
>>>> +  board. The node should contain a child 'port' node for each connector.
>>>
>>> I had understood this as meaning that connectors should be fully described 
>>> in 
>>> the connectors subnode, until I read through the whole patch and saw that 
>>> dedicated DT nodes are needed for the connectors. I thus believe the 
>>> paragraph 
>>> should be reworded to avoid the ambiguity.
>>>
>>
>> I see what you mean, OK I'll make it clear that this only is the list of 
>> ports
>> and that connectors should be described somewhere else (i.e: the root node).
>>
>>> This being said, why do you need a connectors subnode ? Can't we just add 
>>> the 
>>> port nodes for the input ports directly

Re: [RFC PATCH 6/6] [media] davinci: vpif_capture: get subdevs from DT

2016-11-11 Thread Javier Martinez Canillas

On Fri, Nov 11, 2016 at 12:50 PM, Javier Martinez Canillas
<jav...@dowhile0.org> wrote:

[snip]

> So if you can comment on this and see if the DT bindings fits your,
> would be very useful.
>

Sorry, I wanted to write "if the DT bindings fits your needs, it would
be very useful".

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 6/6] [media] davinci: vpif_capture: get subdevs from DT

2016-11-11 Thread Javier Martinez Canillas

Hello,

On Fri, Nov 11, 2016 at 12:36 PM, Hans Verkuil  wrote:
> On 10/26/2016 01:55 AM, Kevin Hilman wrote:
>> First pass at getting subdevs from DT ports and endpoints.
>>
>> The _get_pdata() function was larely inspired by (i.e. stolen from)
>> am437x-vpfe.c
>>
>> Questions:
>> - Legacy board file passes subdev input & output routes via pdata
>>   (e.g. tvp514x svideo or composite selection.)  How is this supposed
>>   to be done via DT?
>
> We have plans to model connectors as well in the device tree, but no
> implementation exists yet. I think Laurent has some code in progress for this,
> but I may be mistaken.
>

I posted a RFC series [0] some time ago, that proposed a DT binding
for input connectors [1] using OF graphs.

I never re-spin the series because Laurent had some comments on the DT
bindings and I was waiting for a response on to my latest email [2].
So if you can comment on this and see if the DT bindings fits your,
would be very useful.

> Anyway, hard-coding it like you do now is for now the only way.
>
> Hans
>
>>

[0]: https://www.mail-archive.com/linux-media@vger.kernel.org/msg96393.html
[1]: http://www.spinics.net/lists/linux-media/msg99421.html
[2]: http://www.spinics.net/lists/linux-media/msg99987.html

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/9] Qualcomm video decoder/encoder driver

2016-11-11 Thread Javier Martinez Canillas

Hello Hans,

On Fri, Nov 11, 2016 at 8:49 AM, Hans Verkuil  wrote:
> Hi Stanimir,
>
> Overall it looks good. As you saw, I do have some comments but nothing major.
>
> One question: you use qcom as the directory name. How about using qualcomm?
>
> It's really not that much longer and a bit more obvious.
>
> Up to you, though.
>

It seems qcom is more consistent to the name used in most subsystems
for Qualcomm:

$ find -name *qcom
./arch/arm/mach-qcom
./arch/arm64/boot/dts/qcom
./Documentation/devicetree/bindings/soc/qcom
./sound/soc/qcom
./drivers/pinctrl/qcom
./drivers/soc/qcom
./drivers/clk/qcom

$ find -name *qualcomm
./drivers/net/ethernet/qualcomm

> Regards,
>
> Hans
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] exynos-gsc: Enable driver on ARCH_EXYNOS

2016-11-09 Thread Javier Martinez Canillas

Hello Marek,

On 11/09/2016 11:29 AM, Marek Szyprowski wrote:
> This driver can be also used on Exynos5433, which is ARM64-based
> platform, which selects only ARCH_EXYNOS symbol.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---
>  drivers/media/platform/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 754edbf1..90ae790 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -266,7 +266,7 @@ config VIDEO_MX2_EMMAPRP
>  config VIDEO_SAMSUNG_EXYNOS_GSC
>   tristate "Samsung Exynos G-Scaler driver"
>   depends on VIDEO_DEV && VIDEO_V4L2
> - depends on ARCH_EXYNOS5 || COMPILE_TEST
> + depends on ARCH_EXYNOS || COMPILE_TEST
>   depends on HAS_DMA
>   select VIDEOBUF2_DMA_CONTIG
>   select V4L2_MEM2MEM_DEV
> 

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] exynos-gsc: Add support for Exynos5433 specific version

2016-11-09 Thread Javier Martinez Canillas

Hello Marek,

On 11/09/2016 11:29 AM, Marek Szyprowski wrote:
> This patch add support for Exynos5433 specific version of GScaller module.

s/GScaller/GScaler

> The main difference is between Exynos 5433 and earlier is addition of
> new clocks that have to be controlled.
> 
> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> ---
>  .../devicetree/bindings/media/exynos5-gsc.txt  |  3 +-
>  drivers/media/platform/exynos-gsc/gsc-core.c   | 74 
> --
>  drivers/media/platform/exynos-gsc/gsc-core.h   |  6 +-
>  3 files changed, 62 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/exynos5-gsc.txt 
> b/Documentation/devicetree/bindings/media/exynos5-gsc.txt

Usually the DT changes go in a separate patch as documented in
Documentation/devicetree/bindings/submitting-patches.txt.

But I guess this is a too small change so is OK to squash it?

> index 5fe9372..26ca25b 100644
> --- a/Documentation/devicetree/bindings/media/exynos5-gsc.txt
> +++ b/Documentation/devicetree/bindings/media/exynos5-gsc.txt
> @@ -3,7 +3,8 @@
>  G-Scaler is used for scaling and color space conversion on EXYNOS5 SoCs.
>  
>  Required properties:
> -- compatible: should be "samsung,exynos5-gsc"
> +- compatible: should be "samsung,exynos5-gsc" (for Exynos 5250, 5420 and
> +   5422 SoCs) or "samsung,exynos5433-gsc" (Exynos 5433)

I would also add 5800 to the list.

Besides these minor comments, the patch looks good to me:

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

And I've also tested in an Exynos5800 Peach Pi Chromebook:

Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/12] media: Exynos GScaller driver fixes

2016-11-09 Thread Javier Martinez Canillas

Hello Marek,

On 11/09/2016 11:23 AM, Marek Szyprowski wrote:
> Hi!
> 
> This is a collection of various fixes and cleanups for Exynos GScaller
> media driver. Most of them comes from the forgotten patchset posted long
> time ago by Ulf Hansson:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg80592.html
> 
> While testing and rebasing them, I added some more cleanups. Tested on
> Exynos5422-based Odroid XU3 board.
> 

I've tested this series on a Exynos5800-based Peach Pi Chromebook. The
patches were tested on top of Sylwester's for-v4.10/media/next branch:

git://linuxtv.org/snawrocki/samsung.git for-v4.10/media/next

So feel free to add for the whole series:

Tested-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch 20/35] media: ti-vpe: vpe: Added MODULE_DEVICE_TABLE hint

2016-10-24 Thread Javier Martinez Canillas

Hello Benoit,

On Wed, Sep 28, 2016 at 5:22 PM, Benoit Parrot <bpar...@ti.com> wrote:
> ti_vpe module currently does not get loaded automatically.
> Added MODULE_DEVICE_TABLE hint to the driver to assist.
>
> Signed-off-by: Benoit Parrot <bpar...@ti.com>
> ---

Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] [media] s5p-cec: Fix module autoload

2016-10-17 Thread Javier Martinez Canillas

If the driver is built as a module, autoload won't work because the module
alias information is not filled. So user-space can't match the registered
device with the corresponding module.

Export the module alias information using the MODULE_DEVICE_TABLE() macro.

Before this patch:

$ modinfo drivers/staging/media/s5p-cec/s5p-cec.ko | grep alias
$

After this patch:

$ modinfo drivers/staging/media/s5p-cec/s5p-cec.ko | grep alias
alias:  of:N*T*Csamsung,s5p-cecC*
alias:  of:N*T*Csamsung,s5p-cec

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/staging/media/s5p-cec/s5p_cec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/s5p-cec/s5p_cec.c 
b/drivers/staging/media/s5p-cec/s5p_cec.c
index 1780a08b73c9..4e41f72dbfaa 100644
--- a/drivers/staging/media/s5p-cec/s5p_cec.c
+++ b/drivers/staging/media/s5p-cec/s5p_cec.c
@@ -263,6 +263,7 @@ static const struct of_device_id s5p_cec_match[] = {
},
{},
 };
+MODULE_DEVICE_TABLE(of, s5p_cec_match);
 
 static struct platform_driver s5p_cec_pdrv = {
.probe  = s5p_cec_probe,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] [media] st-cec: Fix module autoload

2016-10-17 Thread Javier Martinez Canillas

If the driver is built as a module, autoload won't work because the module
alias information is not filled. So user-space can't match the registered
device with the corresponding module.

Export the module alias information using the MODULE_DEVICE_TABLE() macro.

Before this patch:

$ modinfo drivers/staging/media//st-cec/stih-cec.ko | grep alias
$

After this patch:

$ modinfo drivers/staging/media//st-cec/stih-cec.ko | grep alias
alias:  of:N*T*Cst,stih-cecC*
alias:  of:N*T*Cst,stih-cec

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/staging/media/st-cec/stih-cec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/st-cec/stih-cec.c 
b/drivers/staging/media/st-cec/stih-cec.c
index 214344866a6b..19d3ff30c8f8 100644
--- a/drivers/staging/media/st-cec/stih-cec.c
+++ b/drivers/staging/media/st-cec/stih-cec.c
@@ -363,6 +363,7 @@ static const struct of_device_id stih_cec_match[] = {
},
{},
 };
+MODULE_DEVICE_TABLE(of, stih_cec_match);
 
 static struct platform_driver stih_cec_pdrv = {
.probe  = stih_cec_probe,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] [media] rc: meson-ir: Fix module autoload

2016-10-17 Thread Javier Martinez Canillas

If the driver is built as a module, autoload won't work because the module
alias information is not filled. So user-space can't match the registered
device with the corresponding module.

Export the module alias information using the MODULE_DEVICE_TABLE() macro.

Before this patch:

$ modinfo drivers/media/rc/meson-ir.ko | grep alias
$

After this patch:

$ modinfo drivers/media/rc/meson-ir.ko | grep alias
alias:  of:N*T*Camlogic,meson-gxbb-irC*
alias:  of:N*T*Camlogic,meson-gxbb-ir
alias:  of:N*T*Camlogic,meson8b-irC*
alias:  of:N*T*Camlogic,meson8b-ir
alias:  of:N*T*Camlogic,meson6-irC*
alias:  of:N*T*Camlogic,meson6-ir

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/rc/meson-ir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
index 003fff07ade2..7eb3f4f1ddcd 100644
--- a/drivers/media/rc/meson-ir.c
+++ b/drivers/media/rc/meson-ir.c
@@ -218,6 +218,7 @@ static const struct of_device_id meson_ir_match[] = {
{ .compatible = "amlogic,meson-gxbb-ir" },
{ },
 };
+MODULE_DEVICE_TABLE(of, meson_ir_match);
 
 static struct platform_driver meson_ir_driver = {
.probe  = meson_ir_probe,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] [media] Fix module autoload for media platform drivers

2016-10-17 Thread Javier Martinez Canillas

Hello Mauro,

I noticed that module autoload won't be working in a bunch of media
platform drivers because the module alias information is not filled
in the modules. This patch series contains the fixes for them.

Best regards,
Javier


Javier Martinez Canillas (5):
  [media] v4l: vsp1: Fix module autoload for OF registration
  [media] v4l: rcar-fcp: Fix module autoload for OF registration
  [media] rc: meson-ir: Fix module autoload
  [media] s5p-cec: Fix module autoload
  [media] st-cec: Fix module autoload

 drivers/media/platform/rcar-fcp.c   | 1 +
 drivers/media/platform/vsp1/vsp1_drv.c  | 1 +
 drivers/media/rc/meson-ir.c | 1 +
 drivers/staging/media/s5p-cec/s5p_cec.c | 1 +
 drivers/staging/media/st-cec/stih-cec.c | 1 +
 5 files changed, 5 insertions(+)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] [media] v4l: rcar-fcp: Fix module autoload for OF registration

2016-10-17 Thread Javier Martinez Canillas

If the driver is built as a module, autoload won't work because the module
alias information is not filled. So user-space can't match the registered
device with the corresponding module.

Export the module alias information using the MODULE_DEVICE_TABLE() macro.

Before this patch:

$ modinfo drivers/media/platform/rcar-fcp.ko | grep alias
alias:  rcar-fcp

After this patch:

$ modinfo drivers/media/platform/rcar-fcp.ko | grep alias
alias:  rcar-fcp
alias:  of:N*T*Crenesas,fcpvC*
alias:  of:N*T*Crenesas,fcpv
alias:  of:N*T*Crenesas,fcpfC*
alias:  of:N*T*Crenesas,fcpf

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/platform/rcar-fcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/rcar-fcp.c 
b/drivers/media/platform/rcar-fcp.c
index f3a3f31cdfa9..7146fc5ef168 100644
--- a/drivers/media/platform/rcar-fcp.c
+++ b/drivers/media/platform/rcar-fcp.c
@@ -169,6 +169,7 @@ static const struct of_device_id rcar_fcp_of_match[] = {
{ .compatible = "renesas,fcpv" },
{ },
 };
+MODULE_DEVICE_TABLE(of, rcar_fcp_of_match);
 
 static struct platform_driver rcar_fcp_platform_driver = {
.probe  = rcar_fcp_probe,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] [media] v4l: vsp1: Fix module autoload for OF registration

2016-10-17 Thread Javier Martinez Canillas

If the driver is built as a module, autoload won't work because the module
alias information is not filled. So user-space can't match the registered
device with the corresponding module.

Export the module alias information using the MODULE_DEVICE_TABLE() macro.

Before this patch:

$ modinfo drivers/media/platform/vsp1/vsp1.ko | grep alias
alias:  vsp1

After this patch:

$ modinfo drivers/media/platform/vsp1/vsp1.ko | grep alias
alias:  vsp1
alias:  of:N*T*Crenesas,vsp2C*
alias:  of:N*T*Crenesas,vsp2
alias:  of:N*T*Crenesas,vsp1C*
alias:  of:N*T*Crenesas,vsp1

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/platform/vsp1/vsp1_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index 57c713a4e1df..aa237b48ad55 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -770,6 +770,7 @@ static const struct of_device_id vsp1_of_match[] = {
{ .compatible = "renesas,vsp2" },
{ },
 };
+MODULE_DEVICE_TABLE(of, vsp1_of_match);
 
 static struct platform_driver vsp1_platform_driver = {
.probe  = vsp1_probe,
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] [media] exynos-gsc: cleanup m2m src and dst vb2 queues on STREAMOFF

2016-10-07 Thread Javier Martinez Canillas

Media drivers that use the videobuf2 framework have to give back to vb2
all the buffers that received from vb2 using its .buf_queue callback.

But the exynos-gsc driver isn't doing a proper cleanup so vb2 complains
that the number of buffers enqueued and received are not balanced:

WARNING: CPU: 2 PID: 660 at drivers/media/v4l2-core/videobuf2-core.c:1654 
__vb2_queue_cancel+0xec/0x150 [videobuf2_core]
Modules linked in: mwifiex_sdio mwifiex uvcvideo exynos_gsc videobuf2_vmalloc 
s5p_mfc s5p_jpeg
CPU: 2 PID: 660 Comm: lt-gst-validate Not tainted 4.8.0
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x88/0x9c)
[] (dump_stack) from [] (__warn+0xe8/0x100)
[] (__warn) from [] (warn_slowpath_null+0x20/0x28)
[] (warn_slowpath_null) from [] 
(__vb2_queue_cancel+0xec/0x150 [videobuf2_core])
[] (__vb2_queue_cancel [videobuf2_core]) from [] 
(vb2_core_streamoff+0x34/0x9c [videobuf2_core])
[] (vb2_core_streamoff [videobuf2_core]) from [] 
(v4l2_m2m_streamoff+0x2c/0xe4 [v4l2_mem2mem])
[] (v4l2_m2m_streamoff [v4l2_mem2mem]) from [] 
(__video_do_ioctl+0x298/0x30c [videodev])
[] (__video_do_ioctl [videodev]) from [] 
(video_usercopy+0x174/0x4e8 [videodev])
[] (video_usercopy [videodev]) from [] 
(v4l2_ioctl+0xc4/0xd8 [videodev])
[] (v4l2_ioctl [videodev]) from [] (do_vfs_ioctl+0x9c/0x8f4)
[] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c)
[] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c)

Fix this by passing back to vb2 all the received buffers that were not
processed.

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>

---

 drivers/media/platform/exynos-gsc/gsc-m2m.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c 
b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index c8c0bcec35ed..f49f24b4462a 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -66,12 +66,29 @@ static int gsc_m2m_start_streaming(struct vb2_queue *q, 
unsigned int count)
return ret > 0 ? 0 : ret;
 }
 
+static void __gsc_m2m_cleanup_queue(struct gsc_ctx *ctx)
+{
+   struct vb2_v4l2_buffer *src_vb, *dst_vb;
+
+   while (v4l2_m2m_num_src_bufs_ready(ctx->m2m_ctx) > 0) {
+   src_vb = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
+   v4l2_m2m_buf_done(src_vb, VB2_BUF_STATE_ERROR);
+   }
+
+   while (v4l2_m2m_num_dst_bufs_ready(ctx->m2m_ctx) > 0) {
+   dst_vb = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
+   v4l2_m2m_buf_done(dst_vb, VB2_BUF_STATE_ERROR);
+   }
+}
+
 static void gsc_m2m_stop_streaming(struct vb2_queue *q)
 {
struct gsc_ctx *ctx = q->drv_priv;
 
__gsc_m2m_job_abort(ctx);
 
+   __gsc_m2m_cleanup_queue(ctx);
+
pm_runtime_put(>gsc_dev->pdev->dev);
 }
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] [media] exynos-gsc: unregister video device node on driver removal

2016-10-07 Thread Javier Martinez Canillas

The driver doesn't unregister the video device node when the driver is
removed, this keeps video device nodes that makes the machine to crash
with a NULL pointer dereference when nodes are attempted to be opened:

[   36.530006] Unable to handle kernel paging request at virtual address 
bf1f8200
[   36.535985] pgd = edbbc000
[   36.538486] [bf1f8200] *pgd=6d99a811, *pte=, *ppte=
[   36.544727] Internal error: Oops: 7 [#1] PREEMPT SMP ARM
[   36.550016] Modules linked in: s5p_jpeg s5p_mfc v4l2_mem2mem 
videobuf2_dma_contig
[   36.566303] CPU: 6 PID: 533 Comm: v4l2-ctl Not tainted 4.8.0
[   36.574466] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[   36.580526] task: ee3cc600 task.stack: ed626000
[   36.585046] PC is at try_module_get+0x1c/0xac
[   36.589364] LR is at try_module_get+0x1c/0xac
[   36.593698] pc : []lr : []psr: 80070013
[   36.593698] sp : ed627de0  ip : a0070013  fp : 
[   36.605156] r10: 0002  r9 : ed627ed0  r8 : 
[   36.610331] r7 : c01e5f14  r6 : ed57be00  r5 : bf1f8200  r4 : bf1f8200
[   36.616834] r3 : 0002  r2 : 0002  r1 : 01930192  r0 : 0001
..
[   36.785004] [] (try_module_get) from [] 
(cdev_get+0x1c/0x4c)
[   36.792362] [] (cdev_get) from [] 
(chrdev_open+0x2c/0x178)
[   36.799555] [] (chrdev_open) from [] 
(do_dentry_open+0x1e0/0x300)
[   36.807360] [] (do_dentry_open) from [] 
(path_openat+0x35c/0xf58)
[   36.815154] [] (path_openat) from [] 
(do_filp_open+0x5c/0xc0)
[   36.822606] [] (do_filp_open) from [] 
(do_sys_open+0x10c/0x1bc)
[   36.830235] [] (do_sys_open) from [] 
(ret_fast_syscall+0x0/0x3c)
[   36.837942] Code: 0a1c e1a04000 e3a1 ebfec92d (e5943000)

Signed-off-by: Javier Martinez Canillas <jav...@osg.samsung.com>
---

 drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c 
b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index a1cac52ea230..c8c0bcec35ed 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -781,6 +781,8 @@ err_m2m_release:
 
 void gsc_unregister_m2m_device(struct gsc_dev *gsc)
 {
-   if (gsc)
+   if (gsc) {
v4l2_m2m_release(gsc->m2m.m2m_dev);
+   video_unregister_device(>vdev);
+   }
 }
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] [media] exynos-gsc: Another round of cleanup and fixes

2016-10-07 Thread Javier Martinez Canillas

Hello,

This series contains another set of cleanup and fixes for the exynos-gsc
driver. The patches are on top of the previous posted set [0], although
there's no dependency and the patch-sets can be applied in any order.

Patch 1/3 is a cleanup for the gsc_register_m2m_device() error path

Patch 2/3 fixes a NULL pointer deference that happens when the driver
module is removed due the video device node not being unregistered.

Patch 3/3 fixes a warning due the driver not doing proper cleanup of
the m2m source and destination queues.

[0]: https://lkml.org/lkml/2016/9/30/413

Best regards,
Javier


Javier Martinez Canillas (3):
  [media] exynos-gsc: don't release a non-dynamically allocated
video_device
  [media] exynos-gsc: unregister video device node on driver removal
  [media] exynos-gsc: cleanup m2m src and dst vb2 queues on STREAMOFF

 drivers/media/platform/exynos-gsc/gsc-m2m.c | 30 ++---
 1 file changed, 23 insertions(+), 7 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

1 2 3 4 5 >

1 - 100 of 436 matches

Mail list logo