mbox series

[v2,0/6] Add tda998x (HDMI) support to atmel-hlcdc

Message ID 20180417131052.16336-1-peda@axentia.se
Headers show
Series Add tda998x (HDMI) support to atmel-hlcdc | expand

Message

Peter Rosin April 17, 2018, 1:10 p.m. UTC 
Hi!

I naively thought that since there was support for both nxp,tda19988 (in
the tda998x driver) and the atmel-hlcdc, things would be a smooth ride.
But it wasn't, so I started looking around, and found some missing
pieces in the tilcdc driver. I "stole" some things and made it work
for my use case.

In addition to the above, our PCB interface between the SAMA5D3 and the
HDMI encoder is only using 16 bits, and this has to be described
somewhere, or the atmel-hlcdc driver have no chance of selecting the
correct output mode. Since I had similar problems with ds90c185 lvds
encoder I added patches to override the atmel-hlcdc output format via
DT properties compatible with the media video-interface binding and
things start to play together.

Since this series superseeds the bridge series [1], I have included the
leftover bindings patch for the ti,ds90c185 here. I also noticed that
the driver date for atmel-hlcdc is bonkers, and added a patch for that.

However, I don't know if the tilcdc driver is interfacing with the
tda998x driver in a sane and modern way, and I don't know if I have
missed any subtle point when I "stole" the code and componentized the
atmel-hlcdc driver. I also have not tested how this behaves if I run
with the components as modules (not targeting that). Further, I'm
not sure if I interpret the media video-interface binding correctly.

Anyway, this series solves some real issues for my HW.

Cheers,
Peter

Changes since v1   https://lkml.org/lkml/2018/4/9/294
- added reviewed-by from Rob to patch 1/6
- patch 2/6 changed so that the bus format override is in the endpoint
  DT node, and follows the binding of media video-interfaces.
- patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above
  media video-interface binding (partially).
- patch 4/6 now makes use of the above helper (and also fixes problems
  with the 3/5 patch from v1 when no override was specified).
- do not mention unrelated connector display_info details in the cover
  letter and commit messages.

[1]
"Bridge" series v2   https://lkml.org/lkml/2018/3/26/610
"Bridge" series v1   https://lkml.org/lkml/2018/3/17/221

Peter Rosin (6):
  dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185
  dt-bindings: display: atmel: optional video-interface of endpoints
  drm: of: introduce drm_of_media_bus_fmt
  drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes
  drm/atmel-hlcdc: add support for connecting to tda998x HDMI encoder
  drm/atmel-hlcdc: fix broken release date

 .../devicetree/bindings/display/atmel/hlcdc-dc.txt |   8 ++
 .../bindings/display/bridge/lvds-transmitter.txt   |   8 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     |  85 ++++++++++----
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c       |  85 ++++++++++++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h       |  15 +++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   | 130 +++++++++++++++++++++
 drivers/gpu/drm/drm_of.c                           |  38 ++++++
 include/drm/drm_of.h                               |   7 ++
 8 files changed, 347 insertions(+), 29 deletions(-)

Comments

Boris Brezillon April 18, 2018, 7:29 a.m. UTC | #1 
On Tue, 17 Apr 2018 15:10:50 +0200
Peter Rosin <peda@axentia.se> wrote:

> This beats the heuristic that the connector is involved in what format
> should be output for cases where this fails.
> 
> E.g. if there is a bridge that changes format between the encoder and the
> connector, or if some of the RGB pins between the lcd controller and the
> encoder are not routed on the PCB.
> 
> This is critical for the devices that have the "conflicting output
> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> RGB bits move around depending on the selected output mode. For
> devices that do not have the "conflicting output formats" issue
> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 85 ++++++++++++++++++++------
>  1 file changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index d73281095fac..2e718959981e 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -19,12 +19,14 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/of_graph.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pinctrl/consumer.h>
>  
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_of.h>
>  #include <drm/drmP.h>
>  
>  #include <video/videomode.h>
> @@ -226,6 +228,68 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
>  #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
>  #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
>  
> +static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
> +{
> +	struct drm_connector *connector = state->connector;
> +	struct drm_display_info *info = &connector->display_info;
> +	unsigned int supported_fmts = 0;
> +	struct device_node *ep;
> +	int j;
> +
> +	/*
> +	 * Use the connector index as an approximation of the
> +	 * endpoint node index. We know it's true for our case
> +	 * depending on the driver implementation.
> +	 */
> +	ep = of_graph_get_endpoint_by_regs(connector->dev->dev->of_node, 0,
> +					   connector->index);
> +

Hm, this sounds a bit fragile. Can't we have a reference to the of_node
attached to the connector? Or maybe we can parse this earlier and set a
constraint on the accepted modes.

> +	if (ep) {
> +		int bus_fmt = drm_of_media_bus_fmt(ep);

Hm, you're extracting this piece of information from the DT every time
an atomic modeset is done. I'd really prefer to have this done once at
probe time. Since this property is attached to the connector, maybe we
should overwrite the info->bus_formats[] array or mark some of its
entries as invalid.

> +
> +		of_node_put(ep);
> +
> +		if (bus_fmt < 0)
> +			return bus_fmt;
> +
> +		switch (bus_fmt) {
> +		case 0:
> +			break;
> +		case MEDIA_BUS_FMT_RGB444_1X12:
> +			return ATMEL_HLCDC_RGB444_OUTPUT;
> +		case MEDIA_BUS_FMT_RGB565_1X16:
> +			return ATMEL_HLCDC_RGB565_OUTPUT;
> +		case MEDIA_BUS_FMT_RGB666_1X18:
> +			return ATMEL_HLCDC_RGB666_OUTPUT;
> +		case MEDIA_BUS_FMT_RGB888_1X24:
> +			return ATMEL_HLCDC_RGB888_OUTPUT;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for (j = 0; j < info->num_bus_formats; j++) {
> +		switch (info->bus_formats[j]) {
> +		case MEDIA_BUS_FMT_RGB444_1X12:
> +			supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
> +			break;
> +		case MEDIA_BUS_FMT_RGB565_1X16:
> +			supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
> +			break;
> +		case MEDIA_BUS_FMT_RGB666_1X18:
> +			supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
> +			break;
> +		case MEDIA_BUS_FMT_RGB888_1X24:
> +			supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return supported_fmts;
> +}
> +
>  static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
>  {
>  	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
> @@ -238,31 +302,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
>  	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
>  
>  	for_each_new_connector_in_state(state->state, connector, cstate, i) {
> -		struct drm_display_info *info = &connector->display_info;
>  		unsigned int supported_fmts = 0;
> -		int j;
>  
>  		if (!cstate->crtc)
>  			continue;
>  
> -		for (j = 0; j < info->num_bus_formats; j++) {
> -			switch (info->bus_formats[j]) {
> -			case MEDIA_BUS_FMT_RGB444_1X12:
> -				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
> -				break;
> -			case MEDIA_BUS_FMT_RGB565_1X16:
> -				supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
> -				break;
> -			case MEDIA_BUS_FMT_RGB666_1X18:
> -				supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
> -				break;
> -			case MEDIA_BUS_FMT_RGB888_1X24:
> -				supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
> -				break;
> -			default:
> -				break;
> -			}
> -		}
> +		supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
>  
>  		if (crtc->dc->desc->conflicting_output_formats)
>  			output_fmts &= supported_fmts;

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon April 18, 2018, 7:36 a.m. UTC | #2 
On Tue, 17 Apr 2018 15:10:51 +0200
Peter Rosin <peda@axentia.se> wrote:

> When the of-graph points to a tda998x-compatible HDMI encoder, register
> as a component master and bind to the encoder/connector provided by
> the tda998x driver.

Can't we do the opposite: make the tda998x driver expose its devices as
drm bridges. I'd rather not add another way to connect external
encoders (or bridges) to display controller drivers, especially since,
when I asked DRM maintainers/devs what was the good approach to
represent such external encoders they pointed me to the drm_bridge
interface.

> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c     |  81 ++++++++++++--
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h     |  15 +++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 130 +++++++++++++++++++++++
>  3 files changed, 220 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index c1ea5c36b006..8523c40fac94 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/component.h>
>  #include <linux/irq.h>
>  #include <linux/irqchip.h>
>  #include <linux/module.h>
> @@ -568,10 +569,13 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
>  
>  	drm_mode_config_init(dev);
>  
> -	ret = atmel_hlcdc_create_outputs(dev);
> -	if (ret) {
> -		dev_err(dev->dev, "failed to create HLCDC outputs: %d\n", ret);
> -		return ret;
> +	if (!dc->is_componentized) {
> +		ret = atmel_hlcdc_create_outputs(dev);
> +		if (ret) {
> +			dev_err(dev->dev,
> +				"failed to create HLCDC outputs: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	ret = atmel_hlcdc_create_planes(dev);
> @@ -586,6 +590,16 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
>  		return ret;
>  	}
>  
> +	if (dc->is_componentized) {
> +		ret = component_bind_all(dev->dev, dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = atmel_hlcdc_add_component_encoder(dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	dev->mode_config.min_width = dc->desc->min_width;
>  	dev->mode_config.min_height = dc->desc->min_height;
>  	dev->mode_config.max_width = dc->desc->max_width;
> @@ -617,6 +631,9 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>  	if (!dc)
>  		return -ENOMEM;
>  
> +	dc->is_componentized =
> +		atmel_hlcdc_get_external_components(dev->dev, NULL) > 0;
> +
>  	dc->wq = alloc_ordered_workqueue("atmel-hlcdc-dc", 0);
>  	if (!dc->wq)
>  		return -ENOMEM;
> @@ -751,7 +768,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
>  	.minor = 0,
>  };
>  
> -static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> +static int atmel_hlcdc_dc_drm_init(struct platform_device *pdev)
>  {
>  	struct drm_device *ddev;
>  	int ret;
> @@ -779,7 +796,7 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
> +static int atmel_hlcdc_dc_drm_fini(struct platform_device *pdev)
>  {
>  	struct drm_device *ddev = platform_get_drvdata(pdev);
>  
> @@ -790,6 +807,58 @@ static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int atmel_hlcdc_bind(struct device *dev)
> +{
> +	return atmel_hlcdc_dc_drm_init(to_platform_device(dev));
> +}
> +
> +static void atmel_hlcdc_unbind(struct device *dev)
> +{
> +	struct drm_device *ddev = dev_get_drvdata(dev);
> +
> +	/* Check if a subcomponent has already triggered the unloading. */
> +	if (!ddev->dev_private)
> +		return;
> +
> +	atmel_hlcdc_dc_drm_fini(to_platform_device(dev));
> +}
> +
> +static const struct component_master_ops atmel_hlcdc_comp_ops = {
> +	.bind = atmel_hlcdc_bind,
> +	.unbind = atmel_hlcdc_unbind,
> +};
> +
> +static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> +{
> +	struct component_match *match = NULL;
> +	int ret;
> +
> +	ret = atmel_hlcdc_get_external_components(&pdev->dev, &match);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret)
> +		return component_master_add_with_match(&pdev->dev,
> +						       &atmel_hlcdc_comp_ops,
> +						       match);
> +	else
> +		return atmel_hlcdc_dc_drm_init(pdev);
> +}
> +
> +static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	ret = atmel_hlcdc_get_external_components(&pdev->dev, NULL);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret)
> +		component_master_del(&pdev->dev, &atmel_hlcdc_comp_ops);
> +	else
> +		atmel_hlcdc_dc_drm_fini(pdev);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>  {
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> index ab32d5b268d2..cae77c245661 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> @@ -370,6 +370,10 @@ struct atmel_hlcdc_plane_properties {
>   * @wq: display controller workqueue
>   * @suspend: used to store the HLCDC state when entering suspend
>   * @commit: used for async commit handling
> + * @external_encoder: used encoder when componentized
> + * @external_connector: used connector when componentized
> + * @connector_funcs: original helper funcs of the external connector
> + * @is_componentized: operating mode
>   */
>  struct atmel_hlcdc_dc {
>  	const struct atmel_hlcdc_dc_desc *desc;
> @@ -386,6 +390,12 @@ struct atmel_hlcdc_dc {
>  		wait_queue_head_t wait;
>  		bool pending;
>  	} commit;
> +
> +	struct drm_encoder *external_encoder;
> +	struct drm_connector *external_connector;
> +	const struct drm_connector_helper_funcs *connector_funcs;
> +
> +	bool is_componentized;
>  };
>  
>  extern struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_formats;
> @@ -455,4 +465,9 @@ int atmel_hlcdc_crtc_create(struct drm_device *dev);
>  
>  int atmel_hlcdc_create_outputs(struct drm_device *dev);
>  
> +struct component_match;
> +int atmel_hlcdc_add_component_encoder(struct drm_device *dev);
> +int atmel_hlcdc_get_external_components(struct device *dev,
> +					struct component_match **match);
> +
>  #endif /* DRM_ATMEL_HLCDC_H */
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> index 8db51fb131db..3f86527e0473 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
> @@ -6,6 +6,11 @@
>   * Author: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>   * Author: Boris BREZILLON <boris.brezillon@free-electrons.com>
>   *
> + * Handling of external components adapted from the tilcdc driver
> + * by Peter Rosin <peda@axentia.se>. That original code had:
> + *   Copyright (C) 2015 Texas Instruments
> + *   Author: Jyri Sarha <jsarha@ti.com>
> + *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License version 2 as published by
>   * the Free Software Foundation.
> @@ -19,6 +24,7 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/component.h>
>  #include <linux/of_graph.h>
>  
>  #include <drm/drmP.h>
> @@ -88,3 +94,127 @@ int atmel_hlcdc_create_outputs(struct drm_device *dev)
>  
>  	return ret;
>  }
> +
> +static int atmel_hlcdc_external_mode_valid(struct drm_connector *connector,
> +					   struct drm_display_mode *mode)
> +{
> +	struct atmel_hlcdc_dc *dc = connector->dev->dev_private;
> +	int ret;
> +
> +	ret = atmel_hlcdc_dc_mode_valid(dc, mode);
> +	if (ret != MODE_OK)
> +		return ret;
> +
> +	if (WARN_ON(dc->external_connector != connector))
> +		return MODE_ERROR;
> +	if (WARN_ON(!dc->connector_funcs))
> +		return MODE_ERROR;
> +
> +	if (IS_ERR(dc->connector_funcs) || !dc->connector_funcs->mode_valid)
> +		return MODE_OK;
> +
> +	/* The connector has its own mode_valid, call it. */
> +	return dc->connector_funcs->mode_valid(connector, mode);
> +}
> +
> +static int atmel_hlcdc_add_external_connector(struct drm_device *dev,
> +					      struct drm_connector *connector)
> +{
> +	struct atmel_hlcdc_dc *dc = dev->dev_private;
> +	struct drm_connector_helper_funcs *connector_funcs;
> +
> +	/* There should never be more than one connector. */
> +	if (WARN_ON(dc->external_connector))
> +		return -EINVAL;
> +
> +	dc->external_connector = connector;
> +	connector_funcs = devm_kzalloc(dev->dev, sizeof(*connector_funcs),
> +				       GFP_KERNEL);
> +	if (!connector_funcs)
> +		return -ENOMEM;
> +
> +	/*
> +	 * connector->helper_private always contains the struct
> +	 * connector_helper_funcs pointer. For the atmel-hlcdc crtc
> +	 * to have a say if a specific mode is Ok, we install our
> +	 * own helper functions. In our helper functions we copy
> +	 * everything else but use our own mode_valid() (above).
> +	 */
> +	if (connector->helper_private) {
> +		dc->connector_funcs = connector->helper_private;
> +		*connector_funcs = *dc->connector_funcs;
> +	} else {
> +		dc->connector_funcs = ERR_PTR(-ENOENT);
> +	}
> +	connector_funcs->mode_valid = atmel_hlcdc_external_mode_valid;
> +	drm_connector_helper_add(connector, connector_funcs);
> +
> +	dev_dbg(dev->dev, "External connector '%s' connected\n",
> +		connector->name);
> +
> +	return 0;
> +}
> +
> +static struct drm_connector *
> +atmel_hlcdc_encoder_find_connector(struct drm_device *dev,
> +				   struct drm_encoder *encoder)
> +{
> +	struct drm_connector *connector;
> +	int i;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> +		for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++)
> +			if (connector->encoder_ids[i] == encoder->base.id)
> +				return connector;
> +
> +	dev_err(dev->dev, "No connector found for %s encoder (id %d)\n",
> +		encoder->name, encoder->base.id);
> +
> +	return NULL;
> +}
> +
> +int atmel_hlcdc_add_component_encoder(struct drm_device *dev)
> +{
> +	struct atmel_hlcdc_dc *dc = dev->dev_private;
> +	struct drm_connector *connector;
> +	struct drm_encoder *encoder;
> +
> +	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
> +		if (encoder->possible_crtcs & (1 << dc->crtc->index))
> +			break;
> +	}
> +
> +	if (!encoder) {
> +		dev_err(dev->dev, "%s: No suitable encoder found\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	connector = atmel_hlcdc_encoder_find_connector(dev, encoder);
> +	if (!connector)
> +		return -ENODEV;
> +
> +	return atmel_hlcdc_add_external_connector(dev, connector);
> +}
> +
> +static int dev_match_of(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +int atmel_hlcdc_get_external_components(struct device *dev,
> +					struct component_match **match)
> +{
> +	struct device_node *node;
> +
> +	node = of_graph_get_remote_node(dev->of_node, 0, 0);
> +
> +	if (!of_device_is_compatible(node, "nxp,tda998x")) {
> +		of_node_put(node);
> +		return 0;
> +	}
> +
> +	if (match)
> +		drm_of_component_match_add(dev, match, dev_match_of, node);
> +	of_node_put(node);
> +	return 1;
> +}

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon April 18, 2018, 7:44 a.m. UTC | #3 
On Tue, 17 Apr 2018 15:10:52 +0200
Peter Rosin <peda@axentia.se> wrote:

> Bump the minor version while at it.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 8523c40fac94..aa48f287b5ca 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -763,9 +763,9 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
>  	.fops = &fops,
>  	.name = "atmel-hlcdc",
>  	.desc = "Atmel HLCD Controller DRM",
> -	.date = "20141504",

I guess I used YYYYDDMM format :-).

> +	.date = "20180409",
>  	.major = 1,
> -	.minor = 0,
> +	.minor = 1,

Don't know what the strategy is regarding date and minor version
updates. I never had to update that before, so I guess it's not
important to userspace anyway.

>  };
>  
>  static int atmel_hlcdc_dc_drm_init(struct platform_device *pdev)

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Rosin April 18, 2018, 7:46 a.m. UTC | #4 
On 2018-04-18 09:29, Boris Brezillon wrote:
> On Tue, 17 Apr 2018 15:10:50 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> This beats the heuristic that the connector is involved in what format
>> should be output for cases where this fails.
>>
>> E.g. if there is a bridge that changes format between the encoder and the
>> connector, or if some of the RGB pins between the lcd controller and the
>> encoder are not routed on the PCB.
>>
>> This is critical for the devices that have the "conflicting output
>> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
>> RGB bits move around depending on the selected output mode. For
>> devices that do not have the "conflicting output formats" issue
>> (SAMA5D2, SAMA5D4), this is completely irrelevant.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 85 ++++++++++++++++++++------
>>  1 file changed, 65 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index d73281095fac..2e718959981e 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -19,12 +19,14 @@
>>   */
>>  
>>  #include <linux/clk.h>
>> +#include <linux/of_graph.h>
>>  #include <linux/pm.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/pinctrl/consumer.h>
>>  
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_of.h>
>>  #include <drm/drmP.h>
>>  
>>  #include <video/videomode.h>
>> @@ -226,6 +228,68 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
>>  #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
>>  #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
>>  
>> +static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
>> +{
>> +	struct drm_connector *connector = state->connector;
>> +	struct drm_display_info *info = &connector->display_info;
>> +	unsigned int supported_fmts = 0;
>> +	struct device_node *ep;
>> +	int j;
>> +
>> +	/*
>> +	 * Use the connector index as an approximation of the
>> +	 * endpoint node index. We know it's true for our case
>> +	 * depending on the driver implementation.
>> +	 */
>> +	ep = of_graph_get_endpoint_by_regs(connector->dev->dev->of_node, 0,
>> +					   connector->index);
>> +
> 
> Hm, this sounds a bit fragile. Can't we have a reference to the of_node
> attached to the connector? Or maybe we can parse this earlier and set a
> constraint on the accepted modes.
> 
>> +	if (ep) {
>> +		int bus_fmt = drm_of_media_bus_fmt(ep);
> 
> Hm, you're extracting this piece of information from the DT every time
> an atomic modeset is done. I'd really prefer to have this done once at

Yes, not happy about it either. I looked for other sensible places too
hook the info at probe time, but this was just the simplest. I'll take
another look...

> probe time. Since this property is attached to the connector, maybe we
> should overwrite the info->bus_formats[] array or mark some of its
> entries as invalid.

I find it very wrong to mix the connector format with what you want to
output. In my mind it's a broken assumption that they are related. It is
only correct for trivial cases. Also note my comment about the connector
index and the endpoint index, they are only coincidentally the same
based on our implementation. If the driver has more than one port or
initializes endpoints out of order for some reason, this is no longer
true.

I think it would be better to store this info somewhere near the encoder,
since that is what I find closest to what I'm trying to change.

As I said, I'll take another look and see if I can hook this in at some
other place.

>> +
>> +		of_node_put(ep);
>> +
>> +		if (bus_fmt < 0)
>> +			return bus_fmt;
>> +
>> +		switch (bus_fmt) {
>> +		case 0:
>> +			break;
>> +		case MEDIA_BUS_FMT_RGB444_1X12:
>> +			return ATMEL_HLCDC_RGB444_OUTPUT;
>> +		case MEDIA_BUS_FMT_RGB565_1X16:
>> +			return ATMEL_HLCDC_RGB565_OUTPUT;
>> +		case MEDIA_BUS_FMT_RGB666_1X18:
>> +			return ATMEL_HLCDC_RGB666_OUTPUT;
>> +		case MEDIA_BUS_FMT_RGB888_1X24:
>> +			return ATMEL_HLCDC_RGB888_OUTPUT;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	for (j = 0; j < info->num_bus_formats; j++) {
>> +		switch (info->bus_formats[j]) {
>> +		case MEDIA_BUS_FMT_RGB444_1X12:
>> +			supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
>> +			break;
>> +		case MEDIA_BUS_FMT_RGB565_1X16:
>> +			supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
>> +			break;
>> +		case MEDIA_BUS_FMT_RGB666_1X18:
>> +			supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
>> +			break;
>> +		case MEDIA_BUS_FMT_RGB888_1X24:
>> +			supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	return supported_fmts;
>> +}
>> +
>>  static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
>>  {
>>  	unsigned int output_fmts = ATMEL_HLCDC_OUTPUT_MODE_MASK;
>> @@ -238,31 +302,12 @@ static int atmel_hlcdc_crtc_select_output_mode(struct drm_crtc_state *state)
>>  	crtc = drm_crtc_to_atmel_hlcdc_crtc(state->crtc);
>>  
>>  	for_each_new_connector_in_state(state->state, connector, cstate, i) {
>> -		struct drm_display_info *info = &connector->display_info;
>>  		unsigned int supported_fmts = 0;
>> -		int j;
>>  
>>  		if (!cstate->crtc)
>>  			continue;
>>  
>> -		for (j = 0; j < info->num_bus_formats; j++) {
>> -			switch (info->bus_formats[j]) {
>> -			case MEDIA_BUS_FMT_RGB444_1X12:
>> -				supported_fmts |= ATMEL_HLCDC_RGB444_OUTPUT;
>> -				break;
>> -			case MEDIA_BUS_FMT_RGB565_1X16:
>> -				supported_fmts |= ATMEL_HLCDC_RGB565_OUTPUT;
>> -				break;
>> -			case MEDIA_BUS_FMT_RGB666_1X18:
>> -				supported_fmts |= ATMEL_HLCDC_RGB666_OUTPUT;
>> -				break;
>> -			case MEDIA_BUS_FMT_RGB888_1X24:
>> -				supported_fmts |= ATMEL_HLCDC_RGB888_OUTPUT;
>> -				break;
>> -			default:
>> -				break;
>> -			}
>> -		}
>> +		supported_fmts = atmel_hlcdc_connector_output_mode(cstate);
>>  
>>  		if (crtc->dc->desc->conflicting_output_formats)
>>  			output_fmts &= supported_fmts;
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Rosin April 18, 2018, 8:02 a.m. UTC | #5 
On 2018-04-18 09:36, Boris Brezillon wrote:
> On Tue, 17 Apr 2018 15:10:51 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> When the of-graph points to a tda998x-compatible HDMI encoder, register
>> as a component master and bind to the encoder/connector provided by
>> the tda998x driver.
> 
> Can't we do the opposite: make the tda998x driver expose its devices as
> drm bridges. I'd rather not add another way to connect external
> encoders (or bridges) to display controller drivers, especially since,
> when I asked DRM maintainers/devs what was the good approach to
> represent such external encoders they pointed me to the drm_bridge
> interface.

>From the cover letter:

"However, I don't know if the tilcdc driver is interfacing with the
tda998x driver in a sane and modern way"

So, which way is the future? Should bridges become components or should
existing bridge-like components no longer be components? Are there others?

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Rosin April 18, 2018, 8:09 a.m. UTC | #6 
On 2018-04-18 09:44, Boris Brezillon wrote:
> On Tue, 17 Apr 2018 15:10:52 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> Bump the minor version while at it.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> index 8523c40fac94..aa48f287b5ca 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -763,9 +763,9 @@ static struct drm_driver atmel_hlcdc_dc_driver = {
>>  	.fops = &fops,
>>  	.name = "atmel-hlcdc",
>>  	.desc = "Atmel HLCD Controller DRM",
>> -	.date = "20141504",
> 
> I guess I used YYYYDDMM format :-).
> 
>> +	.date = "20180409",
>>  	.major = 1,
>> -	.minor = 0,
>> +	.minor = 1,
> 
> Don't know what the strategy is regarding date and minor version
> updates. I never had to update that before, so I guess it's not
> important to userspace anyway.

I have no clue what strategy should be employed. I assume it's
for easier communication when features are backported into stable
or distro kernels and users wonder what to expect or for easier
triage of bug reports or something?? Anyway, I don't really care,
the date just looked really odd. So feel free to only patch the
.date to 20140415 or whatever. I'll drop this patch.

Cheers,
Peter

>>  };
>>  
>>  static int atmel_hlcdc_dc_drm_init(struct platform_device *pdev)
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon April 18, 2018, 8:27 a.m. UTC | #7 
On Wed, 18 Apr 2018 09:46:09 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-04-18 09:29, Boris Brezillon wrote:
> > On Tue, 17 Apr 2018 15:10:50 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> This beats the heuristic that the connector is involved in what format
> >> should be output for cases where this fails.
> >>
> >> E.g. if there is a bridge that changes format between the encoder and the
> >> connector, or if some of the RGB pins between the lcd controller and the
> >> encoder are not routed on the PCB.
> >>
> >> This is critical for the devices that have the "conflicting output
> >> formats" issue (SAM9N12, SAM9X5, SAMA5D3), since the most significant
> >> RGB bits move around depending on the selected output mode. For
> >> devices that do not have the "conflicting output formats" issue
> >> (SAMA5D2, SAMA5D4), this is completely irrelevant.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 85 ++++++++++++++++++++------
> >>  1 file changed, 65 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >> index d73281095fac..2e718959981e 100644
> >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> >> @@ -19,12 +19,14 @@
> >>   */
> >>  
> >>  #include <linux/clk.h>
> >> +#include <linux/of_graph.h>
> >>  #include <linux/pm.h>
> >>  #include <linux/pm_runtime.h>
> >>  #include <linux/pinctrl/consumer.h>
> >>  
> >>  #include <drm/drm_crtc.h>
> >>  #include <drm/drm_crtc_helper.h>
> >> +#include <drm/drm_of.h>
> >>  #include <drm/drmP.h>
> >>  
> >>  #include <video/videomode.h>
> >> @@ -226,6 +228,68 @@ static void atmel_hlcdc_crtc_atomic_enable(struct drm_crtc *c,
> >>  #define ATMEL_HLCDC_RGB888_OUTPUT	BIT(3)
> >>  #define ATMEL_HLCDC_OUTPUT_MODE_MASK	GENMASK(3, 0)
> >>  
> >> +static int atmel_hlcdc_connector_output_mode(struct drm_connector_state *state)
> >> +{
> >> +	struct drm_connector *connector = state->connector;
> >> +	struct drm_display_info *info = &connector->display_info;
> >> +	unsigned int supported_fmts = 0;
> >> +	struct device_node *ep;
> >> +	int j;
> >> +
> >> +	/*
> >> +	 * Use the connector index as an approximation of the
> >> +	 * endpoint node index. We know it's true for our case
> >> +	 * depending on the driver implementation.
> >> +	 */
> >> +	ep = of_graph_get_endpoint_by_regs(connector->dev->dev->of_node, 0,
> >> +					   connector->index);
> >> +  
> > 
> > Hm, this sounds a bit fragile. Can't we have a reference to the of_node
> > attached to the connector? Or maybe we can parse this earlier and set a
> > constraint on the accepted modes.
> >   
> >> +	if (ep) {
> >> +		int bus_fmt = drm_of_media_bus_fmt(ep);  
> > 
> > Hm, you're extracting this piece of information from the DT every time
> > an atomic modeset is done. I'd really prefer to have this done once at  
> 
> Yes, not happy about it either. I looked for other sensible places too
> hook the info at probe time, but this was just the simplest. I'll take
> another look...
> 
> > probe time. Since this property is attached to the connector, maybe we
> > should overwrite the info->bus_formats[] array or mark some of its
> > entries as invalid.  
> 
> I find it very wrong to mix the connector format with what you want to
> output. In my mind it's a broken assumption that they are related. It is
> only correct for trivial cases. Also note my comment about the connector
> index and the endpoint index, they are only coincidentally the same
> based on our implementation. If the driver has more than one port or
> initializes endpoints out of order for some reason, this is no longer
> true.
> 
> I think it would be better to store this info somewhere near the encoder,
> since that is what I find closest to what I'm trying to change.

Totally agree with you on that: the connector has nothing to do with
how intermediate encoders/bridges exchange data with each other.

> 
> As I said, I'll take another look and see if I can hook this in at some
> other place.

Okay, cool.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Boris Brezillon April 18, 2018, 8:41 a.m. UTC | #8 
On Wed, 18 Apr 2018 10:02:12 +0200
Peter Rosin <peda@axentia.se> wrote:

> On 2018-04-18 09:36, Boris Brezillon wrote:
> > On Tue, 17 Apr 2018 15:10:51 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> >> When the of-graph points to a tda998x-compatible HDMI encoder, register
> >> as a component master and bind to the encoder/connector provided by
> >> the tda998x driver.  
> > 
> > Can't we do the opposite: make the tda998x driver expose its devices as
> > drm bridges. I'd rather not add another way to connect external
> > encoders (or bridges) to display controller drivers, especially since,
> > when I asked DRM maintainers/devs what was the good approach to
> > represent such external encoders they pointed me to the drm_bridge
> > interface.  
> 
> From the cover letter:
> 
> "However, I don't know if the tilcdc driver is interfacing with the
> tda998x driver in a sane and modern way"
> 
> So, which way is the future? Should bridges become components or should
> existing bridge-like components no longer be components? Are there others?

Well, what I've been told a while ago is that drm_bridge will take over
drm_encoder_slave and custom drm_encoder/drm_connector implementations
when it comes to representing bridges.

AFAIU, using the component framework to bind all elements of the
pipeline to the display controller is orthogonal to how you represent
elements in the pipeline. I mean, you could have a bridge that
registers as a component so that display controllers drivers who want
to use the component framework don't have to re-code the
component-to-bridge glue every time, and those who don't use the
component framework can still get access to the bridge.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring April 19, 2018, 4:22 p.m. UTC | #9 
On Tue, Apr 17, 2018 at 8:10 AM, Peter Rosin <peda@axentia.se> wrote:
> Add a central function to parse a node according to the video
> interface binding and get a media bus format.
>
> Start with only supporting a very limited set of a few basic media
> bus formats.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_of.c | 38 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     |  7 +++++++
>  2 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 4c191c050e7d..f9473edb60a7 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -212,6 +212,44 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>
>  /*
> + * drm_of_media_bus_fmt - return the media bus format described in the node
> + * @node: device tree node containing the media bus format
> + *
> + * @node is presumably an of-graph endpoint node.
> + *
> + * Return the media bus format, or zero if none is described. Or one of the
> + * standard error codes.
> + */
> +int drm_of_media_bus_fmt(struct device_node *node)

Should use endpoint or ep here instead of node to be clear what node
this function operates on.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Rosin April 19, 2018, 4:40 p.m. UTC | #10 
On 2018-04-19 18:22, Rob Herring wrote:
> On Tue, Apr 17, 2018 at 8:10 AM, Peter Rosin <peda@axentia.se> wrote:
>> Add a central function to parse a node according to the video
>> interface binding and get a media bus format.
>>
>> Start with only supporting a very limited set of a few basic media
>> bus formats.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_of.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_of.h     |  7 +++++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>> index 4c191c050e7d..f9473edb60a7 100644
>> --- a/drivers/gpu/drm/drm_of.c
>> +++ b/drivers/gpu/drm/drm_of.c
>> @@ -212,6 +212,44 @@ int drm_of_encoder_active_endpoint(struct device_node *node,
>>  EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>>
>>  /*
>> + * drm_of_media_bus_fmt - return the media bus format described in the node
>> + * @node: device tree node containing the media bus format
>> + *
>> + * @node is presumably an of-graph endpoint node.
>> + *
>> + * Return the media bus format, or zero if none is described. Or one of the
>> + * standard error codes.
>> + */
>> +int drm_of_media_bus_fmt(struct device_node *node)
> 
> Should use endpoint or ep here instead of node to be clear what node
> this function operates on.

I just sent v3 and missed this. Anyway, the reason I didn't use ep,
was that you at one point said "port of endpoint", and I wanted this
function to cover both cases. And maybe others. Anyway, if I hear
nothing I'll make the change to ep and I'll also reword the comment
to talk more decisively about endpoints for v4.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html