by Alexander Stein

[permalink] [raw]

Subject: Re: [PATCH v3 1/6] dt-bindings: lcdif: Add i.MX93 LCDIF support

Hi Liu,

Am Mittwoch, 15. Februar 2023, 08:49:56 CET schrieb Liu Ying:
> On Wed, 2023-02-15 at 08:26 +0100, Alexander Stein wrote:
> > Hi Liu,
>
> Hi Alexander,
>
> > thanks for the update.
>
> Thanks for the review.
>
> > Am Montag, 13. Februar 2023, 09:56:07 CET schrieb Liu Ying:
> > > There is one LCDIF embedded in i.MX93 SoC to connect with
> > > MIPI DSI controller through LCDIF cross line pattern(controlled
> > > by mediamix blk-ctrl) or connect with LVDS display bridge(LDB)
> > > directly or connect with a parallel display through parallel
> > > display format(also controlled by mediamix blk-ctrl). i.MX93
> > > LCDIF IP is essentially the same to i.MX8MP LCDIF IP. Add device
> > > tree binding for i.MX93 LCDIF.
> > >
> > > Acked-by: Krzysztof Kozlowski <[email protected]>
> > > Reviewed-by: Marek Vasut <[email protected]>
> > > Signed-off-by: Liu Ying <[email protected]>
> > > ---
> > > v2->v3:
> > > * No change.
> > >
> > > v1->v2:
> > > * Add Krzysztof's A-b and Marek's R-b tags on patch 1/6.
> > >
> > > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 7
> > >
> > > ++++++-
> > >
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml index
> > > 75b4efd70ba8..fc11ab5fc465 100644
> > > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > >
> > > @@ -21,6 +21,7 @@ properties:
> > > - fsl,imx28-lcdif
> > > - fsl,imx6sx-lcdif
> > > - fsl,imx8mp-lcdif
> > >
> > > + - fsl,imx93-lcdif
> > >
> > > - items:
> > > - enum:
> > > - fsl,imx6sl-lcdif
> > >
> > > @@ -88,7 +89,9 @@ allOf:
> > > properties:
> > > compatible:
> > > contains:
> > > - const: fsl,imx8mp-lcdif
> > > + enum:
> > > + - fsl,imx8mp-lcdif
> > > + - fsl,imx93-lcdif
> > >
> > > then:
> > > properties:
> > > clocks:
> > > @@ -107,6 +110,7 @@ allOf:
> > > enum:
> > > - fsl,imx6sx-lcdif
> > > - fsl,imx8mp-lcdif
> > >
> > > + - fsl,imx93-lcdif
> > >
> > > then:
> > > properties:
> > > clocks:
> > > @@ -123,6 +127,7 @@ allOf:
> > > - fsl,imx8mm-lcdif
> > > - fsl,imx8mn-lcdif
> > > - fsl,imx8mp-lcdif
> > >
> > > + - fsl,imx93-lcdif
> > >
> > > then:
> > > required:
> > > - power-domains
> >
> > I would have expected that fsl,imx93-lcdif supports up to 3 endpoints
> > (MIPI
> > DSI, LVDS, and parallel) in a 'ports' subnode. But this binding only
> > supports
> > a single 'port' sub-node. Also an example for this case might be very
> > helpful.
>
> The port node allows multiple endpoints(See graph.yaml[1]). It's
> enough to use the existing port node instead of using ports node.

Ah, I wasn't aware of that possibility for OF graph. Yep, agreed then it's
enough.

> For i.MX93 LCDIF, the port node will be something like this:
> --------------------8<--------------------------
> port {
> #address-cells = <1>;
> #size-cells = <0>;
>
> lcdif_to_pdfc: endpoint@0 {
> reg = <0>;
> };
>
> lcdif_to_ldb: endpoint@1 {
> reg = <1>;
> };
>
> lcdif_to_cross_line_pattern: endpoint@2 {
> reg = <2>;
> };
> };
> --------------------8<--------------------------
>
> Looks like it's not necessary to add a specifc example for i.MX93
> LCDIF.

Fine by me.
Reviewed-by: Alexander Stein <[email protected]>

> [1]
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph
> .yaml#L48
>
> Regards,
> Liu Ying

TQ-Systems GmbH | M?hlstra?e 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M?nchen, HRB 105018
Gesch?ftsf?hrer: Detlef Schneider, R?diger Stahl, Stefan Schneider

2023-02-15 08:53:29

by Liu Ying

[permalink] [raw]

Subject: Re: [PATCH v3 5/6] drm: lcdif: Add multiple encoders and first bridges support

On Wed, 2023-02-15 at 08:54 +0100, Alexander Stein wrote:
> Hi Liu,

Hi Alexander,

>
> thanks for the update.

Thanks for the review.

>
> Am Montag, 13. Februar 2023, 09:56:11 CET schrieb Liu Ying:
> > The single LCDIF embedded in i.MX93 SoC may drive multiple displays
> > simultaneously. Look at LCDIF output port's remote port parents to
> > find all enabled first bridges. Add an encoder for each found
> > bridge
> > and attach the bridge to the encoder. This is a preparation for
> > adding i.MX93 LCDIF support.
> >
> > Signed-off-by: Liu Ying <[email protected]>
> > ---
> > v2->v3:
> > * No change.
> >
> > v1->v2:
> > * Split from patch 2/2 in v1. (Marek, Alexander)
> > * Drop '!remote ||' from lcdif_attach_bridge(). (Lothar)
> > * Drop unneeded 'bridges' member from lcdif_drm_private structure.
> >
> > drivers/gpu/drm/mxsfb/lcdif_drv.c | 68
> > +++++++++++++++++++++++++++----
> > drivers/gpu/drm/mxsfb/lcdif_drv.h | 4 +-
> > drivers/gpu/drm/mxsfb/lcdif_kms.c | 21 ++--------
> > 3 files changed, 66 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > b/drivers/gpu/drm/mxsfb/lcdif_drv.c index
> > b5b9a8e273c6..eb6c265fa2fe 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> > @@ -9,13 +9,16 @@
> > #include <linux/dma-mapping.h>
> > #include <linux/io.h>
> > #include <linux/module.h>
> > +#include <linux/of.h>
> > #include <linux/of_device.h>
> > +#include <linux/of_graph.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> >
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_bridge.h>
> > #include <drm/drm_drv.h>
> > +#include <drm/drm_encoder.h>
> > #include <drm/drm_fbdev_generic.h>
> > #include <drm/drm_gem_dma_helper.h>
> > #include <drm/drm_gem_framebuffer_helper.h>
> > @@ -38,19 +41,68 @@ static const struct
> > drm_mode_config_helper_funcs
> > lcdif_mode_config_helpers = { .atomic_commit_tail =
> > drm_atomic_helper_commit_tail_rpm,
> > };
> >
> > +static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> > + .destroy = drm_encoder_cleanup,
> > +};
> > +
> > static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
> > {
> > - struct drm_device *drm = lcdif->drm;
> > + struct device *dev = lcdif->drm->dev;
> > + struct device_node *ep;
> > struct drm_bridge *bridge;
> > int ret;
> >
> > - bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0,
> > 0);
> > - if (IS_ERR(bridge))
> > - return PTR_ERR(bridge);
> > -
> > - ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
> > - if (ret)
> > - return dev_err_probe(drm->dev, ret, "Failed to attach
>
> bridge\n");
> > + for_each_endpoint_of_node(dev->of_node, ep) {
> > + struct device_node *remote;
> > + struct of_endpoint of_ep;
> > + struct drm_encoder *encoder;
> > +
> > + remote = of_graph_get_remote_port_parent(ep);
>
> Is it possible for remote to be NULL?

Yes. But, no worries. Lothar said the check for '!remote' is not
needed in v1 comment because of_device_is_available() checks that.

>
> > + if (!of_device_is_available(remote)) {
> > + of_node_put(remote);
> > + continue;
> > + }
> > + of_node_put(remote);
> > +
> > + ret = of_graph_parse_endpoint(ep, &of_ep);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to parse endpoint
> > %pOF\n",
>
> ep);
> > + of_node_put(ep);
> > + return ret;
> > + }
> > +
> > + if (of_ep.id >= MAX_DISPLAYS) {
> > + dev_warn(dev, "invalid endpoint id %u\n",
>
> of_ep.id);
>
> I would write
> dev_warn(dev, "ignoring invalid endpoint id %u\n", of_ep.id);
> just because the parsing continues but this one is skipped.

Ok, will do that in next version.

>
> > + continue;
> > + }
> > +
> > + bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0,
>
> of_ep.id);
> > + if (IS_ERR(bridge)) {
> > + of_node_put(ep);
> > + return dev_err_probe(dev, PTR_ERR(bridge),
> > + "Failed to get bridge
>
> for endpoint%u\n",
> > + of_ep.id);
> > + }
> > +
> > + encoder = &lcdif->encoders[of_ep.id];
> > + encoder->possible_crtcs = drm_crtc_mask(&lcdif->crtc);
> > + ret = drm_encoder_init(lcdif->drm, encoder,
>
> &lcdif_encoder_funcs,
> > + DRM_MODE_ENCODER_NONE, NULL);
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize encoder for
>
> endpoint%u: %d\n",
> > + of_ep.id, ret);
> > + of_node_put(ep);
> > + return ret;
> > + }
> > +
> > + ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> > + if (ret) {
> > + of_node_put(ep);
> > + return dev_err_probe(dev, ret,
> > + "Failed to attach
>
> bridge for endpoint%u\n",
> > + of_ep.id);
> > + }
>
> Admittedly I'm not used to the drm API, but do we need to some manual
> cleanup/
> revert if some endpoints is e.g. deferred, but previous endpoints
> already have
> been successfully added? e.g. endpoint 0 is added, but adding
> endpoint 1
> fails.

I think the bailout path is safe, because
drm_mode_config_init_release() is called as the managed release action
added through drm_mode_config_init(). drm_mode_config_cleanup() would
clean things up.

Regards,
Liu Ying