[Intel-gfx] Write GFX_FLSH_CNT after updating GGTT entries
Hi Gurus: I'm curious about the register GFX_FLSH_CNT(0x101008) in i915_gem_gtt.c. Does these register exist in recently generations? After digging into b-spec, it looks only BXT and CHV has this register. Does the desktop platform also have this register which needs to be written after updating GGTT MMIOs? BTW: Looks windows driver haven't used this MMIO... So whose behavior is the right behavior? Thanks, Zhi. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 2/2] drm/i915: make intel_backlight sysfs interface have range 0..100
Historically we have exposed the full backlight PWM duty cycle range from 0 to the PWM modulation frequency as the backlight range to the userspace. Since then, we have had to scale that to respect panel specific minimum duty cycles. Go for fully abstracting the PWM duty cycles and modulation frequencies from the userspace by exposing a fixed range 0..100 in the intel_backlight sysfs interface. In the code, this makes it more obvious that the sysfs max and the modulation frequency are not the same. It is more clear that we could change the modulation frequency on the fly (for example to increase frequency to reduce flickering), while we can't change the sysfs max on the fly. There are a few reasons for picking the fixed range 0..100. First, it's the range defined for Windows drivers in the Windows Hardware Compatibility Program Requirements. There is no reason to differ from that with some NIH agenda, especially when said requirements also tend to guide the choices made in hardware. Second, it's hard to produce more than that many user distinguishable brightness levels using PWM anyway. What is the point of supporting, say, 4882 brightness levels (the value on the laptop I'm writing this on) when you can't tell them apart? Also, we currently (and after this change too) expose a linear scale to the backlight PWM duty cycle. That is usually not the same as a linear scale of backlight luminance. It may be desirable to expose an API to allow the userspace to adjust that to a non-linear curve to reach a linear backlight luminance. It will be easier to do that with a fixed range. This change might cause bug reports due to assumptions that the scale never changes on a machine. However, the ABI is that the brightness is between 0..max_brightness. Not that the range cast in stone. Cc: Chris WilsonCc: Clint Taylor Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_panel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 7ab63486adce..c155777bd142 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1212,7 +1212,7 @@ static int intel_backlight_device_register(struct intel_connector *connector) * Note: Everything should work even if the backlight device max * presented to the userspace is arbitrarily chosen. */ - props.max_brightness = panel->backlight.max; + props.max_brightness = 100; props.brightness = scale_hw_to_user(connector, panel->backlight.level, props.max_brightness); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 1/2] drm/i915: preserve min brightness across backlight disable/enable
We try to preserve the backlight brightness across backlight disable/enable, except for minimum brightness. Currently, if the backlight is at minimum, we crank the backlight to max on enable. Preserve the minimum too, and turn the code into a sanity check instead. Cc: Chris WilsonCc: Clint Taylor Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_panel.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index a24df35e11e7..7ab63486adce 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1116,8 +1116,9 @@ void intel_panel_enable_backlight(struct intel_connector *connector) WARN_ON(panel->backlight.max == 0); - if (panel->backlight.level <= panel->backlight.min) { - panel->backlight.level = panel->backlight.max; + if (panel->backlight.level < panel->backlight.min) { + DRM_DEBUG_KMS("backlight level was below minimum\n"); + panel->backlight.level = panel->backlight.min; if (panel->backlight.device) panel->backlight.device->props.brightness = scale_hw_to_user(connector, -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Add Kabylake PCI IDs
On Wed, Nov 18, 2015 at 10:39:42AM -0800, Wayne Boyer wrote: > Add the Kabylake PCI IDs based on the following patches. > > commit d97044b661d0d56b2a2ae9b2b95ab0b359b417dc > Author: Deepak S> Date: Wed Oct 28 12:19:51 2015 -0700 > > drm/i915/kbl: Add Kabylake PCI ID > > commit 8b10c0cf21ec84618d4bf02c73c0543500ece68d > Author: Deepak S > Date: Wed Oct 28 12:21:12 2015 -0700 > > drm/i915/kbl: Add Kabylake GT4 PCI ID > > Cc: Rodrigo Vivi > Cc: Chris Wilson > Signed-off-by: Wayne Boyer > --- > src/i915_pciids.h | 36 > src/intel_module.c | 6 ++ > 2 files changed, 42 insertions(+) > > diff --git a/src/i915_pciids.h b/src/i915_pciids.h > index 17c4456..6374f58 100644 > --- a/src/i915_pciids.h > +++ b/src/i915_pciids.h > @@ -291,4 +291,40 @@ > INTEL_VGA_DEVICE(0x1A84, info), \ > INTEL_VGA_DEVICE(0x5A84, info) > > +#define INTEL_KBL_GT1_IDS(info) \ > + INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \ > + INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \ > + INTEL_VGA_DEVICE(0x5917, info), /* DT GT1.5 */ \ > + INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \ > + INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \ > + INTEL_VGA_DEVICE(0x5902, info), /* DT GT1 */ \ > + INTEL_VGA_DEVICE(0x590B, info), /* HALO GT1 */ \ > + INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */ > + > +#define INTEL_KBL_GT2_IDS(info) \ > + INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \ > + INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \ > + INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \ > + INTEL_VGA_DEVICE(0x5912, info), /* DT GT2 */ \ > + INTEL_VGA_DEVICE(0x591B, info), /* HALO GT2 */ \ > + INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \ > + INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */ > + > +#define INTEL_KBL_GT3_IDS(info) \ > + INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \ > + INTEL_VGA_DEVICE(0x592B, info), /* HALO GT3 */ \ > + INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */ > + > +#define INTEL_KBL_GT4_IDS(info) \ > + INTEL_VGA_DEVICE(0x5932, info), /* DT GT4 */ \ > + INTEL_VGA_DEVICE(0x593B, info), /* HALO GT4 */ \ > + INTEL_VGA_DEVICE(0x593A, info), /* SRV GT4 */ \ > + INTEL_VGA_DEVICE(0x593D, info) /* WKS GT4 */ > + > +#define INTEL_KBL_IDS(info) \ > + INTEL_KBL_GT1_IDS(info), \ > + INTEL_KBL_GT2_IDS(info), \ > + INTEL_KBL_GT3_IDS(info), \ > + INTEL_KBL_GT4_IDS(info) This is not an exact copy of the kernel i915_pciids.h Urm? -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] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
Hi, > > Another area of extension is how to expose a framebuffer to QEMU for > > seamless integration into a SPICE/VNC channel. For this I believe we > > could use a new region, much like we've done to expose VGA access > > through a vfio device file descriptor. An area within this new > > framebuffer region could be directly mappable in QEMU while a > > non-mappable page, at a standard location with standardized format, > > provides a description of framebuffer and potentially even a > > communication channel to synchronize framebuffer captures. This would > > be new code for QEMU, but something we could share among all vGPU > > implementations. > > Now GVT-g already provides an interface to decode framebuffer information, > w/ an assumption that the framebuffer will be further composited into > OpenGL APIs. Can I have a pointer to docs / code? iGVT-g_Setup_Guide.txt mentions a "Indirect Display Mode", but doesn't explain how the guest framebuffer can be accessed then. > So the format is defined according to OpenGL definition. > Does that meet SPICE requirement? Yes and no ;) Some more background: We basically have two rendering paths in qemu. The classic one, without opengl, and a new, still emerging one, using opengl and dma-bufs (gtk support merged for qemu 2.5, sdl2 support will land in 2.6, spice support still WIP, hopefully 2.6 too). For best performance you probably want use the new opengl-based rendering whenever possible. However I do *not* expect the classic rendering path disappear, we'll continue to need that in various cases, most prominent one being vnc support. So, for non-opengl rendering qemu needs the guest framebuffer data so it can feed it into the vnc server. The vfio framebuffer region is meant to support this use case. > Another thing to be added. Framebuffers are frequently switched in > reality. So either Qemu needs to poll or a notification mechanism is required. The idea is to have qemu poll (and adapt poll rate, i.e. without vnc client connected qemu will poll alot less frequently). > And since it's dynamic, having framebuffer page directly exposed in the > new region might be tricky. We can just expose framebuffer information > (including base, format, etc.) and let Qemu to map separately out of VFIO > interface. Allocate some memory, ask gpu to blit the guest framebuffer there, i.e. provide a snapshot of the current guest display instead of playing mapping tricks? > And... this works fine with vGPU model since software knows all the > detail about framebuffer. However in pass-through case, who do you expect > to provide that information? Is it OK to introduce vGPU specific APIs in > VFIO? It will only be used in the vgpu case, not for pass-though. We think it is better to extend the vfio interface to improve vgpu support rather than inventing something new while vfio can satisfy 90% of the vgpu needs already. We want avoid vendor-specific extensions though, the vgpu extension should work across vendors. > Now there is no standard. We expose vGPU life-cycle mgmt. APIs through > sysfs (under i915 node), which is very Intel specific. In reality different > vendors have quite different capabilities for their own vGPUs, so not sure > how standard we can define such a mechanism. Agree when it comes to create vGPU instances. > But this code should be > minor to be maintained in libvirt. As far I know libvirt only needs to discover those devices. If they look like sr/iov devices in sysfs this might work without any changes to libvirt. cheers, Gerd ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] Revert "drm/i915: skip modeset if compatible for everyone."
This reverts commit 6764e9f8724f1231b4deac53b9a82286ac0830e7 Author: Maarten LankhorstDate: Thu Aug 27 15:44:06 2015 +0200 drm/i915: skip modeset if compatible for everyone. Bring back the i915.fastboot module parameter, disabled by default, due to backlight regression on Chromebook Pixel 2015. Apparently the firmware of the Chromebook in question enables the panel but disables backlight to avoid a brief garbage scanout upon loading the kernel/module. With fastboot, we leave the backlight untouched, in this case disabled. The user would have to do a modeset (i.e. not just crank up the brightness) to enable the backlight. There is no clean fix readily available, so get back to the drawing board by reverting. [N.B. The reference below is for when the thread was included on public lists, and some of the context had already been dropped by then.] Reported-and-tested-by: Olof Johansson Cc: Maarten Lankhorst Cc: Daniel Vetter References: http://marc.info/?i=CAKMK7uES7xk05ki92oeX6gmvZWAh9f2vL7yz=6t+fgk9j3x...@mail.gmail.com Fixes: 6764e9f8724f ("drm/i915: skip modeset if compatible for everyone.") Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_params.c | 5 + drivers/gpu/drm/i915/intel_display.c | 3 ++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6db4e76dc7eb..a47e0f4fab56 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2656,6 +2656,7 @@ struct i915_params { int enable_cmd_parser; /* leave bools at the end to not create holes */ bool enable_hangcheck; + bool fastboot; bool prefault_disable; bool load_detect_test; bool reset; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 0f5af02e90e9..835d6099c769 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -41,6 +41,7 @@ struct i915_params i915 __read_mostly = { .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT), .disable_power_well = -1, .enable_ips = 1, + .fastboot = 0, .prefault_disable = 0, .load_detect_test = 0, .reset = true, @@ -139,6 +140,10 @@ MODULE_PARM_DESC(disable_power_well, module_param_named_unsafe(enable_ips, i915.enable_ips, int, 0600); MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)"); +module_param_named(fastboot, i915.fastboot, bool, 0600); +MODULE_PARM_DESC(fastboot, + "Try to skip unnecessary mode sets at boot time (default: false)"); + module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, 0600); MODULE_PARM_DESC(prefault_disable, "Disable page prefaulting for pread/pwrite/reloc (default:false). " diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9a1c1d48a7d8..46015af47c20 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13236,7 +13236,8 @@ static int intel_atomic_check(struct drm_device *dev, if (ret) return ret; - if (intel_pipe_config_compare(state->dev, + if (i915.fastboot && + intel_pipe_config_compare(state->dev, to_intel_crtc_state(crtc->state), pipe_config, true)) { crtc_state->mode_changed = false; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 maintainer-tools] dim: Replace git commit --amend from dim_apply with dimrc option
On Wed, 2015-11-18 at 16:52 +0100, Daniel Vetter wrote: > On Mon, Nov 16, 2015 at 12:17:15PM +0200, Ander Conselvan de Oliveira wrote: > > Introduce DIM_POST_APPLY_ACTION to dimrc that allows the user to specify > > a command to be run after a patch is applied. Use eval so enviroment > > variables can be overriden with the option. For example: > > > > DIM_POST_APPLY_ACTION="EDITOR=\"gedit -w\" git commit --amend" > > > > v2: Initialize variable with default value. > > Fix dimrc.sample to match default value. > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.olive...@intel.com> > > Reviewed-by: Daniel VetterThanks. Patch pushed. Ander > > > --- > > dim | 5 - > > dimrc.sample | 3 +++ > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/dim b/dim > > index db92c57..78b3f30 100755 > > --- a/dim > > +++ b/dim > > @@ -65,6 +65,9 @@ DIM_DRM_UPSTREAM_REMOTE=${DIM_DRM_UPSTREAM_REMOTE: > > -airlied} > > # usage: $DIM_MUA [-s subject] [-i file] [-c cc-addr] to-addr [...] > > DIM_MUA=${DIM_MUA:-mutt} > > > > +# command to run after dim apply > > +DIM_POST_APPLY_ACTION=${DIM_POST_APPLY_ACTION:-} > > + > > # greetings pull request template > > DIM_TEMPLATE_HELLO=${DIM_TEMPLATE_HELLO:-$HOME/.dim.template.hello} > > > > @@ -383,7 +386,7 @@ function dim_apply > > commit_add_tag "Link" " > > http://patchwork.freedesktop.org/patch/msgid/$message_id; > > fi > > > > - git commit --amend & > > + eval $DRY $DIM_POST_APPLY_ACTION > > } > > > > function magic_patch > > diff --git a/dimrc.sample b/dimrc.sample > > index 5687eaf..ad463b4 100644 > > --- a/dimrc.sample > > +++ b/dimrc.sample > > @@ -21,3 +21,6 @@ > > # Mail User Agent supporting a subset of mutt(1) command line options: > > # [-s subject] [-i file] [-c cc-addr] to-addr [...] > > #DIM_MUA=mutt > > + > > +# Command to run after dim apply > > +#DIM_POST_APPLY_ACTION= > > -- > > 2.4.3 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/1] drm/i915/audio: apply SKL codec wake up patch to BXT
From: "Lu, Han"Signed-off-by: Lu, Han diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 63d4706..8310bf3 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -591,7 +591,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev, struct drm_i915_private *dev_priv = dev_to_i915(dev); u32 tmp; - if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv)) + if (!IS_SKYLAKE(dev_priv) && !IS_BROXTON(dev_priv) && + !IS_KABYLAKE(dev_priv)) return; /* -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Also delay first activation for SKL+
Hi Rodrigo, On 19 November 2015 at 00:39, Rodrigo Viviwrote: > @@ -441,15 +438,14 @@ void intel_psr_enable(struct intel_dp *intel_dp) > /* > * FIXME: Activation should happen immediately since this function > * is just called after pipe is fully trained and enabled. > -* However on every platform we face issues when first activation > +* However on some platforms we face issues when first activation > * follows a modeset so quickly. > * - On VLV/CHV we get bank screen on first activation > * - On HSW/BDW we get a recoverable frozen screen until next > * exit-activate sequence. > */ > - if (INTEL_INFO(dev)->gen < 9) > - schedule_delayed_work(_priv->psr.work, > - > msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); > + schedule_delayed_work(_priv->psr.work, > + > msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); The comment change here seems to be the exact opposite of the code change: aren't we now seeing these issues on every platform? If not, it would be good to elaborate on the issues seen in SKL/BSW. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic-helper: Check encoder/crtc constraints
Hi, On 18 November 2015 at 17:46, Daniel Vetterwrote: > This was totally lost when I originally created the atomic helpers. > > We probably should also check possible_clones in the helpers, but > since the legacy ones didn't do that this is for a separate patch. Heh, before reading this chunk of the commit message, I also went looking for possible_clones and came to the same conclusion. On the grounds that great minds think alike (or fools never differ, not sure): Reviewed-by: Daniel Stone Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: skip modeset if compatible for everyone."
On Thu, 19 Nov 2015, Jani Nikulawrote: > This reverts > > commit 6764e9f8724f1231b4deac53b9a82286ac0830e7 > Author: Maarten Lankhorst > Date: Thu Aug 27 15:44:06 2015 +0200 > > drm/i915: skip modeset if compatible for everyone. > > Bring back the i915.fastboot module parameter, disabled by default, due > to backlight regression on Chromebook Pixel 2015. > > Apparently the firmware of the Chromebook in question enables the panel > but disables backlight to avoid a brief garbage scanout upon loading the > kernel/module. With fastboot, we leave the backlight untouched, in this > case disabled. The user would have to do a modeset (i.e. not just crank > up the brightness) to enable the backlight. > > There is no clean fix readily available, so get back to the drawing > board by reverting. > > [N.B. The reference below is for when the thread was included on public > lists, and some of the context had already been dropped by then.] > > Reported-and-tested-by: Olof Johansson > Cc: Maarten Lankhorst > Cc: Daniel Vetter > References: > http://marc.info/?i=CAKMK7uES7xk05ki92oeX6gmvZWAh9f2vL7yz=6t+fgk9j3x...@mail.gmail.com > Fixes: 6764e9f8724f ("drm/i915: skip modeset if compatible for everyone.") > Signed-off-by: Jani Nikula Pushed to drm-intel-fixes with IRC acks from Daniel and Maarten (*). I took the liberty of adding Tested-by from Olof, as he reported reverting the bad commit fixes the issue for him, and it's a clean revert. BR, Jani. (*) In truth Maarten said "sigh", I presumed short for Sighed-off-by, which I took to be close enough to Acked-by. > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_params.c | 5 + > drivers/gpu/drm/i915/intel_display.c | 3 ++- > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 6db4e76dc7eb..a47e0f4fab56 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2656,6 +2656,7 @@ struct i915_params { > int enable_cmd_parser; > /* leave bools at the end to not create holes */ > bool enable_hangcheck; > + bool fastboot; > bool prefault_disable; > bool load_detect_test; > bool reset; > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index 0f5af02e90e9..835d6099c769 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -41,6 +41,7 @@ struct i915_params i915 __read_mostly = { > .preliminary_hw_support = > IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT), > .disable_power_well = -1, > .enable_ips = 1, > + .fastboot = 0, > .prefault_disable = 0, > .load_detect_test = 0, > .reset = true, > @@ -139,6 +140,10 @@ MODULE_PARM_DESC(disable_power_well, > module_param_named_unsafe(enable_ips, i915.enable_ips, int, 0600); > MODULE_PARM_DESC(enable_ips, "Enable IPS (default: true)"); > > +module_param_named(fastboot, i915.fastboot, bool, 0600); > +MODULE_PARM_DESC(fastboot, > + "Try to skip unnecessary mode sets at boot time (default: false)"); > + > module_param_named_unsafe(prefault_disable, i915.prefault_disable, bool, > 0600); > MODULE_PARM_DESC(prefault_disable, > "Disable page prefaulting for pread/pwrite/reloc (default:false). " > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 9a1c1d48a7d8..46015af47c20 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13236,7 +13236,8 @@ static int intel_atomic_check(struct drm_device *dev, > if (ret) > return ret; > > - if (intel_pipe_config_compare(state->dev, > + if (i915.fastboot && > + intel_pipe_config_compare(state->dev, > to_intel_crtc_state(crtc->state), > pipe_config, true)) { > crtc_state->mode_changed = false; -- Jani Nikula, 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/1] drm/i915/audio: apply SKL codec wake up patch to BXT
On Thu, 19 Nov 2015, han...@intel.com wrote: > From: "Lu, Han"> > Signed-off-by: Lu, Han > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c > index 63d4706..8310bf3 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -591,7 +591,8 @@ static void > i915_audio_component_codec_wake_override(struct device *dev, > struct drm_i915_private *dev_priv = dev_to_i915(dev); > u32 tmp; > > - if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv)) > + if (!IS_SKYLAKE(dev_priv) && !IS_BROXTON(dev_priv) && > + !IS_KABYLAKE(dev_priv)) How about if (INTEL_INFO(dev_priv)->gen < 9)? BR, Jani. > return; > > /* -- Jani Nikula, 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 2/2] drm/i915: Serialise updates to GGTT with access through GGTT on Braswell
On Wed, Nov 18, 2015 at 03:08:47PM -0800, Jesse Barnes wrote: > On 11/17/2015 08:37 AM, Daniel Vetter wrote: > > On Fri, Oct 30, 2015 at 04:58:41PM +, Chris Wilson wrote: > >> On Fri, Oct 30, 2015 at 05:14:21PM +0100, Daniel Vetter wrote: > >>> On Fri, Oct 23, 2015 at 06:43:32PM +0100, Chris Wilson wrote: > When accessing through the GTT from one CPU whilst concurrently updating > the GGTT PTEs in another thread, the hardware likes to return random > data. As we have strong serialisation prevent us from modifying the PTE > of an active GTT mmapping, we have to conclude that it whilst modifying > other PTE's that error occurs. (I have not looked for any pattern such > as modifying PTE within the same page or cacheline as active PTE - > though checking whether revoking neighbouring objects should be enough > to test that theory.) The corruption also seems restricted to Braswell > and disappears with maxcpus=0. This patch stops all access through the > GTT by other CPUs when we update any PTE by stopping the machine around > the GGTT update. > > Testcase: igt/gem_concurrent_blit > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89079 > Signed-off-by: Chris Wilson> >>> > >>> Wild guess, since it wouldn't be the first time hw engineers screwed this > >>> up. > >>> > >>> Cheers, Daniel > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> index d1c5cf89fe77..de983c8e6e54 100644 > >>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>> @@ -2337,12 +2337,8 @@ int i915_gem_gtt_prepare_object(struct > >>> drm_i915_gem_object *obj) > >>> > >>> static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) > >>> { > >>> -#ifdef writeq > >>> - writeq(pte, addr); > >>> -#else > >>> iowrite32((u32)pte, addr); > >>> iowrite32(pte >> 32, addr + 4); > >>> -#endif > >> > >> Tried: > >> static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) > >> { > >> -#ifdef writeq > >> - writeq(pte, addr); > >> -#else > >> - iowrite32((u32)pte, addr); > >> - iowrite32(pte >> 32, addr + 4); > >> -#endif > >> + iowrite32(0, addr); > >> + wmb(); > >> + iowrite32(upper_32_bits(pte), addr + 4); > >> + iowrite32(lower_32_bits(pte), addr); > >> + wmb(); > >>} > >> > >> and just the plain iowrite(lower), iowrite(upper), neither helps. > > > > Added a note about this and applied to dinq. Yay for awesome hw. > > I thought Ville explained how this wasn't necessary? Ville can't repro, Chris claims it fixes something, I don't have a system. We probably should dig into this more, but since I didn't see anything going on I figured I can just pull it in for now. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Mark uneven memory banks on gen4 desktop as unknown swizzling
We have varied reports of swizzling corruption on gen4 desktop, and confirmation that one at least is triggered by uneven memory banks (L-shaped memory). The implication is that the swizzling varies between the paired channels and the remainder of memory on the single channel. As the object then has unpredictable swizzling (it will vary depending on exact page allocation and may even change during the object's lifetime as the pages are replaced), we have to report to userspace that the swizzling is unknown. However, some existing userspace is buggy when it meets an unknown swizzling configuration and so we need to tell another white lie and mark the swizzling as NONE but report it as UNKNOWN through the extended get-tiling-ioctl. See commit 5eb3e5a5e11d14f9deb2a4b83555443b69ab9940 Author: Chris WilsonDate: Sun Jun 28 09:19:26 2015 +0100 drm/i915: Declare the swizzling unknown for L-shaped configurations for the previous example where we found that telling the truth to userspace just ends up in a world of hurt. Also since we don't truly know what the swizzling is on the pages, we need to keep them pinned to prevent swapping as the reports also suggest that some gen4 devices have previously undetected bit17 swizzling. v2: Combine unknown + quirk patches to prevent userspace ever seeing unknown swizzling through the normal get-tiling-ioctl. Also use the same path for the existing uneven bank detection for mobile gen4. Reported-by: Matti Hämäläinen Tested--by: Matti Hämäläinen References: https://bugs.freedesktop.org/show_bug.cgi?id=90725 Signed-off-by: Chris Wilson Cc: Matti Hämäläinen Cc: Daniel Vetter Cc: Jani Nikula Cc: sta...@vger.kernel.org --- drivers/gpu/drm/i915/i915_gem_fence.c | 36 ++- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c index 40a10b25956c..f010391b87f5 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence.c +++ b/drivers/gpu/drm/i915/i915_gem_fence.c @@ -642,11 +642,10 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev) } /* check for L-shaped memory aka modified enhanced addressing */ - if (IS_GEN4(dev)) { - uint32_t ddc2 = I915_READ(DCC2); - - if (!(ddc2 & DCC2_MODIFIED_ENHANCED_DISABLE)) - dev_priv->quirks |= QUIRK_PIN_SWIZZLED_PAGES; + if (IS_GEN4(dev) && + !(I915_READ(DCC2) & DCC2_MODIFIED_ENHANCED_DISABLE)) { + swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN; + swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN; } if (dcc == 0x) { @@ -675,16 +674,35 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev) * matching, which was the case for the swizzling required in * the table above, or from the 1-ch value being less than * the minimum size of a rank. +* +* Reports indicate that the swizzling actually +* varies depending upon page placement inside the +* channels, i.e. we see swizzled pages where the +* banks of memory are paired and unswizzled on the +* uneven portion, so leave that as unknown. */ - if (I915_READ16(C0DRB3) != I915_READ16(C1DRB3)) { - swizzle_x = I915_BIT_6_SWIZZLE_NONE; - swizzle_y = I915_BIT_6_SWIZZLE_NONE; - } else { + if (I915_READ16(C0DRB3) == I915_READ16(C1DRB3)) { swizzle_x = I915_BIT_6_SWIZZLE_9_10; swizzle_y = I915_BIT_6_SWIZZLE_9; } } + if (swizzle_x == I915_BIT_6_SWIZZLE_UNKNOWN || + swizzle_y == I915_BIT_6_SWIZZLE_UNKNOWN) { + /* Userspace likes to explode if it sees unknown swizzling, +* so lie. We will finish the lie when reporting through +* the get-tiling-ioctl by reporting the physical swizzle +* mode as unknown instead. +* +* As we don't strictly know what the swizzling is, it may be +* bit17 dependent, and so we need to also prevent the pages +* from being moved. +*/ + dev_priv->quirks |= QUIRK_PIN_SWIZZLED_PAGES; + swizzle_x = I915_BIT_6_SWIZZLE_NONE; + swizzle_y = I915_BIT_6_SWIZZLE_NONE; + } + dev_priv->mm.bit_6_swizzle_x = swizzle_x; dev_priv->mm.bit_6_swizzle_y = swizzle_y; } -- 2.6.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Only spin whilst waiting on the current request
Hi, On 18/11/15 09:56, Chris Wilson wrote: Limit busywaiting only to the request currently being processed by the GPU. If the request is not currently being processed by the GPU, there is a very low likelihood of it being completed within the 2 microsecond spin timeout and so we will just be wasting CPU cycles. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 8 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8afda459a26e..16095b95d2df 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2190,7 +2190,7 @@ struct drm_i915_gem_request { struct intel_engine_cs *ring; /** GEM sequence number associated with this request. */ - uint32_t seqno; + uint32_t seqno, spin_seqno; Comment needs splitting out. And spin_seqno is not the best name, I think previous_ring_seqno would be better. So it would immediately tell you what it is, and then at the place which uses it it would also be clearer what is the criteria for spinning. /** Position in the ringbuffer of the start of the request */ u32 head; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 414150a0b8d5..af9ffa11ef44 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1193,9 +1193,14 @@ static int __i915_spin_request(struct drm_i915_gem_request *req, int state) * takes to sleep on a request, on the order of a microsecond. */ - if (i915_gem_request_get_ring(req)->irq_refcount) + if (req->ring->irq_refcount) return -EBUSY; + /* Only spin if we know the GPU is processing this request */ + if (i915_seqno_passed(req->ring->get_seqno(req->ring, false), + req->spin_seqno)) + return -EAGAIN; + timeout = local_clock_us() + 2; while (!need_resched()) { if (i915_gem_request_completed(req, true)) @@ -2592,6 +2597,7 @@ void __i915_add_request(struct drm_i915_gem_request *request, request->batch_obj = obj; request->emitted_jiffies = jiffies; + request->spin_seqno = ring->last_submitted_seqno; ring->last_submitted_seqno = request->seqno; list_add_tail(>list, >request_list); Commit message says it will spin only on the request currently being processed by the GPU but from here it looks like it will spin for any request _queued up_ before the last? For example if we have submitted 1, 2, 3 and 4 and GPU is currently processing 1. 2 has spin_seqno 1. 3 has spin_seqno 2. 4 has spin_seqno 3. ring->get_seqno is 0. Wait on 1: seqno_passed(0, 0) = true -> wait Wait on 2: seqno_passed(0, 1) = false -> spin Wait on 3: seqno_passed(0, 2) = false -> spin Wait on 4: seqno_passed(0, 3) = false -> spin So it looks the opposite. Or is it too early for me? :) Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic-helper: Check encoder/crtc constraints
On Wed, Nov 18, 2015 at 06:46:48PM +0100, Daniel Vetter wrote: > This was totally lost when I originally created the atomic helpers. > > We probably should also check possible_clones in the helpers, but > since the legacy ones didn't do that this is for a separate patch. > > Reported-by: Ville Syrjälä> Cc: Ville Syrjälä > Cc: Daniel Stone > Signed-off-by: Daniel Vetter Reviewed-by: Ville Syrjälä Tested-by: Ville Syrjälä But the rest of update_connector_routing() still looks somewhat bonkers to me. For one, it assumes that both the old and new crtc for the connector are part of the atomic state, but drm_atomic_set_crtc_for_connector() only adds the new crtc, not the old one. > --- > drivers/gpu/drm/drm_atomic_helper.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 0c6f62168776..cfdc9931b08a 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, > int conn_idx) > return -EINVAL; > } > > + if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { > + DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with > [CRTC:%d]\n", > + new_encoder->base.id, > + new_encoder->name, > + connector_state->crtc->base.id); > + return -EINVAL; > + } > + > if (new_encoder == connector_state->best_encoder) { > DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now > on [CRTC:%d]\n", >connector->base.id, > -- > 2.5.1 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Only spin whilst waiting on the current request
On Thu, Nov 19, 2015 at 10:05:39AM +, Tvrtko Ursulin wrote: > > Hi, > > On 18/11/15 09:56, Chris Wilson wrote: > >Limit busywaiting only to the request currently being processed by the > >GPU. If the request is not currently being processed by the GPU, there > >is a very low likelihood of it being completed within the 2 microsecond > >spin timeout and so we will just be wasting CPU cycles. > > > >Signed-off-by: Chris Wilson> >--- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/i915_gem.c | 8 +++- > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_drv.h > >b/drivers/gpu/drm/i915/i915_drv.h > >index 8afda459a26e..16095b95d2df 100644 > >--- a/drivers/gpu/drm/i915/i915_drv.h > >+++ b/drivers/gpu/drm/i915/i915_drv.h > >@@ -2190,7 +2190,7 @@ struct drm_i915_gem_request { > > struct intel_engine_cs *ring; > > > > /** GEM sequence number associated with this request. */ > >-uint32_t seqno; > >+uint32_t seqno, spin_seqno; > > Comment needs splitting out. Is it not the sequence associated with the request? The start of request activity, end of active marker? > And spin_seqno is not the best name, I think previous_ring_seqno > would be better. So it would immediately tell you what it is, and > then at the place which uses it it would also be clearer what is the > criteria for spinning. I agree, calling it spin_seqno was a mistake. I didn't like last_seqno either. Though now I think, u32 seqno_active, seqno_complete; and a i915_gem_request_active() helper to match i915_gem_request_completed(). > Commit message says it will spin only on the request currently being > processed by the GPU but from here it looks like it will spin for > any request _queued up_ before the last? Nope, my mistake. Thanks, -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/1] drm/i915/audio: apply SKL codec wake up patch to BXT
On Thu, 19 Nov 2015, Jani Nikulawrote: > On Thu, 19 Nov 2015, han...@intel.com wrote: >> From: "Lu, Han" >> >> Signed-off-by: Lu, Han >> >> diff --git a/drivers/gpu/drm/i915/intel_audio.c >> b/drivers/gpu/drm/i915/intel_audio.c >> index 63d4706..8310bf3 100644 >> --- a/drivers/gpu/drm/i915/intel_audio.c >> +++ b/drivers/gpu/drm/i915/intel_audio.c >> @@ -591,7 +591,8 @@ static void >> i915_audio_component_codec_wake_override(struct device *dev, >> struct drm_i915_private *dev_priv = dev_to_i915(dev); >> u32 tmp; >> >> -if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv)) >> +if (!IS_SKYLAKE(dev_priv) && !IS_BROXTON(dev_priv) && >> +!IS_KABYLAKE(dev_priv)) > > How about if (INTEL_INFO(dev_priv)->gen < 9)? Oh, and I guess we also wonder why this is only relevant for the latest platforms? > > BR, > Jani. > >> return; >> >> /* -- Jani Nikula, 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 2/2] drm/i915: Serialise updates to GGTT with access through GGTT on Braswell
On Thu, Nov 19, 2015 at 10:14:08AM +0100, Daniel Vetter wrote: > On Wed, Nov 18, 2015 at 03:08:47PM -0800, Jesse Barnes wrote: > > On 11/17/2015 08:37 AM, Daniel Vetter wrote: > > > On Fri, Oct 30, 2015 at 04:58:41PM +, Chris Wilson wrote: > > >> On Fri, Oct 30, 2015 at 05:14:21PM +0100, Daniel Vetter wrote: > > >>> On Fri, Oct 23, 2015 at 06:43:32PM +0100, Chris Wilson wrote: > > When accessing through the GTT from one CPU whilst concurrently > > updating > > the GGTT PTEs in another thread, the hardware likes to return random > > data. As we have strong serialisation prevent us from modifying the PTE > > of an active GTT mmapping, we have to conclude that it whilst modifying > > other PTE's that error occurs. (I have not looked for any pattern such > > as modifying PTE within the same page or cacheline as active PTE - > > though checking whether revoking neighbouring objects should be enough > > to test that theory.) The corruption also seems restricted to Braswell > > and disappears with maxcpus=0. This patch stops all access through the > > GTT by other CPUs when we update any PTE by stopping the machine around > > the GGTT update. > > > > Testcase: igt/gem_concurrent_blit > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89079 > > Signed-off-by: Chris Wilson> > >>> > > >>> Wild guess, since it wouldn't be the first time hw engineers screwed > > >>> this > > >>> up. > > >>> > > >>> Cheers, Daniel > > >>> > > >>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>> b/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>> index d1c5cf89fe77..de983c8e6e54 100644 > > >>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > >>> @@ -2337,12 +2337,8 @@ int i915_gem_gtt_prepare_object(struct > > >>> drm_i915_gem_object *obj) > > >>> > > >>> static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) > > >>> { > > >>> -#ifdef writeq > > >>> - writeq(pte, addr); > > >>> -#else > > >>> iowrite32((u32)pte, addr); > > >>> iowrite32(pte >> 32, addr + 4); > > >>> -#endif > > >> > > >> Tried: > > >> static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) > > >> { > > >> -#ifdef writeq > > >> - writeq(pte, addr); > > >> -#else > > >> - iowrite32((u32)pte, addr); > > >> - iowrite32(pte >> 32, addr + 4); > > >> -#endif > > >> + iowrite32(0, addr); > > >> + wmb(); > > >> + iowrite32(upper_32_bits(pte), addr + 4); > > >> + iowrite32(lower_32_bits(pte), addr); > > >> + wmb(); > > >>} > > >> > > >> and just the plain iowrite(lower), iowrite(upper), neither helps. > > > > > > Added a note about this and applied to dinq. Yay for awesome hw. > > > > I thought Ville explained how this wasn't necessary? > > Ville can't repro, Chris claims it fixes something, I don't have a > system. We probably should dig into this more, but since I didn't see > anything going on I figured I can just pull it in for now. Both myself, old QA (when they finally got around to running some of the coherency tests), new QA and VPG have reported coherency issues with access through the GGTT. -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 12/31] drm/i915: Fix PSR initialization.
On Wed, Nov 18, 2015 at 06:39:45PM +, Vivi, Rodrigo wrote: > On Wed, 2015-11-18 at 11:12 +0100, Daniel Vetter wrote: > > On Thu, Nov 05, 2015 at 10:50:04AM -0800, Rodrigo Vivi wrote: > > > PSR is still disabled by default, but even passing > > > i915.enable_psr=1 > > > at this point we weren't able to get PSR working because with > > > fastboot by default in place we weren't executing the path that > > > enables > > > encoder and consequently PSR. > > > > > > Now with psr_ready in place and PSR using crtc signature we can > > > move > > > its enable/disable sequences from the encoder enable to the post > > > atomic modeset functions. > > > > > > i915.enable_psr parameter is still used to enable/disable psr > > > feature > > > on the next primary plane update. So current test cases that relies > > > on this flow still works. > > > > > > Signed-off-by: Rodrigo Vivi> > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 2 -- > > > drivers/gpu/drm/i915/intel_display.c | 15 +++ > > > drivers/gpu/drm/i915/intel_dp.c | 5 - > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > 4 files changed, 17 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > index b8f8dee..36db970 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct > > > intel_encoder *intel_encoder) > > > intel_dp_stop_link_train(intel_dp); > > > > > > intel_edp_backlight_on(intel_dp); > > > - intel_psr_enable(intel_crtc); > > > intel_edp_drrs_enable(intel_dp); > > > } > > > > > > @@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct > > > intel_encoder *intel_encoder) > > > struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); > > > > > > intel_edp_drrs_disable(intel_dp); > > > - intel_psr_disable(intel_crtc); > > > intel_edp_backlight_off(intel_dp); > > > } > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index 869929d..f67e2ee 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct > > > intel_crtc *crtc) > > > if (atomic->enable_ips) > > > intel_ips_enable(crtc); > > > > > > + if (atomic->enable_psr) > > > + intel_psr_enable(crtc); > > > > What we need here is a post-modeset fixup hook for > > encoders/connectors. > > That would avoid the layering violation of going through a crtc and > > then > > doing the crtc->encoder lookup you do in the previous patch. And we > > have > > other uses for this, e.g. mipi self refresh, fixing up infoframes and > > all > > those things. > > I see your point but I'm not sure if I totally got your idea. > > So would it be a generic hook when we detect encoder/connector being > enabled/disabled? > > and how to determine that? could you please elaborate your idea a bit > more? diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c3bc6ab77508..427daac480f5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13273,6 +13273,11 @@ static int intel_atomic_commit(struct drm_device *dev, modeset_put_power_domains(dev_priv, put_domains); intel_post_plane_update(intel_crtc); + + if (needs_modeset || update_pipe) { + for_each_encoder_on_crtc(dev, crtc, encoder) + encoder->enable_fixup(encoder); + } } /* FIXME: add subpixel order */ With this enable_fixup (maybe we should call it post_plane_enable) will will get called both when we do a full modeset, but also when we just do a fast modeset/pipe update. Then we can move everything that we're currently doing in ->enable which also must be done for fastboot into ->post_plane_enable, like infoframes, DRRS, psr or whatever. The only tricky part is that the core won't do the book-keeping for us anymore, i.e. we need to check ourselves in this new hook whether we need to do the setup or not, since if userspace only changes the scaling a lot we might get a lot of ->post_plane_enable calls without any ->disable calls in between. So that's also where you have to put your checks: intel_dp_post_plane_enable() { if (!psr_enabled) enable_psr(); if (!DRRS_on) enable_drrs(); /* maybe in the future */ if (!audio_enabled) enable_audio(); } intel_hdmi_post_plane_enable() { /* ifxup infoframes if they're crap from the bios */ } Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation
Re: [Intel-gfx] [PATCH i-g-t] Add dmesg capture and dumping to tests and a test for it.
On Thu, Nov 19, 2015 at 10:38 AM, Daniel Vetterwrote: > On Wed, Nov 18, 2015 at 05:32:59PM +, Chris Wilson wrote: >> On Wed, Nov 18, 2015 at 04:44:20PM +0100, Daniel Vetter wrote: >> > On Mon, Nov 16, 2015 at 03:22:23PM +0200, Joonas Lahtinen wrote: >> > > Cc: Thomas Wood >> > > Cc: Chris Wilson >> > > Cc: Damien Lespiau >> > > Signed-off-by: Joonas Lahtinen >> > >> > Given that we have all that in piglit already the commit message is a bit >> > thin on justification. Why do we need this in igt too? How does this >> > interact with the piglit dmesg capture? >> >> It's doesn't interfere with anyone else parsing kmsg/dmesg for >> themselves, but it adds very useful functionality to standalone igt. >> Which to me is significantly more valuable and I have been patching it >> into igt for over a year and wished it was taken more seriously given >> the number of incorrect bug reports generated. > > Ah, the "It doesn't interfere ..." is the crucial part I missed, I didn't > know you could read dmesg in parallel without eating message for other > consumers. Jonaas, with the above used as commit message (or something > similar) this is Acked-by: Daniel Vetter Ok, I need to retract this. piglit does some dmesg filtering, how do we make sure these two definitions of what's considered failing dmesg noise match up? -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: Use dpcd read wake for sink crc calls.
On Wed, Nov 18, 2015 at 06:31:05PM +, Vivi, Rodrigo wrote: > On Tue, 2015-11-17 at 15:08 +0100, Daniel Vetter wrote: > > On Mon, Nov 16, 2015 at 04:05:42PM +, Rodrigo Vivi wrote: > > > Ok, so after trying it we saw that we really cannot trust on aux > > > mutex.At > > > least not on all SKL/KBL > > > It worked in a KBL but failed on a SKL that I have here... > > > > > > So without aux mutex option we still need to get sink_crc more > > > reliable and > > > I see only 2 quick ways here: > > > - This read wake > > > - Return -EBUSY to force the drm retries on message size = 0. > > > > > > Daniel, what do you believe? > > > > It's still a mess. My opinion is still that we should move the hacks > > from > > read_wake into a more suitable place: > > a) either into drm_dp_dpcd_read in drm_dp_helper.c > > Well, but drm_dp_helper already does many retries already (32 times) > but only on EBUSY. My idea is that we should consider that and return > EBUSY instead of adding another retry case at drm. > > > > b) or into intel_dp_aux_transfer in intel_dp.c > > Oh, I thought you had nacked this option. > > > > > Option a) is the right one if this is a generic sink issue (and it > > seems > > to be the case, at least for edp panels). Option b) if it's an issue > > with > > our hw. Either way I think intel_dp_dpcd_read_wake should die. > > Well, Jani and Ville kind of nacked this while we don't understand why > this was ever introduced at first place. > Although I believe with the proper EBUSY returns in place and 32 times > retry I believe we could safely remove this as I tried on that series. > > > > > On a personal gut level I'd go with option a). > > So, EBUSY is ok or should we create another case on drm level? Well, what I'd really like is to get rid of our read_wake code, since pretty obviously it's a hack we seem to need everywhere. If the EBUSY trick will allow us to do that I'm all for it. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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 v3] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed
On Wed, Nov 18, 2015 at 05:18:30PM +, Tvrtko Ursulin wrote: > > On 17/11/15 17:56, Daniel Vetter wrote: > > On Tue, Nov 17, 2015 at 05:24:01PM +, Tvrtko Ursulin wrote: > >> > >> On 17/11/15 17:08, Daniel Vetter wrote: > >>> On Tue, Nov 17, 2015 at 04:54:50PM +, Tvrtko Ursulin wrote: > > On 17/11/15 16:39, Daniel Vetter wrote: > > On Tue, Nov 17, 2015 at 04:27:12PM +, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin> >> > >> In the following commit: > >> > >> commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae > >> Author: Tvrtko Ursulin > >> Date: Mon Oct 5 13:26:36 2015 +0100 > >> > >> drm/i915: Clean up associated VMAs on context destruction > >> > >> I added a WARN_ON assertion that VM's active list must be empty > >> at the time of owning context is getting freed, but that turned > >> out to be a wrong assumption. > >> > >> Due ordering of operations in i915_gem_object_retire__read, where > >> contexts are unreferenced before VMAs are moved to the inactive > >> list, the described situation can in fact happen. > >> > >> It feels wrong to do things in such order so this fix makes sure > >> a reference to context is held until the move to inactive list > >> is completed. > >> > >> v2: Rather than hold a temporary context reference move the > >> request unreference to be the last operation. (Daniel Vetter) > >> > >> v3: Fix use after free. (Chris Wilson) > >> > >> Signed-off-by: Tvrtko Ursulin > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 > >> Cc: Michel Thierry > >> Cc: Chris Wilson > >> Cc: Daniel Vetter > >> --- > >> drivers/gpu/drm/i915/i915_gem.c | 33 > >> ++--- > >> 1 file changed, 18 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gem.c > >> b/drivers/gpu/drm/i915/i915_gem.c > >> index 98c83286ab68..094ac17a712d 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> @@ -2404,29 +2404,32 @@ i915_gem_object_retire__read(struct > >> drm_i915_gem_object *obj, int ring) > >>RQ_BUG_ON(!(obj->active & (1 << ring))); > >> > >>list_del_init(>ring_list[ring]); > >> - i915_gem_request_assign(>last_read_req[ring], NULL); > >> > >>if (obj->last_write_req && obj->last_write_req->ring->id == > >> ring) > >>i915_gem_object_retire__write(obj); > >> > >>obj->active &= ~(1 << ring); > >> - if (obj->active) > >> - return; > > > > if (obj->active) { > > i915_gem_request_assign(>last_read_req[ring], > > NULL); > > return; > > } > > > > Would result in less churn in the code and drop the unecessary indent > > level. Also comment is missing as to why we need to do things in a > > specific order. > > Actually I think I changed my mind and that v1 is the way to go. > > Just re-ordering the code here still makes it possible for the context > destructor to run with VMAs on the active list I think. > > If we hold the context then it is 100% clear it is not possible. > >>> > >>> request_assign _is_ the function which adjust the refcounts for us, which > >>> means if we drop that reference too early then grabbing a temp reference > >>> is just papering over the real bug. > >>> > >>> Written out your patch looks something like > >>> > >>> a_reference(a); > >>> a_unreference(a); > >>> > >>> /* more cleanup code that should get run before a_unreference but isn't > >>> */ > >>> > >>> a_unrefernce(a); /* for real this time */ > >>> > >>> Unfortunately foo_assign is a new pattern and not well-established, so > >>> that connection isn't clear. Maybe we should rename it to > >>> foo_reference_assign to make it more obvious. Or just drop the pretense > >>> and open-code it since we unconditionally assign NULL as the new pointer > >>> value, and we know the current value of the pointer is non-NULL. So > >>> there's really no benefit to the helper here, it only obfuscates. And > >>> since that obfuscation tripped you up it's time to remove it ;-) > >> > >> Then foo_reference_unreference_assign. :) > >> > >> But seriously, I think it is more complicated that.. > >> > >> The thing it trips over is that moving VMAs to inactive does not correspond > >> in time to request retirement. But in fact VMAs are moved to inactive only > >> when all requests associated with an object are done. > >> > >> This is the unintuitive thing I was working around. To make sure when > >> context
Re: [Intel-gfx] [PATCH 10/31] drm/i915: Detatch i915.enable_psr from psr_ready
On Wed, Nov 18, 2015 at 06:35:15PM +, Vivi, Rodrigo wrote: > On Wed, 2015-11-18 at 11:07 +0100, Daniel Vetter wrote: > > On Thu, Nov 05, 2015 at 10:50:02AM -0800, Rodrigo Vivi wrote: > > > PSR will be enabled on every post primary update when it is > > > ready and parameter allows. > > > With this we allow test cases to continue using this parameter > > > for enabling disabling the feature. > > > > > > Signed-off-by: Rodrigo Vivi> > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 2 +- > > > drivers/gpu/drm/i915/intel_psr.c | 5 - > > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 6ab127c..e154a2e 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -5334,7 +5334,7 @@ void intel_edp_drrs_enable(struct intel_dp > > > *intel_dp) > > > return; > > > } > > > > > > - if (intel_crtc->config->psr_ready) { > > > + if (intel_crtc->config->psr_ready && i915.enable_psr) { > > > > I don't get this change ... The pipe config should take into account > > enable_psr already. If we allow testcases to change this without a > > modeset, this could result in a lot of racy fun. > > Yes, I agree, however igt test cases uses i915.enable_psr to > enable/disable psr so I'm afraid we will loose this if we add it to > psr_ready. Can't the testcase force a full modeset to update psr state? That should be enough to force-enable psr for testing and then disable it again. And for a full modeset we need to reevaluate psr anyway, so this won't require any special code. full modeset = setCrtc(NULL); setCrtc(mode); -Daniel > > > -Daniel > > > > > DRM_DEBUG_KMS("DRRS: PSR will be enabled on this > > > crtc\n"); > > > return; > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 4a9d620..e690db3 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -314,11 +314,6 @@ bool intel_psr_ready(struct intel_dp > > > *intel_dp, > > > return false; > > > } > > > > > > - if (!i915.enable_psr) { > > > - DRM_DEBUG_KMS("PSR disable by flag\n"); > > > - return false; > > > - } > > > - > > > if (IS_HASWELL(dev) && > > > I915_READ(HSW_STEREO_3D_CTL(pipe_config > > > ->cpu_transcoder)) & > > > S3D_ENABLE) { > > > -- > > > 2.4.3 > > > > > > ___ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Daniel Vetter Software Engineer, Intel Corporation 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 i-g-t] Add dmesg capture and dumping to tests and a test for it.
On Wed, Nov 18, 2015 at 05:32:59PM +, Chris Wilson wrote: > On Wed, Nov 18, 2015 at 04:44:20PM +0100, Daniel Vetter wrote: > > On Mon, Nov 16, 2015 at 03:22:23PM +0200, Joonas Lahtinen wrote: > > > Cc: Thomas Wood> > > Cc: Chris Wilson > > > Cc: Damien Lespiau > > > Signed-off-by: Joonas Lahtinen > > > > Given that we have all that in piglit already the commit message is a bit > > thin on justification. Why do we need this in igt too? How does this > > interact with the piglit dmesg capture? > > It's doesn't interfere with anyone else parsing kmsg/dmesg for > themselves, but it adds very useful functionality to standalone igt. > Which to me is significantly more valuable and I have been patching it > into igt for over a year and wished it was taken more seriously given > the number of incorrect bug reports generated. Ah, the "It doesn't interfere ..." is the crucial part I missed, I didn't know you could read dmesg in parallel without eating message for other consumers. Jonaas, with the above used as commit message (or something similar) this is Acked-by: Daniel Vetter Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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 v3] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed
On 19/11/15 09:17, Daniel Vetter wrote: On Wed, Nov 18, 2015 at 05:18:30PM +, Tvrtko Ursulin wrote: On 17/11/15 17:56, Daniel Vetter wrote: On Tue, Nov 17, 2015 at 05:24:01PM +, Tvrtko Ursulin wrote: On 17/11/15 17:08, Daniel Vetter wrote: On Tue, Nov 17, 2015 at 04:54:50PM +, Tvrtko Ursulin wrote: On 17/11/15 16:39, Daniel Vetter wrote: On Tue, Nov 17, 2015 at 04:27:12PM +, Tvrtko Ursulin wrote: From: Tvrtko UrsulinIn the following commit: commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae Author: Tvrtko Ursulin Date: Mon Oct 5 13:26:36 2015 +0100 drm/i915: Clean up associated VMAs on context destruction I added a WARN_ON assertion that VM's active list must be empty at the time of owning context is getting freed, but that turned out to be a wrong assumption. Due ordering of operations in i915_gem_object_retire__read, where contexts are unreferenced before VMAs are moved to the inactive list, the described situation can in fact happen. It feels wrong to do things in such order so this fix makes sure a reference to context is held until the move to inactive list is completed. v2: Rather than hold a temporary context reference move the request unreference to be the last operation. (Daniel Vetter) v3: Fix use after free. (Chris Wilson) Signed-off-by: Tvrtko Ursulin Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 Cc: Michel Thierry Cc: Chris Wilson Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem.c | 33 ++--- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 98c83286ab68..094ac17a712d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2404,29 +2404,32 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) RQ_BUG_ON(!(obj->active & (1 << ring))); list_del_init(>ring_list[ring]); - i915_gem_request_assign(>last_read_req[ring], NULL); if (obj->last_write_req && obj->last_write_req->ring->id == ring) i915_gem_object_retire__write(obj); obj->active &= ~(1 << ring); - if (obj->active) - return; if (obj->active) { i915_gem_request_assign(>last_read_req[ring], NULL); return; } Would result in less churn in the code and drop the unecessary indent level. Also comment is missing as to why we need to do things in a specific order. Actually I think I changed my mind and that v1 is the way to go. Just re-ordering the code here still makes it possible for the context destructor to run with VMAs on the active list I think. If we hold the context then it is 100% clear it is not possible. request_assign _is_ the function which adjust the refcounts for us, which means if we drop that reference too early then grabbing a temp reference is just papering over the real bug. Written out your patch looks something like a_reference(a); a_unreference(a); /* more cleanup code that should get run before a_unreference but isn't */ a_unrefernce(a); /* for real this time */ Unfortunately foo_assign is a new pattern and not well-established, so that connection isn't clear. Maybe we should rename it to foo_reference_assign to make it more obvious. Or just drop the pretense and open-code it since we unconditionally assign NULL as the new pointer value, and we know the current value of the pointer is non-NULL. So there's really no benefit to the helper here, it only obfuscates. And since that obfuscation tripped you up it's time to remove it ;-) Then foo_reference_unreference_assign. :) But seriously, I think it is more complicated that.. The thing it trips over is that moving VMAs to inactive does not correspond in time to request retirement. But in fact VMAs are moved to inactive only when all requests associated with an object are done. This is the unintuitive thing I was working around. To make sure when context destructor runs there are not active VMAs for that VM. I don't know how to guarantee that with what you propose. Perhaps I am missing something? Ok, my example was slightly off, since we have 2 objects: b_reference(a->b); a_unreference(a); /* might unref a->b if it's the last reference */ /* more cleanup code that should get run before a_unreference but isn't */ b_unrefernce(a->b); /* for real this time */ Holding the ref to a makes sure that b doesn't disappear. We rely on that in a fundamental way (a request really needs the ctx to stick around), and the bug really is that we drop the ref to a too early. That it's the releasing of a->b which is eventually blowing things up doesn't really matter. Btw
Re: [Intel-gfx] [PATCH] drm/i915: Allow unready gpu to be reset on gen8
Daniel Vetterwrites: > On Mon, Nov 02, 2015 at 11:25:08AM +0200, Mika Kuoppala wrote: >> Gen9 has had demonstrated cases where forcing a not ready gpu >> into reset has caused system hang [1]. >> >> Gen8 has never to this date demonstrated such behaviour. >> >> In our CI tests there have been two cases of bsw ending in a state >> where it claims it is not ready for reset, based on reset request, >> after gpu hang [2]. Both of these cases have happened with kernel >> where there are lots of debugs enabled. So it is possible that >> something timing related is at play here like that wait_for_register() >> usleep wakeups collide badly with forcewake. >> >> If we assume that gen8 is safe to reset even with claims of nonreadiness, >> we can alleviate this situations by forcing a reset and revive the gpu. >> Enhance logging so that it will be clear what conditions led to decision >> of proceeding or bailing out, so that we will spot if this way of forcing >> our will against gpu turns out to be foolhardy. >> >> v2: - add bugzilla entry for bsw behaviour (Chris) >> - improve commit message >> >> References [1]: https://bugs.freedesktop.org/show_bug.cgi?id=89959 >> References [2]: https://bugs.freedesktop.org/show_bug.cgi?id=92774 >> Cc: Daniel Vetter >> Cc: Tomi Sarvela >> Cc: Chris Wilson >> Signed-off-by: Mika Kuoppala >> --- >> drivers/gpu/drm/i915/intel_uncore.c | 9 - >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c >> b/drivers/gpu/drm/i915/intel_uncore.c >> index f0f97b2..47c17f2 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1504,7 +1504,14 @@ not_ready: >> I915_WRITE(RING_RESET_CTL(engine->mmio_base), >> _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET)); >> >> -return -EIO; >> +if (INTEL_INFO(dev)->gen == 9) { >> +DRM_ERROR("Reset would risk system stability, bailing out\n"); >> +return -EIO; >> +} >> + >> +DRM_ERROR("Forcing non ready gpu into reset\n"); > > DRM_ERROR will cause dmes-warn failures when running igt. If we agree that > this is ok, it should be at most DRM_INFO. I'd got with DRM_DEBUG_DRIVER. > Ack with that change. This is in the category 'shouldn't never happen' so I would prefer igt to complain loudly if it can't request reset. The real culprit was forcewakes missing on reset path, so this patch can be forgotten and only resurrected if, even with forcewake fix in, gen8 fails to request reset. Here is the patch that fixed the underlying issue: commit 99106bc17e667989b4c0af0a6afcbd6ddbada8fb Author: Mika Kuoppala Date: Thu Nov 5 13:11:38 2015 +0200 drm/i915: Do graphics device reset under forcewake Thanks, -Mika > -Daniel > >> + >> +return gen6_do_reset(dev); >> } >> >> static int (*intel_get_gpu_reset(struct drm_device *dev))(struct drm_device >> *) >> -- >> 2.5.0 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > 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/9] drm/i915: Opt out of vblank disable timer on >gen2
2014-05-26 11:26 GMT-03:00: > From: Ville Syrjälä > > Now that the vblank races are plugged, we can opt out of using > the vblank disable timer and just let vblank interrupts get > disabled immediately when the last reference is dropped. > > Gen2 is the exception since it has no hardware frame counter. Hi Remember last week's FBC vblank optimization patch that had an erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? After I fixed the bug in the patch I realized that it was the unbalanced vblank_get() call that moved PC state residency up. I did some experiments, and on my specific BDW machine, after running "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 residency goes from 60-70% to 85-90% when I revert this patch. I'm running just an idle Cinnamon with an open terminal. So, since the commit message lacks more details, what are the downsides of reverting this patch? What are the advantages of opting out of the vblank timer? I see my desktop does tons and tons of vblank get/put calls per second, so the disable timer makes a lot of sense. I also wish there was some easy way to check how this patch (or its revert) affect a bunch of different workloads... (Also CCing Chris for insightful comments on performance) Thanks, Paulo > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_irq.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 28bae6e..4b2e7af 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) > dev->max_vblank_count = 0xff; /* only 24 bits of frame > count */ > } > > + /* > +* Opt out of the vblank disable timer on everything except gen2. > +* Gen2 doesn't have a hardware frame counter and so depends on > +* vblank interrupts to produce sane vblank seuquence numbers. > +*/ > + if (!IS_GEN2(dev)) > + dev->vblank_disable_immediate = true; > + > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; > dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > -- > 1.8.5.5 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle
On to, 2015-11-19 at 19:00 +0100, Patrik Jakobsson wrote: > On Thu, Nov 19, 2015 at 04:06:47PM +0200, Imre Deak wrote: > > On to, 2015-11-19 at 14:34 +0100, Patrik Jakobsson wrote: > > > On Wed, Nov 18, 2015 at 06:44:43PM +0200, Imre Deak wrote: > > > > On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > > > > > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > > > > > During suspend-to-idle we need to keep the DMC firmware active and > > > > > > DC6 > > > > > > enabled, since otherwise we won't reach deep system power states > > > > > > like > > > > > > PC9/10. The lead for this came from Nivedita who noticed that the > > > > > > kernel's turbostat tool didn't report any PC9/10 residency change > > > > > > across an 'echo freeze > /sys/power/state'. > > > > > > > > > > > > Reported-by: Nivedita Swaminathan > > > > > > Signed-off-by: Imre Deak> > > > > > --- > > > > > > drivers/gpu/drm/i915/i915_drv.c | 44 > > > > > > +++-- > > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > > > index 6344dfb..649e20a 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct > > > > > > drm_i915_private *dev_priv, > > > > > > bool rpm_resume); > > > > > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > > > > > > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > > > > > +{ > > > > > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > > + return true; > > > > > > +#endif > > > > > > + return false; > > > > > > +} > > > > > > > > > > > > static int i915_drm_suspend(struct drm_device *dev) > > > > > > { > > > > > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device > > > > > > *dev) > > > > > > > > > > > > i915_save_state(dev); > > > > > > > > > > > > - opregion_target_state = PCI_D3cold; > > > > > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > > - opregion_target_state = PCI_D1; > > > > > > -#endif > > > > > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : > > > > > > PCI_D3cold; > > > > > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > > > > > > > > > intel_uncore_forcewake_reset(dev, false); > > > > > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device > > > > > > *dev) > > > > > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool > > > > > > hibernation) > > > > > > { > > > > > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > > > > > + bool fw_csr; > > > > > > int ret; > > > > > > > > > > > > - intel_power_domains_suspend(dev_priv); > > > > > > + fw_csr = suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload; > > > > > > + /* > > > > > > + * In case of firmware assisted context save/restore don't > > > > > > manually > > > > > > + * deinit the power domains. This also means the CSR/DMC > > > > > > firmware will > > > > > > + * stay active, it will power down any HW resources as required > > > > > > and > > > > > > + * also enable deeper system power states that would be blocked > > > > > > if the > > > > > > + * firmware was inactive. > > > > > > + */ > > > > > > + if (!fw_csr) > > > > > > + intel_power_domains_suspend(dev_priv); > > > > > > > > > > > > ret = intel_suspend_complete(dev_priv); > > > > > > > > > > > > if (ret) { > > > > > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > > > + if (!fw_csr) > > > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > > > > > return ret; > > > > > > } > > > > > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct > > > > > > drm_device *drm_dev, bool hibernation) > > > > > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > > > > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > > > > > > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct > > > > > > drm_device *dev) > > > > > > * FIXME: This should be solved with a special hdmi sink device > > > > > > or > > > > > > * similar so that power domains can be employed. > > > > > > */ > > > > > > - if (pci_enable_device(dev->pdev)) > > > > > > - return -EIO; >
Re: [Intel-gfx] [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status
On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote: > There are platforms that don't need the full GMBUS power domain > (PCH, BXT) while others do (VLV/CHV). For optimizing this we > would need to add a new power domain, but it's not clear how much we > would benefit given the short time we hold the reference. So for now > let's keep things simple. Actually on PCH platforms the gmbus domain means just just an rpm ref since the gmbus hw lives in the PCH. And IIRC on BXT gmbus lives in pw0 so same deal really. And for vlv/chv we should just need the disp2d well, which is exactly what we get with the gmbus domain. So I don't think there's actually anything to optimize here with current platforms. Both patches look fine to me: Reviewed-by: Ville Syrjälä> > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/intel_hdmi.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > index 17ced03..bdd462e 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool > force) > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > > + intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > + > while (!live_status && --retry) { > live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool > force) > } else > status = connector_status_disconnected; > > + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > + > return status; > } > > -- > 2.5.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+
On Wed, Nov 18, 2015 at 11:45 PM, Jindal, Sonikawrote: > Hi Rodrigo, > > Which platform have you observed this issue on? Skylake and Kabylake > This looks really strange. It is expected actually. On DC5/6 states all read-only registers are reset and cannot be restored. including this counter. > Have you checked whether we are able to enter PSR at sink side or not in such > cases? > Are we sure we are not entering PSR? I mean the PSR_STATE register says it > correctly? I'm sure PSR is working well in entry state with links off and Perf counter goes to zero when DC5/6 states enters. So this patch just removes the counter to avoid confusion. People would think PSR isn't work, when it is. > > Regards, > Sonika > > -Original Message- > From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of > R, Durgadoss > Sent: Thursday, November 19, 2015 11:39 AM > To: Vivi, Rodrigo; intel-gfx@lists.freedesktop.org > Cc: Vivi, Rodrigo > Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for > SKL+ > > > >>-Original Message- >>From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On >>Behalf Of Rodrigo Vivi >>Sent: Thursday, November 19, 2015 6:10 AM >>To: intel-gfx@lists.freedesktop.org >>Cc: Vivi, Rodrigo >>Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for >>SKL+ >> >>Whenever DMC firmware put the HW into DC State a bunch of registers >>including this perf counter is reset to 0 and never restored. >> >>So, even with PSR active and working we could still read >>"Performance_Counter: 0" what will misslead people to believe PSR is >>broken. >> >>So, it is better to remove this counter information while we don't have >>a better way to track PSR residency. > > Agreed.. > > Reviewed-by: Durgadoss R > > Thanks, > Durga > >> >>Signed-off-by: Rodrigo Vivi >>--- >> drivers/gpu/drm/i915/i915_debugfs.c | 7 +-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>b/drivers/gpu/drm/i915/i915_debugfs.c >>index 038d5c6..71e1666 100644 >>--- a/drivers/gpu/drm/i915/i915_debugfs.c >>+++ b/drivers/gpu/drm/i915/i915_debugfs.c >>@@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct seq_file *m, >>void *data) >> } >> seq_puts(m, "\n"); >> >>- /* CHV PSR has no kind of performance counter */ >>- if (HAS_DDI(dev)) { >>+ /* >>+ * VLV/CHV PSR has no kind of performance counter >>+ * SKL+ Perf counter is reset to 0 everytime DC state is entered >>+ */ >>+ if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { >> psrperf = I915_READ(EDP_PSR_PERF_CNT) & >> EDP_PSR_PERF_CNT_MASK; >> >>-- >>2.4.3 >> >>___ >>Intel-gfx mailing list >>Intel-gfx@lists.freedesktop.org >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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
[Intel-gfx] [PATCH 1/2] drm/i915/pm: Unconstify power_domain_str
Let us print human-parseable values from the power domain code; upcoming display code also wants to use it. Signed-off-by: Daniel Stone--- drivers/gpu/drm/i915/i915_debugfs.c | 67 + drivers/gpu/drm/i915/i915_drv.h | 66 2 files changed, 67 insertions(+), 66 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 411a9c6..b28da6f 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2685,71 +2685,6 @@ static int i915_runtime_pm_status(struct seq_file *m, void *unused) return 0; } -static const char *power_domain_str(enum intel_display_power_domain domain) -{ - switch (domain) { - case POWER_DOMAIN_PIPE_A: - return "PIPE_A"; - case POWER_DOMAIN_PIPE_B: - return "PIPE_B"; - case POWER_DOMAIN_PIPE_C: - return "PIPE_C"; - case POWER_DOMAIN_PIPE_A_PANEL_FITTER: - return "PIPE_A_PANEL_FITTER"; - case POWER_DOMAIN_PIPE_B_PANEL_FITTER: - return "PIPE_B_PANEL_FITTER"; - case POWER_DOMAIN_PIPE_C_PANEL_FITTER: - return "PIPE_C_PANEL_FITTER"; - case POWER_DOMAIN_TRANSCODER_A: - return "TRANSCODER_A"; - case POWER_DOMAIN_TRANSCODER_B: - return "TRANSCODER_B"; - case POWER_DOMAIN_TRANSCODER_C: - return "TRANSCODER_C"; - case POWER_DOMAIN_TRANSCODER_EDP: - return "TRANSCODER_EDP"; - case POWER_DOMAIN_PORT_DDI_A_LANES: - return "PORT_DDI_A_LANES"; - case POWER_DOMAIN_PORT_DDI_B_LANES: - return "PORT_DDI_B_LANES"; - case POWER_DOMAIN_PORT_DDI_C_LANES: - return "PORT_DDI_C_LANES"; - case POWER_DOMAIN_PORT_DDI_D_LANES: - return "PORT_DDI_D_LANES"; - case POWER_DOMAIN_PORT_DDI_E_LANES: - return "PORT_DDI_E_LANES"; - case POWER_DOMAIN_PORT_DSI: - return "PORT_DSI"; - case POWER_DOMAIN_PORT_CRT: - return "PORT_CRT"; - case POWER_DOMAIN_PORT_OTHER: - return "PORT_OTHER"; - case POWER_DOMAIN_VGA: - return "VGA"; - case POWER_DOMAIN_AUDIO: - return "AUDIO"; - case POWER_DOMAIN_PLLS: - return "PLLS"; - case POWER_DOMAIN_AUX_A: - return "AUX_A"; - case POWER_DOMAIN_AUX_B: - return "AUX_B"; - case POWER_DOMAIN_AUX_C: - return "AUX_C"; - case POWER_DOMAIN_AUX_D: - return "AUX_D"; - case POWER_DOMAIN_GMBUS: - return "GMBUS"; - case POWER_DOMAIN_MODESET: - return "MODESET"; - case POWER_DOMAIN_INIT: - return "INIT"; - default: - MISSING_CASE(domain); - return "?"; - } -} - static int i915_power_domain_info(struct seq_file *m, void *unused) { struct drm_info_node *node = m->private; @@ -2775,7 +2710,7 @@ static int i915_power_domain_info(struct seq_file *m, void *unused) continue; seq_printf(m, " %-23s %d\n", -power_domain_str(power_domain), +intel_display_power_domain_str(power_domain), power_domains->domain_use_count[power_domain]); } } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a47e0f4..854e678 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -209,6 +209,72 @@ enum intel_display_power_domain { ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ (tran) + POWER_DOMAIN_TRANSCODER_A) +static inline const char * +intel_display_power_domain_str(enum intel_display_power_domain domain) +{ + switch (domain) { + case POWER_DOMAIN_PIPE_A: + return "PIPE_A"; + case POWER_DOMAIN_PIPE_B: + return "PIPE_B"; + case POWER_DOMAIN_PIPE_C: + return "PIPE_C"; + case POWER_DOMAIN_PIPE_A_PANEL_FITTER: + return "PIPE_A_PANEL_FITTER"; + case POWER_DOMAIN_PIPE_B_PANEL_FITTER: + return "PIPE_B_PANEL_FITTER"; + case POWER_DOMAIN_PIPE_C_PANEL_FITTER: + return "PIPE_C_PANEL_FITTER"; + case POWER_DOMAIN_TRANSCODER_A: + return "TRANSCODER_A"; + case POWER_DOMAIN_TRANSCODER_B: + return "TRANSCODER_B"; + case POWER_DOMAIN_TRANSCODER_C: + return "TRANSCODER_C"; + case POWER_DOMAIN_TRANSCODER_EDP: + return "TRANSCODER_EDP"; + case POWER_DOMAIN_PORT_DDI_A_LANES: + return "PORT_DDI_A_LANES"; + case POWER_DOMAIN_PORT_DDI_B_LANES: + return
[Intel-gfx] [PATCH 2/2] drm/i915/pm: Print offending domain in refcount failure
If we experience a refcounting failure in a power domain/well (unref'ing at least one too many times), log the name of the offending domain or well. Signed-off-by: Daniel Stone--- drivers/gpu/drm/i915/intel_runtime_pm.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index a1dc815..df2b8a8 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1433,11 +1433,15 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, mutex_lock(_domains->lock); - WARN_ON(!power_domains->domain_use_count[domain]); + WARN(!power_domains->domain_use_count[domain], +"Use count on domain %s is already zero\n", +intel_display_power_domain_str(domain)); power_domains->domain_use_count[domain]--; for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { - WARN_ON(!power_well->count); + WARN(!power_well->count, +"Use count on power well %s is already zero", +power_well->name); if (!--power_well->count) intel_power_well_disable(dev_priv, power_well); -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Also delay first activation for SKL+
On Thu, 2015-11-19 at 08:44 +, Daniel Stone wrote: > Hi Rodrigo, > > On 19 November 2015 at 00:39, Rodrigo Vivi> wrote: > > @@ -441,15 +438,14 @@ void intel_psr_enable(struct intel_dp > > *intel_dp) > > /* > > * FIXME: Activation should happen immediately since this > > function > > * is just called after pipe is fully trained and enabled. > > -* However on every platform we face issues when first > > activation > > +* However on some platforms we face issues when first > > activation > > * follows a modeset so quickly. > > * - On VLV/CHV we get bank screen on first activation > > * - On HSW/BDW we get a recoverable frozen screen > > until next > > * exit-activate sequence. > > */ > > - if (INTEL_INFO(dev)->gen < 9) > > - schedule_delayed_work(_priv->psr.work, > > - msecs_to_jiffies(intel_dp > > ->panel_power_cycle_delay * 5)); > > + schedule_delayed_work(_priv->psr.work, > > + msecs_to_jiffies(intel_dp > > ->panel_power_cycle_delay * 5)); > > The comment change here seems to be the exact opposite of the code > change: Indeed, this is why I'll keep the fixme on the comment. > aren't we now seeing these issues on every platform? If not, > it would be good to elaborate on the issues seen in SKL/BSW. So far no known issue on SKL/KBL. (BSW is CHV) But since we have no idea why these issues are happening and separated handling was causing confusion I preferred to handle all with the protection. Regarding the issue itself, the Hardware needs to communicate with the Sink using AUX channels. I believe there is just kind of strange conflict going on there because the failures are exactly when we are also using aux channels for our modeset/link-trainings. This is also why I used panel_power_cycle_delay * 5 there because it is the same value we use to force VDD on while we are doing the aux transactions. Please let me know if you have further questions, concerns or suggestions here. Thanks > > Cheers, > Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status
On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote: > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote: > > There are platforms that don't need the full GMBUS power domain > > (PCH, BXT) while others do (VLV/CHV). For optimizing this we > > would need to add a new power domain, but it's not clear how much > > we > > would benefit given the short time we hold the reference. So for > > now > > let's keep things simple. > > Actually on PCH platforms the gmbus domain means just just an > rpm ref since the gmbus hw lives in the PCH. Ah right. > And IIRC on BXT gmbus lives in pw0 so same deal really. It's in PW2 there. I'll fix the commit message. > And for vlv/chv we should just need the disp2d well, which > is exactly what we get with the gmbus domain. > > So I don't think there's actually anything to optimize here > with current platforms. > > Both patches look fine to me: > Reviewed-by: Ville Syrjälä> > > > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > b/drivers/gpu/drm/i915/intel_hdmi.c > > index 17ced03..bdd462e 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector > > *connector, bool force) > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > connector->base.id, connector->name); > > > > + intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > + > > while (!live_status && --retry) { > > live_status = > > intel_digital_port_connected(dev_priv, > > hdmi_to_dig_port(intel_hdmi)); > > @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector > > *connector, bool force) > > } else > > status = connector_status_disconnected; > > > > + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > + > > return status; > > } > > > > -- > > 2.5.0 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/skl: enable PC9/10 power states during suspend-to-idle
On Thu, Nov 19, 2015 at 04:06:47PM +0200, Imre Deak wrote: > On to, 2015-11-19 at 14:34 +0100, Patrik Jakobsson wrote: > > On Wed, Nov 18, 2015 at 06:44:43PM +0200, Imre Deak wrote: > > > On ke, 2015-11-18 at 17:33 +0100, Daniel Vetter wrote: > > > > On Wed, Nov 18, 2015 at 05:32:30PM +0200, Imre Deak wrote: > > > > > During suspend-to-idle we need to keep the DMC firmware active and DC6 > > > > > enabled, since otherwise we won't reach deep system power states like > > > > > PC9/10. The lead for this came from Nivedita who noticed that the > > > > > kernel's turbostat tool didn't report any PC9/10 residency change > > > > > across an 'echo freeze > /sys/power/state'. > > > > > > > > > > Reported-by: Nivedita Swaminathan > > > > > Signed-off-by: Imre Deak> > > > > --- > > > > > drivers/gpu/drm/i915/i915_drv.c | 44 > > > > > +++-- > > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > > 2 files changed, 35 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > > index 6344dfb..649e20a 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -624,6 +624,14 @@ static int vlv_resume_prepare(struct > > > > > drm_i915_private *dev_priv, > > > > > bool rpm_resume); > > > > > static int bxt_resume_prepare(struct drm_i915_private *dev_priv); > > > > > > > > > > +static bool suspend_to_idle(struct drm_i915_private *dev_priv) > > > > > +{ > > > > > +#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > + if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > + return true; > > > > > +#endif > > > > > + return false; > > > > > +} > > > > > > > > > > static int i915_drm_suspend(struct drm_device *dev) > > > > > { > > > > > @@ -676,11 +684,7 @@ static int i915_drm_suspend(struct drm_device > > > > > *dev) > > > > > > > > > > i915_save_state(dev); > > > > > > > > > > - opregion_target_state = PCI_D3cold; > > > > > -#if IS_ENABLED(CONFIG_ACPI_SLEEP) > > > > > - if (acpi_target_system_state() < ACPI_STATE_S3) > > > > > - opregion_target_state = PCI_D1; > > > > > -#endif > > > > > + opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : > > > > > PCI_D3cold; > > > > > intel_opregion_notify_adapter(dev, opregion_target_state); > > > > > > > > > > intel_uncore_forcewake_reset(dev, false); > > > > > @@ -701,15 +705,26 @@ static int i915_drm_suspend(struct drm_device > > > > > *dev) > > > > > static int i915_drm_suspend_late(struct drm_device *drm_dev, bool > > > > > hibernation) > > > > > { > > > > > struct drm_i915_private *dev_priv = drm_dev->dev_private; > > > > > + bool fw_csr; > > > > > int ret; > > > > > > > > > > - intel_power_domains_suspend(dev_priv); > > > > > + fw_csr = suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload; > > > > > + /* > > > > > + * In case of firmware assisted context save/restore don't > > > > > manually > > > > > + * deinit the power domains. This also means the CSR/DMC > > > > > firmware will > > > > > + * stay active, it will power down any HW resources as required > > > > > and > > > > > + * also enable deeper system power states that would be blocked > > > > > if the > > > > > + * firmware was inactive. > > > > > + */ > > > > > + if (!fw_csr) > > > > > + intel_power_domains_suspend(dev_priv); > > > > > > > > > > ret = intel_suspend_complete(dev_priv); > > > > > > > > > > if (ret) { > > > > > DRM_ERROR("Suspend complete failed: %d\n", ret); > > > > > - intel_power_domains_init_hw(dev_priv, true); > > > > > + if (!fw_csr) > > > > > + intel_power_domains_init_hw(dev_priv, true); > > > > > > > > > > return ret; > > > > > } > > > > > @@ -730,6 +745,8 @@ static int i915_drm_suspend_late(struct > > > > > drm_device *drm_dev, bool hibernation) > > > > > if (!(hibernation && INTEL_INFO(dev_priv)->gen < 6)) > > > > > pci_set_power_state(drm_dev->pdev, PCI_D3hot); > > > > > > > > > > + dev_priv->suspended_to_idle = suspend_to_idle(dev_priv); > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -842,8 +859,10 @@ static int i915_drm_resume_early(struct > > > > > drm_device *dev) > > > > > * FIXME: This should be solved with a special hdmi sink device > > > > > or > > > > > * similar so that power domains can be employed. > > > > > */ > > > > > - if (pci_enable_device(dev->pdev)) > > > > > - return -EIO; > > > > > + if (pci_enable_device(dev->pdev)) { > > > > > + ret = -EIO; > > > > > + goto out; > > > > > + } > > > > > > > > > > pci_set_master(dev->pdev); > > > > > > > > > >
Re: [Intel-gfx] [PATCH 1/2] drm/i915/pm: Unconstify power_domain_str
On Thu, Nov 19, 2015 at 05:59:10PM +, Daniel Stone wrote: > Let us print human-parseable values from the power domain code; upcoming > display code also wants to use it. > > Signed-off-by: Daniel Stone> --- > drivers/gpu/drm/i915/i915_debugfs.c | 67 > + > drivers/gpu/drm/i915/i915_drv.h | 66 > 2 files changed, 67 insertions(+), 66 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 411a9c6..b28da6f 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2685,71 +2685,6 @@ static int i915_runtime_pm_status(struct seq_file *m, > void *unused) > return 0; > } > > -static const char *power_domain_str(enum intel_display_power_domain domain) > -{ > - switch (domain) { > - case POWER_DOMAIN_PIPE_A: > - return "PIPE_A"; > - case POWER_DOMAIN_PIPE_B: > - return "PIPE_B"; > - case POWER_DOMAIN_PIPE_C: > - return "PIPE_C"; > - case POWER_DOMAIN_PIPE_A_PANEL_FITTER: > - return "PIPE_A_PANEL_FITTER"; > - case POWER_DOMAIN_PIPE_B_PANEL_FITTER: > - return "PIPE_B_PANEL_FITTER"; > - case POWER_DOMAIN_PIPE_C_PANEL_FITTER: > - return "PIPE_C_PANEL_FITTER"; > - case POWER_DOMAIN_TRANSCODER_A: > - return "TRANSCODER_A"; > - case POWER_DOMAIN_TRANSCODER_B: > - return "TRANSCODER_B"; > - case POWER_DOMAIN_TRANSCODER_C: > - return "TRANSCODER_C"; > - case POWER_DOMAIN_TRANSCODER_EDP: > - return "TRANSCODER_EDP"; > - case POWER_DOMAIN_PORT_DDI_A_LANES: > - return "PORT_DDI_A_LANES"; > - case POWER_DOMAIN_PORT_DDI_B_LANES: > - return "PORT_DDI_B_LANES"; > - case POWER_DOMAIN_PORT_DDI_C_LANES: > - return "PORT_DDI_C_LANES"; > - case POWER_DOMAIN_PORT_DDI_D_LANES: > - return "PORT_DDI_D_LANES"; > - case POWER_DOMAIN_PORT_DDI_E_LANES: > - return "PORT_DDI_E_LANES"; > - case POWER_DOMAIN_PORT_DSI: > - return "PORT_DSI"; > - case POWER_DOMAIN_PORT_CRT: > - return "PORT_CRT"; > - case POWER_DOMAIN_PORT_OTHER: > - return "PORT_OTHER"; > - case POWER_DOMAIN_VGA: > - return "VGA"; > - case POWER_DOMAIN_AUDIO: > - return "AUDIO"; > - case POWER_DOMAIN_PLLS: > - return "PLLS"; > - case POWER_DOMAIN_AUX_A: > - return "AUX_A"; > - case POWER_DOMAIN_AUX_B: > - return "AUX_B"; > - case POWER_DOMAIN_AUX_C: > - return "AUX_C"; > - case POWER_DOMAIN_AUX_D: > - return "AUX_D"; > - case POWER_DOMAIN_GMBUS: > - return "GMBUS"; > - case POWER_DOMAIN_MODESET: > - return "MODESET"; > - case POWER_DOMAIN_INIT: > - return "INIT"; > - default: > - MISSING_CASE(domain); > - return "?"; > - } > -} > - > static int i915_power_domain_info(struct seq_file *m, void *unused) > { > struct drm_info_node *node = m->private; > @@ -2775,7 +2710,7 @@ static int i915_power_domain_info(struct seq_file *m, > void *unused) > continue; > > seq_printf(m, " %-23s %d\n", > - power_domain_str(power_domain), > + intel_display_power_domain_str(power_domain), >power_domains->domain_use_count[power_domain]); > } > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a47e0f4..854e678 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -209,6 +209,72 @@ enum intel_display_power_domain { > ((tran) == TRANSCODER_EDP ? POWER_DOMAIN_TRANSCODER_EDP : \ >(tran) + POWER_DOMAIN_TRANSCODER_A) > > +static inline const char * > +intel_display_power_domain_str(enum intel_display_power_domain domain) It's still const. And I assume now we end up duplicating these strings in every object file that calls this. Why don't you just remove the "static" from the original? > +{ > + switch (domain) { > + case POWER_DOMAIN_PIPE_A: > + return "PIPE_A"; > + case POWER_DOMAIN_PIPE_B: > + return "PIPE_B"; > + case POWER_DOMAIN_PIPE_C: > + return "PIPE_C"; > + case POWER_DOMAIN_PIPE_A_PANEL_FITTER: > + return "PIPE_A_PANEL_FITTER"; > + case POWER_DOMAIN_PIPE_B_PANEL_FITTER: > + return "PIPE_B_PANEL_FITTER"; > + case POWER_DOMAIN_PIPE_C_PANEL_FITTER: > + return "PIPE_C_PANEL_FITTER"; > + case POWER_DOMAIN_TRANSCODER_A: > + return "TRANSCODER_A"; > + case POWER_DOMAIN_TRANSCODER_B: > + return "TRANSCODER_B"; > +
Re: [Intel-gfx] [PATCH 1/2] drm/i915/pm: Unconstify power_domain_str
On Thu, Nov 19, 2015 at 06:26:23PM +, Daniel Stone wrote: > Hey, > > On 19 November 2015 at 18:24, Ville Syrjälä >wrote: > > On Thu, Nov 19, 2015 at 05:59:10PM +, Daniel Stone wrote: > >> +static inline const char * > >> +intel_display_power_domain_str(enum intel_display_power_domain domain) > > > > It's still const. And I assume now we end up duplicating these strings > > in every object file that calls this. Why don't you just remove the > > "static" from the original? > > Right, 'unstatic' is what I meant. Dropping const wouldn't have been > very clever. > > Surely gcc's DCE pass will trivially eliminate this? Dunno. But I rather dislike having code in headers anyway. > I put it so it > could lie next to the enum definition itself, but if you'd prefer, > I'll happily move the definition to intel_runtime_pm.c (or whatever > shed colour is deemed appropriate) instead. > > Cheers, > Daniel -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/skl: re-enable power well support
On Wed, Nov 18, 2015 at 07:53:50PM +0200, Imre Deak wrote: > Now that the known DMC/DC issues are fixed, let's try again and > re-enable the power well support. > > Signed-off-by: Imre DeakTogether with the PC9/10 fix this is: Reviewed-by: Patrik Jakobsson > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index a1dc815..10154a7 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1841,11 +1841,6 @@ sanitize_disable_power_well_option(const struct > drm_i915_private *dev_priv, > if (disable_power_well >= 0) > return !!disable_power_well; > > - if (IS_SKYLAKE(dev_priv)) { > - DRM_DEBUG_KMS("Disabling display power well support\n"); > - return 0; > - } > - > return 1; > } > > -- > 2.5.0 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/pm: Unconstify power_domain_str
Hey, On 19 November 2015 at 18:24, Ville Syrjäläwrote: > On Thu, Nov 19, 2015 at 05:59:10PM +, Daniel Stone wrote: >> +static inline const char * >> +intel_display_power_domain_str(enum intel_display_power_domain domain) > > It's still const. And I assume now we end up duplicating these strings > in every object file that calls this. Why don't you just remove the > "static" from the original? Right, 'unstatic' is what I meant. Dropping const wouldn't have been very clever. Surely gcc's DCE pass will trivially eliminate this? I put it so it could lie next to the enum definition itself, but if you'd prefer, I'll happily move the definition to intel_runtime_pm.c (or whatever shed colour is deemed appropriate) instead. Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status
On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote: > On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote: > > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote: > > > There are platforms that don't need the full GMBUS power domain > > > (PCH, BXT) while others do (VLV/CHV). For optimizing this we > > > would need to add a new power domain, but it's not clear how much > > > we > > > would benefit given the short time we hold the reference. So for > > > now > > > let's keep things simple. > > > > Actually on PCH platforms the gmbus domain means just just an > > rpm ref since the gmbus hw lives in the PCH. > > Ah right. > > > And IIRC on BXT gmbus lives in pw0 so same deal really. > > It's in PW2 there. I'll fix the commit message. Doh. Not sure where I got the PW0 zero idea. Maybe I confused gmbus with hpd. But yes, you're right about that. > > > And for vlv/chv we should just need the disp2d well, which > > is exactly what we get with the gmbus domain. > > > > So I don't think there's actually anything to optimize here > > with current platforms. > > > > Both patches look fine to me: > > Reviewed-by: Ville Syrjälä> > > > > > > > Signed-off-by: Imre Deak > > > --- > > > drivers/gpu/drm/i915/intel_hdmi.c | 4 > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > > b/drivers/gpu/drm/i915/intel_hdmi.c > > > index 17ced03..bdd462e 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector > > > *connector, bool force) > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > > connector->base.id, connector->name); > > > > > > + intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > > + > > > while (!live_status && --retry) { > > > live_status = > > > intel_digital_port_connected(dev_priv, > > > hdmi_to_dig_port(intel_hdmi)); > > > @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector > > > *connector, bool force) > > > } else > > > status = connector_status_disconnected; > > > > > > + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > > + > > > return status; > > > } > > > > > > -- > > > 2.5.0 > > > > > > ___ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2
On Thu, Nov 19, 2015 at 10:53:30PM +0200, Ville Syrjälä wrote: > On Thu, Nov 19, 2015 at 06:35:04PM -0200, Paulo Zanoni wrote: > > 2015-11-19 18:06 GMT-02:00 Ville Syrjälä: > > > On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote: > > >> 2014-05-26 11:26 GMT-03:00 : > > >> > From: Ville Syrjälä > > >> > > > >> > Now that the vblank races are plugged, we can opt out of using > > >> > the vblank disable timer and just let vblank interrupts get > > >> > disabled immediately when the last reference is dropped. > > >> > > > >> > Gen2 is the exception since it has no hardware frame counter. > > >> > > >> Hi > > >> > > >> Remember last week's FBC vblank optimization patch that had an > > >> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? > > >> After I fixed the bug in the patch I realized that it was the > > >> unbalanced vblank_get() call that moved PC state residency up. > > >> > > >> I did some experiments, and on my specific BDW machine, after running > > >> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. > > >> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 > > >> residency goes from 60-70% to 85-90% when I revert this patch. I'm > > >> running just an idle Cinnamon with an open terminal. > > >> > > >> So, since the commit message lacks more details, what are the > > >> downsides of reverting this patch? What are the advantages of opting > > >> out of the vblank timer? I see my desktop does tons and tons of vblank > > >> get/put calls per second, so the disable timer makes a lot of sense. > > > > > > "Idle" desktop :( > > > > My first realization of this little problem was when I was > > implementing runtime PM :) > > > > > > > > > > Really the immediate disable should save power. Where are these tons of > > > vblank get/puts coming from actually? > > > > I'll take a finer look tomorrow, but I assume it's probably some > > application redrawing. I see it does calm down sometimes, but that's > > not enough to get better PC7 residency. > > > > > > > I would assume you'd get a handful > > > per frame at most, and that when you're actually doing something. On an > > > idle system I would expect nothing at all happens during most frames. > > > > > > Not sure, but I guess it's possible the extra register accesses in the > > > get/puts actually cause the display to exit low power states all the time, > > > or something. > > > > I tried replacing the register macros with the _FW version and that didn't > > help. > > Well, that would just get rid of the unclaimed reg checks. Nothing more > I think. > > > > > > > > > > > There's also this note in Bspec (for HSW at least): > > > > I think this not is present on most (all?) gens. > > Doesn't really prove anything. > > > > "Workaround : Do not enable and unmask this interrupt if the associated > > > pipe is disabled. Do not leave this interrupt enabled and unmasked > > > after the associated pipe is disabled." > > > which we took to mean that having the interrupt masked but enabled is > > > fine. > > > > I'm aware of this, but I think the problem is that the resources > > drained by the constant enable+disable+enable+disable outweigh the > > resources saved by turning off vblanks. > > Well the CPU is awake anyway doing the get/put, so not sure why a a few > extra register accesses there would have such a huge impact. > > > Not sure if there's an extra > > reason why BSpec asks us to immediately disable vblanks though... > > > > So, to summarize, the main (only?) reason is the BSpec comment? > > The point is not to wake up due to interrupts when we don't need them. > > > > > > > > But maybe we'd actually have to frob IER too to avoid wasting > > > power somehow? > > > > With the interrupt masked on IMR, I don't think IER matters. > > I'm not sure anyone actually verified that. Well, I just tried this on HSW. And on that one IER didn't make a difference to pc7 residency (~70%) at least. This was on an actually idle system ;) > > > > > > > > >> I also wish there was some easy way to check how this patch (or its > > >> revert) affect a bunch of different workloads... > > >> > > >> (Also CCing Chris for insightful comments on performance) > > > > > > IIRC Chris had a patch to not disable the interrupt immediately when > > > the refcount drops to 0, but instead delay the disable until the next > > > interrupt actually happens. But I guess it didn't go in? Probably I > > > should have reviewed it but didn't. It sounds like a decent idea to > > > me in any case for the active use case. > > > > > >> > > >> Thanks, > > >> Paulo > > >> > > >> > > > >> > Signed-off-by: Ville Syrjälä > > >> > --- > > >> > drivers/gpu/drm/i915/i915_irq.c | 8 > > >> > 1 file changed, 8 insertions(+) > > >> > > > >> > diff --git
Re: [Intel-gfx] [PATCH 5/7] drm/i915: sseu: convert subslice count fields to subslice mask
On Wed, Oct 21, 2015 at 06:40:35PM +0300, Imre Deak wrote: > In an upcoming patch we'll need the actual mask of subslices in addition > to their count, so convert the subslice_per_slice field to a mask. > Also we can easily calculate subslice_total from the other fields, so > instead of storing a cached version of this, add a helper to calculate > it. Oh good, I think I asked for this in patch 1. > > Signed-off-by: Imre Deak> --- > drivers/gpu/drm/i915/i915_debugfs.c | 37 - > drivers/gpu/drm/i915/i915_dma.c | 64 > + > drivers/gpu/drm/i915/i915_drv.h | 8 +++-- > drivers/gpu/drm/i915/intel_lrc.c| 2 +- > 4 files changed, 51 insertions(+), 60 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 390dc59..3fb83ea 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4966,7 +4966,7 @@ static void cherryview_sseu_device_status(struct > drm_device *dev, > continue; > > stat->slice_mask = BIT(0); > - stat->subslice_per_slice++; > + stat->subslice_mask |= BIT(ss); > eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) + >((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) + >((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) + > @@ -4975,7 +4975,6 @@ static void cherryview_sseu_device_status(struct > drm_device *dev, > stat->eu_per_subslice = max_t(unsigned int, > stat->eu_per_subslice, eu_cnt); > } > - stat->subslice_total = stat->subslice_per_slice; > } > > static void gen9_sseu_device_status(struct drm_device *dev, > @@ -5008,8 +5007,6 @@ static void gen9_sseu_device_status(struct drm_device > *dev, >GEN9_PGCTL_SSB_EU311_ACK; > > for (s = 0; s < s_max; s++) { > - unsigned int ss_cnt = 0; > - > if ((s_reg[s] & GEN9_PGCTL_SLICE_ACK) == 0) > /* skip disabled slice */ > continue; > @@ -5017,18 +5014,19 @@ static void gen9_sseu_device_status(struct drm_device > *dev, > stat->slice_mask |= BIT(s); > > if (IS_SKYLAKE(dev)) > - ss_cnt = INTEL_INFO(dev)->sseu.subslice_per_slice; > + stat->subslice_mask = > + INTEL_INFO(dev_priv)->sseu.subslice_mask; > > for (ss = 0; ss < ss_max; ss++) { > unsigned int eu_cnt; > > - if (IS_BROXTON(dev) && > - !(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss > - /* skip disabled subslice */ > - continue; > + if (IS_BROXTON(dev)) { > + if (!(s_reg[s] & (GEN9_PGCTL_SS_ACK(ss > + /* skip disabled subslice */ > + continue; > > - if (IS_BROXTON(dev)) > - ss_cnt++; > + stat->subslice_mask |= BIT(ss); > + } > > eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] & > eu_mask[ss%2]); > @@ -5037,11 +5035,6 @@ static void gen9_sseu_device_status(struct drm_device > *dev, > stat->eu_per_subslice, > eu_cnt); > } > - > - stat->subslice_total += ss_cnt; > - stat->subslice_per_slice = max_t(unsigned int, > - stat->subslice_per_slice, > - ss_cnt); > } > } > > @@ -5055,12 +5048,10 @@ static void broadwell_sseu_device_status(struct > drm_device *dev, > stat->slice_mask = slice_info & GEN8_LSLICESTAT_MASK; > > if (stat->slice_mask) { > - stat->subslice_per_slice = > - INTEL_INFO(dev)->sseu.subslice_per_slice; > - stat->subslice_total = hweight32(stat->slice_mask) * > -stat->subslice_per_slice; > + stat->subslice_mask = INTEL_INFO(dev)->sseu.subslice_mask; > stat->eu_per_subslice = INTEL_INFO(dev)->sseu.eu_per_subslice; > - stat->eu_total = stat->eu_per_subslice * stat->subslice_total; > + stat->eu_total = stat->eu_per_subslice * > + sseu_subslice_total(stat); > > /* subtract fused off EU(s) from enabled slice(s) */ > for (s = 0; s < hweight32(stat->slice_mask); s++) { > @@ -5079,9 +5070,9 @@ static void i915_print_sseu_info(struct seq_file *m, > bool is_available_info, >
Re: [Intel-gfx] [PATCH 7/7] drm/i915/bdw: sseu: fix sseu status parsing
On Wed, Oct 21, 2015 at 06:40:37PM +0300, Imre Deak wrote: > Currently when checking for fused off EUs we may ignore the EU count in > an enabled slice if there is any disabled slice preceding the enabled > one (with a lower slice ID). Perhaps this can't happen in reality, but > there is no reason to have this assumption built-in, the code is clearer > without it. > > Signed-off-by: Imre Deak> --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 24504a3..1d43624 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -5054,7 +5054,7 @@ static void broadwell_sseu_device_status(struct > drm_device *dev, >sseu_subslice_total(stat); > > /* subtract fused off EU(s) from enabled slice(s) */ > - for (s = 0; s < hweight32(stat->slice_mask); s++) { > + for (s = 0; s < fls(stat->slice_mask); s++) { Could use for_each_set_bit here too. > u8 subslice_7eu = INTEL_INFO(dev)->sseu.subslice_7eu[s]; > > stat->eu_total -= hweight8(subslice_7eu); 6 & 7 are: Reviewed-by: Ben Widawsky 1-7 are also: Tested-by: Ben Widawsky -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v1] drm/i915: Fix a false alert of memory leak when free LRC
From: Alex DaiThere is a memory leak warning message from i915_gem_context_clean when GuC submission is enabled. The reason is that when LRC is released, its ppgtt could be still referenced. The assumption that all VMAs are unbound during release of LRC is not true. v1: Move the code inside i915_gem_context_clean() to where ppgtt is released because it is not cleaning context anyway but ppgtt. Signed-off-by: Alex Dai --- drivers/gpu/drm/i915/i915_gem_context.c | 24 drivers/gpu/drm/i915/i915_gem_gtt.c | 12 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 204dc7c..cc5c8e6 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -133,23 +133,6 @@ static int get_context_size(struct drm_device *dev) return ret; } -static void i915_gem_context_clean(struct intel_context *ctx) -{ - struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; - struct i915_vma *vma, *next; - - if (!ppgtt) - return; - - WARN_ON(!list_empty(>base.active_list)); - - list_for_each_entry_safe(vma, next, >base.inactive_list, -mm_list) { - if (WARN_ON(__i915_vma_unbind_no_wait(vma))) - break; - } -} - void i915_gem_context_free(struct kref *ctx_ref) { struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); @@ -159,13 +142,6 @@ void i915_gem_context_free(struct kref *ctx_ref) if (i915.enable_execlists) intel_lr_context_free(ctx); - /* -* This context is going away and we need to remove all VMAs still -* around. This is to handle imported shared objects for which -* destructor did not run when their handles were closed. -*/ - i915_gem_context_clean(ctx); - i915_ppgtt_put(ctx->ppgtt); if (ctx->legacy_hw_ctx.rcs_state) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 016739e..d36943c 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2214,6 +2214,7 @@ void i915_ppgtt_release(struct kref *kref) { struct i915_hw_ppgtt *ppgtt = container_of(kref, struct i915_hw_ppgtt, ref); + struct i915_vma *vma, *next; trace_i915_ppgtt_release(>base); @@ -2221,6 +,17 @@ void i915_ppgtt_release(struct kref *kref) WARN_ON(!list_empty(>base.active_list)); WARN_ON(!list_empty(>base.inactive_list)); + /* +* This ppgtt is going away and we need to remove all VMAs still +* around. This is to handle imported shared objects for which +* destructor did not run when their handles were closed. +*/ + list_for_each_entry_safe(vma, next, >base.inactive_list, +mm_list) { + if (WARN_ON(__i915_vma_unbind_no_wait(vma))) + break; + } + list_del(>base.global_link); drm_mm_takedown(>base.mm); -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915: sseu: move sseu_dev_status to i915_drv.h
On Thu, Nov 19, 2015 at 03:10:14PM -0800, Ben Widawsky wrote: > On Wed, Oct 21, 2015 at 06:40:31PM +0300, Imre Deak wrote: > > The data in this struct is provided both by getting the > > slice/subslice/eu features available on a given platform and the actual > > runtime state of these same features which depends on the HW's current > > power saving state. > > > > Atm members of this struct are duplicated in sseu_dev_status and > I> intel_device_info. For clarity and code reuse we can share one struct > > for both of the above purposes. This patch only moves the struct to the > > header file, the next patch will convert users of intel_device_info to > > use this struct too. > > > > Instead of unsigned int u8 is used now, which is big enough and is used > > anyway in intel_device_info. > > > > No functional change. > > > > Signed-off-by: Imre Deak> > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 29 - > > drivers/gpu/drm/i915/i915_drv.h | 8 > > 2 files changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index a3b22bd..3dd7076 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -4945,16 +4945,8 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops, > > i915_cache_sharing_get, i915_cache_sharing_set, > > "%llu\n"); > > > > -struct sseu_dev_status { > > - unsigned int slice_total; > > - unsigned int subslice_total; > > - unsigned int subslice_per_slice; > > - unsigned int eu_total; > > - unsigned int eu_per_subslice; > > -}; > > - > > static void cherryview_sseu_device_status(struct drm_device *dev, > > - struct sseu_dev_status *stat) > > + struct sseu_dev_info *stat) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > int ss_max = 2; > > @@ -4980,13 +4972,14 @@ static void cherryview_sseu_device_status(struct > > drm_device *dev, > > ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) + > > ((sig2[ss] & CHV_EU311_PG_ENABLE) ? 0 : 2); > > stat->eu_total += eu_cnt; > > - stat->eu_per_subslice = max(stat->eu_per_subslice, eu_cnt); > > + stat->eu_per_subslice = max_t(unsigned int, > > + stat->eu_per_subslice, eu_cnt); > > } > > stat->subslice_total = stat->subslice_per_slice; > > } > > > > static void gen9_sseu_device_status(struct drm_device *dev, > > - struct sseu_dev_status *stat) > > + struct sseu_dev_info *stat) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > int s_max = 3, ss_max = 4; > > @@ -5040,18 +5033,20 @@ static void gen9_sseu_device_status(struct > > drm_device *dev, > > eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] & > >eu_mask[ss%2]); > > stat->eu_total += eu_cnt; > > - stat->eu_per_subslice = max(stat->eu_per_subslice, > > - eu_cnt); > > + stat->eu_per_subslice = max_t(unsigned int, > > + stat->eu_per_subslice, > > + eu_cnt); > > } > > > > stat->subslice_total += ss_cnt; > > - stat->subslice_per_slice = max(stat->subslice_per_slice, > > - ss_cnt); > > + stat->subslice_per_slice = max_t(unsigned int, > > +stat->subslice_per_slice, > > +ss_cnt); > > } > > } > > > > static void broadwell_sseu_device_status(struct drm_device *dev, > > -struct sseu_dev_status *stat) > > +struct sseu_dev_info *stat) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > int s; > > @@ -5079,7 +5074,7 @@ static int i915_sseu_status(struct seq_file *m, void > > *unused) > > { > > struct drm_info_node *node = (struct drm_info_node *) m->private; > > struct drm_device *dev = node->minor->dev; > > - struct sseu_dev_status stat; > > + struct sseu_dev_info stat; > > If you're going through this rename pain with the type anyway you may as well > s/stat/info. > > Also, I never understood what "sseu" was supposed to be short for. The spec I started the use of sseu here as abbreviation of slice/subslice/EU, as in encapsulation of attributes of this hierarchy. Originally we just needed a few summarized count values but there was intent to add lists of per-component details as needed. -Jeff > calls these "global attributes"
[Intel-gfx] [PATCH] drm/i915/guc: Keep irq enabled during GuC cmd submission
From: Alex DaiWhen GuC Work Queue is full, driver will wait GuC for avaliable space by calling wait_for_atomic. If irq is disabled, lockup will happen because jiffies won't be updated. Issue is found in igt/gem_close_race. Signed-off-by: Alex Dai --- drivers/gpu/drm/i915/i915_guc_submission.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 0a6b007..bbfa6ed 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -597,14 +597,13 @@ int i915_guc_submit(struct i915_guc_client *client, { struct intel_guc *guc = client->guc; enum intel_ring_id ring_id = rq->ring->id; - unsigned long flags; int q_ret, b_ret; /* Need this because of the deferred pin ctx and ring */ /* Shall we move this right after ring is pinned? */ lr_context_update(rq); - spin_lock_irqsave(>wq_lock, flags); + spin_lock(>wq_lock); q_ret = guc_add_workqueue_item(client, rq); if (q_ret == 0) @@ -620,7 +619,7 @@ int i915_guc_submit(struct i915_guc_client *client, } else { client->retcode = 0; } - spin_unlock_irqrestore(>wq_lock, flags); + spin_unlock(>wq_lock); spin_lock(>host2guc_lock); guc->submissions[ring_id] += 1; -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH V3 1/1] drm/i915/audio: apply SKL codec wake up patch to BXT
From: "Lu, Han"For SKL we added a commit 632f3ab95fe2 ("drm/i915/audio: add codec wakeup override enabled/disable callback"), in order to enable codec wakeup override signal, to allow re-enumeration of the controller on SKL after resume from low power state. In SKL, HDMI/DP codec and PCH HD Audio Controller are in different power wells, so it's necessary to reset display audio codecs when power well on, otherwise display audio codecs will disappear when resume from low power state. Reset steps when power on: enable codec wakeup -> azx_init_chip() -> disable codec wakeup The work around is applicable for chips with power well design similar to SKL, like BXT. v2: add explanation v3: use gen check instead of list all available chips Signed-off-by: Lu, Han diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 63d4706..4c01018 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -591,7 +591,12 @@ static void i915_audio_component_codec_wake_override(struct device *dev, struct drm_i915_private *dev_priv = dev_to_i915(dev); u32 tmp; - if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv)) + /* +* The patch is only applicable to chips that HDMI/DP codecs and PCH +* HD Audio Controller are in different power wells, such as SKL, +* BXT and KBL. +*/ + if (INTEL_INFO(dev_priv)->gen < 9) return; /* -- 2.5.0 ___ 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: sseu: convert slice count field to mask
On Wed, Oct 21, 2015 at 06:40:34PM +0300, Imre Deak wrote: > In an upcoming patch we'll need the actual mask of slices in addition to > their count, so replace the count field with a mask. > > Signed-off-by: Imre Deak> --- > drivers/gpu/drm/i915/i915_debugfs.c | 14 +++--- > drivers/gpu/drm/i915/i915_dma.c | 37 > +++-- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_lrc.c| 2 +- > 4 files changed, 28 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 183c1f2..390dc59 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4965,7 +4965,7 @@ static void cherryview_sseu_device_status(struct > drm_device *dev, > /* skip disabled subslice */ > continue; > > - stat->slice_total = 1; > + stat->slice_mask = BIT(0); > stat->subslice_per_slice++; > eu_cnt = ((sig1[ss] & CHV_EU08_PG_ENABLE) ? 0 : 2) + >((sig1[ss] & CHV_EU19_PG_ENABLE) ? 0 : 2) + > @@ -5014,7 +5014,7 @@ static void gen9_sseu_device_status(struct drm_device > *dev, > /* skip disabled slice */ > continue; > > - stat->slice_total++; > + stat->slice_mask |= BIT(s); > > if (IS_SKYLAKE(dev)) > ss_cnt = INTEL_INFO(dev)->sseu.subslice_per_slice; > @@ -5052,18 +5052,18 @@ static void broadwell_sseu_device_status(struct > drm_device *dev, > int s; > u32 slice_info = I915_READ(GEN8_GT_SLICE_INFO); > > - stat->slice_total = hweight32(slice_info & GEN8_LSLICESTAT_MASK); > + stat->slice_mask = slice_info & GEN8_LSLICESTAT_MASK; > > - if (stat->slice_total) { > + if (stat->slice_mask) { > stat->subslice_per_slice = > INTEL_INFO(dev)->sseu.subslice_per_slice; > - stat->subslice_total = stat->slice_total * > + stat->subslice_total = hweight32(stat->slice_mask) * ^ hweight8? > stat->subslice_per_slice; > stat->eu_per_subslice = INTEL_INFO(dev)->sseu.eu_per_subslice; > stat->eu_total = stat->eu_per_subslice * stat->subslice_total; > > /* subtract fused off EU(s) from enabled slice(s) */ > - for (s = 0; s < stat->slice_total; s++) { > + for (s = 0; s < hweight32(stat->slice_mask); s++) { ^ hweight8? how about for_each_set_bit(s, stat->slice_mask, 8) {} ? > u8 subslice_7eu = INTEL_INFO(dev)->sseu.subslice_7eu[s]; > > stat->eu_total -= hweight8(subslice_7eu); > @@ -5077,7 +5077,7 @@ static void i915_print_sseu_info(struct seq_file *m, > bool is_available_info, > const char *type = is_available_info ? "Available" : "Enabled"; > > seq_printf(m, " %s Slice Total: %u\n", type, > -sseu->slice_total); > +hweight32(sseu->slice_mask)); ^ hweight8? > seq_printf(m, " %s Subslice Total: %u\n", type, > sseu->subslice_total); > seq_printf(m, " %s Subslice Per Slice: %u\n", type, > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index be8e141..1f5f3a7d 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -559,7 +559,7 @@ static void cherryview_sseu_info_init(struct drm_device > *dev) > info = (struct sseu_dev_info *)_INFO(dev_priv)->sseu; > fuse = I915_READ(CHV_FUSE_GT); > > - info->slice_total = 1; > + info->slice_mask = BIT(0); > > if (!(fuse & CHV_FGT_DISABLE_SS0)) { > info->subslice_per_slice++; > @@ -599,23 +599,21 @@ static void gen9_sseu_info_init(struct drm_device *dev) > struct sseu_dev_info *info; > int s_max = 3, ss_max = 4, eu_max = 8; > int s, ss; > - u32 fuse2, s_enable, ss_disable, eu_disable; > + u32 fuse2, ss_disable, eu_disable; > u8 eu_mask = 0xff; > > info = (struct sseu_dev_info *)_INFO(dev_priv)->sseu; > fuse2 = I915_READ(GEN8_FUSE2); > - s_enable = (fuse2 & GEN8_F2_S_ENA_MASK) >> > -GEN8_F2_S_ENA_SHIFT; > + info->slice_mask = (fuse2 & GEN8_F2_S_ENA_MASK) >> GEN8_F2_S_ENA_SHIFT; > ss_disable = (fuse2 & GEN9_F2_SS_DIS_MASK) >> >GEN9_F2_SS_DIS_SHIFT; > > - info->slice_total = hweight32(s_enable); > /* >* The subslice disable field is global, i.e. it applies >* to each of the enabled slices. > */ > info->subslice_per_slice = ss_max - hweight32(ss_disable); > - info->subslice_total = info->slice_total * > + info->subslice_total =
[Intel-gfx] [PATCH v1] drm/i915: Defer LRC unpin and release
From: Alex DaiCan't immediately free LRC (neither unpin it) even all its referenced requests are completed, because HW still need a short period of time to save data to LRC status page. It is safe to free LRC when HW completes a request from a different LRC. Introduce a new function intel_lr_context_do_unpin that do the actual unpin work. When driver receives unpin call (from retiring of a request), the LRC pin & ref count will be increased to defer the unpin and release. If last LRC is different and its pincount reaches to zero, driver will do the actual unpin work. There will be always a LRC kept until ring itself gets cleaned up. v1: Simplify the update of last context by reusing current ring-> last_context. Be note that it is safe to do so because lrc ring is cleaned up early than i915_gem_context_fini(). Signed-off-by: Alex Dai --- drivers/gpu/drm/i915/intel_lrc.c | 59 1 file changed, 54 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 06180dc..7a3c9cc 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1039,6 +1039,55 @@ unpin_ctx_obj: return ret; } +static void intel_lr_context_do_unpin(struct intel_engine_cs *ring, + struct intel_context *ctx) +{ + struct drm_device *dev = ring->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_gem_object *ctx_obj; + + WARN_ON(!mutex_is_locked(>dev->struct_mutex)); + + ctx_obj = ctx->engine[ring->id].state; + if (!ctx_obj) + return; + + i915_gem_object_ggtt_unpin(ctx_obj); + intel_unpin_ringbuffer_obj(ctx->engine[ring->id].ringbuf); + + /* Invalidate GuC TLB. */ + if (i915.enable_guc_submission) + I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE); +} + +static void set_last_lrc(struct intel_engine_cs *ring, + struct intel_context *ctx) +{ + struct intel_context *last; + + /* Unpin will be deferred, so the release of lrc. Hold pin & ref count +* untill we receive the retire of next request. */ + if (ctx) { + ctx->engine[ring->id].pin_count++; + i915_gem_context_reference(ctx); + } + + last = ring->last_context; + ring->last_context = ctx; + + if (last == NULL) + return; + + /* Unpin is on hold for last context. Release pincount first. Then if HW +* completes request from another lrc, try to do the actual unpin. */ + last->engine[ring->id].pin_count--; + if (last != ctx && !last->engine[ring->id].pin_count) + intel_lr_context_do_unpin(ring, last); + + /* Release previous context refcount that on hold */ + i915_gem_context_unreference(last); +} + static int intel_lr_context_pin(struct drm_i915_gem_request *rq) { int ret = 0; @@ -1062,14 +,11 @@ void intel_lr_context_unpin(struct drm_i915_gem_request *rq) { struct intel_engine_cs *ring = rq->ring; struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state; - struct intel_ringbuffer *ringbuf = rq->ringbuf; if (ctx_obj) { WARN_ON(!mutex_is_locked(>dev->struct_mutex)); - if (--rq->ctx->engine[ring->id].pin_count == 0) { - intel_unpin_ringbuffer_obj(ringbuf); - i915_gem_object_ggtt_unpin(ctx_obj); - } + --rq->ctx->engine[ring->id].pin_count; + set_last_lrc(ring, rq->ctx); } } @@ -1908,6 +1954,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) } lrc_destroy_wa_ctx_obj(ring); + + /* this will clean up last lrc */ + set_last_lrc(ring, NULL); } static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring) -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V2 1/1] drm/i915/audio: apply SKL codec wake up patch to BXT
On 11/20/2015 12:01 AM, Jani Nikula wrote: On Thu, 19 Nov 2015, Daniel Vetterwrote: On Thu, Nov 19, 2015 at 10:44:48PM +0800, han...@intel.com wrote: From: "Lu, Han" For SKL we added a commit 632f3ab95fe2 ("drm/i915/audio: add codec wakeup override enabled/disable callback"), in order to enable codec wakeup override signal, to allow re-enumeration of the controller on SKL after resume from low power state. In SKL, HDMI/DP codec and PCH HD Audio Controller are in different power wells, so it's necessary to reset display audio codecs when power well on, otherwise display audio codecs will disappear when resume from low power state. Reset steps when power on: enable codec wakeup -> azx_init_chip() -> disable codec wakeup Since the power well design did not change from SKL to BXT, we need to apply the workaround to BXT also. v2: add explanation Signed-off-by: Lu, Han diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 63d4706..8310bf3 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -591,7 +591,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev, struct drm_i915_private *dev_priv = dev_to_i915(dev); u32 tmp; - if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv)) + if (!IS_SKYLAKE(dev_priv) && !IS_BROXTON(dev_priv) && + !IS_KABYLAKE(dev_priv)) if (INTEL_INFO(dev)->gen < 9) return; for future-proofing? At least make it an IS_GEN9 check instead of listening all of them in a long list. http://mid.gmane.org/87a8qacq5w@intel.com ... Yes, that's right. Thanks. I'll send next revision. Regards, Han Lu -Daniel return; /* -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2
On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote: > 2014-05-26 11:26 GMT-03:00: > > From: Ville Syrjälä > > > > Now that the vblank races are plugged, we can opt out of using > > the vblank disable timer and just let vblank interrupts get > > disabled immediately when the last reference is dropped. > > > > Gen2 is the exception since it has no hardware frame counter. > > Hi > > Remember last week's FBC vblank optimization patch that had an > erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? > After I fixed the bug in the patch I realized that it was the > unbalanced vblank_get() call that moved PC state residency up. > > I did some experiments, and on my specific BDW machine, after running > "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. > If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 > residency goes from 60-70% to 85-90% when I revert this patch. I'm > running just an idle Cinnamon with an open terminal. > > So, since the commit message lacks more details, what are the > downsides of reverting this patch? What are the advantages of opting > out of the vblank timer? I see my desktop does tons and tons of vblank > get/put calls per second, so the disable timer makes a lot of sense. "Idle" desktop :( Really the immediate disable should save power. Where are these tons of vblank get/puts coming from actually? I would assume you'd get a handful per frame at most, and that when you're actually doing something. On an idle system I would expect nothing at all happens during most frames. Not sure, but I guess it's possible the extra register accesses in the get/puts actually cause the display to exit low power states all the time, or something. There's also this note in Bspec (for HSW at least): "Workaround : Do not enable and unmask this interrupt if the associated pipe is disabled. Do not leave this interrupt enabled and unmasked after the associated pipe is disabled." which we took to mean that having the interrupt masked but enabled is fine. But maybe we'd actually have to frob IER too to avoid wasting power somehow? > I also wish there was some easy way to check how this patch (or its > revert) affect a bunch of different workloads... > > (Also CCing Chris for insightful comments on performance) IIRC Chris had a patch to not disable the interrupt immediately when the refcount drops to 0, but instead delay the disable until the next interrupt actually happens. But I guess it didn't go in? Probably I should have reviewed it but didn't. It sounds like a decent idea to me in any case for the active use case. > > Thanks, > Paulo > > > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/i915_irq.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 28bae6e..4b2e7af 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) > > dev->max_vblank_count = 0xff; /* only 24 bits of frame > > count */ > > } > > > > + /* > > +* Opt out of the vblank disable timer on everything except gen2. > > +* Gen2 doesn't have a hardware frame counter and so depends on > > +* vblank interrupts to produce sane vblank seuquence numbers. > > +*/ > > + if (!IS_GEN2(dev)) > > + dev->vblank_disable_immediate = true; > > + > > if (drm_core_check_feature(dev, DRIVER_MODESET)) { > > dev->driver->get_vblank_timestamp = > > i915_get_vblank_timestamp; > > dev->driver->get_scanout_position = > > i915_get_crtc_scanoutpos; > > -- > > 1.8.5.5 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2
2015-11-19 18:06 GMT-02:00 Ville Syrjälä: > On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote: >> 2014-05-26 11:26 GMT-03:00 : >> > From: Ville Syrjälä >> > >> > Now that the vblank races are plugged, we can opt out of using >> > the vblank disable timer and just let vblank interrupts get >> > disabled immediately when the last reference is dropped. >> > >> > Gen2 is the exception since it has no hardware frame counter. >> >> Hi >> >> Remember last week's FBC vblank optimization patch that had an >> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? >> After I fixed the bug in the patch I realized that it was the >> unbalanced vblank_get() call that moved PC state residency up. >> >> I did some experiments, and on my specific BDW machine, after running >> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. >> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 >> residency goes from 60-70% to 85-90% when I revert this patch. I'm >> running just an idle Cinnamon with an open terminal. >> >> So, since the commit message lacks more details, what are the >> downsides of reverting this patch? What are the advantages of opting >> out of the vblank timer? I see my desktop does tons and tons of vblank >> get/put calls per second, so the disable timer makes a lot of sense. > > "Idle" desktop :( My first realization of this little problem was when I was implementing runtime PM :) > > Really the immediate disable should save power. Where are these tons of > vblank get/puts coming from actually? I'll take a finer look tomorrow, but I assume it's probably some application redrawing. I see it does calm down sometimes, but that's not enough to get better PC7 residency. > I would assume you'd get a handful > per frame at most, and that when you're actually doing something. On an > idle system I would expect nothing at all happens during most frames. > > Not sure, but I guess it's possible the extra register accesses in the > get/puts actually cause the display to exit low power states all the time, > or something. I tried replacing the register macros with the _FW version and that didn't help. > > There's also this note in Bspec (for HSW at least): I think this not is present on most (all?) gens. > "Workaround : Do not enable and unmask this interrupt if the associated > pipe is disabled. Do not leave this interrupt enabled and unmasked > after the associated pipe is disabled." > which we took to mean that having the interrupt masked but enabled is > fine. I'm aware of this, but I think the problem is that the resources drained by the constant enable+disable+enable+disable outweigh the resources saved by turning off vblanks. Not sure if there's an extra reason why BSpec asks us to immediately disable vblanks though... So, to summarize, the main (only?) reason is the BSpec comment? > But maybe we'd actually have to frob IER too to avoid wasting > power somehow? With the interrupt masked on IMR, I don't think IER matters. > >> I also wish there was some easy way to check how this patch (or its >> revert) affect a bunch of different workloads... >> >> (Also CCing Chris for insightful comments on performance) > > IIRC Chris had a patch to not disable the interrupt immediately when > the refcount drops to 0, but instead delay the disable until the next > interrupt actually happens. But I guess it didn't go in? Probably I > should have reviewed it but didn't. It sounds like a decent idea to > me in any case for the active use case. > >> >> Thanks, >> Paulo >> >> > >> > Signed-off-by: Ville Syrjälä >> > --- >> > drivers/gpu/drm/i915/i915_irq.c | 8 >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_irq.c >> > b/drivers/gpu/drm/i915/i915_irq.c >> > index 28bae6e..4b2e7af 100644 >> > --- a/drivers/gpu/drm/i915/i915_irq.c >> > +++ b/drivers/gpu/drm/i915/i915_irq.c >> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) >> > dev->max_vblank_count = 0xff; /* only 24 bits of frame >> > count */ >> > } >> > >> > + /* >> > +* Opt out of the vblank disable timer on everything except gen2. >> > +* Gen2 doesn't have a hardware frame counter and so depends on >> > +* vblank interrupts to produce sane vblank seuquence numbers. >> > +*/ >> > + if (!IS_GEN2(dev)) >> > + dev->vblank_disable_immediate = true; >> > + >> > if (drm_core_check_feature(dev, DRIVER_MODESET)) { >> > dev->driver->get_vblank_timestamp = >> > i915_get_vblank_timestamp; >> > dev->driver->get_scanout_position = >> > i915_get_crtc_scanoutpos; >> > -- >> > 1.8.5.5 >> > >> > ___ >> > Intel-gfx mailing list >> >
Re: [Intel-gfx] [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect
On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote: > Suggested by Ville. Do you mind explaining why this is done at the hdmi level and not the gmbus level? -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 11/9] drm/i915: Opt out of vblank disable timer on >gen2
On Thu, Nov 19, 2015 at 06:35:04PM -0200, Paulo Zanoni wrote: > 2015-11-19 18:06 GMT-02:00 Ville Syrjälä: > > On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote: > >> 2014-05-26 11:26 GMT-03:00 : > >> > From: Ville Syrjälä > >> > > >> > Now that the vblank races are plugged, we can opt out of using > >> > the vblank disable timer and just let vblank interrupts get > >> > disabled immediately when the last reference is dropped. > >> > > >> > Gen2 is the exception since it has no hardware frame counter. > >> > >> Hi > >> > >> Remember last week's FBC vblank optimization patch that had an > >> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? > >> After I fixed the bug in the patch I realized that it was the > >> unbalanced vblank_get() call that moved PC state residency up. > >> > >> I did some experiments, and on my specific BDW machine, after running > >> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. > >> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 > >> residency goes from 60-70% to 85-90% when I revert this patch. I'm > >> running just an idle Cinnamon with an open terminal. > >> > >> So, since the commit message lacks more details, what are the > >> downsides of reverting this patch? What are the advantages of opting > >> out of the vblank timer? I see my desktop does tons and tons of vblank > >> get/put calls per second, so the disable timer makes a lot of sense. > > > > "Idle" desktop :( > > My first realization of this little problem was when I was > implementing runtime PM :) > > > > > > Really the immediate disable should save power. Where are these tons of > > vblank get/puts coming from actually? > > I'll take a finer look tomorrow, but I assume it's probably some > application redrawing. I see it does calm down sometimes, but that's > not enough to get better PC7 residency. > > > > I would assume you'd get a handful > > per frame at most, and that when you're actually doing something. On an > > idle system I would expect nothing at all happens during most frames. > > > > Not sure, but I guess it's possible the extra register accesses in the > > get/puts actually cause the display to exit low power states all the time, > > or something. > > I tried replacing the register macros with the _FW version and that didn't > help. Well, that would just get rid of the unclaimed reg checks. Nothing more I think. > > > > > > There's also this note in Bspec (for HSW at least): > > I think this not is present on most (all?) gens. Doesn't really prove anything. > > "Workaround : Do not enable and unmask this interrupt if the associated > > pipe is disabled. Do not leave this interrupt enabled and unmasked > > after the associated pipe is disabled." > > which we took to mean that having the interrupt masked but enabled is > > fine. > > I'm aware of this, but I think the problem is that the resources > drained by the constant enable+disable+enable+disable outweigh the > resources saved by turning off vblanks. Well the CPU is awake anyway doing the get/put, so not sure why a a few extra register accesses there would have such a huge impact. > Not sure if there's an extra > reason why BSpec asks us to immediately disable vblanks though... > > So, to summarize, the main (only?) reason is the BSpec comment? The point is not to wake up due to interrupts when we don't need them. > > > > But maybe we'd actually have to frob IER too to avoid wasting > > power somehow? > > With the interrupt masked on IMR, I don't think IER matters. I'm not sure anyone actually verified that. > > > > >> I also wish there was some easy way to check how this patch (or its > >> revert) affect a bunch of different workloads... > >> > >> (Also CCing Chris for insightful comments on performance) > > > > IIRC Chris had a patch to not disable the interrupt immediately when > > the refcount drops to 0, but instead delay the disable until the next > > interrupt actually happens. But I guess it didn't go in? Probably I > > should have reviewed it but didn't. It sounds like a decent idea to > > me in any case for the active use case. > > > >> > >> Thanks, > >> Paulo > >> > >> > > >> > Signed-off-by: Ville Syrjälä > >> > --- > >> > drivers/gpu/drm/i915/i915_irq.c | 8 > >> > 1 file changed, 8 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_irq.c > >> > b/drivers/gpu/drm/i915/i915_irq.c > >> > index 28bae6e..4b2e7af 100644 > >> > --- a/drivers/gpu/drm/i915/i915_irq.c > >> > +++ b/drivers/gpu/drm/i915/i915_irq.c > >> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev) > >> > dev->max_vblank_count = 0xff; /* only 24 bits of > >> > frame count */ > >> > } > >> > > >> > + /* > >> > +* Opt out of the vblank disable timer on everything
Re: [Intel-gfx] [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect
On Thu, 2015-11-19 at 20:51 +, Chris Wilson wrote: > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote: > > Suggested by Ville. > > Do you mind explaining why this is done at the hdmi level and not the > gmbus level? To reduce the on/off toggling, since we don't have delayed power-off implemented for power wells. gmbus_xfer also takes a ref to account for accesses from the i2c device node. The solution would be to implement delayed power-off I guess. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] igt/gem_request_retire: Provoke context destruction with active VMAs
On Thu, Nov 19, 2015 at 05:24:03PM +, Tvrtko Ursulin wrote: > >>+ while (i % 4) > >>+ batch[i++] = MI_NOOP; > > Is this ok? I arrived at it by experimentation, kernel was > complaining that relocations are not 4-byte aligned without it. batch is u32, so all is good. What the kernel would be complaining about would be that the batch must be quadword aligned in length I guess. > Although now I'm thinking offsets are in bytes and my calculation > below is completely wrong.. but it did not crash the GPU. Hm. > > >>+ > >>+ if (blit_len == 0) > >>+ blit_len = i; > >>+ } > >>+ > >>+ batch[i++] = MI_BATCH_BUFFER_END; > >>+ batch[i++] = MI_NOOP; > >>+ > >>+ handle = gem_create(fd, sizeof(batch)); > >>+ gem_write(fd, handle, 0, batch, sizeof(batch)); > >>+ > >>+ for (j = 0; j < copies; j++) { > >>+ unsigned int r0 = j * 2; > >>+ unsigned int r1 = r0 + 1; > >>+ > >>+ reloc[r0].target_handle = dst; > >>+ reloc[r0].delta = 0; > >>+ reloc[r0].offset = j * blit_len + 4 * sizeof(uint32_t); > >>+ reloc[r0].presumed_offset = 0; > >>+ reloc[r0].read_domains = I915_GEM_DOMAIN_RENDER; > >>+ reloc[r0].write_domain = I915_GEM_DOMAIN_RENDER; > >>+ > >>+ reloc[r1].target_handle = src; > >>+ reloc[r1].delta = 0; > >>+ reloc[r1].offset = j * blit_len + 7 * sizeof(uint32_t); > >>+ if (intel_gen(intel_get_drm_devid(fd)) >= 8) > >>+ reloc[r1].offset += sizeof(uint32_t); > >>+ reloc[r1].presumed_offset = 0; > >>+ reloc[r1].read_domains = I915_GEM_DOMAIN_RENDER; > >>+ reloc[r1].write_domain = 0; > > > >Just fill the relocation array as you generate the batch. > > To save one loop? :) It was more to do with getting the offsets correct. The blit_len looks very fragile and confusing. > > >>+ } > >>+ > >>+ memset(obj, 0, sizeof(obj)); > >>+ exec.buffer_count = 0; > >>+ obj[exec.buffer_count++].handle = dst; > >>+ if (src != dst) > >>+ obj[exec.buffer_count++].handle = src; > >>+ obj[exec.buffer_count].handle = handle; > >>+ obj[exec.buffer_count].relocation_count = 2 * copies; > >>+ obj[exec.buffer_count].relocs_ptr = (uintptr_t)reloc; > >>+ exec.buffer_count++; > >>+ exec.buffers_ptr = (uintptr_t)obj; > >>+ > > > >memset(, 0, sizeof(exec)); > >instead of: > >>+ exec.batch_start_offset = 0; > >>+ exec.batch_len = i * sizeof(uint32_t); > >>+ exec.DR1 = exec.DR4 = 0; > >>+ exec.num_cliprects = 0; > >>+ exec.cliprects_ptr = 0; > >>+ exec.flags = I915_EXEC_BLT; > >>+ i915_execbuffer2_set_context_id(exec, ctx_id); > >>+ exec.rsvd2 = 0; > >>+ > >>+ ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, ); > >>+ igt_assert_eq(ret, 0); > >gem_execbuf(fd, ); > > Okay, it was copy :) I haven't killed them all yet. > >For the purposes of your test, you don't even need to execute anything, > >just associate the dst handle with an empty batch on the render ring. > >You don't even need a fake relocation since the test already requires a > >fairly recent kernel. > > I thought so but couldn't get it to work. Kept getting EINVAL which > is a pig to find the real reason with execbuf. I'll try some more. > > > > >>+ > >>+ return handle; > >>+} > >>+ > >>+/* > >>+ * A single bo is operated from batchbuffers submitted from two contexts > >>and on > >>+ * different rings. > >>+ * One execbuf finishes way ahead of the other at which point the > >>respective > >>+ * context is destroyed. > >>+ */ > >>+static void > >>+test_retire_vma_not_inactive(int fd) > >>+{ > >>+ uint32_t ctx_id; > >>+ uint32_t src, dst, dst2; > >>+ uint32_t blit_bb, store_bb; > >>+ > >>+ igt_require(HAS_BLT_RING(intel_get_drm_devid(fd))); > >>+ > >>+ ctx_id = gem_context_create(fd); > >>+ igt_assert(ctx_id); > >>+ > >>+ /* Create some bos batch buffers will operate on. */ > >>+ src = gem_create(fd, BO_SIZE); > >>+ igt_assert(src); > >>+ dst = gem_create(fd, BO_SIZE); > >>+ igt_assert(dst); > >>+ dst2 = gem_create(fd, 4096); > >>+ igt_assert(dst2); > >>+ > >>+ /* Submit a long running batch. */ > >>+ blit_bb = blit(fd, dst, src, 0); > >>+ /* Submit a quick batch referencing the same object. */ > >>+ store_bb = store(fd, dst2, src, ctx_id); > >>+ /* Wait for the quick batch to complete. */ > >>+ gem_sync(fd, store_bb); > >>+ gem_sync(fd, dst2); > >>+ gem_close(fd, store_bb); > > > >>+ /* Do it again to ensure in kernel retirement is triggered. */ > >Circumstantial! > > Yes I don't think it is actually required but a leftover from development. > > Key is that retire work handler runs before the context destruction, > since execlist retire is only runs from there, and it is keeping > references to requests which have been processed long ago. > > So I was also thinking - should we call the execlist retire requests > more often in general? Execbuf path
Re: [Intel-gfx] [PATCH] drm/i915: Try to fix MST for SKL
On 2015-11-10 21:15, Jesse Barnes wrote: On 08/17/2015 08:46 AM, ville.syrj...@linux.intel.com wrote: From: Ville SyrjäläSet up the DDI->PLL mapping on SKL also for MST links. Might help make MST operational on SKL. Cc: Maarten Lankhorst Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_ddi.c| 49 ++--- drivers/gpu/drm/i915/intel_dp_mst.c | 8 +- drivers/gpu/drm/i915/intel_drv.h| 2 ++ 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 5dff8b7..10a5a98 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2258,30 +2258,21 @@ uint32_t ddi_signal_levels(struct intel_dp *intel_dp) return DDI_BUF_TRANS_SELECT(level); } -static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) +void intel_ddi_clk_select(struct intel_encoder *encoder, + const struct intel_crtc_state *pipe_config) { - struct drm_encoder *encoder = _encoder->base; - struct drm_device *dev = encoder->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); - enum port port = intel_ddi_get_encoder_port(intel_encoder); - int type = intel_encoder->type; - int hdmi_level; - - if (type == INTEL_OUTPUT_EDP) { - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); - intel_edp_panel_on(intel_dp); - } + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + enum port port = intel_ddi_get_encoder_port(encoder); - if (IS_SKYLAKE(dev)) { - uint32_t dpll = crtc->config->ddi_pll_sel; + if (IS_SKYLAKE(dev_priv)) { + uint32_t dpll = pipe_config->ddi_pll_sel; uint32_t val; /* * DPLL0 is used for eDP and is the only "private" DPLL (as * opposed to shared) on SKL */ - if (type == INTEL_OUTPUT_EDP) { + if (encoder->type == INTEL_OUTPUT_EDP) { WARN_ON(dpll != SKL_DPLL0); val = I915_READ(DPLL_CTRL1); @@ -2289,7 +2280,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) val &= ~(DPLL_CTRL1_HDMI_MODE(dpll) | DPLL_CTRL1_SSC(dpll) | DPLL_CTRL1_LINK_RATE_MASK(dpll)); - val |= crtc->config->dpll_hw_state.ctrl1 << (dpll * 6); + val |= pipe_config->dpll_hw_state.ctrl1 << (dpll * 6); I915_WRITE(DPLL_CTRL1, val); POSTING_READ(DPLL_CTRL1); @@ -2305,11 +2296,29 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) I915_WRITE(DPLL_CTRL2, val); - } else if (INTEL_INFO(dev)->gen < 9) { - WARN_ON(crtc->config->ddi_pll_sel == PORT_CLK_SEL_NONE); - I915_WRITE(PORT_CLK_SEL(port), crtc->config->ddi_pll_sel); + } else if (INTEL_INFO(dev_priv)->gen < 9) { + WARN_ON(pipe_config->ddi_pll_sel == PORT_CLK_SEL_NONE); + I915_WRITE(PORT_CLK_SEL(port), pipe_config->ddi_pll_sel); + } +} + +static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) +{ + struct drm_encoder *encoder = _encoder->base; + struct drm_device *dev = encoder->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *crtc = to_intel_crtc(encoder->crtc); + enum port port = intel_ddi_get_encoder_port(intel_encoder); + int type = intel_encoder->type; + int hdmi_level; + + if (type == INTEL_OUTPUT_EDP) { + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + intel_edp_panel_on(intel_dp); } + intel_ddi_clk_select(intel_encoder, crtc->config); + if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index ebf2054..fd25aeb7 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -163,20 +163,14 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder) intel_mst->port = found->port; if (intel_dp->active_mst_links == 0) { - enum port port = intel_ddi_get_encoder_port(encoder); + intel_ddi_clk_select(encoder, intel_crtc->config); intel_dp_set_link_params(intel_dp, intel_crtc->config); - /* FIXME: add support for SKL */ - if (INTEL_INFO(dev)->gen < 9) - I915_WRITE(PORT_CLK_SEL(port), -
[Intel-gfx] [PATCH i-g-t 1/2] lib/kbl: move KBL check from IS_SKYLAKE() to IS_GEN9()
Remove the KBL check from IS_SKYLAKE() following the kernel definition. Then, add the KBL check to IS_GEN9(). The idea is to avoid confusion. On the kernel side, the mix of SKY and KBL was nacked so the platforms are split. Cc: Rodrigo ViviSigned-off-by: Wayne Boyer --- lib/intel_chipset.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h index 6fcc244..f9fcdd6 100644 --- a/lib/intel_chipset.h +++ b/lib/intel_chipset.h @@ -434,8 +434,7 @@ void intel_check_pch(void); IS_KBL_GT2(devid) || \ IS_KBL_GT3(devid)) -#define IS_SKYLAKE(devid) (IS_KABYLAKE(devid) || \ -IS_SKL_GT1(devid) || \ +#define IS_SKYLAKE(devid) (IS_SKL_GT1(devid) || \ IS_SKL_GT2(devid) || \ IS_SKL_GT3(devid)) @@ -443,7 +442,9 @@ void intel_check_pch(void); (devid) == PCI_CHIP_BROXTON_1 || \ (devid) == PCI_CHIP_BROXTON_2) -#define IS_GEN9(devid) (IS_SKYLAKE(devid) || IS_BROXTON(devid)) +#define IS_GEN9(devid) (IS_KABYLAKE(devid) || \ +IS_SKYLAKE(devid) || \ +IS_BROXTON(devid)) #define IS_965(devid) (IS_GEN4(devid) || \ IS_GEN5(devid) || \ -- 2.6.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 2/2] lib/kbl: Add Kabylake GT4 PCI IDs
Add the Kabylake GT4 PCI IDs as defined in this kernel patch. commit 8b10c0cf21ec84618d4bf02c73c0543500ece68d Author: Deepak SDate: Wed Oct 28 12:21:12 2015 -0700 drm/i915/kbl: Add Kabylake GT4 PCI ID Cc: Rodrigo Vivi Signed-off-by: Wayne Boyer --- lib/intel_chipset.h | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h index f9fcdd6..19fa02f 100644 --- a/lib/intel_chipset.h +++ b/lib/intel_chipset.h @@ -217,13 +217,17 @@ void intel_check_pch(void); #define PCI_CHIP_KABYLAKE_DT_GT2 0x5912 #define PCI_CHIP_KABYLAKE_DT_GT1_5 0x5917 #define PCI_CHIP_KABYLAKE_DT_GT1 0x5902 +#define PCI_CHIP_KABYLAKE_DT_GT4 0x5932 #define PCI_CHIP_KABYLAKE_HALO_GT2 0x591B #define PCI_CHIP_KABYLAKE_HALO_GT3 0x592B #define PCI_CHIP_KABYLAKE_HALO_GT1 0x590B +#define PCI_CHIP_KABYLAKE_HALO_GT4 0x593B #define PCI_CHIP_KABYLAKE_SRV_GT2 0x591A #define PCI_CHIP_KABYLAKE_SRV_GT3 0x592A +#define PCI_CHIP_KABYLAKE_SRV_GT4 0x593A #define PCI_CHIP_KABYLAKE_SRV_GT1 0x590A #define PCI_CHIP_KABYLAKE_WKS_GT2 0x591D +#define PCI_CHIP_KABYLAKE_WKS_GT4 0x593D #define PCI_CHIP_BROXTON_0 0x0A84 #define PCI_CHIP_BROXTON_1 0x1A84 @@ -430,9 +434,15 @@ void intel_check_pch(void); (devid) == PCI_CHIP_KABYLAKE_HALO_GT3|| \ (devid) == PCI_CHIP_KABYLAKE_SRV_GT3) +#define IS_KBL_GT4(devid) ((devid) == PCI_CHIP_KABYLAKE_DT_GT4|| \ +(devid) == PCI_CHIP_KABYLAKE_HALO_GT4|| \ +(devid) == PCI_CHIP_KABYLAKE_SRV_GT4|| \ +(devid) == PCI_CHIP_KABYLAKE_WKS_GT4) + #define IS_KABYLAKE(devid) (IS_KBL_GT1(devid) || \ IS_KBL_GT2(devid) || \ -IS_KBL_GT3(devid)) +IS_KBL_GT3(devid) || \ +IS_KBL_GT4(devid)) #define IS_SKYLAKE(devid) (IS_SKL_GT1(devid) || \ IS_SKL_GT2(devid) || \ -- 2.6.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: take a power domain ref only when needed during HDMI detect
Suggested by Ville. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_hdmi.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index fd86cef..17ced03 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1351,14 +1351,15 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force) struct edid *edid = NULL; bool connected = false; - intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); + if (force) { + intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - if (force) edid = drm_get_edid(connector, intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus)); - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); + } to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: take a power domain reference while checking the HDMI live status
There are platforms that don't need the full GMBUS power domain (PCH, BXT) while others do (VLV/CHV). For optimizing this we would need to add a new power domain, but it's not clear how much we would benefit given the short time we hold the reference. So for now let's keep things simple. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_hdmi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 17ced03..bdd462e 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1393,6 +1393,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); + intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); + while (!live_status && --retry) { live_status = intel_digital_port_connected(dev_priv, hdmi_to_dig_port(intel_hdmi)); @@ -1412,6 +1414,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) } else status = connector_status_disconnected; + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); + return status; } -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
Hi Kevin, On Thu, 2015-11-19 at 04:06 +, Tian, Kevin wrote: > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > Sent: Thursday, November 19, 2015 2:12 AM > > > > [cc +qemu-devel, +paolo, +gerd] > > > > On Tue, 2015-10-27 at 17:25 +0800, Jike Song wrote: > > > Hi all, > > > > > > We are pleased to announce another update of Intel GVT-g for Xen. > > > > > > Intel GVT-g is a full GPU virtualization solution with mediated > > > pass-through, starting from 4th generation Intel Core(TM) processors > > > with Intel Graphics processors. A virtual GPU instance is maintained > > > for each VM, with part of performance critical resources directly > > > assigned. The capability of running native graphics driver inside a > > > VM, without hypervisor intervention in performance critical paths, > > > achieves a good balance among performance, feature, and sharing > > > capability. Xen is currently supported on Intel Processor Graphics > > > (a.k.a. XenGT); and the core logic can be easily ported to other > > > hypervisors. > > > > > > > > > Repositories > > > > > > Kernel: https://github.com/01org/igvtg-kernel (2015q3-3.18.0 branch) > > > Xen: https://github.com/01org/igvtg-xen (2015q3-4.5 branch) > > > Qemu: https://github.com/01org/igvtg-qemu (xengt_public2015q3 branch) > > > > > > > > > This update consists of: > > > > > > - XenGT is now merged with KVMGT in unified repositories(kernel and > > > qemu), but > > currently > > >different branches for qemu. XenGT and KVMGT share same iGVT-g > > > core logic. > > > > Hi! > > > > At redhat we've been thinking about how to support vGPUs from multiple > > vendors in a common way within QEMU. We want to enable code sharing > > between vendors and give new vendors an easy path to add their own > > support. We also have the complication that not all vGPU vendors are as > > open source friendly as Intel, so being able to abstract the device > > mediation and access outside of QEMU is a big advantage. > > > > The proposal I'd like to make is that a vGPU, whether it is from Intel > > or another vendor, is predominantly a PCI(e) device. We have an > > interface in QEMU already for exposing arbitrary PCI devices, vfio-pci. > > Currently vfio-pci uses the VFIO API to interact with "physical" devices > > and system IOMMUs. I highlight /physical/ there because some of these > > physical devices are SR-IOV VFs, which is somewhat of a fuzzy concept, > > somewhere between fixed hardware and a virtual device implemented in > > software. That software just happens to be running on the physical > > endpoint. > > Agree. > > One clarification for rest discussion, is that we're talking about GVT-g vGPU > here which is a pure software GPU virtualization technique. GVT-d (note > some use in the text) refers to passing through the whole GPU or a specific > VF. GVT-d already falls into existing VFIO APIs nicely (though some on-going > effort to remove Intel specific platform stickness from gfx driver). :-) > > > > > vGPUs are similar, with the virtual device created at a different point, > > host software. They also rely on different IOMMU constructs, making use > > of the MMU capabilities of the GPU (GTTs and such), but really having > > similar requirements. > > One important difference between system IOMMU and GPU-MMU here. > System IOMMU is very much about translation from a DMA target > (IOVA on native, or GPA in virtualization case) to HPA. However GPU > internal MMUs is to translate from Graphics Memory Address (GMA) > to DMA target (HPA if system IOMMU is disabled, or IOVA/GPA if system > IOMMU is enabled). GMA is an internal addr space within GPU, not > exposed to Qemu and fully managed by GVT-g device model. Since it's > not a standard PCI defined resource, we don't need abstract this capability > in VFIO interface. > > > > > The proposal is therefore that GPU vendors can expose vGPUs to > > userspace, and thus to QEMU, using the VFIO API. For instance, vfio > > supports modular bus drivers and IOMMU drivers. An intel-vfio-gvt-d > > module (or extension of i915) can register as a vfio bus driver, create > > a struct device per vGPU, create an IOMMU group for that device, and > > register that device with the vfio-core. Since we don't rely on the > > system IOMMU for GVT-d vGPU assignment, another vGPU vendor driver (or > > extension of the same module) can register a "type1" compliant IOMMU > > driver into vfio-core. From the perspective of QEMU then, all of the > > existing vfio-pci code is re-used, QEMU remains largely unaware of any > > specifics of the vGPU being assigned, and the only necessary change so > > far is how QEMU traverses sysfs to find the device and thus the IOMMU > > group leading to the vfio group. > > GVT-g requires to pin guest memory and query GPA->HPA information, > upon which shadow GTTs will be updated accordingly from (GMA->GPA) > to (GMA->HPA). So yes, here a dummy or simple "type1"
Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails
Hi again, On Thu, Nov 19, 2015 at 05:02:04PM +0100, Daniel Vetter wrote: > On Wed, Nov 18, 2015 at 01:43:20PM +0100, Lukas Wunner wrote: > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -408,7 +408,10 @@ static void intel_connector_add_to_fbdev(struct > > intel_connector *connector) > > { > > #ifdef CONFIG_DRM_FBDEV_EMULATION > > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > > - drm_fb_helper_add_one_connector(_priv->fbdev->helper, > > >base); > > + > > + if (dev_priv->fbdev) > > + drm_fb_helper_add_one_connector(_priv->fbdev->helper, > > + >base); > > #endif > > } > > > > @@ -416,7 +419,10 @@ static void intel_connector_remove_from_fbdev(struct > > intel_connector *connector) > > { > > #ifdef CONFIG_DRM_FBDEV_EMULATION > > struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > > - drm_fb_helper_remove_one_connector(_priv->fbdev->helper, > > >base); > > + > > + if (dev_priv->fbdev) > > + drm_fb_helper_remove_one_connector(_priv->fbdev->helper, > > + >base); > > #endif > > } > > > Queued for -next, thanks for the patch. Aside, with this patch and the > static inline dummies from Archit I think we can drop most of the #ifdef > blocks (not the one in debugfs though). Care for a follow-up patch to > remove them around add/remove_one_connector? Looking at it once more I've realized that the fbdev member in struct drm_i915_private is enclosed in an #ifdef CONFIG_DRM_FBDEV_EMULATION, so we can't remove the #ifdefs here: The compiler would complain about non-existence of the dev_priv->fbdev member. On Thu, Nov 19, 2015 at 04:58:44PM +0100, Daniel Vetter wrote: > On Wed, Nov 18, 2015 at 04:29:51PM +0100, Lukas Wunner wrote: > > @@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev) > > > > flush_work(_priv->fbdev_suspend_work); > > > > - async_synchronize_full(); > > + if (!current_is_async()) > > + async_synchronize_full(); > > I think this is a bit too fragile, and the core depency will make merging > tricky. Can't we just push the async_synchronize_full into module unload > for now? Thinking about this a bit longer I believe that if anything the change should make it more robust rather than fragile. After all we eliminate the source of a deadlock that could occur here (async_synchronize_full() waiting forever for itself to finish). That said I'm not married to this solution. If you do find it concerning I could change it according to Ville's suggestion, i.e. splitting intel_fbdev_fini() in two parts. Moving the async_synchronize_full() to i915_driver_unload() would be contrary to the clarity Ville had sought by consolidating everything in intel_fbdev.c. Thanks & best regards, Lukas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/sysfs: Send out uevent when connector->force changes
On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote: > To avoid even more code duplication punt this all to the probe worker, > which needs some slight adjustment to also generate a uevent when the > status has changed to due connector->force. > > v2: Instead of running the output_poll_work (which is kinda the wrong > thing and a layering violation since it's an internal of the probe > helpers), or calling ->detect (which is again a layering violation > since it's used only by probe helpers) just call the official > ->fill_modes function, like a GET_CONNECTOR ioctl call. > > v3: Restore the accidentally removed forced-probe for echo "detect" > > force. > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index 9ac4ffa6cce3..df66d9447cb0 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device, > { > struct drm_connector *connector = to_drm_connector(device); > struct drm_device *dev = connector->dev; > - enum drm_connector_status old_status; > + enum drm_connector_force old_force; > int ret; > > ret = mutex_lock_interruptible(>mode_config.mutex); > if (ret) > return ret; > > - old_status = connector->status; > + old_force = connector->force; > > - if (sysfs_streq(buf, "detect")) { > + if (sysfs_streq(buf, "detect")) > connector->force = 0; > - connector->status = connector->funcs->detect(connector, true); > - } else if (sysfs_streq(buf, "on")) { > + else if (sysfs_streq(buf, "on")) > connector->force = DRM_FORCE_ON; > - } else if (sysfs_streq(buf, "on-digital")) { > + else if (sysfs_streq(buf, "on-digital")) > connector->force = DRM_FORCE_ON_DIGITAL; > - } else if (sysfs_streq(buf, "off")) { > + else if (sysfs_streq(buf, "off")) > connector->force = DRM_FORCE_OFF; > - } else > + else > ret = -EINVAL; > > - if (ret == 0 && connector->force) { > - if (connector->force == DRM_FORCE_ON || > - connector->force == DRM_FORCE_ON_DIGITAL) > - connector->status = connector_status_connected; > - else > - connector->status = connector_status_disconnected; > - if (connector->funcs->force) > - connector->funcs->force(connector); > - } > - > - if (old_status != connector->status) { > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to > %d\n", > + if (old_force != connector->force || !connector->force) { > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d to %d or > reprobing\n", > connector->base.id, > connector->name, > - old_status, connector->status); > + old_force, connector->force); > > - dev->mode_config.delayed_event = true; > - if (dev->mode_config.poll_enabled) > - > schedule_delayed_work(>mode_config.output_poll_work, > - 0); > + connector->funcs->fill_modes(connector, > + dev->mode_config.max_width, > + dev->mode_config.max_height); This now does fill_modes() before we call detect(). We rely on the ordering of detect() before doing fill_modes() as in many cases we use the EDID gathered in detect() to generate the modes (if I am not mistaken, I think we merged those patches to cache the EDID for a detection cycle). -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 11/9] drm/i915: Opt out of vblank disable timer on >gen2
On Thu, Nov 19, 2015 at 10:06:01PM +0200, Ville Syrjälä wrote: > On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote: > > 2014-05-26 11:26 GMT-03:00: > > > From: Ville Syrjälä > > > > > > Now that the vblank races are plugged, we can opt out of using > > > the vblank disable timer and just let vblank interrupts get > > > disabled immediately when the last reference is dropped. > > > > > > Gen2 is the exception since it has no hardware frame counter. > > > > Hi > > > > Remember last week's FBC vblank optimization patch that had an > > erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()? > > After I fixed the bug in the patch I realized that it was the > > unbalanced vblank_get() call that moved PC state residency up. > > > > I did some experiments, and on my specific BDW machine, after running > > "powertop --auto-tune", I get about 15-25% PC7 residency without FBC. > > If I revert this patch, the number jumps to 40-45%. With FBC, the PC7 > > residency goes from 60-70% to 85-90% when I revert this patch. I'm > > running just an idle Cinnamon with an open terminal. > > > > So, since the commit message lacks more details, what are the > > downsides of reverting this patch? What are the advantages of opting > > out of the vblank timer? I see my desktop does tons and tons of vblank > > get/put calls per second, so the disable timer makes a lot of sense. > > "Idle" desktop :( > > Really the immediate disable should save power. Where are these tons of > vblank get/puts coming from actually? I would assume you'd get a handful > per frame at most, and that when you're actually doing something. On an > idle system I would expect nothing at all happens during most frames. Yes, with the immediate disable we end up with a few get/put per frame of rendering, as userspace queries and queues the next flip/swap. With more clients, there are more opportunities for queries prior to queuing the swap. It really shouldn't be more than a handful per frame if all the clients are vrefresh limited. (The oddity was the vblank_mode=0 rendering where we still maintained the handful of get/put per frame...) > > I also wish there was some easy way to check how this patch (or its > > revert) affect a bunch of different workloads... Maybe add the PC residencies to the power meter in intel-gpu-overlay, alongside the RC residencies? > > (Also CCing Chris for insightful comments on performance) > > IIRC Chris had a patch to not disable the interrupt immediately when > the refcount drops to 0, but instead delay the disable until the next > interrupt actually happens. But I guess it didn't go in? Probably I > should have reviewed it but didn't. It sounds like a decent idea to > me in any case for the active use case. The discussion went off into the barriers around the vblank counter, and I forgot to bring it up again. I think even Mario was happy enough about it. Paulo, you may want to try http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly=ef97f4680d316cc8f9656dded1e1e41544c7225e or you can change the KEEPALIVES value in src/sna/sna_dri2.c for a similar effect. -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 06/12] drm/i915: alloc/free the FBC CFB during enable/disable
One of the problems with the current code is that it frees the CFB and releases its drm_mm node as soon as we flip FBC's enable bit. This is bad because after we disable FBC the hardware may still use the CFB for the rest of the frame, so in theory we should only release the drm_mm node one frame after we disable FBC. Otherwise, a stolen memory allocation done right after an FBC disable may result in either corrupted memory for the new owner of that memory region or corrupted screen/underruns in case the new owner changes it while the hardware is still reading it. This case is not exactly easy to reproduce since we currently don't do a lot of stolen memory allocations, but I see patches on the mailing list trying to expose stolen memory to user space, so races will be possible. I thought about three different approaches to solve this, and they all have downsides. The first approach would be to simply use multiple drm_mm nodes and freeing the unused ones only after a frame has passed. The problem with this approach is that since stolen memory is rather small, there's a risk we just won't be able to allocate a new CFB from stolen if the previous one was not freed yet. This could happen in case we quickly disable FBC from pipe A and decide to enable it on pipe B, or just if we change pipe A's fb stride while FBC is enabled. The second approach would be similar to the first one, but maintaining a single drm_mm node and keeping track of when it can be reused. This would remove the disadvantage of not having enough space for two nodes, but would create the new problem where we may not be able to enable FBC at the point intel_fbc_update() is called, so we would have to add more code to retry updating FBC after the time has passed. And that can quickly get too complex since we can get invalidate, flush, disable and other calls in the middle of the wait. Both solutions above - and also the current code - have the problem that we unnecessarily free+realloc FBC during invalidate+flush operations even if the CFB size doesn't change. The third option would be to move the allocation/deallocation to enable/disable. This makes sure that the pipe is always disabled when we allocate/deallocate the CFB, so there's no risk that the FBC hardware may read or write to the memory right after it is freed from drm_mm. The downside is that it is possible for user space to change the buffer stride without triggering a disable/enable - only deactivate/activate -, so we'll have to handle this case somehow - see igt's kms_frontbuffer_tracking test, fbc-stridechange subtest. It could be possible to implement a way to free+alloc the CFB during said stride change, but it would involve a lot of book-keeping - exactly as mentioned above - just for on case, so for now I'll keep it simple and just deactivate FBC. Besides, we may not even need to disable FBC since we do CFB over-allocation. Note from Chris: "Starting a fullscreen client that covers a single monitor in a multi-monitor setup will trigger a change in stride on one of the CRTCs (the monitors will be flipped independently).". It shouldn't be a huge problem if we lose FBC on multi-monitor setups since these setups already have problems reaching deep PC states anyway. v2: Rebase after changing the patch order. v3: - Remove references to the stride change case being "uncommon" and paste Chris' example. - Rebase after a change in a previous patch. Signed-off-by: Paulo Zanoni--- drivers/gpu/drm/i915/intel_fbc.c | 134 --- 1 file changed, 69 insertions(+), 65 deletions(-) I didn't want to resend yet the patches that had "do this trivial change then consider it Reviewed-by", so this one won't apply nicely on top of the others currently on the list. This, along with patch 09 are the only ones missing a RB tag, and I'll skip 09 for now due to the vblank_disable_immediate discussion. diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 6125c7b..958f973 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -64,6 +64,46 @@ static unsigned int get_crtc_fence_y_offset(struct intel_crtc *crtc) return crtc->base.y - crtc->adjusted_y; } +/* + * For SKL+, the plane source size used by the hardware is based on the value we + * write to the PLANE_SIZE register. For BDW-, the hardware looks at the value + * we wrote to PIPESRC. + */ +static void intel_fbc_get_plane_source_size(struct intel_crtc *crtc, + int *width, int *height) +{ + struct intel_plane_state *plane_state = + to_intel_plane_state(crtc->base.primary->state); + int w, h; + + if (intel_rotation_90_or_270(plane_state->base.rotation)) { + w = drm_rect_height(_state->src) >> 16; + h = drm_rect_width(_state->src) >> 16; + } else { + w =
Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails
On Thu, Nov 19, 2015 at 04:58:44PM +0100, Daniel Vetter wrote: > On Wed, Nov 18, 2015 at 04:29:51PM +0100, Lukas Wunner wrote: > > @@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev) > > > > flush_work(_priv->fbdev_suspend_work); > > > > - async_synchronize_full(); > > + if (!current_is_async()) > > + async_synchronize_full(); > > I think this is a bit too fragile, and the core depency will make merging > tricky. Can't we just push the async_synchronize_full into module unload > for now? (intel_fbdev_fini() is already module unload, right? Do you mean just move the async handling into i915_driver_unload() so that we have a single spot for all future potential users of the async framework?) And optimising module unload to avoid one potential grace period when we already have a bunch of grace period waits seems overkill. The alternative to using async_synchronize_full() would be to use an async-domain. -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/2] drm/i915: take a power domain ref only when needed during HDMI detect
On Thu, Nov 19, 2015 at 11:01:49PM +0200, Imre Deak wrote: > On Thu, 2015-11-19 at 20:51 +, Chris Wilson wrote: > > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote: > > > Suggested by Ville. > > > > Do you mind explaining why this is done at the hdmi level and not the > > gmbus level? > > To reduce the on/off toggling, since we don't have delayed power-off > implemented for power wells. gmbus_xfer also takes a ref to account for > accesses from the i2c device node. The solution would be to implement > delayed power-off I guess. As we chase ever finer grained wakelocks, yeah. Looking at the other users of gmbus, they are the old platforms (dvo, sdvo, crt, lvds) so not worth generalising the optimisation of holding the wakelock across the entire i2c operation, I guess? -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/7] drm/i915: sseu: move sseu_dev_status to i915_drv.h
On Wed, Oct 21, 2015 at 06:40:31PM +0300, Imre Deak wrote: > The data in this struct is provided both by getting the > slice/subslice/eu features available on a given platform and the actual > runtime state of these same features which depends on the HW's current > power saving state. > > Atm members of this struct are duplicated in sseu_dev_status and I> intel_device_info. For clarity and code reuse we can share one struct > for both of the above purposes. This patch only moves the struct to the > header file, the next patch will convert users of intel_device_info to > use this struct too. > > Instead of unsigned int u8 is used now, which is big enough and is used > anyway in intel_device_info. > > No functional change. > > Signed-off-by: Imre Deak> --- > drivers/gpu/drm/i915/i915_debugfs.c | 29 - > drivers/gpu/drm/i915/i915_drv.h | 8 > 2 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index a3b22bd..3dd7076 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -4945,16 +4945,8 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops, > i915_cache_sharing_get, i915_cache_sharing_set, > "%llu\n"); > > -struct sseu_dev_status { > - unsigned int slice_total; > - unsigned int subslice_total; > - unsigned int subslice_per_slice; > - unsigned int eu_total; > - unsigned int eu_per_subslice; > -}; > - > static void cherryview_sseu_device_status(struct drm_device *dev, > - struct sseu_dev_status *stat) > + struct sseu_dev_info *stat) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int ss_max = 2; > @@ -4980,13 +4972,14 @@ static void cherryview_sseu_device_status(struct > drm_device *dev, >((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) + >((sig2[ss] & CHV_EU311_PG_ENABLE) ? 0 : 2); > stat->eu_total += eu_cnt; > - stat->eu_per_subslice = max(stat->eu_per_subslice, eu_cnt); > + stat->eu_per_subslice = max_t(unsigned int, > + stat->eu_per_subslice, eu_cnt); > } > stat->subslice_total = stat->subslice_per_slice; > } > > static void gen9_sseu_device_status(struct drm_device *dev, > - struct sseu_dev_status *stat) > + struct sseu_dev_info *stat) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int s_max = 3, ss_max = 4; > @@ -5040,18 +5033,20 @@ static void gen9_sseu_device_status(struct drm_device > *dev, > eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] & > eu_mask[ss%2]); > stat->eu_total += eu_cnt; > - stat->eu_per_subslice = max(stat->eu_per_subslice, > - eu_cnt); > + stat->eu_per_subslice = max_t(unsigned int, > + stat->eu_per_subslice, > + eu_cnt); > } > > stat->subslice_total += ss_cnt; > - stat->subslice_per_slice = max(stat->subslice_per_slice, > -ss_cnt); > + stat->subslice_per_slice = max_t(unsigned int, > + stat->subslice_per_slice, > + ss_cnt); > } > } > > static void broadwell_sseu_device_status(struct drm_device *dev, > - struct sseu_dev_status *stat) > + struct sseu_dev_info *stat) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int s; > @@ -5079,7 +5074,7 @@ static int i915_sseu_status(struct seq_file *m, void > *unused) > { > struct drm_info_node *node = (struct drm_info_node *) m->private; > struct drm_device *dev = node->minor->dev; > - struct sseu_dev_status stat; > + struct sseu_dev_info stat; If you're going through this rename pain with the type anyway you may as well s/stat/info. Also, I never understood what "sseu" was supposed to be short for. The spec calls these "global attributes" which is admittedly a way too generic name. As a suggestion based on hindsight, I believe the following would be a bit nicer. struct slice_attributes { u8 slice_count; u8 eu_total; /* This is sort of useless since if eu_total isn't trivially * eu_per_subslice * subslice_count * slice_count, then we * need to know exactly which subslice is missing EUs. */ struct
Re: [Intel-gfx] [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect
On Thu, 2015-11-19 at 21:38 +, Chris Wilson wrote: > On Thu, Nov 19, 2015 at 11:01:49PM +0200, Imre Deak wrote: > > On Thu, 2015-11-19 at 20:51 +, Chris Wilson wrote: > > > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote: > > > > Suggested by Ville. > > > > > > Do you mind explaining why this is done at the hdmi level and not the > > > gmbus level? > > > > To reduce the on/off toggling, since we don't have delayed power-off > > implemented for power wells. gmbus_xfer also takes a ref to account for > > accesses from the i2c device node. The solution would be to implement > > delayed power-off I guess. > > As we chase ever finer grained wakelocks, yeah. Ok, the delayed-off stuff shouldn't be difficult, since in case of power wells we hold an RPM ref. So AFAICS we would only need to synchronize during system suspend and driver unload. And then find a good timeout value.. > Looking at the other users of gmbus, they are the old platforms (dvo, > sdvo, crt, lvds) so not worth generalising the optimisation of > holding the wakelock across the entire i2c operation, I guess? If you mean to take an extra ref around the higher level op in those places too: well in case of CRT it's also in new HW where it matters, so that's inconsistent and I think we should do it there too. For others it doesn't matter I think. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: take a power domain ref only when needed during HDMI detect
On Thu, 2015-11-19 at 23:50 +0200, Imre Deak wrote: > On Thu, 2015-11-19 at 21:38 +, Chris Wilson wrote: > > On Thu, Nov 19, 2015 at 11:01:49PM +0200, Imre Deak wrote: > > > On Thu, 2015-11-19 at 20:51 +, Chris Wilson wrote: > > > > On Thu, Nov 19, 2015 at 08:55:00PM +0200, Imre Deak wrote: > > > > > Suggested by Ville. > > > > > > > > Do you mind explaining why this is done at the hdmi level and > > > > not the > > > > gmbus level? > > > > > > To reduce the on/off toggling, since we don't have delayed power- > > > off > > > implemented for power wells. gmbus_xfer also takes a ref to > > > account for > > > accesses from the i2c device node. The solution would be to > > > implement > > > delayed power-off I guess. > > > > As we chase ever finer grained wakelocks, yeah. > > Ok, the delayed-off stuff shouldn't be difficult, since in case of > power wells we hold an RPM ref. So AFAICS we would only need to > synchronize during system suspend and driver unload. And then find a > good timeout value.. > > > Looking at the other users of gmbus, they are the old platforms > > (dvo, > > sdvo, crt, lvds) so not worth generalising the optimisation of > > holding the wakelock across the entire i2c operation, I guess? > > If you mean to take an extra ref around the higher level op in those > places too: well in case of CRT it's also in new HW where it matters, > so that's inconsistent and I think we should do it there too. For > others it doesn't matter I think. Err, we do take a ref in the CRT detect code, but it's the port power domain. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails
Hi Chris, On Thu, Nov 19, 2015 at 09:46:34PM +, Chris Wilson wrote: > On Thu, Nov 19, 2015 at 04:58:44PM +0100, Daniel Vetter wrote: > > On Wed, Nov 18, 2015 at 04:29:51PM +0100, Lukas Wunner wrote: > > > @@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev) > > > > > > flush_work(_priv->fbdev_suspend_work); > > > > > > - async_synchronize_full(); > > > + if (!current_is_async()) > > > + async_synchronize_full(); > > > > I think this is a bit too fragile, and the core depency will make merging > > tricky. Can't we just push the async_synchronize_full into module unload > > for now? > > (intel_fbdev_fini() is already module unload, right? With my patch it'd be called from a 2nd site: intel_fbdev_initial_config(). To tear down the fbdev if initialization failed. However since that function is run asynchronously, async_synchronize_full() deadlocks as it waits forever for itself to finish. > Do you mean just > move the async handling into i915_driver_unload() so that we have a > single spot for all future potential users of the async framework?) That precisely was the motivation for Ville's cleanup e00bf69644ba a few days ago, to consolidate things in one place. However he chose to move everything into intel_fbdev.c. That leaves three options to avoid the deadlock: (a) call async_synchronize_full() conditionally if (!current_is_async()), that's what I did. That way it only gets called when the function is entered from i915_driver_unload(), not when it's entered from intel_fbdev_initial_config(). (b) split intel_fbdev_fini() in two, first part is called only called on module unload. (c) revert Ville's patch, consolidate the async stuff in i915_dma.c. > And optimising module unload to avoid one potential grace period when we > already have a bunch of grace period waits seems overkill. > > The alternative to using async_synchronize_full() would be to use an > async-domain. But an async domain wouldn't solve the deadlock, would it? Best regards, Lukas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915: sseu: use sseu_dev_info in device info
On Wed, Oct 21, 2015 at 06:40:32PM +0300, Imre Deak wrote: > Move all slice/subslice/eu related properties to the sseu_dev_info > struct. > > No functional change. > > Signed-off-by: Imre DeakReviewed-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_debugfs.c | 25 > drivers/gpu/drm/i915/i915_dma.c | 34 > + > drivers/gpu/drm/i915/i915_drv.h | 16 ++-- > drivers/gpu/drm/i915/intel_lrc.c| 14 +++--- > drivers/gpu/drm/i915/intel_pm.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++-- > 6 files changed, 47 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 3dd7076..de188d0 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -5017,7 +5017,7 @@ static void gen9_sseu_device_status(struct drm_device > *dev, > stat->slice_total++; > > if (IS_SKYLAKE(dev)) > - ss_cnt = INTEL_INFO(dev)->subslice_per_slice; > + ss_cnt = INTEL_INFO(dev)->sseu.subslice_per_slice; > > for (ss = 0; ss < ss_max; ss++) { > unsigned int eu_cnt; > @@ -5055,15 +5055,16 @@ static void broadwell_sseu_device_status(struct > drm_device *dev, > stat->slice_total = hweight32(slice_info & GEN8_LSLICESTAT_MASK); > > if (stat->slice_total) { > - stat->subslice_per_slice = INTEL_INFO(dev)->subslice_per_slice; > + stat->subslice_per_slice = > + INTEL_INFO(dev)->sseu.subslice_per_slice; > stat->subslice_total = stat->slice_total * > stat->subslice_per_slice; > - stat->eu_per_subslice = INTEL_INFO(dev)->eu_per_subslice; > + stat->eu_per_subslice = INTEL_INFO(dev)->sseu.eu_per_subslice; > stat->eu_total = stat->eu_per_subslice * stat->subslice_total; > > /* subtract fused off EU(s) from enabled slice(s) */ > for (s = 0; s < stat->slice_total; s++) { > - u8 subslice_7eu = INTEL_INFO(dev)->subslice_7eu[s]; > + u8 subslice_7eu = INTEL_INFO(dev)->sseu.subslice_7eu[s]; > > stat->eu_total -= hweight8(subslice_7eu); > } > @@ -5081,21 +5082,21 @@ static int i915_sseu_status(struct seq_file *m, void > *unused) > > seq_puts(m, "SSEU Device Info\n"); > seq_printf(m, " Available Slice Total: %u\n", > -INTEL_INFO(dev)->slice_total); > +INTEL_INFO(dev)->sseu.slice_total); > seq_printf(m, " Available Subslice Total: %u\n", > -INTEL_INFO(dev)->subslice_total); > +INTEL_INFO(dev)->sseu.subslice_total); > seq_printf(m, " Available Subslice Per Slice: %u\n", > -INTEL_INFO(dev)->subslice_per_slice); > +INTEL_INFO(dev)->sseu.subslice_per_slice); > seq_printf(m, " Available EU Total: %u\n", > -INTEL_INFO(dev)->eu_total); > +INTEL_INFO(dev)->sseu.eu_total); > seq_printf(m, " Available EU Per Subslice: %u\n", > -INTEL_INFO(dev)->eu_per_subslice); > +INTEL_INFO(dev)->sseu.eu_per_subslice); > seq_printf(m, " Has Slice Power Gating: %s\n", > -yesno(INTEL_INFO(dev)->has_slice_pg)); > +yesno(INTEL_INFO(dev)->sseu.has_slice_pg)); > seq_printf(m, " Has Subslice Power Gating: %s\n", > -yesno(INTEL_INFO(dev)->has_subslice_pg)); > +yesno(INTEL_INFO(dev)->sseu.has_subslice_pg)); > seq_printf(m, " Has EU Power Gating: %s\n", > -yesno(INTEL_INFO(dev)->has_eu_pg)); > +yesno(INTEL_INFO(dev)->sseu.has_eu_pg)); > > seq_puts(m, "SSEU Device Status\n"); > memset(, 0, sizeof(stat)); > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 2336af9..be8e141 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -154,12 +154,12 @@ static int i915_getparam(struct drm_device *dev, void > *data, > value = 1; > break; > case I915_PARAM_SUBSLICE_TOTAL: > - value = INTEL_INFO(dev)->subslice_total; > + value = INTEL_INFO(dev)->sseu.subslice_total; > if (!value) > return -ENODEV; > break; > case I915_PARAM_EU_TOTAL: > - value = INTEL_INFO(dev)->eu_total; > + value = INTEL_INFO(dev)->sseu.eu_total; > if (!value) > return -ENODEV; > break; > @@ -553,10 +553,10 @@ static void i915_dump_device_info(struct > drm_i915_private *dev_priv) > static void
Re: [Intel-gfx] [PATCH 3/7] drm/i915: sseu: simplify debugfs status/info printing
On Wed, Oct 21, 2015 at 06:40:33PM +0300, Imre Deak wrote: > Signed-off-by: Imre DeakReviewed-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_debugfs.c | 55 > +++-- > 1 file changed, 29 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index de188d0..183c1f2 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -5071,6 +5071,33 @@ static void broadwell_sseu_device_status(struct > drm_device *dev, > } > } > > +static void i915_print_sseu_info(struct seq_file *m, bool is_available_info, > + const struct sseu_dev_info *sseu) > +{ > + const char *type = is_available_info ? "Available" : "Enabled"; > + > + seq_printf(m, " %s Slice Total: %u\n", type, > +sseu->slice_total); > + seq_printf(m, " %s Subslice Total: %u\n", type, > +sseu->subslice_total); > + seq_printf(m, " %s Subslice Per Slice: %u\n", type, > +sseu->subslice_per_slice); > + seq_printf(m, " %s EU Total: %u\n", type, > +sseu->eu_total); > + seq_printf(m, " %s EU Per Subslice: %u\n", type, > +sseu->eu_per_subslice); > + > + if (!is_available_info) > + return; > + > + seq_printf(m, " Has Slice Power Gating: %s\n", > +yesno(sseu->has_slice_pg)); > + seq_printf(m, " Has Subslice Power Gating: %s\n", > +yesno(sseu->has_subslice_pg)); > + seq_printf(m, " Has EU Power Gating: %s\n", > +yesno(sseu->has_eu_pg)); > +} > + > static int i915_sseu_status(struct seq_file *m, void *unused) > { > struct drm_info_node *node = (struct drm_info_node *) m->private; > @@ -5081,22 +5108,7 @@ static int i915_sseu_status(struct seq_file *m, void > *unused) > return -ENODEV; > > seq_puts(m, "SSEU Device Info\n"); > - seq_printf(m, " Available Slice Total: %u\n", > -INTEL_INFO(dev)->sseu.slice_total); > - seq_printf(m, " Available Subslice Total: %u\n", > -INTEL_INFO(dev)->sseu.subslice_total); > - seq_printf(m, " Available Subslice Per Slice: %u\n", > -INTEL_INFO(dev)->sseu.subslice_per_slice); > - seq_printf(m, " Available EU Total: %u\n", > -INTEL_INFO(dev)->sseu.eu_total); > - seq_printf(m, " Available EU Per Subslice: %u\n", > -INTEL_INFO(dev)->sseu.eu_per_subslice); > - seq_printf(m, " Has Slice Power Gating: %s\n", > -yesno(INTEL_INFO(dev)->sseu.has_slice_pg)); > - seq_printf(m, " Has Subslice Power Gating: %s\n", > -yesno(INTEL_INFO(dev)->sseu.has_subslice_pg)); > - seq_printf(m, " Has EU Power Gating: %s\n", > -yesno(INTEL_INFO(dev)->sseu.has_eu_pg)); > + i915_print_sseu_info(m, true, _INFO(dev)->sseu); > > seq_puts(m, "SSEU Device Status\n"); > memset(, 0, sizeof(stat)); > @@ -5107,16 +5119,7 @@ static int i915_sseu_status(struct seq_file *m, void > *unused) > } else if (INTEL_INFO(dev)->gen >= 9) { > gen9_sseu_device_status(dev, ); > } > - seq_printf(m, " Enabled Slice Total: %u\n", > -stat.slice_total); > - seq_printf(m, " Enabled Subslice Total: %u\n", > -stat.subslice_total); > - seq_printf(m, " Enabled Subslice Per Slice: %u\n", > -stat.subslice_per_slice); > - seq_printf(m, " Enabled EU Total: %u\n", > -stat.eu_total); > - seq_printf(m, " Enabled EU Per Subslice: %u\n", > -stat.eu_per_subslice); > + i915_print_sseu_info(m, false, ); > > return 0; > } > -- > 2.1.4 > -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Don't override output type for DDI HDMI
Currently a DDI port may register the DP hotplug handler even though it's used with HDMI, and the DP HPD handler overrides the encoder type forcibly to DP. This caused the inconsistency on a machine connected with a HDMI monitor; upon a hotplug event, the DDI port is suddenly switched to be handled as a DP although the same monitor is kept connected, and this leads to the erroneous blank output. This patch papers over the bug by excluding the previous HDMI encoder type from this override. This should be fixed more fundamentally, e.g. by moving the encoder type reset from the HPD or by having individual encoder objects for HDMI and DP. But since the bug has been present for a long time (3.17), it's better to have a quick-n-dirty fix for now, and keep working on a cleaner fix. Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190 Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)') Cc:# v3.17+ Signed-off-by: Takashi Iwai --- drivers/gpu/drm/i915/intel_dp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 09bdd94ca3ba..d34e64300d66 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5153,7 +5153,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) enum intel_display_power_domain power_domain; enum irqreturn ret = IRQ_NONE; - if (intel_dig_port->base.type != INTEL_OUTPUT_EDP) + if (intel_dig_port->base.type != INTEL_OUTPUT_EDP && + intel_dig_port->base.type != INTEL_OUTPUT_HDMI) intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT; if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) { -- 2.6.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
On 19/11/2015 09:40, Gerd Hoffmann wrote: >> > But this code should be >> > minor to be maintained in libvirt. > As far I know libvirt only needs to discover those devices. If they > look like sr/iov devices in sysfs this might work without any changes to > libvirt. I don't think they will look like SR/IOV devices. The interface may look a little like the sysfs interface that GVT-g is already using. However, it should at least be extended to support multiple vGPUs in a single VM. This might not be possible for Intel integrated graphics, but it should definitely be possible for discrete graphics cards. Another nit is that the VM id should probably be replaced by a UUID (because it's too easy to stumble on an existing VM id), assuming a VM id is needed at all. Paolo ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Fix possible null dereference in two debugfs functions
Found by static code analysis tool. Signed-off-by: Namrta SalonieSigned-off-by: Deepak S --- drivers/gpu/drm/i915/i915_debugfs.c | 19 +++ 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index a3b22bd..d1719e9 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1873,6 +1873,9 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) struct drm_i915_private *dev_priv = dev->dev_private; ifbdev = dev_priv->fbdev; + if (!ifbdev) + goto fb_loop; + fb = to_intel_framebuffer(ifbdev->helper.fb); seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ", @@ -1884,8 +1887,9 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data) atomic_read(>base.refcount.refcount)); describe_obj(m, fb->obj); seq_putc(m, '\n'); -#endif +fb_loop: +#endif mutex_lock(>mode_config.fb_lock); drm_for_each_fb(drm_fb, dev) { fb = to_intel_framebuffer(drm_fb); @@ -3868,12 +3872,19 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, enum intel_pipe_crc_source source) { struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_pipe_crc *pipe_crc = _priv->pipe_crc[pipe]; - struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, - pipe)); + struct intel_pipe_crc *pipe_crc; + struct intel_crtc *crtc; u32 val = 0; /* shut up gcc */ int ret; + if ((pipe < PIPE_A) || (pipe >= I915_MAX_PIPES)) + return -EINVAL; + + pipe_crc = _priv->pipe_crc[pipe]; + + crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, + pipe)); + if (pipe_crc->source == source) return 0; -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v2] lib/igt_core: Add kmsg capture and dumping
On Thu, Nov 19, 2015 at 12:35:23PM +0200, Joonas Lahtinen wrote: > +static void _igt_kmsg_capture_reset(void) > +{ > + if (igt_kmsg_capture_fd != -1) > + close(igt_kmsg_capture_fd); > + > + igt_kmsg_capture_fd = open("/dev/kmsg", > +O_RDONLY | O_NONBLOCK); We should O_CLOEXEC as well. > + > + if (igt_kmsg_capture_fd == -1) > + return; > + > + lseek(igt_kmsg_capture_fd, 0, SEEK_END); > + > + if (igt_kmsg_capture_dump_buf == NULL) > + igt_kmsg_capture_dump_buf = > + malloc(IGT_KMSG_CAPTURE_DUMP_BUF_SIZE); > + > + if (igt_kmsg_capture_dump_buf == NULL) > + igt_warn("Unable to allocate memory, " > + "will not dump kmsg.\n"); If we allocate first, then bail, we know if we have the fd we have the buffer as well. > +} > + > +static int _igt_kmsg_capture_dump(bool notify_empty, int show_priority) > +{ > + size_t nbytes; > + int nlines; > + int prefix; > + int facility; > + int priority; > + char *p; > + int c; > + > + if (igt_kmsg_capture_fd == -1 || > + igt_kmsg_capture_dump_buf == NULL) > + return 0; i.e. we can just do if (fd == -1) return 0; > + > + nlines = 0; > + do { > + errno = 0; > + nbytes = read(igt_kmsg_capture_fd, > + igt_kmsg_capture_dump_buf, > + IGT_KMSG_CAPTURE_DUMP_BUF_SIZE); > + > + if (nbytes == -1) > + continue; > + > + sscanf(igt_kmsg_capture_dump_buf, "%d;", ); > + priority = prefix & 0x7; > + > + if (priority > show_priority) > + continue; > + > + if (!nlines) > + fprintf(stderr, " KMSG \n"); > + > + p = strchr(igt_kmsg_capture_dump_buf, ';') + 1; > + while (p - igt_kmsg_capture_dump_buf < nbytes) { > + if (*p != '\\') { > + fputc(*p++, stderr); > + continue; > + } > + sscanf(p, "\\x%x", ); > + fputc(c, stderr); > + p += 4; Maybe: /* Decode non-printable characters escaped by '\x01' */ int c = *p++; if (c == '\\') { if (p - igt_kmgs_capture_dump_buf > nbytes - 3) break; sscanf(p+1, "%x", ); p += 3; } fputc(c, stderr); > + } > + nlines++; > + } while(errno == 0); > + > + if (nlines) { > + fprintf(stderr, " END \n"); > + } else { > + if (notify_empty) > + fprintf(stderr, "No kmsg.\n"); > + } > + > + if (errno != EAGAIN) > + fprintf(stderr, "Error: Incomplete kmsg!\n"); > + > + close(igt_kmsg_capture_fd); > + igt_kmsg_capture_fd = -1; > + > + free(igt_kmsg_capture_dump_buf); > + igt_kmsg_capture_dump_buf = NULL; Hmm, single-shot. I have in mind more of an automatic debug feature coupled with error detection like how we automatically go back and print the debug log if the test fails. As I understand it, with a FAIL (KMSG) we currently lose any lower priority output. The integration looks good to me otherwise. But someone else will have to vouch for test-runner/piglit handling of "FAIL (KMSG)" (I used WARN). -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 4/4] drm/i915: Fix potential NULL pointer de-reference in ggtt unbind.
On Thu, Nov 19, 2015 at 04:57:31PM +0530, Namrta Salonie wrote: > Found by static analysis tool. What bug? Please fix any callsite where this is an issue, rather than paper over the bug. -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/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.
On Thu, Nov 19, 2015 at 11:24:22AM +, Zanoni, Paulo R wrote: > Em Qui, 2015-11-19 às 13:16 +0200, Jani Nikula escreveu: > > On Wed, 18 Nov 2015, "Zanoni, Paulo R"> > wrote: > > > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: > > > > Cc: Paulo Zanoni > > > s/Cc/Reviewed-by/ > > > > I don't think our patchwork is quite smart enough for that yet, so > > for > > future reference, please be write the whole thing. > > I completely forgot about this. Sorry. > > I guess that also means that if I say "If you do this and this and > that, then I'll give you Reviewed-by: Paulo Zanoni l.com> it will mark the patch as reviewed even though it needs rework, > right? I guess we just can't trust the PW automatic tag parsing since > it can have either false positives or false negatives. Not until some > major breakthroughs in AI. The Reviewed-by parsing is quite simple: it will match '^Reviewed-by:'. That means indenting the tag should be enough to bypass patchwork's accounting. That said, I think we need to extend the language we use for review and make patchwork understand it. For instance, being able to review several patches in one go could be done with: Patches 1-3,6: Reviewed-by: Damien Lespiau damien.lesp...@intel.com (See: https://github.com/dlespiau/patchwork/issues/27) > Is suggesting deprecating the use of emails as a way to handle patches > still considered trolling? No :) The way I see it is that it's easier to progressively enhance what we have to do what we want -- I'll be the first to say patchwork is very fragile today -- rather than switching to something totally different, probably closing the door to non-intel contributors and suddenly having two different systems for core DRM patch handling, among other things. Either way, we have to try to improve what we have. I took one path but it doesn't mean that was the best, someone else can always try to take another set of tools and convince/force people to use that. The goals are the important bit: test every patch before it's merged into -nightly, improve the developer experience and patch flow/tracking along the way. For the first goal, we are almost there (in terms of patchwork, the CI system, piglit, intel-gpu-tools, ... still need quite a bit for work): For instance on series #890, I've uploaded checkpatch.pl test results: http://patchwork.freedesktop.org/series/890/ I'll be deploying that checkpatch.pl test in the next week or so as an example of patchwork/test integration. QA is looking into that integration with BATs at the moment. Of course, email/notifications are part of the plan once happy enough with the tests. For the second goal, it's a long process of small improvements. We're really not there, but interesting things have been created already: I'm quite a fan of git-pw to retrieve series from the ml, for those series correctly parsed that is... Which leads me to the last thing: parsing things from the ml is fragile. The main problems are both people and to a lesser extend mailing-lists: using mails to carry patches does have problems, the vast majority of problems come from people though. People will get stuff wrong: attach patches to the wrong message-id, do things that are plain weird like suddenly attaching patch "2.4/10" as a way to say "oh actually insert a 11th patch in the middle there", typo the review tags, ... I think the last part is solvable, by having a tool wrapping git send-email to do the right things, or at least deterministic things, when sending patches and reviews. That's a bit blue sky stuff, I certainly would love to get there eventually though. Thinking about it, it's wouldn't actually be that complicated to have a start of such a tool, I've captured the first simple thoughts here: https://github.com/dlespiau/patchwork/issues/81 Oh well, it's a way longer email than the one I wanted to write. HTH, -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v4] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW
CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock used for timing execution of tests. When fetching either the starting or ending time of a test, show the time as -1.000s. v4: - Introduce time_valid macro (Chris) - Reduce amount of boilerplate code for calculating elapsed time v3: - Do not exit directly from handler (Chris) - Show elapsed time as -1 if it is not calculable v2: - Cache the used clock (Chris) - Do not change the clock during execution - Spit out and error if monotonic time can not be read Cc: Thomas WoodCc: Chris Wilson Signed-off-by: Joonas Lahtinen --- lib/igt_core.c | 53 - 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/lib/igt_core.c b/lib/igt_core.c index 04a0ab2..3269804 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -220,6 +220,7 @@ static char *run_single_subtest = NULL; static bool run_single_subtest_found = false; static const char *in_subtest = NULL; static struct timespec subtest_time; +static clockid_t igt_clock = (clockid_t)-1; static bool in_fixture = false; static bool test_with_subtests = false; static bool in_atexit_handler = false; @@ -337,14 +338,48 @@ static void kmsg(const char *format, ...) fclose(file); } -static void gettime(struct timespec *ts) +#define time_valid(ts) ((ts)->tv_sec || (ts)->tv_nsec) + +static double +time_elapsed(struct timespec *then, +struct timespec* now) { + double elapsed = -1.; + + if (time_valid(then) && time_valid(now)) { + elapsed = now->tv_sec - then->tv_sec; + elapsed += (now->tv_nsec - then->tv_nsec) * 1e-9; + } + + return elapsed; +} + +static int gettime(struct timespec *ts) { memset(ts, 0, sizeof(*ts)); + errno = 0; + // Stay on the same clock for consistency. + if (igt_clock != (clockid_t)-1) { + if (clock_gettime(igt_clock, ts)) + goto error; + return 0; + } + +#ifdef CLOCK_MONOTONIC_RAW + if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_RAW, ts)) + return 0; +#endif #ifdef CLOCK_MONOTONIC_COARSE - if (clock_gettime(CLOCK_MONOTONIC_COARSE, ts)) + if (!clock_gettime(igt_clock = CLOCK_MONOTONIC_COARSE, ts)) + return 0; #endif - clock_gettime(CLOCK_MONOTONIC, ts); + if (!clock_gettime(igt_clock = CLOCK_MONOTONIC, ts)) + return 0; +error: + igt_warn("Could not read monotonic time: %s\n", +strerror(errno)); + + return -errno; } bool __igt_fixture(void) @@ -831,15 +866,11 @@ static void exit_subtest(const char *) __attribute__((noreturn)); static void exit_subtest(const char *result) { struct timespec now; - double elapsed; gettime(); - elapsed = now.tv_sec - subtest_time.tv_sec; - elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9; - printf("%sSubtest %s: %s (%.3fs)%s\n", (!__igt_plain_output) ? "\x1b[1m" : "", - in_subtest, result, elapsed, + in_subtest, result, time_elapsed(_time, ), (!__igt_plain_output) ? "\x1b[0m" : ""); fflush(stdout); @@ -1088,12 +1119,9 @@ void igt_exit(void) if (!test_with_subtests) { struct timespec now; - double elapsed; const char *result; gettime(); - elapsed = now.tv_sec - subtest_time.tv_sec; - elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9; switch (igt_exitcode) { case IGT_EXIT_SUCCESS: @@ -1109,8 +1137,7 @@ void igt_exit(void) result = "FAIL"; } - - printf("%s (%.3fs)\n", result, elapsed); + printf("%s (%.3fs)\n", result, time_elapsed(_time, )); exit(igt_exitcode); } -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: Fix for potential NULL pointer dereference at ctx access.
Added a null check for the context pointer, before de-referencing it. Found by static analysis tool. Signed-off-by: Namrta Salonie--- drivers/gpu/drm/i915/i915_gpu_error.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 2f04e4f..29ecd0c 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -1036,7 +1036,7 @@ static void i915_gem_record_rings(struct drm_device *dev, * for it to be useful (e.g. dump the context being * executed). */ - if (request) + if (request && request->ctx) rbuf = request->ctx->engine[ring->id].ringbuf; else rbuf = ring->default_context->engine[ring->id].ringbuf; -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/4] Fix issues reported by static code analysis tool
The patchset is created to fix the issues static analysis tool has reported Namrta Salonie (4): drm/i915: Fix for potential NULL pointer dereference at ctx access. drm/i915: Fix possible null dereference in two debugfs functions drm/i915 : Fix to remove unnecsessary checks in postclose function. drm/i915: Fix potential NULL pointer de-reference in ggtt unbind. drivers/gpu/drm/i915/i915_debugfs.c | 19 +++ drivers/gpu/drm/i915/i915_dma.c |2 -- drivers/gpu/drm/i915/i915_drv.h |5 - drivers/gpu/drm/i915/i915_gpu_error.c |2 +- 4 files changed, 20 insertions(+), 8 deletions(-) -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915: Fix potential NULL pointer de-reference in ggtt unbind.
Found by static analysis tool. Signed-off-by: Namrta Salonie--- drivers/gpu/drm/i915/i915_drv.h |5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8afda45..705b1e6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3097,7 +3097,10 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj, static inline int i915_gem_object_ggtt_unbind(struct drm_i915_gem_object *obj) { - return i915_vma_unbind(i915_gem_obj_to_ggtt(obj)); + struct i915_vma *vma = i915_gem_obj_to_ggtt(obj); + if (!vma) + return -EINVAL; + return i915_vma_unbind(vma); } void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj, -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915 : Fix to remove unnecsessary checks in postclose function.
Found by static analysis tool. Signed-off-by: Namrta Salonie--- drivers/gpu/drm/i915/i915_dma.c |2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 2336af9..ac1bca6 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1269,8 +1269,6 @@ void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) { struct drm_i915_file_private *file_priv = file->driver_priv; - if (file_priv && file_priv->bsd_ring) - file_priv->bsd_ring = NULL; kfree(file_priv); } -- 1.7.9.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Fix possible null dereference in two debugfs functions
On Thu, Nov 19, 2015 at 04:57:29PM +0530, Namrta Salonie wrote: > Found by static code analysis tool. > > Signed-off-by: Namrta Salonie> Signed-off-by: Deepak S > --- > drivers/gpu/drm/i915/i915_debugfs.c | 19 +++ > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index a3b22bd..d1719e9 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1873,6 +1873,9 @@ static int i915_gem_framebuffer_info(struct seq_file > *m, void *data) > struct drm_i915_private *dev_priv = dev->dev_private; > > ifbdev = dev_priv->fbdev; > + if (!ifbdev) Just insert a block. > @@ -3868,12 +3872,19 @@ static int pipe_crc_set_source(struct drm_device > *dev, enum pipe pipe, > enum intel_pipe_crc_source source) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_pipe_crc *pipe_crc = _priv->pipe_crc[pipe]; > - struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, > - pipe)); > + struct intel_pipe_crc *pipe_crc; > + struct intel_crtc *crtc; > u32 val = 0; /* shut up gcc */ > int ret; > > + if ((pipe < PIPE_A) || (pipe >= I915_MAX_PIPES)) > + return -EINVAL; See display_crc_ctl_parse_pipe() -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/4] drm/i915: Fix for potential NULL pointer dereference at ctx access.
On Thu, Nov 19, 2015 at 04:57:28PM +0530, Namrta Salonie wrote: > Added a null check for the context pointer, before de-referencing it. > Found by static analysis tool. False positive. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed
On Thu, Nov 19, 2015 at 09:42:17AM +, Tvrtko Ursulin wrote: > > On 19/11/15 09:17, Daniel Vetter wrote: > >On Wed, Nov 18, 2015 at 05:18:30PM +, Tvrtko Ursulin wrote: > >> > >>On 17/11/15 17:56, Daniel Vetter wrote: > >>>On Tue, Nov 17, 2015 at 05:24:01PM +, Tvrtko Ursulin wrote: > > On 17/11/15 17:08, Daniel Vetter wrote: > >On Tue, Nov 17, 2015 at 04:54:50PM +, Tvrtko Ursulin wrote: > >> > >>On 17/11/15 16:39, Daniel Vetter wrote: > >>>On Tue, Nov 17, 2015 at 04:27:12PM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin> > In the following commit: > > commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae > Author: Tvrtko Ursulin > Date: Mon Oct 5 13:26:36 2015 +0100 > > drm/i915: Clean up associated VMAs on context destruction > > I added a WARN_ON assertion that VM's active list must be empty > at the time of owning context is getting freed, but that turned > out to be a wrong assumption. > > Due ordering of operations in i915_gem_object_retire__read, where > contexts are unreferenced before VMAs are moved to the inactive > list, the described situation can in fact happen. > > It feels wrong to do things in such order so this fix makes sure > a reference to context is held until the move to inactive list > is completed. > > v2: Rather than hold a temporary context reference move the > request unreference to be the last operation. (Daniel Vetter) > > v3: Fix use after free. (Chris Wilson) > > Signed-off-by: Tvrtko Ursulin > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 > Cc: Michel Thierry > Cc: Chris Wilson > Cc: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem.c | 33 > ++--- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 98c83286ab68..094ac17a712d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2404,29 +2404,32 @@ i915_gem_object_retire__read(struct > drm_i915_gem_object *obj, int ring) > RQ_BUG_ON(!(obj->active & (1 << ring))); > > list_del_init(>ring_list[ring]); > - i915_gem_request_assign(>last_read_req[ring], NULL); > > if (obj->last_write_req && obj->last_write_req->ring->id == > ring) > i915_gem_object_retire__write(obj); > > obj->active &= ~(1 << ring); > - if (obj->active) > - return; > >>> > >>> if (obj->active) { > >>> i915_gem_request_assign(>last_read_req[ring], > >>> NULL); > >>> return; > >>> } > >>> > >>>Would result in less churn in the code and drop the unecessary indent > >>>level. Also comment is missing as to why we need to do things in a > >>>specific order. > >> > >>Actually I think I changed my mind and that v1 is the way to go. > >> > >>Just re-ordering the code here still makes it possible for the context > >>destructor to run with VMAs on the active list I think. > >> > >>If we hold the context then it is 100% clear it is not possible. > > > >request_assign _is_ the function which adjust the refcounts for us, which > >means if we drop that reference too early then grabbing a temp reference > >is just papering over the real bug. > > > >Written out your patch looks something like > > > > a_reference(a); > > a_unreference(a); > > > > /* more cleanup code that should get run before a_unreference but isn't > > */ > > > > a_unrefernce(a); /* for real this time */ > > > >Unfortunately foo_assign is a new pattern and not well-established, so > >that connection isn't clear. Maybe we should rename it to > >foo_reference_assign to make it more obvious. Or just drop the pretense > >and open-code it since we unconditionally assign NULL as the new pointer > >value, and we know the current value of the pointer is non-NULL. So > >there's really no benefit to the helper here, it only obfuscates. And > >since that obfuscation tripped you up it's time to remove it ;-) > > Then foo_reference_unreference_assign. :) > > But seriously, I think it is more complicated that.. > > The thing it trips over is that moving VMAs to inactive does not
Re: [Intel-gfx] [PATCH v3] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed
Hi, On 19/11/15 12:13, Daniel Vetter wrote: On Thu, Nov 19, 2015 at 09:42:17AM +, Tvrtko Ursulin wrote: On 19/11/15 09:17, Daniel Vetter wrote: On Wed, Nov 18, 2015 at 05:18:30PM +, Tvrtko Ursulin wrote: On 17/11/15 17:56, Daniel Vetter wrote: On Tue, Nov 17, 2015 at 05:24:01PM +, Tvrtko Ursulin wrote: On 17/11/15 17:08, Daniel Vetter wrote: On Tue, Nov 17, 2015 at 04:54:50PM +, Tvrtko Ursulin wrote: On 17/11/15 16:39, Daniel Vetter wrote: On Tue, Nov 17, 2015 at 04:27:12PM +, Tvrtko Ursulin wrote: From: Tvrtko UrsulinIn the following commit: commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae Author: Tvrtko Ursulin Date: Mon Oct 5 13:26:36 2015 +0100 drm/i915: Clean up associated VMAs on context destruction I added a WARN_ON assertion that VM's active list must be empty at the time of owning context is getting freed, but that turned out to be a wrong assumption. Due ordering of operations in i915_gem_object_retire__read, where contexts are unreferenced before VMAs are moved to the inactive list, the described situation can in fact happen. It feels wrong to do things in such order so this fix makes sure a reference to context is held until the move to inactive list is completed. v2: Rather than hold a temporary context reference move the request unreference to be the last operation. (Daniel Vetter) v3: Fix use after free. (Chris Wilson) Signed-off-by: Tvrtko Ursulin Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 Cc: Michel Thierry Cc: Chris Wilson Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem.c | 33 ++--- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 98c83286ab68..094ac17a712d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2404,29 +2404,32 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring) RQ_BUG_ON(!(obj->active & (1 << ring))); list_del_init(>ring_list[ring]); - i915_gem_request_assign(>last_read_req[ring], NULL); if (obj->last_write_req && obj->last_write_req->ring->id == ring) i915_gem_object_retire__write(obj); obj->active &= ~(1 << ring); - if (obj->active) - return; if (obj->active) { i915_gem_request_assign(>last_read_req[ring], NULL); return; } Would result in less churn in the code and drop the unecessary indent level. Also comment is missing as to why we need to do things in a specific order. Actually I think I changed my mind and that v1 is the way to go. Just re-ordering the code here still makes it possible for the context destructor to run with VMAs on the active list I think. If we hold the context then it is 100% clear it is not possible. request_assign _is_ the function which adjust the refcounts for us, which means if we drop that reference too early then grabbing a temp reference is just papering over the real bug. Written out your patch looks something like a_reference(a); a_unreference(a); /* more cleanup code that should get run before a_unreference but isn't */ a_unrefernce(a); /* for real this time */ Unfortunately foo_assign is a new pattern and not well-established, so that connection isn't clear. Maybe we should rename it to foo_reference_assign to make it more obvious. Or just drop the pretense and open-code it since we unconditionally assign NULL as the new pointer value, and we know the current value of the pointer is non-NULL. So there's really no benefit to the helper here, it only obfuscates. And since that obfuscation tripped you up it's time to remove it ;-) Then foo_reference_unreference_assign. :) But seriously, I think it is more complicated that.. The thing it trips over is that moving VMAs to inactive does not correspond in time to request retirement. But in fact VMAs are moved to inactive only when all requests associated with an object are done. This is the unintuitive thing I was working around. To make sure when context destructor runs there are not active VMAs for that VM. I don't know how to guarantee that with what you propose. Perhaps I am missing something? Ok, my example was slightly off, since we have 2 objects: b_reference(a->b); a_unreference(a); /* might unref a->b if it's the last reference */ /* more cleanup code that should get run before a_unreference but isn't */ b_unrefernce(a->b); /* for real this time */ Holding the ref to a makes sure that b doesn't disappear. We rely on that in a fundamental way (a request really needs the ctx to stick around), and the bug really is that we drop the ref to
Re: [Intel-gfx] [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.
Em Qui, 2015-11-19 às 13:16 +0200, Jani Nikula escreveu: > On Wed, 18 Nov 2015, "Zanoni, Paulo R"> wrote: > > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: > > > Cc: Paulo Zanoni > > s/Cc/Reviewed-by/ > > I don't think our patchwork is quite smart enough for that yet, so > for > future reference, please be write the whole thing. I completely forgot about this. Sorry. I guess that also means that if I say "If you do this and this and that, then I'll give you Reviewed-by: Paulo Zanoni it will mark the patch as reviewed even though it needs rework, right? I guess we just can't trust the PW automatic tag parsing since it can have either false positives or false negatives. Not until some major breakthroughs in AI. Is suggesting deprecating the use of emails as a way to handle patches still considered trolling? > > BR, > Jani. > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v4] lib/igt_core: Prefer CLOCK_MONOTONIC_RAW
On Thu, Nov 19, 2015 at 01:00:07PM +0200, Joonas Lahtinen wrote: > CLOCK_MONOTONIC_RAW is not affected by NTP, so it should be THE clock > used for timing execution of tests. > > When fetching either the starting or ending time of a test, show the > time as -1.000s. > > v4: > - Introduce time_valid macro (Chris) > - Reduce amount of boilerplate code for calculating elapsed time > > v3: > - Do not exit directly from handler (Chris) > - Show elapsed time as -1 if it is not calculable > > v2: > - Cache the used clock (Chris) > - Do not change the clock during execution > - Spit out and error if monotonic time can not be read > > Cc: Thomas Wood> Cc: Chris Wilson > Signed-off-by: Joonas Lahtinen Reviewed-by: Chris Wilson > +static int gettime(struct timespec *ts) > { > memset(ts, 0, sizeof(*ts)); > + errno = 0; > > + // Stay on the same clock for consistency. We avoid using "//" for comments, even for single lines. -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] Add Kabylake PCI IDs
On Thu, Nov 19, 2015 at 10:44:51AM +, Chris Wilson wrote: > On Wed, Nov 18, 2015 at 10:39:42AM -0800, Wayne Boyer wrote: > > Add the Kabylake PCI IDs based on the following patches. > > > > commit d97044b661d0d56b2a2ae9b2b95ab0b359b417dc > > Author: Deepak S> > Date: Wed Oct 28 12:19:51 2015 -0700 > > > > drm/i915/kbl: Add Kabylake PCI ID > > > > commit 8b10c0cf21ec84618d4bf02c73c0543500ece68d > > Author: Deepak S > > Date: Wed Oct 28 12:21:12 2015 -0700 > > > > drm/i915/kbl: Add Kabylake GT4 PCI ID > > > > Cc: Rodrigo Vivi > > Cc: Chris Wilson > > Signed-off-by: Wayne Boyer > > --- > > src/i915_pciids.h | 36 > > src/intel_module.c | 6 ++ > > 2 files changed, 42 insertions(+) > > > > diff --git a/src/i915_pciids.h b/src/i915_pciids.h > > index 17c4456..6374f58 100644 > > --- a/src/i915_pciids.h > > +++ b/src/i915_pciids.h > > @@ -291,4 +291,40 @@ > > INTEL_VGA_DEVICE(0x1A84, info), \ > > INTEL_VGA_DEVICE(0x5A84, info) > > > > +#define INTEL_KBL_GT1_IDS(info) \ > > + INTEL_VGA_DEVICE(0x5913, info), /* ULT GT1.5 */ \ > > + INTEL_VGA_DEVICE(0x5915, info), /* ULX GT1.5 */ \ > > + INTEL_VGA_DEVICE(0x5917, info), /* DT GT1.5 */ \ > > + INTEL_VGA_DEVICE(0x5906, info), /* ULT GT1 */ \ > > + INTEL_VGA_DEVICE(0x590E, info), /* ULX GT1 */ \ > > + INTEL_VGA_DEVICE(0x5902, info), /* DT GT1 */ \ > > + INTEL_VGA_DEVICE(0x590B, info), /* HALO GT1 */ \ > > + INTEL_VGA_DEVICE(0x590A, info) /* SRV GT1 */ > > + > > +#define INTEL_KBL_GT2_IDS(info) \ > > + INTEL_VGA_DEVICE(0x5916, info), /* ULT GT2 */ \ > > + INTEL_VGA_DEVICE(0x5921, info), /* ULT GT2F */ \ > > + INTEL_VGA_DEVICE(0x591E, info), /* ULX GT2 */ \ > > + INTEL_VGA_DEVICE(0x5912, info), /* DT GT2 */ \ > > + INTEL_VGA_DEVICE(0x591B, info), /* HALO GT2 */ \ > > + INTEL_VGA_DEVICE(0x591A, info), /* SRV GT2 */ \ > > + INTEL_VGA_DEVICE(0x591D, info) /* WKS GT2 */ > > + > > +#define INTEL_KBL_GT3_IDS(info) \ > > + INTEL_VGA_DEVICE(0x5926, info), /* ULT GT3 */ \ > > + INTEL_VGA_DEVICE(0x592B, info), /* HALO GT3 */ \ > > + INTEL_VGA_DEVICE(0x592A, info) /* SRV GT3 */ > > + > > +#define INTEL_KBL_GT4_IDS(info) \ > > + INTEL_VGA_DEVICE(0x5932, info), /* DT GT4 */ \ > > + INTEL_VGA_DEVICE(0x593B, info), /* HALO GT4 */ \ > > + INTEL_VGA_DEVICE(0x593A, info), /* SRV GT4 */ \ > > + INTEL_VGA_DEVICE(0x593D, info) /* WKS GT4 */ > > + > > +#define INTEL_KBL_IDS(info) \ > > + INTEL_KBL_GT1_IDS(info), \ > > + INTEL_KBL_GT2_IDS(info), \ > > + INTEL_KBL_GT3_IDS(info), \ > > + INTEL_KBL_GT4_IDS(info) > > This is not an exact copy of the kernel i915_pciids.h I copied across i915_pciids.h and pushed. -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] [V2 PATCH 2/2] drm/i915: start adding dp mst audio
(Cc'ing Paulo for the audio power domain question) On Wed, 2015-11-11 at 13:33 +0800, libin.y...@linux.intel.com wrote: > From: Libin Yang> > This patch adds support for DP MST audio in i915. > > Enable audio codec when DP MST is enabled if has_audio flag is set. > Disable audio codec when DP MST is disabled if has_audio flag is set. > > Signed-off-by: Libin Yang > Signed-off-by: Dave Airlie > --- > drivers/gpu/drm/i915/i915_debugfs.c | 14 ++ > drivers/gpu/drm/i915/intel_audio.c | 9 ++--- > drivers/gpu/drm/i915/intel_dp_mst.c | 25 + > 3 files changed, 45 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 5659d4c..38c7a5d 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2873,6 +2873,18 @@ static void intel_dp_info(struct seq_file *m, > intel_panel_info(m, _connector->panel); > } > > +static void intel_dp_mst_info(struct seq_file *m, > + struct intel_connector *intel_connector) > +{ > + struct intel_encoder *intel_encoder = intel_connector->encoder; > + struct intel_dp_mst_encoder *intel_mst = > + enc_to_mst(_encoder->base); > + struct intel_digital_port *intel_dig_port = intel_mst->primary; > + struct intel_dp *intel_dp = _dig_port->dp; > + > + seq_printf(m, "\taudio support: %s\n", > drm_dp_mst_port_has_audio(_dp->mst_mgr, intel_connector->port) ? "yes" : > "no"); Too much going on in just one line. You can replace the ternary operator with yesno(). You could also add a has_audio bool: bool has_audio = drm_dp_mst_port_has_audio(...); seq_printf(..., yesno(has_audio)); > +} > + > static void intel_hdmi_info(struct seq_file *m, > struct intel_connector *intel_connector) > { > @@ -2916,6 +2928,8 @@ static void intel_connector_info(struct seq_file *m, > intel_hdmi_info(m, intel_connector); > else if (intel_encoder->type == INTEL_OUTPUT_LVDS) > intel_lvds_info(m, intel_connector); > + else if (intel_encoder->type == INTEL_OUTPUT_DP_MST) > + intel_dp_mst_info(m, intel_connector); > } > > seq_printf(m, "\tmodes:\n"); > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c > index 63d4706..07b2aa6 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct intel_encoder > *encoder) > tmp |= AUD_CONFIG_N_PROG_ENABLE; > tmp &= ~AUD_CONFIG_UPPER_N_MASK; > tmp &= ~AUD_CONFIG_LOWER_N_MASK; > - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) > + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || > + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) The second line should be aligned with '('. > tmp |= AUD_CONFIG_N_VALUE_INDEX; > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > @@ -478,7 +479,8 @@ static void ilk_audio_codec_enable(struct drm_connector > *connector, > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; > - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) > + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || > + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) Same here. > tmp |= AUD_CONFIG_N_VALUE_INDEX; > else > tmp |= audio_config_hdmi_pixel_clock(adjusted_mode); > @@ -516,7 +518,8 @@ void intel_audio_codec_enable(struct intel_encoder > *intel_encoder) > > /* ELD Conn_Type */ > connector->eld[5] &= ~(3 << 2); > - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) > + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || > + intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST)) > connector->eld[5] |= (1 << 2); And here. > > connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2; > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > b/drivers/gpu/drm/i915/intel_dp_mst.c > index 0639275..4ded0fb 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -78,6 +78,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder > *encoder, > return false; > } > > + if (drm_dp_mst_port_has_audio(_dp->mst_mgr, found->port)) > + pipe_config->has_audio = true; > mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp); > > pipe_config->pbn = mst_pbn; > @@ -102,6 +104,11 @@ static void intel_mst_disable_dp(struct intel_encoder > *encoder) > struct intel_dp_mst_encoder *intel_mst =
Re: [Intel-gfx] [PATCH 1/2] drm/i915: PSR: Let's rely more on frontbuffer tracking.
On Wed, 18 Nov 2015, "Zanoni, Paulo R"wrote: > Em Qua, 2015-11-18 às 11:21 -0800, Rodrigo Vivi escreveu: >> Cc: Paulo Zanoni > s/Cc/Reviewed-by/ I don't think our patchwork is quite smart enough for that yet, so for future reference, please be write the whole thing. BR, Jani. -- Jani Nikula, 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/1] drm/i915/audio: apply SKL codec wake up patch to BXT
On Thu, 19 Nov 2015 10:09:01 +0100, Jani Nikula wrote: > > On Thu, 19 Nov 2015, Jani Nikulawrote: > > On Thu, 19 Nov 2015, han...@intel.com wrote: > >> From: "Lu, Han" > >> > >> Signed-off-by: Lu, Han > >> > >> diff --git a/drivers/gpu/drm/i915/intel_audio.c > >> b/drivers/gpu/drm/i915/intel_audio.c > >> index 63d4706..8310bf3 100644 > >> --- a/drivers/gpu/drm/i915/intel_audio.c > >> +++ b/drivers/gpu/drm/i915/intel_audio.c > >> @@ -591,7 +591,8 @@ static void > >> i915_audio_component_codec_wake_override(struct device *dev, > >>struct drm_i915_private *dev_priv = dev_to_i915(dev); > >>u32 tmp; > >> > >> - if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv)) > >> + if (!IS_SKYLAKE(dev_priv) && !IS_BROXTON(dev_priv) && > >> + !IS_KABYLAKE(dev_priv)) > > > > How about if (INTEL_INFO(dev_priv)->gen < 9)? > > Oh, and I guess we also wonder why this is only relevant for the latest > platforms? My understanding is that it's a workaround specific to HSW/BDW. It's issued only during the audio controller initialization and at the resume. But I never know why it's needed, it's a black magic :) Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v2] lib/igt_core: Add kmsg capture and dumping
Capture the output from /dev/kmsg during test execution independantly of other concurrent watchers like Piglit test runner. The captured output is analyzed and the test will fail if log messages with higher priority than LOG_NOTICE are emitted from any domain. Also adding igt_capture to lib/tests which will fail and produce kmsg output which should be captured by the new code and also demonstrate failing just due to /dev/kmsg. v2: - Reopen/close the kmsg FD per each test (Chris) - Better commit mesage (Daniel) - Display failure due to kmsg only as FAIL (KMSG) Cc: Thomas WoodCc: Chris Wilson Cc: Damien Lespiau Acked-by: Daniel Vetter Signed-off-by: Joonas Lahtinen --- lib/igt_core.c | 113 +++-- lib/igt_core.h | 7 +++ lib/tests/.gitignore | 1 + lib/tests/Makefile.sources | 1 + lib/tests/igt_capture.c| 88 +++ 5 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 lib/tests/igt_capture.c diff --git a/lib/igt_core.c b/lib/igt_core.c index 04a0ab2..cb782dc 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #ifdef __linux__ @@ -211,6 +212,8 @@ * "--help" command line option. */ +#define IGT_KMSG_CAPTURE_DUMP_BUF_SIZE 4096 + static unsigned int exit_handler_count; const char *igt_interactive_debug; @@ -247,6 +250,9 @@ enum { static int igt_exitcode = IGT_EXIT_SUCCESS; static const char *command_str; +static int igt_kmsg_capture_fd = -1; +static char* igt_kmsg_capture_dump_buf = NULL; + static char* igt_log_domain_filter; static struct { char *entries[256]; @@ -312,6 +318,93 @@ static void _igt_log_buffer_dump(void) pthread_mutex_unlock(_buffer_mutex); } +static void _igt_kmsg_capture_reset(void) +{ + if (igt_kmsg_capture_fd != -1) + close(igt_kmsg_capture_fd); + + igt_kmsg_capture_fd = open("/dev/kmsg", + O_RDONLY | O_NONBLOCK); + + if (igt_kmsg_capture_fd == -1) + return; + + lseek(igt_kmsg_capture_fd, 0, SEEK_END); + + if (igt_kmsg_capture_dump_buf == NULL) + igt_kmsg_capture_dump_buf = + malloc(IGT_KMSG_CAPTURE_DUMP_BUF_SIZE); + + if (igt_kmsg_capture_dump_buf == NULL) + igt_warn("Unable to allocate memory, " +"will not dump kmsg.\n"); +} + +static int _igt_kmsg_capture_dump(bool notify_empty, int show_priority) +{ + size_t nbytes; + int nlines; + int prefix; + int facility; + int priority; + char *p; + int c; + + if (igt_kmsg_capture_fd == -1 || + igt_kmsg_capture_dump_buf == NULL) + return 0; + + nlines = 0; + do { + errno = 0; + nbytes = read(igt_kmsg_capture_fd, + igt_kmsg_capture_dump_buf, + IGT_KMSG_CAPTURE_DUMP_BUF_SIZE); + + if (nbytes == -1) + continue; + + sscanf(igt_kmsg_capture_dump_buf, "%d;", ); + priority = prefix & 0x7; + + if (priority > show_priority) + continue; + + if (!nlines) + fprintf(stderr, " KMSG \n"); + + p = strchr(igt_kmsg_capture_dump_buf, ';') + 1; + while (p - igt_kmsg_capture_dump_buf < nbytes) { + if (*p != '\\') { + fputc(*p++, stderr); + continue; + } + sscanf(p, "\\x%x", ); + fputc(c, stderr); + p += 4; + } + nlines++; + } while(errno == 0); + + if (nlines) { + fprintf(stderr, " END \n"); + } else { + if (notify_empty) + fprintf(stderr, "No kmsg.\n"); + } + + if (errno != EAGAIN) + fprintf(stderr, "Error: Incomplete kmsg!\n"); + + close(igt_kmsg_capture_fd); + igt_kmsg_capture_fd = -1; + + free(igt_kmsg_capture_dump_buf); + igt_kmsg_capture_dump_buf = NULL; + + return nlines; +} + __attribute__((format(printf, 1, 2))) static void kmsg(const char *format, ...) #define KERN_EMER "<0>" @@ -676,8 +769,10 @@ out: /* install exit handler, to ensure we clean up */ igt_install_exit_handler(common_exit_handler); - if (!test_with_subtests) + if (!test_with_subtests) { gettime(_time); + _igt_kmsg_capture_reset(); + } for (i = 0; (optind + i) < *argc; i++) argv[i +
Re: [Intel-gfx] Write GFX_FLSH_CNT after updating GGTT entries
On Thu, Nov 19, 2015 at 06:20:23PM +0800, Zhi Wang wrote: > Hi Gurus: > I'm curious about the register GFX_FLSH_CNT(0x101008) in > i915_gem_gtt.c. Does these register exist in recently generations? After > digging into b-spec, it looks only BXT and CHV has this register. Does > the desktop platform also have this register which needs to be written > after updating GGTT MMIOs? > > BTW: Looks windows driver haven't used this MMIO... So whose behavior is > the right behavior? As I understand it that register flushes the CPU GTT TLBs, and we need to do it because of the WC mapping we have for the GTT PTEs. If we used UC mapping we wouldn't need it since there's supposedly an automagic TLB flush that happens on PTE writes. BSpec is bad at finding some registers via bxml. Using dtsearch and looking for both 0x and h is the method I use to track such things down. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] i-g-t/libdrm email tagging & patchwork
On Wed, 18 Nov 2015, Daniel Vetterwrote: > On Sun, Nov 08, 2015 at 12:31:36AM +, Damien Lespiau wrote: >> Hi all, >> >> I've added a feature to sort the patches sent to intel-gfx into 3 >> buckets: i915, intel-gpu-tools and libdrm. This sorting relies on >> tagging patches, using the subject prefixes (which is what most people >> do already anyway). >> >> - i915 (intel-gfx): catchall project, all mails not matching any of >> the other 2 projects will end up there >> >> - intel-gpu-tools: mails need to be tagged with i-g-t, igt or >> intel-gpu-tools >> >> - libdrm-intel: mails need to be tagged with libdrm >> >> This tagging can be set up per git repository with: >> >> $ git config format.subjectprefix "PATCH i-g-t" > > Is there any way we could push this out to users somehow? I have bazillion > of machines, I'll get this wrong eventually ... So will everyone else I > guess. Googling around, I don't think we can automatically force this on people. We could add a script to make it easier for people to set this up. Either a setup that needs to be re-run every time there are changes, or a setup that symlinks a git hook back into a file stored in the repository so changes are deployed automatically. The latter has security implications, so I'd go for the former. BR, Jani. > -Daniel > >> >> And use git send-email as usual. A note of caution though, using the >> command line argument --subject-prefix will override the one configured, >> so the tag will have to be repeated. To limit the number of things one >> needs to think about I'd suggest to use --reroll-count to tag patches >> with the v2/v3/... tags. I'm more and more thinking that wrapping the >> sending logic for developers into the git-pw command would be a good >> thing (especially for --in-reply-to) but that'd be for another time. >> >> There are two new patchwork projects then: >> >> http://patchwork.freedesktop.org/project/intel-gpu-tools/ >> http://patchwork.freedesktop.org/project/libdrm-intel/ >> >> I've also run the sorting on all the existing patches so the entries >> that were historically in the intel-gfx project are now in those new >> projects. >> >> There is still some work left to limit the noise of those lists of >> patches, eg. some patches are still marked as New but, in reality, they >> have been merged. Solving that is quite important and high-ish the TODO >> list. >> >> HTH, >> >> -- >> Damien >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] drm/i915: Nuke fbc members from intel_crtc->atomic.
This leaves intel_crtc->atomic empty, so zap it as well. Signed-off-by: Maarten Lankhorst--- drivers/gpu/drm/i915/intel_display.c | 87 ++-- drivers/gpu/drm/i915/intel_drv.h | 16 --- 2 files changed, 33 insertions(+), 70 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 95501aba7d23..7f69f98d8b23 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4734,7 +4734,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) { struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); struct drm_atomic_state *old_state = old_crtc_state->base.state; - struct intel_crtc_atomic_commit *atomic = >atomic; struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_device *dev = crtc->base.dev; @@ -4750,15 +4749,15 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) if (pipe_config->wm_changed && pipe_config->base.active) intel_update_watermarks(>base); - if (atomic->update_fbc) - intel_fbc_update(dev_priv); - if (old_pri_state) { struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); struct intel_plane_state *old_primary_state = to_intel_plane_state(old_pri_state); + if (primary_state->visible) + intel_fbc_update(dev_priv); + if (primary_state->visible && (needs_modeset(_config->base) || !old_primary_state->visible)) @@ -4767,8 +4766,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) if (needs_modeset(_config->base)) intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); - - memset(atomic, 0, sizeof(*atomic)); } static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) @@ -4776,7 +4773,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc); struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc_atomic_commit *atomic = >atomic; struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->base.state); struct drm_atomic_state *old_state = old_crtc_state->base.state; @@ -4785,9 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) drm_atomic_get_existing_plane_state(old_state, primary); bool modeset = needs_modeset(_config->base); - if (atomic->disable_fbc) - intel_fbc_disable_crtc(crtc); - if (old_pri_state) { struct intel_plane_state *primary_state = to_intel_plane_state(primary->state); @@ -4795,8 +4788,27 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) to_intel_plane_state(old_pri_state); if (old_primary_state->visible && - (modeset || !primary_state->visible)) + (modeset || !primary_state->visible)) { + intel_fbc_disable_crtc(crtc); + intel_pre_disable_primary(>base); + } else if (primary_state->visible && +INTEL_INFO(dev_priv)->gen <= 4 && !IS_G4X(dev_priv) && +dev_priv->fbc.crtc == crtc && +primary_state->base.rotation != BIT(DRM_ROTATE_0)) { + /* +* FBC does not work on some platforms for rotated +* planes, so disable it when rotation is not 0 and +* update it when rotation is set back to 0. +* +* FIXME: This is redundant with the fbc update done in +* the primary plane enable function except that that +* one is done too late. We eventually need to unify +* this. +*/ + + intel_fbc_disable_crtc(crtc); + } } if (pipe_config->disable_cxsr) { @@ -11686,7 +11698,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_plane *plane = plane_state->plane; struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane_state *old_plane_state = to_intel_plane_state(plane->state); int idx =