On Fri, 2019-01-18 at 14:45 +0100, Hans Verkuil wrote: > On 1/18/19 2:42 PM, Philipp Zabel wrote: > > On Fri, 2019-01-18 at 13:41 +0100, Hans Verkuil wrote: > > [...] > > > > I can do that, but what would be the purpose of it showing up in the > > > > media controller? > > > > There is nothing to be configured, no interaction with the rest of the > > > > graph, and the processing subdevice wouldn't even correspond to an > > > > actual hardware unit. I assumed this would clutter the media controller > > > > for no good reason. > > > > > > Just because you can't change routing doesn't mean you can't expose it in the > > > media controller topology. It makes sense to show in the topology that you > > > have this block. > > > > > > That said, I can't decide whether or not to add this. For a standalone m2m > > > device I would not require it, but in this case you already have a media > > > device. > > > > > > I guess it is easy enough to add later, so leave this for now. > > > > Ok. > > > > [...] > > > > > How did you test the rotate control? > > > > Just FTR, I used GStreamer for most of my testing, something like > > (simplified): > > > > gst-launch-1.0 videotestsrc ! v4l2video14convert extra-controls=cid,rotate=90 ! autovideosink > > > > > > > I'm asking because I would expect to see code > > > > > that checks this control in the *_fmt ioctls: > > > > In a way this does happen, since _try_fmt calls ipu_image_convert_adjust > > with a ctx->rot_mode parameter. It only has an influence on the > > alignment though. > > > > [...] > > > > The V4L2_CID_ROTATE documentation [1] states: > > > > > > > > Rotates the image by specified angle. Common angles are 90, 270 and > > > > 180. Rotating the image to 90 and 270 will reverse the height and > > > > width of the display window. It is necessary to set the new height > > > > and width of the picture using the VIDIOC_S_FMT ioctl according to > > > > the rotation angle selected. > > > > > > > > [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/control.html#control-ids > > > > > > > > I didn't understand what the "display window" is in the context of a > > > > mem2mem scaler/rotator/CSC converter. Is this intended to mean that > > > > aspect ratio should be kept as intact as possible, and that every time > > > > V4L2_CID_ROTATE changes between 0/180 and 90/270, an automatic S_FMT > > > > should be issued on the capture queue with width and height switched > > > > compared to the currently set value? This might still slightly modify > > > > width and height due to alignment restrictions. > > > > > > Most drivers that implement rotate do not have a scaler, so rotating a > > > 640x480 image would result in a 480x640 result. Hence for an m2m device > > > the output queue would have format 640x480 and the capture queue 480x640. > > > > > > So the question becomes: what if you can both rotate and scale, what > > > do you do when you change the rotate control? > > > > > > I would expect as a user of this API that if I first scale 640x480 to > > > 320x240, then rotate 90 degrees, that the capture format is now 240x320. > > > > > > In other words, rotating comes after scaling. > > > > Ok, that makes sense. I had always thought of the rotation property > > being set first. > > > > > But even if you keep the current behavior I suspect you still need to > > > update the format due to alignment restriction. Either due to 4:2:2 > > > formats or due to the 'resizer cannot downsize more than 4:1' limitation. > > > > > > E.g. in the latter case it is fine to downscale 640x480 to 640x120, > > > but if you now rotate 90 degrees, then you can no longer downscale > > > 480x640 to 640x120 (640 / 120 > 4). > > > > > > At least, if I understand the code correctly. > > > > Oh. Worse, the output queue's width alignment restrictions also depend > > on the rotation mode. > > > > Are we allowed to change the output queue format to meet alignment > > restrictions when changing the ROTATE property? The same is true > > for HFLIP. > > Certainly. Unless vb2_is_busy() is true, of course, since no format changes > are allowed once buffers are allocated. So s_ctrl would return -EBUSY in > that case. Ok. I'll do that then. > Does HFLIP change the format size? Or just the order? E.g. YUYV becomes VYUY? No, the DMA controller always reads bursts of (at least) 8 pixels, converts them to 32-bit AYUV, and writes them into an internal FIFO. That doesn't change, HFLIP just makes the Image Converter process pixels in each line from right to left. But the first processed pixel then is always the last pixel of a burst, so the only widths we can support hflipped are aligned to a multiple of 8 pixels. Non-flipped reads are still burst aligned, but we can just read over the end of lines with unaligned width into the horizontal padding / next line and ignore the last few pixels. thanks Philipp
