[intel-gvt-linux:gvt-staging 1991/2009] drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4832:10: warning: assignment makes integer from pointer without a cast

David Airlie airlied at redhat.com
Mon Mar 4 22:11:53 UTC 2019


On Tue, Mar 5, 2019 at 8:05 AM Mario Kleiner <mario.kleiner.de at gmail.com> wrote:
>
> Ok,
>
> sorry for the late replies. My mail client somehow didn't show me all
> the conversations around this patch over the weekend :-/.
>
> I had a look at the various trees wrt. to the patch "drm/amd/display:
> Use vrr friendly pageflip throttling in DC."
>
> We apparently have 3 different versions of the patch, modified for
> three different ways that amdgpu DC looks depending on 5.0/5.1/5.2 as
> there has been a lot of refactoring, and then some mixups in which
> patch needs to end up in which branch :-/
>
> The patch in Linux 5.0.0 and current drm-fixes is correct, so the
> released kernel is fine.
>
> The patch in current drm-next (presumably pulled from
> agd5f/drm-next-5.2-wip) and current agd5f/drm-next-5.2-wip and
> drm-next-5.1-wip and drm-next-5.1 is *wrong*. Strangely it doesn't
> correspond to the patch i sent out to the mailing list, and that was
> reviewed-by and tested by Nicholas, which was correct.

Probably my fault, Alex said the patch in his drm-next-5.2-wip was the
one, I pulled it but didn't go back and check for any mailing list
discussion.

I'll make this change in drm-next, I'm trying to decide if it's worth
force pushing it.

Dave.

>
> This is how it looks in above branches:
>
> +               target = (uint32_t)last_flip_vblank + wait_for_vblank;
>
> This is how it should look:
>
> +               target = (uint32_t)last_flip_vblank + *wait_for_vblank;
>
> The patch in Alex current agd5f/amd-staging-drm-next is correct again.
>
> It looks like the mistake comes from pulling the patch unmodified from
> amd-staging-drm-next into drm-next-5.1-wip, drm-next-5.1,
> drm-next-5.2-wip and then into drm-next. There's been big changes in
> the surrounding code between 5.0 and 5.1/5.2-wip and then again a
> smaller change in amd-staging-drm-next, so that the patch that is
> correct in the context of amd-staging-drm-next is wrong in the context
> of the 5.1/5.2/drm-next trees. The change for amd-staging-drm-next was
> changing the function parameter bool *wait_for_vblank into bool
> wait_for_vblank.
>
> But then, the patch that will eventually hopefully go into 5.2
> together with proper fixes for the vblank/pageflip timestampin in VRR
> mode, looks again different from the one in amd-staging-drm-next, as
> it needs to be adapted to the different trigger point for "vblank"
> interrupts at start of the back-porch of vblank instead of start of
> front-porch of vblank. This latest version is only on my disc driver
> atm.
>
> So i think we need above fix for Linux 5.1/drm-next, to be superseded
> for the 5.2 merge window by what is now in
> agd5f/amdgpu-staging-drm-next, which will get again replaced by the
> newest patch asap. and hopefully for the 5.2 merge window.
>
> Not sure what the proper flow for correcting this is? But my original
> patch on the ML should be the correct resolution for Linux 5.1, as
> that has:
>
> +               target = (uint32_t)last_flip_vblank + *wait_for_vblank;
>
> -mario
>
> On Mon, Mar 4, 2019 at 10:55 AM kbuild test robot <lkp at intel.com> wrote:
> >
> > tree:   https://github.com/intel/gvt-linux gvt-staging
> > head:   f3426c668cc5b6b94133ddb4fe60eb91061993c3
> > commit: 289aabbf7bc83a0a268e6e4b24adb0cb9f75d780 [1991/2009] drm/amd/display: Use vrr friendly pageflip throttling in DC.
> > config: i386-randconfig-m3-201909 (attached as .config)
> > compiler: gcc-7 (Debian 7.4.0-5) 7.4.0
> > reproduce:
> >         git checkout 289aabbf7bc83a0a268e6e4b24adb0cb9f75d780
> >         # save the attached .config to linux build tree
> >         make ARCH=i386
> >
> > All warnings (new ones prefixed by >>):
> >
> >    drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 'amdgpu_dm_commit_planes':
> > >> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4832:10: warning: assignment makes integer from pointer without a cast [-Wint-conversion]
> >       target = (uint32_t)last_flip_vblank + wait_for_vblank;
> >              ^
> >
> > vim +4832 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
> >
> >   4625
> >   4626  static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
> >   4627                                      struct dc_state *dc_state,
> >   4628                                      struct drm_device *dev,
> >   4629                                      struct amdgpu_display_manager *dm,
> >   4630                                      struct drm_crtc *pcrtc,
> >   4631                                      bool *wait_for_vblank)
> >   4632  {
> >   4633          uint32_t i, r;
> >   4634          uint64_t timestamp_ns;
> >   4635          struct drm_plane *plane;
> >   4636          struct drm_plane_state *old_plane_state, *new_plane_state;
> >   4637          struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
> >   4638          struct drm_crtc_state *new_pcrtc_state =
> >   4639                          drm_atomic_get_new_crtc_state(state, pcrtc);
> >   4640          struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
> >   4641          struct dm_crtc_state *dm_old_crtc_state =
> >   4642                          to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
> >   4643          int flip_count = 0, planes_count = 0, vpos, hpos;
> >   4644          unsigned long flags;
> >   4645          struct amdgpu_bo *abo;
> >   4646          uint64_t tiling_flags, dcc_address;
> >   4647          uint32_t target, target_vblank;
> >   4648          uint64_t last_flip_vblank;
> >   4649          bool vrr_active = acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE;
> >   4650
> >   4651          struct {
> >   4652                  struct dc_surface_update surface_updates[MAX_SURFACES];
> >   4653                  struct dc_flip_addrs flip_addrs[MAX_SURFACES];
> >   4654                  struct dc_stream_update stream_update;
> >   4655          } *flip;
> >   4656
> >   4657          struct {
> >   4658                  struct dc_surface_update surface_updates[MAX_SURFACES];
> >   4659                  struct dc_plane_info plane_infos[MAX_SURFACES];
> >   4660                  struct dc_scaling_info scaling_infos[MAX_SURFACES];
> >   4661                  struct dc_stream_update stream_update;
> >   4662          } *full;
> >   4663
> >   4664          flip = kzalloc(sizeof(*flip), GFP_KERNEL);
> >   4665          full = kzalloc(sizeof(*full), GFP_KERNEL);
> >   4666
> >   4667          if (!flip || !full) {
> >   4668                  dm_error("Failed to allocate update bundles\n");
> >   4669                  goto cleanup;
> >   4670          }
> >   4671
> >   4672          /* update planes when needed */
> >   4673          for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> >   4674                  struct drm_crtc *crtc = new_plane_state->crtc;
> >   4675                  struct drm_crtc_state *new_crtc_state;
> >   4676                  struct drm_framebuffer *fb = new_plane_state->fb;
> >   4677                  struct amdgpu_framebuffer *afb = to_amdgpu_framebuffer(fb);
> >   4678                  bool pflip_needed;
> >   4679                  struct dc_plane_state *dc_plane;
> >   4680                  struct dm_plane_state *dm_new_plane_state = to_dm_plane_state(new_plane_state);
> >   4681
> >   4682                  /* Cursor plane is handled after stream updates */
> >   4683                  if (plane->type == DRM_PLANE_TYPE_CURSOR)
> >   4684                          continue;
> >   4685
> >   4686                  if (!fb || !crtc || pcrtc != crtc)
> >   4687                          continue;
> >   4688
> >   4689                  new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> >   4690                  if (!new_crtc_state->active)
> >   4691                          continue;
> >   4692
> >   4693                  pflip_needed = old_plane_state->fb &&
> >   4694                          old_plane_state->fb != new_plane_state->fb;
> >   4695
> >   4696                  dc_plane = dm_new_plane_state->dc_state;
> >   4697
> >   4698                  if (pflip_needed) {
> >   4699                          /*
> >   4700                           * Assume even ONE crtc with immediate flip means
> >   4701                           * entire can't wait for VBLANK
> >   4702                           * TODO Check if it's correct
> >   4703                           */
> >   4704                          if (new_pcrtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC)
> >   4705                                  *wait_for_vblank = false;
> >   4706
> >   4707                          /*
> >   4708                           * TODO This might fail and hence better not used, wait
> >   4709                           * explicitly on fences instead
> >   4710                           * and in general should be called for
> >   4711                           * blocking commit to as per framework helpers
> >   4712                           */
> >   4713                          abo = gem_to_amdgpu_bo(fb->obj[0]);
> >   4714                          r = amdgpu_bo_reserve(abo, true);
> >   4715                          if (unlikely(r != 0))
> >   4716                                  DRM_ERROR("failed to reserve buffer before flip\n");
> >   4717
> >   4718                          /*
> >   4719                           * Wait for all fences on this FB. Do limited wait to avoid
> >   4720                           * deadlock during GPU reset when this fence will not signal
> >   4721                           * but we hold reservation lock for the BO.
> >   4722                           */
> >   4723                          r = reservation_object_wait_timeout_rcu(abo->tbo.resv,
> >   4724                                                                  true, false,
> >   4725                                                                  msecs_to_jiffies(5000));
> >   4726                          if (unlikely(r == 0))
> >   4727                                  DRM_ERROR("Waiting for fences timed out.");
> >   4728
> >   4729
> >   4730
> >   4731                          amdgpu_bo_get_tiling_flags(abo, &tiling_flags);
> >   4732
> >   4733                          amdgpu_bo_unreserve(abo);
> >   4734
> >   4735                          flip->flip_addrs[flip_count].address.grph.addr.low_part = lower_32_bits(afb->address);
> >   4736                          flip->flip_addrs[flip_count].address.grph.addr.high_part = upper_32_bits(afb->address);
> >   4737
> >   4738                          dcc_address = get_dcc_address(afb->address, tiling_flags);
> >   4739                          flip->flip_addrs[flip_count].address.grph.meta_addr.low_part = lower_32_bits(dcc_address);
> >   4740                          flip->flip_addrs[flip_count].address.grph.meta_addr.high_part = upper_32_bits(dcc_address);
> >   4741
> >   4742                          flip->flip_addrs[flip_count].flip_immediate =
> >   4743                                          (crtc->state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC) != 0;
> >   4744
> >   4745                          timestamp_ns = ktime_get_ns();
> >   4746                          flip->flip_addrs[flip_count].flip_timestamp_in_us = div_u64(timestamp_ns, 1000);
> >   4747                          flip->surface_updates[flip_count].flip_addr = &flip->flip_addrs[flip_count];
> >   4748                          flip->surface_updates[flip_count].surface = dc_plane;
> >   4749
> >   4750                          if (!flip->surface_updates[flip_count].surface) {
> >   4751                                  DRM_ERROR("No surface for CRTC: id=%d\n",
> >   4752                                                  acrtc_attach->crtc_id);
> >   4753                                  continue;
> >   4754                          }
> >   4755
> >   4756                          if (plane == pcrtc->primary)
> >   4757                                  update_freesync_state_on_stream(
> >   4758                                          dm,
> >   4759                                          acrtc_state,
> >   4760                                          acrtc_state->stream,
> >   4761                                          dc_plane,
> >   4762                                          flip->flip_addrs[flip_count].flip_timestamp_in_us);
> >   4763
> >   4764                          DRM_DEBUG_DRIVER("%s Flipping to hi: 0x%x, low: 0x%x\n",
> >   4765                                           __func__,
> >   4766                                           flip->flip_addrs[flip_count].address.grph.addr.high_part,
> >   4767                                           flip->flip_addrs[flip_count].address.grph.addr.low_part);
> >   4768
> >   4769                          flip_count += 1;
> >   4770                  }
> >   4771
> >   4772                  full->surface_updates[planes_count].surface = dc_plane;
> >   4773                  if (new_pcrtc_state->color_mgmt_changed) {
> >   4774                          full->surface_updates[planes_count].gamma = dc_plane->gamma_correction;
> >   4775                          full->surface_updates[planes_count].in_transfer_func = dc_plane->in_transfer_func;
> >   4776                  }
> >   4777
> >   4778
> >   4779                  full->scaling_infos[planes_count].scaling_quality = dc_plane->scaling_quality;
> >   4780                  full->scaling_infos[planes_count].src_rect = dc_plane->src_rect;
> >   4781                  full->scaling_infos[planes_count].dst_rect = dc_plane->dst_rect;
> >   4782                  full->scaling_infos[planes_count].clip_rect = dc_plane->clip_rect;
> >   4783                  full->surface_updates[planes_count].scaling_info = &full->scaling_infos[planes_count];
> >   4784
> >   4785
> >   4786                  full->plane_infos[planes_count].color_space = dc_plane->color_space;
> >   4787                  full->plane_infos[planes_count].format = dc_plane->format;
> >   4788                  full->plane_infos[planes_count].plane_size = dc_plane->plane_size;
> >   4789                  full->plane_infos[planes_count].rotation = dc_plane->rotation;
> >   4790                  full->plane_infos[planes_count].horizontal_mirror = dc_plane->horizontal_mirror;
> >   4791                  full->plane_infos[planes_count].stereo_format = dc_plane->stereo_format;
> >   4792                  full->plane_infos[planes_count].tiling_info = dc_plane->tiling_info;
> >   4793                  full->plane_infos[planes_count].visible = dc_plane->visible;
> >   4794                  full->plane_infos[planes_count].per_pixel_alpha = dc_plane->per_pixel_alpha;
> >   4795                  full->plane_infos[planes_count].dcc = dc_plane->dcc;
> >   4796                  full->surface_updates[planes_count].plane_info = &full->plane_infos[planes_count];
> >   4797
> >   4798                  planes_count += 1;
> >   4799
> >   4800          }
> >   4801
> >   4802          /*
> >   4803           * TODO: For proper atomic behaviour, we should be calling into DC once with
> >   4804           * all the changes.  However, DC refuses to do pageflips and non-pageflip
> >   4805           * changes in the same call.  Change DC to respect atomic behaviour,
> >   4806           * hopefully eliminating dc_*_update structs in their entirety.
> >   4807           */
> >   4808          if (flip_count) {
> >   4809                  if (!vrr_active) {
> >   4810                          /* Use old throttling in non-vrr fixed refresh rate mode
> >   4811                           * to keep flip scheduling based on target vblank counts
> >   4812                           * working in a backwards compatible way, e.g., for
> >   4813                           * clients using the GLX_OML_sync_control extension or
> >   4814                           * DRI3/Present extension with defined target_msc.
> >   4815                           */
> >   4816                          last_flip_vblank = drm_crtc_vblank_count(pcrtc);
> >   4817                  }
> >   4818                  else {
> >   4819                          /* For variable refresh rate mode only:
> >   4820                           * Get vblank of last completed flip to avoid > 1 vrr
> >   4821                           * flips per video frame by use of throttling, but allow
> >   4822                           * flip programming anywhere in the possibly large
> >   4823                           * variable vrr vblank interval for fine-grained flip
> >   4824                           * timing control and more opportunity to avoid stutter
> >   4825                           * on late submission of flips.
> >   4826                           */
> >   4827                          spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> >   4828                          last_flip_vblank = acrtc_attach->last_flip_vblank;
> >   4829                          spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
> >   4830                  }
> >   4831
> > > 4832                  target = (uint32_t)last_flip_vblank + wait_for_vblank;
> >   4833
> >   4834                  /* Prepare wait for target vblank early - before the fence-waits */
> >   4835                  target_vblank = target - (uint32_t)drm_crtc_vblank_count(pcrtc) +
> >   4836                                  amdgpu_get_vblank_counter_kms(pcrtc->dev, acrtc_attach->crtc_id);
> >   4837
> >   4838                  /*
> >   4839                   * Wait until we're out of the vertical blank period before the one
> >   4840                   * targeted by the flip
> >   4841                   */
> >   4842                  while ((acrtc_attach->enabled &&
> >   4843                          (amdgpu_display_get_crtc_scanoutpos(dm->ddev, acrtc_attach->crtc_id,
> >   4844                                                              0, &vpos, &hpos, NULL,
> >   4845                                                              NULL, &pcrtc->hwmode)
> >   4846                           & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK)) ==
> >   4847                          (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK) &&
> >   4848                          (int)(target_vblank -
> >   4849                            amdgpu_get_vblank_counter_kms(dm->ddev, acrtc_attach->crtc_id)) > 0)) {
> >   4850                          usleep_range(1000, 1100);
> >   4851                  }
> >   4852
> >   4853                  if (acrtc_attach->base.state->event) {
> >   4854                          drm_crtc_vblank_get(pcrtc);
> >   4855
> >   4856                          spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> >   4857
> >   4858                          WARN_ON(acrtc_attach->pflip_status != AMDGPU_FLIP_NONE);
> >   4859                          prepare_flip_isr(acrtc_attach);
> >   4860
> >   4861                          spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
> >   4862                  }
> >   4863
> >   4864                  if (acrtc_state->stream) {
> >   4865
> >   4866                          if (acrtc_state->freesync_timing_changed)
> >   4867                                  flip->stream_update.adjust =
> >   4868                                          &acrtc_state->stream->adjust;
> >   4869
> >   4870                          if (acrtc_state->freesync_vrr_info_changed)
> >   4871                                  flip->stream_update.vrr_infopacket =
> >   4872                                          &acrtc_state->stream->vrr_infopacket;
> >   4873                  }
> >   4874
> >   4875                  mutex_lock(&dm->dc_lock);
> >   4876                  dc_commit_updates_for_stream(dm->dc,
> >   4877                                                       flip->surface_updates,
> >   4878                                                       flip_count,
> >   4879                                                       acrtc_state->stream,
> >   4880                                                       &flip->stream_update,
> >   4881                                                       dc_state);
> >   4882                  mutex_unlock(&dm->dc_lock);
> >   4883          }
> >   4884
> >   4885          if (planes_count) {
> >   4886                  if (new_pcrtc_state->mode_changed) {
> >   4887                          full->stream_update.src = acrtc_state->stream->src;
> >   4888                          full->stream_update.dst = acrtc_state->stream->dst;
> >   4889                  }
> >   4890
> >   4891                  if (new_pcrtc_state->color_mgmt_changed)
> >   4892                          full->stream_update.out_transfer_func = acrtc_state->stream->out_transfer_func;
> >   4893
> >   4894                  acrtc_state->stream->abm_level = acrtc_state->abm_level;
> >   4895                  if (acrtc_state->abm_level != dm_old_crtc_state->abm_level)
> >   4896                          full->stream_update.abm_level = &acrtc_state->abm_level;
> >   4897
> >   4898                  mutex_lock(&dm->dc_lock);
> >   4899                  dc_commit_updates_for_stream(dm->dc,
> >   4900                                                       full->surface_updates,
> >   4901                                                       planes_count,
> >   4902                                                       acrtc_state->stream,
> >   4903                                                       &full->stream_update,
> >   4904                                                       dc_state);
> >   4905                  mutex_unlock(&dm->dc_lock);
> >   4906          }
> >   4907
> >   4908          for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> >   4909                  if (plane->type == DRM_PLANE_TYPE_CURSOR)
> >   4910                          handle_cursor_update(plane, old_plane_state);
> >   4911
> >   4912  cleanup:
> >   4913          kfree(flip);
> >   4914          kfree(full);
> >   4915  }
> >   4916
> >
> > ---
> > 0-DAY kernel test infrastructure                Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation


More information about the intel-gvt-dev mailing list