[PATCH 0/9] drm/tegra: Miscellaneous fixes

[PATCH 0/9] drm/tegra: Miscellaneous fixes

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Here's a random assortment of fixes for issues that I ran into as I've
been testing suspend/resume and other things recently.

Thierry

Thierry Reding (9):
  drm/tegra: hub: Remove bogus connection mutex check
  drm/tegra: gem: Properly pin imported buffers
  drm/tegra: gem: Remove premature import restrictions
  drm/tegra: Use proper IOVA address for cursor image
  drm/tegra: sor: Implement system suspend/resume
  drm/tegra: vic: Export module device table
  drm/tegra: Silence expected errors on IOMMU attach
  drm/tegra: dpaux: Add missing runtime PM references
  drm/tegra: sor: Make the +5V HDMI supply optional

 drivers/gpu/drm/tegra/dc.c    | 18 ++++++-------
 drivers/gpu/drm/tegra/dpaux.c | 16 +++++++++--
 drivers/gpu/drm/tegra/drm.c   |  4 +--
 drivers/gpu/drm/tegra/gem.c   | 50 ++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/tegra/hub.c   |  3 ---
 drivers/gpu/drm/tegra/sor.c   | 38 ++++++++++++++++++++++----
 drivers/gpu/drm/tegra/vic.c   |  7 ++---
 7 files changed, 104 insertions(+), 32 deletions(-)

-- 
2.23.0

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

[PATCH 1/9] drm/tegra: hub: Remove bogus connection mutex check

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

I have no recollection why that check is there, but it seems to trigger
all the time, so remove it. Everything works fine without.

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);
-- 
2.23.0

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

[PATCH 2/9] drm/tegra: gem: Properly pin imported buffers

[Thierry Reding]

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>
---
 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)
-- 
2.23.0

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

[PATCH 3/9] drm/tegra: gem: Remove premature import restrictions

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

It's not known at import time whether or not all users of a DMA-BUF will
be able to deal with non-contiguous memory. Each user needs to verify at
map-time whether it can access the buffer.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/gem.c | 7 -------
 1 file changed, 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;
-- 
2.23.0

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

[PATCH 4/9] drm/tegra: Use proper IOVA address for cursor image

[Thierry Reding]

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.

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);
 }
 
-- 
2.23.0

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

[PATCH 5/9] drm/tegra: sor: Implement system suspend/resume

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

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 = {
-- 
2.23.0

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

[PATCH 6/9] drm/tegra: vic: Export module device table

[Thierry Reding]

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,
-- 
2.23.0

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

[PATCH 7/9] drm/tegra: Silence expected errors on IOMMU attach

[Thierry Reding]

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.

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;
 	}
-- 
2.23.0

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

[PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Ensure that a runtime PM reference is acquired each time the DPAUX
registers are accessed. Otherwise the code may end up running without
the controller being powered, out-of-reset or clocked in some corner
cases, resulting in a crash.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
index 622cdf1ad246..4b2b86aed1a5 100644
--- a/drivers/gpu/drm/tegra/dpaux.c
+++ b/drivers/gpu/drm/tegra/dpaux.c
@@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
 			       unsigned int function, unsigned int group)
 {
 	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
+	int err;
+
+	pm_runtime_get_sync(dpaux->dev);
+	err = tegra_dpaux_pad_config(dpaux, function);
+	pm_runtime_put(dpaux->dev);
 
-	return tegra_dpaux_pad_config(dpaux, function);
+	return err;
 }
 
 static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
@@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
 int drm_dp_aux_enable(struct drm_dp_aux *aux)
 {
 	struct tegra_dpaux *dpaux = to_dpaux(aux);
+	int err;
+
+	pm_runtime_get_sync(dpaux->dev);
+	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
+	pm_runtime_put(dpaux->dev);
 
-	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
+	return err;
 }
 
 int drm_dp_aux_disable(struct drm_dp_aux *aux)
 {
 	struct tegra_dpaux *dpaux = to_dpaux(aux);
 
+	pm_runtime_get_sync(dpaux->dev);
 	tegra_dpaux_pad_power_down(dpaux);
+	pm_runtime_put(dpaux->dev);
 
 	return 0;
 }
-- 
2.23.0

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

[PATCH 9/9] drm/tegra: sor: Make the +5V HDMI supply optional

[Thierry Reding]

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.

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 = {
-- 
2.23.0

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

Re: [PATCH 1/9] drm/tegra: hub: Remove bogus connection mutex check

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:37:33PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> I have no recollection why that check is there, but it seems to trigger
> all the time, so remove it. Everything works fine without.
> 
> 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));

I suspect copypasta from the mst private state stuff, which relied on this
lock to protect it. Except your code never bothered to grab that lock (or
any other) so was technically broken until we added generic locking in

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

Hence this is now ok to drop, originally it wasnt.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside: You're single-thread all your atomic updates on the hub->lock,
which might not be what you want. At least updates to separate crtc should
go through in parallel. Usual way to fix this is to add a
tegra_crtc_state->hub_changed that your earlier code sets, and then you
walk the crtc states in the atomic commit (only those, not all, otherwise
you just rebuild that global lock again), and then only grab the hub state
when you need to update something.

Cheers, Daniel

> -
>  	priv = drm_atomic_get_private_obj_state(state, &hub->base);
>  	if (IS_ERR(priv))
>  		return ERR_CAST(priv);
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/tegra: gem: Properly pin imported buffers

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:37:34PM +0100, Thierry Reding wrote:
> 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>
> ---
>  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);

Why duplicate this instead of just handing out obj->sgt, and then in unpin
making sure you don't release it? You could also only map/unmap the
dma_buf here in your pin/unpin, but that's a pile of work plus the mapping
is cached anyway so won't change a thing.
-Daniel

> +		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)
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/9] drm/tegra: gem: Remove premature import restrictions

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:37:35PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> It's not known at import time whether or not all users of a DMA-BUF will
> be able to deal with non-contiguous memory. Each user needs to verify at
> map-time whether it can access the buffer.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

I'm not seeing any other check for nents ... does this mean that there's
not actually any block that requires contig mem?
-Daniel

> ---
>  drivers/gpu/drm/tegra/gem.c | 7 -------
>  1 file changed, 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;
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Ensure that a runtime PM reference is acquired each time the DPAUX
> registers are accessed. Otherwise the code may end up running without
> the controller being powered, out-of-reset or clocked in some corner
> cases, resulting in a crash.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

On this one here I'm very confused.

- Why do you drop the runtime pm between enable and disable? Is that just
  how the hw works, i.e. the pad config stays, just the registers go away?

- I'm not seeing any locking between the different users (dp aux and
  pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
  make this easier (but I'm not super fond of that pattern from i2c).

- Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
  callback, otherwise the various userspace interface (dp aux, but also
  i2c on top of that) won't work. Some pre/post_transfer functions like
  i2c has might be useful for stuff like this.

Cheers, Daniel

> ---
>  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> index 622cdf1ad246..4b2b86aed1a5 100644
> --- a/drivers/gpu/drm/tegra/dpaux.c
> +++ b/drivers/gpu/drm/tegra/dpaux.c
> @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
>  			       unsigned int function, unsigned int group)
>  {
>  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> +	int err;
> +
> +	pm_runtime_get_sync(dpaux->dev);
> +	err = tegra_dpaux_pad_config(dpaux, function);
> +	pm_runtime_put(dpaux->dev);
>  
> -	return tegra_dpaux_pad_config(dpaux, function);
> +	return err;
>  }
>  
>  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
>  int drm_dp_aux_enable(struct drm_dp_aux *aux)
>  {
>  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> +	int err;
> +
> +	pm_runtime_get_sync(dpaux->dev);
> +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> +	pm_runtime_put(dpaux->dev);
>  
> -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> +	return err;
>  }
>  
>  int drm_dp_aux_disable(struct drm_dp_aux *aux)
>  {
>  	struct tegra_dpaux *dpaux = to_dpaux(aux);
>  
> +	pm_runtime_get_sync(dpaux->dev);
>  	tegra_dpaux_pad_power_down(dpaux);
> +	pm_runtime_put(dpaux->dev);
>  
>  	return 0;
>  }
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 9/9] drm/tegra: sor: Make the +5V HDMI supply optional

[Daniel Vetter]

On Thu, Nov 28, 2019 at 04:37:41PM +0100, Thierry Reding wrote:
> 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.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  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 = {
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/tegra: hub: Remove bogus connection mutex check

[Thierry Reding]

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

On Fri, Nov 29, 2019 at 10:06:43AM +0100, Daniel Vetter wrote:
> On Thu, Nov 28, 2019 at 04:37:33PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > I have no recollection why that check is there, but it seems to trigger
> > all the time, so remove it. Everything works fine without.
> > 
> > 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));
> 
> I suspect copypasta from the mst private state stuff, which relied on this
> lock to protect it. Except your code never bothered to grab that lock (or
> any other) so was technically broken until we added generic locking in
> 
> 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
> 
> Hence this is now ok to drop, originally it wasnt.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Great, thanks for pointing that out. I'll update the commit message with
that explanation.

> Aside: You're single-thread all your atomic updates on the hub->lock,
> which might not be what you want. At least updates to separate crtc should
> go through in parallel. Usual way to fix this is to add a
> tegra_crtc_state->hub_changed that your earlier code sets, and then you
> walk the crtc states in the atomic commit (only those, not all, otherwise
> you just rebuild that global lock again), and then only grab the hub state
> when you need to update something.

I'm confused. Where do you see hub->lock? Did you mean wgrp->lock?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 2/9] drm/tegra: gem: Properly pin imported buffers

[Thierry Reding]

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

On Fri, Nov 29, 2019 at 10:10:38AM +0100, Daniel Vetter wrote:
> On Thu, Nov 28, 2019 at 04:37:34PM +0100, Thierry Reding wrote:
> > 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>
> > ---
> >  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);
> 
> Why duplicate this instead of just handing out obj->sgt, and then in unpin
> making sure you don't release it? You could also only map/unmap the
> dma_buf here in your pin/unpin, but that's a pile of work plus the mapping
> is cached anyway so won't change a thing.

The problem with just handing out obj->sgt is that these buffers may be
used by several of the host1x engines in the same job. This means that
they may end up getting dma_map()'ed by multiple devices. dma_map_*()
stores the DMA addresses for the buffer in the SG entries, so subsequent
calls would effectively overwrite the earlier mappings, so we need a new
SG table for each device.

Thierry

> -Daniel
> 
> > +		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)
> > -- 
> > 2.23.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 3/9] drm/tegra: gem: Remove premature import restrictions

[Thierry Reding]

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

On Fri, Nov 29, 2019 at 10:12:44AM +0100, Daniel Vetter wrote:
> On Thu, Nov 28, 2019 at 04:37:35PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > It's not known at import time whether or not all users of a DMA-BUF will
> > be able to deal with non-contiguous memory. Each user needs to verify at
> > map-time whether it can access the buffer.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> I'm not seeing any other check for nents ... does this mean that there's
> not actually any block that requires contig mem?

All the blocks require contiguous memory. However, they are all behind
an IOMMU and in practice will always end up mapping the buffers through
the IOMMU. Techically this check should now be in tegra_dc_pin(), which
is called by the ->prepare_fb() callback. I didn't add it because there
are no practical use-cases where this happens, although I guess you
could come up with a kernel and DTB combination where this is actually
possible by jumping through some hoops.

This fix here is to make Tegra DRM interoperation with Nouveau work
again since that's currently broken after moving to the IOMMU-backed DMA
API as an alternative to explicit IOMMU usage. With explicit IOMMU usage
(that's the if corresponding to the else removed below) the IOMMU domain
was shared between the display controllers at the driver level, so it
was fine to make this determination in the else branch because this was
the case where no IOMMU was in play. After the move to the DMA API, this
else branch is also taken when the DMA API is backed by an IOMMU and at
it is unfortunately not known at import time which display controller
ends up scanning out the DMA BUF, nor if that display controller is
behind an IOMMU. We only know that when the actual mapping takes place,
so we'd need to look at sgt->nents after dma_map_sg() in in
tegra_dc_pin().

I'll add that check there, just in case anyone manages to conjure up
such a configuration.

Thierry

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/tegra/gem.c | 7 -------
> >  1 file changed, 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;
> > -- 
> > 2.23.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Thierry Reding]

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

On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Ensure that a runtime PM reference is acquired each time the DPAUX
> > registers are accessed. Otherwise the code may end up running without
> > the controller being powered, out-of-reset or clocked in some corner
> > cases, resulting in a crash.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> On this one here I'm very confused.
> 
> - Why do you drop the runtime pm between enable and disable? Is that just
>   how the hw works, i.e. the pad config stays, just the registers go away?

Now you've made me doubt this. I don't think the pad configuration stays
across runtime suspend/resume, so you're right, this shouldn't work.
I'll need to go retest this one specifically.

I had added these runtime PM references to ensure the device was
properly configured at resume from suspend, but there ended up being an
additional issue with the I2C driver that relies on this, so perhaps
this may not be necessary in the end.

> - I'm not seeing any locking between the different users (dp aux and
>   pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
>   make this easier (but I'm not super fond of that pattern from i2c).

There should be no need to lock here. DP AUX transfers will only be used
between drm_dp_aux_enable() and drm_dp_aux_disable().

> - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
>   callback, otherwise the various userspace interface (dp aux, but also
>   i2c on top of that) won't work. Some pre/post_transfer functions like
>   i2c has might be useful for stuff like this.

I suppose it would be possible for someone to attempt to use those
userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
and then the locking would be required.

I'll look into that.

Thierry

> 
> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > index 622cdf1ad246..4b2b86aed1a5 100644
> > --- a/drivers/gpu/drm/tegra/dpaux.c
> > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
> >  			       unsigned int function, unsigned int group)
> >  {
> >  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > +	int err;
> > +
> > +	pm_runtime_get_sync(dpaux->dev);
> > +	err = tegra_dpaux_pad_config(dpaux, function);
> > +	pm_runtime_put(dpaux->dev);
> >  
> > -	return tegra_dpaux_pad_config(dpaux, function);
> > +	return err;
> >  }
> >  
> >  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
> >  int drm_dp_aux_enable(struct drm_dp_aux *aux)
> >  {
> >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > +	int err;
> > +
> > +	pm_runtime_get_sync(dpaux->dev);
> > +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > +	pm_runtime_put(dpaux->dev);
> >  
> > -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > +	return err;
> >  }
> >  
> >  int drm_dp_aux_disable(struct drm_dp_aux *aux)
> >  {
> >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> >  
> > +	pm_runtime_get_sync(dpaux->dev);
> >  	tegra_dpaux_pad_power_down(dpaux);
> > +	pm_runtime_put(dpaux->dev);
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.23.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 1/9] drm/tegra: hub: Remove bogus connection mutex check

[Daniel Vetter]

On Fri, Nov 29, 2019 at 11:12:55AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:06:43AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:33PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > I have no recollection why that check is there, but it seems to trigger
> > > all the time, so remove it. Everything works fine without.
> > > 
> > > 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));
> > 
> > I suspect copypasta from the mst private state stuff, which relied on this
> > lock to protect it. Except your code never bothered to grab that lock (or
> > any other) so was technically broken until we added generic locking in
> > 
> > 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
> > 
> > Hence this is now ok to drop, originally it wasnt.
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Great, thanks for pointing that out. I'll update the commit message with
> that explanation.
> 
> > Aside: You're single-thread all your atomic updates on the hub->lock,
> > which might not be what you want. At least updates to separate crtc should
> > go through in parallel. Usual way to fix this is to add a
> > tegra_crtc_state->hub_changed that your earlier code sets, and then you
> > walk the crtc states in the atomic commit (only those, not all, otherwise
> > you just rebuild that global lock again), and then only grab the hub state
> > when you need to update something.
> 
> I'm confused. Where do you see hub->lock? Did you mean wgrp->lock?

struct tegra_display_hub->base.lock I have no idea what wgrp->lock is
protecting - the functions seem to be only called from driver load/cleanup
code, and that is single-threaded. If I'm not missing anything then
wgrp->lock does nothing for you.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/tegra: gem: Properly pin imported buffers

[Daniel Vetter]

On Fri, Nov 29, 2019 at 11:15:37AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:10:38AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:34PM +0100, Thierry Reding wrote:
> > > 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>
> > > ---
> > >  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);
> > 
> > Why duplicate this instead of just handing out obj->sgt, and then in unpin
> > making sure you don't release it? You could also only map/unmap the
> > dma_buf here in your pin/unpin, but that's a pile of work plus the mapping
> > is cached anyway so won't change a thing.
> 
> The problem with just handing out obj->sgt is that these buffers may be
> used by several of the host1x engines in the same job. This means that
> they may end up getting dma_map()'ed by multiple devices. dma_map_*()
> stores the DMA addresses for the buffer in the SG entries, so subsequent
> calls would effectively overwrite the earlier mappings, so we need a new
> SG table for each device.

So strictly speaking, i.e. if you don't want to contribute to the living
mess that's called dma-buf, you're supposed to dma_buf_attach each and
every device (definitely those with different iommu/dma-api routing). That
entire "let me peak behind the abstraction" thing is kinda not how this
was meant to be used. But the first drm prime drivers did it really badly,
plus drm_prime.c is somewhat backwards in some regards by always attaching
for you right away to /something/, but maybe not the right thing.

Adding more dma-buf interface abuse doesn't feel like the right thing
really.

Also, you're not supposed to call dma_map_sg on the sg tables you get from
dma-buf, they're supposed to be mapped already.
-Daniel


> 
> Thierry
> 
> > -Daniel
> > 
> > > +		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)
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/9] drm/tegra: gem: Remove premature import restrictions

[Daniel Vetter]

On Fri, Nov 29, 2019 at 11:33:45AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:12:44AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:35PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > It's not known at import time whether or not all users of a DMA-BUF will
> > > be able to deal with non-contiguous memory. Each user needs to verify at
> > > map-time whether it can access the buffer.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > I'm not seeing any other check for nents ... does this mean that there's
> > not actually any block that requires contig mem?
> 
> All the blocks require contiguous memory. However, they are all behind
> an IOMMU and in practice will always end up mapping the buffers through
> the IOMMU. Techically this check should now be in tegra_dc_pin(), which
> is called by the ->prepare_fb() callback. I didn't add it because there
> are no practical use-cases where this happens, although I guess you
> could come up with a kernel and DTB combination where this is actually
> possible by jumping through some hoops.
> 
> This fix here is to make Tegra DRM interoperation with Nouveau work
> again since that's currently broken after moving to the IOMMU-backed DMA
> API as an alternative to explicit IOMMU usage. With explicit IOMMU usage
> (that's the if corresponding to the else removed below) the IOMMU domain
> was shared between the display controllers at the driver level, so it
> was fine to make this determination in the else branch because this was
> the case where no IOMMU was in play. After the move to the DMA API, this
> else branch is also taken when the DMA API is backed by an IOMMU and at
> it is unfortunately not known at import time which display controller
> ends up scanning out the DMA BUF, nor if that display controller is
> behind an IOMMU. We only know that when the actual mapping takes place,
> so we'd need to look at sgt->nents after dma_map_sg() in in
> tegra_dc_pin().
> 
> I'll add that check there, just in case anyone manages to conjure up
> such a configuration.

Imo just paste the above explanation into the commit message and

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Or also add the check, but imo the explanation here is the important part.
-Daniel

> 
> Thierry
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/tegra/gem.c | 7 -------
> > >  1 file changed, 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;
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Daniel Vetter]

On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > registers are accessed. Otherwise the code may end up running without
> > > the controller being powered, out-of-reset or clocked in some corner
> > > cases, resulting in a crash.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > On this one here I'm very confused.
> > 
> > - Why do you drop the runtime pm between enable and disable? Is that just
> >   how the hw works, i.e. the pad config stays, just the registers go away?
> 
> Now you've made me doubt this. I don't think the pad configuration stays
> across runtime suspend/resume, so you're right, this shouldn't work.
> I'll need to go retest this one specifically.
> 
> I had added these runtime PM references to ensure the device was
> properly configured at resume from suspend, but there ended up being an
> additional issue with the I2C driver that relies on this, so perhaps
> this may not be necessary in the end.
> 
> > - I'm not seeing any locking between the different users (dp aux and
> >   pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> >   make this easier (but I'm not super fond of that pattern from i2c).
> 
> There should be no need to lock here. DP AUX transfers will only be used
> between drm_dp_aux_enable() and drm_dp_aux_disable().

So dp aux vs. dp aux aside (that's the next point below), there's
guaranteed no one else can get at that pinctrl mux? Since the other
setting is labelled I2C I assumed there's an i2c controller hanging of it,
exposed to userspace, and userspace might probe that whenever it feels
like (similar to the issue below). But I don't know your hw, nor do I
really know pinctrl. Just looked a bit strange.

Cheers, Daniel


> > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> >   callback, otherwise the various userspace interface (dp aux, but also
> >   i2c on top of that) won't work. Some pre/post_transfer functions like
> >   i2c has might be useful for stuff like this.
> 
> I suppose it would be possible for someone to attempt to use those
> userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> and then the locking would be required.
> 
> I'll look into that.
> 
> Thierry
> 
> > 
> > Cheers, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
> > >  			       unsigned int function, unsigned int group)
> > >  {
> > >  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > +	int err;
> > > +
> > > +	pm_runtime_get_sync(dpaux->dev);
> > > +	err = tegra_dpaux_pad_config(dpaux, function);
> > > +	pm_runtime_put(dpaux->dev);
> > >  
> > > -	return tegra_dpaux_pad_config(dpaux, function);
> > > +	return err;
> > >  }
> > >  
> > >  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
> > >  int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > >  {
> > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > +	int err;
> > > +
> > > +	pm_runtime_get_sync(dpaux->dev);
> > > +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > +	pm_runtime_put(dpaux->dev);
> > >  
> > > -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > +	return err;
> > >  }
> > >  
> > >  int drm_dp_aux_disable(struct drm_dp_aux *aux)
> > >  {
> > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > >  
> > > +	pm_runtime_get_sync(dpaux->dev);
> > >  	tegra_dpaux_pad_power_down(dpaux);
> > > +	pm_runtime_put(dpaux->dev);
> > >  
> > >  	return 0;
> > >  }
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Thierry Reding]

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

On Fri, Nov 29, 2019 at 09:20:25PM +0100, Daniel Vetter wrote:
> On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> > On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > > registers are accessed. Otherwise the code may end up running without
> > > > the controller being powered, out-of-reset or clocked in some corner
> > > > cases, resulting in a crash.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > 
> > > On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > On this one here I'm very confused.
> > > 
> > > - Why do you drop the runtime pm between enable and disable? Is that just
> > >   how the hw works, i.e. the pad config stays, just the registers go away?
> > 
> > Now you've made me doubt this. I don't think the pad configuration stays
> > across runtime suspend/resume, so you're right, this shouldn't work.
> > I'll need to go retest this one specifically.
> > 
> > I had added these runtime PM references to ensure the device was
> > properly configured at resume from suspend, but there ended up being an
> > additional issue with the I2C driver that relies on this, so perhaps
> > this may not be necessary in the end.
> > 
> > > - I'm not seeing any locking between the different users (dp aux and
> > >   pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> > >   make this easier (but I'm not super fond of that pattern from i2c).
> > 
> > There should be no need to lock here. DP AUX transfers will only be used
> > between drm_dp_aux_enable() and drm_dp_aux_disable().
> 
> So dp aux vs. dp aux aside (that's the next point below), there's
> guaranteed no one else can get at that pinctrl mux? Since the other
> setting is labelled I2C I assumed there's an i2c controller hanging of it,
> exposed to userspace, and userspace might probe that whenever it feels
> like (similar to the issue below). But I don't know your hw, nor do I
> really know pinctrl. Just looked a bit strange.

Well technically anyone could get at the mux, but since it controls the
pins of a single I2C controller, only that I2C controller should ever
get its hands on the pinmux. Anything else would be an error in the DT.

Now, the pins can also be used in AUX mode when the SOR is used in DP
mode. However, since DP and HDMI are mutually exclusive, this is a board
level decision, so in practice you're only ever going to see them used
in either I2C or AUX mode. The "off" mode is used only for power saving
when I2C or DPAUX don't use the pins.

Regarding the runtime PM references, it turns out that those are
completely bogus because we already take a runtime PM reference at
probe time. I'm going to drop this patch and look into fixing the other,
real issues that you pointed out.

Thierry

> 
> Cheers, Daniel
> 
> 
> > > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> > >   callback, otherwise the various userspace interface (dp aux, but also
> > >   i2c on top of that) won't work. Some pre/post_transfer functions like
> > >   i2c has might be useful for stuff like this.
> > 
> > I suppose it would be possible for someone to attempt to use those
> > userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> > and then the locking would be required.
> > 
> > I'll look into that.
> > 
> > Thierry
> > 
> > > 
> > > Cheers, Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
> > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
> > > >  			       unsigned int function, unsigned int group)
> > > >  {
> > > >  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > > +	int err;
> > > > +
> > > > +	pm_runtime_get_sync(dpaux->dev);
> > > > +	err = tegra_dpaux_pad_config(dpaux, function);
> > > > +	pm_runtime_put(dpaux->dev);
> > > >  
> > > > -	return tegra_dpaux_pad_config(dpaux, function);
> > > > +	return err;
> > > >  }
> > > >  
> > > >  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > > > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
> > > >  int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > > >  {
> > > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > > +	int err;
> > > > +
> > > > +	pm_runtime_get_sync(dpaux->dev);
> > > > +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > > +	pm_runtime_put(dpaux->dev);
> > > >  
> > > > -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > > +	return err;
> > > >  }
> > > >  
> > > >  int drm_dp_aux_disable(struct drm_dp_aux *aux)
> > > >  {
> > > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > >  
> > > > +	pm_runtime_get_sync(dpaux->dev);
> > > >  	tegra_dpaux_pad_power_down(dpaux);
> > > > +	pm_runtime_put(dpaux->dev);
> > > >  
> > > >  	return 0;
> > > >  }
> > > > -- 
> > > > 2.23.0
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 1/9] drm/tegra: hub: Remove bogus connection mutex check

[Thierry Reding]

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

On Fri, Nov 29, 2019 at 08:03:09PM +0100, Daniel Vetter wrote:
> On Fri, Nov 29, 2019 at 11:12:55AM +0100, Thierry Reding wrote:
> > On Fri, Nov 29, 2019 at 10:06:43AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 28, 2019 at 04:37:33PM +0100, Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > I have no recollection why that check is there, but it seems to trigger
> > > > all the time, so remove it. Everything works fine without.
> > > > 
> > > > 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));
> > > 
> > > I suspect copypasta from the mst private state stuff, which relied on this
> > > lock to protect it. Except your code never bothered to grab that lock (or
> > > any other) so was technically broken until we added generic locking in
> > > 
> > > 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
> > > 
> > > Hence this is now ok to drop, originally it wasnt.
> > > 
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Great, thanks for pointing that out. I'll update the commit message with
> > that explanation.
> > 
> > > Aside: You're single-thread all your atomic updates on the hub->lock,
> > > which might not be what you want. At least updates to separate crtc should
> > > go through in parallel. Usual way to fix this is to add a
> > > tegra_crtc_state->hub_changed that your earlier code sets, and then you
> > > walk the crtc states in the atomic commit (only those, not all, otherwise
> > > you just rebuild that global lock again), and then only grab the hub state
> > > when you need to update something.
> > 
> > I'm confused. Where do you see hub->lock? Did you mean wgrp->lock?
> 
> struct tegra_display_hub->base.lock I have no idea what wgrp->lock is
> protecting - the functions seem to be only called from driver load/cleanup
> code, and that is single-threaded. If I'm not missing anything then
> wgrp->lock does nothing for you.

This is currently single-threaded, but the idea was to make window group
assignment more dynamic. I currently always enable window groups because
there's no good place to dynamically do that. Once I figure that out the
lock would become necessary, so I'd prefer to leave it in place as sort
of a reminder that I need to actually worry about the locking.

I'll have to look into the hub->base.lock, I don't recall exactly what I
thought at the time and it seems like I didn't leave enough comments in
the code to quickly refresh my memory...

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH 8/9] drm/tegra: dpaux: Add missing runtime PM references

[Daniel Vetter]

On Mon, Dec 02, 2019 at 03:58:33PM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 09:20:25PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 29, 2019 at 11:44:12AM +0100, Thierry Reding wrote:
> > > On Fri, Nov 29, 2019 at 10:23:19AM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 28, 2019 at 04:37:40PM +0100, Thierry Reding wrote:
> > > > > From: Thierry Reding <treding@nvidia.com>
> > > > > 
> > > > > Ensure that a runtime PM reference is acquired each time the DPAUX
> > > > > registers are accessed. Otherwise the code may end up running without
> > > > > the controller being powered, out-of-reset or clocked in some corner
> > > > > cases, resulting in a crash.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > On patches 4,5,7 in this series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > 
> > > > On this one here I'm very confused.
> > > > 
> > > > - Why do you drop the runtime pm between enable and disable? Is that just
> > > >   how the hw works, i.e. the pad config stays, just the registers go away?
> > > 
> > > Now you've made me doubt this. I don't think the pad configuration stays
> > > across runtime suspend/resume, so you're right, this shouldn't work.
> > > I'll need to go retest this one specifically.
> > > 
> > > I had added these runtime PM references to ensure the device was
> > > properly configured at resume from suspend, but there ended up being an
> > > additional issue with the I2C driver that relies on this, so perhaps
> > > this may not be necessary in the end.
> > > 
> > > > - I'm not seeing any locking between the different users (dp aux and
> > > >   pinctrl). We might want to change drm_dp_aux->hw_mutex to a pointer to
> > > >   make this easier (but I'm not super fond of that pattern from i2c).
> > > 
> > > There should be no need to lock here. DP AUX transfers will only be used
> > > between drm_dp_aux_enable() and drm_dp_aux_disable().
> > 
> > So dp aux vs. dp aux aside (that's the next point below), there's
> > guaranteed no one else can get at that pinctrl mux? Since the other
> > setting is labelled I2C I assumed there's an i2c controller hanging of it,
> > exposed to userspace, and userspace might probe that whenever it feels
> > like (similar to the issue below). But I don't know your hw, nor do I
> > really know pinctrl. Just looked a bit strange.
> 
> Well technically anyone could get at the mux, but since it controls the
> pins of a single I2C controller, only that I2C controller should ever
> get its hands on the pinmux. Anything else would be an error in the DT.
> 
> Now, the pins can also be used in AUX mode when the SOR is used in DP
> mode. However, since DP and HDMI are mutually exclusive, this is a board
> level decision, so in practice you're only ever going to see them used
> in either I2C or AUX mode. The "off" mode is used only for power saving
> when I2C or DPAUX don't use the pins.

Oh, so you don't support DP+ with some auto level shifter magic? At least
in other drivers DP+ is why you need to have a lock between dp aux and i2c
at runtime, because otherwise things go wrong if you reprobe DP while
userspace is probing the hdmi i2c side (or the other way round).

> Regarding the runtime PM references, it turns out that those are
> completely bogus because we already take a runtime PM reference at
> probe time. I'm going to drop this patch and look into fixing the other,
> real issues that you pointed out.

Ah that explains the confusion :-)
-Daniel
> 
> Thierry
> 
> > 
> > Cheers, Daniel
> > 
> > 
> > > > - Your drm_dp_aux_enable/disable needs to be moved into the ->transfer
> > > >   callback, otherwise the various userspace interface (dp aux, but also
> > > >   i2c on top of that) won't work. Some pre/post_transfer functions like
> > > >   i2c has might be useful for stuff like this.
> > > 
> > > I suppose it would be possible for someone to attempt to use those
> > > userspace interfaces outside of drm_dp_aux_enable()/drm_dp_aux_disable()
> > > and then the locking would be required.
> > > 
> > > I'll look into that.
> > > 
> > > Thierry
> > > 
> > > > 
> > > > Cheers, Daniel
> > > > 
> > > > > ---
> > > > >  drivers/gpu/drm/tegra/dpaux.c | 16 ++++++++++++++--
> > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c
> > > > > index 622cdf1ad246..4b2b86aed1a5 100644
> > > > > --- a/drivers/gpu/drm/tegra/dpaux.c
> > > > > +++ b/drivers/gpu/drm/tegra/dpaux.c
> > > > > @@ -434,8 +434,13 @@ static int tegra_dpaux_set_mux(struct pinctrl_dev *pinctrl,
> > > > >  			       unsigned int function, unsigned int group)
> > > > >  {
> > > > >  	struct tegra_dpaux *dpaux = pinctrl_dev_get_drvdata(pinctrl);
> > > > > +	int err;
> > > > > +
> > > > > +	pm_runtime_get_sync(dpaux->dev);
> > > > > +	err = tegra_dpaux_pad_config(dpaux, function);
> > > > > +	pm_runtime_put(dpaux->dev);
> > > > >  
> > > > > -	return tegra_dpaux_pad_config(dpaux, function);
> > > > > +	return err;
> > > > >  }
> > > > >  
> > > > >  static const struct pinmux_ops tegra_dpaux_pinmux_ops = {
> > > > > @@ -809,15 +814,22 @@ enum drm_connector_status drm_dp_aux_detect(struct drm_dp_aux *aux)
> > > > >  int drm_dp_aux_enable(struct drm_dp_aux *aux)
> > > > >  {
> > > > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > > > +	int err;
> > > > > +
> > > > > +	pm_runtime_get_sync(dpaux->dev);
> > > > > +	err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > > > +	pm_runtime_put(dpaux->dev);
> > > > >  
> > > > > -	return tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_AUX);
> > > > > +	return err;
> > > > >  }
> > > > >  
> > > > >  int drm_dp_aux_disable(struct drm_dp_aux *aux)
> > > > >  {
> > > > >  	struct tegra_dpaux *dpaux = to_dpaux(aux);
> > > > >  
> > > > > +	pm_runtime_get_sync(dpaux->dev);
> > > > >  	tegra_dpaux_pad_power_down(dpaux);
> > > > > +	pm_runtime_put(dpaux->dev);
> > > > >  
> > > > >  	return 0;
> > > > >  }
> > > > > -- 
> > > > > 2.23.0
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > 
> > > > -- 
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel