On 06/13/2014 08:37 AM, Philipp Zabel wrote: > Am Donnerstag, den 12.06.2014, 14:05 -0700 schrieb Steve Longerbeam: >> Ok. Yes, we definitely need preview and MIPI CSI-2, and adding IC to the >> capture path is nice too, since it allows userland to select arbitrary user >> resolutions, pixel format color space, and also rotation controls. > > No question about that. It's just that mostly everyone around here seems > to want to capture at least 1080p, or 10-bit grayscale. I hope Freescale > drops the 1024-pixel output limitation in their next SoC ... yeah that would be nice. > >> The >> capture driver decides whether to include the IC in the capture pipeline >> based on user format and rotation control. I.e. if user colorspace is >> different from what the sensor can output, IC CSC is required. If user >> resolution is different from the selected capture cropping rectangle, >> IC resizer is required, and finally if user requests rotation, the IC >> rotation unit is required. If none of those are true, the capture driver >> decides to exclude the IC from the pipeline and send raw sensor frames >> (well, after cropping anyway) directly to memory via the SMFC. > > That is too much magic for my taste. Especially since whether or not you > can use the IC not only depends on the current video format, but also on > whether the other CSI or the MEM_VDIC_MEM path are using the IC at the > moment. Right. Our current capture driver just returns -EBUSY if the IC PRPENC task is currently in use by the other CSI. But I have come around, I agree with you now that, if we are to make the capture driver a media device that is controlling an IC entity, that the links to/from the IC need to be controlled by media controller API. I guess I've been thinking too much in terms of "classical" V4L2. > Since this can change dynamically, it throws a wrench into GStreamer's > static capability negotiation, for example. I'd rather have userspace > select which CSI should be routed through the IC with media-ctl and then > reflect the possible conversions in the respective video_dev's > capabilities. yes, the capabilities will change dynamically depending on the current video pipeline that has been setup. > >> So in our driver, the decision to link the IC in a pipeline is made >> internally by the driver and is not a decision exported to userland. > > This is exactly the point I am worried about. You lose flexibility and > need all sorts of clever conditional code in the driver. It'd be much > cleaner to just let userspace control the mux. the conditionals weren't actually that clever. But like I said, I was thinking in terms of classic V4L2 where internal video blocks are used or not depending on user requested format. I agree now that IC usage should be part of media control. > >> My plan was to add media device framework support, but only after basic >> video capture is in place. Our driver is full featured in terms of basic >> capture support, and it works on all three reference platforms. But I >> agree it needs to convert subdev's to media entities and allow some of >> them to be linked via the media controller API. > > Alright, so we agree that using the media controller API internally is a > good idea ... > >> But only some linkages make sense to me. As I explain above, if the IC were >> to be made a media entity, I think it's linkage should be made internally >> by the capture driver, and this should not be controllable by userspace. > > ... but we disagree on whether to export the control to userspace. For > more complicated pipelines in front of the CSIs we'll need media-ctl > anyway, so using that same API for the internal components, makes sense. > It also allows userspace to get a clear and stable picture of the > available features for any given multiplexer setting. > >> Heh, we have a mem2mem driver as well, and it also uses IC post-processor >> task. It uses banding and striping to support resized output frames larger >> than 1024x1024. It also makes use of IC rotation and CSC. > > Of course :) > >> But again this is not converted to a media entity. And again, if IC were to >> be made a standalone media entity, then the mem2mem device would _always_ >> require the IC post-processor be linked to it, since the essential feature >> of mem2mem is to make use of IC post-processor task for CSC, resize, and >> rotation operations. > > Since the three IC tasks are transparently time-multiplexed, the IC > media entity representation could have input and output pads for each of > them. > The preprocessing (encoding, viewfinder) tasks share an input pad that > would be connected to either CSI0, CSI1, or VDI output pad. These links > should control the IC mux. The encoding task output pad would represent > IDMAC channel 20/CB0 or channel 48/CB8, depending on whether the rotator > is active. Since rotation requires tight integration between IC and > IDMAC, I don't think the IRT should be represented as a separate media > entity. > The viewfinder task output pad would correspond to channel 21/CB1 or > channel 49/CB9, and maybe in the future control whether to send that > data off to the DMFC or to memory. right. Currently we don't attempt to use the direct IC-PRPVF --> DMFC path. Viewfinder code looks much like PRPENC (like you or Sascha pointed out earlier), it just transfers scaled/CSC/rotated frames to a framebuffer addr in memory provided by s_fbuf. > The postprocessing input and output pads would go straight to memory and > are not configurable, so I see no need to describe IC-PP as media > entity. er, now I'm being your own best advocate! :) If IC-PP I/O is going to be described by pads, then I think IC-PP would have to be described as a media entity. For mem2mem device, the sink and source links to IC-PP would have to marked as immutable (always enabled). > > I'm not quite sure about the VDIC, but I guess that also should be > configurable from userspace as one input to IC. For the deinterlacer to > work, IC_INPUT needs to be set, so while this is active there is no way > to route CSI0/1 through the IC directly. > sounds right. But there's too much to think about already. Btw we have tried to implement direct CSI --> VDIC --> IC path without success. Freescale's BSP does not use the VDIC in this way, but rather as mem --> VDIC --> mem, i.e. a kind of post-processing after capture. I'm a bit vague about this since I haven't looked at VDIC yet in much detail. > [...] >>> No conflict here, there are different multiplexers to talk about. >>> >>> First, there are two external multiplexers controlled by IOMUXC (on >>> i.MX6, these don't exist on i.MX5): MIPI_IPU1/2_MUX on i.MX6Q and >>> IPU_CSI0/1_MUX. They are not part of the IPU. > [...] >> right, this is one place where subdev linking makes sense to me. I.e. >> linking sensors to CSI ports. >> >> But you don't mean to allow userspace to make this link arbitrarily, >> correct? You mean the driver uses the mediactrl framework to implement >> the links defined in the device tree. Maybe that's where I was confused. > > No, this is exactly what I mean. In my opinion, using media-ctl to throw > the switches is much better than letting the driver decide depending on > the old S_INPUT API. Especially since this gives userspace a unified API > when there are input multiplexers (or any other subdevices configurable > on the pad level) external to the SoC. > Also, pad format configuration of the CSI subdev exported to userspace > may be useful to control the compander in the CSI. > > [...] >>> From an organizational standpoint, with all the other register access >>> code in gpu/ipu-v3, having the ipu-csi code in there too looks nice and >>> as expected. >>> On the other hand, this should really be only used by one >>> v4l2_subdev driver. When I look at it this way, I see a driver that is >>> split in two parts, wasting exported symbol space for no very good >>> reason. >> >> I agree about making CSI a subdev, but I also think we can keep all of the >> register access in IPU core as well. The CSI subdev would be a wrapper >> around the ipu-csi APIs. I agree it's more use of symbol space, but we >> might be able to simplify the ipu-csi API. > > So be it. In any case, this decision could be changed later with little > effort and without any externally visible changes, if deemed necessary, > as long as the CSI v4l2_subdev driver is the only consumer of this API. > >>> The IC I'd like to describe as a v4l2_subdev. But I'd also like to use >>> the IC from the DRM driver. So the IC core code has to stay in >>> gpu/ipu-v3. I'd just like to pool all V4L2 code that uses this into a IC >>> v4l2_subdev driver if possible. The only use we have for the IC >>> currently is >> >> is mem2mem? And you would like to see an IC pads linked with mem2mem >> pads. Well, something like this: >> >> input frame from userspace ---> m2m ---> IC-PP ---> m2m ---> to userspace > > As long as there is no internal video bus switch somewhere, the > usefulness of this is debatable. You see we also implemented this by > directly calling the IPU IC API, and I'm fine with this. well, as I said above I think if we are going to make the IC a media entity we might as well make mem2mem a media device and include IC-PP as an entity with immutable links to it. Steve > > [...] >> Ok, in summary I'm aligned with everything you said. Only that I am still >> pondering about which media entity links make sense, and who should be >> allowed to make those links. > > At least the decision whether to route the CSI0/1 into the IC > preprocessing input should be handled by userspace, as this changes the > capabilities of the corresponding video_dev. > >> So how should we proceed? I would argue to use our capture driver as a base, >> since it is fully functional and fairly full-featured. It's just missing the >> media framework. > > Initially, I don't care as much about a full featureset as I do about > getting the userspace API and device tree bindings right, since those > won't be easy to change later. > Pending rework into CSI/IC subdevices and agreement on the userspace > facing API, I think your patchset can be a good base. > > regards > Philipp > -- 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
