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

[PATCH v2 0/2] drm/tegra: Fix CRTC association 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                         | 41 ++++++++++++++++++++--
 drivers/gpu/drm/tegra/rgb.c                        |  2 +-
 3 files changed, 43 insertions(+), 3 deletions(-)

-- 
1.8.4.2

[PATCH v2 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>
---
This depends on the following patch:

	[v3,1/3] drm: provide a helper for the encoder possible_crtcs mask

a copy of which can be found here:

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

 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 v2 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.

By explicitly specifying the head number in the device tree, it is no
longer necessary to rely on these assumptions. As a fallback, if the
property isn't available, derive the head number from the display
controller node's position in the device tree. That's somewhat more
reliable than the previous default but not a proper solution.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- if the nvidia,head property isn't present, find the position of the
  display controller within the DT and derive the head number from it

 .../bindings/gpu/nvidia,tegra20-host1x.txt         |  3 ++
 drivers/gpu/drm/tegra/dc.c                         | 41 ++++++++++++++++++++--
 2 files changed, 42 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..9336006b475d 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);
@@ -1187,6 +1185,41 @@ static const struct of_device_id tegra_dc_of_match[] = {
 	}
 };
 
+static int tegra_dc_parse_dt(struct tegra_dc *dc)
+{
+	struct device_node *np;
+	u32 value = 0;
+	int err;
+
+	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
+	if (err < 0) {
+		dev_err(dc->dev, "missing \"nvidia,head\" property\n");
+
+		/*
+		 * If the nvidia,head property isn't present, try to find the
+		 * correct head number by looking up the position of this
+		 * display controller's node within the device tree. Assuming
+		 * that the nodes are ordered properly in the DTS file and
+		 * that the translation into a flattened device tree blob
+		 * preserves that ordering this will actually yield the right
+		 * head number.
+		 *
+		 * If those assumptions don't hold, this will still work for
+		 * cases where only a single display controller is used.
+		 */
+		for_each_matching_node(np, tegra_dc_of_match) {
+			if (np == dc->dev->of_node)
+				break;
+
+			value++;
+		}
+	}
+
+	dc->pipe = value;
+
+	return 0;
+}
+
 static int tegra_dc_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *id;
@@ -1207,6 +1240,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 v2 2/2] drm/tegra: Obtain head number from DT

[Stephen Warren]

On 01/14/2014 07:45 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.
> 
> By explicitly specifying the head number in the device tree, it is no
> longer necessary to rely on these assumptions. As a fallback, if the
> property isn't available, derive the head number from the display
> controller node's position in the device tree. That's somewhat more
> reliable than the previous default but not a proper solution.

The series,
Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This patch should really have been sent to the DT maintainers and list
since it changes a DT binding...

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

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

If of_property_read_u32() returns an error, does it guarantee that value
is left unchanged? I suspect it'd be safer to add ...

> +	if (err < 0) {
> +		dev_err(dc->dev, "missing \"nvidia,head\" property\n");
> +
> +		/*
> +		 * If the nvidia,head property isn't present, try to find the
> +		 * correct head number by looking up the position of this
> +		 * display controller's node within the device tree. Assuming
> +		 * that the nodes are ordered properly in the DTS file and
> +		 * that the translation into a flattened device tree blob
> +		 * preserves that ordering this will actually yield the right
> +		 * head number.
> +		 *
> +		 * If those assumptions don't hold, this will still work for
> +		 * cases where only a single display controller is used.
> +		 */

... "value = 0;" here?

> +		for_each_matching_node(np, tegra_dc_of_match) {
> +			if (np == dc->dev->of_node)
> +				break;
> +
> +			value++;
> +		}
> +	}
> +
> +	dc->pipe = value;

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

[Thierry Reding]

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

On Tue, Jan 14, 2014 at 10:53:19AM -0700, Stephen Warren wrote:
> On 01/14/2014 07:45 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.
> > 
> > By explicitly specifying the head number in the device tree, it is no
> > longer necessary to rely on these assumptions. As a fallback, if the
> > property isn't available, derive the head number from the display
> > controller node's position in the device tree. That's somewhat more
> > reliable than the previous default but not a proper solution.
> 
> The series,
> Tested-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This patch should really have been sent to the DT maintainers and list
> since it changes a DT binding...

Indeed. I'll resend this to the appropriate people and lists. Sorry
about that.

> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> 
> > +static int tegra_dc_parse_dt(struct tegra_dc *dc)
> > +{
> > +	struct device_node *np;
> > +	u32 value = 0;
> > +	int err;
> > +
> > +	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
> 
> If of_property_read_u32() returns an error, does it guarantee that value
> is left unchanged? I suspect it'd be safer to add ...

That's the way it's always been at least. of_property_read_u32() ends up
calling of_property_read_u32_array(), which looking at the code only
modifies the out_values parameter when it knows that it will succeed.

Furthermore the function's kernel-doc explicitly says that "out_values
is modified only if a valid u32 value can be decoded" (i.e. on success).

Thierry

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

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

[Stephen Warren]

On 01/15/2014 02:06 AM, Thierry Reding wrote:
> On Tue, Jan 14, 2014 at 10:53:19AM -0700, Stephen Warren wrote:
>> On 01/14/2014 07:45 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.
>>>
>>> By explicitly specifying the head number in the device tree, it is no
>>> longer necessary to rely on these assumptions. As a fallback, if the
>>> property isn't available, derive the head number from the display
>>> controller node's position in the device tree. That's somewhat more
>>> reliable than the previous default but not a proper solution.

>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>
>>> +static int tegra_dc_parse_dt(struct tegra_dc *dc)
>>> +{
>>> +	struct device_node *np;
>>> +	u32 value = 0;
>>> +	int err;
>>> +
>>> +	err = of_property_read_u32(dc->dev->of_node, "nvidia,head", &value);
>>
>> If of_property_read_u32() returns an error, does it guarantee that value
>> is left unchanged? I suspect it'd be safer to add ...
> 
> That's the way it's always been at least. of_property_read_u32() ends up
> calling of_property_read_u32_array(), which looking at the code only
> modifies the out_values parameter when it knows that it will succeed.
> 
> Furthermore the function's kernel-doc explicitly says that "out_values
> is modified only if a valid u32 value can be decoded" (i.e. on success).

OK, that last bit is the important part. So, this is fine.