linux-kernel - Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 3 Jul 2017 18:35:01 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
Cc:     dri-devel@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
        nouveau@...ts.freedesktop.org,
        Thierry Reding <thierry.reding@...il.com>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        Ben Skeggs <bskeggs@...hat.com>, CK Hu <ck.hu@...iatek.com>,
        linux-tegra@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org,
        linux-mediatek@...ts.infradead.org, Jyri Sarha <jsarha@...com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Philipp Zabel <p.zabel@...gutronix.de>,
        freedreno@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/2] drm/atomic: Change
 drm_atomic_helper_swap_state to return an error.

On Mon, Jul 03, 2017 at 01:01:55PM +0200, Maarten Lankhorst wrote:
> Op 30-06-17 om 15:56 schreef Daniel Vetter:
> > On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
> >> We want to change swap_state to wait indefinitely, but to do this
> >> swap_state should wait interruptibly. This requires propagating
> >> the error to each driver. All drivers have changes to deal with the
> >> clean up. In order to allow easy reverting, the commit that changes
> >> behavior is separate so someone only has to revert that for testing.
> >>
> >> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
> >> failed cleanup_planes was not called.
> >>
> >> Cc: Boris Brezillon <boris.brezillon@...e-electrons.com>
> >> Cc: David Airlie <airlied@...ux.ie>
> >> Cc: Daniel Vetter <daniel.vetter@...el.com>
> >> Cc: Jani Nikula <jani.nikula@...ux.intel.com>
> >> Cc: Sean Paul <seanpaul@...omium.org>
> >> Cc: CK Hu <ck.hu@...iatek.com>
> >> Cc: Philipp Zabel <p.zabel@...gutronix.de>
> >> Cc: Matthias Brugger <matthias.bgg@...il.com>
> >> Cc: Rob Clark <robdclark@...il.com>
> >> Cc: Ben Skeggs <bskeggs@...hat.com>
> >> Cc: Thierry Reding <thierry.reding@...il.com>
> >> Cc: Jonathan Hunter <jonathanh@...dia.com>
> >> Cc: Jyri Sarha <jsarha@...com>
> >> Cc: Tomi Valkeinen <tomi.valkeinen@...com>
> >> Cc: Eric Anholt <eric@...olt.net>
> >> Cc: dri-devel@...ts.freedesktop.org
> >> Cc: linux-kernel@...r.kernel.org
> >> Cc: intel-gfx@...ts.freedesktop.org
> >> Cc: linux-arm-kernel@...ts.infradead.org
> >> Cc: linux-mediatek@...ts.infradead.org
> >> Cc: linux-arm-msm@...r.kernel.org
> >> Cc: freedreno@...ts.freedesktop.org
> >> Cc: nouveau@...ts.freedesktop.org
> >> Cc: linux-tegra@...r.kernel.org
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> > We kinda need to backport this to older kernels, but I don't really see
> > how :( Maybe we should split this up:
> > patch 1: Change to int return type
> > patches 2-(n-1): Driver conversions
> > patch n: __must_check addition
> >
> > That would at least somewhat make this backportable ...
> >
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 ++++++++++++--
> >>  drivers/gpu/drm/drm_atomic_helper.c          | 18 ++++++++++++------
> >>  drivers/gpu/drm/i915/intel_display.c         | 10 +++++++++-
> >>  drivers/gpu/drm/mediatek/mtk_drm_drv.c       |  7 ++++++-
> >>  drivers/gpu/drm/msm/msm_atomic.c             | 14 +++++++++-----
> >>  drivers/gpu/drm/nouveau/nv50_display.c       | 10 ++++++++--
> >>  drivers/gpu/drm/tegra/drm.c                  |  7 ++++++-
> >>  drivers/gpu/drm/tilcdc/tilcdc_drv.c          |  6 +++++-
> >>  drivers/gpu/drm/vc4/vc4_kms.c                | 21 +++++++++++++--------
> >>  include/drm/drm_atomic_helper.h              |  4 ++--
> >>  10 files changed, 82 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> index 516d9547d331..d4f787bf1d4a 100644
> >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,
> >>  		goto error;
> >>  	}
> >>  
> >> -	/* Swap the state, this is the point of no return. */
> >> -	drm_atomic_helper_swap_state(state, true);
> > Push the swap_state up over the commit setup (but after the allocation)
> > and there's no more a problem with unrolling.
> This can't be done higher up because of the interruptible wait.
> 
> Unless we change the patch series to move the waiting for hw_done to a separate step and get rid of the stall argument to swap_state once everything is converted. This could be useful for all drivers that have some kind of setup, because we could move the wait up slightly to suit the drivers needs.

right, swap_state (well the swapping part, not the stalling part) must be
done as the last step and can't fail. Might be a reason do split them, but
not sure that's a good idea either. Please disregard my comment ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch