[09:55] <mchehab> hverkuil: just sent the pull request for the CEC patches I had on my fixes tree before taking vacations
[09:56] <mchehab> please check if something is missing there and send me a pull request if someting else is needed
[09:57] <mchehab> PS.: I'm having some troubles with my corporate's VPN access... so I may be missing e-mails sent to my empoyeer's email... so it is best to c/c me at my @infradead.org e-mail ultil this gets sorted
[09:58] <mchehab> I'm avoiding handling patches until it gets sorted, to avoid missing some comments/nacks on submitted patches
[10:45] <rshanmu> hverkuil: Any chance of conclusion on this thread please - "[PATCH v2 6/7] dt-bindings: media: Add Renesas R-Car DRIF binding"
[10:55] <hverkuil> mchehab: I'm missing the patches from this pull request in your 4.10 pull request: https://patchwork.linuxtv.org/patch/38980/
[10:55] <hverkuil> Those really should go to 4.10.
[10:58] <mchehab> I'll handle it next week
[10:58] <mchehab> let's hope that my email will be restore by then
[10:58] <mchehab> s/restore/restored/
[10:58] <hverkuil> Let me know if you need any help.
[10:59] <mchehab> ok
[10:59] <mchehab> btw, you probably missed my question, but on Monday I asked if that pull request was the same that was merged already on fixes
[10:59] <mchehab> apparently, it is not the same ;)
[11:00] <mchehab> (I didn't double-checked myself, as I got flooded by pending stuff and by meetings due to my return from vacations - sorry for that)
[11:11] <hverkuil> No, it's not the same. You were already on vacation when I posted it.
[11:11] <mchehab> ah!
[13:24] *** hverkuil has left 
[13:50] <sailus> mchehab: At the same time with hverkuil's fixes for v4.10, could you check this one: https://patchwork.linuxtv.org/patch/38912/ ?
[13:51] <sailus> It'd be nice to have that in v4.10, as well as for the stable kernels.
[13:53] <mchehab> sailus: I applied it internally on my tree already, but, as it is broken since v4.5, it is hardly something for 4.10
[13:54] <mchehab> the idea is that patches for the latest release should be addressing regressions on current version only... specially when we're late at -rc cycle, Linus doesn't like patches fixing stuff for earlier releases
[13:56] <mchehab> (in the specific case of CEC patches, as CEC is promoted from staging for v4.10, there's a reason why it will be submitted on a late -rc cycle)
[13:56] <sailus> mchehab: Fair enough.
[13:57] <sailus> The pull request was based on media-fixes. The patch that you applied --- did that contain cc: stable as well? (Just making sure, because it wasn't in the patch I posted to the list, just in the pull request.)
[14:01] <mchehab> commit 98d85f3cb912fde14593ead54dea4c1a00b3966f
[14:01] <mchehab> Author:     Sakari Ailus <sakari.ailus@linux.intel.com>
[14:01] <mchehab> AuthorDate: Mon Jan 2 08:32:47 2017 -0200
[14:01] <mchehab> Commit:     Mauro Carvalho Chehab <mchehab@s-opensource.com>
[14:01] <mchehab> CommitDate: Fri Jan 27 11:20:33 2017 -0200
[14:01] <mchehab> ...
[14:01] <mchehab>     Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
[14:01] <mchehab>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[14:01] <mchehab>     Cc: stable@vger.kernel.org # For v4.5 and up
[14:01] <mchehab>     Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
[14:01] <mchehab> yep, c/c stable for v4.5+ is there
[14:01] <mchehab> sailus: ^
[14:02] <sailus> mchehab: Ack. Thank you! :-)
[14:03] *** awalls1 has left 
[14:04] <mchehab> anytime
[15:34] <ndufresne> thiblahute: javier__: hello
[15:34] <ndufresne> looking at https://www.irccloud.com/pastebin/GZFho8Bd/0001-media-exynos-gsc-Handle-NV21M-pixel-format.patch
[15:34] <ndufresne> I'm surprise no changes was needed to gsc_prepare_addr()
[15:35] <ndufresne> ah, I think I see, regardless, they read cb, cr in that order and swap later, not needed for NV12M since we call vb2_dma_contig_plane_dma_addr() twice
[15:36] <ndufresne> thiblahute: javier__: not sure this is right, pix_size = frame->f_width * frame->f_height;
[15:37] <ndufresne> works if f_width is padded width
[15:37] <ndufresne> (actually f_height needs to be padded to)
[15:40] <ndufresne> hmm, it is padded, still don't know what's broken there
[15:41] <ndufresne> thiblahute: does it work before you patch if you do: videotestsrc ! video/x-raw,format=NV21 ! vl42videoNconvert ! video/x-raw,format=BGRx ?
[15:42] <thiblahute> ndufresne: Checking.
[15:46] *** benjiG has left 
[15:47] <thiblahute> ndufresne: Same bug as with the decoder
[15:48] <thiblahute> It should also fail with my patch, let me check.
[15:49] <ndufresne> not sure, it will possibly work with your patch in fact
[15:50] <ndufresne> thiblahute: I would also command out NV21M, I'm suspecting that this might also be broken
[15:50] <thiblahute> It does, not sure why though :)
[15:50] <ndufresne> (after all, it's the same code path), and then NV61 and NV16 probably suffer the same bug
[15:51] <ndufresne> I think there is a bug in the driver when dealing with n_planes=1 and n_comps=2
[15:51] <ndufresne> the driver works with memory pointer, so there is no obvious reason this should be a problem
[15:52] <thiblahute> ndufresne: It work for NV16 (same as NV12) but failes with NV61
[15:53] <ndufresne> ok, so it's specific to when then cb/cr get swapped
[15:53] <thiblahute> Right.
[15:53] <ndufresne> we should test VYUY, YVYU, YVU420 and YVU420M too
[15:54] <ndufresne> hmm, why the hell did YVU420M ended up being swapped, make no sense to me
[15:55] <ndufresne> all this if() blok, that leads to swap should be moved inside the 	if (frame->fmt->num_planes == 1)  case
[15:58] <thiblahute> ndufresne: Well, V4L2_PIX_FMT_YVU420M is multiplanar
[15:58] <ndufresne> yes, so calling swap there is wrong, as you get each plane seperatly from the framework, hence they are already ordered properly
[15:59] <thiblahute> I guess indeed.
[16:01] <ndufresne> thiblahute: haha, found the bug indeed
[16:03] <ndufresne> thiblahute: remove all semi-planer form that If, NV61 and NV21, will work way better, right now you are passing a chroma plane as being NULL pointer ;-P
[16:03] <ndufresne> in NV12/21 and NV16/61, the driver will only look at cr pointer, so if you swap, you ended setting NULL to CR and something in CB, which cause troubles
[16:04] <ndufresne> javier__: ^
[16:04] <ndufresne> and you can remove the 420M from there two in a seperate patch
[16:05] <ndufresne> well, the other way around (CB, CR) but you see what I mean
[16:06] <ndufresne> thiblahute: javier__ : the spec is clear, GSCALER_IN_BASE_ADDR_CRX is stricly used for 3 planes +, and because of that swap, we set that register instead of CB one, and it gets ignored
[16:07] <thiblahute> ndufresne: I did not get why CR will be NULL in those cases
[16:08] <ndufresne> thiblahute: NV21 have 2 plane, Y pane, and CRCB plane, what we do, is set addr->y, then addr->cb (using an calculated offset), so addr->cr remind 0, then later we swap, so cb is 0 and cr has a calue
[16:09] <ndufresne> in gsc_hw_set_input_addr(), we'll set the registers for Y, CB, CR pointers, but CR is ignored when there is only 2 planes
[16:09] <ndufresne> and CB is 0
[16:09] <thiblahute> Oo, right you are right indeed.
[16:09] <thiblahute> And.. it fixes the NV61 case
[16:10] <ndufresne> and you get more kernel patches !
[16:10] <thiblahute> ndufresne: You want to propose it?
[16:10] <ndufresne> go ahead, will be easyer for you to take care and upstream atm
[16:11] <thiblahute> ndufresne: OK, will mention you as  'Suggested-by: '
[16:11] <ndufresne> good good
[16:12] <ndufresne> note that adding NV21M is good to allow dmabuf importation from the decoder, and those patch will avoid leaving hidden bugs behind
[16:12] <thiblahute> ndufresne: Yeah, I am also adding NV61M
[16:12] <ndufresne> but as soon as you add NV21M, NV21 will no longer be reachable through GStreamer
[16:36] <thiblahute> ndufresne: Actually for V4L2_PIX_FMT_YVU420M the swapping is necessary I think as we can see that cb is set to the second plane in memory and cr to the third but it is in fact the contrary
[16:37] <ndufresne> thiblahute: ah, your are right on this one
[16:41] <javier__> ndufresne, thiblahute: sorry, I wasn't online before. This has been a tough day for personal reasons...
[16:41] <ndufresne> oh, sorry to ear that, kids ?
[16:42] * ndufresne have no voice, but having bunch of calls today, fantastic day
[16:42] <javier__> no, a relative passed away
[16:42] <javier__> anyways, the exynos-gsc stride/bytesperline/sizeimage calculation is pretty broken indeed
[16:42] <ndufresne> oh, sad day, apologies
[16:42] <javier__> ndufresne: thanks
[16:43] <javier__> ndufresne: I tried to fix for most of the formats in commit 652bb68018a5 ("[media] exynos-gsc: do proper bytesperline and sizeimage calculation")
[16:43] <ndufresne> javier__: I thought you already fixed some of it in the last round no ?
[16:43] <ndufresne> ah, so still in the queue, but I guess thiblahute has that in the branch he's looking at
[16:44] <javier__> ndufresne: yes, although as mentioned in the commit message some formats are still broken (NV21 being one of them)
[16:44] <thiblahute> ndufresne, javier__: That should be it. https://www.irccloud.com/pastebin/MkAYNpZT/0001-media-exynos-gsc-Do-not-swap-cb-cr-for-semi-planar-f.patch
[16:44] <javier__> ndufresne: thanks a lot for your insights. I'll work with thiblahute to fix the remaining ones
[16:44] <ndufresne> javier__: yes, that's exactly what thiblahute and I was looking at
[16:44] <ndufresne> javier__: we need to remove the swap for NV21 and NV61, cause it least to cb to be 0
[16:45] <javier__> ndufresne: I see, thanks for pointing that out
[16:46] <ndufresne> javier__: this is just for self conscience, since GStreamer won't reproduce that bug as soon as NV21M and NV61M are added
[16:47] <javier__> ndufresne: yes, because if possible it chooses planar formats as you mentioned yesterday, right?
[16:48] <thiblahute> Right
[16:48] <javier__> thiblahute: we have the same issues in s5p-mfc btw, that's why we had the "Video device did not suggest any buffer size"
[16:49] <javier__> since gst correctly sets buffersize to 0 for the capture case (the driver should fill with the correct size)
[16:49] <thiblahute> javier__: What same issue?
[16:49] <ndufresne> hmm, that worked before
[16:50] <javier__> thiblahute: well, not same but similar. The bytesperline/sizeimage not calculated correctly
[16:52] <javier__> ndufresne: it's not clear to me how it works, but S_FMT fails (-EBUSY) for s5p-mfc capture queue
[16:53] <javier__> so it seems gst does some assumptions that makes it work. But as soon as you fix s5p-mfc to avoid failing with -EBUSY, it doesn't work anymore
[16:54] * ndufresne isn't sure in which context that happens
[16:55] <javier__> ndufresne: in any pipeline using the s5p-mfc. gst does S_FMT output queue, STREAMON output, S_FMT capture and STREAMON capture
[16:55] <javier__> ndufresne: but s5p-mfc S_FMT callback checks if is_streaming(output) || is_streaming(capture) and returns -EBUSY
[16:56] <ndufresne> ok, that looks like a regression, because it worked before, didn't you submitted a patch for that already ?
[16:57] <javier__> ndufresne: no, I had this patch for a while in my tree https://hastebin.com/ozebakonig.php
[16:57] <ndufresne> GStreamer can't S_FMT the capture before it has passed the header to the decoder
[16:57] <ndufresne> cause it rely on the decoder to parse the header in order for the decoder to provide the capture size
[16:58] <javier__> ndufresne: yes, gst does the correct thing
[16:58] <ndufresne> the question is what patch went that wrong direction
[16:58] <ndufresne> cause it was not a problem before
[16:58] <ndufresne> someone must have merged something that was wrong
[16:59] <javier__> ndufresne: sorry, maybe I didn't explain myself clearly. gst + linux-next works, but S_FMT for capture fails
[16:59] <javier__> this isn't an issue for gst right now, it can recover and playback works
[17:00] <ndufresne> ah, ok, I see, it's probably no what I think
[17:00] <javier__> ndufresne: but after fixing the S_FMT callback, another driver in the bug is exposed (sizeimage being 0)
[17:00] <javier__> that's because s_fmt (or better try_fmt) should set the bytesperline and sizeimage but it doesn't
[17:01] <ndufresne> the CODA decoder (freescale) has multiple output formats, so this is probably the S_FMT that is used to try and change the format to what is negotiated downstream, failing on this call should not cause a problem
[17:04] <javier__> ndufresne: yes, and it doesn't. Succeeding on this call is what exposes the other driver bug
[17:05] <javier__> so we first need to fix that one and then post the patch I shared to split the streaming for output and capture in S_FMT
[17:05] <javier__> *the streaming check
[17:06] <javier__> ndufresne: but don't worry, we are already looking at it too with thiblahute
[17:07] <ndufresne> ok
[18:28] * headless is porting Renesaas IMR driver to upstream
[18:28] <headless> what has become with vb2_dma_contig_init_ctx() and friends?
[18:39] <headless> mchehab?
[18:40] <mchehab> headless: don't remember.... just returned from vacations :-)
[18:41] <headless> mchehab: :-)
[18:41] <mchehab> there were some parts of vb2 that were moved to mm
[18:41] <mchehab> related to dma contig stuff, if I remember well
[18:42] <mchehab> the patches were authored by Jan Kara
[18:43] <mchehab> I would start seeking for it with those patches:
[18:43] <mchehab> git log --author "Jan Kara" drivers/media/v4l2-core
[18:44] <headless> mchehab: TY!
[18:44] <headless> looks like I found the commit
[18:44] <mchehab> good!
[18:45] <headless> it's by hverkuil
[18:45] <headless> 53ddcc683faef8c730c7162fa1ef2261a385d16d
[18:45] <mchehab> yeah, hans did some cleanups there too
[19:25] <headless> mchehab: how's the vacations?
[19:25] * headless didn't have ones in 2016