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(-)
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@nvidia.com --- 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; }
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@nvidia.com --- 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");
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@nvidia.com
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;
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@nvidia.com
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
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.
dri-devel@lists.freedesktop.org
-
Stephen Warren -
Thierry Reding