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

From: Stefan Agner <stefan@agner.ch>
To: Meng Yi <meng.yi@nxp.com>
Cc: Jianwei Wang <jianwei.wang.chn@gmail.com>,
	Xiubo Li <lixiubo@cmss.chinamobile.com>,
	Alison Wang <alison.wang@nxp.com>,
	emil.l.velikov@gmail.com, dri-devel@lists.freedesktop.org,
	maxime.ripard@free-electrons.com,
	alexander.stein@systec-electronic.com
Subject: Re: [PATCH v2 1/2] drm/fsl-dcu: Add HDMI driver for freescale DCU
Date: Sat, 18 Jun 2016 20:00:59 -0700	[thread overview]
Message-ID: <75453188206f2fc756711cbd34c6dc85@agner.ch> (raw)
In-Reply-To: <1465896037-537-1-git-send-email-meng.yi@nxp.com>

On 2016-06-14 02:20, 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@nxp.com>
> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
> Signed-off-by: Jianwei Wang <jianwei.wang.chn@gmail.com>
> Signed-off-by: Meng Yi <meng.yi@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

Hi Meng,

I took a bit more time to understand what it is all about with this
patchset and the DRM bridge infrastructure in general.

I agree with Boris Brezillions comment of the first version of this
patchset: You "only" want to add DRM bridge support here, it doesn't has
anything to do with HDMI. So there should not be a file called
fsl_dcu_drm_hdmi.c for instance....

In fact, you should fold the DRM bridge code into fsl_dcu_drm_rgb.c.
Have a look at Boris patch which added such support to the Atmel HLDCD
driver:
https://lists.freedesktop.org/archives/dri-devel/2016-January/098050.html

However, the HLDCD driver already supported the endpoint syntax for
panels at that time, which is not the case for the DCU driver.

Panel support with endpoint DT syntax seems to be more generic and is a
easier first step towards DRM bridge support. Hence I suggest to take
this two steps:

1. Extend the current code to support panels through endpoint syntax
(of_graph..)
2. Extend the endpoint support to also support DRM bridges

During 1 you should be careful to not remove the old fsl,panel support
(backward compatibility). But it shouldn't be a big deal, just check if
the fsl,panel property is there, if it is, do not parse the endpoints.
Such backward compatibility has been implemented here:
https://patchwork.kernel.org/patch/7706481/

--
Stefan

> 
> diff --git a/drivers/gpu/drm/fsl-dcu/Makefile b/drivers/gpu/drm/fsl-dcu/Makefile
> index b35a292..12e2245 100644
> --- a/drivers/gpu/drm/fsl-dcu/Makefile
> +++ b/drivers/gpu/drm/fsl-dcu/Makefile
> @@ -1,6 +1,7 @@
>  fsl-dcu-drm-y := fsl_dcu_drm_drv.o \
>  		 fsl_dcu_drm_kms.o \
>  		 fsl_dcu_drm_rgb.o \
> +		 fsl_dcu_drm_hdmi.o \
>  		 fsl_dcu_drm_plane.o \
>  		 fsl_dcu_drm_crtc.o \
>  		 fsl_dcu_drm_fbdev.o \
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> new file mode 100644
> index 0000000..f567534
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_hdmi.c
> @@ -0,0 +1,189 @@
> +/*
> + * Copyright 2016 NXP Semiconductor, Inc.
> + *
> + * NXP DCU drm device driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/of_graph.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc_helper.h>
> +
> +#include "fsl_dcu_drm_drv.h"
> +#include "fsl_dcu_drm_output.h"
> +#include "fsl_tcon.h"
> +
> +static void
> +fsl_dcu_drm_hdmienc_mode_set(struct drm_encoder *encoder,
> +				 struct drm_display_mode *mode,
> +				 struct drm_display_mode *adjusted_mode)
> +{
> +	/*TODO*/
> +}
> +
> +static int
> +fsl_dcu_drm_hdmienc_atomic_check(struct drm_encoder *encoder,
> +				 struct drm_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state)
> +{
> +	return 0;
> +}
> +
> +static void
> +fsl_dcu_drm_hdmienc_disable(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> +
> +	if (fsl_dev->tcon)
> +		fsl_tcon_bypass_disable(fsl_dev->tcon);
> +}
> +
> +static void
> +fsl_dcu_drm_hdmienc_enable(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> +
> +	if (fsl_dev->tcon)
> +		fsl_tcon_bypass_enable(fsl_dev->tcon);
> +}
> +
> +static const struct
> +drm_encoder_helper_funcs encoder_helper_funcs = {
> +	.atomic_check = fsl_dcu_drm_hdmienc_atomic_check,
> +	.disable = fsl_dcu_drm_hdmienc_disable,
> +	.enable = fsl_dcu_drm_hdmienc_enable,
> +	.mode_set = fsl_dcu_drm_hdmienc_mode_set,
> +};
> +
> +static void
> +fsl_dcu_drm_hdmienc_destroy(struct drm_encoder *encoder)
> +{
> +	drm_encoder_cleanup(encoder);
> +}
> +
> +static const struct
> +drm_encoder_funcs encoder_funcs = {
> +	.destroy = fsl_dcu_drm_hdmienc_destroy,
> +};
> +
> +int fsl_dcu_drm_hdmienc_create(struct fsl_dcu_drm_device *fsl_dev,
> +			       struct drm_crtc *crtc)
> +{
> +	struct drm_encoder *encoder;
> +	int ret;
> +
> +	encoder = devm_kzalloc(fsl_dev->dev,
> +				sizeof(struct drm_encoder), GFP_KERNEL);
> +	if (!encoder)
> +		return -ENOMEM;
> +
> +	encoder->possible_crtcs = 1;
> +	ret = drm_encoder_init(fsl_dev->drm, encoder, &encoder_funcs,
> +			       DRM_MODE_ENCODER_TMDS, NULL);
> +	if (ret)
> +		goto fail_encoder;
> +
> +	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +	encoder->crtc = crtc;
> +
> +	return 0;
> +
> +fail_encoder:
> +	devm_kfree(fsl_dev->dev, encoder);
> +	return ret;
> +}
> +
> +static struct
> +drm_encoder *fsl_dcu_drm_hdmi_find_encoder(struct drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> +		if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS)
> +			return encoder;
> +	}
> +
> +	return NULL;
> +}
> +
> +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);
> +	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;
> +}
> +
> +int fsl_dcu_drm_hdmienc_attach_bridge(struct fsl_dcu_drm_device *fsl_dev)
> +{
> +	struct drm_device *drm_dev = fsl_dev->drm;
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *bridge;
> +	struct device_node *remote_port;
> +	int ret;
> +
> +	remote_port = detect_hdmi_connection(fsl_dev);
> +	if (!remote_port)
> +		return -ENODEV;
> +
> +	bridge = of_drm_find_bridge(remote_port);
> +	if (!bridge)
> +		return -ENODEV;
> +
> +	encoder = fsl_dcu_drm_hdmi_find_encoder(drm_dev);
> +	if (!encoder)
> +		return -ENODEV;
> +
> +	encoder->bridge = bridge;
> +	bridge->encoder = encoder;
> +
> +	ret = drm_bridge_attach(drm_dev, bridge);
> +	if (ret)
> +		goto error;
> +
> +	return 0;
> +error:
> +	encoder->funcs->destroy(encoder);
> +	return ret;
> +}
> 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");
> +
>  	drm_mode_config_reset(fsl_dev->drm);
>  	drm_kms_helper_poll_init(fsl_dev->drm);
>  
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> index 7093109..8915b07 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_output.h
> @@ -29,5 +29,9 @@ int fsl_dcu_drm_connector_create(struct
> fsl_dcu_drm_device *fsl_dev,
>  				 struct drm_encoder *encoder);
>  int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev,
>  			       struct drm_crtc *crtc);
> +int fsl_dcu_drm_hdmienc_attach_bridge(struct fsl_dcu_drm_device *fsl_dev);
> +int fsl_dcu_drm_hdmienc_create(struct fsl_dcu_drm_device *fsl_dev,
> +			       struct drm_crtc *crtc);
> +
>  
>  #endif /* __FSL_DCU_DRM_CONNECTOR_H__ */
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel