devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Inki Dae <inki.dae@samsung.com>
Cc: Sam Ravnborg <sam@ravnborg.org>,
	Jagan Teki <jagan@amarulasolutions.com>,
	Joonyoung Shim <jy0922.shim@samsung.com>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Robert Foss <robert.foss@linaro.org>,
	Frieder Schrempf <frieder.schrempf@kontron.de>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Marek Vasut <marex@denx.de>,
	Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
	Fabio Estevam <festevam@gmail.com>,
	devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
	dri-devel@lists.freedesktop.org,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-amarula <linux-amarula@amarulasolutions.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH 06/17] drm/exynos: dsi: Handle exynos specifics via driver_data
Date: Fri, 13 Aug 2021 15:16:33 +0300	[thread overview]
Message-ID: <YRZioZvYrU99fQxJ@pendragon.ideasonboard.com> (raw)
In-Reply-To: <79ef7f71-b167-2368-e0fd-d4ccaee596c2@samsung.com>

Hi Inki,

On Fri, Aug 13, 2021 at 03:50:56PM +0900, Inki Dae wrote:
> 21. 7. 26. 오전 2:25에 Sam Ravnborg 이(가) 쓴 글:
> > On Sun, Jul 04, 2021 at 02:32:19PM +0530, Jagan Teki wrote:
> >> Exynos DSI driver is actually a Samsung MIPI DSIM bridge
> >> IP which is also used in i.MX8MM platforms.
> >>
> >> Right now the existing driver has some exynos drm specific
> >> code bases like te_irq, crtc and component_ops.
> >>
> >> In order to switch this driver into a common bridge driver
> >> We can see 2 options to handle the exynos specific code.
> >>
> >> A. Drop the component_ops, and rework other specifics.
> >>    This may lead to more foundation work as it requires
> >>    more changes in exynos drm drivers stack.
> >>
> >> B. Handle the exynos specifics via driver data, and make
> >>    the common bridge work in different platforms and plan
> >>    for option A in future.
> >>
> >> So, this patch is trying to add option B) changes to handle
> >> exynos specifics via driver_data.
> > 
> > We really should find someone that has the time, energy, knowledge and
> > hardware that can include device_link support once anf for all for
> > bridges.
> > Then we would avoid hacks like this.
> > 
> > I see no other options at the moment, but look forward for a better
> > solution.
> 
> I'm not sure that it's correct to share this mipi dsi driver with
> I.MX8MM SoC even though it's a same IP because this MIPI DSI device
> isn't peripheral device but in SoC.
>
> It would mean that Exynos MIPI DSI device depends on SoC architecture,
> and Exynos and I.MX series are totally different SoC. So if we share
> the same driver for the MIPI DSI device then many things we didn't
> predict may happen in the future.

Isn't that true for external components true thought ? Any driver shared
by multiple systems will face this issue, where it will be developed
with some use cases in mind, and regressions may happen when the driver
is then extended to support other use cases not required for the
original platform.

In general we don't want multiple drivers for the same IP core unless
there are valid technical reasons for that. It's the whole point of the
device tree, being able to describe how IP cores are integrated, so that
code can be reused across platforms. Of course, integration differences
between SoCs can sometimes vary wildly and require some amount of glue
code.

> I don't want to make Jagan's efforts
> in vain for the community but clarify whether this is correct way or
> not. If this is only the way we have to go then we could more focus on
> actual solution not such hack. Impossible work with Jagan alone I
> think.

I do agree that we need more correct solutions and less hacks in general
:-) The issues faced on Exynos also exist on other platforms, so it
would be much better to solve them well once that duplicating
implementations with less test coverage and reviews. There have been
efforts in the past to address some of these issues, which have resulted
in solutions such as the component framework. However, I'd argued that
we've never taken it to the last step, and have always stopped with half
solutions. The component framework, for instance, is painful to use, and
the handling of .remove() in most drivers is completely broken because
of that (not just because of that though, we have issues in the DRM core
that make hot-unplug just impossible to handle safetly).

> So let's get started with a question,
> - Is MIPI-DSI bridge device or Encoder device? I think that MIPI-DSI
> is a Encoder device managed by atomic KMS. If MIPI-DSI should be
> handled as bridge device then does now drm bridge framework provide
> everything to share one driver with one more SoC? I mean something
> that drm bridge has to consider for such driver support, which is
> shared with one more SoC.

The DRM "encoder" concept was a bit of a historical mistake that we are
stuck with as drm_encoder is exposed to userspace. It comes from a time
where nobody was envisioning chaining multiple encoders. DRM is moving
to modelling every component after the CRTC as a bridge. This brings
much more flexibility, and in that model, the drm_encoder becomes more
or less a stub.

The DRM bridge API has been extended in the past to support more
features, and if anything is still missing that makes it difficult to
move away from drm_encoder, we can of course address the issues in
drm_bridge.

> And Display mode - VIDEO and COMMAND mode - is generic type of MIPI
> DSI, and also componentised subsystem is a generic solution to resolve
> probing order issue not Exynos specific feature. These are driver
> specific ones not Exynos SoC I think. As SoC specific things should be
> considered, I think MIPI DSI Driver - interrupt handler and probing
> order things are really specific to device driver - should be
> separated but we could share the control part of the device.
> 
> I was busy with other projects so didn't care of Linux DRM world so
> there may be my missing something.
> 
> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >> ---
> >>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 37 +++++++++++++++++++------
> >>  1 file changed, 29 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >> index 99a1b8c22313..53d878d4d2d7 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >> @@ -250,6 +250,7 @@ struct exynos_dsi_driver_data {
> >>  	unsigned int wait_for_reset;
> >>  	unsigned int num_bits_resol;
> >>  	const unsigned int *reg_values;
> >> +	bool exynos_specific;
> >>  };
> >>  
> >>  struct exynos_dsi {
> >> @@ -459,6 +460,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
> >>  	.wait_for_reset = 1,
> >>  	.num_bits_resol = 11,
> >>  	.reg_values = reg_values,
> >> +	.exynos_specific = true,
> >>  };
> >>  
> >>  static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
> >> @@ -471,6 +473,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
> >>  	.wait_for_reset = 1,
> >>  	.num_bits_resol = 11,
> >>  	.reg_values = reg_values,
> >> +	.exynos_specific = true,
> >>  };
> >>  
> >>  static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
> >> @@ -481,6 +484,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
> >>  	.wait_for_reset = 1,
> >>  	.num_bits_resol = 11,
> >>  	.reg_values = reg_values,
> >> +	.exynos_specific = true,
> >>  };
> >>  
> >>  static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
> >> @@ -492,6 +496,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
> >>  	.wait_for_reset = 0,
> >>  	.num_bits_resol = 12,
> >>  	.reg_values = exynos5433_reg_values,
> >> +	.exynos_specific = true,
> >>  };
> >>  
> >>  static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
> >> @@ -503,6 +508,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
> >>  	.wait_for_reset = 1,
> >>  	.num_bits_resol = 12,
> >>  	.reg_values = exynos5422_reg_values,
> >> +	.exynos_specific = true,
> >>  };
> >>  
> >>  static const struct of_device_id exynos_dsi_of_match[] = {
> >> @@ -1484,7 +1490,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> >>  	 * If attached panel device is for command mode one, dsi should register
> >>  	 * TE interrupt handler.
> >>  	 */
> >> -	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> >> +	if (dsi->driver_data->exynos_specific &&
> >> +	    !(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> >>  		int ret = exynos_dsi_register_te_irq(dsi, &device->dev);
> >>  		if (ret)
> >>  			return ret;
> >> @@ -1495,8 +1502,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> >>  	dsi->lanes = device->lanes;
> >>  	dsi->format = device->format;
> >>  	dsi->mode_flags = device->mode_flags;
> >> -	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
> >> -			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
> >> +	if (dsi->driver_data->exynos_specific)
> >> +		exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
> >> +					    !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
> >>  
> >>  	mutex_unlock(&drm->mode_config.mutex);
> >>  
> >> @@ -1515,7 +1523,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
> >>  	if (drm->mode_config.poll_enabled)
> >>  		drm_kms_helper_hotplug_event(drm);
> >>  
> >> -	exynos_dsi_unregister_te_irq(dsi);
> >> +	if (dsi->driver_data->exynos_specific)
> >> +		exynos_dsi_unregister_te_irq(dsi);
> >>  
> >>  	return 0;
> >>  }
> >> @@ -1737,6 +1746,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> +	if (!dsi->driver_data->exynos_specific) {
> >> +		ret = mipi_dsi_host_register(&dsi->dsi_host);
> >> +		if (ret) {
> >> +			dev_err(dev, "failed to register mipi dsi host: %d\n",
> >> +				ret);
> >> +			return ret;
> >> +		}
> >> +	}
> >> +
> >>  	platform_set_drvdata(pdev, dsi);
> >>  
> >>  	pm_runtime_enable(dev);
> >> @@ -1747,9 +1765,11 @@ static int exynos_dsi_probe(struct platform_device *pdev)
> >>  
> >>  	drm_bridge_add(&dsi->bridge);
> >>  
> >> -	ret = component_add(dev, &exynos_dsi_component_ops);
> >> -	if (ret)
> >> -		goto err_disable_runtime;
> >> +	if (dsi->driver_data->exynos_specific) {
> >> +		ret = component_add(dev, &exynos_dsi_component_ops);
> >> +		if (ret)
> >> +			goto err_disable_runtime;
> >> +	}
> >>  
> >>  	return 0;
> >>  
> >> @@ -1767,7 +1787,8 @@ static int exynos_dsi_remove(struct platform_device *pdev)
> >>  
> >>  	pm_runtime_disable(&pdev->dev);
> >>  
> >> -	component_del(&pdev->dev, &exynos_dsi_component_ops);
> >> +	if (dsi->driver_data->exynos_specific)
> >> +		component_del(&pdev->dev, &exynos_dsi_component_ops);
> >>  
> >>  	return 0;
> >>  }

-- 
Regards,

Laurent Pinchart

  reply	other threads:[~2021-08-13 12:16 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-04  9:02 [RFC PATCH 00/17] drm: bridge: Samsung MIPI DSIM bridge Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 01/17] drm/exynos: dsi: Convert to bridge driver Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 02/17] drm/exynos: dsi: Handle drm_device for bridge Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 03/17] drm/exynos: dsi: Use the drm_panel_bridge API Jagan Teki
2021-07-05 11:47   ` Marek Szyprowski
2021-07-05 12:00     ` Jagan Teki
2021-07-05 12:13       ` Marek Szyprowski
2021-07-05 12:34         ` Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 04/17] drm/exynos: dsi: Create bridge connector for encoder Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 05/17] drm/exynos: dsi: Get the mode from bridge Jagan Teki
2021-07-29 13:20   ` Robert Foss
2021-07-04  9:02 ` [RFC PATCH 06/17] drm/exynos: dsi: Handle exynos specifics via driver_data Jagan Teki
     [not found]   ` <YP2el40V3K4R7ner@ravnborg.org>
2021-07-25 17:31     ` Jagan Teki
2021-08-13  6:50     ` Inki Dae
2021-08-13 12:16       ` Laurent Pinchart [this message]
2021-08-18  6:09         ` Inki Dae
2021-07-04  9:02 ` [RFC PATCH 07/17] drm: bridge: Move exynos_drm_dsi into bridges Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 08/17] dt-bindings: display: bridge: Add Samsung MIPI DSIM bridge Jagan Teki
2021-07-12 15:13   ` Rob Herring
2021-07-12 15:23     ` Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 09/17] drm: bridge: samsung-dsim: Add module init, exit Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 10/17] drm: bridge: samsung-dsim: Update the of_node for port(s) Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 11/17] drm: bridge: samsung-dsim: Find the possible DSI devices Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 12/17] dt-bindings: display: bridge: samsung,mipi-dsim: Add i.MX8MM support Jagan Teki
2021-07-14 22:59   ` Rob Herring
2021-07-04  9:02 ` [RFC PATCH 13/17] drm: bridge: samsung-dsim: " Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 14/17] drm: bridge: samsung-dsim: Add input_bus_flags Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 15/17] drm: bridge: samsung-dsim: Move DSI init in bridge enable Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 16/17] drm: bridge: samsung-dsim: Fix PLL_P offset Jagan Teki
2021-07-04  9:02 ` [RFC PATCH 17/17] drm: bridge: samsung-dsim: Add bridge mode_fixup Jagan Teki
     [not found] ` <YP2ZvoVQyvwTXP++@ravnborg.org>
2021-07-25 17:13   ` [RFC PATCH 00/17] drm: bridge: Samsung MIPI DSIM bridge Jagan Teki
2021-10-05 21:43     ` Tim Harvey
2021-12-09  8:36       ` Michael Nazzareno Trimarchi
2021-12-09 16:40         ` Tim Harvey
2021-12-09 17:09           ` Michael Nazzareno Trimarchi
2021-12-09 17:57             ` Tim Harvey
2021-12-09 20:24             ` Lucas Stach
2021-12-09 21:24               ` Michael Nazzareno Trimarchi
2021-12-15 13:34                 ` Adam Ford

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YRZioZvYrU99fQxJ@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=daniel.vetter@intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=festevam@gmail.com \
    --cc=frieder.schrempf@kontron.de \
    --cc=inki.dae@samsung.com \
    --cc=jagan@amarulasolutions.com \
    --cc=jy0922.shim@samsung.com \
    --cc=krzysztof.kozlowski@canonical.com \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-amarula@amarulasolutions.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=marex@denx.de \
    --cc=narmstrong@baylibre.com \
    --cc=robert.foss@linaro.org \
    --cc=sam@ravnborg.org \
    --cc=sw0312.kim@samsung.com \
    --subject='Re: [RFC PATCH 06/17] drm/exynos: dsi: Handle exynos specifics via driver_data' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).