From mboxrd@z Thu Jan 1 00:00:00 1970 From: moinejf@free.fr (Jean-Francois Moine) Date: Tue, 25 Mar 2014 16:55:48 +0100 Subject: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector In-Reply-To: <1458827.cQ6aDWdh1W@avalon> References: <1458827.cQ6aDWdh1W@avalon> Message-ID: <20140325165548.0065b639@armhf> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 24 Mar 2014 23:39:01 +0100 Laurent Pinchart wrote: > Hi Jean-Fran?ois, Hi Laurent, > Thank you for the patch. > > On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote: > > The 'slave encoder' structure of the tda998x driver asks for glue > > between the DRM driver and the encoder/connector structures. > > > > This patch changes the driver to a normal DRM encoder/connector > > thanks to the infrastructure for componentised subsystems. > > I like the idea, but I'm not really happy with the implementation. Let me try > to explain why below. > > > Signed-off-by: Jean-Francois Moine > > --- > > drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++---------------- > > 1 file changed, 188 insertions(+), 135 deletions(-) > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644 > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > > [snip] > > > @@ -44,10 +45,14 @@ struct tda998x_priv { > > > > wait_queue_head_t wq_edid; > > volatile int wq_edid_wait; > > - struct drm_encoder *encoder; > > + struct drm_encoder encoder; > > + struct drm_connector connector; > > }; > > [snip] > > > -static int > > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) > > +static int tda_bind(struct device *dev, struct device *master, void *data) > > { > > + struct drm_device *drm = data; > > This is the part that bothers me. You're making two assumptions here, that the > DRM driver will pass a struct drm_device pointer to the bind operation, and > that the I2C encoder driver can take control of DRM encoder and connector > creation. Although it could become problematic later, the first assumption > isn't too much of an issue for now. I'll thus focus on the second one. > > The component framework isolate the encoder and DRM master drivers as far as > component creation and binding is concerned, but doesn't provide a way for the > two drivers to communicate together (nor should it). You're solving this by > passing a pointer to the DRM device to the encoder bind operation, making the > encoder driver create a DRM encoder and connector, and relying on the DRM core > to orchestrate CRTCs, encoders and connectors. You thus assume that the > encoder hardware should be represented by a DRM encoder object, and that its > output is connected to a connector that should be represented by a DRM > connector object. While this can work in your use case, that won't always hold > true. Hardware encoders can be chained together, while DRM encoders can't. The > DRM core has recently received support for bridge objects to overcome that > limitation. Depending on the hardware topology, a given hardware encoder > should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't > be taken by the encoder driver but by the DRM master driver. The I2C encoder > driver thus shouldn't create the DRM encoder and DRM connector itself. > > I believe the encoder/master communication problem should be solved > differently. Instead of passing a pointer to the DRM device to the encoder > driver and making the encoder driver control DRM encoder and connector > creation, the encoder driver should instead create an object not visible to > userspace that can be retrieved by the DRM master driver (possibly through > registration with the DRM core, or by going through drvdata in the encoder's > struct device). The DRM master could use that object to communicate with the > encoder, and would register the DRM encoder and DRM connector itself based on > hardware topology. > > > + struct i2c_client *i2c_client = to_i2c_client(dev); > > + struct tda998x_priv *priv = i2c_get_clientdata(i2c_client); > > + struct drm_connector *connector = &priv->connector; > > + struct drm_encoder *encoder = &priv->encoder; > > + int ret; > > + > > + if (!try_module_get(THIS_MODULE)) { > > + dev_err(dev, "cannot get module %s\n", THIS_MODULE->name); > > + return -EINVAL; > > + } > > + > > + ret = drm_connector_init(drm, connector, > > + &connector_funcs, > > + DRM_MODE_CONNECTOR_HDMIA); > > This is one example of the shortcomings I've explained above. An encoder > driver can't always know what connector type it is connected to. If I'm not > mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be > up to the master driver to select the connector type based on its overall view > of the hardware, or even to a connector driver that would be bound to a > connector DT node (as proposed in https://www.mail-archive.com/devicetree at vger.kernel.org/msg16585.html). [snip] The tda998x, as a HDMI transmitter, has to deal with both video and audio. Whereas the hardware connection schemes are the same in both worlds, the way they are translated to computer objects are very different: - video DRM card -> CRTCs -> encoders -> (bridges) -> connectors - audio ALSA card -> CPUs -> (CODECs) -> CODECs and it would be nice to have a common layout. Actually, the tda998x is a slave encoder, that is, it plays the roles of both encoder and connector. In the 2 DRM drivers (armada and tilcdc) which use it, yes, the encoders and connectors are created by the main DRM drivers, but, there is no notion of bridge, and, also, the encoder is DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA. Then, nothing is changed in the global system working. About the connector, yes, I let its type as hard-coded, but this could be changed by configuration in the platform data or in the DT. Anyway, there is nothing as such in the proposed patch 'Add DT binding documentation for HDMI Connector' I also dislike this patch because it adds a device which is of no use. I had a same remark from Mark Brown about a tda998x CODEC proposal of mine: hdmi_codec: hdmi-codec { compatible = "nxp,tda998x-codec"; audio-ports = <0x03>, <0x04>; }; So, the next tda998x CODEC will be directly included in the tda998x driver, the audio output being the HDMI connector. Here is the DT definition I have for the Cubox: &i2c0 { hdmi: hdmi-encoder { compatible = "nxp,tda9989"; reg = <0x70>; interrupt-parent = <&gpio0>; interrupts = <27 IRQ_TYPE_EDGE_FALLING>; pinctrl-0 = <&pmx_camera>; pinctrl-names = "default"; audio-ports = <0x03>, <0x04>; /* 2 audio input ports */ audio-port-names = "i2s", "spdif"; #sound-dai-cells = <1>; port { /* 1 video input port */ hdmi_0: endpoint at 0 { remote-endpoint = <&lcd0_0>; }; }; }; }; Back to the DRM device pointer given to the tda998x driver, as the tda998x is an encoder/connector, it has no bridge function, so, there is no need to add a complex API for information exchange between both drivers. Anyway, there is a big lack in my proposal: the tda998x encoder is hard-coded to the first CRTC. This could be solved by a scan of the DT and of the encoder list by the DRM driver, but I think that the actual definitions as proposed by media/video-interfaces.txt are not easy to use. Do you think that a description as done for ALSA could work? A sound card creation is done by a global sound configuration and a description of the links between the card elements. Here is the tda998x part of the Cubox audio card ('audio1' is the audio device): sound { compatible = "simple-audio-card"; simple-audio-card,name = "Cubox Audio"; simple-audio-card,dai-link at 0 { /* I2S - HDMI */ format = "i2s"; cpu { sound-dai = <&audio1 0>; /* I2S output */ }; codec { sound-dai = <&hdmi 0>; /* I2S input */ }; }; simple-audio-card,dai-link at 1 { /* S/PDIF - HDMI */ cpu { sound-dai = <&audio1 1>; /* S/PDIF output */ }; codec { sound-dai = <&hdmi 1>; /* S/PDIF input */ }; }; }; Using the same elements, here is what could be the video card of the Armada 510 with a panel, the tda998x and the display controller: video { compatible = "simple-video-card"; simple-video-card,dvi-link { crtc { dvi = <&lcd0>; }; encoder { dvi = <&panel>; connector-type = 7; /* LVDS */ }; }; simple-video-card,dvi-link { crtc { dvi = <&lcd0>; }; encoder { dvi = <&hdmi>; connector-type = 11; /* HDMI-A */ }; }; }; lcd0: lcd-controller at 820000 { compatible = "marvell,armada-510-lcd"; ... hardware definitions ... }; hdmi : hdmi-encoder { .. same as above, but without the video input port .. }; panel: panel { .. panel parameters .. }; Then, the generic 'simple-video-card' has all elements to create the DRM device. -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Francois Moine Subject: Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector Date: Tue, 25 Mar 2014 16:55:48 +0100 Message-ID: <20140325165548.0065b639@armhf> References: <1458827.cQ6aDWdh1W@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from smtp3-g21.free.fr (smtp3-g21.free.fr [212.27.42.3]) by gabe.freedesktop.org (Postfix) with ESMTP id 788E06E0CB for ; Tue, 25 Mar 2014 08:55:21 -0700 (PDT) In-Reply-To: <1458827.cQ6aDWdh1W@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: Russell King , linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org T24gTW9uLCAyNCBNYXIgMjAxNCAyMzozOTowMSArMDEwMApMYXVyZW50IFBpbmNoYXJ0IDxsYXVy ZW50LnBpbmNoYXJ0QGlkZWFzb25ib2FyZC5jb20+IHdyb3RlOgoKPiBIaSBKZWFuLUZyYW7Dp29p cywKCkhpIExhdXJlbnQsCgo+IFRoYW5rIHlvdSBmb3IgdGhlIHBhdGNoLgo+IAo+IE9uIEZyaWRh eSAyMSBNYXJjaCAyMDE0IDA5OjE3OjMyIEplYW4tRnJhbmNvaXMgTW9pbmUgd3JvdGU6Cj4gPiBU aGUgJ3NsYXZlIGVuY29kZXInIHN0cnVjdHVyZSBvZiB0aGUgdGRhOTk4eCBkcml2ZXIgYXNrcyBm b3IgZ2x1ZQo+ID4gYmV0d2VlbiB0aGUgRFJNIGRyaXZlciBhbmQgdGhlIGVuY29kZXIvY29ubmVj dG9yIHN0cnVjdHVyZXMuCj4gPiAKPiA+IFRoaXMgcGF0Y2ggY2hhbmdlcyB0aGUgZHJpdmVyIHRv IGEgbm9ybWFsIERSTSBlbmNvZGVyL2Nvbm5lY3Rvcgo+ID4gdGhhbmtzIHRvIHRoZSBpbmZyYXN0 cnVjdHVyZSBmb3IgY29tcG9uZW50aXNlZCBzdWJzeXN0ZW1zLgo+IAo+IEkgbGlrZSB0aGUgaWRl YSwgYnV0IEknbSBub3QgcmVhbGx5IGhhcHB5IHdpdGggdGhlIGltcGxlbWVudGF0aW9uLiBMZXQg bWUgdHJ5IAo+IHRvIGV4cGxhaW4gd2h5IGJlbG93Lgo+IAo+ID4gU2lnbmVkLW9mZi1ieTogSmVh bi1GcmFuY29pcyBNb2luZSA8bW9pbmVqZkBmcmVlLmZyPgo+ID4gLS0tCj4gPiAgZHJpdmVycy9n cHUvZHJtL2kyYy90ZGE5OTh4X2Rydi5jIHwgMzIzICsrKysrKysrKysrKysrKysrKystLS0tLS0t LS0tLS0tLS0tCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDE4OCBpbnNlcnRpb25zKCspLCAxMzUgZGVs ZXRpb25zKC0pCj4gPiAKPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTJjL3RkYTk5 OHhfZHJ2LmMKPiA+IGIvZHJpdmVycy9ncHUvZHJtL2kyYy90ZGE5OTh4X2Rydi5jIGluZGV4IGZk Njc1MWMuLjFjMjVlNDAgMTAwNjQ0Cj4gPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTJjL3RkYTk5 OHhfZHJ2LmMKPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pMmMvdGRhOTk4eF9kcnYuYwo+IAo+ IFtzbmlwXQo+IAo+ID4gQEAgLTQ0LDEwICs0NSwxNCBAQCBzdHJ1Y3QgdGRhOTk4eF9wcml2IHsK PiA+IAo+ID4gIAl3YWl0X3F1ZXVlX2hlYWRfdCB3cV9lZGlkOwo+ID4gIAl2b2xhdGlsZSBpbnQg d3FfZWRpZF93YWl0Owo+ID4gLQlzdHJ1Y3QgZHJtX2VuY29kZXIgKmVuY29kZXI7Cj4gPiArCXN0 cnVjdCBkcm1fZW5jb2RlciBlbmNvZGVyOwo+ID4gKwlzdHJ1Y3QgZHJtX2Nvbm5lY3RvciBjb25u ZWN0b3I7Cj4gPiAgfTsKPiAKPiBbc25pcF0KPiAKPiA+IC1zdGF0aWMgaW50Cj4gPiAtdGRhOTk4 eF9wcm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50LCBjb25zdCBzdHJ1Y3QgaTJjX2Rldmlj ZV9pZCAqaWQpCj4gPiArc3RhdGljIGludCB0ZGFfYmluZChzdHJ1Y3QgZGV2aWNlICpkZXYsIHN0 cnVjdCBkZXZpY2UgKm1hc3Rlciwgdm9pZCAqZGF0YSkKPiA+ICB7Cj4gPiArCXN0cnVjdCBkcm1f ZGV2aWNlICpkcm0gPSBkYXRhOwo+IAo+IFRoaXMgaXMgdGhlIHBhcnQgdGhhdCBib3RoZXJzIG1l LiBZb3UncmUgbWFraW5nIHR3byBhc3N1bXB0aW9ucyBoZXJlLCB0aGF0IHRoZSAKPiBEUk0gZHJp dmVyIHdpbGwgcGFzcyBhIHN0cnVjdCBkcm1fZGV2aWNlIHBvaW50ZXIgdG8gdGhlIGJpbmQgb3Bl cmF0aW9uLCBhbmQgCj4gdGhhdCB0aGUgSTJDIGVuY29kZXIgZHJpdmVyIGNhbiB0YWtlIGNvbnRy b2wgb2YgRFJNIGVuY29kZXIgYW5kIGNvbm5lY3RvciAKPiBjcmVhdGlvbi4gQWx0aG91Z2ggaXQg Y291bGQgYmVjb21lIHByb2JsZW1hdGljIGxhdGVyLCB0aGUgZmlyc3QgYXNzdW1wdGlvbiAKPiBp c24ndCB0b28gbXVjaCBvZiBhbiBpc3N1ZSBmb3Igbm93LiBJJ2xsIHRodXMgZm9jdXMgb24gdGhl IHNlY29uZCBvbmUuCj4gCj4gVGhlIGNvbXBvbmVudCBmcmFtZXdvcmsgaXNvbGF0ZSB0aGUgZW5j b2RlciBhbmQgRFJNIG1hc3RlciBkcml2ZXJzIGFzIGZhciBhcyAKPiBjb21wb25lbnQgY3JlYXRp b24gYW5kIGJpbmRpbmcgaXMgY29uY2VybmVkLCBidXQgZG9lc24ndCBwcm92aWRlIGEgd2F5IGZv ciB0aGUgCj4gdHdvIGRyaXZlcnMgdG8gY29tbXVuaWNhdGUgdG9nZXRoZXIgKG5vciBzaG91bGQg aXQpLiBZb3UncmUgc29sdmluZyB0aGlzIGJ5IAo+IHBhc3NpbmcgYSBwb2ludGVyIHRvIHRoZSBE Uk0gZGV2aWNlIHRvIHRoZSBlbmNvZGVyIGJpbmQgb3BlcmF0aW9uLCBtYWtpbmcgdGhlIAo+IGVu Y29kZXIgZHJpdmVyIGNyZWF0ZSBhIERSTSBlbmNvZGVyIGFuZCBjb25uZWN0b3IsIGFuZCByZWx5 aW5nIG9uIHRoZSBEUk0gY29yZSAKPiB0byBvcmNoZXN0cmF0ZSBDUlRDcywgZW5jb2RlcnMgYW5k IGNvbm5lY3RvcnMuIFlvdSB0aHVzIGFzc3VtZSB0aGF0IHRoZSAKPiBlbmNvZGVyIGhhcmR3YXJl IHNob3VsZCBiZSByZXByZXNlbnRlZCBieSBhIERSTSBlbmNvZGVyIG9iamVjdCwgYW5kIHRoYXQg aXRzIAo+IG91dHB1dCBpcyBjb25uZWN0ZWQgdG8gYSBjb25uZWN0b3IgdGhhdCBzaG91bGQgYmUg cmVwcmVzZW50ZWQgYnkgYSBEUk0gCj4gY29ubmVjdG9yIG9iamVjdC4gV2hpbGUgdGhpcyBjYW4g d29yayBpbiB5b3VyIHVzZSBjYXNlLCB0aGF0IHdvbid0IGFsd2F5cyBob2xkIAo+IHRydWUuIEhh cmR3YXJlIGVuY29kZXJzIGNhbiBiZSBjaGFpbmVkIHRvZ2V0aGVyLCB3aGlsZSBEUk0gZW5jb2Rl cnMgY2FuJ3QuIFRoZSAKPiBEUk0gY29yZSBoYXMgcmVjZW50bHkgcmVjZWl2ZWQgc3VwcG9ydCBm b3IgYnJpZGdlIG9iamVjdHMgdG8gb3ZlcmNvbWUgdGhhdCAKPiBsaW1pdGF0aW9uLiBEZXBlbmRp bmcgb24gdGhlIGhhcmR3YXJlIHRvcG9sb2d5LCBhIGdpdmVuIGhhcmR3YXJlIGVuY29kZXIgCj4g c2hvdWxkIGJlIG1vZGVsZWQgYXMgYSBEUk0gZW5jb2RlciBvciBhcyBhIERSTSBicmlkZ2UuIFRo YXQgZGVjaXNpb24gc2hvdWxkbid0IAo+IGJlIHRha2VuIGJ5IHRoZSBlbmNvZGVyIGRyaXZlciBi dXQgYnkgdGhlIERSTSBtYXN0ZXIgZHJpdmVyLiBUaGUgSTJDIGVuY29kZXIgCj4gZHJpdmVyIHRo dXMgc2hvdWxkbid0IGNyZWF0ZSB0aGUgRFJNIGVuY29kZXIgYW5kIERSTSBjb25uZWN0b3IgaXRz ZWxmLgo+IAo+IEkgYmVsaWV2ZSB0aGUgZW5jb2Rlci9tYXN0ZXIgY29tbXVuaWNhdGlvbiBwcm9i bGVtIHNob3VsZCBiZSBzb2x2ZWQgCj4gZGlmZmVyZW50bHkuIEluc3RlYWQgb2YgcGFzc2luZyBh IHBvaW50ZXIgdG8gdGhlIERSTSBkZXZpY2UgdG8gdGhlIGVuY29kZXIgCj4gZHJpdmVyIGFuZCBt YWtpbmcgdGhlIGVuY29kZXIgZHJpdmVyIGNvbnRyb2wgRFJNIGVuY29kZXIgYW5kIGNvbm5lY3Rv ciAKPiBjcmVhdGlvbiwgdGhlIGVuY29kZXIgZHJpdmVyIHNob3VsZCBpbnN0ZWFkIGNyZWF0ZSBh biBvYmplY3Qgbm90IHZpc2libGUgdG8gCj4gdXNlcnNwYWNlIHRoYXQgY2FuIGJlIHJldHJpZXZl ZCBieSB0aGUgRFJNIG1hc3RlciBkcml2ZXIgKHBvc3NpYmx5IHRocm91Z2ggCj4gcmVnaXN0cmF0 aW9uIHdpdGggdGhlIERSTSBjb3JlLCBvciBieSBnb2luZyB0aHJvdWdoIGRydmRhdGEgaW4gdGhl IGVuY29kZXIncyAKPiBzdHJ1Y3QgZGV2aWNlKS4gVGhlIERSTSBtYXN0ZXIgY291bGQgdXNlIHRo YXQgb2JqZWN0IHRvIGNvbW11bmljYXRlIHdpdGggdGhlIAo+IGVuY29kZXIsIGFuZCB3b3VsZCBy ZWdpc3RlciB0aGUgRFJNIGVuY29kZXIgYW5kIERSTSBjb25uZWN0b3IgaXRzZWxmIGJhc2VkIG9u IAo+IGhhcmR3YXJlIHRvcG9sb2d5Lgo+IAo+ID4gKwlzdHJ1Y3QgaTJjX2NsaWVudCAqaTJjX2Ns aWVudCA9IHRvX2kyY19jbGllbnQoZGV2KTsKPiA+ICsJc3RydWN0IHRkYTk5OHhfcHJpdiAqcHJp diA9IGkyY19nZXRfY2xpZW50ZGF0YShpMmNfY2xpZW50KTsKPiA+ICsJc3RydWN0IGRybV9jb25u ZWN0b3IgKmNvbm5lY3RvciA9ICZwcml2LT5jb25uZWN0b3I7Cj4gPiArCXN0cnVjdCBkcm1fZW5j b2RlciAqZW5jb2RlciA9ICZwcml2LT5lbmNvZGVyOwo+ID4gKwlpbnQgcmV0Owo+ID4gKwo+ID4g KwlpZiAoIXRyeV9tb2R1bGVfZ2V0KFRISVNfTU9EVUxFKSkgewo+ID4gKwkJZGV2X2VycihkZXYs ICJjYW5ub3QgZ2V0IG1vZHVsZSAlc1xuIiwgVEhJU19NT0RVTEUtPm5hbWUpOwo+ID4gKwkJcmV0 dXJuIC1FSU5WQUw7Cj4gPiArCX0KPiA+ICsKPiA+ICsJcmV0ID0gZHJtX2Nvbm5lY3Rvcl9pbml0 KGRybSwgY29ubmVjdG9yLAo+ID4gKwkJCQkmY29ubmVjdG9yX2Z1bmNzLAo+ID4gKwkJCQlEUk1f TU9ERV9DT05ORUNUT1JfSERNSUEpOwo+IAo+IFRoaXMgaXMgb25lIGV4YW1wbGUgb2YgdGhlIHNo b3J0Y29taW5ncyBJJ3ZlIGV4cGxhaW5lZCBhYm92ZS4gQW4gZW5jb2RlciAKPiBkcml2ZXIgY2Fu J3QgYWx3YXlzIGtub3cgd2hhdCBjb25uZWN0b3IgdHlwZSBpdCBpcyBjb25uZWN0ZWQgdG8uIElm IEknbSBub3QgCj4gbWlzdGFrZW4gcG9zc2libGUgb3B0aW9ucyBoZXJlIGFyZSBEVklJLCBEVklE LCBIRE1JQSBhbmQgSERNSUIuIEl0IHNob3VsZCBiZSAKPiB1cCB0byB0aGUgbWFzdGVyIGRyaXZl ciB0byBzZWxlY3QgdGhlIGNvbm5lY3RvciB0eXBlIGJhc2VkIG9uIGl0cyBvdmVyYWxsIHZpZXcg Cj4gb2YgdGhlIGhhcmR3YXJlLCBvciBldmVuIHRvIGEgY29ubmVjdG9yIGRyaXZlciB0aGF0IHdv dWxkIGJlIGJvdW5kIHRvIGEgCj4gY29ubmVjdG9yIERUIG5vZGUgKGFzIHByb3Bvc2VkIGluIGh0 dHBzOi8vd3d3Lm1haWwtYXJjaGl2ZS5jb20vZGV2aWNldHJlZUB2Z2VyLmtlcm5lbC5vcmcvbXNn MTY1ODUuaHRtbCkuCglbc25pcF0KClRoZSB0ZGE5OTh4LCBhcyBhIEhETUkgdHJhbnNtaXR0ZXIs IGhhcyB0byBkZWFsIHdpdGggYm90aCB2aWRlbyBhbmQKYXVkaW8uCgpXaGVyZWFzIHRoZSBoYXJk d2FyZSBjb25uZWN0aW9uIHNjaGVtZXMgYXJlIHRoZSBzYW1lIGluIGJvdGggd29ybGRzLAp0aGUg d2F5IHRoZXkgYXJlIHRyYW5zbGF0ZWQgdG8gY29tcHV0ZXIgb2JqZWN0cyBhcmUgdmVyeSBkaWZm ZXJlbnQ6CgotIHZpZGVvCglEUk0gY2FyZCAtPiBDUlRDcyAtPiBlbmNvZGVycyAtPiAoYnJpZGdl cykgLT4gY29ubmVjdG9ycwoKLSBhdWRpbwoJQUxTQSBjYXJkIC0+IENQVXMgLT4gKENPREVDcykg LT4gQ09ERUNzCgphbmQgaXQgd291bGQgYmUgbmljZSB0byBoYXZlIGEgY29tbW9uIGxheW91dC4K CkFjdHVhbGx5LCB0aGUgdGRhOTk4eCBpcyBhIHNsYXZlIGVuY29kZXIsIHRoYXQgaXMsIGl0IHBs YXlzIHRoZSByb2xlcwpvZiBib3RoIGVuY29kZXIgYW5kIGNvbm5lY3Rvci4gSW4gdGhlIDIgRFJN IGRyaXZlcnMgKGFybWFkYSBhbmQgdGlsY2RjKQp3aGljaCB1c2UgaXQsIHllcywgdGhlIGVuY29k ZXJzIGFuZCBjb25uZWN0b3JzIGFyZSBjcmVhdGVkIGJ5IHRoZSBtYWluCkRSTSBkcml2ZXJzLCBi dXQsIHRoZXJlIGlzIG5vIG5vdGlvbiBvZiBicmlkZ2UsIGFuZCwgYWxzbywgdGhlIGVuY29kZXIK aXMgRFJNX01PREVfRU5DT0RFUl9UTURTIGFuZCB0aGUgY29ubmVjdG9yIGlzIERSTV9NT0RFX0NP Tk5FQ1RPUl9IRE1JQS4KVGhlbiwgbm90aGluZyBpcyBjaGFuZ2VkIGluIHRoZSBnbG9iYWwgc3lz dGVtIHdvcmtpbmcuCgpBYm91dCB0aGUgY29ubmVjdG9yLCB5ZXMsIEkgbGV0IGl0cyB0eXBlIGFz IGhhcmQtY29kZWQsIGJ1dCB0aGlzIGNvdWxkCmJlIGNoYW5nZWQgYnkgY29uZmlndXJhdGlvbiBp biB0aGUgcGxhdGZvcm0gZGF0YSBvciBpbiB0aGUgRFQuIEFueXdheSwKdGhlcmUgaXMgbm90aGlu ZyBhcyBzdWNoIGluIHRoZSBwcm9wb3NlZCBwYXRjaAoKCSdBZGQgRFQgYmluZGluZyBkb2N1bWVu dGF0aW9uIGZvciBIRE1JIENvbm5lY3RvcicKCkkgYWxzbyBkaXNsaWtlIHRoaXMgcGF0Y2ggYmVj YXVzZSBpdCBhZGRzIGEgZGV2aWNlIHdoaWNoIGlzIG9mIG5vIHVzZS4KSSBoYWQgYSBzYW1lIHJl bWFyayBmcm9tIE1hcmsgQnJvd24gYWJvdXQgYSB0ZGE5OTh4IENPREVDIHByb3Bvc2FsIG9mCm1p bmU6CgoJaGRtaV9jb2RlYzogaGRtaS1jb2RlYyB7CgkJY29tcGF0aWJsZSA9ICJueHAsdGRhOTk4 eC1jb2RlYyI7CgkJYXVkaW8tcG9ydHMgPSA8MHgwMz4sIDwweDA0PjsKCX07CgpTbywgdGhlIG5l eHQgdGRhOTk4eCBDT0RFQyB3aWxsIGJlIGRpcmVjdGx5IGluY2x1ZGVkIGluIHRoZSB0ZGE5OTh4 CmRyaXZlciwgdGhlIGF1ZGlvIG91dHB1dCBiZWluZyB0aGUgSERNSSBjb25uZWN0b3IuIEhlcmUg aXMgdGhlIERUCmRlZmluaXRpb24gSSBoYXZlIGZvciB0aGUgQ3Vib3g6CgomaTJjMCB7CgloZG1p OiBoZG1pLWVuY29kZXIgewoJCWNvbXBhdGlibGUgPSAibnhwLHRkYTk5ODkiOwoJCXJlZyA9IDww eDcwPjsKCQlpbnRlcnJ1cHQtcGFyZW50ID0gPCZncGlvMD47CgkJaW50ZXJydXB0cyA9IDwyNyBJ UlFfVFlQRV9FREdFX0ZBTExJTkc+OwoJCXBpbmN0cmwtMCA9IDwmcG14X2NhbWVyYT47CgkJcGlu Y3RybC1uYW1lcyA9ICJkZWZhdWx0IjsKCgkJYXVkaW8tcG9ydHMgPSA8MHgwMz4sIDwweDA0PjsJ CS8qIDIgYXVkaW8gaW5wdXQgcG9ydHMgKi8KCQlhdWRpby1wb3J0LW5hbWVzID0gImkycyIsICJz cGRpZiI7CgkJI3NvdW5kLWRhaS1jZWxscyA9IDwxPjsKCgkJcG9ydCB7CQkJCQkvKiAxIHZpZGVv IGlucHV0IHBvcnQgKi8KCQkJaGRtaV8wOiBlbmRwb2ludEAwIHsKCQkJCXJlbW90ZS1lbmRwb2lu dCA9IDwmbGNkMF8wPjsKCQkJfTsKCQl9OwoJfTsKfTsKCkJhY2sgdG8gdGhlIERSTSBkZXZpY2Ug cG9pbnRlciBnaXZlbiB0byB0aGUgdGRhOTk4eCBkcml2ZXIsIGFzIHRoZQp0ZGE5OTh4IGlzIGFu IGVuY29kZXIvY29ubmVjdG9yLCBpdCBoYXMgbm8gYnJpZGdlIGZ1bmN0aW9uLCBzbywgdGhlcmUK aXMgbm8gbmVlZCB0byBhZGQgYSBjb21wbGV4IEFQSSBmb3IgaW5mb3JtYXRpb24gZXhjaGFuZ2Ug YmV0d2VlbiBib3RoCmRyaXZlcnMuCgpBbnl3YXksIHRoZXJlIGlzIGEgYmlnIGxhY2sgaW4gbXkg cHJvcG9zYWw6IHRoZSB0ZGE5OTh4IGVuY29kZXIgaXMKaGFyZC1jb2RlZCB0byB0aGUgZmlyc3Qg Q1JUQy4gVGhpcyBjb3VsZCBiZSBzb2x2ZWQgYnkgYSBzY2FuIG9mIHRoZSBEVAphbmQgb2YgdGhl IGVuY29kZXIgbGlzdCBieSB0aGUgRFJNIGRyaXZlciwgYnV0IEkgdGhpbmsgdGhhdCB0aGUgYWN0 dWFsCmRlZmluaXRpb25zIGFzIHByb3Bvc2VkIGJ5IG1lZGlhL3ZpZGVvLWludGVyZmFjZXMudHh0 IGFyZSBub3QgZWFzeSB0bwp1c2UuCgpEbyB5b3UgdGhpbmsgdGhhdCBhIGRlc2NyaXB0aW9uIGFz IGRvbmUgZm9yIEFMU0EgY291bGQgd29yaz8KCkEgc291bmQgY2FyZCBjcmVhdGlvbiBpcyBkb25l IGJ5IGEgZ2xvYmFsIHNvdW5kIGNvbmZpZ3VyYXRpb24gYW5kIGEKZGVzY3JpcHRpb24gb2YgdGhl IGxpbmtzIGJldHdlZW4gdGhlIGNhcmQgZWxlbWVudHMuIEhlcmUgaXMgdGhlIHRkYTk5OHgKcGFy dCBvZiB0aGUgQ3Vib3ggYXVkaW8gY2FyZCAoJ2F1ZGlvMScgaXMgdGhlIGF1ZGlvIGRldmljZSk6 CgoJc291bmQgewoJCWNvbXBhdGlibGUgPSAic2ltcGxlLWF1ZGlvLWNhcmQiOwoJCXNpbXBsZS1h dWRpby1jYXJkLG5hbWUgPSAiQ3Vib3ggQXVkaW8iOwoKCQlzaW1wbGUtYXVkaW8tY2FyZCxkYWkt bGlua0AwIHsJCS8qIEkyUyAtIEhETUkgKi8KCQkJZm9ybWF0ID0gImkycyI7CgkJCWNwdSB7CgkJ CQlzb3VuZC1kYWkgPSA8JmF1ZGlvMSAwPjsJLyogSTJTIG91dHB1dCAqLwoJCQl9OwoJCQljb2Rl YyB7CgkJCQlzb3VuZC1kYWkgPSA8JmhkbWkgMD47CQkvKiBJMlMgaW5wdXQgKi8KCQkJfTsKCQl9 OwoKCQlzaW1wbGUtYXVkaW8tY2FyZCxkYWktbGlua0AxIHsJCS8qIFMvUERJRiAtIEhETUkgKi8K CQkJY3B1IHsKCQkJCXNvdW5kLWRhaSA9IDwmYXVkaW8xIDE+OwkvKiBTL1BESUYgb3V0cHV0ICov CgkJCX07CgkJCWNvZGVjIHsKCQkJCXNvdW5kLWRhaSA9IDwmaGRtaSAxPjsJCS8qIFMvUERJRiBp bnB1dCAqLwoJCQl9OwoJCX07Cgl9OwoKVXNpbmcgdGhlIHNhbWUgZWxlbWVudHMsIGhlcmUgaXMg d2hhdCBjb3VsZCBiZSB0aGUgdmlkZW8gY2FyZCBvZiB0aGUKQXJtYWRhIDUxMCB3aXRoIGEgcGFu ZWwsIHRoZSB0ZGE5OTh4IGFuZCB0aGUgZGlzcGxheSBjb250cm9sbGVyOgoKCXZpZGVvIHsKCQlj b21wYXRpYmxlID0gInNpbXBsZS12aWRlby1jYXJkIjsKCgkJc2ltcGxlLXZpZGVvLWNhcmQsZHZp LWxpbmsgewoJCQljcnRjIHsKCQkJCWR2aSA9IDwmbGNkMD47CgkJCX07CgkJCWVuY29kZXIgewoJ CQkJZHZpID0gPCZwYW5lbD47CgkJCQljb25uZWN0b3ItdHlwZSA9IDc7CS8qIExWRFMgKi8KCQkJ fTsKCQl9OwoJCXNpbXBsZS12aWRlby1jYXJkLGR2aS1saW5rIHsKCQkJY3J0YyB7CgkJCQlkdmkg PSA8JmxjZDA+OwoJCQl9OwoJCQllbmNvZGVyIHsKCQkJCWR2aSA9IDwmaGRtaT47CgkJCQljb25u ZWN0b3ItdHlwZSA9IDExOwkvKiBIRE1JLUEgKi8KCQkJfTsKCQl9OwoJfTsKCglsY2QwOiBsY2Qt Y29udHJvbGxlckA4MjAwMDAgewoJCWNvbXBhdGlibGUgPSAibWFydmVsbCxhcm1hZGEtNTEwLWxj ZCI7CgkJLi4uIGhhcmR3YXJlIGRlZmluaXRpb25zIC4uLgoJfTsKCgloZG1pIDogaGRtaS1lbmNv ZGVyIHsKCQkuLiBzYW1lIGFzIGFib3ZlLCBidXQgd2l0aG91dCB0aGUgdmlkZW8gaW5wdXQgcG9y dCAuLgoJfTsKCglwYW5lbDogcGFuZWwgewoJCS4uIHBhbmVsIHBhcmFtZXRlcnMgLi4KCX07CgpU aGVuLCB0aGUgZ2VuZXJpYyAnc2ltcGxlLXZpZGVvLWNhcmQnIGhhcyBhbGwgZWxlbWVudHMgdG8g Y3JlYXRlIHRoZQpEUk0gZGV2aWNlLgoKLS0gCktlbiBhciBjJ2hlbnRhw7EJfAkgICAgICAqKiBC cmVpemggaGEgTGludXggYXRhdiEgKioKSmVmCQl8CQlodHRwOi8vbW9pbmVqZi5mcmVlLmZyLwpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwg bWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp3-g21.free.fr ([212.27.42.3]:40727 "EHLO smtp3-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752203AbaCYPzX convert rfc822-to-8bit (ORCPT ); Tue, 25 Mar 2014 11:55:23 -0400 Date: Tue, 25 Mar 2014 16:55:48 +0100 From: Jean-Francois Moine To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org, Russell King , Rob Clark , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH RFC v2 2/6] drm/i2c: tda998x: Move tda998x to a couple encoder/connector Message-ID: <20140325165548.0065b639@armhf> In-Reply-To: <1458827.cQ6aDWdh1W@avalon> References: <1458827.cQ6aDWdh1W@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-media-owner@vger.kernel.org List-ID: On Mon, 24 Mar 2014 23:39:01 +0100 Laurent Pinchart wrote: > Hi Jean-François, Hi Laurent, > Thank you for the patch. > > On Friday 21 March 2014 09:17:32 Jean-Francois Moine wrote: > > The 'slave encoder' structure of the tda998x driver asks for glue > > between the DRM driver and the encoder/connector structures. > > > > This patch changes the driver to a normal DRM encoder/connector > > thanks to the infrastructure for componentised subsystems. > > I like the idea, but I'm not really happy with the implementation. Let me try > to explain why below. > > > Signed-off-by: Jean-Francois Moine > > --- > > drivers/gpu/drm/i2c/tda998x_drv.c | 323 +++++++++++++++++++---------------- > > 1 file changed, 188 insertions(+), 135 deletions(-) > > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > > b/drivers/gpu/drm/i2c/tda998x_drv.c index fd6751c..1c25e40 100644 > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > > [snip] > > > @@ -44,10 +45,14 @@ struct tda998x_priv { > > > > wait_queue_head_t wq_edid; > > volatile int wq_edid_wait; > > - struct drm_encoder *encoder; > > + struct drm_encoder encoder; > > + struct drm_connector connector; > > }; > > [snip] > > > -static int > > -tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) > > +static int tda_bind(struct device *dev, struct device *master, void *data) > > { > > + struct drm_device *drm = data; > > This is the part that bothers me. You're making two assumptions here, that the > DRM driver will pass a struct drm_device pointer to the bind operation, and > that the I2C encoder driver can take control of DRM encoder and connector > creation. Although it could become problematic later, the first assumption > isn't too much of an issue for now. I'll thus focus on the second one. > > The component framework isolate the encoder and DRM master drivers as far as > component creation and binding is concerned, but doesn't provide a way for the > two drivers to communicate together (nor should it). You're solving this by > passing a pointer to the DRM device to the encoder bind operation, making the > encoder driver create a DRM encoder and connector, and relying on the DRM core > to orchestrate CRTCs, encoders and connectors. You thus assume that the > encoder hardware should be represented by a DRM encoder object, and that its > output is connected to a connector that should be represented by a DRM > connector object. While this can work in your use case, that won't always hold > true. Hardware encoders can be chained together, while DRM encoders can't. The > DRM core has recently received support for bridge objects to overcome that > limitation. Depending on the hardware topology, a given hardware encoder > should be modeled as a DRM encoder or as a DRM bridge. That decision shouldn't > be taken by the encoder driver but by the DRM master driver. The I2C encoder > driver thus shouldn't create the DRM encoder and DRM connector itself. > > I believe the encoder/master communication problem should be solved > differently. Instead of passing a pointer to the DRM device to the encoder > driver and making the encoder driver control DRM encoder and connector > creation, the encoder driver should instead create an object not visible to > userspace that can be retrieved by the DRM master driver (possibly through > registration with the DRM core, or by going through drvdata in the encoder's > struct device). The DRM master could use that object to communicate with the > encoder, and would register the DRM encoder and DRM connector itself based on > hardware topology. > > > + struct i2c_client *i2c_client = to_i2c_client(dev); > > + struct tda998x_priv *priv = i2c_get_clientdata(i2c_client); > > + struct drm_connector *connector = &priv->connector; > > + struct drm_encoder *encoder = &priv->encoder; > > + int ret; > > + > > + if (!try_module_get(THIS_MODULE)) { > > + dev_err(dev, "cannot get module %s\n", THIS_MODULE->name); > > + return -EINVAL; > > + } > > + > > + ret = drm_connector_init(drm, connector, > > + &connector_funcs, > > + DRM_MODE_CONNECTOR_HDMIA); > > This is one example of the shortcomings I've explained above. An encoder > driver can't always know what connector type it is connected to. If I'm not > mistaken possible options here are DVII, DVID, HDMIA and HDMIB. It should be > up to the master driver to select the connector type based on its overall view > of the hardware, or even to a connector driver that would be bound to a > connector DT node (as proposed in https://www.mail-archive.com/devicetree@vger.kernel.org/msg16585.html). [snip] The tda998x, as a HDMI transmitter, has to deal with both video and audio. Whereas the hardware connection schemes are the same in both worlds, the way they are translated to computer objects are very different: - video DRM card -> CRTCs -> encoders -> (bridges) -> connectors - audio ALSA card -> CPUs -> (CODECs) -> CODECs and it would be nice to have a common layout. Actually, the tda998x is a slave encoder, that is, it plays the roles of both encoder and connector. In the 2 DRM drivers (armada and tilcdc) which use it, yes, the encoders and connectors are created by the main DRM drivers, but, there is no notion of bridge, and, also, the encoder is DRM_MODE_ENCODER_TMDS and the connector is DRM_MODE_CONNECTOR_HDMIA. Then, nothing is changed in the global system working. About the connector, yes, I let its type as hard-coded, but this could be changed by configuration in the platform data or in the DT. Anyway, there is nothing as such in the proposed patch 'Add DT binding documentation for HDMI Connector' I also dislike this patch because it adds a device which is of no use. I had a same remark from Mark Brown about a tda998x CODEC proposal of mine: hdmi_codec: hdmi-codec { compatible = "nxp,tda998x-codec"; audio-ports = <0x03>, <0x04>; }; So, the next tda998x CODEC will be directly included in the tda998x driver, the audio output being the HDMI connector. Here is the DT definition I have for the Cubox: &i2c0 { hdmi: hdmi-encoder { compatible = "nxp,tda9989"; reg = <0x70>; interrupt-parent = <&gpio0>; interrupts = <27 IRQ_TYPE_EDGE_FALLING>; pinctrl-0 = <&pmx_camera>; pinctrl-names = "default"; audio-ports = <0x03>, <0x04>; /* 2 audio input ports */ audio-port-names = "i2s", "spdif"; #sound-dai-cells = <1>; port { /* 1 video input port */ hdmi_0: endpoint@0 { remote-endpoint = <&lcd0_0>; }; }; }; }; Back to the DRM device pointer given to the tda998x driver, as the tda998x is an encoder/connector, it has no bridge function, so, there is no need to add a complex API for information exchange between both drivers. Anyway, there is a big lack in my proposal: the tda998x encoder is hard-coded to the first CRTC. This could be solved by a scan of the DT and of the encoder list by the DRM driver, but I think that the actual definitions as proposed by media/video-interfaces.txt are not easy to use. Do you think that a description as done for ALSA could work? A sound card creation is done by a global sound configuration and a description of the links between the card elements. Here is the tda998x part of the Cubox audio card ('audio1' is the audio device): sound { compatible = "simple-audio-card"; simple-audio-card,name = "Cubox Audio"; simple-audio-card,dai-link@0 { /* I2S - HDMI */ format = "i2s"; cpu { sound-dai = <&audio1 0>; /* I2S output */ }; codec { sound-dai = <&hdmi 0>; /* I2S input */ }; }; simple-audio-card,dai-link@1 { /* S/PDIF - HDMI */ cpu { sound-dai = <&audio1 1>; /* S/PDIF output */ }; codec { sound-dai = <&hdmi 1>; /* S/PDIF input */ }; }; }; Using the same elements, here is what could be the video card of the Armada 510 with a panel, the tda998x and the display controller: video { compatible = "simple-video-card"; simple-video-card,dvi-link { crtc { dvi = <&lcd0>; }; encoder { dvi = <&panel>; connector-type = 7; /* LVDS */ }; }; simple-video-card,dvi-link { crtc { dvi = <&lcd0>; }; encoder { dvi = <&hdmi>; connector-type = 11; /* HDMI-A */ }; }; }; lcd0: lcd-controller@820000 { compatible = "marvell,armada-510-lcd"; ... hardware definitions ... }; hdmi : hdmi-encoder { .. same as above, but without the video input port .. }; panel: panel { .. panel parameters .. }; Then, the generic 'simple-video-card' has all elements to create the DRM device. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/