Re: [Intel-gfx] i915 and eDP issue !?
On Sun, Mar 17, 2013 at 11:21 AM, Michael Zimmermann wrote: > I own a Dell XPS 12 with an IvyBridge Graphic card, a Mini DisplayPort and > an internal display which is accessible via eDP1. So far the only chance to > get this machine running properly with Ubuntu 12.10 64-bit is to set > acpi=off as boot parameter with all the disadvantages of not having the ACPI > features. Without the setting the screen is blank (=black), but I hear the > drumsticks and can access the machine via ssh. Checking dmesg and Xorg.0.log > gives no serious hint of having a problem. Astonishingly if I plug in an > external monitor I can see the normal ubuntu user interface on it and work > with it, where the monitor of the laptop remains blank. As far as I can see > ACPI and the graphic card work well except for not detecting eDP1 as the > device of choice then ACPI is turned on. Turning off ACPI means that the > kernel starts SFI (Simple firmware interface) which does not have the > problem and that works. I would like to get the ACPI features, especially > power management and cpu usage, which is quite essential on a laptop, > running so I would like to ask you for advice, if there is any possibility > for solving this you may be the right person having the internals of > tweaking the i915. Please test with the latest released kernels (3.8.x) and preferrably also with latest drm-intel-nightly (ubuntu has a nice ppa at http://kernel.ubuntu.com/~kernel-ppa/mainline/drm-intel-experimental/). If that's still broken please file a bug report at bugs.freedesktop.org against dri -> drm (Intel). Also when poking please never only send a mail to your maintainer, but always cc a mailing list (since he might be on vacation or just really busy). Thanks, 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] 3.8.2->3.8.3 i915 regression: GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5
On Sat, Mar 16, 2013 at 11:35 AM, Arkadiusz Miskiewicz wrote: > On Thursday 14 of March 2013, Arkadiusz Miskiewicz wrote: >> Hello. >> >> After upgrading from 3.8.2 to 3.8.3 I'm getting regression : > > More people hits this: > https://bugzilla.redhat.com/show_bug.cgi?id=922304 > https://bugs.archlinux.org/task/34327 > (seems always GM45 gpu in these reports) > > archlinux people also noticed that xrandr reports VGA1 as connected (while in > reality nothing is connected to VGA1) Can you please test whether 3.9-rc kernels are affected, too? We need to know this since stable rules mandate that a regression is fixed on upstream first. Once that's figured out we can backport a fix (if 3.9-rc works) or start working on a fix for 3.8-rc kernels first and backport afterwards. Thanks, 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: Sanity check incoming ioctl data for a NULL pointer
2013/3/14 Chris Wilson :
> In order to prevent a potential NULL deference with hostile userspace,
> we need to check whether the ioctl was passed an invalid args pointer.
>
> Reported-by: Tommi Rantala
> Link:
> http://lkml.kernel.org/r/ca+ydwtpubvbwxbt-tdgpuvj1eu7itmcho_2b3w13hkd5+jw...@mail.gmail.com
> Signed-off-by: Chris Wilson
Thanks, looks good. I still see a flood of oopses from the other
ioctls, so it's a bit difficult to test this patch properly.
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 365e41a..9f5602e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret, i;
>
> - if (args->buffer_count < 1) {
> + if (args == NULL)
> + return -EINVAL;
> +
> + if (args->buffer_count < 1 ||
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
> @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void
> *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret;
>
> + if (args == NULL)
> + return -EINVAL;
> +
> if (args->buffer_count < 1 ||
> - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
> --
> 1.7.10.4
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [pull] drm-intel-next
On Thu, Sep 13, 2012 at 7:18 AM, Daniel Vetter wrote: > Hi Dave, > > The big ticket item here is the new i915 modeset infrastructure. > Shockingly it didn't not blow up all over the place (i.e. I've managed to > fix the ugly issues before merging). 1-2 smaller corner cases broke, but > we have patches. Also, there's tons of patches on top of this that clean > out cruft and fix a few bugs that couldn't be fixed with the crtc helper > based stuff. So more stuff to come ;-) > > Also a few other things: > - Tiny fix in the fb helper to go through the official dpms interface > instead of calling the crtc helper code. > - forcewake code frobbery from Ben, code should be more in-line with > what Windows does now. > - fixes for the render ring flush on hsw (Paulo) > - gpu frequency tracepoint > - vlv forcewake changes to better align it with our understanding of the > forcewake magic. > - a few smaller cleanups > > Cheers, Daniel > > > The following changes since commit d7c3b937bdf45f0b844400b7bf6fd3ed50bac604: > > drm/i915: Remove __GFP_NO_KSWAPD (2012-08-27 17:11:38 +0200) > > are available in the git repository at: > > git://people.freedesktop.org/~danvet/drm-intel > tags/drm-intel-next-2012-09-09 > > for you to fetch changes up to e04190e0ecb236c51af181c18c545ea076fb9cca: > > drm/fb helper: don't call drm_helper_connector_dpms directly (2012-09-08 > 00:51:15 +0200) > > > > Ben Widawsky (5): > drm/i915: Extract forcewake ack timeout > drm/i915: use cpu_relax() in wait_for_atomic > drm/i915: Change forcewake timeout to 2ms > drm/i915: Never read FORCEWAKE > drm/i915: Enable some sysfs stuff without CONFIG_PM > > Chris Wilson (1): > drm/i915: Convert remaining debugfs iterators over rings to > for_each_ring() > > Daniel Vetter (66): > drm/ips: move drps/ips/ilk related variables into dev_priv->ips > drm/i915: add a tracepoint for gpu frequency changes > drm/i915: align vlv forcewake with common lore > drm/i915: differ error message between forcwake timeouts > drm/i915: add crtc->enable/disable vfuncs insted of dpms > drm/i915: rip out crtc prepare/commit indirection > drm/i915: add direct encoder disable/enable infrastructure > drm/i915/hdmi: convert to encoder->disable/enable > drm/i915/tv: convert to encoder enable/disable > drm/i915/lvds: convert to encoder disable/enable > drm/i915/dp: convert to encoder disable/enable > drm/i915/crt: convert to encoder disable/enable > drm/i915/sdvo: convert to encoder disable/enable > drm/i915/dvo: convert to encoder disable/enable > drm/i915: convert dpms functions of dvo/sdvo/crt > drm/i915: rip out encoder->disable/enable checks > drm/i915: clean up encoder_prepare/commit > drm/i915: copy&paste drm_crtc_helper_set_config > drm/i915: call set_base directly > drm/i915: inline intel_best_encoder > drm/i915: copy&paste drm_crtc_helper_set_mode > drm/i915: simplify intel_crtc_prepare_encoders > drm/i915: rip out encoder->prepare/commit > drm/i915: call crtc functions directly > drm/i915: WARN when trying to enabled an unused crtc > drm/i915: Add interfaces to read out encoder/connector hw state > drm/i915/dp: implement get_hw_state > drm/i915/hdmi: implement get_hw_state > drm/i915/tv: implement get_hw_state > drm/i915/lvds: implement get_hw_state > drm/i915/crt: implement get_hw_state > drm/i915/sdvo: implement get_hw_state > drm/i915/dvo: implement get_hw_state > drm/i915: read out the modeset hw state at load and resume time Hi Daniel, This commit regresses modeset on the samsung series 5 chromebook (it is basically a pineview machine with an lvds panel). I don't seem to be able to set any mode on it any longer. Any idea? Stéphane > drm/i915: check connector hw/sw state > drm/i915: rip out intel_crtc->dpms_mode > drm/i915: rip out intel_dp->dpms_mode > drm/i915: ensure the force pipe A quirk is actually followed > drm/i915: introduce struct intel_set_config > drm/i915: extract modeset config save/restore code > drm/i915: extract intel_set_config_compute_mode_changes > drm/i915: extract intel_set_config_update_output_state > drm/i915: implement crtc helper semantics relied upon by the fb helper > drm/i915: don't update the fb base if there is no fb > drm/i915: convert pointless error checks in set_config to BUGs > drm/i915: don't save all the encoder/crtc state in set_config > drm/i915: stage modeset output changes > drm/i915: push crtc->fb update into pipe_set_base > drm/i915: remove crtc disabling special case > drm/i915: move output commit and crtc disabling into set_mode > drm/i915: extract adjusted mode computation > drm/i915: use staged outup
[Intel-gfx] 3.8.2->3.8.3 i915 regression: GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5
Hello. After upgrading from 3.8.2 to 3.8.3 I'm getting regression : diff: [drm] Supports vblank timestamp caching Rev 1 (10.10.2010). [drm] Driver supports precise vblank timestamp query. vgaarb: device changed decodes: PCI::00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem + [drm] GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5 fbcon: inteldrmfb (fb0) is primary device - Console: switching to colour frame buffer device 180x56 + Console: switching to colour frame buffer device 128x48 i915 :00:02.0: fb0: inteldrmfb frame buffer device i915 :00:02.0: registered panic notifier acpi device:01: registered as cooling_device0 console becomes weird, ends up in 2/3 of screen, X when starts gets tiny fonts etc. Machine is Thinkpad T400 with gm45 GPU 00:02.0 VGA compatible controller [0300]: Intel Corporation Mobile 4 Series Chipset Integrated Graphics Controller [8086:2a42] (rev 07) (prog-if 00 [VGA controller]) Subsystem: Lenovo Device [17aa:2112] Reverting bisected commit from 3.8.3 fixes the problem 2a9810441fcc26cf3f006f015f8a62094fe57a90 is the first bad commit commit 2a9810441fcc26cf3f006f015f8a62094fe57a90 Author: Daniel Vetter Date: Sat Dec 1 21:03:22 2012 +0100 drm/i915: reorder setup sequence to have irqs for output setup commit 52d7ecedac3f96fb562cb482c139015372728638 upstream. Otherwise the new&shiny irq-driven gmbus and dp aux code won't work that well. Noticed since the dp aux code doesn't have an automatic fallback with a timeout (since the hw provides for that already). v2: Simple move drm_irq_install before intel_modeset_gem_init, as suggested by Ben Widawsky. v3: Now that interrupts are enabled before all connectors are fully set up, we might fall over serving a HPD interrupt while things are still being set up. Instead of jumping through massive hoops and complicating the code with a separate hpd irq enable step, simply block out the hotplug work item from doing anything until things are in place. v4: Actually, we can enable hotplug processing only after the fbdev is fully set up, since we call down into the fbdev from the hotplug work functions. So stick the hpd enabling right next to the poll helper initialization. v5: We need to enable irqs before intel_modeset_init, since that function sets up the outputs. v6: Fixup cleanup sequence, too. Reviewed-by: Imre Deak Signed-off-by: Daniel Vetter Signed-off-by: Greg Kroah-Hartman :04 04 26e83b14e8d49da8c451dc2c837646a337a79085 fa2cbfcf159808ce188675115888242245e4e69d M drivers relevant dmesg: [drm] Initialized drm 1.1.0 20060810 [drm] radeon defaulting to userspace modesetting. i915 :00:02.0: setting latency timer to 64 i915 :00:02.0: irq 47 for MSI/MSI-X [drm] Supports vblank timestamp caching Rev 1 (10.10.2010). [drm] Driver supports precise vblank timestamp query. [drm] GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5 fbcon: inteldrmfb (fb0) is primary device i915 :00:02.0: fb0: inteldrmfb frame buffer device i915 :00:02.0: registered panic notifier [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0 -- Arkadiusz Miśkiewicz, arekm / maven.pl ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 3.8.2->3.8.3 i915 regression: GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5
On Thursday 14 of March 2013, Arkadiusz Miskiewicz wrote: > Hello. > > After upgrading from 3.8.2 to 3.8.3 I'm getting regression : More people hits this: https://bugzilla.redhat.com/show_bug.cgi?id=922304 https://bugs.archlinux.org/task/34327 (seems always GM45 gpu in these reports) archlinux people also noticed that xrandr reports VGA1 as connected (while in reality nothing is connected to VGA1) [arekm@t400 ~]$ more xrandr Screen 0: minimum 320 x 200, current 1440 x 900, maximum 32767 x 32767 LVDS1 connected 1440x900+0+0 (normal left inverted right x axis y axis) 303mm x 190mm 1440x900 60.0*+ 50.0 1024x768 60.0 800x60060.3 56.2 640x48059.9 VGA1 connected 1024x768+0+0 (normal left inverted right x axis y axis) 0mm x 0mm 1024x768 60.0* 800x60060.3 56.2 848x48060.0 640x48059.9 DP1 disconnected (normal left inverted right x axis y axis) [arekm@t400 ~]$ xrandr --output VGA --mode 1440x900 warning: output VGA not found; ignoring Normally this looks like: [arekm@t400 ~]$ more xrandr Screen 0: minimum 320 x 200, current 1440 x 900, maximum 32767 x 32767 LVDS1 connected 1440x900+0+0 (normal left inverted right x axis y axis) 303mm x 190mm 1440x900 60.0*+ 50.0 1024x768 60.0 800x60060.3 56.2 640x48059.9 VGA1 disconnected (normal left inverted right x axis y axis) DP1 disconnected (normal left inverted right x axis y axis) > > diff: > [drm] Supports vblank timestamp caching Rev 1 (10.10.2010). > [drm] Driver supports precise vblank timestamp query. > vgaarb: device changed decodes: > PCI::00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem + [drm] > GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5 > fbcon: inteldrmfb (fb0) is primary device > - Console: switching to colour frame buffer device 180x56 > + Console: switching to colour frame buffer device 128x48 > i915 :00:02.0: fb0: inteldrmfb frame buffer device > i915 :00:02.0: registered panic notifier > acpi device:01: registered as cooling_device0 > > console becomes weird, ends up in 2/3 of screen, X when starts gets tiny > fonts etc. > > Machine is Thinkpad T400 with gm45 GPU > 00:02.0 VGA compatible controller [0300]: Intel Corporation Mobile 4 Series > Chipset Integrated Graphics Controller [8086:2a42] (rev 07) (prog-if 00 > [VGA controller]) Subsystem: Lenovo Device [17aa:2112] > > Reverting bisected commit from 3.8.3 fixes the problem > > 2a9810441fcc26cf3f006f015f8a62094fe57a90 is the first bad commit > commit 2a9810441fcc26cf3f006f015f8a62094fe57a90 > Author: Daniel Vetter > Date: Sat Dec 1 21:03:22 2012 +0100 > > drm/i915: reorder setup sequence to have irqs for output setup > > commit 52d7ecedac3f96fb562cb482c139015372728638 upstream. > > Otherwise the new&shiny irq-driven gmbus and dp aux code won't work > that well. Noticed since the dp aux code doesn't have an automatic > fallback with a timeout (since the hw provides for that already). > > v2: Simple move drm_irq_install before intel_modeset_gem_init, as > suggested by Ben Widawsky. > > v3: Now that interrupts are enabled before all connectors are fully > set up, we might fall over serving a HPD interrupt while things are > still being set up. Instead of jumping through massive hoops and > complicating the code with a separate hpd irq enable step, simply > block out the hotplug work item from doing anything until things are > in place. > > v4: Actually, we can enable hotplug processing only after the fbdev is > fully set up, since we call down into the fbdev from the hotplug work > functions. So stick the hpd enabling right next to the poll helper > initialization. > > v5: We need to enable irqs before intel_modeset_init, since that > function sets up the outputs. > > v6: Fixup cleanup sequence, too. > > Reviewed-by: Imre Deak > Signed-off-by: Daniel Vetter > Signed-off-by: Greg Kroah-Hartman > > :04 04 26e83b14e8d49da8c451dc2c837646a337a79085 > :fa2cbfcf159808ce188675115888242245e4e69d M drivers > > relevant dmesg: > > [drm] Initialized drm 1.1.0 20060810 > [drm] radeon defaulting to userspace modesetting. > i915 :00:02.0: setting latency timer to 64 > i915 :00:02.0: irq 47 for MSI/MSI-X > [drm] Supports vblank timestamp caching Rev 1 (10.10.2010). > [drm] Driver supports precise vblank timestamp query. > [drm] GMBUS [i915 gmbus dpb] timed
Re: [Intel-gfx] [pull] drm-intel-next
On Fri, Mar 15, 2013 at 3:11 AM, Stéphane Marchesin wrote: >> drm/i915: read out the modeset hw state at load and resume time > This commit regresses modeset on the samsung series 5 chromebook (it > is basically a pineview machine with an lvds panel). I don't seem to > be able to set any mode on it any longer. Does that mean the kernel refuses to set the mode, or that you get a black screen? For starters I guess we need: - drm.debug=0xe dmesg from just before that commit - same for latest 3.9-rc kernels, presuming it's not broken there Latest upstream has a minor chance to work better I think since we've improved the pfit handling in the setup and teardown sequence a bit. Generally lvds has been hit&miss on way too many machines unfortunately with things randomly breaking and getting fixed again (e.g. one of Chris' machines works again with the new code ...). And the commit above doesn't really change much in the code itself but it does change the order (and timing) of the different enable/disable codepaths. Thanks, 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: Sanity check incoming ioctl data for a NULL pointer
On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson wrote: > On Fri, Mar 15, 2013 at 04:49:42PM -0700, Ben Widawsky wrote: >> On Fri, Mar 15, 2013 at 10:06:19PM +, Chris Wilson wrote: >> > On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote: >> > > On Fri, Mar 15, 2013 at 08:24:03AM +, Chris Wilson wrote: >> > > > That's what I thought too. Looking at the stack trace, the empirical >> > > > evidence is that we need the check. >> > > > -Chris >> > > >> > > I think we need to investigate the issue more then, or put a BUG_ON() in >> > > the drm code and run it through trinity. We have other places where arg >> > > can't/shouldn't be NULL and we don't check. >> > >> > Actually we are both wrong. drm_ioctl() does not validate that the >> > user struct matches the expected size, just ensures that if that user >> > cmd specifies that the arg is to be used that it only up to the known >> > size is copied. >> > >> > A hostile userspace can bludgen a NULL pointer through drm_ioctl() into >> > the driver->ioctl->func(). >> >> > > > + if (args == NULL) >> > > > + return -EINVAL; >> > > > + >> >> I must be failing to see the obvious, but I'm still not getting how args >> can ever be NULL. kdata which is passed to us as "data" and cast to >> "args' is either always some stack variable, or some kmalloc'd memory. I >> see how the arguments themselves can be crap which is really only when >> user size != drv_size. >> >> So tell me, which case can result in a NULL arg? >> 1. user size == drv_size // better not be this one >> 2. user size < driver size >> 3. user size > driver size >> >> It seems to me we still must [simply] be missing something in our >> parameter validation. > > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl > command (which are seperate from the ioctl number), then kdata is set to > NULL. Doesn't that mean that we need these checks everywhere? Or at least a fixup in drm core proper? And I think we need to add trinity to our test setup eventually ;-) -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] 3.8.2->3.8.3 i915 regression: GMBUS [i915 gmbus dpb] timed out, falling back to bit banging on pin 5
On Sunday 17 of March 2013, Daniel Vetter wrote: > On Sat, Mar 16, 2013 at 11:35 AM, Arkadiusz Miskiewicz > > wrote: > > On Thursday 14 of March 2013, Arkadiusz Miskiewicz wrote: > >> Hello. > > > >> After upgrading from 3.8.2 to 3.8.3 I'm getting regression : > > More people hits this: > > https://bugzilla.redhat.com/show_bug.cgi?id=922304 > > https://bugs.archlinux.org/task/34327 > > (seems always GM45 gpu in these reports) > > > > archlinux people also noticed that xrandr reports VGA1 as connected > > (while in reality nothing is connected to VGA1) > > Can you please test whether 3.9-rc kernels are affected, too? 3.9.0-rc2-00341-g0863702 works fine here [0.349462] [drm] Initialized drm 1.1.0 20060810 [0.349566] [drm] radeon kernel modesetting enabled. [0.350049] [drm] Memory usable by graphics device = 2048M [0.350110] i915 :00:02.0: setting latency timer to 64 [0.374161] i915 :00:02.0: irq 47 for MSI/MSI-X [0.374170] [drm] Supports vblank timestamp caching Rev 1 (10.10.2010). [0.374229] [drm] Driver supports precise vblank timestamp query. [0.436916] fbcon: inteldrmfb (fb0) is primary device [0.983961] i915 :00:02.0: fb0: inteldrmfb frame buffer device [0.983996] i915 :00:02.0: registered panic notifier [0.986191] [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0 xrandr reports VGA-1 as disconnected properly > We need > to know this since stable rules mandate that a regression is fixed on > upstream first. Once that's figured out we can backport a fix (if > 3.9-rc works) or start working on a fix for 3.8-rc kernels first and > backport afterwards. > > Thanks, Daniel -- Arkadiusz Miśkiewicz, arekm / maven.pl ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/15] drm/i915: only disable DDI sound if intel_crtc->eld_vld
On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni
> >
> > We already have the same check on intel_enable_ddi. This patch
> > prevents "unclaimed register" messages when the power well is
> > disabled.
> >
> > Signed-off-by: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c |9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 56bb7cb..cd2f519 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder
> > *intel_encoder)
> > ironlake_edp_backlight_off(intel_dp);
> > }
> >
> > - tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > - tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
> > - I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> > + if (intel_crtc->eld_vld) {
> > + tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > + tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
> > +(pipe * 4));
> > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> > + }
>
> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> I think you need to rearrange that so that we clear eld_vld only
> after ->crtc_disable has been called.
Just looked a bit through the code here and ->eld_vld is another case of
where the ddi encoder needs to know something which only the connector
really knows currently. Since in our dp/hdmi/sdvo code we just check
->has_audio in the respective modeset function.
And it's not really the only place where the apparently common ddi
functions are just if ladders gropping around in connector details.
I guess we need to eventually clean this mess up, once things have settled
a bit, since I fear the duplication it icky little bugs this fragmentation
of state keeping might (or probably will) cause.
Ideas welcome ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/15] drm/i915: only disable DDI sound if intel_crtc->eld_vld
On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni
> >
> > We already have the same check on intel_enable_ddi. This patch
> > prevents "unclaimed register" messages when the power well is
> > disabled.
> >
> > Signed-off-by: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c |9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 56bb7cb..cd2f519 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder
> > *intel_encoder)
> > ironlake_edp_backlight_off(intel_dp);
> > }
> >
> > - tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > - tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
> > - I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> > + if (intel_crtc->eld_vld) {
> > + tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > + tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
> > +(pipe * 4));
> > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> > + }
>
> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> I think you need to rearrange that so that we clear eld_vld only
> after ->crtc_disable has been called.
I've forgotten to drop my bikeshed on the patch itself:
This looks a bit fishy since currently we assume that disabling something
just works (especially clearing a few registers). And I don't really
understand how we can hit unclaimed register issues since the pipe should
be enabled when we call this function here ...
So either transcoder eDP doesn't have audio, in which case I think it'd be
better to check for that here (plus ensure that we yell at callers for
integrated eDP in e.g. hsw_write_eld). Or it _does_ have audio, but the
audio stuff is in the power well. In which case we need to add a check.
Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
don't want to fire up the work machine ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/15] drm/i915: disable sound first on intel_disable_ddi
On Fri, Mar 15, 2013 at 11:24:03AM -0700, Ben Widawsky wrote: > On Wed, Mar 06, 2013 at 08:03:09PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > Our mode set sequence documentation says audio must be disabled before > > the backlight. > > > > Signed-off-by: Paulo Zanoni > > At least what I see it says, Audio must be disabled, "first." So I'd > recommend modifying the commit message to not mention backlight > specifically > > Reviewed-by: Ben Widawsky Queued for -next with Ben's bikeshed, thanks for the patch. -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 05/15] drm/i915: capture the correct cursor registers on IVB
On Fri, Mar 15, 2013 at 11:45:47AM -0700, Ben Widawsky wrote:
> On Thu, Mar 07, 2013 at 11:34:08AM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 06, 2013 at 08:03:12PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni
> > >
> > > This solves some "unclaimed register" messages when there's a GPU hang
> > > on Haswell.
> > >
> > > Signed-off-by: Paulo Zanoni
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 12 +---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 9a9f6d7..789a95a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9336,9 +9336,15 @@ intel_display_capture_error_state(struct
> > > drm_device *dev)
> > > for_each_pipe(i) {
> > > cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> > >
> > > - error->cursor[i].control = I915_READ(CURCNTR(i));
> > > - error->cursor[i].position = I915_READ(CURPOS(i));
> > > - error->cursor[i].base = I915_READ(CURBASE(i));
> > > + if (INTEL_INFO(dev)->gen <= 6) {
> > > + error->cursor[i].control = I915_READ(CURCNTR(i));
> > > + error->cursor[i].position = I915_READ(CURPOS(i));
> > > + error->cursor[i].base = I915_READ(CURBASE(i));
> > > + } else {
> > > + error->cursor[i].control = I915_READ(CURCNTR_IVB(i));
> > > + error->cursor[i].position = I915_READ(CURPOS_IVB(i));
> > > + error->cursor[i].base = I915_READ(CURBASE_IVB(i));
> > > + }
> >
> > Needs a VLV check.
>
> Has anyone ever used this to actually debug an issue?
>
> Ville's right, I suppose (I'm too lazy to find VLV docs). The non-VLV
> part of the patch is:
> Reviewed-by: Ben Widawsky
Queued for -next with the IS_VLV check added, thanks for the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+
On Fri, Mar 15, 2013 at 12:08:10PM -0700, Ben Widawsky wrote: > On Fri, Mar 15, 2013 at 12:04:11PM -0700, Ben Widawsky wrote: > > On Thu, Mar 07, 2013 at 11:38:24AM +0200, Ville Syrjälä wrote: > > > On Wed, Mar 06, 2013 at 08:03:13PM -0300, Paulo Zanoni wrote: > > > > From: Paulo Zanoni > > > > > > > > So don't read it when capturing the error state. This solves some > > > > "unclaimed register" messages on Haswell when we hang the GPU. > > > > > > You're sure about gen4? I haven't really checked but my impression is > > > that gen4 is more like gen3, and gen4.5 more like gen5 as far as the > > > display is concerned (eg. gen4 would have the old video overlay, and > > > 4.5 would have video sprites). Can anyone confirm? > > > > This register isn't anywhere in modern docs, what's up? > > Ooops, inverted the logic in my head. If you make the check for gen <=6 > which I can lazily check with modern docs: > Reviewed-by: Ben Widawsky I've also checked vanilla gen4 docs and that offset is already marked as reserved, so we're good here. Queued for -next, thanks for the patch. -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 07/15] drm/i915: there's no DSPADDR register on Haswell
On Fri, Mar 15, 2013 at 12:10:02PM -0700, Ben Widawsky wrote: > On Wed, Mar 06, 2013 at 08:03:14PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > So don't read it when we hang the GPU. This solves "unclaimed > > register" messages. > > > > Signed-off-by: Paulo Zanoni > It would be nice if you could make this a bit more future proof, but > looks correct to me: Done. > Reviewed-by: Ben Widawsky Queued for -next, thanks for the patch. -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 08/15] drm/i915: remove DSPPOS register
On Thu, Mar 07, 2013 at 11:43:14AM +0200, Ville Syrjälä wrote: > On Wed, Mar 06, 2013 at 08:03:15PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > I couldn't find any evidence that this register exists on Gen2+. On > > Gen 2/3/4 documents this register is listed as reserved and read-only. > > On the newer Gens this register is not even documented. > > DSPPOS goes hand in hand with DSPSIZE. IIRC DSPA never had the > windowing capability, but DSPB and DSPC on older gens had it. Indeed Ville's right here, gen2/3 have this on sprite B/C (you could move those things around back then to different pipes ...). So I think we want a gen <= 3 check here (gen4+ lost this apparently with the fixed pipe->plane mapping). -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 10/15] drm/i915: check the power well when capturing error state
On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > This solves many "unclaimed register" messages when the power well is > down and we get a GPU hang. > > Also print the power well register and each pipe's CPU transcoder on > the error state to allow proper interpratation of the registers. And > kzalloc the error state structure since we may not read some of the > registers (to avoid the "unclaimed register" messages). > > Signed-off-by: Paulo Zanoni Ok, I've thought some more about all and you're all going to hate me. Essentially I don't like that we have rather invasive patches for a minor feature with _very_ delicate limits as to what hits the debug yelling and what doesn't. Furthermore the debug feature isn't that clear-cut since interrupts get in the way (and we cant fix that since that would also disable underrun interrupts, which we want). And the audio driver also randomly inteferes. Now where it's justified that a register doesn't exist I don't mind merging the patches - it's always good to have accurate documentation of what's really used by the hw. But for debug features I'm really uneasy with runtime checks, since it might very well be that those are wrong. Hence I vote for a I915_READ_NOCHECK to avoid all this madness. This one will never check for errors, but silently clear them after a read. Note that we don't need a I915_WRITE_NOCHECK, and imo that's good since writing random crap could indeed be a serious bug. So I'll punt on this patch since imo it's too messy for future maintenance. Same for the other debug patches (e.g. the assert_pipe_enabled check). Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/15] drm/i915: add HAS_POWER_WELL
On Thu, Mar 07, 2013 at 09:14:56AM -0800, Jesse Barnes wrote: > On Wed, 6 Mar 2013 20:03:18 -0300 > Paulo Zanoni wrote: > > > From: Paulo Zanoni > > > > We're starting to add many IS_HASWELL checks for the power well code, > > so add a HAS_POWER_WELL macro to properly document that we're checking > > for hardware that has the power down well. > > > > To be more future proof, we'll need to differentiate multiple power > wells. Not sure we have good names for them though... I think we'll just invent names once they show up ... Queued for -next, thanks for the patch. I've had to drop the first hunk though since that has been introduced by the previous patch, which I've rejected. -Daniel > > -- > Jesse Barnes, Intel Open Source Technology Center > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/15] drm/i915: check the power well when capturing error state
On Sun, Mar 17, 2013 at 09:46:37PM +0100, Daniel Vetter wrote: > On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > This solves many "unclaimed register" messages when the power well is > > down and we get a GPU hang. > > > > Also print the power well register and each pipe's CPU transcoder on > > the error state to allow proper interpratation of the registers. And > > kzalloc the error state structure since we may not read some of the > > registers (to avoid the "unclaimed register" messages). > > > > Signed-off-by: Paulo Zanoni > > Ok, I've thought some more about all and you're all going to hate me. > > Essentially I don't like that we have rather invasive patches for a minor > feature with _very_ delicate limits as to what hits the debug yelling and > what doesn't. Furthermore the debug feature isn't that clear-cut since > interrupts get in the way (and we cant fix that since that would also > disable underrun interrupts, which we want). And the audio driver also > randomly inteferes. > > Now where it's justified that a register doesn't exist I don't mind > merging the patches - it's always good to have accurate documentation of > what's really used by the hw. But for debug features I'm really uneasy > with runtime checks, since it might very well be that those are wrong. > > Hence I vote for a I915_READ_NOCHECK to avoid all this madness. This one > will never check for errors, but silently clear them after a read. Note > that we don't need a I915_WRITE_NOCHECK, and imo that's good since writing > random crap could indeed be a serious bug. > > So I'll punt on this patch since imo it's too messy for future > maintenance. Same for the other debug patches (e.g. the assert_pipe_enabled > check). Forgotten to add: I like the powerwell related additions to the error state, but that's imo a separate patch. -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 12/15] drm/i915: reorganize intel_lvds_supported
On Fri, Mar 15, 2013 at 01:57:52PM -0700, Ben Widawsky wrote: > On Wed, Mar 06, 2013 at 08:03:19PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > Now it returns false for all platforms unless they're explicitly > > listed on the function. There should be no real difference, except for > > the fact that it now returns false on Haswell. > > > > Signed-off-by: Paulo Zanoni > > I wouldn't mind comments in the commit or code reminding me that IBX and > CPT cover ILk->IVB, and gm45 counts as gen4, but that's just 'cause I > have a bad memory. > Reviewed-by: Ben Widawsky Queued for -next, thanks for the patch. -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 13/15] drm/i915: don't save/restore PCH_LVDS on LPT
On Fri, Mar 15, 2013 at 02:04:22PM -0700, Ben Widawsky wrote:
> On Wed, Mar 06, 2013 at 08:03:20PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni
> >
> > Because the register does not exist on LPT. The interesting fact is
> > that reading/writing PCH_LVDS on LPT does *not* give us "unclaimed
> > register" messages, but the register value is always 0.
> >
> > Signed-off-by: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/i915_suspend.c |7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > b/drivers/gpu/drm/i915/i915_suspend.c
> > index c1e02b0..41f0fde 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -209,7 +209,8 @@ static void i915_save_display(struct drm_device *dev)
> > dev_priv->regfile.saveBLC_PWM_CTL2 =
> > I915_READ(BLC_PWM_PCH_CTL2);
> > dev_priv->regfile.saveBLC_CPU_PWM_CTL =
> > I915_READ(BLC_PWM_CPU_CTL);
> > dev_priv->regfile.saveBLC_CPU_PWM_CTL2 =
> > I915_READ(BLC_PWM_CPU_CTL2);
> > - dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
> > + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> > + dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
> > } else {
> > dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
> > dev_priv->regfile.savePFIT_PGM_RATIOS =
> > I915_READ(PFIT_PGM_RATIOS);
> > @@ -271,9 +272,9 @@ static void i915_restore_display(struct drm_device *dev)
> > if (drm_core_check_feature(dev, DRIVER_MODESET))
> > mask = ~LVDS_PORT_EN;
> >
> > - if (HAS_PCH_SPLIT(dev)) {
> > + if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> > I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS & mask);
> > - } else if (IS_MOBILE(dev) && !IS_I830(dev))
> > + else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
> > I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
> >
> > if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))
>
> We don't support UMS on gen6+ so yuo can probably just check IS_GEN5
> instead of if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
Hah, trapped! We need to restore the LVDS reg even on kms safe for bit31
(which would enable the panel port) since we don't track all the bits of
the panel state. And the bios doesn't really restore that ...
Queued for -next, thanks for the patch.
-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 14/15] drm/i915: check the power well on i915_pipe_enabled
On Wed, Mar 06, 2013 at 08:03:21PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > This fixes "unclaimed register" messages when the power well is > disabled and there's a GPU hang. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_irq.c |3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 29b1bb1..aefc674 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -129,6 +129,9 @@ i915_pipe_enabled(struct drm_device *dev, int pipe) > enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, > pipe); > > + if (intel_power_well_is_down(dev) && cpu_transcoder != TRANSCODER_EDP) > + return false; > + > return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE; Again I vote for I915_READ_NOCHECK instead. -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 15/15] drm/i915: add missing space in error message
On Fri, Mar 15, 2013 at 02:10:20PM -0700, Ben Widawsky wrote: > On Wed, Mar 06, 2013 at 08:03:22PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > > > To avoid this: > > [ 256.798060] [drm] capturing error event; look for more information > > in/sys/kernel/debug/dri/0/i915_error_state > > > > Signed-off-by: Paulo Zanoni > Reviewed-by: Ben Widawsky Queued for -next, thanks for the patch. > You may want to add to the commit message that this fixes the issue > introduced below: > commit 2f86f1916504525a6fdd6b412374b4ebf1102cbe > Author: Ben Widawsky > Date: Mon Jan 28 15:32:15 2013 -0800 > > drm/i915: Error state should print /sys/kernel/debug > ... > [danvet: split up long line.] <- he did it > Signed-off-by: Daniel Vetter > > I personally still believe we shouldn't chop up strings like this. /me hides in shame -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 05/10] [v2] drm/i915: Don't initialize watermark stuff with PCH_NOP
On Wed, Mar 13, 2013 at 02:06:12PM -0700, Ben Widawsky wrote:
> v2: Move check to the top (Chris)
> Add BUG_ON for !ivybridge, since it's all we support for now (Ben)
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/intel_pm.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 52203fd..8e7908b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4153,7 +4153,12 @@ void intel_init_pm(struct drm_device *dev)
> i915_ironlake_get_mem_freq(dev);
>
> /* For FIFO watermark updates */
> - if (HAS_PCH_SPLIT(dev)) {
> + if (HAS_PCH_NOP(dev)) {
> + BUG_ON(!IS_IVYBRIDGE(dev));
> + dev_priv->display.init_clock_gating =
> ivybridge_init_clock_gating;
> + dev_priv->display.update_wm = NULL;
> + dev_priv->display.update_sprite_wm = NULL;
> + } else if (HAS_PCH_SPLIT(dev)) {
> if (IS_GEN5(dev)) {
> if (I915_READ(MLTR_ILK) & ILK_SRLT_MASK)
> dev_priv->display.update_wm =
> ironlake_update_wm;
> @@ -4175,7 +4180,7 @@ void intel_init_pm(struct drm_device *dev)
> dev_priv->display.init_clock_gating =
> gen6_init_clock_gating;
> } else if (IS_IVYBRIDGE(dev)) {
> /* FIXME: detect B0+ stepping and use auto training */
> - if (SNB_READ_WM0_LATENCY()) {
> + if (SNB_READ_WM0_LATENCY() && !HAS_PCH_NOP(dev)) {
> dev_priv->display.update_wm =
> ivybridge_update_wm;
> dev_priv->display.update_sprite_wm =
> sandybridge_update_sprite_wm;
> } else {
I'm confused why we need this patch here - update_wm functions should only
be called when we have a pipe. If there's any caller left I think we
should fix those up, not paper over it here.
And imo it's ok to have non-NULL vfuncs here (or anywhere else, e.g.
pageflips) as long as we don't call them. After all the num_pips/PCH_NOP
checks are here to make this as unobtrusive as possible.
-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 3/9] drm/i915: PCH_NOP
On Wed, Mar 13, 2013 at 11:21:02AM -0700, Ben Widawsky wrote:
> Given certain fusing options discussed in the previous patch, it's
> possible to end up with platforms that normally have PCH but that PCH
> doesn't actually exist. In many cases, this is easily remedied with
> setting 0 pipes. This covers the other corners.
>
> Requiring this is a symptom of improper code splitting (using
> HAS_PCH_SPLIT instead of proper GEN checking, basically). I do not want
> to fix this.
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_display.c | 4 +++-
> drivers/gpu/drm/i915/intel_lvds.c| 4
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 587dca0..ceed199 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -461,6 +461,7 @@ enum intel_pch {
> PCH_IBX,/* Ibexpeak PCH */
> PCH_CPT,/* Cougarpoint PCH */
> PCH_LPT,/* Lynxpoint PCH */
> + PCH_NOP,
> };
>
> enum intel_sbi_destination {
> @@ -1351,6 +1352,7 @@ struct drm_i915_file_private {
> #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
> #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
> #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
> +#define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP)
> #define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
>
> #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index d6dbffd..1f0624e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5085,7 +5085,9 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
> */
> void intel_init_pch_refclk(struct drm_device *dev)
> {
Since this function here only deals with display refclocks I think a
num_pipes == 0 check should equally work. And also makes it clear what's
exactly going on.
> - if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> + if (HAS_PCH_NOP(dev))
> + return;
> + else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> ironlake_init_pch_refclk(dev);
> else if (HAS_PCH_LPT(dev))
> lpt_init_pch_refclk(dev);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 6b24fc5..613ac43 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1018,6 +1018,10 @@ static bool compute_is_dual_link_lvds(struct
> intel_lvds_encoder *lvds_encoder)
>
> static bool intel_lvds_supported(struct drm_device *dev)
> {
> +
> + if (HAS_PCH_NOP(dev))
> + return false;
This hunk here looks superflous, imo we should stop any and all output
probing much earlier in the callchain.
-Daniel
> +
> /* With the introduction of the PCH we gained a dedicated
>* LVDS presence pin, use it. */
> if (HAS_PCH_SPLIT(dev))
> --
> 1.8.1.5
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/i915: Support PCH no display
On Wed, Mar 13, 2013 at 11:21:01AM -0700, Ben Widawsky wrote:
> GEN supports a fusing option which subtracts the PCH display (making the
> CPU display also useless). In this configuration MMIO which gets decoded
> to a certain range will hang the CPU.
>
> For us, this is sort of the equivalent of having no pipes, and we can
> easily modify some code to not do certain things with no pipes.
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_dma.c | 20 ++--
> drivers/gpu/drm/i915/intel_crt.c | 3 +++
> drivers/gpu/drm/i915/intel_display.c | 10 --
> drivers/gpu/drm/i915/intel_fb.c | 3 +++
> drivers/gpu/drm/i915/intel_overlay.c | 3 +++
> 5 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index ebcfe2e..d925504 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1322,6 +1322,10 @@ static int i915_load_modeset_init(struct drm_device
> *dev)
> /* Always safe in the mode setting case. */
> /* FIXME: do pre/post-mode set stuff in core KMS code */
> dev->vblank_disable_allowed = 1;
> + if (INTEL_INFO(dev)->num_pipes == 0) {
> + dev_priv->mm.suspended = 0;
> + return 0;
> + }
>
> ret = intel_fbdev_init(dev);
> if (ret)
> @@ -1630,9 +1634,11 @@ int i915_driver_load(struct drm_device *dev, unsigned
> long flags)
> mutex_init(&dev_priv->rps.hw_lock);
> mutex_init(&dev_priv->modeset_restore_lock);
>
> - ret = drm_vblank_init(dev, INTEL_INFO(dev)->num_pipes);
> - if (ret)
> - goto out_gem_unload;
> + if (INTEL_INFO(dev)->num_pipes) {
> + ret = drm_vblank_init(dev, INTEL_INFO(dev)->num_pipes);
> + if (ret)
> + goto out_gem_unload;
> + }
>
> /* Start out suspended */
> dev_priv->mm.suspended = 1;
> @@ -1647,9 +1653,11 @@ int i915_driver_load(struct drm_device *dev, unsigned
> long flags)
>
> i915_setup_sysfs(dev);
>
> - /* Must be done after probing outputs */
> - intel_opregion_init(dev);
> - acpi_video_register();
> + if (INTEL_INFO(dev)->num_pipes) {
> + /* Must be done after probing outputs */
> + intel_opregion_init(dev);
> + acpi_video_register();
> + }
>
> if (IS_GEN5(dev))
> intel_gpu_ips_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_crt.c
> b/drivers/gpu/drm/i915/intel_crt.c
> index cfc9687..e794c6c 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -736,6 +736,9 @@ void intel_crt_init(struct drm_device *dev)
> if (dmi_check_system(intel_no_crt))
> return;
>
> + if (INTEL_INFO(dev)->num_pipes == 0)
> + return;
Imo better to move this up in the callchain since we should never ever
bother to probe connectors with no pipes.
> +
> crt = kzalloc(sizeof(struct intel_crt), GFP_KERNEL);
> if (!crt)
> return;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 23379e7..d6dbffd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7682,6 +7682,9 @@ intel_modeset_check_state(struct drm_device *dev)
> struct intel_encoder *encoder;
> struct intel_connector *connector;
>
> + if (INTEL_INFO(dev)->num_pipes == 0)
> + return;
Presuming our works correctly this should never be called if there's no
pipes. Could be that the lid handler is offending us, but that one
shouldn't be registered without a panel. Can you please check whether we
really need this?
> +
> list_for_each_entry(connector, &dev->mode_config.connector_list,
> base.head) {
> /* This also checks the encoder/connector hw state with the
> @@ -8326,7 +8329,9 @@ static void intel_setup_outputs(struct drm_device *dev)
> if (!(HAS_DDI(dev) && (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)))
> intel_crt_init(dev);
>
> - if (HAS_DDI(dev)) {
> + if (INTEL_INFO(dev)->num_pipes == 0) {
> + DRM_DEBUG_KMS("Skipping output detection\n");
Move this up and add an early return, then all the other output checks
should be redundant. There's some other stuff called from
intel_modeset_init which might need to be guarded.
> + } else if (HAS_DDI(dev)) {
> int found;
>
> /* Haswell uses DDI functions to detect digital outputs */
> @@ -8443,7 +8448,8 @@ static void intel_setup_outputs(struct drm_device *dev)
>
> intel_init_pch_refclk(dev);
If you drop everything in setup_outputs you also don't have to deal with
the pch_refclock stuff.
Cheers, Daniel
> - drm_helper_move_panel_connectors_to_head(dev);
> + if (INTEL_INFO(dev)->num_pipes)
> + drm_helper_move_p
Re: [Intel-gfx] [PATCH 3/9] drm/i915: PCH_NOP
On Sun, Mar 17, 2013 at 10:05:41PM +0100, Daniel Vetter wrote:
> On Wed, Mar 13, 2013 at 11:21:02AM -0700, Ben Widawsky wrote:
> > Given certain fusing options discussed in the previous patch, it's
> > possible to end up with platforms that normally have PCH but that PCH
> > doesn't actually exist. In many cases, this is easily remedied with
> > setting 0 pipes. This covers the other corners.
> >
> > Requiring this is a symptom of improper code splitting (using
> > HAS_PCH_SPLIT instead of proper GEN checking, basically). I do not want
> > to fix this.
> >
> > Signed-off-by: Ben Widawsky
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_display.c | 4 +++-
> > drivers/gpu/drm/i915/intel_lvds.c| 4
> > 3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 587dca0..ceed199 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -461,6 +461,7 @@ enum intel_pch {
> > PCH_IBX,/* Ibexpeak PCH */
> > PCH_CPT,/* Cougarpoint PCH */
> > PCH_LPT,/* Lynxpoint PCH */
> > + PCH_NOP,
> > };
> >
> > enum intel_sbi_destination {
> > @@ -1351,6 +1352,7 @@ struct drm_i915_file_private {
> > #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
> > #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
> > #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
> > +#define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP)
> > #define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
> >
> > #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index d6dbffd..1f0624e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5085,7 +5085,9 @@ static void lpt_init_pch_refclk(struct drm_device
> > *dev)
> > */
> > void intel_init_pch_refclk(struct drm_device *dev)
> > {
>
> Since this function here only deals with display refclocks I think a
> num_pipes == 0 check should equally work. And also makes it clear what's
> exactly going on.
See my comment on the first patch, I think if we never run the code in
setup_outputs we should be fine here already and don't need anything more.
-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 04/10] [v2] drm/i915: Don't touch South Display when PCH_NOP
On Thu, Mar 14, 2013 at 08:55:16AM -0700, Ben Widawsky wrote:
> Interrupts, clock gating, and GMBUS are all within the, "this will hang
> the CPU" range when we have PCH_NOP.
>
> There is a bit of a hack in init clock gating. We want to do most of the
> clock gating, but the part we skip will hang the system. It could
> probably be abstracted a bit better, but I don't feel it's too
> unsightly.
>
> v2: Use inverse HAS_PCH_NOP check (Jani)
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_irq.c | 20 ++--
> drivers/gpu/drm/i915/intel_bios.c | 3 +++
> drivers/gpu/drm/i915/intel_i2c.c | 4 +++-
> drivers/gpu/drm/i915/intel_pm.c | 3 ++-
> 4 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b860f0b..15b4668 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -738,14 +738,16 @@ static irqreturn_t ivybridge_irq_handler(int irq, void
> *arg)
> }
> }
>
> - /* check event from PCH */
> - if (de_iir & DE_PCH_EVENT_IVB) {
> - u32 pch_iir = I915_READ(SDEIIR);
> + if (!HAS_PCH_NOP(dev)) {
> + /* check event from PCH */
> + if (de_iir & DE_PCH_EVENT_IVB) {
&& the two conditions to not require another indent level (it starts to
look a bit ugly). And the PCH_NOP looks here better than a num_pipes
check, so I'll merge that patch (i.e. disregard my other suggestion to
replace PCH_NOP with num_pipes checks from a previous patch).
Also note that Paulo's SDEIIR race fix added more reads/writes to that
register in -fixes, so please rebase these patches on top of -nightly so
that I can correctly apply them (I'll do the backmerge before slurping in
your series).
> + u32 pch_iir = I915_READ(SDEIIR);
>
> - cpt_irq_handler(dev, pch_iir);
> + cpt_irq_handler(dev, pch_iir);
>
> - /* clear PCH hotplug event before clear CPU irq */
> - I915_WRITE(SDEIIR, pch_iir);
> + /* clear PCH hotplug event before CPU irq */
> + I915_WRITE(SDEIIR, pch_iir);
> + }
> }
>
> I915_WRITE(DEIIR, de_iir);
> @@ -1910,6 +1912,9 @@ static void ironlake_irq_preinstall(struct drm_device
> *dev)
> I915_WRITE(GTIER, 0x0);
> POSTING_READ(GTIER);
>
> + if (HAS_PCH_NOP(dev))
> + return;
> +
> /* south display irq */
> I915_WRITE(SDEIMR, 0x);
> I915_WRITE(SDEIER, 0x0);
> @@ -1982,6 +1987,9 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> SDE_GMBUS_CPT |
> SDE_AUX_MASK_CPT;
>
> + if (HAS_PCH_NOP(dev))
> + return;
> +
> I915_WRITE(SDEIIR, I915_READ(SDEIIR));
> I915_WRITE(SDEIMR, ~mask);
> I915_WRITE(SDEIER, mask);
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 55ffba1..194df27 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -692,6 +692,9 @@ intel_parse_bios(struct drm_device *dev)
> struct bdb_header *bdb = NULL;
> u8 __iomem *bios = NULL;
>
> + if (HAS_PCH_NOP(dev))
> + return -ENODEV;
> +
> init_vbt_defaults(dev_priv);
>
> /* XXX Should this validation be moved to intel_opregion.c? */
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index acf8aec..fc19e49 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -513,7 +513,9 @@ int intel_setup_gmbus(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret, i;
>
> - if (HAS_PCH_SPLIT(dev))
> + if (HAS_PCH_NOP(dev))
> + return 0;
> + else if (HAS_PCH_SPLIT(dev))
> dev_priv->gpio_mmio_base = PCH_GPIOA - GPIOA;
> else if (IS_VALLEYVIEW(dev))
> dev_priv->gpio_mmio_base = VLV_DISPLAY_BASE;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5479363..52203fd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3874,7 +3874,8 @@ static void ivybridge_init_clock_gating(struct
> drm_device *dev)
> snpcr |= GEN6_MBC_SNPCR_MED;
> I915_WRITE(GEN6_MBCUNIT_SNPCR, snpcr);
>
> - cpt_init_clock_gating(dev);
> + if (HAS_PCH_NOP(dev))
> + cpt_init_clock_gating(dev);
Inverted check?
Cheers, Daniel
>
> gen6_check_mch_setup(dev);
> }
> --
> 1.8.1.5
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Ve
Re: [Intel-gfx] [PATCH 6/9] drm/i915: PCH_NOP suspend/resume
On Wed, Mar 13, 2013 at 11:21:05AM -0700, Ben Widawsky wrote:
> More registers we can't write.
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_suspend.c | 57
> ++---
> 1 file changed, 41 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> b/drivers/gpu/drm/i915/i915_suspend.c
> index c1e02b0..dd5766a 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -333,11 +333,19 @@ int i915_save_state(struct drm_device *dev)
>
> mutex_lock(&dev->struct_mutex);
>
> - i915_save_display(dev);
> + if (!HAS_PCH_NOP(dev))
> + i915_save_display(dev);
This here looks a bit funny - imo it's better to move this check to the
only two places where we still touch registers in the kms case: lvds & pp
restore.
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> /* Interrupt state */
> - if (HAS_PCH_SPLIT(dev)) {
> + if (HAS_PCH_NOP(dev)) {
> + dev_priv->regfile.saveDEIER = I915_READ(DEIER);
> + dev_priv->regfile.saveDEIMR = I915_READ(DEIMR);
> + dev_priv->regfile.saveGTIER = I915_READ(GTIER);
> + dev_priv->regfile.saveGTIMR = I915_READ(GTIMR);
> + dev_priv->regfile.saveMCHBAR_RENDER_STANDBY =
> + I915_READ(RSTDBYCTL);
> + } else if (HAS_PCH_SPLIT(dev)) {
> dev_priv->regfile.saveDEIER = I915_READ(DEIER);
> dev_priv->regfile.saveDEIMR = I915_READ(DEIMR);
> dev_priv->regfile.saveGTIER = I915_READ(GTIER);
> @@ -361,13 +369,18 @@ int i915_save_state(struct drm_device *dev)
> /* Memory Arbitration state */
> dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE);
>
> - /* Scratch space */
> - for (i = 0; i < 16; i++) {
> - dev_priv->regfile.saveSWF0[i] = I915_READ(SWF00 + (i << 2));
> - dev_priv->regfile.saveSWF1[i] = I915_READ(SWF10 + (i << 2));
> + if (!HAS_PCH_NOP(dev)) {
> + /* Scratch space */
> + for (i = 0; i < 16; i++) {
> + dev_priv->regfile.saveSWF0[i] =
> + I915_READ(SWF00 + (i << 2));
> + dev_priv->regfile.saveSWF1[i] =
> + I915_READ(SWF10 + (i << 2));
> + }
> + for (i = 0; i < 3; i++)
> + dev_priv->regfile.saveSWF2[i] =
> + I915_READ(SWF30 + (i << 2));
Blergh, I hate those registers, and I have no idea where we actually need
to restore them for kms. Can you please also add a big "XXX: Do we really
need this for kms?" comment in the scratch space block?
> }
> - for (i = 0; i < 3; i++)
> - dev_priv->regfile.saveSWF2[i] = I915_READ(SWF30 + (i << 2));
>
> mutex_unlock(&dev->struct_mutex);
>
> @@ -383,11 +396,17 @@ int i915_restore_state(struct drm_device *dev)
>
> mutex_lock(&dev->struct_mutex);
>
> - i915_restore_display(dev);
> + if (!HAS_PCH_NOP(dev))
> + i915_restore_display(dev);
>
> if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> /* Interrupt state */
> - if (HAS_PCH_SPLIT(dev)) {
> + if (HAS_PCH_NOP(dev)) {
> + I915_WRITE(DEIER, dev_priv->regfile.saveDEIER);
> + I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR);
> + I915_WRITE(GTIER, dev_priv->regfile.saveGTIER);
> + I915_WRITE(GTIMR, dev_priv->regfile.saveGTIMR);
> + } else if (HAS_PCH_SPLIT(dev)) {
> I915_WRITE(DEIER, dev_priv->regfile.saveDEIER);
> I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR);
> I915_WRITE(GTIER, dev_priv->regfile.saveGTIER);
> @@ -407,16 +426,22 @@ int i915_restore_state(struct drm_device *dev)
> /* Memory arbitration state */
> I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE |
> 0x);
>
> - for (i = 0; i < 16; i++) {
> - I915_WRITE(SWF00 + (i << 2), dev_priv->regfile.saveSWF0[i]);
> - I915_WRITE(SWF10 + (i << 2), dev_priv->regfile.saveSWF1[i]);
> + if (!HAS_PCH_NOP(dev)) {
> + for (i = 0; i < 16; i++) {
> + I915_WRITE(SWF00 + (i << 2),
> +dev_priv->regfile.saveSWF0[i]);
> + I915_WRITE(SWF10 + (i << 2),
> +dev_priv->regfile.saveSWF1[i]);
> + }
> + for (i = 0; i < 3; i++)
> + I915_WRITE(SWF30 + (i << 2),
> +dev_priv->regfile.saveSWF2[i]);
> }
> - for (i = 0; i < 3; i++)
> - I915_WRITE(SWF30 + (i << 2), dev_priv->regfile.saveSWF2[i]);
>
> m
Re: [Intel-gfx] [PATCH] tests: add gem_reloc_overflow to check wrapping
On Fri, Mar 15, 2013 at 11:09:07AM -0700, Kees Cook wrote: > This adds a test to make sure that the execbuffer validation routine is > checking for invalid addresses, single entry overflow, and multi-entry > wrapping overflow. > > Signed-off-by: Kees Cook Thanks for writing this testcase, patch merged to i-g-t. -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 8/9] drm/i915: Set PCH_NOP
On Wed, Mar 13, 2013 at 11:21:07AM -0700, Ben Widawsky wrote:
> Set up PCH_NOP when we match a certain platform.
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_drv.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0849651..0fa55cb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -398,11 +398,22 @@ static const struct pci_device_id pciidlist[] = {
> /* aka */
> MODULE_DEVICE_TABLE(pci, pciidlist);
> #endif
>
> +static bool intel_pch_displayless(struct drm_device *dev)
> +{
> + return false;
> +}
> +
> void intel_detect_pch(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct pci_dev *pch;
>
> + if (intel_pch_displayless(dev)) {
Just checking for num_pipes == 0 + a comment not good enough? I know, a
bit hapzardous, but I don't see how the indirection helps code clarity.
-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 10/10] [v2] drm/i915: Add a pipeless ivybridge configuration
On Fri, Mar 15, 2013 at 11:17:55AM -0700, Ben Widawsky wrote:
> FIXME: This is based on some HW being used for a demo. We should
> probably wait until we have confirmation on the IDs before upstreaming
> this patch.
I don't mind too much if we need to fixup the device after the fact, but
checking whether this is the shipping configuration shouldn't hurt.
More important is probably whether there's any quanta platform with the
same sdev/svendor ids without a fused pch. In that case I guess we need to
check for something else (maybe some fuse flags in the pch?).
Anyway, I've done a pretty careful review of everything, mostly looking to
reduce the impact of this feature on our code. So if you respin and
quickly test this on an real ivb (just for paranoia) and the special box
I'll merge it right away. Imo the maintenance burned is really small, so
I'm not against merging this in the demo stage.
Cheers, Daniel
>
> v2: Use GEN7_FEATURES (Chris)
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_drv.c | 19 ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a67e8c7..bd8dfa6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -135,6 +135,16 @@ extern int intel_agp_enabled;
> .subdevice = PCI_ANY_ID,\
> .driver_data = (unsigned long) info }
>
> +#define INTEL_QUANTA_VGA_DEVICE(info) { \
> + .class = PCI_BASE_CLASS_DISPLAY << 16, \
> + .class_mask = 0xff, \
> + .vendor = 0x8086, \
> + .device = 0x16a,\
> + .subvendor = 0x152d,\
> + .subdevice = 0x8990,\
> + .driver_data = (unsigned long) info }
> +
> +
> static const struct intel_device_info intel_i830_info = {
> .gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 2,
> .has_overlay = 1, .overlay_needs_physical = 1,
> @@ -267,6 +277,12 @@ static const struct intel_device_info
> intel_ivybridge_m_info = {
> .is_mobile = 1,
> };
>
> +static const struct intel_device_info intel_ivybridge_q_info = {
> + GEN7_FEATURES,
> + .is_ivybridge = 1,
> + .num_pipes = 0, /* legal, last one wins */
> +};
> +
> static const struct intel_device_info intel_valleyview_m_info = {
> GEN7_FEATURES,
> .is_mobile = 1,
> @@ -337,6 +353,7 @@ static const struct pci_device_id pciidlist[] = {
> /* aka */
> INTEL_VGA_DEVICE(0x0152, &intel_ivybridge_d_info), /* GT1 desktop */
> INTEL_VGA_DEVICE(0x0162, &intel_ivybridge_d_info), /* GT2 desktop */
> INTEL_VGA_DEVICE(0x015a, &intel_ivybridge_d_info), /* GT1 server */
> + INTEL_QUANTA_VGA_DEVICE(&intel_ivybridge_q_info), /* Quanta transcode */
> INTEL_VGA_DEVICE(0x016a, &intel_ivybridge_d_info), /* GT2 server */
> INTEL_VGA_DEVICE(0x0402, &intel_haswell_d_info), /* GT1 desktop */
> INTEL_VGA_DEVICE(0x0412, &intel_haswell_d_info), /* GT2 desktop */
> @@ -386,7 +403,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>
> static bool intel_pch_displayless(struct drm_device *dev)
> {
> - return false;
> + return INTEL_INFO(dev) == &intel_ivybridge_q_info;
> }
>
> void intel_detect_pch(struct drm_device *dev)
> --
> 1.8.1.5
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote: > On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson > wrote: > > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl > > command (which are seperate from the ioctl number), then kdata is set to > > NULL. > > Doesn't that mean that we need these checks everywhere? Or at least a > fixup in drm core proper? That's my conclusion. We either add a flag to ask drm_ioctl to prevent passing NULL pointers (as the existing behaviour may be useful somewhere, and I have not checked all callees) or saturate our callbacks with NULL checks. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson wrote: > On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote: >> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson >> wrote: >> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl >> > command (which are seperate from the ioctl number), then kdata is set to >> > NULL. >> >> Doesn't that mean that we need these checks everywhere? Or at least a >> fixup in drm core proper? > > That's my conclusion. We either add a flag to ask drm_ioctl to prevent > passing NULL pointers (as the existing behaviour may be useful > somewhere, and I have not checked all callees) or saturate our callbacks > with NULL checks. Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well? we could check them and block NULL in that case. Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 10/16] drm/i915: add struct ctx_reset_state
On Fri, Mar 15, 2013 at 09:52:43AM +, Chris Wilson wrote: > On Thu, Mar 14, 2013 at 05:52:11PM +0200, Mika Kuoppala wrote: > > To count context losses, add struct ctx_reset_state for > > both i915_hw_context and drm_i915_file_private. > > drm_i915_file_private is used when there is no context. > > Being really picky, but can we device a better name than reset_state. I > keep reading 'reset' as a verb and get very confused... > > Even just gpu_reset reads better. Suggestions? hang_stats? failure_stats? In any case the struct definition itself needs a i915_ prefix. -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 v2 04/16] drm/i915: Resurrect ring kicking for semaphores, selectively
On Thu, Mar 14, 2013 at 05:52:05PM +0200, Mika Kuoppala wrote: > From: Chris Wilson > > Once we thought we got semaphores working, we disabled kicking the ring > if hangcheck fired whilst waiting upon a ring as it was doing more harm > than good: > > commit 4e0e90dcb8a7df1229c69e30abebb59b0b3c2a1f > Author: Daniel Vetter > Date: Wed Dec 14 13:56:58 2011 +0100 > > drm/i915: kicking rings stuck on semaphores considered harmful > > However, life is never that easy and semaphores are still causing > problems whereby the value written by one ring (bcs) is not being > propagated to the waiter (rcs). Thus the waiter never wakes up and we > declare the GPU hung, which often has unfortunate consequences, even if > we successfully reset the GPU. > > But the GPU is idle as it has completed the work, just didn't notify its > clients. So we can detect the incomplete wait during hang check and > probe the target ring to see if has indeed emitted the breadcrumb seqno > following the work and then and only then kick the waiter. > > Based on a suggestion by Ben Widawsky. > > v2: cross-check wait with iphdr. fix signaller calculation. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=54226 > Signed-off-by: Chris Wilson > Signed-off-by: Mika Kuoppala > Cc: Daniel Vetter > Cc: Ben Widawsky > Acked-by: Ben Widawsky I'll throw in the towel, let's all hail duct-tape. Queued for -next, thanks for the patch. -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: Sanity check incoming ioctl data for a NULL pointer
On Mon, Mar 18, 2013 at 07:42:58AM +1000, Dave Airlie wrote:
> On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson
> wrote:
> > On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
> >> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson
> >> wrote:
> >> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
> >> > command (which are seperate from the ioctl number), then kdata is set to
> >> > NULL.
> >>
> >> Doesn't that mean that we need these checks everywhere? Or at least a
> >> fixup in drm core proper?
> >
> > That's my conclusion. We either add a flag to ask drm_ioctl to prevent
> > passing NULL pointers (as the existing behaviour may be useful
> > somewhere, and I have not checked all callees) or saturate our callbacks
> > with NULL checks.
>
> Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well?
>
> we could check them and block NULL in that case.
Yes. For the core ioctls, we use drm_ioctls[nr].cmd rather than the
value passed in by userspace for the IOC_IN|IN_OUT bits. So:
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 25f91cd..79b8bd1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
usize = asize = _IOC_SIZE(cmd);
if (drv_size > asize)
asize = drv_size;
+ cmd = ioctl->cmd;
}
else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
ioctl = &drm_ioctls[nr];
--
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] Revert "drm/i915: try to train DP even harder"
On Mon, Mar 11, 2013 at 06:40:16PM +0100, Takashi Iwai wrote: > This reverts commit 0d71068835e2610576d369d6d4cbf90e0f802a71. > > Not only that the commit introduces a bogus check (voltage_tries == 5 > will never meet at the inserted code path), it brings the i915 driver > into an endless dp-train loop on HP Z1 desktop machine with IVY+eDP. > > At least reverting this commit recovers the framebuffer (but X is > still broken by other reasons...) > > Cc: > Signed-off-by: Takashi Iwai Picked up for -fixes, thanks for the patch. Adding Paulo since it's his patch. To assign proper blame please cc: relevant people when sending out reverts. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Use the fixed pixel clock for eDP in intel_dp_set_m_n()
On Tue, Mar 12, 2013 at 04:32:28PM +0100, Takashi Iwai wrote: > The eDP output on HP Z1 is still broken when X is started even after > fixing the infinite link-train loop. The regression was introduced in > 3.6 kernel for cleaning up the mode clock handling code in intel_dp.c. > > In the past, the clock of the reference mode was modified in > intel_dp_mode_fixup() in the case of eDP fixed clock, and this clock was > used for calculating in intel_dp_set_m_n(). This override was removed, > thus the wrong mode clock is used for the calculation, resulting in a > psychedelic smoking output in the end. > > This patch corrects the clock to be used in the place. > > Cc: > Signed-off-by: Takashi Iwai I truly hate this mess of dotclock vs portclock vs. whatever. Can you pls apply a little bikeshed and use the existing intel_edp_target_clock like in ironlake_set_m_n? And if you have the regressing commit a little citation to assign the blame (it's probably me) would be good. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: Remove unneeded dev argument
On Wed, Mar 13, 2013 at 05:21:06PM -0700, Ben Widawsky wrote: > Signed-off-by: Ben Widawsky Merged the first two patches for -next, thanks for the patches. I need to discuss a bit more with you what exactly you're aiming for - as is with the two patches and your quick description I fear way too many object lifetime issues due to lack of refcounts are ahead ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
