mchehab: what is the status of this pull request: https://patchwork.linuxtv.org/patch/34176/  It was posted May 4th, so that's a rather long time ago.
hverkuil: waiting for firmware
if I remember well, there is a request-firmware somewhere at the driver, but I found no evidence that Mediatek has sent the firmware to linux-firmware or to the ML for someone to add it there
Ah, OK. That's the holdup.
It wasn't clear to me.
well, I answered to the thread requesting there
s/there/that/
hverkuil: I'm checking internally with Aviv to make sure the description is buggy --- he wrote the patch. For some reason I thought Guennadi did.
I'll send a pull req then.
OK. The patch is probably correct, but you never know...
hverkuil: hmm... you didn't address my comments I pade on v16 4/13 related to the code that it is now at patch "cec-edid: add module for EDID CEC helper functions" ?
s/pade/made/
the more serious one is that the logic allows to go past edid[] array
Oops, I never saw those comments. I thought you never reviewed any further after seeing the coding style warnings.
My mistake. I'll look at this on Friday or Monday.
https://patchwork.linuxtv.org/patch/34134/
there are lots of comments there
ok. for now, I'll just reject this pull request then
Yes. I thought is was too easy :-)
:)
ah, please base on this branch:
To ssh://linuxtv.org/git/media_tree.git
 * [new branch]                topic/cec -> cec
it has a pull from Dmitry's stable branch
and two of your patches already applied
(plus the ones that Dmitry applied on his tree)
Thanks, I've prepared my branch for that.
 ah, one last thing: although I asked you to move the docbook patch to happen before the code changes...
as I already reviewed it, you can put it later, as otherwise it will break bisectability for documentation builds
pwclient update -s 'superseded' 34691 # patches/lmml_34691_patchv18_04_15_docbook_media_add_cec_documentation.patch
(and I ended by marking it as superseded already, although it is not applied)
BTW, include/linux/cec.h is there (for now) instead of in include/uapi/linux/cec.h because the CEC framework is in staging.
I don't want to put it in uapi, since that would make it a stable API, right?
If you prefer it in uapi (even though the API will likely change a little bit before cec goes out of staging), then I'm happy to move it.
yeh, I noticed that after reviewing the other patches. OK to keep it at include/linux.cec.h. Just a notice there that it should not be used yet
there were cases in the past where people used some of those headers that are out of uapi
lirc.h is one of such examples - but there are(or used to have?) other cases of private ioctls whose header files is only at include/!uapi
include/media/i2c/saa6588.h:#define SAA6588_CMD_READ	_IOR('R', 3, int)
include/media/i2c/saa6588.h:#define SAA6588_CMD_POLL	_IOR('R', 4, int)
we still have this one header with such stuff
(it used to have a lot more, but we fixed the other cases over the time)
funny... it is written there:
: /* These ioctls are internal to the kernel */
funny design... they indeed look like internal Kernel stuff
although they implement syscall handling code
for RDS open(), read() and poll()
Looking at your review comments I see nothing major. Which is good.
hackish
(I suspect that old API can likely be removed from saa6588 since we now have a proper RDS API)
agreed. this was kept for a long time now... unlikely that people are still relying on it
yeah, but still will require some care to address some of them ;)
nothing really big
hi, in the VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl, what should be the difference between TRY and ACTIVE in the which field? It's not really clear in the docs
I don't understand why we have a try in the enum frame size if this enum doesn't modify anything
or maybe I misunderstood it
"This ioctl allows applications to enumerate all frame sizes supported by a sub-device on the given pad for the given media bus format."
I guess the last part of the sentence is the answer
or maybe not
larsc, but what is the difference if I fill the which field with TRY or ACTIVE?
do you know the difference between TRY and ACTIVE in general?
larsc, in set format ioctl yes, the try will pretend it has set the format, and the active will set the format for real, no?
yes
and this try state is preserved across an open file descriptor
the so called try state
so if you make modifications using TRY for one configuration option other IOCTLs called on the same fd also using TRY will reflect that change
larsc, right, but the VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl doesn't change anything, does it?
but the supported framesizes might depend on a different configuration setting
larsc, aaaah, I see, thanks
that's how I understand it
I might be wrong
larsc, my problem now is that the .set_fmt callback in the struct v4l2_subdev_pad_ops doesn't receive a file in it's parameters as in .vidioc_try_fmt_vid_cap from struct v4l2_ioctl_ops (who receives a file), so I can't associated the format with the file descriptor. I'll check in other drivers how it's done
larsc: your understanding is correct
koike: enumerating frame sizes on a source pad of a subdev can report different sizes based on the configuration of the sink pad(s) of the same subdev
in the latest kernel pad::set_fmt doesn't have a file argument anymore
it has a struct v4l2_subdev_pad_config *cfg instead
when the operation is called from userspace through the VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl there will always be a file handle, so cfg will always be non-NULL
when the operation is called from another driver, such as a bridge driver to implement VIDIOC_TRY_FMT, that driver needs to allocate a struct v4l2_subdev_pad_config for the subdev
with v4l2_subdev_alloc_pad_config()
and use that configuration for the pad operations
pinchartl, right, thank you. Just to make it clearer: so the v4l2_subdev_pad_config *cfg is already associated with a file descriptor, I see that there is a struct v4l2_mbus_framefmt try_fmt inside cfg. But if I am calling enum frame size in the source with try, how can the driver check of the try format configured in the sink for that file descriptor?
v4l2_subdev_get_try_format, seems to be this, ok, found it, thank you :)
:-)
from a subdev point of view you shouldn't care whether you're called from the subdev userspace API or from a bridge driver
you'll get a cfg
and a which field
the cfg is guaranteed to be non-NULL if which == TRY
that guarantee is implemented in v4l2-subdev.c for the subdev userspace API
and in bridge drivers otherwise, using v4l2_subdev_alloc_pad_config()
pinchartl, thank you. I need to re-work the try mechanisms in vimc, it's all wrong :(
nobody gets it right the first time, except sometimes by chance :-)
this reminds me the matrix jump, "No one's ever made the first jump." hehe
can the .get_fmt from struct v4l2_subdev_pad_ops return an invalid format? I was just wondering if the user space call .get_fmt with which==TRY, the try format in struct v4l2_subdev_pad_config *cfg is not valid if the .set_fmt with TRY was not called before, I could use the .init_cfg from from struct v4l2_subdev_pad_ops but I notice that only few drivers use it
most of the drivers return v4l2_subdev_get_try_format directly in the .get_fmt if which==TRY