| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| mchehab | hverkuil: I'm trying to merge back from 5.5-rc5, but there are several non-trivial conflicts with drivers/media/usb/pulse8-cec/pulse8-cec.c | [09:10] |
| hverkuil | Hm. Perhaps the pulse8-cec patches in our tree should be reverted first, then merge rc5, then re-apply on top?
I'm not sure what the best way forward is here. | [09:17] |
| ......... (idle for 43mn) | ||
| mchehab | I can try to do that. just a little bit afraid of breaking something | [10:01] |
| hverkuil | mchehab: store a copy of pulse8-cec.c, then revert/merge/reapply and check if the final version is identical to the stored version. It should be the same. | [10:03] |
| mchehab | the problem is with e5a52a1d15c79bb48a430fb263852263ec1d3f11
it fails if we try to apply on the top of our patchwork's master branch I guess I can try to manually apply it | [10:03] |
| hverkuil | it should be skipped since it is already in the master. Which is the core reason for these problems.
reverting all pulse8 patches first, then merge, then apply them again (minus that patch) should work (I think) | [10:05] |
| mchehab | Hmm.. the code has now:
WARN_ON(pulse8->tx_done_status); + WARN_ON(pulse8->work_result); I *suspect* that you applied another patch changing work_result to tx_done_status on some places | [10:09] |
| hverkuil | That's done in patch "media: pulse8-cec: set tx_done_status for transmit_done status" | [10:10] |
| mchehab | hmm...
commit c4e8f760581b8607a1989acb8925be25d6628760 Author: Hans Verkuil <hverkuil-cisco@xs4all.nl> Date: Sat Dec 7 23:43:23 2019 +0100 that's identical to the one applied upstream | [10:11] |
| hverkuil | yes, you accidentally merged that fix in master instead of in the fixes branch.
Then another pulse8-cec cleanup series was applied on top of that in master. | [10:12] |
| mchehab | ok. I'm fixing it by backing up the latest version and use it as-is after the merge
let me try building it, in order to check if verything builds fine | [10:13] |
| hverkuil: built fine. please check if everything looks ok to you after 5.5-rc5 merge?
just pushing it | [10:27] | |
| .......... (idle for 48mn) | ||
| hverkuil | mchehab: please merge https://patchwork.linuxtv.org/patch/61062/ asap. It fixes a nasty bug that was introduced by the y2038 series. | [11:15] |
| ............... (idle for 1h12mn) | ||
| mchehab | hverkuil: applying right now | [12:27] |
| hverkuil | I really should add some v4l2-compliance tests for 'bad' ioctls to better catch such issues... | [12:29] |
| mchehab | heh
hverkuil: at anotherseries, you forgot your SOB on a revert patch # ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s) # patches/0001-0016-Revert-media-v4l2-Fix-fourcc-names-for-BAYER12P.patch has style problems, please review. I'll mark the series as changes requiested | [12:39] |
| hverkuil | I can respin the PR.
can -> will | [12:42] |
| mchehab: jenkins didn't complain about that issue, strangely enough. I'll post a new PR with a proper SoB.
done. | [12:48] | |
| .... (idle for 15mn) | ||
| mchehab | thanks
weird that jenkins didn't notice it | [13:04] |
| ...... (idle for 26mn) | ||
| hverkuil | added tests for malformed ioctls to v4l2/cec-compliance. This would have caught the v4l2-ioctl regression. Oh well, better late than never. | [13:30] |
| .......... (idle for 49mn) | ||
| mchehab | hverkuil: that hevc first patch seems to be defining a crappy api
​V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX | [14:19] |
| hverkuil | my understanding was that you actually needed them all, but I never checked that against the spec, so you raised a good question. | [14:21] |
| mchehab | not saying that the API shouldn't support it... just that the implementation doesn't seem nice
and won't scale, if further versions of HEVC add other matrixes | [14:23] |
| hverkuil | I think the API is correct based on my reading of the H265 spec, but let's wait for confirmation from Jernej. | [14:32] |
| mchehab | the H265 spec says that all those matrixes should be updated and used at the same time? | [14:32] |
| hverkuil | If I read the spec correctly, then different macroblock sizes can be mixed in one frame, hence you need to provide the scaling matrices for all four possibilities. | [14:33] |
| mchehab | ok, but what happpens if userspace doesn't call this ioctl?
the driver should somehow default to something in other words, it has to be defaults for all different macroblock sizes what this ioctl seems to be implementing is a way to change from the default we can very likely change the default for just one type while keeping the other types using the default ones | [14:34] |
| hverkuil | The spec defines defaults AFAICT. Scaling matrices appear to be an optional feature.
I suspect that if you need to provide custom values for one size, you'll need to provide them for all. But let's wait for Jernej's feedback. He actually knows what he is talking about :-) | [14:37] |
| mchehab | also, if further versions of H265 adds more block types, by having everything packed altogether makes a lot harder to extend it to accept other macroblock types than if using an struct with { enum foo; union { u8 array[][]; u8 arr2[][]; ...;}
anyway, let's wait for his reply imo, at least the first 4 patches require some further discussions not sure if the other patches on that series are related or independent | [14:38] |
| hverkuil | independent | [14:40] |
| mchehab | btw, with regards to the ioctl...
one may want to just get the current matrixes and not change anything | [14:40] |
| hverkuil | patches 3/4 and 4/4 are also independent of 1/4 and 2/4. | [14:41] |
| mchehab | yes, but patch 3/4 seems to be breaking uAPI | [14:41] |
| hverkuil | staging driver. It's clearly documented in the spec that this staging API will change.
It's WHY it is in staging. | [14:42] |
| mchehab | i know... still, if we can avoid breaking, that would be better
I want to avoid an useless discussion if someone ever complains about a change like that caused an api breakage, if needed so, if this is not an issue, I would prefer to append stuff at the end of the struc on the other hand, if appending it to the end is an issue, I want to understand why, as maybe the entire API was wrongly designed so, I'd like to get a feedback about 3/4 from Jerej anyway, I'll drop the first 4 patches from the tree and review/apply the remaining ones if ok | [14:43] |
| hverkuil | It's been discussed: there are two possibilities for future extensions: either create new controls, or add support in v4l2-ctrls.c for variable size compound controls, allowing them to be extended in the future.
I'd like the second option, but haven't had time to actually implement this. The first option is of course always possible. | [14:48] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |