Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support

[Liu Ying <victor.liu@nxp.com>]

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