[PATCH 0/2] drm/tegra: Fix CRTC associated with outputs

[PATCH 0/2] drm/tegra: Fix CRTC associated with outputs

[Thierry Reding]

The Tegra DRM driver assumes that CRTCs are probed in the order listed
in the DT. While DT makes no such guarantees in the first place, this
used to work fine. With the introduction of the panel support, however,
more often than not one of the CRTCs will defer probing (caused by the
panel not having been registered yet) and the assumptions are broken.

To fix this, we use the new drm_crtc_mask() function to obtain the mask
of the display controller that an RGB output can be associated with,
thereby making it resistant to changes in probe order.

A second patch is required to obtain the number of the head from the
device tree, since we can no longer rely on the probe order providing us
with the right one.

Thierry

Thierry Reding (2):
  drm/tegra: Fix possible CRTC mask for RGB outputs
  drm/tegra: Obtain head number from DT

 .../bindings/gpu/nvidia,tegra20-host1x.txt           |  3 +++
 drivers/gpu/drm/tegra/dc.c                           | 20 ++++++++++++++++++--
 drivers/gpu/drm/tegra/rgb.c                          |  2 +-
 3 files changed, 22 insertions(+), 3 deletions(-)

-- 
1.8.4.2

[PATCH 1/2] drm/tegra: Fix possible CRTC mask for RGB outputs

[Thierry Reding]

The mask of possible CRTCs that an output (DRM encoder) can be attached
to is relative to the position within the DRM device's list of CRTCs.
Deferred probing can cause this to not match the pipe number associated
with a CRTC. Use the newly introduced drm_crtc_mask() to compute the
mask by looking up the proper index of the given CRTC in the list.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/rgb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 03885bb8dcc0..338f7f6561d7 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -258,7 +258,7 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
 	 * RGB outputs are an exception, so we make sure they can be attached
 	 * to only their parent display controller.
 	 */
-	rgb->output.encoder.possible_crtcs = 1 << dc->pipe;
+	rgb->output.encoder.possible_crtcs = drm_crtc_mask(&dc->base);
 
 	return 0;
 }
-- 
1.8.4.2

[PATCH 2/2] drm/tegra: Obtain head number from DT

[Thierry Reding]

The head number of a given display controller is fixed in hardware and
required to program outputs appropriately. Relying on the driver probe
order to determine this number will not work, since that could yield a
situation where the second head was probed first and would be assigned
head number 0 instead of 1.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/gpu/nvidia,tegra20-host1x.txt           |  3 +++
 drivers/gpu/drm/tegra/dc.c                           | 20 ++++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
index 9e9008f8fa32..efaeec8961b6 100644
--- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
@@ -118,6 +118,9 @@ of the following host1x client modules:
     See ../reset/reset.txt for details.
   - reset-names: Must include the following entries:
     - dc
+  - nvidia,head: The number of the display controller head. This is used to
+    setup the various types of output to receive video data from the given
+    head.
 
   Each display controller node has a child node, named "rgb", that represents
   the RGB output associated with the controller. It can take the following
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 386f3b4b0094..ce0d2c2c7aac 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1100,8 +1100,6 @@ static int tegra_dc_init(struct host1x_client *client)
 	struct tegra_dc *dc = host1x_client_to_dc(client);
 	int err;
 
-	dc->pipe = tegra->drm->mode_config.num_crtc;
-
 	drm_crtc_init(tegra->drm, &dc->base, &tegra_crtc_funcs);
 	drm_mode_crtc_set_gamma_size(&dc->base, 256);
 	drm_crtc_helper_add(&dc->base, &tegra_crtc_helper_funcs);
@@ -1160,6 +1158,20 @@ static const struct host1x_client_ops dc_client_ops = {
 	.exit = tegra_dc_exit,
 };
 
+static int tegra_dc_parse_dt(struct tegra_dc *dc)
+{
+	u32 value;
+	int err;
+
+	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
+	if (err < 0)
+		return err;
+
+	dc->pipe = value;
+
+	return 0;
+}
+
 static const struct tegra_dc_soc_info tegra20_dc_soc_info = {
 	.supports_interlacing = false,
 };
@@ -1207,6 +1219,10 @@ static int tegra_dc_probe(struct platform_device *pdev)
 	dc->dev = &pdev->dev;
 	dc->soc = id->data;
 
+	err = tegra_dc_parse_dt(dc);
+	if (err < 0)
+		return err;
+
 	dc->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dc->clk)) {
 		dev_err(&pdev->dev, "failed to get clock\n");
-- 
1.8.4.2

Re: [PATCH 1/2] drm/tegra: Fix possible CRTC mask for RGB outputs

[Stephen Warren]

On 01/13/2014 07:21 AM, Thierry Reding wrote:
> The mask of possible CRTCs that an output (DRM encoder) can be attached
> to is relative to the position within the DRM device's list of CRTCs.
> Deferred probing can cause this to not match the pipe number associated
> with a CRTC. Use the newly introduced drm_crtc_mask() to compute the
> mask by looking up the proper index of the given CRTC in the list.

> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c

> @@ -258,7 +258,7 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)

> -	rgb->output.encoder.possible_crtcs = 1 << dc->pipe;
> +	rgb->output.encoder.possible_crtcs = drm_crtc_mask(&dc->base);

For me, on top of either next-20140109 or next-20140113, this causes:

> drivers/gpu/drm/tegra/rgb.c: In function ‘tegra_dc_rgb_init’:
> drivers/gpu/drm/tegra/rgb.c:261:2: error: implicit declaration of function ‘drm_crtc_mask’ [-Werror=implicit-function-declaration]

Re: [PATCH 2/2] drm/tegra: Obtain head number from DT

[Stephen Warren]

On 01/13/2014 07:21 AM, Thierry Reding wrote:
> The head number of a given display controller is fixed in hardware and
> required to program outputs appropriately. Relying on the driver probe
> order to determine this number will not work, since that could yield a
> situation where the second head was probed first and would be assigned
> head number 0 instead of 1.

This change makes the new properties mandatory, yet they aren't part of
the DT files yet. So, won't this patch break all display on Tegra?

To avoid having to modify the Tegra DTs in this patch, can't the code
fall back to the existing broken algorithm if the property is missing, i.e.:

> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c

> -	dc->pipe = tegra->drm->mode_config.num_crtc;

Instead,:

	if (dc->pipe == -1)
		dc->pipe = tegra->drm->mode_config.num_crtc;

> +static int tegra_dc_parse_dt(struct tegra_dc *dc)
> +{
> +	u32 value;
> +	int err;
> +
> +	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
> +	if (err < 0)
> +		return err;
> +
> +	dc->pipe = value;

Instead:

	err = ...
	if (!err)
		dc->pipe = value;
	else
		/* Perhaps also emit an error message here */
		dc->pipe = -1;

Re: [PATCH 1/2] drm/tegra: Fix possible CRTC mask for RGB outputs

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On Mon, Jan 13, 2014 at 10:44:26AM -0700, Stephen Warren wrote:
> On 01/13/2014 07:21 AM, Thierry Reding wrote:
> > The mask of possible CRTCs that an output (DRM encoder) can be attached
> > to is relative to the position within the DRM device's list of CRTCs.
> > Deferred probing can cause this to not match the pipe number associated
> > with a CRTC. Use the newly introduced drm_crtc_mask() to compute the
> > mask by looking up the proper index of the given CRTC in the list.
> 
> > diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> 
> > @@ -258,7 +258,7 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
> 
> > -	rgb->output.encoder.possible_crtcs = 1 << dc->pipe;
> > +	rgb->output.encoder.possible_crtcs = drm_crtc_mask(&dc->base);
> 
> For me, on top of either next-20140109 or next-20140113, this causes:
> 
> > drivers/gpu/drm/tegra/rgb.c: In function ‘tegra_dc_rgb_init’:
> > drivers/gpu/drm/tegra/rgb.c:261:2: error: implicit declaration of function ‘drm_crtc_mask’ [-Werror=implicit-function-declaration]

It depends on a separate patch that I sent earlier yesterday. I thought
I had Cc'ed you and the linux-tegra mailing list on that patch, but I'm
misremembering apparently. Here's a link to the patch:

	https://patchwork.kernel.org/patch/3475421/

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] drm/tegra: Obtain head number from DT

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2586 bytes --]

On Mon, Jan 13, 2014 at 10:46:45AM -0700, Stephen Warren wrote:
> On 01/13/2014 07:21 AM, Thierry Reding wrote:
> > The head number of a given display controller is fixed in hardware and
> > required to program outputs appropriately. Relying on the driver probe
> > order to determine this number will not work, since that could yield a
> > situation where the second head was probed first and would be assigned
> > head number 0 instead of 1.
> 
> This change makes the new properties mandatory, yet they aren't part of
> the DT files yet. So, won't this patch break all display on Tegra?

I don't think it'll make anything worse than it currently is, since both
display controllers can't run at the same time with the current code.

They can do so on Dalmore, so I guess that would be broken by the patch.

> To avoid having to modify the Tegra DTs in this patch, can't the code
> fall back to the existing broken algorithm if the property is missing, i.e.:
> 
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> 
> > -	dc->pipe = tegra->drm->mode_config.num_crtc;
> 
> Instead,:
> 
> 	if (dc->pipe == -1)
> 		dc->pipe = tegra->drm->mode_config.num_crtc;
> 
> > +static int tegra_dc_parse_dt(struct tegra_dc *dc)
> > +{
> > +	u32 value;
> > +	int err;
> > +
> > +	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	dc->pipe = value;
> 
> Instead:
> 
> 	err = ...
> 	if (!err)
> 		dc->pipe = value;
> 	else
> 		/* Perhaps also emit an error message here */
> 		dc->pipe = -1;

Yeah, that should work. It's still suboptimal because we fallback to
something that's broken and known not to work.

My original proposal was to make the dc->pipe assignment depend on the
physical address of the display controller's registers. That's ugly,
but all SoCs in existence do use the very same offset. So we could
reason that for anything that's still using the old DTB files we can
rely on the physical address of the registers, while any new DTBs
should include the new nvidia,head property.

I have a feeling that you won't like it, though.

One other method would be to iterate over all DT nodes that match the
display controller compatible and use the index for the pipe number.
That should fix the current issues, but is still a wee bit hackish
because it assumes that nodes in DTB will be in the same order as in
the DTS. That has been true since the beginning of DT on Linux AFAIK
and therefore should be reasonably safe.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/tegra: Obtain head number from DT

[Stephen Warren]

On 01/14/2014 07:14 AM, Thierry Reding wrote:
> On Mon, Jan 13, 2014 at 10:46:45AM -0700, Stephen Warren wrote:
>> On 01/13/2014 07:21 AM, Thierry Reding wrote:
>>> The head number of a given display controller is fixed in hardware and
>>> required to program outputs appropriately. Relying on the driver probe
>>> order to determine this number will not work, since that could yield a
>>> situation where the second head was probed first and would be assigned
>>> head number 0 instead of 1.
>>
>> This change makes the new properties mandatory, yet they aren't part of
>> the DT files yet. So, won't this patch break all display on Tegra?
> 
> I don't think it'll make anything worse than it currently is, since both
> display controllers can't run at the same time with the current code.

Sure it will; it will prevent any dc device from probing at all:

> +static int tegra_dc_parse_dt(struct tegra_dc *dc)
...
> +	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
> +	if (err < 0)
 +		return err;
                ^^^^^^^^^^^
...
> @@ -1207,6 +1219,10 @@ static int tegra_dc_probe(struct platform_device *pdev)
...
> +	err = tegra_dc_parse_dt(dc);
> +	if (err < 0)
> +		return err;
                ^^^^^^^^^^^