From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Sat, 21 Apr 2018 11:20:00 +0300 Subject: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc In-Reply-To: <20180420112204.GK4235@w540> References: <20180419162751.25223-1-peda@axentia.se> <6354350.l8DgUyNhLT@avalon> <20180420112204.GK4235@w540> Message-ID: <1980444.sF26are89T@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Jacopo, On Friday, 20 April 2018 14:22:04 EEST jacopo mondi wrote: > On Fri, Apr 20, 2018 at 01:18:13PM +0300, Laurent Pinchart wrote: > > On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > > > Hi Peter, > > > > > > I've been a bit a pain in the arse for you recently, but please > > > bear with me a bit more, and sorry for jumping late on the band wagon. > > > > > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > > > Hi! > > > > > > > > I naively thought that since there was support for both nxp,tda19988 > > > > (in the tda998x driver) and the atmel-hlcdc, things would be a smooth > > > > ride. But it wasn't, so I started looking around and realized I had to > > > > fix things. > > > > > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > > > component, but now in v3 I fix things by making the tda998x driver > > > > a bridge instead. This was after a suggestion from Boris Brezillion > > > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > > > after comparing what was needed, I too find the bridge approach > > > > better. > > > > > > > > In addition to the above, our PCB interface between the SAMA5D3 and > > > > the HDMI encoder is only using 16 bits, and this has to be described > > > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > > > correct output mode. Since I have similar problems with a ds90c185 > > > > lvds encoder I added patches to override the atmel-hlcdc output format > > > > via DT properties compatible with the media video-interface binding > > > > and things start to play together. > > > > > > > > Since this series superseeds the bridge series [1], I have included > > > > the leftover bindings patch for the ti,ds90c185 here. > > > > > > I feel like this series would look better if it would make use of the > > > proposed bridge media bus format support I have recently sent out [1] > > > (and which was not there when you first sent v1). > > > > > > I understand your fundamental problem here is that the bus format > > > that should be reported by your bridge is different from the ones > > > actually supported by the TDA19988 chip, as the wirings ground some > > > of the input pins. > > > > > > Although this is defintely something that could be described in the > > > bridge's own OF node with the 'bus_width' property, and what I would do, > > > now that you have made a bridge out from the tda19988 driver, is: > > > > > > 1) Set the bridge accepted input bus_format parsing its pin > > > configuration, or default it if that's not implemented yet. > > > This will likely be rgb888. You can do that using the trivial > > > support for bridge input image formats implemented by my series. > > > 2) Specify in the bridge endpoint a 'bus_width' of <16> > > > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > > > and parse the remote endpoint property 'bus_width' and get the <16> > > > value back. > > > > Parsing properties of remote nodes should be avoided as much as possible, > > as they need to be interpreted in the context of the DT bindings related > > to the compatible string applicable to that node. I'd rather have the > > bus_width property in the local endpoint node. > > Right, I see... > Well, in Peter's case, the bus width, being a consequence of > wirings, can be described safely in both endpoints, right? If we look at it from the endpoints' point of view, the display controller has to output a 16-bit format, and the HDMI encoder receives a 24-bit format. The conversion is due to PCB wiring so there's no DT node to describe that. I thus believe the right description to be bus-width = <16> on the display controller side, and bus-width = <24> on the HDMI encoder side. This being said, even if the bus-width property was set to 16 on both sides, what I frown upon is parsing a property of the HDMI encoder DT node in the display controller driver. > > > 4) Set the correct format in the atmel hlcd driver to accommodate a > > > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > > > or are there other possible combinations I am missing?) > > > > > > I would consider this better mostly because in this series you are > > > creating a semantic for the whole DRM subsystem on the 'bus_width' > > > property, where numerical values as '12', '16' etc are arbitrary tied > > > to the selection of a media bus format. At least you should use a > > > common set of defines [1] between the device tree and the driver, > > > but this looks anyway fragile imho. > > > > This I agree with though. Combining the remote bus format with the local > > bus width should fix the problem without having to parse remote > > properties. > > > > > Have I maybe missed some parts of the problem you are trying to solve > > > here? > > > > > > [1] drm: bridge: Add support for static image formats > > > > > > https://lwn.net/Articles/752296/ > > > > > > [2] include/uapi/linux/media-bus-format.h > > > > > > > Anyway, this series solves some real issues for my HW. > > > > > > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > > > > - patch 2/7 fixed spelling and added an example > > > > - patch 4/7 parse the DT up front and store the result indexed by > > > > encoder > > > > - old patch 5/6 and 6/6 dropped > > > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > > > > > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > > > > - added reviewed-by from Rob to patch 1/6 > > > > - patch 2/6 changed so that the bus format override is in the endpoint > > > > > > > > DT node, and follows the binding of media video-interfaces. > > > > > > > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > > > > > > > > media video-interface binding (partially). > > > > > > > > - patch 4/6 now makes use of the above helper (and also fixes problems > > > > > > > > with the 3/5 patch from v1 when no override was specified). > > > > > > > > - do not mention unrelated connector display_info details in the cover > > > > > > > > letter and commit messages. > > > > > > > > [1] > > > > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 > > > > "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221 > > > > > > > > Peter Rosin (7): > > > > dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > > > > dt-bindings: display: atmel: optional video-interface of endpoints > > > > drm: of: introduce drm_of_media_bus_fmt > > > > drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes > > > > drm/i2c: tda998x: find the drm_device via the drm_connector > > > > drm/i2c: tda998x: split encoder and component functions from the > > > > work > > > > drm/i2c: tda998x: register as a drm bridge > > > > > > > > .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++ > > > > .../bindings/display/bridge/lvds-transmitter.txt | 8 +- > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 71 ++++++-- > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 + > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 40 +++- > > > > drivers/gpu/drm/drm_of.c | 38 ++++ > > > > drivers/gpu/drm/i2c/tda998x_drv.c | 201 ++++++++++-- > > > > include/drm/drm_of.h | 7 + > > > > 8 files changed, 342 insertions(+), 51 deletions(-) -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc Date: Sat, 21 Apr 2018 11:20:00 +0300 Message-ID: <1980444.sF26are89T@avalon> References: <20180419162751.25223-1-peda@axentia.se> <6354350.l8DgUyNhLT@avalon> <20180420112204.GK4235@w540> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20180420112204.GK4235@w540> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: jacopo mondi Cc: Mark Rutland , Boris Brezillon , Alexandre Belloni , devicetree@vger.kernel.org, David Airlie , Nicolas Ferre , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Rob Herring , Jacopo Mondi , Daniel Vetter , Peter Rosin , Russell King , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGkgSmFjb3BvLAoKT24gRnJpZGF5LCAyMCBBcHJpbCAyMDE4IDE0OjIyOjA0IEVFU1QgamFjb3Bv IG1vbmRpIHdyb3RlOgo+IE9uIEZyaSwgQXByIDIwLCAyMDE4IGF0IDAxOjE4OjEzUE0gKzAzMDAs IExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gPiBPbiBGcmlkYXksIDIwIEFwcmlsIDIwMTggMTE6 NTI6MzUgRUVTVCBqYWNvcG8gbW9uZGkgd3JvdGU6Cj4gPiA+IEhpIFBldGVyLAo+ID4gPiAKPiA+ ID4gSSd2ZSBiZWVuIGEgYml0IGEgcGFpbiBpbiB0aGUgYXJzZSBmb3IgeW91IHJlY2VudGx5LCBi dXQgcGxlYXNlCj4gPiA+IGJlYXIgd2l0aCBtZSBhIGJpdCBtb3JlLCBhbmQgc29ycnkgZm9yIGp1 bXBpbmcgbGF0ZSBvbiB0aGUgYmFuZCB3YWdvbi4KPiA+ID4gCj4gPiA+IE9uIFRodSwgQXByIDE5 LCAyMDE4IGF0IDA2OjI3OjQ0UE0gKzAyMDAsIFBldGVyIFJvc2luIHdyb3RlOgo+ID4gPiA+IEhp IQo+ID4gPiA+IAo+ID4gPiA+IEkgbmFpdmVseSB0aG91Z2h0IHRoYXQgc2luY2UgdGhlcmUgd2Fz IHN1cHBvcnQgZm9yIGJvdGggbnhwLHRkYTE5OTg4Cj4gPiA+ID4gKGluIHRoZSB0ZGE5OTh4IGRy aXZlcikgYW5kIHRoZSBhdG1lbC1obGNkYywgdGhpbmdzIHdvdWxkIGJlIGEgc21vb3RoCj4gPiA+ ID4gcmlkZS4gQnV0IGl0IHdhc24ndCwgc28gSSBzdGFydGVkIGxvb2tpbmcgYXJvdW5kIGFuZCBy ZWFsaXplZCBJIGhhZCB0bwo+ID4gPiA+IGZpeCB0aGluZ3MuCj4gPiA+ID4gCj4gPiA+ID4gSW4g djEgYW5kIHYyIEkgZml4ZWQgdGhpbmdzIGJ5IG1ha2luZyB0aGUgYXRtZWwtaGxjZGMgZHJpdmVy IGEgbWFzdGVyCj4gPiA+ID4gY29tcG9uZW50LCBidXQgbm93IGluIHYzIEkgZml4IHRoaW5ncyBi eSBtYWtpbmcgdGhlIHRkYTk5OHggZHJpdmVyCj4gPiA+ID4gYSBicmlkZ2UgaW5zdGVhZC4gVGhp cyB3YXMgYWZ0ZXIgYSBzdWdnZXN0aW9uIGZyb20gQm9yaXMgQnJlemlsbGlvbgo+ID4gPiA+ICh0 aGUgYXRtZWwtaGxjZGMgbWFpbnRhaW5lciksIHNvIHRoZXJlIHdhcyBzb21lIHJpc2sgb2YgYmlh cyAuLi4gYnV0Cj4gPiA+ID4gYWZ0ZXIgY29tcGFyaW5nIHdoYXQgd2FzIG5lZWRlZCwgSSB0b28g ZmluZCB0aGUgYnJpZGdlIGFwcHJvYWNoCj4gPiA+ID4gYmV0dGVyLgo+ID4gPiA+IAo+ID4gPiA+ IEluIGFkZGl0aW9uIHRvIHRoZSBhYm92ZSwgb3VyIFBDQiBpbnRlcmZhY2UgYmV0d2VlbiB0aGUg U0FNQTVEMyBhbmQKPiA+ID4gPiB0aGUgSERNSSBlbmNvZGVyIGlzIG9ubHkgdXNpbmcgMTYgYml0 cywgYW5kIHRoaXMgaGFzIHRvIGJlIGRlc2NyaWJlZAo+ID4gPiA+IHNvbWV3aGVyZSwgb3IgdGhl IGF0bWVsLWhsY2RjIGRyaXZlciBoYXZlIG5vIGNoYW5jZSBvZiBzZWxlY3RpbmcgdGhlCj4gPiA+ ID4gY29ycmVjdCBvdXRwdXQgbW9kZS4gU2luY2UgSSBoYXZlIHNpbWlsYXIgcHJvYmxlbXMgd2l0 aCBhIGRzOTBjMTg1Cj4gPiA+ID4gbHZkcyBlbmNvZGVyIEkgYWRkZWQgcGF0Y2hlcyB0byBvdmVy cmlkZSB0aGUgYXRtZWwtaGxjZGMgb3V0cHV0IGZvcm1hdAo+ID4gPiA+IHZpYSBEVCBwcm9wZXJ0 aWVzIGNvbXBhdGlibGUgd2l0aCB0aGUgbWVkaWEgdmlkZW8taW50ZXJmYWNlIGJpbmRpbmcKPiA+ ID4gPiBhbmQgdGhpbmdzIHN0YXJ0IHRvIHBsYXkgdG9nZXRoZXIuCj4gPiA+ID4gCj4gPiA+ID4g U2luY2UgdGhpcyBzZXJpZXMgc3VwZXJzZWVkcyB0aGUgYnJpZGdlIHNlcmllcyBbMV0sIEkgaGF2 ZSBpbmNsdWRlZAo+ID4gPiA+IHRoZSBsZWZ0b3ZlciBiaW5kaW5ncyBwYXRjaCBmb3IgdGhlIHRp LGRzOTBjMTg1IGhlcmUuCj4gPiA+IAo+ID4gPiBJIGZlZWwgbGlrZSB0aGlzIHNlcmllcyB3b3Vs ZCBsb29rIGJldHRlciBpZiBpdCB3b3VsZCBtYWtlIHVzZSBvZiB0aGUKPiA+ID4gcHJvcG9zZWQg YnJpZGdlIG1lZGlhIGJ1cyBmb3JtYXQgc3VwcG9ydCBJIGhhdmUgcmVjZW50bHkgc2VudCBvdXQg WzFdCj4gPiA+IChhbmQgd2hpY2ggd2FzIG5vdCB0aGVyZSB3aGVuIHlvdSBmaXJzdCBzZW50IHYx KS4KPiA+ID4gCj4gPiA+IEkgdW5kZXJzdGFuZCB5b3VyIGZ1bmRhbWVudGFsIHByb2JsZW0gaGVy ZSBpcyB0aGF0IHRoZSBidXMgZm9ybWF0Cj4gPiA+IHRoYXQgc2hvdWxkIGJlIHJlcG9ydGVkIGJ5 IHlvdXIgYnJpZGdlIGlzIGRpZmZlcmVudCBmcm9tIHRoZSBvbmVzCj4gPiA+IGFjdHVhbGx5IHN1 cHBvcnRlZCBieSB0aGUgVERBMTk5ODggY2hpcCwgYXMgdGhlIHdpcmluZ3MgZ3JvdW5kIHNvbWUK PiA+ID4gb2YgdGhlIGlucHV0IHBpbnMuCj4gPiA+IAo+ID4gPiBBbHRob3VnaCB0aGlzIGlzIGRl ZmludGVseSBzb21ldGhpbmcgdGhhdCBjb3VsZCBiZSBkZXNjcmliZWQgaW4gdGhlCj4gPiA+IGJy aWRnZSdzIG93biBPRiBub2RlIHdpdGggdGhlICdidXNfd2lkdGgnIHByb3BlcnR5LCBhbmQgd2hh dCBJIHdvdWxkIGRvLAo+ID4gPiBub3cgdGhhdCB5b3UgaGF2ZSBtYWRlIGEgYnJpZGdlIG91dCBm cm9tIHRoZSB0ZGExOTk4OCBkcml2ZXIsIGlzOgo+ID4gPiAKPiA+ID4gMSkgU2V0IHRoZSBicmlk Z2UgYWNjZXB0ZWQgaW5wdXQgYnVzX2Zvcm1hdCBwYXJzaW5nIGl0cyBwaW4KPiA+ID4gY29uZmln dXJhdGlvbiwgb3IgZGVmYXVsdCBpdCBpZiB0aGF0J3Mgbm90IGltcGxlbWVudGVkIHlldC4KPiA+ ID4gVGhpcyB3aWxsIGxpa2VseSBiZSByZ2I4ODguIFlvdSBjYW4gZG8gdGhhdCB1c2luZyB0aGUg dHJpdmlhbAo+ID4gPiBzdXBwb3J0IGZvciBicmlkZ2UgaW5wdXQgaW1hZ2UgZm9ybWF0cyBpbXBs ZW1lbnRlZCBieSBteSBzZXJpZXMuCj4gPiA+IDIpIFNwZWNpZnkgaW4gdGhlIGJyaWRnZSBlbmRw b2ludCBhICdidXNfd2lkdGgnIG9mIDwxNj4KPiA+ID4gMykgSW4geW91ciBhdG1lbC1obGNkIGdl dCBib3RoIHRoZSBpbWFnZSBmb3JtYXQgb2YgdGhlIGJyaWRnZSAocmdiODg4KQo+ID4gPiBhbmQg cGFyc2UgdGhlIHJlbW90ZSBlbmRwb2ludCBwcm9wZXJ0eSAnYnVzX3dpZHRoJyBhbmQgZ2V0IHRo ZSA8MTY+Cj4gPiA+IHZhbHVlIGJhY2suCj4gPiAKPiA+IFBhcnNpbmcgcHJvcGVydGllcyBvZiBy ZW1vdGUgbm9kZXMgc2hvdWxkIGJlIGF2b2lkZWQgYXMgbXVjaCBhcyBwb3NzaWJsZSwKPiA+IGFz IHRoZXkgbmVlZCB0byBiZSBpbnRlcnByZXRlZCBpbiB0aGUgY29udGV4dCBvZiB0aGUgRFQgYmlu ZGluZ3MgcmVsYXRlZAo+ID4gdG8gdGhlIGNvbXBhdGlibGUgc3RyaW5nIGFwcGxpY2FibGUgdG8g dGhhdCBub2RlLiBJJ2QgcmF0aGVyIGhhdmUgdGhlCj4gPiBidXNfd2lkdGggcHJvcGVydHkgaW4g dGhlIGxvY2FsIGVuZHBvaW50IG5vZGUuCj4gCj4gUmlnaHQsIEkgc2VlLi4uCj4gV2VsbCwgaW4g UGV0ZXIncyBjYXNlLCB0aGUgYnVzIHdpZHRoLCBiZWluZyBhIGNvbnNlcXVlbmNlIG9mCj4gd2ly aW5ncywgY2FuIGJlIGRlc2NyaWJlZCBzYWZlbHkgaW4gYm90aCBlbmRwb2ludHMsIHJpZ2h0PwoK SWYgd2UgbG9vayBhdCBpdCBmcm9tIHRoZSBlbmRwb2ludHMnIHBvaW50IG9mIHZpZXcsIHRoZSBk aXNwbGF5IGNvbnRyb2xsZXIgaGFzIAp0byBvdXRwdXQgYSAxNi1iaXQgZm9ybWF0LCBhbmQgdGhl IEhETUkgZW5jb2RlciByZWNlaXZlcyBhIDI0LWJpdCBmb3JtYXQuIFRoZSAKY29udmVyc2lvbiBp cyBkdWUgdG8gUENCIHdpcmluZyBzbyB0aGVyZSdzIG5vIERUIG5vZGUgdG8gZGVzY3JpYmUgdGhh dC4gSSB0aHVzIApiZWxpZXZlIHRoZSByaWdodCBkZXNjcmlwdGlvbiB0byBiZSBidXMtd2lkdGgg PSA8MTY+IG9uIHRoZSBkaXNwbGF5IGNvbnRyb2xsZXIgCnNpZGUsIGFuZCBidXMtd2lkdGggPSA8 MjQ+IG9uIHRoZSBIRE1JIGVuY29kZXIgc2lkZS4KClRoaXMgYmVpbmcgc2FpZCwgZXZlbiBpZiB0 aGUgYnVzLXdpZHRoIHByb3BlcnR5IHdhcyBzZXQgdG8gMTYgb24gYm90aCBzaWRlcywgCndoYXQg SSBmcm93biB1cG9uIGlzIHBhcnNpbmcgYSBwcm9wZXJ0eSBvZiB0aGUgSERNSSBlbmNvZGVyIERU IG5vZGUgaW4gdGhlIApkaXNwbGF5IGNvbnRyb2xsZXIgZHJpdmVyLgoKPiA+ID4gNCkgU2V0IHRo ZSBjb3JyZWN0IGZvcm1hdCBpbiB0aGUgYXRtZWwgaGxjZCBkcml2ZXIgdG8gYWNjb21tb2RhdGUg YQo+ID4gPiBicmlkZ2UgdGhhdCB3YW50cyByZ2I4ODggb24gYSAxNiBiaXQgd2lkZSBidXMgKHRo YXQgd291bGQgYmUgcmdiNTY1LAo+ID4gPiBvciBhcmUgdGhlcmUgb3RoZXIgcG9zc2libGUgY29t YmluYXRpb25zIEkgYW0gbWlzc2luZz8pCj4gPiA+IAo+ID4gPiBJIHdvdWxkIGNvbnNpZGVyIHRo aXMgYmV0dGVyIG1vc3RseSBiZWNhdXNlIGluIHRoaXMgc2VyaWVzIHlvdSBhcmUKPiA+ID4gY3Jl YXRpbmcgYSBzZW1hbnRpYyBmb3IgdGhlIHdob2xlIERSTSBzdWJzeXN0ZW0gb24gdGhlICdidXNf d2lkdGgnCj4gPiA+IHByb3BlcnR5LCB3aGVyZSBudW1lcmljYWwgdmFsdWVzIGFzICcxMicsICcx NicgZXRjIGFyZSBhcmJpdHJhcnkgdGllZAo+ID4gPiB0byB0aGUgc2VsZWN0aW9uIG9mIGEgbWVk aWEgYnVzIGZvcm1hdC4gQXQgbGVhc3QgeW91IHNob3VsZCB1c2UgYQo+ID4gPiBjb21tb24gc2V0 IG9mIGRlZmluZXMgWzFdIGJldHdlZW4gdGhlIGRldmljZSB0cmVlIGFuZCB0aGUgZHJpdmVyLAo+ ID4gPiBidXQgdGhpcyBsb29rcyBhbnl3YXkgZnJhZ2lsZSBpbWhvLgo+ID4gCj4gPiBUaGlzIEkg YWdyZWUgd2l0aCB0aG91Z2guIENvbWJpbmluZyB0aGUgcmVtb3RlIGJ1cyBmb3JtYXQgd2l0aCB0 aGUgbG9jYWwKPiA+IGJ1cyB3aWR0aCBzaG91bGQgZml4IHRoZSBwcm9ibGVtIHdpdGhvdXQgaGF2 aW5nIHRvIHBhcnNlIHJlbW90ZQo+ID4gcHJvcGVydGllcy4KPiA+IAo+ID4gPiBIYXZlIEkgbWF5 YmUgbWlzc2VkIHNvbWUgcGFydHMgb2YgdGhlIHByb2JsZW0geW91IGFyZSB0cnlpbmcgdG8gc29s dmUKPiA+ID4gaGVyZT8KPiA+ID4gCj4gPiA+IFsxXSBkcm06IGJyaWRnZTogQWRkIHN1cHBvcnQg Zm9yIHN0YXRpYyBpbWFnZSBmb3JtYXRzCj4gPiA+IAo+ID4gPiAgICAgaHR0cHM6Ly9sd24ubmV0 L0FydGljbGVzLzc1MjI5Ni8KPiA+ID4gCj4gPiA+IFsyXSBpbmNsdWRlL3VhcGkvbGludXgvbWVk aWEtYnVzLWZvcm1hdC5oCj4gPiA+IAo+ID4gPiA+IEFueXdheSwgdGhpcyBzZXJpZXMgc29sdmVz IHNvbWUgcmVhbCBpc3N1ZXMgZm9yIG15IEhXLgo+ID4gPiA+IAo+ID4gPiA+IENoYW5nZXMgc2lu Y2UgdjIgICBodHRwczovL2xrbWwub3JnL2xrbWwvMjAxOC80LzE3LzM4NQo+ID4gPiA+IC0gcGF0 Y2ggMi83IGZpeGVkIHNwZWxsaW5nIGFuZCBhZGRlZCBhbiBleGFtcGxlCj4gPiA+ID4gLSBwYXRj aCA0LzcgcGFyc2UgdGhlIERUIHVwIGZyb250IGFuZCBzdG9yZSB0aGUgcmVzdWx0IGluZGV4ZWQg YnkKPiA+ID4gPiBlbmNvZGVyCj4gPiA+ID4gLSBvbGQgcGF0Y2ggNS82IGFuZCA2LzYgZHJvcHBl ZAo+ID4gPiA+IC0gcGF0Y2ggNS03LzcgYXJlIG5ldyBhbmQgbWFrZXMgdGhlIHRkYTk5OHggZHJp dmVyIGEgZHJtX2JyaWRnZQo+ID4gPiA+IAo+ID4gPiA+IENoYW5nZXMgc2luY2UgdjEgICBodHRw czovL2xrbWwub3JnL2xrbWwvMjAxOC80LzkvMjk0Cj4gPiA+ID4gLSBhZGRlZCByZXZpZXdlZC1i eSBmcm9tIFJvYiB0byBwYXRjaCAxLzYKPiA+ID4gPiAtIHBhdGNoIDIvNiBjaGFuZ2VkIHNvIHRo YXQgdGhlIGJ1cyBmb3JtYXQgb3ZlcnJpZGUgaXMgaW4gdGhlIGVuZHBvaW50Cj4gPiA+ID4gCj4g PiA+ID4gICBEVCBub2RlLCBhbmQgZm9sbG93cyB0aGUgYmluZGluZyBvZiBtZWRpYSB2aWRlby1p bnRlcmZhY2VzLgo+ID4gPiA+IAo+ID4gPiA+IC0gcGF0Y2ggMy82IGlzIG5ldywgaXQgYWRkcyBk cm1fb2ZfbWVkaWFfYnVzX2ZtdCB3aGljaCBwYXJzZXMgYWJvdmUKPiA+ID4gPiAKPiA+ID4gPiAg IG1lZGlhIHZpZGVvLWludGVyZmFjZSBiaW5kaW5nIChwYXJ0aWFsbHkpLgo+ID4gPiA+IAo+ID4g PiA+IC0gcGF0Y2ggNC82IG5vdyBtYWtlcyB1c2Ugb2YgdGhlIGFib3ZlIGhlbHBlciAoYW5kIGFs c28gZml4ZXMgcHJvYmxlbXMKPiA+ID4gPiAKPiA+ID4gPiAgIHdpdGggdGhlIDMvNSBwYXRjaCBm cm9tIHYxIHdoZW4gbm8gb3ZlcnJpZGUgd2FzIHNwZWNpZmllZCkuCj4gPiA+ID4gCj4gPiA+ID4g LSBkbyBub3QgbWVudGlvbiB1bnJlbGF0ZWQgY29ubmVjdG9yIGRpc3BsYXlfaW5mbyBkZXRhaWxz IGluIHRoZSBjb3Zlcgo+ID4gPiA+IAo+ID4gPiA+ICAgbGV0dGVyIGFuZCBjb21taXQgbWVzc2Fn ZXMuCj4gPiA+ID4gCj4gPiA+ID4gWzFdCj4gPiA+ID4gIkJyaWRnZSIgc2VyaWVzIHYyICAgaHR0 cHM6Ly9sa21sLm9yZy9sa21sLzIwMTgvMy8yNi82MTAKPiA+ID4gPiAiQnJpZGdlIiBzZXJpZXMg djEgICBodHRwczovL2xrbWwub3JnL2xrbWwvMjAxOC8zLzE3LzIyMQo+ID4gPiA+IAo+ID4gPiA+ IFBldGVyIFJvc2luICg3KToKPiA+ID4gPiAgIGR0LWJpbmRpbmdzOiBkaXNwbGF5OiBicmlkZ2U6 IGx2ZHMtdHJhbnNtaXR0ZXI6IGFkZCB0aSxkczkwYzE4NQo+ID4gPiA+ICAgZHQtYmluZGluZ3M6 IGRpc3BsYXk6IGF0bWVsOiBvcHRpb25hbCB2aWRlby1pbnRlcmZhY2Ugb2YgZW5kcG9pbnRzCj4g PiA+ID4gICBkcm06IG9mOiBpbnRyb2R1Y2UgZHJtX29mX21lZGlhX2J1c19mbXQKPiA+ID4gPiAg IGRybS9hdG1lbC1obGNkYzogc3VwcG9ydCBidXMtd2lkdGggKDEyLzE2LzE4LzI0KSBpbiBlbmRw b2ludCBub2Rlcwo+ID4gPiA+ICAgZHJtL2kyYzogdGRhOTk4eDogZmluZCB0aGUgZHJtX2Rldmlj ZSB2aWEgdGhlIGRybV9jb25uZWN0b3IKPiA+ID4gPiAgIGRybS9pMmM6IHRkYTk5OHg6IHNwbGl0 IGVuY29kZXIgYW5kIGNvbXBvbmVudCBmdW5jdGlvbnMgZnJvbSB0aGUKPiA+ID4gPiAgIHdvcmsK PiA+ID4gPiAgIGRybS9pMmM6IHRkYTk5OHg6IHJlZ2lzdGVyIGFzIGEgZHJtIGJyaWRnZQo+ID4g PiA+ICAKPiA+ID4gPiAgLi4uL2RldmljZXRyZWUvYmluZGluZ3MvZGlzcGxheS9hdG1lbC9obGNk Yy1kYy50eHQgfCAgMjYgKysrCj4gPiA+ID4gIC4uLi9iaW5kaW5ncy9kaXNwbGF5L2JyaWRnZS9s dmRzLXRyYW5zbWl0dGVyLnR4dCAgIHwgICA4ICstCj4gPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9h dG1lbC1obGNkYy9hdG1lbF9obGNkY19jcnRjLmMgICAgIHwgIDcxICsrKysrKy0tCj4gPiA+ID4g IGRyaXZlcnMvZ3B1L2RybS9hdG1lbC1obGNkYy9hdG1lbF9obGNkY19kYy5oICAgICAgIHwgICAy ICsKPiA+ID4gPiAgZHJpdmVycy9ncHUvZHJtL2F0bWVsLWhsY2RjL2F0bWVsX2hsY2RjX291dHB1 dC5jICAgfCAgNDAgKysrLQo+ID4gPiA+ICBkcml2ZXJzL2dwdS9kcm0vZHJtX29mLmMgICAgICAg ICAgICAgICAgICAgICAgICAgICB8ICAzOCArKysrCj4gPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9p MmMvdGRhOTk4eF9kcnYuYyAgICAgICAgICAgICAgICAgIHwgMjAxICsrKysrKysrKystLQo+ID4g PiA+ICBpbmNsdWRlL2RybS9kcm1fb2YuaCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8 ICAgNyArCj4gPiA+ID4gIDggZmlsZXMgY2hhbmdlZCwgMzQyIGluc2VydGlvbnMoKyksIDUxIGRl bGV0aW9ucygtKQoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgoKCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxp c3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNr dG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752722AbeDUITx (ORCPT ); Sat, 21 Apr 2018 04:19:53 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:59592 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751364AbeDUITu (ORCPT ); Sat, 21 Apr 2018 04:19:50 -0400 From: Laurent Pinchart To: jacopo mondi Cc: Peter Rosin , linux-kernel@vger.kernel.org, David Airlie , Rob Herring , Mark Rutland , Nicolas Ferre , Alexandre Belloni , Boris Brezillon , Daniel Vetter , Gustavo Padovan , Sean Paul , Russell King , Jacopo Mondi , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc Date: Sat, 21 Apr 2018 11:20:00 +0300 Message-ID: <1980444.sF26are89T@avalon> Organization: Ideas on Board Oy In-Reply-To: <20180420112204.GK4235@w540> References: <20180419162751.25223-1-peda@axentia.se> <6354350.l8DgUyNhLT@avalon> <20180420112204.GK4235@w540> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jacopo, On Friday, 20 April 2018 14:22:04 EEST jacopo mondi wrote: > On Fri, Apr 20, 2018 at 01:18:13PM +0300, Laurent Pinchart wrote: > > On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > > > Hi Peter, > > > > > > I've been a bit a pain in the arse for you recently, but please > > > bear with me a bit more, and sorry for jumping late on the band wagon. > > > > > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > > > Hi! > > > > > > > > I naively thought that since there was support for both nxp,tda19988 > > > > (in the tda998x driver) and the atmel-hlcdc, things would be a smooth > > > > ride. But it wasn't, so I started looking around and realized I had to > > > > fix things. > > > > > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > > > component, but now in v3 I fix things by making the tda998x driver > > > > a bridge instead. This was after a suggestion from Boris Brezillion > > > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > > > after comparing what was needed, I too find the bridge approach > > > > better. > > > > > > > > In addition to the above, our PCB interface between the SAMA5D3 and > > > > the HDMI encoder is only using 16 bits, and this has to be described > > > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > > > correct output mode. Since I have similar problems with a ds90c185 > > > > lvds encoder I added patches to override the atmel-hlcdc output format > > > > via DT properties compatible with the media video-interface binding > > > > and things start to play together. > > > > > > > > Since this series superseeds the bridge series [1], I have included > > > > the leftover bindings patch for the ti,ds90c185 here. > > > > > > I feel like this series would look better if it would make use of the > > > proposed bridge media bus format support I have recently sent out [1] > > > (and which was not there when you first sent v1). > > > > > > I understand your fundamental problem here is that the bus format > > > that should be reported by your bridge is different from the ones > > > actually supported by the TDA19988 chip, as the wirings ground some > > > of the input pins. > > > > > > Although this is defintely something that could be described in the > > > bridge's own OF node with the 'bus_width' property, and what I would do, > > > now that you have made a bridge out from the tda19988 driver, is: > > > > > > 1) Set the bridge accepted input bus_format parsing its pin > > > configuration, or default it if that's not implemented yet. > > > This will likely be rgb888. You can do that using the trivial > > > support for bridge input image formats implemented by my series. > > > 2) Specify in the bridge endpoint a 'bus_width' of <16> > > > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > > > and parse the remote endpoint property 'bus_width' and get the <16> > > > value back. > > > > Parsing properties of remote nodes should be avoided as much as possible, > > as they need to be interpreted in the context of the DT bindings related > > to the compatible string applicable to that node. I'd rather have the > > bus_width property in the local endpoint node. > > Right, I see... > Well, in Peter's case, the bus width, being a consequence of > wirings, can be described safely in both endpoints, right? If we look at it from the endpoints' point of view, the display controller has to output a 16-bit format, and the HDMI encoder receives a 24-bit format. The conversion is due to PCB wiring so there's no DT node to describe that. I thus believe the right description to be bus-width = <16> on the display controller side, and bus-width = <24> on the HDMI encoder side. This being said, even if the bus-width property was set to 16 on both sides, what I frown upon is parsing a property of the HDMI encoder DT node in the display controller driver. > > > 4) Set the correct format in the atmel hlcd driver to accommodate a > > > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > > > or are there other possible combinations I am missing?) > > > > > > I would consider this better mostly because in this series you are > > > creating a semantic for the whole DRM subsystem on the 'bus_width' > > > property, where numerical values as '12', '16' etc are arbitrary tied > > > to the selection of a media bus format. At least you should use a > > > common set of defines [1] between the device tree and the driver, > > > but this looks anyway fragile imho. > > > > This I agree with though. Combining the remote bus format with the local > > bus width should fix the problem without having to parse remote > > properties. > > > > > Have I maybe missed some parts of the problem you are trying to solve > > > here? > > > > > > [1] drm: bridge: Add support for static image formats > > > > > > https://lwn.net/Articles/752296/ > > > > > > [2] include/uapi/linux/media-bus-format.h > > > > > > > Anyway, this series solves some real issues for my HW. > > > > > > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > > > > - patch 2/7 fixed spelling and added an example > > > > - patch 4/7 parse the DT up front and store the result indexed by > > > > encoder > > > > - old patch 5/6 and 6/6 dropped > > > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > > > > > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > > > > - added reviewed-by from Rob to patch 1/6 > > > > - patch 2/6 changed so that the bus format override is in the endpoint > > > > > > > > DT node, and follows the binding of media video-interfaces. > > > > > > > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > > > > > > > > media video-interface binding (partially). > > > > > > > > - patch 4/6 now makes use of the above helper (and also fixes problems > > > > > > > > with the 3/5 patch from v1 when no override was specified). > > > > > > > > - do not mention unrelated connector display_info details in the cover > > > > > > > > letter and commit messages. > > > > > > > > [1] > > > > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 > > > > "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221 > > > > > > > > Peter Rosin (7): > > > > dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > > > > dt-bindings: display: atmel: optional video-interface of endpoints > > > > drm: of: introduce drm_of_media_bus_fmt > > > > drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes > > > > drm/i2c: tda998x: find the drm_device via the drm_connector > > > > drm/i2c: tda998x: split encoder and component functions from the > > > > work > > > > drm/i2c: tda998x: register as a drm bridge > > > > > > > > .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++ > > > > .../bindings/display/bridge/lvds-transmitter.txt | 8 +- > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 71 ++++++-- > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 + > > > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 40 +++- > > > > drivers/gpu/drm/drm_of.c | 38 ++++ > > > > drivers/gpu/drm/i2c/tda998x_drv.c | 201 ++++++++++-- > > > > include/drm/drm_of.h | 7 + > > > > 8 files changed, 342 insertions(+), 51 deletions(-) -- Regards, Laurent Pinchart