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

[Alexander Stein]

On Tuesday 14 June 2016 17:20:36, Meng Yi wrote:
> This patch creates another Encoder for HDMI port, and linking the Encoder
> to appropriate DRM bridge. And this Encoder using same CRTC with RGB-LCD.
> For RGB-LCD and HDMI using the same hardware connection to DCU, RGB-LCD
> panel should be unplugged when using the HDMI connection.
> 
> Signed-off-by: Alison Wang <alison.wang at nxp.com>
> Signed-off-by: Xiubo Li <lixiubo at cmss.chinamobile.com>
> Signed-off-by: Jianwei Wang <jianwei.wang.chn at gmail.com>
> Signed-off-by: Meng Yi <meng.yi at nxp.com>
> ---
> Changes in V2:
> -remove unused headers inclusion
> -remove module declarations
> -fix error handling coding style
> -drop moulde parameters and auto detect HDMI connection relying on deviece
> tree -modified comment lines
> ---
>  drivers/gpu/drm/fsl-dcu/Makefile             |   1 +
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c   | 189
> +++++++++++++++++++++++++++ drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c    | 
> 16 +++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h |   4 +
>  4 files changed, 210 insertions(+)
>  create mode 100644 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> [...]
> +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.

> +	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? 
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?

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

Best regards,
Alexander