Hi John, On 10/18/2013 02:03 AM, John Sheu wrote: > On Thu, Oct 17, 2013 at 3:54 PM, Sylwester Nawrocki > <sylvester.nawrocki@xxxxxxxxx> wrote: >> On 10/18/2013 12:25 AM, John Sheu wrote: >>> On Thu, Oct 17, 2013 at 2:46 PM, John Sheu<sheu@xxxxxxxxxx> wrote: >>>>> Sweet. Thanks for spelling things out explicitly like this. The fact >>>>> that the CAPTURE and OUTPUT queues "invert" their sense of "crop-ness" >>>>> when used in a m2m device is definitely all sorts of confusing. >>> >>> Just to double-check: this means that we have another bug. >>> >>> In drivers/media/v4l2-core/v4l2-ioctl.c, in v4l_s_crop and v4l_g_crop, >>> we "simulate" a G_CROP or S_CROP, if the entry point is not defined >>> for that device, by doing the appropriate S_SELECTION or G_SELECTION. >>> Unfortunately then, for M2M this is incorrect then. >>> >>> Am I reading this right? >> >> You are right, John. Firstly a clear specification needs to be written, >> something along the lines of Tomasz's explanation in this thread, once >> all agree to that the ioctl code should be corrected if needed. I don't understand the problem here. The specification has always been clear: s_crop for output devices equals s_selection(V4L2_SEL_TGT_COMPOSE_ACTIVE). Drivers should only implement the selection API and the v4l2 core will do the correct translation of s_crop. Yes, I know it's weird, but that's the way the crop API was defined way back and that's what should be used. My advise: forget about s_crop and just implement s_selection. >> >> It seems this [1] RFC is an answer exactly to your question. >> >> Exact meaning of the selection ioctl is only part of the problem, also >> interaction with VIDIOC_S_FMT is not currently defined in the V4L2 spec. >> >> [1] http://www.spinics.net/lists/linux-media/msg56078.html > > I think the "inversion" behavior is confusing and we should remove it > if at all possible. > > I took a look through all the drivers in linux-media which implement > S_CROP. Most of them are either OUTPUT or CAPTURE/OVERLAY-only. Of > those that aren't: > > * drivers/media/pci/zoran/zoran_driver.c : this driver explicitly accepts both > OUTPUT and CAPTURE queues in S_CROP, but they both configure the same state. > No functional difference. Yeah, I guess that's a driver bug. This is a very old driver that originally used a custom API for these things, and since no selection API existed at the time it was just mapped to the crop API. Eventually it should use the selection API as well and do it correctly. But to be honest, nobody cares about this driver :-) It is however on my TODO list of drivers that need to be converted to the latest frameworks, so I might fix this eventually. > * drivers/media/platform/davinci/vpfe_capture.c : this driver doesn't specify > the queue, but is a CAPTURE-only device. Probably an (unrelated) bug. Yes, that's a driver bug. It should check the buffer type. > * drivers/media/platform/exynos4-is/fimc-m2m.c : this driver is a m2m driver > with both OUTPUT and CAPTURE queues. It has uninverted behavior: > S_CROP(CAPTURE) -> source > S_CROP(OUTPUT) -> destination This is the wrong behavior. > * drivers/media/platform/s5p-g2d/g2d.c : this driver is a m2m driver with both > OUTPUT and CAPTURE queues. It has inverted behavior: > S_CROP(CAPTURE) -> destination > S_CROP(OUTPUT) -> source This is the correct behavior. > > The last two points above are the most relevant. So we already have > at least one broken driver, regardless of whether we allow inversion > or not; I'd think this grants us a certain freedom to redefine the > specification to be more logical. Can we do this please? No. The fimc-m2m.c driver needs to be fixed. That's the broken one. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
