Hi,
This patch set aims to add i.MX93 LCDIF display controller support
in the existing LCDIF DRM driver. The LCDIF embedded in i.MX93 SoC
is essentially the same to those embedded in i.MX8mp SoC. Through
internal bridges, i.MX93 LCDIF may drive a MIPI DSI display or a LVDS
display or a parallel display.
Patch 1/6 adds device tree binding support for i.MX93 LCDIF in the
existing fsl,lcdif.yaml.
Patch 2/6 drops lcdif->bridge NULL pointer check as a cleanup patch.
Patch 3/6~5/6 prepare for adding i.MX93 LCDIF support step by step.
Patch 6/6 adds i.MX93 LCDIF compatible string as the last step of
adding i.MX93 LCDIF support.
v4->v5:
* Rebase this patch set upon v6.4-rc1 and resolve a trivial conflict for
patch 5/6.
* Collect Alexander's R-b tag on patch 3/6, A-b tags on patch 4/6 and 5/6,
T-b tags on patch 2/6~6/6.
v3->v4:
* Improve warning message when ignoring invalid LCDIF OF endpoint ids in
patch 5/6. (Alexander)
* Use 'new_{c,p}state' instead of 'new_{crtc,plane}_state' in patch 3/6.
(Alexander)
* Simplify lcdif_crtc_reset() by calling lcdif_crtc_atomic_destroy_state()
in patch 3/6. (Alexander)
* Add '!crtc->state' check in lcdif_crtc_atomic_duplicate_state() in patch 3/6.
(Alexander)
* Collect Alexander's R-b tags on patch 1/6, 2/6 and 6/6.
v2->v3:
* Fix a trivial typo in patch 6/6's commit message.
v1->v2:
* Add Krzysztof's A-b and Marek's R-b tags on patch 1/6.
* Split patch 2/2 in v1 into patch 2/6~6/6 in v2. (Marek, Alexander)
* Drop '!remote ||' from lcdif_attach_bridge(). (Lothar)
* Add comment on the 'base' member of lcdif_crtc_state structure to
note it should always be the first member. (Lothar)
* Drop unneeded 'bridges' member from lcdif_drm_private structure.
* Drop a comment about bridge input bus format from lcdif_crtc_atomic_check().
Liu Ying (6):
dt-bindings: lcdif: Add i.MX93 LCDIF support
drm: lcdif: Drop unnecessary NULL pointer check on lcdif->bridge
drm: lcdif: Determine bus format and flags in ->atomic_check()
drm: lcdif: Check consistent bus format and flags across first bridges
drm: lcdif: Add multiple encoders and first bridges support
drm: lcdif: Add i.MX93 LCDIF compatible string
.../bindings/display/fsl,lcdif.yaml | 7 +-
drivers/gpu/drm/mxsfb/lcdif_drv.c | 71 ++++++-
drivers/gpu/drm/mxsfb/lcdif_drv.h | 5 +-
drivers/gpu/drm/mxsfb/lcdif_kms.c | 198 ++++++++++++------
4 files changed, 206 insertions(+), 75 deletions(-)
--
2.37.1
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.
Tested-by: Alexander Stein <[email protected]>
Acked-by: Alexander Stein <[email protected]>
Signed-off-by: Liu Ying <[email protected]>
---
v4->v5:
* Rebase upon v6.4-rc1 and resolve a trivial conflict.
* Add Alexander's A-b and T-b tags.
v3->v4:
* Improve warning message when ignoring invalid LCDIF OF endpoint ids.
(Alexander)
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 e816f87828fb..cf27b63b1899 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_dma.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);
+ 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, "ingoring 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 = &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);
+ }
+ }
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 <drm/drm_encoder.h>
#include <drm/drm_plane.h>
+#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 d6009b353a16..c35d769f91dd 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -656,14 +656,6 @@ static const struct drm_crtc_funcs lcdif_crtc_funcs = {
.disable_vblank = lcdif_crtc_disable_vblank,
};
-/* -----------------------------------------------------------------------------
- * Encoder
- */
-
-static const struct drm_encoder_funcs lcdif_encoder_funcs = {
- .destroy = drm_encoder_cleanup,
-};
-
/* -----------------------------------------------------------------------------
* Planes
*/
@@ -756,7 +748,6 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
BIT(DRM_COLOR_YCBCR_BT2020);
const u32 supported_ranges = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
BIT(DRM_COLOR_YCBCR_FULL_RANGE);
- struct drm_encoder *encoder = &lcdif->encoder;
struct drm_crtc *crtc = &lcdif->crtc;
int ret;
@@ -780,13 +771,7 @@ int lcdif_kms_init(struct lcdif_drm_private *lcdif)
return ret;
drm_crtc_helper_add(crtc, &lcdif_crtc_helper_funcs);
- ret = drm_crtc_init_with_planes(lcdif->drm, crtc,
- &lcdif->planes.primary, NULL,
- &lcdif_crtc_funcs, NULL);
- if (ret)
- return ret;
-
- encoder->possible_crtcs = drm_crtc_mask(crtc);
- return drm_encoder_init(lcdif->drm, encoder, &lcdif_encoder_funcs,
- DRM_MODE_ENCODER_NONE, NULL);
+ return drm_crtc_init_with_planes(lcdif->drm, crtc,
+ &lcdif->planes.primary, NULL,
+ &lcdif_crtc_funcs, NULL);
}
--
2.37.1
On 5/8/23 07:57, Liu Ying wrote:
> 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.
>
> Tested-by: Alexander Stein <[email protected]>
> Acked-by: Alexander Stein <[email protected]>
> Signed-off-by: Liu Ying <[email protected]>
> ---
> v4->v5:
> * Rebase upon v6.4-rc1 and resolve a trivial conflict.
> * Add Alexander's A-b and T-b tags.
>
> v3->v4:
> * Improve warning message when ignoring invalid LCDIF OF endpoint ids.
> (Alexander)
>
> 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 e816f87828fb..cf27b63b1899 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_dma.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);
> + 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) {
Can we make the maximum number of displays, or really bridge, specific
to IP instance instead (1 for mx8mp, 3 for mx93) ? If so, then I think
we need to track a list of bridges in some linked list or some such
dynamic structure, which would allow us to get rid of MAX_DISPLAYS macro.
> + dev_warn(dev, "ingoring invalid endpoint id %u\n", of_ep.id);
s@ingoring@ignoring@
[...]
On Tue, May 9, 2023 at 10:14 AM Marek Vasut <[email protected]> wrote:
>
> On 5/8/23 07:57, Liu Ying wrote:
> > 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.
> >
> > Tested-by: Alexander Stein <[email protected]>
> > Acked-by: Alexander Stein <[email protected]>
> > Signed-off-by: Liu Ying <[email protected]>
> > ---
> > v4->v5:
> > * Rebase upon v6.4-rc1 and resolve a trivial conflict.
> > * Add Alexander's A-b and T-b tags.
> >
> > v3->v4:
> > * Improve warning message when ignoring invalid LCDIF OF endpoint ids.
> > (Alexander)
> >
> > 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 e816f87828fb..cf27b63b1899 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_dma.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);
> > + 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) {
>
> Can we make the maximum number of displays, or really bridge, specific
> to IP instance instead (1 for mx8mp, 3 for mx93) ? If so, then I think
> we need to track a list of bridges in some linked list or some such
> dynamic structure, which would allow us to get rid of MAX_DISPLAYS macro.
I don't think we need to track bridges, since bridges can be found
through connector's encoder, like lcdif_crtc_atomic_check() does
in patch 4/6. Instead, MAX_DISPLAYS and the encoder member
in struct lcdif_drm_private can be dropped if we call devm_kzalloc()
to allocate encoders in this function.
>
> > + dev_warn(dev, "ingoring invalid endpoint id %u\n", of_ep.id);
>
> s@ingoring@ignoring@
Right. But the endpoint id check is not mandatory and is better
to be dropped together with MAX_DISPLAYS. I mean the driver
doesn't have to validate the endpoint id set in device tree.
Regards,
Liu Ying
>
> [...]