[bug report] drm/amd/display: If one stream full updates, full update all planes

[bug report] drm/amd/display: If one stream full updates, full update all planes

[Dan Carpenter]

Hello David Francis,

This is a semi-automatic email about new static checker warnings.

The patch c238bfe0be9e: "drm/amd/display: If one stream full updates,
full update all planes" from Mar 29, 2019, leads to the following
Smatch complaint:

    drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:1492 det_surface_update()
    warn: variable dereferenced before check 'u->surface' (see line 1467)

drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c
  1466	
  1467		if (u->surface->force_full_update) {
                    ^^^^^^^^^^^^
The patch adds a new dereference

  1468			update_flags->bits.full_update = 1;
  1469			return UPDATE_TYPE_FULL;
  1470		}
  1471	
  1472		type = get_plane_info_update_type(u);
  1473		elevate_update_type(&overall_type, type);
  1474	
  1475		type = get_scaling_info_update_type(u);
  1476		elevate_update_type(&overall_type, type);
  1477	
  1478		if (u->in_transfer_func)
  1479			update_flags->bits.in_transfer_func_change = 1;
  1480	
  1481		if (u->input_csc_color_matrix)
  1482			update_flags->bits.input_csc_change = 1;
  1483	
  1484		if (u->coeff_reduction_factor)
  1485			update_flags->bits.coeff_reduction_change = 1;
  1486	
  1487		if (u->gamma) {
  1488			enum surface_pixel_format format = SURFACE_PIXEL_FORMAT_GRPH_BEGIN;
  1489	
  1490			if (u->plane_info)
  1491				format = u->plane_info->format;
  1492			else if (u->surface)
                                 ^^^^^^^^^^
But the old code assumed it could be NULL

  1493				format = u->surface->format;
  1494	

regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [bug report] drm/amd/display: If one stream full updates, full update all planes

[Kazlauskas, Nicholas]

On 4/23/19 10:10 AM, Dan Carpenter wrote:
> Hello David Francis,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch c238bfe0be9e: "drm/amd/display: If one stream full updates,
> full update all planes" from Mar 29, 2019, leads to the following
> Smatch complaint:
> 
>      drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:1492 det_surface_update()
>      warn: variable dereferenced before check 'u->surface' (see line 1467)
> 
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c
>    1466	
>    1467		if (u->surface->force_full_update) {
>                      ^^^^^^^^^^^^
> The patch adds a new dereference
> 
>    1468			update_flags->bits.full_update = 1;
>    1469			return UPDATE_TYPE_FULL;
>    1470		}
>    1471	
>    1472		type = get_plane_info_update_type(u);
>    1473		elevate_update_type(&overall_type, type);
>    1474	
>    1475		type = get_scaling_info_update_type(u);
>    1476		elevate_update_type(&overall_type, type);
>    1477	
>    1478		if (u->in_transfer_func)
>    1479			update_flags->bits.in_transfer_func_change = 1;
>    1480	
>    1481		if (u->input_csc_color_matrix)
>    1482			update_flags->bits.input_csc_change = 1;
>    1483	
>    1484		if (u->coeff_reduction_factor)
>    1485			update_flags->bits.coeff_reduction_change = 1;
>    1486	
>    1487		if (u->gamma) {
>    1488			enum surface_pixel_format format = SURFACE_PIXEL_FORMAT_GRPH_BEGIN;
>    1489	
>    1490			if (u->plane_info)
>    1491				format = u->plane_info->format;
>    1492			else if (u->surface)
>                                   ^^^^^^^^^^
> But the old code assumed it could be NULL

This looks like a check that could probably be dropped. This function 
shouldn't be called with a u->surface = NULL - there's even that other 
deference at the top of the function with &u->surface->update_flags;

Nicholas Kazlauskas

> 
>    1493				format = u->surface->format;
>    1494	
> 
> regards,
> dan carpenter
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [bug report] drm/amd/display: If one stream full updates, full update all planes

[Dan Carpenter]

On Tue, Apr 23, 2019 at 02:16:53PM +0000, Kazlauskas, Nicholas wrote:
> On 4/23/19 10:10 AM, Dan Carpenter wrote:
> > Hello David Francis,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch c238bfe0be9e: "drm/amd/display: If one stream full updates,
> > full update all planes" from Mar 29, 2019, leads to the following
> > Smatch complaint:
> > 
> >      drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:1492 det_surface_update()
> >      warn: variable dereferenced before check 'u->surface' (see line 1467)
> > 
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c
> >    1466	
> >    1467		if (u->surface->force_full_update) {
> >                      ^^^^^^^^^^^^
> > The patch adds a new dereference
> > 
> >    1468			update_flags->bits.full_update = 1;
> >    1469			return UPDATE_TYPE_FULL;
> >    1470		}
> >    1471	
> >    1472		type = get_plane_info_update_type(u);
> >    1473		elevate_update_type(&overall_type, type);
> >    1474	
> >    1475		type = get_scaling_info_update_type(u);
> >    1476		elevate_update_type(&overall_type, type);
> >    1477	
> >    1478		if (u->in_transfer_func)
> >    1479			update_flags->bits.in_transfer_func_change = 1;
> >    1480	
> >    1481		if (u->input_csc_color_matrix)
> >    1482			update_flags->bits.input_csc_change = 1;
> >    1483	
> >    1484		if (u->coeff_reduction_factor)
> >    1485			update_flags->bits.coeff_reduction_change = 1;
> >    1486	
> >    1487		if (u->gamma) {
> >    1488			enum surface_pixel_format format = SURFACE_PIXEL_FORMAT_GRPH_BEGIN;
> >    1489	
> >    1490			if (u->plane_info)
> >    1491				format = u->plane_info->format;
> >    1492			else if (u->surface)
> >                                   ^^^^^^^^^^
> > But the old code assumed it could be NULL
> 
> This looks like a check that could probably be dropped. This function 
> shouldn't be called with a u->surface = NULL - there's even that other 
> deference at the top of the function with &u->surface->update_flags;
> 

Yeah...  Techincally, that's not a dereference though so it doesn't
generate a static checker warning.  (It's just taking the address).
But then we dereference the address on the next line when we do
"update_flags->raw = 0;".  That code is complicated for static analysis.

regards,
dan carpenter

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx