drm/todo: Add entry for moving to dma_resv_lock

Commit Message

Daniel Vetter Jan. 22, 2021, 1:36 p.m. UTC

Requested by Thomas. I think it justifies a new level, since I tried
to make some forward progress on this last summer, and gave up (for
now). This is very tricky.

Signed-off-by: Daniel Vetter <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: "Christian König" <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 Documentation/gpu/todo.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Christian König Jan. 22, 2021, 1:40 p.m. UTC | #1

Am 22.01.21 um 14:36 schrieb Daniel Vetter:
> Requested by Thomas. I think it justifies a new level, since I tried
> to make some forward progress on this last summer, and gave up (for
> now). This is very tricky.
>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>   Documentation/gpu/todo.rst | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index dea9082c0e5f..f872d3d33218 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -23,6 +23,9 @@ Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem
>   and graphics topics. Generally need the relevant hardware for development and
>   testing.
>   
> +Expert: Only attempt these if you've successfully completed some tricky
> +refactorings already and are an expert in the specific area
> +
>   Subsystem-wide refactorings
>   ===========================
>   
> @@ -168,6 +171,22 @@ Contact: Daniel Vetter, respective driver maintainers
>   
>   Level: Advanced
>   
> +Move Buffer Object Locking to dma_resv_lock()
> +---------------------------------------------
> +
> +Many drivers have their own per-object locking scheme, usually using
> +mutex_lock(). This causes all kinds of trouble for buffer sharing, since
> +depending which driver is the exporter and importer, the locking hierarchy is
> +reversed.
> +
> +To solve this we need one standard per-object locking mechanism, which is
> +dma_resv_lock(). This lock needs to be called as the outermost lock, with all
> +other driver specific per-object locks removed. The problem is tha rolling out
> +the actual change to the locking contract is a flag day, due to struct dma_buf
> +buffer sharing.
> +
> +Level: Expert
> +

Could you name some examples of driver locks here? I'm not aware in 
anything like this in amdgpu, radeon or neveau.

And yes sounds like a job for the appropriate driver maintainer.

Thanks,
Christian.

>   Convert logging to drm_* functions with drm_device paramater
>   ------------------------------------------------------------
>

Daniel Vetter Jan. 22, 2021, 1:45 p.m. UTC | #2

On Fri, Jan 22, 2021 at 2:40 PM Christian König
<[e-mail]> wrote:
>
> Am 22.01.21 um 14:36 schrieb Daniel Vetter:
> > Requested by Thomas. I think it justifies a new level, since I tried
> > to make some forward progress on this last summer, and gave up (for
> > now). This is very tricky.
> >
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Sumit Semwal <[e-mail]>
> > Cc: "Christian König" <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >   Documentation/gpu/todo.rst | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index dea9082c0e5f..f872d3d33218 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -23,6 +23,9 @@ Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem
> >   and graphics topics. Generally need the relevant hardware for development and
> >   testing.
> >
> > +Expert: Only attempt these if you've successfully completed some tricky
> > +refactorings already and are an expert in the specific area
> > +
> >   Subsystem-wide refactorings
> >   ===========================
> >
> > @@ -168,6 +171,22 @@ Contact: Daniel Vetter, respective driver maintainers
> >
> >   Level: Advanced
> >
> > +Move Buffer Object Locking to dma_resv_lock()
> > +---------------------------------------------
> > +
> > +Many drivers have their own per-object locking scheme, usually using
> > +mutex_lock(). This causes all kinds of trouble for buffer sharing, since
> > +depending which driver is the exporter and importer, the locking hierarchy is
> > +reversed.
> > +
> > +To solve this we need one standard per-object locking mechanism, which is
> > +dma_resv_lock(). This lock needs to be called as the outermost lock, with all
> > +other driver specific per-object locks removed. The problem is tha rolling out
> > +the actual change to the locking contract is a flag day, due to struct dma_buf
> > +buffer sharing.
> > +
> > +Level: Expert
> > +
>
> Could you name some examples of driver locks here? I'm not aware in
> anything like this in amdgpu, radeon or neveau.

ttm based drivers are all fine. It's everything else which is a
problem, and it gets worse if you mix helpers (like shmem helpers,
which have their own locks internally) with render drivers (again with
their own mutexes).

> And yes sounds like a job for the appropriate driver maintainer.

The problem is, this one you can't do driver-by-driver because of the
dma-buf contract. I mean we tried for p2p at first, it's just too
much. I tried to do it last summer just for shmem gem helpers, and you
really have to tackle all the drivers in one go (even if you ignore
dma-buf for now, where we side-stepped the problem with pinning). This
is "scares danvet" levels of nasty.
-Daniel

> Thanks,
> Christian.
>
> >   Convert logging to drm_* functions with drm_device paramater
> >   ------------------------------------------------------------
> >
>

Thomas Zimmermann Jan. 22, 2021, 2:06 p.m. UTC | #3

Hi

Am 22.01.21 um 14:36 schrieb Daniel Vetter:
> Requested by Thomas. I think it justifies a new level, since I tried
> to make some forward progress on this last summer, and gave up (for
> now). This is very tricky.

Adding it to the TODO list is a first step. :)

Acked-by: Thomas Zimmermann <[e-mail]>

> 
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: "Christian König" <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>   Documentation/gpu/todo.rst | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index dea9082c0e5f..f872d3d33218 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -23,6 +23,9 @@ Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem
>   and graphics topics. Generally need the relevant hardware for development and
>   testing.
>   
> +Expert: Only attempt these if you've successfully completed some tricky
> +refactorings already and are an expert in the specific area
> +
>   Subsystem-wide refactorings
>   ===========================
>   
> @@ -168,6 +171,22 @@ Contact: Daniel Vetter, respective driver maintainers
>   
>   Level: Advanced
>   
> +Move Buffer Object Locking to dma_resv_lock()
> +---------------------------------------------
> +
> +Many drivers have their own per-object locking scheme, usually using
> +mutex_lock(). This causes all kinds of trouble for buffer sharing, since
> +depending which driver is the exporter and importer, the locking hierarchy is
> +reversed.
> +
> +To solve this we need one standard per-object locking mechanism, which is
> +dma_resv_lock(). This lock needs to be called as the outermost lock, with all
> +other driver specific per-object locks removed. The problem is tha rolling out
> +the actual change to the locking contract is a flag day, due to struct dma_buf
> +buffer sharing.
> +
> +Level: Expert
> +
>   Convert logging to drm_* functions with drm_device paramater
>   ------------------------------------------------------------
>   
>

Daniel Vetter Feb. 3, 2021, 1:11 p.m. UTC | #4

On Fri, Jan 22, 2021 at 03:06:44PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 22.01.21 um 14:36 schrieb Daniel Vetter:
> > Requested by Thomas. I think it justifies a new level, since I tried
> > to make some forward progress on this last summer, and gave up (for
> > now). This is very tricky.
> 
> Adding it to the TODO list is a first step. :)
> 
> Acked-by: Thomas Zimmermann <[e-mail]>

Applied.
-Daniel

> 
> > 
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: Sumit Semwal <[e-mail]>
> > Cc: "Christian König" <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >   Documentation/gpu/todo.rst | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> > 
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index dea9082c0e5f..f872d3d33218 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -23,6 +23,9 @@ Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem
> >   and graphics topics. Generally need the relevant hardware for development and
> >   testing.
> > +Expert: Only attempt these if you've successfully completed some tricky
> > +refactorings already and are an expert in the specific area
> > +
> >   Subsystem-wide refactorings
> >   ===========================
> > @@ -168,6 +171,22 @@ Contact: Daniel Vetter, respective driver maintainers
> >   Level: Advanced
> > +Move Buffer Object Locking to dma_resv_lock()
> > +---------------------------------------------
> > +
> > +Many drivers have their own per-object locking scheme, usually using
> > +mutex_lock(). This causes all kinds of trouble for buffer sharing, since
> > +depending which driver is the exporter and importer, the locking hierarchy is
> > +reversed.
> > +
> > +To solve this we need one standard per-object locking mechanism, which is
> > +dma_resv_lock(). This lock needs to be called as the outermost lock, with all
> > +other driver specific per-object locks removed. The problem is tha rolling out
> > +the actual change to the locking contract is a flag day, due to struct dma_buf
> > +buffer sharing.
> > +
> > +Level: Expert
> > +
> >   Convert logging to drm_* functions with drm_device paramater
> >   ------------------------------------------------------------
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>

Patch

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index dea9082c0e5f..f872d3d33218 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -23,6 +23,9 @@  Advanced: Tricky tasks that need fairly good understanding of the DRM subsystem
 and graphics topics. Generally need the relevant hardware for development and
 testing.
 
+Expert: Only attempt these if you've successfully completed some tricky
+refactorings already and are an expert in the specific area
+
 Subsystem-wide refactorings
 ===========================
 
@@ -168,6 +171,22 @@  Contact: Daniel Vetter, respective driver maintainers
 
 Level: Advanced
 
+Move Buffer Object Locking to dma_resv_lock()
+---------------------------------------------
+
+Many drivers have their own per-object locking scheme, usually using
+mutex_lock(). This causes all kinds of trouble for buffer sharing, since
+depending which driver is the exporter and importer, the locking hierarchy is
+reversed.
+
+To solve this we need one standard per-object locking mechanism, which is
+dma_resv_lock(). This lock needs to be called as the outermost lock, with all
+other driver specific per-object locks removed. The problem is tha rolling out
+the actual change to the locking contract is a flag day, due to struct dma_buf
+buffer sharing.
+
+Level: Expert
+
 Convert logging to drm_* functions with drm_device paramater
 ------------------------------------------------------------