From: Thierry Reding treding@nvidia.com
The Tegra DRM never actually took that lock, so the driver was broken until generic locking was added in commit:
commit b962a12050a387e4bbf3a48745afe1d29d396b0d Author: Rob Clark robdclark@gmail.com Date: Mon Oct 22 14:31:22 2018 +0200
drm/atomic: integrate modeset lock with private objects
It's now no longer necessary to take that lock, so drop the check.
v2: add rationale for why it is now safe to drop the check (Daniel)
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/hub.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c index 6aca0fd5a8e5..e56c0f7d3a13 100644 --- a/drivers/gpu/drm/tegra/hub.c +++ b/drivers/gpu/drm/tegra/hub.c @@ -615,11 +615,8 @@ static struct tegra_display_hub_state * tegra_display_hub_get_state(struct tegra_display_hub *hub, struct drm_atomic_state *state) { - struct drm_device *drm = dev_get_drvdata(hub->client.parent); struct drm_private_state *priv;
- WARN_ON(!drm_modeset_is_locked(&drm->mode_config.connection_mutex)); - priv = drm_atomic_get_private_obj_state(state, &hub->base); if (IS_ERR(priv)) return ERR_CAST(priv);
From: Thierry Reding treding@nvidia.com
Buffers that are imported from a DMA-BUF don't have pages allocated with them. At the same time an SG table for them can't be derived using the DMA API helpers because the necessary information doesn't exist. However there's already an SG table that was created during import, so this can simply be duplicated.
Signed-off-by: Thierry Reding treding@nvidia.com --- Note that for proper DMA-BUF usage, the display controllers should each attach to the DMA-BUF for imported buffers. That's a fairly large change and will be sent out as a follow-up, targetting v5.6. This patch is a fix for a recent regression in v5.5-rc1.
drivers/gpu/drm/tegra/gem.c | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 746dae32c484..6dfad56eee2b 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -27,6 +27,29 @@ static void tegra_bo_put(struct host1x_bo *bo) drm_gem_object_put_unlocked(&obj->gem); }
+/* XXX move this into lib/scatterlist.c? */ +static int sg_alloc_table_from_sg(struct sg_table *sgt, struct scatterlist *sg, + unsigned int nents, gfp_t gfp_mask) +{ + struct scatterlist *dst; + unsigned int i; + int err; + + err = sg_alloc_table(sgt, nents, gfp_mask); + if (err < 0) + return err; + + dst = sgt->sgl; + + for (i = 0; i < nents; i++) { + sg_set_page(dst, sg_page(sg), sg->length, 0); + dst = sg_next(dst); + sg = sg_next(sg); + } + + return 0; +} + static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo, dma_addr_t *phys) { @@ -52,11 +75,31 @@ static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo, return ERR_PTR(-ENOMEM);
if (obj->pages) { + /* + * If the buffer object was allocated from the explicit IOMMU + * API code paths, construct an SG table from the pages. + */ err = sg_alloc_table_from_pages(sgt, obj->pages, obj->num_pages, 0, obj->gem.size, GFP_KERNEL); if (err < 0) goto free; + } else if (obj->sgt) { + /* + * If the buffer object already has an SG table but no pages + * were allocated for it, it means the buffer was imported and + * the SG table needs to be copied to avoid overwriting any + * other potential users of the original SG table. + */ + err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl, obj->sgt->nents, + GFP_KERNEL); + if (err < 0) + goto free; } else { + /* + * If the buffer object had no pages allocated and if it was + * not imported, it had to be allocated with the DMA API, so + * the DMA API helper can be used. + */ err = dma_get_sgtable(dev, sgt, obj->vaddr, obj->iova, obj->gem.size); if (err < 0)
From: Thierry Reding treding@nvidia.com
All the display related blocks on Tegra require contiguous memory. Using the DMA API, there is no knowing at import time which device will end up using the buffer, so it's not known whether or not an IOMMU will be used to map the buffer.
Move the check for non-contiguous buffers/mappings to the tegra_dc_pin() function which is now the earliest point where it is known if a DMA BUF can be used by the given device or not.
v2: add check for contiguous buffer/mapping in tegra_dc_pin()
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/gem.c | 7 ------- drivers/gpu/drm/tegra/plane.c | 11 +++++++++++ 2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 6dfad56eee2b..bc15b430156d 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -440,13 +440,6 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm, err = tegra_bo_iommu_map(tegra, bo); if (err < 0) goto detach; - } else { - if (bo->sgt->nents > 1) { - err = -EINVAL; - goto detach; - } - - bo->iova = sg_dma_address(bo->sgt->sgl); }
bo->gem.import_attach = attach; diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c index 163b590be224..cadcdd9ea427 100644 --- a/drivers/gpu/drm/tegra/plane.c +++ b/drivers/gpu/drm/tegra/plane.c @@ -129,6 +129,17 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state) goto unpin; }
+ /* + * The display controller needs contiguous memory, so + * fail if the buffer is discontiguous and we fail to + * map its SG table to a single contiguous chunk of + * I/O virtual memory. + */ + if (err > 1) { + err = -EINVAL; + goto unpin; + } + state->iova[i] = sg_dma_address(sgt->sgl); state->sgt[i] = sgt; } else {
From: Thierry Reding treding@nvidia.com
The IOVA address for the cursor is the result of mapping the buffer object for the given display controller. Make sure to use the proper IOVA address as stored in the cursor's plane state.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/dc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index d03b33c3b114..0a5f86b61fda 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -847,16 +847,15 @@ static int tegra_cursor_atomic_check(struct drm_plane *plane, static void tegra_cursor_atomic_update(struct drm_plane *plane, struct drm_plane_state *old_state) { - struct tegra_bo *bo = tegra_fb_get_plane(plane->state->fb, 0); + struct tegra_plane_state *state = to_tegra_plane_state(plane->state); struct tegra_dc *dc = to_tegra_dc(plane->state->crtc); - struct drm_plane_state *state = plane->state; u32 value = CURSOR_CLIP_DISPLAY;
/* rien ne va plus */ if (!plane->state->crtc || !plane->state->fb) return;
- switch (state->crtc_w) { + switch (plane->state->crtc_w) { case 32: value |= CURSOR_SIZE_32x32; break; @@ -874,16 +873,16 @@ static void tegra_cursor_atomic_update(struct drm_plane *plane, break;
default: - WARN(1, "cursor size %ux%u not supported\n", state->crtc_w, - state->crtc_h); + WARN(1, "cursor size %ux%u not supported\n", + plane->state->crtc_w, plane->state->crtc_h); return; }
- value |= (bo->iova >> 10) & 0x3fffff; + value |= (state->iova[0] >> 10) & 0x3fffff; tegra_dc_writel(dc, value, DC_DISP_CURSOR_START_ADDR);
#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT - value = (bo->iova >> 32) & 0x3; + value = (state->iova[0] >> 32) & 0x3; tegra_dc_writel(dc, value, DC_DISP_CURSOR_START_ADDR_HI); #endif
@@ -902,7 +901,8 @@ static void tegra_cursor_atomic_update(struct drm_plane *plane, tegra_dc_writel(dc, value, DC_DISP_BLEND_CURSOR_CONTROL);
/* position the cursor */ - value = (state->crtc_y & 0x3fff) << 16 | (state->crtc_x & 0x3fff); + value = (plane->state->crtc_y & 0x3fff) << 16 | + (plane->state->crtc_x & 0x3fff); tegra_dc_writel(dc, value, DC_DISP_CURSOR_POSITION); }
From: Thierry Reding treding@nvidia.com
Upon system suspend, make sure the +5V HDMI regulator is disabled. This avoids potentially leaking current to the HDMI connector. This also makes sure that upon resume the regulator is enabled again, which in some cases is necessary to properly restore the state of the supply on resume.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/sor.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 615cb319fa8b..2200f4cd397a 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3912,8 +3912,7 @@ static int tegra_sor_remove(struct platform_device *pdev) return 0; }
-#ifdef CONFIG_PM -static int tegra_sor_suspend(struct device *dev) +static int tegra_sor_runtime_suspend(struct device *dev) { struct tegra_sor *sor = dev_get_drvdata(dev); int err; @@ -3935,7 +3934,7 @@ static int tegra_sor_suspend(struct device *dev) return 0; }
-static int tegra_sor_resume(struct device *dev) +static int tegra_sor_runtime_resume(struct device *dev) { struct tegra_sor *sor = dev_get_drvdata(dev); int err; @@ -3967,10 +3966,25 @@ static int tegra_sor_resume(struct device *dev)
return 0; } -#endif + +static int tegra_sor_suspend(struct device *dev) +{ + struct tegra_sor *sor = dev_get_drvdata(dev); + + return regulator_disable(sor->hdmi_supply); +} + +static int tegra_sor_resume(struct device *dev) +{ + struct tegra_sor *sor = dev_get_drvdata(dev); + + return regulator_enable(sor->hdmi_supply); +}
static const struct dev_pm_ops tegra_sor_pm_ops = { - SET_RUNTIME_PM_OPS(tegra_sor_suspend, tegra_sor_resume, NULL) + SET_RUNTIME_PM_OPS(tegra_sor_runtime_suspend, tegra_sor_runtime_resume, + NULL) + SET_SYSTEM_SLEEP_PM_OPS(tegra_sor_suspend, tegra_sor_resume) };
struct platform_driver tegra_sor_driver = {
03.12.2019 19:19, Thierry Reding пишет:
From: Thierry Reding treding@nvidia.com
Upon system suspend, make sure the +5V HDMI regulator is disabled. This avoids potentially leaking current to the HDMI connector. This also makes sure that upon resume the regulator is enabled again, which in some cases is necessary to properly restore the state of the supply on resume.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/tegra/sor.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 615cb319fa8b..2200f4cd397a 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3912,8 +3912,7 @@ static int tegra_sor_remove(struct platform_device *pdev) return 0; }
-#ifdef CONFIG_PM -static int tegra_sor_suspend(struct device *dev) +static int tegra_sor_runtime_suspend(struct device *dev) { struct tegra_sor *sor = dev_get_drvdata(dev); int err; @@ -3935,7 +3934,7 @@ static int tegra_sor_suspend(struct device *dev) return 0; }
-static int tegra_sor_resume(struct device *dev) +static int tegra_sor_runtime_resume(struct device *dev) { struct tegra_sor *sor = dev_get_drvdata(dev); int err; @@ -3967,10 +3966,25 @@ static int tegra_sor_resume(struct device *dev)
return 0; } -#endif
+static int tegra_sor_suspend(struct device *dev) +{
- struct tegra_sor *sor = dev_get_drvdata(dev);
- return regulator_disable(sor->hdmi_supply);
+}
+static int tegra_sor_resume(struct device *dev) +{
- struct tegra_sor *sor = dev_get_drvdata(dev);
- return regulator_enable(sor->hdmi_supply);
+}
These functions should be annotated with __maybe_unused attribute.
static const struct dev_pm_ops tegra_sor_pm_ops = {
- SET_RUNTIME_PM_OPS(tegra_sor_suspend, tegra_sor_resume, NULL)
- SET_RUNTIME_PM_OPS(tegra_sor_runtime_suspend, tegra_sor_runtime_resume,
NULL)- SET_SYSTEM_SLEEP_PM_OPS(tegra_sor_suspend, tegra_sor_resume)
};
struct platform_driver tegra_sor_driver = {
From: Thierry Reding treding@nvidia.com
Export the module device table to ensure the VIC compatible strings are listed in the module's aliases table. This in turn causes the driver to be automatically loaded on boot if VIC is the only enabled subdevice of the logical host1x DRM device.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/vic.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index 9444ba183990..c4d82b8b3065 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -386,13 +386,14 @@ static const struct vic_config vic_t194_config = { .supports_sid = true, };
-static const struct of_device_id vic_match[] = { +static const struct of_device_id tegra_vic_of_match[] = { { .compatible = "nvidia,tegra124-vic", .data = &vic_t124_config }, { .compatible = "nvidia,tegra210-vic", .data = &vic_t210_config }, { .compatible = "nvidia,tegra186-vic", .data = &vic_t186_config }, { .compatible = "nvidia,tegra194-vic", .data = &vic_t194_config }, { }, }; +MODULE_DEVICE_TABLE(of, tegra_vic_of_match);
static int vic_probe(struct platform_device *pdev) { @@ -516,7 +517,7 @@ static const struct dev_pm_ops vic_pm_ops = { struct platform_driver tegra_vic_driver = { .driver = { .name = "tegra-vic", - .of_match_table = vic_match, + .of_match_table = tegra_vic_of_match, .pm = &vic_pm_ops }, .probe = vic_probe,
From: Thierry Reding treding@nvidia.com
Subdevices may not be hooked up to an IOMMU via device tree. Detect such situations and avoid confusing users by not emitting an error message.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/dc.c | 2 +- drivers/gpu/drm/tegra/drm.c | 4 +--- drivers/gpu/drm/tegra/vic.c | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 0a5f86b61fda..2b9a25c977c0 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -2027,7 +2027,7 @@ static int tegra_dc_init(struct host1x_client *client) dev_warn(dc->dev, "failed to allocate syncpoint\n");
err = host1x_client_iommu_attach(client); - if (err < 0) { + if (err < 0 && err != -ENODEV) { dev_err(client->dev, "failed to attach to domain: %d\n", err); return err; } diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 56e5e7a5c108..7a16b51eaa2d 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -920,10 +920,8 @@ int host1x_client_iommu_attach(struct host1x_client *client)
if (tegra->domain) { group = iommu_group_get(client->dev); - if (!group) { - dev_err(client->dev, "failed to get IOMMU group\n"); + if (!group) return -ENODEV; - }
if (domain != tegra->domain) { err = iommu_attach_group(tegra->domain, group); diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index c4d82b8b3065..3526c2892ddb 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -167,7 +167,7 @@ static int vic_init(struct host1x_client *client) int err;
err = host1x_client_iommu_attach(client); - if (err < 0) { + if (err < 0 && err != -ENODEV) { dev_err(vic->dev, "failed to attach to domain: %d\n", err); return err; }
From: Thierry Reding treding@nvidia.com
The SOR supports multiple display modes, but only when driving an HDMI monitor does it make sense to control the +5V power supply. eDP and DP don't need this, so make it optional.
This fixes a crash observed during system suspend/resume.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/sor.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 2200f4cd397a..a68d3b36b972 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3970,15 +3970,29 @@ static int tegra_sor_runtime_resume(struct device *dev) static int tegra_sor_suspend(struct device *dev) { struct tegra_sor *sor = dev_get_drvdata(dev); + int err; + + if (sor->hdmi_supply) { + err = regulator_disable(sor->hdmi_supply); + if (err < 0) + return err; + }
- return regulator_disable(sor->hdmi_supply); + return 0; }
static int tegra_sor_resume(struct device *dev) { struct tegra_sor *sor = dev_get_drvdata(dev); + int err; + + if (sor->hdmi_supply) { + err = regulator_enable(sor->hdmi_supply); + if (err < 0) + return err; + }
- return regulator_enable(sor->hdmi_supply); + return 0; }
static const struct dev_pm_ops tegra_sor_pm_ops = {
From: Thierry Reding treding@nvidia.com
The call to tegra_display_hub_cleanup() that takes care of disabling the window groups is missing from the driver's ->remove() callback. Call it to make sure the runtime PM reference counts for the display controllers are balanced.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/tegra/drm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 7a16b51eaa2d..f455ce71e85d 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1241,6 +1241,9 @@ static int host1x_drm_remove(struct host1x_device *dev) drm_atomic_helper_shutdown(drm); drm_mode_config_cleanup(drm);
+ if (tegra->hub) + tegra_display_hub_cleanup(tegra->hub); + err = host1x_device_exit(dev); if (err < 0) dev_err(&dev->dev, "host1x device cleanup failed: %d\n", err);
dri-devel@lists.freedesktop.org
-
Dmitry Osipenko -
Thierry Reding