diff mbox

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

Message ID 1465896037-537-1-git-send-email-meng.yi@nxp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Meng Yi June 14, 2016, 9:20 a.m. UTC
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

Comments

Alexander Stein June 14, 2016, 9:54 a.m. UTC | #1
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@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
> [...]
> +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
Meng Yi June 15, 2016, 2:25 a.m. UTC | #2
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
Stefan Agner June 19, 2016, 3 a.m. UTC | #3
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__ */
Meng Yi June 20, 2016, 7:02 a.m. UTC | #4
Hi Stefan,

Thank for your reply.

> 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/
> 

That's more reasonable. Thank much for your information and guidance!
I will working on that and send the patch later.

Best Regards,
Meng Yi
diff mbox

Patch

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__ */