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(-)
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);
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
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 objectsHence 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
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 objectsHence 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
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 objectsHence 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
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)
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.*/- } 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;/** 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.*/-- 2.23.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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.*/- } 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;/** 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.*/-- 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
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.*/- } 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;/** 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.*/-- 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
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;
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
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
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
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); }
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 = {
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.
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
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; }
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
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
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
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
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
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 = {
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Thierry Reding