Hi Steve, [Added Russell to Cc: because of the question how to send IPU core patches to drm-next] Am Mittwoch, den 11.06.2014, 18:04 -0700 schrieb Steve Longerbeam: > Hi Philipp and Sascha, > > First of all, thanks for the detailed review. You are welcome. I am tasked to prepare our own capture drivers for mainline submission, but they are not quite there yet. I'd be very interested in getting this worked out together, especially since we seem to be interested in orthogonal features (we had no use for the preview and and encoder IC tasks or MIPI CSI-2 so far, but we need media controller support and mem2mem scaling via the post-processor IC task). I'm going to post our current series as is, if only to illustrate my point of view. > I think it's obvious that this patch set should be split into two: first, > the changes to IPU core driver submitted to drm-next, and the capture driver > to media-tree. I agree. > Or, do you prefer I submit the IPU core patches to your own pengutronix git > tree, and we can correspond on one of your internal mailing lists? Why would we move discussion to an internal mailing list? Since we are sitting right inbetween dri-devel and linux-media with the IPU core code, I think either mailing list is an appropriate place for discussing these patches, depending on the context. > I can then leave it to you to push those changes to drm-next. I think a central place to collect IPU core patches before sending them off to drm-next is a good idea. I offer to do the collecting in my tree. I also know Russell volunteered to collect and send off imx-drm patches towards staging. Russell, I'm not sure if this offer extends to non-DRM IPU core patches to be submitted to drm-next? (And if yes, whether you'd rather keep at it or have this taken off your hands.) > I agree with most of your feedback, and most is specific to the IPU core > changes. Excellent. For the core changes, rebasing them onto next and then integrating some remaining changes from our patchset shouldn't be a problem. The big issue I have with the media parts is the missing media controller support and the code organization around the IC tasks instead of around hardware submodules. > We can discuss those in detail elsewhere, but just in summary here, > some of your comments seem to conflict: > > 1. Regarding the input muxes to the CSI and IC, Philipp you acked those > functions but would like to see these muxes as v4l2 subdevs and configured > in the DT, but Sascha, you had a comment that this should be a job for > mediactrl. 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. They each sit between a set of parallel input pads and the CSI2IPU gasket on one side and one CSI on the other side. In my opinion, these should be handled by separate v4l2_subdev drivers and their connections described in the device tree. The links between subdevices can then be controlled via the media controller framework. This is what my comments on patches 05/43 and 08/43 are about. Second, there are the CSI0/1_DATA_SOURCE muxes and the CSI_SEL/IC_INPUT muxes controlled by the IPU_CONF register. These should not be described in the device tree. Just as CSI and IC, they are an IPU internal detail. The CSI0/1_DATA_SOURCE, if I understand correctly, muxes not the data bus, but the 8-bit mct_di bus which carries the MIPI channel id and data type. This should be controlled by the CSI subdevice driver directly when choosing its input format. The CSI_SEL/IC_INPUT multiplexer selects the input to the IC to come from either CSI0, CSI1, or the VDI. Each of those subunits would fit the media entity abstraction well. This multiplexer should be controlled by the media controller framework. I acked the CSI/IC source mux functions in patch 06/43 because I think those functions are in the right place and useful in the above scenario. > 2. Philipp, you would like to see CSI, IC, and SMFC units moved out of IPU > core and become v4l2 subdevs. I agree with that, but drm-next has ipu-smfc > as part of IPU core, and SMFC is most definitely v4l2 capture specific. About the CSI I'm just a tiny bit conflicted myself, that seems to show. >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. 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 The SMFC code is small and, more importantly, its functionality is limited enough that it doesn't warrant its own v4l2_subdev. I see not enough reason to move this away from the other IPU core code, since the v4l2_device capture driver that uses it certainly is not the right place to map SMFC registers directly. So the CSI only is a special case because the code in ipu-csi.c aligns so well with a single v4l2_subdev driver. 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
