Re: [GIT PULL] go7007 firmware updates

[Hans Verkuil]

On Mon May 27 2013 23:53:15 Ben Hutchings wrote:
 On Mon, 2013-05-27 at 21:56 +0200, Hans Verkuil wrote:
  On Mon May 27 2013 18:24:32 Ben Hutchings wrote:
   On Thu, 2013-05-23 at 10:25 +0200, Hans Verkuil wrote:
Hi Ben, David,

The go7007 staging driver has been substantially overhauled for kernel 
3.10.
As part of that process the firmware situation has been improved as 
well.

While Micronas allowed the firmware to be redistributed, it was never 
made
part of linux-firmware. Only the firmwares for the Sensoray S2250 were 
added
in the past, but those need the go7007*.bin firmwares as well to work.

This pull request collects all the firmwares necessary to support all 
the
go7007 devices into the go7007 directory. With this change the go7007 
driver
will work out-of-the-box starting with kernel 3.10.
   [...]
   
   You should not rename files like this.  linux-firmware is not versioned
   and needs to be compatible with old and new kernel versions, so far as
   possible.
  
  I understand, and I wouldn't have renamed these two firmware files if it
  wasn't for the fact that 1) it concerns a staging driver, so in my view
  backwards compatibility is not a requirement,
 
 This driver (or set of drivers) has been requesting go7007fw.bin,
 go7007tv.bin, s2250.fw and s2250_loader.fw for nearly 5 years.  It's a
 bit late to say those were just temporary filenames.

Why not? It is a staging driver for good reasons. Just because it is in staging
for a long time (because nobody found the time to actually work on it until
3.10) doesn't mean it magically becomes non-staging. The Kconfig in staging
says:

  This option allows you to select a number of drivers that are
  not of the normal Linux kernel quality level.  These drivers
  are placed here in order to get a wider audience to make use of
  them.  Please note that these drivers are under heavy
  development, may or may not work, and may contain userspace
  interfaces that most likely will be changed in the near
  future.

In other words, there are no guarantees. That's the whole point of staging.
Once it is moved out of staging everything changes, of course.

Note that it is not just the firmware loading that has changed in 3.10, also
several custom ioctls were removed, thus changing the userspace API, and for
3.11/12 I will add new ioctls to support motion detection. Once that's done
the driver can move out of staging.

  and 2) the firmware files
  currently in linux-firmware were never enough to make the Sensoray S2250
  work, you always needed additional external firmwares as well.
  
   So the filenames in linux-firmware should match whatever the driver has
   used up to now.  If the driver has been changed in 3.10-rc to use
   different filenames, it's not too late to revert this mistake in the
   driver.  But if such a change was made earlier, we'll need to add
   symlinks.
  
  I can revert the rename action, but I would rather not do it. I believe
  there are good reasons for doing this, especially since the current
  situation is effectively broken anyway due to the missing firmware files.
 
 Were the 'new' files unavailable to the public, or only available from a
 manufacturer web site?

They have been available from various websites where people hosted their own
version of the out-of-tree driver from before the driver was moved to staging.
They originated from the manufacturer, which no longer exists. To my knowledge
there isn't any 'canonical' site containing the firmwares.

Loading the firmware was actually quite complex for some go7007 devices,
requiring a userspace utility and special udev rules. That's all changed in
3.10.

Also note that some of the firmware files on those websites were in a hex
format. Starting with 3.10 these files are converted to a smaller binary
format and now use a standard cypress loader kernel module instead of requiring
in-kernel hex parsing.

Regards,

Hans

 
 Ben.
 
  If you really don't want to rename the two S2250 files, then I'll make
  a patch reverting those to the original filename.
  
  Pete, if you have an opinion regarding this, please let us know. After all,
  it concerns a Sensoray device.
 
 
--
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] hdpvr: Simplify the logic that checks for error

[Hans Verkuil]

On Mon May 27 2013 14:04:29 Mauro Carvalho Chehab wrote:
 At get_video_info, there's a somewhat complex logic that checks
 for error.
 
 That logic can be highly simplified, as usb_control_msg will
 only return a negative value, or the buffer length, as it does
 the transfers via DMA.
 
 While here, document why this particular driver is returning -EFAULT,
 instead of the USB error code.

Nacked-by: Hans Verkuil hans.verk...@cisco.com

The EFAULT comment is wrong. The way it is done today is that the error
return of this function is never passed on to userspace.

It's getting messy, so I think it is best if I make two patches based on this
patch and on Leo's fourth patch and post those. If everyone agrees on my 
solution,
then they can be merged.

Sorry Leo, I wasn't aware when we discussed the usb_control_msg return values
before that usb_control_msg() will either return an error or the buffer length,
and nothing else.

Your fourth patch introduced some bugs which I hadn't realized until yesterday.
Which is why it wasn't merged. The main problem with your fourth patch was that
it passed on the get_video_info error code to userspace, but that error code was
for internal use only, and -EFAULT is an inappropriate error code to pass on.

Regards,

Hans

 
 Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
 ---
  drivers/media/usb/hdpvr/hdpvr-control.c | 23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/media/usb/hdpvr/hdpvr-control.c 
 b/drivers/media/usb/hdpvr/hdpvr-control.c
 index d1a3d84..a015a24 100644
 --- a/drivers/media/usb/hdpvr/hdpvr-control.c
 +++ b/drivers/media/usb/hdpvr/hdpvr-control.c
 @@ -56,12 +56,6 @@ int get_video_info(struct hdpvr_device *dev, struct 
 hdpvr_video_info *vidinf)
 0x1400, 0x0003,
 dev-usbc_buf, 5,
 1000);
 - if (ret == 5) {
 - vidinf-width   = dev-usbc_buf[1]  8 | dev-usbc_buf[0];
 - vidinf-height  = dev-usbc_buf[3]  8 | dev-usbc_buf[2];
 - vidinf-fps = dev-usbc_buf[4];
 - }
 -
  #ifdef HDPVR_DEBUG
   if (hdpvr_debug  MSG_INFO) {
   char print_buf[15];
 @@ -73,11 +67,20 @@ int get_video_info(struct hdpvr_device *dev, struct 
 hdpvr_video_info *vidinf)
  #endif
   mutex_unlock(dev-usbc_mutex);
  
 - if (ret  0  ret != 5) { /* fail if unexpected byte count returned */
 - ret = -EFAULT;
 - }
 + /*
 +  * Returning EFAULT is wrong. Unfortunately, MythTV hdpvr
 +  * handling code was written to expect this specific error,
 +  * instead of accepting any error code. So, we can't fix it
 +  * in Kernel without breaking userspace.
 +  */
 + if (ret  0)
 + return -EFAULT;
  
 - return ret  0 ? ret : 0;
 + vidinf-width   = dev-usbc_buf[1]  8 | dev-usbc_buf[0];
 + vidinf-height  = dev-usbc_buf[3]  8 | dev-usbc_buf[2];
 + vidinf-fps = dev-usbc_buf[4];
 +
 + return 0;
  }
  
  int get_input_lines_info(struct hdpvr_device *dev)
 
--
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: Introduce a new helper framework for buffer synchronization

[Maarten Lankhorst]

Hey,

Op 28-05-13 04:49, Inki Dae schreef:

 -Original Message-
 From: Maarten Lankhorst [mailto:maarten.lankho...@canonical.com]
 Sent: Tuesday, May 28, 2013 12:23 AM
 To: Inki Dae
 Cc: 'Daniel Vetter'; 'Rob Clark'; 'linux-fbdev'; 'YoungJun Cho'; 'Kyungmin
 Park'; 'myungjoo.ham'; 'DRI mailing list'; linux-arm-
 ker...@lists.infradead.org; linux-media@vger.kernel.org
 Subject: Re: Introduce a new helper framework for buffer synchronization

 Hey,

 Op 27-05-13 12:38, Inki Dae schreef:
 Hi all,

 I have been removed previous branch and added new one with more cleanup.
 This time, the fence helper doesn't include user side interfaces and
 cache
 operation relevant codes anymore because not only we are not sure that
 coupling those two things, synchronizing caches and buffer access
 between
 CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
 a
 good idea yet but also existing codes for user side have problems with
 badly
 behaved or crashing userspace. So this could be more discussed later.

 The below is a new branch,

 https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
 exynos.git/?h=dma-f
 ence-helper

 And fence helper codes,

 https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
 exynos.git/commit/?
 h=dma-fence-helperid=adcbc0fe7e285ce866e5816e5e21443dcce01005

 And example codes for device driver,

 https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
 exynos.git/commit/?
 h=dma-fence-helperid=d2ce7af23835789602a99d0ccef1f53cdd5caaae

 I think the time is not yet ripe for RFC posting: maybe existing dma
 fence
 and reservation need more review and addition work. So I'd glad for
 somebody
 giving other opinions and advices in advance before RFC posting.

 NAK.

 For examples for how to handle locking properly, see Documentation/ww-
 mutex-design.txt in my recent tree.
 I could list what I believe is wrong with your implementation, but real
 problem is that the approach you're taking is wrong.
 I just removed ticket stubs to show my approach you guys as simple as
 possible, and I just wanted to show that we could use buffer synchronization
 mechanism without ticket stubs.
The tickets have been removed in favor of a ww_context. Moving it in as a base 
primitive
allows more locking abuse to be detected, and makes some other things easier 
too.

 Question, WW-Mutexes could be used for all devices? I guess this has
 dependence on x86 gpu: gpu has VRAM and it means different memory domain.
 And could you tell my why shared fence should have only eight objects? I
 think we could need more than eight objects for read access. Anyway I think
 I don't surely understand yet so there might be my missing point.
Yes, ww mutexes are not limited in any way to x86. They're a locking mechanism.
When you acquired the ww mutexes for all buffer objects, all it does is say at
that point in time you have exclusively acquired the locks of all bo's.

After locking everything you can read the fence pointers safely, queue waits, 
and set a
new fence pointer on all reservation_objects. You only need a single fence
on all those objects, so 8 is plenty. Nonetheless this was a limitation of my
earlier design, and I'll dynamically allocate fence_shared in the future.

~Maarten

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

[PATCH 0/3] s5p-mfc encoder fixes

[Andrzej Hajda]

Those three independent patches are fixes for s5p-mfc encoder.
The first one is a serious bug fix - some controls were not working properly.
The latter two fixes minor issues.

Regards
Andrzej Hajda

Andrzej Hajda (3):
  s5p-mfc: separate encoder parameters for h264 and mpeg4
  s5p-mfc: v4l2 controls setup routine moved to initialization code
  s5p-mfc: added missing end-of-lines in debug messages

 drivers/media/platform/s5p-mfc/s5p_mfc.c|  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 18 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 14 +++---
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c |  4 ++--
 7 files changed, 24 insertions(+), 24 deletions(-)

-- 
1.8.1.2

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

[PATCH 1/3] s5p-mfc: separate encoder parameters for h264 and mpeg4

[Andrzej Hajda]

This patch fixes a bug which caused overwriting h264 codec
parameters by mpeg4 parameters during V4L2 control setting.

Signed-off-by: Andrzej Hajda a.ha...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h 
b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 202d1d7..098459e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -438,7 +438,7 @@ struct s5p_mfc_enc_params {
u32 rc_framerate_num;
u32 rc_framerate_denom;
 
-   union {
+   struct {
struct s5p_mfc_h264_enc_params h264;
struct s5p_mfc_mpeg4_enc_params mpeg4;
} codec;
-- 
1.8.1.2

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

[PATCH 2/3] s5p-mfc: v4l2 controls setup routine moved to initialization code

[Andrzej Hajda]

Callback .start_streaming is called once for every queue,
so v4l2_ctrl_handler_setup was called twice during stream start.
Moving v4l2_ctrl_handler_setup to context initialization
reduces numbers of calls and seems to be more consistent with API.

Signed-off-by: Andrzej Hajda a.ha...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 4f6b553..64bb31e 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1760,7 +1760,6 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, 
unsigned int count)
struct s5p_mfc_ctx *ctx = fh_to_ctx(q-drv_priv);
struct s5p_mfc_dev *dev = ctx-dev;
 
-   v4l2_ctrl_handler_setup(ctx-ctrl_handler);
/* If context is ready then dev = work-data;schedule it to run */
if (s5p_mfc_ctx_ready(ctx))
set_work_bit_irqsave(ctx);
@@ -1920,6 +1919,7 @@ int s5p_mfc_enc_ctrls_setup(struct s5p_mfc_ctx *ctx)
if (controls[i].is_volatile  ctx-ctrls[i])
ctx-ctrls[i]-flags |= V4L2_CTRL_FLAG_VOLATILE;
}
+   v4l2_ctrl_handler_setup(ctx-ctrl_handler);
return 0;
 }
 
-- 
1.8.1.2

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

[PATCH 3/3] s5p-mfc: added missing end-of-lines in debug messages

[Andrzej Hajda]

Many debug messages missed end-of-line.

Signed-off-by: Andrzej Hajda a.ha...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c|  2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 16 
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 
 drivers/media/platform/s5p-mfc/s5p_mfc_pm.c |  4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index 01f9ae0..e04d97f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -562,7 +562,7 @@ static void s5p_mfc_handle_stream_complete(struct 
s5p_mfc_ctx *ctx,
struct s5p_mfc_dev *dev = ctx-dev;
struct s5p_mfc_buf *mb_entry;
 
-   mfc_debug(2, Stream completed);
+   mfc_debug(2, Stream completed\n);
 
s5p_mfc_clear_int_flags(dev);
ctx-int_type = reason;
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_debug.h 
b/drivers/media/platform/s5p-mfc/s5p_mfc_debug.h
index bd5cd4a..8e608f5 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_debug.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_debug.h
@@ -30,8 +30,8 @@ extern int debug;
 #define mfc_debug(level, fmt, args...)
 #endif
 
-#define mfc_debug_enter() mfc_debug(5, enter)
-#define mfc_debug_leave() mfc_debug(5, leave)
+#define mfc_debug_enter() mfc_debug(5, enter\n)
+#define mfc_debug_leave() mfc_debug(5, leave\n)
 
 #define mfc_err(fmt, args...)  \
do {\
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 64bb31e..f6eb28d 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -717,9 +717,9 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
 
slice_type = s5p_mfc_hw_call(dev-mfc_ops, get_enc_slice_type, dev);
strm_size = s5p_mfc_hw_call(dev-mfc_ops, get_enc_strm_size, dev);
-   mfc_debug(2, Encoded slice type: %d, slice_type);
-   mfc_debug(2, Encoded stream size: %d, strm_size);
-   mfc_debug(2, Display order: %d,
+   mfc_debug(2, Encoded slice type: %d\n, slice_type);
+   mfc_debug(2, Encoded stream size: %d\n, strm_size);
+   mfc_debug(2, Display order: %d\n,
  mfc_read(dev, S5P_FIMV_ENC_SI_PIC_CNT));
spin_lock_irqsave(dev-irqlock, flags);
if (slice_type = 0) {
@@ -1533,14 +1533,14 @@ int vidioc_encoder_cmd(struct file *file, void *priv,
 
spin_lock_irqsave(dev-irqlock, flags);
if (list_empty(ctx-src_queue)) {
-   mfc_debug(2, EOS: empty src queue, entering finishing 
state);
+   mfc_debug(2, EOS: empty src queue, entering finishing 
state\n);
ctx-state = MFCINST_FINISHING;
if (s5p_mfc_ctx_ready(ctx))
set_work_bit_irqsave(ctx);
spin_unlock_irqrestore(dev-irqlock, flags);
s5p_mfc_hw_call(dev-mfc_ops, try_run, dev);
} else {
-   mfc_debug(2, EOS: marking last buffer of stream);
+   mfc_debug(2, EOS: marking last buffer of stream\n);
buf = list_entry(ctx-src_queue.prev,
struct s5p_mfc_buf, list);
if (buf-flags  MFC_BUF_FLAG_USED)
@@ -1609,9 +1609,9 @@ static int check_vb_with_fmt(struct s5p_mfc_fmt *fmt, 
struct vb2_buffer *vb)
mfc_err(failed to get plane cookie\n);
return -EINVAL;
}
-   mfc_debug(2, index: %d, plane[%d] cookie: 0x%08zx,
-   vb-v4l2_buf.index, i,
-   vb2_dma_contig_plane_dma_addr(vb, i));
+   mfc_debug(2, index: %d, plane[%d] cookie: 0x%08zx\n,
+ vb-v4l2_buf.index, i,
+ vb2_dma_contig_plane_dma_addr(vb, i));
}
return 0;
 }
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
index 0af05a2..368582b 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c
@@ -1275,8 +1275,8 @@ static int s5p_mfc_run_enc_frame(struct s5p_mfc_ctx *ctx)
spin_unlock_irqrestore(dev-irqlock, flags);
dev-curr_ctx = ctx-num;
s5p_mfc_clean_ctx_int_flags(ctx);
-   mfc_debug(2, encoding buffer with index=%d state=%d,
-   src_mb ? src_mb-b-v4l2_buf.index : -1, ctx-state);
+   mfc_debug(2, encoding buffer with index=%d 

[REVIEW PATCH 3/3] hdpvr: improve error handling

[Hans Verkuil]

From: Hans Verkuil hans.verk...@cisco.com

get_video_info() should never return EFAULT, instead it should return
the low-level usb_control_msg() error. Add a valid field to the hdpvr_video_info
struct so the driver can easily check if a valid format was detected.

Whenever get_video_info is called and it returns an error (e.g. usb_control_msg
failed), then return that error to userspace as well. The sole exception is
the legacy code in vidioc_g_fmt_vid_cap: there EFAULT is returned to avoid
breaking MythTV.

Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
 drivers/media/usb/hdpvr/hdpvr-control.c |   21 +
 drivers/media/usb/hdpvr/hdpvr-video.c   |   16 +---
 drivers/media/usb/hdpvr/hdpvr.h |1 +
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-control.c 
b/drivers/media/usb/hdpvr/hdpvr-control.c
index df6bcb5..6053661 100644
--- a/drivers/media/usb/hdpvr/hdpvr-control.c
+++ b/drivers/media/usb/hdpvr/hdpvr-control.c
@@ -49,6 +49,7 @@ int get_video_info(struct hdpvr_device *dev, struct 
hdpvr_video_info *vidinf)
 {
int ret;
 
+   vidinf-valid = false;
mutex_lock(dev-usbc_mutex);
ret = usb_control_msg(dev-udev,
  usb_rcvctrlpipe(dev-udev, 0),
@@ -56,11 +57,6 @@ int get_video_info(struct hdpvr_device *dev, struct 
hdpvr_video_info *vidinf)
  0x1400, 0x0003,
  dev-usbc_buf, 5,
  1000);
-   if (ret == 5) {
-   vidinf-width   = dev-usbc_buf[1]  8 | dev-usbc_buf[0];
-   vidinf-height  = dev-usbc_buf[3]  8 | dev-usbc_buf[2];
-   vidinf-fps = dev-usbc_buf[4];
-   }
 
 #ifdef HDPVR_DEBUG
if (hdpvr_debug  MSG_INFO) {
@@ -73,14 +69,15 @@ int get_video_info(struct hdpvr_device *dev, struct 
hdpvr_video_info *vidinf)
 #endif
mutex_unlock(dev-usbc_mutex);
 
-   if ((ret  0  ret != 5) ||/* fail if unexpected byte count returned */
-   !vidinf-width ||   /* preserve original behavior -  */
-   !vidinf-height ||  /* fail if no signal is detected */
-   !vidinf-fps) {
-   ret = -EFAULT;
-   }
+   if (ret  0)
+   return ret;
 
-   return ret  0 ? ret : 0;
+   vidinf-width   = dev-usbc_buf[1]  8 | dev-usbc_buf[0];
+   vidinf-height  = dev-usbc_buf[3]  8 | dev-usbc_buf[2];
+   vidinf-fps = dev-usbc_buf[4];
+   vidinf-valid   = vidinf-width  vidinf-height  vidinf-fps;
+
+   return 0;
 }
 
 int get_input_lines_info(struct hdpvr_device *dev)
diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index e947105..eb0e806 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -285,7 +285,10 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
return -EAGAIN;
 
ret = get_video_info(dev, vidinf);
-   if (ret) {
+   if (ret  0)
+   return ret;
+
+   if (!vidinf.valid) {
msleep(250);
v4l2_dbg(MSG_INFO, hdpvr_debug, dev-v4l2_dev,
no video signal at input %d\n, 
dev-options.video_input);
@@ -617,15 +620,12 @@ static int vidioc_querystd(struct file *file, void *_fh, 
v4l2_std_id *a)
if (dev-options.video_input == HDPVR_COMPONENT)
return fh-legacy_mode ? 0 : -ENODATA;
ret = get_video_info(dev, vid_info);
-   if (ret)
-   return 0;
-   if (vid_info.width == 720 
+   if (vid_info.valid  vid_info.width == 720 
(vid_info.height == 480 || vid_info.height == 576)) {
*a = (vid_info.height == 480) ?
V4L2_STD_525_60 : V4L2_STD_625_50;
}
-
-   return 0;
+   return ret;
 }
 
 static int vidioc_s_dv_timings(struct file *file, void *_fh,
@@ -679,6 +679,8 @@ static int vidioc_query_dv_timings(struct file *file, void 
*_fh,
return -ENODATA;
ret = get_video_info(dev, vid_info);
if (ret)
+   return ret;
+   if (!vid_info.valid)
return -ENOLCK;
interlaced = vid_info.fps = 30;
for (i = 0; i  ARRAY_SIZE(hdpvr_dv_timings); i++) {
@@ -1008,7 +1010,7 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void 
*_fh,
struct hdpvr_video_info vid_info;
 
ret = get_video_info(dev, vid_info);
-   if (ret)
+   if (!vid_info.valid)
return -EFAULT;
f-fmt.pix.width = vid_info.width;
f-fmt.pix.height = vid_info.height;
diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
index 808ea7a..dc685d4 100644
--- a/drivers/media/usb/hdpvr/hdpvr.h
+++ b/drivers/media/usb/hdpvr/hdpvr.h
@@ -154,6 +154,7 @@ struct hdpvr_video_info {
u16 width;
u16 

[REVIEW PATCH 1/3] hdpvr: fix querystd 'unknown format' return.

[Hans Verkuil]

From: Hans Verkuil hans.verk...@cisco.com

If no format has been detected, then querystd should return V4L2_STD_UNKNOWN,
not V4L2_STD_ALL.

Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
 drivers/media/usb/hdpvr/hdpvr-video.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index 2d02b49..81018c4 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -613,7 +613,7 @@ static int vidioc_querystd(struct file *file, void *_fh, 
v4l2_std_id *a)
struct hdpvr_fh *fh = _fh;
int ret;
 
-   *a = V4L2_STD_ALL;
+   *a = V4L2_STD_UNKNOWN;
if (dev-options.video_input == HDPVR_COMPONENT)
return fh-legacy_mode ? 0 : -ENODATA;
ret = get_video_info(dev, vid_info);
-- 
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

[REVIEW PATCH 2/3] hdpvr: code cleanup

[Hans Verkuil]

From: Hans Verkuil hans.verk...@cisco.com

Remove an unnecessary 'else' and invert a condition which makes the code
more readable.

Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
 drivers/media/usb/hdpvr/hdpvr-video.c |   54 -
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c 
b/drivers/media/usb/hdpvr/hdpvr-video.c
index 81018c4..e947105 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -281,43 +281,43 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 
if (dev-status == STATUS_STREAMING)
return 0;
-   else if (dev-status != STATUS_IDLE)
+   if (dev-status != STATUS_IDLE)
return -EAGAIN;
 
ret = get_video_info(dev, vidinf);
+   if (ret) {
+   msleep(250);
+   v4l2_dbg(MSG_INFO, hdpvr_debug, dev-v4l2_dev,
+   no video signal at input %d\n, 
dev-options.video_input);
+   return -EAGAIN;
+   }
 
-   if (!ret) {
-   v4l2_dbg(MSG_BUFFER, hdpvr_debug, dev-v4l2_dev,
-video signal: %dx%d@%dhz\n, vidinf.width,
-vidinf.height, vidinf.fps);
+   v4l2_dbg(MSG_BUFFER, hdpvr_debug, dev-v4l2_dev,
+   video signal: %dx%d@%dhz\n, vidinf.width,
+   vidinf.height, vidinf.fps);
 
-   /* start streaming 2 request */
-   ret = usb_control_msg(dev-udev,
- usb_sndctrlpipe(dev-udev, 0),
- 0xb8, 0x38, 0x1, 0, NULL, 0, 8000);
-   v4l2_dbg(MSG_BUFFER, hdpvr_debug, dev-v4l2_dev,
-encoder start control request returned %d\n, ret);
-   if (ret  0)
-   return ret;
+   /* start streaming 2 request */
+   ret = usb_control_msg(dev-udev,
+   usb_sndctrlpipe(dev-udev, 0),
+   0xb8, 0x38, 0x1, 0, NULL, 0, 8000);
+   v4l2_dbg(MSG_BUFFER, hdpvr_debug, dev-v4l2_dev,
+   encoder start control request returned %d\n, ret);
+   if (ret  0)
+   return ret;
 
-   ret = hdpvr_config_call(dev, CTRL_START_STREAMING_VALUE, 0x00);
-   if (ret)
-   return ret;
+   ret = hdpvr_config_call(dev, CTRL_START_STREAMING_VALUE, 0x00);
+   if (ret)
+   return ret;
 
-   dev-status = STATUS_STREAMING;
+   dev-status = STATUS_STREAMING;
 
-   INIT_WORK(dev-worker, hdpvr_transmit_buffers);
-   queue_work(dev-workqueue, dev-worker);
+   INIT_WORK(dev-worker, hdpvr_transmit_buffers);
+   queue_work(dev-workqueue, dev-worker);
 
-   v4l2_dbg(MSG_BUFFER, hdpvr_debug, dev-v4l2_dev,
-streaming started\n);
+   v4l2_dbg(MSG_BUFFER, hdpvr_debug, dev-v4l2_dev,
+   streaming started\n);
 
-   return 0;
-   }
-   msleep(250);
-   v4l2_dbg(MSG_INFO, hdpvr_debug, dev-v4l2_dev,
-no video signal at input %d\n, dev-options.video_input);
-   return -EAGAIN;
+   return 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

[REVIEW PATCH 0/3] hdpvr: various fixes

[Hans Verkuil]

The first patch fixes a bug in querystd: if there is no signal, then
querystd should return V4L2_STD_UNKNOWN. There are more drivers that
return the wrong value here, I have a patch series pending to fix that
and also to improve the spec.

The second does a code cleanup that improves readability, but it doesn't
change the logic.

The third patch is based on a patch from Mauro and a patch from Leo:

https://patchwork.linuxtv.org/patch/18573/
https://linuxtv.org/patch/18399/

This improves the error handling in case usb_control_msg() returns an
error.

Regards,

Hans

--
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] s5p-mfc: separate encoder parameters for h264 and mpeg4

[Sachin Kamat]

Hi Andrzej,

On 28 May 2013 12:56, Andrzej Hajda a.ha...@samsung.com wrote:
 This patch fixes a bug which caused overwriting h264 codec
 parameters by mpeg4 parameters during V4L2 control setting.

Just curious, what was the use case that triggered this issue?

-- 
With warm regards,
Sachin
--
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 3/3] s5p-mfc: added missing end-of-lines in debug messages

[Sachin Kamat]

Hi Andrzej,

On 28 May 2013 12:56, Andrzej Hajda a.ha...@samsung.com wrote:
 Many debug messages missed end-of-line.

 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/media/platform/s5p-mfc/s5p_mfc.c|  2 +-
  drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 16 
  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 
  drivers/media/platform/s5p-mfc/s5p_mfc_pm.c |  4 ++--
  6 files changed, 23 insertions(+), 23 deletions(-)

Instead of changing in so many places, can't we add it in the macro
itself, something like this?
 #define mfc_debug(level, fmt, args...) \
do {\
if (debug = level) \
-   printk(KERN_DEBUG %s:%d:  fmt,\
+   printk(KERN_DEBUG %s:%d:  fmt \n,   \
__func__, __LINE__, ##args);\
} while (0)


-- 
With warm regards,
Sachin
--
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] s5p-mfc: separate encoder parameters for h264 and mpeg4

[Andrzej Hajda]

On 05/28/2013 10:31 AM, Sachin Kamat wrote:
 Hi Andrzej,

 On 28 May 2013 12:56, Andrzej Hajda a.ha...@samsung.com wrote:
 This patch fixes a bug which caused overwriting h264 codec
 parameters by mpeg4 parameters during V4L2 control setting.
 Just curious, what was the use case that triggered this issue?

For example it was not possible to set h264 profile and level -
they were overwritten by struct s5p_mfc_mpeg4_enc_params fields.

In general all 'union' fields of s5p_mfc_h264_enc_params were
overwritten by
s5p_mfc_mpeg4_enc_params and vice versa, the control which was set later
was 'the winner'.
Furthermore during stream start v4l2_ctrl_handler_setup was called so
all controls
were refreshed, so the final winners order was determined by controls
definition order.

Regards
Andrzej





--
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 3/3] s5p-mfc: added missing end-of-lines in debug messages

[Andrzej Hajda]

Hi Sachin,

Thanks for comment.

On 05/28/2013 10:42 AM, Sachin Kamat wrote:
 Hi Andrzej,

 On 28 May 2013 12:56, Andrzej Hajda a.ha...@samsung.com wrote:
 Many debug messages missed end-of-line.

 Signed-off-by: Andrzej Hajda a.ha...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/media/platform/s5p-mfc/s5p_mfc.c|  2 +-
  drivers/media/platform/s5p-mfc/s5p_mfc_debug.h  |  4 ++--
  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c| 16 
  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v5.c |  4 ++--
  drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c | 16 
  drivers/media/platform/s5p-mfc/s5p_mfc_pm.c |  4 ++--
  6 files changed, 23 insertions(+), 23 deletions(-)
 Instead of changing in so many places, can't we add it in the macro
 itself, something like this?
  #define mfc_debug(level, fmt, args...) \
 do {\
 if (debug = level) \
 -   printk(KERN_DEBUG %s:%d:  fmt,\
 +   printk(KERN_DEBUG %s:%d:  fmt \n,   \
 __func__, __LINE__, ##args);\
 } while (0)
Enforcing EOL in mfc_debug will result in removing EOL from above 120 places
where it is currently used :) Also similar change probably should be
made with
mfc_err to make it consistent.
Anyway such change seems to be not consistent with other printk related
functions.

Regards
Andrzej
--
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] hdpvr: Simplify the logic that checks for error

[leo]

Hi Hans,

Passing on the actual error code was intentional.
My main goal was to give user space ability to
distinguish between the no-lock and usb failure
conditions. HDPVR firmware instability usually
manifests itself as a usb failure, and passing
the error code on gives the application ability,
for example to use this device
(https://sites.google.com/site/hdpvrkillerdevice/)
as soon as firmware crash is detected and not
loose the recording. Unfortunately today because
of inability to see the usb failures the failed
recording is only detected by the zero-length
file after it's too late...

If you remember I did a some research and testing
with MythTV (email from Apr 17):
***
...I checked MythTV function that is
monitoring video signal (AnalogSignalMonitor::handleHDPVR() in
analogsignalmonitor.cpp). Please see below that the no-signal
condition is checked as both - the ioctl failure and
pix.width is equal to 0:

 if ((ioctl(videofd, VIDIOC_G_FMT, vfmt) == 0) 
 vfmt.fmt.pix.width ...

Then I tested MythTV for no-signal condition in two cases:
 1. Loosing signal during live preview.
 2. Trying to enter preview with no signal.
Both cases showed the same behavior as with the current
driver...
***

So based on this, returning the actual error code
will NOT break MythTV logic, it still will handle
it as a no-lock condition.

One thing I have not done though is I did not contact
MythTV to confirm the above conclusions, but I
wouldn't mind if that would help to shift the decision...


Everything else in the patches looks good to me!

Best regards,
-Leo.





 Original Message 
Subject: Re: [PATCH] [media] hdpvr: Simplify the logic that checks for
error
From: Hans Verkuil hverk...@xs4all.nl
Date: Mon, May 27, 2013 11:58 pm
To: Mauro Carvalho Chehab mche...@redhat.com
Cc: Linux Media Mailing List linux-media@vger.kernel.org,
l...@lumanate.com

On Mon May 27 2013 14:04:29 Mauro Carvalho Chehab wrote:
 At get_video_info, there's a somewhat complex logic that checks
 for error.
 
 That logic can be highly simplified, as usb_control_msg will
 only return a negative value, or the buffer length, as it does
 the transfers via DMA.
 
 While here, document why this particular driver is returning -EFAULT,
 instead of the USB error code.

Nacked-by: Hans Verkuil hans.verk...@cisco.com

The EFAULT comment is wrong. The way it is done today is that the error
return of this function is never passed on to userspace.

It's getting messy, so I think it is best if I make two patches based on
this
patch and on Leo's fourth patch and post those. If everyone agrees on my
solution,
then they can be merged.

Sorry Leo, I wasn't aware when we discussed the usb_control_msg return
values
before that usb_control_msg() will either return an error or the buffer
length,
and nothing else.

Your fourth patch introduced some bugs which I hadn't realized until
yesterday.
Which is why it wasn't merged. The main problem with your fourth patch
was that
it passed on the get_video_info error code to userspace, but that error
code was
for internal use only, and -EFAULT is an inappropriate error code to
pass on.

Regards,

 Hans

 
 Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
 ---
 drivers/media/usb/hdpvr/hdpvr-control.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/media/usb/hdpvr/hdpvr-control.c 
 b/drivers/media/usb/hdpvr/hdpvr-control.c
 index d1a3d84..a015a24 100644
 --- a/drivers/media/usb/hdpvr/hdpvr-control.c
 +++ b/drivers/media/usb/hdpvr/hdpvr-control.c
 @@ -56,12 +56,6 @@ int get_video_info(struct hdpvr_device *dev, struct 
 hdpvr_video_info *vidinf)
 0x1400, 0x0003,
 dev-usbc_buf, 5,
 1000);
 - if (ret == 5) {
 - vidinf-width = dev-usbc_buf[1]  8 | dev-usbc_buf[0];
 - vidinf-height = dev-usbc_buf[3]  8 | dev-usbc_buf[2];
 - vidinf-fps = dev-usbc_buf[4];
 - }
 -
 #ifdef HDPVR_DEBUG
 if (hdpvr_debug  MSG_INFO) {
 char print_buf[15];
 @@ -73,11 +67,20 @@ int get_video_info(struct hdpvr_device *dev, struct 
 hdpvr_video_info *vidinf)
 #endif
 mutex_unlock(dev-usbc_mutex);
 
 - if (ret  0  ret != 5) { /* fail if unexpected byte count returned */
 - ret = -EFAULT;
 - }
 + /*
 + * Returning EFAULT is wrong. Unfortunately, MythTV hdpvr
 + * handling code was written to expect this specific error,
 + * instead of accepting any error code. So, we can't fix it
 + * in Kernel without breaking userspace.
 + */
 + if (ret  0)
 + return -EFAULT;
 
 - return ret  0 ? ret : 0;
 + vidinf-width = dev-usbc_buf[1]  8 | dev-usbc_buf[0];
 + vidinf-height = dev-usbc_buf[3]  8 | dev-usbc_buf[2];
 + vidinf-fps = dev-usbc_buf[4];
 +
 + return 0;
 }
 
 int get_input_lines_info(struct hdpvr_device *dev)
 
--
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 

Re: Introduce a new helper framework for buffer synchronization

[Daniel Vetter]

On Tue, May 28, 2013 at 12:56:57PM +0900, Inki Dae wrote:
 
 
  -Original Message-
  From: linux-fbdev-ow...@vger.kernel.org [mailto:linux-fbdev-
  ow...@vger.kernel.org] On Behalf Of Rob Clark
  Sent: Tuesday, May 28, 2013 12:48 AM
  To: Inki Dae
  Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
  Park; myungjoo.ham; DRI mailing list;
 linux-arm-ker...@lists.infradead.org;
  linux-media@vger.kernel.org
  Subject: Re: Introduce a new helper framework for buffer synchronization
  
  On Mon, May 27, 2013 at 6:38 AM, Inki Dae inki@samsung.com wrote:
   Hi all,
  
   I have been removed previous branch and added new one with more cleanup.
   This time, the fence helper doesn't include user side interfaces and
  cache
   operation relevant codes anymore because not only we are not sure that
   coupling those two things, synchronizing caches and buffer access
  between
   CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
  a
   good idea yet but also existing codes for user side have problems with
  badly
   behaved or crashing userspace. So this could be more discussed later.
  
   The below is a new branch,
  
   https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
  exynos.git/?h=dma-f
   ence-helper
  
   And fence helper codes,
  
   https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
  exynos.git/commit/?
   h=dma-fence-helperid=adcbc0fe7e285ce866e5816e5e21443dcce01005
  
   And example codes for device driver,
  
   https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
  exynos.git/commit/?
   h=dma-fence-helperid=d2ce7af23835789602a99d0ccef1f53cdd5caaae
  
   I think the time is not yet ripe for RFC posting: maybe existing dma
  fence
   and reservation need more review and addition work. So I'd glad for
  somebody
   giving other opinions and advices in advance before RFC posting.
  
  thoughts from a *really* quick, pre-coffee, first look:
  * any sort of helper to simplify single-buffer sort of use-cases (v4l)
  probably wouldn't want to bake in assumption that seqno_fence is used.
  * I guess g2d is probably not actually a simple use case, since I
  expect you can submit blits involving multiple buffers :-P
 
 I don't think so. One and more buffers can be used: seqno_fence also has
 only one buffer. Actually, we have already applied this approach to most
 devices; multimedia, gpu and display controller. And this approach shows
 more performance; reduced power consumption against traditional way. And g2d
 example is just to show you how to apply my approach to device driver.

Note that seqno_fence is an implementation pattern for a certain type of
direct hw-hw synchronization which uses a shared dma_buf to exchange the
sync cookie. The dma_buf attached to the seqno_fence has _nothing_ to do
with the dma_buf the fence actually coordinates access to.

I think that confusing is a large reason for why MaartenI don't
understand what you want to achieve with your fence helpers. Currently
they're using the seqno_fence, but totally not in a way the seqno_fence
was meant to be used.

Note that with the current code there is only a pointer from dma_bufs to
the fence. The fence itself has _no_ pointer to the dma_buf it syncs. This
shouldn't be a problem since the fence fastpath for already signalled
fences is completely barrierlock free (it's just a load+bit-test), and
fences are meant to be embedded into whatever dma tracking structure you
already have, so no overhead there. The only ugly part is the fence
refcounting, but I don't think we can drop that.

Note that you completely reinvent this part of Maarten's fence patches by
adding new r/w_complete completions to the reservation object, which
completely replaces the fence stuff.

Also note that a list of reservation entries is again meant to be used
only when submitting the dma to the gpu. With your patches you seem to
hang onto that list until dma completes. This has the ugly side-effect
that you need to allocate these reservation entries with kmalloc, whereas
if you just use them in the execbuf ioctl to construct the dma you can
usually embed it into something else you need already anyway. At least
i915 and ttm based drivers can work that way.

Furthermore fences are specifically constructed as frankenstein-monsters
between completion/waitqueues and callbacks. All the different use-cases
need the different aspects:
- busy/idle checks and bo retiring need the completion semantics
- callbacks (in interrupt context) are used for hybrid hw-irq handler-hw
  sync approaches

 
  * otherwise, you probably don't want to depend on dmabuf, which is why
  reservation/fence is split out the way it is..  you want to be able to
  use a single reservation/fence mechanism within your driver without
  having to care about which buffers are exported to dmabuf's and which
  are not.  Creating a dmabuf for every GEM bo is too heavyweight.
 
 Right. But I think we should dealt with this separately. 

Re: Introduce a new helper framework for buffer synchronization

[Rob Clark]

On Mon, May 27, 2013 at 11:56 PM, Inki Dae inki@samsung.com wrote:


 -Original Message-
 From: linux-fbdev-ow...@vger.kernel.org [mailto:linux-fbdev-
 ow...@vger.kernel.org] On Behalf Of Rob Clark
 Sent: Tuesday, May 28, 2013 12:48 AM
 To: Inki Dae
 Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
 Park; myungjoo.ham; DRI mailing list;
 linux-arm-ker...@lists.infradead.org;
 linux-media@vger.kernel.org
 Subject: Re: Introduce a new helper framework for buffer synchronization

 On Mon, May 27, 2013 at 6:38 AM, Inki Dae inki@samsung.com wrote:
  Hi all,
 
  I have been removed previous branch and added new one with more cleanup.
  This time, the fence helper doesn't include user side interfaces and
 cache
  operation relevant codes anymore because not only we are not sure that
  coupling those two things, synchronizing caches and buffer access
 between
  CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
 a
  good idea yet but also existing codes for user side have problems with
 badly
  behaved or crashing userspace. So this could be more discussed later.
 
  The below is a new branch,
 
  https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
 exynos.git/?h=dma-f
  ence-helper
 
  And fence helper codes,
 
  https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
 exynos.git/commit/?
  h=dma-fence-helperid=adcbc0fe7e285ce866e5816e5e21443dcce01005
 
  And example codes for device driver,
 
  https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
 exynos.git/commit/?
  h=dma-fence-helperid=d2ce7af23835789602a99d0ccef1f53cdd5caaae
 
  I think the time is not yet ripe for RFC posting: maybe existing dma
 fence
  and reservation need more review and addition work. So I'd glad for
 somebody
  giving other opinions and advices in advance before RFC posting.

 thoughts from a *really* quick, pre-coffee, first look:
 * any sort of helper to simplify single-buffer sort of use-cases (v4l)
 probably wouldn't want to bake in assumption that seqno_fence is used.
 * I guess g2d is probably not actually a simple use case, since I
 expect you can submit blits involving multiple buffers :-P

 I don't think so. One and more buffers can be used: seqno_fence also has
 only one buffer. Actually, we have already applied this approach to most
 devices; multimedia, gpu and display controller. And this approach shows
 more performance; reduced power consumption against traditional way. And g2d
 example is just to show you how to apply my approach to device driver.

no, you need the ww-mutex / reservation stuff any time you have
multiple independent devices (or rings/contexts for hw that can
support multiple contexts) which can do operations with multiple
buffers.  So you could conceivably hit this w/ gpu + g2d if multiple
buffers where shared between the two.  vram migration and such
'desktop stuff' might make the problem worse, but just because you
don't have vram doesn't mean you don't have a problem with multiple
buffers.

 * otherwise, you probably don't want to depend on dmabuf, which is why
 reservation/fence is split out the way it is..  you want to be able to
 use a single reservation/fence mechanism within your driver without
 having to care about which buffers are exported to dmabuf's and which
 are not.  Creating a dmabuf for every GEM bo is too heavyweight.

 Right. But I think we should dealt with this separately. Actually, we are
 trying to use reservation for gpu pipe line synchronization such as sgx sync
 object and this approach is used without dmabuf. In order words, some device
 can use only reservation for such pipe line synchronization and at the same
 time, fence helper or similar thing with dmabuf for buffer synchronization.

it is probably easier to approach from the reverse direction.. ie, get
reservation/synchronization right first, and then dmabuf.  (Well, that
isn't really a problem because Maarten's reservation/fence patches
support dmabuf from the beginning.)

BR,
-R


 I'm not entirely sure if reservation/fence could/should be made any
 simpler for multi-buffer users.  Probably the best thing to do is just
 get reservation/fence rolled out in a few drivers and see if some
 common patterns emerge.

 BR,
 -R

 
  Thanks,
  Inki Dae
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-fbdev 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] v4l2: mem2mem: save irq flags correctly

[Kamil Debski]

Hi Sheu,

Thank you for this patch. May I also ask you to add me to Cc of next mem2mem
patches, as I am the mem2mem submaintainer?

Best wishes,
-- 
Kamil Debski
Linux Kernel Developer
Samsung RD Institute Poland


 -Original Message-
 From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
 ow...@vger.kernel.org] On Behalf Of John Sheu
 Sent: Friday, May 24, 2013 2:42 AM
 To: linux-media@vger.kernel.org
 Cc: mche...@redhat.com; pa...@osciak.com; John Sheu
 Subject: [PATCH] [media] v4l2: mem2mem: save irq flags correctly
 
 Save flags correctly when taking spinlocks in v4l2_m2m_try_schedule.
 
 Signed-off-by: John Sheu s...@google.com

Acked-by: Kamil Debski k.deb...@samsung.com

 ---
  drivers/media/v4l2-core/v4l2-mem2mem.c | 19 +++
  1 file changed, 11 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c
 b/drivers/media/v4l2-core/v4l2-mem2mem.c
 index 66f599f..3606ff2 100644
 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
 +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
 @@ -205,7 +205,7 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev
 *m2m_dev)  static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx
 *m2m_ctx)  {
   struct v4l2_m2m_dev *m2m_dev;
 - unsigned long flags_job, flags;
 + unsigned long flags_job, flags_out, flags_cap;
 
   m2m_dev = m2m_ctx-m2m_dev;
   dprintk(Trying to schedule a job for m2m_ctx: %p\n, m2m_ctx);
 @@ -223,23 +223,26 @@ static void v4l2_m2m_try_schedule(struct
 v4l2_m2m_ctx *m2m_ctx)
   return;
   }
 
 - spin_lock_irqsave(m2m_ctx-out_q_ctx.rdy_spinlock, flags);
 + spin_lock_irqsave(m2m_ctx-out_q_ctx.rdy_spinlock, flags_out);
   if (list_empty(m2m_ctx-out_q_ctx.rdy_queue)) {
 - spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 flags);
 + spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 + flags_out);
   spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
   dprintk(No input buffers available\n);
   return;
   }
 - spin_lock_irqsave(m2m_ctx-cap_q_ctx.rdy_spinlock, flags);
 + spin_lock_irqsave(m2m_ctx-cap_q_ctx.rdy_spinlock, flags_cap);
   if (list_empty(m2m_ctx-cap_q_ctx.rdy_queue)) {
 - spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock,
 flags);
 - spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 flags);
 + spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock,
 + flags_cap);
 + spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 + flags_out);
   spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
   dprintk(No output buffers available\n);
   return;
   }
 - spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock, flags);
 - spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock, flags);
 + spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock,
 flags_cap);
 + spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 flags_out);
 
   if (m2m_dev-m2m_ops-job_ready
(!m2m_dev-m2m_ops-job_ready(m2m_ctx-priv))) {
 --
 1.8.2.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


--
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 v5] V4L2: soc_camera: Renesas R-Car VIN driver

[phil . edworthy]

Hi Sergei, Vladimir,

  Oops, the comments about the captured image contents are my fault.
  However, the unhandled irq after stopping capture is still an issue.
 
 Thanks for letting us know.
The good news is that your driver works fine. 

The problem I saw only occurs when your patches were integrated into 
Simon's next branch (which was renesas-next-20130515v2+v3.10-rc1), so I 
guess something in the rc1 caused problems. Once I tested the driver 
against 3.9, it works fine.

Thanks
Phil
--
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] v4l2: mem2mem: save irq flags correctly

[Pawel Osciak]

John, thanks for the patch.

On Thu, May 23, 2013 at 5:41 PM, John Sheu s...@google.com wrote:
 Save flags correctly when taking spinlocks in v4l2_m2m_try_schedule.

 Signed-off-by: John Sheu s...@google.com

Acked-by: Pawel Osciak pa...@osciak.com


 ---
  drivers/media/v4l2-core/v4l2-mem2mem.c | 19 +++
  1 file changed, 11 insertions(+), 8 deletions(-)

 diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c 
 b/drivers/media/v4l2-core/v4l2-mem2mem.c
 index 66f599f..3606ff2 100644
 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
 +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
 @@ -205,7 +205,7 @@ static void v4l2_m2m_try_run(struct v4l2_m2m_dev *m2m_dev)
  static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
  {
 struct v4l2_m2m_dev *m2m_dev;
 -   unsigned long flags_job, flags;
 +   unsigned long flags_job, flags_out, flags_cap;

 m2m_dev = m2m_ctx-m2m_dev;
 dprintk(Trying to schedule a job for m2m_ctx: %p\n, m2m_ctx);
 @@ -223,23 +223,26 @@ static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx 
 *m2m_ctx)
 return;
 }

 -   spin_lock_irqsave(m2m_ctx-out_q_ctx.rdy_spinlock, flags);
 +   spin_lock_irqsave(m2m_ctx-out_q_ctx.rdy_spinlock, flags_out);
 if (list_empty(m2m_ctx-out_q_ctx.rdy_queue)) {
 -   spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock, 
 flags);
 +   spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 +   flags_out);
 spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
 dprintk(No input buffers available\n);
 return;
 }
 -   spin_lock_irqsave(m2m_ctx-cap_q_ctx.rdy_spinlock, flags);
 +   spin_lock_irqsave(m2m_ctx-cap_q_ctx.rdy_spinlock, flags_cap);
 if (list_empty(m2m_ctx-cap_q_ctx.rdy_queue)) {
 -   spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock, 
 flags);
 -   spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock, 
 flags);
 +   spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock,
 +   flags_cap);
 +   spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock,
 +   flags_out);
 spin_unlock_irqrestore(m2m_dev-job_spinlock, flags_job);
 dprintk(No output buffers available\n);
 return;
 }
 -   spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock, flags);
 -   spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock, flags);
 +   spin_unlock_irqrestore(m2m_ctx-cap_q_ctx.rdy_spinlock, flags_cap);
 +   spin_unlock_irqrestore(m2m_ctx-out_q_ctx.rdy_spinlock, flags_out);

 if (m2m_dev-m2m_ops-job_ready
  (!m2m_dev-m2m_ops-job_ready(m2m_ctx-priv))) {
 --
 1.8.2.1




--
Best regards,
Pawel Osciak
--
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: Introduce a new helper framework for buffer synchronization

[Inki Dae]

Hi Daniel,

Thank you so much. And so very useful.:) Sorry but could be give me more
comments to the below my comments? There are still things making me
confusing.:(


 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
 Vetter
 Sent: Tuesday, May 28, 2013 7:33 PM
 To: Inki Dae
 Cc: 'Rob Clark'; 'Maarten Lankhorst'; 'Daniel Vetter'; 'linux-fbdev';
 'YoungJun Cho'; 'Kyungmin Park'; 'myungjoo.ham'; 'DRI mailing list';
 linux-arm-ker...@lists.infradead.org; linux-media@vger.kernel.org
 Subject: Re: Introduce a new helper framework for buffer synchronization
 
 On Tue, May 28, 2013 at 12:56:57PM +0900, Inki Dae wrote:
 
 
   -Original Message-
   From: linux-fbdev-ow...@vger.kernel.org [mailto:linux-fbdev-
   ow...@vger.kernel.org] On Behalf Of Rob Clark
   Sent: Tuesday, May 28, 2013 12:48 AM
   To: Inki Dae
   Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho;
 Kyungmin
   Park; myungjoo.ham; DRI mailing list;
  linux-arm-ker...@lists.infradead.org;
   linux-media@vger.kernel.org
   Subject: Re: Introduce a new helper framework for buffer
 synchronization
  
   On Mon, May 27, 2013 at 6:38 AM, Inki Dae inki@samsung.com
wrote:
Hi all,
   
I have been removed previous branch and added new one with more
 cleanup.
This time, the fence helper doesn't include user side interfaces and
   cache
operation relevant codes anymore because not only we are not sure
 that
coupling those two things, synchronizing caches and buffer access
   between
CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel
 side is
   a
good idea yet but also existing codes for user side have problems
 with
   badly
behaved or crashing userspace. So this could be more discussed
later.
   
The below is a new branch,
   
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
   exynos.git/?h=dma-f
ence-helper
   
And fence helper codes,
   
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
   exynos.git/commit/?
h=dma-fence-helperid=adcbc0fe7e285ce866e5816e5e21443dcce01005
   
And example codes for device driver,
   
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
   exynos.git/commit/?
h=dma-fence-helperid=d2ce7af23835789602a99d0ccef1f53cdd5caaae
   
I think the time is not yet ripe for RFC posting: maybe existing dma
   fence
and reservation need more review and addition work. So I'd glad for
   somebody
giving other opinions and advices in advance before RFC posting.
  
   thoughts from a *really* quick, pre-coffee, first look:
   * any sort of helper to simplify single-buffer sort of use-cases (v4l)
   probably wouldn't want to bake in assumption that seqno_fence is used.
   * I guess g2d is probably not actually a simple use case, since I
   expect you can submit blits involving multiple buffers :-P
 
  I don't think so. One and more buffers can be used: seqno_fence also has
  only one buffer. Actually, we have already applied this approach to most
  devices; multimedia, gpu and display controller. And this approach shows
  more performance; reduced power consumption against traditional way. And
 g2d
  example is just to show you how to apply my approach to device driver.
 
 Note that seqno_fence is an implementation pattern for a certain type of
 direct hw-hw synchronization which uses a shared dma_buf to exchange the
 sync cookie.

I'm afraid that I don't understand hw-hw synchronization. hw-hw
synchronization means that device has a hardware feature which supports
buffer synchronization hardware internally? And what is the sync cookie?

 The dma_buf attached to the seqno_fence has _nothing_ to do
 with the dma_buf the fence actually coordinates access to.
 
 I think that confusing is a large reason for why MaartenI don't
 understand what you want to achieve with your fence helpers. Currently
 they're using the seqno_fence, but totally not in a way the seqno_fence
 was meant to be used.
 
 Note that with the current code there is only a pointer from dma_bufs to
 the fence. The fence itself has _no_ pointer to the dma_buf it syncs. This
 shouldn't be a problem since the fence fastpath for already signalled
 fences is completely barrierlock free (it's just a load+bit-test), and
 fences are meant to be embedded into whatever dma tracking structure you
 already have, so no overhead there. The only ugly part is the fence
 refcounting, but I don't think we can drop that.

The below is the proposed way,
dma device has to create a fence before accessing a shared buffer, and then
check if there are other dma which are accessing the shared buffer; if exist
then the dma device should be blocked, and then  it sets the fence to
reservation object of the shared buffer. And then the dma begins access to
the shared buffer. And finally, the dma signals its own fence so that other
blocked can be waked up. However, if there was another dma blocked before
signaling then 

[PATCH v4 1/4] arch: make __mutex_fastpath_lock_retval return whether fastpath succeeded or not.

[Maarten Lankhorst]

This will allow me to call functions that have multiple arguments if fastpath 
fails.
This is required to support ticket mutexes, because they need to be able to 
pass an
extra argument to the fail function.

Originally I duplicated the functions, by adding 
__mutex_fastpath_lock_retval_arg.
This ended up being just a duplication of the existing function, so a way to 
test
if fastpath was called ended up being better.

This also cleaned up the reservation mutex patch some by being able to call an
atomic_set instead of atomic_xchg, and making it easier to detect if the wrong
unlock function was previously used.

Changes since v1, pointed out by Francesco Lavra:
- fix a small comment issue in mutex_32.h
- fix the __mutex_fastpath_lock_retval macro for mutex-null.h

Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
---
 arch/ia64/include/asm/mutex.h|   10 --
 arch/powerpc/include/asm/mutex.h |   10 --
 arch/sh/include/asm/mutex-llsc.h |4 ++--
 arch/x86/include/asm/mutex_32.h  |   11 ---
 arch/x86/include/asm/mutex_64.h  |   11 ---
 include/asm-generic/mutex-dec.h  |   10 --
 include/asm-generic/mutex-null.h |2 +-
 include/asm-generic/mutex-xchg.h |   10 --
 kernel/mutex.c   |   32 ++--
 9 files changed, 41 insertions(+), 59 deletions(-)

diff --git a/arch/ia64/include/asm/mutex.h b/arch/ia64/include/asm/mutex.h
index bed73a6..f41e66d 100644
--- a/arch/ia64/include/asm/mutex.h
+++ b/arch/ia64/include/asm/mutex.h
@@ -29,17 +29,15 @@ __mutex_fastpath_lock(atomic_t *count, void 
(*fail_fn)(atomic_t *))
  *  __mutex_fastpath_lock_retval - try to take the lock by moving the count
  * from 1 to a 0 value
  *  @count: pointer of type atomic_t
- *  @fail_fn: function to call if the original value was not 1
  *
- * Change the count from 1 to a value lower than 1, and call fail_fn if
- * it wasn't 1 originally. This function returns 0 if the fastpath succeeds,
- * or anything the slow path function returns.
+ * Change the count from 1 to a value lower than 1. This function returns 0
+ * if the fastpath succeeds, or -1 otherwise.
  */
 static inline int
-__mutex_fastpath_lock_retval(atomic_t *count, int (*fail_fn)(atomic_t *))
+__mutex_fastpath_lock_retval(atomic_t *count)
 {
if (unlikely(ia64_fetchadd4_acq(count, -1) != 1))
-   return fail_fn(count);
+   return -1;
return 0;
 }
 
diff --git a/arch/powerpc/include/asm/mutex.h b/arch/powerpc/include/asm/mutex.h
index 5399f7e..127ab23 100644
--- a/arch/powerpc/include/asm/mutex.h
+++ b/arch/powerpc/include/asm/mutex.h
@@ -82,17 +82,15 @@ __mutex_fastpath_lock(atomic_t *count, void 
(*fail_fn)(atomic_t *))
  *  __mutex_fastpath_lock_retval - try to take the lock by moving the count
  * from 1 to a 0 value
  *  @count: pointer of type atomic_t
- *  @fail_fn: function to call if the original value was not 1
  *
- * Change the count from 1 to a value lower than 1, and call fail_fn if
- * it wasn't 1 originally. This function returns 0 if the fastpath succeeds,
- * or anything the slow path function returns.
+ * Change the count from 1 to a value lower than 1. This function returns 0
+ * if the fastpath succeeds, or -1 otherwise.
  */
 static inline int
-__mutex_fastpath_lock_retval(atomic_t *count, int (*fail_fn)(atomic_t *))
+__mutex_fastpath_lock_retval(atomic_t *count)
 {
if (unlikely(__mutex_dec_return_lock(count)  0))
-   return fail_fn(count);
+   return -1;
return 0;
 }
 
diff --git a/arch/sh/include/asm/mutex-llsc.h b/arch/sh/include/asm/mutex-llsc.h
index 090358a..dad29b6 100644
--- a/arch/sh/include/asm/mutex-llsc.h
+++ b/arch/sh/include/asm/mutex-llsc.h
@@ -37,7 +37,7 @@ __mutex_fastpath_lock(atomic_t *count, void 
(*fail_fn)(atomic_t *))
 }
 
 static inline int
-__mutex_fastpath_lock_retval(atomic_t *count, int (*fail_fn)(atomic_t *))
+__mutex_fastpath_lock_retval(atomic_t *count)
 {
int __done, __res;
 
@@ -51,7 +51,7 @@ __mutex_fastpath_lock_retval(atomic_t *count, int 
(*fail_fn)(atomic_t *))
: t);
 
if (unlikely(!__done || __res != 0))
-   __res = fail_fn(count);
+   __res = -1;
 
return __res;
 }
diff --git a/arch/x86/include/asm/mutex_32.h b/arch/x86/include/asm/mutex_32.h
index 03f90c8..0208c3c 100644
--- a/arch/x86/include/asm/mutex_32.h
+++ b/arch/x86/include/asm/mutex_32.h
@@ -42,17 +42,14 @@ do {
\
  *  __mutex_fastpath_lock_retval - try to take the lock by moving the count
  * from 1 to a 0 value
  *  @count: pointer of type atomic_t
- *  @fail_fn: function to call if the original value was not 1
  *
- * Change the count from 1 to a value lower than 1, and call fail_fn if it
- * wasn't 1 originally. This function returns 0 if the 

[PATCH v4 4/4] mutex: w/w mutex slowpath debugging

[Maarten Lankhorst]

From: Daniel Vetter daniel.vet...@ffwll.ch

Injects EDEADLK conditions at pseudo-random interval, with exponential
backoff up to UINT_MAX (to ensure that every lock operation still
completes in a reasonable time).

This way we can test the wound slowpath even for ww mutex users where
contention is never expected, and the ww deadlock avoidance algorithm
is only needed for correctness against malicious userspace. An example
would be protecting kernel modesetting properties, which thanks to
single-threaded X isn't really expected to contend, ever.

I've looked into using the CONFIG_FAULT_INJECTION infrastructure, but
decided against it for two reasons:

- EDEADLK handling is mandatory for ww mutex users and should never
  affect the outcome of a syscall. This is in contrast to -ENOMEM
  injection. So fine configurability isn't required.

- The fault injection framework only allows to set a simple
  probability for failure. Now the probability that a ww mutex acquire
  stage with N locks will never complete (due to too many injected
  EDEADLK backoffs) is zero. But the expected number of ww_mutex_lock
  operations for the completely uncontended case would be O(exp(N)).
  The per-acuiqire ctx exponential backoff solution choosen here only
  results in O(log N) overhead due to injection and so O(log N * N)
  lock operations. This way we can fail with high probability (and so
  have good test coverage even for fancy backoff and lock acquisition
  paths) without running into patalogical cases.

Note that EDEADLK will only ever be injected when we managed to
acquire the lock. This prevents any behaviour changes for users which
rely on the EALREADY semantics.

v2: Drop the cargo-culted __sched (I should read docs next time
around) and annotate the non-debug case with inline to prevent gcc
from doing something horrible.

v3: Rebase on top of Maarten's latest patches.

v4: Actually make this stuff compile, I've misplace the hunk in the
wrong #ifdef block.

v5: Simplify ww_mutex_deadlock_injection definition, and fix
lib/locking-selftest.c warnings. Fix lib/Kconfig.debug definition
to work correctly. (mlankhorst)

v6:
Do not inject -EDEADLK when ctx-acquired == 0, because
the _slow paths are merged now. (mlankhorst)

Cc: Steven Rostedt rost...@goodmis.org
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
---
 include/linux/mutex.h  |8 
 kernel/mutex.c |   44 +---
 lib/Kconfig.debug  |   13 +
 lib/locking-selftest.c |5 +
 4 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index f3ad181..2ff9178 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -95,6 +95,10 @@ struct ww_acquire_ctx {
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lockdep_map dep_map;
 #endif
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+   unsigned deadlock_inject_interval;
+   unsigned deadlock_inject_countdown;
+#endif
 };
 
 struct ww_mutex {
@@ -280,6 +284,10 @@ static inline void ww_acquire_init(struct ww_acquire_ctx 
*ctx,
 ww_class-acquire_key, 0);
mutex_acquire(ctx-dep_map, 0, 0, _RET_IP_);
 #endif
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+   ctx-deadlock_inject_interval = 1;
+   ctx-deadlock_inject_countdown = ctx-stamp  0xf;
+#endif
 }
 
 /**
diff --git a/kernel/mutex.c b/kernel/mutex.c
index 75fc7c4..e40004b 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -508,22 +508,60 @@ mutex_lock_interruptible_nested(struct mutex *lock, 
unsigned int subclass)
 
 EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested);
 
+static inline int
+ww_mutex_deadlock_injection(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
+{
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+   unsigned tmp;
+
+   if (ctx-deadlock_inject_countdown-- == 0) {
+   tmp = ctx-deadlock_inject_interval;
+   if (tmp  UINT_MAX/4)
+   tmp = UINT_MAX;
+   else
+   tmp = tmp*2 + tmp + tmp/2;
+
+   ctx-deadlock_inject_interval = tmp;
+   ctx-deadlock_inject_countdown = tmp;
+   ctx-contending_lock = lock;
+
+   ww_mutex_unlock(lock);
+
+   return -EDEADLK;
+   }
+#endif
+
+   return 0;
+}
 
 int __sched
 __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 {
+   int ret;
+
might_sleep();
-   return __mutex_lock_common(lock-base, TASK_UNINTERRUPTIBLE,
+   ret =  __mutex_lock_common(lock-base, TASK_UNINTERRUPTIBLE,
   0, ctx-dep_map, _RET_IP_, ctx);
+   if (!ret  ctx-acquired  0)
+   return ww_mutex_deadlock_injection(lock, ctx);
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(__ww_mutex_lock);
 
 int __sched
 __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx 
*ctx)
 {
+   int ret;
+
  

[PATCH v4 3/4] mutex: Add ww tests to lib/locking-selftest.c. v4

[Maarten Lankhorst]

This stresses the lockdep code in some ways specifically useful to
ww_mutexes. It adds checks for most of the common locking errors.

Changes since v1:
 - Add tests to verify reservation_id is untouched.
 - Use L() and U() macros where possible.

Changes since v2:
 - Use the ww_mutex api directly.
 - Use macros for most of the code.
Changes since v3:
 - Rework tests for the api changes.

Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
---
 lib/locking-selftest.c |  405 ++--
 1 file changed, 386 insertions(+), 19 deletions(-)

diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
index c3eb261..b18f1d3 100644
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -26,6 +26,8 @@
  */
 static unsigned int debug_locks_verbose;
 
+static DEFINE_WW_CLASS(ww_lockdep);
+
 static int __init setup_debug_locks_verbose(char *str)
 {
get_option(str, debug_locks_verbose);
@@ -42,6 +44,10 @@ __setup(debug_locks_verbose=, setup_debug_locks_verbose);
 #define LOCKTYPE_RWLOCK0x2
 #define LOCKTYPE_MUTEX 0x4
 #define LOCKTYPE_RWSEM 0x8
+#define LOCKTYPE_WW0x10
+
+static struct ww_acquire_ctx t, t2;
+static struct ww_mutex o, o2;
 
 /*
  * Normal standalone locks, for the circular and irq-context
@@ -193,6 +199,16 @@ static void init_shared_classes(void)
 #define RSU(x) up_read(rwsem_##x)
 #define RWSI(x)init_rwsem(rwsem_##x)
 
+#define WWAI(x)ww_acquire_init(x, ww_lockdep)
+#define WWAD(x)ww_acquire_done(x)
+#define WWAF(x)ww_acquire_fini(x)
+
+#define WWL(x, c)  ww_mutex_lock(x, c)
+#define WWT(x) ww_mutex_trylock(x)
+#define WWL1(x)ww_mutex_lock(x, NULL)
+#define WWU(x) ww_mutex_unlock(x)
+
+
 #define LOCK_UNLOCK_2(x,y) LOCK(x); LOCK(y); UNLOCK(y); UNLOCK(x)
 
 /*
@@ -894,11 +910,13 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft)
 # define I_RWLOCK(x)   lockdep_reset_lock(rwlock_##x.dep_map)
 # define I_MUTEX(x)lockdep_reset_lock(mutex_##x.dep_map)
 # define I_RWSEM(x)lockdep_reset_lock(rwsem_##x.dep_map)
+# define I_WW(x)   lockdep_reset_lock(x.dep_map)
 #else
 # define I_SPINLOCK(x)
 # define I_RWLOCK(x)
 # define I_MUTEX(x)
 # define I_RWSEM(x)
+# define I_WW(x)
 #endif
 
 #define I1(x)  \
@@ -920,11 +938,20 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft)
 static void reset_locks(void)
 {
local_irq_disable();
+   lockdep_free_key_range(ww_lockdep.acquire_key, 1);
+   lockdep_free_key_range(ww_lockdep.mutex_key, 1);
+
I1(A); I1(B); I1(C); I1(D);
I1(X1); I1(X2); I1(Y1); I1(Y2); I1(Z1); I1(Z2);
+   I_WW(t); I_WW(t2); I_WW(o.base); I_WW(o2.base);
lockdep_reset();
I2(A); I2(B); I2(C); I2(D);
init_shared_classes();
+
+   ww_mutex_init(o, ww_lockdep); ww_mutex_init(o2, ww_lockdep);
+   memset(t, 0, sizeof(t)); memset(t2, 0, sizeof(t2));
+   memset(ww_lockdep.acquire_key, 0, sizeof(ww_lockdep.acquire_key));
+   memset(ww_lockdep.mutex_key, 0, sizeof(ww_lockdep.mutex_key));
local_irq_enable();
 }
 
@@ -938,7 +965,6 @@ static int unexpected_testcase_failures;
 static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
 {
unsigned long saved_preempt_count = preempt_count();
-   int expected_failure = 0;
 
WARN_ON(irqs_disabled());
 
@@ -946,26 +972,16 @@ static void dotest(void (*testcase_fn)(void), int 
expected, int lockclass_mask)
/*
 * Filter out expected failures:
 */
+   if (debug_locks != expected) {
 #ifndef CONFIG_PROVE_LOCKING
-   if ((lockclass_mask  LOCKTYPE_SPIN)  debug_locks != expected)
-   expected_failure = 1;
-   if ((lockclass_mask  LOCKTYPE_RWLOCK)  debug_locks != expected)
-   expected_failure = 1;
-   if ((lockclass_mask  LOCKTYPE_MUTEX)  debug_locks != expected)
-   expected_failure = 1;
-   if ((lockclass_mask  LOCKTYPE_RWSEM)  debug_locks != expected)
-   expected_failure = 1;
+   expected_testcase_failures++;
+   printk(failed|);
+#else
+   unexpected_testcase_failures++;
+   printk(FAILED|);
+
+   dump_stack();
 #endif
-   if (debug_locks != expected) {
-   if (expected_failure) {
-   expected_testcase_failures++;
-   printk(failed|);
-   } else {
-   unexpected_testcase_failures++;
-
-   printk(FAILED|);
-   dump_stack();
-   }
} else {
testcase_successes++;
printk(  ok  |);
@@ -1108,6 +1124,355 @@ static inline void print_testname(const char *testname)
DO_TESTCASE_6IRW(desc, name, 312);  \

[PATCH v4 2/4] mutex: add support for wound/wait style locks, v5

[Maarten Lankhorst]

Changes since RFC patch v1:
 - Updated to use atomic_long instead of atomic, since the reservation_id was a 
long.
 - added mutex_reserve_lock_slow and mutex_reserve_lock_intr_slow
 - removed mutex_locked_set_reservation_id (or w/e it was called)
Changes since RFC patch v2:
 - remove use of __mutex_lock_retval_arg, add warnings when using wrong 
combination of
   mutex_(,reserve_)lock/unlock.
Changes since v1:
 - Add __always_inline to __mutex_lock_common, otherwise reservation paths can 
be
   triggered from normal locks, because __builtin_constant_p might evaluate to 
false
   for the constant 0 in that case. Tests for this have been added in the next 
patch.
 - Updated documentation slightly.
Changes since v2:
 - Renamed everything to ww_mutex. (mlankhorst)
 - Added ww_acquire_ctx and ww_class. (mlankhorst)
 - Added a lot of checks for wrong api usage. (mlankhorst)
 - Documentation updates. (danvet)
Changes since v3:
 - Small documentation fixes (robclark)
 - Memory barrier fix (danvet)
Changes since v4:
 - Remove ww_mutex_unlock_single and ww_mutex_lock_single.
 - Rename ww_mutex_trylock_single to ww_mutex_trylock.
 - Remove separate implementations of ww_mutex_lock_slow*, normal
   functions can be used. Inline versions still exist for extra
   debugging.
 - Cleanup unneeded memory barriers, add comment to the remaining
   smp_mb().

Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Signed-off-by: Rob Clark robdcl...@gmail.com
---
 Documentation/ww-mutex-design.txt |  344 
 include/linux/mutex-debug.h   |1 
 include/linux/mutex.h |  355 +
 kernel/mutex.c|  318 +++--
 lib/debug_locks.c |2 
 5 files changed, 1003 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/ww-mutex-design.txt

diff --git a/Documentation/ww-mutex-design.txt 
b/Documentation/ww-mutex-design.txt
new file mode 100644
index 000..8bd1761
--- /dev/null
+++ b/Documentation/ww-mutex-design.txt
@@ -0,0 +1,344 @@
+Wait/Wound Deadlock-Proof Mutex Design
+==
+
+Please read mutex-design.txt first, as it applies to wait/wound mutexes too.
+
+Motivation for WW-Mutexes
+-
+
+GPU's do operations that commonly involve many buffers.  Those buffers
+can be shared across contexts/processes, exist in different memory
+domains (for example VRAM vs system memory), and so on.  And with
+PRIME / dmabuf, they can even be shared across devices.  So there are
+a handful of situations where the driver needs to wait for buffers to
+become ready.  If you think about this in terms of waiting on a buffer
+mutex for it to become available, this presents a problem because
+there is no way to guarantee that buffers appear in a execbuf/batch in
+the same order in all contexts.  That is directly under control of
+userspace, and a result of the sequence of GL calls that an application
+makes. Which results in the potential for deadlock.  The problem gets
+more complex when you consider that the kernel may need to migrate the
+buffer(s) into VRAM before the GPU operates on the buffer(s), which
+may in turn require evicting some other buffers (and you don't want to
+evict other buffers which are already queued up to the GPU), but for a
+simplified understanding of the problem you can ignore this.
+
+The algorithm that TTM came up with for dealing with this problem is quite
+simple.  For each group of buffers (execbuf) that need to be locked, the caller
+would be assigned a unique reservation id/ticket, from a global counter.  In
+case of deadlock while locking all the buffers associated with a execbuf, the
+one with the lowest reservation ticket (i.e. the oldest task) wins, and the one
+with the higher reservation id (i.e. the younger task) unlocks all of the
+buffers that it has already locked, and then tries again.
+
+In the RDBMS literature this deadlock handling approach is called wait/wound:
+The older tasks waits until it can acquire the contended lock. The younger 
tasks
+needs to back off and drop all the locks it is currently holding, i.e. the
+younger task is wounded.
+
+Concepts
+
+
+Compared to normal mutexes two additional concepts/objects show up in the lock
+interface for w/w mutexes:
+
+Acquire context: To ensure eventual forward progress it is important the a task
+trying to acquire locks doesn't grab a new reservation id, but keeps the one it
+acquired when starting the lock acquisition. This ticket is stored in the
+acquire context. Furthermore the acquire context keeps track of debugging state
+to catch w/w mutex interface abuse.
+
+W/w class: In contrast to normal mutexes the lock class needs to be explicit 
for
+w/w mutexes, since it is required to initialize the acquire context.
+
+Furthermore there are three different class of w/w lock 

RE: Introduce a new helper framework for buffer synchronization

[Inki Dae]

 -Original Message-
 From: linux-fbdev-ow...@vger.kernel.org [mailto:linux-fbdev-
 ow...@vger.kernel.org] On Behalf Of Rob Clark
 Sent: Tuesday, May 28, 2013 10:49 PM
 To: Inki Dae
 Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
 Park; myungjoo.ham; DRI mailing list;
linux-arm-ker...@lists.infradead.org;
 linux-media@vger.kernel.org
 Subject: Re: Introduce a new helper framework for buffer synchronization
 
 On Mon, May 27, 2013 at 11:56 PM, Inki Dae inki@samsung.com wrote:
 
 
  -Original Message-
  From: linux-fbdev-ow...@vger.kernel.org [mailto:linux-fbdev-
  ow...@vger.kernel.org] On Behalf Of Rob Clark
  Sent: Tuesday, May 28, 2013 12:48 AM
  To: Inki Dae
  Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho;
 Kyungmin
  Park; myungjoo.ham; DRI mailing list;
  linux-arm-ker...@lists.infradead.org;
  linux-media@vger.kernel.org
  Subject: Re: Introduce a new helper framework for buffer
 synchronization
 
  On Mon, May 27, 2013 at 6:38 AM, Inki Dae inki@samsung.com wrote:
   Hi all,
  
   I have been removed previous branch and added new one with more
 cleanup.
   This time, the fence helper doesn't include user side interfaces and
  cache
   operation relevant codes anymore because not only we are not sure
 that
   coupling those two things, synchronizing caches and buffer access
  between
   CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side
 is
  a
   good idea yet but also existing codes for user side have problems
 with
  badly
   behaved or crashing userspace. So this could be more discussed later.
  
   The below is a new branch,
  
   https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
  exynos.git/?h=dma-f
   ence-helper
  
   And fence helper codes,
  
   https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
  exynos.git/commit/?
   h=dma-fence-helperid=adcbc0fe7e285ce866e5816e5e21443dcce01005
  
   And example codes for device driver,
  
   https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
  exynos.git/commit/?
   h=dma-fence-helperid=d2ce7af23835789602a99d0ccef1f53cdd5caaae
  
   I think the time is not yet ripe for RFC posting: maybe existing dma
  fence
   and reservation need more review and addition work. So I'd glad for
  somebody
   giving other opinions and advices in advance before RFC posting.
 
  thoughts from a *really* quick, pre-coffee, first look:
  * any sort of helper to simplify single-buffer sort of use-cases (v4l)
  probably wouldn't want to bake in assumption that seqno_fence is used.
  * I guess g2d is probably not actually a simple use case, since I
  expect you can submit blits involving multiple buffers :-P
 
  I don't think so. One and more buffers can be used: seqno_fence also has
  only one buffer. Actually, we have already applied this approach to most
  devices; multimedia, gpu and display controller. And this approach shows
  more performance; reduced power consumption against traditional way. And
 g2d
  example is just to show you how to apply my approach to device driver.
 
 no, you need the ww-mutex / reservation stuff any time you have
 multiple independent devices (or rings/contexts for hw that can
 support multiple contexts) which can do operations with multiple
 buffers.

I think I already used reservation stuff any time in that way except
ww-mutex. And I'm not sure that embedded system really needs ww-mutex. If
there is any case, 
could you tell me the case? I really need more advice and understanding :)

Thanks,
Inki Dae

  So you could conceivably hit this w/ gpu + g2d if multiple
 buffers where shared between the two.  vram migration and such
 'desktop stuff' might make the problem worse, but just because you
 don't have vram doesn't mean you don't have a problem with multiple
 buffers.
 
  * otherwise, you probably don't want to depend on dmabuf, which is why
  reservation/fence is split out the way it is..  you want to be able to
  use a single reservation/fence mechanism within your driver without
  having to care about which buffers are exported to dmabuf's and which
  are not.  Creating a dmabuf for every GEM bo is too heavyweight.
 
  Right. But I think we should dealt with this separately. Actually, we
 are
  trying to use reservation for gpu pipe line synchronization such as sgx
 sync
  object and this approach is used without dmabuf. In order words, some
 device
  can use only reservation for such pipe line synchronization and at the
 same
  time, fence helper or similar thing with dmabuf for buffer
 synchronization.
 
 it is probably easier to approach from the reverse direction.. ie, get
 reservation/synchronization right first, and then dmabuf.  (Well, that
 isn't really a problem because Maarten's reservation/fence patches
 support dmabuf from the beginning.)
 
 BR,
 -R
 
 
  I'm not entirely sure if reservation/fence could/should be made any
  simpler for multi-buffer users.  Probably the best thing to do is just
  get reservation/fence 

[PATCH v4 0/4] add mutex wait/wound/style style locks

[Maarten Lankhorst]

Version 4 already?

Small api changes since v3:
- Remove ww_mutex_unlock_single and ww_mutex_lock_single.
- Rename ww_mutex_trylock_single to ww_mutex_trylock.
- Remove separate implementations of ww_mutex_lock_slow*, normal
  functions can be used. Inline versions still exist for extra
  debugging, and to annotate.
- Cleanup unneeded memory barriers, add comment to the remaining
  smp_mb().

Thanks to Daniel Vetter, Rob Clark and Peter Zijlstra for their feedback.
---

Daniel Vetter (1):
  mutex: w/w mutex slowpath debugging

Maarten Lankhorst (3):
  arch: make __mutex_fastpath_lock_retval return whether fastpath succeeded 
or not.
  mutex: add support for wound/wait style locks, v5
  mutex: Add ww tests to lib/locking-selftest.c. v4


 Documentation/ww-mutex-design.txt |  344 +++
 arch/ia64/include/asm/mutex.h |   10 -
 arch/powerpc/include/asm/mutex.h  |   10 -
 arch/sh/include/asm/mutex-llsc.h  |4 
 arch/x86/include/asm/mutex_32.h   |   11 -
 arch/x86/include/asm/mutex_64.h   |   11 -
 include/asm-generic/mutex-dec.h   |   10 -
 include/asm-generic/mutex-null.h  |2 
 include/asm-generic/mutex-xchg.h  |   10 -
 include/linux/mutex-debug.h   |1 
 include/linux/mutex.h |  363 +
 kernel/mutex.c|  384 ---
 lib/Kconfig.debug |   13 +
 lib/debug_locks.c |2 
 lib/locking-selftest.c|  410 +++--
 15 files changed, 1492 insertions(+), 93 deletions(-)
 create mode 100644 Documentation/ww-mutex-design.txt

-- 
~Maarten
--
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] go7007 firmware updates

[Pete Eberlein]

Hi Hans,

On 05/27/2013 12:56 PM, Hans Verkuil wrote:

I can revert the rename action,  but I would rather not do it. I

 believe there are good reasons for doing this, especially since the
 current situation is effectively broken anyway due to the missing
 firmware files.

 If you really don't want to rename the two S2250 files, then I'll
 make a patch reverting those to the original filename.

 Pete, if you have an opinion regarding this, please let us know.
 After all, it concerns a Sensoray device.

I am okay with the change of the firmware filenames.  I've changed them 
myself several times in the past before it got into kernel staging.



Regards,


 Hans


Regards,
Pete
--
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: Introduce a new helper framework for buffer synchronization

[Daniel Vetter]

On Tue, May 28, 2013 at 4:50 PM, Inki Dae inki@samsung.com wrote:
 I think I already used reservation stuff any time in that way except
 ww-mutex. And I'm not sure that embedded system really needs ww-mutex. If
 there is any case,
 could you tell me the case? I really need more advice and understanding :)

If you have only one driver, you can get away without ww_mutex.
drm/i915 does it, all buffer state is protected by dev-struct_mutex.
But as soon as you have multiple drivers sharing buffers with dma_buf
things will blow up.

Yep, current prime is broken and can lead to deadlocks.

In practice it doesn't (yet) matter since only the X server does the
sharing dance, and that one's single-threaded. Now you can claim that
since you have all buffers pinned in embedded gfx anyway, you don't
care. But both in desktop gfx and embedded gfx the real fun starts
once you put fences into the mix and link them up with buffers, then
every command submission risks that deadlock. Furthermore you can get
unlucky and construct a circle of fences waiting on each another (only
though if the fence singalling fires off the next batchbuffer
asynchronously).

To prevent such deadlocks you _absolutely_ need to lock _all_ buffers
that take part in a command submission at once. To do that you either
need a global lock (ugh) or ww_mutexes.

So ww_mutexes are the fundamental ingredient of all this, if you don't
see why you need them then everything piled on top is broken. I think
until you've understood why exactly we need ww_mutexes there's not
much point in discussing the finer issues of fences, reservation
objects and how to integrate it with dma_bufs exactly.

I'll try to clarify the motivating example in the ww_mutex
documentation a bit, but I dunno how else I could explain this ...

Yours, 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 09/13] media: Change media device link_notify behaviour

[Sylwester Nawrocki]

Hi All,

(replying to myself, probably a bad sign... ;))

On 05/09/2013 05:36 PM, Sylwester Nawrocki wrote:
 Currently the media device link_notify callback is invoked before the
 actual change of state of a link when the link is being enabled, and
 after the actual change of state when the link is being disabled.
 
 This doesn't allow a media device driver to perform any operations
 on a full graph before a link is disabled, as well as performing
 any tasks on a modified graph right after a link's state is changed.
 
 This patch modifies signature of the link_notify callback. This
 callback is now called always before and after a link's state change.
 To distinguish the notifications a 'notification' argument is added
 to the link_notify callback: MEDIA_DEV_NOTIFY_PRE_LINK_CH indicates
 notification before link's state change and
 MEDIA_DEV_NOTIFY_POST_LINK_CH corresponds to a notification after
 link flags change.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/media/media-entity.c  |   18 +++
  drivers/media/platform/exynos4-is/media-dev.c |   16 +-
  drivers/media/platform/omap3isp/isp.c |   41 
 +++--
  include/media/media-device.h  |9 --
  4 files changed, 46 insertions(+), 38 deletions(-)
 
 diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
 index 7c2b51c..0fcf4c0 100644
 --- a/drivers/media/media-entity.c
 +++ b/drivers/media/media-entity.c
 @@ -547,25 +547,17 @@ int __media_entity_setup_link(struct media_link *link, 
 u32 flags)
  
   mdev = source-parent;
  
 - if ((flags  MEDIA_LNK_FL_ENABLED)  mdev-link_notify) {
 - ret = mdev-link_notify(link-source, link-sink,
 - MEDIA_LNK_FL_ENABLED);
 + if (mdev-link_notify) {
 + ret = mdev-link_notify(link, MEDIA_LNK_FL_ENABLED,
 + MEDIA_DEV_NOTIFY_PRE_LINK_CH);

After some testing I found it really should have been

ret = mdev-link_notify(link, flags,
MEDIA_DEV_NOTIFY_PRE_LINK_CH);

Otherwise the pipeline never gets powered off upon link disconnection.

I will repost the whole series in the end of week, after some more testing.
Still, any further comments on this are appreciated.

Thanks,
Sylwester
   if (ret  0)
   return ret;
   }
  
   ret = __media_entity_setup_link_notify(link, flags);
 - if (ret  0)
 - goto err;
  
 - if (!(flags  MEDIA_LNK_FL_ENABLED)  mdev-link_notify)
 - mdev-link_notify(link-source, link-sink, 0);
 -
 - return 0;
 -
 -err:
 - if ((flags  MEDIA_LNK_FL_ENABLED)  mdev-link_notify)
 - mdev-link_notify(link-source, link-sink, 0);
 + if (mdev-link_notify)
 + mdev-link_notify(link, flags, MEDIA_DEV_NOTIFY_POST_LINK_CH);
  
   return ret;
  }

-- 
Sylwester Nawrocki
Samsung RD Institute Poland
Samsung Electronics
--
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

[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 May 28 19:00:18 CEST 2013
git branch: test
git hash:   7eac97d7e714429f7ef1ba5d35f94c07f4c34f8e
gcc version:i686-linux-gcc (GCC) 4.8.0
host hardware:  x86_64
host os:3.8-3.slh.2-amd64

linux-git-arm-davinci: OK
linux-git-arm-exynos: WARNINGS
linux-git-arm-omap: WARNINGS
linux-git-blackfin: WARNINGS
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.31.14-i686: WARNINGS
linux-2.6.32.27-i686: WARNINGS
linux-2.6.33.7-i686: WARNINGS
linux-2.6.34.7-i686: WARNINGS
linux-2.6.35.9-i686: WARNINGS
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.10-rc1-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: OK
linux-3.9.2-i686: OK
linux-2.6.31.14-x86_64: WARNINGS
linux-2.6.32.27-x86_64: WARNINGS
linux-2.6.33.7-x86_64: WARNINGS
linux-2.6.34.7-x86_64: WARNINGS
linux-2.6.35.9-x86_64: WARNINGS
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.10-rc1-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: OK
linux-3.9.2-x86_64: OK
apps: WARNINGS
spec-git: OK
sparse: ERRORS

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

cx88 dvb initialization fails

[Martin Kittel]

Hi,

with Debian kernel 3.2 I had the rare problem that dvb on my  Hauppauge
WinTV-HVR1300 was not always initialized properly on boot. After an upgrade
to Debian kernel 3.8 and then for testing reasons to vanilla 3.9.4 I can
reproduce this problem on every boot. This is from my dmesg output for
vanilla 3.9.4 with debug for the cx88* modules enabled:

[6.329655] hda_intel: Disabling MSI
[6.329773] snd_hda_intel :00:07.0: setting latency timer to 64
[6.811772] cx88/2: cx2388x MPEG-TS Driver Manager version 0.0.9 loaded
[6.814199] cx88[0]: subsystem: 0070:9601, board: Hauppauge WinTV-HVR1300
DVB-T/Hybrid MPEG Encoder [card=56,autodetected], frontend(s): 1
[6.814273] cx88[0]: TV tuner type 63, Radio tuner type -1
[6.833393] cx88/0: cx2388x v4l2 driver version 0.0.9 loaded
[6.934169] cx88[0]: i2c init: enabling analog demod on HVR1300/3000/4000
tuner
[6.999507] tda9887 2-0043: creating new instance
[6.999570] tda9887 2-0043: tda988[5/6/7] found
[7.000465] tuner 2-0043: Tuner 74 found with type(s) Radio TV.
[7.004727] tuner 2-0061: Tuner -1 found with type(s) Radio TV.
[7.043558] tveeprom 2-0050: Hauppauge model 96019, rev D6D3, serial# 3106328
[7.043609] tveeprom 2-0050: MAC address is 00:0d:fe:2f:66:18
[7.043657] tveeprom 2-0050: tuner model is Philips FMD1216MEX (idx 133,
type 78)
[7.043717] tveeprom 2-0050: TV standards PAL(B/G) PAL(I) SECAM(L/L')
PAL(D/D1/K) ATSC/DVB Digital (eeprom 0xf4)
[7.043777] tveeprom 2-0050: audio processor is CX882 (idx 33)
[7.043824] tveeprom 2-0050: decoder processor is CX882 (idx 25)
[7.043872] tveeprom 2-0050: has radio, has IR receiver, has IR transmitter
[7.043919] cx88[0]: hauppauge eeprom: model=96019
[7.068691] tuner-simple 2-0061: creating new instance
[7.068744] tuner-simple 2-0061: type set to 78 (Philips FMD1216MEX MK3
Hybrid Tuner)
[7.074643] cx88[0]/2: cx2388x 8802 Driver Manager
[7.075297] ACPI: PCI Interrupt Link [LNKB] enabled at IRQ 17
[7.075388] cx88[0]/2: found at :01:07.2, rev: 5, irq: 17, latency:
64, mmio: 0xfa00
[7.076348] cx88[0]/0: found at :01:07.0, rev: 5, irq: 17, latency:
64, mmio: 0xfc00
[7.134043] cx88/2: cx2388x dvb driver version 0.0.9 loaded
[7.134101] cx88/2: registering cx8802 driver, type: dvb access: shared
[7.134154] cx88[0]/2: subsystem: 0070:9601, board: Hauppauge
WinTV-HVR1300 DVB-T/Hybrid MPEG Encoder [card=56]
[7.134221] cx88[0]/2-dvb: cx8802_dvb_probe
[7.134228] cx88[0]/2-dvb:  -being probed by Card=56 Name=cx88[0], PCI 01:07
[7.134232] cx88[0]/2: cx2388x based DVB/ATSC card
[7.134280] cx8802_alloc_frontends() allocating 1 frontend(s)
[7.147301] i2c i2c-2: sendbytes: NAK bailout.
[7.147380] cx22702_readreg: error (reg == 0x1f, ret == -5)
[7.147429] cx88[0]/2: frontend initialization failed
[7.147477] cx88[0]/2: dvb_register failed (err = -22)
[7.147523] cx88[0]/2: cx8802 probe failed, err = -22
[7.158710] wm8775 2-001b: chip found @ 0x36 (cx88[0])
[7.174150] cx88[0]/0: registered device video0 [v4l2]
[7.174332] cx88[0]/0: registered device vbi0
[7.174477] cx88[0]/0: registered device radio0
[7.202891] cx2388x blackbird driver version 0.0.9 loaded
[7.202949] cx88/2: registering cx8802 driver, type: blackbird access: shared
[7.203002] cx88[0]/2: subsystem: 0070:9601, board: Hauppauge
WinTV-HVR1300 DVB-T/Hybrid MPEG Encoder [card=56]
[7.203143] cx88[0]/2: cx23416 based mpeg encoder (blackbird reference
design)
[7.203421] cx88[0]/2-bb: Firmware and/or mailbox pointer not initialized
or corrupted
...
[9.680686] cx88[0]/2-bb: Firmware upload successful.
[9.689682] cx88[0]/2-bb: Firmware version is 0x02060039
[9.705095] cx88[0]/2: registered device video1 [mpeg]
[9.709579] cx88[0]/2-mpeg: cx8802_request_acquire() Post acquire GPIO=ef9e
[9.715684] cx88[0]/2-mpeg: cx8802_cancel_buffers
[9.715687] cx88[0]/2-mpeg: cx8802_stop_dma
[9.715697] cx88[0]/2-mpeg: cx8802_request_release() Post release GPIO=ef9e
[   11.194791] Adding 979928k swap on /dev/sdb6.  Priority:-1 extents:1
across:979928k 
...
[   12.012055] Registered IR keymap rc-hauppauge
[   12.012300] input: i2c IR (cx88 Hauppauge XVR remo as
/devices/virtual/rc/rc0/input12
[   12.012505] rc0: i2c IR (cx88 Hauppauge XVR remo as /devices/virtual/rc/rc0
[   12.012551] ir-kbd-i2c: i2c IR (cx88 Hauppauge XVR remo detected at
i2c-2/2-0071/ir0 [cx88[0]]
...

After booting, if I remove the modules via 
 modprobe -r cx88_blackbird cx88_dvb cx88_alsa cx88_vp3054_i2c cx22702 
and reinsert them using
 modprobe  cx8802
initialization is running fine and dvb is available:

[  478.523553] cx88/2: cx2388x MPEG-TS Driver Manager version 0.0.9 loaded
[  478.525947] cx88[0]: subsystem: 0070:9601, board: Hauppauge WinTV-HVR1300 DVB
-T/Hybrid MPEG Encoder [card=56,autodetected], frontend(s): 1
[  478.526021] cx88[0]: TV tuner type 63, Radio tuner type -1
[  478.646200] cx88[0]: i2c 

Re: [PATCH v4 3/4] mutex: Add ww tests to lib/locking-selftest.c. v4

[Daniel Vetter]

On Tue, May 28, 2013 at 04:48:45PM +0200, Maarten Lankhorst wrote:
 This stresses the lockdep code in some ways specifically useful to
 ww_mutexes. It adds checks for most of the common locking errors.
 
 Changes since v1:
  - Add tests to verify reservation_id is untouched.
  - Use L() and U() macros where possible.
 
 Changes since v2:
  - Use the ww_mutex api directly.
  - Use macros for most of the code.
 Changes since v3:
  - Rework tests for the api changes.
 
 Signed-off-by: Maarten Lankhorst maarten.lankho...@canonical.com
 ---
  lib/locking-selftest.c |  405 
 ++--
  1 file changed, 386 insertions(+), 19 deletions(-)
 
 diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
 index c3eb261..b18f1d3 100644
 --- a/lib/locking-selftest.c
 +++ b/lib/locking-selftest.c
 @@ -26,6 +26,8 @@
   */
  static unsigned int debug_locks_verbose;
  
 +static DEFINE_WW_CLASS(ww_lockdep);
 +
  static int __init setup_debug_locks_verbose(char *str)
  {
   get_option(str, debug_locks_verbose);
 @@ -42,6 +44,10 @@ __setup(debug_locks_verbose=, setup_debug_locks_verbose);
  #define LOCKTYPE_RWLOCK  0x2
  #define LOCKTYPE_MUTEX   0x4
  #define LOCKTYPE_RWSEM   0x8
 +#define LOCKTYPE_WW  0x10
 +
 +static struct ww_acquire_ctx t, t2;
 +static struct ww_mutex o, o2;
  
  /*
   * Normal standalone locks, for the circular and irq-context
 @@ -193,6 +199,16 @@ static void init_shared_classes(void)
  #define RSU(x)   up_read(rwsem_##x)
  #define RWSI(x)  init_rwsem(rwsem_##x)
  
 +#define WWAI(x)  ww_acquire_init(x, ww_lockdep)
 +#define WWAD(x)  ww_acquire_done(x)
 +#define WWAF(x)  ww_acquire_fini(x)
 +
 +#define WWL(x, c)ww_mutex_lock(x, c)
 +#define WWT(x)   ww_mutex_trylock(x)
 +#define WWL1(x)  ww_mutex_lock(x, NULL)
 +#define WWU(x)   ww_mutex_unlock(x)
 +
 +
  #define LOCK_UNLOCK_2(x,y)   LOCK(x); LOCK(y); UNLOCK(y); UNLOCK(x)
  
  /*
 @@ -894,11 +910,13 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft)
  # define I_RWLOCK(x) lockdep_reset_lock(rwlock_##x.dep_map)
  # define I_MUTEX(x)  lockdep_reset_lock(mutex_##x.dep_map)
  # define I_RWSEM(x)  lockdep_reset_lock(rwsem_##x.dep_map)
 +# define I_WW(x) lockdep_reset_lock(x.dep_map)
  #else
  # define I_SPINLOCK(x)
  # define I_RWLOCK(x)
  # define I_MUTEX(x)
  # define I_RWSEM(x)
 +# define I_WW(x)
  #endif
  
  #define I1(x)\
 @@ -920,11 +938,20 @@ GENERATE_PERMUTATIONS_3_EVENTS(irq_read_recursion_soft)
  static void reset_locks(void)
  {
   local_irq_disable();
 + lockdep_free_key_range(ww_lockdep.acquire_key, 1);
 + lockdep_free_key_range(ww_lockdep.mutex_key, 1);
 +
   I1(A); I1(B); I1(C); I1(D);
   I1(X1); I1(X2); I1(Y1); I1(Y2); I1(Z1); I1(Z2);
 + I_WW(t); I_WW(t2); I_WW(o.base); I_WW(o2.base);
   lockdep_reset();
   I2(A); I2(B); I2(C); I2(D);
   init_shared_classes();
 +
 + ww_mutex_init(o, ww_lockdep); ww_mutex_init(o2, ww_lockdep);
 + memset(t, 0, sizeof(t)); memset(t2, 0, sizeof(t2));
 + memset(ww_lockdep.acquire_key, 0, sizeof(ww_lockdep.acquire_key));
 + memset(ww_lockdep.mutex_key, 0, sizeof(ww_lockdep.mutex_key));
   local_irq_enable();
  }
  
 @@ -938,7 +965,6 @@ static int unexpected_testcase_failures;
  static void dotest(void (*testcase_fn)(void), int expected, int 
 lockclass_mask)
  {
   unsigned long saved_preempt_count = preempt_count();
 - int expected_failure = 0;
  
   WARN_ON(irqs_disabled());
  
 @@ -946,26 +972,16 @@ static void dotest(void (*testcase_fn)(void), int 
 expected, int lockclass_mask)
   /*
* Filter out expected failures:
*/
 + if (debug_locks != expected) {
  #ifndef CONFIG_PROVE_LOCKING
 - if ((lockclass_mask  LOCKTYPE_SPIN)  debug_locks != expected)
 - expected_failure = 1;
 - if ((lockclass_mask  LOCKTYPE_RWLOCK)  debug_locks != expected)
 - expected_failure = 1;
 - if ((lockclass_mask  LOCKTYPE_MUTEX)  debug_locks != expected)
 - expected_failure = 1;
 - if ((lockclass_mask  LOCKTYPE_RWSEM)  debug_locks != expected)
 - expected_failure = 1;
 + expected_testcase_failures++;
 + printk(failed|);
 +#else
 + unexpected_testcase_failures++;
 + printk(FAILED|);
 +
 + dump_stack();
  #endif
 - if (debug_locks != expected) {
 - if (expected_failure) {
 - expected_testcase_failures++;
 - printk(failed|);
 - } else {
 - unexpected_testcase_failures++;
 -
 - printk(FAILED|);
 - dump_stack();
 - }
   } else {
   testcase_successes++;
   printk(  ok  |);
 @@ -1108,6 

Re: [PATCH v4 3/4] mutex: Add ww tests to lib/locking-selftest.c. v4

[Maarten Lankhorst]

Op 28-05-13 21:18, Daniel Vetter schreef:
 On Tue, May 28, 2013 at 04:48:45PM +0200, Maarten Lankhorst wrote:
 This stresses the lockdep code in some ways specifically useful to
 ww_mutexes. It adds checks for most of the common locking errors.

 Changes since v1:
  - Add tests to verify reservation_id is untouched.
  - Use L() and U() macros where possible.

 Changes since v2:
  - Use the ww_mutex api directly.
  - Use macros for most of the code.
 Changes since v3:
  - Rework tests for the api changes.

 snip

 +static void ww_test_normal(void)
 +{
 +int ret;
 +
 +WWAI(t);
 +
 +/*
 + * test if ww_id is kept identical if not
 + * called with any of the ww_* locking calls
 + */
 +
 +/* mutex_lock (and indirectly, mutex_lock_nested) */
 +o.ctx = (void *)~0UL;
 +mutex_lock(o.base);
 +mutex_unlock(o.base);
 +WARN_ON(o.ctx != (void *)~0UL);
 +
 +/* mutex_lock_interruptible (and *_nested) */
 +o.ctx = (void *)~0UL;
 +ret = mutex_lock_interruptible(o.base);
 +if (!ret)
 +mutex_unlock(o.base);
 +else
 +WARN_ON(1);
 +WARN_ON(o.ctx != (void *)~0UL);
 +
 +/* mutex_lock_killable (and *_nested) */
 +o.ctx = (void *)~0UL;
 +ret = mutex_lock_killable(o.base);
 +if (!ret)
 +mutex_unlock(o.base);
 +else
 +WARN_ON(1);
 +WARN_ON(o.ctx != (void *)~0UL);
 +
 +/* trylock, succeeding */
 +o.ctx = (void *)~0UL;
 +ret = mutex_trylock(o.base);
 +WARN_ON(!ret);
 +if (ret)
 +mutex_unlock(o.base);
 +else
 +WARN_ON(1);
 +WARN_ON(o.ctx != (void *)~0UL);
 +
 +/* trylock, failing */
 +o.ctx = (void *)~0UL;
 +mutex_lock(o.base);
 +ret = mutex_trylock(o.base);
 +WARN_ON(ret);
 +mutex_unlock(o.base);
 +WARN_ON(o.ctx != (void *)~0UL);
 +
 +/* nest_lock */
 +o.ctx = (void *)~0UL;
 +mutex_lock_nest_lock(o.base, t);
 +mutex_unlock(o.base);
 +WARN_ON(o.ctx != (void *)~0UL);
 +}
 Since we don't really allow this any more (instead allow ww_mutex_lock
 without context) do we need this test here really?
Yes. This test verifies all the normal locking paths are not affected by the 
ww_ctx changes.

 +
 +static void ww_test_two_contexts(void)
 +{
 +WWAI(t);
 +WWAI(t2);
 +}
 +
 +static void ww_test_context_unlock_twice(void)
 +{
 +WWAI(t);
 +WWAD(t);
 +WWAF(t);
 +WWAF(t);
 +}
 +
 +static void ww_test_object_unlock_twice(void)
 +{
 +WWL1(o);
 +WWU(o);
 +WWU(o);
 +}
 +
 +static void ww_test_spin_nest_unlocked(void)
 +{
 +raw_spin_lock_nest_lock(lock_A, o.base);
 +U(A);
 +}
 I don't quite see the point of this one here ...
It's a lockdep test that was missing. o.base is not locked. So lock_A is being 
nested into an unlocked lock, resulting in a lockdep error.

 +
 +static void ww_test_unneeded_slow(void)
 +{
 +int ret;
 +
 +WWAI(t);
 +
 +ww_mutex_lock_slow(o, t);
 +}
 I think checking the _slow debug stuff would be neat, i.e.
 - fail/success tests for properly unlocking all held locks
 - fail/success tests for lock_slow acquiring the right lock.

 Otherwise I didn't spot anything that seems missing in these self-tests
 here.

Yes it would be nice, doing so is left as an excercise for the reviewer, who 
failed to raise this point sooner. ;-)

~Maarten
--
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/5] media: davinci: vpif: Convert to devm_* api

[Laurent Pinchart]

Hi Sergei,

On Sunday 26 May 2013 18:15:19 Sergei Shtylyov wrote:
 On 26-05-2013 4:49, Laurent Pinchart wrote:
  From: Lad, Prabhakar prabhakar.cse...@gmail.com
  
  Use devm_ioremap_resource instead of reques_mem_region()/ioremap().
  This ensures more consistent error values and simplifies error paths.
  
  Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
  ---
  
drivers/media/platform/davinci/vpif.c |   27
---
1 files changed, 4 insertions(+), 23 deletions(-)
  
  diff --git a/drivers/media/platform/davinci/vpif.c
  b/drivers/media/platform/davinci/vpif.c index 761c825..164c1b7 100644
  --- a/drivers/media/platform/davinci/vpif.c
  +++ b/drivers/media/platform/davinci/vpif.c
 
 [...]
 
  @@ -421,23 +419,12 @@ EXPORT_SYMBOL(vpif_channel_getfid);
  
static int vpif_probe(struct platform_device *pdev)
{
  
  -  int status = 0;
  +  static struct resource  *res;
  
 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  
  -  if (!res)
  -  return -ENOENT;
  -
  -  res_len = resource_size(res);
  -
  -  res = request_mem_region(res-start, res_len, res-name);
  -  if (!res)
  -  return -EBUSY;
  -
  -  vpif_base = ioremap(res-start, res_len);
  -  if (!vpif_base) {
  -  status = -EBUSY;
  -  goto fail;
  -  }
  +  vpif_base = devm_ioremap_resource(pdev-dev, res);
  +  if (IS_ERR(vpif_base))
  +  return PTR_ERR(vpif_base);
  
  You're loosing the request_mem_region().
 
 He's not losing anything, first look at how devm_ioremp_resource() is
 defined.

My bad. I'm not sure where I got that idea from. Thanks for pointing out the 
mistake.

  You should use devm_request_and_ioremap()
 
  Already deprecated by now.
 
  function instead of devm_ioremap_resource(). With that change,
  
  Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

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

Re: [PATCH v5] media: i2c: tvp514x: add OF support

[Laurent Pinchart]

Hi Prabhakar,

Thanks for the patch.

On Sunday 26 May 2013 18:49:46 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 add OF support for the tvp514x driver.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@redhat.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Rob Landley r...@landley.net
 Cc: devicetree-disc...@lists.ozlabs.org
 Cc: linux-...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org
 Cc: davinci-linux-open-sou...@linux.davincidsp.com

Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

(with two small comment below).

 ---
 Tested on da850-evm.
 
  RFC v1: https://patchwork.kernel.org/patch/2030061/
  RFC v2: https://patchwork.kernel.org/patch/2061811/
 
  Changes for current version from RFC v2:
  1: Fixed review comments pointed by Sylwester.
 
  Changes for v2:
  1: Listed all the compatible property values in the documentation text
 file. 2: Removed -decoder from compatible property values.
  3: Added a reference to the V4L2 DT bindings documentation to explain
 what the port and endpoint nodes are for.
  4: Fixed some Nits pointed by Laurent.
  5: Removed unnecessary header file includes and sort them alphabetically.
 
  Changes for v3:
  1: Rebased on patch https://patchwork.kernel.org/patch/2539411/
 
  Changes for v4:
  1: added missing call for of_node_put().
  2: Rebased the patch on v3.11.
 
  Changes for v5:
  1: Fixed calling to a wrong label.
 
  .../devicetree/bindings/media/i2c/tvp514x.txt  |   45 ++
  drivers/media/i2c/tvp514x.c|   62 +++--
  2 files changed, 101 insertions(+), 6 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/media/i2c/tvp514x.txt
 
 diff --git a/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
 b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt new file mode
 100644
 index 000..cc09424
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/i2c/tvp514x.txt
 @@ -0,0 +1,45 @@
 +* Texas Instruments TVP514x video decoder
 +
 +The TVP5146/TVP5146m2/TVP5147/TVP5147m1 device is high quality, single-chip
 +digital video decoder that digitizes and decodes all popular baseband
 analog +video formats into digital video component. The tvp514x decoder
 supports analog- +to-digital (A/D) conversion of component RGB and YPbPr
 signals as well as A/D +conversion and decoding of NTSC, PAL and SECAM
 composite and S-video into +component YCbCr.
 +
 +Required Properties :
 +- compatible : value should be either one among the following
 + (a) ti,tvp5146 for tvp5146 decoder.
 + (b) ti,tvp5146m2 for tvp5146m2 decoder.
 + (c) ti,tvp5147 for tvp5147 decoder.
 + (d) ti,tvp5147m1 for tvp5147m1 decoder.
 +
 +- hsync-active: HSYNC Polarity configuration for endpoint.
 +
 +- vsync-active: VSYNC Polarity configuration for endpoint.
 +
 +- pclk-sample: Clock polarity of the endpoint.
 +
 +
 +For further reading of port node refer

s/of port/on port/
s/refer/refer to/

 Documentation/devicetree/bindings/media/video-interfaces.txt.
 +
 +Example:
 +
 + i2c0@1c22000 {
 + ...
 + ...
 + tvp514x@5c {
 + compatible = ti,tvp5146;
 + reg = 0x5c;
 +
 + port {
 + tvp514x_1: endpoint {
 + hsync-active = 1;
 + vsync-active = 1;
 + pclk-sample = 0;
 + };
 + };
 + };
 + ...
 + };
 diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
 index 7438e01..7ed999b 100644
 --- a/drivers/media/i2c/tvp514x.c
 +++ b/drivers/media/i2c/tvp514x.c
 @@ -39,6 +39,7 @@
  #include media/v4l2-device.h
  #include media/v4l2-common.h
  #include media/v4l2-mediabus.h
 +#include media/v4l2-of.h
  #include media/v4l2-chip-ident.h
  #include media/v4l2-ctrls.h
  #include media/tvp514x.h
 @@ -1055,6 +1056,42 @@ static struct tvp514x_decoder tvp514x_dev = {
 
  };
 
 +static struct tvp514x_platform_data *
 +tvp514x_get_pdata(struct i2c_client *client)
 +{
 + struct tvp514x_platform_data *pdata = NULL;

No need to initialize pdata to NULL.

 + struct v4l2_of_endpoint bus_cfg;
 + struct device_node *endpoint;
 + unsigned int flags;
 +
 + if (!IS_ENABLED(CONFIG_OF) || !client-dev.of_node)
 + return client-dev.platform_data;
 +
 + endpoint = v4l2_of_get_next_endpoint(client-dev.of_node, NULL);
 + if (!endpoint)
 + return NULL;
 +
 + pdata = devm_kzalloc(client-dev, sizeof(*pdata), GFP_KERNEL);
 + 

RE: Introduce a new helper framework for buffer synchronization

[Inki Dae]

 -Original Message-
 From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf Of
 Daniel Vetter
 Sent: Wednesday, May 29, 2013 1:50 AM
 To: Inki Dae
 Cc: Rob Clark; Maarten Lankhorst; linux-fbdev; YoungJun Cho; Kyungmin
Park;
 myungjoo.ham; DRI mailing list; linux-arm-ker...@lists.infradead.org;
 linux-media@vger.kernel.org
 Subject: Re: Introduce a new helper framework for buffer synchronization
 
 On Tue, May 28, 2013 at 4:50 PM, Inki Dae inki@samsung.com wrote:
  I think I already used reservation stuff any time in that way except
  ww-mutex. And I'm not sure that embedded system really needs ww-mutex.
 If
  there is any case,
  could you tell me the case? I really need more advice and
 understanding :)
 
 If you have only one driver, you can get away without ww_mutex.
 drm/i915 does it, all buffer state is protected by dev-struct_mutex.
 But as soon as you have multiple drivers sharing buffers with dma_buf
 things will blow up.
 
 Yep, current prime is broken and can lead to deadlocks.
 
 In practice it doesn't (yet) matter since only the X server does the
 sharing dance, and that one's single-threaded. Now you can claim that
 since you have all buffers pinned in embedded gfx anyway, you don't
 care. But both in desktop gfx and embedded gfx the real fun starts
 once you put fences into the mix and link them up with buffers, then
 every command submission risks that deadlock. Furthermore you can get
 unlucky and construct a circle of fences waiting on each another (only
 though if the fence singalling fires off the next batchbuffer
 asynchronously).

In our case, we haven't ever experienced deadlock yet but there is still
possible to face with deadlock in case that a process is sharing two buffer
with another process like below,
Process A committed buffer A and  waits for buffer B,
Process B committed buffer B and waits for buffer A

That is deadlock and it seems that you say we can resolve deadlock issue
with ww-mutexes. And it seems that we can replace our block-wakeup mechanism
with mutex lock for more performance.

 
 To prevent such deadlocks you _absolutely_ need to lock _all_ buffers
 that take part in a command submission at once. To do that you either
 need a global lock (ugh) or ww_mutexes.
 
 So ww_mutexes are the fundamental ingredient of all this, if you don't
 see why you need them then everything piled on top is broken. I think
 until you've understood why exactly we need ww_mutexes there's not
 much point in discussing the finer issues of fences, reservation
 objects and how to integrate it with dma_bufs exactly.
 
 I'll try to clarify the motivating example in the ww_mutex
 documentation a bit, but I dunno how else I could explain this ...
 

I don't really want for you waste your time on me. I will trying to apply
ww-mutexes (v4) to the proposed framework for more understanding.

Thanks for your advices.:) 
Inki Dae

 Yours, 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 v3 4/9] media: davinci: vpif_capture: move the freeing of irq and global variables to remove()

[Laurent Pinchart]

Hi Prabhakar,

Thanks for the patch.

On Sunday 26 May 2013 17:30:07 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 Ideally the freeing of irq's and the global variables needs to be
 done in the remove() rather than module_exit(), this patch moves
 the freeing up of irq's and freeing the memory allocated to channel
 objects to remove() callback of struct platform_driver.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---
  drivers/media/platform/davinci/vpif_capture.c |   31 ++
  1 files changed, 13 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/media/platform/davinci/vpif_capture.c
 b/drivers/media/platform/davinci/vpif_capture.c index caaf4fe..f8b7304
 100644
 --- a/drivers/media/platform/davinci/vpif_capture.c
 +++ b/drivers/media/platform/davinci/vpif_capture.c
 @@ -2225,17 +2225,29 @@ vpif_int_err:
   */
  static int vpif_remove(struct platform_device *device)
  {
 - int i;
 + struct platform_device *pdev;
   struct channel_obj *ch;
 + struct resource *res;
 + int irq_num, i = 0;
 +
 + pdev = container_of(vpif_dev, struct platform_device, dev);

As Sergei mentioned, the platform device is already passed to the function as 
an argument.

 + while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
 + for (irq_num = res-start; irq_num = res-end; irq_num++)
 + free_irq(irq_num,
 +  (void *)(vpif_obj.dev[i]-channel_id));

A quick look at board code shows that each IRQ resource contains a single IRQ. 
The second loop could thus be removed. You could also add another patch to 
perform similar cleanup for the probe code.

 + i++;
 + }
 
   v4l2_device_unregister(vpif_obj.v4l2_dev);
 
 + kfree(vpif_obj.sd);
   /* un-register device */
   for (i = 0; i  VPIF_CAPTURE_MAX_DEVICES; i++) {
   /* Get the pointer to the channel object */
   ch = vpif_obj.dev[i];
   /* Unregister video device */
   video_unregister_device(ch-video_dev);
 + kfree(vpif_obj.dev[i]);
   }
   return 0;
  }
 @@ -2347,24 +2359,7 @@ static __init int vpif_init(void)
   */
  static void vpif_cleanup(void)
  {
 - struct platform_device *pdev;
 - struct resource *res;
 - int irq_num;
 - int i = 0;
 -
 - pdev = container_of(vpif_dev, struct platform_device, dev);
 - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
 - for (irq_num = res-start; irq_num = res-end; irq_num++)
 - free_irq(irq_num,
 -  (void *)(vpif_obj.dev[i]-channel_id));
 - i++;
 - }
 -
   platform_driver_unregister(vpif_driver);
 -
 - kfree(vpif_obj.sd);
 - for (i = 0; i  VPIF_CAPTURE_MAX_DEVICES; i++)
 - kfree(vpif_obj.dev[i]);
  }
 
  /* Function for module initialization and cleanup */
-- 
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

Re: [PATCH] media: i2c: mt9p031: add OF support

[Laurent Pinchart]

Hi Prabhakar,

Thanks for the patch.

On Sunday 26 May 2013 18:38:54 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 add OF support for the mt9p031 sensor driver.
 Alongside this patch sorts the header inclusion alphabetically.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@redhat.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Sascha Hauer s.ha...@pengutronix.de
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Rob Landley r...@landley.net
 Cc: Arnd Bergmann a...@arndb.de
 Cc: devicetree-disc...@lists.ozlabs.org
 Cc: davinci-linux-open-sou...@linux.davincidsp.com
 Cc: linux-...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org

Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

And added to my tree with three small changes (please see below).

 ---
  Changes for NON RFC v1:
  1: added missing call for of_node_put().
 
  Changes for RFC v4 (https://patchwork.kernel.org/patch/2556251/):
  1: Renamed gpio-reset property to reset-gpios.
  2: Dropped assigning the driver data from the of node.
 
  Changes for RFC v3(https://patchwork.kernel.org/patch/2515921/):
  1: Dropped check if gpio-reset is valid.
  2: Fixed some code nits.
  3: Included a reference to the V4L2 DT bindings documentation.
 
  Changes for RFC v2 (https://patchwork.kernel.org/patch/2510201/):
  1: Used '-' instead of '_' for device properties.
  2: Specified gpio reset pin as phandle in device node.
  3: Handle platform data properly even if kernel is compiled with
 devicetree support.
  4: Used dev_* for messages in drivers instead of pr_*.
  5: Changed compatible property to aptina,mt9p031 and aptina,mt9p031m.
  6: Sorted the header inclusion alphabetically and fixed some minor code
 nits.
 
  RFC v1: https://patchwork.kernel.org/patch/2498791/
 
  .../devicetree/bindings/media/i2c/mt9p031.txt  |   40
 ++ drivers/media/i2c/mt9p031.c|  
 43 +++- 2 files changed, 81 insertions(+), 2 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9p031.txt
 
 diff --git a/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
 b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt new file mode
 100644
 index 000..59d613c
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/media/i2c/mt9p031.txt
 @@ -0,0 +1,40 @@
 +* Aptina 1/2.5-Inch 5Mp CMOS Digital Image Sensor
 +
 +The Aptina MT9P031 is a 1/2.5-inch CMOS active pixel digital image sensor
 with +an active array size of 2592H x 1944V. It is programmable through a
 simple +two-wire serial interface.
 +
 +Required Properties :
 +- compatible : value should be either one among the following
 + (a) aptina,mt9p031 for mt9p031 sensor
 + (b) aptina,mt9p031m for mt9p031m sensor
 +
 +- input-clock-frequency : Input clock frequency.
 +
 +- pixel-clock-frequency : Pixel clock frequency.
 +
 +Optional Properties :
 +- reset-gpios: Chip reset GPIO

There's usually no space before a colon in English.

 +For further reading of port node refer
 Documentation/devicetree/bindings/media/video-interfaces.txt.
 +
 +Example:
 +
 + i2c0@1c22000 {
 + ...
 + ...
 + mt9p031@5d {
 + compatible = aptina,mt9p031;
 + reg = 0x5d;
 + reset-gpios = gpio3 30 0;
 +
 + port {
 + mt9p031_1: endpoint {
 + input-clock-frequency = 600;
 + pixel-clock-frequency = 9600;
 + };
 + };
 + };
 + ...
 + };
 diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
 index bf49899..bb1f993 100644
 --- a/drivers/media/i2c/mt9p031.c
 +++ b/drivers/media/i2c/mt9p031.c
 @@ -16,9 +16,10 @@
  #include linux/delay.h
  #include linux/device.h
  #include linux/gpio.h
 -#include linux/module.h
  #include linux/i2c.h
  #include linux/log2.h
 +#include linux/module.h
 +#include linux/of_gpio.h
  #include linux/pm.h
  #include linux/regulator/consumer.h
  #include linux/slab.h
 @@ -28,6 +29,7 @@
  #include media/v4l2-chip-ident.h
  #include media/v4l2-ctrls.h
  #include media/v4l2-device.h
 +#include media/v4l2-of.h
  #include media/v4l2-subdev.h
 
  #include aptina-pll.h
 @@ -928,10 +930,37 @@ static const struct v4l2_subdev_internal_ops
 mt9p031_subdev_internal_ops = { * Driver initialization and probing
   */
 
 +static struct mt9p031_platform_data *
 +mt9p031_get_pdata(struct i2c_client *client)
 +{
 + struct mt9p031_platform_data *pdata = NULL;

No need to initialize pdata to NULL here.

 + struct device_node *np;
 +
 + 

Re: [PATCH v3 5/9] media: davinci: vpif_capture: use module_platform_driver()

[Laurent Pinchart]

On Sunday 26 May 2013 17:30:08 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 This patch uses module_platform_driver() to simplify the code.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com

Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

 ---
  drivers/media/platform/davinci/vpif_capture.c |   28 +-
  1 files changed, 1 insertions(+), 27 deletions(-)
 
 diff --git a/drivers/media/platform/davinci/vpif_capture.c
 b/drivers/media/platform/davinci/vpif_capture.c index f8b7304..38c1fba
 100644
 --- a/drivers/media/platform/davinci/vpif_capture.c
 +++ b/drivers/media/platform/davinci/vpif_capture.c
 @@ -2338,30 +2338,4 @@ static __refdata struct platform_driver vpif_driver =
 { .remove = vpif_remove,
  };
 
 -/**
 - * vpif_init: initialize the vpif driver
 - *
 - * This function registers device and driver to the kernel, requests irq
 - * handler and allocates memory
 - * for channel objects
 - */
 -static __init int vpif_init(void)
 -{
 - return platform_driver_register(vpif_driver);
 -}
 -
 -/**
 - * vpif_cleanup : This function clean up the vpif capture resources
 - *
 - * This will un-registers device and driver to the kernel, frees
 - * requested irq handler and de-allocates memory allocated for channel
 - * objects.
 - */
 -static void vpif_cleanup(void)
 -{
 - platform_driver_unregister(vpif_driver);
 -}
 -
 -/* Function for module initialization and cleanup */
 -module_init(vpif_init);
 -module_exit(vpif_cleanup);
 +module_platform_driver(vpif_driver);
-- 
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

Re: [PATCH v3 8/9] media: davinci: vpif_display: use module_platform_driver()

[Laurent Pinchart]

On Sunday 26 May 2013 17:30:11 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 This patch uses module_platform_driver() to simplify the code.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com

Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

 ---
  drivers/media/platform/davinci/vpif_display.c |   18 +-
  1 files changed, 1 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/media/platform/davinci/vpif_display.c
 b/drivers/media/platform/davinci/vpif_display.c index 9c308e7..7bcfe7d
 100644
 --- a/drivers/media/platform/davinci/vpif_display.c
 +++ b/drivers/media/platform/davinci/vpif_display.c
 @@ -2005,20 +2005,4 @@ static __refdata struct platform_driver vpif_driver =
 { .remove = vpif_remove,
  };
 
 -static __init int vpif_init(void)
 -{
 - return platform_driver_register(vpif_driver);
 -}
 -
 -/*
 - * vpif_cleanup: This function un-registers device and driver to the
 kernel, - * frees requested irq handler and de-allocates memory allocated
 for channel - * objects.
 - */
 -static void vpif_cleanup(void)
 -{
 - platform_driver_unregister(vpif_driver);
 -}
 -
 -module_init(vpif_init);
 -module_exit(vpif_cleanup);
 +module_platform_driver(vpif_driver);
-- 
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

Re: [PATCH v3 6/9] media: davinci: vpif_capture: Convert to devm_* api

[Laurent Pinchart]

On Sunday 26 May 2013 17:30:09 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 use devm_request_irq() instead of request_irq(). This ensures
 more consistent error values and simplifies error paths.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com

Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

 ---
  drivers/media/platform/davinci/vpif_capture.c |   38 --
  1 files changed, 12 insertions(+), 26 deletions(-)
 
 diff --git a/drivers/media/platform/davinci/vpif_capture.c
 b/drivers/media/platform/davinci/vpif_capture.c index 38c1fba..5e1e5f6
 100644
 --- a/drivers/media/platform/davinci/vpif_capture.c
 +++ b/drivers/media/platform/davinci/vpif_capture.c
 @@ -2082,14 +2082,14 @@ static __init int vpif_probe(struct platform_device
 *pdev)
 
   while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
   for (i = res-start; i = res-end; i++) {
 - if (request_irq(i, vpif_channel_isr, IRQF_SHARED,
 - VPIF_Capture, (void *)
 - (vpif_obj.dev[res_idx]-channel_id))) {
 - err = -EBUSY;
 - for (j = 0; j  i; j++)
 - free_irq(j, (void *)
 - (vpif_obj.dev[res_idx]-channel_id));
 - goto vpif_int_err;
 + err = devm_request_irq(pdev-dev, i, vpif_channel_isr,
 +  IRQF_SHARED, VPIF_Capture,
 +  (void *)(vpif_obj.dev[res_idx]-
 +  channel_id));
 + if (err) {
 + err = -EINVAL;
 + goto vpif_unregister;
 +
   }
   }
   res_idx++;
 @@ -2106,7 +2106,7 @@ static __init int vpif_probe(struct platform_device
 *pdev) video_device_release(ch-video_dev);
   }
   err = -ENOMEM;
 - goto vpif_int_err;
 + goto vpif_unregister;
   }
 
   /* Initialize field of video device */
 @@ -2207,13 +2207,9 @@ vpif_sd_error:
   /* Note: does nothing if ch-video_dev == NULL */
   video_device_release(ch-video_dev);
   }
 -vpif_int_err:
 +vpif_unregister:
   v4l2_device_unregister(vpif_obj.v4l2_dev);
 - for (i = 0; i  res_idx; i++) {
 - res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 - for (j = res-start; j = res-end; j++)
 - free_irq(j, (void *)(vpif_obj.dev[i]-channel_id));
 - }
 +
   return err;
  }
 
 @@ -2225,18 +2221,8 @@ vpif_int_err:
   */
  static int vpif_remove(struct platform_device *device)
  {
 - struct platform_device *pdev;
   struct channel_obj *ch;
 - struct resource *res;
 - int irq_num, i = 0;
 -
 - pdev = container_of(vpif_dev, struct platform_device, dev);
 - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
 - for (irq_num = res-start; irq_num = res-end; irq_num++)
 - free_irq(irq_num,
 -  (void *)(vpif_obj.dev[i]-channel_id));
 - i++;
 - }
 + int i;
 
   v4l2_device_unregister(vpif_obj.v4l2_dev);
-- 
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

Re: [PATCH v3 9/9] media: davinci: vpif_display: Convert to devm_* api

[Laurent Pinchart]

On Sunday 26 May 2013 17:30:12 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com
 
 use devm_request_irq() instead of request_irq(). This ensures
 more consistent error values and simplifies error paths.
 
 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com

Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

with a small comment below.

 ---
  drivers/media/platform/davinci/vpif_display.c |   35 ++
  1 files changed, 9 insertions(+), 26 deletions(-)
 
 diff --git a/drivers/media/platform/davinci/vpif_display.c
 b/drivers/media/platform/davinci/vpif_display.c index 7bcfe7d..e2f080b
 100644
 --- a/drivers/media/platform/davinci/vpif_display.c
 +++ b/drivers/media/platform/davinci/vpif_display.c
 @@ -1718,15 +1718,14 @@ static __init int vpif_probe(struct platform_device
 *pdev)
 
   while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
   for (i = res-start; i = res-end; i++) {
 - if (request_irq(i, vpif_channel_isr, IRQF_SHARED,
 - VPIF_Display, (void *)
 - (vpif_obj.dev[res_idx]-channel_id))) {
 - err = -EBUSY;
 - for (j = 0; j  i; j++)
 - free_irq(j, (void *)
 - (vpif_obj.dev[res_idx]-channel_id));
 + err = devm_request_irq(pdev-dev, i, vpif_channel_isr,
 +  IRQF_SHARED, VPIF_Display,
 +  (void *)(vpif_obj.dev[res_idx]-
 +  channel_id));
 + if (err) {
 + err = -EINVAL;
   vpif_err(VPIF IRQ request failed\n);
 - goto vpif_int_err;
 + goto vpif_unregister;
   }
   }
   res_idx++;
 @@ -1744,7 +1743,7 @@ static __init int vpif_probe(struct platform_device
 *pdev) video_device_release(ch-video_dev);
   }
   err = -ENOMEM;
 - goto vpif_int_err;
 + goto vpif_unregister;
   }
 
   /* Initialize field of video device */
 @@ -1878,13 +1877,8 @@ vpif_sd_error:
   /* Note: does nothing if ch-video_dev == NULL */
   video_device_release(ch-video_dev);
   }
 -vpif_int_err:
 +vpif_unregister:
   v4l2_device_unregister(vpif_obj.v4l2_dev);
 - for (i = 0; i  res_idx; i++) {
 - res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 - for (j = res-start; j = res-end; j++)
 - free_irq(j, (void *)(vpif_obj.dev[i]-channel_id));
 - }
 
   return err;
  }
 @@ -1894,20 +1888,9 @@ vpif_int_err:
   */
  static int vpif_remove(struct platform_device *device)
  {
 - struct platform_device *pdev;
   struct channel_obj *ch;
 - struct resource *res;
 - int irq_num;
   int i = 0;

There's no need to initialize i to 0 anymore (same comment for patch 6/9).

 - pdev = container_of(vpif_dev, struct platform_device, dev);
 - while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
 - for (irq_num = res-start; irq_num = res-end; irq_num++)
 - free_irq(irq_num,
 -  (void *)(vpif_obj.dev[i]-channel_id));
 - i++;
 - }
 -
   v4l2_device_unregister(vpif_obj.v4l2_dev);
 
   kfree(vpif_obj.sd);
-- 
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

Re: [PATCH] media: i2c: mt9p031: add OF support

[Prabhakar Lad]

Hi Laurent,

On Wed, May 29, 2013 at 9:01 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 Thanks for the patch.

 On Sunday 26 May 2013 18:38:54 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 add OF support for the mt9p031 sensor driver.
 Alongside this patch sorts the header inclusion alphabetically.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@redhat.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Sascha Hauer s.ha...@pengutronix.de
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Rob Landley r...@landley.net
 Cc: Arnd Bergmann a...@arndb.de
 Cc: devicetree-disc...@lists.ozlabs.org
 Cc: davinci-linux-open-sou...@linux.davincidsp.com
 Cc: linux-...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org

 Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

 And added to my tree with three small changes (please see below).

Thanks for applying it, with the changes :)

Regards,
--Prabhakar lad
--
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/9] media: davinci: vpif_capture: move the freeing of irq and global variables to remove()

[Prabhakar Lad]

Hi Laurent,

Thanks for the review.

On Wed, May 29, 2013 at 8:02 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 Thanks for the patch.

 On Sunday 26 May 2013 17:30:07 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 Ideally the freeing of irq's and the global variables needs to be
 done in the remove() rather than module_exit(), this patch moves
 the freeing up of irq's and freeing the memory allocated to channel
 objects to remove() callback of struct platform_driver.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 ---
  drivers/media/platform/davinci/vpif_capture.c |   31 ++
  1 files changed, 13 insertions(+), 18 deletions(-)

 diff --git a/drivers/media/platform/davinci/vpif_capture.c
 b/drivers/media/platform/davinci/vpif_capture.c index caaf4fe..f8b7304
 100644
 --- a/drivers/media/platform/davinci/vpif_capture.c
 +++ b/drivers/media/platform/davinci/vpif_capture.c
 @@ -2225,17 +2225,29 @@ vpif_int_err:
   */
  static int vpif_remove(struct platform_device *device)
  {
 - int i;
 + struct platform_device *pdev;
   struct channel_obj *ch;
 + struct resource *res;
 + int irq_num, i = 0;
 +
 + pdev = container_of(vpif_dev, struct platform_device, dev);

 As Sergei mentioned, the platform device is already passed to the function as
 an argument.

OK

 + while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, i))) {
 + for (irq_num = res-start; irq_num = res-end; irq_num++)
 + free_irq(irq_num,
 +  (void *)(vpif_obj.dev[i]-channel_id));

 A quick look at board code shows that each IRQ resource contains a single IRQ.
 The second loop could thus be removed. You could also add another patch to
 perform similar cleanup for the probe code.

Any way this will code will be removed in the next patch of the same
series due to usage
of devm_* api. I'll do this change only in the probe code.

Regards,
--Prabhakar Lad
--
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 9/9] media: davinci: vpif_display: Convert to devm_* api

[Prabhakar Lad]

Hi Laurent,

On Wed, May 29, 2013 at 9:08 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 On Sunday 26 May 2013 17:30:12 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 use devm_request_irq() instead of request_irq(). This ensures
 more consistent error values and simplifies error paths.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com

 Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

Thanks for the ack.

 with a small comment below.

 ---
  drivers/media/platform/davinci/vpif_display.c |   35 ++
  1 files changed, 9 insertions(+), 26 deletions(-)

 diff --git a/drivers/media/platform/davinci/vpif_display.c
 b/drivers/media/platform/davinci/vpif_display.c index 7bcfe7d..e2f080b
 100644
 --- a/drivers/media/platform/davinci/vpif_display.c
 +++ b/drivers/media/platform/davinci/vpif_display.c
 @@ -1718,15 +1718,14 @@ static __init int vpif_probe(struct platform_device
 *pdev)

   while ((res = platform_get_resource(pdev, IORESOURCE_IRQ, res_idx))) {
   for (i = res-start; i = res-end; i++) {
 - if (request_irq(i, vpif_channel_isr, IRQF_SHARED,
 - VPIF_Display, (void *)
 - (vpif_obj.dev[res_idx]-channel_id))) 
 {
 - err = -EBUSY;
 - for (j = 0; j  i; j++)
 - free_irq(j, (void *)
 - (vpif_obj.dev[res_idx]-channel_id));
 + err = devm_request_irq(pdev-dev, i, vpif_channel_isr,
 +  IRQF_SHARED, VPIF_Display,
 +  (void *)(vpif_obj.dev[res_idx]-
 +  channel_id));
 + if (err) {
 + err = -EINVAL;
   vpif_err(VPIF IRQ request failed\n);
 - goto vpif_int_err;
 + goto vpif_unregister;
   }
   }
   res_idx++;
 @@ -1744,7 +1743,7 @@ static __init int vpif_probe(struct platform_device
 *pdev) video_device_release(ch-video_dev);
   }
   err = -ENOMEM;
 - goto vpif_int_err;
 + goto vpif_unregister;
   }

   /* Initialize field of video device */
 @@ -1878,13 +1877,8 @@ vpif_sd_error:
   /* Note: does nothing if ch-video_dev == NULL */
   video_device_release(ch-video_dev);
   }
 -vpif_int_err:
 +vpif_unregister:
   v4l2_device_unregister(vpif_obj.v4l2_dev);
 - for (i = 0; i  res_idx; i++) {
 - res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 - for (j = res-start; j = res-end; j++)
 - free_irq(j, (void *)(vpif_obj.dev[i]-channel_id));
 - }

   return err;
  }
 @@ -1894,20 +1888,9 @@ vpif_int_err:
   */
  static int vpif_remove(struct platform_device *device)
  {
 - struct platform_device *pdev;
   struct channel_obj *ch;
 - struct resource *res;
 - int irq_num;
   int i = 0;

 There's no need to initialize i to 0 anymore (same comment for patch 6/9).

Ok will fix it in the next version.

Regards,
--Prabhakar Lad
--
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 v5] media: i2c: tvp514x: add OF support

[Prabhakar Lad]

Hi Laurent,

On Wed, May 29, 2013 at 6:52 AM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Prabhakar,

 Thanks for the patch.

 On Sunday 26 May 2013 18:49:46 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar.cse...@gmail.com

 add OF support for the tvp514x driver.

 Signed-off-by: Lad, Prabhakar prabhakar.cse...@gmail.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@redhat.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Grant Likely grant.lik...@secretlab.ca
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Rob Landley r...@landley.net
 Cc: devicetree-disc...@lists.ozlabs.org
 Cc: linux-...@vger.kernel.org
 Cc: linux-ker...@vger.kernel.org
 Cc: davinci-linux-open-sou...@linux.davincidsp.com

 Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com

Thanks for the ack.

 (with two small comment below).

 ---
 Tested on da850-evm.

[snip]

 s/of port/on port/
 s/refer/refer to/

OK

 Documentation/devicetree/bindings/media/video-interfaces.txt.
 +
 +Example:
 +
 + i2c0@1c22000 {
 + ...
 + ...
 + tvp514x@5c {
 + compatible = ti,tvp5146;
 + reg = 0x5c;
 +
 + port {
 + tvp514x_1: endpoint {
 + hsync-active = 1;
 + vsync-active = 1;
 + pclk-sample = 0;
 + };
 + };
 + };
 + ...
 + };
 diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
 index 7438e01..7ed999b 100644
 --- a/drivers/media/i2c/tvp514x.c
 +++ b/drivers/media/i2c/tvp514x.c
 @@ -39,6 +39,7 @@
  #include media/v4l2-device.h
  #include media/v4l2-common.h
  #include media/v4l2-mediabus.h
 +#include media/v4l2-of.h
  #include media/v4l2-chip-ident.h
  #include media/v4l2-ctrls.h
  #include media/tvp514x.h
 @@ -1055,6 +1056,42 @@ static struct tvp514x_decoder tvp514x_dev = {

  };

 +static struct tvp514x_platform_data *
 +tvp514x_get_pdata(struct i2c_client *client)
 +{
 + struct tvp514x_platform_data *pdata = NULL;

 No need to initialize pdata to NULL.

OK will fix it in the next version.

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

[PATCH 1/1] [media] s5p-mfc: Add NULL check for allocated buffer

[Sachin Kamat]

In certain cases, dma_alloc_coherent returns NULL. Add check for
NULL pointer.

Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
---
 drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c 
b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
index 2e5f30b..dc1fc94 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c
@@ -38,7 +38,7 @@ int s5p_mfc_alloc_firmware(struct s5p_mfc_dev *dev)
dev-fw_virt_addr = dma_alloc_coherent(dev-mem_dev_l, dev-fw_size,
dev-bank1, GFP_KERNEL);
 
-   if (IS_ERR(dev-fw_virt_addr)) {
+   if (IS_ERR_OR_NULL(dev-fw_virt_addr)) {
dev-fw_virt_addr = NULL;
mfc_err(Allocating bitprocessor buffer failed\n);
return -ENOMEM;
-- 
1.7.9.5

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