| Message ID | 20230123072358.1060670-3-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: Add i.MX93 LCDIF support | expand |
Hi, On Mon, 23 Jan 2023 15:23:58 +0800 Liu Ying wrote: > The LCDIF embedded in i.MX93 SoC is essentially the same to those > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI > controller through LCDIF cross line pattern(controlled by mediamix > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a > parallel display(also through mediamix blk-ctrl), so add multiple > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the > LCDIF DRM driver and find a bridge to attach the relevant encoder's > chain when needed. While at it, derive lcdif_crtc_state structure > from drm_crtc_state structure to introduce bus_format and bus_flags > states so that the next downstream bridges may use consistent bus > format and bus flags. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > drivers/gpu/drm/mxsfb/lcdif_drv.c | 73 +++++++++-- > drivers/gpu/drm/mxsfb/lcdif_drv.h | 6 +- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 206 ++++++++++++++++++++---------- > 3 files changed, 208 insertions(+), 77 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c > index cc2ceb301b96..4d41f6b6eb14 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,21 +41,70 @@ 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"); > - > - lcdif->bridge = bridge; > + 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 (!remote || !of_device_is_available(remote)) { '!remote ||' is redundant, since of_device_is_available already checks for a NULL pointer. [...] > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c > index 262bc43b1079..ba36447ed900 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c [...] > @@ -529,6 +580,46 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc *crtc, > pm_runtime_put_sync(drm->dev); > } > > +static void lcdif_crtc_reset(struct drm_crtc *crtc) > +{ > + struct lcdif_crtc_state *state; > + > + if (crtc->state) > + __drm_atomic_helper_crtc_destroy_state(crtc->state); > + > + kfree(to_lcdif_crtc_state(crtc->state)); > If crtc-state can be NULL at this point, this will only work as long as 'base' is the first member of the lcdif_crtc_state struct (which currently is the case, but there is no guarantee that this will always be this way), otherwise the if clause above is not needed. Lothar Waßmann
On 1/23/23 08:23, Liu Ying wrote: > The LCDIF embedded in i.MX93 SoC is essentially the same to those > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI > controller through LCDIF cross line pattern(controlled by mediamix > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a > parallel display(also through mediamix blk-ctrl), so add multiple > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the > LCDIF DRM driver and find a bridge to attach the relevant encoder's > chain when needed. While at it, derive lcdif_crtc_state structure > from drm_crtc_state structure to introduce bus_format and bus_flags > states so that the next downstream bridges may use consistent bus > format and bus flags. Would it be possible to split this patch into preparatory clean up and i.MX93 addition ? It seems like the patch is doing two things according to the commit message.
On Mon, 2023-01-23 at 09:13 +0100, Lothar Waßmann wrote: > Hi, Hi, > > On Mon, 23 Jan 2023 15:23:58 +0800 Liu Ying wrote: > > The LCDIF embedded in i.MX93 SoC is essentially the same to those > > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI > > controller through LCDIF cross line pattern(controlled by mediamix > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a > > parallel display(also through mediamix blk-ctrl), so add multiple > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the > > LCDIF DRM driver and find a bridge to attach the relevant encoder's > > chain when needed. While at it, derive lcdif_crtc_state structure > > from drm_crtc_state structure to introduce bus_format and bus_flags > > states so that the next downstream bridges may use consistent bus > > format and bus flags. > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > drivers/gpu/drm/mxsfb/lcdif_drv.c | 73 +++++++++-- > > drivers/gpu/drm/mxsfb/lcdif_drv.h | 6 +- > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 206 ++++++++++++++++++++---- > > ------ > > 3 files changed, 208 insertions(+), 77 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c > > b/drivers/gpu/drm/mxsfb/lcdif_drv.c > > index cc2ceb301b96..4d41f6b6eb14 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,21 +41,70 @@ 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"); > > - > > - lcdif->bridge = bridge; > > + 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 (!remote || !of_device_is_available(remote)) { > > '!remote ||' is redundant, since of_device_is_available already > checks > for a NULL pointer. of_device_is_available() does check for a NULL pointer when it calls __of_device_is_available() (after it takes devtree_lock raw spinlock and then releases devtree_lock). So, if remote is NULL, '!remote ||' exits the if clause a bit earlier without calling of_device_is_available() to take/release devtree_lock, which means a little bit better performance. But, people may say that the performance gain is trivial. drm_of_component_probe() in drm_of.c also checks '!remote' before checking '!of_device_is_available(remote)'. Do you still think that both drm_of_component_probe() and lcdif_attach_bridge() should drop '!remote ||'? > > [...] > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > index 262bc43b1079..ba36447ed900 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > [...] > > @@ -529,6 +580,46 @@ static void lcdif_crtc_atomic_disable(struct > > drm_crtc *crtc, > > pm_runtime_put_sync(drm->dev); > > } > > > > +static void lcdif_crtc_reset(struct drm_crtc *crtc) > > +{ > > + struct lcdif_crtc_state *state; > > + > > + if (crtc->state) > > + __drm_atomic_helper_crtc_destroy_state(crtc->state); > > + > > + kfree(to_lcdif_crtc_state(crtc->state)); > > > > If crtc-state can be NULL at this point, this will only work as long > as > 'base' is the first member of the lcdif_crtc_state struct (which > currently is the case, but there is no guarantee that this will > always > be this way), otherwise the if clause above is not needed. crtc->state can be NULL when e.g. at driver load time, like kerneldoc for drm_atomic_helper_crtc_reset() mentions. I may add a comment for the 'base' member to note that it should be always the first member of the lcdif_crtc_state structure. Do you think that will be ok? Thanks, Liu Ying > > > > Lothar Waßmann
On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote: > On 1/23/23 08:23, Liu Ying wrote: > > The LCDIF embedded in i.MX93 SoC is essentially the same to those > > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI > > controller through LCDIF cross line pattern(controlled by mediamix > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a > > parallel display(also through mediamix blk-ctrl), so add multiple > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the > > LCDIF DRM driver and find a bridge to attach the relevant encoder's > > chain when needed. While at it, derive lcdif_crtc_state structure > > from drm_crtc_state structure to introduce bus_format and bus_flags > > states so that the next downstream bridges may use consistent bus > > format and bus flags. > > Would it be possible to split this patch into preparatory clean up > and > i.MX93 addition ? It seems like the patch is doing two things > according > to the commit message. IMHO, all the patch does is for i.MX93 addition, not for clean up. Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI DSI/LVDS/parallel related bridges to drive triple displays _simultaneously_ in theory, while the three LCDIF instances embedded in i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one LCDIF maps to one display). The multiple encoders addition and the new checks for consistent bus format and bus flags are only for i.MX93 LCDIF, not for i.MX8mp LCDIF. Also, I think the multiple encoders addition and the new checks should be done together - if the new checks come first, then the new checks do not make sense(no multiple displays driven by LCDIF); if the new checks come later, then it would be a bug to allow inconsistent bus format and bus flags across the next downstream bridges when only adding multiple encoders support(also, I don't know which encoder's bridge should determine the LCDIF output bus format and bus flags, since the three encoders come together with the three next bridges). Regards, Liu Ying
Hi, Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying: > On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote: > > On 1/23/23 08:23, Liu Ying wrote: > > > The LCDIF embedded in i.MX93 SoC is essentially the same to those > > > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI > > > controller through LCDIF cross line pattern(controlled by mediamix > > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a > > > parallel display(also through mediamix blk-ctrl), so add multiple > > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the > > > LCDIF DRM driver and find a bridge to attach the relevant encoder's > > > chain when needed. While at it, derive lcdif_crtc_state structure > > > from drm_crtc_state structure to introduce bus_format and bus_flags > > > states so that the next downstream bridges may use consistent bus > > > format and bus flags. > > > > Would it be possible to split this patch into preparatory clean up > > and > > i.MX93 addition ? It seems like the patch is doing two things > > according > > to the commit message. > > IMHO, all the patch does is for i.MX93 addition, not for clean up. > Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI > DSI/LVDS/parallel related bridges to drive triple displays > _simultaneously_ in theory, while the three LCDIF instances embedded in > i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one > LCDIF maps to one display). The multiple encoders addition and the new > checks for consistent bus format and bus flags are only for i.MX93 > LCDIF, not for i.MX8mp LCDIF. Also, I think the multiple encoders > addition and the new checks should be done together - if the new checks > come first, then the new checks do not make sense(no multiple displays > driven by LCDIF); You are right on this one, but on the other hand there are lot of preparing patches already. Even if it is useless by itself, having the bus format & flag checks in a separate patch, it is easier to review, IMHO. > if the new checks come later, then it would be a bug > to allow inconsistent bus format and bus flags across the next > downstream bridges when only adding multiple encoders support(also, I > don't know which encoder's bridge should determine the LCDIF output bus > format and bus flags, since the three encoders come together with the > three next bridges). Agreed, this order is a no-go. Best regards, Alexander > Regards, > Liu Ying
On Tue, 2023-01-24 at 12:15 +0100, Alexander Stein wrote: > Hi, Hi, > > Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying: > > On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote: > > > On 1/23/23 08:23, Liu Ying wrote: > > > > The LCDIF embedded in i.MX93 SoC is essentially the same to > > > > those > > > > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI > > > > DSI > > > > controller through LCDIF cross line pattern(controlled by > > > > mediamix > > > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or > > > > a > > > > parallel display(also through mediamix blk-ctrl), so add > > > > multiple > > > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in > > > > the > > > > LCDIF DRM driver and find a bridge to attach the relevant > > > > encoder's > > > > chain when needed. While at it, derive lcdif_crtc_state > > > > structure > > > > from drm_crtc_state structure to introduce bus_format and > > > > bus_flags > > > > states so that the next downstream bridges may use consistent > > > > bus > > > > format and bus flags. > > > > > > Would it be possible to split this patch into preparatory clean > > > up > > > and > > > i.MX93 addition ? It seems like the patch is doing two things > > > according > > > to the commit message. > > > > IMHO, all the patch does is for i.MX93 addition, not for clean up. > > Note that the single LCDIF embedded in i.MX93 SoC may connect with > > MIPI > > DSI/LVDS/parallel related bridges to drive triple displays > > _simultaneously_ in theory, while the three LCDIF instances > > embedded in > > i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays > > respectively(one > > LCDIF maps to one display). The multiple encoders addition and the > > new > > checks for consistent bus format and bus flags are only for i.MX93 > > LCDIF, not for i.MX8mp LCDIF. Also, I think the multiple encoders > > addition and the new checks should be done together - if the new > > checks > > come first, then the new checks do not make sense(no multiple > > displays > > driven by LCDIF); > > You are right on this one, but on the other hand there are lot of > preparing > patches already. Even if it is useless by itself, having the bus > format & flag > checks in a separate patch, it is easier to review, IMHO. TBH, this way of separating patch looks too artificial. It's odd to check consistent bus format and bus flags for multiple bridges while there is only one encoder. What I can do is to make a separate patch to introduce the lcdif_crtc_state structure(with bus_format and bus_flags members) and determine the format and flags in ->atomic_check() instead of ->atomic_enable(). And then, add i.MX93 LCDIF support with an another patch which adds multiple encoders+bridges and new checks for consistent bus format and bus flags. Sounds good? Regards, Liu Ying > > > if the new checks come later, then it would be a bug > > to allow inconsistent bus format and bus flags across the next > > downstream bridges when only adding multiple encoders support(also, > > I > > don't know which encoder's bridge should determine the LCDIF output > > bus > > format and bus flags, since the three encoders come together with > > the > > three next bridges). > > Agreed, this order is a no-go. > > Best regards, > Alexander > > > Regards, > > Liu Ying > > > >
On 1/24/23 12:15, Alexander Stein wrote: > Hi, Hi, > Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying: >> On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote: >>> On 1/23/23 08:23, Liu Ying wrote: >>>> The LCDIF embedded in i.MX93 SoC is essentially the same to those >>>> in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI >>>> controller through LCDIF cross line pattern(controlled by mediamix >>>> blk-ctrl) or connect with LVDS display bridge(LDB) directly or a >>>> parallel display(also through mediamix blk-ctrl), so add multiple >>>> encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the >>>> LCDIF DRM driver and find a bridge to attach the relevant encoder's >>>> chain when needed. While at it, derive lcdif_crtc_state structure >>>> from drm_crtc_state structure to introduce bus_format and bus_flags >>>> states so that the next downstream bridges may use consistent bus >>>> format and bus flags. >>> >>> Would it be possible to split this patch into preparatory clean up >>> and >>> i.MX93 addition ? It seems like the patch is doing two things >>> according >>> to the commit message. >> >> IMHO, all the patch does is for i.MX93 addition, not for clean up. >> Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI >> DSI/LVDS/parallel related bridges to drive triple displays >> _simultaneously_ in theory, while the three LCDIF instances embedded in >> i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one >> LCDIF maps to one display). The multiple encoders addition and the new >> checks for consistent bus format and bus flags are only for i.MX93 >> LCDIF, not for i.MX8mp LCDIF. Also, I think the multiple encoders >> addition and the new checks should be done together - if the new checks >> come first, then the new checks do not make sense(no multiple displays >> driven by LCDIF); > > You are right on this one, but on the other hand there are lot of preparing > patches already. Even if it is useless by itself, having the bus format & flag > checks in a separate patch, it is easier to review, IMHO. I agree on the ease of review. [...]
diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c index cc2ceb301b96..4d41f6b6eb14 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,21 +41,70 @@ 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"); - - lcdif->bridge = bridge; + 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 (!remote || !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); + 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); + } + + lcdif->bridges[of_ep.id] = bridge; + } return 0; } @@ -199,6 +251,7 @@ static const struct drm_driver lcdif_driver = { static const struct of_device_id lcdif_dt_ids[] = { { .compatible = "fsl,imx8mp-lcdif" }, + { .compatible = "fsl,imx93-lcdif" }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, lcdif_dt_ids); diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h b/drivers/gpu/drm/mxsfb/lcdif_drv.h index 6cdba6e20c02..64459c3904e8 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,8 +32,8 @@ struct lcdif_drm_private { /* i.MXRT does support overlay planes, add them here. */ } planes; struct drm_crtc crtc; - struct drm_encoder encoder; - struct drm_bridge *bridge; + struct drm_encoder encoders[MAX_DISPLAYS]; + struct drm_bridge *bridges[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 262bc43b1079..ba36447ed900 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c @@ -17,6 +17,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_color_mgmt.h> +#include <drm/drm_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_encoder.h> #include <drm/drm_fb_dma_helper.h> @@ -30,6 +31,18 @@ #include "lcdif_drv.h" #include "lcdif_regs.h" +struct lcdif_crtc_state { + struct drm_crtc_state base; + u32 bus_format; + u32 bus_flags; +}; + +static inline struct lcdif_crtc_state * +to_lcdif_crtc_state(struct drm_crtc_state *s) +{ + return container_of(s, struct lcdif_crtc_state, base); +} + /* ----------------------------------------------------------------------------- * CRTC */ @@ -385,48 +398,109 @@ static void lcdif_reset_block(struct lcdif_drm_private *lcdif) readl(lcdif->base + LCDC_V8_CTRL); } -static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif, - struct drm_plane_state *plane_state, - struct drm_bridge_state *bridge_state, - const u32 bus_format) +static void lcdif_crtc_mode_set_nofb(struct drm_crtc_state *crtc_state, + struct drm_plane_state *plane_state) { - struct drm_device *drm = lcdif->crtc.dev; - struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode; - u32 bus_flags = 0; - - if (lcdif->bridge && lcdif->bridge->timings) - bus_flags = lcdif->bridge->timings->input_bus_flags; - else if (bridge_state) - bus_flags = bridge_state->input_bus_cfg.flags; + struct lcdif_crtc_state *lcdif_crtc_state = to_lcdif_crtc_state(crtc_state); + struct drm_device *drm = crtc_state->crtc->dev; + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm); + struct drm_display_mode *m = &crtc_state->adjusted_mode; DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)\n", m->crtc_clock, (int)(clk_get_rate(lcdif->clk) / 1000)); DRM_DEV_DEBUG_DRIVER(drm->dev, "Bridge bus_flags: 0x%08X\n", - bus_flags); + lcdif_crtc_state->bus_flags); DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags); /* Mandatory eLCDIF reset as per the Reference Manual */ lcdif_reset_block(lcdif); - lcdif_set_formats(lcdif, plane_state, bus_format); + lcdif_set_formats(lcdif, plane_state, lcdif_crtc_state->bus_format); - lcdif_set_mode(lcdif, bus_flags); + lcdif_set_mode(lcdif, lcdif_crtc_state->bus_flags); } static int lcdif_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) { + struct drm_device *drm = crtc->dev; struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + struct lcdif_crtc_state *lcdif_crtc_state = to_lcdif_crtc_state(crtc_state); bool has_primary = crtc_state->plane_mask & drm_plane_mask(crtc->primary); + struct drm_connector_state *connector_state; + struct drm_connector *connector; + struct drm_encoder *encoder; + struct drm_bridge_state *bridge_state; + struct drm_bridge *bridge; + u32 bus_format, bus_flags; + bool format_set = false, flags_set = false; + int ret, i; /* The primary plane has to be enabled when the CRTC is active. */ if (crtc_state->active && !has_primary) return -EINVAL; - return drm_atomic_add_affected_planes(state, crtc); + ret = drm_atomic_add_affected_planes(state, crtc); + if (ret) + return ret; + + /* Try to find consistent bus format and flags across first bridges. */ + for_each_new_connector_in_state(state, connector, connector_state, i) { + if (!connector_state->crtc) + continue; + + encoder = connector_state->best_encoder; + + bridge = drm_bridge_chain_get_first_bridge(encoder); + if (!bridge) + continue; + + /* + * Use bridge's input bus format if possible, otherwise use + * MEDIA_BUS_FMT_FIXED. + */ + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); + if (!bridge_state) + bus_format = MEDIA_BUS_FMT_FIXED; + else + bus_format = bridge_state->input_bus_cfg.format; + + if (bus_format == MEDIA_BUS_FMT_FIXED) { + dev_warn(drm->dev, + "[ENCODER:%d:%s]'s bridge does not provide bus format, assuming MEDIA_BUS_FMT_RGB888_1X24.\n" + "Please fix bridge driver by handling atomic_get_input_bus_fmts.\n", + encoder->base.id, encoder->name); + bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } + + if (!format_set) { + lcdif_crtc_state->bus_format = bus_format; + format_set = true; + } else if (lcdif_crtc_state->bus_format != bus_format) { + DRM_DEV_DEBUG_DRIVER(drm->dev, "inconsistent bus format\n"); + return -EINVAL; + } + + if (bridge->timings) + bus_flags = bridge->timings->input_bus_flags; + else if (bridge_state) + bus_flags = bridge_state->input_bus_cfg.flags; + else + bus_flags = 0; + + if (!flags_set) { + lcdif_crtc_state->bus_flags = bus_flags; + flags_set = true; + } else if (lcdif_crtc_state->bus_flags != bus_flags) { + DRM_DEV_DEBUG_DRIVER(drm->dev, "inconsistent bus flags\n"); + return -EINVAL; + } + } + + return 0; } static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc, @@ -458,44 +532,21 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state) { struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev); - struct drm_plane_state *new_pstate = drm_atomic_get_new_plane_state(state, - crtc->primary); + struct drm_crtc_state *new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, + crtc->primary); struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode; - struct drm_bridge_state *bridge_state = NULL; struct drm_device *drm = lcdif->drm; - u32 bus_format = 0; dma_addr_t paddr; - /* If there is a bridge attached to the LCDIF, use its bus format */ - if (lcdif->bridge) { - bridge_state = - drm_atomic_get_new_bridge_state(state, - lcdif->bridge); - if (!bridge_state) - bus_format = MEDIA_BUS_FMT_FIXED; - else - bus_format = bridge_state->input_bus_cfg.format; - - if (bus_format == MEDIA_BUS_FMT_FIXED) { - dev_warn_once(drm->dev, - "Bridge does not provide bus format, assuming MEDIA_BUS_FMT_RGB888_1X24.\n" - "Please fix bridge driver by handling atomic_get_input_bus_fmts.\n"); - bus_format = MEDIA_BUS_FMT_RGB888_1X24; - } - } - - /* If all else fails, default to RGB888_1X24 */ - if (!bus_format) - bus_format = MEDIA_BUS_FMT_RGB888_1X24; - clk_set_rate(lcdif->clk, m->crtc_clock * 1000); pm_runtime_get_sync(drm->dev); - lcdif_crtc_mode_set_nofb(lcdif, new_pstate, bridge_state, bus_format); + lcdif_crtc_mode_set_nofb(new_crtc_state, new_plane_state); /* Write cur_buf as well to avoid an initial corrupt frame */ - paddr = drm_fb_dma_get_gem_addr(new_pstate->fb, new_pstate, 0); + paddr = drm_fb_dma_get_gem_addr(new_plane_state->fb, new_plane_state, 0); if (paddr) { writel(lower_32_bits(paddr), lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4); @@ -529,6 +580,46 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc *crtc, pm_runtime_put_sync(drm->dev); } +static void lcdif_crtc_reset(struct drm_crtc *crtc) +{ + struct lcdif_crtc_state *state; + + if (crtc->state) + __drm_atomic_helper_crtc_destroy_state(crtc->state); + + kfree(to_lcdif_crtc_state(crtc->state)); + crtc->state = NULL; + + state = kzalloc(sizeof(*state), GFP_KERNEL); + if (state) + __drm_atomic_helper_crtc_reset(crtc, &state->base); +} + +static struct drm_crtc_state * +lcdif_crtc_atomic_duplicate_state(struct drm_crtc *crtc) +{ + struct lcdif_crtc_state *old = to_lcdif_crtc_state(crtc->state); + struct lcdif_crtc_state *new; + + new = kzalloc(sizeof(*new), GFP_KERNEL); + if (!new) + return NULL; + + __drm_atomic_helper_crtc_duplicate_state(crtc, &new->base); + + new->bus_format = old->bus_format; + new->bus_flags = old->bus_flags; + + return &new->base; +} + +static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + __drm_atomic_helper_crtc_destroy_state(state); + kfree(to_lcdif_crtc_state(state)); +} + static int lcdif_crtc_enable_vblank(struct drm_crtc *crtc) { struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev); @@ -557,24 +648,16 @@ static const struct drm_crtc_helper_funcs lcdif_crtc_helper_funcs = { }; static const struct drm_crtc_funcs lcdif_crtc_funcs = { - .reset = drm_atomic_helper_crtc_reset, + .reset = lcdif_crtc_reset, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, - .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, + .atomic_duplicate_state = lcdif_crtc_atomic_duplicate_state, + .atomic_destroy_state = lcdif_crtc_atomic_destroy_state, .enable_vblank = lcdif_crtc_enable_vblank, .disable_vblank = lcdif_crtc_disable_vblank, }; -/* ----------------------------------------------------------------------------- - * Encoder - */ - -static const struct drm_encoder_funcs lcdif_encoder_funcs = { - .destroy = drm_encoder_cleanup, -}; - /* ----------------------------------------------------------------------------- * Planes */ @@ -667,7 +750,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; @@ -691,13 +773,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); }
The LCDIF embedded in i.MX93 SoC is essentially the same to those in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI controller through LCDIF cross line pattern(controlled by mediamix blk-ctrl) or connect with LVDS display bridge(LDB) directly or a parallel display(also through mediamix blk-ctrl), so add multiple encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the LCDIF DRM driver and find a bridge to attach the relevant encoder's chain when needed. While at it, derive lcdif_crtc_state structure from drm_crtc_state structure to introduce bus_format and bus_flags states so that the next downstream bridges may use consistent bus format and bus flags. Signed-off-by: Liu Ying <victor.liu@nxp.com> --- drivers/gpu/drm/mxsfb/lcdif_drv.c | 73 +++++++++-- drivers/gpu/drm/mxsfb/lcdif_drv.h | 6 +- drivers/gpu/drm/mxsfb/lcdif_kms.c | 206 ++++++++++++++++++++---------- 3 files changed, 208 insertions(+), 77 deletions(-)