Re: [Intel-gfx] RFC frontbuffer write tracking
On Thu, Jun 20, 2013 at 06:18:27PM +0100, Chris Wilson wrote: > Having thrown around a few ideas for how to do PSR and FBC write > tracking on the frontbuffer, including the creation of a new SCANOUT > domain, the implementation that I've settled on is to detect writes to > either the GTT domain (hmm, a CPU write flush should also set the fb as > dirty) and then a deferred SCANOUT flush. The deferred flush then resets > the write domain and kicks the GTT mapping so that any further dumb > writes (i.e. dumb kms buffers or fbcon) cause a new frontbuffer > invalidation. This patch series also introduces framebuffer parameters > (which maybe should be properties) to allow userspace to opt out of the > dumb mechanism and elect to call dirtyfb itself when it requires the > scanout to be flushed. > > I've been playing around with GFDT on SNB and IVB as a means for > prototyping the delayed updates and framebuffer parameters. Sorry for the massive delay in writing down my review here, it's been sitting on my table for way too long. Mostly unsorted thoughts on the entire topic: - dirtyfb locking is really heavyweight, it takes all modeset locks currently. But it's easy to just push that down into driver callbacks so that we can do something saner. - It's not part of this patch series, but imo if we want to expose a hint to userspace that frontbuffer rendering might not be a good idea we should do that as a crtc or maybe plane property (Ville's primary plane conversion is missing for that to work out perfectly, but meh). - If we really need properties on framebuffer we imo should add support for that in drm core. But I don't think that's required, since we can just mark framebuffers as non-legacy as soon as we've seen the first ->dirtyfb ioctl call. Doesn't even need any locking since we can only ever upgrade from false to true. New userspace which explicitly tells the kernel when to invalidate an fb can just do a dummy dirtyfb call at alloc time (or not bother if it never does frontbuffer rendering at all). - I don't think we want to have a delayed reenable timer for old userspace which doesn't do ->dirtyfb calls. Afaics we've established that we need userspace's cooperation, and adding the complexity to reanable power saving features for legacy userspace is imo too much trouble. It's also something which we'll have a hard time to validate and regression test. And I really don't like new code which doesn't have good automated test coverage ;-) - The issue with just killing psr/fbc (i.e. not just disabling it for a bit) on the first frontbuffer rendering is that we seem to have too many false positives with the current checks. I haven't traced stuff really but at least across a pageflip the ->pin_count check is too coarse and will fire for the logical new backbuffer if we're unlucky. So I think we need to add new, precise (from the userspaces pov) tracking of when a gem buffer object is used for psr or fbc: bool fbc_target; int psr_target; I think we need to add those to the gem object and not the framebuffer object since that's the place where we can check it in the busy/set_domain ioctl. To avoid locking madness this should be protected with a new frontbuffer tracking lock. Of course we'll still first check pin_count to optimize away the lock taking for most objects. Note that we only need a bool for fbc since only ever one framebuffer object can be used for fbc, so no refcounting is needed. But potentially for psr we can display more than one framebuffer object on a crtc with psr enabled, hence the refcount. - Then we can fully disable fbc if fbc_target is set and fully disable psr if psr_target is set. And if we update them in setcrtc/setplane and for pageflips we'll always know which objects are logically the frontbuffer and won't have a mess with false-positive invalidation events due to us pipelining everthing. For legacy userspace we'd only ever reanable psr/fbc again at modeset and pageflip time. For non-legacy framebuffers (i.e. those where userspace called ->dirtyfb at least once) we'll just never set those tracking bits. There's a bit of ugliness around when userspace changes the fb from legacy to non-legacy mode while it's in use, but I guess we can fix that by simply clearing all the tracking bits when that happens (to avoid forgetting about an fbc/psr reference). - To make this really useful (and also make sure it's properly tested) I think we should abolish all the hw frontbuffer rendering tracking in the gtt and system agent. We only need the hw to invalidate fbc/psr on an mmio write to the buffer address (or an MI_FLIP), which seems to be enabled unconditionally (excluding frobbing debug registers). For fbc (I haven't looked too closely at the psr patches) we certainly need to fix up our code to no longer disable fbc temporarily across a pageflip - that was only necessary with
Re: [Intel-gfx] [PATCH 04/11] drm/i915: Enable/Disable PSR
On Wed, Jul 17, 2013 at 02:02:58PM -0300, Paulo Zanoni wrote:
> 2013/7/11 Rodrigo Vivi :
> > +static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
> > + struct edp_vsc_psr *vsc_psr)
> > +{
> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > + struct drm_device *dev = dig_port->base.base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> > + u32 ctl_reg = HSW_TVIDEO_DIP_CTL(crtc->config.cpu_transcoder);
> > + u32 data_reg = HSW_TVIDEO_DIP_VSC_DATA(crtc->config.cpu_transcoder);
> > + uint32_t *data = (uint32_t *) vsc_psr;
> > + unsigned int i;
> > +
> > + /* As per BSPec (Pipe Video Data Island Packet), we need to disable
> > + the video DIP being updated before program video DIP data buffer
> > + registers for DIP being updated. */
> > + I915_WRITE(ctl_reg, ~VIDEO_DIP_ENABLE_VSC_HSW);
>
> This should be zero.With that fixed:
Fixed while applying.
> Reviewed-by: Paulo Zanoni
Bikeshed: We now have two pieces of code writing DIPs, the other copy is
in intel_hdmi.c. And they don't match.
Slightly related, but: I'd really like to see our conversion to the common
infoframe helpers rsn ...
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.
On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote:
> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
> > Paulo Zanoni.
> > v4: Avoiding reading non-existent registers - noticed by Paulo
> > on first psr debugfs patch.
> > v5: Accepting more suggestions from Paulo:
> > * check sw interlace flag instead of i915_read
> > * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
> >
> > Cc: Paulo Zanoni
> > Signed-off-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 44 ++
> > drivers/gpu/drm/i915/i915_drv.h | 13 +++
> > drivers/gpu/drm/i915/i915_reg.h | 7
> > drivers/gpu/drm/i915/intel_dp.c | 74
> > -
> > 4 files changed, 130 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index fe3cd5a..e679968 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m,
> > void *data)
> > struct drm_info_node *node = m->private;
> > struct drm_device *dev = node->minor->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - u32 psrctl, psrstat, psrperf;
> > + u32 psrstat, psrperf;
> >
> > - if (!IS_HASWELL(dev)) {
> > - seq_puts(m, "PSR not supported on this platform\n");
> > + if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
> > + seq_puts(m, "PSR enabled\n");
> > + } else {
> > + seq_puts(m, "PSR disabled: ");
> > + switch (dev_priv->no_psr_reason) {
> > + case PSR_NO_SOURCE:
>
> I am not a fan of using a continually expanding set of enums for
> no_psr_reason (and no_fbc_reason). If we just stored a const char
> *no_psr_reason, it would make like much easier for us (fewer lines and a
> smaller object).
>
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index d4b52a9..c0bd887 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct
> > intel_dp *intel_dp)
> >EDP_PSR_ENABLE);
> > }
> >
> > +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
> > +{
> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > + struct drm_device *dev = dig_port->base.base.dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_crtc *crtc = dig_port->base.base.crtc;
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > + struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
> > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> > +
> > + if (!IS_HASWELL(dev)) {
> > + DRM_DEBUG_KMS("PSR not supported on this platform\n");
> > + dev_priv->no_psr_reason = PSR_NO_SOURCE;
> > + return false;
> > + }
> > +
> > + if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
> > + (dig_port->port != PORT_A)) {
> > + DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
> > + dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
> > + return false;
> > + }
> > +
> > + if (!is_edp_psr(intel_dp)) {
> > + DRM_DEBUG_KMS("PSR not supported by this panel\n");
> > + dev_priv->no_psr_reason = PSR_NO_SINK;
> > + return false;
> > + }
> > +
> > + if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
> > + DRM_DEBUG_KMS("crtc not active for PSR\n");
> > + dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
> > + return false;
> > + }
> > +
> > + if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
> > + (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
>
> Any time we resort to reading back register state is a failure in our
> state tracking (and pipe_config).
Highly agreed, especially if it's a resource which is out of our control
like the KVMR bits. Since that's just plain broken I've removed it from
the patch.
-Daniel
> -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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
On Wed, Jul 17, 2013 at 07:31:37PM -0700, Ben Widawsky wrote: > On Thu, Jul 18, 2013 at 01:12:17AM +0100, Chris Wilson wrote: > > Big-bada-boom; > > Just from looking at the code I think I see a bug. A bug which didn't > exist in the original version of the code, and doesn't exist after the > very next patch in the overall series. Indeed, that was the bug. > Now I am terribly curious - why in the world (if that's indeed the bug) > can I not seem to hit this locally on my machine? I'll send the patch > for the fix now, but I'd really like to know what's different in our > setup. I've tried, UXA, SNA, and the igt test suite... Just requires testing on the forgotten non-LLC generations. Or Baytrail. -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 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.
On Wed, Jul 17, 2013 at 10:08:34PM +0100, Chris Wilson wrote: > On Wed, Jul 17, 2013 at 06:01:03PM -0300, Rodrigo Vivi wrote: > > On Wed, Jul 17, 2013 at 5:18 PM, Chris Wilson > > wrote: > > > On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote: > > >> Hi Chris, > > >> > > >> could you please review this specific one or give you ack here? > > > > > > Didn't see anything wrong with it. The only caveat I have is that the > > > GETPARAM must be accurate immediately following a setcrtc. > > > > To be truly honest I have no idea, mainly when we alternate with fbcon > > updating psr state at set_base. > > Could you please also review subsequent patches in this series... 10 and 11. > > I think 11 answer this question... > > If it is not clear by this point, and the changelog doesn't make it > clear, then something is missing from this patch. Hint ;-) I'll punt on userspace interface changes, at least until we've figured out a clear picture how to do this. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add functions to force psr exit
On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote:
> PSR tracking engine in HSW doesn't detect automagically some directly copy
> area
> operations through scanout so we will have to kick it manually and
> reschedule it to come back to normal operation as soon as possible.
>
> v2: Before PSR Hook. Don't force it when busy yet.
> v3/v4: Solved small conflict.
> v5: setup once function was already added on previous commit.
> v6: Use POSTING_READ to make sure the mutex works as a write barrier as
> suggested by Chris Wilson
>
> CC: Chris Wilson
> Reviewed-by: Shobhit Kumar
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_dp.c | 48
>
> drivers/gpu/drm/i915/intel_drv.h | 3 +++
> 3 files changed, 52 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3bca337..dc10345 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1840,6 +1840,7 @@
> #define EDP_PSR_PERF_CNT_MASK 0xff
>
> #define EDP_PSR_DEBUG_CTL0x64860
> +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30)
> #define EDP_PSR_DEBUG_MASK_LPSP(1<<27)
> #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26)
> #define EDP_PSR_DEBUG_MASK_HPD (1<<25)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3c9473c..47e1676 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
> return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> }
>
> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
> +{
> + struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
> + struct intel_dp,
> + edp_psr_delayed_normal_work);
> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + mutex_lock(&intel_dp->psr_exit_mutex);
> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
> +~EDP_PSR_DEBUG_FORCE_EXIT);
> + POSTING_READ(EDP_PSR_DEBUG_CTL);
> + mutex_unlock(&intel_dp->psr_exit_mutex);
> +}
> +
> +void intel_edp_psr_force_exit(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_encoder *encoder;
> + struct intel_dp *intel_dp = NULL;
> +
> + if (!intel_edp_is_psr_enabled(dev))
> + return;
> +
> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
> + if (encoder->type == INTEL_OUTPUT_EDP)
> + intel_dp = enc_to_intel_dp(&encoder->base);
> +
> + if (!intel_dp)
> + return;
> +
> + if (WARN_ON(!intel_dp->psr_setup_done))
> + return;
If you have to dig out your data like this it usually means that it's at
the wrong spot. Imo we should track a psr_possible bit in the pipe_config
and a psr_enabled (and the locking for the runtime mutex) in the crtc
itself. Also, the psr_setup_done thing is another hint that we should move
this into the crtc.
Second I prefer if functions with tricky tie-in with existing code are
used in the same patch they're created - if it's split over two patches
review is a bit a pain. But if I read the follow-up patch correctly we
call this function from the busy_ioctl unconditionally, which will
obviously result in tons of falls postives - e.g. libdrm uses this ioctl
to manage it's buffer cache.
I think I'll punt on this patch and merge just the parts from the next one
for normal backbuffer rendering with pageflips model.
-Daniel
> +
> + mutex_lock(&intel_dp->psr_exit_mutex);
> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
> +EDP_PSR_DEBUG_FORCE_EXIT);
> + POSTING_READ(EDP_PSR_DEBUG_CTL);
> + mutex_unlock(&intel_dp->psr_exit_mutex);
> +
> + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
> + msecs_to_jiffies(100));
> +}
> +
> static void intel_edp_psr_write_vsc(struct intel_dp *intel_dp,
> struct edp_vsc_psr *vsc_psr)
> {
> @@ -1443,6 +1487,10 @@ static void intel_edp_psr_setup(struct intel_dp
> *intel_dp)
> I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
> EDP_PSR_DEBUG_MASK_HPD);
>
> + INIT_DELAYED_WORK(&intel_dp->edp_psr_delayed_normal_work,
> + intel_edp_psr_delayed_normal_work);
> + mutex_init(&intel_dp->psr_exit_mutex);
> +
> intel_dp->psr_setup_done = true;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0f52362..e47f3f3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @
Re: [Intel-gfx] [alsa-devel] [PATCH 0/4 V7] Power-well API implementation for Haswell
> -Original Message- > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Thursday, July 18, 2013 2:44 PM > To: Wang, Xingchao > Cc: Paulo Zanoni; daniel.vet...@ffwll.ch; alsa-de...@alsa-project.org; Daniel > Vetter; intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; Jin, > Gordon; Takashi Iwai; David Henningsson > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API > implementation for Haswell > > On Thu, Jul 18, 2013 at 01:00:07AM +, Wang, Xingchao wrote: > > Hi Paulo/Daniel, > > > > Do you agree to export an API in gfx side for eDP case? > > Basically the api will let audio drver know which pipe in use. i.e. in > > the eDP only caes, audio driver Will know gfx is not using HDMI/DP and would > like to let power-well off. > > As there's the conflict when user expect display audio driver always active > > but > gfx need audio driver off. > > Audio driver could make decision to release power-well if it knows the eDP > only case through the API. > > > > OTOH, I think audio driver could also export an API for gfx side, if > > gfx driver need audio driver release power-well but it's in usage, It will > > call > this API and audio drvier will release power-well accordingly. > > > > This change make HDMI/DP hotplug handling complicated in audio driver side, > if audio driver release power-well, it would enter suspend mode. > > Meanwhile the user may expect it's in active mode, this may cause some > confuse. > > Afaik (and I know very little about audio) the audio side already knows which > pipes have audio enabled, since we set the respective bit only when it's > needed. > And audio will receive the unsolicited even interrupt (or whatever it's > called) > when this happens. > For haswell, Audio driver can get DDI port/Pin usage information according to the unsolicited event, not Pipe info. However at this stage, seems only that is enough: if no pin has valid ELD, audio driver can think about that no monitor connected with DDI ports. In this case, Audio driver could release power-well and enter suspend mode automatically, this avoid blocking eDP feature enabling. And once gfx dirver Detect external monitor connected, it will also wake up audio driver. Takashi, do you think this solution acceptable? Thanks --xingchao > So I think we already have the means (albeit with that quirky hw interface, > but > it seems to have been good enough for a long time already) to do that. Or do I > miss something? > -Daniel > > > > > Thanks > > --xingchao > > > > > -Original Message- > > > From: Wang, Xingchao > > > Sent: Thursday, July 18, 2013 7:18 AM > > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni > > > Cc: alsa-de...@alsa-project.org; Daniel Vetter; > > > daniel.vet...@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang > > > xingchao; Girdwood, Liam R; Jin, Gordon > > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API > > > implementation for Haswell > > > > > > > > > > > > > -Original Message- > > > > From: Takashi Iwai [mailto:ti...@suse.de] > > > > Sent: Wednesday, July 17, 2013 10:22 PM > > > > To: David Henningsson > > > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-de...@alsa-project.org; > > > > Daniel Vetter; daniel.vet...@ffwll.ch; > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; > > > > Jin, Gordon > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well > > > > API implementation for Haswell > > > > > > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote: > > > > > > > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote: > > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote: > > > > > >> > > > > > >> 2013/7/17 Wang, Xingchao : > > > > > >>> > > > > > >>> > > > > > -Original Message- > > > > > From: Takashi Iwai [mailto:ti...@suse.de] > > > > > Sent: Wednesday, July 17, 2013 4:18 PM > > > > > To: Wang, Xingchao > > > > > Cc: Paulo Zanoni; alsa-de...@alsa-project.org; Daniel > > > > > Vetter; daniel.vet...@ffwll.ch; > > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, > > > > > Liam R; david.hennings...@canonical.com > > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] > > > > > Power-well API implementation for Haswell > > > > > > > > > > At Wed, 17 Jul 2013 08:03:38 +, Wang, Xingchao wrote: > > > > > > > > > > > > > > > > > > > > > > > >> -Original Message- > > > > > >> From: Takashi Iwai [mailto:ti...@suse.de] > > > > > >> Sent: Wednesday, July 17, 2013 3:34 PM > > > > > >> To: Wang, Xingchao > > > > > >> Cc: Paulo Zanoni; alsa-de...@alsa-project.org; Daniel > > > > > >> Vetter; daniel.vet...@ffwll.ch; > > > > > >> intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, > > > > > >> Liam R; david.hennings...@canonical.com > > > > > >> Subject: Re: [alsa-devel] [Intel-gfx]
Re: [Intel-gfx] [alsa-devel] [PATCH 0/4 V7] Power-well API implementation for Haswell
At Thu, 18 Jul 2013 09:23:57 +, Wang, Xingchao wrote: > > > > > -Original Message- > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Thursday, July 18, 2013 2:44 PM > > To: Wang, Xingchao > > Cc: Paulo Zanoni; daniel.vet...@ffwll.ch; alsa-de...@alsa-project.org; > > Daniel > > Vetter; intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; > > Jin, > > Gordon; Takashi Iwai; David Henningsson > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API > > implementation for Haswell > > > > On Thu, Jul 18, 2013 at 01:00:07AM +, Wang, Xingchao wrote: > > > Hi Paulo/Daniel, > > > > > > Do you agree to export an API in gfx side for eDP case? > > > Basically the api will let audio drver know which pipe in use. i.e. in > > > the eDP only caes, audio driver Will know gfx is not using HDMI/DP and > > > would > > like to let power-well off. > > > As there's the conflict when user expect display audio driver always > > > active but > > gfx need audio driver off. > > > Audio driver could make decision to release power-well if it knows the eDP > > only case through the API. > > > > > > OTOH, I think audio driver could also export an API for gfx side, if > > > gfx driver need audio driver release power-well but it's in usage, It > > > will call > > this API and audio drvier will release power-well accordingly. > > > > > > This change make HDMI/DP hotplug handling complicated in audio driver > > > side, > > if audio driver release power-well, it would enter suspend mode. > > > Meanwhile the user may expect it's in active mode, this may cause some > > confuse. > > > > Afaik (and I know very little about audio) the audio side already knows > > which > > pipes have audio enabled, since we set the respective bit only when it's > > needed. > > And audio will receive the unsolicited even interrupt (or whatever it's > > called) > > when this happens. > > > For haswell, Audio driver can get DDI port/Pin usage information according to > the unsolicited event, not Pipe info. > However at this stage, seems only that is enough: if no pin has valid ELD, > audio driver can think about that no monitor connected with DDI ports. > In this case, Audio driver could release power-well and enter suspend mode > automatically, this avoid blocking eDP feature enabling. And once gfx dirver > Detect external monitor connected, it will also wake up audio driver. > > Takashi, do you think this solution acceptable? It's the current situation, isn't it? So the question is only whether this works _as expected_. Basically system/user needs to set up two parameters for the audio power-saving. If both are set well, but it still doesn't work, we need to debug. Of course, we can improve things, for example, the default runtime PM setup. (Note that this is about the default value, not the value force to set.) Takashi > > Thanks > --xingchao > > > So I think we already have the means (albeit with that quirky hw interface, > > but > > it seems to have been good enough for a long time already) to do that. Or > > do I > > miss something? > > -Daniel > > > > > > > > Thanks > > > --xingchao > > > > > > > -Original Message- > > > > From: Wang, Xingchao > > > > Sent: Thursday, July 18, 2013 7:18 AM > > > > To: 'Takashi Iwai'; David Henningsson; Paulo Zanoni > > > > Cc: alsa-de...@alsa-project.org; Daniel Vetter; > > > > daniel.vet...@ffwll.ch; intel-gfx@lists.freedesktop.org; Wang > > > > xingchao; Girdwood, Liam R; Jin, Gordon > > > > Subject: RE: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well API > > > > implementation for Haswell > > > > > > > > > > > > > > > > > -Original Message- > > > > > From: Takashi Iwai [mailto:ti...@suse.de] > > > > > Sent: Wednesday, July 17, 2013 10:22 PM > > > > > To: David Henningsson > > > > > Cc: Paulo Zanoni; Wang, Xingchao; alsa-de...@alsa-project.org; > > > > > Daniel Vetter; daniel.vet...@ffwll.ch; > > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, Liam R; > > > > > Jin, Gordon > > > > > Subject: Re: [alsa-devel] [Intel-gfx] [PATCH 0/4 V7] Power-well > > > > > API implementation for Haswell > > > > > > > > > > At Wed, 17 Jul 2013 16:05:43 +0200, David Henningsson wrote: > > > > > > > > > > > > On 07/17/2013 04:00 PM, Takashi Iwai wrote: > > > > > > > At Wed, 17 Jul 2013 10:31:26 -0300, Paulo Zanoni wrote: > > > > > > >> > > > > > > >> 2013/7/17 Wang, Xingchao : > > > > > > >>> > > > > > > >>> > > > > > > -Original Message- > > > > > > From: Takashi Iwai [mailto:ti...@suse.de] > > > > > > Sent: Wednesday, July 17, 2013 4:18 PM > > > > > > To: Wang, Xingchao > > > > > > Cc: Paulo Zanoni; alsa-de...@alsa-project.org; Daniel > > > > > > Vetter; daniel.vet...@ffwll.ch; > > > > > > intel-gfx@lists.freedesktop.org; Wang xingchao; Girdwood, > > > > > > Liam R; david.hennings...@canonical.com > > > > > > Subject: Re: [alsa-devel]
Re: [Intel-gfx] [PATCH 11/11] drm/i915: Hook PSR functionality
On Thu, Jul 11, 2013 at 06:45:05PM -0300, Rodrigo Vivi wrote:
> PSR must be enabled after transcoder and port are running.
> And it is only available for HSW.
>
> v2: move enable/disable to intel_ddi
> v3: The spec suggests PSR should be disabled even before backlight (by
> pzanoni)
> v4: also disabling and enabling whenever panel is disabled/enabled.
> v5: make it last patch to avoid breaking whenever bisecting. So calling for
> update and force exit came to this patch along with enable/disable calls.
> v6: Remove unused and unecessary psr_enable/disable calls, as notice by Paulo.
>
> CC: Paulo Zanoni
> Signed-off-by: Rodrigo Vivi
Ok, I've slurped this series in with the few bikesheds from Paulo applied
and two patches not merged:
- The userspace interface part, since I don't want to commit yet to an
interface as long as things are unclear.
- And the invalidation part since imo that parts isn't properly thought
through yet. See my other mail to Chris' RFC for ideas.
Now on the topic fbc, psr and frontbuffer rendering:
Now that I've appeased our dear PDT it's time to stop building castles on
sand imo. Items to pay back a bit of the technical dept:
- Untangle the "is fbc/psr" allowed mess. Imo we should add more static
(i.e. only changes at modesets) information to the pipe config and track
dynamic reasons for disabling fbc/psr somewhere in the crtc. Also this
way update_fbc/psr would only need to check dynamic state and more
important would never need to frob the basic setup (like reallocating
the compressed buffer and similar things).
- Implement precise frontbuffer rendering tracking and disabling of
fbc/psr for legacy userspace and rip out the hw tracking. If we have to
add tons of workarounds (like for fbc), have performance this or just
can't use it in a bunch of cases (sprites for psr) we might as well just
track everything in the kernel.
- Testcases. With pipe CRC checksums we should be able to make sure that
the frontbuffer invalidation actually happens. And if we do it all in
the kernel the difference should be all-or-nothing, so much easier to
test than with hw tracking where sometimes something seems to slip
through.
- low-level improvements. I've only really reviewed the fbc code in-depth
and discussed a few things with Art, but there's definitely a few things
we need to do. One of the important things imo is to enable fbc
everywhere we can to have as wide test coverage as possible for this
feature.
I'll block future hw enabling in this area on the above list (i.e. vlv psr
or patches for -internal). I'll reconsider my stance if e.g. vlv psr is
used as a demonstration vehicle for the refactored psr tracking. But since
fbc can be used on many more machines (even desktops and apparently even
when other pipes are enabled) I think we should push that forward first
and use fbc to create solid tests and interfaces for userspace&kernel to
cooperate on frontbuffer rendering.
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 ++
> drivers/gpu/drm/i915/intel_ddi.c | 2 ++
> drivers/gpu/drm/i915/intel_display.c | 1 +
> 3 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 46bf7e3..703bc69 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3758,6 +3758,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> goto unlock;
> }
>
> + intel_edp_psr_force_exit(dev);
> +
> /* Count all active objects as busy, even if they are currently not used
>* by the gpu. Users of this interface expect objects to eventually
>* become non-busy without any further actions, therefore emit any
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 324211a..4211925 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder
> *intel_encoder)
> intel_dp_stop_link_train(intel_dp);
>
> ironlake_edp_backlight_on(intel_dp);
> + intel_edp_psr_enable(intel_dp);
> }
>
> if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
> @@ -1147,6 +1148,7 @@ static void intel_disable_ddi(struct intel_encoder
> *intel_encoder)
> if (type == INTEL_OUTPUT_EDP) {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>
> + intel_edp_psr_disable(intel_dp);
> ironlake_edp_backlight_off(intel_dp);
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 10a3629..eb4e49b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2272,6 +2272,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> }
>
> intel_update_fbc(dev);
> + intel_edp
Re: [Intel-gfx] [PATCH] drm/i915: correctly restore fences with objects attached
On Wed, Jul 17, 2013 at 03:44:16PM +0100, Chris Wilson wrote: > On Wed, Jul 17, 2013 at 02:51:28PM +0200, Daniel Vetter wrote: > > To avoid stalls we delay tiling changes and especially hold of > > committing the new fence state for as long as possible. > > Synchronization points are in the execbuf code and in our gtt fault > > handler. > > > > Unfortunately we've missed that tricky detail when adding proper fence > > restore code in > > > > commit 19b2dbde5732170a03bd82cc8bd442cf88d856f7 > > Author: Chris Wilson > > Date: Wed Jun 12 10:15:12 2013 +0100 > > > > drm/i915: Restore fences after resume and GPU resets > > > > The result was that we've restored fences for objects with no tiling, > > since the object<->fence link still existed after resume. Now that > > wouldn't have been too bad since any subsequent access would have > > fixed things up, but if we've changed from tiled to untiled real havoc > > happened: > > > > The tiling stride is stored -1 in the fence register, so a stride of 0 > > resulted in all 1s in the top 32bits, and so a completely bogus fence > > spanning everything from the start of the object to the top of the > > GTT. The tell-tale in the register dumps looks like: > > > > FENCE START 2: 0x0214d001 > > FENCE END 2: 0xf3ff > > > > Bit 11 isn't set since the hw doesn't store it, even when writing all > > 1s (at least on my snb here). > > > > To prevent such a gaffle in the future add a sanity check for fences > > with an untiled object attached in i915_gem_write_fence. > > > > v2: Fix the WARN, spotted by Chris. > > > > v3: Trying to reuse get_fences looked ugly and obfuscated the code. > > Instead reuse update_fence and to make it really dtrt also move the > > fence dirty state clearing into update_fence. > > > > Cc: Chris Wilson > > Cc: Stéphane Marchesin > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=60530 > > Cc: sta...@vger.kernel.org (for 3.10 only) > > Signed-off-by: Daniel Vetter > > Sigh. I thought we were covered because before anything accessed this > dirty object, the fence would have been rewritten. However, Daniel > correctly points out that the stride==0 fence clobbers the entire GTT > and so may be used by the hardware in preference to any other fence. > Reviewed-by: Chris Wilson Picked up for -fixes, thanks for the review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-next for 3.12
Hi Dave, I know you usually don't open -next this early, but since this here contains a few changes to drm_mm I've figured I should unblock David Herrmann. Highlights: - follow-up refactoring after the shared dpll rework that landed in 3.11 - oddball prep cleanups from Ben for ppgtt - encoder->get_config state tracking infrastructure from Jesse - used by the experimental fastboot support from Jesse (disabled by default) - make the error state file official and add it to our sysfs interface (Mika) - drm_mm prep changes from Ben, prepares to embedd the drm_mm_node (which will be used by the vma rework later on) - interrupt handling rework, follow up cleanups to the VECS enabling, hpd storm handling and fifo underrun reporting. - Big pile of smaller cleanups, code improvements and related stuff. My current -next queue and -fixes have a bunch of conflicts, but a quit test merge of just this pull confirmed that the fun only starts later on and the merge doesn't have real conflicts, there's only two benign cases in i915_gem_stolen.c of adjacent changes. Cheers, Daniel The following changes since commit baf27f9b17bf2f369f3865e38c41d2163e8d815d: drm/i915: Break up the large vsnprintf() in print_error_buffers() (2013-07-01 11:15:01 +0200) are available in the git repository at: git://people.freedesktop.org/~danvet/drm-intel tags/drm-intel-next-2013-07-12 for you to fetch changes up to 50b44a449ff1a19712ebc36ffccf9ac0a68033bf: drm/i915: clear DPLL reg when disabling i9xx dplls (2013-07-12 18:54:02 +0200) Ben Widawsky (14): drm/i915: Remove extra error state NULL drm/i915: Extract error buffer capture drm/i915: make PDE|PTE platform specific drm/i915: Really share scratch page drm/i915: Combine scratch members into a struct drm/i915: Drop dev from pte_encode drm/i915: Use gtt shortform where possible drm/i915: Move fbc members out of line drm/i915: Move gtt_mtrr to i915_gtt drm: pre allocate node for create_block drm: Change create block to reserve node drm/i915: Getter/setter for object attributes drm/i915: Kill obj->gtt_offset drm/i915: Embed drm_mm_node in i915 gem obj Chris Wilson (1): drm/i915: Verify that our stolen memory doesn't conflict Damien Lespiau (8): drm/i915: Make intel_enable_fbc() static drm/i915: Fix reason for per-chip disabling of FBC drm/i915: Use seq_puts/seq_putc when possible drm/i915: Fix a few style issues found by checkpatch.pl drm/i915: Fix a couple of "should it be static?" sparse warnings drm/i915: Bail out once we've found the context object drm/i915: Use for_each_pipe() when possible drm/i915: Don't attempt to read an unitialized stack value Daniel Vetter (32): drm/i915: consolidate pch pll enable sequence drm/i915: use sw tracked state to select shared dplls drm/i915: duplicate intel_enable_pll into i9xx and vlv versions drm/i915: asserts for lvds pre_enable drm/i915: move encoder pre enable hooks togther on ilk+ drm/i915: hw state readout for i9xx dplls drm/i915: move i9xx dpll enabling into crtc enable function drm/i915: s/pre_pll/pre/ on the lvds port enable function drm/i915: pixel multiplier readout support for pch ports drm/i915: explicitly cast pipe -> cpu_transcoder drm/i915: Explicitly cast pipe -> intel_dpll_id drm/i915: less magic for stolen preallocated objects w/o gtt offset drm/i915: assert_spin_locked for pipestat interrupt enable/disable drm/i915: fix dvo DPLL regression drm/i915: dvo needs a P2 divisor of 4 drm/i915: convert debugfs creation/destruction to table drm/i915: clean up media reset on gm45 drm/i915: WARN if the bios reserved range is bigger than stolen size drm/i915: don't frob mm.suspended when not using ums drm/i915: extract ibx_display_interrupt_update drm/i915: improve SERR_INT clearing for fifo underrun reporting drm/i915: improve GEN7_ERR_INT clearing for fifo underrun reporting drm/i915: kill lpt pch transcoder->crtc mapping code for fifo underruns drm/i915: irq handlers don't need interrupt-safe spinlocks drm/i915: streamline hsw_pm_irq_handler drm/i915: queue work outside spinlock in hsw_pm_irq_handler drm/i915: kill dev_priv->rps.lock drm/i915: unify ring irq refcounts (again) drm/i915: don't enable PM_VEBOX_CS_ERROR_INTERRUPT drm/i915: clean up vlv ->pre_pll_enable and pll enable sequence drm/i915: Fix up cpt pixel multiplier enable sequence drm/i915: clear DPLL reg when disabling i9xx dplls Jesse Barnes (7): drm/i915: add fastboot param for fast & loose mode setting drm/i915: get mode clock when reading the pipe config v9 drm/i915: copy fetched mode state into crtc at setup_hw time v5 drm/i915: turn off
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Create VMAs
On Wed, 2013-07-17 at 12:19 -0700, Ben Widawsky wrote:
> Formerly: "drm/i915: Create VMAs (part 1)"
>
> In a previous patch, the notion of a VM was introduced. A VMA describes
> an area of part of the VM address space. A VMA is similar to the concept
> in the linux mm. However, instead of representing regular memory, a VMA
> is backed by a GEM BO. There may be many VMAs for a given object, one
> for each VM the object is to be used in. This may occur through flink,
> dma-buf, or a number of other transient states.
>
> Currently the code depends on only 1 VMA per object, for the global GTT
> (and aliasing PPGTT). The following patches will address this and make
> the rest of the infrastructure more suited
>
> v2: s/i915_obj/i915_gem_obj (Chris)
>
> v3: Only move an object to the now global unbound list if there are no
> more VMAs for the object which are bound into a VM (ie. the list is
> empty).
>
> v4: killed obj->gtt_space
> some reworks due to rebase
>
> v5: Free vma on error path (Imre)
>
> v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> (Imre)
> Fixed vma freeing in stolen preallocation (Imre)
>
> Signed-off-by: Ben Widawsky
Looks ok, so on patches 5-6:
Reviewed-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.h| 48 +-
> drivers/gpu/drm/i915/i915_gem.c| 74
> +++---
> drivers/gpu/drm/i915/i915_gem_evict.c | 12 --
> drivers/gpu/drm/i915/i915_gem_gtt.c| 5 ++-
> drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +--
> 5 files changed, 120 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3ba428..1a32412 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
> int (*enable)(struct drm_device *dev);
> };
>
> +/* To make things as simple as possible (ie. no refcounting), a VMA's
> lifetime
> + * will always be <= an objects lifetime. So object refcounting should cover
> us.
> + */
> +struct i915_vma {
> + struct drm_mm_node node;
> + struct drm_i915_gem_object *obj;
> + struct i915_address_space *vm;
> +
> + struct list_head vma_link; /* Link in the object's VMA list */
> +};
> +
> struct i915_ctx_hang_stats {
> /* This context had batch pending when hang was declared */
> unsigned batch_pending;
> @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
>
> const struct drm_i915_gem_object_ops *ops;
>
> - /** Current space allocated to this object in the GTT, if any. */
> - struct drm_mm_node gtt_space;
> + /** List of VMAs backed by this object */
> + struct list_head vma_list;
> +
> /** Stolen memory for this object, instead of being backed by shmem. */
> struct drm_mm_node *stolen;
> struct list_head global_list;
> @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
>
> #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
>
> -/* Offset of the first PTE pointing to this object */
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +/* This is a temporary define to help transition us to real VMAs. If you see
> + * this, you're either reviewing code, or bisecting it. */
> +static inline struct i915_vma *
> +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
> {
> - return o->gtt_space.start;
> + if (list_empty(&obj->vma_list))
> + return NULL;
> + return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
> }
>
> /* Whether or not this object is currently mapped by the translation tables
> */
> static inline bool
> i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> {
> - return drm_mm_node_allocated(&o->gtt_space);
> + struct i915_vma *vma = __i915_gem_obj_to_vma(o);
> + if (vma == NULL)
> + return false;
> + return drm_mm_node_allocated(&vma->node);
> +}
> +
> +/* Offset of the first PTE pointing to this object */
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +{
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.start;
> }
>
> /* The size used in the translation tables may be larger than the actual
> size of
> @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
> static inline unsigned long
> i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
> {
> - return o->gtt_space.size;
> + BUG_ON(list_empty(&o->vma_list));
> + return __i915_gem_obj_to_vma(o)->node.size;
> }
>
> static inline void
> i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
> enum i915_cache_level color)
> {
> - o->gtt_space.color = color;
> + __i915_gem_obj_to_vma(o)->node.color = color;
> }
>
> /**
> @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file
On Tue, Jul 16, 2013 at 08:02:11PM +0100, Chris Wilson wrote:
> Currently, the register access code is split between i915_drv.c and
> intel_pm.c. It only bares a superficial resemblance to the reset of the
> powermanagement code, so move it all into its own file. This is to ease
> further patches to enforce serialised register access.
>
> v2: Scan for random abuse of I915_WRITE_NOTRACE
> v3: Take the opportunity to rename the GT functions as uncore. Uncore is
> the term used by the hardware design (and bspec) for all functions
> outside of the GPU (and CPU) cores in what is also known as the System
> Agent.
>
> Signed-off-by: Chris Wilson
> Reviewed-by: Ben Widawsky
I've tried to apply it, but this patch does way to many things at once. So
the oddball change we have compared to the baseline of these patches
resulted in conflict hell.
And it's too big for -fixes. Imo the following should be dropped, at least
for -fixes:
- renaming stuff from gt to uncore
- moving code to intel_uncore.c which we don't strictly need to apply the
bugfix like the reset code.
So just a plain boring "move code together" patch.
-Daniel
> ---
> drivers/gpu/drm/i915/Makefile| 1 +
> drivers/gpu/drm/i915/i915_debugfs.c | 12 +-
> drivers/gpu/drm/i915/i915_dma.c | 8 +-
> drivers/gpu/drm/i915/i915_drv.c | 268 +
> drivers/gpu/drm/i915/i915_drv.h | 30 +-
> drivers/gpu/drm/i915/i915_irq.c | 6 +-
> drivers/gpu/drm/i915/intel_display.c | 3 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> drivers/gpu/drm/i915/intel_pm.c | 258 +---
> drivers/gpu/drm/i915/intel_uncore.c | 569
> +++
> 10 files changed, 609 insertions(+), 547 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/intel_uncore.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 9d1da7c..b8449a8 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -38,6 +38,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
> intel_sprite.o \
> intel_opregion.o \
> intel_sideband.o \
> + intel_uncore.o \
> dvo_ch7xxx.o \
> dvo_ch7017.o \
> dvo_ivch.o \
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index acbc28c..8d600a7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -987,9 +987,9 @@ static int gen6_drpc_info(struct seq_file *m)
> if (ret)
> return ret;
>
> - spin_lock_irq(&dev_priv->gt_lock);
> - forcewake_count = dev_priv->forcewake_count;
> - spin_unlock_irq(&dev_priv->gt_lock);
> + spin_lock_irq(&dev_priv->uncore.lock);
> + forcewake_count = dev_priv->uncore.forcewake_count;
> + spin_unlock_irq(&dev_priv->uncore.lock);
>
> if (forcewake_count) {
> seq_puts(m, "RC information inaccurate because somebody "
> @@ -1367,9 +1367,9 @@ static int i915_gen6_forcewake_count_info(struct
> seq_file *m, void *data)
> struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned forcewake_count;
>
> - spin_lock_irq(&dev_priv->gt_lock);
> - forcewake_count = dev_priv->forcewake_count;
> - spin_unlock_irq(&dev_priv->gt_lock);
> + spin_lock_irq(&dev_priv->uncore.lock);
> + forcewake_count = dev_priv->uncore.forcewake_count;
> + spin_unlock_irq(&dev_priv->uncore.lock);
>
> seq_printf(m, "forcewake count = %u\n", forcewake_count);
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a1d04b2..4607841 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1445,10 +1445,7 @@ static void i915_dump_device_info(struct
> drm_i915_private *dev_priv)
> */
> static void intel_early_sanitize_regs(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - if (HAS_FPGA_DBG_UNCLAIMED(dev))
> - I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> + intel_uncore_early_sanitize(dev);
> }
>
> /**
> @@ -1590,7 +1587,8 @@ int i915_driver_load(struct drm_device *dev, unsigned
> long flags)
> intel_detect_pch(dev);
>
> intel_irq_init(dev);
> - intel_gt_init(dev);
> + intel_uncore_init(dev);
> + intel_pm_init(dev);
>
> /* Try to make sure MCHBAR is enabled before poking at it */
> intel_setup_mchbar(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b07362f..3c438a7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -714,7 +714,7 @@ static int i915_drm_thaw(struct drm_device *dev)
> {
> int error = 0;
>
> - intel_gt_reset(dev);
> + intel_uncore_reset(dev);
>
> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> mutex_lock(&dev->struct_mutex);
> @@ -740,7 +740,7 @@
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file
On Thu, Jul 18, 2013 at 02:15:59PM +0200, Daniel Vetter wrote: > On Tue, Jul 16, 2013 at 08:02:11PM +0100, Chris Wilson wrote: > > Currently, the register access code is split between i915_drv.c and > > intel_pm.c. It only bares a superficial resemblance to the reset of the > > powermanagement code, so move it all into its own file. This is to ease > > further patches to enforce serialised register access. > > > > v2: Scan for random abuse of I915_WRITE_NOTRACE > > v3: Take the opportunity to rename the GT functions as uncore. Uncore is > > the term used by the hardware design (and bspec) for all functions > > outside of the GPU (and CPU) cores in what is also known as the System > > Agent. > > > > Signed-off-by: Chris Wilson > > Reviewed-by: Ben Widawsky > > I've tried to apply it, but this patch does way to many things at once. So > the oddball change we have compared to the baseline of these patches > resulted in conflict hell. > > And it's too big for -fixes. Imo the following should be dropped, at least > for -fixes: > - renaming stuff from gt to uncore > - moving code to intel_uncore.c which we don't strictly need to apply the > bugfix like the reset code. > > So just a plain boring "move code together" patch. I've forgotten to add: For -fixes I want to only merge up to "drm/i915: Serialize all register access", so wrestling just those patches is ok. We can slurp the later ones (I do like them) once I've done a backmerge into dinq. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file
On Thu, Jul 18, 2013 at 02:18:19PM +0200, Daniel Vetter wrote: > On Thu, Jul 18, 2013 at 02:15:59PM +0200, Daniel Vetter wrote: > > On Tue, Jul 16, 2013 at 08:02:11PM +0100, Chris Wilson wrote: > > > Currently, the register access code is split between i915_drv.c and > > > intel_pm.c. It only bares a superficial resemblance to the reset of the > > > powermanagement code, so move it all into its own file. This is to ease > > > further patches to enforce serialised register access. > > > > > > v2: Scan for random abuse of I915_WRITE_NOTRACE > > > v3: Take the opportunity to rename the GT functions as uncore. Uncore is > > > the term used by the hardware design (and bspec) for all functions > > > outside of the GPU (and CPU) cores in what is also known as the System > > > Agent. > > > > > > Signed-off-by: Chris Wilson > > > Reviewed-by: Ben Widawsky > > > > I've tried to apply it, but this patch does way to many things at once. So > > the oddball change we have compared to the baseline of these patches > > resulted in conflict hell. > > > > And it's too big for -fixes. Imo the following should be dropped, at least > > for -fixes: > > - renaming stuff from gt to uncore > > - moving code to intel_uncore.c which we don't strictly need to apply the > > bugfix like the reset code. > > > > So just a plain boring "move code together" patch. > > I've forgotten to add: For -fixes I want to only merge up to "drm/i915: > Serialize all register access", so wrestling just those patches is ok. We > can slurp the later ones (I do like them) once I've done a backmerge into > dinq. I disagree, the 6 line patch seems quite adequate for stable. -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/6] drm/i915: Colocate all GT access routines in the same file
On Thu, Jul 18, 2013 at 02:15:59PM +0200, Daniel Vetter wrote: > On Tue, Jul 16, 2013 at 08:02:11PM +0100, Chris Wilson wrote: > > Currently, the register access code is split between i915_drv.c and > > intel_pm.c. It only bares a superficial resemblance to the reset of the > > powermanagement code, so move it all into its own file. This is to ease > > further patches to enforce serialised register access. > > > > v2: Scan for random abuse of I915_WRITE_NOTRACE > > v3: Take the opportunity to rename the GT functions as uncore. Uncore is > > the term used by the hardware design (and bspec) for all functions > > outside of the GPU (and CPU) cores in what is also known as the System > > Agent. > > > > Signed-off-by: Chris Wilson > > Reviewed-by: Ben Widawsky > > I've tried to apply it, but this patch does way to many things at once. So > the oddball change we have compared to the baseline of these patches > resulted in conflict hell. The conflict is trivial. -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
[Intel-gfx] [PATCH] drm/i915: make i915_get_extra_instdone() static
commit 84734a049d0ef2f6f5fb0a1fe060cd51480dd855
Author: Mika Kuoppala
Date: Fri Jul 12 16:50:57 2013 +0300
drm/i915: move error state to own compilation unit
had to export i915_get_extra_instdone() as it was used
also in i915_irq.c. Daniel Vetter noticed that
as i915_capture_error_state() is already storing instdone,
there is no need to do it twice in a row nor need to export it.
Requested-by: Daniel Vetter
Signed-off-by: Mika Kuoppala
---
drivers/gpu/drm/i915/i915_drv.h |2 --
drivers/gpu/drm/i915/i915_gpu_error.c | 56 -
drivers/gpu/drm/i915/i915_irq.c |2 --
3 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c93ab68..2c3f358 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1963,8 +1963,6 @@ void i915_error_state_get(struct drm_device *dev,
struct i915_error_state_file_priv *error_priv);
void i915_error_state_put(struct i915_error_state_file_priv *error_priv);
void i915_destroy_error_state(struct drm_device *dev);
-
-void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone);
const char *i915_cache_level_str(int type);
/* i915_suspend.c */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 58386ce..bbcfd88 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -806,6 +806,34 @@ static void i915_gem_capture_buffers(struct
drm_i915_private *dev_priv,
&dev_priv->mm.bound_list);
}
+/* NB: please notice the memset */
+static void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ memset(instdone, 0, sizeof(*instdone) * I915_NUM_INSTDONE_REG);
+
+ switch (INTEL_INFO(dev)->gen) {
+ case 2:
+ case 3:
+ instdone[0] = I915_READ(INSTDONE);
+ break;
+ case 4:
+ case 5:
+ case 6:
+ instdone[0] = I915_READ(INSTDONE_I965);
+ instdone[1] = I915_READ(INSTDONE1);
+ break;
+ default:
+ WARN_ONCE(1, "Unsupported platform\n");
+ case 7:
+ instdone[0] = I915_READ(GEN7_INSTDONE_1);
+ instdone[1] = I915_READ(GEN7_SC_INSTDONE);
+ instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
+ instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
+ break;
+ }
+}
+
/**
* i915_capture_error_state - capture an error record for later analysis
* @dev: drm device
@@ -941,31 +969,3 @@ const char *i915_cache_level_str(int type)
default: return "";
}
}
-
-/* NB: please notice the memset */
-void i915_get_extra_instdone(struct drm_device *dev, uint32_t *instdone)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- memset(instdone, 0, sizeof(*instdone) * I915_NUM_INSTDONE_REG);
-
- switch (INTEL_INFO(dev)->gen) {
- case 2:
- case 3:
- instdone[0] = I915_READ(INSTDONE);
- break;
- case 4:
- case 5:
- case 6:
- instdone[0] = I915_READ(INSTDONE_I965);
- instdone[1] = I915_READ(INSTDONE1);
- break;
- default:
- WARN_ONCE(1, "Unsupported platform\n");
- case 7:
- instdone[0] = I915_READ(GEN7_INSTDONE_1);
- instdone[1] = I915_READ(GEN7_SC_INSTDONE);
- instdone[2] = I915_READ(GEN7_SAMPLER_INSTDONE);
- instdone[3] = I915_READ(GEN7_ROW_INSTDONE);
- break;
- }
-}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9910699..9ec3080 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1500,8 +1500,6 @@ static void i915_report_and_clear_eir(struct drm_device
*dev)
pr_err("render error detected, EIR: 0x%08x\n", eir);
- i915_get_extra_instdone(dev, instdone);
-
if (IS_G4X(dev)) {
if (eir & (GM45_ERROR_MEM_PRIV | GM45_ERROR_CP_PRIV)) {
u32 ipeir = I915_READ(IPEIR_I965);
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: restore debug message lost in merge resolution
Restore debug message lost in merge commit e1b73cba13. Also clarify it
that we are only clamping bpp not overwriting it.
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/intel_dp.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9868600..79b7490 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -705,8 +705,11 @@ intel_dp_compute_config(struct intel_encoder *encoder,
/* Walk through all bpp values. Luckily they're all nicely spaced with 2
* bpc in between. */
bpp = pipe_config->pipe_bpp;
- if (is_edp(intel_dp) && dev_priv->vbt.edp_bpp)
+ if (is_edp(intel_dp) && dev_priv->vbt.edp_bpp) {
+ DRM_DEBUG_KMS("clamping bpp for eDP panel to BIOS-provided
%i\n",
+ dev_priv->vbt.edp_bpp);
bpp = min_t(int, bpp, dev_priv->vbt.edp_bpp);
+ }
for (; bpp >= 6*3; bpp -= 2*3) {
mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
--
1.8.3.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [drm-intel:drm-intel-fixes 52/52] ERROR: Unrecognized email address: 'stable.]'
Joe, > (It would have been nice to get the content that failes > instead of having to pull the tree) Good suggestion! I'll attach the git-format-patch result in the checkpatch.pl reports in future. Thanks, Fengguang ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Provide a simulation friendly test environment v4
It was high time to follow up on: http://lists.freedesktop.org/archives/intel-gfx/2013-April/027361.html Changes from v2: • Take into account Daniel's comments and add the tests he listed • Actually runs on simulation now -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/8] lib: Rename IGT_QUICK to INTEL_SIMULATION
It's more accurate this way as the quick mode is really useful for in
the simulation environment.
v2: Use the INTEL_ prefix to have a chance to share the same environment
variable as piglit OpenGL tests
Signed-off-by: Damien Lespiau
---
lib/drmtest.c | 10 +-
lib/drmtest.h | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/lib/drmtest.c b/lib/drmtest.c
index 011d8c1..76c84b1 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -699,14 +699,14 @@ static bool env_set(const char *env_var)
return atoi(val) != 0;
}
-bool drmtest_run_quick(void)
+bool drmtest_run_in_simulation(void)
{
- static int run_quick = -1;
+ static int simulation = -1;
- if (run_quick == -1)
- run_quick = env_set("IGT_QUICK");
+ if (simulation == -1)
+ simulation = env_set("INTEL_SIMULATION");
- return run_quick;
+ return simulation;
}
/* other helpers */
diff --git a/lib/drmtest.h b/lib/drmtest.h
index e3a9275..5050a5d 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -93,8 +93,8 @@ void drmtest_subtest_init(int argc, char **argv);
bool drmtest_run_subtest(const char *subtest_name);
bool drmtest_only_list_subtests(void);
-bool drmtest_run_quick(void);
-#define SLOW_QUICK(slow,quick) (drmtest_run_quick() ? (quick) : (slow))
+bool drmtest_run_in_simulation(void);
+#define SLOW_QUICK(slow,quick) (drmtest_run_in_simulation() ? (quick) : (slow))
/* helpers based upon the libdrm buffer manager */
void drmtest_init_aperture_trashers(drm_intel_bufmgr *bufmgr);
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
This will allow us to explicitely blacklist tests we don't want to run
on simulation.
Signed-off-by: Damien Lespiau
---
lib/drmtest.c | 12
lib/drmtest.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/lib/drmtest.c b/lib/drmtest.c
index 76c84b1..a9a7498 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include "drm_fourcc.h"
#include "drmtest.h"
@@ -709,6 +710,17 @@ bool drmtest_run_in_simulation(void)
return simulation;
}
+/* Skip the test when running on simulation (and that's relevant only when
+ * we're not in the mode where we list the subtests) */
+void drmtest_skip_on_simulation(void)
+{
+ if (drmtest_only_list_subtests())
+ return;
+
+ if (drmtest_run_in_simulation())
+ exit(77);
+}
+
/* other helpers */
void drmtest_exchange_int(void *array, unsigned i, unsigned j)
{
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 5050a5d..c31fed1 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -95,6 +95,7 @@ bool drmtest_only_list_subtests(void);
bool drmtest_run_in_simulation(void);
#define SLOW_QUICK(slow,quick) (drmtest_run_in_simulation() ? (quick) : (slow))
+void drmtest_skip_on_simulation(void);
/* helpers based upon the libdrm buffer manager */
void drmtest_init_aperture_trashers(drm_intel_bufmgr *bufmgr);
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/8] tests: Black list tests we don't want to run on simulation
Let's start by a small set of tests, to eventually consider running
more.
The current list should then be:
gem_mmap
gem_pread_after_blit
gem_ring_sync_loop
gem_ctx_basic
gem_pipe_control_store_loop
gem_storedw_loop_render
gem_storedw_loop_blt
gem_storedw_loop_bsd
gem_render_linear_blits
gem_tiled_blits
gem_cpu_reloc
gem_exec_nop
gem_mmap_gtt
v2 add (Daniel Vetter)
gem_exec_bad_domains
gem_exec_faulting_reloc
gem_flink
gem_reg_read
gem_reloc_overflow
gem_tiling_max_stride
prime_*
Signed-off-by: Damien Lespiau
---
tests/drm_lib.sh | 7 +++
tests/drm_vma_limiter.c| 2 ++
tests/drm_vma_limiter_cached.c | 2 ++
tests/drm_vma_limiter_cpu.c| 2 ++
tests/drm_vma_limiter_gtt.c| 2 ++
tests/gem_bad_length.c | 2 ++
tests/gem_cacheing.c | 1 +
tests/gem_cpu_concurrent_blit.c| 1 +
tests/gem_cs_prefetch.c| 2 ++
tests/gem_cs_tlb.c | 1 +
tests/gem_ctx_bad_destroy.c| 2 ++
tests/gem_ctx_bad_exec.c | 3 +++
tests/gem_ctx_create.c | 2 ++
tests/gem_ctx_exec.c | 3 +++
tests/gem_double_irq_loop.c| 2 ++
tests/gem_dummy_reloc_loop.c | 1 +
tests/gem_exec_big.c | 2 ++
tests/gem_exec_blt.c | 2 ++
tests/gem_exec_lut_handle.c| 2 ++
tests/gem_fence_thrash.c | 1 +
tests/gem_fenced_exec_thrash.c | 12 +---
tests/gem_gtt_concurrent_blit.c| 1 +
tests/gem_gtt_cpu_tlb.c| 2 ++
tests/gem_gtt_speed.c | 2 ++
tests/gem_hangcheck_forcewake.c| 2 ++
tests/gem_largeobject.c| 2 ++
tests/gem_linear_blits.c | 2 ++
tests/gem_lut_handle.c | 2 ++
tests/gem_mmap_offset_exhaustion.c | 2 ++
tests/gem_partial_pwrite_pread.c | 1 +
tests/gem_pin.c| 2 ++
tests/gem_pwrite.c | 2 ++
tests/gem_readwrite.c | 2 ++
tests/gem_reloc_vs_gpu.c | 2 ++
tests/gem_render_tiled_blits.c | 2 ++
tests/gem_ringfill.c | 1 +
tests/gem_seqno_wrap.c | 2 ++
tests/gem_set_tiling_vs_blt.c | 1 +
tests/gem_set_tiling_vs_gtt.c | 2 ++
tests/gem_set_tiling_vs_pwrite.c | 2 ++
tests/gem_storedw_batches_loop.c | 2 ++
tests/gem_threaded_access_tiled.c | 2 ++
tests/gem_tiled_fence_blits.c | 2 ++
tests/gem_tiled_partial_pwrite_pread.c | 1 +
tests/gem_tiled_pread.c| 2 ++
tests/gem_tiled_pread_pwrite.c | 1 +
tests/gem_tiled_swapping.c | 2 ++
tests/gem_unfence_active_buffers.c | 2 ++
tests/gem_unref_active_buffers.c | 2 ++
tests/gem_vmap_blits.c | 2 ++
tests/gem_wait_render_timeout.c| 2 ++
tests/gem_write_read_ring_switch.c | 1 +
tests/kms_flip.c | 1 +
tests/kms_render.c | 1 +
tests/prime_udl.c | 3 +++
tests/sysfs_edid_timing| 2 ++
tests/sysfs_rc6_residency.c| 2 ++
tests/sysfs_rps.c | 2 ++
tests/testdisplay.c| 2 ++
59 files changed, 119 insertions(+), 3 deletions(-)
diff --git a/tests/drm_lib.sh b/tests/drm_lib.sh
index 5ca815b..2532352 100755
--- a/tests/drm_lib.sh
+++ b/tests/drm_lib.sh
@@ -37,3 +37,10 @@ if [ -d /sys/class/drm ] ; then
fi
fi
# sysfs may not exist as the 'error' is a new interface in 3.11
+
+function drmtest_skip_on_simulation()
+{
+ [ -n "$INTEL_SIMULATION" ] && exit 77
+}
+
+drmtest_skip_on_simulation
diff --git a/tests/drm_vma_limiter.c b/tests/drm_vma_limiter.c
index 1971e2d..e5025ef 100644
--- a/tests/drm_vma_limiter.c
+++ b/tests/drm_vma_limiter.c
@@ -60,6 +60,8 @@ int main(int argc, char **argv)
int i;
char *ptr;
+ drmtest_skip_on_simulation();
+
fd = drm_open_any();
bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
diff --git a/tests/drm_vma_limiter_cached.c b/tests/drm_vma_limiter_cached.c
index 3797618..1144796 100644
--- a/tests/drm_vma_limiter_cached.c
+++ b/tests/drm_vma_limiter_cached.c
@@ -61,6 +61,8 @@ int main(int argc, char **argv)
char *ptr;
drm_intel_bo *load_bo;
+ drmtest_skip_on_simulation();
+
fd = drm_open_any();
bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
diff --git a/tests/drm_vma_limiter_cpu.c b/tests/drm_vma_limiter_cpu.c
index 24ce188..6f65cdb 100644
--- a/tests/drm_vma_limiter_cpu.c
+++ b/tests/drm_vma_limiter_cpu.c
@@ -61,6 +61,8 @@ int main(int argc, char **argv)
int i;
char *ptr;
+ drmtest_skip_on_simulation();
+
fd = drm_open_any();
bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
diff --git a/tests/drm_vma_limiter_gtt.c b/tests/drm_vma_
[Intel-gfx] [PATCH 5/8] tests: Instrument tests run in simulation to run quickly
We tweak the tests marked as runnable in simulation to run more quickly,
more often then not at the expense of stress testing (which is of an
arguable interest for the initial bring up in simulation). Hopefully the
values chosen still test something, which is not always straightforward.
It does run quickly, the number on an IVB machines are:
$ time sudo IGT_SIMULATION=0 ./piglit-run.py tests/igt.tests foo
[...]
real2m0.141s
user0m16.365s
sys 1m33.382s
Vs.
$ time sudo IGT_SIMULATION=1 ./piglit-run.py tests/igt.tests foo
[...]
real0m0.448s
user0m0.226s
sys 0m0.183s
Signed-off-by: Damien Lespiau
---
tests/gem_cpu_reloc.c | 3 +++
tests/gem_ctx_basic.c | 5 +
tests/gem_exec_nop.c| 2 +-
tests/gem_mmap_gtt.c| 5 -
tests/gem_pipe_control_store_loop.c | 2 +-
tests/gem_render_linear_blits.c | 6 ++
tests/gem_ring_sync_loop.c | 2 +-
tests/gem_storedw_batches_loop.c| 8 +---
tests/gem_storedw_loop_blt.c| 6 --
tests/gem_storedw_loop_bsd.c| 6 --
tests/gem_storedw_loop_render.c | 6 --
tests/gem_tiled_blits.c | 5 +
12 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/tests/gem_cpu_reloc.c b/tests/gem_cpu_reloc.c
index baf8301..ad70e40 100644
--- a/tests/gem_cpu_reloc.c
+++ b/tests/gem_cpu_reloc.c
@@ -153,6 +153,9 @@ int main(int argc, char **argv)
}
count = aper_size / 4096 * 2;
+ if (drmtest_run_in_simulation())
+ count = 10;
+
handles = malloc (count * sizeof(uint32_t));
assert(handles);
diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
index 3757d84..729c79c 100644
--- a/tests/gem_ctx_basic.c
+++ b/tests/gem_ctx_basic.c
@@ -143,6 +143,11 @@ int main(int argc, char *argv[])
fd = drm_open_any();
devid = intel_get_drm_devid(fd);
+ if (drmtest_run_in_simulation()) {
+ num_contexts = 2;
+ iter = 4;
+ }
+
parse(argc, argv);
threads = calloc(num_contexts, sizeof(*threads));
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index 5888432..35cb23d 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -96,7 +96,7 @@ static void loop(int fd, uint32_t handle, unsigned ring_id,
const char *ring_nam
skipped_all = false;
- for (count = 1; count <= 1<<17; count <<= 1) {
+ for (count = 1; count <= SLOW_QUICK(1<<17, 1<<4); count <<= 1) {
struct timeval start, end;
gettimeofday(&start, NULL);
diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c
index d759340..e53a942 100644
--- a/tests/gem_mmap_gtt.c
+++ b/tests/gem_mmap_gtt.c
@@ -40,7 +40,7 @@
#include "i915_drm.h"
#include "drmtest.h"
-#define OBJECT_SIZE (16*1024*1024)
+static int OBJECT_SIZE = 16*1024*1024;
static void set_domain(int fd, uint32_t handle)
{
@@ -148,6 +148,9 @@ int main(int argc, char **argv)
{
int fd;
+ if (drmtest_run_in_simulation())
+ OBJECT_SIZE = 1 * 1024 * 1024;
+
drmtest_subtest_init(argc, argv);
fd = drm_open_any();
diff --git a/tests/gem_pipe_control_store_loop.c
b/tests/gem_pipe_control_store_loop.c
index e03cddd..af6a758 100644
--- a/tests/gem_pipe_control_store_loop.c
+++ b/tests/gem_pipe_control_store_loop.c
@@ -70,7 +70,7 @@ store_pipe_control_loop(void)
uint32_t *buf;
drm_intel_bo *target_bo;
- for (i = 0; i < 0x1; i++) {
+ for (i = 0; i < SLOW_QUICK(0x1, 4); i++) {
/* we want to check tlb consistency of the pipe_control target,
* so get a new buffer every time around */
target_bo = drm_intel_bo_alloc(bufmgr, "target bo", 4096, 4096);
diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c
index a7e0189..c94f451 100644
--- a/tests/gem_render_linear_blits.c
+++ b/tests/gem_render_linear_blits.c
@@ -81,8 +81,11 @@ int main(int argc, char **argv)
batch = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
count = 0;
+ if (drmtest_run_in_simulation())
+ count = 2;
if (argc > 1)
count = atoi(argv[1]);
+
if (count == 0)
count = 3 * gem_aperture_size(fd) / SIZE / 2;
else if (count < 2) {
@@ -127,6 +130,9 @@ int main(int argc, char **argv)
for (i = 0; i < count; i++)
check_bo(fd, bo[i]->handle, start_val[i]);
+ if (drmtest_run_in_simulation())
+ return 0;
+
printf("Cyclic blits, backward...\n");
for (i = 0; i < count * 4; i++) {
struct scratch_buf src, dst;
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
index af40590..b111275 100644
--- a/tests/gem_ring_sync_loop.c
+++ b/tests/gem_ring_sync_loop.c
@@ -63,7 +63,7 @@ store_dword_loop(int fd)
srandom(0xdeadbeef);
- for (i = 0; i
[Intel-gfx] [PATCH 7/8] tests: Instrument gem_seqno_wrap to run in simulation
Signed-off-by: Damien Lespiau --- tests/gem_seqno_wrap.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c index f354a52..8d94858 100644 --- a/tests/gem_seqno_wrap.c +++ b/tests/gem_seqno_wrap.c @@ -577,7 +577,7 @@ static void parse_options(int argc, char **argv) }; strcpy(options.cmd, ""); - options.rounds = 50; + options.rounds = SLOW_QUICK(50, 2); options.background = 0; options.dontwrap = 0; options.timeout = 20; @@ -647,8 +647,6 @@ int main(int argc, char **argv) int wcount = 0; int r = -1; - drmtest_skip_on_simulation(); - parse_options(argc, argv); card_index = drm_get_card(0); -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 8/8] tests: Add some tiled tests to the runs on simulation
Signed-off-by: Damien Lespiau
---
tests/gem_tiled_pread.c| 2 --
tests/gem_tiled_pread_pwrite.c | 4 +---
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/tests/gem_tiled_pread.c b/tests/gem_tiled_pread.c
index 779f66f..189affc 100644
--- a/tests/gem_tiled_pread.c
+++ b/tests/gem_tiled_pread.c
@@ -134,8 +134,6 @@ main(int argc, char **argv)
uint32_t handle;
uint32_t devid;
- drmtest_skip_on_simulation();
-
fd = drm_open_any();
handle = create_bo(fd);
diff --git a/tests/gem_tiled_pread_pwrite.c b/tests/gem_tiled_pread_pwrite.c
index 0eb7098..18a593a 100644
--- a/tests/gem_tiled_pread_pwrite.c
+++ b/tests/gem_tiled_pread_pwrite.c
@@ -123,10 +123,8 @@ main(int argc, char **argv)
uint32_t handle, handle_target;
int count;
- drmtest_skip_on_simulation();
-
fd = drm_open_any();
- count = intel_get_total_ram_mb() * 9 / 10;
+ count = SLOW_QUICK(intel_get_total_ram_mb() * 9 / 10, 8) ;
for (i = 0; i < count/2; i++) {
current_tiling_mode = I915_TILING_X;
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/8] lib: Remove old dead code intel_batchbuffer_emit_mi_flush()
Signed-off-by: Damien Lespiau
---
lib/intel_batchbuffer.c | 7 ---
lib/intel_batchbuffer.h | 3 ---
2 files changed, 10 deletions(-)
diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 724e23d..c6c8153 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -232,10 +232,3 @@ intel_copy_bo(struct intel_batchbuffer *batch,
intel_batchbuffer_flush(batch);
}
-
-void
-intel_batchbuffer_emit_mi_flush(struct intel_batchbuffer *batch)
-{
- intel_batchbuffer_require_space(batch, 4);
- intel_batchbuffer_emit_dword(batch, MI_FLUSH);
-}
diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
index 9f45ac6..67617ce 100644
--- a/lib/intel_batchbuffer.h
+++ b/lib/intel_batchbuffer.h
@@ -96,9 +96,6 @@ intel_batchbuffer_require_space(struct intel_batchbuffer
*batch,
#define ADVANCE_BATCH() do { \
} while(0)
-void
-intel_batchbuffer_emit_mi_flush(struct intel_batchbuffer *batch);
-
void intel_copy_bo(struct intel_batchbuffer *batch,
drm_intel_bo *dst_bo, drm_intel_bo *src_bo,
int width, int height);
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/8] tests: Instrument gem_lut_handle for simulation
Signed-off-by: Damien Lespiau
---
tests/gem_lut_handle.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/gem_lut_handle.c b/tests/gem_lut_handle.c
index fdd2ab8..f3e5734 100644
--- a/tests/gem_lut_handle.c
+++ b/tests/gem_lut_handle.c
@@ -183,8 +183,6 @@ int main(int argc, char **argv)
uint32_t handle;
int fd, i;
- drmtest_skip_on_simulation();
-
fd = drm_open_any();
handle = gem_create(fd, 4096);
@@ -199,7 +197,7 @@ int main(int argc, char **argv)
do_or_die(exec(fd, handle, USE_LUT));
fail(exec(fd, handle, USE_LUT | BROKEN));
- for (i = 2; i <= 65536; i *= 2) {
+ for (i = 2; i <= SLOW_QUICK(65536, 8); i *= 2) {
if (many_exec(fd, handle, i+1, i+1, NORMAL) == -1 &&
errno == ENOSPC)
break;
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Provide a simulation friendly test environment v4
On Thu, Jul 18, 2013 at 04:19:05PM +0100, Damien Lespiau wrote: > It was high time to follow up on: > http://lists.freedesktop.org/archives/intel-gfx/2013-April/027361.html > > Changes from v2: > • Take into account Daniel's comments and add the tests he listed > • Actually runs on simulation now lgtm overall, please push. One thing we might want to do is add a generic drmtest_dummy_blt_load function which auto-tunes for simulation runs. Ever time I need to reliably hit a seqno wait in a test somewhere I just copy-paste that little bit of lore so we'll have a constant game of whack-a-mole for those. With a helper it would Just Work. Most other tests I've recently added are fairly small, so I guess we should be fine by just adding them to the simulation enviroment test cases. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file
On Thu, Jul 18, 2013 at 01:29:58PM +0100, Chris Wilson wrote: > On Thu, Jul 18, 2013 at 02:15:59PM +0200, Daniel Vetter wrote: > > On Tue, Jul 16, 2013 at 08:02:11PM +0100, Chris Wilson wrote: > > > Currently, the register access code is split between i915_drv.c and > > > intel_pm.c. It only bares a superficial resemblance to the reset of the > > > powermanagement code, so move it all into its own file. This is to ease > > > further patches to enforce serialised register access. > > > > > > v2: Scan for random abuse of I915_WRITE_NOTRACE > > > v3: Take the opportunity to rename the GT functions as uncore. Uncore is > > > the term used by the hardware design (and bspec) for all functions > > > outside of the GPU (and CPU) cores in what is also known as the System > > > Agent. > > > > > > Signed-off-by: Chris Wilson > > > Reviewed-by: Ben Widawsky > > > > I've tried to apply it, but this patch does way to many things at once. So > > the oddball change we have compared to the baseline of these patches > > resulted in conflict hell. > > The conflict is trivial. Yeah it's just a few lines that I'd need to copy around, but git am refused to apply the patch due to lack of a baseline and wiggle made one giant mess out of it. So I've given up, especially since I've inked in a tedious rebase tour for -internal today. At least that one worked better than planned ;-) My plan was kinda to apply the first 4 patches to -fixes since they're the more correct solution and we're fairly early, and only backport the minimal change. But if you think it's better to put the entire series into dinq and only the minimal fix into -fixes with cc: stable I can do that, too. Wrt the minimal fix I haven't found it on the mailing list, pointers for the blind? Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/11] drm/i915: Hook PSR functionality
On Thu, Jul 18, 2013 at 6:54 AM, Daniel Vetter wrote: > On Thu, Jul 11, 2013 at 06:45:05PM -0300, Rodrigo Vivi wrote: >> PSR must be enabled after transcoder and port are running. >> And it is only available for HSW. >> >> v2: move enable/disable to intel_ddi >> v3: The spec suggests PSR should be disabled even before backlight (by >> pzanoni) >> v4: also disabling and enabling whenever panel is disabled/enabled. >> v5: make it last patch to avoid breaking whenever bisecting. So calling for >> update and force exit came to this patch along with enable/disable calls. >> v6: Remove unused and unecessary psr_enable/disable calls, as notice by >> Paulo. >> >> CC: Paulo Zanoni >> Signed-off-by: Rodrigo Vivi > > Ok, I've slurped this series in with the few bikesheds from Paulo applied > and two patches not merged: > - The userspace interface part, since I don't want to commit yet to an > interface as long as things are unclear. > - And the invalidation part since imo that parts isn't properly thought > through yet. See my other mail to Chris' RFC for ideas. Thank you very much. > > Now on the topic fbc, psr and frontbuffer rendering: > > Now that I've appeased our dear PDT it's time to stop building castles on > sand imo. What about psr at Baytrail? :/ > Items to pay back a bit of the technical dept: > > - Untangle the "is fbc/psr" allowed mess. Imo we should add more static > (i.e. only changes at modesets) information to the pipe config and track > dynamic reasons for disabling fbc/psr somewhere in the crtc. Also this > way update_fbc/psr would only need to check dynamic state and more > important would never need to frob the basic setup (like reallocating > the compressed buffer and similar things). > > - Implement precise frontbuffer rendering tracking and disabling of > fbc/psr for legacy userspace and rip out the hw tracking. If we have to > add tons of workarounds (like for fbc), have performance this or just > can't use it in a bunch of cases (sprites for psr) we might as well just > track everything in the kernel. > > - Testcases. With pipe CRC checksums we should be able to make sure that > the frontbuffer invalidation actually happens. And if we do it all in > the kernel the difference should be all-or-nothing, so much easier to > test than with hw tracking where sometimes something seems to slip > through. > > - low-level improvements. I've only really reviewed the fbc code in-depth > and discussed a few things with Art, but there's definitely a few things > we need to do. One of the important things imo is to enable fbc > everywhere we can to have as wide test coverage as possible for this > feature. > > I'll block future hw enabling in this area on the above list (i.e. vlv psr > or patches for -internal). I'll reconsider my stance if e.g. vlv psr is > used as a demonstration vehicle for the refactored psr tracking. But since > fbc can be used on many more machines (even desktops and apparently even > when other pipes are enabled) I think we should push that forward first > and use fbc to create solid tests and interfaces for userspace&kernel to > cooperate on frontbuffer rendering. I fully agree with you. I'm going to start playing with fbc and psr at pipe_config, etc and check that discussion with art and try other changes in fbc. I'm just afraid the pressure for psr-vlv will be big as well. Anyway, I give up on those 2 patches you didn't accepted because I think this other things have more priority compared to xdm/kde bug. Since it is disabled by default it won't cause troubles for kde users and it is working well on gnome and unity that by any chance has a hsw with edp that supports psr and use i915.enable_psr=1 parameter :/ > > Cheers, Daniel > >> --- >> drivers/gpu/drm/i915/i915_gem.c | 2 ++ >> drivers/gpu/drm/i915/intel_ddi.c | 2 ++ >> drivers/gpu/drm/i915/intel_display.c | 1 + >> 3 files changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index 46bf7e3..703bc69 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -3758,6 +3758,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, >> goto unlock; >> } >> >> + intel_edp_psr_force_exit(dev); >> + >> /* Count all active objects as busy, even if they are currently not >> used >>* by the gpu. Users of this interface expect objects to eventually >>* become non-busy without any further actions, therefore emit any >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 324211a..4211925 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1117,6 +1117,7 @@ static void intel_enable_ddi(struct intel_encoder >> *intel_encoder) >> intel_dp_stop_link_train(intel_dp); >> >> ironlake_edp_backlight_on(intel_dp); >>
Re: [Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
On Thu, 18 Jul 2013 16:19:07 +0100 Damien Lespiau wrote: > This will allow us to explicitely blacklist tests we don't want to run > on simulation. So FWIW I'll reiterate that I'd prefer to manage this in either the Makefile target for the tests (e.g. have a single_kernel_sim_tests or somesuch), or separate scripts altogether for running the different types of tests, with specific subsets for running on simulation. Having to sprinkle skip_on_simulation into the actual tests seems like duplicated effort across every test. For the cases where the args or iterations differ, it might be better to simply take an argv for the values, and have the sim vs. full scripts pass different values. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add functions to force psr exit
On Thu, Jul 18, 2013 at 5:33 AM, Daniel Vetter wrote:
> On Mon, Jul 15, 2013 at 05:29:15PM -0300, Rodrigo Vivi wrote:
>> PSR tracking engine in HSW doesn't detect automagically some directly copy
>> area
>> operations through scanout so we will have to kick it manually and
>> reschedule it to come back to normal operation as soon as possible.
>>
>> v2: Before PSR Hook. Don't force it when busy yet.
>> v3/v4: Solved small conflict.
>> v5: setup once function was already added on previous commit.
>> v6: Use POSTING_READ to make sure the mutex works as a write barrier as
>> suggested by Chris Wilson
>>
>> CC: Chris Wilson
>> Reviewed-by: Shobhit Kumar
>> Signed-off-by: Rodrigo Vivi
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>> drivers/gpu/drm/i915/intel_dp.c | 48
>>
>> drivers/gpu/drm/i915/intel_drv.h | 3 +++
>> 3 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 3bca337..dc10345 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1840,6 +1840,7 @@
>> #define EDP_PSR_PERF_CNT_MASK 0xff
>>
>> #define EDP_PSR_DEBUG_CTL0x64860
>> +#define EDP_PSR_DEBUG_FORCE_EXIT (3<<30)
>> #define EDP_PSR_DEBUG_MASK_LPSP(1<<27)
>> #define EDP_PSR_DEBUG_MASK_MEMUP (1<<26)
>> #define EDP_PSR_DEBUG_MASK_HPD (1<<25)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index 3c9473c..47e1676 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1393,6 +1393,50 @@ bool intel_edp_is_psr_enabled(struct drm_device *dev)
>> return I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>> }
>>
>> +static void intel_edp_psr_delayed_normal_work(struct work_struct *__work)
>> +{
>> + struct intel_dp *intel_dp = container_of(to_delayed_work(__work),
>> + struct intel_dp,
>> + edp_psr_delayed_normal_work);
>> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + mutex_lock(&intel_dp->psr_exit_mutex);
>> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) &
>> +~EDP_PSR_DEBUG_FORCE_EXIT);
>> + POSTING_READ(EDP_PSR_DEBUG_CTL);
>> + mutex_unlock(&intel_dp->psr_exit_mutex);
>> +}
>> +
>> +void intel_edp_psr_force_exit(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct intel_encoder *encoder;
>> + struct intel_dp *intel_dp = NULL;
>> +
>> + if (!intel_edp_is_psr_enabled(dev))
>> + return;
>> +
>> + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>> + if (encoder->type == INTEL_OUTPUT_EDP)
>> + intel_dp = enc_to_intel_dp(&encoder->base);
>> +
>> + if (!intel_dp)
>> + return;
>> +
>> + if (WARN_ON(!intel_dp->psr_setup_done))
>> + return;
>
> If you have to dig out your data like this it usually means that it's at
> the wrong spot. Imo we should track a psr_possible bit in the pipe_config
> and a psr_enabled (and the locking for the runtime mutex) in the crtc
> itself. Also, the psr_setup_done thing is another hint that we should move
> this into the crtc.
Ok, I'm going to play a bit with this and see what I can come up with.
>
> Second I prefer if functions with tricky tie-in with existing code are
> used in the same patch they're created - if it's split over two patches
> review is a bit a pain.
Agreed. I just splited out because I accepted review suggestions to
let all hooks for the last patch to avoid breaking bisects or
something like that.
> But if I read the follow-up patch correctly we
> call this function from the busy_ioctl unconditionally, which will
> obviously result in tons of falls postives - e.g. libdrm uses this ioctl
> to manage it's buffer cache.
Yes, you are right. Although I noticed that psr was still working
fine, I agree that this is too many unecessary calls :(
But couldn't find a better way to fix xdm/kde issue.
Other ideas was letting psr disabled for so long when it could be
there saving power.
But as I said, low priority on this right now... Maybe when following
your suggestions and the list you made we get it fixed properly ;)
> I think I'll punt on this patch and merge just the parts from the next one
> for normal backbuffer rendering with pageflips model.
> -Daniel
Thanks again,
Rodrigo
>
>> +
>> + mutex_lock(&intel_dp->psr_exit_mutex);
>> + I915_WRITE(EDP_PSR_DEBUG_CTL, I915_READ(EDP_PSR_DEBUG_CTL) |
>> +EDP_PSR_DEBUG_FORCE_EXIT);
>> + POSTING_READ(EDP_PSR_DEBUG_CTL);
>> + mutex_unlock(&intel_dp->psr_exit_mutex);
>> +
>> + schedule_delayed_work(&intel_dp->edp_psr_delayed_normal_work,
>> +
Re: [Intel-gfx] [PATCH 09/11] drm/i915: Adding global I915_PARAM for PSR ENABLED.
On Thu, Jul 18, 2013 at 5:24 AM, Daniel Vetter wrote: > On Wed, Jul 17, 2013 at 10:08:34PM +0100, Chris Wilson wrote: >> On Wed, Jul 17, 2013 at 06:01:03PM -0300, Rodrigo Vivi wrote: >> > On Wed, Jul 17, 2013 at 5:18 PM, Chris Wilson >> > wrote: >> > > On Wed, Jul 17, 2013 at 02:46:52PM -0300, Rodrigo Vivi wrote: >> > >> Hi Chris, >> > >> >> > >> could you please review this specific one or give you ack here? >> > > >> > > Didn't see anything wrong with it. The only caveat I have is that the >> > > GETPARAM must be accurate immediately following a setcrtc. >> > >> > To be truly honest I have no idea, mainly when we alternate with fbcon >> > updating psr state at set_base. >> > Could you please also review subsequent patches in this series... 10 and >> > 11. >> > I think 11 answer this question... >> >> If it is not clear by this point, and the changelog doesn't make it >> clear, then something is missing from this patch. Hint ;-) > > I'll punt on userspace interface changes, at least until we've figured out > a clear picture how to do this. agreed > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
On Thu, Jul 18, 2013 at 09:18:34AM -0700, Jesse Barnes wrote: > On Thu, 18 Jul 2013 16:19:07 +0100 > Damien Lespiau wrote: > > > This will allow us to explicitely blacklist tests we don't want to run > > on simulation. I agree with Jesse on this. Mostly what I've wanted is an "opt-in" approach as opposed to an "opt-out" one. Such a thing is better controlled via a Makefile target, or separate script. Generally I loathe the idea of a separate script, but in a case like simulation where I'm thinking we may want to impose higher level timeouts via kill... maybe it's not so bad. > > So FWIW I'll reiterate that I'd prefer to manage this in either the > Makefile target for the tests (e.g. have a single_kernel_sim_tests or > somesuch), or separate scripts altogether for running the different > types of tests, with specific subsets for running on simulation. > > Having to sprinkle skip_on_simulation into the actual tests seems like > duplicated effort across every test. More importantly, it's too easy to forget to add, or not know when it's relevant to add. "Doesn't scale" as Daniel likes to say. > > For the cases where the args or iterations differ, it might be better > to simply take an argv for the values, and have the sim vs. full > scripts pass different values. Time to develop our own programming language with for_each semantics :D I'm okay with what we ended up on the iterators. It's somewhat ugly, but I couldn't come up with anything less ugly, and the argv thing I think just won't work well for some of the weirder tests. > > -- > Jesse Barnes, Intel Open Source Technology Center > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.
On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter wrote:
> On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote:
>> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
>> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
>> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
>> > Paulo Zanoni.
>> > v4: Avoiding reading non-existent registers - noticed by Paulo
>> > on first psr debugfs patch.
>> > v5: Accepting more suggestions from Paulo:
>> > * check sw interlace flag instead of i915_read
>> > * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
>> >
>> > Cc: Paulo Zanoni
>> > Signed-off-by: Rodrigo Vivi
>> > ---
>> > drivers/gpu/drm/i915/i915_debugfs.c | 44 ++
>> > drivers/gpu/drm/i915/i915_drv.h | 13 +++
>> > drivers/gpu/drm/i915/i915_reg.h | 7
>> > drivers/gpu/drm/i915/intel_dp.c | 74
>> > -
>> > 4 files changed, 130 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> > b/drivers/gpu/drm/i915/i915_debugfs.c
>> > index fe3cd5a..e679968 100644
>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file *m,
>> > void *data)
>> > struct drm_info_node *node = m->private;
>> > struct drm_device *dev = node->minor->dev;
>> > struct drm_i915_private *dev_priv = dev->dev_private;
>> > - u32 psrctl, psrstat, psrperf;
>> > + u32 psrstat, psrperf;
>> >
>> > - if (!IS_HASWELL(dev)) {
>> > - seq_puts(m, "PSR not supported on this platform\n");
>> > + if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
>> > + seq_puts(m, "PSR enabled\n");
>> > + } else {
>> > + seq_puts(m, "PSR disabled: ");
>> > + switch (dev_priv->no_psr_reason) {
>> > + case PSR_NO_SOURCE:
>>
>> I am not a fan of using a continually expanding set of enums for
>> no_psr_reason (and no_fbc_reason). If we just stored a const char
>> *no_psr_reason, it would make like much easier for us (fewer lines and a
>> smaller object).
>>
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > b/drivers/gpu/drm/i915/intel_dp.c
>> > index d4b52a9..c0bd887 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct
>> > intel_dp *intel_dp)
>> >EDP_PSR_ENABLE);
>> > }
>> >
>> > +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>> > +{
>> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> > + struct drm_device *dev = dig_port->base.base.dev;
>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>> > + struct drm_crtc *crtc = dig_port->base.base.crtc;
>> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> > + struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
>> > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> > +
>> > + if (!IS_HASWELL(dev)) {
>> > + DRM_DEBUG_KMS("PSR not supported on this platform\n");
>> > + dev_priv->no_psr_reason = PSR_NO_SOURCE;
>> > + return false;
>> > + }
>> > +
>> > + if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
>> > + (dig_port->port != PORT_A)) {
>> > + DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
>> > + dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
>> > + return false;
>> > + }
>> > +
>> > + if (!is_edp_psr(intel_dp)) {
>> > + DRM_DEBUG_KMS("PSR not supported by this panel\n");
>> > + dev_priv->no_psr_reason = PSR_NO_SINK;
>> > + return false;
>> > + }
>> > +
>> > + if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
>> > + DRM_DEBUG_KMS("crtc not active for PSR\n");
>> > + dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
>> > + return false;
>> > + }
>> > +
>> > + if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
>> > + (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
>>
>> Any time we resort to reading back register state is a failure in our
>> state tracking (and pipe_config).
>
> Highly agreed, especially if it's a resource which is out of our control
> like the KVMR bits. Since that's just plain broken I've removed it from
> the patch.
Since Audio guys will enable power well and let it enabled all the
time, I'm wondering in mask LPSP at PSR_DEBUG_CTL.
So PSR can work even with power well enabled.
What do you think?
> -Daniel
>
>> -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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corpor
Re: [Intel-gfx] [PATCH 06/11] drm/i915: Match all PSR mode entry conditions before enabling it.
On Thu, Jul 18, 2013 at 6:36 PM, Rodrigo Vivi wrote:
> On Thu, Jul 18, 2013 at 5:02 AM, Daniel Vetter wrote:
>> On Mon, Jul 15, 2013 at 03:06:22PM +0100, Chris Wilson wrote:
>>> On Thu, Jul 11, 2013 at 06:45:00PM -0300, Rodrigo Vivi wrote:
>>> > v2: Prefer seq_puts to seq_printf by Paulo Zanoni.
>>> > v3: small changes like avoiding calling dp_to_dig_port twice as noticed by
>>> > Paulo Zanoni.
>>> > v4: Avoiding reading non-existent registers - noticed by Paulo
>>> > on first psr debugfs patch.
>>> > v5: Accepting more suggestions from Paulo:
>>> > * check sw interlace flag instead of i915_read
>>> > * introduce PSR_S3D_ENABLED to avoid forgeting it whenever added.
>>> >
>>> > Cc: Paulo Zanoni
>>> > Signed-off-by: Rodrigo Vivi
>>> > ---
>>> > drivers/gpu/drm/i915/i915_debugfs.c | 44 ++
>>> > drivers/gpu/drm/i915/i915_drv.h | 13 +++
>>> > drivers/gpu/drm/i915/i915_reg.h | 7
>>> > drivers/gpu/drm/i915/intel_dp.c | 74
>>> > -
>>> > 4 files changed, 130 insertions(+), 8 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> > b/drivers/gpu/drm/i915/i915_debugfs.c
>>> > index fe3cd5a..e679968 100644
>>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> > @@ -1948,17 +1948,47 @@ static int i915_edp_psr_status(struct seq_file
>>> > *m, void *data)
>>> > struct drm_info_node *node = m->private;
>>> > struct drm_device *dev = node->minor->dev;
>>> > struct drm_i915_private *dev_priv = dev->dev_private;
>>> > - u32 psrctl, psrstat, psrperf;
>>> > + u32 psrstat, psrperf;
>>> >
>>> > - if (!IS_HASWELL(dev)) {
>>> > - seq_puts(m, "PSR not supported on this platform\n");
>>> > + if (IS_HASWELL(dev) && I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE) {
>>> > + seq_puts(m, "PSR enabled\n");
>>> > + } else {
>>> > + seq_puts(m, "PSR disabled: ");
>>> > + switch (dev_priv->no_psr_reason) {
>>> > + case PSR_NO_SOURCE:
>>>
>>> I am not a fan of using a continually expanding set of enums for
>>> no_psr_reason (and no_fbc_reason). If we just stored a const char
>>> *no_psr_reason, it would make like much easier for us (fewer lines and a
>>> smaller object).
>>>
>>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> > b/drivers/gpu/drm/i915/intel_dp.c
>>> > index d4b52a9..c0bd887 100644
>>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> > @@ -1497,11 +1497,83 @@ static void intel_edp_psr_enable_source(struct
>>> > intel_dp *intel_dp)
>>> >EDP_PSR_ENABLE);
>>> > }
>>> >
>>> > +static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>>> > +{
>>> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>> > + struct drm_device *dev = dig_port->base.base.dev;
>>> > + struct drm_i915_private *dev_priv = dev->dev_private;
>>> > + struct drm_crtc *crtc = dig_port->base.base.crtc;
>>> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>> > + struct drm_i915_gem_object *obj = to_intel_framebuffer(crtc->fb)->obj;
>>> > + struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>> > +
>>> > + if (!IS_HASWELL(dev)) {
>>> > + DRM_DEBUG_KMS("PSR not supported on this platform\n");
>>> > + dev_priv->no_psr_reason = PSR_NO_SOURCE;
>>> > + return false;
>>> > + }
>>> > +
>>> > + if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
>>> > + (dig_port->port != PORT_A)) {
>>> > + DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
>>> > + dev_priv->no_psr_reason = PSR_HSW_NOT_DDIA;
>>> > + return false;
>>> > + }
>>> > +
>>> > + if (!is_edp_psr(intel_dp)) {
>>> > + DRM_DEBUG_KMS("PSR not supported by this panel\n");
>>> > + dev_priv->no_psr_reason = PSR_NO_SINK;
>>> > + return false;
>>> > + }
>>> > +
>>> > + if (!intel_crtc->active || !crtc->fb || !crtc->mode.clock) {
>>> > + DRM_DEBUG_KMS("crtc not active for PSR\n");
>>> > + dev_priv->no_psr_reason = PSR_CRTC_NOT_ACTIVE;
>>> > + return false;
>>> > + }
>>> > +
>>> > + if ((I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE) ||
>>> > + (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE)) {
>>>
>>> Any time we resort to reading back register state is a failure in our
>>> state tracking (and pipe_config).
>>
>> Highly agreed, especially if it's a resource which is out of our control
>> like the KVMR bits. Since that's just plain broken I've removed it from
>> the patch.
>
> Since Audio guys will enable power well and let it enabled all the
> time, I'm wondering in mask LPSP at PSR_DEBUG_CTL.
> So PSR can work even with power well enabled.
> What do you think?
I have honestly no idea ... have you poked Art about this?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 -
Re: [Intel-gfx] [PATCH] drm/i915: Extend i915_powersave parameter.
On Wed, Jul 17, 2013 at 6:30 PM, Daniel Vetter wrote: > On Wed, Jul 17, 2013 at 11:23 PM, Rodrigo Vivi wrote: My opinion is that we respect the specific module parameters, and if they are left to default values, then apply the global powersave parameter. If that too is default, then we apply the module default. >>> >>> Jumping in a bit late, but: I've honestly never understood why we have >>> two levels of module options. Imo having individual knobs for each >>> delicate feature makes more sense, strange dependencies in module >>> option will only confuse dim-witted developers like me when looking at >>> a bug ;-) >>> >>> So could we just reduce powersave to the few things that we haven't >>> touched yet (iirc only DRRS)? >> >> That is fine for me too... either add all features under this umbrella >> or make it be only one feature like drrs that doesn't have its own >> parameter... >> the only cons I see in this case is the name of parameter that is too >> generic... > > We're allowed to kill module options now, so if we fix up drrs and > make powersave completely useless, we can just remove. I'm in favor of remove powersave parameter completely. Just let me know what are the issues with drrs and how I can help to fix them and remove this parameter completely. > >> But honestly I don't have a stronger position I just wanted to start >> the discussion because I don't like the way it is today... So it is up >> to you... I can either send v2 or a new simple patch that removes fbc >> from this i915_powersave. Just let me know what is better... > > I vote for moving fbc out of powersave as a separate option. Worst > case we need to educate a users and tell him that frobbing around with > random module options isn't a good idea ;-) Agree > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
On Thu, Jul 18, 2013 at 09:18:34AM -0700, Jesse Barnes wrote: > On Thu, 18 Jul 2013 16:19:07 +0100 > Damien Lespiau wrote: > > > This will allow us to explicitely blacklist tests we don't want to run > > on simulation. > > So FWIW I'll reiterate that I'd prefer to manage this in either the > Makefile target for the tests (e.g. have a single_kernel_sim_tests or > somesuch), or separate scripts altogether for running the different > types of tests, with specific subsets for running on simulation. We have lots of tests with subtests, and sometimes we might want to exclude just a few of them, not all. Doing that at the Makefile level is a bit too coarse imo. > Having to sprinkle skip_on_simulation into the actual tests seems like > duplicated effort across every test. > > For the cases where the args or iterations differ, it might be better > to simply take an argv for the values, and have the sim vs. full > scripts pass different values. We have tons of tests meanwhile, if you want to run them with piglit passing individual options with argv for each test doesn't scale better imo than just baking those into the tests. It's gonna suck a bit one way or the other I guess :( -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
On Thu, Jul 18, 2013 at 09:31:15AM -0700, Ben Widawsky wrote:
> On Thu, Jul 18, 2013 at 09:18:34AM -0700, Jesse Barnes wrote:
> > On Thu, 18 Jul 2013 16:19:07 +0100
> > Damien Lespiau wrote:
> >
> > > This will allow us to explicitely blacklist tests we don't want to run
> > > on simulation.
>
> I agree with Jesse on this. Mostly what I've wanted is an "opt-in"
> approach as opposed to an "opt-out" one. Such a thing is better
> controlled via a Makefile target, or separate script.
Well, for this one I'd like a slightly different approach. Actually have
a drmtest structure with metadata about the test. Something like:
#define DECLARE_SUBTEST(n, func, d)\
{ \
.name = n, \
.run = func,\
.data = d, \
}
enum drmtest_category {
TEST_DISPLAY= (1 << 0),
TEST_GT = (1 << 1),
TEST_CONTEXT = (1 << 2),
TEST_GEM= (1 << 3),
TEST_TORTURE= (1 << 31),
};
struct drm_subtest {
const char *name;
int (*run)(struct drm_test *test);
void *data;
};
struct drm_test {
const char *name;
int fd;
enum drmtest_category categories;
int subtests_nr;
struct drm_subtest *subtests;
};
and in the test:
struct drm_subtest subtests[] = {
DECLARE_SUBTEST("bad-close", test_bad_close, &test_data),
DECLARE_SUBTEST("create-close", test_create_close, &test_data),
DECLARE_SUBTEST("create-fd-close", test_create_fd_close, &test_data),
};
struct drm_test test = {
.name = "gem-basic",
.subtests = subtests,
.subtests_nr = ARRAY_SIZE(subtests),
};
(better macros possible)
and:
drmtest_init(&test, argc, argv);
drmtest_run(&test);
That'll allow you to do things like "please run all the context tests" for
instance.
But well, still quite far away, and it maybe quite hard/time consuming to
convert some tests to that.
--
Damien
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
On Thu, 18 Jul 2013 17:55:00 +0100
Damien Lespiau wrote:
> On Thu, Jul 18, 2013 at 09:31:15AM -0700, Ben Widawsky wrote:
> > On Thu, Jul 18, 2013 at 09:18:34AM -0700, Jesse Barnes wrote:
> > > On Thu, 18 Jul 2013 16:19:07 +0100
> > > Damien Lespiau wrote:
> > >
> > > > This will allow us to explicitely blacklist tests we don't want to run
> > > > on simulation.
> >
> > I agree with Jesse on this. Mostly what I've wanted is an "opt-in"
> > approach as opposed to an "opt-out" one. Such a thing is better
> > controlled via a Makefile target, or separate script.
>
> Well, for this one I'd like a slightly different approach. Actually have
> a drmtest structure with metadata about the test. Something like:
>
> #define DECLARE_SUBTEST(n, func, d)\
>{ \
>.name = n, \
>.run = func,\
>.data = d, \
>}
>
> enum drmtest_category {
>TEST_DISPLAY= (1 << 0),
>TEST_GT = (1 << 1),
>TEST_CONTEXT = (1 << 2),
>TEST_GEM= (1 << 3),
>TEST_TORTURE= (1 << 31),
> };
>
>
> struct drm_subtest {
>const char *name;
>int (*run)(struct drm_test *test);
>void *data;
> };
>
> struct drm_test {
>const char *name;
>int fd;
>enum drmtest_category categories;
>int subtests_nr;
>struct drm_subtest *subtests;
> };
>
> and in the test:
>
> struct drm_subtest subtests[] = {
>DECLARE_SUBTEST("bad-close", test_bad_close, &test_data),
>DECLARE_SUBTEST("create-close", test_create_close, &test_data),
>DECLARE_SUBTEST("create-fd-close", test_create_fd_close, &test_data),
> };
>
> struct drm_test test = {
>.name = "gem-basic",
>.subtests = subtests,
>.subtests_nr = ARRAY_SIZE(subtests),
> };
>
> (better macros possible)
>
> and:
>drmtest_init(&test, argc, argv);
>drmtest_run(&test);
>
> That'll allow you to do things like "please run all the context tests" for
> instance.
>
> But well, still quite far away, and it maybe quite hard/time consuming to
> convert some tests to that.
>
Yeah that sounds like the ideal situation, so you can just run a set of
subtests for the stuff you're working on... It'll only get harder to
convert as we add more tests. :)
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file
On Thu, Jul 18, 2013 at 05:53:52PM +0200, Daniel Vetter wrote: > My plan was kinda to apply the first 4 patches to -fixes since they're the > more correct solution and we're fairly early, and only backport the > minimal change. But if you think it's better to put the entire series into > dinq and only the minimal fix into -fixes with cc: stable I can do that, > too. Wrt the minimal fix I haven't found it on the mailing list, pointers > for the blind? It's the first attempt on the bug. It should be possible to juggle this series so that we have the minimal (nearly complete) fix first. -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
[Intel-gfx] [PATCH 2/7] drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk
From: Paulo Zanoni
Because lpt_init_pch_refclk implements the "Sequence to enable
CLKOUT_DP for FDI usage and configure PCH FDI I/O", which is very
similar to "Sequence to enable CLKOUT_DP" and "Sequence to enable
CLKOUT_DP without spread". With the extracted functions we can more
easily implement the two missing sequences.
v2: Rebase (WaMPhyProgramming:hsw comment).
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/intel_display.c | 79
1 file changed, 45 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 2dab208..bc586ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5164,41 +5164,9 @@ static void ironlake_init_pch_refclk(struct drm_device
*dev)
BUG_ON(val != final);
}
-/*
- * Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O.
- * WaMPhyProgramming:hsw
- */
-static void lpt_init_pch_refclk(struct drm_device *dev)
+static void lpt_reset_fdi_mphy(struct drm_i915_private *dev_priv)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_mode_config *mode_config = &dev->mode_config;
- struct intel_encoder *encoder;
- bool has_vga = false;
- u32 tmp;
-
- list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
- switch (encoder->type) {
- case INTEL_OUTPUT_ANALOG:
- has_vga = true;
- break;
- }
- }
-
- if (!has_vga)
- return;
-
- mutex_lock(&dev_priv->dpio_lock);
-
- tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
- tmp &= ~SBI_SSCCTL_DISABLE;
- tmp |= SBI_SSCCTL_PATHALT;
- intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
-
- udelay(24);
-
- tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
- tmp &= ~SBI_SSCCTL_PATHALT;
- intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+ uint32_t tmp;
tmp = I915_READ(SOUTH_CHICKEN2);
tmp |= FDI_MPHY_IOSFSB_RESET_CTL;
@@ -5215,6 +5183,12 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
if (wait_for_atomic_us((I915_READ(SOUTH_CHICKEN2) &
FDI_MPHY_IOSFSB_RESET_STATUS) == 0, 100))
DRM_ERROR("FDI mPHY reset de-assert timeout\n");
+}
+
+/* WaMPhyProgramming:hsw */
+static void lpt_program_fdi_mphy(struct drm_i915_private *dev_priv)
+{
+ uint32_t tmp;
tmp = intel_sbi_read(dev_priv, 0x8008, SBI_MPHY);
tmp &= ~(0xFF << 24);
@@ -5284,6 +5258,43 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
tmp &= ~(0xF << 28);
tmp |= (4 << 28);
intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
+}
+
+/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
+static void lpt_init_pch_refclk(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_mode_config *mode_config = &dev->mode_config;
+ struct intel_encoder *encoder;
+ bool has_vga = false;
+ u32 tmp;
+
+ list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+ switch (encoder->type) {
+ case INTEL_OUTPUT_ANALOG:
+ has_vga = true;
+ break;
+ }
+ }
+
+ if (!has_vga)
+ return;
+
+ mutex_lock(&dev_priv->dpio_lock);
+
+ tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+ tmp &= ~SBI_SSCCTL_DISABLE;
+ tmp |= SBI_SSCCTL_PATHALT;
+ intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+ udelay(24);
+
+ tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
+ tmp &= ~SBI_SSCCTL_PATHALT;
+ intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
+
+ lpt_reset_fdi_mphy(dev_priv);
+ lpt_program_fdi_mphy(dev_priv);
/* ULT uses SBI_GEN0, but ULT doesn't have VGA, so we don't care. */
tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
--
1.8.1.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
From: Paulo Zanoni
For now there are no callers, but these functions are going to be
needed for the code that allows Package C8+. Other future features may
also require this code.
v2: - Rebase.
- Fix D_COMP wait timeout.
Signed-off-by: Paulo Zanoni
---
drivers/gpu/drm/i915/i915_reg.h | 7 +++
drivers/gpu/drm/i915/intel_display.c | 92
drivers/gpu/drm/i915/intel_drv.h | 3 ++
3 files changed, 102 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bfa6f7f..b57d564 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5007,7 +5007,14 @@
#define LCPLL_CLK_FREQ_450(0<<26)
#define LCPLL_CD_CLOCK_DISABLE(1<<25)
#define LCPLL_CD2X_CLOCK_DISABLE (1<<23)
+#define LCPLL_POWER_DOWN_ALLOW(1<<22)
#define LCPLL_CD_SOURCE_FCLK (1<<21)
+#define LCPLL_CD_SOURCE_FCLK_DONE (1<<19)
+
+#define D_COMP (MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
+#define D_COMP_RCOMP_IN_PROGRESS (1<<9)
+#define D_COMP_COMP_FORCE (1<<8)
+#define D_COMP_COMP_DISABLE (1<<0)
/* Pipe WM_LINETIME - watermark line time */
#define PIPE_WM_LINETIME_A 0x45270
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 0741e1f..915da2d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5924,6 +5924,98 @@ static bool ironlake_get_pipe_config(struct intel_crtc
*crtc,
return true;
}
+/*
+ * This function implements pieces of two sequences from BSpec:
+ * - Sequence for display software to disable LCPLL
+ * - Sequence for display software to allow package C8+
+ * The steps implemented here are just the steps that actually touch the LCPLL
+ * register. Callers should take care of disabling all the display engine
+ * functions, doing the mode unset, fixing interrupts, etc.
+ */
+void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
+ bool switch_to_fclk, bool allow_power_down)
+{
+ uint32_t val;
+
+ val = I915_READ(LCPLL_CTL);
+
+ if (switch_to_fclk) {
+ val |= LCPLL_CD_SOURCE_FCLK;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ udelay(1);
+
+ val = I915_READ(LCPLL_CTL);
+ if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
+ DRM_ERROR("Switching to FCLK failed\n");
+ }
+
+ val |= LCPLL_PLL_DISABLE;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
+ DRM_ERROR("LCPLL still locked\n");
+
+ val = I915_READ(D_COMP);
+ val |= D_COMP_COMP_DISABLE;
+ I915_WRITE(D_COMP, val);
+ POSTING_READ(D_COMP);
+
+ if (wait_for((I915_READ(D_COMP) & D_COMP_RCOMP_IN_PROGRESS) == 0, 1))
+ DRM_ERROR("D_COMP RCOMP still in progress\n");
+
+ if (allow_power_down) {
+ val = I915_READ(LCPLL_CTL);
+ val |= LCPLL_POWER_DOWN_ALLOW;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+ }
+}
+
+/*
+ * Fully restores LCPLL, disallowing power down and switching back to LCPLL
+ * source.
+ */
+void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
+{
+ uint32_t val;
+
+ val = I915_READ(LCPLL_CTL);
+
+ if (val & LCPLL_POWER_DOWN_ALLOW) {
+ val &= ~LCPLL_POWER_DOWN_ALLOW;
+ I915_WRITE(LCPLL_CTL, val);
+ }
+
+ val = I915_READ(D_COMP);
+ val |= D_COMP_COMP_FORCE;
+ val &= ~D_COMP_COMP_DISABLE;
+ I915_WRITE(D_COMP, val);
+
+ val = I915_READ(LCPLL_CTL);
+ val &= ~LCPLL_PLL_DISABLE;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
+ DRM_ERROR("LCPLL not locked yet\n");
+
+ if (val & LCPLL_CD_SOURCE_FCLK) {
+ val = I915_READ(LCPLL_CTL);
+ val &= ~LCPLL_CD_SOURCE_FCLK;
+ I915_WRITE(LCPLL_CTL, val);
+ POSTING_READ(LCPLL_CTL);
+
+ udelay(1);
+
+ val = I915_READ(LCPLL_CTL);
+ if (val & LCPLL_CD_SOURCE_FCLK_DONE)
+ DRM_ERROR("Switching back to LCPLL failed\n");
+ }
+}
+
static void haswell_modeset_global_resources(struct drm_device *dev)
{
bool enable = false;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 31087ff..3fbe80b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -838,5 +838,8 @@ extern bool intel_set_pch_fifo_underrun_reporting(struct
drm_device *dev,
extern void intel_edp_psr_enable(struct intel_dp *intel_dp);
extern void intel_edp_psr_disable(struct intel
Re: [Intel-gfx] [PATCH 2/8] lib: Introduce drmtest_skip_on_simulation()
On Thu, Jul 18, 2013 at 05:55:00PM +0100, Damien Lespiau wrote:
> On Thu, Jul 18, 2013 at 09:31:15AM -0700, Ben Widawsky wrote:
> > On Thu, Jul 18, 2013 at 09:18:34AM -0700, Jesse Barnes wrote:
> > > On Thu, 18 Jul 2013 16:19:07 +0100
> > > Damien Lespiau wrote:
> > >
> > > > This will allow us to explicitely blacklist tests we don't want to run
> > > > on simulation.
> >
> > I agree with Jesse on this. Mostly what I've wanted is an "opt-in"
> > approach as opposed to an "opt-out" one. Such a thing is better
> > controlled via a Makefile target, or separate script.
>
> Well, for this one I'd like a slightly different approach. Actually have
> a drmtest structure with metadata about the test. Something like:
>
> #define DECLARE_SUBTEST(n, func, d)\
>{ \
>.name = n, \
>.run = func,\
>.data = d, \
>}
>
> enum drmtest_category {
>TEST_DISPLAY= (1 << 0),
>TEST_GT = (1 << 1),
>TEST_CONTEXT = (1 << 2),
>TEST_GEM= (1 << 3),
>TEST_TORTURE= (1 << 31),
> };
>
>
> struct drm_subtest {
>const char *name;
>int (*run)(struct drm_test *test);
>void *data;
> };
>
> struct drm_test {
>const char *name;
>int fd;
>enum drmtest_category categories;
>int subtests_nr;
>struct drm_subtest *subtests;
> };
>
> and in the test:
>
> struct drm_subtest subtests[] = {
>DECLARE_SUBTEST("bad-close", test_bad_close, &test_data),
>DECLARE_SUBTEST("create-close", test_create_close, &test_data),
>DECLARE_SUBTEST("create-fd-close", test_create_fd_close, &test_data),
> };
>
> struct drm_test test = {
>.name = "gem-basic",
>.subtests = subtests,
>.subtests_nr = ARRAY_SIZE(subtests),
> };
>
> (better macros possible)
>
> and:
>drmtest_init(&test, argc, argv);
>drmtest_run(&test);
>
> That'll allow you to do things like "please run all the context tests" for
> instance.
>
> But well, still quite far away, and it maybe quite hard/time consuming to
> convert some tests to that.
More metaprogramming for subtest would certainly be nice, currently we use
a lot of add-hoc macro infrastructure. For a group of tests that just
share a bunch of helper functions but not the overall logic like kms_flip
approach used in prime quite a bit. Maybe we need a few different variants
to declare subtests ...
Not sure how useful the grouping of tests is, since the big usecase for
igt is to check for unrelated breakage imo. In any way we have lots of
room for improvement ;-) I've added a few points to our internal wiki for
infrastructure ideas around igt.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: Colocate all GT access routines in the same file
On Thu, Jul 18, 2013 at 7:44 PM, Chris Wilson wrote: > On Thu, Jul 18, 2013 at 05:53:52PM +0200, Daniel Vetter wrote: >> My plan was kinda to apply the first 4 patches to -fixes since they're the >> more correct solution and we're fairly early, and only backport the >> minimal change. But if you think it's better to put the entire series into >> dinq and only the minimal fix into -fixes with cc: stable I can do that, >> too. Wrt the minimal fix I haven't found it on the mailing list, pointers >> for the blind? > > It's the first attempt on the bug. It should be possible to juggle this > series so that we have the minimal (nearly complete) fix first. Ok, I'll hold off for the reordered series then. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7] drm/i915: extend lpt_enable_clkout_dp
On Fri, Jul 12, 2013 at 02:19:39PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni
>
> Now it implements 3 different sequences from BSpec and also has
> support for ULT.
>
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_display.c | 41
> +---
> 2 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc3d6a7..be6164f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4863,6 +4863,8 @@
> #define SBI_SSCAUXDIV_FINALDIV2SEL(x) ((x)<<4)
> #define SBI_DBUFF0 0x2a00
> #define SBI_DBUFF0_ENABLE (1<<0)
> +#define SBI_GEN00x1f00
> +#define SBI_GEN0_ENABLE(1<<0)
bikeshed: I wouldn't haved bothered defining SBI_GEN0_ENABLE
>
> /* LPT PIXCLK_GATE */
> #define PIXCLK_GATE 0xC6020
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f4c5263..5f3b636 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5258,12 +5258,20 @@ static void lpt_program_fdi_mphy(struct
> drm_i915_private *dev_priv)
> intel_sbi_write(dev_priv, 0x21EC, tmp, SBI_MPHY);
> }
>
> -/* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
> -static void lpt_enable_clkout_dp(struct drm_device *dev)
> +/* Implements 3 different sequences from BSpec chapter "Display iCLK
> + * Programming" based on the parameters passed:
> + * - Sequence to enable CLKOUT_DP
> + * - Sequence to enable CLKOUT_DP without spread
> + * - Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O
> + */
> +static void lpt_enable_clkout_dp(struct drm_device *dev, bool with_spread,
> + bool with_fdi)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> uint32_t tmp;
>
> + WARN(with_fdi && !with_spread, "FDI requires downspread\n");
> +
WARN_ON(with_fdi && IS_ULT(dev))?
Should we proceed after the WARN, or break out early?
> mutex_lock(&dev_priv->dpio_lock);
>
> tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
> @@ -5273,17 +5281,26 @@ static void lpt_enable_clkout_dp(struct drm_device
> *dev)
>
> udelay(24);
>
> - tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
> - tmp &= ~SBI_SSCCTL_PATHALT;
> - intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
> + if (with_spread) {
> + tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
> + tmp &= ~SBI_SSCCTL_PATHALT;
> + intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
>
> - lpt_reset_fdi_mphy(dev_priv);
> - lpt_program_fdi_mphy(dev_priv);
> + if (with_fdi) {
> + lpt_reset_fdi_mphy(dev_priv);
> + lpt_program_fdi_mphy(dev_priv);
> + }
> + }
>
> - /* ULT uses SBI_GEN0, but ULT doesn't have VGA, so we don't care. */
> - tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
> - tmp |= SBI_DBUFF0_ENABLE;
> - intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
> + if (IS_ULT(dev)) {
> + tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
> + tmp |= SBI_GEN0_ENABLE;
> + intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
> + } else {
> + tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
> + tmp |= SBI_DBUFF0_ENABLE;
> + intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
> + }
>
> mutex_unlock(&dev_priv->dpio_lock);
> }
> @@ -5305,7 +5322,7 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
> if (!has_vga)
> return;
>
> - lpt_enable_clkout_dp(dev);
> + lpt_enable_clkout_dp(dev, true, true);
> }
>
> /*
> --
> 1.8.1.2
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] drm/i915: disable CLKOUT_DP when it's not needed
On Fri, Jul 12, 2013 at 02:19:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni
>
> We currently don't support HDMI clock bending nor use SSC for DP or
> HDMI on Haswell, so the only case where we need CLKOUT_DP is for VGA.
>
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/intel_display.c | 38
>
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5f3b636..059c9a8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5305,6 +5305,36 @@ static void lpt_enable_clkout_dp(struct drm_device
> *dev, bool with_spread,
> mutex_unlock(&dev_priv->dpio_lock);
> }
>
> +/* Sequence to disable CLKOUT_DP */
> +static void lpt_disable_clkout_dp(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + uint32_t tmp;
> +
> + mutex_lock(&dev_priv->dpio_lock);
> +
> + if (IS_ULT(dev_priv->dev)) {
> + tmp = intel_sbi_read(dev_priv, SBI_GEN0, SBI_ICLK);
> + tmp &= ~SBI_GEN0_ENABLE;
> + intel_sbi_write(dev_priv, SBI_GEN0, tmp, SBI_ICLK);
> + } else {
> + tmp = intel_sbi_read(dev_priv, SBI_DBUFF0, SBI_ICLK);
> + tmp &= ~SBI_DBUFF0_ENABLE;
> + intel_sbi_write(dev_priv, SBI_DBUFF0, tmp, SBI_ICLK);
> + }
> +
> + tmp = intel_sbi_read(dev_priv, SBI_SSCCTL, SBI_ICLK);
> + if (!(tmp & SBI_SSCCTL_PATHALT)) {
> + tmp |= SBI_SSCCTL_PATHALT;
> + intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
> + udelay(32);
> + }
Apologies if this is stupid, I'm new to this part of the doc, shouldn't
it be:
if (tmp & ~SBI_SSCCTL_DISABLE == 0)
> + tmp |= SBI_SSCCTL_DISABLE;
> + intel_sbi_write(dev_priv, SBI_SSCCTL, tmp, SBI_ICLK);
> +
> + mutex_unlock(&dev_priv->dpio_lock);
> +}
> +
> static void lpt_init_pch_refclk(struct drm_device *dev)
> {
> struct drm_mode_config *mode_config = &dev->mode_config;
> @@ -5319,10 +5349,10 @@ static void lpt_init_pch_refclk(struct drm_device
> *dev)
> }
> }
>
> - if (!has_vga)
> - return;
> -
> - lpt_enable_clkout_dp(dev, true, true);
> + if (has_vga)
> + lpt_enable_clkout_dp(dev, true, true);
> + else
> + lpt_disable_clkout_dp(dev);
> }
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni
>
> For now there are no callers, but these functions are going to be
> needed for the code that allows Package C8+. Other future features may
> also require this code.
>
The thing that's missing from the patches is any sort of assertions
about things being on before the disable sequence. Is this something we
don't need to address?
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/i915_reg.h | 7 +++
> drivers/gpu/drm/i915/intel_display.c | 95
>
> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> 3 files changed, 105 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index be6164f..8e5a5ec 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4930,7 +4930,14 @@
> #define LCPLL_CLK_FREQ_450 (0<<26)
> #define LCPLL_CD_CLOCK_DISABLE (1<<25)
> #define LCPLL_CD2X_CLOCK_DISABLE(1<<23)
> +#define LCPLL_POWER_DOWN_ALLOW (1<<22)
> #define LCPLL_CD_SOURCE_FCLK(1<<21)
> +#define LCPLL_CD_SOURCE_FCLK_DONE (1<<19)
Hmm... the doc I am looking at says
> +
> +#define D_COMP (MCHBAR_MIRROR_BASE_SNB +
> 0x5F0C)
> +#define D_COMP_RCOMP_IN_PROGRESS(1<<9)
> +#define D_COMP_COMP_FORCE (1<<8)
> +#define D_COMP_COMP_DISABLE (1<<0)
>
> /* Pipe WM_LINETIME - watermark line time */
> #define PIPE_WM_LINETIME_A 0x45270
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 059c9a8..ffb08bf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct
> intel_crtc *crtc,
> return true;
> }
>
> +/*
> + * This function implements pieces of two sequences from BSpec:
> + * - Sequence for display software to disable LCPLL
> + * - Sequence for display software to allow package C8+
> + * The steps implemented here are just the steps that actually touch the
> LCPLL
> + * register. Callers should take care of disabling all the display engine
> + * functions, doing the mode unset, fixing interrupts, etc.
> + */
> +void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> +bool switch_to_fclk, bool allow_power_down)
> +{
> + uint32_t val;
> +
> + val = I915_READ(LCPLL_CTL);
> +
> + if (switch_to_fclk) {
> + val |= LCPLL_CD_SOURCE_FCLK;
> + I915_WRITE(LCPLL_CTL, val);
> + POSTING_READ(LCPLL_CTL);
> +
> + udelay(1);
> +
> + val = I915_READ(LCPLL_CTL);
> + if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
> + DRM_ERROR("Switching to FCLK failed\n");
wait_for_us(..., 1)?
> + }
> +
> + val |= LCPLL_PLL_DISABLE;
> + I915_WRITE(LCPLL_CTL, val);
> + POSTING_READ(LCPLL_CTL);
> +
> + if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
> + DRM_ERROR("LCPLL still locked\n");
> +
> + val = I915_READ(D_COMP);
> + val |= D_COMP_COMP_DISABLE;
> + I915_WRITE(D_COMP, val);
> + POSTING_READ(D_COMP);
> +
> + udelay(2);
ndelay(100)?
> +
> + val = I915_READ(D_COMP);
> + if (val & D_COMP_RCOMP_IN_PROGRESS)
> + DRM_ERROR("D_COMP RCOMP still in progress\n");
wait_for(..., 1)?
> +
> + if (allow_power_down) {
> + val = I915_READ(LCPLL_CTL);
> + val |= LCPLL_POWER_DOWN_ALLOW;
> + I915_WRITE(LCPLL_CTL, val);
> + POSTING_READ(LCPLL_CTL);
> + }
> +}
> +
> +/*
> + * Fully restores LCPLL, disallowing power down and switching back to LCPLL
> + * source.
> + */
> +void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> +{
> + uint32_t val;
> +
> + val = I915_READ(LCPLL_CTL);
> +
I think we could potentially exit early here if the PLL is already
locked, and we're on CDclk. And indeed, I've already seen this case
occur, but I'm not sure I will ever see that case again.
> + if (val & LCPLL_POWER_DOWN_ALLOW) {
> + val &= ~LCPLL_POWER_DOWN_ALLOW;
> + I915_WRITE(LCPLL_CTL, val);
> + }
> +
> + val = I915_READ(D_COMP);
> + val |= D_COMP_COMP_FORCE;
> + val &= ~D_COMP_COMP_DISABLE;
> + I915_WRITE(D_COMP, val);
> +
I think you need a posting read here. I am not sure we're allowed to
read LCPLL_CTL until we know the write has landed.
> + val = I915_READ(LCPLL_CTL);
> + val &= ~LCPLL_PLL_DISABLE;
> + I915_WRITE(LCPLL_CTL, val);
> + POSTING_READ(LCPLL_CTL);
^ unnecessary POSTING_READ - but meh
> +
> + if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK, 5))
> + DRM_ERROR("LCPLL not locked yet\n");
> +
> + if (val & LCPLL_CD_SOURCE_FCLK) {
> + val = I915_READ(LCPLL_CTL);
> + val &= ~LCP
Re: [Intel-gfx] [PATCH 7/7] drm/i915: add some assertions to hsw_disable_lcpll
On Fri, Jul 12, 2013 at 02:19:42PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni
>
> Most of the hardware needs to be disabled before LCPLL is disabled, so
> let's add a function to assert some of items listed in the "Display
> Sequences for LCPLL disabling" documentation.
>
> The idea is that hsw_disable_lcpll should not disable the hardware,
> the callers need to take care of calling hsw_disable_lcpll only once
> everything is already disabled.
>
> Signed-off-by: Paulo Zanoni
I don't see a reason to separate this patch from the previous one. It
makes review easier, but it's weird bisect wise. The correct order, if
you wanted to do them as separate patches would be to introduce the
assertions, and then add the functionality (I think).
> ---
> drivers/gpu/drm/i915/i915_reg.h | 8
> drivers/gpu/drm/i915/intel_display.c | 28
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e5a5ec..9556dff 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2195,6 +2195,8 @@
> #define BLC_PWM_CPU_CTL2 0x48250
> #define BLC_PWM_CPU_CTL 0x48254
>
> +#define HSW_BLC_PWM2_CTL 0x48350
> +
> /* PCH CTL1 is totally different, all but the below bits are reserved. CTL2
> is
> * like the normal CTL from gen4 and earlier. Hooray for confusing naming. */
> #define BLC_PWM_PCH_CTL1 0xc8250
> @@ -2203,6 +2205,12 @@
> #define BLM_PCH_POLARITY (1 << 29)
> #define BLC_PWM_PCH_CTL2 0xc8254
>
> +#define UTIL_PIN_CTL 0x48400
> +#define UTIL_PIN_ENABLE(1 << 31)
> +
> +#define PCH_GTC_CTL 0xe7000
> +#define PCH_GTC_ENABLE (1 << 31)
> +
> /* TV port control */
> #define TV_CTL 0x68000
> /** Enables the TV encoder */
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index ffb08bf..9055f60 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5922,6 +5922,32 @@ static bool ironlake_get_pipe_config(struct intel_crtc
> *crtc,
> return true;
> }
>
> +static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> +{
> + struct drm_device *dev = dev_priv->dev;
> + struct intel_ddi_plls *plls = &dev_priv->ddi_plls;
> + struct intel_crtc *crtc;
> +
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head)
> + WARN(crtc->base.enabled, "CRTC for pipe %c enabled\n",
> + pipe_name(crtc->pipe));
> +
> + WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> + WARN(plls->spll_refcount, "SPLL enabled\n");
> + WARN(plls->wrpll1_refcount, "WRPLL1 enabled\n");
> + WARN(plls->wrpll2_refcount, "WRPLL2 enabled\n");
> + WARN(I915_READ(PCH_PP_STATUS) & PP_ON, "Panel power on\n");
> + WARN(I915_READ(BLC_PWM_CPU_CTL2) & BLM_PWM_ENABLE,
> + "CPU PWM1 enabled\n");
> + WARN(I915_READ(HSW_BLC_PWM2_CTL) & BLM_PWM_ENABLE,
> + "CPU PWM2 enabled\n");
> + WARN(I915_READ(BLC_PWM_PCH_CTL1) & BLM_PCH_PWM_ENABLE,
> + "PCH PWM1 enabled\n");
> + WARN(I915_READ(UTIL_PIN_CTL) & UTIL_PIN_ENABLE,
> + "Utility pin enabled\n");
> + WARN(I915_READ(PCH_GTC_CTL) & PCH_GTC_ENABLE, "PCH GTC enabled\n");
Looking at probably the same list you've looked at, I don't see:
Audio controller
eDP HPD
Other CPU/PCH interrupts
You've worked with this code longer than I have, so maybe you can
explain why you've skipped them.
> +}
> +
> /*
> * This function implements pieces of two sequences from BSpec:
> * - Sequence for display software to disable LCPLL
> @@ -5935,6 +5961,8 @@ void hsw_disable_lcpll(struct drm_i915_private
> *dev_priv,
> {
> uint32_t val;
>
> + assert_can_disable_lcpll(dev_priv);
> +
Do we want to proceed if the above fails? I was sort of under the
impression that hard hangs can occur as a result of some functions still
being enabled when we change clocks. I'm fine with continuing on since
we have the WARNS, just wondering if you've thought about it.
> val = I915_READ(LCPLL_CTL);
>
> if (switch_to_fclk) {
> --
> 1.8.1.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] HSW/LPT clocking code additional sequences
On Fri, Jul 12, 2013 at 02:19:35PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > Hi > > On the code that allows Package C8+ we need to disable/reenable the PCH > reference clocks and also disable/reenable LCPLL. This series implements the > sequences that are going to be needed. I have local patches that use this code > and they seem to work (we survive going to C10 and back to C7), and I also > sent > earlier versions of these patches to the mailing list, so open-source code > that > uses these functions already exists on the intel-gfx mailing list. The goal > here > is allow people to review/merge this while the final PC8+ patches are not yet > 100% reviewer-compliant. > > On the first 3 patches we massage our code so we can reuse it for PC8+ code. > On > patch 4 we extend the current code to also implement one of the sequences > required by PC8+. On patch 5 we implement the equivalent disable sequence, > also > needed by PC8+. On patch 6 we implement the functions to disable and restore > LCPLL, and on patch 7 we add some assertions that are going to be used. > > If you want to review patches 1-3 all you need is C experience. For patches 4 > and 5 you need to read the "Display iCLK programming" chapter of BSpec. For > patches 6 and 7 you need to read "Display sequences for LCPLL disabling" and > "Display sequences for Package C8". > > Cheers, > Paulo > > Paulo Zanoni (7): > drm/i915: remove SDV support from lpt_pch_init_refclk > drm/i915: extract FDI mPHY functions from lpt_init_pch_refclk > drm/i915: extract lpt_enable_clkout_dp from lpt_init_pch_refclk > drm/i915: extend lpt_enable_clkout_dp > drm/i915: disable CLKOUT_DP when it's not needed > drm/i915: add functions to disable and restore LCPLL > drm/i915: add some assertions to hsw_disable_lcpll > > drivers/gpu/drm/i915/i915_reg.h | 17 ++ > drivers/gpu/drm/i915/intel_display.c | 359 > +-- > drivers/gpu/drm/i915/intel_drv.h | 3 + > 3 files changed, 276 insertions(+), 103 deletions(-) > With the comments addressed in patches 1-5, they are: Reviewed-by: Ben Widawsky I'll wait for feedback before slapping tags on 6 & 7. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915: add functions to disable and restore LCPLL
On Thu, Jul 18, 2013 at 04:26:42PM -0700, Ben Widawsky wrote:
> On Fri, Jul 12, 2013 at 02:19:41PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni
> >
> > For now there are no callers, but these functions are going to be
> > needed for the code that allows Package C8+. Other future features may
> > also require this code.
> >
>
> The thing that's missing from the patches is any sort of assertions
> about things being on before the disable sequence. Is this something we
> don't need to address?
>
> > Signed-off-by: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 7 +++
> > drivers/gpu/drm/i915/intel_display.c | 95
> >
> > drivers/gpu/drm/i915/intel_drv.h | 3 ++
> > 3 files changed, 105 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index be6164f..8e5a5ec 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4930,7 +4930,14 @@
> > #define LCPLL_CLK_FREQ_450(0<<26)
> > #define LCPLL_CD_CLOCK_DISABLE(1<<25)
> > #define LCPLL_CD2X_CLOCK_DISABLE (1<<23)
> > +#define LCPLL_POWER_DOWN_ALLOW(1<<22)
> > #define LCPLL_CD_SOURCE_FCLK (1<<21)
> > +#define LCPLL_CD_SOURCE_FCLK_DONE (1<<19)
>
> Hmm... the doc I am looking at says
Oops. The doc I was looking at had some different names for things, was
what I wanted to say.
>
> > +
> > +#define D_COMP (MCHBAR_MIRROR_BASE_SNB +
> > 0x5F0C)
> > +#define D_COMP_RCOMP_IN_PROGRESS (1<<9)
> > +#define D_COMP_COMP_FORCE (1<<8)
> > +#define D_COMP_COMP_DISABLE (1<<0)
> >
> > /* Pipe WM_LINETIME - watermark line time */
> > #define PIPE_WM_LINETIME_A 0x45270
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 059c9a8..ffb08bf 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5922,6 +5922,101 @@ static bool ironlake_get_pipe_config(struct
> > intel_crtc *crtc,
> > return true;
> > }
> >
> > +/*
> > + * This function implements pieces of two sequences from BSpec:
> > + * - Sequence for display software to disable LCPLL
> > + * - Sequence for display software to allow package C8+
> > + * The steps implemented here are just the steps that actually touch the
> > LCPLL
> > + * register. Callers should take care of disabling all the display engine
> > + * functions, doing the mode unset, fixing interrupts, etc.
> > + */
> > +void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> > + bool switch_to_fclk, bool allow_power_down)
> > +{
> > + uint32_t val;
> > +
> > + val = I915_READ(LCPLL_CTL);
> > +
> > + if (switch_to_fclk) {
> > + val |= LCPLL_CD_SOURCE_FCLK;
> > + I915_WRITE(LCPLL_CTL, val);
> > + POSTING_READ(LCPLL_CTL);
> > +
> > + udelay(1);
> > +
> > + val = I915_READ(LCPLL_CTL);
> > + if (!(val & LCPLL_CD_SOURCE_FCLK_DONE))
> > + DRM_ERROR("Switching to FCLK failed\n");
>
> wait_for_us(..., 1)?
>
> > + }
> > +
> > + val |= LCPLL_PLL_DISABLE;
> > + I915_WRITE(LCPLL_CTL, val);
> > + POSTING_READ(LCPLL_CTL);
> > +
> > + if (wait_for((I915_READ(LCPLL_CTL) & LCPLL_PLL_LOCK) == 0, 1))
> > + DRM_ERROR("LCPLL still locked\n");
> > +
> > + val = I915_READ(D_COMP);
> > + val |= D_COMP_COMP_DISABLE;
> > + I915_WRITE(D_COMP, val);
> > + POSTING_READ(D_COMP);
> > +
> > + udelay(2);
>
> ndelay(100)?
>
> > +
> > + val = I915_READ(D_COMP);
> > + if (val & D_COMP_RCOMP_IN_PROGRESS)
> > + DRM_ERROR("D_COMP RCOMP still in progress\n");
>
> wait_for(..., 1)?
>
> > +
> > + if (allow_power_down) {
> > + val = I915_READ(LCPLL_CTL);
> > + val |= LCPLL_POWER_DOWN_ALLOW;
> > + I915_WRITE(LCPLL_CTL, val);
> > + POSTING_READ(LCPLL_CTL);
> > + }
> > +}
> > +
> > +/*
> > + * Fully restores LCPLL, disallowing power down and switching back to LCPLL
> > + * source.
> > + */
> > +void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> > +{
> > + uint32_t val;
> > +
> > + val = I915_READ(LCPLL_CTL);
> > +
>
> I think we could potentially exit early here if the PLL is already
> locked, and we're on CDclk. And indeed, I've already seen this case
> occur, but I'm not sure I will ever see that case again.
>
> > + if (val & LCPLL_POWER_DOWN_ALLOW) {
> > + val &= ~LCPLL_POWER_DOWN_ALLOW;
> > + I915_WRITE(LCPLL_CTL, val);
> > + }
> > +
> > + val = I915_READ(D_COMP);
> > + val |= D_COMP_COMP_FORCE;
> > + val &= ~D_COMP_COMP_DISABLE;
> > + I915_WRITE(D_COMP, val);
> > +
>
> I think you need a posting read here. I am not sure we're allowed to
> read LCPLL_CTL until we know the write has landed.
>
>
> > + val = I915_READ(LCPLL_CTL);
> > + val &= ~LCPLL_
[Intel-gfx] [PATCH 3/3] [v2] drm/i915: Make i915 events part of uapi
Make the uevent strings part of the user API for people who wish to
write their own listeners.
v2: Make a space in the string concatenation. (Chad)
Use the "UEVENT" suffix intead of "EVENT" (Chad)
Make kernel-doc (Daniel)
Thanks to Daniel Vetter for a majority of the parity error comments.
CC: Chad Versace
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_irq.c | 8
include/uapi/drm/i915_drm.h | 22 ++
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 03071d7..3e19ce0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -812,7 +812,7 @@ static void ivybridge_parity_work(struct work_struct *work)
mutex_unlock(&dev_priv->dev->struct_mutex);
- parity_event[0] = "L3_PARITY_ERROR=1";
+ parity_event[0] = I915_L3_PARITY_UEVENT "=1";
parity_event[1] = kasprintf(GFP_KERNEL, "ROW=%d", row);
parity_event[2] = kasprintf(GFP_KERNEL, "BANK=%d", bank);
parity_event[3] = kasprintf(GFP_KERNEL, "SUBBANK=%d", subbank);
@@ -1435,8 +1435,8 @@ static void i915_error_work_func(struct work_struct *work)
gpu_error);
struct drm_device *dev = dev_priv->dev;
struct intel_ring_buffer *ring;
- char *reset_event[] = { "RESET=1", NULL };
- char *reset_done_event[] = { "RESET=0", NULL };
+ char *reset_event[] = { I915_RESET_UEVENT "=1", NULL };
+ char *reset_done_event[] = { I915_RESET_UEVENT "=0", NULL };
int i, ret;
/*
@@ -1591,7 +1591,7 @@ void i915_handle_error(struct drm_device *dev, bool
wedged)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_ring_buffer *ring;
- char *error_event[] = { "ERROR=1", NULL };
+ char *error_event[] = { I915_ERROR_UEVENT "=1", NULL };
int i;
i915_capture_error_state(dev);
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 923ed7f..f5fe31b 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -33,6 +33,28 @@
* subject to backwards-compatibility constraints.
*/
+/**
+ * DOC: uevents generated by i915 on it's device node
+ *
+ * I915_L3_PARITY_UEVENT - Generated when the driver receives a parity mismatch
+ * event from the gpu l3 cache. Additional information supplied is ROW,
+ * BANK, SUBBANK of the affected cacheline. Userspace should keep track of
+ * these events and if a specific cache-line seems to have a persistent
+ * error remap it with the l3 remapping tool supplied in intel-gpu-tools.
+ * The value supplied with the event is always 1.
+ *
+ * I915_ERROR_UEVENT - Generated upon error detection, currently only via
+ * hangcheck. The error detection event is a good indicator of when things
+ * began to go badly. The value supplied with the event is always 1.
+ *
+ * I915_RESET_UEVENT - Event is generated just before an attempt to reset the
+ * the GPU. Reset has historically been a precarious attempt which the GPU
+ * may or may not recover from. The value supplied with the event is 1
+ * before the reset attempt, and a 0 after it has completed.
+ */
+#define I915_L3_PARITY_UEVENT "L3_PARITY_ERROR"
+#define I915_ERROR_UEVENT "ERROR"
+#define I915_RESET_UEVENT "RESET"
/* Each region is a minimum of 16k, and there are at most 255 of them.
*/
--
1.8.3.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Move error uevent to detection time
We've recently deferred error handling with a workqueue for a number of
reasons. However, when we're trying to pass the information to
userspace, it's likely more interesting to know when the error was
detected by the kernel, and not when we eventually get around to
handling it in the workqueue.
This was inspired as a result of the patch to move these events to the
uapi.
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_irq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9910699..9fe430a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1435,13 +1435,10 @@ static void i915_error_work_func(struct work_struct
*work)
gpu_error);
struct drm_device *dev = dev_priv->dev;
struct intel_ring_buffer *ring;
- char *error_event[] = { "ERROR=1", NULL };
char *reset_event[] = { "RESET=1", NULL };
char *reset_done_event[] = { "ERROR=0", NULL };
int i, ret;
- kobject_uevent_env(&dev->primary->kdev.kobj, KOBJ_CHANGE, error_event);
-
/*
* Note that there's only one work item which does gpu resets, so we
* need not worry about concurrent gpu resets potentially incrementing
@@ -1594,11 +1591,14 @@ void i915_handle_error(struct drm_device *dev, bool
wedged)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_ring_buffer *ring;
+ char *error_event[] = { "ERROR=1", NULL };
int i;
i915_capture_error_state(dev);
i915_report_and_clear_eir(dev);
+ kobject_uevent_env(&dev->primary->kdev.kobj, KOBJ_CHANGE, error_event);
+
if (wedged) {
atomic_set_mask(I915_RESET_IN_PROGRESS_FLAG,
&dev_priv->gpu_error.reset_counter);
--
1.8.3.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: Change uevent for reset completion
Prior to this patch, we used ERROR=0 as a way of signifying the reset
was complete. The functionality dates back quite a ways to:
commit f316a42cc49eca73b33d85feb6177e32431747ff
Author: Ben Gamari
Date: Mon Sep 14 17:48:46 2009 -0400
drm/i915: Hookup chip reset in error handler
I'm not really sure what the motivation for this was originally, but to
me it makes more sense to have a distinct event for error detection, and
another event for reset start/finish (since reset is prone to failure).
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_irq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9fe430a..03071d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1436,7 +1436,7 @@ static void i915_error_work_func(struct work_struct *work)
struct drm_device *dev = dev_priv->dev;
struct intel_ring_buffer *ring;
char *reset_event[] = { "RESET=1", NULL };
- char *reset_done_event[] = { "ERROR=0", NULL };
+ char *reset_done_event[] = { "RESET=0", NULL };
int i, ret;
/*
--
1.8.3.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [patch 1/2] drm/i915: checking for NULL instead of IS_ERR()
i915_gem_vma_create() returns and ERR_PTR() or a valid pointer, it never
returns NULL.
Signed-off-by: Dan Carpenter
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40c2fc6..9a9a77a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3113,9 +3113,9 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object
*obj,
i915_gem_object_pin_pages(obj);
vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
- if (vma == NULL) {
+ if (IS_ERR(vma)) {
i915_gem_object_unpin_pages(obj);
- return -ENOMEM;
+ return PTR_ERR(vma);
}
search_free:
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
b/drivers/gpu/drm/i915/i915_gem_stolen.c
index f526136..27ffb4c 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -395,8 +395,8 @@ i915_gem_object_create_stolen_for_preallocated(struct
drm_device *dev,
return obj;
vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
- if (!vma) {
- ret = -ENOMEM;
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
goto err_out;
}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [patch 2/2] drm/i915: use after free on error path
i915_gem_vma_destroy() frees its argument so we have to move the drm_mm_remove_node() call up a few lines. Signed-off-by: Dan Carpenter diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 9a9a77a..f347ad5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3161,9 +3161,9 @@ search_free: return 0; err_out: + drm_mm_remove_node(&vma->node); i915_gem_vma_destroy(vma); i915_gem_object_unpin_pages(obj); - drm_mm_remove_node(&vma->node); return ret; } ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: add prefault_disable module option
prefault is stll enabled by default which prevent most of pwrite/pread/reloc
from running slow path, in order to verify these slow pathes, prefault need
to be disabled.
Signed-off-by: Xiong Zhang
---
drivers/gpu/drm/i915/i915_drv.c| 5 +
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_gem.c| 12 +++-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 --
4 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b07362f..dac6bd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -137,6 +137,11 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
"(default: false)");
+bool i915_prefault_disable __read_mostly = false;
+module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
+MODULE_PARM_DESC(prefault_disable,
+ "Try to disable pre page fault for pread/pwrite/reloc
(default:false)");
+
static struct drm_driver driver;
extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fdc8e3..9516e19 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1587,6 +1587,7 @@ extern unsigned int i915_preliminary_hw_support
__read_mostly;
extern int i915_disable_power_well __read_mostly;
extern int i915_enable_ips __read_mostly;
extern bool i915_fastboot __read_mostly;
+extern bool i915_prefault_disable __read_mostly;
extern int i915_suspend(struct drm_device *dev, pm_message_t state);
extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c9d9d20..de59154 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -465,7 +465,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
mutex_unlock(&dev->struct_mutex);
- if (!prefaulted) {
+ if (likely(!i915_prefault_disable) && !prefaulted) {
ret = fault_in_multipages_writeable(user_data, remain);
/* Userspace is tricking us, but we've already clobbered
* its pages with the prefault and promised to write the
@@ -860,10 +860,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
args->size))
return -EFAULT;
- ret = fault_in_multipages_readable(to_user_ptr(args->data_ptr),
- args->size);
- if (ret)
- return -EFAULT;
+ if (likely(!i915_prefault_disable)) {
+ ret = fault_in_multipages_readable(to_user_ptr(args->data_ptr),
+ args->size);
+ if (ret)
+ return -EFAULT;
+ }
ret = i915_mutex_lock_interruptible(dev);
if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1b58694..1734825 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -759,8 +759,10 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (!access_ok(VERIFY_WRITE, ptr, length))
return -EFAULT;
- if (fault_in_multipages_readable(ptr, length))
- return -EFAULT;
+ if (likely(!i915_prefault_disable)) {
+ if (fault_in_multipages_readable(ptr, length))
+ return -EFAULT;
+ }
}
return 0;
--
1.8.3.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
fault_in_multipages_writeable() will load fault page into physical memory, then pread can run fast path, no need to run slow path Signed-off-by: Xiong Zhang --- drivers/gpu/drm/i915/i915_gem.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f194d7e..982df1e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -459,6 +459,7 @@ i915_gem_shmem_pread(struct drm_device *dev, page_do_bit17_swizzling = obj_do_bit17_swizzling && (page_to_phys(page) & (1 << 17)) != 0; +read_again: ret = shmem_pread_fast(page, shmem_page_offset, page_length, user_data, page_do_bit17_swizzling, needs_clflush); @@ -475,6 +476,8 @@ i915_gem_shmem_pread(struct drm_device *dev, * and just continue. */ (void)ret; prefaulted = 1; + mutex_lock(&dev->struct_mutex); + goto read_again; } ret = shmem_pread_slow(page, shmem_page_offset, page_length, -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: add debug info in slow path
Signed-off-by: Xiong Zhang
---
drivers/gpu/drm/i915/i915_gem.c| 4
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index de59154..f194d7e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -376,6 +376,8 @@ shmem_pread_slow(struct page *page, int shmem_page_offset,
int page_length,
char *vaddr;
int ret;
+ DRM_DEBUG("execute pread slow path\n");
+
vaddr = kmap(page);
if (needs_clflush)
shmem_clflush_swizzled_range(vaddr + shmem_page_offset,
@@ -689,6 +691,8 @@ shmem_pwrite_slow(struct page *page, int shmem_page_offset,
int page_length,
char *vaddr;
int ret;
+ DRM_DEBUG("execute pwrite slow path\n");
+
vaddr = kmap(page);
if (unlikely(needs_clflush_before || page_do_bit17_swizzling))
shmem_clflush_swizzled_range(vaddr + shmem_page_offset,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1734825..3065297 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -587,6 +587,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
int i, total, ret;
int count = args->buffer_count;
+ DRM_DEBUG("Execute relocate slow path\n");
/* We may process another execbuffer during the unlock... */
while (!list_empty(&eb->objects)) {
obj = list_first_entry(&eb->objects,
--
1.8.3.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/5] lib/drmtest: add drmtest_disable/enable_prefault() function
Signed-off-by: Xiong Zhang
---
lib/drmtest.c | 43 +++
lib/drmtest.h | 3 +++
2 files changed, 46 insertions(+)
diff --git a/lib/drmtest.c b/lib/drmtest.c
index 011d8c1..713c5ff 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -1593,3 +1593,46 @@ void kmstest_free_connector_config(struct
kmstest_connector_config *config)
drmModeFreeEncoder(config->encoder);
drmModeFreeConnector(config->connector);
}
+
+#define PREFAULT_DEBUGFS "/sys/module/i915/parameters/prefault_disable"
+static int drmtest_prefault_control(bool enable)
+{
+ char *name = PREFAULT_DEBUGFS;
+ int fd;
+ char buf[2] = {'Y', 'N'};
+ int index;
+ int result = 0;
+
+ fd = open(name, O_RDWR);
+ if (fd == -1) {
+ fprintf(stderr, "Couldn't open prefault_debugfs.%s\n",
+ strerror(errno));
+ return -1;
+ }
+
+ if (enable)
+ index = 1;
+ else
+ index = 0;
+
+ if (write(fd, &buf[index], 1) != 1) {
+ fprintf(stderr, "write prefault_debugfs error.%s\n",
+ strerror(errno));
+ result = -1;
+ }
+
+ close(fd);
+
+ return result;
+}
+
+int drmtest_disable_prefault(void)
+{
+ return drmtest_prefault_control(false);
+}
+
+int drmtest_enable_prefault(void)
+{
+ return drmtest_prefault_control(true);
+}
+
diff --git a/lib/drmtest.h b/lib/drmtest.h
index e3a9275..80b344c 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -179,3 +179,6 @@ void drmtest_enable_exit_handler(void);
void drmtest_disable_exit_handler(void);
int drmtest_set_vt_graphics_mode(void);
+
+int drmtest_disable_prefault(void);
+int drmtest_enable_prefault(void);
--
1.8.3.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/5] lib/drmtest: move gem_linear_blt() to drm_test.c
Several test functions will call gem_linear_blt(), so move this function
to drm_test.c.
Signed-off-by: Xiong Zhang
---
lib/drmtest.c | 75
lib/drmtest.h | 5 +++
tests/gem_exec_blt.c| 84 +
tests/gem_exec_faulting_reloc.c | 84 +
tests/gem_pin.c | 51 ++---
5 files changed, 88 insertions(+), 211 deletions(-)
diff --git a/lib/drmtest.c b/lib/drmtest.c
index 713c5ff..7f79fd3 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -554,6 +554,81 @@ int gem_madvise(int fd, uint32_t handle, int state)
return madv.retained;
}
+
+int gem_linear_blt(uint32_t *batch,
+ uint32_t src,
+ uint32_t dst,
+ uint32_t length,
+ struct drm_i915_gem_relocation_entry *reloc)
+{
+ uint32_t *b = batch;
+ int height = length / (16 * 1024);
+
+ assert(height <= 1<<16);
+
+ if (height) {
+ b[0] = XY_SRC_COPY_BLT_CMD | XY_SRC_COPY_BLT_WRITE_ALPHA |
XY_SRC_COPY_BLT_WRITE_RGB;
+ b[1] = 0xcc << 16 | 1 << 25 | 1 << 24 | (16*1024);
+ b[2] = 0;
+ b[3] = height << 16 | (4*1024);
+ b[4] = 0;
+ reloc->offset = (b-batch+4) * sizeof(uint32_t);
+ reloc->delta = 0;
+ reloc->target_handle = dst;
+ reloc->read_domains = I915_GEM_DOMAIN_RENDER;
+ reloc->write_domain = I915_GEM_DOMAIN_RENDER;
+ reloc->presumed_offset = 0;
+ reloc++;
+
+ b[5] = 0;
+ b[6] = 16*1024;
+ b[7] = 0;
+ reloc->offset = (b-batch+7) * sizeof(uint32_t);
+ reloc->delta = 0;
+ reloc->target_handle = src;
+ reloc->read_domains = I915_GEM_DOMAIN_RENDER;
+ reloc->write_domain = 0;
+ reloc->presumed_offset = 0;
+ reloc++;
+
+ b += 8;
+ length -= height * 16*1024;
+ }
+
+ if (length) {
+ b[0] = XY_SRC_COPY_BLT_CMD | XY_SRC_COPY_BLT_WRITE_ALPHA |
XY_SRC_COPY_BLT_WRITE_RGB;
+ b[1] = 0xcc << 16 | 1 << 25 | 1 << 24 | (16*1024);
+ b[2] = height << 16;
+ b[3] = (1+height) << 16 | (length / 4);
+ b[4] = 0;
+ reloc->offset = (b-batch+4) * sizeof(uint32_t);
+ reloc->delta = 0;
+ reloc->target_handle = dst;
+ reloc->read_domains = I915_GEM_DOMAIN_RENDER;
+ reloc->write_domain = I915_GEM_DOMAIN_RENDER;
+ reloc->presumed_offset = 0;
+ reloc++;
+
+ b[5] = height << 16;
+ b[6] = 16*1024;
+ b[7] = 0;
+ reloc->offset = (b-batch+7) * sizeof(uint32_t);
+ reloc->delta = 0;
+ reloc->target_handle = src;
+ reloc->read_domains = I915_GEM_DOMAIN_RENDER;
+ reloc->write_domain = 0;
+ reloc->presumed_offset = 0;
+ reloc++;
+
+ b += 8;
+ }
+
+ b[0] = MI_BATCH_BUFFER_END;
+ b[1] = 0;
+
+ return (b+2 - batch) * sizeof(uint32_t);
+}
+
/* prime */
int prime_handle_to_fd(int fd, uint32_t handle)
{
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 80b344c..c179aed 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -36,6 +36,7 @@
#include "xf86drm.h"
#include "xf86drmMode.h"
#include "intel_batchbuffer.h"
+#include "i915_drm.h"
drm_intel_bo * gem_handle_to_libdrm_bo(drm_intel_bufmgr *bufmgr, int fd,
const char *name, uint32_t handle);
@@ -72,6 +73,10 @@ uint64_t gem_aperture_size(int fd);
uint64_t gem_mappable_aperture_size(void);
int gem_madvise(int fd, uint32_t handle, int state);
+int gem_linear_blt(uint32_t *batch,uint32_t src,
+ uint32_t dst, uint32_t length,
+ struct drm_i915_gem_relocation_entry *reloc);
+
/* feature test helpers */
bool gem_uses_aliasing_ppgtt(int fd);
int gem_available_fences(int fd);
diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
index eb5ae66..6a0f863 100644
--- a/tests/gem_exec_blt.c
+++ b/tests/gem_exec_blt.c
@@ -46,94 +46,14 @@
#define OBJECT_SIZE 16384
-#define COPY_BLT_CMD (2<<29|0x53<<22|0x6)
-#define BLT_WRITE_ALPHA(1<<21)
-#define BLT_WRITE_RGB (1<<20)
-#define BLT_SRC_TILED (1<<15)
-#define BLT_DST_TILED (1<<11)
-
-static int gem_linear_blt(uint32_t *batch,
- uint32_t src,
- uint32_t dst,
- uint32_t length,
- struct drm_i915_gem_relocation_entry *reloc)
-{
- uint32_t *b = batch;
- int he
[Intel-gfx] [PATCH 4/5] tests/gem_disable_prefault: add pwrite slow path subtest
First disable prefault, then map src bo to gtt space, so when write src bo
to dst bo, it will run into pwrite_slow path. Finally enable prefault
Signed-off-by: Xiong Zhang
---
tests/gem_disable_prefault.c | 43 +++
1 file changed, 43 insertions(+)
diff --git a/tests/gem_disable_prefault.c b/tests/gem_disable_prefault.c
index 9e6f0ad..b6b2d59 100644
--- a/tests/gem_disable_prefault.c
+++ b/tests/gem_disable_prefault.c
@@ -43,6 +43,7 @@
#include "intel_gpu_tools.h"
#define OBJECT_SIZE (16*1024*1024)
+#define TEST_VALUE 0x5a
static void *
mmap_bo(int fd, uint32_t handle, int size)
@@ -144,6 +145,45 @@ test_disable_prefault_reloc(int fd)
gem_close(fd, handle);
}
+static void
+test_disable_prefault_write(int fd)
+{
+ uint32_t src, dst;
+ void *ptr;
+ int i;
+ uint8_t *tmp;
+
+ src = gem_create(fd, OBJECT_SIZE);
+ dst = gem_create(fd, OBJECT_SIZE);
+
+ /* write test_val to src object */
+ ptr = gem_mmap__cpu(fd, src, OBJECT_SIZE, PROT_READ | PROT_WRITE);
+ assert(ptr != NULL);
+ tmp = (uint8_t *)ptr;
+ memset(tmp, TEST_VALUE, OBJECT_SIZE);
+
+ drmtest_disable_prefault();
+ ptr = mmap_bo(fd, src, OBJECT_SIZE);
+ gem_write(fd, dst, 0, ptr, OBJECT_SIZE);
+ munmap(ptr, OBJECT_SIZE);
+ drmtest_enable_prefault();
+
+ /* compare dst object to test_val */
+ ptr = gem_mmap__cpu(fd, dst, OBJECT_SIZE, PROT_READ | PROT_WRITE);
+ assert(ptr != NULL);
+ tmp = (uint8_t *)ptr;
+ for (i = 0; i < OBJECT_SIZE; i++) {
+ if (tmp[i] != TEST_VALUE) {
+ printf("mismatch at %i, got: %i\n",
+ i, tmp[i]);
+ break;
+ }
+ }
+
+ gem_close(fd, dst);
+ gem_close(fd, src);
+}
+
int main(int argc, char **argv)
{
int fd;
@@ -161,6 +201,9 @@ int main(int argc, char **argv)
if (drmtest_run_subtest("reloc"))
test_disable_prefault_reloc(fd);
+ if (drmtest_run_subtest("write"))
+ test_disable_prefault_write(fd);
+
close(fd);
return 0;
--
1.8.3.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/5] tests/gem_disable_prefault: add reloc slow path subtest
first disable prefault, then put relocate bo in gtt space and exec cmd,
so it will run relocate_object_slow path, finally enable prefault
Signed-off-by: Xiong Zhang
---
tests/Makefile.am| 1 +
tests/gem_disable_prefault.c | 168 +++
2 files changed, 169 insertions(+)
create mode 100644 tests/gem_disable_prefault.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3003aa0..f0804b3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -106,6 +106,7 @@ TESTS_progs = \
gem_reg_read \
gem_tiling_max_stride \
prime_udl \
+ gem_disable_prefault \
$(NULL)
# IMPORTANT: The ZZ_ tests need to be run last!
diff --git a/tests/gem_disable_prefault.c b/tests/gem_disable_prefault.c
new file mode 100644
index 000..9e6f0ad
--- /dev/null
+++ b/tests/gem_disable_prefault.c
@@ -0,0 +1,168 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Xiong Zhang
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+#include "intel_chipset.h"
+#include "intel_gpu_tools.h"
+
+#define OBJECT_SIZE (16*1024*1024)
+
+static void *
+mmap_bo(int fd, uint32_t handle, int size)
+{
+ void *ptr;
+
+ ptr = gem_mmap(fd, handle, size, PROT_READ | PROT_WRITE);
+ assert(ptr != MAP_FAILED);
+
+ return ptr;
+}
+
+static void gem_exec(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
+{
+ int ret;
+
+ ret = drmIoctl(fd,
+ DRM_IOCTL_I915_GEM_EXECBUFFER2,
+ execbuf);
+ assert(ret == 0);
+}
+
+static void
+test_disable_prefault_reloc(int fd)
+{
+ struct drm_i915_gem_execbuffer2 execbuf;
+ struct drm_i915_gem_exec_object2 exec[3];
+ struct drm_i915_gem_relocation_entry reloc[4];
+ uint32_t buf[20];
+ uint32_t handle, handle_relocs, src, dst;
+ void *gtt_relocs;
+ int len;
+ int ring;
+
+ handle = gem_create(fd, 4096);
+ src = gem_create(fd, OBJECT_SIZE);
+ dst = gem_create(fd, OBJECT_SIZE);
+
+ len = gem_linear_blt(buf, src, dst, OBJECT_SIZE, reloc);
+ gem_write(fd, handle, 0, buf, len);
+
+ exec[0].handle = src;
+ exec[0].relocation_count = 0;
+ exec[0].relocs_ptr = 0;
+ exec[0].alignment = 0;
+ exec[0].offset = 0;
+ exec[0].flags = 0;
+ exec[0].rsvd1 = 0;
+ exec[0].rsvd2 = 0;
+
+ exec[1].handle = dst;
+ exec[1].relocation_count = 0;
+ exec[1].relocs_ptr = 0;
+ exec[1].alignment = 0;
+ exec[1].offset = 0;
+ exec[1].flags = 0;
+ exec[1].rsvd1 = 0;
+ exec[1].rsvd2 = 0;
+
+ handle_relocs = gem_create(fd, 4096);
+ gem_write(fd, handle_relocs, 0, reloc, sizeof(reloc));
+ gtt_relocs = mmap_bo(fd, handle_relocs, 4096);
+
+ exec[2].handle = handle;
+ exec[2].relocation_count = len > 40 ? 4 : 2;
+ /* A newly mmap gtt bo will fault on first access. */
+ exec[2].relocs_ptr = (uintptr_t)gtt_relocs;
+ exec[2].alignment = 0;
+ exec[2].offset = 0;
+ exec[2].flags = 0;
+ exec[2].rsvd1 = 0;
+ exec[2].rsvd2 = 0;
+
+ ring = 0;
+ if (HAS_BLT_RING(intel_get_drm_devid(fd)))
+ ring = I915_EXEC_BLT;
+
+ execbuf.buffers_ptr = (uintptr_t)exec;
+ execbuf.buffer_count = 3;
+ execbuf.batch_start_offset = 0;
+ execbuf.batch_len = len;
+ execbuf.cliprects_ptr = 0;
+ execbuf.num_cliprects = 0;
+ execbuf.DR1 = 0;
+ execbuf.DR4 = 0;
+ execbuf.flags = ring;
+ i915_execbuffer2_set_context_id(execbuf, 0);
+ execbuf.rsvd2 = 0;
+
+ drmtest_disable_prefault();
+ gem_exec(fd, &execbuf);
+
[Intel-gfx] [PATCH 5/5] tests/gem_disable_prefault: add pread slow path subtest
First disable prefault, then map src bo to gtt map, so pread from dst bo to
src bo, it will run pread_slow path, finally enable prefault
Signed-off-by: Xiong Zhang
---
tests/gem_disable_prefault.c | 42 ++
1 file changed, 42 insertions(+)
diff --git a/tests/gem_disable_prefault.c b/tests/gem_disable_prefault.c
index b6b2d59..76663ec 100644
--- a/tests/gem_disable_prefault.c
+++ b/tests/gem_disable_prefault.c
@@ -184,6 +184,45 @@ test_disable_prefault_write(int fd)
gem_close(fd, src);
}
+static void
+test_disable_prefault_read(int fd)
+{
+ uint32_t src, dst;
+ void *ptr;
+ int i;
+ uint8_t *tmp;
+
+ src = gem_create(fd, OBJECT_SIZE);
+ dst = gem_create(fd, OBJECT_SIZE);
+
+ /* write test_val to dst object */
+ ptr = gem_mmap__cpu(fd, dst, OBJECT_SIZE, PROT_READ | PROT_WRITE);
+ assert(ptr != NULL);
+ tmp = (uint8_t *)ptr;
+ memset(tmp, TEST_VALUE, OBJECT_SIZE);
+
+ drmtest_disable_prefault();
+ ptr = mmap_bo(fd, src, OBJECT_SIZE);
+ gem_read(fd, dst, 0, ptr, OBJECT_SIZE);
+ munmap(ptr, OBJECT_SIZE);
+ drmtest_enable_prefault();
+
+ /* compare src object to test_val */
+ ptr = gem_mmap__cpu(fd, src, OBJECT_SIZE, PROT_READ | PROT_WRITE);
+ assert(ptr != NULL);
+ tmp = (uint8_t *)ptr;
+ for (i = 0; i < OBJECT_SIZE; i++) {
+ if (tmp[i] != TEST_VALUE) {
+ printf("mismatch at %i, got: %i\n",
+ i, tmp[i]);
+ break;
+ }
+ }
+
+ gem_close(fd, dst);
+ gem_close(fd, src);
+}
+
int main(int argc, char **argv)
{
int fd;
@@ -204,6 +243,9 @@ int main(int argc, char **argv)
if (drmtest_run_subtest("write"))
test_disable_prefault_write(fd);
+ if (drmtest_run_subtest("read"))
+ test_disable_prefault_read(fd);
+
close(fd);
return 0;
--
1.8.3.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
