Re: [Intel-gfx] [PATCH 0/5] refactor power management into intel_pm
On Mon, Apr 16, 2012 at 10:20:33PM -0300, Eugeni Dodonov wrote: As Chris Wilson noticed, my previous patch that did the refactoring as one big patch which moved everything at once was extremely difficult to review and maintain. So I split the same refactoring into a series of smaller patches, which move one subsystem at a time. Yeah, diffs are not actually readable, so looks like we have to do this piece-wise. One thing that really bugs me is that we have all these per-chip function declared as extern now, and the setup code for them is somewhere completely different. So can you add another patch add the end that moves all that setup code intel intel_init_pm (at the end of intel_pm.c to avoid forward decl) and removes all these chip-specific extern declarations? If that patch doesn't result in only a few generic functinos exported from intel_pm.c (and hence a simple interface) I think we need to reconsider whether we've moved the right things in there (maybe it needs more/less). But I can't easily judge that atm. Yours, Daniel As previously, this reduces the intel_display.c module by around 2800 lines while also simplifying future power-related developments. Eugeni Dodonov (5): drm/i915: move fbc-related functionality into intel_pm module drm/i915: move watermarks settings into intel_pm module drm/i915: move drps, rps and rc6-related functions to intel_pm drm/i915: move emon functionality into intel_pm module drm/i915: move clock gating functionality into intel_pm module drivers/gpu/drm/i915/Makefile|1 + drivers/gpu/drm/i915/intel_display.c | 7803 +++--- drivers/gpu/drm/i915/intel_drv.h | 72 + drivers/gpu/drm/i915/intel_pm.c | 2898 + 4 files changed, 5432 insertions(+), 5342 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_pm.c -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Wait for all pending operations to the fb before disabling the pipe
During modeset we have to disable the pipe to reconfigure its timings and maybe its size. Userspace may have queued up command buffers that depend upon the pipe running in a certain configuration and so the commands may become confused across the modeset. At the moment, we use a less than satisfactory kick-scanline-waits should the GPU hang during the modeset. It should be more reliable to wait for the pending operations to complete first, even though we still have a window for userspace to submit a broken command buffer during the modeset. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_display.c | 27 --- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8be3091..72980be 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2910,16 +2910,14 @@ static void intel_clear_scanline_wait(struct drm_device *dev) static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc) { - struct drm_i915_gem_object *obj; - struct drm_i915_private *dev_priv; + struct drm_device *dev = crtc-dev; if (crtc-fb == NULL) return; - obj = to_intel_framebuffer(crtc-fb)-obj; - dev_priv = crtc-dev-dev_private; - wait_event(dev_priv-pending_flip_queue, - atomic_read(obj-pending_flip) == 0); + mutex_lock(dev-struct_mutex); + intel_finish_fb(crtc-fb); + mutex_unlock(dev-struct_mutex); } static bool intel_crtc_driving_pch(struct drm_crtc *crtc) @@ -3381,23 +3379,6 @@ static void intel_crtc_disable(struct drm_crtc *crtc) struct drm_crtc_helper_funcs *crtc_funcs = crtc-helper_private; struct drm_device *dev = crtc-dev; - /* Flush any pending WAITs before we disable the pipe. Note that -* we need to drop the struct_mutex in order to acquire it again -* during the lowlevel dpms routines around a couple of the -* operations. It does not look trivial nor desirable to move -* that locking higher. So instead we leave a window for the -* submission of further commands on the fb before we can actually -* disable it. This race with userspace exists anyway, and we can -* only rely on the pipe being disabled by userspace after it -* receives the hotplug notification and has flushed any pending -* batches. -*/ - if (crtc-fb) { - mutex_lock(dev-struct_mutex); - intel_finish_fb(crtc-fb); - mutex_unlock(dev-struct_mutex); - } - crtc_funcs-dpms(crtc, DRM_MODE_DPMS_OFF); assert_plane_disabled(dev-dev_private, to_intel_crtc(crtc)-plane); assert_pipe_disabled(dev-dev_private, to_intel_crtc(crtc)-pipe); -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: set stc evict disable lra evict w/a
On Wed, Apr 11, 2012 at 08:42:42PM +0200, Daniel Vetter wrote: Our workaround list kindly lists that this new default value needs to be updated in Bspec. Naturally, this did not happen. Acked-by: Ben Widawsky b...@bwidawsk.net Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch I've queued the entire series for -next. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: enable semaphores on gen6 if dmar is not active
On Tue, Apr 03, 2012 at 12:32:05AM +0200, Daniel Vetter wrote: On Mon, Apr 02, 2012 at 02:44:14PM -0700, Andrew Lutomirski wrote: On Mon, Apr 2, 2012 at 1:52 PM, Daniel Vetter dan...@ffwll.ch wrote: On Mon, Apr 02, 2012 at 01:45:39PM -0700, Andrew Lutomirski wrote: On Mon, Apr 2, 2012 at 11:48 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote: Inspired by the recent ppgtt regression report, where switching of dmar only for the gpu seems to fix things completely, I've looked again at the semaphores+vt-d situation. Contrary to my earlier testing a few months back my system is now stable with dmar disabled for the igd, and not only when disabling dmar completely. So I'm rather hopeful that all our recent fixes for snb have changed things for code and it's time to try enabling semaphores again. We've also had issues with enabling semaphores which are not vt-d related, but I guess these are all fixed by the autoreport-disabling and lazy request fix. And there's only one way to find out whether there are still other issues ... Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch --- If no further vt-d regressions show up in the 3.4 cycle I plan to merge this into -next for 3.5 (in a month or so). Comments about how unfeasibly you deem this highly welcome. -Daniel --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 8e0b686..ac52433 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -849,9 +849,11 @@ intel_enable_semaphores(struct drm_device *dev) if (i915_semaphores = 0) return i915_semaphores; - /* Disable semaphores on SNB */ - if (INTEL_INFO(dev)-gen == 6) - return 0; +#ifdef CONFIG_INTEL_IOMMU + /* Disable semaphores on SNB if VT-d is on. */ + if (INTEL_INFO(dev)-gen == 6 intel_iommu_gfx_mapped) + return false; It might be nice to have a printk here giving instructions for how to work around this. For example: i915: Disabling semaphores to work around a DMAR issue. As an alternative, boot with intel_iommu=igfx_off [or on, igfx_off, or whatever it's supposed to be]. The documentation in kernel-parameters.txt is at best unclear to the uninitiated. If this really does work, I'll look into this. Atm it's still unclear where to exactly put the plain, we need to annoy internal hardware people to analyze this. Once we have enough evidence that the combination of gpu with various features enable plus VT-d just doesn't seem to work reliably. So can you check whether things do indeed work with this patch, atop kernel 3.4-rc1? Iirc you've been the one with the most trouble when semaphores are enabled ... I've had a hard time reproducing the problems lately. The unconditional instant hard hang went away a few months ago, I think. I'll give it another shot when I get home to that machine. Well, I've managed to kill my machine shortly after login with semaphores, rc6 and dmar enabled. It hasn't died in the same configuration after a few hours of light usage (in my case that seems to be the best killer) with dmar disabled for just the gpu. So if you can give your machine some serious beating with the different options, that's really great. Do you already have some preliminary results or has your machine not yet died with this patch applied? Thanks, Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only free the unpin_work if cancelled before being run
On Tue, Apr 17, 2012 at 10:29:38AM +0100, Chris Wilson wrote: The unpin worker frees it work struct and so during intel_crtc_disable s/disable/destroy/ we should only also free the work struct if cancel_work_sync() reports that it successfully cancelled the work prior to it being executed and thus avoid the double free. The impact is only for people unloading modules during a fullscreen game or movie playback, so extremely small. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/intel_display.c |4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8298b72..78390e8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7602,10 +7602,8 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) intel_crtc-unpin_work = NULL; spin_unlock_irqrestore(dev-event_lock, flags); - if (work) { - cancel_work_sync(work-work); + if (work cancel_work_sync(work-work)) kfree(work); - } drm_crtc_cleanup(crtc); -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only free the unpin_work if cancelled before being run
On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson ch...@chris-wilson.co.uk wrote: The unpin worker frees it work struct and so during intel_crtc_disable we should only also free the work struct if cancel_work_sync() reports that it successfully cancelled the work prior to it being executed and thus avoid the double free. The impact is only for people unloading modules during a fullscreen game or movie playback, so extremely small. Futher review (hunting for some sign of workqueue corruption, cf https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is non-NULL here it will not have been scheduled so cancel_work_sync() will always return true. Which also means that we have no way of waiting upon the scheduled unpin_work. :| -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 0/5] refactor power management into intel_pm
On Tue, Apr 17, 2012 at 05:57, Daniel Vetter dan...@ffwll.ch wrote: On Mon, Apr 16, 2012 at 10:20:33PM -0300, Eugeni Dodonov wrote: As Chris Wilson noticed, my previous patch that did the refactoring as one big patch which moved everything at once was extremely difficult to review and maintain. So I split the same refactoring into a series of smaller patches, which move one subsystem at a time. Yeah, diffs are not actually readable, so looks like we have to do this piece-wise. Please, tell me that you meant 'diffs are *now* actually readable' with a 'not' being a typo instead of 'now' there by a chance... Because otherwise I couldn't understand what you meant with piece-wise. Should I move each function at a time? :) I'll do the intel_pm_init (or intel_init_pm) and send it on top of this series. It makes lots of sense and will avoid exposing all those platform-specific stuff where we won't need it except for initialization. -- Eugeni Dodonov http://eugeni.dodonov.net/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/29] drm/i915: add Haswell DIP controls registers
On Fri, Apr 13, 2012 at 05:08:39PM -0300, Eugeni Dodonov wrote: Haswell has different DIP control registers and offsets. Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com I've read a bit through Bspec wrt dip writing and it looks like hsw is rather differen from previous chips: - with have a data reg for every type of dip - the bits in the ctl reg moved around completely ... so I guess this patch and the follow-on one are pretty bogus. One thing I've noticed is that intel_infoframe_index and intel_infoframe_flags have way too generic names, they're only useful to frob the dip ctl reg on pre-hsw afaics. I think we should rename them to i9xx_infoframe_ctl_inde and _flags or something similar. -Daniel --- drivers/gpu/drm/i915/i915_reg.h | 16 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 05d98f2..8cc53fb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3488,6 +3488,22 @@ #define VLV_TVIDEO_DIP_GCP(pipe) \ _PIPE(pipe, VLV_VIDEO_DIP_GDCP_PAYLOAD_A, VLV_VIDEO_DIP_GDCP_PAYLOAD_B) +/* Haswell DIP controls */ +#define HSW_VIDEO_DIP_CTL_A 0x60200 +#define HSW_VIDEO_DIP_AVI_DATA_A 0x60220 +#define HSW_VIDEO_DIP_GCP_A 0x60210 + +#define HSW_VIDEO_DIP_CTL_B 0x61200 +#define HSW_VIDEO_DIP_AVI_DATA_B 0x61220 +#define HSW_VIDEO_DIP_GCP_B 0x61210 + +#define HSW_TVIDEO_DIP_CTL(pipe) \ + _PIPE(pipe, HSW_VIDEO_DIP_CTL_A, HSW_VIDEO_DIP_CTL_B) +#define HSW_TVIDEO_DIP_AVI_DATA(pipe) \ + _PIPE(pipe, HSW_VIDEO_DIP_AVI_DATA_A, HSW_VIDEO_DIP_AVI_DATA_B) +#define HSW_TVIDEO_DIP_GCP(pipe) \ + _PIPE(pipe, HSW_VIDEO_DIP_GCP_A, HSW_VIDEO_DIP_GCP_B) + #define _TRANS_HTOTAL_B 0xe1000 #define _TRANS_HBLANK_B 0xe1004 #define _TRANS_HSYNC_B 0xe1008 -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/29] drm/i915: show unknown sdvox registers on hdmi init
On Fri, Apr 13, 2012 at 05:08:49PM -0300, Eugeni Dodonov wrote: Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com This is pretty much a implementation bug, and with the new DDI split I'm not too sure we'll need this. I'm ingoring this for now ;-) -Daniel --- drivers/gpu/drm/i915/intel_hdmi.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 700bd0b..0978fb7 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -614,6 +614,8 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg) intel_encoder-clone_mask = (1 INTEL_HDMIF_CLONE_BIT); intel_hdmi-ddc_bus = GMBUS_PORT_DPD; dev_priv-hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS; + } else { + DRM_DEBUG_DRIVER(Unknown sdvox register %x\n, sdvox_reg); } intel_hdmi-sdvox_reg = sdvox_reg; -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/29] drm/i915: detect PCH encoders on Haswell
On Fri, Apr 13, 2012 at 05:08:52PM -0300, Eugeni Dodonov wrote: On Haswell, the only PCH-connected output is the one driven by DDI E in FDI mode, used for VGA connection. All the others are handled by the CPU. Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com --- drivers/gpu/drm/i915/intel_display.c |7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e4ebd39..b01afb0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3098,6 +3098,13 @@ static bool intel_crtc_driving_pch(struct drm_crtc *crtc) if (encoder-base.crtc != crtc) continue; + /* On Haswell, only DDI E can connect to PCH through FDI to drive VGA */ + if (IS_HASWELL(dev) (encoder-type != DRM_MODE_ENCODER_DAC)) { + DRM_DEBUG_KMS(Non-PCH encoder detected on Haswell. Allowed: %d, detected: %d\n, + DRM_MODE_ENCODER_DAC, encoder-type); + return false; + } Shouldn't we use a PCH_LTP check here, given that it looks like hsd can be combined with ppt? Or is it really only DDI E that can be used for fdi and hsw+ppt would only be able to drive 1 pch output? -Daniel + switch (encoder-type) { case INTEL_OUTPUT_EDP: if (!intel_encoder_is_pch_edp(encoder-base)) -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only free the unpin_work if cancelled before being run
On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote: On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson ch...@chris-wilson.co.uk wrote: The unpin worker frees it work struct and so during intel_crtc_disable we should only also free the work struct if cancel_work_sync() reports that it successfully cancelled the work prior to it being executed and thus avoid the double free. The impact is only for people unloading modules during a fullscreen game or movie playback, so extremely small. Futher review (hunting for some sign of workqueue corruption, cf https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is non-NULL here it will not have been scheduled so cancel_work_sync() will always return true. Well, I've failed to notice this while reviewing the unpin_work life-cycle ... Which also means that we have no way of waiting upon the scheduled unpin_work. :| ... but have noticed that we lose any reference from intel_crtc to the unpin work once it's scheduled, and checked that we properly flush the work queue: See the ordering of irq disable, flush workqueue, then crtc destroy (in mode_config_cleanup). We even have a flush_workqueu in i915_dma.c before tearing down gem, but that won't work too well now that unpin also frobs around with the fbc state (which is gone by now). But there's still the misleading comment that this syncs up with unpin work. Care to clean this up a bit by - ditching the unnecessary flush_workqueu in i915_dma.c - move the comment about syncing up with unpin_work to where we actually sync up - and kill the superflous cancel_work? Cheers, Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/29] drm/i915: share pipe count handling with Ivybridge
On Tue, 17 Apr 2012 12:19:16 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 13, 2012 at 05:08:47PM -0300, Eugeni Dodonov wrote: Reviewed-by: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com --- drivers/gpu/drm/i915/intel_display.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3d78686..5ee652d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2427,7 +2427,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, case 1: break; case 2: - if (IS_IVYBRIDGE(dev)) + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) break; /* fall through otherwise */ default: Imo this code is a rather funky way to check for 3 plane support ... I think we should just replace this entire switch statement with a if(WARN_ON(intel_crtc-plane dev_priv-num_pipes)) return -EINVAL; Or has there been another reason for this? Chris, git blame says you've originally added this in 5c3b82e2, any comments? Yup, it's just a userspace (and internal consistency) validation check, so if (pipe = dev_priv-num_pipes) return -EINVAL; would have sufficed. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/29] Haswell round 3
On Fri, Apr 13, 2012 at 05:08:36PM -0300, Eugeni Dodonov wrote: Hi forks, Just in time for everyone's weekend, here comes the 3rd round of patches on Haswell. As major highlights, it also adds support for HDMI/DVI outputs and multi-head modes (I tried with VGA and HDMI). Other than that, it is the same patches with comments from the past round addressed (SBI locking support, proper WM calculation, better PCH items detection, and so on). Daniel, I think that the bits definitions, power wells, clocks programming, and modesetting for both FDI and HDMI modes should be good to go unless someone spots additional issues with them - so please, all of you who have something to say about them, say it now and bikeshed as you please :). I've picked up a few more patches I couldn't find anything to bikeshed about, the others grew some comments. I haven't looked too hard at the actual modeset stuff given that you're already really busy reworking things and moving stuff to intel_ddi.c For the modeset stuff 2 general comments so you know from where the bikeshed will hit you in the next review round: - if possible, I'd like to hide most of the 'disable pch stuff on hsw' stuff behind crtc_driving_pch checks. - I think we need to be careful about IS_HASWELL vs PCH_LPT checks, otherwise hsw+ppt will not work so great. For the dip/infoframe stuff, maybe you can volunteer Paulo to help you out, he's looking into this atm anyway. Cheers, Daniel Note that the DP and eDP modesetting support is not there yet - it will still require a considerable amount of patches. Also, there is one patch which fixes null pointer exceptions in gmbus code I was having with some of the drm-intel-next-queued iterations, but I don't think it is necessary at the moment now that gmbus stuff was disabled again (patch 5). I am not even sure if we'll hit those code paths with invalid values in real life, so I simple added some checks for cases when we don't have a valid adapter as it was looking too error-prone otherwise. Perhaps we could add a WARN into them as well. Eugeni Dodonov (29): drm/i915: add definition of LPT FDI port width registers drm/i915: add WRPLL divider programming bits drm/i915: add Haswell DIP controls registers drm/i915: support infoframes on Haswell drm/i915: prevent NULL pointer exception when using gmbus drm/i915: add support for SBI ops drm/i915: calculate same watermarks on Haswell as on Ivy Bridge drm/i915: share forcewaking code between IVB and HSW drm/i915: haswell has 3 pipes as well drm/i915: reuse Ivybridge interrupts code for Haswell drm/i915: share pipe count handling with Ivybridge drm/i915: share IVB cursor routine with Haswell drm/i915: show unknown sdvox registers on hdmi init drm/i915: do not use fdi_normal_train on haswell drm/i915: do not enable PCH PLL on pre-haswell drm/i915: detect PCH encoders on Haswell drm/i915: enable power wells on haswell init drm/i915: disable rc6 on haswell for now drm/i915: program WM_LINETIME on Haswell drm/i915: do not use old code paths on Haswell drm/i915: initialize DDI buffer translations drm/i915: perform Haswell DDI link training in FDI mode drm/i915: disable pipe DDI function when disabling pipe drm/i915: program iCLKIP on Lynx Point drm/i915: detect digital outputs on Haswell drm/i915: add support for DDI-controlled digital outputs drm/i915: add WR PLL programming table drm/i915: prepare HDMI link for Haswell drm/i915: hook Haswell devices in place drivers/char/agp/intel-agp.c |4 + drivers/gpu/drm/i915/i915_dma.c |2 +- drivers/gpu/drm/i915/i915_drv.c |7 + drivers/gpu/drm/i915/i915_irq.c |6 +- drivers/gpu/drm/i915/i915_reg.h | 23 + drivers/gpu/drm/i915/intel_display.c | 763 -- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_hdmi.c| 602 ++- 8 files changed, 1357 insertions(+), 51 deletions(-) -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/29] drm/i915: share pipe count handling with Ivybridge
On Tue, Apr 17, 2012 at 11:36:33AM +0100, Chris Wilson wrote: On Tue, 17 Apr 2012 12:19:16 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 13, 2012 at 05:08:47PM -0300, Eugeni Dodonov wrote: Reviewed-by: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com --- drivers/gpu/drm/i915/intel_display.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3d78686..5ee652d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2427,7 +2427,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, case 1: break; case 2: - if (IS_IVYBRIDGE(dev)) + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) break; /* fall through otherwise */ default: Imo this code is a rather funky way to check for 3 plane support ... I think we should just replace this entire switch statement with a if(WARN_ON(intel_crtc-plane dev_priv-num_pipes)) return -EINVAL; Or has there been another reason for this? Chris, git blame says you've originally added this in 5c3b82e2, any comments? Yup, it's just a userspace (and internal consistency) validation check, so if (pipe = dev_priv-num_pipes) return -EINVAL; would have sufficed. Hm, how can userspace trigger this? It can only pass in crtc ids, which the drm core validates. intel_crtc-plane is completely in our control, hence why I think this can only be a driver bug. Or do I miss something? -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/29] drm/i915: share pipe count handling with Ivybridge
On Tue, 17 Apr 2012 13:26:19 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 17, 2012 at 11:36:33AM +0100, Chris Wilson wrote: On Tue, 17 Apr 2012 12:19:16 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 13, 2012 at 05:08:47PM -0300, Eugeni Dodonov wrote: Reviewed-by: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com --- drivers/gpu/drm/i915/intel_display.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3d78686..5ee652d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2427,7 +2427,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, case 1: break; case 2: - if (IS_IVYBRIDGE(dev)) + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) break; /* fall through otherwise */ default: Imo this code is a rather funky way to check for 3 plane support ... I think we should just replace this entire switch statement with a if(WARN_ON(intel_crtc-plane dev_priv-num_pipes)) return -EINVAL; Or has there been another reason for this? Chris, git blame says you've originally added this in 5c3b82e2, any comments? Yup, it's just a userspace (and internal consistency) validation check, so if (pipe = dev_priv-num_pipes) return -EINVAL; would have sufficed. Hm, how can userspace trigger this? It can only pass in crtc ids, which the drm core validates. intel_crtc-plane is completely in our control, hence why I think this can only be a driver bug. Or do I miss something? Shooting the messanger here. :-p Right, userspace cannot assign pipes, that is purely an internal detail. So this can be promoted to a if (WARN(pipe = num_pipes)) return -EINVAL; or killed outright. All I did in that commit was perform the existing consistency check upfront, and return -EINVAL alongside the DRM_ERROR. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/29] Haswell round 3
On Tue, Apr 17, 2012 at 07:49, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 13, 2012 at 05:08:36PM -0300, Eugeni Dodonov wrote: Hi forks, Just in time for everyone's weekend, here comes the 3rd round of patches on Haswell. As major highlights, it also adds support for HDMI/DVI outputs and multi-head modes (I tried with VGA and HDMI). Other than that, it is the same patches with comments from the past round addressed (SBI locking support, proper WM calculation, better PCH items detection, and so on). Daniel, I think that the bits definitions, power wells, clocks programming, and modesetting for both FDI and HDMI modes should be good to go unless someone spots additional issues with them - so please, all of you who have something to say about them, say it now and bikeshed as you please :). I've picked up a few more patches I couldn't find anything to bikeshed about, the others grew some comments. I haven't looked too hard at the actual modeset stuff given that you're already really busy reworking things and moving stuff to intel_ddi.c ...and into intel_pm.c as well when those patches land :). At least the power wells-related part is much more welcome there. So I'll address the bikeshedding^w comments on the remaining patches and will resend them when intel_pm.c stuff lands. -- Eugeni Dodonov http://eugeni.dodonov.net/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/29] drm/i915: add Haswell DIP controls registers
On Tue, Apr 17, 2012 at 07:12, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Apr 13, 2012 at 05:08:39PM -0300, Eugeni Dodonov wrote: Haswell has different DIP control registers and offsets. Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com I've read a bit through Bspec wrt dip writing and it looks like hsw is rather differen from previous chips: - with have a data reg for every type of dip - the bits in the ctl reg moved around completely ... so I guess this patch and the follow-on one are pretty bogus. One thing I've noticed is that intel_infoframe_index and intel_infoframe_flags have way too generic names, they're only useful to frob the dip ctl reg on pre-hsw afaics. I think we should rename them to i9xx_infoframe_ctl_inde and _flags or something similar. Yep, I only did the minimally-required stuff to have HDMI output, so the ones willing to use Haswell could have anything on their screen besides 'No Signal' :). But yes, those patches should receive more care for full hdmi and DIP support going forward. -- Eugeni Dodonov http://eugeni.dodonov.net/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only free the unpin_work if cancelled before being run
On Tue, Apr 17, 2012 at 03:44:01PM +0200, Daniel Vetter wrote: On Tue, Apr 17, 2012 at 02:30:35PM +0100, Chris Wilson wrote: On Tue, 17 Apr 2012 12:33:46 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 17, 2012 at 10:53:24AM +0100, Chris Wilson wrote: On Tue, 17 Apr 2012 10:29:38 +0100, Chris Wilson ch...@chris-wilson.co.uk wrote: The unpin worker frees it work struct and so during intel_crtc_disable we should only also free the work struct if cancel_work_sync() reports that it successfully cancelled the work prior to it being executed and thus avoid the double free. The impact is only for people unloading modules during a fullscreen game or movie playback, so extremely small. Futher review (hunting for some sign of workqueue corruption, cf https://bugs.freedesktop.org/show_bug.cgi?id=48798) says that if work is non-NULL here it will not have been scheduled so cancel_work_sync() will always return true. Well, I've failed to notice this while reviewing the unpin_work life-cycle ... Which also means that we have no way of waiting upon the scheduled unpin_work. :| ... but have noticed that we lose any reference from intel_crtc to the unpin work once it's scheduled, and checked that we properly flush the work queue: See the ordering of irq disable, flush workqueue, then crtc destroy (in mode_config_cleanup). We even have a flush_workqueu in i915_dma.c before tearing down gem, but that won't work too well now that unpin also frobs around with the fbc state (which is gone by now). But there's still the misleading comment that this syncs up with unpin work. Care to clean this up a bit by - ditching the unnecessary flush_workqueu in i915_dma.c Indeed looks superfluous. If we consider the unlikelihood the cleanup code being well-tested, I'd prefer that we did do something like drain_workqueue() as the first step in unload(). Sounds good. - move the comment about syncing up with unpin_work to where we actually sync up I'm not finding another point where we explicitly sync with outstanding unpin work. Probably due to the comment being in the wrong place... Well, I've just noticed that we call flush_scheduled_work instead of flush_workqueue (i.e. the global one instead of our own), but we also put the unpin work onto the global queue with schedule_work instead of our own with queue_work - and kill the superflous cancel_work? Which also reminds me that the unpin_work holds onto a few references that need to be released as well as the kfree. See above about these code paths being relatively untrod. Oops. The problem is that unpin_work also calls the fbc update, so we need to ensure that this is done before the fbc cleanup happened. Fun. While we bitch around about this code: unpin_work is a bit misleading given the fbc frobbing. So maybe we should call it finish_pageflip_work or something like that ... -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/1] drm/i915: add generic power management initialization
This adds intel_pm routine for generic power-related infrastructure initialization. Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 125 +- drivers/gpu/drm/i915/intel_drv.h | 45 +-- drivers/gpu/drm/i915/intel_pm.c | 142 ++ 3 files changed, 145 insertions(+), 167 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6649301..b08bccd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6369,23 +6369,6 @@ static void intel_init_display(struct drm_device *dev) dev_priv-display.update_plane = i9xx_update_plane; } - if (I915_HAS_FBC(dev)) { - if (HAS_PCH_SPLIT(dev)) { - dev_priv-display.fbc_enabled = ironlake_fbc_enabled; - dev_priv-display.enable_fbc = ironlake_enable_fbc; - dev_priv-display.disable_fbc = ironlake_disable_fbc; - } else if (IS_GM45(dev)) { - dev_priv-display.fbc_enabled = g4x_fbc_enabled; - dev_priv-display.enable_fbc = g4x_enable_fbc; - dev_priv-display.disable_fbc = g4x_disable_fbc; - } else if (IS_CRESTLINE(dev)) { - dev_priv-display.fbc_enabled = i8xx_fbc_enabled; - dev_priv-display.enable_fbc = i8xx_enable_fbc; - dev_priv-display.disable_fbc = i8xx_disable_fbc; - } - /* 855GM needs testing */ - } - /* Returns the core display clock speed */ if (IS_VALLEYVIEW(dev)) dev_priv-display.get_display_clock_speed = @@ -6412,130 +6395,24 @@ static void intel_init_display(struct drm_device *dev) dev_priv-display.get_display_clock_speed = i830_get_display_clock_speed; - /* For FIFO watermark updates */ if (HAS_PCH_SPLIT(dev)) { - dev_priv-display.force_wake_get = __gen6_gt_force_wake_get; - dev_priv-display.force_wake_put = __gen6_gt_force_wake_put; - - /* IVB configs may use multi-threaded forcewake */ - if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) { - u32 ecobus; - - /* A small trick here - if the bios hasn't configured MT forcewake, -* and if the device is in RC6, then force_wake_mt_get will not wake -* the device and the ECOBUS read will return zero. Which will be -* (correctly) interpreted by the test below as MT forcewake being -* disabled. -*/ - mutex_lock(dev-struct_mutex); - __gen6_gt_force_wake_mt_get(dev_priv); - ecobus = I915_READ_NOTRACE(ECOBUS); - __gen6_gt_force_wake_mt_put(dev_priv); - mutex_unlock(dev-struct_mutex); - - if (ecobus FORCEWAKE_MT_ENABLE) { - DRM_DEBUG_KMS(Using MT version of forcewake\n); - dev_priv-display.force_wake_get = - __gen6_gt_force_wake_mt_get; - dev_priv-display.force_wake_put = - __gen6_gt_force_wake_mt_put; - } - } - - if (HAS_PCH_IBX(dev)) - dev_priv-display.init_pch_clock_gating = ibx_init_clock_gating; - else if (HAS_PCH_CPT(dev)) - dev_priv-display.init_pch_clock_gating = cpt_init_clock_gating; - if (IS_GEN5(dev)) { - if (I915_READ(MLTR_ILK) ILK_SRLT_MASK) - dev_priv-display.update_wm = ironlake_update_wm; - else { - DRM_DEBUG_KMS(Failed to get proper latency. - Disable CxSR\n); - dev_priv-display.update_wm = NULL; - } dev_priv-display.fdi_link_train = ironlake_fdi_link_train; - dev_priv-display.init_clock_gating = ironlake_init_clock_gating; dev_priv-display.write_eld = ironlake_write_eld; } else if (IS_GEN6(dev)) { - if (SNB_READ_WM0_LATENCY()) { - dev_priv-display.update_wm = sandybridge_update_wm; - dev_priv-display.update_sprite_wm = sandybridge_update_sprite_wm; - } else { - DRM_DEBUG_KMS(Failed to read display plane latency. -
[Intel-gfx] [PATCH] drm/i915: intel_update_fbc() requires struct_mutex, so no longer atomic
As we need to manipulate our device structure and allocate queue a task, it is no longer a simple atomic operation and cannot be performed along the atomic modeset paths. Instead make sure that we disable FBC (which must be therefore kept as a set of simple register writes) when performing the atomic modeset and leave the heavy-weight intel_update_fbc() for the normal modeset. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_display.c | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 39c13e3..1a0483f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2330,15 +2330,11 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, { struct drm_device *dev = crtc-dev; struct drm_i915_private *dev_priv = dev-dev_private; - int ret; - - ret = dev_priv-display.update_plane(crtc, fb, x, y); - if (ret) - return ret; - intel_update_fbc(dev); + if (dev_priv-display.disable_fbc) + dev_priv-display.disable_fbc(dev); - return 0; + return dev_priv-display.update_plane(crtc, fb, x, y); } static int @@ -2373,6 +2369,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) { struct drm_device *dev = crtc-dev; + struct drm_i915_private *dev_priv = dev-dev_private; struct drm_i915_master_private *master_priv; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int ret; @@ -2409,8 +2406,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, if (old_fb) intel_finish_fb(old_fb); - ret = intel_pipe_set_base_atomic(crtc, crtc-fb, x, y, -LEAVE_ATOMIC_MODE_SET); + ret = dev_priv-display.update_plane(crtc, crtc-fb, x, y); if (ret) { intel_unpin_fb_obj(to_intel_framebuffer(crtc-fb)-obj); mutex_unlock(dev-struct_mutex); @@ -2423,6 +2419,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y, intel_unpin_fb_obj(to_intel_framebuffer(old_fb)-obj); } + intel_update_fbc(dev); mutex_unlock(dev-struct_mutex); intel_increase_pllclock(crtc); -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Kill unused fence pipelining
Since we have never succeeded in making the pipelined fence updates stable and it appears we never will, delete the dead code. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/12] drm/i915: Remove the unsightly optimisation from flush_fence()
As i915_wait_request() will first check for an already passed seqno, doing it also in the caller is a waste of space for a cold path. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f7cd3461..bac3570 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2280,11 +2280,6 @@ static int i830_write_fence_reg(struct drm_i915_gem_object *obj) return 0; } -static bool ring_passed_seqno(struct intel_ring_buffer *ring, u32 seqno) -{ - return i915_seqno_passed(ring-get_seqno(ring), seqno); -} - static int i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) { @@ -2302,14 +2297,11 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) } if (obj-last_fenced_seqno) { - if (!ring_passed_seqno(obj-ring, - obj-last_fenced_seqno)) { - ret = i915_wait_request(obj-ring, - obj-last_fenced_seqno, - true); - if (ret) - return ret; - } + ret = i915_wait_request(obj-ring, + obj-last_fenced_seqno, + true); + if (ret) + return ret; obj-last_fenced_seqno = 0; } -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/12] drm/i915: Refactor put_fence() to use the common fence writing routine
One clarification that we make is to the existing semantics of obj-tiling_changed to only mean that we need to update an associated fence register (including the NO_FENCE when executing an untiled but fenced GPU command). If we do not have a fence register or pending fenced GPU access for the object (after put_fence() for example), then we can clear the tiling_changed flag as any fence will necessarily be rewritten upon acquisition. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 62 --- 1 file changed, 51 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 199306d..3601b8b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -50,10 +50,28 @@ static int i915_gem_phys_pwrite(struct drm_device *dev, struct drm_file *file); static void i915_gem_free_object_tail(struct drm_i915_gem_object *obj); +static void i915_gem_write_fence(struct drm_device *dev, int reg, +struct drm_i915_gem_object *obj); +static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj, +struct drm_i915_fence_reg *fence, +bool enable); + static int i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc); static void i915_gem_object_truncate(struct drm_i915_gem_object *obj); +static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj) +{ + if (obj-tiling_mode) + i915_gem_release_mmap(obj); + + /* As we do not have an associated fence register, we will force +* a tiling change if we ever need to acquire one. +*/ + obj-tiling_changed = false; + obj-fence_reg = I915_FENCE_REG_NONE; +} + /* some bookkeeping */ static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv, size_t size) @@ -2301,6 +2319,32 @@ static void i915_gem_write_fence(struct drm_device *dev, int reg, } } +static inline int fence_number(struct drm_i915_private *dev_priv, + struct drm_i915_fence_reg *fence) +{ + return fence - dev_priv-fence_regs; +} + +static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj, +struct drm_i915_fence_reg *fence, +bool enable) +{ + struct drm_i915_private *dev_priv = obj-base.dev-dev_private; + int reg = fence_number(dev_priv, fence); + + i915_gem_write_fence(obj-base.dev, reg, enable ? obj : NULL); + + if (enable) { + obj-fence_reg = reg; + fence-obj = obj; + list_move_tail(fence-lru_list, dev_priv-mm.fence_list); + } else { + obj-fence_reg = I915_FENCE_REG_NONE; + fence-obj = NULL; + list_del_init(fence-lru_list); + } +} + static int i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) { @@ -2339,24 +2383,20 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) int i915_gem_object_put_fence(struct drm_i915_gem_object *obj) { + struct drm_i915_private *dev_priv = obj-base.dev-dev_private; int ret; - if (obj-tiling_mode) - i915_gem_release_mmap(obj); - ret = i915_gem_object_flush_fence(obj); if (ret) return ret; - if (obj-fence_reg != I915_FENCE_REG_NONE) { - struct drm_i915_private *dev_priv = obj-base.dev-dev_private; - - WARN_ON(dev_priv-fence_regs[obj-fence_reg].pin_count); - i915_gem_clear_fence_reg(obj-base.dev, -dev_priv-fence_regs[obj-fence_reg]); + if (obj-fence_reg == I915_FENCE_REG_NONE) + return 0; - obj-fence_reg = I915_FENCE_REG_NONE; - } + i915_gem_object_update_fence(obj, +dev_priv-fence_regs[obj-fence_reg], +false); + i915_gem_object_fence_lost(obj); return 0; } -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/12] drm/i915: Clarify the semantics of tiling-changed by renaming it
Rename obj-tiling_changed to obj-fence_change so that it is clear that it flags when the parameters for an active fence (including the no-fence) register are changed. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_drv.h|7 ++- drivers/gpu/drm/i915/i915_gem.c|8 drivers/gpu/drm/i915/i915_gem_tiling.c |5 - 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8c32ada..3d7ad9b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -869,7 +869,12 @@ struct drm_i915_gem_object { * Current tiling mode for the object. */ unsigned int tiling_mode:2; - unsigned int tiling_changed:1; + /** +* Whether the tiling parameters for the currently associated fence +* register have changed. Note that for the purposes of tracking +* tiling changes we also treat the unfenced register as a fence. +*/ + unsigned int fence_changed:1; /** How many users have pinned this object in GTT space. The following * users can each hold at most one reference: pwrite/pread, pin_ioctl diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7bc4a40..5edab3f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -66,7 +66,7 @@ static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj) /* As we do not have an associated fence register, we will force * a tiling change if we ever need to acquire one. */ - obj-tiling_changed = false; + obj-fence_changed = false; obj-fence_reg = I915_FENCE_REG_NONE; } @@ -2456,7 +2456,7 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj) /* Have we updated the tiling parameters upon the object and so * will need to serialise the write to the associated fence register? */ - if (obj-tiling_changed) { + if (obj-fence_changed) { ret = i915_gem_object_flush_fence(obj); if (ret) return ret; @@ -2465,7 +2465,7 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj) /* Just update our place in the LRU if our fence is getting reused. */ if (obj-fence_reg != I915_FENCE_REG_NONE) { reg = dev_priv-fence_regs[obj-fence_reg]; - if (!obj-tiling_changed) { + if (!obj-fence_changed) { list_move_tail(reg-lru_list, dev_priv-mm.fence_list); return 0; @@ -2488,7 +2488,7 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj) return 0; i915_gem_object_update_fence(obj, reg, enable); - obj-tiling_changed = false; + obj-fence_changed = false; return 0; } diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index 1a93066..e51d892 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -374,7 +374,10 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, } if (ret == 0) { - obj-tiling_changed = true; + obj-fence_changed = + obj-fenced_gpu_access || + obj-fence_reg != I915_FENCE_REG_NONE; + obj-tiling_mode = args-tiling_mode; obj-stride = args-stride; } -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/12] drm/i915: Refactor fence clearing to use the common fence writing routine
Now that we have a routine that is able to clear the fences as well as setup up the register for a tiled object, remove the surplus routines to clear the fences. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 62 ++- 1 file changed, 9 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3601b8b..e09ac3a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -42,8 +42,6 @@ static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *o static __must_check int i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, unsigned alignment, bool map_and_fenceable); -static void i915_gem_clear_fence_reg(struct drm_device *dev, -struct drm_i915_fence_reg *reg); static int i915_gem_phys_pwrite(struct drm_device *dev, struct drm_i915_gem_object *obj, struct drm_i915_gem_pwrite *args, @@ -1655,19 +1653,18 @@ static void i915_gem_reset_fences(struct drm_device *dev) for (i = 0; i dev_priv-num_fence_regs; i++) { struct drm_i915_fence_reg *reg = dev_priv-fence_regs[i]; - struct drm_i915_gem_object *obj = reg-obj; - if (!obj) - continue; + i915_gem_write_fence(dev, i, NULL); - if (obj-tiling_mode) - i915_gem_release_mmap(obj); + if (reg-obj) + i915_gem_object_fence_lost(reg-obj); - reg-obj-fence_reg = I915_FENCE_REG_NONE; - reg-obj-fenced_gpu_access = false; - reg-obj-last_fenced_seqno = 0; - i915_gem_clear_fence_reg(dev, reg); + reg-pin_count = 0; + reg-obj = NULL; + INIT_LIST_HEAD(reg-lru_list); } + + INIT_LIST_HEAD(dev_priv-mm.fence_list); } void i915_gem_reset(struct drm_device *dev) @@ -2513,45 +2510,6 @@ update: } /** - * i915_gem_clear_fence_reg - clear out fence register info - * @obj: object to clear - * - * Zeroes out the fence register itself and clears out the associated - * data structures in dev_priv and obj. - */ -static void -i915_gem_clear_fence_reg(struct drm_device *dev, -struct drm_i915_fence_reg *reg) -{ - drm_i915_private_t *dev_priv = dev-dev_private; - uint32_t fence_reg = reg - dev_priv-fence_regs; - - switch (INTEL_INFO(dev)-gen) { - case 7: - case 6: - I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + fence_reg*8, 0); - break; - case 5: - case 4: - I915_WRITE64(FENCE_REG_965_0 + fence_reg*8, 0); - break; - case 3: - if (fence_reg = 8) - fence_reg = FENCE_REG_945_8 + (fence_reg - 8) * 4; - else - case 2: - fence_reg = FENCE_REG_830_0 + fence_reg * 4; - - I915_WRITE(fence_reg, 0); - break; - } - - list_del_init(reg-lru_list); - reg-obj = NULL; - reg-pin_count = 0; -} - -/** * Finds free space in the GTT aperture and binds the object there. */ static int @@ -3788,9 +3746,7 @@ i915_gem_load(struct drm_device *dev) dev_priv-num_fence_regs = 8; /* Initialize fence registers to zero */ - for (i = 0; i dev_priv-num_fence_regs; i++) { - i915_gem_clear_fence_reg(dev, dev_priv-fence_regs[i]); - } + i915_gem_reset_fences(dev); i915_gem_detect_bit_6_swizzle(dev); init_waitqueue_head(dev_priv-pending_flip_queue); -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/12] drm/i915: Prepare to consolidate fence writing
Update the existing architecture specific fence writing routines to either update the fence to point to a tiled object or to clear them in preparation to remove the other fence writing routes. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 211 --- 1 file changed, 108 insertions(+), 103 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index bac3570..199306d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2163,121 +2163,142 @@ int i915_gpu_idle(struct drm_device *dev, bool do_retire) return 0; } -static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj) +static void sandybridge_write_fence_reg(struct drm_device *dev, int reg, + struct drm_i915_gem_object *obj) { - struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; - u32 size = obj-gtt_space-size; - int regnum = obj-fence_reg; uint64_t val; - val = (uint64_t)((obj-gtt_offset + size - 4096) -0xf000) 32; - val |= obj-gtt_offset 0xf000; - val |= (uint64_t)((obj-stride / 128) - 1) - SANDYBRIDGE_FENCE_PITCH_SHIFT; + if (obj) { + u32 size = obj-gtt_space-size; - if (obj-tiling_mode == I915_TILING_Y) - val |= 1 I965_FENCE_TILING_Y_SHIFT; - val |= I965_FENCE_REG_VALID; + val = (uint64_t)((obj-gtt_offset + size - 4096) +0xf000) 32; + val |= obj-gtt_offset 0xf000; + val |= (uint64_t)((obj-stride / 128) - 1) + SANDYBRIDGE_FENCE_PITCH_SHIFT; - I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + regnum * 8, val); + if (obj-tiling_mode == I915_TILING_Y) + val |= 1 I965_FENCE_TILING_Y_SHIFT; + val |= I965_FENCE_REG_VALID; + } else + val = 0; - return 0; + I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + reg * 8, val); + POSTING_READ(FENCE_REG_SANDYBRIDGE_0 + reg * 8); } -static int i965_write_fence_reg(struct drm_i915_gem_object *obj) +static void i965_write_fence_reg(struct drm_device *dev, int reg, +struct drm_i915_gem_object *obj) { - struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; - u32 size = obj-gtt_space-size; - int regnum = obj-fence_reg; uint64_t val; - val = (uint64_t)((obj-gtt_offset + size - 4096) - 0xf000) 32; - val |= obj-gtt_offset 0xf000; - val |= ((obj-stride / 128) - 1) I965_FENCE_PITCH_SHIFT; - if (obj-tiling_mode == I915_TILING_Y) - val |= 1 I965_FENCE_TILING_Y_SHIFT; - val |= I965_FENCE_REG_VALID; + if (obj) { + u32 size = obj-gtt_space-size; - I915_WRITE64(FENCE_REG_965_0 + regnum * 8, val); + val = (uint64_t)((obj-gtt_offset + size - 4096) +0xf000) 32; + val |= obj-gtt_offset 0xf000; + val |= ((obj-stride / 128) - 1) I965_FENCE_PITCH_SHIFT; + if (obj-tiling_mode == I915_TILING_Y) + val |= 1 I965_FENCE_TILING_Y_SHIFT; + val |= I965_FENCE_REG_VALID; + } else + val = 0; - return 0; + I915_WRITE64(FENCE_REG_965_0 + reg * 8, val); + POSTING_READ(FENCE_REG_965_0 + reg * 8); } -static int i915_write_fence_reg(struct drm_i915_gem_object *obj) +static void i915_write_fence_reg(struct drm_device *dev, int reg, +struct drm_i915_gem_object *obj) { - struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; - u32 size = obj-gtt_space-size; - u32 fence_reg, val, pitch_val; - int tile_width; - - if (WARN((obj-gtt_offset ~I915_FENCE_START_MASK) || -(size -size) != size || -(obj-gtt_offset (size - 1)), -object 0x%08x [fenceable? %d] not 1M or pot-size (0x%08x) aligned\n, -obj-gtt_offset, obj-map_and_fenceable, size)) - return -EINVAL; + u32 val; - if (obj-tiling_mode == I915_TILING_Y HAS_128_BYTE_Y_TILING(dev)) - tile_width = 128; - else - tile_width = 512; - - /* Note: pitch better be a power of two tile widths */ - pitch_val = obj-stride / tile_width; - pitch_val = ffs(pitch_val) - 1; - - val = obj-gtt_offset; - if (obj-tiling_mode == I915_TILING_Y) - val |= 1 I830_FENCE_TILING_Y_SHIFT; - val |= I915_FENCE_SIZE_BITS(size); - val |= pitch_val I830_FENCE_PITCH_SHIFT; - val |= I830_FENCE_REG_VALID;
[Intel-gfx] [PATCH 05/12] drm/i915: Simplify fence finding
As the fences are stored in LRU order, we can simply reuse the oldest if we do not have an unused register. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 16 +++- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b25d229..f7cd3461 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2352,7 +2352,7 @@ static struct drm_i915_fence_reg * i915_find_fence_reg(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; - struct drm_i915_fence_reg *reg, *first, *avail; + struct drm_i915_fence_reg *reg, *avail; int i; /* First try to find a free reg */ @@ -2370,24 +2370,14 @@ i915_find_fence_reg(struct drm_device *dev) return NULL; /* None available, try to steal one or wait for a user to finish */ - avail = first = NULL; list_for_each_entry(reg, dev_priv-mm.fence_list, lru_list) { if (reg-pin_count) continue; - if (first == NULL) - first = reg; - - if (reg-obj-last_fenced_seqno == 0) { - avail = reg; - break; - } + return reg; } - if (avail == NULL) - avail = first; - - return avail; + return NULL; } /** -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/12] drm/i915: Only the zap the VMA after updating the tiling parameters
If we fail to unbind and so abort the change in tiling, we will have removed the VMA for the object for no reason. The likelihood of unbind failing is slim (other than ERESTARTSYS which will cause userspace to try again), so the change is mostly for the principle. Also improve the slightly stale comment. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem_tiling.c |8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index e51d892..6a2bd54 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -354,9 +354,10 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, /* We need to rebind the object if its current allocation * no longer meets the alignment restrictions for its new * tiling mode. Otherwise we can just leave it alone, but -* need to ensure that any fence register is cleared. +* need to ensure that any fence register is updated before +* the next fenced (either through the GTT or by the BLT unit +* on older GPUs) access. */ - i915_gem_release_mmap(obj); obj-map_and_fenceable = obj-gtt_space == NULL || @@ -380,6 +381,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, obj-tiling_mode = args-tiling_mode; obj-stride = args-stride; + + /* Force the fence to be reacquired for GTT access */ + i915_gem_release_mmap(obj); } } /* we have to maintain this existing ABI... */ -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] drm/i915: Refactor get_fence() to use the common fence writing routine
We can also take advantage of the new 'no retire' mode for seqno waiting to avoid having to take a reference on the old fence object whilst flushing an existing fence. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 70 +++ 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e09ac3a..7bc4a40 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2361,7 +2361,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) if (obj-last_fenced_seqno) { ret = i915_wait_request(obj-ring, obj-last_fenced_seqno, - true); + false); if (ret) return ret; @@ -2449,63 +2449,47 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj) { struct drm_device *dev = obj-base.dev; struct drm_i915_private *dev_priv = dev-dev_private; + bool enable = obj-tiling_mode != I915_TILING_NONE; struct drm_i915_fence_reg *reg; int ret; - if (obj-tiling_mode == I915_TILING_NONE) - return i915_gem_object_put_fence(obj); + /* Have we updated the tiling parameters upon the object and so +* will need to serialise the write to the associated fence register? +*/ + if (obj-tiling_changed) { + ret = i915_gem_object_flush_fence(obj); + if (ret) + return ret; + } /* Just update our place in the LRU if our fence is getting reused. */ if (obj-fence_reg != I915_FENCE_REG_NONE) { reg = dev_priv-fence_regs[obj-fence_reg]; - list_move_tail(reg-lru_list, dev_priv-mm.fence_list); + if (!obj-tiling_changed) { + list_move_tail(reg-lru_list, + dev_priv-mm.fence_list); + return 0; + } + } else if (enable) { + reg = i915_find_fence_reg(dev); + if (reg == NULL) + return -EDEADLK; + + if (reg-obj) { + struct drm_i915_gem_object *old = reg-obj; - if (obj-tiling_changed) { - ret = i915_gem_object_flush_fence(obj); + ret = i915_gem_object_flush_fence(old); if (ret) return ret; - goto update; + i915_gem_object_fence_lost(old); } - + } else return 0; - } - - reg = i915_find_fence_reg(dev); - if (reg == NULL) - return -EDEADLK; - - ret = i915_gem_object_flush_fence(obj); - if (ret) - return ret; - - if (reg-obj) { - struct drm_i915_gem_object *old = reg-obj; - drm_gem_object_reference(old-base); - - if (old-tiling_mode) - i915_gem_release_mmap(old); - - ret = i915_gem_object_flush_fence(old); - if (ret) { - drm_gem_object_unreference(old-base); - return ret; - } - - old-fence_reg = I915_FENCE_REG_NONE; - old-last_fenced_seqno = 0; - - drm_gem_object_unreference(old-base); - } - - reg-obj = obj; - list_move_tail(reg-lru_list, dev_priv-mm.fence_list); - obj-fence_reg = reg - dev_priv-fence_regs; - -update: + i915_gem_object_update_fence(obj, reg, enable); obj-tiling_changed = false; - i915_gem_write_fence(dev, reg - dev_priv-fence_regs, obj); + return 0; } -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] drm/i915: Discard the unused obj-last_fenced_ring
As we now never pipeline a fence update, obj-last_fenced_ring is always the same as the obj-ring whenever obj-last_fenced_seqno is active, so remove it. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_drv.h |5 ++--- drivers/gpu/drm/i915/i915_gem.c | 16 +--- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 735af61..8c32ada 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -930,13 +930,12 @@ struct drm_i915_gem_object { */ uint32_t gtt_offset; - /** Breadcrumb of last rendering to the buffer. */ - uint32_t last_rendering_seqno; struct intel_ring_buffer *ring; + /** Breadcrumb of last rendering to the buffer. */ + uint32_t last_rendering_seqno; /** Breadcrumb of last fenced GPU access to the buffer. */ uint32_t last_fenced_seqno; - struct intel_ring_buffer *last_fenced_ring; /** Current tiling stride for the object, if it's tiled. */ uint32_t stride; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3a091f5..b25d229 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1398,7 +1398,6 @@ i915_gem_object_move_to_active(struct drm_i915_gem_object *obj, if (obj-fenced_gpu_access) { obj-last_fenced_seqno = seqno; - obj-last_fenced_ring = ring; /* Bump MRU to take account of the delayed flush */ if (obj-fence_reg != I915_FENCE_REG_NONE) { @@ -1445,7 +1444,6 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) BUG_ON(!list_empty(obj-gpu_write_list)); BUG_ON(!obj-active); obj-ring = NULL; - obj-last_fenced_ring = NULL; i915_gem_object_move_off_active(obj); obj-fenced_gpu_access = false; @@ -1650,7 +1648,6 @@ static void i915_gem_reset_fences(struct drm_device *dev) reg-obj-fence_reg = I915_FENCE_REG_NONE; reg-obj-fenced_gpu_access = false; reg-obj-last_fenced_seqno = 0; - reg-obj-last_fenced_ring = NULL; i915_gem_clear_fence_reg(dev, reg); } } @@ -2295,7 +2292,7 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) if (obj-fenced_gpu_access) { if (obj-base.write_domain I915_GEM_GPU_DOMAINS) { - ret = i915_gem_flush_ring(obj-last_fenced_ring, + ret = i915_gem_flush_ring(obj-ring, 0, obj-base.write_domain); if (ret) return ret; @@ -2304,10 +2301,10 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) obj-fenced_gpu_access = false; } - if (obj-last_fenced_seqno NULL != obj-last_fenced_ring) { - if (!ring_passed_seqno(obj-last_fenced_ring, + if (obj-last_fenced_seqno) { + if (!ring_passed_seqno(obj-ring, obj-last_fenced_seqno)) { - ret = i915_wait_request(obj-last_fenced_ring, + ret = i915_wait_request(obj-ring, obj-last_fenced_seqno, true); if (ret) @@ -2315,7 +2312,6 @@ i915_gem_object_flush_fence(struct drm_i915_gem_object *obj) } obj-last_fenced_seqno = 0; - obj-last_fenced_ring = NULL; } /* Ensure that all CPU reads are completed before installing a fence @@ -2382,7 +2378,7 @@ i915_find_fence_reg(struct drm_device *dev) if (first == NULL) first = reg; - if (reg-obj-last_fenced_ring == NULL) { + if (reg-obj-last_fenced_seqno == 0) { avail = reg; break; } @@ -2458,7 +2454,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj) } old-fence_reg = I915_FENCE_REG_NONE; - old-last_fenced_ring = NULL; old-last_fenced_seqno = 0; drm_gem_object_unreference(old-base); @@ -2467,7 +2462,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj) reg-obj = obj; list_move_tail(reg-lru_list, dev_priv-mm.fence_list); obj-fence_reg = reg - dev_priv-fence_regs; - obj-last_fenced_ring = NULL; update: obj-tiling_changed = false; -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/12] drm/i915: Remove fence pipelining
Step 2 is then to replace the pipelined parameter with NULL and perform constant folding to remove dead code. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 155 +-- 1 file changed, 36 insertions(+), 119 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40e0808..5a9d90f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2166,8 +2166,7 @@ int i915_gpu_idle(struct drm_device *dev, bool do_retire) return 0; } -static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *pipelined) +static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj) { struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; @@ -2185,26 +2184,12 @@ static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj, val |= 1 I965_FENCE_TILING_Y_SHIFT; val |= I965_FENCE_REG_VALID; - if (pipelined) { - int ret = intel_ring_begin(pipelined, 6); - if (ret) - return ret; - - intel_ring_emit(pipelined, MI_NOOP); - intel_ring_emit(pipelined, MI_LOAD_REGISTER_IMM(2)); - intel_ring_emit(pipelined, FENCE_REG_SANDYBRIDGE_0 + regnum*8); - intel_ring_emit(pipelined, (u32)val); - intel_ring_emit(pipelined, FENCE_REG_SANDYBRIDGE_0 + regnum*8 + 4); - intel_ring_emit(pipelined, (u32)(val 32)); - intel_ring_advance(pipelined); - } else - I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + regnum * 8, val); + I915_WRITE64(FENCE_REG_SANDYBRIDGE_0 + regnum * 8, val); return 0; } -static int i965_write_fence_reg(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *pipelined) +static int i965_write_fence_reg(struct drm_i915_gem_object *obj) { struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; @@ -2220,26 +2205,12 @@ static int i965_write_fence_reg(struct drm_i915_gem_object *obj, val |= 1 I965_FENCE_TILING_Y_SHIFT; val |= I965_FENCE_REG_VALID; - if (pipelined) { - int ret = intel_ring_begin(pipelined, 6); - if (ret) - return ret; - - intel_ring_emit(pipelined, MI_NOOP); - intel_ring_emit(pipelined, MI_LOAD_REGISTER_IMM(2)); - intel_ring_emit(pipelined, FENCE_REG_965_0 + regnum*8); - intel_ring_emit(pipelined, (u32)val); - intel_ring_emit(pipelined, FENCE_REG_965_0 + regnum*8 + 4); - intel_ring_emit(pipelined, (u32)(val 32)); - intel_ring_advance(pipelined); - } else - I915_WRITE64(FENCE_REG_965_0 + regnum * 8, val); + I915_WRITE64(FENCE_REG_965_0 + regnum * 8, val); return 0; } -static int i915_write_fence_reg(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *pipelined) +static int i915_write_fence_reg(struct drm_i915_gem_object *obj) { struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; @@ -2276,24 +2247,12 @@ static int i915_write_fence_reg(struct drm_i915_gem_object *obj, else fence_reg = FENCE_REG_945_8 + (fence_reg - 8) * 4; - if (pipelined) { - int ret = intel_ring_begin(pipelined, 4); - if (ret) - return ret; - - intel_ring_emit(pipelined, MI_NOOP); - intel_ring_emit(pipelined, MI_LOAD_REGISTER_IMM(1)); - intel_ring_emit(pipelined, fence_reg); - intel_ring_emit(pipelined, val); - intel_ring_advance(pipelined); - } else - I915_WRITE(fence_reg, val); + I915_WRITE(fence_reg, val); return 0; } -static int i830_write_fence_reg(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *pipelined) +static int i830_write_fence_reg(struct drm_i915_gem_object *obj) { struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; @@ -2319,18 +2278,7 @@ static int i830_write_fence_reg(struct drm_i915_gem_object *obj, val |= pitch_val I830_FENCE_PITCH_SHIFT; val |= I830_FENCE_REG_VALID; - if (pipelined) { - int ret = intel_ring_begin(pipelined, 4); - if (ret) - return ret; - - intel_ring_emit(pipelined, MI_NOOP); - intel_ring_emit(pipelined, MI_LOAD_REGISTER_IMM(1)); - intel_ring_emit(pipelined, FENCE_REG_830_0 + regnum*4); - intel_ring_emit(pipelined, val);
[Intel-gfx] [PATCH 01/12] drm/i915: Remove the pipelined parameter from get_fence()
We never succeeded in getting pipelined fencing to work (unresolved spurious GPU hangs), so begin the process of dismantling and removal the broken code. Step 1 is the removal of the pipeline parameter to get_fence(). Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_drv.h|3 +-- drivers/gpu/drm/i915/i915_gem.c|7 +++ drivers/gpu/drm/i915/i915_gem_execbuffer.c |2 +- drivers/gpu/drm/i915/intel_display.c |2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 422f424..e99a02a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1255,8 +1255,7 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2) u32 i915_gem_next_request_seqno(struct intel_ring_buffer *ring); -int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *pipelined); +int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj); int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj); static inline bool diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index aa44ff2..40e0808 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1079,7 +1079,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) if (!obj-has_global_gtt_mapping) i915_gem_gtt_bind_object(obj, obj-cache_level); - ret = i915_gem_object_get_fence(obj, NULL); + ret = i915_gem_object_get_fence(obj); if (ret) goto unlock; @@ -2453,7 +2453,6 @@ i915_find_fence_reg(struct drm_device *dev, /** * i915_gem_object_get_fence - set up fencing for an object * @obj: object to map through a fence reg - * @pipelined: ring on which to queue the change, or NULL for CPU access * * When mapping objects through the GTT, userspace wants to be able to write * to them without having to worry about swizzling if the object is tiled. @@ -2466,11 +2465,11 @@ i915_find_fence_reg(struct drm_device *dev, * For an untiled surface, this removes any existing fence. */ int -i915_gem_object_get_fence(struct drm_i915_gem_object *obj, - struct intel_ring_buffer *pipelined) +i915_gem_object_get_fence(struct drm_i915_gem_object *obj) { struct drm_device *dev = obj-base.dev; struct drm_i915_private *dev_priv = dev-dev_private; + struct intel_ring_buffer *pipelined; struct drm_i915_fence_reg *reg; int ret; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 2a24d0c..b5262fd 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -530,7 +530,7 @@ pin_and_fence_object(struct drm_i915_gem_object *obj, if (has_fenced_gpu_access) { if (entry-flags EXEC_OBJECT_NEEDS_FENCE) { - ret = i915_gem_object_get_fence(obj, ring); + ret = i915_gem_object_get_fence(obj); if (ret) goto err_unpin; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7506a72..95d7713 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2151,7 +2151,7 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev, * framebuffer compression. For simplicity, we always install * a fence as the cost is not that onerous. */ - ret = i915_gem_object_get_fence(obj, pipelined); + ret = i915_gem_object_get_fence(obj); if (ret) goto err_unpin; -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/12] drm/edid: Update range descriptor struct for EDID 1.4
At Fri, 13 Apr 2012 16:33:37 -0400, Adam Jackson wrote: Signed-off-by: Adam Jackson a...@redhat.com --- include/drm/drm_edid.h | 26 -- 1 files changed, 20 insertions(+), 6 deletions(-) diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index bcb9a66..8cefbbe 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -90,12 +90,26 @@ struct detailed_data_monitor_range { u8 min_hfreq_khz; u8 max_hfreq_khz; u8 pixel_clock_mhz; /* need to multiply by 10 */ - __le16 sec_gtf_toggle; /* A000=use above, 20=use below */ - u8 hfreq_start_khz; /* need to multiply by 2 */ - u8 c; /* need to divide by 2 */ - __le16 m; - u8 k; - u8 j; /* need to divide by 2 */ + u8 flags; + union { + struct { + u8 reserved; + u8 hfreq_start_khz; /* need to multiply by 2 */ + u8 c; /* need to divide by 2 */ + __le16 m; + u8 k; + u8 j; /* need to divide by 2 */ + } gtf2; + struct { + u8 version; + u8 data1; /* high 6 bits: extra clock resolution */ + u8 data2; /* plus low 2 of above: max hactive */ + u8 supported_aspects; + u8 flags; /* preferred aspect and blanking support */ + u8 supported_scalings; + u8 preferred_refresh; + } cvt; These new structs must be marked with __attribute__((packed)) although the struct detailed_data_monitor_range itself is already marked. At least, with gcc 4.6 and x86-64 here, they get unaligned. thanks, Takashi + } formula; } __attribute__((packed)); struct detailed_data_wpindex { -- 1.7.7.6 ___ 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 00/12] Add more DMT and common modes
At Fri, 13 Apr 2012 16:56:26 -0400, Adam Jackson wrote: On 4/13/12 4:33 PM, Adam Jackson wrote: Incorporates some feedback from Rodrigo and Takashi. Major themes of the series: - Fix the DMT list to include reduced-blanking modes - Add modes from DMT for any range descriptor type - Add an extra modes list for things not in DMT - For ranges that specify a formula, generate timings from the extra modes This still doesn't address the driver policy decision of I know I have a scaler, add modes as if there were a range descriptor even if there's not one. But it should at least make clear what such a helper function should do. One minor buglet in this series that's not obvious from inspection (and that I didn't realize until just now) is that putting 1366x768 in the minimode list won't do what you want, since the mode you get back will be 1368x768. Probably we should move the manual-underscan hack into the timing generators themselves. Sounds like a good compromise. We have already hacks in drm_edid.c for HDMI TV, so one exception more isn't that bad ;) Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes
At Fri, 13 Apr 2012 16:33:28 -0400, Adam Jackson wrote: Incorporates some feedback from Rodrigo and Takashi. Major themes of the series: - Fix the DMT list to include reduced-blanking modes - Add modes from DMT for any range descriptor type - Add an extra modes list for things not in DMT - For ranges that specify a formula, generate timings from the extra modes This still doesn't address the driver policy decision of I know I have a scaler, add modes as if there were a range descriptor even if there's not one. But it should at least make clear what such a helper function should do. I tried these patches now with a few monitors here and it looks working well (after fixing the alignment as I posted in another mail). I got new 1600x900 output on two monitors. Yay! I guess this will solve major of problems we've face for HD+ panel. The rest is the 1366x768 mode, but it's often covered by 1360x768. So, once when LVDS gets additional de facto standard modes, it'll be fixed as well. thanks, Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Replace open coded MI_BATCH_GTT
The (26) virtual memory space selector harks back to gen3 and is mandatory given our use of GTT space for batchbuffers. On gen4+, use of the GTT became mandatory and bit6 marked reserved. However the code must now explicitly set (17), which conveniently is also (26). To clarify the meaning for future readers, replace the open coded (26) with MI_BATCH_GTT. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_reg.h |1 + drivers/gpu/drm/i915/intel_ringbuffer.c |5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b5f937a..f617437 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -231,6 +231,7 @@ #define MI_BATCH_NON_SECURE (1) #define MI_BATCH_NON_SECURE_I965 (18) #define MI_BATCH_BUFFER_START MI_INSTR(0x31, 0) +#define MI_BATCH_GTT (26) /* aliased with (17) on gen4 */ #define MI_SEMAPHORE_MBOX MI_INSTR(0x16, 1) /* gen6+ */ #define MI_SEMAPHORE_GLOBAL_GTT(122) #define MI_SEMAPHORE_UPDATE (121) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 90eddd3..7270d06 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -817,7 +817,8 @@ i965_dispatch_execbuffer(struct intel_ring_buffer *ring, u32 offset, u32 length) return ret; intel_ring_emit(ring, - MI_BATCH_BUFFER_START | (2 6) | + MI_BATCH_BUFFER_START | + MI_BATCH_GTT | MI_BATCH_NON_SECURE_I965); intel_ring_emit(ring, offset); intel_ring_advance(ring); @@ -854,7 +855,7 @@ i915_dispatch_execbuffer(struct intel_ring_buffer *ring, if (ret) return ret; - intel_ring_emit(ring, MI_BATCH_BUFFER_START | (2 6)); + intel_ring_emit(ring, MI_BATCH_BUFFER_START | MI_BATCH_GTT); intel_ring_emit(ring, offset | MI_BATCH_NON_SECURE); intel_ring_advance(ring); -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: intel_update_fbc() requires struct_mutex, so no longer atomic
On Tue, 17 Apr 2012 15:08:19 +0100 Chris Wilson ch...@chris-wilson.co.uk wrote: As we need to manipulate our device structure and allocate queue a task, it is no longer a simple atomic operation and cannot be performed along the atomic modeset paths. Instead make sure that we disable FBC (which must be therefore kept as a set of simple register writes) when performing the atomic modeset and leave the heavy-weight intel_update_fbc() for the normal modeset. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Jesse Barnes jbar...@virtuousgeek.org --- Looks good. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes
At Tue, 17 Apr 2012 17:33:17 +0200, Takashi Iwai wrote: At Fri, 13 Apr 2012 16:33:28 -0400, Adam Jackson wrote: Incorporates some feedback from Rodrigo and Takashi. Major themes of the series: - Fix the DMT list to include reduced-blanking modes - Add modes from DMT for any range descriptor type - Add an extra modes list for things not in DMT - For ranges that specify a formula, generate timings from the extra modes This still doesn't address the driver policy decision of I know I have a scaler, add modes as if there were a range descriptor even if there's not one. But it should at least make clear what such a helper function should do. I tried these patches now with a few monitors here and it looks working well (after fixing the alignment as I posted in another mail). I got new 1600x900 output on two monitors. Yay! Oh, feel free to add my tested-by tag: Tested-by: Takashi Iwai ti...@suse.de thanks, Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Mask reserved bits in display/sprite address registers
On Mon, 16 Apr 2012 21:02:41 + Reese, Armin C armin.c.re...@intel.com wrote: Corrected my name in the patch (acreese - Armin Reese) ... The attached patch file was updated to reflect reviewer's comments. The only change I did not make was using PAGE_MASK instead of DISP_BASEADDR_MASK. PAGE_MASK is CPU architecture dependent and I didn't want to tie our GPU addressing arch to that of the CPU. Looks good, thanks Armin. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Add more DMT and common modes
Thanks for the patches ajax Feel free to use Reviewed-by: Rodrigo Vivi rodrigo.v...@intel.com On Tue, Apr 17, 2012 at 12:50 PM, Takashi Iwai ti...@suse.de wrote: At Tue, 17 Apr 2012 17:33:17 +0200, Takashi Iwai wrote: At Fri, 13 Apr 2012 16:33:28 -0400, Adam Jackson wrote: Incorporates some feedback from Rodrigo and Takashi. Major themes of the series: - Fix the DMT list to include reduced-blanking modes - Add modes from DMT for any range descriptor type - Add an extra modes list for things not in DMT - For ranges that specify a formula, generate timings from the extra modes This still doesn't address the driver policy decision of I know I have a scaler, add modes as if there were a range descriptor even if there's not one. But it should at least make clear what such a helper function should do. I tried these patches now with a few monitors here and it looks working well (after fixing the alignment as I posted in another mail). I got new 1600x900 output on two monitors. Yay! Oh, feel free to add my tested-by tag: Tested-by: Takashi Iwai ti...@suse.de thanks, Takashi ___ dri-devel mailing list dri-de...@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Rodrigo Vivi Blog: http://blog.vivi.eng.br GPG: 0x905BE242 @ wwwkeys.pgp.net ___ 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: split power-related items into intel_pm module
On Mon, 16 Apr 2012 20:06:01 -0300 Eugeni Dodonov eugeni.dodo...@intel.com wrote: As previously discussed on irc with Daniel, Ben and Jesse, This patch moves the power-related functionality into intel_pm module, aiming at simplifying the intel_display code and make it less cluttered. The functionality affected by this move are: clock gating, rc6 and rps, display watermarks, display fifo-related items, cxsr and FBC. This simplifies intel_display by +/- 2800 lines of code and centralizes most of the power-related items in one place to simplify future hardware enablement and subsystems interaction. CC: Jesse Barnes jbar...@virtuousgeek.org CC: Ben Widawsky b...@bwidawsk.net CC: Daniel Vetter daniel.vet...@ffwll.ch CC: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com Acked-by: Jesse Barnes jbar...@virtuousgeek.org -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/12] drm/edid: Update range descriptor struct for EDID 1.4
On Tue, 2012-04-17 at 17:25 +0200, Takashi Iwai wrote: At Fri, 13 Apr 2012 16:33:37 -0400, Adam Jackson wrote: + u8 flags; + union { + struct { + u8 reserved; + u8 hfreq_start_khz; /* need to multiply by 2 */ + u8 c; /* need to divide by 2 */ + __le16 m; + u8 k; + u8 j; /* need to divide by 2 */ + } gtf2; + struct { + u8 version; + u8 data1; /* high 6 bits: extra clock resolution */ + u8 data2; /* plus low 2 of above: max hactive */ + u8 supported_aspects; + u8 flags; /* preferred aspect and blanking support */ + u8 supported_scalings; + u8 preferred_refresh; + } cvt; These new structs must be marked with __attribute__((packed)) although the struct detailed_data_monitor_range itself is already marked. At least, with gcc 4.6 and x86-64 here, they get unaligned. Eek. Good catch, thanks. - ajax signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Unpin the flip target if we fail to queue the flip
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_display.c | 50 -- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 85f1814..b2c29a5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7761,14 +7761,14 @@ static int intel_gen2_queue_flip(struct drm_device *dev, ret = intel_pin_and_fence_fb_obj(dev, obj, LP_RING(dev_priv)); if (ret) - goto out; + goto err; /* Offset into the new buffer for cases of shared fbs between CRTCs */ offset = crtc-y * fb-pitches[0] + crtc-x * fb-bits_per_pixel/8; ret = BEGIN_LP_RING(6); if (ret) - goto out; + goto err_unpin; /* Can't queue multiple flips, so wait for the previous * one to finish before executing the next. @@ -7785,7 +7785,11 @@ static int intel_gen2_queue_flip(struct drm_device *dev, OUT_RING(obj-gtt_offset + offset); OUT_RING(0); /* aux display base address, unused */ ADVANCE_LP_RING(); -out: + return 0; + +err_unpin: + intel_unpin_fb_obj(obj); +err: return ret; } @@ -7802,14 +7806,14 @@ static int intel_gen3_queue_flip(struct drm_device *dev, ret = intel_pin_and_fence_fb_obj(dev, obj, LP_RING(dev_priv)); if (ret) - goto out; + goto err; /* Offset into the new buffer for cases of shared fbs between CRTCs */ offset = crtc-y * fb-pitches[0] + crtc-x * fb-bits_per_pixel/8; ret = BEGIN_LP_RING(6); if (ret) - goto out; + goto err_unpin; if (intel_crtc-plane) flip_mask = MI_WAIT_FOR_PLANE_B_FLIP; @@ -7824,7 +7828,11 @@ static int intel_gen3_queue_flip(struct drm_device *dev, OUT_RING(MI_NOOP); ADVANCE_LP_RING(); -out: + return 0; + +err_unpin: + intel_unpin_fb_obj(obj); +err: return ret; } @@ -7840,11 +7848,11 @@ static int intel_gen4_queue_flip(struct drm_device *dev, ret = intel_pin_and_fence_fb_obj(dev, obj, LP_RING(dev_priv)); if (ret) - goto out; + goto err; ret = BEGIN_LP_RING(4); if (ret) - goto out; + goto err_unpin; /* i965+ uses the linear or tiled offsets from the * Display Registers (which do not change across a page-flip) @@ -7863,7 +7871,11 @@ static int intel_gen4_queue_flip(struct drm_device *dev, pipesrc = I915_READ(PIPESRC(intel_crtc-pipe)) 0x0fff0fff; OUT_RING(pf | pipesrc); ADVANCE_LP_RING(); -out: + return 0; + +err_unpin: + intel_unpin_fb_obj(obj); +err: return ret; } @@ -7879,11 +7891,11 @@ static int intel_gen6_queue_flip(struct drm_device *dev, ret = intel_pin_and_fence_fb_obj(dev, obj, LP_RING(dev_priv)); if (ret) - goto out; + goto err; ret = BEGIN_LP_RING(4); if (ret) - goto out; + goto err_unpin; OUT_RING(MI_DISPLAY_FLIP | MI_DISPLAY_FLIP_PLANE(intel_crtc-plane)); @@ -7894,7 +7906,11 @@ static int intel_gen6_queue_flip(struct drm_device *dev, pipesrc = I915_READ(PIPESRC(intel_crtc-pipe)) 0x0fff0fff; OUT_RING(pf | pipesrc); ADVANCE_LP_RING(); -out: + return 0; + +err_unpin: + intel_unpin_fb_obj(obj); +err: return ret; } @@ -7920,18 +7936,22 @@ static int intel_gen7_queue_flip(struct drm_device *dev, ret = intel_pin_and_fence_fb_obj(dev, obj, ring); if (ret) - goto out; + goto err; ret = intel_ring_begin(ring, 4); if (ret) - goto out; + goto err_unpin; intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | (intel_crtc-plane 19)); intel_ring_emit(ring, (fb-pitches[0] | obj-tiling_mode)); intel_ring_emit(ring, (obj-gtt_offset)); intel_ring_emit(ring, (MI_NOOP)); intel_ring_advance(ring); -out: + return 0; + +err_unpin: + intel_unpin_fb_obj(obj); +err: return ret; } -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] TIMESTAMP register
Hi, Starting from IVB the main TIMESTAMP register is longer than 32 bits (it’s 36 bits long). How should we pass its 36 bit value from i915 to user space? GET_PARAM ioctls supports only 32 bit params. Should we add GET_PARAM64 or create separate ioctl to access the TIMESTAMP? Any suggestions? This functionality is required for any OpenGL driver which implements GL_ARB_timer_query. Regards, Jacek smime.p7s Description: S/MIME cryptographic signature - Intel Technology Poland sp. z o.o. z siedziba w Gdansku ul. Slowackiego 173 80-298 Gdansk Sad Rejonowy Gdansk Polnoc w Gdansku, VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, numer KRS 101882 NIP 957-07-52-316 Kapital zakladowy 200.000 zl This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Do not set Enable Panel Fitter on SNB pageflips
Not only do the pageflip work without it at non-native modes (i.e. with the panel fitter enabled), it also causes normal (non-pageflipped) modesets to fail. Reported-by: Adam Jackson a...@redhat.com Cc: Adam Jackson a...@redhat.com Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_display.c |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b2c29a5..2319838 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7902,7 +7902,13 @@ static int intel_gen6_queue_flip(struct drm_device *dev, OUT_RING(fb-pitches[0] | obj-tiling_mode); OUT_RING(obj-gtt_offset); - pf = I915_READ(PF_CTL(intel_crtc-pipe)) PF_ENABLE; + /* Contrary to the suggestions in the documentation, +* Enable Panel Fitter does not seem to be required when page +* flipping with a non-native mode, and worse causes a normal +* modeset to fail. +* pf = I915_READ(PF_CTL(intel_crtc-pipe)) PF_ENABLE; +*/ + pf = 0; pipesrc = I915_READ(PIPESRC(intel_crtc-pipe)) 0x0fff0fff; OUT_RING(pf | pipesrc); ADVANCE_LP_RING(); -- 1.7.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] TIMESTAMP register
On Tue, Apr 17, 2012 at 07:12:40PM +, Lawrynowicz, Jacek wrote: Starting from IVB the main TIMESTAMP register is longer than 32 bits (it’s 36 bits long). How should we pass its 36 bit value from i915 to user space? GET_PARAM ioctls supports only 32 bit params. Should we add GET_PARAM64 or create separate ioctl to access the TIMESTAMP? Any suggestions? This functionality is required for any OpenGL driver which implements GL_ARB_timer_query. Like occlusion queries I presume this should be done with a pipe control qword write issued from a batch. GETPARAM makes absolutely no sense, that is for driver features, not hw features and just used so that a new driver also can work on an older kernel. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] TIMESTAMP register
ARB_timer_query allows client read TIMESTAMP both asynchronously and synchronously. The former can be implemented as you said but the latter requires support from the KMD. This must be a simple MMIO read as this is the only way to report current GPU time. Implementing synchronous TIMESTAMP query using pipe control would render the third example from ARB_timer_query spec useless. Regards, Jacek -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, April 17, 2012 9:40 PM To: Lawrynowicz, Jacek Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] TIMESTAMP register On Tue, Apr 17, 2012 at 07:12:40PM +, Lawrynowicz, Jacek wrote: Starting from IVB the main TIMESTAMP register is longer than 32 bits (it’s 36 bits long). How should we pass its 36 bit value from i915 to user space? GET_PARAM ioctls supports only 32 bit params. Should we add GET_PARAM64 or create separate ioctl to access the TIMESTAMP? Any suggestions? This functionality is required for any OpenGL driver which implements GL_ARB_timer_query. Like occlusion queries I presume this should be done with a pipe control qword write issued from a batch. GETPARAM makes absolutely no sense, that is for driver features, not hw features and just used so that a new driver also can work on an older kernel. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 smime.p7s Description: S/MIME cryptographic signature - Intel Technology Poland sp. z o.o. z siedziba w Gdansku ul. Slowackiego 173 80-298 Gdansk Sad Rejonowy Gdansk Polnoc w Gdansku, VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, numer KRS 101882 NIP 957-07-52-316 Kapital zakladowy 200.000 zl This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] TIMESTAMP register
On Tue, Apr 17, 2012 at 08:34:11PM +, Lawrynowicz, Jacek wrote: ARB_timer_query allows client read TIMESTAMP both asynchronously and synchronously. The former can be implemented as you said but the latter requires support from the KMD. This must be a simple MMIO read as this is the only way to report current GPU time. Implementing synchronous TIMESTAMP query using pipe control would render the third example from ARB_timer_query spec useless. Ok, I've looked like a dofus again, but now I've read the spec and we indeed seem to need a synchronous readout of the TIMESTAMP register. I guess a new register will do, together with some fixed-point integer that tells userspace how to convert it to nanoseconds. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do not set Enable Panel Fitter on SNB pageflips
On Tue, 2012-04-17 at 20:37 +0100, Chris Wilson wrote: Not only do the pageflip work without it at non-native modes (i.e. with the panel fitter enabled), it also causes normal (non-pageflipped) modesets to fail. Testcase: enable a GLX compositor like gnome-shell, resize X to something other than LVDS native size, vt switch to text console, frown at black screen of doom. Fixes the above testcase on a Lenovo X220. Tested-by: Adam Jackson a...@redhat.com - ajax signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do not set Enable Panel Fitter on SNB pageflips
On Tue, Apr 17, 2012 at 05:19:45PM -0400, Adam Jackson wrote: On Tue, 2012-04-17 at 20:37 +0100, Chris Wilson wrote: Not only do the pageflip work without it at non-native modes (i.e. with the panel fitter enabled), it also causes normal (non-pageflipped) modesets to fail. Testcase: enable a GLX compositor like gnome-shell, resize X to something other than LVDS native size, vt switch to text console, frown at black screen of doom. Fixes the above testcase on a Lenovo X220. Tested-by: Adam Jackson a...@redhat.com Queued for -next, thanks for the patch and testing. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ 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: IBX+ doesn't have separate vsync/hsync controls on the VGA DAC
Like this? From fee9f6fbf2230f96b9195baf32fd67b243969a14 Mon Sep 17 00:00:00 2001 From: Jesse Barnes jbar...@virtuousgeek.org Date: Wed, 11 Apr 2012 09:23:34 -0700 Subject: [PATCH] drm/i915: IBX+ doesn't have separate vsync/hsync controls on the VGA DAC When the PCH split occurred, we dropped support for separate hsync and vsync disable in the VGA DAC. So add a PCH specific DPMS function that just uses the port enable bit for controlling DPMS states. Before this fix, when anything other than a full DPMS off occurred, the VGA port would be left enabled and scanning out while all the other heads would turn off as expected. v2: duplicate encoder helper vtable into pch and gmch versions (Daniel) Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org --- drivers/gpu/drm/i915/intel_crt.c | 54 ++--- 1 files changed, 43 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 70b0f1a..fb516e9 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -55,18 +55,36 @@ static struct intel_crt *intel_attached_crt(struct drm_connector *connector) struct intel_crt, base); } -static void intel_crt_dpms(struct drm_encoder *encoder, int mode) +static void pch_crt_dpms(struct drm_encoder *encoder, int mode) { struct drm_device *dev = encoder-dev; struct drm_i915_private *dev_priv = dev-dev_private; - u32 temp, reg; + u32 temp; - if (HAS_PCH_SPLIT(dev)) - reg = PCH_ADPA; - else - reg = ADPA; + temp = I915_READ(PCH_ADPA); + temp = ~ADPA_DAC_ENABLE; + + switch (mode) { + case DRM_MODE_DPMS_ON: + temp |= ADPA_DAC_ENABLE; + break; + case DRM_MODE_DPMS_STANDBY: + case DRM_MODE_DPMS_SUSPEND: + case DRM_MODE_DPMS_OFF: + /* Just leave port enable cleared */ + break; + } + + I915_WRITE(PCH_ADPA, temp); +} - temp = I915_READ(reg); +static void intel_crt_dpms(struct drm_encoder *encoder, int mode) +{ + struct drm_device *dev = encoder-dev; + struct drm_i915_private *dev_priv = dev-dev_private; + u32 temp; + + temp = I915_READ(ADPA); temp = ~(ADPA_HSYNC_CNTL_DISABLE | ADPA_VSYNC_CNTL_DISABLE); temp = ~ADPA_DAC_ENABLE; @@ -85,7 +103,7 @@ static void intel_crt_dpms(struct drm_encoder *encoder, int mode) break; } - I915_WRITE(reg, temp); + I915_WRITE(ADPA, temp); } static int intel_crt_mode_valid(struct drm_connector *connector, @@ -516,12 +534,20 @@ static void intel_crt_reset(struct drm_connector *connector) * Routines for controlling stuff on the analog port */ -static const struct drm_encoder_helper_funcs intel_crt_helper_funcs = { - .dpms = intel_crt_dpms, +static const struct drm_encoder_helper_funcs pch_encoder_funcs = { + .mode_fixup = intel_crt_mode_fixup, + .prepare = intel_encoder_prepare, + .commit = intel_encoder_commit, + .mode_set = intel_crt_mode_set, + .dpms = pch_crt_dpms, +}; + +static const struct drm_encoder_helper_funcs gmch_encoder_funcs = { .mode_fixup = intel_crt_mode_fixup, .prepare = intel_encoder_prepare, .commit = intel_encoder_commit, .mode_set = intel_crt_mode_set, + .dpms = intel_crt_dpms, }; static const struct drm_connector_funcs intel_crt_connector_funcs = { @@ -567,6 +593,7 @@ void intel_crt_init(struct drm_device *dev) struct intel_crt *crt; struct intel_connector *intel_connector; struct drm_i915_private *dev_priv = dev-dev_private; + const struct drm_encoder_helper_funcs *encoder_helper_funcs; /* Skip machines without VGA that falsely report hotplug events */ if (dmi_check_system(intel_no_crt)) @@ -602,7 +629,12 @@ void intel_crt_init(struct drm_device *dev) connector-interlace_allowed = 1; connector-doublescan_allowed = 0; - drm_encoder_helper_add(crt-base.base, intel_crt_helper_funcs); + if (HAS_PCH_SPLIT(dev)) + encoder_helper_funcs = pch_encoder_funcs; + else + encoder_helper_funcs = gmch_encoder_funcs; + + drm_encoder_helper_add(crt-base.base, encoder_helper_funcs); drm_connector_helper_add(connector, intel_crt_connector_helper_funcs); drm_sysfs_connector_add(connector); -- 1.7.5.4 ___ 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: IBX+ doesn't have separate vsync/hsync controls on the VGA DAC
On Tue, 17 Apr 2012 15:06:33 -0700, Jesse Barnes jbar...@virtuousgeek.org wrote: Like this? From fee9f6fbf2230f96b9195baf32fd67b243969a14 Mon Sep 17 00:00:00 2001 From: Jesse Barnes jbar...@virtuousgeek.org Date: Wed, 11 Apr 2012 09:23:34 -0700 Subject: [PATCH] drm/i915: IBX+ doesn't have separate vsync/hsync controls on the VGA DAC When the PCH split occurred, we dropped support for separate hsync and vsync disable in the VGA DAC. So add a PCH specific DPMS function that just uses the port enable bit for controlling DPMS states. Before this fix, when anything other than a full DPMS off occurred, the VGA port would be left enabled and scanning out while all the other heads would turn off as expected. v2: duplicate encoder helper vtable into pch and gmch versions (Daniel) Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org With s/intel_crt_dpms/gmch_crt_dpms/ and Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48491 Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -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] TIMESTAMP register
On Tue, 17 Apr 2012 23:04:18 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 17, 2012 at 08:34:11PM +, Lawrynowicz, Jacek wrote: ARB_timer_query allows client read TIMESTAMP both asynchronously and synchronously. The former can be implemented as you said but the latter requires support from the KMD. This must be a simple MMIO read as this is the only way to report current GPU time. Implementing synchronous TIMESTAMP query using pipe control would render the third example from ARB_timer_query spec useless. Ok, I've looked like a dofus again, but now I've read the spec and we indeed seem to need a synchronous readout of the TIMESTAMP register. I guess a new register will do, together with some fixed-point integer that tells userspace how to convert it to nanoseconds. -Daniel I've not read the spec, but synchronous and current doesn't mean the exact same thing to me. I assume the spec doesn't allow getting the value in a batch and then just waiting for rendering to complete? If we go with the register read approach, I vote sysfs. ___ 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: [sparse] don't use variable size arrays
2012/4/16 Ben Widawsky b...@bwidawsk.net: Sparse doesn't like: error: bad constant expression bikeshedding I know you'll hate me for asking, but: how difficult is it to fix sparse? Adding those mallocs/frees increases the code complexity, making it harder to read... /bikeshedding -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx