date:20150211

Re: [PATCH] media: Pinnacle 73e infrared control stopped working since kernel 3.17

2015-02-11 Thread David Cimbůrek

I'll try to describe my thoughts.

The changed structure "dib0700_rc_response" is used in
dib0700_core.c:dib0700_rc_urb_completion(struct urb *purb) function:

struct dib0700_rc_response *poll_reply;
...
poll_reply = purb->transfer_buffer;

dib0700_rc_urb_completion() is then used in
dib0700_core.c:dib0700_rc_setup() in macros usb_fill_bulk_urb and
usb_fill_int_urb. These macros are defined in header file usb.h. Here
I have found in macro description this:

 * @transfer_buffer: pointer to the transfer buffer

I suppose that it means that the struct dib0700_rc_response is being
filled from this transfer buffer. Therefore I suppose that the order
of structure members IS important.

Of course it's only my guess but my patch is really working for me :-)



2015-02-12 1:10 GMT+01:00 Luis de Bethencourt :
> On Tue, Feb 10, 2015 at 11:38:11AM +0100, David Cimbůrek wrote:
>> Please include this patch to kernel! It takes too much time for such a
>> simple fix!
>>
>
> The patch is simple but why it fixes the issue isn't that simple. Could you
> explain why the order of the variables inside the structure is breaking 
> things?
>
> All the uses of the variables inside the structure that I can see are by name.
> Not by memory offsets.
>
> Thanks,
> Luis
>
>>
>> 2015-01-07 13:51 GMT+01:00 David Cimbůrek :
>> > No one is interested? I'd like to get this patch to kernel to fix the
>> > issue. Can someone here do it please?
>> >
>> >
>> > 2014-12-20 14:36 GMT+01:00 David Cimbůrek :
>> >> Hi,
>> >>
>> >> with kernel 3.17 remote control for Pinnacle 73e (ID 2304:0237
>> >> Pinnacle Systems, Inc. PCTV 73e [DiBcom DiB7000PC]) does not work
>> >> anymore.
>> >>
>> >> I checked the changes and found out the problem in commit
>> >> af3a4a9bbeb00df3e42e77240b4cdac5479812f9.
>> >>
>> >> In dib0700_core.c in struct dib0700_rc_response the following union:
>> >>
>> >> union {
>> >> u16 system16;
>> >> struct {
>> >> u8 not_system;
>> >> u8 system;
>> >> };
>> >> };
>> >>
>> >> has been replaced by simple variables:
>> >>
>> >> u8 system;
>> >> u8 not_system;
>> >>
>> >> But these variables are in reverse order! When I switch the order
>> >> back, the remote works fine again! Here is the patch:
>> >>
>> >>
>> >> --- a/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
>> >> 14:27:15.0 +0100
>> >> +++ b/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
>> >> 14:27:36.0 +0100
>> >> @@ -658,8 +658,8 @@
>> >>  struct dib0700_rc_response {
>> >>  u8 report_id;
>> >>  u8 data_state;
>> >> -u8 system;
>> >>  u8 not_system;
>> >> +u8 system;
>> >>  u8 data;
>> >>  u8 not_data;
>> >>  };
>> >>
>> >>
>> >> Regards,
>> >> David
>> --
>> 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
--
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

cron job: media_tree daily build: WARNINGS

2015-02-11 Thread Hans Verkuil

This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.

Results of the daily build of media_tree:

date:   Thu Feb 12 04:00:26 CET 2015
git branch: test
git hash:   d6d4c0e00fe559ef54b414e2e6266beaa50b4d8e
gcc version:i686-linux-gcc (GCC) 4.9.1
sparse version: v0.5.0-41-g6c2d743
smatch version: 0.4.1-3153-g7d56ab3
host hardware:  x86_64
host os:3.18.0-5.slh.1-amd64

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-exynos: OK
linux-git-arm-mx: OK
linux-git-arm-omap: OK
linux-git-arm-omap1: OK
linux-git-arm-pxa: OK
linux-git-blackfin: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.32.27-i686: OK
linux-2.6.33.7-i686: OK
linux-2.6.34.7-i686: OK
linux-2.6.35.9-i686: OK
linux-2.6.36.4-i686: OK
linux-2.6.37.6-i686: OK
linux-2.6.38.8-i686: OK
linux-2.6.39.4-i686: OK
linux-3.0.60-i686: OK
linux-3.1.10-i686: OK
linux-3.2.37-i686: OK
linux-3.3.8-i686: OK
linux-3.4.27-i686: OK
linux-3.5.7-i686: OK
linux-3.6.11-i686: OK
linux-3.7.4-i686: OK
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: OK
linux-3.11.1-i686: OK
linux-3.12.23-i686: OK
linux-3.13.11-i686: OK
linux-3.14.9-i686: OK
linux-3.15.2-i686: OK
linux-3.16-i686: OK
linux-3.17.8-i686: OK
linux-3.18-i686: OK
linux-3.19-rc4-i686: OK
linux-2.6.32.27-x86_64: OK
linux-2.6.33.7-x86_64: OK
linux-2.6.34.7-x86_64: OK
linux-2.6.35.9-x86_64: OK
linux-2.6.36.4-x86_64: OK
linux-2.6.37.6-x86_64: OK
linux-2.6.38.8-x86_64: OK
linux-2.6.39.4-x86_64: OK
linux-3.0.60-x86_64: OK
linux-3.1.10-x86_64: OK
linux-3.2.37-x86_64: OK
linux-3.3.8-x86_64: OK
linux-3.4.27-x86_64: OK
linux-3.5.7-x86_64: OK
linux-3.6.11-x86_64: OK
linux-3.7.4-x86_64: OK
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: OK
linux-3.11.1-x86_64: OK
linux-3.12.23-x86_64: OK
linux-3.13.11-x86_64: OK
linux-3.14.9-x86_64: OK
linux-3.15.2-x86_64: OK
linux-3.16-x86_64: OK
linux-3.17.8-x86_64: OK
linux-3.18-x86_64: OK
linux-3.19-rc4-x86_64: OK
apps: OK
spec-git: OK
sparse: WARNINGS
smatch: ERRORS

Detailed results are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.log

Full logs are available here:

http://www.xs4all.nl/~hverkuil/logs/Thursday.tar.bz2

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/media.html
--
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] [media] si2165: Fix possible leak in si2165_upload_firmware()

2015-02-11 Thread Luis de Bethencourt

On Thu, Feb 12, 2015 at 01:42:45AM +0200, Antti Palosaari wrote:
> On 02/12/2015 01:38 AM, Luis de Bethencourt wrote:
> >On Wed, Feb 11, 2015 at 10:45:01PM +0100, Matthias Schwarzott wrote:
> >>On 11.02.2015 21:58, Christian Engelmayer wrote:
> >>>In case of an error function si2165_upload_firmware() releases the already
> >>>requested firmware in the exit path. However, there is one deviation where
> >>>the function directly returns. Use the correct cleanup so that the firmware
> >>>memory gets freed correctly. Detected by Coverity CID 1269120.
> >>>
> >>>Signed-off-by: Christian Engelmayer 
> >>>---
> >>>Compile tested only. Applies against linux-next.
> >>>---
> >>>  drivers/media/dvb-frontends/si2165.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/media/dvb-frontends/si2165.c 
> >>>b/drivers/media/dvb-frontends/si2165.c
> >>>index 98ddb49ad52b..4cc5d10ed0d4 100644
> >>>--- a/drivers/media/dvb-frontends/si2165.c
> >>>+++ b/drivers/media/dvb-frontends/si2165.c
> >>>@@ -505,7 +505,7 @@ static int si2165_upload_firmware(struct si2165_state 
> >>>*state)
> >>>   /* reset crc */
> >>>   ret = si2165_writereg8(state, 0x0379, 0x01);
> >>>   if (ret)
> >>>-  return ret;
> >>>+  goto error;
> >>>
> >>>   ret = si2165_upload_firmware_block(state, data, len,
> >>>  &offset, block_count);
> >>>
> >>Good catch.
> >>
> >>Signed-off-by: Matthias Schwarzott 
> >>
> >
> >Good catch indeed.
> >
> >Can I sign off? Not sure what the rules are.
> >
> >Signed-off-by: Luis de Bethencourt 
> 
> 
> You cannot sign it unless patch is going through hands. Probably you want
> review it. Check documentation "SubmittingPatches".
> 
> https://www.kernel.org/doc/Documentation/SubmittingPatches
> 
> regards
> Antti
> 
> -- 
> http://palosaari.fi/
> --

Hi Antti,

That was an interesting read. Now I know how these tags work :)
Thanks for the pointing it out to me.

So I meant "Reviewed-by:"

Luis
--
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] dvb-usb-v2: add support for the media controller at USB driver

2015-02-11 Thread Antti Palosaari


Moikka!

On 02/12/2015 02:04 AM, Rafael Lourenço de Lima Chehab wrote:

Create a struct media_device and add it to the dvb adapter.

Please notice that the tuner is not mapped yet by the dvb core.

Signed-off-by: Rafael Lourenço de Lima Chehab 
---
  drivers/media/usb/dvb-usb-v2/dvb_usb.h  |  5 +++
  drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 61 +
  2 files changed, 66 insertions(+)


I am not against that patch, but I don't simply understand media 
controller concept enough detailed level. So it is all up to Mauro.


regards
Antti

--
http://palosaari.fi/
--
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] media: Pinnacle 73e infrared control stopped working since kernel 3.17

2015-02-11 Thread Luis de Bethencourt

On Tue, Feb 10, 2015 at 11:38:11AM +0100, David Cimbůrek wrote:
> Please include this patch to kernel! It takes too much time for such a
> simple fix!
> 

The patch is simple but why it fixes the issue isn't that simple. Could you
explain why the order of the variables inside the structure is breaking things?

All the uses of the variables inside the structure that I can see are by name.
Not by memory offsets.

Thanks,
Luis

> 
> 2015-01-07 13:51 GMT+01:00 David Cimbůrek :
> > No one is interested? I'd like to get this patch to kernel to fix the
> > issue. Can someone here do it please?
> >
> >
> > 2014-12-20 14:36 GMT+01:00 David Cimbůrek :
> >> Hi,
> >>
> >> with kernel 3.17 remote control for Pinnacle 73e (ID 2304:0237
> >> Pinnacle Systems, Inc. PCTV 73e [DiBcom DiB7000PC]) does not work
> >> anymore.
> >>
> >> I checked the changes and found out the problem in commit
> >> af3a4a9bbeb00df3e42e77240b4cdac5479812f9.
> >>
> >> In dib0700_core.c in struct dib0700_rc_response the following union:
> >>
> >> union {
> >> u16 system16;
> >> struct {
> >> u8 not_system;
> >> u8 system;
> >> };
> >> };
> >>
> >> has been replaced by simple variables:
> >>
> >> u8 system;
> >> u8 not_system;
> >>
> >> But these variables are in reverse order! When I switch the order
> >> back, the remote works fine again! Here is the patch:
> >>
> >>
> >> --- a/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
> >> 14:27:15.0 +0100
> >> +++ b/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
> >> 14:27:36.0 +0100
> >> @@ -658,8 +658,8 @@
> >>  struct dib0700_rc_response {
> >>  u8 report_id;
> >>  u8 data_state;
> >> -u8 system;
> >>  u8 not_system;
> >> +u8 system;
> >>  u8 data;
> >>  u8 not_data;
> >>  };
> >>
> >>
> >> Regards,
> >> David
> --
> 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
--
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 RFC] dvb-usb-v2: add support for the media controller at USB driver

2015-02-11 Thread Rafael Lourenço de Lima Chehab

Create a struct media_device and add it to the dvb adapter.

Please notice that the tuner is not mapped yet by the dvb core.

Signed-off-by: Rafael Lourenço de Lima Chehab 
---
 drivers/media/usb/dvb-usb-v2/dvb_usb.h  |  5 +++
 drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 61 +
 2 files changed, 66 insertions(+)

diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb.h 
b/drivers/media/usb/dvb-usb-v2/dvb_usb.h
index 14e111e13e54..b273250d0e31 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb.h
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "dvb_frontend.h"
 #include "dvb_demux.h"
@@ -389,6 +390,10 @@ struct dvb_usb_device {
struct delayed_work rc_query_work;
 
void *priv;
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+   struct media_device *media_dev;
+#endif
 };
 
 extern int dvb_usbv2_probe(struct usb_interface *,
diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c 
b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
index 1950f37df835..ea4d7bec8fc1 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c
@@ -86,6 +86,8 @@ static int dvb_usbv2_i2c_init(struct dvb_usb_device *d)
goto err;
}
 
+   dvb_create_media_graph(d->media_dev);
+
return 0;
 err:
dev_dbg(&d->udev->dev, "%s: failed=%d\n", __func__, ret);
@@ -400,6 +402,55 @@ skip_feed_stop:
return ret;
 }
 
+static void dvb_usbv2_media_device_register(struct dvb_usb_device *d)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+
+   struct media_device *mdev;
+   struct usb_device *udev = d->udev;
+   int ret;
+
+   mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+   if (!mdev)
+   return;
+
+   mdev->dev = &udev->dev;
+   strlcpy(mdev->model, d->name, sizeof(mdev->model));
+   if (udev->serial)
+   strlcpy(mdev->serial, udev->serial, sizeof(mdev->serial));
+   strcpy(mdev->bus_info, udev->devpath);
+   mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
+   mdev->driver_version = LINUX_VERSION_CODE;
+
+   ret = media_device_register(mdev);
+   if (ret) {
+   dev_err(&d->udev->dev,
+   "Couldn't create a media device. Error: %d\n",
+   ret);
+   kfree(mdev);
+   return;
+   }
+
+   d->media_dev = mdev;
+
+   dev_info(&d->udev->dev, "media controller created\n");
+
+#endif
+}
+
+static void dvb_usbv2_media_device_unregister (struct dvb_usb_device *d)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+   if (!d->media_dev)
+   return;
+
+   media_device_unregister(d->media_dev);
+   kfree(d->media_dev);
+   d->media_dev = NULL;
+
+#endif
+}
+
 static int dvb_usbv2_adapter_dvb_init(struct dvb_usb_adapter *adap)
 {
int ret;
@@ -416,6 +467,11 @@ static int dvb_usbv2_adapter_dvb_init(struct 
dvb_usb_adapter *adap)
 
adap->dvb_adap.priv = adap;
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+   dvb_usbv2_media_device_register(d);
+   adap->dvb_adap.mdev = d->media_dev;
+#endif
+
if (d->props->read_mac_address) {
ret = d->props->read_mac_address(adap,
adap->dvb_adap.proposed_mac);
@@ -464,6 +520,7 @@ err_dvb_net_init:
 err_dvb_dmxdev_init:
dvb_dmx_release(&adap->demux);
 err_dvb_dmx_init:
+   dvb_usbv2_media_device_unregister(d);
dvb_unregister_adapter(&adap->dvb_adap);
 err_dvb_register_adapter:
adap->dvb_adap.priv = NULL;
@@ -472,6 +529,8 @@ err_dvb_register_adapter:
 
 static int dvb_usbv2_adapter_dvb_exit(struct dvb_usb_adapter *adap)
 {
+   struct dvb_usb_device *d = adap_to_d(adap);
+
dev_dbg(&adap_to_d(adap)->udev->dev, "%s: adap=%d\n", __func__,
adap->id);
 
@@ -480,6 +539,7 @@ static int dvb_usbv2_adapter_dvb_exit(struct 
dvb_usb_adapter *adap)
adap->demux.dmx.close(&adap->demux.dmx);
dvb_dmxdev_release(&adap->dmxdev);
dvb_dmx_release(&adap->demux);
+   dvb_usbv2_media_device_unregister(d);
dvb_unregister_adapter(&adap->dvb_adap);
}
 
@@ -954,6 +1014,7 @@ void dvb_usbv2_disconnect(struct usb_interface *intf)
struct dvb_usb_device *d = usb_get_intfdata(intf);
const char *name = d->name;
struct device dev = d->udev->dev;
+
dev_dbg(&d->udev->dev, "%s: bInterfaceNumber=%d\n", __func__,
intf->cur_altsetting->desc.bInterfaceNumber);
 
-- 
2.1.0

--
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] [media] si2165: Fix possible leak in si2165_upload_firmware()

2015-02-11 Thread Antti Palosaari


On 02/12/2015 01:38 AM, Luis de Bethencourt wrote:

On Wed, Feb 11, 2015 at 10:45:01PM +0100, Matthias Schwarzott wrote:

On 11.02.2015 21:58, Christian Engelmayer wrote:

In case of an error function si2165_upload_firmware() releases the already
requested firmware in the exit path. However, there is one deviation where
the function directly returns. Use the correct cleanup so that the firmware
memory gets freed correctly. Detected by Coverity CID 1269120.

Signed-off-by: Christian Engelmayer 
---
Compile tested only. Applies against linux-next.
---
  drivers/media/dvb-frontends/si2165.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/si2165.c 
b/drivers/media/dvb-frontends/si2165.c
index 98ddb49ad52b..4cc5d10ed0d4 100644
--- a/drivers/media/dvb-frontends/si2165.c
+++ b/drivers/media/dvb-frontends/si2165.c
@@ -505,7 +505,7 @@ static int si2165_upload_firmware(struct si2165_state 
*state)
/* reset crc */
ret = si2165_writereg8(state, 0x0379, 0x01);
if (ret)
-   return ret;
+   goto error;

ret = si2165_upload_firmware_block(state, data, len,
   &offset, block_count);


Good catch.

Signed-off-by: Matthias Schwarzott 



Good catch indeed.

Can I sign off? Not sure what the rules are.

Signed-off-by: Luis de Bethencourt 



You cannot sign it unless patch is going through hands. Probably you 
want review it. Check documentation "SubmittingPatches".


https://www.kernel.org/doc/Documentation/SubmittingPatches

regards
Antti

--
http://palosaari.fi/
--
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] [media] si2165: Fix possible leak in si2165_upload_firmware()

2015-02-11 Thread Luis de Bethencourt

On Wed, Feb 11, 2015 at 10:45:01PM +0100, Matthias Schwarzott wrote:
> On 11.02.2015 21:58, Christian Engelmayer wrote:
> > In case of an error function si2165_upload_firmware() releases the already
> > requested firmware in the exit path. However, there is one deviation where
> > the function directly returns. Use the correct cleanup so that the firmware
> > memory gets freed correctly. Detected by Coverity CID 1269120.
> > 
> > Signed-off-by: Christian Engelmayer 
> > ---
> > Compile tested only. Applies against linux-next.
> > ---
> >  drivers/media/dvb-frontends/si2165.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/dvb-frontends/si2165.c 
> > b/drivers/media/dvb-frontends/si2165.c
> > index 98ddb49ad52b..4cc5d10ed0d4 100644
> > --- a/drivers/media/dvb-frontends/si2165.c
> > +++ b/drivers/media/dvb-frontends/si2165.c
> > @@ -505,7 +505,7 @@ static int si2165_upload_firmware(struct si2165_state 
> > *state)
> > /* reset crc */
> > ret = si2165_writereg8(state, 0x0379, 0x01);
> > if (ret)
> > -   return ret;
> > +   goto error;
> >  
> > ret = si2165_upload_firmware_block(state, data, len,
> >&offset, block_count);
> > 
> Good catch.
> 
> Signed-off-by: Matthias Schwarzott 
> 

Good catch indeed.

Can I sign off? Not sure what the rules are.

Signed-off-by: Luis de Bethencourt 
--
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] [media] si2165: Fix possible leak in si2165_upload_firmware()

2015-02-11 Thread Matthias Schwarzott

On 11.02.2015 21:58, Christian Engelmayer wrote:
> In case of an error function si2165_upload_firmware() releases the already
> requested firmware in the exit path. However, there is one deviation where
> the function directly returns. Use the correct cleanup so that the firmware
> memory gets freed correctly. Detected by Coverity CID 1269120.
> 
> Signed-off-by: Christian Engelmayer 
> ---
> Compile tested only. Applies against linux-next.
> ---
>  drivers/media/dvb-frontends/si2165.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/dvb-frontends/si2165.c 
> b/drivers/media/dvb-frontends/si2165.c
> index 98ddb49ad52b..4cc5d10ed0d4 100644
> --- a/drivers/media/dvb-frontends/si2165.c
> +++ b/drivers/media/dvb-frontends/si2165.c
> @@ -505,7 +505,7 @@ static int si2165_upload_firmware(struct si2165_state 
> *state)
>   /* reset crc */
>   ret = si2165_writereg8(state, 0x0379, 0x01);
>   if (ret)
> - return ret;
> + goto error;
>  
>   ret = si2165_upload_firmware_block(state, data, len,
>  &offset, block_count);
> 
Good catch.

Signed-off-by: Matthias Schwarzott 

--
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] si2165: Fix possible leak in si2165_upload_firmware()

2015-02-11 Thread Christian Engelmayer

In case of an error function si2165_upload_firmware() releases the already
requested firmware in the exit path. However, there is one deviation where
the function directly returns. Use the correct cleanup so that the firmware
memory gets freed correctly. Detected by Coverity CID 1269120.

Signed-off-by: Christian Engelmayer 
---
Compile tested only. Applies against linux-next.
---
 drivers/media/dvb-frontends/si2165.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/si2165.c 
b/drivers/media/dvb-frontends/si2165.c
index 98ddb49ad52b..4cc5d10ed0d4 100644
--- a/drivers/media/dvb-frontends/si2165.c
+++ b/drivers/media/dvb-frontends/si2165.c
@@ -505,7 +505,7 @@ static int si2165_upload_firmware(struct si2165_state 
*state)
/* reset crc */
ret = si2165_writereg8(state, 0x0379, 0x01);
if (ret)
-   return ret;
+   goto error;
 
ret = si2165_upload_firmware_block(state, data, len,
   &offset, block_count);
-- 
1.9.1

--
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: docs: Correct NV{12,21}/M pixel formats, chroma samples used.

2015-02-11 Thread Miguel Casas-Sanchez

Docos says for these pixel formats:

start... : Cb00 Cr00 Cb01 Cr01
start... : Cb10 Cr10 Cb11 Cr11

whereas it should read:

start... : Cb00 Cr00 Cb11 Cr11
start... : Cb20 Cr20 Cb21 Cr21

where ... depends on the exact multi/single planar format.

See e.g. http://linuxtv.org/downloads/v4l-dvb-apis/re30.html
and http://linuxtv.org/downloads/v4l-dvb-apis/re31.html


Signed-off-by: Miguel Casas-Sanchez 
---
 Documentation/DocBook/media/v4l/pixfmt-nv12.xml  |  8 
 Documentation/DocBook/media/v4l/pixfmt-nv12m.xml | 12 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/pixfmt-nv12.xml
b/Documentation/DocBook/media/v4l/pixfmt-nv12.xml
index 84dd4fd..4148696 100644
--- a/Documentation/DocBook/media/v4l/pixfmt-nv12.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt-nv12.xml
@@ -73,15 +73,15 @@ pixel image
  start + 16:
  Cb00
  Cr00
- Cb01
- Cr01
+ Cb02
+ Cr02


  start + 20:
  Cb10
  Cr10
- Cb11
- Cr11
+ Cb22
+ Cr22

  

diff --git a/Documentation/DocBook/media/v4l/pixfmt-nv12m.xml
b/Documentation/DocBook/media/v4l/pixfmt-nv12m.xml
index f3a3d45..e0a35ea 100644
--- a/Documentation/DocBook/media/v4l/pixfmt-nv12m.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt-nv12m.xml
@@ -83,15 +83,15 @@ CbCr plane has as many pad bytes after its rows.
  start1 + 0:
  Cb00
  Cr00
- Cb01
- Cr01
+ Cb02
+ Cr02


  start1 + 4:
- Cb10
- Cr10
- Cb11
- Cr11
+ Cb20
+ Cr20
+ Cb22
+ Cr22

  


--
2.2.0.rc0.207.ga3a616c
--
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: vivid test device: Add NV{12,21} and Y{U,V}12 pixel format.

2015-02-11 Thread Miguel Casas-Sanchez

Add support for vertical + horizontal subsampled formats to vivid and use it to
generate YU12, YV12, NV12, NV21 as defined in [1,2]. These formats are tightly
packed N planar, because they provide chroma(s) as a separate array, but they
are not mplanar yet, as discussed in the list.

The modus operandi is to let tpg_fillbuffer() create a YUYV packed format per
pattern line as usual and apply downsampling if needed immediately afterwards,
in a new function called tpg_apply_downsampling(). This one will unpack as
needed, and average the chroma samples (note that luma samples are never
downsampled). (Some provisions for horizontal downsampling are made, so it can
be followed up with e.g. YUV410 etc formats, please understand in this context).
Writing the text information on top of the produced pattern also needs a bit of
a retouch.

[1] http://linuxtv.org/downloads/v4l-dvb-apis/re30.html
[2] http://linuxtv.org/downloads/v4l-dvb-apis/re24.html

Signed-off-by: Miguel Casas-Sanchez 
---
 drivers/media/platform/vivid/vivid-kthread-cap.c |   6 +-
 drivers/media/platform/vivid/vivid-tpg.c | 232 +++
 drivers/media/platform/vivid/vivid-tpg.h |  15 +-
 drivers/media/platform/vivid/vivid-vid-common.c  |  28 +++
 4 files changed, 238 insertions(+), 43 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c
b/drivers/media/platform/vivid/vivid-kthread-cap.c
index 39a67cf..93c6ca3 100644
--- a/drivers/media/platform/vivid/vivid-kthread-cap.c
+++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
@@ -669,8 +669,7 @@ static void vivid_thread_vid_cap_tick(struct
vivid_dev *dev, int dropped_bufs)
if (vid_cap_buf) {
/* Fill buffer */
vivid_fillbuff(dev, vid_cap_buf);
-   dprintk(dev, 1, "filled buffer %d\n",
-   vid_cap_buf->vb.v4l2_buf.index);
+   dprintk(dev, 1, "filled buffer %d\n",
vid_cap_buf->vb.v4l2_buf.index);

/* Handle overlay */
if (dev->overlay_cap_owner && dev->fb_cap.base &&
@@ -679,8 +678,7 @@ static void vivid_thread_vid_cap_tick(struct
vivid_dev *dev, int dropped_bufs)

vb2_buffer_done(&vid_cap_buf->vb, dev->dqbuf_error ?
VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
-   dprintk(dev, 2, "vid_cap buffer %d done\n",
-   vid_cap_buf->vb.v4l2_buf.index);
+   dprintk(dev, 2, "vid_cap buffer %d done\n",
vid_cap_buf->vb.v4l2_buf.index);
}

if (vbi_cap_buf) {
diff --git a/drivers/media/platform/vivid/vivid-tpg.c
b/drivers/media/platform/vivid/vivid-tpg.c
index 34493f4..d72e19a 100644
--- a/drivers/media/platform/vivid/vivid-tpg.c
+++ b/drivers/media/platform/vivid/vivid-tpg.c
@@ -193,6 +193,10 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_UYVY:
case V4L2_PIX_FMT_YVYU:
case V4L2_PIX_FMT_VYUY:
+   case V4L2_PIX_FMT_NV12:
+   case V4L2_PIX_FMT_NV21:
+   case V4L2_PIX_FMT_YUV420:
+   case V4L2_PIX_FMT_YVU420:
tpg->is_yuv = true;
break;
default:
@@ -224,12 +228,32 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_ABGR32:
tpg->twopixelsize[0] = 2 * 4;
break;
+   case V4L2_PIX_FMT_NV12:
+   case V4L2_PIX_FMT_NV21:
+   case V4L2_PIX_FMT_YUV420:
+   case V4L2_PIX_FMT_YVU420:
+   tpg->twopixelsize[0] = 3;
+   break;
case V4L2_PIX_FMT_NV16M:
case V4L2_PIX_FMT_NV61M:
tpg->twopixelsize[0] = 2;
tpg->twopixelsize[1] = 2;
break;
}
+
+   switch (fourcc) {
+   case V4L2_PIX_FMT_NV12:
+   case V4L2_PIX_FMT_NV21:
+   case V4L2_PIX_FMT_YUV420:
+   case V4L2_PIX_FMT_YVU420:
+   tpg->vertical_downsampling = 2;
+   tpg->horizontal_downsampling = 2;
+   break;
+   default:
+   tpg->vertical_downsampling = 0;
+   tpg->horizontal_downsampling = 0;
+   }
+
return true;
 }

@@ -271,6 +295,12 @@ void tpg_reset_source(struct tpg_data *tpg,
unsigned width, unsigned height,
tpg->recalc_square_border = true;
 }

+/* Vertically downsampled pixel formats use YUYV as intermediate. */
+static unsigned tpg_get_packed_twopixsize(struct tpg_data *tpg, unsigned p)
+{
+   return tpg->vertical_downsampling ? 4 : tpg->twopixelsize[p];
+}
+
 static enum tpg_color tpg_get_textbg_color(struct tpg_data *tpg)
 {
switch (tpg->pattern) {
@@ -673,7 +703,15 @@ static void gen_twopix(struct tpg_data *tpg,
buf[0][offset] = r_y;
buf[1][offset] = odd ? g_u : b_v;
break;
-
+   /*
+* For these cases we compose a YUYV macropixel. They will be
verticallly
+* downsampled later on.
+*/
+   case V4L2_PIX_FMT_NV12:
+   case V4L2_P

Re: [PATCH] media: Pinnacle 73e infrared control stopped working since kernel 3.17

2015-02-11 Thread David Cimbůrek

Let me know what exactly do you want me to do (which commands, which
traces etc.). I'm not very familiar with the Linux media stuff...

2015-02-11 15:40 GMT+01:00 David Härdeman :
> Can you generate some scancodes before and after commit
> af3a4a9bbeb00df3e42e77240b4cdac5479812f9?
>
>
>
> On 2015-02-11 14:53, David Cimbůrek wrote:
>>
>> David Härdeman: I'm using defaults, I have no custom modifications.
>>
>>
>> 2015-02-11 14:24 GMT+01:00 David Härdeman :
>>>
>>> David C: are you using the in-kernel keymap or loading a custom one?
>>>
>>>
>>> On 2015-02-10 11:45, Antti Palosaari wrote:


 David Härdeman,
 Could you look that as it is your patch which has broken it

 commit af3a4a9bbeb00df3e42e77240b4cdac5479812f9
 Author: David Härdeman 
 Date:   Thu Apr 3 20:31:51 2014 -0300

 [media] dib0700: NEC scancode cleanup


 Antti

 On 02/10/2015 12:38 PM, David Cimbůrek wrote:
>
>
> Please include this patch to kernel! It takes too much time for such a
> simple fix!
>
>
> 2015-01-07 13:51 GMT+01:00 David Cimbůrek :
>>
>>
>> No one is interested? I'd like to get this patch to kernel to fix the
>> issue. Can someone here do it please?
>>
>>
>> 2014-12-20 14:36 GMT+01:00 David Cimbůrek :
>>>
>>>
>>> Hi,
>>>
>>> with kernel 3.17 remote control for Pinnacle 73e (ID 2304:0237
>>> Pinnacle Systems, Inc. PCTV 73e [DiBcom DiB7000PC]) does not work
>>> anymore.
>>>
>>> I checked the changes and found out the problem in commit
>>> af3a4a9bbeb00df3e42e77240b4cdac5479812f9.
>>>
>>> In dib0700_core.c in struct dib0700_rc_response the following union:
>>>
>>> union {
>>>  u16 system16;
>>>  struct {
>>>  u8 not_system;
>>>  u8 system;
>>>  };
>>> };
>>>
>>> has been replaced by simple variables:
>>>
>>> u8 system;
>>> u8 not_system;
>>>
>>> But these variables are in reverse order! When I switch the order
>>> back, the remote works fine again! Here is the patch:
>>>
>>>
>>> --- a/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
>>> 14:27:15.0 +0100
>>> +++ b/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
>>> 14:27:36.0 +0100
>>> @@ -658,8 +658,8 @@
>>>   struct dib0700_rc_response {
>>>   u8 report_id;
>>>   u8 data_state;
>>> -u8 system;
>>>   u8 not_system;
>>> +u8 system;
>>>   u8 data;
>>>   u8 not_data;
>>>   };
>>>
>>>
>>> Regards,
>>> David
>
>
> --
> 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
>
>>>
>
--
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: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Russell King - ARM Linux

On Wed, Feb 11, 2015 at 01:20:24PM +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-02-11 12:12, Russell King - ARM Linux wrote:
> >Which is a damn good reason to NAK it - by that admission, it's a half-baked
> >idea.
> >
> >If all we want to know is whether the importer can accept only contiguous
> >memory or not, make a flag to do that, and allow the exporter to test this
> >flag.  Don't over-engineer this to make it _seem_ like it can do something
> >that it actually totally fails with.
> >
> >As I've already pointed out, there's a major problem if you have already
> >had a less restrictive attachment which has an active mapping, and a new
> >more restrictive attachment comes along later.
> >
> >It seems from Rob's descriptions that we also need another flag in the
> >importer to indicate whether it wants to have a valid struct page in the
> >scatter list, or whether it (correctly) uses the DMA accessors on the
> >scatter list - so that exporters can reject importers which are buggy.
> 
> Okay, but flag-based approach also have limitations.

Yes, the flag-based approach doesn't let you describe in detail what
the importer can accept - which, given the issues that I've raised
is a *good* thing.  We won't be misleading anyone into thinking that
we can do something that's really half-baked, and which we have no
present requirement for.

This is precisely what Linus talks about when he says "don't over-
engineer" - if we over-engineer this, we end up with something that
sort-of works, and that's a bad thing.

The Keep It Simple approach here makes total sense - what are our
current requirements - to be able to say that an importer can only accept:
  - contiguous memory rather than a scatterlist
  - scatterlists with struct page pointers

Does solving that need us to compare all the constraints of each and
every importer, possibly ending up with constraints which can't be
satisfied?  No.  Does the flag approach satisfy the requirements?  Yes.

> Frankly, if we want to make it really portable and sharable between devices,
> then IMO we should get rid of struct scatterlist and replace it with simple
> array of pfns in dma_buf. This way at least the problem of missing struct
> page will be solved and the buffer representation will be also a bit more
> compact.

... and move the mapping and unmapping of the PFNs to the importer,
which IMHO is where it should already be (so the importer can decide
when it should map the buffer itself independently of getting the
details of the buffer.)

> Such solution however also requires extending dma-mapping API to handle
> mapping and unmapping of such pfn arrays. The advantage of this approach
> is the fact that it will be completely new API, so it can be designed
> well from the beginning.

As far as I'm aware, there was no big discussion of the dma_buf API -
it's something that just appeared one day (I don't remember seeing it
discussed.)  So, that may well be a good thing if it means we can get
these kinds of details better hammered out.

However, I don't share your view of "completely new API" - that would
be far too disruptive.  I think we can modify the existing API, to
achieve our goals.

I don't think changing the dma-mapping API just for this case is really
on though - if we're passed a list of PFNs, they either must be all
associated with a struct page - iow, pfn_valid(pfn) returns true for
all of them or none of them.  If none of them, then we need to be able
to convert those PFNs to a dma_addr_t for the target device (yes, that
may need the dma-mapping API augmenting.)

However, if they are associated with a struct page, then we should use
the established APIs and use a scatterlist, otherwise we're looking
at rewriting all IOMMUs and all DMA mapping implementations to handle
what would become a special case for dma_buf.

I'd rather... Keep It Simple.

So, maybe, as a first step, let's augment dma_buf with a pair of
functions which get the "raw" unmapped scatterlist:

struct sg_table *dma_buf_get_scatterlist(struct dma_buf_attachment *attach)
{
struct sg_table *sg_table;

might_sleep();

if (!attach->dmabuf->ops->get_scatterlist)
return ERR_PTR(-EINVAL);

sg_table = attach->dmabuf->ops->get_scatterlist(attach);
if (!sg_table)
sg_table = ERR_PTR(-ENOMEM);

return sg_table;
}

void dma_buf_put_scatterlist(struct dma_buf_attachment *attach,
 struct sg_table *sg_table)
{
might_sleep();

attach->dmabuf->ops->put_scatterlist(attach, sg_table);
}

Implementations should arrange for dma_buf_get_scatterlist() to return
the EINVAL error pointer if they are unable to provide an unmapped
scatterlist (in other words, they are exporting a set of PFNs or
already-mapped dma_addr_t's.)  This can be done by either not
implementing the get_scatterlist method, or by implementing it and
returning that forementioned error pointer value.

Where these two are i

Re: [PATCH] media: Pinnacle 73e infrared control stopped working since kernel 3.17

2015-02-11 Thread David Härdeman

Can you generate some scancodes before and after commit 
af3a4a9bbeb00df3e42e77240b4cdac5479812f9?



On 2015-02-11 14:53, David Cimbůrek wrote:

David Härdeman: I'm using defaults, I have no custom modifications.


2015-02-11 14:24 GMT+01:00 David Härdeman :

David C: are you using the in-kernel keymap or loading a custom one?


On 2015-02-10 11:45, Antti Palosaari wrote:


David Härdeman,
Could you look that as it is your patch which has broken it

commit af3a4a9bbeb00df3e42e77240b4cdac5479812f9
Author: David Härdeman 
Date:   Thu Apr 3 20:31:51 2014 -0300

[media] dib0700: NEC scancode cleanup


Antti

On 02/10/2015 12:38 PM, David Cimbůrek wrote:


Please include this patch to kernel! It takes too much time for such 
a

simple fix!


2015-01-07 13:51 GMT+01:00 David Cimbůrek 
:


No one is interested? I'd like to get this patch to kernel to fix 
the

issue. Can someone here do it please?


2014-12-20 14:36 GMT+01:00 David Cimbůrek 
:


Hi,

with kernel 3.17 remote control for Pinnacle 73e (ID 2304:0237
Pinnacle Systems, Inc. PCTV 73e [DiBcom DiB7000PC]) does not work
anymore.

I checked the changes and found out the problem in commit
af3a4a9bbeb00df3e42e77240b4cdac5479812f9.

In dib0700_core.c in struct dib0700_rc_response the following 
union:


union {
 u16 system16;
 struct {
 u8 not_system;
 u8 system;
 };
};

has been replaced by simple variables:

u8 system;
u8 not_system;

But these variables are in reverse order! When I switch the order
back, the remote works fine again! Here is the patch:


--- a/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
14:27:15.0 +0100
+++ b/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
14:27:36.0 +0100
@@ -658,8 +658,8 @@
  struct dib0700_rc_response {
  u8 report_id;
  u8 data_state;
-u8 system;
  u8 not_system;
+u8 system;
  u8 data;
  u8 not_data;
  };


Regards,
David


--
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




--
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] [media] si470x: fixup wait_for_completion_timeout return handling

2015-02-11 Thread Nicholas Mc Guire

return type of wait_for_completion_timeout is unsigned long not int. A
appropriately named variable of type unsigned long is added and the
assignments fixed up.

Signed-off-by: Nicholas Mc Guire 
---

v2: added config used for compile testing.

Patch was only compile tested with x86_64_defconfig + CONFIG_USB=m,
CONFIG_MEDIA_SUPPORT=m, CONFIG_MEDIA_ANALOG_TV_SUPPORT=y
CONFIG_MEDIA_DIGITAL_TV_SUPPORT=y, (implies VIDEO_V4L2=m)
CONFIG_MEDIA_RADIO_SUPPORT=y, CONFIG_RADIO_SI470X=y
CONFIG_USB_SI470X=m

Patch is against 3.19.0 (localversion-next is -next-20150211)

 drivers/media/radio/si470x/radio-si470x-common.c |   14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/radio/si470x/radio-si470x-common.c 
b/drivers/media/radio/si470x/radio-si470x-common.c
index 909c3f9..1d827ad 100644
--- a/drivers/media/radio/si470x/radio-si470x-common.c
+++ b/drivers/media/radio/si470x/radio-si470x-common.c
@@ -208,6 +208,7 @@ static int si470x_set_band(struct si470x_device *radio, int 
band)
 static int si470x_set_chan(struct si470x_device *radio, unsigned short chan)
 {
int retval;
+   unsigned long time_left;
bool timed_out = false;
 
/* start tuning */
@@ -219,9 +220,9 @@ static int si470x_set_chan(struct si470x_device *radio, 
unsigned short chan)
 
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
-   retval = wait_for_completion_timeout(&radio->completion,
-   msecs_to_jiffies(tune_timeout));
-   if (!retval)
+   time_left = wait_for_completion_timeout(&radio->completion,
+   msecs_to_jiffies(tune_timeout));
+   if (time_left == 0)
timed_out = true;
 
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
@@ -301,6 +302,7 @@ static int si470x_set_seek(struct si470x_device *radio,
int band, retval;
unsigned int freq;
bool timed_out = false;
+   unsigned long time_left;
 
/* set band */
if (seek->rangelow || seek->rangehigh) {
@@ -342,9 +344,9 @@ static int si470x_set_seek(struct si470x_device *radio,
 
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
-   retval = wait_for_completion_timeout(&radio->completion,
-   msecs_to_jiffies(seek_timeout));
-   if (!retval)
+   time_left = wait_for_completion_timeout(&radio->completion,
+   msecs_to_jiffies(seek_timeout));
+   if (time_left == 0)
timed_out = true;
 
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
-- 
1.7.10.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] si470x: fixup wait_for_completion_timeout return handling

2015-02-11 Thread Nicholas Mc Guire

return type of wait_for_completion_timeout is unsigned long not int. A
appropriately named variable of type unsigned long is added and the
assignments fixed up.

Signed-off-by: Nicholas Mc Guire 
---

Patch was only compile tested with 

Patch is against 3.19.0 (localversion-next is -next-20150211)

 drivers/media/radio/si470x/radio-si470x-common.c |   14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/radio/si470x/radio-si470x-common.c 
b/drivers/media/radio/si470x/radio-si470x-common.c
index 909c3f9..1d827ad 100644
--- a/drivers/media/radio/si470x/radio-si470x-common.c
+++ b/drivers/media/radio/si470x/radio-si470x-common.c
@@ -208,6 +208,7 @@ static int si470x_set_band(struct si470x_device *radio, int 
band)
 static int si470x_set_chan(struct si470x_device *radio, unsigned short chan)
 {
int retval;
+   unsigned long time_left;
bool timed_out = false;
 
/* start tuning */
@@ -219,9 +220,9 @@ static int si470x_set_chan(struct si470x_device *radio, 
unsigned short chan)
 
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
-   retval = wait_for_completion_timeout(&radio->completion,
-   msecs_to_jiffies(tune_timeout));
-   if (!retval)
+   time_left = wait_for_completion_timeout(&radio->completion,
+   msecs_to_jiffies(tune_timeout));
+   if (time_left == 0)
timed_out = true;
 
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
@@ -301,6 +302,7 @@ static int si470x_set_seek(struct si470x_device *radio,
int band, retval;
unsigned int freq;
bool timed_out = false;
+   unsigned long time_left;
 
/* set band */
if (seek->rangelow || seek->rangehigh) {
@@ -342,9 +344,9 @@ static int si470x_set_seek(struct si470x_device *radio,
 
/* wait till tune operation has completed */
reinit_completion(&radio->completion);
-   retval = wait_for_completion_timeout(&radio->completion,
-   msecs_to_jiffies(seek_timeout));
-   if (!retval)
+   time_left = wait_for_completion_timeout(&radio->completion,
+   msecs_to_jiffies(seek_timeout));
+   if (time_left == 0)
timed_out = true;
 
if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
-- 
1.7.10.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] media: Pinnacle 73e infrared control stopped working since kernel 3.17

2015-02-11 Thread David Cimbůrek

David Härdeman: I'm using defaults, I have no custom modifications.


2015-02-11 14:24 GMT+01:00 David Härdeman :
> David C: are you using the in-kernel keymap or loading a custom one?
>
>
> On 2015-02-10 11:45, Antti Palosaari wrote:
>>
>> David Härdeman,
>> Could you look that as it is your patch which has broken it
>>
>> commit af3a4a9bbeb00df3e42e77240b4cdac5479812f9
>> Author: David Härdeman 
>> Date:   Thu Apr 3 20:31:51 2014 -0300
>>
>> [media] dib0700: NEC scancode cleanup
>>
>>
>> Antti
>>
>> On 02/10/2015 12:38 PM, David Cimbůrek wrote:
>>>
>>> Please include this patch to kernel! It takes too much time for such a
>>> simple fix!
>>>
>>>
>>> 2015-01-07 13:51 GMT+01:00 David Cimbůrek :

 No one is interested? I'd like to get this patch to kernel to fix the
 issue. Can someone here do it please?


 2014-12-20 14:36 GMT+01:00 David Cimbůrek :
>
> Hi,
>
> with kernel 3.17 remote control for Pinnacle 73e (ID 2304:0237
> Pinnacle Systems, Inc. PCTV 73e [DiBcom DiB7000PC]) does not work
> anymore.
>
> I checked the changes and found out the problem in commit
> af3a4a9bbeb00df3e42e77240b4cdac5479812f9.
>
> In dib0700_core.c in struct dib0700_rc_response the following union:
>
> union {
>  u16 system16;
>  struct {
>  u8 not_system;
>  u8 system;
>  };
> };
>
> has been replaced by simple variables:
>
> u8 system;
> u8 not_system;
>
> But these variables are in reverse order! When I switch the order
> back, the remote works fine again! Here is the patch:
>
>
> --- a/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
> 14:27:15.0 +0100
> +++ b/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
> 14:27:36.0 +0100
> @@ -658,8 +658,8 @@
>   struct dib0700_rc_response {
>   u8 report_id;
>   u8 data_state;
> -u8 system;
>   u8 not_system;
> +u8 system;
>   u8 data;
>   u8 not_data;
>   };
>
>
> Regards,
> David
>>>
>>> --
>>> 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
>>>
>
--
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: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Rob Clark

On Wed, Feb 11, 2015 at 7:56 AM, Daniel Vetter  wrote:
> On Wed, Feb 11, 2015 at 06:23:52AM -0500, Rob Clark wrote:
>> On Wed, Feb 11, 2015 at 6:12 AM, Russell King - ARM Linux
>>  wrote:
>> > As I've already pointed out, there's a major problem if you have already
>> > had a less restrictive attachment which has an active mapping, and a new
>> > more restrictive attachment comes along later.
>> >
>> > It seems from Rob's descriptions that we also need another flag in the
>> > importer to indicate whether it wants to have a valid struct page in the
>> > scatter list, or whether it (correctly) uses the DMA accessors on the
>> > scatter list - so that exporters can reject importers which are buggy.
>>
>> to be completely generic, we would really need a way that the device
>> could take over only just the last iommu (in case there were multiple
>> levels of address translation)..
>
> I still hold that if the dma api steals the iommu your gpu needs for
> context switching then that's a bug in the platform setup code. dma api
> really doesn't have any concept of switchable hw contexts. So trying to
> work around this brokeness by mandating it as a valid dma-buf use-case is
> totally backwards.

sure, my only point is that if I'm the odd man out, I can live with a
hack (ie. requiring drm/msm to be aware enough of the platform to know
if there is >1 level of address translation and frob'ing my 'struct
device' accordingly)... no point in a generic solution for one user.
I like to be practical.

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
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] media: Pinnacle 73e infrared control stopped working since kernel 3.17

2015-02-11 Thread David Härdeman


David C: are you using the in-kernel keymap or loading a custom one?

On 2015-02-10 11:45, Antti Palosaari wrote:

David Härdeman,
Could you look that as it is your patch which has broken it

commit af3a4a9bbeb00df3e42e77240b4cdac5479812f9
Author: David Härdeman 
Date:   Thu Apr 3 20:31:51 2014 -0300

[media] dib0700: NEC scancode cleanup


Antti

On 02/10/2015 12:38 PM, David Cimbůrek wrote:

Please include this patch to kernel! It takes too much time for such a
simple fix!


2015-01-07 13:51 GMT+01:00 David Cimbůrek :

No one is interested? I'd like to get this patch to kernel to fix the
issue. Can someone here do it please?


2014-12-20 14:36 GMT+01:00 David Cimbůrek :

Hi,

with kernel 3.17 remote control for Pinnacle 73e (ID 2304:0237
Pinnacle Systems, Inc. PCTV 73e [DiBcom DiB7000PC]) does not work
anymore.

I checked the changes and found out the problem in commit
af3a4a9bbeb00df3e42e77240b4cdac5479812f9.

In dib0700_core.c in struct dib0700_rc_response the following union:

union {
 u16 system16;
 struct {
 u8 not_system;
 u8 system;
 };
};

has been replaced by simple variables:

u8 system;
u8 not_system;

But these variables are in reverse order! When I switch the order
back, the remote works fine again! Here is the patch:


--- a/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
14:27:15.0 +0100
+++ b/drivers/media/usb/dvb-usr/dib0700_core.c2014-12-20
14:27:36.0 +0100
@@ -658,8 +658,8 @@
  struct dib0700_rc_response {
  u8 report_id;
  u8 data_state;
-u8 system;
  u8 not_system;
+u8 system;
  u8 data;
  u8 not_data;
  };


Regards,
David

--
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


--
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] rtl2832: remove compiler warning

2015-02-11 Thread Antti Palosaari


On 02/11/2015 01:08 PM, Luis de Bethencourt wrote:

Cleaning up the following compiler warning:
rtl2832.c:703:12: warning: 'tmp' may be used uninitialized in this function

Even though it could never happen since if rtl2832_rd_demod_reg () doesn't set
tmp, this line would never run because we go to err. It is still nice to avoid
compiler warnings.

Signed-off-by: Luis de Bethencourt 



Reviewed-by: Antti Palosaari 

Antti


---
  drivers/media/dvb-frontends/rtl2832.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/rtl2832.c 
b/drivers/media/dvb-frontends/rtl2832.c
index 5d2d8f4..20fa245 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -685,7 +685,7 @@ static int rtl2832_read_status(struct dvb_frontend *fe, 
fe_status_t *status)
struct rtl2832_dev *dev = fe->demodulator_priv;
struct i2c_client *client = dev->client;
int ret;
-   u32 tmp;
+   u32 uninitialized_var(tmp);

dev_dbg(&client->dev, "\n");




--
http://palosaari.fi/
--
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: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Daniel Vetter

On Wed, Feb 11, 2015 at 06:23:52AM -0500, Rob Clark wrote:
> On Wed, Feb 11, 2015 at 6:12 AM, Russell King - ARM Linux
>  wrote:
> > As I've already pointed out, there's a major problem if you have already
> > had a less restrictive attachment which has an active mapping, and a new
> > more restrictive attachment comes along later.
> >
> > It seems from Rob's descriptions that we also need another flag in the
> > importer to indicate whether it wants to have a valid struct page in the
> > scatter list, or whether it (correctly) uses the DMA accessors on the
> > scatter list - so that exporters can reject importers which are buggy.
> 
> to be completely generic, we would really need a way that the device
> could take over only just the last iommu (in case there were multiple
> levels of address translation)..

I still hold that if the dma api steals the iommu your gpu needs for
context switching then that's a bug in the platform setup code. dma api
really doesn't have any concept of switchable hw contexts. So trying to
work around this brokeness by mandating it as a valid dma-buf use-case is
totally backwards.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
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: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-11 12:12, Russell King - ARM Linux wrote:

On Wed, Feb 11, 2015 at 09:28:37AM +0100, Marek Szyprowski wrote:

On 2015-01-27 09:25, Sumit Semwal wrote:

Add some helpers to share the constraints of devices while attaching
to the dmabuf buffer.

At each attach, the constraints are calculated based on the following:
- max_segment_size, max_segment_count, segment_boundary_mask from
device_dma_parameters.

In case the attaching device's constraints don't match up, attach() fails.

At detach, the constraints are recalculated based on the remaining
attached devices.

Two helpers are added:
- dma_buf_get_constraints - which gives the current constraints as calculated
   during each attach on the buffer till the time,
- dma_buf_recalc_constraints - which recalculates the constraints for all
   currently attached devices for the 'paranoid' ones amongst us.

The idea of this patch is largely taken from Rob Clark's RFC at
https://lkml.org/lkml/2012/7/19/285, and the comments received on it.

Cc: Rob Clark 
Signed-off-by: Sumit Semwal 

The code looks okay, although it will probably will work well only with
typical cases like 'contiguous memory needed' or 'no constraints at all'
(iommu).

Which is a damn good reason to NAK it - by that admission, it's a half-baked
idea.

If all we want to know is whether the importer can accept only contiguous
memory or not, make a flag to do that, and allow the exporter to test this
flag.  Don't over-engineer this to make it _seem_ like it can do something
that it actually totally fails with.

As I've already pointed out, there's a major problem if you have already
had a less restrictive attachment which has an active mapping, and a new
more restrictive attachment comes along later.

It seems from Rob's descriptions that we also need another flag in the
importer to indicate whether it wants to have a valid struct page in the
scatter list, or whether it (correctly) uses the DMA accessors on the
scatter list - so that exporters can reject importers which are buggy.


Okay, but flag-based approach also have limitations.

Frankly, if we want to make it really portable and sharable between devices,
then IMO we should get rid of struct scatterlist and replace it with simple
array of pfns in dma_buf. This way at least the problem of missing struct
page will be solved and the buffer representation will be also a bit more
compact.

Such solution however also requires extending dma-mapping API to handle
mapping and unmapping of such pfn arrays. The advantage of this approach
is the fact that it will be completely new API, so it can be designed
well from the beginning.

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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][RFC] add raw video stream support for Samsung SUR40

2015-02-11 Thread Florian Echtler

Hello again,

does anyone have any suggestions why USERPTR still fails with dma-sg?

Could I just disable the corresponding capability for the moment so that
the patch could perhaps be merged, and investigate this separately?

Best, Florian

On 04.02.2015 16:30, Florian Echtler wrote:
> This patch adds raw video support for the Samsung SUR40, now finally using
> videobuf2-dma-sg and the usb_sg_init/_wait helper functions. Further comments
> regarding buffer handling are invited, as v4l2-compliance -s still fails the
> USERPTR test.
> 
> Signed-off-by: Florian Echtler 
> ---
>  drivers/input/touchscreen/sur40.c | 424 
> --
>  1 file changed, 412 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sur40.c 
> b/drivers/input/touchscreen/sur40.c
> index f1cb051..33bc1b8 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -1,7 +1,7 @@
>  /*
>   * Surface2.0/SUR40/PixelSense input driver
>   *
> - * Copyright (c) 2013 by Florian 'floe' Echtler 
> + * Copyright (c) 2014 by Florian 'floe' Echtler 
>   *
>   * Derived from the USB Skeleton driver 1.1,
>   * Copyright (c) 2003 Greg Kroah-Hartman (g...@kroah.com)
> @@ -12,6 +12,9 @@
>   * and from the generic hid-multitouch driver,
>   * Copyright (c) 2010-2012 Stephane Chatty 
>   *
> + * and from the v4l2-pci-skeleton driver,
> + * Copyright (c) Copyright 2014 Cisco Systems, Inc.
> + *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License as
>   * published by the Free Software Foundation; either version 2 of
> @@ -31,6 +34,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  /* read 512 bytes from endpoint 0x86 -> get header + blobs */
>  struct sur40_header {
> @@ -82,9 +90,19 @@ struct sur40_data {
>   struct sur40_blob   blobs[];
>  } __packed;
>  
> +/* read 512 bytes from endpoint 0x82 -> get header below
> + * continue reading 16k blocks until header.size bytes read */
> +struct sur40_image_header {
> + __le32 magic; /* "SUBF" */
> + __le32 packet_id;
> + __le32 size;  /* always 0x0007e900 = 960x540 */
> + __le32 timestamp; /* milliseconds (increases by 16 or 17 each frame) */
> + __le32 unknown;   /* "epoch?" always 02/03 00 00 00 */
> +} __packed;
>  
>  /* version information */
>  #define DRIVER_SHORT   "sur40"
> +#define DRIVER_LONG"Samsung SUR40"
>  #define DRIVER_AUTHOR  "Florian 'floe' Echtler "
>  #define DRIVER_DESC"Surface2.0/SUR40/PixelSense input driver"
>  
> @@ -99,6 +117,13 @@ struct sur40_data {
>  /* touch data endpoint */
>  #define TOUCH_ENDPOINT 0x86
>  
> +/* video data endpoint */
> +#define VIDEO_ENDPOINT 0x82
> +
> +/* video header fields */
> +#define VIDEO_HEADER_MAGIC 0x46425553
> +#define VIDEO_PACKET_SIZE  16384
> +
>  /* polling interval (ms) */
>  #define POLL_INTERVAL 10
>  
> @@ -113,21 +138,23 @@ struct sur40_data {
>  #define SUR40_GET_STATE   0xc5 /*  4 bytes state (?) */
>  #define SUR40_GET_SENSORS 0xb1 /*  8 bytes sensors   */
>  
> -/*
> - * Note: an earlier, non-public version of this driver used 
> USB_RECIP_ENDPOINT
> - * here by mistake which is very likely to have corrupted the firmware EEPROM
> - * on two separate SUR40 devices. Thanks to Alan Stern who spotted this bug.
> - * Should you ever run into a similar problem, the background story to this
> - * incident and instructions on how to fix the corrupted EEPROM are available
> - * at https://floe.butterbrot.org/matrix/hacking/surface/brick.html
> -*/
> -
> +/* master device state */
>  struct sur40_state {
>  
>   struct usb_device *usbdev;
>   struct device *dev;
>   struct input_polled_dev *input;
>  
> + struct v4l2_device v4l2;
> + struct video_device vdev;
> + struct mutex lock;
> +
> + struct vb2_queue queue;
> + struct vb2_alloc_ctx *alloc_ctx;
> + struct list_head buf_list;
> + spinlock_t qlock;
> + int sequence;
> +
>   struct sur40_data *bulk_in_buffer;
>   size_t bulk_in_size;
>   u8 bulk_in_epaddr;
> @@ -135,6 +162,27 @@ struct sur40_state {
>   char phys[64];
>  };
>  
> +struct sur40_buffer {
> + struct vb2_buffer vb;
> + struct list_head list;
> +};
> +
> +/* forward declarations */
> +static const struct video_device sur40_video_device;
> +static const struct v4l2_pix_format sur40_video_format;
> +static const struct vb2_queue sur40_queue;
> +static void sur40_process_video(struct sur40_state *sur40);
> +
> +/*
> + * Note: an earlier, non-public version of this driver used 
> USB_RECIP_ENDPOINT
> + * here by mistake which is very likely to have corrupted the firmware EEPROM
> + * on two separate SUR40 devices. Thanks to Alan Stern who spotted this bug.
> + * Should you ever run into a similar problem, the background story to this
> + * incident and instructions on how to fix the corrupted EEPROM

Re: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Rob Clark

On Wed, Feb 11, 2015 at 6:12 AM, Russell King - ARM Linux
 wrote:
> On Wed, Feb 11, 2015 at 09:28:37AM +0100, Marek Szyprowski wrote:
>> Hello,
>>
>> On 2015-01-27 09:25, Sumit Semwal wrote:
>> >Add some helpers to share the constraints of devices while attaching
>> >to the dmabuf buffer.
>> >
>> >At each attach, the constraints are calculated based on the following:
>> >- max_segment_size, max_segment_count, segment_boundary_mask from
>> >device_dma_parameters.
>> >
>> >In case the attaching device's constraints don't match up, attach() fails.
>> >
>> >At detach, the constraints are recalculated based on the remaining
>> >attached devices.
>> >
>> >Two helpers are added:
>> >- dma_buf_get_constraints - which gives the current constraints as 
>> >calculated
>> >   during each attach on the buffer till the time,
>> >- dma_buf_recalc_constraints - which recalculates the constraints for all
>> >   currently attached devices for the 'paranoid' ones amongst us.
>> >
>> >The idea of this patch is largely taken from Rob Clark's RFC at
>> >https://lkml.org/lkml/2012/7/19/285, and the comments received on it.
>> >
>> >Cc: Rob Clark 
>> >Signed-off-by: Sumit Semwal 
>>
>> The code looks okay, although it will probably will work well only with
>> typical cases like 'contiguous memory needed' or 'no constraints at all'
>> (iommu).
>
> Which is a damn good reason to NAK it - by that admission, it's a half-baked
> idea.
>
> If all we want to know is whether the importer can accept only contiguous
> memory or not, make a flag to do that, and allow the exporter to test this
> flag.  Don't over-engineer this to make it _seem_ like it can do something
> that it actually totally fails with.

jfyi, I agree with that.. I think the flag is probably the right
approach to start with.  At the end of the day it *is* still just an
in-kernel API (and not something that ends up as userspace ABI) so
when we come up with the use case to make it more generic we can.  Vs.
making it look like something more generic when it isn't really yet.

> As I've already pointed out, there's a major problem if you have already
> had a less restrictive attachment which has an active mapping, and a new
> more restrictive attachment comes along later.
>
> It seems from Rob's descriptions that we also need another flag in the
> importer to indicate whether it wants to have a valid struct page in the
> scatter list, or whether it (correctly) uses the DMA accessors on the
> scatter list - so that exporters can reject importers which are buggy.

to be completely generic, we would really need a way that the device
could take over only just the last iommu (in case there were multiple
levels of address translation)..

I'm not completely sure, but I *think* the other arm gpu's have their
own internal mmu for doing context switching, etc, so if there is an
additional iommu in front of them they may actually still want to use
the normal dma api's.  Someone please contradict me if I am wrong.  If
this ends up being an issue only for msm, then I'm completely ok with
the easier option of a less generic solution..

BR,
-R

>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
--
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] rtl2832: remove compiler warning

2015-02-11 Thread Luis de Bethencourt

On Tue, Feb 10, 2015 at 09:35:52PM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 10 Feb 2015 12:57:24 +0200
> Antti Palosaari  escreveu:
> 
> > On 02/09/2015 12:44 AM, Luis de Bethencourt wrote:
> > > Cleaning the following compiler warning:
> > > rtl2832.c:703:12: warning: 'tmp' may be used uninitialized in this 
> > > function
> > >
> > > Even though it could never happen since if rtl2832_rd_demod_reg () 
> > > doesn't set
> > > tmp, this line would never run because we go to err. It is still nice to 
> > > avoid
> > > compiler warnings.
> > >
> > > Signed-off-by: Luis de Bethencourt 
> > > ---
> > >   drivers/media/dvb-frontends/rtl2832.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/dvb-frontends/rtl2832.c 
> > > b/drivers/media/dvb-frontends/rtl2832.c
> > > index 5d2d8f4..ad36d1c 100644
> > > --- a/drivers/media/dvb-frontends/rtl2832.c
> > > +++ b/drivers/media/dvb-frontends/rtl2832.c
> > > @@ -685,7 +685,7 @@ static int rtl2832_read_status(struct dvb_frontend 
> > > *fe, fe_status_t *status)
> > >   struct rtl2832_dev *dev = fe->demodulator_priv;
> > >   struct i2c_client *client = dev->client;
> > >   int ret;
> > > - u32 tmp;
> > > + u32 tmp = 0;
> > >
> > >   dev_dbg(&client->dev, "\n");
> > 
> > I looked the code and I cannot see how it could used as uninitialized. 
> > Dunno how it could be fixed properly.
> > 
> > Also, I think idiom to say compiler that variable could be uninitialized 
> > is to store its own value. But I am fine with zero initialization too.
> > 
> > u32 tmp = tmp;
> 
> Actually, the right way is to declare it as:
> 
>   u32 uninitialized_var(tmp)
> 
> The syntax to suppress compiler warnings depends on the compiler:
> 
> include/linux/compiler-clang.h:#define uninitialized_var(x) x = *(&(x))
> include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x
> 
> Also, using uninitialized_var() better documents it.
> 
> Regards,
> Mauro

Hi Mauro,

That is a way more elegant solution. Great!
I will check out that compiler-clang header file, it's interesting.

I just sent a revised patch using this :)

Thanks,
Luis
--
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: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Russell King - ARM Linux

On Wed, Feb 11, 2015 at 09:28:37AM +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-01-27 09:25, Sumit Semwal wrote:
> >Add some helpers to share the constraints of devices while attaching
> >to the dmabuf buffer.
> >
> >At each attach, the constraints are calculated based on the following:
> >- max_segment_size, max_segment_count, segment_boundary_mask from
> >device_dma_parameters.
> >
> >In case the attaching device's constraints don't match up, attach() fails.
> >
> >At detach, the constraints are recalculated based on the remaining
> >attached devices.
> >
> >Two helpers are added:
> >- dma_buf_get_constraints - which gives the current constraints as calculated
> >   during each attach on the buffer till the time,
> >- dma_buf_recalc_constraints - which recalculates the constraints for all
> >   currently attached devices for the 'paranoid' ones amongst us.
> >
> >The idea of this patch is largely taken from Rob Clark's RFC at
> >https://lkml.org/lkml/2012/7/19/285, and the comments received on it.
> >
> >Cc: Rob Clark 
> >Signed-off-by: Sumit Semwal 
> 
> The code looks okay, although it will probably will work well only with
> typical cases like 'contiguous memory needed' or 'no constraints at all'
> (iommu).

Which is a damn good reason to NAK it - by that admission, it's a half-baked
idea.

If all we want to know is whether the importer can accept only contiguous
memory or not, make a flag to do that, and allow the exporter to test this
flag.  Don't over-engineer this to make it _seem_ like it can do something
that it actually totally fails with.

As I've already pointed out, there's a major problem if you have already
had a less restrictive attachment which has an active mapping, and a new
more restrictive attachment comes along later.

It seems from Rob's descriptions that we also need another flag in the
importer to indicate whether it wants to have a valid struct page in the
scatter list, or whether it (correctly) uses the DMA accessors on the
scatter list - so that exporters can reject importers which are buggy.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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] rtl2832: remove compiler warning

2015-02-11 Thread Luis de Bethencourt

Cleaning up the following compiler warning:
rtl2832.c:703:12: warning: 'tmp' may be used uninitialized in this function

Even though it could never happen since if rtl2832_rd_demod_reg () doesn't set
tmp, this line would never run because we go to err. It is still nice to avoid
compiler warnings.

Signed-off-by: Luis de Bethencourt 
---
 drivers/media/dvb-frontends/rtl2832.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/rtl2832.c 
b/drivers/media/dvb-frontends/rtl2832.c
index 5d2d8f4..20fa245 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -685,7 +685,7 @@ static int rtl2832_read_status(struct dvb_frontend *fe, 
fe_status_t *status)
struct rtl2832_dev *dev = fe->demodulator_priv;
struct i2c_client *client = dev->client;
int ret;
-   u32 tmp;
+   u32 uninitialized_var(tmp);
 
dev_dbg(&client->dev, "\n");
 
-- 
2.1.3

--
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 3/3] media/videobuf2-dma-vmalloc: Save output from dma_map_sg

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-11 11:33, Ricardo Ribalda Delgado wrote:

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

Signed-off-by: Ricardo Ribalda Delgado 


Reviewed-by: Marek Szyprowski 


---
  drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c 
b/drivers/media/v4l2-core/videobuf2-vmalloc.c
index bcde885..f92bc9e 100644
--- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
+++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
@@ -287,7 +287,6 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
/* stealing dmabuf mutex to serialize map/unmap operations */
struct mutex *lock = &db_attach->dmabuf->lock;
struct sg_table *sgt;
-   int ret;
  
  	mutex_lock(lock);
  
@@ -306,8 +305,9 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(

}
  
  	/* mapping to the client with new direction */

-   ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-   if (ret <= 0) {
+   sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+   dma_dir);
+   if (!sgt->nents) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);


Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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/3] media/videobuf2-dma-contig: Save output from dma_map_sg

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-11 11:33, Ricardo Ribalda Delgado wrote:

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

Signed-off-by: Ricardo Ribalda Delgado 


Reviewed-by: Marek Szyprowski 


---
  drivers/media/v4l2-core/videobuf2-dma-contig.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index b481d20..bfb5917 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -299,7 +299,6 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
/* stealing dmabuf mutex to serialize map/unmap operations */
struct mutex *lock = &db_attach->dmabuf->lock;
struct sg_table *sgt;
-   int ret;
  
  	mutex_lock(lock);
  
@@ -318,8 +317,9 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(

}
  
  	/* mapping to the client with new direction */

-   ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-   if (ret <= 0) {
+   sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+   dma_dir);
+   if (!sgt->nents) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);


Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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 1/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-11 11:33, Ricardo Ribalda Delgado wrote:

When sg_alloc_table_from_pages() does not fail it returns a sg_table
structure with nents and nents_orig initialized to the same value.

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

The output of dma_map, should be used to transverse the scatter list.

dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).

sg_free_tables uses also orig_nent.

This patch fix the file to follow this paradigm.

Signed-off-by: Ricardo Ribalda Delgado 


Reviewed-by: Marek Szyprowski 

I would also consider sending it to stable.


---
  drivers/media/v4l2-core/videobuf2-dma-sg.c | 22 +-
  1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c 
b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index b1838ab..40c330f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -147,8 +147,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned 
long size,
 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-buf->dma_dir, &attrs) == 0)
+   sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+ buf->dma_dir, &attrs);
+   if (!sgt->nents)
goto fail_map;
  
  	buf->handler.refcount = &buf->refcount;

@@ -187,7 +188,7 @@ static void vb2_dma_sg_put(void *buf_priv)
dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
buf->num_pages);
-   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
+   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
   buf->dma_dir, &attrs);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
@@ -314,9 +315,11 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, 
unsigned long vaddr,
 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-buf->dma_dir, &attrs) == 0)
+   sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+ buf->dma_dir, &attrs);
+   if (!sgt->nents)
goto userptr_fail_map;
+
return buf;
  
  userptr_fail_map:

@@ -351,7 +354,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
  
  	dprintk(1, "%s: Releasing userspace buffer of %d pages\n",

   __func__, buf->num_pages);
-   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir, 
&attrs);
+   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
+  &attrs);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
sg_free_table(buf->dma_sgt);
@@ -502,7 +506,6 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
/* stealing dmabuf mutex to serialize map/unmap operations */
struct mutex *lock = &db_attach->dmabuf->lock;
struct sg_table *sgt;
-   int ret;
  
  	mutex_lock(lock);
  
@@ -521,8 +524,9 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(

}
  
  	/* mapping to the client with new direction */

-   ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-   if (ret <= 0) {
+   sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+   dma_dir);
+   if (!sgt->nents) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);


Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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/3] media/videobuf2-dma-contig: Save output from dma_map_sg

2015-02-11 Thread Ricardo Ribalda Delgado

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

Signed-off-by: Ricardo Ribalda Delgado 
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index b481d20..bfb5917 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -299,7 +299,6 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
/* stealing dmabuf mutex to serialize map/unmap operations */
struct mutex *lock = &db_attach->dmabuf->lock;
struct sg_table *sgt;
-   int ret;
 
mutex_lock(lock);
 
@@ -318,8 +317,9 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
}
 
/* mapping to the client with new direction */
-   ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-   if (ret <= 0) {
+   sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+   dma_dir);
+   if (!sgt->nents) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);
-- 
2.1.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 3/3] media/videobuf2-dma-vmalloc: Save output from dma_map_sg

2015-02-11 Thread Ricardo Ribalda Delgado

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

Signed-off-by: Ricardo Ribalda Delgado 
---
 drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c 
b/drivers/media/v4l2-core/videobuf2-vmalloc.c
index bcde885..f92bc9e 100644
--- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
+++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
@@ -287,7 +287,6 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
/* stealing dmabuf mutex to serialize map/unmap operations */
struct mutex *lock = &db_attach->dmabuf->lock;
struct sg_table *sgt;
-   int ret;
 
mutex_lock(lock);
 
@@ -306,8 +305,9 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(
}
 
/* mapping to the client with new direction */
-   ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-   if (ret <= 0) {
+   sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+   dma_dir);
+   if (!sgt->nents) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);
-- 
2.1.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 1/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

2015-02-11 Thread Ricardo Ribalda Delgado

When sg_alloc_table_from_pages() does not fail it returns a sg_table
structure with nents and nents_orig initialized to the same value.

dma_map_sg returns the number of areas mapped by the hardware,
which could be different than the areas given as an input.
The output must be saved to nent.

The output of dma_map, should be used to transverse the scatter list.

dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).

sg_free_tables uses also orig_nent.

This patch fix the file to follow this paradigm.

Signed-off-by: Ricardo Ribalda Delgado 
---
 drivers/media/v4l2-core/videobuf2-dma-sg.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c 
b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index b1838ab..40c330f 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -147,8 +147,9 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned 
long size,
 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-buf->dma_dir, &attrs) == 0)
+   sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+ buf->dma_dir, &attrs);
+   if (!sgt->nents)
goto fail_map;
 
buf->handler.refcount = &buf->refcount;
@@ -187,7 +188,7 @@ static void vb2_dma_sg_put(void *buf_priv)
dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
buf->num_pages);
-   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
+   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
   buf->dma_dir, &attrs);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
@@ -314,9 +315,11 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, 
unsigned long vaddr,
 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-buf->dma_dir, &attrs) == 0)
+   sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+ buf->dma_dir, &attrs);
+   if (!sgt->nents)
goto userptr_fail_map;
+
return buf;
 
 userptr_fail_map:
@@ -351,7 +354,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
 
dprintk(1, "%s: Releasing userspace buffer of %d pages\n",
   __func__, buf->num_pages);
-   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir, 
&attrs);
+   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
+  &attrs);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
sg_free_table(buf->dma_sgt);
@@ -502,7 +506,6 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
/* stealing dmabuf mutex to serialize map/unmap operations */
struct mutex *lock = &db_attach->dmabuf->lock;
struct sg_table *sgt;
-   int ret;
 
mutex_lock(lock);
 
@@ -521,8 +524,9 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
}
 
/* mapping to the client with new direction */
-   ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dma_dir);
-   if (ret <= 0) {
+   sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
+   dma_dir);
+   if (!sgt->nents) {
pr_err("failed to map scatterlist\n");
mutex_unlock(lock);
return ERR_PTR(-EIO);
-- 
2.1.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 1/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

2015-02-11 Thread Ricardo Ribalda Delgado

Hello again

On Wed, Feb 11, 2015 at 10:00 AM, Marek Szyprowski
 wrote:

> Well, this int return value seems to be misleading, but according to
> Documentation/DMA-API.txt, the only error value is zero:
>
> "As with the other mapping interfaces, dma_map_sg() can fail. When it
> does, 0 is returned and a driver must take appropriate action. It is
> critical that the driver do something, in the case of a block driver
> aborting the request or even oopsing is better than doing nothing and
> corrupting the filesystem."
>
> I've also checked various dma-mapping implementation for different
> architectures and they follow this convention.
>
> Maybe one should add some comments to include/linux/dma_mapping.h to
> clarify this and avoid further confusion.
>
>

Or maybe change it to unsigned int...

Let me redo the patch and resend. I will also try to ping whoever is
the maintainer of dma_mapping

Thanks!



-- 
Ricardo Ribalda
--
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/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-11 09:37, Ricardo Ribalda Delgado wrote:

Hello Marek
On Wed, Feb 11, 2015 at 9:06 AM, Marek Szyprowski
 wrote:

Unfortunately nent differs in sign to the output of dma_map_sg, so an
intermediate value must be used.


I don't get this part. dma_map_sg() returns the number of scatter list
entries mapped
to the hardware or zero if anything fails. What is the problem of assigning
it directly
to nents?

Are you sure about that?

The prototype of the function is (from dma-mapping-common.h)
static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
   int nents, enum dma_data_direction dir,
   struct dma_attrs *attrs)

which calls map_sg at the struct dma_map_ops (dma-mapping.h)

int (*map_sg)(struct device *dev, struct scatterlist *sg,
  int nents, enum dma_data_direction dir,
  struct dma_attrs *attrs);

Both return int instead of unsigned int


Well, this int return value seems to be misleading, but according to
Documentation/DMA-API.txt, the only error value is zero:

"As with the other mapping interfaces, dma_map_sg() can fail. When it
does, 0 is returned and a driver must take appropriate action. It is
critical that the driver do something, in the case of a block driver
aborting the request or even oopsing is better than doing nothing and
corrupting the filesystem."

I've also checked various dma-mapping implementation for different
architectures and they follow this convention.

Maybe one should add some comments to include/linux/dma_mapping.h to
clarify this and avoid further confusion.





dma_map_sg_attrs() return 0 in case of error, so the check can be
simplified,
there is no need for temporary variable.

Check last comment


 vm_unmap_ram(buf->vaddr, buf->num_pages);
 sg_free_table(buf->dma_sgt);
@@ -463,7 +470,7 @@ static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf
*dbuf, struct device *dev
 rd = buf->dma_sgt->sgl;
 wr = sgt->sgl;
-   for (i = 0; i < sgt->orig_nents; ++i) {
+   for (i = 0; i < sgt->nents; ++i) {


Here the code iterates over every memory page in the scatter list (to create
a copy of it), not the device mapped chunks, so it must use orig_nents
like it was already there.

At that point both have the same value, but you are right, it is more
clear to use orig_nents



I will resend a version using orig_nents in dmabug_ops attach, but
please take a look the map_sg, I think it can return <0




Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

2015-02-11 Thread Ricardo Ribalda Delgado

Hello Marek
On Wed, Feb 11, 2015 at 9:06 AM, Marek Szyprowski
 wrote:
>> Unfortunately nent differs in sign to the output of dma_map_sg, so an
>> intermediate value must be used.
>
>
> I don't get this part. dma_map_sg() returns the number of scatter list
> entries mapped
> to the hardware or zero if anything fails. What is the problem of assigning
> it directly
> to nents?

Are you sure about that?

The prototype of the function is (from dma-mapping-common.h)
static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
  int nents, enum dma_data_direction dir,
  struct dma_attrs *attrs)

which calls map_sg at the struct dma_map_ops (dma-mapping.h)

int (*map_sg)(struct device *dev, struct scatterlist *sg,
 int nents, enum dma_data_direction dir,
 struct dma_attrs *attrs);

Both return int instead of unsigned int

>
>
> dma_map_sg_attrs() return 0 in case of error, so the check can be
> simplified,
> there is no need for temporary variable.

Check last comment

>> vm_unmap_ram(buf->vaddr, buf->num_pages);
>> sg_free_table(buf->dma_sgt);
>> @@ -463,7 +470,7 @@ static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf
>> *dbuf, struct device *dev
>> rd = buf->dma_sgt->sgl;
>> wr = sgt->sgl;
>> -   for (i = 0; i < sgt->orig_nents; ++i) {
>> +   for (i = 0; i < sgt->nents; ++i) {
>
>
> Here the code iterates over every memory page in the scatter list (to create
> a copy of it), not the device mapped chunks, so it must use orig_nents
> like it was already there.

At that point both have the same value, but you are right, it is more
clear to use orig_nents

>

>
> Best regards


I will resend a version using orig_nents in dmabug_ops attach, but
please take a look the map_sg, I think it can return <0

Best regards!

-- 
Ricardo Ribalda
--
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: [RFCv3 2/2] dma-buf: add helpers for sharing attacher constraints with dma-parms

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-01-27 09:25, Sumit Semwal wrote:

Add some helpers to share the constraints of devices while attaching
to the dmabuf buffer.

At each attach, the constraints are calculated based on the following:
- max_segment_size, max_segment_count, segment_boundary_mask from
device_dma_parameters.

In case the attaching device's constraints don't match up, attach() fails.

At detach, the constraints are recalculated based on the remaining
attached devices.

Two helpers are added:
- dma_buf_get_constraints - which gives the current constraints as calculated
   during each attach on the buffer till the time,
- dma_buf_recalc_constraints - which recalculates the constraints for all
   currently attached devices for the 'paranoid' ones amongst us.

The idea of this patch is largely taken from Rob Clark's RFC at
https://lkml.org/lkml/2012/7/19/285, and the comments received on it.

Cc: Rob Clark 
Signed-off-by: Sumit Semwal 


The code looks okay, although it will probably will work well only with 
typical

cases like 'contiguous memory needed' or 'no constraints at all' (iommu).

Acked-by: Marek Szyprowski 


---
v3:
- Thanks to Russell's comment, remove dma_mask and coherent_dma_mask from
   constraints' calculation; has a nice side effect of letting us use
   device_dma_parameters directly to list constraints.
- update the debugfs output to show constraint info as well.
   
v2: split constraints-sharing and allocation helpers


  drivers/dma-buf/dma-buf.c | 126 +-
  include/linux/dma-buf.h   |   7 +++
  2 files changed, 132 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 5be225c2ba98..f363f1440803 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -264,6 +264,66 @@ static inline int is_dma_buf_file(struct file *file)
return file->f_op == &dma_buf_fops;
  }
  
+static inline void init_constraints(struct device_dma_parameters *cons)

+{
+   cons->max_segment_count = (unsigned int)-1;
+   cons->max_segment_size = (unsigned int)-1;
+   cons->segment_boundary_mask = (unsigned long)-1;
+}
+
+/*
+ * calc_constraints - calculates if the new attaching device's constraints
+ * match, with the constraints of already attached devices; if yes, returns
+ * the constraints; else return ERR_PTR(-EINVAL)
+ */
+static int calc_constraints(struct device *dev,
+   struct device_dma_parameters *calc_cons)
+{
+   struct device_dma_parameters cons = *calc_cons;
+
+   cons.max_segment_count = min(cons.max_segment_count,
+   dma_get_max_seg_count(dev));
+   cons.max_segment_size = min(cons.max_segment_size,
+   dma_get_max_seg_size(dev));
+   cons.segment_boundary_mask &= dma_get_seg_boundary(dev);
+
+   if (!cons.max_segment_count ||
+   !cons.max_segment_size ||
+   !cons.segment_boundary_mask) {
+   pr_err("Dev: %s's constraints don't match\n", dev_name(dev));
+   return -EINVAL;
+   }
+
+   *calc_cons = cons;
+
+   return 0;
+}
+
+/*
+ * recalc_constraints - recalculates constraints for all attached devices;
+ *  useful for detach() recalculation, and for dma_buf_recalc_constraints()
+ *  helper.
+ *  Returns recalculated constraints in recalc_cons, or error in the unlikely
+ *  case when constraints of attached devices might have changed.
+ */
+static int recalc_constraints(struct dma_buf *dmabuf,
+ struct device_dma_parameters *recalc_cons)
+{
+   struct device_dma_parameters calc_cons;
+   struct dma_buf_attachment *attach;
+   int ret = 0;
+
+   init_constraints(&calc_cons);
+
+   list_for_each_entry(attach, &dmabuf->attachments, node) {
+   ret = calc_constraints(attach->dev, &calc_cons);
+   if (ret)
+   return ret;
+   }
+   *recalc_cons = calc_cons;
+   return 0;
+}
+
  /**
   * dma_buf_export_named - Creates a new dma_buf, and associates an anon file
   * with this buffer, so it can be exported.
@@ -313,6 +373,9 @@ struct dma_buf *dma_buf_export_named(void *priv, const 
struct dma_buf_ops *ops,
dmabuf->ops = ops;
dmabuf->size = size;
dmabuf->exp_name = exp_name;
+
+   init_constraints(&dmabuf->constraints);
+
init_waitqueue_head(&dmabuf->poll);
dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
@@ -422,7 +485,7 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf 
*dmabuf,
  struct device *dev)
  {
struct dma_buf_attachment *attach;
-   int ret;
+   int ret = 0;
  
  	if (WARN_ON(!dmabuf || !dev))

return ERR_PTR(-EINVAL);
@@ -436,6 +499,9 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf 
*dma

Re: [PATCH 2/3] media/videobuf2-dma-contig: Fix handling of sg_table structure

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-09 17:14, Ricardo Ribalda Delgado wrote:

when sg_alloc_table_from_pages() does not fail it returns a sg_table
structure with nents and nents_orig initialized to the same value.

dma_map_sg returns the dma_map_sg returns the number of areas mapped
by the hardware, which could be different than the areas given as an input.
The output must be saved to nent.
Unfortunately nent differs in sign to the output of dma_map_sg, so an
intermediate value must be used.

The output of dma_map, should be used to transverse the scatter list.

dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).

sg_free_tables uses also orig_nent.

This patch fix the file to follow this paradigm.

Signed-off-by: Ricardo Ribalda Delgado 
---
  drivers/media/v4l2-core/videobuf2-dma-contig.c | 10 ++
  1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c 
b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index b481d20..c7e4bdd 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -56,7 +56,7 @@ static void vb2_dc_sgt_foreach_page(struct sg_table *sgt,
struct scatterlist *s;
unsigned int i;
  
-	for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {

+   for_each_sg(sgt->sgl, s, sgt->nents, i) {
struct page *page = sg_page(s);
unsigned int n_pages = PAGE_ALIGN(s->offset + s->length)
>> PAGE_SHIFT;


This code iterates over memory pages added to the scatter list not the 
dma chunks,

so orig_nents must be used. This change is not needed.


@@ -260,7 +260,7 @@ static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, 
struct device *dev,
  
  	rd = buf->sgt_base->sgl;

wr = sgt->sgl;
-   for (i = 0; i < sgt->orig_nents; ++i) {
+   for (i = 0; i < sgt->nents; ++i) {
sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
rd = sg_next(rd);
wr = sg_next(wr);


Same comment as for videobuf2-dma-sg.c patch.


@@ -324,6 +324,7 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
mutex_unlock(lock);
return ERR_PTR(-EIO);
}
+   sgt->nents = ret;
  
  	attach->dma_dir = dma_dir;
  
@@ -669,13 +670,14 @@ static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,

 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+   ret = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
  buf->dma_dir, &attrs);
-   if (sgt->nents <= 0) {
+   if (ret <= 0) {
pr_err("failed to map scatterlist\n");
ret = -EIO;
goto fail_sgt_init;
}
+   sgt->nents = ret;


This one is okay, although the check for error could be simplified to a 
check

for zero value.

  
  	contig_size = vb2_dc_get_contiguous_size(sgt);

if (contig_size < size) {


Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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/3] media/videobuf2-dma-sg: Fix handling of sg_table structure

2015-02-11 Thread Marek Szyprowski


Hello,

On 2015-02-09 17:14, Ricardo Ribalda Delgado wrote:

when sg_alloc_table_from_pages() does not fail it returns a sg_table
structure with nents and nents_orig initialized to the same value.

dma_map_sg returns the dma_map_sg returns the number of areas mapped
by the hardware, which could be different than the areas given as an input.
The output must be saved to nent.


Thanks for catching this issue!


Unfortunately nent differs in sign to the output of dma_map_sg, so an
intermediate value must be used.


I don't get this part. dma_map_sg() returns the number of scatter list 
entries mapped
to the hardware or zero if anything fails. What is the problem of 
assigning it directly

to nents?


The output of dma_map, should be used to transverse the scatter list.

dma_unmap_sg needs the value passed to dma_map_sg (nents_orig).

sg_free_tables uses also orig_nent.

This patch fix the file to follow this paradigm.

Signed-off-by: Ricardo Ribalda Delgado 
---
  drivers/media/v4l2-core/videobuf2-dma-sg.c | 22 +++---
  1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c 
b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index b1838ab..30bac99 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -147,9 +147,11 @@ static void *vb2_dma_sg_alloc(void *alloc_ctx, unsigned 
long size,
 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-buf->dma_dir, &attrs) == 0)
+   ret = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+   buf->dma_dir, &attrs);
+   if (ret <= 0)
goto fail_map;
+   sgt->nents = ret;
  
  	buf->handler.refcount = &buf->refcount;

buf->handler.put = vb2_dma_sg_put;
@@ -187,7 +189,7 @@ static void vb2_dma_sg_put(void *buf_priv)
dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
dprintk(1, "%s: Freeing buffer of %d pages\n", __func__,
buf->num_pages);
-   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
+   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
   buf->dma_dir, &attrs);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
@@ -240,6 +242,7 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, 
unsigned long vaddr,
struct vm_area_struct *vma;
struct sg_table *sgt;
DEFINE_DMA_ATTRS(attrs);
+   int ret;
  
  	dma_set_attr(DMA_ATTR_SKIP_CPU_SYNC, &attrs);
  
@@ -314,9 +317,12 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,

 * No need to sync to the device, this will happen later when the
 * prepare() memop is called.
 */
-   if (dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->nents,
-buf->dma_dir, &attrs) == 0)
+   ret = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
+buf->dma_dir, &attrs);
+   if (ret <= 0)
goto userptr_fail_map;


dma_map_sg_attrs() return 0 in case of error, so the check can be 
simplified,

there is no need for temporary variable.


+   sgt->nents = ret;
+
return buf;
  
  userptr_fail_map:

@@ -351,7 +357,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
  
  	dprintk(1, "%s: Releasing userspace buffer of %d pages\n",

   __func__, buf->num_pages);
-   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir, 
&attrs);
+   dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,
+   &attrs);
if (buf->vaddr)
vm_unmap_ram(buf->vaddr, buf->num_pages);
sg_free_table(buf->dma_sgt);
@@ -463,7 +470,7 @@ static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf 
*dbuf, struct device *dev
  
  	rd = buf->dma_sgt->sgl;

wr = sgt->sgl;
-   for (i = 0; i < sgt->orig_nents; ++i) {
+   for (i = 0; i < sgt->nents; ++i) {


Here the code iterates over every memory page in the scatter list (to create
a copy of it), not the device mapped chunks, so it must use orig_nents
like it was already there.


sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
rd = sg_next(rd);
wr = sg_next(wr);
@@ -527,6 +534,7 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(
mutex_unlock(lock);
return ERR_PTR(-EIO);
}
+   sgt->nents = ret;


This one is okay.

  
  	attach->dma_dir = dma_dir;
  


Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
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

40 matches

Mail list logo