Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Daniel Vetter]

On Thu, Mar 22, 2018 at 05:22:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> I really just wanted to fix i915 to re-enable its planes afer load
> detection (a two line patch). This is what I actually ended up with
> after I ran into a framebuffer refcount leak with said two line patch.
> 
> I've tested this on a few i915 boxes and so far it's looking
> good. Everything else is just compile tested.
> 
> Entire series available here:
> git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke
> 
> Cc: Alex Deucher 
> Cc: amd-...@lists.freedesktop.org
> Cc: Benjamin Gaignard 
> Cc: Boris Brezillon 
> Cc: ch...@chris-wilson.co.uk
> Cc: "Christian König" 
> Cc: Daniel Vetter 
> Cc: Dave Airlie 
> Cc: David Airlie 
> Cc: "David (ChunMing) Zhou" 
> Cc: Eric Anholt 
> Cc: freedreno@lists.freedesktop.org
> Cc: Gerd Hoffmann 
> Cc: Harry Wentland 
> Cc: Inki Dae 
> Cc: Joonyoung Shim 
> Cc: Kyungmin Park 
> Cc: linux-arm-...@vger.kernel.org
> Cc: Maarten Lankhorst 
> Cc: martin.pe...@free.fr
> Cc: Rob Clark 
> Cc: Seung-Woo Kim 
> Cc: Shawn Guo 
> Cc: Sinclair Yeh 
> Cc: Thomas Hellstrom 
> Cc: Vincent Abriou 
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: VMware Graphics 
> 
> Ville Syrjälä (23):
>   Revert "drm/atomic-helper: Fix leak in disable_all"
>   drm/atomic-helper: Make drm_atomic_helper_disable_all() update the
> plane->fb pointers
>   drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc()
>   drm/atomic-helper: WARN if legacy plane fb pointers are bogus when
> committing duplicated state
>   drm: Add local 'plane' variable for primary/cursor planes
>   drm: Adjust whitespace for legibility
>   drm: Make the fb refcount handover less magic
>   drm: Use plane->state->fb over plane->fb
>   drm/i915: Stop consulting plane->fb
>   drm/msm: Stop consulting plane->fb
>   drm/sti: Stop consulting plane->fb
>   drm/vmwgfx: Stop consulting plane->fb
>   drm/zte: Stop consulting plane->fb
>   drm/atmel-hlcdc: Stop using plane->fb
>   drm: Stop updating plane->crtc/fb/old_fb on atomic drivers
>   drm/amdgpu/dc: Stop updating plane->fb
>   drm/i915: Stop updating plane->fb/crtc
>   drm/exynos: Stop updating plane->crtc
>   drm/msm: Stop updating plane->fb/crtc
>   drm/virtio: Stop updating plane->fb
>   drm/vc4: Stop updating plane->fb/crtc
>   drm/i915: Restore planes after load detection
>   drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug

Ok, I reviewed the core patches, looks all good.

Also starting auditing all the drivers. I think there's some small
oversights in there, and I need to update my grep foo a bit, but by and
large looks all reasonable.

Imo we should start merging, that will also make the auditing easier.
-Daniel

> 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 -
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   | 12 +---
>  drivers/gpu/drm/drm_atomic.c  | 55 ++--
>  drivers/gpu/drm/drm_atomic_helper.c   | 79 
> ++-
>  drivers/gpu/drm/drm_crtc.c| 51 ++-
>  drivers/gpu/drm/drm_fb_helper.c   |  7 --
>  drivers/gpu/drm/drm_framebuffer.c |  5 --
>  drivers/gpu/drm/drm_plane.c   | 64 +++---
>  drivers/gpu/drm/drm_plane_helper.c|  4 +-
>  drivers/gpu/drm/exynos/exynos_drm_plane.c |  2 -
>  drivers/gpu/drm/i915/intel_crt.c  |  6 ++
>  drivers/gpu/drm/i915/intel_display.c  |  9 +--
>  drivers/gpu/drm/i915/intel_fbdev.c|  2 +-
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c |  3 +-
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c|  2 -
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|  2 -
>  drivers/gpu/drm/sti/sti_plane.c   |  9 +--
>  drivers/gpu/drm/vc4/vc4_crtc.c|  3 -
>  drivers/gpu/drm/virtio/virtgpu_display.c  |  2 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |  6 +-
>  drivers/gpu/drm/zte/zx_vou.c  |  2 +-
>  include/drm/drm_atomic.h  |  3 -
>  22 files changed, 143 insertions(+), 187 deletions(-)
> 
> -- 
> 2.16.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Freedreno mailing list
Freedreno@lists.freedesktop.org

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

Den 22.03.2018 19.49, skrev Ville Syrjälä:

On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote:

tinydrm is also using plane->fb:

$ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/
drivers/gpu/drm/tinydrm/repaper.c:  if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb =
mipi->tinydrm.pipe.plane.fb;

Oh dear, and naturally it's the most annoying one of the bunch. So we
want to take the plane lock in the fb.dirty() hook to look at the fb,
but mipi-dbi.c calls that directly from the bowels of its
.atomic_enable() hook. So that would deadlock pretty neatly if the
commit happens while already holding the lock.

So we'd either need to plumb an acquire context into fb.dirty(),
or maybe have tinydrm provide a lower level lockless dirty() hook
that gets called by both (there are just too many layers in this
particular cake to immediately see where such a hook would be best
placed). Or maybe there's some other solution I didn't think of.

Looking at this I'm also left wondering how this is supposed
handle fb.dirty() getting called concurrently with a modeset.
The dirty_mutex only seems to offer any protection for
fb.dirty() against another fb.dirty()...



I have been waiting for the 'page-flip with damage'[1] work to come to
fruition so I could handle all flushing during atomic commit.
The idea is to use the same damage rect for a generic dirtyfb callback.

Maybe a temporary tinydrm specific solution is possible until that work
lands, but I don't know enough about how to implement such a dirtyfb
callback.

I imagine it could look something like this:

 struct tinydrm_device {
+    struct drm_clip_rect dirtyfb_rect;
 };

static int tinydrm_fb_dirty(struct drm_framebuffer *fb,
                struct drm_file *file_priv,
                unsigned int flags, unsigned int color,
                struct drm_clip_rect *clips,
                unsigned int num_clips)
{
    struct tinydrm_device *tdev = fb->dev->dev_private;
    struct drm_framebuffer *planefb;

    /* Grab whatever lock needed to check this */
    planefb = tdev->pipe.plane.state->fb;

    /* fbdev can flush even when we're not interested */
    if (fb != planefb)
        return 0;

    /* Protect dirtyfb_rect with a lock */
    tinydrm_merge_clips(>dirty_rectfb, clips, num_clips, flags,
                fb->width, fb->height);

    /*
     * Somehow do an atomic commit that results in the atomic update
     * callback being called
     */
    ret = drm_atomic_commit(state);
...
}

static void mipi_dbi_flush(struct drm_framebuffer *fb,
               struct drm_clip_rect *clip)
{
    /* Flush out framebuffer */
}

void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
              struct drm_plane_state *old_state)
{
    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
    struct drm_framebuffer *fb = pipe->plane.state->fb;
    struct drm_crtc *crtc = >pipe.crtc;

    if (!fb || (fb == old_state->fb && !tdev->dirty_rect.x2))
        goto out_vblank;

    /* Don't flush if the controller isn't initialized yet */
    if (!mipi->enabled)
        goto out_vblank;

    if (fb != old_state->fb) { /* Page flip or new, flush all */
        mipi_dbi_flush(fb, NULL);
    } else { /* dirtyfb ioctl */
        mipi_dbi_flush(fb, >dirtyfb_rect);
        memset(>dirtyfb_rect, 0, sizeof(tdev->dirtyfb_rect));
    }

out_vblank:
    if (crtc->state->event) {
        spin_lock_irq(>dev->event_lock);
        drm_crtc_send_vblank_event(crtc, crtc->state->event);
        spin_unlock_irq(>dev->event_lock);
        crtc->state->event = NULL;
    }
}

This is called from the driver pipe enable callback after the controller
has been initialized:

 void mipi_dbi_pipe_enable(struct drm_simple_display_pipe *pipe,
           struct drm_crtc_state *crtc_state)
 {
 struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
 struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
-     struct drm_framebuffer *fb = pipe->plane.fb;
+    struct drm_framebuffer *fb = pipe->plane.state->fb;

 DRM_DEBUG_KMS("\n");

 mipi->enabled = true;
-     if (fb)
-         fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+    mipi_dbi_flush(fb, NULL);
 tinydrm_enable_backlight(mipi->backlight);
 }

I can make patches if this makes any sense and you can help me with the
dirtyfb implementation.

Noralf.

[1] 
https://lists.freedesktop.org/archives/dri-devel/2017-December/161003.html


___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Ville Syrjälä]

On Thu, Mar 22, 2018 at 05:51:35PM +0100, Noralf Trønnes wrote:
> tinydrm is also using plane->fb:
> 
> $ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/
> drivers/gpu/drm/tinydrm/repaper.c:  if (tdev->pipe.plane.fb != fb)
> drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb)
> drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = 
> mipi->tinydrm.pipe.plane.fb;

Oh dear, and naturally it's the most annoying one of the bunch. So we
want to take the plane lock in the fb.dirty() hook to look at the fb,
but mipi-dbi.c calls that directly from the bowels of its
.atomic_enable() hook. So that would deadlock pretty neatly if the
commit happens while already holding the lock.

So we'd either need to plumb an acquire context into fb.dirty(),
or maybe have tinydrm provide a lower level lockless dirty() hook
that gets called by both (there are just too many layers in this
particular cake to immediately see where such a hook would be best
placed). Or maybe there's some other solution I didn't think of.

Looking at this I'm also left wondering how this is supposed
handle fb.dirty() getting called concurrently with a modeset.
The dirty_mutex only seems to offer any protection for
fb.dirty() against another fb.dirty()...

-- 
Ville Syrjälä
Intel OTC
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

On 22 March 2018 at 18:03, Harry Wentland  wrote:
> On 2018-03-22 01:54 PM, Emil Velikov wrote:
>> Hi Ville,
>>
>> On 22 March 2018 at 15:22, Ville Syrjala  
>> wrote:
>>> From: Ville Syrjälä 
>>>
>>> I really just wanted to fix i915 to re-enable its planes afer load
>>> detection (a two line patch). This is what I actually ended up with
>>> after I ran into a framebuffer refcount leak with said two line patch.
>>>
>>> I've tested this on a few i915 boxes and so far it's looking
>>> good. Everything else is just compile tested.
>>>
>> Mostly thinking out loud:
>>
>> Wondering if one cannot somehow (re)move plane->fb/crtc altogether.
>> Otherwise drivers will reintroduce similar code, despite the WARNs and
>> beefy documentation :-\
>
> Wouldn't that require an atomic conversion of all remaining drivers?
>
That or maybe move into plane->legacy->{fb,crtc}. Feel free to swap
'legacy' with flashier name.

Hmm back in 2015 we had a GSoC that updated BOCHS and CIRRUS drivers,
but they never got merged.
Don't recall the details - from memory the conversion seemed fine, but
there was either shortage on review/other.

Might be worth reviving that... regardless it's getting a bit off-topic.
-Emil

[1] 
https://www.google-melange.com/archive/gsoc/2015/orgs/xorg/projects/johnhunter.html
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

Hi Ville,

On 22 March 2018 at 15:22, Ville Syrjala  wrote:
> From: Ville Syrjälä 
>
> I really just wanted to fix i915 to re-enable its planes afer load
> detection (a two line patch). This is what I actually ended up with
> after I ran into a framebuffer refcount leak with said two line patch.
>
> I've tested this on a few i915 boxes and so far it's looking
> good. Everything else is just compile tested.
>
Mostly thinking out loud:

Wondering if one cannot somehow (re)move plane->fb/crtc altogether.
Otherwise drivers will reintroduce similar code, despite the WARNs and
beefy documentation :-\

HTH
Emil
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Noralf Trønnes]

Den 22.03.2018 16.22, skrev Ville Syrjala:

From: Ville Syrjälä 

I really just wanted to fix i915 to re-enable its planes afer load
detection (a two line patch). This is what I actually ended up with
after I ran into a framebuffer refcount leak with said two line patch.

I've tested this on a few i915 boxes and so far it's looking
good. Everything else is just compile tested.

Entire series available here:
git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke

Cc: Alex Deucher 
Cc: amd-...@lists.freedesktop.org
Cc: Benjamin Gaignard 
Cc: Boris Brezillon 
Cc: ch...@chris-wilson.co.uk
Cc: "Christian König" 
Cc: Daniel Vetter 
Cc: Dave Airlie 
Cc: David Airlie 
Cc: "David (ChunMing) Zhou" 
Cc: Eric Anholt 
Cc: freedreno@lists.freedesktop.org
Cc: Gerd Hoffmann 
Cc: Harry Wentland 
Cc: Inki Dae 
Cc: Joonyoung Shim 
Cc: Kyungmin Park 
Cc: linux-arm-...@vger.kernel.org
Cc: Maarten Lankhorst 
Cc: martin.pe...@free.fr
Cc: Rob Clark 
Cc: Seung-Woo Kim 
Cc: Shawn Guo 
Cc: Sinclair Yeh 
Cc: Thomas Hellstrom 
Cc: Vincent Abriou 
Cc: virtualizat...@lists.linux-foundation.org
Cc: VMware Graphics 

Ville Syrjälä (23):
   Revert "drm/atomic-helper: Fix leak in disable_all"
   drm/atomic-helper: Make drm_atomic_helper_disable_all() update the
 plane->fb pointers
   drm: Clear crtc->primary->crtc when disabling the crtc via setcrtc()
   drm/atomic-helper: WARN if legacy plane fb pointers are bogus when
 committing duplicated state
   drm: Add local 'plane' variable for primary/cursor planes
   drm: Adjust whitespace for legibility
   drm: Make the fb refcount handover less magic
   drm: Use plane->state->fb over plane->fb
   drm/i915: Stop consulting plane->fb
   drm/msm: Stop consulting plane->fb
   drm/sti: Stop consulting plane->fb
   drm/vmwgfx: Stop consulting plane->fb
   drm/zte: Stop consulting plane->fb
   drm/atmel-hlcdc: Stop using plane->fb
   drm: Stop updating plane->crtc/fb/old_fb on atomic drivers
   drm/amdgpu/dc: Stop updating plane->fb
   drm/i915: Stop updating plane->fb/crtc
   drm/exynos: Stop updating plane->crtc
   drm/msm: Stop updating plane->fb/crtc
   drm/virtio: Stop updating plane->fb
   drm/vc4: Stop updating plane->fb/crtc
   drm/i915: Restore planes after load detection
   drm/i915: Make force_load_detect effective even w/ DMI quirks/hotplug


tinydrm is also using plane->fb:

$ grep -r "plane\.fb" drivers/gpu/drm/tinydrm/
drivers/gpu/drm/tinydrm/repaper.c:  if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/mipi-dbi.c: struct drm_framebuffer *fb = 
mipi->tinydrm.pipe.plane.fb;

drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c: pipe->plane.fb = fb;
drivers/gpu/drm/tinydrm/ili9225.c:  if (tdev->pipe.plane.fb != fb)
drivers/gpu/drm/tinydrm/st7586.c:   if (tdev->pipe.plane.fb != fb)

Noralf.


  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 -
  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   | 12 +---
  drivers/gpu/drm/drm_atomic.c  | 55 ++--
  drivers/gpu/drm/drm_atomic_helper.c   | 79 ++-
  drivers/gpu/drm/drm_crtc.c| 51 ++-
  drivers/gpu/drm/drm_fb_helper.c   |  7 --
  drivers/gpu/drm/drm_framebuffer.c |  5 --
  drivers/gpu/drm/drm_plane.c   | 64 +++---
  drivers/gpu/drm/drm_plane_helper.c|  4 +-
  drivers/gpu/drm/exynos/exynos_drm_plane.c |  2 -
  drivers/gpu/drm/i915/intel_crt.c  |  6 ++
  drivers/gpu/drm/i915/intel_display.c  |  9 +--
  drivers/gpu/drm/i915/intel_fbdev.c|  2 +-
  drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c |  3 +-
  drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c|  2 -
  drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|  2 -
  drivers/gpu/drm/sti/sti_plane.c   |  9 +--
  drivers/gpu/drm/vc4/vc4_crtc.c|  3 -
  drivers/gpu/drm/virtio/virtgpu_display.c  |  2 -
  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |  6 +-
  drivers/gpu/drm/zte/zx_vou.c  |  2 +-
  include/drm/drm_atomic.h  |  3 -
  22 files changed, 143 insertions(+), 187 deletions(-)



___
Freedreno mailing list
Freedreno@lists.freedesktop.org