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@xxxxxxx>
> > ---
> > 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
