Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > "Vivi, Rodrigo" writes: > > > > > Nak. > > > > > > I don't intend to update the symbolic links on linux-firmware.git > > > repository anymore so if we receive a new minor version update we > > > are > > > not going to load. > > > > > > I was the one advocating in the favor for the symbolic link > > > flexibility > > > but I lost the discussions for the stability and validation etc. > > > > > > > And I was one advocating in favor of getting rid of symlink. But the > > filename versioning is superfluous as the contents has the version > > info > > which we can solely rely to not run something we dont want. > > > > So I am not sure what we lose in stability and validation front > > with the strict version check. > > Bisection is more cumbersome with a symlink. Did you miss a without there? Because when bisecting the kernel it's harder without the symlink as the build breaks otherwise and the runtime is not bisectable either. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On Mon, Jul 11, 2016 at 03:45:21PM +0300, Imre Deak wrote: > On ma, 2016-07-11 at 13:39 +0100, ch...@chris-wilson.co.uk wrote: > > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > > "Vivi, Rodrigo" writes: > > > > > > > > > Nak. > > > > > > > > > > I don't intend to update the symbolic links on linux- > > > > > firmware.git > > > > > repository anymore so if we receive a new minor version update > > > > > we > > > > > are > > > > > not going to load. > > > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > > flexibility > > > > > but I lost the discussions for the stability and validation > > > > > etc. > > > > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. But > > > > the > > > > filename versioning is superfluous as the contents has the > > > > version > > > > info > > > > which we can solely rely to not run something we dont want. > > > > > > > > So I am not sure what we lose in stability and validation front > > > > with the strict version check. > > > > > > Bisection is more cumbersome with a symlink. > > > > Did you miss a without there? Because when bisecting the kernel it's > > harder without the symlink as the build breaks otherwise and the > > runtime > > is not bisectable either. > > No, I meant when bisecting we also want to load the proper fw version > for each bisect commit point. So using exact filenames we'll > automatically load the proper firmware file for a given commit, whereas > with symlinks we have to adjust the symlink at each bisect step. You mean you have to adjust the kernel build at each point. That is the root of the problem as we do not have deferred firmware loading. And then you get random changes in the firmare whilst bisecting the kernel. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/dmc: Accept symbolic link in firmware name
On Mon, Jul 11, 2016 at 04:24:50PM +0300, Imre Deak wrote: > On ma, 2016-07-11 at 13:55 +0100, ch...@chris-wilson.co.uk wrote: > > And then you get random changes in the firmare whilst bisecting the > > kernel. > > What do you mean random? During bisecting we want to load the firmware > version that was used with a particular commit. With a symlink pointing > to the wrong firmware file for a given commit, we'd fail loading the > firmware due to the version check and hide/introduce bugs for that > commit. No. You want to be changing exactly one variable, which means leaving the firmware constant. The firmware should be side-ways compatible for everything with the same minor version (thus resolvable from the same symlink), right? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/dp: Clarify clock recovery and channel equalization failures
On Thu, Aug 04, 2016 at 04:50:35PM +, Pandiyan, Dhinakaran wrote:
> On Thu, 2016-08-04 at 04:12 +0100, Chris Wilson wrote:
> > On Wed, Aug 03, 2016 at 08:07:40PM -0700, Dhinakaran Pandiyan wrote:
> > > The causes of clock recovery and channel equalization failures are not
> > > explicitly printed in debug messages. Help debugging link training
> > > failures by printing why it failed.
> > >
> > > Doing this in the driver would mean re-implementing some of the drm static
> > > functions that decode link status. Let's avoid that with these debug
> > > messages in drm.
> > >
> > > Signed-off-by: Dhinakaran Pandiyan
> > > ---
> > > drivers/gpu/drm/drm_dp_helper.c | 12 +---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > > b/drivers/gpu/drm/drm_dp_helper.c
> > > index 091053e..d763b57 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -64,12 +64,16 @@ bool drm_dp_channel_eq_ok(const u8
> > > link_status[DP_LINK_STATUS_SIZE],
> > >
> > > lane_align = dp_link_status(link_status,
> > > DP_LANE_ALIGN_STATUS_UPDATED);
> > > - if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0)
> > > + if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0) {
> > > + DRM_DEBUG_KMS("Inter-lane alignment not done\n");
> > > return false;
> > > + }
> > > for (lane = 0; lane < lane_count; lane++) {
> > > lane_status = dp_get_lane_status(link_status, lane);
> > > - if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS)
> > > + if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS) {
> > > + DRM_DEBUG_KMS("Channel equalization not done for lane
> > > %d\n", lane);
> > > return false;
> > > + }
> > > }
> > > return true;
> > > }
> > > @@ -83,8 +87,10 @@ bool drm_dp_clock_recovery_ok(const u8
> > > link_status[DP_LINK_STATUS_SIZE],
> > >
> > > for (lane = 0; lane < lane_count; lane++) {
> > > lane_status = dp_get_lane_status(link_status, lane);
> > > - if ((lane_status & DP_LANE_CR_DONE) == 0)
> > > + if ((lane_status & DP_LANE_CR_DONE) == 0) {
> > > + DRM_DEBUG_KMS("Clock recovery not done for lane %d\n",
> > > lane);
> >
> > Please petition, with patch, for DRM_DEBUG_DP.
> >
> Is that because DRM_DEBUG_KMS in not appropriate or that we have plenty
> of DP related debug messages that it warrants it's own debug macro?
In the case of link failure, we will have plenty of messages from DP.
One debug category just for DP may be overkill, and we may want
something more like DRM_DEBUG_KMS_LOWLEVEL (or DRM_DEBUG_KMS_DRIVER, or
DRM_DEBUG_KMS_HW etc) that suits all similar link training without
cluttering up either the high levels telling what the user selected, and
what the driver picked and the control flow through the modesetting.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/25] drm/i915/fbc: fix the FBC state checking code
On Mon, Aug 15, 2016 at 10:47:18PM +, Zanoni, Paulo R wrote: > Em Seg, 2016-08-15 às 21:55 +0100, Chris Wilson escreveu: > > So this change is broken as intel_fbc_pre_update() depends upon state > > derived from the pinned framebuffer object but is being called by > > intel_crtc_page_flip() before that object is pinned with > > intel_pin_and_fence_fb - i.e. state such as the gtt_offset or the > > fence > > for the object are incorrect, and even trying to look at them can > > cause > > an oops. > > Nope: > $ git show > 1eb52238a5f5b6a3f497b47e6da39ccfebe6b878:drivers/gpu/drm/i915/intel_dis > play.c > > Back when the commit was merged, the pre_update() call was done after > pin_and_fence_fb(). It looks like some later commit introduced the > problem you point. Hmm, 5a21b6650a239ebc020912968a44047701104159 ? > Still, looks like we have code to fix. Well the good news is that answers the question of whether that code is intended to be safe under the struct_mutex as it once previously was. So we can just rearrange the code to do the pre_update after the pin? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Call intel_fbc_pre_update() after pinning the new pageflip
On Tue, Aug 16, 2016 at 04:49:26PM +, Zanoni, Paulo R wrote:
> Em Ter, 2016-08-16 às 08:43 +0100, Chris Wilson escreveu:
> > intel_fbc_pre_update() depends upon the new state being already
> > pinned
> > in place in the Global GTT (primarily for both fencing which wants
> > both
> > an offset and a fence register, if assigned). This requires the call
> > to
> > intel_fbc_pre_update() be after intel_pin_and_fence_fb() - but commit
> > 5a21b6650a23 ("drm/i915: Revert async unpin and nonblocking atomic
> > commit") seems to have returned the code to a different state.
> >
> > Fixes 5a21b6650a23 ("drm/i915: Revert async unpin and
> > nonblocking...")
>
> I think it actually fixes e8216e502acaad129210c3c8b30cb4ab41e70239
> ("drm/i915/fbc: call intel_fbc_pre_update earlier during page flips").
Hmm, I was pretty sure that the before the revert it was in the right
place, but I could well be mistaken. Certainly that commit sounds more
than a likely suspect.
> > Signed-off-by: Chris Wilson
> > Cc: Daniel Vetter
> > Cc: "Zanoni, Paulo R"
> > Cc: Ville Syrjälä
> > Cc: Maarten Lankhorst
> > Cc: Patrik Jakobsson
> > Cc: drm-intel-fi...@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 37d71d5d2369..916ce7b2d4d7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12061,9 +12061,6 @@ static int intel_crtc_page_flip(struct
> > drm_crtc *crtc,
> > crtc->primary->fb = fb;
> > update_state_fb(crtc->primary);
> >
> > - intel_fbc_pre_update(intel_crtc, intel_crtc->config,
> > - to_intel_plane_state(primary->state));
> > -
> > work->pending_flip_obj = i915_gem_object_get(obj);
> >
> > ret = i915_mutex_lock_interruptible(dev);
> > @@ -12109,6 +12106,9 @@ static int intel_crtc_page_flip(struct
> > drm_crtc *crtc,
> > work->gtt_offset += intel_crtc->dspaddr_offset;
> > work->rotation = crtc->primary->state->rotation;
> >
> > + intel_fbc_pre_update(intel_crtc, intel_crtc->config,
> > + to_intel_plane_state(primary->state));
> > +
>
> After you reported the problem yesterday I wrote almost the exact same
> patch and just didn't submit it because I didn't have time to test it.
> I also added a little comment on top of the call to try to prevent us
> from further regressions:
>
> /* There's the potential that the next frame will not be compatible
> with FBC, so we want to call pre_update() before the actual page flip.
> The problem is that pre_update() caches some information about the fb
> object, so we want to do this only after the object is pinned. Let's be
> on the safe side and do this immediately before scheduling the
> flip. */
>
> With or without this or some other comment:
> Reviewed-by: Paulo Zanoni
Submit yours, and I'll look it over ;)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/fbc: Allow on unfenced surfaces, for recent gen
On Thu, Aug 18, 2016 at 01:56:56PM +, Zanoni, Paulo R wrote: > Em Qui, 2016-08-18 às 09:21 +0100, Chris Wilson escreveu: > > Only fbc1 is tied to using a fence. Later iterations of fbc are more > > flexible and allow operation on unfenced frontbuffers. > > But then we'll lose GTT tracking - which we currently rely on - and I'm > 87.5% sure we'll need to implement some workarounds we skipped. You've missed the patches that point out that the GTT tracking is broken? :) We don't rely on GTT tracking at all because we don't use the tracked GTT address for pwrite or GTT mmap access (and not for WC or CPU mmap access). And CS uses a completely different address space (caveat those older gen in which it looks like the GGTT). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Call intel_fbc_pre_update() after pinning the new pageflip
On Wed, Aug 17, 2016 at 08:00:09PM +, Zanoni, Paulo R wrote:
> Em Qua, 2016-08-17 às 20:49 +0100, Chris Wilson escreveu:
> > On Wed, Aug 17, 2016 at 04:41:44PM -0300, Paulo Zanoni wrote:
> > >
> > > From: Chris Wilson
> > >
> > > intel_fbc_pre_update() depends upon the new state being already
> > > pinned
> > > in place in the Global GTT (primarily for both fencing which wants
> > > both
> > > an offset and a fence register, if assigned). This requires the
> > > call to
> > > intel_fbc_pre_update() be after intel_pin_and_fence_fb() - but
> > > commit
> > > e8216e502aca ("drm/i915/fbc: call intel_fbc_pre_update earlier
> > > during
> > > page flips") moved the code way too much up in its attempt to call
> > > it
> > > before the page flip.
> > >
> > > v2 (from Paulo):
> > > - Point the original bad commit.
> > > - Add a comment to maybe prevent further regressions.
> > >
> > > Fixes: e8216e502aca ("drm/i915/fbc: call intel_fbc_pre_update
> > > earlier...")
> > > Signed-off-by: Chris Wilson
> > > Signed-off-by: Paulo Zanoni
> > > Cc: Daniel Vetter
> > > Cc: Ville Syrjälä
> > > Cc: Maarten Lankhorst
> > > Cc: Patrik Jakobsson
> > > Cc: drm-intel-fi...@lists.freedesktop.org
> >
> > If you had just claimed this as your own, I could have reviewed it ;)
>
> I don't want to steal your patch, you submitted it first, so you should
> get the credit.
Pushed in your name since you did most of the work!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [CI] Revert "drm/i915/fbc: Allow on unfenced surfaces, for recent gen"
On Wed, Aug 24, 2016 at 09:14:59PM +, Zanoni, Paulo R wrote: > Em Qua, 2016-08-24 às 19:00 +0100, Chris Wilson escreveu: > > . In the meantime lets hope that all > > framebuffers are idle and naturally fit within the mappable aperture. > > What exactly do you mean with the sentence above? Is there some other > bug you spotted? Please share the information. Not all framebuffers will be fenced, especially if they are being rendered to before being flipped to the first time. The pressure is less for full-ppgtt systems (now that we have independent activity tracking on the vma) but that is not sufficient to guarrantee the object will be assigned a map_and_fenceable VMA for display. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [[PATCH ddx]] intel: Adding Marketing names for Skylake, Kabylake and Apollolake/Broxton.
On Thu, Nov 17, 2016 at 09:03:42PM +, Vivi, Rodrigo wrote:
> On Thu, 2016-11-17 at 19:35 +, Chris Wilson wrote:
> > On Thu, Nov 17, 2016 at 11:06:54AM -0800, Rodrigo Vivi wrote:
> > > On Thu, Nov 17, 2016 at 10:53:04AM +0200, David Weinehall wrote:
> > > > On Tue, Nov 15, 2016 at 02:21:01PM -0800, Rodrigo Vivi wrote:
> > > > > This commit adding all known marketing names for latest gen9
> > > > > platforms.
> > > > >
> > > > > Cc: Chris Wilson
> > > > > Signed-off-by: Rodrigo Vivi
> > > > > ---
> > > > > README | 2 +-
> > > > > man/intel.man | 2 +-
> > > > > src/intel_module.c | 29 -
> > > > > 3 files changed, 30 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/README b/README
> > > > > index cf4d88d..e58477d 100644
> > > > > --- a/README
> > > > > +++ b/README
> > > > > @@ -15,7 +15,7 @@ Intel graphics chipsets including:
> > > > > G/Q33,G/Q35,G41,G/Q43,G/GM/Q45
> > > > > PineView-M (Atom N400 series)
> > > > > PineView-D (Atom D400/D500 series)
> > > > > - Intel(R) HD Graphics: 2000-6000,
> > > > > + Intel(R) HD Graphics: 500-6000/P530/P555/P580,
> > > > > Intel(R) Iris(TM) Graphics: 5100/6100, and
> > > > > Intel(R) Iris(TM) Pro Graphics: 5200/6200/P6300.
> > > > >
> > > > > diff --git a/man/intel.man b/man/intel.man
> > > > > index 8da496e..16cc5d9 100644
> > > > > --- a/man/intel.man
> > > > > +++ b/man/intel.man
> > > > > @@ -27,7 +27,7 @@ supports the i810, i810-DC100, i810e, i815, i830M,
> > > > > 845G, 852GM, 855GM,
> > > > > 865G, 915G, 915GM, 945G, 945GM, 965G, 965Q, 946GZ, 965GM, 945GME,
> > > > > G33, Q33, Q35, G35, GM45, G45, Q45, G43, G41 chipsets, Pineview-M in
> > > > > Atom N400 series, Pineview-D in Atom D400/D500 series,
> > > > > -Intel(R) HD Graphics: 2000-6000,
> > > > > +Intel(R) HD Graphics: 500-6000/P530/P555/P580,
> > > > > Intel(R) Iris(TM) Graphics: 5100/6100, and
> > > > > Intel(R) Iris(TM) Pro Graphics: 5200/6200/P6300.
> > > > >
> > > > > diff --git a/src/intel_module.c b/src/intel_module.c
> > > > > index e443c9e..86b4aae 100644
> > > > > --- a/src/intel_module.c
> > > > > +++ b/src/intel_module.c
> > > > > @@ -272,6 +272,33 @@ static const SymTabRec intel_chipsets[] = {
> > > > > {0x22b2, "HD Graphics"},
> > > > > {0x22b3, "HD Graphics"},
> > > > >
> > > > > + /* Skylake */
> > > > > + {0x1902, "HD Graphics 510"},
> > > > > + {0x1906, "HD Graphics 510"},
> > > > > + {0x190B, "HD Graphics 510"},
> > > > > + {0x1912, "HD Graphics 530"},
> > > > > + {0x1916, "HD Graphics 520"},
> > > > > + {0x191B, "HD Graphics 530"},
> > > > > + {0x191D, "HD Graphics P530"},
> > > > > + {0x191E, "HD Graphics 515"},
> > > > > + {0x1921, "HD Graphics 520"},
> > > > > + {0x1926, "Iris Graphics 540"},
> > > > > + {0x1927, "Iris Graphics 550"},
> > > > > + {0x192B, "Iris Graphics 555"},
> > > > > + {0x192D, "Iris Graphics P555"},
> > > > > + {0x1932, "Iris Pro Graphics 580"},
> > > > > + {0x193A, "Iris Pro Graphics P580"},
> > > > > + {0x193B, "Iris Pro Graphics 580"},
> > > > > + {0x193D, "Iris Pro Graphics P580"},
> > > > > +
> > > > > + /* Broxton (Apollolake) */
> > > > > + {0x5A84, "HD Graphics 505"},
> > > > > + {0x5A85, "HD Graphics 500"},
> > > > > +
> > > > > + /* Kabylake */
> > > > > + {0x5916, "HD Graphics 620"},
> > > > > + {0x591E, "HD Graphics 615"},
> > > > > +
> > > > > /* When adding new identifiers, also update:
> > > > >* 1. intel_identify()
> > > > >* 2. man/intel.man
> > > > > @@ -465,7 +492,7 @@ static void intel_identify(int flags)
> > > > > if (unique != stack)
> > > > > free(unique);
> > > > >
> > > > > - xf86Msg(X_INFO, INTEL_NAME ": Driver for Intel(R) HD Graphics:
> > > > > 2000-6000\n");
> > > > > + xf86Msg(X_INFO, INTEL_NAME ": Driver for Intel(R) HD Graphics:
> > > > > 500-6000/P530/P555/P580\n");
> > > > > xf86Msg(X_INFO, INTEL_NAME ": Driver for Intel(R) Iris(TM)
> > > > > Graphics: 5100, 6100\n");
> > > > > xf86Msg(X_INFO, INTEL_NAME ": Driver for Intel(R) Iris(TM) Pro
> > > > > Graphics: 5200, 6200, P6300\n");
> > > >
> > > > You missed the Iris & Iris Pro models.
> > >
> > > Thanks
> > >
> > > >
> > > > Also, might it make sense to use 5xx, P5xx instead?
> > >
> > > What about ont those generic lists we just kill all numbers and let just:
> > > "
> > >Intel(R) HD Graphics.
> > >Intel(R) Iris(TM) Graphics.
> > >Intel(R) Iris(TM) Pro Graphics.
> > > '
> > > ?
> > >
> > > Chris?
> >
> > Seems reasonable, and as demonstrated in the past simply listing
> > everything becomes unwieldy, if not unfathomable, quickly.
> >
> > I honestly don't think the listing here is the primary source for
> > compatibility info
>
> agree
>
> > (since to get here means that the driver recognised
> > the chipset anyway).
>
> If it is already recognized at this int
Re: [Intel-gfx] [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC
On Thu, Mar 24, 2016 at 08:53:21PM +, Zanoni, Paulo R wrote: > Em Qui, 2016-03-24 às 19:31 +, Chris Wilson escreveu: > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote: > > > > > > FBC and the frontbuffer tracking infrastructure were designed > > > assuming > > > that user space applications would follow a specific set of rules > > > regarding frontbuffer management and mmapping. I recently > > > discovered > > > that current user space is not exactly following these rules: my > > > investigation led me to the conclusion that the generic backend > > > from > > > SNA - used by SKL and the other new platforms without a specific > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the > > > CPU > > > and WC mmaps. I discovered this when running lightdm: I would type > > > the > > > password and nothing would appear on the screen unless I moved the > > > mouse over the place where the letters were supposed to appear. > > Yes, that is a kernel bug. The protocol we said the kernel would > > follow > > is to disable FBC/WC when userspace marks the object for writing by > > the > > CPU and would only reestablish FBC/WC upon dirtyfb. > > But on WC mmaps we mark the object for writing by the GTT instead of > the CPU, and while the tracking engine is able to see "normal" GTT mmap > writes, it's not able to see WC mmap writes, so we established that > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which is > something that the DDX never implemented. This was discussed on #intel- > gfx on Nov 5 2014, and also possibly other places, but I can't find the > logs. Daniel also confirmed this to me again on private IRC on Jun 16 > 2015. So I still don't understand why this is a Kernel bug instead of a > DDX bug. Because we said that once invalidated, it would not be restored until dirtyfb. The kernel is not doing that. Your patch does not do that. To be even close, you should be setting the origin flag based on the existence of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you are not implementing even close to the protocol you say you are. That is invalidate on set-domain, flush on dirtyfb. The kernel's bug is that is not cancelling FBC. Userspace's bug is not signalling when to reenable it. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC
On Thu, Mar 24, 2016 at 09:03:59PM +, ch...@chris-wilson.co.uk wrote:
> On Thu, Mar 24, 2016 at 08:53:21PM +, Zanoni, Paulo R wrote:
> > Em Qui, 2016-03-24 às 19:31 +, Chris Wilson escreveu:
> > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > >
> > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > assuming
> > > > that user space applications would follow a specific set of rules
> > > > regarding frontbuffer management and mmapping. I recently
> > > > discovered
> > > > that current user space is not exactly following these rules: my
> > > > investigation led me to the conclusion that the generic backend
> > > > from
> > > > SNA - used by SKL and the other new platforms without a specific
> > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the
> > > > CPU
> > > > and WC mmaps. I discovered this when running lightdm: I would type
> > > > the
> > > > password and nothing would appear on the screen unless I moved the
> > > > mouse over the place where the letters were supposed to appear.
> > > Yes, that is a kernel bug. The protocol we said the kernel would
> > > follow
> > > is to disable FBC/WC when userspace marks the object for writing by
> > > the
> > > CPU and would only reestablish FBC/WC upon dirtyfb.
> >
> > But on WC mmaps we mark the object for writing by the GTT instead of
> > the CPU, and while the tracking engine is able to see "normal" GTT mmap
> > writes, it's not able to see WC mmap writes, so we established that
> > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which is
> > something that the DDX never implemented. This was discussed on #intel-
> > gfx on Nov 5 2014, and also possibly other places, but I can't find the
> > logs. Daniel also confirmed this to me again on private IRC on Jun 16
> > 2015. So I still don't understand why this is a Kernel bug instead of a
> > DDX bug.
>
> Because we said that once invalidated, it would not be restored until
> dirtyfb. The kernel is not doing that. Your patch does not do that. To
> be even close, you should be setting the origin flag based on the existence
> of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you
> are not implementing even close to the protocol you say you are. That is
> invalidate on set-domain, flush on dirtyfb.
>
> The kernel's bug is that is not cancelling FBC. Userspace's bug is not
> signalling when to reenable it.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8dec2e8..0314346 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2145,6 +2145,7 @@ struct drm_i915_gem_object {
unsigned int cache_dirty:1;
unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+ unsigned int has_wc_mmap:1;
/** Count of VMA actually bound by this object */
unsigned int bind_count;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05ae706..29ca96d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1310,6 +1310,13 @@ unlock:
return ret;
}
+static enum fb_op_origin
+write_origin(struct drm_i915_gem_object *obj, unsigned domain)
+{
+ return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
+ ORIGIN_GTT : ORIGIN_CPU;
+}
+
/**
* Called when user space prepares to use an object with the CPU, either
* through the mmap ioctl's mapping or a GTT mapping.
@@ -1363,9 +1370,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void
*data,
ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
if (write_domain != 0)
- intel_fb_obj_invalidate(obj,
- write_domain == I915_GEM_DOMAIN_GTT ?
- ORIGIN_GTT : ORIGIN_CPU);
+ intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
unref:
drm_gem_object_unreference(&obj->base);
@@ -1466,6 +1471,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
else
addr = -ENOMEM;
up_write(&mm->mmap_sem);
+
+ /* This may race, but that's ok, it only gets set */
+ to_intel_bo(obj)->has_wc_mmap = true;
}
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC
On Thu, Mar 24, 2016 at 09:21:49PM +, Zanoni, Paulo R wrote: > Em Qui, 2016-03-24 às 21:03 +0000, ch...@chris-wilson.co.uk escreveu: > > On Thu, Mar 24, 2016 at 08:53:21PM +, Zanoni, Paulo R wrote: > > > > > > Em Qui, 2016-03-24 às 19:31 +, Chris Wilson escreveu: > > > > > > > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote: > > > > > > > > > > > > > > > FBC and the frontbuffer tracking infrastructure were designed > > > > > assuming > > > > > that user space applications would follow a specific set of > > > > > rules > > > > > regarding frontbuffer management and mmapping. I recently > > > > > discovered > > > > > that current user space is not exactly following these rules: > > > > > my > > > > > investigation led me to the conclusion that the generic backend > > > > > from > > > > > SNA - used by SKL and the other new platforms without a > > > > > specific > > > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using > > > > > the > > > > > CPU > > > > > and WC mmaps. I discovered this when running lightdm: I would > > > > > type > > > > > the > > > > > password and nothing would appear on the screen unless I moved > > > > > the > > > > > mouse over the place where the letters were supposed to appear. > > > > Yes, that is a kernel bug. The protocol we said the kernel would > > > > follow > > > > is to disable FBC/WC when userspace marks the object for writing > > > > by > > > > the > > > > CPU and would only reestablish FBC/WC upon dirtyfb. > > > But on WC mmaps we mark the object for writing by the GTT instead > > > of > > > the CPU, and while the tracking engine is able to see "normal" GTT > > > mmap > > > writes, it's not able to see WC mmap writes, so we established that > > > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which > > > is > > > something that the DDX never implemented. This was discussed on > > > #intel- > > > gfx on Nov 5 2014, and also possibly other places, but I can't find > > > the > > > logs. Daniel also confirmed this to me again on private IRC on Jun > > > 16 > > > 2015. So I still don't understand why this is a Kernel bug instead > > > of a > > > DDX bug. > > Because we said that once invalidated, it would not be restored until > > dirtyfb. The kernel is not doing that. Your patch does not do that. > > To > > be even close, you should be setting the origin flag based on the > > existence > > of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you > > are not implementing even close to the protocol you say you are. That > > is > > invalidate on set-domain, flush on dirtyfb. > > I don't recall this being said in the earlier conversations, but now > that you point it, it can be done. Also, we recently pinged/emailed you > many times about this problem, so I wonder why it took you so long to > point this... All that needed to be said had been, I felt I would have been repeating myself and earlier discussions. > > The kernel's bug is that is not cancelling FBC. Userspace's bug is > > not > > signalling when to reenable it. > > So at least you agree user space was missing something :) Yes. It has sat there waiting for the missing piece to be added for 3 years. (Almost 3 years, 2 years 9 months.) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC
On Fri, Mar 25, 2016 at 02:05:42PM +, Zanoni, Paulo R wrote: > What if I have both a WC mmap and a GTT mmap, and I'm actually using > the GTT mmap now? My set_domain call will be treated as WC mmap usage, > while in fact it should be treated as GTT usage. Is there a way to > differentiate between them with the current set_domain API? No, there is not. As far as the cache domain is regarded WC/GTT appear to be identical, i.e. GTT is just WC. That the GTT is not as coherent as previously regarded is yet another bug. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC
On Tue, Mar 29, 2016 at 01:55:02PM +0200, Daniel Vetter wrote: > *Blonk* or whatever the sound for suddenly realization is. Totally forgot > that we're reuseding set_domain(GTT) for wc mmaps because this it's a nice > trick. > > Otoh, is that trick the reason why wc mmaps aren't coherent enough? One > possible difference is that this won't do the magic chipset flush in > intel-gtt.c that we have on gen3-5. Let's pretend wbinv doesn't exist on > gen2 ;-) But that's just an aside really ... No, writes through the *GTT* are not coherent. igt/gem_mmap_wc/coherency -> PASS igt/gem_mmap_gtt/coherency -> FAIL on byt/bsw Also see patch on list to insert a magic delay that fixes the issue exposed in the kernel (which causes various hangs and EINVALs). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [CI 10/21] drm/i915: Mark up all locked waiters
On Tue, Sep 13, 2016 at 03:21:48PM +0300, Mika Kuoppala wrote: > Mika Kuoppala writes: > > "Zanoni, Paulo R" writes: > >>> +#if IS_ENABLED(CONFIG_LOCKDEP) > >>> + GEM_BUG_ON(!!lockdep_is_held(&req->i915->drm.struct_mutex) > >>> != > >>> + !!(flags & I915_WAIT_LOCKED)); > >>> +#endif > >> > >> I'm hitting this on my SKL. It usually happens right after the login, > >> when I click the terminal icon on the toolbar in order to open it (on > >> Cinnamon). When it doesn't happen at that time, I just open a browser > >> and browse for like 30 seconds until it happens. > >> > >> I did manual reverts of this series up to this patch and obviously the > >> problem goes away (no more GEM_BUG_ON to hit). I didn't try to do a > >> real bisect to check if this is the first bad commit or if it's > >> something later on the series. It could even be an older problem that > >> just got exposed by the new BUG(). I'm available to test patches, just > >> send them to me. > > > > No patches yet but there is a comment on > > intel_prepare_plane_fb() that says that it must be called with mutex > > held. > > > > Quite likely the new GEM_BUG_ON catches this not being true. > > > > As Chris pointed out in irc, its about the reverse. Waiting > with mutex when you shouldn't. Fwiw, the fix is now on the list. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [CI 10/21] drm/i915: Mark up all locked waiters
On Thu, Sep 22, 2016 at 04:05:51PM -0300, Paulo Zanoni wrote: > Em Qua, 2016-09-14 às 09:40 +0100, ch...@chris-wilson.co.uk escreveu: > > On Tue, Sep 13, 2016 at 03:21:48PM +0300, Mika Kuoppala wrote: > > > > > > Mika Kuoppala writes: > > > > > > > > "Zanoni, Paulo R" writes: > > > > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_LOCKDEP) > > > > > > + GEM_BUG_ON(!!lockdep_is_held(&req->i915- > > > > > > >drm.struct_mutex) > > > > > > != > > > > > > + !!(flags & I915_WAIT_LOCKED)); > > > > > > +#endif > > > > > > > > > > I'm hitting this on my SKL. It usually happens right after the > > > > > login, > > > > > when I click the terminal icon on the toolbar in order to open > > > > > it (on > > > > > Cinnamon). When it doesn't happen at that time, I just open a > > > > > browser > > > > > and browse for like 30 seconds until it happens. > > > > > > > > > > I did manual reverts of this series up to this patch and > > > > > obviously the > > > > > problem goes away (no more GEM_BUG_ON to hit). I didn't try to > > > > > do a > > > > > real bisect to check if this is the first bad commit or if it's > > > > > something later on the series. It could even be an older > > > > > problem that > > > > > just got exposed by the new BUG(). I'm available to test > > > > > patches, just > > > > > send them to me. > > > > > > > > No patches yet but there is a comment on > > > > intel_prepare_plane_fb() that says that it must be called with > > > > mutex > > > > held. > > > > > > > > Quite likely the new GEM_BUG_ON catches this not being true. > > > > > > > > > > As Chris pointed out in irc, its about the reverse. Waiting > > > with mutex when you shouldn't. > > > > Fwiw, the fix is now on the list. > > Can you please tell us which patch it is? I see we still have this > problem. https://patchwork.freedesktop.org/series/12703/ Note that the problem is only shown when you enable the BUG_ON. The impact otherwise is very low. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp/mst: fix kernel oops when turning off secondary monitor
On Mon, Dec 05, 2016 at 09:39:49PM +, Pandiyan, Dhinakaran wrote:
> On Mon, 2016-12-05 at 08:02 +, Chris Wilson wrote:
> > On Sun, Dec 04, 2016 at 07:31:18PM -0600, Pierre-Louis Bossart wrote:
> > > 100% reproducible issue found on SKL SkullCanyon NUC with two external
> > > DP daisy-chained monitors in DP/MST mode. When turning off or changing
> > > the input of the second monitor the machine stops with a kernel
> > > oops. This issue happened with 4.8.8 as well as drm/drm-intel-nightly.
> > >
> > > This issue is traced to an inconsistent control flow in
> > > drm_dp_update_payload_part1(): the 'port' pointer is set to NULL at
> > > the same time as'req_payload.num_slots' is set to zero, but the pointer
> > > is dereferenced even when req_payload.num_slot is zero.
> > >
> > > Fix by adding test condition to make sure both variables
> > > are used consistently. This removes the kernel oops.
> > >
> > > There are still annoying cases where the primary display goes black
> > > when the secondary display is turned off but it can be recovered from
> > > by playing with the monitor inputs and power buttons.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990
> > > Signed-off-by: Pierre-Louis Bossart
> > > ---
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index aa64448..5481fde 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1815,7 +1815,7 @@ int drm_dp_update_payload_part1(struct
> > > drm_dp_mst_topology_mgr *mgr)
> > > drm_dp_create_payload_step1(mgr,
> > > mgr->proposed_vcpis[i]->vcpi, &req_payload);
> > > mgr->payloads[i].num_slots =
> > > req_payload.num_slots;
> > > mgr->payloads[i].vcpi = req_payload.vcpi;
> > > - } else if (mgr->payloads[i].num_slots) {
> > > + } else if (mgr->payloads[i].num_slots && port != NULL) {
> > > mgr->payloads[i].num_slots = 0;
> > > drm_dp_destroy_payload_step1(mgr, port,
> > > port->vcpi.vcpi, &mgr->payloads[i]);
> >
> > s/port->vcpi.vcpi/mgr->payloads[i].vcpi/ here looks to be the correct
> > fix.
> > -Chris
> >
>
> Hmm, not sure if that is the correct fix either. With port = NULL,
> doesn't look like we send drm_dp_payload_send_msg(..., pbn = 0).
> Although, we do update the payload table via DPCD
I'll leave that for you to work out if something needs to be sent after
the downstream disappeared.
> Also, if port is set to NULL, I wonder if we are messing up the
> reference counting. Because, this is done below.
> ...
> if (port)
> drm_dp_put_port(port);
This is ok, as this is clearing the local ref which is only obtained if
port != NULL. (If port == NULL, we have no local ref which we need to
put.)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/psr: Fix compiler warnings for hsw_psr_disable()
On Tue, Jan 17, 2017 at 07:05:04PM +, Vivi, Rodrigo wrote: > same for the other... > dm old gcc... > > Also my bad here since I asked Vathsala to organize like this... > > Anyways, for this patch > Reviewed-by: Rodrigo Vivi Ta, another couple of warnings squashed. We are not that far from being able to run CI with W=1, which will help keep us out of the eyes of the kbuild static analysers. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake
On Fri, Jun 17, 2016 at 07:02:57PM +, Zanoni, Paulo R wrote:
> Em Sex, 2016-06-17 às 17:39 +0100, Chris Wilson escreveu:
> > Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are
> > Enabled
> >
> > "Display flickering may occur when both FBC (Frame Buffer
> > Compression)
> > and VT - d (Intel® Virtualization Technology for Directed I/O) are
> > enabled
> > and in use by the display controller."
>
> Ouch...
>
> >
> > Signed-off-by: Chris Wilson
> > Cc: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/intel_fbc.c | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index fddba1eed5ad..e47785467220 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct
> > drm_i915_private *dev_priv)
> > dev_priv->fbc.visible_pipes_mask |= (1 <<
> > crtc->pipe);
> > }
> >
> > +static bool need_vtd_wa(struct drm_i915_private *dev_priv)
>
> Notice we have a function with the exact same name in intel_display.c.
It's amazing how often we need to write w/a for vtd. I wasn't feeling
imaginative and the uniformity may be useful?
> > +{
> > +#ifdef CONFIG_INTEL_IOMMU
> > + if (!intel_iommu_gfx_mapped)
> > + return false;
> > +
> > + if (INTEL_GEN(dev_priv) == 9)
> > + return true;
> > +#endif
> > + return false;
> > +}
> > +
> > /**
> > * intel_fbc_init - Initialize FBC
> > * @dev_priv: the i915 device
> > @@ -1247,6 +1259,12 @@ void intel_fbc_init(struct drm_i915_private
> > *dev_priv)
> > fbc->active = false;
> > fbc->work.scheduled = false;
> >
> > + if (need_vtd_wa(dev_priv)) {
> > + struct intel_device_info *info =
> > + (struct intel_device_info *)&dev_priv->info;
> > + info->has_fbc = false;
> > + }
>
> I just find this piece above a little not-so-beautiful and possibly
> confusing for people debugging failures and/or IGT. Alternatives:
Honestly, I thought you were going to suggest something more like:
#define mkwrite_intel_info(ptr) ((struct intel_device_info *)&(ptr)->info)
if (need_vta_wa(dev_priv))
mkwrite_intel_info(dev_priv)->has_fbc = false;
We have a few other places that require such a beast.
> 1 - Move the check to intel_fbc_can_choose(), so we can give a nice
> no_fbc_reason. Problem: this would not be as efficient as what you
> wrote.
>
> 2 - Move the check to intel_sanitize_fbc_option(), which is reviewed
> but not merged yet. Problem: same as 1.
>
> 3 - Patch fbc_supported() and make the line below call fbc_supported()
> instead of HAS_FBC(). Problem: we call it many times.
>
> 4 - Create dev_priv->fbc.is_supported (or some other meaningful name),
> set it at init time, and make fbc_supported() use it (or just replace
> fbc_supported() calls with the variable check).
>
> All solutions above would probably require some adjustments to debugfs
> and/or IGT (which relies on debugfs), but at least they wouldn't
> surprise users or IGT runners with "why does it say SKL is not
> supported by FBC?".
Or, we use DRM_INFO() and explain the quirk, which is what I should have
done (since that is the mo for applying quirks).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC
On Fri, Nov 13, 2015 at 09:17:04PM +, Zanoni, Paulo R wrote:
> Em Sex, 2015-11-13 às 21:03 +, Chris Wilson escreveu:
> > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > Instead of waiting for 50ms, just wait until the next vblank, since
> > > it's the minimum requirement.
> > >
> > > This moves PC7 residency on my specific BDW machine running
> > > Cinnamon
> > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > > 3200x1800 eDP panel. Notice: this was the case when the patch was
> > > originally proposed, the order of the FBC patches changed since
> > > then,
> > > so the actual numbers might be slightly different now.
> > >
> > > v2:
> > > - Rebase after changing the patch order.
> > > - Update the commit message.
> > >
> > > Signed-off-by: Paulo Zanoni
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > drivers/gpu/drm/i915/intel_fbc.c | 12 +++-
> > > 2 files changed, 4 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 9418bd5..ea08714 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > >
> > > struct intel_fbc_work {
> > > bool scheduled;
> > > + u32 scheduled_vblank;
> > > struct work_struct work;
> > > struct drm_framebuffer *fb;
> > > - unsigned long enable_jiffies;
> > > } work;
> > >
> > > const char *no_fbc_reason;
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index aa82075..72de8a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > work_struct *__work)
> > > container_of(__work, struct drm_i915_private,
> > > fbc.work.work);
> > > struct intel_fbc_work *work = &dev_priv->fbc.work;
> > > struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > - unsigned long delay_jiffies = msecs_to_jiffies(50);
> > >
> > > retry:
> > > /* Delay the actual enabling to let pageflipping cease and
> > > the
> > > @@ -400,14 +399,9 @@ retry:
> > > * vblank to pass after disabling the FBC before we
> > > attempt
> > > * to modify the control registers.
> > > *
> > > - * A more complicated solution would involve tracking
> > > vblanks
> > > - * following the termination of the page-flipping sequence
> > > - * and indeed performing the enable as a co-routine and
> > > not
> > > - * waiting synchronously upon the vblank.
> > > - *
> > > * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > > */
> > > - wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > delay_jiffies);
> > > + intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > >
> > > mutex_lock(&dev_priv->fbc.lock);
> > >
> > > @@ -416,7 +410,7 @@ retry:
> > > goto out;
> > >
> > > /* Were we delayed again while this function was sleeping?
> > > */
> > > - if (time_after(work->enable_jiffies + delay_jiffies,
> > > jiffies)) {
> > > + if (drm_crtc_vblank_get(&crtc->base) == work-
> > > >scheduled_vblank) {
> > > mutex_unlock(&dev_priv->fbc.lock);
> > > goto retry;
> > > }
> > > @@ -449,7 +443,7 @@ static void
> > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > * jiffy count. */
> > > work->fb = crtc->base.primary->fb;
> > > work->scheduled = true;
> > > - work->enable_jiffies = jiffies;
> > > + work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > >base);
> >
> > Isn't the frame counter only incrementing whilst the vblank IRQ is
> > enabled? Ville?
>
> At the work function we call intel_wait_for_vblank(), which calls
> drm_wait_one_vblank(), which calls drm_vblank_get().
And drm_vblank_put...
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/11] Yet another FBC series, v3 part 2 v2
On Wed, Dec 02, 2015 at 05:19:33PM +, Zanoni, Paulo R wrote: > Em Qua, 2015-12-02 às 12:37 +, Chris Wilson escreveu: > > My r-b still stand. > > What about patch 6? Any concerns or additional requests? > drm/i915: alloc/free the FBC CFB during enable/disable I thought I had spotted one without a tag! The code looks fine, the explanation in the changelog sounds ok, and the logging is present for anyway chasing an issue in 6 months, so Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: Enable PSR by default.
On Tue, Mar 24, 2015 at 08:55:04PM +, Vivi, Rodrigo wrote: > On Tue, 2015-03-24 at 10:08 +, Chris Wilson wrote: > > On Tue, Mar 24, 2015 at 11:03:30AM +0100, Daniel Vetter wrote: > > > On Mon, Mar 23, 2015 at 01:20:07PM -0700, Rodrigo Vivi wrote: > > > > Hi Daniel, > > > > > > > > Is something missing to enable it by default? > > > > > > Patch 1 has a small comment from me and latest version of patch 2 lacks an > > > r-b afaict. Hence why I didn't pull in the series yet. But it's also a > > > longer discussion, so ca you please resend the entire series with r-b tags > > > added to make sure I don't pick up the wrong versions? > > > > Do I yet have a method to tell when PSR is active on an output? > > Is that pre computed pipe_config->psr_enabled enough or you mean the > immediate psr.active also exposed there on pipe_config? I want a property exposed to userspace. Having a tristate that says unsupported, disabled, active would be most useful. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: Enable PSR by default.
On Wed, Mar 25, 2015 at 07:27:35PM +, Vivi, Rodrigo wrote: > On Tue, 2015-03-24 at 22:05 +0000, ch...@chris-wilson.co.uk wrote: > > On Tue, Mar 24, 2015 at 08:55:04PM +, Vivi, Rodrigo wrote: > > > On Tue, 2015-03-24 at 10:08 +, Chris Wilson wrote: > > > > On Tue, Mar 24, 2015 at 11:03:30AM +0100, Daniel Vetter wrote: > > > > > On Mon, Mar 23, 2015 at 01:20:07PM -0700, Rodrigo Vivi wrote: > > > > > > Hi Daniel, > > > > > > > > > > > > Is something missing to enable it by default? > > > > > > > > > > Patch 1 has a small comment from me and latest version of patch 2 > > > > > lacks an > > > > > r-b afaict. Hence why I didn't pull in the series yet. But it's also a > > > > > longer discussion, so ca you please resend the entire series with r-b > > > > > tags > > > > > added to make sure I don't pick up the wrong versions? > > > > > > > > Do I yet have a method to tell when PSR is active on an output? > > > > > > Is that pre computed pipe_config->psr_enabled enough or you mean the > > > immediate psr.active also exposed there on pipe_config? > > > > I want a property exposed to userspace. Having a tristate that says > > unsupported, disabled, active would be most useful. > > I got your patch that added that and rebased on currently intel_psr.c > changing transition and other small things and put here: > > http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=intel_psr&id=b99147c4849668a04e794003746fa22708406f56 > > But even with PSR transitioning well from idle to active, xrandr prop > always shows me as idle: You'll be falling foul of property caching. My immediate concern is simply knowing when PSR is enabled on an output as a guide to knowing when to avoid frontbuffer access. But IDLE/ACTIVE sound fun as well. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Attach a PSR property on eDP
On Wed, Mar 25, 2015 at 08:37:29PM +, Vivi, Rodrigo wrote: > Also talking about visible names I'm not sure about "Idle" as well... > Every time I read it get confused... I believe it is because PSR active > needs Idle usage... > > What do you think about changing to Idle to Enable-Exit and Active to > Enable-Active? If we switch states rapidly, we don't want to change the output property (as it will trigger uevents, or at least it should). So just "Enabled" ? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests/drm_hw_lock: Tests for hw_lock fixes.
On Tue, Apr 28, 2015 at 09:28:51AM +, Antoine, Peter wrote: > Also, the PARAM should give legitimate code the opportunity to avoid the > problems but avoiding these features completely. There are no legitimate users. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm: Kernel Crash in drm_unlock
On Tue, Apr 28, 2015 at 10:21:49AM +0100, Dave Gordon wrote: > On 24/04/15 06:52, Antoine, Peter wrote: > > I picked up this work due to the following Jira ticket created by the > > security team (on Android) and was asked to give it a second look and > > found a few more issues with the hw lock code. > > > > https://jira01.devtools.intel.com/browse/GMINL-5388 > > I/O control on /dev/dri/card0 crashes the kernel (0x4008642b) > > > > It also stops Linux as it kills the driver, I guess it might be possible > > to reload the gfx driver. On a unpatched system the test that is > > included in the issue or the igt test that has been posted for the issue > > will show the problem. > > > > I ran the test on an unpatched system here and the gui stopped and the > > keyboard stopped responding, so I rebooted. With the patched system I > > did not need to reboot. > > > > Should I change the SIGTERM to SIGSEGV, not quite the same thing but > > tooling is better at handling a segfault than a SIGTERM and the > > application that calls this IOCTL is using an uninitialised hw lock so > > it is kind of the same as differencing an uninitialised pointer (kind > > of). Or, I could just remove it, but the bug has been in the code for at > > least two years (and known about), and I would guess that any code that > > is calling this is fuzzing the IOCTLs (as this is how the security team > > found it) and we should reward them with a application exit. > > > > Peter. > > SIGSEGV would be a better choice. > > SIGTERM is normally sent by a user -- it's the default signal sent by > kill(1). It's also commonly used to tell a long-running daemon process > to tidy up and exit cleanly. > > SIGSEGV commonly means "you accessed something that doesn't exist/isn't > mapped/you don't have permissions for". There are specific subcases that > can be indicated via the siginfo data; this is from the sigaction(1) > manpage: > > The following values can be placed in si_code for a SIGSEGV signal: > > SEGV_MAPERRaddress not mapped to object > > SEGV_ACCERRinvalid permissions for mapped object > > SIGBUS would also be a possibility but that's generally taken to mean > that an access got all the way to some physical bus and then faulted, > whereas SIGSEGV suggests the access was rejected during the > virtual-to-physical mapping process. None of the above. Just return -EINVAL, -EPERM, -EACCESS as appropriate. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915 : Added Programming of the MOCS
On Wed, Jun 17, 2015 at 03:02:31PM +, Antoine, Peter wrote:
> On Wed, 2015-06-10 at 11:37 +0100, Chris Wilson wrote:
> > > +/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
> > > +#define MOCS_CACHEABILITY(value)((value & 0x03) << 0)
> >
> > value & mask? These macros should only be feed enums so the masking of
> > the input is superfluous and indicative of a programming bug.
> Superfluous yes, but it lets the coder know the layout of the field, but
> may hide a programming bug but does not indicate one (other coding
> standards (for safe systems) that I have used require this as it reduces
> the impact of coding errors).
I'm wary. Maybe not so much for the MOCS table, but it means that the
value being programmed to hw does not match what the programmer
intended. It's a bug either way, the hardware is being programmed to an
invalid value - and that may be catastrophic to use either the original
value of the transformed value.
> I have removed them.
I kind of liked it. With a bit of work we could make it catch
programming bugs at compile time. I think we have idly discussed such in
the past, something like a
#define SET_FIELD(x, shift, width) ({
if (__builtin_constant_p(x)) {
BUILD_BUG_ON(x & -(1
Re: [Intel-gfx] [PATCH v4] drm/i915 : Added Programming of the MOCS
On Thu, Jun 18, 2015 at 07:36:41AM +, Antoine, Peter wrote:
>
> On Wed, 2015-06-17 at 17:33 +0100, Chris Wilson wrote:
> > On Wed, Jun 17, 2015 at 04:19:22PM +0100, Peter Antoine wrote:
> > > This change adds the programming of the MOCS registers to the gen 9+
> > > platforms. This change set programs the MOCS register values to a set
> > > of values that are defined to be optimal.
> > >
> > > It creates a fixed register set that is programmed across the different
> > > engines so that all engines have the same table. This is done as the
> > > main RCS context only holds the registers for itself and the shared
> > > L3 values. By trying to keep the registers consistent across the
> > > different engines it should make the programming for the registers
> > > consistent.
> > >
> > > v2:
> > > -'static const' for private data structures and style changes.(Matt
> > > Turner)
> > > v3:
> > > - Make the tables "slightly" more readable. (Damien Lespiau)
> > > - Updated tables fix performance regression.
> > > v4:
> > > - Code formatting. (Chris Wilson)
> > > - re-privatised mocs code. (Daniel Vetter)
> >
> > Being really picky now, but reading your comments impressed upon me
> > the importance of reinforcing one particular point...
> >
> > >
> > > + /*
> > > + * Failing to program the MOCS is non-fatal.The system will not
> > > + * run at peak performance. So generate a warning and carry on.
> > > + */
> > > + if (gen9_program_mocs(ring, ctx) != 0)
> >
> > I think this is better as intel_rcs_context_init_mocs(). Too me it is
> > important that you emphasize this is to be run once during very early
> > initialisation to setup the first context prior to anything else. i.e.
> > All subsequent execution state must be derived from this. Renaming it as
> > intel_rcs_context_init_mocs():
> >
> > 1 - indicates you have written it to handle all generation, this is
> > important as you are otherwise passing in gen8 into a gen9 function.
> >
> > 2 - it is only called during RCS->init_context() and must not be called
> > at any other time - this avoids the issue of modifying registers
> > used by other rings at runtime, which is the trap you lead me into
> > last time.
> No problem with that.But adding rcs to the original name suggests that it
> is only setting up the rcs engine and not all the engines. If any of the
> other context engines have there context extended then we may need to call
> the function from other ring initialise functions.
"intel_rcs_context" is the object
"init_mocs" is the verb, with "init" being a fairly well defined phase
of context operatinons.
My suggestion is that is only run during RCS context init. The comments
tell us that it affects all rings - and so we must emphasize that the
RCS context init *must* be run before the other rings are enabled for
submission.
If we have contexts being initialised on other rings, then one would not
think of calling intel_rcs_context_init* but instead think of how we
would need to interact with concurrent engine initialisation. Being
specifc here should stop someone simply calling the function and hoping
for the best.
> I'll change it to intel_context_emit_mocs() as this does say what it does
> on the tin, it only emits the mocs to the context and does not program them.
That misses the point I am trying to make.
> > > + if (IS_SKYLAKE(dev)) {
> > > + table->size = ARRAY_SIZE(skylake_mocs_table);
> > > + table->table = skylake_mocs_table;
> > > + result = true;
> > > + } else if (IS_BROXTON(dev)) {
> > > + table->size = ARRAY_SIZE(broxton_mocs_table);
> > > + table->table = broxton_mocs_table;
> > > + result = true;
> > > + } else {
> > > + /* Platform that should have a MOCS table does not */
> > > + WARN_ON(INTEL_INFO(dev)->gen >= 9);
> >
> > result = false; here would be fewer lines of code today and tomorrow. :)
> Fail safe return value. Makes not difference here, but golden in larger
> functions.
Actually I don't see why you can't encode the ARRAY_SIZE into the static
const tables, then the return value is just the appropriate table. If
you don't set a default value, then you get a compiler warning telling
you missed adding it your new code.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/i915 : Added Programming of the MOCS
On Thu, Jun 18, 2015 at 08:45:10AM +, Antoine, Peter wrote: > On Thu, 2015-06-18 at 08:49 +0100, ch...@chris-wilson.co.uk wrote: > > On Thu, Jun 18, 2015 at 07:36:41AM +, Antoine, Peter wrote: > > > > > > On Wed, 2015-06-17 at 17:33 +0100, Chris Wilson wrote: > > > > On Wed, Jun 17, 2015 at 04:19:22PM +0100, Peter Antoine wrote: > > > > > This change adds the programming of the MOCS registers to the gen 9+ > > > > > platforms. This change set programs the MOCS register values to a set > > > > > of values that are defined to be optimal. > > > > > > > > > > It creates a fixed register set that is programmed across the > > > > > different > > > > > engines so that all engines have the same table. This is done as the > > > > > main RCS context only holds the registers for itself and the shared > > > > > L3 values. By trying to keep the registers consistent across the > > > > > different engines it should make the programming for the registers > > > > > consistent. > > > > > > > > > > v2: > > > > > -'static const' for private data structures and style changes.(Matt > > > > > Turner) > > > > > v3: > > > > > - Make the tables "slightly" more readable. (Damien Lespiau) > > > > > - Updated tables fix performance regression. > > > > > v4: > > > > > - Code formatting. (Chris Wilson) > > > > > - re-privatised mocs code. (Daniel Vetter) > > > > > > > > Being really picky now, but reading your comments impressed upon me > > > > the importance of reinforcing one particular point... > > > > > > > > > > > > > > + /* > > > > > + * Failing to program the MOCS is non-fatal.The system will not > > > > > + * run at peak performance. So generate a warning and carry on. > > > > > + */ > > > > > + if (gen9_program_mocs(ring, ctx) != 0) > > > > > > > > I think this is better as intel_rcs_context_init_mocs(). Too me it is > > > > important that you emphasize this is to be run once during very early > > > > initialisation to setup the first context prior to anything else. i.e. > > > > All subsequent execution state must be derived from this. Renaming it as > > > > intel_rcs_context_init_mocs(): > > > > > > > > 1 - indicates you have written it to handle all generation, this is > > > > important as you are otherwise passing in gen8 into a gen9 function. > > > > > > > > 2 - it is only called during RCS->init_context() and must not be called > > > > at any other time - this avoids the issue of modifying registers > > > > used by other rings at runtime, which is the trap you lead me into > > > > last time. > > > No problem with that.But adding rcs to the original name suggests that it > > > is only setting up the rcs engine and not all the engines. If any of the > > > other context engines have there context extended then we may need to call > > > the function from other ring initialise functions. > > > > "intel_rcs_context" is the object > > "init_mocs" is the verb, with "init" being a fairly well defined phase > > of context operatinons. > > > > My suggestion is that is only run during RCS context init. The comments > > tell us that it affects all rings - and so we must emphasize that the > > RCS context init *must* be run before the other rings are enabled for > > submission. > > > > If we have contexts being initialised on other rings, then one would not > > think of calling intel_rcs_context_init* but instead think of how we > > would need to interact with concurrent engine initialisation. Being > > specifc here should stop someone simply calling the function and hoping > > for the best. > > > > > I'll change it to intel_context_emit_mocs() as this does say what it does > > > on the tin, it only emits the mocs to the context and does not program > > > them. > > > > That misses the point I am trying to make. > > I don't get your point, the original seemed good to me. > Changing name to what you want as this needs to get in. My point is that it is not a generic function and must be called at a certain phase of context construction and lrc initialisation. I am trying to suggest
Re: [Intel-gfx] [PATCH v5] drm/i915 : Added Programming of the MOCS
On Thu, Jun 18, 2015 at 04:25:47PM +0100, Damien Lespiau wrote: > >>On Thu, Jun 18, 2015 at 03:45:44PM +0100, Antoine, Peter wrote: > >> So, intializing the other (non-render) MOCS in gen8_init_rcs_context() > >> isn't the most logical thing to do I'm afraid. What happens if we > >> suddenly decide that we don't want to fully initialize the default > >> context at startup but initialize each ring on-demand for that context > >> as well? We can end up in a situation where we use the blitter first > >> and we wouldn't have the blitter MOCS initialized. > >> > >> In that sense, that code makes an assumption about how we do things in > >> a completely different part of the driver and that's always a > >> potential source of bugs. > >> > > > >Yes, but this is the same with the golden context and the workarounds > >(as I understand it) so all this code would have to be moved. > > Ah, but the workarounds in that function are only for registers in the > render context, not other rings/engine. Yes, but it just so happens that we initialise the default context before userspace so that we know that context is pristine before sending batches to the GPU. This is the reason why I think it is important to mark this function as being executed at that stage, so that all parties can be sure that the execution is before real use of the GPU and so we can use the RCS to initialise the other rings. At the moment, I am happy with baking that assumption into the code, we can readdress it later if there are non-RCS operations that must be performed at context init and conflict with the RCS programming. If you can think of a suitable comment to forewarn us in future about potential conflicts in adding xcs->init_context(), be my guest. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/18] drm/i915: extract crtc_is_valid() on the FBC code
On Wed, Oct 21, 2015 at 05:16:04PM +, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-20 às 16:52 +0100, Chris Wilson escreveu:
> > On Tue, Oct 20, 2015 at 11:49:50AM -0200, Paulo Zanoni wrote:
> > > We're going to kill intel_fbc_find_crtc(), that's why a big part of
> > > the logic moved from intel_fbc_find_crtc() to crtc_is_valid().
> > >
> > > Signed-off-by: Paulo Zanoni
> > > ---
> > > drivers/gpu/drm/i915/intel_fbc.c | 26 --
> > > 1 file changed, 16 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index b9cfd16..1162787 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -538,27 +538,33 @@ static void set_no_fbc_reason(struct
> > > drm_i915_private *dev_priv,
> > > DRM_DEBUG_KMS("Disabling FBC: %s\n", reason);
> > > }
> > >
> > > +static bool crtc_is_valid(struct intel_crtc *crtc)
> > > +{
> > > + struct drm_i915_private *dev_priv = crtc->base.dev-
> > > >dev_private;
> > > + enum pipe pipe = crtc->pipe;
> > > +
> > > + if ((IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >=
> > > 8) &&
> > > + pipe != PIPE_A)
> >
> > Keeping
> >
> > if (pipe_a_only(dev_priv) && pipe != PIPE_A)
> > return false;
> >
> > would have been nicer.
>
> I can do that on v2.
>
> >
> > > + return false;
> > > +
> > > + return intel_crtc_active(&crtc->base) &&
> > > + to_intel_plane_state(crtc->base.primary->state)-
> > > >visible &&
> > > + crtc->base.primary->fb != NULL;
> >
> > And then you can split this line up for a little more clarity. If you
> > are taking the time to refactor into a separate function for
> > readability, you may as well apply a little polish as well.
>
> I really don't get what you're suggesting here. Can you please clarify?
I think multiline conditionals do not add to readibilty. Since you have
already taken the effort to split the predicate out into its own
function,
if (!intel_crtc_active(&crtc->base))
return false;
if (!to_intel_plane_state(crtc->base.primary->state)->visible)
return false;
/* both of these are used repeatedly in intel_display.c, probably worth
* refactoring into intel_plane_active(crtc, crtc->base.primary); later
* on
*/
/* given the two above, this is redundant. more evidence that we should
* not be writing this code ourselves but calling an intel_display
* helper
*/
if (crtc->base.primary->fb)
return false;
return true;
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/18] drm/i915: only nuke FBC when a drawing operation triggers a flush
On Wed, Oct 21, 2015 at 05:08:42PM +, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-20 às 16:59 +0100, Chris Wilson escreveu:
> > On Tue, Oct 20, 2015 at 11:49:49AM -0200, Paulo Zanoni wrote:
> > > There's no need to stop and restart FBC: a nuke should be fine.
> > >
> > > Signed-off-by: Paulo Zanoni
> > > ---
> > > drivers/gpu/drm/i915/intel_fbc.c | 6 --
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 9477379..b9cfd16 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -1088,8 +1088,10 @@ void intel_fbc_flush(struct drm_i915_private
> > > *dev_priv,
> > > if (origin == ORIGIN_FLIP) {
> > > __intel_fbc_update(dev_priv);
> > > } else {
> > > - __intel_fbc_disable(dev_priv);
> > > - __intel_fbc_update(dev_priv);
> > > + if (dev_priv->fbc.enabled)
> > > + intel_fbc_nuke(dev_priv);
> >
> > Ok, what does nuke actually do? From the name, I would expect FBC to
> > be
> > left in an unusable state.
>
> As far as I understand, it triggers a full recompression of the CFB. It
> should be equivalent to disable+reenable.
Maybe intel_fbc_recompress(), that seems a little more obvious than nuke?
> >
> > > + else
> > > + __intel_fbc_update(dev_priv);
> > > }
> > > }
> >
> > This becomes
> >
> > if (enabled && origin != ORIGIN_FLIP)
> > intel_fbc_nuke();
> > else
> > __intel_fbc_update();
>
> Now I see this code could definitely have been made simpler... Fixing
> this here would require me to redo many of the next patches. I hope you
> accept patch 19/18 as a possible "fix".
Sure.
> >
> > It seems a little odd that anything is done if disabled, so care to
> > elaborate that reason
>
> When we're drawing on the frontbuffer we may get an invalidate() call
> first, which will trigger an FBC deactivation. Then later we'll get a
> flush() and will have to reenable. Sometimes we may just get the
> flush() without the previous invalidate(), and for this case a nuke is
> the easiest thing to do. That's all just the normal frontbuffer
> tracking mechanism.
>
>
> > , and I presume there is an equally good comment
> > before the context that explains why FLIP is special?
>
> It's just that we ignore flushes() for the FLIP case if FBC is active
> due to the hardware tracking, which automatically does a nuke. There's
> a check for this earlier on this function, which you can't see on this
> diff context but you can see on patch 02/18. So if origin is FLIP, and
> FBC is active, we return early.
I like this comment. Care to add it to the function?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/18] drm/i915: fix the __intel_fbc_update() comments
On Wed, Oct 21, 2015 at 05:32:16PM +, Zanoni, Paulo R wrote: > Em Qua, 2015-10-21 às 13:37 +0100, Chris Wilson escreveu: > > On Tue, Oct 20, 2015 at 11:49:53AM -0200, Paulo Zanoni wrote: > > > Don't try to list in comments the cases where we should enable or > > > disable FBC: it varies a lot with the hardware generations and the > > > code should be the documentation. Also notice that there's already > > > a > > > huge gap between the comments and what's in the code. > > > > > > Signed-off-by: Paulo Zanoni > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 26 ++ > > > 1 file changed, 2 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index e856304..5fa4ce4 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -853,20 +853,8 @@ static bool > > > intel_fbc_hw_tracking_covers_screen(struct intel_crtc *crtc) > > > * __intel_fbc_update - enable/disable FBC as needed, unlocked > > > * @dev_priv: i915 device instance > > > * > > > - * Set up the framebuffer compression hardware at mode set > > > time. We > > > - * enable it if possible: > > > - * - plane A only (on pre-965) > > > - * - no pixel mulitply/line duplication > > > - * - no alpha buffer discard > > > - * - no dual wide > > > - * - framebuffer <= max_hdisplay in width, max_vdisplay in > > > height > > > - * > > > - * We can't assume that any compression will take place (worst > > > case), > > > - * so the compressed buffer has to be the same size as the > > > uncompressed > > > - * one. It also must reside (along with the line length buffer) > > > in > > > - * stolen memory. > > > - * > > > - * We need to enable/disable FBC on a global basis. > > > + * This function completely reevaluates the status of FBC, then > > > enables, > > > + * disables or maintains it on the same state. > > > > By the end, this comment is not strictly true ,right? Since you move > > some of the status checking into modeset enable paths. Could you > > refine > > the function comment? > > I only move the status checking into modeset enable paths on patch 12. > The comment above is modified on patch 12 to reflect > activate/deactivate instead of enable/disable. Honestly, I would just squash this patch, or apply it after the moving the code to correctly reflect the split in work between update/enable. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/18] drm/i915: introduce is_active/activate/deactivate to the FBC terminology
On Wed, Oct 21, 2015 at 05:45:33PM +, Zanoni, Paulo R wrote: > Em Qua, 2015-10-21 às 13:34 +0100, Chris Wilson escreveu: > > On Tue, Oct 20, 2015 at 11:49:56AM -0200, Paulo Zanoni wrote: > > > The long term goal is to have enable/disable as the higher level > > > functions and activate/deactivate as the lower level functions, > > > just > > > like we do for PSR and for the CRTC. Let's start this by renaming > > > the > > > functions that touch the hardware state and their wrappers. > > > > So enable() calls activate() and disable() calls deactivate(). So > > what's the > > benefit? > > I explained each individual change on its own patch, but I guess I > should have put a higher level description here. Will fix this in v2. > > With just this patch there's really no benefit. The main benefit is patch 12, > when we actually have separate enable/disable and activate/deactivate > functions. > > One of the main points is that enable/disable are called once per modeset, > while update/activate/deactivate can be called tons of times during normal > operation, so moving code to enable() when possible makes sure it is not ran > over and over again unnecessarily. > > > > What mistakes and confusion are made right now > > The confusion right now is that we don't have the real higher level > enable/disable that we get on patch 12. > > > and is the > > mismatch between low/high worth it? This is your chance to justify > > the > > churn and sell us on the new naming scheme, and explain your long > > term > > vision in making the driver consistent everywhere. > > Maybe I should just redirect users to patch 12 on the commit, since > this patch does not add any value by itself. I could have squashed this > and 12, but I don't like huge patches: they're not easy to review and > are a pain to rebase. > > Anyway, v2 will hopefully have a better commit message. Yes, just explain how you intend to separate the levels should be enough to justify the change, and the overview in how they are intended to be used invaluable. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/18] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work
On Wed, Oct 21, 2015 at 05:27:32PM +, Zanoni, Paulo R wrote: > Em Ter, 2015-10-20 às 17:03 +0100, Chris Wilson escreveu: > > On Tue, Oct 20, 2015 at 11:49:51AM -0200, Paulo Zanoni wrote: > > > This thing where we need to get the crtc either from the work > > > structure or the fbc structure itself is confusing and unnecessary. > > > Set fbc.crtc right when scheduling the enable work so we can always > > > use it. > > > > Pardon? It was confusing to have the crtc passed along with the work > > item as opposed to digging it out from an indirect link on the > > dev_priv. > > iirc work->crtc was part of the serialisation of the work item. > > > > What I think you meant was that you were passing around the wrong > > struct, e.g. > > intel_fbc_enable() just wants the crtc, in intel_fbc_flip_prepare() > > you > > check for the work struct, and then you should just use the work > > struct. > > The problem is not what gets passed and how to retrieve it. The problem > is that when we're in the other parts of the code we always have to > keep in mind that if FBC is already enabled we have to get the CRTC > from place A, if FBC is scheduled we have to get the CRTC from place B, > and if it's disabled there's no CRTC. Having a single place to retrieve > the CRTC from allows us to treat the "is enabled" and "is scheduled" > cases as the same case, reducing the mistake surface. I guess I should > add this to the commit message. Sure that's also my argument why I don't like this particular patch as is. :) The functions here had just one place to retreive everything they needed, and now they need two. I guess the way to make us both happy is to embed the work_struct inside the fbc struct - and that way you can eliminate the divergence. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/18] drm/i915: remove too-frequent FBC debug message
On Wed, Oct 21, 2015 at 06:19:23PM +, Zanoni, Paulo R wrote: > Em Qua, 2015-10-21 às 14:01 +0100, Chris Wilson escreveu: > > On Tue, Oct 20, 2015 at 11:49:59AM -0200, Paulo Zanoni wrote: > > > If we run igt/kms_frontbuffer_tracking, this message will appear > > > thousands of times, eating a significant part of our dmesg buffer. > > > It's part of the expected FBC behavior, so let's just silence it. > > > > > > Signed-off-by: Paulo Zanoni > > > > Looks fine. Out of curiosity what metrics do we have for FBC > > activity? I > > presume we have tracepoints for activate/deactivate. and perhaps a sw > > timer, and a hw debug register? > > The most important metric is PC state residency, but that requires the > machine to be properly configured: if SATA is preventing our machine > from going deeper than PC3, FBC won't change PC state residencies. > Also, our public processor datasheet docs mention the maximum expected > PC states for the most common screen resolutions. > > We can work on adding more things later, such as the tracepoints or > software timer you mentioned. I've been 100% focused on getting the > bugs out first. Is there anything specific you think you could use? The tracepoints are primarily a debug tool, effectively less noisy printks that can be easily hooked up to a bit of python for processing (if just read huge logs isn't satisfying). For monitoring efficacy, I had in mode a timer for fbc active and perhaps one for timing compression (ideally the active timer would only start when the compressed frame was complete, but that may be too much). That should get us to the point where we can quickly see if we are enabling FBC for significant periods. Now, this can be done with good tracepoints and a userspace script. So really just planning good coverage of tracepoints is the starting point. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 23/26] drm/i915: use a single intel_fbc_work struct
On Wed, Oct 28, 2015 at 05:24:18PM +, Zanoni, Paulo R wrote:
> Em Ter, 2015-10-27 às 20:29 +, Chris Wilson escreveu:
> > On Tue, Oct 27, 2015 at 02:50:25PM -0200, Paulo Zanoni wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index a9434d1..fdbe068 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -917,9 +917,11 @@ struct i915_fbc {
> > > bool active;
> > >
> > > struct intel_fbc_work {
> > > - struct delayed_work work;
> > > + bool scheduled;
> >
> > Ah, I found this confusingly named. scheduled implied to me that you
> > wanted work_pending(), but you just want to asynchronously cancel the
> > fbc worker. Just bool cancel? Or bool active? Though now I've come
> > full
> > circle and suggest bool scheduled. So
>
> I agree 'bool scheduled' is not the perfect name. I thought about
> renaming it to 'bool cancel' many times. I'm willing to change in case
> someone requests it, but my understanding is that the paragraph above
> is not asking for a rename.
>
> >
> > /* Track whether the FBC worker has already been queued,
> > * or asynchronously cancel the worker whilst it waits
> > * before activation.
> > */
>
> I can add this, although if someone suggests a better name we may not
> need it :)
Even if we do rename the variable, we might as well keep the comment. I
think it's a reasonably succinct description of why that bool should
exist.
> >
> > What happens then if we quickly queue, cancel and want to requeue?
> > The
> > schedule_work() fails as the task is already pending, but the
> > scheduled
> > flag gets reset, so it just works. Perfect.
>
> /me is confused.
>
> This case should work since everything is done with fbc.lock grabbed.
I was just thinking about the case where the bool scheduled=false but
the work was still queued, and you then wanted to reschedule it. It
works just fine, the original work item remains queued and gets
reactivated correctly
(I was trying to find a flaw in the code and decided that it wasn't a
flaw at all, at which point I was happy!)
>
> > > + struct work_struct work;
> > > struct drm_framebuffer *fb;
> >
> > Hmm, don't we actually need to take references on the fb we schedule
> > for
> > activation? Since we already account for that the crtc->fb may be
> > changed between queuing the work and executing it, for extra paranoia
> > we
> > should ensure that we have a reference in work->fb. (long standing
> > bug,
> > might as well fix it before we see it in the wild, time for another
> > kms-flip race!)
>
> I'm not super familiar with this area, so I have to ask: what bad
> things can happen if we don't have a reference on work->fb?
The worst depends on the locking - if we borrow the reference and
operate on work->fb == crtc->fb without the right lock, that can blow
up. Otherwise, as just use work->fb as a pointer, it is possible for the
fb to unref'ed and reallocated without us noticing and for us to then
incorrectly schedule the fbc (unless you can demonstrate that the
work->fb is guarded by the fbc.lock + bool scheduled).
> We're just comparing pointers here, so if work->fb is not referenced by
> the CRTC we won't do anything with it. If work->fb is referenced by the
> CRTC, it will already have a reference, right?
>
> I'm also not 100% sure if it's even possible to have crtc->fb != work-
> >fb without anything else canceling the work thread, so I just kept the
> old code around for now.
Indeed, I'm not sure if it is possible but (a) we should check that we
do have sufficient locks to prevent crtc->fb disappearing, and (b)
verify that when crtc->fb is changed the work is cancelled. As a bonus,
document that work->fb is just a pointer whose validity is guarded by
bool scheduled (or whatever). Or even better, having just completed (a)
+ (b), you can drop the drm_framebuffer pointer from the work item
entirely and rely on crtc->fb.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/16] Revert "drm/i915: Allocate fbcon from stolen memory"
On Tue, Aug 18, 2015 at 09:54:57PM +, Zanoni, Paulo R wrote: > Em Sáb, 2015-08-15 às 09:24 +0100, Chris Wilson escreveu: > > On Fri, Aug 14, 2015 at 06:34:20PM -0300, Paulo Zanoni wrote: > > > This reverts commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb. > > > > > > Technology has evolved and now we have eDP panels with 3200x1800 > > > resolution. In the meantime, the BIOS guys didn't change the > > > default > > > 32mb for stolen memory. And we can't assume our users will be able > > > to > > > increase the default stolen memory size to more than 32mb - I'm not > > > even sure all BIOSes allow that. > > > > > > So just the fbcon buffer alone eats 22mb of my stolen memroy, and > > > due > > > to the BDW/SKL restriction of not using the last 8mb of stolen > > > memory, > > > all that's left for FBC is 2mb! Since fbcon is not the coolest > > > feature > > > ever, I think it's better to save our precious stolen resource to > > > FBC > > > and the other guys. > > > > > > Besides, neither the original commit message nor the review > > > comments > > > showed any arguments in favor of actually allocating fbcon from > > > stolen. > > > > Pardon? > > > pzanoni@panetone:~/git/kernel/kernel$ git show > 0ffb0ff283cca16f72caf29c44496d83b0c291fb | head -n 12 > commit 0ffb0ff283cca16f72caf29c44496d83b0c291fb > Author: Chris Wilson > Date: Thu Nov 15 11:32:27 2012 + > > drm/i915: Allocate fbcon from stolen memory > > Signed-off-by: Chris Wilson > Reviewed-by: Jesse Barnes > Acked-by: Ben Widawsky > Signed-off-by: Daniel Vetter > > diff --git a/drivers/gpu/drm/i915/intel_fb.c > b/drivers/gpu/drm/i915/intel_fb.c > pzanoni@panetone:~/git/kernel/kernel$ Sorry, I though it was pretty self explanatory that we want to reuse as much as stolen as is possible on all machines, epecially those where even 8MiB reserved is about 10% of available memory. So not using it for pinned memory is a double whammy. That would have been said in the preceeding patches to enable stolen memory. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/16] drm/i915: avoid the last 8mb of stolen on BDW/SKL
On Tue, Aug 18, 2015 at 09:49:34PM +, Zanoni, Paulo R wrote: > Em Sáb, 2015-08-15 às 09:29 +0100, Chris Wilson escreveu: > > On Fri, Aug 14, 2015 at 06:34:13PM -0300, Paulo Zanoni wrote: > > > The FBC hardware for these platforms doesn't have access to the > > > bios_reserved range, so it always assumes the maximum (8mb) is > > > used. > > > So avoid this range while allocating. > > > > > > This solves a bunch of FIFO underruns that happen if you end up > > > putting the CFB in that memory range. On my machine, with 32mb of > > > stolen, I need a 2560x1440 mode for that. > > > > If the restriction applies only to FBC, apply it to only fbc. > > That's what the patch is doing. The part where we set the unusual > start/end is at intel_fbc.c:find_compression_threshold(). Everything > else should be behaving just as before. > > Maybe you're being confused by the addition of the stolen_usable_size > variable. Hmm, I expected to see the constaint being passed into the search routine.a It looked like you were adjusting the stolen_size probably the root of my confusion. Anyway we have a quandary. You want to reserve stolen space, and I want to make sure as much of it gets used as possible (especially for long lived objects). What I have in mind is adding a priority to the search and allow us to evict anything of lower priority. We can use a page replacement algorithm even for the pinned fbcon (since only the GGTT itself is pinned and we can swap the pages underneath). This of course requires a callback for low priority evictable objects. I have 3 priorities in mind plus the evictable flag: USER, KERNEL, POWER (with FBC taking the highest priority of POWER). That will allow us to do the BIOS takeover and only if we run out of space force the copy. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: restrict unclaimed register checking
On Wed, Sep 02, 2015 at 08:20:52PM +, Zanoni, Paulo R wrote:
> Em Qua, 2015-08-26 às 08:44 +0100, Chris Wilson escreveu:
> > On Tue, Aug 25, 2015 at 07:03:42PM -0300, Paulo Zanoni wrote:
> > > The unclaimed register bit is only triggered when someone touches
> > > the
> > > specified register range.
> > >
> > > For the normal use case (with i915.mmio_debug=0), this commit will
> > > avoid the extra __raw_i915_read32() call for every register outside
> > > the specified range, at the expense of a few additional "if"
> > > statements.
> > >
> > > Cc: Chris Wilson
> > > Signed-off-by: Paulo Zanoni
> >
> >
> > > @@ -612,7 +614,7 @@ hsw_unclaimed_reg_debug(struct drm_i915_private
> > > *dev_priv, u32 reg, bool read,
> > > const char *op = read ? "reading" : "writing to";
> > > const char *when = before ? "before" : "after";
> > >
> > > - if (!i915.mmio_debug)
> > > + if (!i915.mmio_debug || !UNCLAIMED_CHECK_RANGE(reg))
> > > return;
> >
> > Place the register check on the preceding line so that it is not
> > confused with checking the debug-enabled status. (Also you can argue
> > that reg will be register/cache-hot and so a cheaper test to do first
> > than loading a module parameter.)
>
> That makes sense, I'll do it.
>
> >
> > > if (__raw_i915_read32(dev_priv, FPGA_DBG) &
> > > FPGA_DBG_RM_NOCLAIM) {
> > > @@ -624,11 +626,11 @@ hsw_unclaimed_reg_debug(struct
> > > drm_i915_private *dev_priv, u32 reg, bool read,
> > > }
> > >
> > > static void
> > > -hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv)
> > > +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv, u32
> > > reg)
> > > {
> > > static bool mmio_debug_once = true;
> > >
> > > - if (i915.mmio_debug || !mmio_debug_once)
> > > + if (i915.mmio_debug || !UNCLAIMED_CHECK_RANGE(reg) ||
> > > !mmio_debug_once)
> > > return;
> >
> > The use is wrong here though because you never reviewed my patch to
> > enable the single-shot debugging from the interrupt handler to catch
> > invalid usage from elsewhere.
>
> If you're talking about intel_uncore_check_errors(), I think we can
> just kill it now. I'll submit a patch with the arguments, so we can
> continue this topic there.
>
> Moving back to the main subject:
>
> In the last time you sent the patch to do the unclaimed checks only on
> forcewake code, I started the conversations with the HW guys about the
> range of registers relevant by FPGA_DBG (and CCd you on these
> conversations). My plan was to write this patch at that time, but
> priorities happened and it got postponed :(. I also hoped that maybe
> you would eventually end up writing it and I would just have to review
> it :)
>
> My main argument was that by doing unclaimed register checking only at
> forcewake time we would be running a check that is only relevant to
> display code during non-display code. So my idea is that if we restrict
> the unclaimed check to the actual range of registers that can be caught
> we basically remove unclaimed register checking for most (all?) of the
> performance-sensitive code.
>
> Since we have multiple solutions, I decided to do some experiments.
> First of all, since I was not really seeing hsw_unclaimed_reg_detect()
> on perf, I decided to patch it so it would do the read/write check
> around FPGA_DBG 50 times per call instead of just one. With this, by
> running "perf record glxgears" for a few seconds I could see
> hsw_unclaimed_reg_detect() taking about 4.6% of the total time.
Something is very suspect with your system if you are not observing much
hsw_unclaimed_reg_check during trivial workloads.
> Then I applied just this patch, and the time went down to 0.13%. I also
> applied your patch on top of it all, and it went up to 0.52% (I guess
> the extra checks at forcewake time cost a little bit). Also notice that
> since I add a "reg" argument to hsw_unclaimed_reg_detect(), I changed
> the forcewake call to use a register in the display range.
> I also tested your patch without my patch, and the measured result was
> about 0.56%.
>
> Now this isn't a super relevant experiment: just glxgears with a
> modified hsw_unclaimed_reg_detect(), but I thought it would be useful
> information, and maybe you could provide me suggestions for better
> workloads.
>
> So I'd like to know if you're ok with proceeding with just this patch
> (considering I implement your suggestions), or if you think we should
> go with just your patch or both or none.
If you really wanted to, you could combine approaches a check in the
forcewake handler as demonstrated is an order of magnitude less frequent
than the register accesses. The key point is that we *need* the checking
against other users, not just our known register access. Checking our own
access basically amounts to detecting invalid registers, which your
approach more or less erradicates (since it is a flag we add to known good
registers, we may as well just compile it and only turn it on during hw
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Refactor common ringbuffer allocation code
On Thu, Sep 03, 2015 at 02:01:51PM +, Zanoni, Paulo R wrote:
> Em Qui, 2015-09-03 às 13:01 +0100, Chris Wilson escreveu:
> > A small, very small, step to sharing the duplicate code between
> > execlists and legacy submission engines, starting with the ringbuffer
> > allocation code.
> >
> > Signed-off-by: Chris Wilson
> > Cc: Arun Siluvery
> > Cc: Mika Kuoppala
> > Cc: Dave Gordon
> > ---
> > drivers/gpu/drm/i915/intel_lrc.c| 49 +-
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 89 ---
> > --
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +--
> > 3 files changed, 70 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 40cbba4ea4ba..28a712e7d2d0 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -2340,8 +2340,7 @@ void intel_lr_context_free(struct intel_context
> > *ctx)
> > i915_gem_object_ggtt_unpin(ctx_obj);
> > }
> > WARN_ON(ctx->engine[ring->id].pin_count);
> > - intel_destroy_ringbuffer_obj(ringbuf);
> > - kfree(ringbuf);
> > + intel_ringbuffer_free(ringbuf);
> > drm_gem_object_unreference(&ctx_obj->base);
> > }
> > }
> > @@ -2442,42 +2441,20 @@ int intel_lr_context_deferred_create(struct
> > intel_context *ctx,
> > I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> > }
> >
> > - ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
> > - if (!ringbuf) {
> > - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer
> > %s\n",
>
> We got rid of this message, but I suppose it's not a problem, since it
> was not a loud error message.
I additionally added these to patch 2. I removed the ones in intel_lrc
that were simply repeating the error message (albeit in a more generic
fashion due to loss of information). At this level an oom squalk
followed by ENOMEM reported to userspace is fairly obvious (and
definitely not our error).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Remove unneeded struct_mutex around rpm
On Mon, Feb 20, 2017 at 08:36:31AM +, Szwichtenberg, Radoslaw wrote: > On Sat, 2017-02-18 at 15:00 +, Chris Wilson wrote: > > We don't need struct_mutex for acquiring an rpm wakeref, and do not need > > to serialise those register read (it's the wrong mutex for those > > registers in any case). Begone! > > > > Signed-off-by: Chris Wilson > Reviewed-by: Radoslaw Szwichtenberg Applied, thanks. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Prevent divide-by-zero in debugfs/i915_rps_boost_info
On Mon, Feb 20, 2017 at 11:02:10AM +, Szwichtenberg, Radoslaw wrote: > On Sat, 2017-02-18 at 11:27 +, Chris Wilson wrote: > > Either by chance, or by misread, the current evaluation interval may be > > zero. If that is the case, don't divide by it! > > > > Signed-off-by: Chris Wilson > Reviewed-by: Radoslaw Szwichtenberg Thanks, pushed. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/7] drm/i915: Remove unrequired POSTING_READ from gen6_set_rps()
On Mon, Feb 20, 2017 at 11:15:23AM +, Szwichtenberg, Radoslaw wrote: > On Mon, 2017-02-20 at 09:47 +, Chris Wilson wrote: > > The uncached mmio is sufficient to queue the mmio writes without raising > > forcewake. The forced flush along with acquiring forcewake from the > > posting read is not required for adjusting the RPS frequency. > > > > Signed-off-by: Chris Wilson > Reviewed-by: Radoslaw Szwichtenberg Applied up to this point, as these 3 are minor. It's the last 4 that I think are the secret sauce for Barytrail. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Suppress fbc suggestion to increase stolen if disabled
On Thu, Feb 23, 2017 at 05:39:24PM +, Vivi, Rodrigo wrote: > Reviewed-by: Rodrigo Vivi Thanks, applied. Just noticed the odd message when browsing the gvt logs. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/2] GuC logger redesign
On Thu, Apr 27, 2017 at 09:22:11AM +, Olinski, Krzysztof E wrote: > On Thu, 2017-04-27 at 10:05 +0100, Chris Wilson wrote: > > On Thu, Apr 27, 2017 at 10:59:18AM +0200, Krzysztof E. Olinski wrote: > > > GuC logger implementation simplified and moved to a library > > > (GuCLAW). > > > Adds simple buffering utility for logging routine (BUC). > > > > Bigger question, why? What designs goals do you want to achieve? > > -Chris > > > Currently, there are problems with compilation for Android platform due > to pthread dependencies. The proposed implementation should work both > for Linux and Android. I thought that this will be also a good occasion > to introduce lockless mechanisms to improve efficiency. I dispute the improved efficiency -- you add an extra copy in reading the log data ;) If the xfer of the log data is not dominant, something is very wrong in the framework. Ok, I missed that pthreads are unworkable on android. If you can kill the copy, be my guest. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ZaphodHeads with intel X driver
On Wed, Feb 06, 2013 at 06:43:53PM +, Kamble, Nitin A wrote: > > On a Intel core i3 (ivybridge) system (NUC) with 2 HDMI ports, I am trying to > get two independent screens by following this method. > http://en.gentoo-wiki.com/wiki/X.Org/ > Dual_Monitors#Single_graphics_card.2C_Multiple_X_screens_with_ZaphodHeads > > And X is failing to start complaining about no usable screens. > > I also came across this commit. > http://cgit.freedesktop.org/xorg/driver/xf86-video-intel/commit/ > ?id=265d94e0aa46b30a3198893544dd3619cc9145de > > So Looks like the config with ZaphodHeads should work. Is there anything wrong > in this config? How can I get it to work? That's quite difficult to tell since you did not attach the xorg.conf you experimented with, or the resultant Xorg.log. Quite probably you simply missed the Option "AccelMethod" "SNA", but since you got an error message about no screens detected, the error is actually likely to be something else. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ZaphodHeads with intel X driver
On Wed, Feb 06, 2013 at 11:00:49PM +, Kamble, Nitin A wrote: > Thanks Chris for the reply. Attached are all the relevant log files. Both screen stanzas are pointing to the same device. It should be: Screen0 -> Device0, Screen1 -> Device1 -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ZaphodHeads with intel X driver
On Wed, Feb 06, 2013 at 11:43:51PM +, Kamble, Nitin A wrote: > Chris, > > X is failing even with this reduced 1 screen config. > > Section "Device" > Identifier "Intel0" > Driver "intel" > BusID "PCI:00:02:0" > Option "ZaphodHeads" "HDMI-A-1" The ZaphodHead connector name should match the name given by xrandr, which would appear to be HDMI1. Getting closer... -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for Enhancement to intel_dp_aux_backlight driver
On Mon, Mar 13, 2017 at 08:31:28AM +, Saarinen, Jani wrote: > HI, > > -Original Message- > > From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf > > Of Patchwork > > Sent: Saturday, March 11, 2017 1:23 AM > > To: Puthikorn Voravootivat > > Cc: intel-gfx@lists.freedesktop.org > > Subject: [Intel-gfx] ✗ Fi.CI.BAT: failure for Enhancement to > > intel_dp_aux_backlight driver > > > > == Series Details == > > > > Series: Enhancement to intel_dp_aux_backlight driver > > URL : https://patchwork.freedesktop.org/series/21086/ > > State : failure > > > > == Summary == > > > > Series 21086v1 Enhancement to intel_dp_aux_backlight driver > > https://patchwork.freedesktop.org/api/1.0/series/21086/revisions/1/mbox/ > > > > Test gem_exec_flush: > > Subgroup basic-batch-kernel-default-wb: > > pass -> INCOMPLETE (fi-skl-6260u) > I suppose so that same as https://bugs.freedesktop.org/show_bug.ci?id=100112 > But on different system. Chris, ack? Nah, it looks like the previous bug was fixed, (9759df989f1 lib: Fix hang detector) - at least we haven't had a spurious incomplete on a main CI run since. So I am hoping this is something else; but we still do not have any information on the cause. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
