Re: [patch] [media] uvcvideo: freeing an error pointer

[Julia Lawall]

To put it another way, tools can figure out what is missing if they have
access to good exmples of what should be there.

To be clear, I can see both points of view.

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

Re: [PATCH v4l-utils v7 1/7] mediactl: Add support for v4l2-ctrl-binding config

[Sakari Ailus]

Hi Jacek,

On Mon, Nov 28, 2016 at 10:32:43PM +0100, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> On 11/24/2016 03:23 PM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Wed, Oct 12, 2016 at 04:35:16PM +0200, Jacek Anaszewski wrote:
> >>Make struct v4l2_subdev capable of aggregating v4l2-ctrl-bindings -
> >>media device configuration entries. Added are also functions for
> >>validating support for the control on given media entity and checking
> >>whether a v4l2-ctrl-binding has been defined for a media entity.
> >
> >I still don't think this belongs here.
> >
> >I think I told you about the generic pipeline configuration library I worked
> >on years ago; unfortunately it was left on prototype stage. Still, what I
> >realised was that something very similar is needed in that library ---
> >associating information to the representation of the media entities (or the
> >V4L2 sub-devices) in user space that has got nothing to do with the devices
> >themselves.
> >
> >We could have e.g. a list of key--value pairs where the key is a pointer
> >provided by the user (i.e. application, another library) that could be
> >associated with the value. That would avoid having explicit information on
> >that in the struct media_entity itself.
> >
> >An quicker alternative would be to manage a list of controls e.g. in the
> >plugin itself and store the media entity where they're implemented in that
> >list, with the control value.

s/value/id/

> 
> We are not interested in media entity -> control value relation but
> but media entity -> control id. The value is an arbitrary choice of
> userspace. Binding's task is to route the ctrl ioctl to a desired
> pipeline entity if more than one supports same control.

Correct. But even that's more efficient if you don't need to iterate over
all the entities.

> 
> Effectively we'd need a list of controls as a keys and entities
> as values. The list should be allocated dynamically as it would
> make no sense to keep keys for all v4l2 controls if only few bindings
> are defined.
> 
> Best regards,
> Jacek Anaszewski
> 
> >Cc Laurent as well.
> >
> >>
> >>Signed-off-by: Jacek Anaszewski 
> >>Acked-by: Kyungmin Park 
> >>---
> >> utils/media-ctl/libv4l2subdev.c | 32 
> >> utils/media-ctl/v4l2subdev.h| 19 +++
> >> 2 files changed, 51 insertions(+)
> >>
> >>diff --git a/utils/media-ctl/libv4l2subdev.c 
> >>b/utils/media-ctl/libv4l2subdev.c
> >>index c3439d7..4f8ee7f 100644
> >>--- a/utils/media-ctl/libv4l2subdev.c
> >>+++ b/utils/media-ctl/libv4l2subdev.c
> >>@@ -50,7 +50,15 @@ int v4l2_subdev_create(struct media_entity *entity)
> >>
> >>entity->sd->fd = -1;
> >>
> >>+   entity->sd->v4l2_ctrl_bindings = malloc(sizeof(__u32));
> >>+   if (entity->sd->v4l2_ctrl_bindings == NULL)
> >>+   goto err_v4l2_ctrl_bindings_alloc;
> >>+
> >>return 0;
> >>+
> >>+err_v4l2_ctrl_bindings_alloc:
> >>+   free(entity->sd);
> >>+   return -ENOMEM;
> >> }
> >>
> >> int v4l2_subdev_create_opened(struct media_entity *entity, int fd)
> >>@@ -102,6 +110,7 @@ void v4l2_subdev_close(struct media_entity *entity)
> >>if (entity->sd->fd_owner)
> >>close(entity->sd->fd);
> >>
> >>+   free(entity->sd->v4l2_ctrl_bindings);
> >>free(entity->sd);
> >> }
> >>
> >>@@ -884,3 +893,26 @@ const enum v4l2_mbus_pixelcode 
> >>*v4l2_subdev_pixelcode_list(unsigned int *length)
> >>
> >>return mbus_codes;
> >> }
> >>+
> >>+int v4l2_subdev_supports_v4l2_ctrl(struct media_device *media,
> >>+  struct media_entity *entity,
> >>+  __u32 ctrl_id)
> >>+{
> >>+   struct v4l2_queryctrl queryctrl = {};
> >>+   int ret;
> >>+
> >>+   ret = v4l2_subdev_open(entity);
> >>+   if (ret < 0)
> >>+   return ret;
> >>+
> >>+   queryctrl.id = ctrl_id;
> >>+
> >>+   ret = ioctl(entity->sd->fd, VIDIOC_QUERYCTRL, );
> >>+   if (ret < 0)
> >>+   return ret;
> >>+
> >>+   media_dbg(media, "Validated control \"%s\" (0x%8.8x) on entity %s\n",
> >>+ queryctrl.name, queryctrl.id, entity->info.name);
> >>+
> >>+   return 0;
> >>+}
> >>diff --git a/utils/media-ctl/v4l2subdev.h b/utils/media-ctl/v4l2subdev.h
> >>index 011fab1..4dee6b1 100644
> >>--- a/utils/media-ctl/v4l2subdev.h
> >>+++ b/utils/media-ctl/v4l2subdev.h
> >>@@ -26,10 +26,14 @@
> >>
> >> struct media_device;
> >> struct media_entity;
> >>+struct media_device;
> >>
> >> struct v4l2_subdev {
> >>int fd;
> >>unsigned int fd_owner:1;
> >>+
> >>+   __u32 *v4l2_ctrl_bindings;
> >>+   unsigned int num_v4l2_ctrl_bindings;
> >> };
> >>
> >> /**
> >>@@ -314,4 +318,19 @@ enum v4l2_field v4l2_subdev_string_to_field(const char 
> >>*string);
> >> const enum v4l2_mbus_pixelcode *v4l2_subdev_pixelcode_list(
> >>unsigned int *length);
> >>
> >>+/**
> >>+ * @brief Check if sub-device supports given v4l2 control
> >>+ * @param media - media device.
> >>+ * @param entity - media entity.
> 

cron job: media_tree daily build: ERRORS

[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:   Tue Nov 29 05:00:17 CET 2016
media-tree git hash:d3d83ee20afda16ad0133ba00f63c11a8d842a35
media_build git hash:   1606032398b1d79149c1507be2029e1a00d8dff0
v4l-utils git hash: dab9bf5687eddea2f4cb8cdb38b3bbc5b079a037
gcc version:i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware:  x86_64
host os:4.8.0-164

linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: 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.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: OK
linux-3.12.67-i686: OK
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-rc5-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: OK
linux-3.12.67-x86_64: OK
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: OK
linux-4.9-rc5-x86_64: OK
apps: WARNINGS
spec-git: OK
smatch: ERRORS
sparse: WARNINGS

Detailed results are available here:

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

Full logs are available here:

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

The Media Infrastructure API from this daily build is here:

http://www.xs4all.nl/~hverkuil/spec/index.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 2/2] media: protect enable and disable source handler checks and calls

[Shuah Khan]

Protect enable and disable source handler checks and calls from dvb-core
and v4l2-core. Hold graph_mutex to check if enable and disable source
handlers are present and invoke them while holding the mutex. This change
ensures these handlers will not be removed while they are being checked
and invoked.

au08282 enable and disable source handlers are changed to not hold the
graph_mutex.

Signed-off-by: Shuah Khan 
---
 drivers/media/dvb-core/dvb_frontend.c  | 24 ++--
 drivers/media/usb/au0828/au0828-core.c | 17 +
 drivers/media/v4l2-core/v4l2-mc.c  | 26 ++
 3 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c 
b/drivers/media/dvb-core/dvb_frontend.c
index 01511e5..2f09c7e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct 
file *file)
fepriv->voltage = -1;
 
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-   if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
-   ret = fe->dvb->mdev->enable_source(dvbdev->entity,
+   if (fe->dvb->mdev) {
+   mutex_lock(>dvb->mdev->graph_mutex);
+   if (fe->dvb->mdev->enable_source)
+   ret = fe->dvb->mdev->enable_source(
+  dvbdev->entity,
   >pipe);
+   mutex_unlock(>dvb->mdev->graph_mutex);
if (ret) {
dev_err(fe->dvb->device,
"Tuner is busy. Error %d\n", ret);
@@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, struct 
file *file)
 
 err3:
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-   if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-   fe->dvb->mdev->disable_source(dvbdev->entity);
+   if (fe->dvb->mdev) {
+   mutex_lock(>dvb->mdev->graph_mutex);
+   if (fe->dvb->mdev->disable_source)
+   fe->dvb->mdev->disable_source(dvbdev->entity);
+   mutex_unlock(>dvb->mdev->graph_mutex);
+   }
 err2:
 #endif
dvb_generic_release(inode, file);
@@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, 
struct file *file)
if (dvbdev->users == -1) {
wake_up(>wait_queue);
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-   if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-   fe->dvb->mdev->disable_source(dvbdev->entity);
+   if (fe->dvb->mdev) {
+   mutex_lock(>dvb->mdev->graph_mutex);
+   if (fe->dvb->mdev->disable_source)
+   fe->dvb->mdev->disable_source(dvbdev->entity);
+   mutex_unlock(>dvb->mdev->graph_mutex);
+   }
 #endif
if (fe->exit != DVB_FE_NO_EXIT)
wake_up(>wait_queue);
diff --git a/drivers/media/usb/au0828/au0828-core.c 
b/drivers/media/usb/au0828/au0828-core.c
index a1f696a..bfd6482 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -280,6 +280,7 @@ static void au0828_media_graph_notify(struct media_entity 
*new,
}
 }
 
+/* Callers should hold graph_mutex */
 static int au0828_enable_source(struct media_entity *entity,
struct media_pipeline *pipe)
 {
@@ -293,8 +294,6 @@ static int au0828_enable_source(struct media_entity *entity,
if (!mdev)
return -ENODEV;
 
-   mutex_lock(>graph_mutex);
-
dev = mdev->source_priv;
 
/*
@@ -421,12 +420,12 @@ static int au0828_enable_source(struct media_entity 
*entity,
 dev->active_source->name, dev->active_sink->name,
 dev->active_link_owner->name, ret);
 end:
-   mutex_unlock(>graph_mutex);
pr_debug("au0828_enable_source() end %s %d %d\n",
 entity->name, entity->function, ret);
return ret;
 }
 
+/* Callers should hold graph_mutex */
 static void au0828_disable_source(struct media_entity *entity)
 {
int ret = 0;
@@ -436,13 +435,10 @@ static void au0828_disable_source(struct media_entity 
*entity)
if (!mdev)
return;
 
-   mutex_lock(>graph_mutex);
dev = mdev->source_priv;
 
-   if (!dev->active_link) {
-   ret = -ENODEV;
-   goto end;
-   }
+   if (!dev->active_link)
+   return;
 
/* link is active - stop pipeline from source (tuner) */
if (dev->active_link->sink->entity == dev->active_sink &&
@@ -452,7 +448,7 @@ static void au0828_disable_source(struct media_entity 
*entity)
 * has active pipeline
   

[PATCH 1/2] media: au0828 fix to protect enable/disable source set and clear

[Shuah Khan]

Protect enable/disable source set and clear to avoid races with callers.
There is a possibility clear could occur while dvb-core and v4l2 try to
access these handlers.

Signed-off-by: Shuah Khan 
---
 drivers/media/usb/au0828/au0828-core.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c 
b/drivers/media/usb/au0828/au0828-core.c
index bf53553..a1f696a 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -153,9 +153,11 @@ static void au0828_unregister_media_device(struct 
au0828_dev *dev)
}
 
/* clear enable_source, disable_source */
+   mutex_lock(>graph_mutex);
dev->media_dev->source_priv = NULL;
dev->media_dev->enable_source = NULL;
dev->media_dev->disable_source = NULL;
+   mutex_unlock(>graph_mutex);
 
media_device_unregister(dev->media_dev);
media_device_cleanup(dev->media_dev);
@@ -549,9 +551,11 @@ static int au0828_media_device_register(struct au0828_dev 
*dev,
return ret;
}
/* set enable_source */
+   mutex_lock(>media_dev->graph_mutex);
dev->media_dev->source_priv = (void *) dev;
dev->media_dev->enable_source = au0828_enable_source;
dev->media_dev->disable_source = au0828_disable_source;
+   mutex_unlock(>media_dev->graph_mutex);
 #endif
return 0;
 }
-- 
2.7.4

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

[PATCH 0/2] media protect enable and disable source handler paths

[Shuah Khan]

These two patches fix enable and disable source handler paths. These
aren't dependent patches, grouped because they fix similar problems.

This work is triggered by a review comment from Mauro Chehab on a
snd_usb_audio patch about protecting the enable and disabel handler
path in it.

Ran tests to make sure enable and disable handler paths work. When
digital stream is active, analog app finds the tuner busy and vice
versa. Also ran the Sakari's unbind while video stream is active test.

Shuah Khan (2):
  media: au0828 fix to protect enable/disable source set and clear
  media: protect enable and disable source handler checks and calls

 drivers/media/dvb-core/dvb_frontend.c  | 24 ++--
 drivers/media/usb/au0828/au0828-core.c | 21 +
 drivers/media/v4l2-core/v4l2-mc.c  | 26 ++
 3 files changed, 45 insertions(+), 26 deletions(-)

-- 
2.7.4

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

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Hi Rob,

Rob Herring  writes:

> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> Signed-off-by: Kevin Hilman 
>> ---
>>  .../bindings/media/ti,da850-vpif-capture.txt   | 65 
>> ++
>>  .../devicetree/bindings/media/ti,da850-vpif.txt|  8 +++
>>  2 files changed, 73 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt 
>> b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> new file mode 100644
>> index ..c447ac482c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> @@ -0,0 +1,65 @@
>> +Texas Instruments VPIF Capture
>> +--
>> +
>> +The TI Video Port InterFace (VPIF) capture component is the primary
>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>> +
>> +TI Document number reference: SPRUH82C
>> +
>> +Required properties:
>> +- compatible: must be "ti,da850-vpif-capture"
>> +- reg: physical base address and length of the registers set for the device;
>> +- interrupts: should contain IRQ line for the VPIF
>> +
>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>> +channels or a single 16-bit channel.  It should contain at least one
>> +port child node with child 'endpoint' node. Please refer to the
>> +bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example using 2 8-bit input channels, one of which is connected to an
>> +I2C-connected TVP5147 decoder:
>> +
>> +vpif_capture: video-capture@0x00217000 {
>> +reg = <0x00217000 0x1000>;
>> +interrupts = <92>;
>> +
>> +port {
>> +vpif_ch0: endpoint@0 {
>> +  reg = <0>;
>> +  bus-width = <8>;
>> +  remote-endpoint = <>;
>> +};
>> +
>> +vpif_ch1: endpoint@1 {
>
> I think probably channels here should be ports rather than endpoints. 
> AIUI, having multiple endpoints is for cases like a mux or 1 to many 
> connections. There's only one data flow, but multiple sources or sinks.

Looking at this closer... , I used an endpoint because it's bascially a
16-bit parallel bus, that can be configured as (up to) 2 8-bit
"channels.  So, based on the video-interfaces.txt doc, I configured this
as a single port, with (up to) 2 endpoints.  That also allows me to
connect output of the decoder directly, using the remote-endpoint
property.

So I guess I'm not fully understanding your suggestion.

Kevin
--
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: Enabling peer to peer device transactions for PCIe devices

[Jason Gunthorpe]

On Mon, Nov 28, 2016 at 04:55:23PM -0500, Serguei Sagalovitch wrote:

> >We haven't touch this in a long time and perhaps it changed, but there
> >definitely was a call back in the PeerDirect API to allow the GPU to
> >invalidate the mapping. That's what we don't want.

> I assume that you are talking about "invalidate_peer_memory()' callback?
> I was told that it is the "last resort" because HCA (and driver) is not
> able to handle  it in the safe manner so it is basically "abort" everything.

If it is a last resort to save system stability then kill the impacted
process, that will release the MRs.

Jason
--
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 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Rob Herring  writes:

> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> Signed-off-by: Kevin Hilman 
>> ---
>>  .../bindings/media/ti,da850-vpif-capture.txt   | 65 
>> ++
>>  .../devicetree/bindings/media/ti,da850-vpif.txt|  8 +++
>>  2 files changed, 73 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt 
>> b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> new file mode 100644
>> index ..c447ac482c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> @@ -0,0 +1,65 @@
>> +Texas Instruments VPIF Capture
>> +--
>> +
>> +The TI Video Port InterFace (VPIF) capture component is the primary
>> +component for video capture on the DA850 family of TI DaVinci SoCs.
>> +
>> +TI Document number reference: SPRUH82C
>> +
>> +Required properties:
>> +- compatible: must be "ti,da850-vpif-capture"
>> +- reg: physical base address and length of the registers set for the device;
>> +- interrupts: should contain IRQ line for the VPIF
>> +
>> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
>> +channels or a single 16-bit channel.  It should contain at least one
>> +port child node with child 'endpoint' node. Please refer to the
>> +bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example using 2 8-bit input channels, one of which is connected to an
>> +I2C-connected TVP5147 decoder:
>> +
>> +vpif_capture: video-capture@0x00217000 {
>
> Drop the 0x00.
>
>> +compatible = "ti,da850-vpif-capture";
>> +reg = <0x00217000 0x1000>;
>> +interrupts = <92>;
>> +
>> +port {
>> +vpif_ch0: endpoint@0 {
>> +  reg = <0>;
>
> This is missing #size-cells and #addr-cells.
>

Yup.

>> +  bus-width = <8>;
>> +  remote-endpoint = <>;
>> +};
>> +
>> +vpif_ch1: endpoint@1 {
>
> I think probably channels here should be ports rather than endpoints. 
> AIUI, having multiple endpoints is for cases like a mux or 1 to many 
> connections. There's only one data flow, but multiple sources or sinks.

OK.

>> +  reg = <1>;
>> +  bus-width = <8>;
>> +  data-shift = <8>;
>> +};
>> +};
>> +};
>> +
>> +[ ... ]
>> +
>> + {
>> +
>> +tvp5147@5d {
>> +compatible = "ti,tvp5147";
>> +reg = <0x5d>;
>> +status = "okay";
>> +
>> +port {
>> +composite: endpoint {
>> +hsync-active = <1>;
>> +vsync-active = <1>;
>> +pclk-sample = <0>;
>> +
>> +/* VPIF channel 0 (lower 8-bits) */
>> +remote-endpoint = <_ch0>;
>> +bus-width = <8>;
>> +};
>> +};
>> +};
>> +
>> +};
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt 
>> b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> new file mode 100644
>> index ..d004e600aabe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> @@ -0,0 +1,8 @@
>> +Texas Instruments VPIF
>> +--
>> +
>> +The Video Port InterFace (VPIF) is the core component for video output
>> +and capture on DA850 TI Davinci SoCs.
>> +
>> +- compatible: must be "ti,da850-vpif"
>> +- reg: physical base address and length of the registers set for the device;
>
> That's it? How does this block relate to the capture block?

I separated them because the current legacy drivers are separated into 3
different platform drivers.

However, after some discussions with Laurent, I'm going to just create a
single VPIF node with input (capture) and output (display) ports, and
then have to tweak the existing drivers a bit more than I had wanted to.

IOW, I was lazy.

Kevin

--
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: Enabling peer to peer device transactions for PCIe devices

[Serguei Sagalovitch]

On 2016-11-28 04:36 PM, Logan Gunthorpe wrote:

On 28/11/16 12:35 PM, Serguei Sagalovitch wrote:

As soon as PeerDirect mapping is called then GPU must not "move" the
such memory.  It is by PeerDirect design. It is similar how it is works
with system memory and RDMA MR: when "get_user_pages" is called then the
memory is pinned.

We haven't touch this in a long time and perhaps it changed, but there
definitely was a call back in the PeerDirect API to allow the GPU to
invalidate the mapping. That's what we don't want.

I assume that you are talking about "invalidate_peer_memory()' callback?
I was told that it is the "last resort" because HCA (and driver) is not
able to handle  it in the safe manner so it is basically "abort" 
everything.


--
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 4/4] [media] dt-bindings: add TI VPIF documentation

[Rob Herring]

On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> Signed-off-by: Kevin Hilman 
> ---
>  .../bindings/media/ti,da850-vpif-capture.txt   | 65 
> ++
>  .../devicetree/bindings/media/ti,da850-vpif.txt|  8 +++
>  2 files changed, 73 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>  create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> 
> diff --git 
> a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt 
> b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> new file mode 100644
> index ..c447ac482c1d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> @@ -0,0 +1,65 @@
> +Texas Instruments VPIF Capture
> +--
> +
> +The TI Video Port InterFace (VPIF) capture component is the primary
> +component for video capture on the DA850 family of TI DaVinci SoCs.
> +
> +TI Document number reference: SPRUH82C
> +
> +Required properties:
> +- compatible: must be "ti,da850-vpif-capture"
> +- reg: physical base address and length of the registers set for the device;
> +- interrupts: should contain IRQ line for the VPIF
> +
> +VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
> +channels or a single 16-bit channel.  It should contain at least one
> +port child node with child 'endpoint' node. Please refer to the
> +bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example using 2 8-bit input channels, one of which is connected to an
> +I2C-connected TVP5147 decoder:
> +
> + vpif_capture: video-capture@0x00217000 {

Drop the 0x00.

> + compatible = "ti,da850-vpif-capture";
> + reg = <0x00217000 0x1000>;
> + interrupts = <92>;
> +
> + port {
> + vpif_ch0: endpoint@0 {
> +   reg = <0>;

This is missing #size-cells and #addr-cells.

> +   bus-width = <8>;
> +   remote-endpoint = <>;
> + };
> +
> + vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. 
AIUI, having multiple endpoints is for cases like a mux or 1 to many 
connections. There's only one data flow, but multiple sources or sinks.

> +   reg = <1>;
> +   bus-width = <8>;
> +   data-shift = <8>;
> + };
> + };
> + };
> +
> +[ ... ]
> +
> + {
> +
> + tvp5147@5d {
> + compatible = "ti,tvp5147";
> + reg = <0x5d>;
> + status = "okay";
> +
> + port {
> + composite: endpoint {
> + hsync-active = <1>;
> + vsync-active = <1>;
> + pclk-sample = <0>;
> +
> + /* VPIF channel 0 (lower 8-bits) */
> + remote-endpoint = <_ch0>;
> + bus-width = <8>;
> + };
> + };
> + };
> +
> +};
> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt 
> b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> new file mode 100644
> index ..d004e600aabe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> @@ -0,0 +1,8 @@
> +Texas Instruments VPIF
> +--
> +
> +The Video Port InterFace (VPIF) is the core component for video output
> +and capture on DA850 TI Davinci SoCs.
> +
> +- compatible: must be "ti,da850-vpif"
> +- reg: physical base address and length of the registers set for the device;

That's it? How does this block relate to the capture block?

Rob
--
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: Enabling peer to peer device transactions for PCIe devices

[Logan Gunthorpe]

On 28/11/16 12:35 PM, Serguei Sagalovitch wrote:
> As soon as PeerDirect mapping is called then GPU must not "move" the
> such memory.  It is by PeerDirect design. It is similar how it is works
> with system memory and RDMA MR: when "get_user_pages" is called then the
> memory is pinned.

We haven't touch this in a long time and perhaps it changed, but there
definitely was a call back in the PeerDirect API to allow the GPU to
invalidate the mapping. That's what we don't want.

Logan

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

Re: [PATCH v4l-utils v7 1/7] mediactl: Add support for v4l2-ctrl-binding config

[Jacek Anaszewski]

Hi Sakari,

On 11/24/2016 03:23 PM, Sakari Ailus wrote:

Hi Jacek,

On Wed, Oct 12, 2016 at 04:35:16PM +0200, Jacek Anaszewski wrote:

Make struct v4l2_subdev capable of aggregating v4l2-ctrl-bindings -
media device configuration entries. Added are also functions for
validating support for the control on given media entity and checking
whether a v4l2-ctrl-binding has been defined for a media entity.


I still don't think this belongs here.

I think I told you about the generic pipeline configuration library I worked
on years ago; unfortunately it was left on prototype stage. Still, what I
realised was that something very similar is needed in that library ---
associating information to the representation of the media entities (or the
V4L2 sub-devices) in user space that has got nothing to do with the devices
themselves.

We could have e.g. a list of key--value pairs where the key is a pointer
provided by the user (i.e. application, another library) that could be
associated with the value. That would avoid having explicit information on
that in the struct media_entity itself.

An quicker alternative would be to manage a list of controls e.g. in the
plugin itself and store the media entity where they're implemented in that
list, with the control value.


We are not interested in media entity -> control value relation but
but media entity -> control id. The value is an arbitrary choice of
userspace. Binding's task is to route the ctrl ioctl to a desired
pipeline entity if more than one supports same control.

Effectively we'd need a list of controls as a keys and entities
as values. The list should be allocated dynamically as it would
make no sense to keep keys for all v4l2 controls if only few bindings
are defined.

Best regards,
Jacek Anaszewski


Cc Laurent as well.



Signed-off-by: Jacek Anaszewski 
Acked-by: Kyungmin Park 
---
 utils/media-ctl/libv4l2subdev.c | 32 
 utils/media-ctl/v4l2subdev.h| 19 +++
 2 files changed, 51 insertions(+)

diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index c3439d7..4f8ee7f 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -50,7 +50,15 @@ int v4l2_subdev_create(struct media_entity *entity)

entity->sd->fd = -1;

+   entity->sd->v4l2_ctrl_bindings = malloc(sizeof(__u32));
+   if (entity->sd->v4l2_ctrl_bindings == NULL)
+   goto err_v4l2_ctrl_bindings_alloc;
+
return 0;
+
+err_v4l2_ctrl_bindings_alloc:
+   free(entity->sd);
+   return -ENOMEM;
 }

 int v4l2_subdev_create_opened(struct media_entity *entity, int fd)
@@ -102,6 +110,7 @@ void v4l2_subdev_close(struct media_entity *entity)
if (entity->sd->fd_owner)
close(entity->sd->fd);

+   free(entity->sd->v4l2_ctrl_bindings);
free(entity->sd);
 }

@@ -884,3 +893,26 @@ const enum v4l2_mbus_pixelcode 
*v4l2_subdev_pixelcode_list(unsigned int *length)

return mbus_codes;
 }
+
+int v4l2_subdev_supports_v4l2_ctrl(struct media_device *media,
+  struct media_entity *entity,
+  __u32 ctrl_id)
+{
+   struct v4l2_queryctrl queryctrl = {};
+   int ret;
+
+   ret = v4l2_subdev_open(entity);
+   if (ret < 0)
+   return ret;
+
+   queryctrl.id = ctrl_id;
+
+   ret = ioctl(entity->sd->fd, VIDIOC_QUERYCTRL, );
+   if (ret < 0)
+   return ret;
+
+   media_dbg(media, "Validated control \"%s\" (0x%8.8x) on entity %s\n",
+ queryctrl.name, queryctrl.id, entity->info.name);
+
+   return 0;
+}
diff --git a/utils/media-ctl/v4l2subdev.h b/utils/media-ctl/v4l2subdev.h
index 011fab1..4dee6b1 100644
--- a/utils/media-ctl/v4l2subdev.h
+++ b/utils/media-ctl/v4l2subdev.h
@@ -26,10 +26,14 @@

 struct media_device;
 struct media_entity;
+struct media_device;

 struct v4l2_subdev {
int fd;
unsigned int fd_owner:1;
+
+   __u32 *v4l2_ctrl_bindings;
+   unsigned int num_v4l2_ctrl_bindings;
 };

 /**
@@ -314,4 +318,19 @@ enum v4l2_field v4l2_subdev_string_to_field(const char 
*string);
 const enum v4l2_mbus_pixelcode *v4l2_subdev_pixelcode_list(
unsigned int *length);

+/**
+ * @brief Check if sub-device supports given v4l2 control
+ * @param media - media device.
+ * @param entity - media entity.
+ * @param ctrl_id - id of the v4l2 control to check.
+ *
+ * Verify if the sub-device associated with given media entity
+ * supports v4l2-control with given ctrl_id.
+ *
+ * @return 1 if the control is supported, 0 otherwise.
+ */
+int v4l2_subdev_supports_v4l2_ctrl(struct media_device *device,
+  struct media_entity *entity,
+  __u32 ctrl_id);
+
 #endif



--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to 

Re: Enabling peer to peer device transactions for PCIe devices

[Haggai Eran]

On Mon, 2016-11-28 at 09:57 -0700, Jason Gunthorpe wrote:
> On Sun, Nov 27, 2016 at 04:02:16PM +0200, Haggai Eran wrote:
> > I think blocking mmu notifiers against something that is basically
> > controlled by user-space can be problematic. This can block things
> > like
> > memory reclaim. If you have user-space access to the device's
> > queues,
> > user-space can block the mmu notifier forever.
> Right, I mentioned that..
Sorry, I must have missed it.

> > On PeerDirect, we have some kind of a middle-ground solution for
> > pinning
> > GPU memory. We create a non-ODP MR pointing to VRAM but rely on
> > user-space and the GPU not to migrate it. If they do, the MR gets
> > destroyed immediately.
> That sounds horrible. How can that possibly work? What if the MR is
> being used when the GPU decides to migrate? 
Naturally this doesn't support migration. The GPU is expected to pin
these pages as long as the MR lives. The MR invalidation is done only as
a last resort to keep system correctness.

I think it is similar to how non-ODP MRs rely on user-space today to
keep them correct. If you do something like madvise(MADV_DONTNEED) on a
non-ODP MR's pages, you can still get yourself into a data corruption
situation (HCA sees one page and the process sees another for the same
virtual address). The pinning that we use only guarentees the HCA's page
won't be reused.

> I would not support that
> upstream without a lot more explanation..
> 
> I know people don't like requiring new hardware, but in this case we
> really do need ODP hardware to get all the semantics people want..
> 
> > 
> > Another thing I think is that while HMM is good for user-space
> > applications, for kernel p2p use there is no need for that. Using
> From what I understand we are not really talking about kernel p2p,
> everything proposed so far is being mediated by a userspace VMA, so
> I'd focus on making that work.
Fair enough, although we will need both eventually, and I hope the
infrastructure can be shared to some degree.--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] Samsung fixes for 4.8

[Mauro Carvalho Chehab]

Em Mon, 28 Nov 2016 16:29:25 -0300
Javier Martinez Canillas  escreveu:

> Hello Mauro,
> 
> On 11/16/2016 12:19 PM, Mauro Carvalho Chehab wrote:
> > Em Wed, 16 Nov 2016 16:08:19 +0100
> > Sylwester Nawrocki  escreveu:
> >   
> >> On 11/16/2016 03:46 PM, Mauro Carvalho Chehab wrote:  
> >> Marek Szyprowski (1):
> >   s5p-mfc: fix failure path of s5p_mfc_alloc_memdev()  
> >
> > Mauro, this patch seems to had slipped through the cracks, I can't see 
> > it
> > in neither media fixes nor the master branch. Could you please check 
> > it?
> >>>
> >>> The patch seems to be on my tree:
> >>  
> 
> This patch is indeed in your tree as commit:
> 
> https://git.linuxtv.org/media_tree.git/commit/?id=3467c9a7e7f9
> 
> and also in present in the media/v4.9-2 tag.
> 
> But the patch never made to mainline. In fact, I don't see any of the
> patches in the media/v4.9-2 tag to be merged in v4.9-rc7.

It is part of a group of 187 patches for Kernel 4.10. There aren't
anything there that fixes a regression on 4.8 or 4.9. So, no
hush.

I double-checked and all the patches at media/v4.9-2 are there at
media master. So, should be sent to 4.10.

Regards,
Mauro
--
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: Enabling peer to peer device transactions for PCIe devices

[Serguei Sagalovitch]

On 2016-11-28 01:20 PM, Logan Gunthorpe wrote:


On 28/11/16 09:57 AM, Jason Gunthorpe wrote:

On PeerDirect, we have some kind of a middle-ground solution for pinning
GPU memory. We create a non-ODP MR pointing to VRAM but rely on
user-space and the GPU not to migrate it. If they do, the MR gets
destroyed immediately.

That sounds horrible. How can that possibly work? What if the MR is
being used when the GPU decides to migrate? I would not support that
upstream without a lot more explanation..

Yup, this was our experience when playing around with PeerDirect. There
was nothing we could do if the GPU decided to invalidate the P2P
mapping.

As soon as PeerDirect mapping is called then GPU must not "move" the
such memory.  It is by PeerDirect design. It is similar how it is works
with system memory and RDMA MR: when "get_user_pages" is called then the
memory is pinned.

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

Re: [GIT PULL] Samsung fixes for 4.8

[Javier Martinez Canillas]

Hello Mauro,

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

This patch is indeed in your tree as commit:

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

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

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

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

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

Re: Enabling peer to peer device transactions for PCIe devices

[Jason Gunthorpe]

On Mon, Nov 28, 2016 at 06:19:40PM +, Haggai Eran wrote:
> > > GPU memory. We create a non-ODP MR pointing to VRAM but rely on
> > > user-space and the GPU not to migrate it. If they do, the MR gets
> > > destroyed immediately.
> > That sounds horrible. How can that possibly work? What if the MR is
> > being used when the GPU decides to migrate? 
> Naturally this doesn't support migration. The GPU is expected to pin
> these pages as long as the MR lives. The MR invalidation is done only as
> a last resort to keep system correctness.

That just forces applications to handle horrible unexpected
failures. If this sort of thing is needed for correctness then OOM
kill the offending process, don't corrupt its operation.

> I think it is similar to how non-ODP MRs rely on user-space today to
> keep them correct. If you do something like madvise(MADV_DONTNEED) on a
> non-ODP MR's pages, you can still get yourself into a data corruption
> situation (HCA sees one page and the process sees another for the same
> virtual address). The pinning that we use only guarentees the HCA's page
> won't be reused.

That is not really data corruption - the data still goes where it was
originally destined. That is an application violating the
requirements of a MR. An application cannot munmap/mremap a VMA
while a non ODP MR points to it and then keep using the MR.

That is totally different from a GPU driver wanthing to mess with
translation to physical pages.

> > From what I understand we are not really talking about kernel p2p,
> > everything proposed so far is being mediated by a userspace VMA, so
> > I'd focus on making that work.

> Fair enough, although we will need both eventually, and I hope the
> infrastructure can be shared to some degree.

What use case do you see for in kernel?

Presumably in-kernel could use a vmap or something and the same basic
flow?

Jason
--
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: Enabling peer to peer device transactions for PCIe devices

[Haggai Eran]

On Mon, 2016-11-28 at 09:48 -0500, Serguei Sagalovitch wrote:
> On 2016-11-27 09:02 AM, Haggai Eran wrote
> > 
> > On PeerDirect, we have some kind of a middle-ground solution for
> > pinning
> > GPU memory. We create a non-ODP MR pointing to VRAM but rely on
> > user-space and the GPU not to migrate it. If they do, the MR gets
> > destroyed immediately. This should work on legacy devices without
> > ODP
> > support, and allows the system to safely terminate a process that
> > misbehaves. The downside of course is that it cannot transparently
> > migrate memory but I think for user-space RDMA doing that
> > transparently
> > requires hardware support for paging, via something like HMM.
> > 
> > ...
> May be I am wrong but my understanding is that PeerDirect logic
> basically
> follow  "RDMA register MR" logic 
Yes. The only difference from regular MRs is the invalidation process I
mentioned, and the fact that we get the addresses not from
get_user_pages but from a peer driver.

> so basically nothing prevent to "terminate"
> process for "MMU notifier" case when we are very low on memory
> not making it similar (not worse) then PeerDirect case.
I'm not sure I understand. I don't think any solution prevents
terminating an application. The paragraph above is just trying to
explain how a non-ODP device/MR can handle an invalidation.

> > > I'm hearing most people say ZONE_DEVICE is the way to handle this,
> > > which means the missing remaing piece for RDMA is some kind of DMA
> > > core support for p2p address translation..
> > Yes, this is definitely something we need. I think Will Davis's
> > patches
> > are a good start.
> > 
> > Another thing I think is that while HMM is good for user-space
> > applications, for kernel p2p use there is no need for that.
> About HMM: I do not think that in the current form HMM would  fit in
> requirement for generic P2P transfer case. My understanding is that at
> the current stage HMM is good for "caching" system memory
> in device memory for fast GPU access but in RDMA MR non-ODP case
> it will not work because  the location of memory should not be
> changed so memory should be allocated directly in PCIe memory.
The way I see it there are two ways to handle non-ODP MRs. Either you
prevent the GPU from migrating / reusing the MR's VRAM pages for as long
as the MR is alive (if I understand correctly you didn't like this
solution), or you allow the GPU to somehow notify the HCA to invalidate
the MR. If you do that, you can use mmu notifiers or HMM or something
else, but HMM provides a nice framework to facilitate that notification.

> > 
> > Using ZONE_DEVICE with or without something like DMA-BUF to pin and
> > unpin
> > pages for the short duration as you wrote above could work fine for
> > kernel uses in which we can guarantee they are short.
> Potentially there is another issue related to pin/unpin. If memory
> could
> be used a lot of time then there is no sense to rebuild and program
> s/g tables each time if location of memory was not changed.
Is this about the kernel use or user-space? In user-space I think the MR
concept captures a long-lived s/g table so you don't need to rebuild it
(unless the mapping changes).

Haggai--
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: Enabling peer to peer device transactions for PCIe devices

[Logan Gunthorpe]

On 28/11/16 09:57 AM, Jason Gunthorpe wrote:
>> On PeerDirect, we have some kind of a middle-ground solution for pinning
>> GPU memory. We create a non-ODP MR pointing to VRAM but rely on
>> user-space and the GPU not to migrate it. If they do, the MR gets
>> destroyed immediately.
> 
> That sounds horrible. How can that possibly work? What if the MR is
> being used when the GPU decides to migrate? I would not support that
> upstream without a lot more explanation..

Yup, this was our experience when playing around with PeerDirect. There
was nothing we could do if the GPU decided to invalidate the P2P
mapping. It just meant the application would fail or need complicated
logic to detect this and redo just about everything. And given that it
was a reasonably rare occurrence during development it probably means
not a lot of applications will be developed to handle it and most would
end up being randomly broken in environments with memory pressure.

Logan

--
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: Enabling peer to peer device transactions for PCIe devices

[Jason Gunthorpe]

On Sun, Nov 27, 2016 at 04:02:16PM +0200, Haggai Eran wrote:

> > Like in ODP, MMU notifiers/HMM are used to monitor for translation
> > changes. If a change comes in the GPU driver checks if an executing
> > command is touching those pages and blocks the MMU notifier until the
> > command flushes, then unfaults the page (blocking future commands) and
> > unblocks the mmu notifier.

> I think blocking mmu notifiers against something that is basically
> controlled by user-space can be problematic. This can block things like
> memory reclaim. If you have user-space access to the device's queues,
> user-space can block the mmu notifier forever.

Right, I mentioned that..

> On PeerDirect, we have some kind of a middle-ground solution for pinning
> GPU memory. We create a non-ODP MR pointing to VRAM but rely on
> user-space and the GPU not to migrate it. If they do, the MR gets
> destroyed immediately.

That sounds horrible. How can that possibly work? What if the MR is
being used when the GPU decides to migrate? I would not support that
upstream without a lot more explanation..

I know people don't like requiring new hardware, but in this case we
really do need ODP hardware to get all the semantics people want..

> Another thing I think is that while HMM is good for user-space
> applications, for kernel p2p use there is no need for that. Using

>From what I understand we are not really talking about kernel p2p,
everything proposed so far is being mediated by a userspace VMA, so
I'd focus on making that work.

Jason
--
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] media: ti-vpe: vpdma: fix a timeout loop

[Benoit Parrot]

Dan,

Thanks for the patch.

Acked-by: Benoit Parrot 

Dan Carpenter  wrote on Sat [2016-Nov-26 00:28:34 
+0300]:
> The check assumes that we end on zero but actually we end on -1.  Change
> the post-op to a pre-op so that we do end on zero.  Techinically now we
> only loop 499 times instead of 500 but that's fine.
> 
> Fixes: dc12b124353b ("[media] media: ti-vpe: vpdma: Add abort channel desc 
> and cleanup APIs")
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/media/platform/ti-vpe/vpdma.c 
> b/drivers/media/platform/ti-vpe/vpdma.c
> index 13bfd71..23472e3 100644
> --- a/drivers/media/platform/ti-vpe/vpdma.c
> +++ b/drivers/media/platform/ti-vpe/vpdma.c
> @@ -453,7 +453,7 @@ int vpdma_list_cleanup(struct vpdma_data *vpdma, int 
> list_num,
>   if (ret)
>   return ret;
>  
> - while (vpdma_list_busy(vpdma, list_num) && timeout--)
> + while (vpdma_list_busy(vpdma, list_num) && --timeout)
>   ;
>  
>   if (timeout == 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 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Alan Stern]

On Mon, 28 Nov 2016, Laurent Pinchart wrote:

> > Well, I admit it would be nicer if drivers didn't have to worry about
> > whether or not CONFIG_PM was enabled.  A slightly cleaner approach
> > from the one outlined above would have the probe routine do this:
> > 
> > my_power_up(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_get_noresume(dev);
> > pm_runtime_enable(dev);
> > 
> > and have the runtime-resume callback routine call my_power_up() to do
> > its work.  (Or make my_power_up() actually be the runtime-resume
> > callback routine.)  That's pretty straightforward and hard to mess up.
> 
> You'd be surprised how easy drivers can mess simple things up ;-)

No -- I wouldn't!  :-)

> We'd still 
> have to get the message out there, that would be the most difficult part.

Agreed.

> > In theory, we could have pm_runtime_enable() invoke the runtime-resume
> > callback when CONFIG_PM is disabled.  In practice, it would be rather
> > awkward.  drivers/base/power/runtime.c, which is where
> > pm_runtime_enable() is defined and the runtime-PM callbacks are
> > invoked, doesn't even get compiled if CONFIG_PM is off.
> 
> Sure, but that can easily be fixed.
> 
> > (Also, it would run against the grain.  CONFIG_PM=n means the kernel
> > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.)
> 
> I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't 
> perform 
> runtime PM, not that it should do absolutely nothing. If semantics is the 
> biggest concern, we could introduce a helper (whose name is TBD) that would 
> enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n

Or have the driver call _both_ the helper routine and
pm_runtime_enable() -- the helper would do nothing if CONFIG_PM=y, and
it would invoke the runtime-resume callback if CONFIG_PM=n.

Either way would be a good approach.  Having pm_runtime_enable() call
the runtime-resume handler wouldn't work well if the driver has already
powered-up the device or the device starts out in the power-on state
(which is often the case).

> I want to make it as easy as possible for drivers to make sure they won't get 
> this wrong, which in my opinion requires a simple and straightforward API 
> with 
> no code in the driver that would depend on the value of CONFIG_PM.

Well, the approach I outlined above is pretty simple and it doesn't
depend on the value of CONFIG_PM.

Your proposal is just as simple, but it does require drivers to 
remember to call the new helper routine.

> > There's a corollary aspect to this.  If you depend on runtime PM for
> > powering up your device during probe, does that mean you also depend on
> > runtime PM for powering down the device during remove?  That is likely
> > not to work, because the user can prevent runtime suspends by writing
> > to /sys/.../power/control.
> 
> Yes, I do, and I expect most runtime PM-enabled driver to do the same. When 
> runtime suspend is disabled through /sys/.../power/control does 
> pm_runtime_disable() invoke the runtime PM suspend handler if the device is 
> powered on ?

No, it doesn't, and neither does pm_runtime_put().  After all, if the
user has told the system not to do runtime PM on that device, it
doesn't make sense to call the runtime-suspend handler.  But you can
always blame the user when this happens.  :-)

Alan Stern

--
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: Enabling peer to peer device transactions for PCIe devices

[Serguei Sagalovitch]

On 2016-11-27 09:02 AM, Haggai Eran wrote

On PeerDirect, we have some kind of a middle-ground solution for pinning
GPU memory. We create a non-ODP MR pointing to VRAM but rely on
user-space and the GPU not to migrate it. If they do, the MR gets
destroyed immediately. This should work on legacy devices without ODP
support, and allows the system to safely terminate a process that
misbehaves. The downside of course is that it cannot transparently
migrate memory but I think for user-space RDMA doing that transparently
requires hardware support for paging, via something like HMM.

...

May be I am wrong but my understanding is that PeerDirect logic basically
follow  "RDMA register MR" logic so basically nothing prevent to "terminate"
process for "MMU notifier" case when we are very low on memory
not making it similar (not worse) then PeerDirect case.

I'm hearing most people say ZONE_DEVICE is the way to handle this,
which means the missing remaing piece for RDMA is some kind of DMA
core support for p2p address translation..

Yes, this is definitely something we need. I think Will Davis's patches
are a good start.

Another thing I think is that while HMM is good for user-space
applications, for kernel p2p use there is no need for that.

About HMM: I do not think that in the current form HMM would  fit in
requirement for generic P2P transfer case. My understanding is that at
the current stage HMM is good for "caching" system memory
in device memory for fast GPU access but in RDMA MR non-ODP case
it will not work because  the location of memory should not be
changed so memory should be allocated directly in PCIe memory.

Using ZONE_DEVICE with or without something like DMA-BUF to pin and unpin
pages for the short duration as you wrote above could work fine for
kernel uses in which we can guarantee they are short.

Potentially there is another issue related to pin/unpin. If memory could
be used a lot of time then there is no sense to rebuild and program
s/g tables each time if location of memory was not changed.


--
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] uvcvideo: freeing an error pointer

[Laurent Pinchart]

Hi Julia and Dan,

On Monday 28 Nov 2016 14:54:58 Julia Lawall wrote:
> On Mon, 28 Nov 2016, Dan Carpenter wrote:
> > I understand the comparison, but I just think it's better if people
> > always keep track of what has been allocated and what has not.  I tried
> > so hard to get Markus to stop sending those hundreds of patches where
> > he's like "this function has a sanity check so we can pass pointers
> > that weren't allocated"...  It's garbage code.
> > 
> > But I understand that other people don't agree.
> 
> In my opinion, it is good for code understanding to only do what is useful
> to do.  It's not a hard and fast rule, but I think it is something to take
> into account.

On the other hand it complicates the error handling code by increasing the 
number of goto labels, and it then becomes pretty easy when reworking code to 
goto the wrong label. This is even more of an issue when the rework doesn't 
touch the error handling code, in which case the reviewers can easily miss the 
issue if they don't open the original source file to check the goto labels.

-- 
Regards,

Laurent Pinchart

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

3A / auto-exposure Region of Interest setting

[Guennadi Liakhovetski]

Hi,

Has anyone already considered supporting 3A (e.g. auto-exposure) Region of 
Interest selection? In UVC this is the "Digital Region of Interest (ROI) 
Control." Android defines ANDROID_CONTROL_AE_REGIONS, 
ANDROID_CONTROL_AWB_REGIONS, ANDROID_CONTROL_AF_REGIONS. The UVC control 
defines just a single rectangle for all (supported) 3A functions. That 
could be implemented, defining a new selection target. However, Android 
allows arbitrary numbers of ROI rectangles with associated weights. Any 
ideas?

Thanks
Guennadi
--
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] uvcvideo: freeing an error pointer

[Julia Lawall]

On Mon, 28 Nov 2016, Dan Carpenter wrote:

> I understand the comparison, but I just think it's better if people
> always keep track of what has been allocated and what has not.  I tried
> so hard to get Markus to stop sending those hundreds of patches where
> he's like "this function has a sanity check so we can pass pointers
> that weren't allocated"...  It's garbage code.
>
> But I understand that other people don't agree.

In my opinion, it is good for code understanding to only do what is useful
to do.  It's not a hard and fast rule, but I think it is something to take
into account.

julia

>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 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] [media] uvcvideo: freeing an error pointer

[Dan Carpenter]

I understand the comparison, but I just think it's better if people
always keep track of what has been allocated and what has not.  I tried
so hard to get Markus to stop sending those hundreds of patches where
he's like "this function has a sanity check so we can pass pointers
that weren't allocated"...  It's garbage code.

But I understand that other people don't agree.

regards,
dan carpenter

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

[GIT PULL] Samsung SoC related updates

[Sylwester Nawrocki]

Mauro,

this change set adds support for the Exynos5433 SoC variant 
of the MFC subsystem, it also includes related clean up 
and fixes/improvements.

The following changes since commit 36f94a5cf0f9afb527f18166ae56bd3cc7204f63:

  Merge tag 'v4.9-rc5' into patchwork (2016-11-16 16:42:27 -0200)

are available in the git repository at:


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

for you to fetch changes up to d9f2586c6c302d4db39c5aa92b803dcd30b06f4e:

  s5p-mfc: Add support for MFC v8 available in Exynos 5433 SoCs (2016-11-17 
12:11:26 +0100)


Douglas Anderson (1):
  s5p-mfc: Set DMA_ATTR_ALLOC_SINGLE_PAGES

Marek Szyprowski (8):
  s5p-mfc: Use printk_ratelimited for reporting ioctl errors
  s5p-mfc: Remove special clock rate management
  s5p-mfc: Ensure that clock is disabled before turning power off
  s5p-mfc: Remove dead conditional code
  s5p-mfc: Kill all IS_ERR_OR_NULL in clocks management code
  s5p-mfc: Don't keep clock prepared all the time
  s5p-mfc: Rework clock handling
  s5p-mfc: Add support for MFC v8 available in Exynos 5433 SoCs

 .../devicetree/bindings/media/s5p-mfc.txt |   1 +
 drivers/media/platform/s5p-mfc/s5p_mfc.c  |  61 +---
 .../media/platform/s5p-mfc/s5p_mfc_common.h   |  10 +-
 .../media/platform/s5p-mfc/s5p_mfc_debug.h|   6 +
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c  |   2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c  |   2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c   | 139 ++---
 7 files changed, 103 insertions(+), 118 deletions(-)

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

Re: [RFC v3 00/21] Make use of kref in media device, grab references as needed

[Sakari Ailus]

Hi Mauro,

On Tue, Nov 22, 2016 at 03:44:29PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 14 Nov 2016 15:27:22 +0200
> Sakari Ailus  escreveu:
> 
> > Hi Mauro,
> > 
> > I'm replying below but let me first summarise the remaining problem area
> > that this patchset addresses.
> 
> Sorry for answering too late. Somehow, I missed this email in the cloud.
> 
> > The problems you and Shuah have seen and partially addressed are related to
> > a larger picture which is the lifetime of (mostly) memory resources related
> > to various objects used by as well both the Media controller and V4L2
> > frameworks (including videobuf2) as the drivers which make use of these
> > frameworks.
> > 
> > The Media controller and V4L2 interfaces exposed by drivers consist of
> > multiple devices nodes, data structures with interdependencies within the
> > frameworks themselves and dependencies from the driver's own data structures
> > towards the framework data structures. The Media device and the media graph
> > objects are central to the problem area as well.
> > 
> > So what are the issues then? Until now, we've attempted to regulate the
> > users' ability to access the devices at the time they're being unregistered
> > (and the associated memory released), but that approach does not really
> > scale: you have to make sure that the unregistering also will not take place
> > _during_ the system call --- not just in the beginning of it.
> >
> > The media graph contains media graph objects, some of which are media
> > entities (contained in struct video_device or struct v4l2_subdev, for
> > instance). Media entities as graph nodes have links to other entities. In
> > order to implement the system calls, the drivers do parse this graph in
> > order to obtain information they need to obtain from it. For instance, it's
> > not uncommon for an implementation for video node format enumeration to
> > figure out which sub-device the link from that video nodes leads to. Drivers
> > may also have similar paths they follow.
> > 
> > Interrupt handling may also be taking place during the device removal during
> > which a number of data structures are now freed. This really does call for a
> > solution based on reference counting.
> > 
> > This leads to the conclusion that all the memory resources that could be
> > accessed by the drivers or the kernel frameworks must stay intact until the
> > last file handle to the said devices is closed. Otherwise, there is a
> > possibility of accessing released memory.
> 
> So far, we're aligned.
> 
> > Right now in a lot of the cases, such as for video device and sub-device
> > nodes, we do release the memory when a device (as in struct device) is being
> > unregistered. There simply is in the current mainline kernel a way to do
> > this in a safe way.
> 
> > Drivers do use devm_() family of functions to allocate
> > the memory of the media graph object and their internal data structures.
> 
> Removing devm_() from those drivers seem to be the first thing to do,
> and it is independent from any MC rework.
> 
> As you'll see below, we have different opinions on other matters,
> so, my suggestion about how to proceed is that you should submit
> first the things we're aligned.
> 
> In other words, please submit the patches that get rid of devm_()
> first. Then, we can address the remaining stuff.

Removing devm_*() is needed, but when should the memory be released then?
There's no callback currently from the media device the driver could use.

OTOH devm_*() interfaces are very convenient to use, it's a lot of extra
work for drivers to handle releasing all the resources. It'd be great to
find another object where to bind those resources. Still, device_release()
does first release devres resources and then calls the release() callback,
which obviously makes the setup problematic to begin with.

> 
> > 
> > With this patchset:
> > 
> > - The media_device which again contains the media_devnode is allocated
> >   dynamically. The lifetime of the media device --- and the media graph
> >   objects it contains --- is bound to device nodes that are bound to the
> >   media device (video and sub-device nodes) as well as open file handles.
> 
> No. Data structures with cdev embedded into them have their lifetime
> controlled by the driver's core, and are destroyed only when there's
> no pending fops. The current approach uses device's core dev.release()

Fair enough; that part is indeed handled towards the user space as far as I
can tell. However that's still not enough: the media graph contains the
graph objects, and the media device that holds the graph, must outlive the
graph objects themselves.

Also removing entities doesn't really work currently: touching an entity, a
link or any kind of a graph object is not guaranteed to work unless you hold
the media graph lock. And that's simply unfeasible. Just look at what the
drivers do with entities: they use the v4l2_subdev interface and the 

[PATCH v3 3/3] ARM: multi_v7_defconfig: enable STMicroelectronics HVA debugfs

[Jean-Christophe Trotin]

Signed-off-by: Jean-Christophe Trotin 
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index eb14ab6..7a15107 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -563,6 +563,7 @@ CONFIG_VIDEO_SAMSUNG_S5P_JPEG=m
 CONFIG_VIDEO_SAMSUNG_S5P_MFC=m
 CONFIG_VIDEO_STI_BDISP=m
 CONFIG_VIDEO_STI_HVA=m
+CONFIG_VIDEO_STI_HVA_DEBUGFS=y
 CONFIG_DYNAMIC_DEBUG=y
 CONFIG_VIDEO_RENESAS_JPU=m
 CONFIG_VIDEO_RENESAS_VSP1=m
-- 
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 v3 1/3] st-hva: encoding summary at instance release

[Jean-Christophe Trotin]

This patch prints unconditionnaly a short summary about the encoding
operation at each instance closing, for debug purpose:
- information about the frame (format, resolution)
- information about the stream (format, profile, level, resolution)
- number of encoded frames
- potential (system, encoding...) errors

Signed-off-by: Yannick Fertre 
Signed-off-by: Jean-Christophe Trotin 
---
 drivers/media/platform/sti/hva/hva-h264.c |  6 
 drivers/media/platform/sti/hva/hva-hw.c   |  5 
 drivers/media/platform/sti/hva/hva-mem.c  |  5 +++-
 drivers/media/platform/sti/hva/hva-v4l2.c | 49 ---
 drivers/media/platform/sti/hva/hva.h  |  8 +
 5 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/sti/hva/hva-h264.c 
b/drivers/media/platform/sti/hva/hva-h264.c
index 8cc8467..e6f247a 100644
--- a/drivers/media/platform/sti/hva/hva-h264.c
+++ b/drivers/media/platform/sti/hva/hva-h264.c
@@ -607,6 +607,7 @@ static int hva_h264_prepare_task(struct hva_ctx *pctx,
"%s   width(%d) or height(%d) exceeds limits (%dx%d)\n",
pctx->name, frame_width, frame_height,
H264_MAX_SIZE_W, H264_MAX_SIZE_H);
+   pctx->frame_errors++;
return -EINVAL;
}
 
@@ -717,6 +718,7 @@ static int hva_h264_prepare_task(struct hva_ctx *pctx,
default:
dev_err(dev, "%s   invalid source pixel format\n",
pctx->name);
+   pctx->frame_errors++;
return -EINVAL;
}
 
@@ -741,6 +743,7 @@ static int hva_h264_prepare_task(struct hva_ctx *pctx,
 
if (td->framerate_den == 0) {
dev_err(dev, "%s   invalid framerate\n", pctx->name);
+   pctx->frame_errors++;
return -EINVAL;
}
 
@@ -831,6 +834,7 @@ static int hva_h264_prepare_task(struct hva_ctx *pctx,
(payload > MAX_SPS_PPS_SIZE)) {
dev_err(dev, "%s   invalid sps/pps size %d\n", pctx->name,
payload);
+   pctx->frame_errors++;
return -EINVAL;
}
 
@@ -842,6 +846,7 @@ static int hva_h264_prepare_task(struct hva_ctx *pctx,
   (u8 *)stream->vaddr,
   )) {
dev_err(dev, "%s   fail to get SEI nal\n", pctx->name);
+   pctx->frame_errors++;
return -EINVAL;
}
 
@@ -963,6 +968,7 @@ static int hva_h264_open(struct hva_ctx *pctx)
 err_ctx:
devm_kfree(dev, ctx);
 err:
+   pctx->sys_errors++;
return ret;
 }
 
diff --git a/drivers/media/platform/sti/hva/hva-hw.c 
b/drivers/media/platform/sti/hva/hva-hw.c
index 68d625b..5104068 100644
--- a/drivers/media/platform/sti/hva/hva-hw.c
+++ b/drivers/media/platform/sti/hva/hva-hw.c
@@ -470,6 +470,7 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum 
hva_hw_cmd_type cmd,
 
if (pm_runtime_get_sync(dev) < 0) {
dev_err(dev, "%s failed to get pm_runtime\n", ctx->name);
+   ctx->sys_errors++;
ret = -EFAULT;
goto out;
}
@@ -481,6 +482,7 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum 
hva_hw_cmd_type cmd,
break;
default:
dev_dbg(dev, "%s unknown command 0x%x\n", ctx->name, cmd);
+   ctx->encode_errors++;
ret = -EFAULT;
goto out;
}
@@ -511,6 +513,7 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum 
hva_hw_cmd_type cmd,
 msecs_to_jiffies(2000))) {
dev_err(dev, "%s %s: time out on completion\n", ctx->name,
__func__);
+   ctx->encode_errors++;
ret = -EFAULT;
goto out;
}
@@ -518,6 +521,8 @@ int hva_hw_execute_task(struct hva_ctx *ctx, enum 
hva_hw_cmd_type cmd,
/* get encoding status */
ret = ctx->hw_err ? -EFAULT : 0;
 
+   ctx->encode_errors += ctx->hw_err ? 1 : 0;
+
 out:
disable_irq(hva->irq_its);
disable_irq(hva->irq_err);
diff --git a/drivers/media/platform/sti/hva/hva-mem.c 
b/drivers/media/platform/sti/hva/hva-mem.c
index 649f660..821c78e 100644
--- a/drivers/media/platform/sti/hva/hva-mem.c
+++ b/drivers/media/platform/sti/hva/hva-mem.c
@@ -17,14 +17,17 @@ int hva_mem_alloc(struct hva_ctx *ctx, u32 size, const char 
*name,
void *base;
 
b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
-   if (!b)
+   if (!b) {
+   ctx->sys_errors++;
return -ENOMEM;
+   }
 
base = dma_alloc_attrs(dev, size, , GFP_KERNEL | GFP_DMA,
   DMA_ATTR_WRITE_COMBINE);
if (!base) {
dev_err(dev, "%s %s : dma_alloc_attrs failed for %s 

[PATCH v3 0/3] add debug capabilities to v4l2 encoder for STMicroelectronics SOC

[Jean-Christophe Trotin]

version 3:
- the encoding summary (first patch) is moved from hva-debug.c to hva-v4l2.c.
  As suggested by Hans, dev_info() is used instead of snprintf() in the
  hva_dbg_summary() function.

- About the debugfs entries for HVA (second patch), a driver-specific Kconfig
  option (VIDEO_STI_HVA_DEBUGFS) is added as suggested by Hans. This option
  depends both on VIDEO_STI_HVA and on DEBUG_FS.

- The third (new) patch enables STMicroelectronics HVA debugfs in
  multi_v7_defconfig (VIDEO_STI_HVA_DEBUGFS).

version 2:
- the encoding summary (first patch) doesn't include any longer information
  about the encoding performance. Thus, after each frame encoding, only one or
  two variables are increased (number of encoded frames, number of encoding
  errors), but no computation is executed (as it was in version 1). When the
  encoding instance is closed, the short summary that is printed (dev_info),
  also doesn't require any computation, and gives a useful brief status about
  the last operation: that are the reasons why there's no Kconfig option to
  explicitly enable this summary.

- the second patch enables the computation of the performances 
(hva_dbg_perf_begin
  and hva_dbg_perf_end) only if DEBUG_FS is enabled. The functions that
  create or remove the debugfs entries (hva_debugfs_create,
  hva_debugfs_remove, hva_dbg_ctx_create, hva_dbg_ctx_remove) are not under
  CONFIG_DEBUG_FS switch: if DEBUG_FS is disabled, the debugfs functions
  (debugfs_create_dir and debugfs_create_file) are available, but no entry is
  created. The "show" operations (hva_dbg_device, hva_dbg_encoders,
  hva_dbg_last, hva_dbg_regs, hva_dbg_ctx) are also not under
  CONFIG_DEBUG_FS switch: if DEBUG_FS is disabled, they will never be called.
  So, with this version 2, no new Kconfig option is introduced, but the
  performance computations and the debugfs entries depend on whether DEBUG_FS
  is enabled or not.

version 1:
- Initial submission

With the first patch, a short summary about the encoding operation is
unconditionnaly printed at each instance closing:
- information about the frame (format, resolution)
- information about the stream (format, profile, level, resolution)
- number of encoded frames
- potential (system, encoding...) errors

With the second patch, 4 static debugfs entries are created to dump:
- the device-related information ("st-hva/device")
- the list of registered encoders ("st-hva/encoders")
- the current values of the hva registers ("st-hva/regs")
- the information about the last closed instance ("st-hva/last")

Moreover, a debugfs entry is dynamically created for each opened instance,
("st-hva/") to dump:
 - the information about the frame (format, resolution)
- the information about the stream (format, profile, level,
  resolution)
- the control parameters (bitrate mode, framerate, GOP size...)
- the potential (system, encoding...) errors
- the performance information about the encoding (HW processing
  duration, average bitrate, average framerate...)
Each time a running instance is closed, its context (including the debug
information) is saved to feed, on demand, the last closed instance debugfs
entry.

These debug capabilities are mainly implemented in the hva-debugfs.c file.

Jean-Christophe Trotin (3):
  st-hva: encoding summary at instance release
  st-hva: add debug file system
  ARM: multi_v7_defconfig: enable STMicroelectronics HVA debugfs

 arch/arm/configs/multi_v7_defconfig  |   1 +
 drivers/media/platform/Kconfig   |  11 +
 drivers/media/platform/sti/hva/Makefile  |   1 +
 drivers/media/platform/sti/hva/hva-debugfs.c | 422 +++
 drivers/media/platform/sti/hva/hva-h264.c|   6 +
 drivers/media/platform/sti/hva/hva-hw.c  |  48 +++
 drivers/media/platform/sti/hva/hva-hw.h  |   3 +
 drivers/media/platform/sti/hva/hva-mem.c |   5 +-
 drivers/media/platform/sti/hva/hva-v4l2.c|  78 -
 drivers/media/platform/sti/hva/hva.h |  96 +-
 10 files changed, 657 insertions(+), 14 deletions(-)
 create mode 100644 drivers/media/platform/sti/hva/hva-debugfs.c

-- 
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 v3 2/3] st-hva: add debug file system

[Jean-Christophe Trotin]

This patch creates 4 static debugfs entries to dump:
- the device-related information ("st-hva/device")
- the list of registered encoders ("st-hva/encoders")
- the current values of the hva registers ("st-hva/regs")
- the information about the last closed instance ("st-hva/last")

It also creates dynamically a debugfs entry for each opened instance,
("st-hva/") to dump:
- the information about the frame (format, resolution)
- the information about the stream (format, profile, level,
  resolution)
- the control parameters (bitrate mode, framerate, GOP size...)
- the potential (system, encoding...) errors
- the performance information about the encoding (HW processing
  duration, average bitrate, average framerate...)
Each time a running instance is closed, its context (including the
debug information) is saved to feed, on demand, the last closed
instance debugfs entry.

Signed-off-by: Yannick Fertre 
Signed-off-by: Jean-Christophe Trotin 
---
 drivers/media/platform/Kconfig   |  11 +
 drivers/media/platform/sti/hva/Makefile  |   1 +
 drivers/media/platform/sti/hva/hva-debugfs.c | 422 +++
 drivers/media/platform/sti/hva/hva-hw.c  |  43 +++
 drivers/media/platform/sti/hva/hva-hw.h  |   3 +
 drivers/media/platform/sti/hva/hva-v4l2.c|  29 +-
 drivers/media/platform/sti/hva/hva.h |  88 +-
 7 files changed, 594 insertions(+), 3 deletions(-)
 create mode 100644 drivers/media/platform/sti/hva/hva-debugfs.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 60e20e5..2bb1a6f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -298,6 +298,17 @@ config VIDEO_STI_HVA
  To compile this driver as a module, choose M here:
  the module will be called st-hva.
 
+config VIDEO_STI_HVA_DEBUGFS
+   bool "Export STMicroelectronics HVA internals in debugfs"
+   depends on VIDEO_STI_HVA
+   depends on DEBUG_FS
+   help
+ Select this to see information about the internal state and the last
+  operation of STMicroelectronics HVA multi-format video encoder in
+  debugfs.
+
+  Choose N unless you know you need this.
+
 config VIDEO_SH_VEU
tristate "SuperH VEU mem2mem video processing driver"
depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA
diff --git a/drivers/media/platform/sti/hva/Makefile 
b/drivers/media/platform/sti/hva/Makefile
index ffb69ce..e3ebe968 100644
--- a/drivers/media/platform/sti/hva/Makefile
+++ b/drivers/media/platform/sti/hva/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_VIDEO_STI_HVA) := st-hva.o
 st-hva-y := hva-v4l2.o hva-hw.o hva-mem.o hva-h264.o
+st-hva-$(CONFIG_VIDEO_STI_HVA_DEBUGFS) += hva-debugfs.o
diff --git a/drivers/media/platform/sti/hva/hva-debugfs.c 
b/drivers/media/platform/sti/hva/hva-debugfs.c
new file mode 100644
index 000..83a6258
--- /dev/null
+++ b/drivers/media/platform/sti/hva/hva-debugfs.c
@@ -0,0 +1,422 @@
+/*
+ * Copyright (C) STMicroelectronics SA 2015
+ * Authors: Yannick Fertre 
+ *  Hugues Fruchet 
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include 
+
+#include "hva.h"
+#include "hva-hw.h"
+
+static void format_ctx(struct seq_file *s, struct hva_ctx *ctx)
+{
+   struct hva_streaminfo *stream = >streaminfo;
+   struct hva_frameinfo *frame = >frameinfo;
+   struct hva_controls *ctrls = >ctrls;
+   struct hva_ctx_dbg *dbg = >dbg;
+   u32 bitrate_mode, aspect, entropy, vui_sar, sei_fp;
+
+   seq_printf(s, "|-%s\n  |\n", ctx->name);
+
+   seq_printf(s, "  |-[%sframe info]\n",
+  ctx->flags & HVA_FLAG_FRAMEINFO ? "" : "default ");
+   seq_printf(s, "  | |- pixel format=%4.4s\n"
+ "  | |- wxh=%dx%d\n"
+ "  | |- wxh (w/ encoder alignment constraint)=%dx%d\n"
+ "  |\n",
+ (char *)>pixelformat,
+ frame->width, frame->height,
+ frame->aligned_width, frame->aligned_height);
+
+   seq_printf(s, "  |-[%sstream info]\n",
+  ctx->flags & HVA_FLAG_STREAMINFO ? "" : "default ");
+   seq_printf(s, "  | |- stream format=%4.4s\n"
+ "  | |- wxh=%dx%d\n"
+ "  | |- %s\n"
+ "  | |- %s\n"
+ "  |\n",
+ (char *)>streamformat,
+ stream->width, stream->height,
+ stream->profile, stream->level);
+
+   bitrate_mode = V4L2_CID_MPEG_VIDEO_BITRATE_MODE;
+   aspect = V4L2_CID_MPEG_VIDEO_ASPECT;
+   seq_puts(s, "  |-[parameters]\n");
+   seq_printf(s, "  | |- %s\n"
+ "  | |- bitrate=%d bps\n"
+ "  | |- GOP size=%d\n"
+ "  | |- video aspect=%s\n"
+ "  | |- 

Re: ir-keytable: infinite loops, segfaults

[Vincent McIntyre]

On Sun, Nov 27, 2016 at 07:35:10PM +, Sean Young wrote:
> > The application I am trying to use it with is the mythtv frontend.  I
> > am doing the keycode munging from an SSH session while myth is still
> > running on the main screen. I didn't think this would matter (since it
> > worked for KEY_OK->KEY_ENTER) but perhaps it does. Obviously
> > ir-keytable -t intercepts the scancodes when it is running, but when I
> > kill it myth responds normally to some keys, but not all.
> 
> X and keycodes is a bit messy. You might need xmodmap mappings. You
> can check them xev. I don't know much about this, I'm afraid. What
> linux distribution, version and keyboard layout are you using? I could
> try and see if I can reproduce/fix this.

I mostly figured this out but something weird happens with the most
significant bit (see my follow-on email). FWIW, this is on ubuntu 16.04
with their standard kernel (4.4) and a bog-standard US english layout.


> > I wanted to mention that the IR protocol is still showing as unknown.
> > Is there anything that can be done to sort that out?
> 
> It would be nice if that could be sorted out, although that would be 
> a separate patch.

That's fine. For the current patch, please feel free to add my
Tested-By: vincent.mcint...@gmail.com

> So all we know right now is what scancode the IR receiver hardware
> produces but we have no idea what IR protocol is being used. In
> order to figure this out we need a recording of the IR the remote
> sends, for which a different IR receiver is needed. Neither your
> imon nor your dvb_usb_af9035 can do this, something like a mce usb
> IR receiver would be best. Do you have access to one? One with an IR
> emitter would be best.
> 
> So with that we can have a recording of the IR the remote sends, and
> with the emitter we can see which IR protocols the IR receiver
> understands.
> 

I'll poke around to see if I can find something, will take a few days.
Thanks again for your interest in this.
Vince
--
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