From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752738AbcHKOKw (ORCPT ); Thu, 11 Aug 2016 10:10:52 -0400 Received: from mga04.intel.com ([192.55.52.120]:46350 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750882AbcHKOKu (ORCPT ); Thu, 11 Aug 2016 10:10:50 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,505,1464678000"; d="scan'208";a="1039360892" Subject: Re: [PATCH REBASED v10 6/6] drm/i915/skl: Update DDB values atomically with wms/plane attrs To: Lyude , intel-gfx@lists.freedesktop.org, =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , Matt Roper References: <1470839281-19639-1-git-send-email-cpaul@redhat.com> <1470839281-19639-14-git-send-email-cpaul@redhat.com> Cc: Daniel Vetter , Radhakrishna Sripada , Hans de Goede , Jani Nikula , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org From: Maarten Lankhorst Message-ID: <4a0e7ae6-8adc-24ba-dd19-8c04cfadb803@linux.intel.com> Date: Thu, 11 Aug 2016 16:10:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <1470839281-19639-14-git-send-email-cpaul@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hey, Op 10-08-16 om 16:28 schreef Lyude: > Now that we can hook into update_crtcs and control the order in which we > update CRTCs at each modeset, we can finish the final step of fixing > Skylake's watermark handling by performing DDB updates at the same time > as plane updates and watermark updates. > > The first major change in this patch is skl_update_crtcs(), which > handles ensuring that we order each CRTC update in our atomic commits > properly so that they honor the DDB flush order. > > The second major change in this patch is the order in which we flush the > pipes. While the previous order may have worked, it can't be used in > this approach since it no longer will do the right thing. For example, > using the old ddb flush order: > > We have pipes A, B, and C enabled, and we're disabling C. Initial ddb > allocation looks like this: > > | A | B |xxxxxxx| > > Since we're performing the ddb updates after performing any CRTC > disablements in intel_atomic_commit_tail(), the space to the right of > pipe B is unallocated. > > 1. Flush pipes with new allocation contained into old space. None > apply, so we skip this > 2. Flush pipes having their allocation reduced, but overlapping with a > previous allocation. None apply, so we also skip this > 3. Flush pipes that got more space allocated. This applies to A and B, > giving us the following update order: A, B > > This is wrong, since updating pipe A first will cause it to overlap with > B and potentially burst into flames. Our new order (see the code > comments for details) would update the pipes in the proper order: B, A. > > As well, we calculate the order for each DDB update during the check > phase, and reference it later in the commit phase when we hit > skl_update_crtcs(). > > This long overdue patch fixes the rest of the underruns on Skylake. > > Changes since v1: > - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm() > Changes since v2: > - Use the method for updating CRTCs that Ville suggested > - In skl_update_wm(), only copy the watermarks for the crtc that was > passed to us > Changes since v3: > - Small comment fix in skl_ddb_allocation_overlaps() > > Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration") > Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation") > [omitting CC for stable, since this patch will need to be changed for > such backports first] This series breaks on kms_atomic_transition.plane-all-transition (just uploaded the changed tests to igt, please rebuild) [ 5455.543871] [drm:verify_wm_state.isra.72 [i915]] *ERROR* mismatch in DDB state pipe A plane 1 (expected (0,0), found (0,860)) There's also a WARN_ON(... && total_data_rate == 0) which you need to comment out for the tests to pass cleanly. ~Maarten