Re: [bug report] drm/ingenic: Add support for the IPU

From: Daniel Vetter <daniel@ffwll.ch>
To: dan.carpenter@oracle.com
Cc: paul@crapouillou.net, dri-devel@lists.freedesktop.org
Subject: Re: [bug report] drm/ingenic: Add support for the IPU
Date: Mon, 20 Jul 2020 09:44:12 +0200	[thread overview]
Message-ID: <20200720074412.GX3278063@phenom.ffwll.local> (raw)
In-Reply-To: <20200720072237.GA8232@mwanda>

On Mon, Jul 20, 2020 at 10:22:37AM +0300, dan.carpenter@oracle.com wrote:
> Hello Paul Cercueil,
> 
> The patch fc1acf317b01: "drm/ingenic: Add support for the IPU" from
> Jul 16, 2020, leads to the following static checker warning:
> 
> 	drivers/gpu/drm/ingenic/ingenic-drm-drv.c:232 ingenic_drm_crtc_atomic_check()
> 	error: potentially dereferencing uninitialized 'ipu_state'.
> 
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>    197  static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>    198                                           struct drm_crtc_state *state)
>    199  {
>    200          struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>    201          struct drm_plane_state *f1_state, *f0_state, *ipu_state;
>    202          long rate;
>    203  
>    204          if (!drm_atomic_crtc_needs_modeset(state))
>    205                  return 0;
>    206  
>    207          if (state->mode.hdisplay > priv->soc_info->max_width ||
>    208              state->mode.vdisplay > priv->soc_info->max_height)
>    209                  return -EINVAL;

Random entirely unrelated drive-through comment: These mode checks should
be in crtc_helper_funcs->mode_valid so that they're shared between the
atomic_check and output probe paths.

But preexisting issues, just something that would be nice to rectify.

Cheers, Daniel

>    210  
>    211          rate = clk_round_rate(priv->pix_clk,
>    212                                state->adjusted_mode.clock * 1000);
>    213          if (rate < 0)
>    214                  return rate;
>    215  
>    216          if (priv->soc_info->has_osd) {
>    217                  f1_state = drm_atomic_get_plane_state(state->state, &priv->f1);
>    218                  f0_state = drm_atomic_get_plane_state(state->state, &priv->f0);
>    219  
>    220                  if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU) && priv->ipu_plane) {
> 
> Do we need to check both the CONFIG and the priv->ipu_plane or would it
> be sufficient to just check if (priv->ipu_plane) {?
> 
>    221                          ipu_state = drm_atomic_get_plane_state(state->state, priv->ipu_plane);
>    222  
>    223                          /* IPU and F1 planes cannot be enabled at the same time. */
>    224                          if (f1_state->fb && ipu_state->fb) {
>    225                                  dev_dbg(priv->dev, "Cannot enable both F1 and IPU\n");
>    226                                  return -EINVAL;
>    227                          }
>    228                  }
>    229  
>    230                  /* If all the planes are disabled, we won't get a VBLANK IRQ */
>    231                  priv->no_vblank = !f1_state->fb && !f0_state->fb &&
>    232                                    !(priv->ipu_plane && ipu_state->fb);
>                                             ^^^^^^^^^^^^^^^
> Because here we're only checking "priv->ipu_plane".
> 
>    233          }
>    234  
>    235          return 0;
>    236  }
> 
> regards,
> dan carpenter
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel