[PATCH v2 1/2] drm/fsl-dcu: Add HDMI driver for freescale DCU

[Meng Yi]

Hi Alexander,

Thanks for your comments!

On Tuesday, June 14, 2016 5:54 PM, Alexander wrote:

> > +static struct
> > +device_node *detect_hdmi_connection(struct fsl_dcu_drm_device
> > +*fsl_dev) {
> > +	struct device_node *remote_port;
> > +	struct of_endpoint *ep;
> > +	struct device_node *ep_node;
> > +	int ret;
> > +	struct device_node *parent = fsl_dev->dev->of_node;
> > +
> > +	ep = devm_kzalloc(fsl_dev->dev,
> > +				sizeof(struct of_endpoint), GFP_KERNEL);
> > +	if (!ep)
> > +		return NULL;
> > +
> > +	ep_node = devm_kzalloc(fsl_dev->dev,
> > +				sizeof(struct device_node), GFP_KERNEL);
> > +	if (!ep_node)
> > +		return NULL;
> > +
> > +	ep_node = of_graph_get_next_endpoint(parent, NULL);
>      ^^^^^^^
> You overwrite the pointer to previously allocated memory.
> 

Woops! I will fix that.

> > +	if (!ep_node)
> > +		goto error;
> > +
> > +	ret = of_graph_parse_endpoint(ep_node, ep);
> > +	if (ret) {
> > +		of_node_put(ep_node);
> > +		goto error;
> > +	}
> > +
> > +	remote_port = of_graph_get_remote_port_parent(ep->local_node);
> > +	if (!remote_port)
> > +		goto error;
> > +
> > +	return remote_port;
> > +error:
> > +	devm_kfree(ep);
> > +	devm_kfree(ep_node);
> > +	return NULL;
> > +}
> 
> So, unless you hit the error label the memory allocated using devm_kzalloc
> stays around until the device is removed. I don't know DRM internals much, but
> can load (within drm_driver) be called multiple times during device lifetime?

I think load called once every drm_device registered.

> This would allocate memory each time it is called. Why not allocate 'ep' on stack
> while ep_node don't need allocation at all, no?

Yep, allocate 'ep' on stack is better, and error label can be removed. 

> 
> > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c index c564ec6..658bc92
> > 100644
> > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c
> > @@ -17,10 +17,18 @@
> >  #include "fsl_dcu_drm_crtc.h"
> >  #include "fsl_dcu_drm_drv.h"
> >
> > +static void fsl_dcu_drm_output_poll_changed(struct drm_device *dev) {
> > +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> > +
> > +	drm_fbdev_cma_hotplug_event(fsl_dev->fbdev);
> > +}
> > +
> >  static const struct drm_mode_config_funcs
> > fsl_dcu_drm_mode_config_funcs = { .atomic_check =
> drm_atomic_helper_check,
> >  	.atomic_commit = drm_atomic_helper_commit,
> >  	.fb_create = drm_fb_cma_create,
> > +	.output_poll_changed = fsl_dcu_drm_output_poll_changed,
> >  };
> >
> >  int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device *fsl_dev) @@
> > -47,6 +55,14 @@ int fsl_dcu_drm_modeset_init(struct fsl_dcu_drm_device
> > *fsl_dev) if (ret)
> >  		goto fail_connector;
> >
> > +	ret = fsl_dcu_drm_hdmienc_create(fsl_dev, &fsl_dev->crtc);
> > +	if (ret)
> > +		DRM_ERROR("Failed to create HDMI encoder\n");
> > +
> > +	ret = fsl_dcu_drm_hdmienc_attach_bridge(fsl_dev);
> > +	if (ret)
> > +		DRM_ERROR("Failed to attach HDMI bridge\n");
> > +
> 
> Are those really errors? What about setups without any HDMI bridge at all? I

If device-tree is given, it just print error message, and HDMI don't display, that all. RGB-LCD
can display normally.

If device-tree is not given, attach bridge will return when detecting HDMI connection.
I think this should also be checked within " hdmienc_create ", or encoder will be allocated anyway.
Will fix that next version.

> would conside it an error if a bridge is given in device-tree but detecting or
> whatsoever fails for some reason.
> 

Yep, I just think that two function's return value should be handled, so put the
error message here. Maybe return value check should be dropped. What do you think?

Best Regards,
Meng