[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
Mario Kleiner
mario.kleiner.de at gmail.com
Mon Mar 4 22:05:12 UTC 2019
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.
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