[Intel-gfx] drm/i915: Don't use staged config for VLV cdclk calculations

[Dan Carpenter]

[ It seems a bit unfair to email you, but this is the patch that seems
  to actually trigger the warning.  Anyway, the intel-gfx list is also
  CC'd.]

Hello Ander Conselvan de Oliveira,

The patch 304603f47a74: "drm/i915: Don't use staged config for VLV
cdclk calculations" from Apr 2, 2015, leads to the following Smatch
warning:

	drivers/gpu/drm/i915/intel_display.c:12850 intel_crtc_set_config()
	error: we previously assumed 'pipe_config' could be null (see line 12834)

drivers/gpu/drm/i915/intel_display.c
 12808          /* Compute whether we need a full modeset, only an fb base update or no
 12809           * change at all. In the future we might also check whether only the
 12810           * mode changed, e.g. for LVDS where we only change the panel fitter in
 12811           * such cases. */
 12812          intel_set_config_compute_mode_changes(set, config);

This call can set "config->mode_changed" to true.

 12813  
 12814          state = drm_atomic_state_alloc(dev);
 12815          if (!state) {
 12816                  ret = -ENOMEM;
 12817                  goto out_config;
 12818          }
 12819  
 12820          state->acquire_ctx = dev->mode_config.acquire_ctx;
 12821  
 12822          ret = intel_modeset_stage_output_state(dev, set, config, state);
 12823          if (ret)
 12824                  goto fail;
 12825  
 12826          pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
 12827                                                     state,
 12828                                                     &modeset_pipes,
 12829                                                     &prepare_pipes,
 12830                                                     &disable_pipes);
 12831          if (IS_ERR(pipe_config)) {
 12832                  ret = PTR_ERR(pipe_config);
 12833                  goto fail;
 12834          } else if (pipe_config) {
 12835                  if (pipe_config->has_audio !=
 12836                      to_intel_crtc(set->crtc)->config->has_audio)
 12837                          config->mode_changed = true;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here we also set it to true if pipe_config is non-NULL.

 12838  
 12839                  /*
 12840                   * Note we have an issue here with infoframes: current code
 12841                   * only updates them on the full mode set path per hw
 12842                   * requirements.  So here we should be checking for any
 12843                   * required changes and forcing a mode set.
 12844                   */
 12845          }
 12846  
 12847          intel_update_pipe_size(to_intel_crtc(set->crtc));
 12848  
 12849          if (config->mode_changed) {
 12850                  ret = intel_set_mode_pipes(set->crtc, set->mode,
 12851                                             set->x, set->y, set->fb, pipe_config,
                                                                            ^^^^^^^^^^^
Before this patch, then Smatch didn't print a warning because there were
some paths which didn't dereference pipe_config.  The code here seems to
assume that if ->mode_changed is true then pipe_config is non-NULL but
my concern is that it's not considering the call to
intel_set_config_compute_mode_changes().

It's a weird thing that when you look at the code without considering
cross function analysis the code looks correct but when you do cross
function analysis it is very subtle.

 12852                                             modeset_pipes, prepare_pipes,
 12853                                             disable_pipes);
 12854          } else if (config->fb_changed) {
 12855                  struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
 12856                  struct drm_plane *primary = set->crtc->primary;
 12857                  int vdisplay, hdisplay;
 12858  

regards,
dan carpenter