[Intel-gfx] [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere

Mon Nov 12 18:21:27 UTC 2018

On Mon, Nov 12, 2018 at 04:01:14PM +0100, Maarten Lankhorst wrote:
> We already have __drm_atomic_helper_connector_reset() and
> __drm_atomic_helper_plane_reset(), extend this to crtc as well.
> 
> Most drivers already have a gpu reset hook, correct it.
> Nouveau already implemented its own __drm_atomic_helper_crtc_reset(),
> convert it to the common one.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: "Christian KÃ¶nig" <christian.koenig at amd.com>
> Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Liviu Dudau <liviu.dudau at arm.com>
> Cc: Brian Starkey <brian.starkey at arm.com>
> Cc: Mali DP Maintainers <malidp at foss.arm.com>
> Cc: Boris Brezillon <boris.brezillon at bootlin.com>
> Cc: Nicolas Ferre <nicolas.ferre at microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni at bootlin.com>
> Cc: Ludovic Desroches <ludovic.desroches at microchip.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard at bootlin.com>
> Cc: Sean Paul <sean at poorly.run>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Philipp Zabel <p.zabel at pengutronix.de>
> Cc: CK Hu <ck.hu at mediatek.com>
> Cc: Matthias Brugger <matthias.bgg at gmail.com>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas at ideasonboard.com>
> Cc: Sandy Huang <hjc at rock-chips.com>
> Cc: "Heiko StÃ¼bner" <heiko at sntech.de>
> Cc: Thierry Reding <thierry.reding at gmail.com>
> Cc: Jonathan Hunter <jonathanh at nvidia.com>
> Cc: Eric Anholt <eric at anholt.net>
> Cc: VMware Graphics <linux-graphics-maintainer at vmware.com>
> Cc: Sinclair Yeh <syeh at vmware.com>
> Cc: Thomas Hellstrom <thellstrom at vmware.com>
> Cc: Tony Cheng <Tony.Cheng at amd.com>
> Cc: Shirish S <shirish.s at amd.com>
> Cc: Mikita Lipski <mikita.lipski at amd.com>
> Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha at amd.com>
> Cc: David Francis <David.Francis at amd.com>
> Cc: Anthony Koo <Anthony.Koo at amd.com>
> Cc: Jeykumar Sankaran <jsanka at codeaurora.org>
> Cc: Jordan Crouse <jcrouse at codeaurora.org>
> Cc: Bruce Wang <bzwang at chromium.org>
> Cc: Sravanthi Kollukuduru <skolluku at codeaurora.org>
> Cc: Archit Taneja <architt at codeaurora.org>
> Cc: Steve Kowalik <steven at wedontsleep.org>
> Cc: Carsten Behling <carsten.behling at googlemail.com>
> Cc: Haneen Mohammed <hamohammed.sa at gmail.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Cc: Mahesh Kumar <mahesh1.kumar at intel.com>
> Cc: amd-gfx at lists.freedesktop.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: intel-gfx at lists.freedesktop.org
> Cc: linux-mediatek at lists.infradead.org
> Cc: linux-arm-msm at vger.kernel.org
> Cc: freedreno at lists.freedesktop.org
> Cc: nouveau at lists.freedesktop.org
> Cc: linux-renesas-soc at vger.kernel.org
> Cc: linux-rockchip at lists.infradead.org
> Cc: linux-tegra at vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +--
>  drivers/gpu/drm/arm/malidp_crtc.c             |  5 +--
>  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c    |  5 +--
>  drivers/gpu/drm/drm_atomic_state_helper.c     | 31 ++++++++++++++++---
>  drivers/gpu/drm/i915/intel_display.c          |  2 +-
>  drivers/gpu/drm/imx/ipuv3-crtc.c              |  5 +--
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |  5 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 12 ++-----
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c     |  6 +---
>  drivers/gpu/drm/nouveau/dispnv50/head.c       | 13 ++------
>  drivers/gpu/drm/omapdrm/omap_crtc.c           |  7 ++---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  4 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  7 +++--
>  drivers/gpu/drm/tegra/dc.c                    |  5 +--
>  drivers/gpu/drm/vc4/vc4_crtc.c                |  8 ++---
>  drivers/gpu/drm/vkms/vkms_crtc.c              |  7 +----
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           |  9 +-----
>  include/drm/drm_atomic_state_helper.h         |  2 ++
>  18 files changed, 56 insertions(+), 81 deletions(-)
> 
<snip>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 3ba996069d69..3eee1ca33fc5 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -55,6 +55,29 @@
>   * for these functions.
>   */
>  
> +/**
> + * __drm_atomic_helper_crtc_reset - reset state on CRTC
> + * @crtc: drm CRTC
> + * @crtc_state: CRTC state to assign
> + *
> + * Initializes the newly allocated @crtc_state and assigns it to
> + * the &drm_conector->state pointer of @crtc, usually required when
> + * initializing the drivers or when called from the &drm_crtc_funcs.reset
> + * hook.
> + *
> + * This is useful for drivers that subclass the CRTC state.
> + */
> +void
> +__drm_atomic_helper_crtc_reset(struct drm_crtc *crtc,
> +			       struct drm_crtc_state *crtc_state)
> +{
> +	if (crtc_state)
> +		crtc_state->crtc = crtc;

So apparently crtc and connector reset expect a NULL state, but plane
reset does not. That seems a bit inconsistent. Migth be nice to get
them all to agree on what is the correct behaviour.

> +
> +	crtc->state = crtc_state;
> +}
> +EXPORT_SYMBOL(__drm_atomic_helper_crtc_reset);
> +
>  /**
>   * drm_atomic_helper_crtc_reset - default &drm_crtc_funcs.reset hook for CRTCs
>   * @crtc: drm CRTC
> @@ -64,14 +87,14 @@
>   */
>  void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
>  {
> +	struct drm_crtc_state *crtc_state =
> +		kzalloc(sizeof(*crtc->state), GFP_KERNEL);
> +
>  	if (crtc->state)
>  		__drm_atomic_helper_crtc_destroy_state(crtc->state);
>  
>  	kfree(crtc->state);
> -	crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
> -
> -	if (crtc->state)
> -		crtc->state->crtc = crtc;
> +	__drm_atomic_helper_crtc_reset(crtc, crtc_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f383417571ec..907ffeb64781 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15457,7 +15457,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		__drm_atomic_helper_crtc_destroy_state(&crtc_state->base);
>  		memset(crtc_state, 0, sizeof(*crtc_state));
> -		crtc_state->base.crtc = &crtc->base;
> +		__drm_atomic_helper_crtc_reset(&crtc->base, &crtc_state->base);

intel_crtc_init() could use the same treatment.

And intel_create_plane_state() could use the plane reset. In fact it
looks like we aren't intializing plane constant alpha at all. So it'll
start out as zero which probably isn't what most people would expect.

I also wonder if this patch shouldn't be split up more. Just in case
there's some unforseen regression somewhere I'd hate to see the
entire thing get reverted.

-- 
Ville Syrjälä
Intel