Message-ID: <3f873837-82e0-81b0-8cbf-db6c8af8f677@codeaurora.org>
Date:   Thu, 4 May 2017 14:28:31 +0530
From:   Archit Taneja <architt@...eaurora.org>
To:     Eric Anholt <eric@...olt.net>, dri-devel@...ts.freedesktop.org
Cc:     Yannick Fertre <yannick.fertre@...com>,
        linux-kernel@...r.kernel.org, Thierry Reding <treding@...dia.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH 1/2] drm/bridge: Refactor out the panel wrapper from the
 lvds-encoder bridge.



On 05/03/2017 10:00 PM, Eric Anholt wrote:
> Archit Taneja <architt@...eaurora.org> writes:
>
>> Hi,
>>
>> On 04/27/2017 10:06 PM, Eric Anholt wrote:
>>> Many DRM drivers have common code to make a stub connector
>>> implementation that wraps a drm_panel.  By wrapping the panel in a DRM
>>> bridge, all of the connector code (including calls during encoder
>>> enable/disable) goes away.
>>>
>>> Signed-off-by: Eric Anholt <eric@...olt.net>
>>> ---
>>>  Documentation/gpu/drm-kms-helpers.rst |   3 +
>>>  drivers/gpu/drm/bridge/Kconfig        |  11 +-
>>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>>  drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------
>>>  drivers/gpu/drm/bridge/panel.c        | 188 ++++++++++++++++++++++++++++++++++
>>>  include/drm/drm_bridge.h              |   8 ++
>>>  6 files changed, 228 insertions(+), 141 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/bridge/panel.c
>>>
>>> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
>>> index c075aadd7078..60eb3b41702b 100644
>>> --- a/Documentation/gpu/drm-kms-helpers.rst
>>> +++ b/Documentation/gpu/drm-kms-helpers.rst
>>> @@ -143,6 +143,9 @@ Bridge Helper Reference
>>>  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
>>>     :export:
>>>
>>> +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c
>>> +   :export:
>>> +
>>>  .. _drm_panel_helper:
>>>
>>>  Panel Helper Reference
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index f6968d3b4b41..c4daca38743c 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -4,6 +4,14 @@ config DRM_BRIDGE
>>>  	help
>>>  	  Bridge registration and lookup framework.
>>>
>>> +config DRM_PANEL_BRIDGE
>>> +	def_bool y
>>> +	depends on DRM_BRIDGE
>>> +	select DRM_KMS_HELPER
>>> +	select DRM_PANEL
>>> +	help
>>> +	  DRM bridge wrapper of DRM panels
>>> +
>>>  menu "Display Interface Bridges"
>>>  	depends on DRM && DRM_BRIDGE
>>>
>>> @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC
>>>  config DRM_LVDS_ENCODER
>>>  	tristate "Transparent parallel to LVDS encoder support"
>>>  	depends on OF
>>> -	select DRM_KMS_HELPER
>>> -	select DRM_PANEL
>>> +	select DRM_PANEL_BRIDGE
>>>  	help
>>>  	  Support for transparent parallel to LVDS encoders that don't require
>>>  	  any configuration.
>>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
>>> index 3fe2226ee2f2..40a43750ad55 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
>>>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>>>  obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
>>>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>>> +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o
>>>  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
>>> index f1f67a279426..04e1504c4d8f 100644
>>> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
>>> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
>>> @@ -8,144 +8,18 @@
>>>   */
>>>
>>>  #include <drm/drmP.h>
>>> -#include <drm/drm_atomic_helper.h>
>>> -#include <drm/drm_connector.h>
>>> -#include <drm/drm_crtc_helper.h>
>>> -#include <drm/drm_encoder.h>
>>> -#include <drm/drm_modeset_helper_vtables.h>
>>> +#include <drm/drm_bridge.h>
>>>  #include <drm/drm_panel.h>
>>>
>>>  #include <linux/of_graph.h>
>>>
>>> -struct lvds_encoder {
>>> -	struct device *dev;
>>> -
>>> -	struct drm_bridge bridge;
>>> -	struct drm_connector connector;
>>> -	struct drm_panel *panel;
>>> -};
>>> -
>>> -static inline struct lvds_encoder *
>>> -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge)
>>> -{
>>> -	return container_of(bridge, struct lvds_encoder, bridge);
>>> -}
>>> -
>>> -static inline struct lvds_encoder *
>>> -drm_connector_to_lvds_encoder(struct drm_connector *connector)
>>> -{
>>> -	return container_of(connector, struct lvds_encoder, connector);
>>> -}
>>> -
>>> -static int lvds_connector_get_modes(struct drm_connector *connector)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector);
>>> -
>>> -	return drm_panel_get_modes(lvds->panel);
>>> -}
>>> -
>>> -static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = {
>>> -	.get_modes = lvds_connector_get_modes,
>>> -};
>>> -
>>> -static const struct drm_connector_funcs lvds_connector_funcs = {
>>> -	.dpms = drm_atomic_helper_connector_dpms,
>>> -	.reset = drm_atomic_helper_connector_reset,
>>> -	.fill_modes = drm_helper_probe_single_connector_modes,
>>> -	.destroy = drm_connector_cleanup,
>>> -	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>> -	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> -};
>>> -
>>> -static int lvds_encoder_attach(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
>>> -	struct drm_connector *connector = &lvds->connector;
>>> -	int ret;
>>> -
>>> -	if (!bridge->encoder) {
>>> -		DRM_ERROR("Missing encoder\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> -	drm_connector_helper_add(connector, &lvds_connector_helper_funcs);
>>> -
>>> -	ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs,
>>> -				 DRM_MODE_CONNECTOR_LVDS);
>>> -	if (ret) {
>>> -		DRM_ERROR("Failed to initialize connector\n");
>>> -		return ret;
>>> -	}
>>> -
>>> -	drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder);
>>> -
>>> -	ret = drm_panel_attach(lvds->panel, &lvds->connector);
>>> -	if (ret < 0)
>>> -		return ret;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static void lvds_encoder_detach(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
>>> -
>>> -	drm_panel_detach(lvds->panel);
>>> -}
>>> -
>>> -static void lvds_encoder_pre_enable(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
>>> -
>>> -	drm_panel_prepare(lvds->panel);
>>> -}
>>> -
>>> -static void lvds_encoder_enable(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
>>> -
>>> -	drm_panel_enable(lvds->panel);
>>> -}
>>> -
>>> -static void lvds_encoder_disable(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
>>> -
>>> -	drm_panel_disable(lvds->panel);
>>> -}
>>> -
>>> -static void lvds_encoder_post_disable(struct drm_bridge *bridge)
>>> -{
>>> -	struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge);
>>> -
>>> -	drm_panel_unprepare(lvds->panel);
>>> -}
>>> -
>>> -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = {
>>> -	.attach = lvds_encoder_attach,
>>> -	.detach = lvds_encoder_detach,
>>> -	.pre_enable = lvds_encoder_pre_enable,
>>> -	.enable = lvds_encoder_enable,
>>> -	.disable = lvds_encoder_disable,
>>> -	.post_disable = lvds_encoder_post_disable,
>>> -};
>>> -
>>>  static int lvds_encoder_probe(struct platform_device *pdev)
>>>  {
>>> -	struct lvds_encoder *lvds;
>>>  	struct device_node *port;
>>>  	struct device_node *endpoint;
>>> -	struct device_node *panel;
>>> -
>>> -	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
>>> -	if (!lvds)
>>> -		return -ENOMEM;
>>> -
>>> -	lvds->dev = &pdev->dev;
>>> -	platform_set_drvdata(pdev, lvds);
>>> -
>>> -	lvds->bridge.funcs = &lvds_encoder_bridge_funcs;
>>> -	lvds->bridge.of_node = pdev->dev.of_node;
>>> +	struct device_node *panel_node;
>>> +	struct drm_panel *panel;
>>> +	struct drm_bridge *bridge;
>>>
>>>  	/* Locate the panel DT node. */
>>>  	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
>>> @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>>>  		return -ENXIO;
>>>  	}
>>>
>>> -	panel = of_graph_get_remote_port_parent(endpoint);
>>> +	panel_node = of_graph_get_remote_port_parent(endpoint);
>>>  	of_node_put(endpoint);
>>> -	if (!panel) {
>>> +	if (!panel_node) {
>>>  		dev_dbg(&pdev->dev, "no remote endpoint for port 1\n");
>>>  		return -ENXIO;
>>>  	}
>>>
>>> -	lvds->panel = of_drm_find_panel(panel);
>>> -	of_node_put(panel);
>>> -	if (!lvds->panel) {
>>> +	panel = of_drm_find_panel(panel_node);
>>> +	of_node_put(panel_node);
>>> +	if (!panel) {
>>>  		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
>>>  		return -EPROBE_DEFER;
>>>  	}
>>>
>>> -	/* Register the bridge. */
>>> -	return drm_bridge_add(&lvds->bridge);
>>> +	bridge = drm_panel_bridge_add(&pdev->dev, panel,
>>> +				      DRM_MODE_CONNECTOR_LVDS);
>>> +	if (IS_ERR(bridge))
>>> +		return PTR_ERR(bridge);
>>> +
>>> +	platform_set_drvdata(pdev, bridge);
>>> +
>>> +	return 0;
>>>  }
>>>
>>>  static int lvds_encoder_remove(struct platform_device *pdev)
>>>  {
>>> -	struct lvds_encoder *encoder = platform_get_drvdata(pdev);
>>> +	struct drm_bridge *bridge = platform_get_drvdata(pdev);
>>>
>>> -	drm_bridge_remove(&encoder->bridge);
>>> +	drm_bridge_remove(bridge);
>>>
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>>> new file mode 100644
>>> index 000000000000..2081245455c6
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/panel.c
>>> @@ -0,0 +1,188 @@
>>> +/*
>>> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@...asonboard.com>
>>> + * Copyright (C) 2017 Broadcom
>>> + *
>>> + * 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 <drm/drmP.h>
>>> +#include <drm/drm_panel.h>
>>> +#include <drm/drm_atomic_helper.h>
>>> +#include <drm/drm_connector.h>
>>> +#include <drm/drm_crtc_helper.h>
>>> +#include <drm/drm_encoder.h>
>>> +#include <drm/drm_modeset_helper_vtables.h>
>>> +#include <drm/drm_panel.h>
>>> +
>>> +struct panel_bridge {
>>> +	struct drm_bridge bridge;
>>> +	struct device *dev;
>>> +	struct drm_connector connector;
>>> +	struct drm_panel *panel;
>>> +	u32 connector_type;
>>> +};
>>> +
>>> +static inline struct panel_bridge *
>>> +drm_bridge_to_panel_bridge(struct drm_bridge *bridge)
>>> +{
>>> +	return container_of(bridge, struct panel_bridge, bridge);
>>> +}
>>> +
>>> +static inline struct panel_bridge *
>>> +drm_connector_to_panel_bridge(struct drm_connector *connector)
>>> +{
>>> +	return container_of(connector, struct panel_bridge, connector);
>>> +}
>>> +
>>> +static int panel_bridge_connector_get_modes(struct drm_connector *connector)
>>> +{
>>> +	struct panel_bridge *panel_bridge =
>>> +		drm_connector_to_panel_bridge(connector);
>>> +
>>> +	return drm_panel_get_modes(panel_bridge->panel);
>>> +}
>>> +
>>> +static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = {
>>> +	.get_modes = panel_bridge_connector_get_modes,
>>> +};
>>> +
>>> +static const struct drm_connector_funcs panel_bridge_connector_funcs = {
>>> +	.dpms = drm_atomic_helper_connector_dpms,
>>> +	.reset = drm_atomic_helper_connector_reset,
>>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>>> +	.destroy = drm_connector_cleanup,
>>> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> +};
>>> +
>>> +static int panel_bridge_attach(struct drm_bridge *bridge)
>>> +{
>>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>> +	struct drm_connector *connector = &panel_bridge->connector;
>>> +	int ret;
>>> +
>>> +	if (!bridge->encoder) {
>>> +		DRM_ERROR("Missing encoder\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	drm_connector_helper_add(connector,
>>> +				 &panel_bridge_connector_helper_funcs);
>>> +
>>> +	ret = drm_connector_init(bridge->dev, connector,
>>> +				 &panel_bridge_connector_funcs,
>>> +				 panel_bridge->connector_type);
>>> +	if (ret) {
>>> +		DRM_ERROR("Failed to initialize connector\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	drm_mode_connector_attach_encoder(&panel_bridge->connector,
>>> +					  bridge->encoder);
>>> +
>>> +	ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void panel_bridge_detach(struct drm_bridge *bridge)
>>> +{
>>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>> +
>>> +	drm_panel_detach(panel_bridge->panel);
>>> +}
>>> +
>>> +static void panel_bridge_pre_enable(struct drm_bridge *bridge)
>>> +{
>>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>> +
>>> +	drm_panel_prepare(panel_bridge->panel);
>>> +}
>>> +
>>> +static void panel_bridge_enable(struct drm_bridge *bridge)
>>> +{
>>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>> +
>>> +	drm_panel_enable(panel_bridge->panel);
>>> +}
>>> +
>>> +static void panel_bridge_disable(struct drm_bridge *bridge)
>>> +{
>>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>> +
>>> +	drm_panel_disable(panel_bridge->panel);
>>> +}
>>> +
>>> +static void panel_bridge_post_disable(struct drm_bridge *bridge)
>>> +{
>>> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>>> +
>>> +	drm_panel_unprepare(panel_bridge->panel);
>>> +}
>>> +
>>> +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
>>> +	.attach = panel_bridge_attach,
>>> +	.detach = panel_bridge_detach,
>>> +	.pre_enable = panel_bridge_pre_enable,
>>> +	.enable = panel_bridge_enable,
>>> +	.disable = panel_bridge_disable,
>>> +	.post_disable = panel_bridge_post_disable,
>>> +};
>>> +
>>> +/**
>>> + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that
>>> + * just call the appropriate functions from drm_panel.
>>> + *
>>> + * @dev: The struct device of the panel device.  This is used for
>>> + * registering the drm_bridge.
>>> + * @panel: The drm_panel being wrapped.  Must be non-NULL.
>>> + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be
>>> + * created.
>>> + *
>>> + * For drivers converting from directly using drm_panel: The expected
>>> + * usage pattern is that during either encoder module probe or DSI
>>> + * host attach, a drm_panel will be looked up through
>>> + * drm_of_find_panel_or_bridge().  drm_panel_bridge_add() is used to
>>> + * wrap that panel in the new bridge, and the result can then be
>>> + * passed to drm_bridge_attach().  The drm_panel_prepare() and related
>>> + * functions can be dropped from the encoder driver (they're now
>>> + * called by the KMS helpers before calling into the encoder), along
>>> + * with connector creation.
>>
>> +panel/bridge reviewers.
>>
>> This does make things much cleaner, but it seems a bit strange to create
>> a drm_bridge when there isn't really a HW bridge in the display chain (i.e,
>> when the DSI encoder is directly connected to a DSI panel).
>>
>> There are kms drivers that use drm_panel, but don't have simple stub connectors
>> that wrap around a drm_panel. They have more complicated connector ops, and
>> may call drm_panel_prepare() and related functions a bit differently. We won't
>> be able to use drm_panel_bridge for those drivers.
>
> Can you point to examples?
>
> The only thing on my mind that would be missing if drivers converted to
> bridges only is having your encoder be able to filter the mode list for
> timing limits.  That seems like something you'd want any link in the
> display chain to be able to do, so we may want to extend drm_bridge for
> handling that.

Yeah, that's one of the reasons, and probably the most important one since
the majority of the kms drivers use the connector's mode_valid() to check
their encoder/crtc related mode constraints.

Some drivers subclass the connector atomic state, this would require their
own versions of connector ops to duplicate, copy, destroy their atomic
state, which would be hard to do if connector creation was moved outside
of the kms driver. I see at least the tegra DSI driver do that. It's another
question whether that stuff should belong to the connector state or elsewhere.

The msm DSI driver has special connector ops that does some tricks to ensure
a dual DSI link panel is configured properly. In this case, there are 2
connectors created, but only one drm_panel. We also need to configure some
msm components to work in dual DSI mode, this is done as callbacks from the
connector ops. That being said, I think we could move some of this from the kms
driver to drm_panel_bridge() as a special case.

About the usage of drm_panel_prepare() and other funcs, the majority of the
users mostly do:

encoder_enable_helper_func() {
	drm_panel_prepare();

	/* do stuff to actually enable the encoder */

	drm_panel_enable();
}

The above usage can be replaced by creating a drm_panel_bridge(), but other
types of usage would mess up the sequence a bit. For example, rockchip's
dw-mipi-dsi seems to something like do:

dw_mipi_dsi_encoder_enable() {

	/* some DSI encoder preparation, configure DSI in command mode */

	drm_panel_prepare();

	/* configure DSI in video mode */

	drm_panel_enable();

	/* some more DSI configuration */
}

It's possible that we can adjust these non-complying cases without breaking
functionality, but it would need some testing.

>
> Maybe you're thinking of something else, though.
>
>> For msm, we check whether the DSI encoder is connected directly to a panel
>> or an external bridge. If it's connected to an external bridge, we skip the
>> creation of the stub connector, and rely on the external bridge driver to
>> create the connector:
>>
>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227
>>
>> The msm solution isn't very neat, but it avoids the need to create another
>> bridge to glue things together.
>
> Yeah, that's basically the code I was going to have to replicate, which
> is why I wrote a generic helper instead.  I think allocating a
> drm_bridge (which isn't visible to userspace) is a small price to pay
> for cutting 100 lines of boilerplate from a driver.

That sounds fair enough, I guess.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists