Re: [PATCH v2] drm: Use state helper instead of CRTC state pointer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 05, 2020 at 05:45:18PM +0100, Maxime Ripard wrote:
> Many drivers reference the crtc->pointer in order to get the current CRTC
> state in their atomic_begin or atomic_flush hooks, which would be the new
> CRTC state in the global atomic state since _swap_state happened when those
> hooks are run.
> 
> Use the drm_atomic_get_new_crtc_state helper to get that state to make it
> more obvious.
> 
> This was made using the coccinelle script below:
> 
> @ crtc_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> (
> static struct drm_crtc_helper_funcs helpers = {
> 	...,
> 	.atomic_begin = func,
> 	...,
> };
> |
> static struct drm_crtc_helper_funcs helpers = {
> 	...,
> 	.atomic_flush = func,
> 	...,
> };
> )
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> symbol crtc_state;
> expression e;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct tegra_dc_state *crtc_state = e;
> + struct tegra_dc_state *dc_state = e;
>   <+...
> -       crtc_state
> +	dc_state
>   ...+>
>   }
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> symbol crtc_state;
> expression e;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct mtk_crtc_state *crtc_state = e;
> + struct mtk_crtc_state *mtk_crtc_state = e;
>   <+...
> -       crtc_state
> +	mtk_crtc_state
>   ...+>
>   }
> 
> @ replaces_new_state @
> identifier crtc_atomic_func.func;
> identifier crtc, state, crtc_state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   ...
> - struct drm_crtc_state *crtc_state = crtc->state;
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>   ...
>  }
> 
> @@
> identifier crtc_atomic_func.func;
> identifier crtc, state, crtc_state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
>   struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>   ...
> - crtc->state
> + crtc_state
>   ...
>  }
> 
> @ adds_new_state @
> identifier crtc_atomic_func.func;
> identifier crtc, state;
> @@
> 
>   func(struct drm_crtc *crtc, struct drm_atomic_state *state) {
> + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>   ...
> - crtc->state
> + crtc_state
>   ...
>  }
> 
> @ include depends on adds_new_state || replaces_new_state @
> @@
> 
>  #include <drm/drm_atomic.h>
> 
> @ no_include depends on !include && (adds_new_state || replaces_new_state) @
> @@
> 
> + #include <drm/drm_atomic.h>
>   #include <drm/...>
> 
> Cc: "James (Qian) Wang" <james.qian.wang@xxxxxxx>
> Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> Cc: Mihail Atanassov <mihail.atanassov@xxxxxxx>
> Cc: Brian Starkey <brian.starkey@xxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
> Cc: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> Cc: Chun-Kuang Hu <chunkuang.hu@xxxxxxxxxx>
> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Cc: Sandy Huang <hjc@xxxxxxxxxxxxxx>
> Cc: "Heiko Stübner" <heiko@xxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Suggested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> 
> ---
> 
> Changes from v1:
>   - Fixed checkpatch warnings
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c |  4 +++-
>  drivers/gpu/drm/armada/armada_crtc.c             |  8 ++++++--
>  drivers/gpu/drm/ast/ast_mode.c                   |  4 +++-
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c        |  7 +++++--
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c          | 15 +++++++++------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c      |  6 ++++--
>  drivers/gpu/drm/tegra/dc.c                       |  8 +++++---
>  drivers/gpu/drm/virtio/virtgpu_display.c         |  4 +++-
>  8 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index df0b9eeb8933..4b485eb512e2 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -387,10 +387,12 @@ static void
>  komeda_crtc_atomic_flush(struct drm_crtc *crtc,
>  			 struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct drm_crtc_state *old = drm_atomic_get_old_crtc_state(state,
>  								   crtc);
>  	/* commit with modeset will be handled in enable/disable */
> -	if (drm_atomic_crtc_needs_modeset(crtc->state))
> +	if (drm_atomic_crtc_needs_modeset(crtc_state))
>  		return;
>  
>  	komeda_crtc_do_flush(crtc, old);

For the komeda part:

Acked-by: Liviu Dudau <liviu@xxxxxxxxxxx>


> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index ca643f4e2064..3ebcf5a52c8b 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -431,11 +431,13 @@ static int armada_drm_crtc_atomic_check(struct drm_crtc *crtc,
>  static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>  					 struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
>  
>  	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
>  
> -	if (crtc->state->color_mgmt_changed)
> +	if (crtc_state->color_mgmt_changed)
>  		armada_drm_update_gamma(crtc);
>  
>  	dcrtc->regs_idx = 0;
> @@ -445,6 +447,8 @@ static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>  static void armada_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  					 struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
>  
>  	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
> @@ -455,7 +459,7 @@ static void armada_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  	 * If we aren't doing a full modeset, then we need to queue
>  	 * the event here.
>  	 */
> -	if (!drm_atomic_crtc_needs_modeset(crtc->state)) {
> +	if (!drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		dcrtc->update_pending = true;
>  		armada_drm_crtc_queue_state_event(crtc);
>  		spin_lock_irq(&dcrtc->irq_lock);
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 22f0e65fbe9a..9db371f4054f 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -782,10 +782,12 @@ static void
>  ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
>  			     struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
>  									      crtc);
>  	struct ast_private *ast = to_ast_private(crtc->dev);
> -	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc->state);
> +	struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
>  	struct ast_crtc_state *old_ast_crtc_state = to_ast_crtc_state(old_crtc_state);
>  
>  	/*
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index b9c156e13156..7603f86dd0d1 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -305,11 +305,13 @@ ingenic_drm_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode
>  static void ingenic_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>  					  struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>  	u32 ctrl = 0;
>  
>  	if (priv->soc_info->has_osd &&
> -	    drm_atomic_crtc_needs_modeset(crtc->state)) {
> +	    drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		/*
>  		 * If IPU plane is enabled, enable IPU as source for the F1
>  		 * plane; otherwise use regular DMA.
> @@ -326,7 +328,8 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>  					  struct drm_atomic_state *state)
>  {
>  	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> -	struct drm_crtc_state *crtc_state = crtc->state;
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct drm_pending_vblank_event *event = crtc_state->event;
>  
>  	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 23f5c10b0c67..193848fd7515 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -11,6 +11,7 @@
>  #include <asm/barrier.h>
>  #include <soc/mediatek/smi.h>
>  
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
> @@ -577,17 +578,19 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
>  static void mtk_drm_crtc_atomic_begin(struct drm_crtc *crtc,
>  				      struct drm_atomic_state *state)
>  {
> -	struct mtk_crtc_state *crtc_state = to_mtk_crtc_state(crtc->state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
> +	struct mtk_crtc_state *mtk_crtc_state = to_mtk_crtc_state(crtc_state);
>  	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
>  
> -	if (mtk_crtc->event && crtc_state->base.event)
> +	if (mtk_crtc->event && mtk_crtc_state->base.event)
>  		DRM_ERROR("new event while there is still a pending event\n");
>  
> -	if (crtc_state->base.event) {
> -		crtc_state->base.event->pipe = drm_crtc_index(crtc);
> +	if (mtk_crtc_state->base.event) {
> +		mtk_crtc_state->base.event->pipe = drm_crtc_index(crtc);
>  		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> -		mtk_crtc->event = crtc_state->base.event;
> -		crtc_state->base.event = NULL;
> +		mtk_crtc->event = mtk_crtc_state->base.event;
> +		mtk_crtc_state->base.event = NULL;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 8cd39fca81a3..d1e05482641b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1248,6 +1248,8 @@ static void vop_crtc_gamma_set(struct vop *vop, struct drm_crtc *crtc,
>  static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
>  				  struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state,
>  									      crtc);
>  	struct vop *vop = to_vop(crtc);
> @@ -1256,8 +1258,8 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
>  	 * Only update GAMMA if the 'active' flag is not changed,
>  	 * otherwise it's updated by .atomic_enable.
>  	 */
> -	if (crtc->state->color_mgmt_changed &&
> -	    !crtc->state->active_changed)
> +	if (crtc_state->color_mgmt_changed &&
> +	    !crtc_state->active_changed)
>  		vop_crtc_gamma_set(vop, crtc, old_crtc_state);
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 2d86627b0d4e..85dd7131553a 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1939,15 +1939,17 @@ static void tegra_crtc_atomic_begin(struct drm_crtc *crtc,
>  static void tegra_crtc_atomic_flush(struct drm_crtc *crtc,
>  				    struct drm_atomic_state *state)
>  {
> -	struct tegra_dc_state *crtc_state = to_dc_state(crtc->state);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
> +	struct tegra_dc_state *dc_state = to_dc_state(crtc_state);
>  	struct tegra_dc *dc = to_tegra_dc(crtc);
>  	u32 value;
>  
> -	value = crtc_state->planes << 8 | GENERAL_UPDATE;
> +	value = dc_state->planes << 8 | GENERAL_UPDATE;
>  	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>  	value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL);
>  
> -	value = crtc_state->planes | GENERAL_ACT_REQ;
> +	value = dc_state->planes | GENERAL_ACT_REQ;
>  	tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
>  	value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL);
>  }
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 4bf74836bd53..a6caebd4a0dd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -119,6 +119,8 @@ static int virtio_gpu_crtc_atomic_check(struct drm_crtc *crtc,
>  static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
>  					 struct drm_atomic_state *state)
>  {
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
> +									  crtc);
>  	struct virtio_gpu_output *output = drm_crtc_to_virtio_gpu_output(crtc);
>  
>  	/*
> @@ -127,7 +129,7 @@ static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc,
>  	 * in the plane update callback, and here we just check
>  	 * whenever we must force the modeset.
>  	 */
> -	if (drm_atomic_crtc_needs_modeset(crtc->state)) {
> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		output->needs_modeset = true;
>  	}
>  }
> -- 
> 2.28.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux