From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Laurent Pinchart To: Hans Verkuil Cc: Niklas =?ISO-8859-1?Q?S=F6derlund?= , linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org, tomoharu.fukawa.eb@renesas.com, Kieran Bingham Subject: Re: [PATCH v13 00/33] rcar-vin: Add Gen3 with media controller Date: Tue, 03 Apr 2018 16:51:35 +0300 Message-ID: <4443207.o8kk4zUXaG@avalon> In-Reply-To: <66498032-7cd9-c744-0555-52f9a03437a9@xs4all.nl> References: <20180326214456.6655-1-niklas.soderlund+renesas@ragnatech.se> <66498032-7cd9-c744-0555-52f9a03437a9@xs4all.nl> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Hans, On Tuesday, 3 April 2018 15:30:40 EEST Hans Verkuil wrote: > On 26/03/18 23:44, Niklas S=F6derlund wrote: > > Hi, > >=20 > > This series adds Gen3 VIN support to rcar-vin driver for Renesas r8a779= 5, > > r8a7796 and r8a77970. It is based on the media-tree and depends on > > Fabrizio Castro patches as they touches the order of the compatible > > strings in the documentation to reduce merge conflicts. The dependencies > > are included in this series. >=20 > Laurent, Kieran, >=20 > Unless there are any objections I want to make a pull request for this > series once 4.17-rc1 has been merged back into our master tree. It all > looks good to me, and it will be nice to get this in (finally!). >=20 > I don't want to postpone the pull request for small improvements, they > can be applied later. But if there are more serious concerns, then let > us know. There's just a handful of patchs I haven't acked (but I have reviewed previ= ous=20 version) and I don't think there's any blocker there, small problems can=20 always be fixed later. I'll still try to review them, and if I notice any=20 issue I'll ask Niklas to submit follow-up patches. v13 can be merged upstre= am=20 from my point of view. Thank you for handling this. > > The driver is tested on Renesas H3 (r8a7795, ES2.0), > > M3-W (r8a7796) together with the rcar-csi2 driver (posted separately and > > not yet upstream) and the Salvator-X onboard ADV7482. It is also tested > > on the V3M (r8a77970) on the Eagle board together with its expansion > > board with a ADV7482 and out of tree patches for GMSL capture using the > > max9286 and rdacm20 drivers. > >=20 > > It is possible to capture both CVBS and HDMI video streams, > > v4l2-compliance passes with no errors and media-ctl can be used to > > change the routing and formats for the different entities in the media > > graph. > >=20 > > Gen2 compatibility is verified on Koelsch and no problems where found, > > video can be captured just like before and v4l2-compliance passes > > without errors or warnings just like before this series. > >=20 > > For convenience the series can be fetched from: > > git://git.ragnatech.se/linux rcar/vin/mc-v13 > >=20 > > I have started on a very basic test suite for the VIN driver at: > > https://git.ragnatech.se/vin-tests > >=20 > > And as before the state of the driver and information about how to test > >=20 > > it can be found on the elinux wiki: > > http://elinux.org/R-Car/Tests:rcar-vin > >=20 > > * Changes from v12 > > - Rebase to latest media-tree/master changed a 'return ret' to a 'goto > >=20 > > out' in rvin_start_streaming() to take recent changes to the VIN > > driver into account. > >=20 > > - Moved field !=3D V4L2_FIELD_ANY in 'rcar-vin: set a default field to > >=20 > > fallback on' check from a later commit 'rcar-vin: simplify how formats > > are set and reset' in the series. This is to avoid ignoring the field > > returned from the sensor if FIELD_ANY was requested by the user. This > > was only a problem between this change and a few patches later, but > > better to fix it now. Reported by Hans, thanks for spotting this. > >=20 > > - Fix spelling. > > - Add review tags from Hans. > >=20 > > * Changes since v11 > > - Rewrote commit message for '[PATCH v11 22/32] rcar-vin: force default > >=20 > > colorspace for media centric mode'. Also set fixed values for > > xfer_func, quantization and ycbcr_enc. > >=20 > > - Reorderd filed order in struct rvin_group_route. > > - Renamed chan to channel in struct rvin_group_route. > > - Rework 'rcar-vin: read subdevice format for crop only when > >=20 > > needed' into 'rcar-vin: simplify how formats are set and reset'. > >=20 > > - Keep caching the source dimensions and drop all changes to > >=20 > > rvin_g_selection() and rvin_s_selection(). > >=20 > > - Inline rvin_get_vin_format_from_source() into rvin_reset_format() > >=20 > > which now is the only user left. > >=20 > > - Add patch to cache the video standard instead of reading it at stream > >=20 > > on. > >=20 > > - Fix error labels in rvin_mc_open(). > > - Fixed spelling in commit messages and comment, thanks Laurent! > > - Added reviewed tags from Laurent, Thanks! > >=20 > > * Changes since v10 > > - Corrected spelling in comments and commit messages. > > - Reworked 'rcar-vin: read subdevice format for crop only when needed' > >=20 > > to only get the source format once per operation. > >=20 > > - Moved some patches around to make it easier to review, moved: > > - rcar-vin: set a default field to fallback on > > - rcar-vin: fix handling of single field frames (top, bottom and > > alternate fields) - rcar-vin: update bytesperline and sizeimage > > calculation > > - rcar-vin: break out format alignment and checking > > - rcar-vin: update pixelformat check for M1 > > =20 > > Before: > > - rcar-vin: read subdevice format for crop only when needed > >=20 > > - Rename variable 'code' to 'mbus_code' in struct rvin_dev. > > - Add comment describing no locking is needed in > >=20 > > rvin_set_channel_routing(). > >=20 > > - Check return value of pm_runtime_get_sync() in > >=20 > > rvin_set_channel_routing(). > >=20 > > - Rework 'rcar-vin: add check for colorspace' to not try to check the > >=20 > > format, instead force a default format. This should be revisited once > > either v4l2-compliance or v4l2 framework changes are worked out to > > allow for MC centric drivers to validate user supplied colorspace. > >=20 > > - Add error checking for pm_runtime_get_sync() and > >=20 > > v4l2_pipeline_pm_use(). > >=20 > > - Change mutex_lock() to mutex_lock_interruptible() in rvin_mc_open(). > > - Rewrote documentation for struct rvin_group_route. > > - Rename rvin_mc_parse_v4l2() to rvin_mc_parse_of_endpoint(). > > - Reword error messages in rvin_mc_parse_of_endpoint(). > > - Removed unneeded loop in rvin_mc_parse_of_endpoint(). > > - Remove check !is_media_entity_v4l2_subdev() in > >=20 > > rvin_group_entity_to_csi_id(). > >=20 > > - Add documentation for the algorithm used to figure out if a link can > >=20 > > be enabled or not in rvin_group_link_notify(). > >=20 > > - Break out format validation to rvin_mc_validate_format(). > > - Include two DT documentation patches from Fabrizio Castro which > >=20 > > previously where mentioned as the only dependency for this series. > >=20 > > - Added reviewed tags from Laurent, Thanks! > >=20 > > * Changes since v9 > > - Fixed mistakes in the device tree description pointed out by Laurent. > >=20 > > - GenX -> GenX platforms > > - portX -> port X > > - Explicitly state the on Gen3 platforms port 0 can only describe > > =20 > > one endpoint and that only VIN instances connected to external > > pins should have a port 0 node. > > =20 > > - s/which is/connected to/ in he endpoint description for Gen3 > > =20 > > platforms. > >=20 > > - Update some poorly written commit messages. > > - Moved the digital subdevice attach and detach code to two separate > >=20 > > functions to increase readability. > >=20 > > - Rename the struct rvin_info member chip to model to better describe > >=20 > > its purpose. > >=20 > > - Change the video name from "rcar_vin e6ef0000.video" to "VINx output" > >=20 > > where x is the VIN id. > >=20 > > - Dropped patch 'rcar-vin: do not allow changing scaling and composing > >=20 > > while streaming' as it removed Gen2 functionality which is valid as > > pointed out by Laurent. > >=20 > > - Rename rvin_get_sd_format() to rvin_get_source_format() and change its > >=20 > > parameter from struct v4l2_pix_format* to struct v4l2_mbus_framefmt*. > >=20 > > - Clarified commit message and add a few move comments to 'rcar-vin: fix > >=20 > > handling of single field frames (top, bottom and alternate fields'. > >=20 > > - Update documentation for struct rvin_dev fields mbus_cfg and code. > > - Fix argument in VNCSI_IFMD_CSI_CHSEL macro. > > - Renamed rvin_set_chsel() to rvin_set_channel_routing(). > > - Restore the VNMC register after changing CHSEL setting. > > - Broke patch 'rcar-vin: break out format alignment and checking' into > >=20 > > three parts to ease review. > >=20 > > - Add new patch to introduce a default field. > > - Only include media/v4l2-mc.h in the .c files that needs it and not in > >=20 > > rcar-vin.h. > >=20 > > - Rename rvin_group_allocate() rvin_group_get() > > - Rename rvin_group_delete() rvin_group_put() > > - Updated error message "VIN number %d already occupied\n" to "Duplicate > >=20 > > renesas,id property value %u\n". > >=20 > > - Removed dev_dbg messages which only where useful during development. > > - Dropped patch '[PATCH v9 10/28] rcar-vin: do not reset crop and > >=20 > > compose when setting format' as it introduces a regression on Gen2. > >=20 > > - Inline rvin_group_read_id() and rvin_group_add_vin() into > >=20 > > rvin_group_get(). > >=20 > > - Define static variables at the top of rcar-core.c. > > - Fix potential deadlock in rvin_group_get(). > > - Set media device model name to the VIN module name. > > - Set media device model to matched complicity string. > > - Do not use a 2 dimensional sparse array for chsel values in struct > >=20 > > rvin_info, instead use a flat array and store VIN and CHSEL value > > inside each array item. > >=20 > > - Reworked DT parsing code to make use of the new > >=20 > > v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper removing a > > lot of iffy custom parsing code. > >=20 > > - Rework how CHSEL value is calculated in the link notifier callback. > >=20 > > Using a bitmap of possible values instead of looping over an array > > turned out great reducing both LoC and increasing readability of the > > code which was a bit difficult before. It also reduced to memory > > needed to contain the static routing information. > >=20 > > - Verify the media bus pixel code when starting a stream. > > - Take the media device graph lock when figuring out and starting a > >=20 > > stream so not to race between simultaneous stream start from multiple > > rcar-vin instances as they might share common subdevices. > >=20 > > - Added review tags from Laurent. > > - Dropped tags from patches that where changed more then just correcting > >=20 > > spelling or other gramma errors. > >=20 > > * Changes since v8 > > - Fixed issue in rvin_group_init() where rvin_group_update_links() was > >=20 > > called, but after moving the video device registration to the > > complete() callback this is now invalid. > >=20 > > - Fixed spelling in commit messages. > > - Added review and acks from Hans, Kieran and Rob. > >=20 > > * Changes since v7 > > - Dropped '[PATCH v7 02/25] rcar-vin: register the video device at probe > > time' - Add patch which renames four badly name functions. Some of the > >=20 > > renaming was in v7 part of the dropped patch 02/25. Make it a own > > patch and rename all badly named functions in one patch. > >=20 > > - Add patch to replace part of the functionality of the dropped patch v7 > >=20 > > 02/25. The new patch keeps the subdevice (un)registration calls in the > > async callbacks bind() and unbind() but moves the direct subdevice > > initialization which only is used on Gen2 from the Gen2 and Gen3 > > shared rvin_v4l2_register(). > >=20 > > - Add patch to enable Renesas V3M (r8a77970) > > - Patch 'rcar-vin: parse Gen3 OF and setup media graph' have had code > >=20 > > additions since v7 since it now registers the video devices in the > > async complete() callback instead of at probe time as an effect of > > dropping v7 02/25. > > - The complete() callback now register all video devices. > > - The unbind() callback now unregister all video devices. > > - A new member '*notifier' is added to struct rvin_group which keeps > > =20 > > track of which rcar-vin instance have registered its notifier on the > > groups behalf. > > =20 > > For the reason above all Reviewed-by tags have been dropped for this > > patch. > >=20 > > - Replaced all kernel messages which used of_node_full_name() as now > >=20 > > only returns the basename and not till full path, thanks Geert. > > =20 > > printk("%s", of_node_full_name(ep)); -> printk("%pOF", ep); > >=20 > > - Added Reviewed-by tags from Hans, big thanks! > >=20 > > * Changes since v6 > > - Rebase ontop of latest media-tree which brings in the use of the > >=20 > > fwnode async helpers for Gen2. > >=20 > > - Updated DT binding documentation, thanks Laurent for very helpful > >=20 > > input! > >=20 > > - Removed help text which where copied in from v4l2_ctrl_handler_init() > >=20 > > documentation when moving that code block, this was a residue from the > > soc_camera conversion and should have been removed at that time. > >=20 > > - Removed bad check of tvnorms which disables IOCTLs if it's not set, > >=20 > > this was a residue from soc_camera conversion and have use in the > > current driver. > >=20 > > - Moved all subdevice initialization from complete to bound handler > >=20 > > while improving the unbind handler. With this move all operations of > > the ctrl_handler from the subdevice is handled in either bound or > > unbind removing races pointed out by Laurent. > >=20 > > - Renamed rvin_v4l2_probe() -> rvin_v4l2_register() and > >=20 > > rvin_v4l2_remove() -> rvin_v4l2_unregister(). > >=20 > > - Fold rvin_mbus_supported() into its only caller. > > - Sort compatible string entries in ascending order. > > - Improved documentation for struct rvin_group_chsel. > > - Clarify comment in rvin_group_csi_pad_to_chan(). > > - Make use of of_device_get_match_data() as suggested by Geert. > > - Fixed spelling mistakes. > > - Added review tags from Hans. > >=20 > > * Changes since v5 > > - Extract and make use of common format checking for both Gen2 and Gen3. > > - Assign pad at declaration time in rvin_get_sd_format() > > - Always call pm_runtime_{get_sync,put}() and v4l2_pipeline_pm_use() > >=20 > > when opening/closing a video device, remove the check of > > v4l2_fh_is_singular_file(). > >=20 > > - Make rvin_set_chsel() return void instead of int since it always > >=20 > > return 0. > >=20 > > - Simplify the VIN group allocator functions. > > - Make the group notifier callbacks and setup more robust. > > - Moved the video device registration back to probe time. > > - Add H3 ES2.0 support. > > - Fix handling of single field formats (top, bottom, alternate) as this > >=20 > > was obviously wrong before but hidden by the Gen2 scaler support. > >=20 > > - Added review tags from Kieran. > >=20 > > * Changes since v4 (Not posted to ML) > > - Updated to the new fwnode functions. > > - Moved the registration of the video devices to the async notification > >=20 > > callback. > >=20 > > * Changes since v3 > > - Only add neighboring subdevices to the async notifier. Instead of > >=20 > > parsing the whole OF graph depend on incremental async subnotifier to > > discover the whole pipeline. This is needed to support arbitrarily > > long graphs and support the new ADV7482 prototype driver which Kieran > > is working on. > >=20 > > - Fix warning from lockdep, reported by Kieran. > > - Fix commit messages from feedback from Sergei, thanks. > > - Fix chip info an OF device ids sorting order, thanks Geert. > > - Use subdev->of_node instead of subdev->dev->of_node, thanks Kieran. > >=20 > > * Changes since v2 > > - Do not try to control the subdevices in the media graph from the > > rcar-vin > >=20 > > driver. Have user-space configure to format in the pipeline instead. > >=20 > > - Add link validation before starting the stream. > > - Rework on how the subdevices are and the video node behave by defining > >=20 > > specific V4L2 operations for the MC mode of operation, this simplified > > the driver quit a bit, thanks Laurent! > >=20 > > - Add a new 'renesas,id' DT property which is needed to to be able to > >=20 > > keep the VIN to CSI-2 routing table inside the driver. Previously this > > information was taken from the CSI-2 DT node which is obviously the > > wrong way to do things. Thanks Laurent for pointing this out. > >=20 > > - Fixed a memory leek in the group allocator function. > > - Return -EMLINK instead of -EBUSY if a MC link is not possible given > >=20 > > the current routing setup. > >=20 > > - Add comments to clarify that the 4 channels from the CSI-2 node is not > >=20 > > directly related to CSI-2 virtual channels, the CSI-2 node can output > > any VC on any of its output channels. > >=20 > > * Changes since v1 > > - Remove unneeded casts as pointed out by Geert. > > - Fix spelling and DT documentation as pointed out by Geert and Sergei, > >=20 > > thanks! > >=20 > > - Refresh patch 2/32 with an updated version, thanks Sakari for pointing > >=20 > > this out. > >=20 > > - Add Sakaris Ack to patch 1/32. > > - Rebase on top of v4.9-rc1 instead of v4.9-rc3 to ease integration > >=20 > > testing together with renesas-drivers tree. > >=20 > > Fabrizio Castro (2): > > dt-bindings: media: rcar_vin: Reverse SoC part number list > > dt-bindings: media: rcar_vin: add device tree support for r8a774[35] > >=20 > > Niklas S=F6derlund (31): > > rcar-vin: add Gen3 devicetree bindings documentation > > rcar-vin: rename poorly named initialize and cleanup functions > > rcar-vin: unregister video device on driver removal > > rcar-vin: move subdevice handling to async callbacks > > rcar-vin: move model information to own struct > > rcar-vin: move max width and height information to chip information > > rcar-vin: move functions regarding scaling > > rcar-vin: all Gen2 boards can scale simplify logic > > rcar-vin: set a default field to fallback on > > rcar-vin: fix handling of single field frames (top, bottom and > > =20 > > alternate fields) > > =20 > > rcar-vin: update bytesperline and sizeimage calculation > > rcar-vin: align pixelformat check > > rcar-vin: break out format alignment and checking > > rcar-vin: simplify how formats are set and reset > > rcar-vin: cache video standard > > rcar-vin: move media bus configuration to struct rvin_dev > > rcar-vin: enable Gen3 hardware configuration > > rcar-vin: add function to manipulate Gen3 chsel value > > rcar-vin: add flag to switch to media controller mode > > rcar-vin: use different v4l2 operations in media controller mode > > rcar-vin: force default colorspace for media centric mode > > rcar-vin: prepare for media controller mode initialization > > rcar-vin: add group allocator functions > > rcar-vin: change name of video device > > rcar-vin: add chsel information to rvin_info > > rcar-vin: parse Gen3 OF and setup media graph > > rcar-vin: add link notify for Gen3 > > rcar-vin: extend {start,stop}_streaming to work with media controller > > rcar-vin: enable support for r8a7795 > > rcar-vin: enable support for r8a7796 > > rcar-vin: enable support for r8a77970 > > =20 > > .../devicetree/bindings/media/rcar_vin.txt | 137 ++- > > drivers/media/platform/rcar-vin/Kconfig | 2 +- > > drivers/media/platform/rcar-vin/rcar-core.c | 962 > > +++++++++++++++++++-- drivers/media/platform/rcar-vin/rcar-dma.c = =20 > > | 785 ++++++++++------- drivers/media/platform/rcar-vin/rcar-v4l2.c = =20 > > | 488 ++++++----- drivers/media/platform/rcar-vin/rcar-vin.h = | > > 146 +++- > > 6 files changed, 1913 insertions(+), 607 deletions(-) =2D-=20 Regards, Laurent Pinchart