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
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > +#include
> > #include
> > #include
> >
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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(>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, _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 = >encoders[of_ep.id];
> > + encoder->possible_crtcs = drm_crtc_mask(>crtc);
> > + ret = drm_encoder_init(lcdif->drm, encoder,
>
> _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
Re: [PATCH v3 5/6] drm: lcdif: Add multiple encoders and first bridges support
Hi Liu,
thanks for the update.
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
> ---
> 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
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
>
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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(>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?
> + if (!of_device_is_available(remote)) {
> + of_node_put(remote);
> + continue;
> + }
> + of_node_put(remote);
> +
> + ret = of_graph_parse_endpoint(ep, _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.
> + 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 = >encoders[of_ep.id];
> + encoder->possible_crtcs = drm_crtc_mask(>crtc);
> + ret = drm_encoder_init(lcdif->drm, encoder,
_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.
Best regards,
Alexander
> + }
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> b/drivers/gpu/drm/mxsfb/lcdif_drv.h index aa6d099a1897..c7400bd9bbd9 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> +++
[PATCH v3 5/6] drm: lcdif: Add multiple encoders and first bridges support
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
---
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
#include
#include
+#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -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(>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);
+ if (!of_device_is_available(remote)) {
+ of_node_put(remote);
+ continue;
+ }
+ of_node_put(remote);
+
+ ret = of_graph_parse_endpoint(ep, _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);
+ 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 = >encoders[of_ep.id];
+ encoder->possible_crtcs = drm_crtc_mask(>crtc);
+ ret = drm_encoder_init(lcdif->drm, encoder,
_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);
+ }
+ }
return 0;
}
diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h
b/drivers/gpu/drm/mxsfb/lcdif_drv.h
index aa6d099a1897..c7400bd9bbd9 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
+++ b/drivers/gpu/drm/mxsfb/lcdif_drv.h
@@ -14,6 +14,8 @@
#include
#include
+#define MAX_DISPLAYS 3
+
struct clk;
struct lcdif_drm_private {
@@ -30,7 +32,7 @@ struct lcdif_drm_private {
/* i.MXRT does support overlay planes, add them here. */
} planes;
struct drm_crtc crtc;
- struct drm_encoder encoder;
+ struct drm_encoder encoders[MAX_DISPLAYS];
};
static inline struct lcdif_drm_private *
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index 4ea3d2b2cf61..5f34d01e133e 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++
