| Message ID | 20201105144517.1826692-4-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rid W=1 warnings from GPU | expand |
Hello Lee, On 11/5/20 3:45 PM, Lee Jones wrote: > In the macro for_each_oldnew_crtc_in_state() 'crtc_state' is provided > as a container for state->crtcs[i].new_state, but is not utilised in > this use-case. We cannot simply delete the variable, so here we tell > the compiler that we're intentionally discarding the read value. for_each_oldnew_crtc_in_state already (void) casts the drm_crtc and the old drm_crtc_state to silence unused-but-set-variable warning. Should we maybe (void) cast the new crtc_state as well? Cheers Ahmad > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/imx/ipuv3-plane.c: In function ‘ipu_planes_assign_pre’: > drivers/gpu/drm/imx/ipuv3-plane.c:746:42: warning: variable ‘crtc_state’ set but not used [-Wunused-but-set-variable] > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: NXP Linux Team <linux-imx@nxp.com> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > index 8a4235d9d9f1e..acc0a3ce4992f 100644 > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > @@ -743,7 +743,7 @@ bool ipu_plane_atomic_update_pending(struct drm_plane *plane) > int ipu_planes_assign_pre(struct drm_device *dev, > struct drm_atomic_state *state) > { > - struct drm_crtc_state *old_crtc_state, *crtc_state; > + struct drm_crtc_state *old_crtc_state, __always_unused *crtc_state; > struct drm_plane_state *plane_state; > struct ipu_plane_state *ipu_state; > struct ipu_plane *ipu_plane; >
On Thu, 05 Nov 2020, Ahmad Fatoum wrote: > Hello Lee, > > On 11/5/20 3:45 PM, Lee Jones wrote: > > In the macro for_each_oldnew_crtc_in_state() 'crtc_state' is provided > > as a container for state->crtcs[i].new_state, but is not utilised in > > this use-case. We cannot simply delete the variable, so here we tell > > the compiler that we're intentionally discarding the read value. > > for_each_oldnew_crtc_in_state already (void) casts the drm_crtc and the old > drm_crtc_state to silence unused-but-set-variable warning. Should we maybe > (void) cast the new crtc_state as well? From what I saw, it only void casts the ones which aren't assigned. > > Fixes the following W=1 kernel build warning(s): > > > > drivers/gpu/drm/imx/ipuv3-plane.c: In function ‘ipu_planes_assign_pre’: > > drivers/gpu/drm/imx/ipuv3-plane.c:746:42: warning: variable ‘crtc_state’ set but not used [-Wunused-but-set-variable] > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: Sascha Hauer <s.hauer@pengutronix.de> > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > > Cc: Fabio Estevam <festevam@gmail.com> > > Cc: NXP Linux Team <linux-imx@nxp.com> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > > index 8a4235d9d9f1e..acc0a3ce4992f 100644 > > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > > @@ -743,7 +743,7 @@ bool ipu_plane_atomic_update_pending(struct drm_plane *plane) > > int ipu_planes_assign_pre(struct drm_device *dev, > > struct drm_atomic_state *state) > > { > > - struct drm_crtc_state *old_crtc_state, *crtc_state; > > + struct drm_crtc_state *old_crtc_state, __always_unused *crtc_state; > > struct drm_plane_state *plane_state; > > struct ipu_plane_state *ipu_state; > > struct ipu_plane *ipu_plane; > > >
On 11/6/20 8:41 AM, Lee Jones wrote: > On Thu, 05 Nov 2020, Ahmad Fatoum wrote: > >> Hello Lee, >> >> On 11/5/20 3:45 PM, Lee Jones wrote: >>> In the macro for_each_oldnew_crtc_in_state() 'crtc_state' is provided >>> as a container for state->crtcs[i].new_state, but is not utilised in >>> this use-case. We cannot simply delete the variable, so here we tell >>> the compiler that we're intentionally discarding the read value. >> >> for_each_oldnew_crtc_in_state already (void) casts the drm_crtc and the old >> drm_crtc_state to silence unused-but-set-variable warning. Should we maybe >> (void) cast the new crtc_state as well? > > From what I saw, it only void casts the ones which aren't assigned. How do you mean? I wonder if #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_crtc; \ (__i)++) \ for_each_if ((__state)->crtcs[__i].ptr && \ ((crtc) = (__state)->crtcs[__i].ptr, \ (void)(crtc) /* Only to avoid unused-but-set-variable warning */, \ (old_crtc_state) = (__state)->crtcs[__i].old_state, \ (void)(old_crtc_state) /* Only to avoid unused-but-set-variable warning */, \ - (new_crtc_state) = (__state)->crtcs[__i].new_state, 1)) + (new_crtc_state) = (__state)->crtcs[__i].new_state, \ + (void)(new_crtc_state), 1)) wouldn't be better. > >>> Fixes the following W=1 kernel build warning(s): >>> >>> drivers/gpu/drm/imx/ipuv3-plane.c: In function ‘ipu_planes_assign_pre’: >>> drivers/gpu/drm/imx/ipuv3-plane.c:746:42: warning: variable ‘crtc_state’ set but not used [-Wunused-but-set-variable] >>> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de> >>> Cc: David Airlie <airlied@linux.ie> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Shawn Guo <shawnguo@kernel.org> >>> Cc: Sascha Hauer <s.hauer@pengutronix.de> >>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> >>> Cc: Fabio Estevam <festevam@gmail.com> >>> Cc: NXP Linux Team <linux-imx@nxp.com> >>> Cc: dri-devel@lists.freedesktop.org >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c >>> index 8a4235d9d9f1e..acc0a3ce4992f 100644 >>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c >>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c >>> @@ -743,7 +743,7 @@ bool ipu_plane_atomic_update_pending(struct drm_plane *plane) >>> int ipu_planes_assign_pre(struct drm_device *dev, >>> struct drm_atomic_state *state) >>> { >>> - struct drm_crtc_state *old_crtc_state, *crtc_state; >>> + struct drm_crtc_state *old_crtc_state, __always_unused *crtc_state; >>> struct drm_plane_state *plane_state; >>> struct ipu_plane_state *ipu_state; >>> struct ipu_plane *ipu_plane; >>> >> >
On Fri, 06 Nov 2020, Ahmad Fatoum wrote: > On 11/6/20 8:41 AM, Lee Jones wrote: > > On Thu, 05 Nov 2020, Ahmad Fatoum wrote: > > > >> Hello Lee, > >> > >> On 11/5/20 3:45 PM, Lee Jones wrote: > >>> In the macro for_each_oldnew_crtc_in_state() 'crtc_state' is provided > >>> as a container for state->crtcs[i].new_state, but is not utilised in > >>> this use-case. We cannot simply delete the variable, so here we tell > >>> the compiler that we're intentionally discarding the read value. > >> > >> for_each_oldnew_crtc_in_state already (void) casts the drm_crtc and the old > >> drm_crtc_state to silence unused-but-set-variable warning. Should we maybe > >> (void) cast the new crtc_state as well? > > > > From what I saw, it only void casts the ones which aren't assigned. > > How do you mean? I wonder if > > #define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ > for ((__i) = 0; \ > (__i) < (__state)->dev->mode_config.num_crtc; \ > (__i)++) \ > for_each_if ((__state)->crtcs[__i].ptr && \ > ((crtc) = (__state)->crtcs[__i].ptr, \ > (void)(crtc) /* Only to avoid unused-but-set-variable warning */, \ > (old_crtc_state) = (__state)->crtcs[__i].old_state, \ > (void)(old_crtc_state) /* Only to avoid unused-but-set-variable warning */, \ > - (new_crtc_state) = (__state)->crtcs[__i].new_state, 1)) > + (new_crtc_state) = (__state)->crtcs[__i].new_state, \ > + (void)(new_crtc_state), 1)) > > wouldn't be better. That also works for me. I can fix this up.
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 8a4235d9d9f1e..acc0a3ce4992f 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -743,7 +743,7 @@ bool ipu_plane_atomic_update_pending(struct drm_plane *plane) int ipu_planes_assign_pre(struct drm_device *dev, struct drm_atomic_state *state) { - struct drm_crtc_state *old_crtc_state, *crtc_state; + struct drm_crtc_state *old_crtc_state, __always_unused *crtc_state; struct drm_plane_state *plane_state; struct ipu_plane_state *ipu_state; struct ipu_plane *ipu_plane;
In the macro for_each_oldnew_crtc_in_state() 'crtc_state' is provided as a container for state->crtcs[i].new_state, but is not utilised in this use-case. We cannot simply delete the variable, so here we tell the compiler that we're intentionally discarding the read value. Fixes the following W=1 kernel build warning(s): drivers/gpu/drm/imx/ipuv3-plane.c: In function ‘ipu_planes_assign_pre’: drivers/gpu/drm/imx/ipuv3-plane.c:746:42: warning: variable ‘crtc_state’ set but not used [-Wunused-but-set-variable] Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Fabio Estevam <festevam@gmail.com> Cc: NXP Linux Team <linux-imx@nxp.com> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/gpu/drm/imx/ipuv3-plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)