[Intel-gfx] [PATCH 7/7] drm/i915: Add two-stage ILK-style watermark programming (v9)
In addition to calculating final watermarks, let's also pre-calculate a set of intermediate watermark values at atomic check time. These intermediate watermarks are a combination of the watermarks for the old state and the new state; they should satisfy the requirements of both states which means they can be programmed immediately when we commit the atomic state (without waiting for a vblank). Once the vblank does happen, we can then re-program watermarks to the more optimal final value. v2: Significant rebasing/rewriting. v3: - Move 'need_postvbl_update' flag to CRTC state (Daniel) - Don't forget to check intermediate watermark values for validity (Maarten) - Don't due async watermark optimization; just do it at the end of the atomic transaction, after waiting for vblanks. We do want it to be async eventually, but adding that now will cause more trouble for Maarten's in-progress work. (Maarten) - Don't allocate space in crtc_state for intermediate watermarks on platforms that don't need it (gen9+). - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit now that ilk_update_wm is gone. v4: - Add a wm_mutex to cover updates to intel_crtc->active and the need_postvbl_update flag. Since we don't have async yet it isn't terribly important yet, but might as well add it now. - Change interface to program watermarks. Platforms will now expose .initial_watermarks() and .optimize_watermarks() functions to do watermark programming. These should lock wm_mutex, copy the appropriate state values into intel_crtc->active, and then call the internal program watermarks function. v5: - Skip intermediate watermark calculation/check during initial hardware readout since we don't trust the existing HW values (and don't have valid values of our own yet). - Don't try to call .optimize_watermarks() on platforms that don't have atomic watermarks yet. (Maarten) v6: - Rebase v7: - Further rebase v8: - A few minor indentation and line length fixes v9: - Yet another rebase since Maarten's patches reworked a bunch of the code (wm_pre, wm_post, etc.) that this was previously based on. Cc: Maarten LankhorstSigned-off-by: Matt Roper Reviewed-by(v5): Maarten Lankhorst --- drivers/gpu/drm/i915/i915_drv.h | 6 +- drivers/gpu/drm/i915/intel_atomic.c | 1 + drivers/gpu/drm/i915/intel_display.c | 93 +++- drivers/gpu/drm/i915/intel_drv.h | 31 ++- drivers/gpu/drm/i915/intel_pm.c | 163 --- 5 files changed, 238 insertions(+), 56 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e7e54e2..2ee95cb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -630,7 +630,11 @@ struct drm_i915_display_funcs { struct dpll *best_clock); int (*compute_pipe_wm)(struct intel_crtc *crtc, struct drm_atomic_state *state); - void (*program_watermarks)(struct intel_crtc_state *cstate); + int (*compute_intermediate_wm)(struct drm_device *dev, + struct intel_crtc *intel_crtc, + struct intel_crtc_state *newstate); + void (*initial_watermarks)(struct intel_crtc_state *cstate); + void (*optimize_watermarks)(struct intel_crtc_state *cstate); void (*update_wm)(struct drm_crtc *crtc); int (*modeset_calc_cdclk)(struct drm_atomic_state *state); void (*modeset_commit_cdclk)(struct drm_atomic_state *state); diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index d0b1c9a..7501358 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) crtc_state->disable_lp_wm = false; crtc_state->disable_cxsr = false; crtc_state->wm_changed = false; + crtc_state->wm.need_postvbl_update = false; return _state->base; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bf2e97c..05379c9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4858,7 +4858,42 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) intel_set_memory_cxsr(dev_priv, false); } - if (!needs_modeset(_config->base) && pipe_config->wm_changed) + /* +* IVB workaround: must disable low power watermarks for at least +* one frame before enabling scaling. LP watermarks can be re-enabled +* when scaling is disabled. +* +* WaCxSRDisabledForSpriteScaling:ivb +*/ + if (pipe_config->disable_lp_wm) { + ilk_disable_lp_wm(dev); +
Re: [Intel-gfx] [PATCH v2] drm/i915: Correct max delay for HDMI hotplug live status checking
Yes, your code is clear. It tried to check hot-plug with 4 times based on delay setup (30ms) in following patch, commit 237ed86c693d8a8e4db476976aeb30df4deac74b Author: Sonika JindalDate: Tue Sep 15 09:44:20 2015 +0530 drm/i915: Check live status before reading edid I will refine patch for your review again. Thanks! Gary -Original Message- From: Jani Nikula [mailto:jani.nik...@linux.intel.com] Sent: Monday, December 14, 2015 7:29 PM To: Wang, Gary C ; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Correct max delay for HDMI hotplug live status checking On Mon, 14 Dec 2015, Gary Wang wrote: > The total delay of HDMI hotplug detecting with 30ms have already been > split into a resolution of 3 retries of 10ms each, for the worst > cases. But it still suffered from only waiting 10ms at most in > intel_hdmi_detect(). This patch corrects it by reading hotplug status > with 4 times at most for 30ms delay. It used to try twice, with 10 ms in between, but also with 10 ms after the last attempt, success or not. > > v2: > - straight up to loop execution for more clear in code readability > > Reviewed-by: Cooper Chiou > Tested-by: Gary Wang > Cc: Daniel Vetter > Cc: Gavin Hindman > Cc: Sonika Jindal > Cc: Shashank Sharma > Signed-off-by: Gary Wang > --- > drivers/gpu/drm/i915/intel_hdmi.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) mode change 100644 > => 100755 drivers/gpu/drm/i915/intel_hdmi.c > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > old mode 100644 > new mode 100755 > index 6825543..97ed16f > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1387,16 +1387,18 @@ intel_hdmi_detect(struct drm_connector *connector, > bool force) > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct drm_i915_private *dev_priv = to_i915(connector->dev); > bool live_status = false; > - unsigned int retry = 3; > + unsigned int retry; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - while (!live_status && --retry) { > + for (retry = 0; retry <= 3; retry++) { > live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + if (live_status || (retry == 3)) > + break; > mdelay(10); > } Basically the "retry <= 3" is never the stop condition in your loop, and is thus misleading. How about this instead, with the paradigm counting for loop: for (try = 0; !live_status && try < 4; try++) { if (try) mdelay(10); live_status = intel_digital_port_connected(...); } Note that I didn't actually check if this is the right thing to do; this is just your loop rewritten as I'd write it... ;) BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Fail the execbuff using stolen objects as batchbuffers
On Mon, Dec 14, 2015 at 11:16:11AM +0530, ankitprasad.r.sha...@intel.com wrote: > From: Ankitprasad Sharma> > Using stolen backed objects as a batchbuffer may result into a kernel > panic during relocation. Added a check to prevent the panic and fail > the execbuffer call. It is not recommended to use stolen object as > a batchbuffer. Nope. Let's fix it properly. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/i915: Support for creating Stolen memory backed objects
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index d727b49..ebce8c9 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -357,6 +357,7 @@ typedef struct drm_i915_irq_wait { > #define I915_PARAM_HAS_GPU_RESET 35 > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > +#define I915_PARAM_CREATE_VERSION 38 > > typedef struct drm_i915_getparam { > __s32 param; > @@ -456,6 +457,21 @@ struct drm_i915_gem_create { >*/ > __u32 handle; > __u32 pad; > + /** > + * Requested flags (currently used for placement > + * (which memory domain)) > + * > + * You can request that the object be created from special memory > + * rather than regular system pages using this parameter. Such > + * irregular objects may have certain restrictions (such as CPU > + * access to a stolen object is verboten). > + * > + * This can be used in the future for other purposes too > + * e.g. specifying tiling/caching/madvise > + */ > + __u32 flags; > +#define I915_CREATE_PLACEMENT_STOLEN (1<<0) /* Cannot use CPU mmaps > */ > +#define __I915_CREATE_UNKNOWN_FLAGS -(I915_CREATE_PLACEMENT_STOLEN << 1) Alignment. sizeof(drm_i915_gem_create) must be aligned to u64 since we contain u64 (to keep ABI compat for 32bit). -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] [MIPI SEQ PARSING v3 02/13] drm/i915: Updating asle structure with new fields
On Tue, 01 Dec 2015, Deepak Mwrote: > v3: rebase > > Cc: Jani Nikula > Signed-off-by: Deepak M Pushed this one as "drm/i915: add VBT address and size fields to ASLE mailbox struct". Thanks for the patch. BR, Jani. > --- > drivers/gpu/drm/i915/intel_opregion.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c > b/drivers/gpu/drm/i915/intel_opregion.c > index e362a30..64efedf 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -120,7 +120,9 @@ struct opregion_asle { > u64 fdss; > u32 fdsp; > u32 stat; > - u8 rsvd[70]; > + u64 rvda; /* Physical address of raw vbt data */ > + u32 rvds; /* Size of raw vbt data */ > + u8 rsvd[58]; > } __packed; > > /* Driver readiness indicator */ -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/11] drm/i915: Track active vma requests
Hook the vma itself into the i915_gem_request_retire() so that we can accurately track when a solitary vma is inactive (as opposed to having to wait for the entire object to be idle). This improves the interaction when using multiple contexts (with full-ppgtt) and eliminates some frequent list walking. A side-effect is that we get an active vma reference for free. The consequence of this is shown in the next patch... Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c| 2 +- drivers/gpu/drm/i915/i915_gem.c| 36 -- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 ++ drivers/gpu/drm/i915/i915_gem_gtt.c| 20 + drivers/gpu/drm/i915/i915_gem_gtt.h| 5 + 5 files changed, 47 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 179e3c5c5022..4df4ebbd56d6 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -356,7 +356,7 @@ static int per_file_stats(int id, void *ptr, void *data) continue; } - if (obj->active) /* XXX per-vma statistic */ + if (vma->active) stats->active += vma->node.size; else stats->inactive += vma->node.size; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8a824c5d5348..1d21c5b79215 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2040,7 +2040,6 @@ i915_gem_object_retire__read(struct drm_i915_gem_request_active *active, int ring = request->engine->id; struct drm_i915_gem_object *obj = container_of(active, struct drm_i915_gem_object, last_read[ring]); - struct i915_vma *vma; RQ_BUG_ON((obj->flags & (1 << (ring + I915_BO_ACTIVE_SHIFT))) == 0); @@ -2052,12 +2051,9 @@ i915_gem_object_retire__read(struct drm_i915_gem_request_active *active, * so that we don't steal from recently used but inactive objects * (unless we are forced to ofc!) */ - list_move_tail(>global_list, >i915->mm.bound_list); - - list_for_each_entry(vma, >vma_list, obj_link) { - if (!list_empty(>vm_link)) - list_move_tail(>vm_link, >vm->inactive_list); - } + if (!list_empty(>vma_list)) + list_move_tail(>global_list, + >i915->mm.bound_list); drm_gem_object_unreference(>base); } @@ -2567,7 +2563,19 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) { struct drm_i915_gem_object *obj = vma->obj; struct drm_i915_private *dev_priv = obj->base.dev->dev_private; - int ret; + int ret, i; + + /* First wait upon any activity as retiring the request may +* have side-effects such as unpinning or even unbinding this vma. +*/ + if (vma->active && wait) { + for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) { + ret = i915_wait_request(vma->last_read[i].request); + if (ret) + return ret; + } + RQ_BUG_ON(vma->active); + } if (list_empty(>obj_link)) return 0; @@ -2582,12 +2590,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) BUG_ON(obj->pages == NULL); - if (wait) { - ret = i915_gem_object_wait_rendering(obj, false); - if (ret) - return ret; - } - if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { i915_gem_object_finish_gtt(obj); @@ -3023,9 +3025,8 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) /* And bump the LRU for this access */ vma = i915_gem_obj_to_ggtt(obj); - if (vma && drm_mm_node_allocated(>node) && !obj->active) - list_move_tail(>vm_link, - _i915(obj->base.dev)->gtt.base.inactive_list); + if (vma && drm_mm_node_allocated(>node) && !vma->active) + list_move_tail(>vm_link, >vm->inactive_list); return 0; } @@ -3874,6 +3875,7 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj, void i915_gem_vma_destroy(struct i915_vma *vma) { WARN_ON(vma->node.allocated); + RQ_BUG_ON(vma->active); /* Keep the vma as a placeholder in the execbuffer reservation lists */ if (!list_empty(>exec_list)) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 6de8681bb64c..1d4378a4501e 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1099,6 +1099,8 @@ void
Re: [Intel-gfx] [PATCH 15/32] drm/i915: Slaughter the thundering i915_wait_request herd
Hi, On 11/12/15 11:33, Chris Wilson wrote: One particularly stressful scenario consists of many independent tasks all competing for GPU time and waiting upon the results (e.g. realtime transcoding of many, many streams). One bottleneck in particular is that each client waits on its own results, but every client is woken up after every batchbuffer - hence the thunder of hooves as then every client must do its heavyweight dance to read a coherent seqno to see if it is the lucky one. Ideally, we only want one client to wake up after the interrupt and check its request for completion. Since the requests must retire in order, we can select the first client on the oldest request to be woken. Once that client has completed his wait, we can then wake up the next client and so on. However, all clients then incur latency as every process in the chain may be delayed for scheduling - this may also then cause some priority inversion. To reduce the latency, when a client is added or removed from the list, we scan the tree for completed seqno and wake up all the completed waiters in parallel. v2: Convert from a kworker per engine into a dedicated kthread for the bottom-half. v3: Rename request members and tweak comments. v4: Use a per-engine spinlock in the breadcrumbs bottom-half. v5: Fix race in locklessly checking waiter status and kicking the task on adding a new waiter. v6: Fix deciding when to force the timer to hide missing interrupts. v7: Move the bottom-half from the kthread to the first client process. v8: Reword a few comments v9: Break the busy loop when the interrupt is unmasked or has fired. v10: Comments, unnecessary churn, better debugging from Tvrtko v11: Wake all completed waiters on removing the current bottom-half to reduce the latency of waking up a herd of clients all waiting on the same request. v12: Rearrange missed-interrupt fault injection so that it works with igt/drv_missed_irq_hang Signed-off-by: Chris WilsonCc: "Rogozhkin, Dmitry V" Cc: "Gong, Zhipeng" Cc: Tvrtko Ursulin Cc: Dave Gordon --- drivers/gpu/drm/i915/Makefile| 1 + drivers/gpu/drm/i915/i915_debugfs.c | 19 ++- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/i915_gem.c | 152 - drivers/gpu/drm/i915/i915_gpu_error.c| 2 +- drivers/gpu/drm/i915/i915_irq.c | 14 +- drivers/gpu/drm/i915/intel_breadcrumbs.c | 274 +++ drivers/gpu/drm/i915/intel_lrc.c | 5 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +- drivers/gpu/drm/i915/intel_ringbuffer.h | 63 ++- 10 files changed, 436 insertions(+), 102 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_breadcrumbs.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 0851de07bd13..d3b9d3618719 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -35,6 +35,7 @@ i915-y += i915_cmd_parser.o \ i915_gem_userptr.o \ i915_gpu_error.o \ i915_trace_points.o \ + intel_breadcrumbs.o \ intel_lrc.o \ intel_mocs.o \ intel_ringbuffer.o \ diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index d5f66bbdb160..48e574247a30 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -730,10 +730,22 @@ static int i915_gem_request_info(struct seq_file *m, void *data) static void i915_ring_seqno_info(struct seq_file *m, struct intel_engine_cs *ring) { + struct rb_node *rb; + if (ring->get_seqno) { seq_printf(m, "Current sequence (%s): %x\n", ring->name, ring->get_seqno(ring, false)); } + + spin_lock(>breadcrumbs.lock); + for (rb = rb_first(>breadcrumbs.requests); +rb != NULL; +rb = rb_next(rb)) { + struct intel_breadcrumb *b = container_of(rb, typeof(*b), node); + seq_printf(m, "Waiting (%s): %s [%d] on %x\n", + ring->name, b->task->comm, b->task->pid, b->seqno); + } + spin_unlock(>breadcrumbs.lock); } static int i915_gem_seqno_info(struct seq_file *m, void *data) @@ -1356,8 +1368,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused) for_each_ring(ring, dev_priv, i) { seq_printf(m, "%s:\n", ring->name); - seq_printf(m, "\tseqno = %x [current %x]\n", - ring->hangcheck.seqno, seqno[i]); + seq_printf(m, "\tseqno = %x [current %x], waiters? %d\n", + ring->hangcheck.seqno, seqno[i], + intel_engine_has_waiter(ring)); seq_printf(m, "\tACTHD = 0x%08llx [current
Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Record the ringbuffer associated with the request
On 11/12/15 22:59, Chris Wilson wrote: The request tells us where to read the ringbuf from, so use that information to simplify the error capture. If no request was active at the time of the hang, the ring is idle and there is no information inside the ring pertaining to the hang. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_gpu_error.c | 29 ++--- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 3e137fc701cf..6eefe9c36931 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -995,7 +995,7 @@ static void i915_gem_record_rings(struct drm_device *dev, for (i = 0; i < I915_NUM_RINGS; i++) { struct intel_engine_cs *ring = _priv->ring[i]; - struct intel_ringbuffer *rbuf; + struct intel_ringbuffer *rbuf = NULL; error->ring[i].pid = -1; @@ -1039,26 +1039,17 @@ static void i915_gem_record_rings(struct drm_device *dev, } rcu_read_unlock(); } + + rbuf = request->ringbuf; } - if (i915.enable_execlists) { - /* TODO: This is only a small fix to keep basic error -* capture working, but we need to add more information -* for it to be useful (e.g. dump the context being -* executed). -*/ - if (request) - rbuf = request->ctx->engine[ring->id].ringbuf; - else - rbuf = ring->default_context->engine[ring->id].ringbuf; - } else - rbuf = ring->buffer; - - error->ring[i].cpu_ring_head = rbuf->head; - error->ring[i].cpu_ring_tail = rbuf->tail; - - error->ring[i].ringbuffer = - i915_error_ggtt_object_create(dev_priv, rbuf->obj); + if (rbuf) { + error->ring[i].cpu_ring_head = rbuf->head; + error->ring[i].cpu_ring_tail = rbuf->tail; + error->ring[i].ringbuffer = + i915_error_ggtt_object_create(dev_priv, + rbuf->obj); + } error->ring[i].hws_page = i915_error_ggtt_object_create(dev_priv, ring->status_page.obj); I think the code you deleted is intended to capture the *default* ringbuffer if there is no request active -- sometimes we will switch an engine to the default context (and therefore ringbuffer) when there's no work to be done. Another option that's sometimes useful is to capture the ringbuffer pointed to by the START register. This helps in finding situations where the driver and the GPU disagree about what should be in progress. I've got a few patches that update some of the error capture that's always been missing in execlist mode (like, actually capturing the active context), and add some more decoding of what we do capture. John Harrison posted them as part of the "Preemption support for GPU scheduler" patchset last week, although they're not really anything to do with preemption per se. One of them "drm/i915/error: improve CSB reporting" also updates this area of the code, so maybe I should incorporate your change into the next revision of that patch? .Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/10] drm/i915: Rename vma->*_list to *_link for consistency
Elsewhere we have adopted the convention of using '_link' to denote elements in the list (and '_list' for the actual list_head itself), and that the name should indicate which list the link belongs to (and preferrably not just where the link is being stored). s/vma_link/obj_link/ (we iterate over obj->vma_list) s/mm_list/vm_link/ (we iterate over vm->[in]active_list) Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c | 17 +-- drivers/gpu/drm/i915/i915_gem.c | 50 drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/i915_gem_evict.c| 6 ++-- drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++ drivers/gpu/drm/i915/i915_gem_gtt.h | 4 +-- drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 +-- drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- drivers/gpu/drm/i915/i915_gem_userptr.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c| 8 ++--- 10 files changed, 52 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 68310ba73d23..fdcf90757eb0 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -117,9 +117,8 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) u64 size = 0; struct i915_vma *vma; - list_for_each_entry(vma, >vma_list, vma_link) { - if (i915_is_ggtt(vma->vm) && - drm_mm_node_allocated(>node)) + list_for_each_entry(vma, >vma_list, obj_link) { + if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(>node)) size += vma->node.size; } @@ -155,7 +154,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); if (obj->base.name) seq_printf(m, " (name: %d)", obj->base.name); - list_for_each_entry(vma, >vma_list, vma_link) { + list_for_each_entry(vma, >vma_list, obj_link) { if (vma->pin_count > 0) pin_count++; } @@ -164,7 +163,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_printf(m, " (display)"); if (obj->fence_reg != I915_FENCE_REG_NONE) seq_printf(m, " (fence: %d)", obj->fence_reg); - list_for_each_entry(vma, >vma_list, vma_link) { + list_for_each_entry(vma, >vma_list, obj_link) { seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", i915_is_ggtt(vma->vm) ? "g" : "pp", vma->node.start, vma->node.size); @@ -229,7 +228,7 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data) } total_obj_size = total_gtt_size = count = 0; - list_for_each_entry(vma, head, mm_list) { + list_for_each_entry(vma, head, vm_link) { seq_printf(m, " "); describe_obj(m, vma->obj); seq_printf(m, "\n"); @@ -341,7 +340,7 @@ static int per_file_stats(int id, void *ptr, void *data) stats->shared += obj->base.size; if (USES_FULL_PPGTT(obj->base.dev)) { - list_for_each_entry(vma, >vma_list, vma_link) { + list_for_each_entry(vma, >vma_list, obj_link) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(>node)) @@ -453,12 +452,12 @@ static int i915_gem_object_info(struct seq_file *m, void* data) count, mappable_count, size, mappable_size); size = count = mappable_size = mappable_count = 0; - count_vmas(>active_list, mm_list); + count_vmas(>active_list, vm_link); seq_printf(m, " %u [%u] active objects, %llu [%llu] bytes\n", count, mappable_count, size, mappable_size); size = count = mappable_size = mappable_count = 0; - count_vmas(>inactive_list, mm_list); + count_vmas(>inactive_list, vm_link); seq_printf(m, " %u [%u] inactive objects, %llu [%llu] bytes\n", count, mappable_count, size, mappable_size); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a7c39d5fbcca..e6ace74616b2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -128,10 +128,10 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, pinned = 0; mutex_lock(>struct_mutex); - list_for_each_entry(vma, >base.active_list, mm_list) + list_for_each_entry(vma, >base.active_list, vm_link) if (vma->pin_count) pinned += vma->node.size; - list_for_each_entry(vma, >base.inactive_list, mm_list) + list_for_each_entry(vma, >base.inactive_list, vm_link) if (vma->pin_count) pinned +=
[Intel-gfx] [PATCH 02/11] drm/i915: Refactor activity tracking for requests
With the introduction of requests, we amplified the number of atomic refcounted objects we use and update every execbuffer; from none to several references, and a set of references that need to be changed. We also introduced interesting side-effects in the order of retiring requests and objects. Instead of independently tracking the last request for an object, track the active objects for each request. The object will reside in the buffer list of its most recent active request and so we reduce the kref interchange to a list_move. Now retirements are entirely driven by the request, dramatically simplifying activity tracking on the object themselves, and removing the ambiguity between retiring objects and retiring requests. All told, less code, simpler and faster, and more extensible. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/Makefile | 1 - drivers/gpu/drm/i915/i915_debugfs.c | 4 +- drivers/gpu/drm/i915/i915_drv.h | 10 --- drivers/gpu/drm/i915/i915_gem.c | 149 drivers/gpu/drm/i915/i915_gem_debug.c | 70 --- drivers/gpu/drm/i915/i915_gem_fence.c | 10 +-- drivers/gpu/drm/i915/i915_gem_request.c | 39 ++--- drivers/gpu/drm/i915/i915_gem_request.h | 26 +- drivers/gpu/drm/i915/i915_gpu_error.c | 4 +- drivers/gpu/drm/i915/intel_lrc.c| 1 - drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +- drivers/gpu/drm/i915/intel_ringbuffer.h | 12 --- 12 files changed, 93 insertions(+), 240 deletions(-) delete mode 100644 drivers/gpu/drm/i915/i915_gem_debug.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 3df88be4752e..14794370db44 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -21,7 +21,6 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o i915-y += i915_cmd_parser.o \ i915_gem_batch_pool.o \ i915_gem_context.o \ - i915_gem_debug.o \ i915_gem_dmabuf.o \ i915_gem_evict.o \ i915_gem_execbuffer.o \ diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index dafc58b94c9f..68310ba73d23 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -695,13 +695,13 @@ static int i915_gem_request_info(struct seq_file *m, void *data) int count; count = 0; - list_for_each_entry(req, >request_list, list) + list_for_each_entry(req, >request_list, link) count++; if (count == 0) continue; seq_printf(m, "%s requests: %d\n", ring->name, count); - list_for_each_entry(req, >request_list, list) { + list_for_each_entry(req, >request_list, link) { struct task_struct *task; rcu_read_lock(); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 43de7c2c6b3e..dbcb7659ba2b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -444,8 +444,6 @@ void intel_link_compute_m_n(int bpp, int nlanes, #define DRIVER_MINOR 6 #define DRIVER_PATCHLEVEL 0 -#define WATCH_LISTS0 - struct opregion_header; struct opregion_acpi; struct opregion_swsci; @@ -2014,7 +2012,6 @@ struct drm_i915_gem_object { struct drm_mm_node *stolen; struct list_head global_list; - struct list_head ring_list[I915_NUM_RINGS]; /** Used in execbuf to temporarily hold a ref */ struct list_head obj_exec_link; @@ -3079,13 +3076,6 @@ static inline bool i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec obj->tiling_mode != I915_TILING_NONE; } -/* i915_gem_debug.c */ -#if WATCH_LISTS -int i915_verify_lists(struct drm_device *dev); -#else -#define i915_verify_lists(dev) 0 -#endif - /* i915_debugfs.c */ int i915_debugfs_init(struct drm_minor *minor); void i915_debugfs_cleanup(struct drm_minor *minor); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b412b9291d91..a7c39d5fbcca 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -38,14 +38,8 @@ #include #include -#define RQ_BUG_ON(expr) - static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj); static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj); -static void -i915_gem_object_retire__write(struct drm_i915_gem_object *obj); -static void -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring); static bool cpu_cache_is_coherent(struct drm_device *dev, enum i915_cache_level level) @@ -119,7 +113,6 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) if (ret) return ret; - WARN_ON(i915_verify_lists(dev));
[Intel-gfx] [PATCH 01/10] drm/i915: Refactor activity tracking for requests
With the introduction of requests, we amplified the number of atomic refcounted objects we use and update every execbuffer; from none to several references, and a set of references that need to be changed. We also introduced interesting side-effects in the order of retiring requests and objects. Instead of independently tracking the last request for an object, track the active objects for each request. The object will reside in the buffer list of its most recent active request and so we reduce the kref interchange to a list_move. Now retirements are entirely driven by the request, dramatically simplifying activity tracking on the object themselves, and removing the ambiguity between retiring objects and retiring requests. All told, less code, simpler and faster, and more extensible. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/Makefile | 1 - drivers/gpu/drm/i915/i915_debugfs.c | 4 +- drivers/gpu/drm/i915/i915_drv.h | 10 --- drivers/gpu/drm/i915/i915_gem.c | 149 drivers/gpu/drm/i915/i915_gem_debug.c | 70 --- drivers/gpu/drm/i915/i915_gem_fence.c | 10 +-- drivers/gpu/drm/i915/i915_gem_request.c | 39 ++--- drivers/gpu/drm/i915/i915_gem_request.h | 26 +- drivers/gpu/drm/i915/i915_gpu_error.c | 4 +- drivers/gpu/drm/i915/intel_lrc.c| 1 - drivers/gpu/drm/i915/intel_ringbuffer.c | 7 +- drivers/gpu/drm/i915/intel_ringbuffer.h | 12 --- 12 files changed, 93 insertions(+), 240 deletions(-) delete mode 100644 drivers/gpu/drm/i915/i915_gem_debug.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 3df88be4752e..14794370db44 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -21,7 +21,6 @@ i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o i915-y += i915_cmd_parser.o \ i915_gem_batch_pool.o \ i915_gem_context.o \ - i915_gem_debug.o \ i915_gem_dmabuf.o \ i915_gem_evict.o \ i915_gem_execbuffer.o \ diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index dafc58b94c9f..68310ba73d23 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -695,13 +695,13 @@ static int i915_gem_request_info(struct seq_file *m, void *data) int count; count = 0; - list_for_each_entry(req, >request_list, list) + list_for_each_entry(req, >request_list, link) count++; if (count == 0) continue; seq_printf(m, "%s requests: %d\n", ring->name, count); - list_for_each_entry(req, >request_list, list) { + list_for_each_entry(req, >request_list, link) { struct task_struct *task; rcu_read_lock(); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 43de7c2c6b3e..dbcb7659ba2b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -444,8 +444,6 @@ void intel_link_compute_m_n(int bpp, int nlanes, #define DRIVER_MINOR 6 #define DRIVER_PATCHLEVEL 0 -#define WATCH_LISTS0 - struct opregion_header; struct opregion_acpi; struct opregion_swsci; @@ -2014,7 +2012,6 @@ struct drm_i915_gem_object { struct drm_mm_node *stolen; struct list_head global_list; - struct list_head ring_list[I915_NUM_RINGS]; /** Used in execbuf to temporarily hold a ref */ struct list_head obj_exec_link; @@ -3079,13 +3076,6 @@ static inline bool i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec obj->tiling_mode != I915_TILING_NONE; } -/* i915_gem_debug.c */ -#if WATCH_LISTS -int i915_verify_lists(struct drm_device *dev); -#else -#define i915_verify_lists(dev) 0 -#endif - /* i915_debugfs.c */ int i915_debugfs_init(struct drm_minor *minor); void i915_debugfs_cleanup(struct drm_minor *minor); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b412b9291d91..a7c39d5fbcca 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -38,14 +38,8 @@ #include #include -#define RQ_BUG_ON(expr) - static void i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj); static void i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj); -static void -i915_gem_object_retire__write(struct drm_i915_gem_object *obj); -static void -i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring); static bool cpu_cache_is_coherent(struct drm_device *dev, enum i915_cache_level level) @@ -119,7 +113,6 @@ int i915_mutex_lock_interruptible(struct drm_device *dev) if (ret) return ret; - WARN_ON(i915_verify_lists(dev));
[Intel-gfx] [PATCH 01/11] drm/i915: Introduce drm_i915_gem_request_node for request tracking
In the next patch, request tracking is made more generic and for that we need a new expanded struct and to separate out the logic changes from the mechanical churn, we split out the structure renaming into this patch. For extra fun, create a new i915_gem_request.h. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c| 10 +++--- drivers/gpu/drm/i915/i915_drv.h| 9 +++-- drivers/gpu/drm/i915/i915_gem.c| 56 +++--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +-- drivers/gpu/drm/i915/i915_gem_fence.c | 6 ++-- drivers/gpu/drm/i915/i915_gem_request.h| 11 ++ drivers/gpu/drm/i915/i915_gem_tiling.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c | 6 ++-- drivers/gpu/drm/i915/intel_display.c | 10 +++--- 9 files changed, 62 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 77bae3151fca..dafc58b94c9f 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -146,10 +146,10 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) obj->base.write_domain); for_each_ring(ring, dev_priv, i) seq_printf(m, "%x ", - i915_gem_request_get_seqno(obj->last_read_req[i])); + i915_gem_request_get_seqno(obj->last_read[i].request)); seq_printf(m, "] %x %x%s%s%s", - i915_gem_request_get_seqno(obj->last_write_req), - i915_gem_request_get_seqno(obj->last_fenced_req), + i915_gem_request_get_seqno(obj->last_write.request), + i915_gem_request_get_seqno(obj->last_fence.request), i915_cache_level_str(to_i915(obj->base.dev), obj->cache_level), obj->dirty ? " dirty" : "", obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); @@ -184,8 +184,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) *t = '\0'; seq_printf(m, " (%s mappable)", s); } - if (obj->last_write_req != NULL) - seq_printf(m, " (%s)", obj->last_write_req->engine->name); + if (obj->last_write.request != NULL) + seq_printf(m, " (%s)", obj->last_write.request->engine->name); if (obj->frontbuffer_bits) seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8d3818cb5815..43de7c2c6b3e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2100,11 +2100,10 @@ struct drm_i915_gem_object { * requests on one ring where the write request is older than the * read request. This allows for the CPU to read from an active * buffer by only waiting for the write to complete. -* */ - struct drm_i915_gem_request *last_read_req[I915_NUM_RINGS]; - struct drm_i915_gem_request *last_write_req; - /** Breadcrumb of last fenced GPU access to the buffer. */ - struct drm_i915_gem_request *last_fenced_req; +*/ + struct drm_i915_gem_request_active last_read[I915_NUM_RINGS]; + struct drm_i915_gem_request_active last_write; + struct drm_i915_gem_request_active last_fence; /** 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 65f40d0e1cb7..b412b9291d91 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1119,23 +1119,23 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj, return 0; if (readonly) { - if (obj->last_write_req != NULL) { - ret = i915_wait_request(obj->last_write_req); + if (obj->last_write.request != NULL) { + ret = i915_wait_request(obj->last_write.request); if (ret) return ret; - i = obj->last_write_req->engine->id; - if (obj->last_read_req[i] == obj->last_write_req) + i = obj->last_write.request->engine->id; + if (obj->last_read[i].request == obj->last_write.request) i915_gem_object_retire__read(obj, i); else i915_gem_object_retire__write(obj); } } else { for (i = 0; i < I915_NUM_RINGS; i++) { - if (obj->last_read_req[i] == NULL) + if (obj->last_read[i].request == NULL) continue; - ret = i915_wait_request(obj->last_read_req[i]); +
[Intel-gfx] [PATCH 04/11] drm/i915: Amalgamate GGTT/ppGTT vma debug list walkers
As we can now have multiple VMA inside the global GTT (with partial mappings, rotations, etc), it is no longer true that there may just be a single GGTT entry and so we should walk the full vma_list to count up the actual usage. In addition to unifying the two walkers, switch from multiplying the object size for each vma to summing the bound vma sizes. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c | 46 +++-- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fdcf90757eb0..c04ba9981e9b 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -332,6 +332,7 @@ static int per_file_stats(int id, void *ptr, void *data) struct drm_i915_gem_object *obj = ptr; struct file_stats *stats = data; struct i915_vma *vma; + int bound = 0; stats->count++; stats->total += obj->base.size; @@ -339,41 +340,30 @@ static int per_file_stats(int id, void *ptr, void *data) if (obj->base.name || obj->base.dma_buf) stats->shared += obj->base.size; - if (USES_FULL_PPGTT(obj->base.dev)) { - list_for_each_entry(vma, >vma_list, obj_link) { - struct i915_hw_ppgtt *ppgtt; + list_for_each_entry(vma, >vma_list, obj_link) { + if (!drm_mm_node_allocated(>node)) + continue; - if (!drm_mm_node_allocated(>node)) - continue; + bound++; - if (i915_is_ggtt(vma->vm)) { - stats->global += obj->base.size; - continue; - } - - ppgtt = container_of(vma->vm, struct i915_hw_ppgtt, base); + if (i915_is_ggtt(vma->vm)) { + stats->global += vma->node.size; + } else { + struct i915_hw_ppgtt *ppgtt + = container_of(vma->vm, + struct i915_hw_ppgtt, + base); if (ppgtt->file_priv != stats->file_priv) continue; - - if (obj->active) /* XXX per-vma statistic */ - stats->active += obj->base.size; - else - stats->inactive += obj->base.size; - - return 0; - } - } else { - if (i915_gem_obj_ggtt_bound(obj)) { - stats->global += obj->base.size; - if (obj->active) - stats->active += obj->base.size; - else - stats->inactive += obj->base.size; - return 0; } + + if (obj->active) /* XXX per-vma statistic */ + stats->active += vma->node.size; + else + stats->inactive += vma->node.size; } - if (!list_empty(>global_list)) + if (!bound) stats->unbound += obj->base.size; return 0; -- 2.6.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/11] drm/i915: Store owning file on the i915_address_space
For the global GTT (and aliasing GTT), the address space is owned by the device (it is a global resource) and so the per-file owner field is NULL. For per-process GTT (where we create an address space per context), each is owned by the opening file. We can use this ownership information to both distinguish GGTT and ppGTT address spaces, as well as occasionally inspect the owner. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_gem_context.c | 3 ++- drivers/gpu/drm/i915/i915_gem_gtt.c | 25 + drivers/gpu/drm/i915/i915_gem_gtt.h | 13 ++--- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 9ec133f5af00..179e3c5c5022 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -352,7 +352,7 @@ static int per_file_stats(int id, void *ptr, void *data) = container_of(vma->vm, struct i915_hw_ppgtt, base); - if (ppgtt->file_priv != stats->file_priv) + if (ppgtt->base.file != stats->file_priv) continue; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6ce163a681f2..b32a00f60e98 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2924,7 +2924,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm) return container_of(vm, struct i915_hw_ppgtt, base); } - static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj) { return i915_gem_obj_ggtt_bound_view(obj, _ggtt_view_normal); diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 25a8498f0b5a..dcb4603a7f03 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -296,7 +296,8 @@ i915_gem_create_context(struct drm_device *dev, } if (USES_FULL_PPGTT(dev)) { - struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); + struct i915_hw_ppgtt *ppgtt = + i915_ppgtt_create(to_i915(dev), file_priv); if (IS_ERR_OR_NULL(ppgtt)) { DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index da150c27a76c..130ccefb2491 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2112,11 +2112,12 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) return 0; } -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) +static int __hw_ppgtt_init(struct i915_hw_ppgtt *ppgtt, + struct drm_i915_private *dev_priv) { - ppgtt->base.dev = dev; + ppgtt->base.dev = dev_priv->dev; - if (INTEL_INFO(dev)->gen < 8) + if (INTEL_INFO(dev_priv)->gen < 8) return gen6_ppgtt_init(ppgtt); else return gen8_ppgtt_init(ppgtt); @@ -2132,15 +2133,17 @@ static void i915_address_space_init(struct i915_address_space *vm, list_add_tail(>global_link, _priv->vm_list); } -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) +int i915_ppgtt_init(struct i915_hw_ppgtt *ppgtt, + struct drm_i915_private *dev_priv, + struct drm_i915_file_private *file_priv) { - struct drm_i915_private *dev_priv = dev->dev_private; int ret = 0; - ret = __hw_ppgtt_init(dev, ppgtt); + ret = __hw_ppgtt_init(ppgtt, dev_priv); if (ret == 0) { kref_init(>ref); i915_address_space_init(>base, dev_priv); + ppgtt->base.file = file_priv; } return ret; @@ -2183,7 +2186,8 @@ int i915_ppgtt_init_ring(struct drm_i915_gem_request *req) } struct i915_hw_ppgtt * -i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv) +i915_ppgtt_create(struct drm_i915_private *dev_priv, + struct drm_i915_file_private *fpriv) { struct i915_hw_ppgtt *ppgtt; int ret; @@ -2192,14 +2196,12 @@ i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv) if (!ppgtt) return ERR_PTR(-ENOMEM); - ret = i915_ppgtt_init(dev, ppgtt); + ret = i915_ppgtt_init(ppgtt, dev_priv, fpriv); if (ret) { kfree(ppgtt); return ERR_PTR(ret); } - ppgtt->file_priv = fpriv; - trace_i915_ppgtt_create(>base); return ppgtt; @@ -2717,7 +2719,7 @@ static int
Re: [Intel-gfx] [PATCH 2/9] drm/i915: Use insert_page for pwrite_fast
On Mon, Dec 14, 2015 at 11:16:04AM +0530, ankitprasad.r.sha...@intel.com wrote: > From: Ankitprasad Sharma> > In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First, > we try a nonblocking pin for the whole object (since that is fastest if > reused), then failing that we try to grab one page in the mappable > aperture. It also allows us to handle objects larger than the mappable > aperture (e.g. if we need to pwrite with vGPU restricting the aperture > to a measely 8MiB or something like that). > > v2: Pin pages before starting pwrite, Combined duplicate loops (Chris) > > v3: Combined loops based on local patch by Chris (Chris) > > v4: Added i915 wrapper function for drm_mm_insert_node_in_range (Chris) > > Signed-off-by: Ankitprasad Sharma > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_gem.c | 86 > ++--- > 1 file changed, 64 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index bf7f203..46c1e75 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -61,6 +61,21 @@ static bool cpu_write_needs_clflush(struct > drm_i915_gem_object *obj) > return obj->pin_display; > } > > +static int > +i915_gem_insert_node_in_range(struct drm_i915_private *i915, > + struct drm_mm_node *node, u64 size, > + unsigned alignment, u64 start, u64 end) > +{ > + int ret; > + > + ret = drm_mm_insert_node_in_range_generic(>gtt.base.mm, node, > + size, alignment, 0, start, > + end, DRM_MM_SEARCH_DEFAULT, > + DRM_MM_SEARCH_DEFAULT); > + > + return ret; > +} No. It encodes a very bad assumption (i915->gtt) that is not made clear in anyway. -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 6/9] drm/i915: Add support for stealing purgable stolen pages
On Mon, Dec 14, 2015 at 11:16:08AM +0530, ankitprasad.r.sha...@intel.com wrote: > @@ -2039,6 +2052,8 @@ struct drm_i915_gem_object { > struct list_head obj_exec_link; > > struct list_head batch_pool_link; > + /** Used during stolen memory allocations to temporarily hold a ref */ > + struct list_head stolen_link; It would be very useful to me if you could rename this tmp_link. /** Used by eviction/debugfs only under the struct_mutex when the caller * is certain they are the only users of this list. */ > + INIT_LIST_HEAD(>stolen_link); Doesn't require init. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo
From: Ville SyrjäläThe vma may have been rebound between the last time the cursor was enabled and now, so skipping the cursor gtt offset deduction is not safe unless we would also reset cursor_bo to NULL when disabling the cursor. Just thow cursor_bo to the bin instead since it's lost all other uses thanks to universal plane support. Cc: Takashi Iwai Cc: Jani Nikula Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 5 - drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9bb860a55dc..f2a0151b3f14 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14105,9 +14105,6 @@ intel_commit_cursor_plane(struct drm_plane *plane, crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); - if (intel_crtc->cursor_bo == obj) - goto update; - if (!obj) addr = 0; else if (!INTEL_INFO(dev)->cursor_needs_physical) @@ -14116,9 +14113,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, addr = obj->phys_handle->busaddr; intel_crtc->cursor_addr = addr; - intel_crtc->cursor_bo = obj; -update: intel_crtc_update_cursor(crtc, state->visible); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 76dfa286cd95..6324c782d062 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -568,7 +568,6 @@ struct intel_crtc { int adjusted_x; int adjusted_y; - struct drm_i915_gem_object *cursor_bo; uint32_t cursor_addr; uint32_t cursor_cntl; uint32_t cursor_size; -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing
From: Ville SyrjäläThe cursor code tries to treat base==0 to mean disabled. That fails when the cursor bo gets bound at ggtt offset 0, and the user is left looking at an invisible cursor. We lose the disabled->disabled optimization, but that seems like something better handled at a slightly higher level. Cc: Takashi Iwai Cc: Jani Nikula Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 29 - 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f2a0151b3f14..ee84b517eb74 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -10013,14 +10013,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, return true; } -static void i845_update_cursor(struct drm_crtc *crtc, u32 base) +static void i845_update_cursor(struct drm_crtc *crtc, u32 base, bool on) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); uint32_t cntl = 0, size = 0; - if (base) { + if (on) { unsigned int width = intel_crtc->base.cursor->state->crtc_w; unsigned int height = intel_crtc->base.cursor->state->crtc_h; unsigned int stride = roundup_pow_of_two(width) * 4; @@ -10075,16 +10075,15 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) } } -static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) +static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, bool on) { struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int pipe = intel_crtc->pipe; - uint32_t cntl; + uint32_t cntl = 0; - cntl = 0; - if (base) { + if (on) { cntl = MCURSOR_GAMMA_ENABLE; switch (intel_crtc->base.cursor->state->crtc_w) { case 64: @@ -10135,18 +10134,17 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, int y = cursor_state->crtc_y; u32 base = 0, pos = 0; - if (on) - base = intel_crtc->cursor_addr; + base = intel_crtc->cursor_addr; if (x >= intel_crtc->config->pipe_src_w) - base = 0; + on = false; if (y >= intel_crtc->config->pipe_src_h) - base = 0; + on = false; if (x < 0) { if (x + cursor_state->crtc_w <= 0) - base = 0; + on = false; pos |= CURSOR_POS_SIGN << CURSOR_X_SHIFT; x = -x; @@ -10155,16 +10153,13 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, if (y < 0) { if (y + cursor_state->crtc_h <= 0) - base = 0; + on = false; pos |= CURSOR_POS_SIGN << CURSOR_Y_SHIFT; y = -y; } pos |= y << CURSOR_Y_SHIFT; - if (base == 0 && intel_crtc->cursor_base == 0) - return; - I915_WRITE(CURPOS(pipe), pos); /* ILK+ do this automagically */ @@ -10175,9 +10170,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, } if (IS_845G(dev) || IS_I865G(dev)) - i845_update_cursor(crtc, base); + i845_update_cursor(crtc, base, on); else - i9xx_update_cursor(crtc, base); + i9xx_update_cursor(crtc, base, on); } static bool cursor_size_ok(struct drm_device *dev, -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] drm/i915: Fix the invisible cursor syndrome
From: Ville SyrjäläThese two should provide the minimal fix for the invisible cursor problem that's been plaguing me and some other people. It's caused by the cursor bo getting bound at ggtt offset 0, which the code then considers to mean "not enabled". These patches get rid of that assumption, with the caveat that we may do some needless cursor register writes. I have a more generic fix for that (and a ton of other cursor improvements) at [1], but I figured that I'll just post the critical fixes now and allow the others to stew a bit more in the branch until maybe early next year. I assume the problem is also related to the lack of fbdev pinning that Chris has fixed [2]. With fbdev permanalty pinned at offset 0, there's no opportunity for the cursor bo to travel there. Without fbdev I suppose something else rings, status page, etc. could get permanently placed at offset 0, so not sure if this can be hit in that case (didn't try it). I'm a bit wary about adding cc stable to these since I suspect that backporting them too far will cause regressions, on account of the atomic/plane stuff probably not being up to the task in older kernels. So maybe better get these in w/o cc stable and consider a backport again later if it looks necessary. [1] git://github.com/vsyrjala/linux.git cursor_improvements [2] http://lists.freedesktop.org/archives/intel-gfx/2015-December/082114.html Ville Syrjälä (2): drm/i915: Kill intel_crtc->cursor_bo drm/i915: Drop the broken cursor base==0 special casing drivers/gpu/drm/i915/intel_display.c | 34 -- drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 12 insertions(+), 23 deletions(-) Cc: Takashi Iwai Cc: Jani Nikula -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/11] drm/i915: Rename vma->*_list to *_link for consistency
Elsewhere we have adopted the convention of using '_link' to denote elements in the list (and '_list' for the actual list_head itself), and that the name should indicate which list the link belongs to (and preferrably not just where the link is being stored). s/vma_link/obj_link/ (we iterate over obj->vma_list) s/mm_list/vm_link/ (we iterate over vm->[in]active_list) Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c | 17 +-- drivers/gpu/drm/i915/i915_gem.c | 50 drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/i915_gem_evict.c| 6 ++-- drivers/gpu/drm/i915/i915_gem_gtt.c | 10 +++ drivers/gpu/drm/i915/i915_gem_gtt.h | 4 +-- drivers/gpu/drm/i915/i915_gem_shrinker.c | 4 +-- drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- drivers/gpu/drm/i915/i915_gem_userptr.c | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c| 8 ++--- 10 files changed, 52 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 68310ba73d23..fdcf90757eb0 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -117,9 +117,8 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) u64 size = 0; struct i915_vma *vma; - list_for_each_entry(vma, >vma_list, vma_link) { - if (i915_is_ggtt(vma->vm) && - drm_mm_node_allocated(>node)) + list_for_each_entry(vma, >vma_list, obj_link) { + if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(>node)) size += vma->node.size; } @@ -155,7 +154,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) obj->madv == I915_MADV_DONTNEED ? " purgeable" : ""); if (obj->base.name) seq_printf(m, " (name: %d)", obj->base.name); - list_for_each_entry(vma, >vma_list, vma_link) { + list_for_each_entry(vma, >vma_list, obj_link) { if (vma->pin_count > 0) pin_count++; } @@ -164,7 +163,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_printf(m, " (display)"); if (obj->fence_reg != I915_FENCE_REG_NONE) seq_printf(m, " (fence: %d)", obj->fence_reg); - list_for_each_entry(vma, >vma_list, vma_link) { + list_for_each_entry(vma, >vma_list, obj_link) { seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", i915_is_ggtt(vma->vm) ? "g" : "pp", vma->node.start, vma->node.size); @@ -229,7 +228,7 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data) } total_obj_size = total_gtt_size = count = 0; - list_for_each_entry(vma, head, mm_list) { + list_for_each_entry(vma, head, vm_link) { seq_printf(m, " "); describe_obj(m, vma->obj); seq_printf(m, "\n"); @@ -341,7 +340,7 @@ static int per_file_stats(int id, void *ptr, void *data) stats->shared += obj->base.size; if (USES_FULL_PPGTT(obj->base.dev)) { - list_for_each_entry(vma, >vma_list, vma_link) { + list_for_each_entry(vma, >vma_list, obj_link) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(>node)) @@ -453,12 +452,12 @@ static int i915_gem_object_info(struct seq_file *m, void* data) count, mappable_count, size, mappable_size); size = count = mappable_size = mappable_count = 0; - count_vmas(>active_list, mm_list); + count_vmas(>active_list, vm_link); seq_printf(m, " %u [%u] active objects, %llu [%llu] bytes\n", count, mappable_count, size, mappable_size); size = count = mappable_size = mappable_count = 0; - count_vmas(>inactive_list, mm_list); + count_vmas(>inactive_list, vm_link); seq_printf(m, " %u [%u] inactive objects, %llu [%llu] bytes\n", count, mappable_count, size, mappable_size); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a7c39d5fbcca..e6ace74616b2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -128,10 +128,10 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, pinned = 0; mutex_lock(>struct_mutex); - list_for_each_entry(vma, >base.active_list, mm_list) + list_for_each_entry(vma, >base.active_list, vm_link) if (vma->pin_count) pinned += vma->node.size; - list_for_each_entry(vma, >base.inactive_list, mm_list) + list_for_each_entry(vma, >base.inactive_list, vm_link) if (vma->pin_count) pinned +=
[Intel-gfx] [PATCH 11/11] Revert "drm/i915: Clean up associated VMAs on context destruction"
This reverts commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae. The patch was incomplete, introduced more problems of greater severity than it solved and worse the solution was known and available at the time of the patch. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_drv.h | 5 - drivers/gpu/drm/i915/i915_gem.c | 14 ++ drivers/gpu/drm/i915/i915_gem_context.c | 22 -- 3 files changed, 2 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 66ecd6b3df95..32427a063213 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2717,11 +2717,6 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj, int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags); int __must_check i915_vma_unbind(struct i915_vma *vma); -/* - * BEWARE: Do not use the function below unless you can _absolutely_ - * _guarantee_ VMA in question is _not in use_ anywhere. - */ -int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma); void i915_vma_close(struct i915_vma *vma); int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 08ea0b7eda8b..566eb4338497 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2584,7 +2584,7 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj) old_write_domain); } -static int __i915_vma_unbind(struct i915_vma *vma, bool wait) +int i915_vma_unbind(struct i915_vma *vma) { struct drm_i915_gem_object *obj = vma->obj; struct drm_i915_private *dev_priv = obj->base.dev->dev_private; @@ -2593,7 +2593,7 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) /* First wait upon any activity as retiring the request may * have side-effects such as unpinning or even unbinding this vma. */ - if (vma->active && wait) { + if (vma->active) { for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) { ret = i915_wait_request(vma->last_read[i].request); if (ret) @@ -2655,16 +2655,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) return 0; } -int i915_vma_unbind(struct i915_vma *vma) -{ - return __i915_vma_unbind(vma, true); -} - -int __i915_vma_unbind_no_wait(struct i915_vma *vma) -{ - return __i915_vma_unbind(vma, false); -} - int i915_gpu_idle(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 9669547c7c2d..40250667fcf2 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -133,21 +133,6 @@ static int get_context_size(struct drm_device *dev) return ret; } -static void i915_gem_context_clean(struct intel_context *ctx) -{ - struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; - struct i915_vma *vma, *next; - - if (!ppgtt) - return; - - list_for_each_entry_safe(vma, next, >base.inactive_list, -vm_link) { - if (WARN_ON(__i915_vma_unbind_no_wait(vma))) - break; - } -} - void i915_gem_context_free(struct kref *ctx_ref) { struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); @@ -158,13 +143,6 @@ void i915_gem_context_free(struct kref *ctx_ref) if (i915.enable_execlists) intel_lr_context_free(ctx); - /* -* This context is going away and we need to remove all VMAs still -* around. This is to handle imported shared objects for which -* destructor did not run when their handles were closed. -*/ - i915_gem_context_clean(ctx); - i915_ppgtt_put(ctx->ppgtt); if (ctx->legacy_hw_ctx.rcs_state) -- 2.6.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/11] drm/i915: Mark the context and address space as closed
When the user closes the context mark it and the dependent address space as closed. As we use an asynchronous destruct method, this has two purposes. First it allows us to flag the closed context and detect internal errors if we to create any new objects for it (as it is removed from the user's namespace, these should be internal bugs only). And secondly, it allows us to immediately reap stale vma. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_drv.h | 4 drivers/gpu/drm/i915/i915_gem.c | 15 - drivers/gpu/drm/i915/i915_gem_context.c | 39 + drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 9 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +- 6 files changed, 66 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 696469a06715..66ecd6b3df95 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -892,6 +892,8 @@ struct intel_context { } engine[I915_NUM_RINGS]; struct list_head link; + + bool closed:1; }; enum fb_op_origin { @@ -2720,6 +2722,8 @@ int __must_check i915_vma_unbind(struct i915_vma *vma); * _guarantee_ VMA in question is _not in use_ anywhere. */ int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma); +void i915_vma_close(struct i915_vma *vma); + int i915_gem_object_put_pages(struct drm_i915_gem_object *obj); void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv); void i915_gem_release_mmap(struct drm_i915_gem_object *obj); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 7c13c27a6470..08ea0b7eda8b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2367,12 +2367,13 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj) return 0; } -static void i915_vma_close(struct i915_vma *vma) +void i915_vma_close(struct i915_vma *vma) { RQ_BUG_ON(vma->closed); vma->closed = true; list_del_init(>obj_link); + list_del_init(>vm_link); if (!vma->active) WARN_ON(i915_vma_unbind(vma)); } @@ -2620,12 +2621,13 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) return ret; } - trace_i915_vma_unbind(vma); - - vma->vm->unbind_vma(vma); + if (likely(!vma->vm->closed)) { + trace_i915_vma_unbind(vma); + vma->vm->unbind_vma(vma); + } vma->bound = 0; - list_del_init(>vm_link); + list_move_tail(>vm_link, >vm->unbound_list); if (vma->is_ggtt) { if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { obj->map_and_fenceable = false; @@ -2882,7 +2884,7 @@ search_free: goto err_remove_node; list_move_tail(>global_list, _priv->mm.bound_list); - list_add_tail(>vm_link, >inactive_list); + list_move_tail(>vm_link, >inactive_list); return vma; @@ -3890,6 +3892,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma) if (!list_empty(>exec_list)) return; + list_del(>vm_link); if (!vma->is_ggtt) i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index c4a8a64cd1b2..9669547c7c2d 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -153,6 +153,7 @@ void i915_gem_context_free(struct kref *ctx_ref) struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref); trace_i915_context_free(ctx); + RQ_BUG_ON(!ctx->closed); if (i915.enable_execlists) intel_lr_context_free(ctx); @@ -209,6 +210,36 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size) return obj; } +static void i915_ppgtt_close(struct i915_address_space *vm) +{ + struct list_head *phases[] = { + >active_list, + >inactive_list, + >unbound_list, + NULL, + }, **phase; + + RQ_BUG_ON(vm->is_ggtt); + RQ_BUG_ON(vm->closed); + vm->closed = true; + + for (phase = phases; *phase; phase++) { + struct i915_vma *vma, *vn; + + list_for_each_entry_safe(vma, vn, *phase, vm_link) + i915_vma_close(vma); + } +} + +static void context_close(struct intel_context *ctx) +{ + RQ_BUG_ON(ctx->closed); + ctx->closed = true; + if (ctx->ppgtt) + i915_ppgtt_close(>ppgtt->base); + i915_gem_context_unreference(ctx); +} + static struct intel_context * __create_hw_context(struct drm_device *dev, struct drm_i915_file_private *file_priv) @@ -256,7 +287,7 @@
[Intel-gfx] [PATCH 05/11] drm/i915: Reduce the pointer dance of i915_is_ggtt()
The multiple levels of indirect do nothing but hinder the compiler and the pointer chasing turns to be quite painful but painless to fix. Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_debugfs.c| 13 ++--- drivers/gpu/drm/i915/i915_drv.h| 7 --- drivers/gpu/drm/i915/i915_gem.c| 18 +++--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++--- drivers/gpu/drm/i915/i915_gem_gtt.c| 12 +--- drivers/gpu/drm/i915/i915_gem_gtt.h| 5 + drivers/gpu/drm/i915/i915_trace.h | 27 --- 7 files changed, 33 insertions(+), 54 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index c04ba9981e9b..9ec133f5af00 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -118,7 +118,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj) struct i915_vma *vma; list_for_each_entry(vma, >vma_list, obj_link) { - if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(>node)) + if (vma->is_ggtt && drm_mm_node_allocated(>node)) size += vma->node.size; } @@ -165,12 +165,11 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_printf(m, " (fence: %d)", obj->fence_reg); list_for_each_entry(vma, >vma_list, obj_link) { seq_printf(m, " (%sgtt offset: %08llx, size: %08llx", - i915_is_ggtt(vma->vm) ? "g" : "pp", + vma->is_ggtt ? "g" : "pp", vma->node.start, vma->node.size); - if (i915_is_ggtt(vma->vm)) - seq_printf(m, ", type: %u)", vma->ggtt_view.type); - else - seq_puts(m, ")"); + if (vma->is_ggtt) + seq_printf(m, ", type: %u", vma->ggtt_view.type); + seq_puts(m, ")"); } if (obj->stolen) seq_printf(m, " (stolen: %08llx)", obj->stolen->start); @@ -346,7 +345,7 @@ static int per_file_stats(int id, void *ptr, void *data) bound++; - if (i915_is_ggtt(vma->vm)) { + if (vma->is_ggtt) { stats->global += vma->node.size; } else { struct i915_hw_ppgtt *ppgtt diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index dbcb7659ba2b..6ce163a681f2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2916,18 +2916,11 @@ bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj); /* Some GGTT VM helpers */ #define i915_obj_to_ggtt(obj) \ (&((struct drm_i915_private *)(obj)->base.dev->dev_private)->gtt.base) -static inline bool i915_is_ggtt(struct i915_address_space *vm) -{ - struct i915_address_space *ggtt = - &((struct drm_i915_private *)(vm)->dev->dev_private)->gtt.base; - return vm == ggtt; -} static inline struct i915_hw_ppgtt * i915_vm_to_ppgtt(struct i915_address_space *vm) { WARN_ON(i915_is_ggtt(vm)); - return container_of(vm, struct i915_hw_ppgtt, base); } diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e6ace74616b2..144e92df8137 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2603,8 +2603,7 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) return ret; } - if (i915_is_ggtt(vma->vm) && - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { + if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { i915_gem_object_finish_gtt(obj); /* release the fence reg _after_ flushing */ @@ -2619,7 +2618,7 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) vma->bound = 0; list_del_init(>vm_link); - if (i915_is_ggtt(vma->vm)) { + if (vma->is_ggtt) { if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) { obj->map_and_fenceable = false; } else if (vma->ggtt_view.pages) { @@ -3889,17 +3888,14 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj, void i915_gem_vma_destroy(struct i915_vma *vma) { - struct i915_address_space *vm = NULL; WARN_ON(vma->node.allocated); /* Keep the vma as a placeholder in the execbuffer reservation lists */ if (!list_empty(>exec_list)) return; - vm = vma->vm; - - if (!i915_is_ggtt(vm)) - i915_ppgtt_put(i915_vm_to_ppgtt(vm)); + if (!vma->is_ggtt) + i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); list_del(>obj_link); @@ -4425,7 +4421,7 @@ u64 i915_gem_obj_offset(struct
[Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed
In order to prevent a leak of the vma on shared objects, we need to hook into the object_close callback to destroy the vma on the object for this file. However, if we destroyed that vma immediately we may cause unexpected application stalls as we try to unbind a busy vma - hence we defer the unbind to when we retire the vma. Testcase: igt/gem_ppggtt/flink-and-close-vma-leak Signed-off-by: Chris WilsonCc: Tvrtko Ursulin Cc: Daniele Ceraolo Spurio closed); + vma->closed = true; + + list_del_init(>obj_link); + if (!vma->active) + WARN_ON(i915_vma_unbind(vma)); +} + +void i915_gem_close_object(struct drm_gem_object *gem, + struct drm_file *file) +{ + struct drm_i915_gem_object *obj = to_intel_bo(gem); + struct drm_i915_file_private *fpriv = file->driver_priv; + struct i915_vma *vma, *vn; + + mutex_lock(>base.dev->struct_mutex); + list_for_each_entry_safe(vma, vn, >vma_list, obj_link) + if (vma->vm->file == fpriv) + i915_vma_close(vma); + mutex_unlock(>base.dev->struct_mutex); +} + /** * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT * @DRM_IOCTL_ARGS: standard ioctl arguments @@ -2577,9 +2601,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) RQ_BUG_ON(vma->active); } - if (list_empty(>obj_link)) - return 0; - if (!drm_mm_node_allocated(>node)) { i915_gem_vma_destroy(vma); return 0; @@ -3792,20 +3813,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) trace_i915_gem_object_destroy(obj); list_for_each_entry_safe(vma, next, >vma_list, obj_link) { - int ret; - vma->pin_count = 0; - ret = i915_vma_unbind(vma); - if (WARN_ON(ret == -ERESTARTSYS)) { - bool was_interruptible; - - was_interruptible = dev_priv->mm.interruptible; - dev_priv->mm.interruptible = false; - - WARN_ON(i915_vma_unbind(vma)); - - dev_priv->mm.interruptible = was_interruptible; - } + i915_vma_close(vma); } /* Stolen objects don't hold a ref, but do hold pin count. Fix that up diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 5505603f52af..9f594c33bd0a 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3240,6 +3240,8 @@ i915_vma_retire(struct drm_i915_gem_request_active *active, return; list_move_tail(>vm_link, >vm->inactive_list); + if (unlikely(vma->closed)) + WARN_ON(i915_vma_unbind(vma)); } static struct i915_vma * diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index c2f2c62ac88d..be7e8526b219 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++
[Intel-gfx] [PATCH 07/11] drm/i915: i915_vma_move_to_active prep patch
This patch is broken out of the next just to remove the code motion from that patch and make it more readable. What we do here is move the i915_vma_move_to_active() to i915_gem_execbuffer.c and put the three stages (read, write, fenced) together so that future modifications to active handling are all located in the same spot. The importance of this is so that we can more simply control the order in which the requests are place in the retirement list (i.e. control the order at which we retire and so control the lifetimes to avoid having to hold onto references). Signed-off-by: Chris Wilson--- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/i915_gem.c | 15 --- drivers/gpu/drm/i915/i915_gem_context.c | 7 ++-- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 63 ++-- drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +- 5 files changed, 49 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b32a00f60e98..eb775eb1c693 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2775,7 +2775,8 @@ int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); int i915_gem_object_sync(struct drm_i915_gem_object *obj, struct drm_i915_gem_request *to); void i915_vma_move_to_active(struct i915_vma *vma, -struct drm_i915_gem_request *req); +struct drm_i915_gem_request *req, +unsigned flags); int i915_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 144e92df8137..8a824c5d5348 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2016,21 +2016,6 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj) return obj->vmapping; } -void i915_vma_move_to_active(struct i915_vma *vma, -struct drm_i915_gem_request *req) -{ - struct drm_i915_gem_object *obj = vma->obj; - struct intel_engine_cs *engine = req->engine; - - /* Add a reference if we're newly entering the active list. */ - if (obj->active == 0) - drm_gem_object_reference(>base); - obj->active |= intel_engine_flag(engine); - - i915_gem_request_mark_active(req, >last_read[engine->id]); - list_move_tail(>vm_link, >vm->active_list); -} - static void i915_gem_object_retire__fence(struct drm_i915_gem_request_active *active, struct drm_i915_gem_request *req) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index dcb4603a7f03..c4a8a64cd1b2 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -766,8 +766,8 @@ static int do_switch(struct drm_i915_gem_request *req) * MI_SET_CONTEXT instead of when the next seqno has completed. */ if (from != NULL) { - from->legacy_hw_ctx.rcs_state->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; - i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->legacy_hw_ctx.rcs_state), req); + struct drm_i915_gem_object *obj = from->legacy_hw_ctx.rcs_state; + /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the * whole damn pipeline, we don't need to explicitly mark the * object dirty. The only exception is that the context must be @@ -775,7 +775,8 @@ static int do_switch(struct drm_i915_gem_request *req) * able to defer doing this until we know the object would be * swapped, but there is no way to do that yet. */ - from->legacy_hw_ctx.rcs_state->dirty = 1; + obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; + i915_vma_move_to_active(i915_gem_obj_to_ggtt(obj), req, 0); /* obj is kept alive until the next request by its active ref */ i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 6788f71ad989..6de8681bb64c 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1064,6 +1064,44 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file, return ctx; } +void i915_vma_move_to_active(struct i915_vma *vma, +struct drm_i915_gem_request *req, +unsigned flags) +{ + struct drm_i915_gem_object *obj = vma->obj; + const unsigned engine = req->engine->id; + +
Re: [Intel-gfx] [PATCH 11/13] android/sync: Fix reversed sense of signaled fence
On 14/12/15 11:22, John Harrison wrote: On 11/12/2015 15:57, Tvrtko Ursulin wrote: On 11/12/15 13:11, john.c.harri...@intel.com wrote: From: Peter LawthersIn the 3.14 kernel, a signaled fence was indicated by the status field == 1. In 4.x, a status == 0 indicates signaled, status < 0 indicates error, and status > 0 indicates active. This patch wraps the check for a signaled fence in a function so that callers no longer needs to know the underlying implementation. v3: New patch for series. Change-Id: I8e565e49683e3efeb9474656cd84cf4add6ad6a2 Tracked-On: https://jira01.devtools.intel.com/browse/ACD-308 Signed-off-by: Peter Lawthers --- drivers/android/sync.h | 21 + 1 file changed, 21 insertions(+) diff --git a/drivers/android/sync.h b/drivers/android/sync.h index d57fa0a..75532d8 100644 --- a/drivers/android/sync.h +++ b/drivers/android/sync.h @@ -345,6 +345,27 @@ int sync_fence_cancel_async(struct sync_fence *fence, */ int sync_fence_wait(struct sync_fence *fence, long timeout); +/** + * sync_fence_is_signaled() - Return an indication if the fence is signaled + * @fence:fence to check + * + * returns 1 if fence is signaled + * returns 0 if fence is not signaled + * returns < 0 if fence is in error state + */ +static inline int +sync_fence_is_signaled(struct sync_fence *fence) +{ +int status; + +status = atomic_read(>status); +if (status == 0) +return 1; +if (status > 0) +return 0; +return status; +} Not so important but could simply return bool, like "return status <= 0"? Since it is called "is_signaled" and it is only used in boolean mode in future patches. There is no point in throwing away the error code unnecessarily. It can be useful in debug output and indeed will show up the scheduler status dump via debugfs. You could still grab it directly from that call site. Or by adding another accessor like sync_fence_get_error(). Just saying that it may be good to decouple more from sync_fence implementation, since the internals have changed once already. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Propagating correct error codes to the userspace
On Mon, Dec 14, 2015 at 11:16:07AM +0530, ankitprasad.r.sha...@intel.com wrote: > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 17d679e..366080b9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -492,6 +492,7 @@ i915_pages_create_for_stolen(struct drm_device *dev, > struct drm_i915_private *dev_priv = dev->dev_private; > struct sg_table *st; > struct scatterlist *sg; > + int ret; > > DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size); > BUG_ON(offset > dev_priv->gtt.stolen_size - size); > @@ -503,11 +504,12 @@ i915_pages_create_for_stolen(struct drm_device *dev, > > st = kmalloc(sizeof(*st), GFP_KERNEL); > if (st == NULL) > - return NULL; > + return ERR_PTR(-ENOMEM); > > - if (sg_alloc_table(st, 1, GFP_KERNEL)) { > + ret = sg_alloc_table(st, 1, GFP_KERNEL); > + if (ret) { > kfree(st); > - return NULL; > + return ERR_PTR(ret); > } > > sg = st->sgl; > @@ -559,15 +561,17 @@ _i915_gem_object_create_stolen(struct drm_device *dev, > > obj = i915_gem_object_alloc(dev); > if (obj == NULL) > - return NULL; > + return ERR_PTR(-ENOMEM); > > drm_gem_private_object_init(dev, >base, stolen->size); > i915_gem_object_init(obj, _gem_object_stolen_ops); > > obj->pages = i915_pages_create_for_stolen(dev, > stolen->start, stolen->size); > - if (obj->pages == NULL) > - goto cleanup; > + if (IS_ERR(obj->pages)) { > + i915_gem_object_free(obj); > + return (void*) obj->pages; This is a bad idiom to use. Looks ok here (as only one caller sees the invalid obj->pages) but it was an immediate red-flag for me as a reader of the code (since you are storing an invalid pointer in a very common field). Anyway the correct use is return ERR_CAST(obj->pages); However, I would much prefer a temporary variable: pages = i915_pages_crate_for_stolen(); if (IS_ERR(pages)) { object_free(obj); return ERR_CAST(pages); } obj->pages = pages; Just so that I don't have to think about who might chase that invalid pointer, today or in the future. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Update to post-reset execlist queue clean-up
Dave Gordonwrites: > On 01/12/15 11:46, Tvrtko Ursulin wrote: >> >> On 23/10/15 18:02, Tomas Elf wrote: >>> When clearing an execlist queue, instead of traversing it and >>> unreferencing all >>> requests while holding the spinlock (which might lead to thread >>> sleeping with >>> IRQs are turned off - bad news!), just move all requests to the retire >>> request >>> list while holding spinlock and then drop spinlock and invoke the >>> execlists >>> request retirement path, which already deals with the intricacies of >>> purging/dereferencing execlist queue requests. >>> >>> This patch can be considered v3 of: >>> >>> commit b96db8b81c54ef30485ddb5992d63305d86ea8d3 >>> Author: Tomas Elf >>> drm/i915: Grab execlist spinlock to avoid post-reset concurrency >>> issues >>> >>> This patch assumes v2 of the above patch is part of the baseline, >>> reverts v2 >>> and adds changes on top to turn it into v3. >>> >>> Signed-off-by: Tomas Elf >>> Cc: Tvrtko Ursulin >>> Cc: Chris Wilson >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 15 --- >>> 1 file changed, 4 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c >>> b/drivers/gpu/drm/i915/i915_gem.c >>> index 2c7a0b7..b492603 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -2756,20 +2756,13 @@ static void i915_gem_reset_ring_cleanup(struct >>> drm_i915_private *dev_priv, >>> >>> if (i915.enable_execlists) { >>> spin_lock_irq(>execlist_lock); >>> -while (!list_empty(>execlist_queue)) { >>> -struct drm_i915_gem_request *submit_req; >>> >>> -submit_req = list_first_entry(>execlist_queue, >>> -struct drm_i915_gem_request, >>> -execlist_link); >>> -list_del(_req->execlist_link); >>> +/* list_splice_tail_init checks for empty lists */ >>> +list_splice_tail_init(>execlist_queue, >>> + >execlist_retired_req_list); >>> >>> -if (submit_req->ctx != ring->default_context) >>> -intel_lr_context_unpin(submit_req); >>> - >>> -i915_gem_request_unreference(submit_req); >>> -} >>> spin_unlock_irq(>execlist_lock); >>> +intel_execlists_retire_requests(ring); >>> } >>> >>> /* >> >> Fallen through the cracks.. >> >> This looks to be even more serious, since lockdep notices possible >> deadlock involving vmap_area_lock: >> >> Possible interrupt unsafe locking scenario: >> >> CPU0CPU1 >> >>lock(vmap_area_lock); >> local_irq_disable(); >> lock(&(>execlist_lock)->rlock); >> lock(vmap_area_lock); >> >> lock(&(>execlist_lock)->rlock); >> >> *** DEADLOCK *** >> >> Because it unpins LRC context and ringbuffer which ends up in the VM >> code under the execlist_lock. >> >> intel_execlists_retire_requests is slightly different from the code in >> the reset handler because it concerns itself with ctx_obj existence >> which the other one doesn't. >> >> Could people more knowledgeable of this code check if it is OK and R-B? >> >> Regards, >> >> Tvrtko > > Hi Tvrtko, > > I didn't understand this message at first, I thought you'd found a > problem with this ("v3") patch, but now I see what you actually meant is > that there is indeed a problem with the (v2) that got merged, not the > original question about unreferencing an object while holding a spinlock > (because it can't be the last reference), but rather because of the > unpin, which can indeed cause a problem with a non-i915-defined kernel lock. > > So we should certainly update the current (v2) upstream with this. > Thomas Daniel already R-B'd this code on 23rd October, when it was: > > [PATCH v3 7/8] drm/i915: Grab execlist spinlock to avoid post-reset > concurrency issues. > > and it hasn't changed in substance since then, so you can carry his R-B > over, plus I said on that same day that this was a better solution. So: > > Reviewed-by: Thomas Daniel > Reviewed-by: Dave Gordon > Bat farm did encounter with this few weeks back, so it was vaguely registered. But I just failed with timely review. Thanks for pushing it forward, -Mika > ___ > 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 2/9] drm/i915: Use insert_page for pwrite_fast
On Mon, Dec 14, 2015 at 09:54:06AM +, Chris Wilson wrote: > On Mon, Dec 14, 2015 at 11:16:04AM +0530, ankitprasad.r.sha...@intel.com > wrote: > > From: Ankitprasad Sharma> > > > In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First, > > we try a nonblocking pin for the whole object (since that is fastest if > > reused), then failing that we try to grab one page in the mappable > > aperture. It also allows us to handle objects larger than the mappable > > aperture (e.g. if we need to pwrite with vGPU restricting the aperture > > to a measely 8MiB or something like that). > > > > v2: Pin pages before starting pwrite, Combined duplicate loops (Chris) > > > > v3: Combined loops based on local patch by Chris (Chris) > > > > v4: Added i915 wrapper function for drm_mm_insert_node_in_range (Chris) > > > > Signed-off-by: Ankitprasad Sharma > > Signed-off-by: Chris Wilson > > --- > > drivers/gpu/drm/i915/i915_gem.c | 86 > > ++--- > > 1 file changed, 64 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c > > index bf7f203..46c1e75 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -61,6 +61,21 @@ static bool cpu_write_needs_clflush(struct > > drm_i915_gem_object *obj) > > return obj->pin_display; > > } > > > > +static int > > +i915_gem_insert_node_in_range(struct drm_i915_private *i915, > > + struct drm_mm_node *node, u64 size, > > + unsigned alignment, u64 start, u64 end) > > +{ > > + int ret; > > + > > + ret = drm_mm_insert_node_in_range_generic(>gtt.base.mm, node, > > + size, alignment, 0, start, > > + end, DRM_MM_SEARCH_DEFAULT, > > + DRM_MM_SEARCH_DEFAULT); > > + > > + return ret; > > +} > > No. It encodes a very bad assumption (i915->gtt) that is not made clear > in anyway. static int insert_mappable_node(struct drm_i915_private *i915, struct drm_mm_node *node) { return drm_mm_insert_node_in_range_generic(>gtt.base.mm, node, 4096, 0, 0, 0, i915->gtt.mappable_end, DRM_MM_SEARCH_DEFAULT, DRM_MM_SEARCH_DEFAULT); } Should do the trick -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 v2 3/3] drm/i915: Clean up GPU hang message
On Mon, Dec 14, 2015 at 11:28:39AM +, Dave Gordon wrote: > On 11/12/15 22:59, Chris Wilson wrote: > >Remove some redundant kernel messages as we deduce a hung GPU and > >capture the error state. > > > >Signed-off-by: Chris Wilson> >--- > > drivers/gpu/drm/i915/i915_irq.c | 16 ++-- > > 1 file changed, 6 insertions(+), 10 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_irq.c > >b/drivers/gpu/drm/i915/i915_irq.c > >index 4cfbd694b3a8..365d4872a67d 100644 > >--- a/drivers/gpu/drm/i915/i915_irq.c > >+++ b/drivers/gpu/drm/i915/i915_irq.c > >@@ -2963,7 +2963,7 @@ static void i915_hangcheck_elapsed(struct work_struct > >*work) > > struct drm_device *dev = dev_priv->dev; > > struct intel_engine_cs *ring; > > int i; > >-int busy_count = 0, rings_hung = 0; > >+int busy_count = 0; > > bool stuck[I915_NUM_RINGS] = { 0 }; > > #define BUSY 1 > > #define KICK 5 > >@@ -3057,17 +3057,13 @@ static void i915_hangcheck_elapsed(struct > >work_struct *work) > > } > > > > for_each_ring(ring, dev_priv, i) { > >-if (ring->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG) { > >-DRM_INFO("%s on %s\n", > >- stuck[i] ? "stuck" : "no progress", > >- ring->name); > >-rings_hung++; > >-} > >+if (ring->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG) > >+return i915_handle_error(dev, true, > >+ "%s on %s", > >+ stuck[i] ? "No progress" : > >"Hang", > >+ ring->name); > > } > > > >-if (rings_hung) > >-return i915_handle_error(dev, true, "Ring hung"); > >- > > /* Reset timer in case GPU hangs without another request being added */ > > if (busy_count) > > i915_queue_hangcheck(dev_priv); > > This version provides less information (in dmesg & syslog) in the > case that multiple rings have (been detected as) hung. Does this > ever happen? Not often. And intended, since that information is already in the error state. > Is the original more useful for finding bugs in > hangcheck itself? No. See i915_hangcheck_info. > If this is a test that has disabled error state > capture (because it's *trying* to hang one or more rings) can we > still know how many rings have been diagnosed as hung? If you have a use case, then you can look at the interface you require. i915_hangcheck_info should be sufficient in most cases, or at least a good starting point. But you may want a more specific debugfs to avoid parsing. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Correct max delay for HDMI hotplug live status checking
The total delay of HDMI hotplug detecting with 30ms have already been split into a resolution of 3 retries of 10ms each, for the worst cases. But it still suffered from only waiting 10ms at most in intel_hdmi_detect(). This patch corrects it by reading hotplug status with 4 times at most for 30ms delay. v2: - straight up to loop execution for more clear in code readability Reviewed-by: Cooper ChiouTested-by: Gary Wang Cc: Daniel Vetter Cc: Gavin Hindman Cc: Sonika Jindal Cc: Shashank Sharma Signed-off-by: Gary Wang --- drivers/gpu/drm/i915/intel_hdmi.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) mode change 100644 => 100755 drivers/gpu/drm/i915/intel_hdmi.c diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c old mode 100644 new mode 100755 index 6825543..97ed16f --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1387,16 +1387,18 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct drm_i915_private *dev_priv = to_i915(connector->dev); bool live_status = false; - unsigned int retry = 3; + unsigned int retry; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - while (!live_status && --retry) { + for (retry = 0; retry <= 3; retry++) { live_status = intel_digital_port_connected(dev_priv, hdmi_to_dig_port(intel_hdmi)); + if (live_status || (retry == 3)) + break; mdelay(10); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4 2/2] drm/i915: start adding dp mst audio
On Mon, 14 Dec 2015 10:33:44 +0100, Jani Nikula wrote: > > On Fri, 11 Dec 2015, Takashi Iwaiwrote: > > On Fri, 11 Dec 2015 07:07:53 +0100, > > Libin Yang wrote: > >> > >> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c > >> >>> b/drivers/gpu/drm/i915/intel_audio.c > >> >>> index 9aa83e7..5ad2e66 100644 > >> >>> --- a/drivers/gpu/drm/i915/intel_audio.c > >> >>> +++ b/drivers/gpu/drm/i915/intel_audio.c > >> >>> @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct > >> >>> intel_encoder *encoder) > >> >>>tmp |= AUD_CONFIG_N_PROG_ENABLE; > >> >>>tmp &= ~AUD_CONFIG_UPPER_N_MASK; > >> >>>tmp &= ~AUD_CONFIG_LOWER_N_MASK; > >> >>> - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) > >> >>> + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || > >> >>> + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) > >> >>>tmp |= AUD_CONFIG_N_VALUE_INDEX; > > > > The same check is missing in hsw_audio_codec_enable()? > > > >> >>>I915_WRITE(HSW_AUD_CFG(pipe), tmp); > >> >>> > >> >>> @@ -474,7 +475,8 @@ static void ilk_audio_codec_enable(struct > >> >>> drm_connector *connector, > >> >>>tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > >> >>>tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > >> >>>tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; > >> >>> - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) > >> >>> + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || > >> >>> + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) > >> >>>tmp |= AUD_CONFIG_N_VALUE_INDEX; > > > > ... and missing for ilk_audio_codec_disable()? > > > > > >> >>>else > >> >>>tmp |= audio_config_hdmi_pixel_clock(adjusted_mode); > >> >>> @@ -512,7 +514,8 @@ void intel_audio_codec_enable(struct intel_encoder > >> >>> *intel_encoder) > >> >>> > >> >>>/* ELD Conn_Type */ > >> >>>connector->eld[5] &= ~(3 << 2); > >> >>> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) > >> >>> + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || > >> >>> + intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST)) > > > > IMO, it's better to have a macro to cover this two-line check instead > > of open-coding at each place. We'll have 5 places in the end. > > I don't think that's necessary. Only the hsw_* functions need it, the > other platforms (using the other functions) don't support DP MST. Then the patch changes the functions. And still three places have this check. 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: Allow objects to go back above 4GB in the address range
On 12/11/2015 6:57 PM, Daniel Vetter wrote: On Fri, Dec 11, 2015 at 02:49:52PM +, Chris Wilson wrote: On Fri, Dec 11, 2015 at 02:34:13PM +, Michel Thierry wrote: We detected if objects should be moved to the lower parts when 48-bit support flag was not set, but not the other way around. This handles the case in which an object was allocated in the 32-bit address range, but it has been marked as safe to move above it, which theoretically would help to keep the lower addresses available for objects which really need to be there. Cc: Daniele Ceraolo SpurioSigned-off-by: Michel Thierry No. This is not lazy. When we run out of low space, we evict. Until then don't cause extra work for no reason. Yeah, this stuff should just work. First the eviction code should kick stuff out, and if we totally deadlock then we'll retry with everything placed nicely. Long-term objects should segregate (assuming you're not mixing them up badly in the userspace cache). How did this come up? I think there's a more in-depth story to be shared here, with some perf data to illustrate it ... Hi, It came from some local testing; Daniele saw bo's with the support flag enabled staying in the 32-bit range (the test was changing the flag between submissions). If there's no space constraints, re-enabling the flag won't relocate them, only when evict is required as you said... and that's not really an issue. Sorry for the noise. -Michel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/9] drm/i915: Migrate stolen objects before hibernation
On Mon, Dec 14, 2015 at 11:16:10AM +0530, ankitprasad.r.sha...@intel.com wrote: > +static int > +copy_content(struct drm_i915_gem_object *obj, > + struct drm_i915_private *i915, > + struct address_space *mapping) > +{ > + struct drm_mm_node node; > + int ret, i; > + > + /* stolen objects are already pinned to prevent shrinkage */ > + memset(, 0, sizeof(node)); > + ret = i915_gem_insert_node_in_range(i915, , 4096, 0, > + 0, i915->gtt.mappable_end); > + if (ret) > + return ret; > + > + for (i = 0; i < obj->base.size / PAGE_SIZE; i++) { > + struct page *page; > + void *__iomem src; > + void *dst; > + > + wmb(); > + i915->gtt.base.insert_page(>gtt.base, > +i915_gem_object_get_dma_address(obj, > i), > +node.start, > +I915_CACHE_NONE, > +0); > + wmb(); > + > + page = shmem_read_mapping_page(mapping, i); > + if (IS_ERR(page)) { > + ret = PTR_ERR(page); > + break; > + } > + > + src = io_mapping_map_atomic_wc(i915->gtt.mappable, node.start); > + dst = kmap_atomic(page); The wmb() barriers are here... > + memcpy_fromio(dst, src, PAGE_SIZE); ...and here. > + kunmap_atomic(dst); > + io_mapping_unmap_atomic(src); > + > + page_cache_release(page); > + } > + > + wmb(); > + i915->gtt.base.clear_range(>gtt.base, > +node.start, node.size, > +true); > + drm_mm_remove_node(); > + return ret; > +} > + > +/** > + * i915_gem_object_migrate_stolen_to_shmemfs() - migrates a stolen backed > + * object to shmemfs > + * @obj: stolen backed object to be migrated > + * > + * Returns: 0 on successful migration, errno on failure > + */ > + > +static int > +i915_gem_object_migrate_stolen_to_shmemfs(struct drm_i915_gem_object *obj) > +{ > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > + struct i915_vma *vma, *vn; > + struct file *file; > + struct address_space *mapping; > + struct sg_table *stolen_pages, *shmemfs_pages; > + int ret; > + > + if (WARN_ON_ONCE(i915_gem_object_needs_bit17_swizzle(obj))) > + return -EINVAL; > + > + ret = i915_gem_object_set_to_gtt_domain(obj, false); > + if (ret) > + return ret; This should be in copy_content. > + file = shmem_file_setup("drm mm object", obj->base.size, VM_NORESERVE); > + if (IS_ERR(file)) > + return PTR_ERR(file); > + mapping = i915_gem_set_inode_gfp(obj->base.dev, file); > + > + list_for_each_entry_safe(vma, vn, >vma_list, vma_link) > + if (i915_vma_unbind(vma)) > + continue; > + > + if (obj->madv != I915_MADV_WILLNEED && list_empty(>vma_list)) { > + /* Discard the stolen reservation, and replace with > + * an unpopulated shmemfs object. > + */ > + obj->madv = __I915_MADV_PURGED; > + goto swap_pages; A goto over one line? else? > + } > + > + ret = copy_content(obj, i915, mapping); > + if (ret) > + goto err_file; > + > +swap_pages: > + stolen_pages = obj->pages; > + obj->pages = NULL; > + > + obj->base.filp = file; > + obj->base.read_domains = I915_GEM_DOMAIN_CPU; > + obj->base.write_domain = I915_GEM_DOMAIN_CPU; Again, these domains are a result of copy_content. > + > + /* Recreate any pinned binding with pointers to the new storage */ > + if (!list_empty(>vma_list)) { > + ret = i915_gem_object_get_pages_gtt(obj); > + if (ret) { > + obj->pages = stolen_pages; > + goto err_file; > + } > + > + ret = i915_gem_object_set_to_gtt_domain(obj, true); Why? The pages are allocated, the domain is irrelevant (just so long as it is accurate, see above). > + obj->get_page.sg = obj->pages->sgl; > + obj->get_page.last = 0; > + > + list_for_each_entry(vma, >vma_list, vma_link) { > + if (!drm_mm_node_allocated(>node)) > + continue; > + > + WARN_ON(i915_vma_bind(vma, > + obj->cache_level, > + PIN_UPDATE)); > + } > + } else > + list_del(>global_list); This is very confusing (and wrong). This should only be a result of setting PURGED above. > + /* drop the stolen pin and backing */ > + shmemfs_pages = obj->pages; > + obj->pages = stolen_pages; > + > +
Re: [Intel-gfx] [PATCH V4 2/2] drm/i915: start adding dp mst audio
On Mon, 14 Dec 2015 11:34:53 +0100, Jani Nikula wrote: > > On Mon, 14 Dec 2015, Takashi Iwaiwrote: > > On Mon, 14 Dec 2015 10:33:44 +0100, > > Jani Nikula wrote: > >> > >> On Fri, 11 Dec 2015, Takashi Iwai wrote: > >> > On Fri, 11 Dec 2015 07:07:53 +0100, > >> > Libin Yang wrote: > >> >> > >> >> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c > >> >> >>> b/drivers/gpu/drm/i915/intel_audio.c > >> >> >>> index 9aa83e7..5ad2e66 100644 > >> >> >>> --- a/drivers/gpu/drm/i915/intel_audio.c > >> >> >>> +++ b/drivers/gpu/drm/i915/intel_audio.c > >> >> >>> @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct > >> >> >>> intel_encoder *encoder) > >> >> >>> tmp |= AUD_CONFIG_N_PROG_ENABLE; > >> >> >>> tmp &= ~AUD_CONFIG_UPPER_N_MASK; > >> >> >>> tmp &= ~AUD_CONFIG_LOWER_N_MASK; > >> >> >>> - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) > >> >> >>> + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || > >> >> >>> + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) > >> >> >>> tmp |= AUD_CONFIG_N_VALUE_INDEX; > >> > > >> > The same check is missing in hsw_audio_codec_enable()? > >> > > >> >> >>> I915_WRITE(HSW_AUD_CFG(pipe), tmp); > >> >> >>> > >> >> >>> @@ -474,7 +475,8 @@ static void ilk_audio_codec_enable(struct > >> >> >>> drm_connector *connector, > >> >> >>> tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > >> >> >>> tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > >> >> >>> tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; > >> >> >>> - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) > >> >> >>> + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || > >> >> >>> + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) > >> >> >>> tmp |= AUD_CONFIG_N_VALUE_INDEX; > >> > > >> > ... and missing for ilk_audio_codec_disable()? > >> > > >> > > >> >> >>> else > >> >> >>> tmp |= audio_config_hdmi_pixel_clock(adjusted_mode); > >> >> >>> @@ -512,7 +514,8 @@ void intel_audio_codec_enable(struct > >> >> >>> intel_encoder *intel_encoder) > >> >> >>> > >> >> >>> /* ELD Conn_Type */ > >> >> >>> connector->eld[5] &= ~(3 << 2); > >> >> >>> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) > >> >> >>> + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || > >> >> >>> + intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST)) > >> > > >> > IMO, it's better to have a macro to cover this two-line check instead > >> > of open-coding at each place. We'll have 5 places in the end. > >> > >> I don't think that's necessary. Only the hsw_* functions need it, the > >> other platforms (using the other functions) don't support DP MST. > > > > Then the patch changes the functions. And still three places have > > this check. > > Probably the mistake in the patch was to add the check to > ilk_audio_codec_enable when it should have been added to > hsw_audio_codec_enable. Yeah, I wanted to write "the patch changes the wrong functions", too. Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT
On Mon, 14 Dec 2015, Chris Wilsonwrote: > On Mon, Dec 14, 2015 at 12:50:54PM +0200, Jani Nikula wrote: >> In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, >> thus unavailable in i915_opregion, so add a separate file for the VBT. >> >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 22 ++ >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- >> 3 files changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index a9e1f18c36d1..aef1393e707f 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1857,6 +1857,27 @@ out: >> return 0; >> } >> >> +static int i915_vbt(struct seq_file *m, void *unused) >> +{ >> +struct drm_info_node *node = m->private; >> +struct drm_device *dev = node->minor->dev; >> +struct drm_i915_private *dev_priv = dev->dev_private; >> +struct intel_opregion *opregion = _priv->opregion; >> +int ret; >> + >> +ret = mutex_lock_interruptible(>struct_mutex); > > The contents of opregion->vbt are not serialised by struct_mutex, right? Cargo culting ftw. I figured it was maybe for suspend/resume paths, but that doesn't really make sense does it? BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/i915: Use insert_page for pwrite_fast
On Mon, Dec 14, 2015 at 10:48:51AM +, Chris Wilson wrote: > On Mon, Dec 14, 2015 at 09:54:06AM +, Chris Wilson wrote: > > On Mon, Dec 14, 2015 at 11:16:04AM +0530, ankitprasad.r.sha...@intel.com > > wrote: > > > From: Ankitprasad Sharma> > > > > > In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First, > > > we try a nonblocking pin for the whole object (since that is fastest if > > > reused), then failing that we try to grab one page in the mappable > > > aperture. It also allows us to handle objects larger than the mappable > > > aperture (e.g. if we need to pwrite with vGPU restricting the aperture > > > to a measely 8MiB or something like that). > > > > > > v2: Pin pages before starting pwrite, Combined duplicate loops (Chris) > > > > > > v3: Combined loops based on local patch by Chris (Chris) > > > > > > v4: Added i915 wrapper function for drm_mm_insert_node_in_range (Chris) > > > > > > Signed-off-by: Ankitprasad Sharma > > > Signed-off-by: Chris Wilson > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 86 > > > ++--- > > > 1 file changed, 64 insertions(+), 22 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > > b/drivers/gpu/drm/i915/i915_gem.c > > > index bf7f203..46c1e75 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -61,6 +61,21 @@ static bool cpu_write_needs_clflush(struct > > > drm_i915_gem_object *obj) > > > return obj->pin_display; > > > } > > > > > > +static int > > > +i915_gem_insert_node_in_range(struct drm_i915_private *i915, > > > + struct drm_mm_node *node, u64 size, > > > + unsigned alignment, u64 start, u64 end) > > > +{ > > > + int ret; > > > + > > > + ret = drm_mm_insert_node_in_range_generic(>gtt.base.mm, node, > > > + size, alignment, 0, start, > > > + end, DRM_MM_SEARCH_DEFAULT, > > > + DRM_MM_SEARCH_DEFAULT); > > > + > > > + return ret; > > > +} > > > > No. It encodes a very bad assumption (i915->gtt) that is not made clear > > in anyway. > > static int > insert_mappable_node(struct drm_i915_private *i915, >struct drm_mm_node *node) > { > return drm_mm_insert_node_in_range_generic(>gtt.base.mm, node, > 4096, 0, 0, > 0, i915->gtt.mappable_end, > DRM_MM_SEARCH_DEFAULT, > DRM_MM_SEARCH_DEFAULT); DRM_MM_SEARCH_DEFAULT, DRM_MM_CREATE_DEFAULT -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/13] android/sync: Fix reversed sense of signaled fence
On 11/12/2015 15:57, Tvrtko Ursulin wrote: On 11/12/15 13:11, john.c.harri...@intel.com wrote: From: Peter LawthersIn the 3.14 kernel, a signaled fence was indicated by the status field == 1. In 4.x, a status == 0 indicates signaled, status < 0 indicates error, and status > 0 indicates active. This patch wraps the check for a signaled fence in a function so that callers no longer needs to know the underlying implementation. v3: New patch for series. Change-Id: I8e565e49683e3efeb9474656cd84cf4add6ad6a2 Tracked-On: https://jira01.devtools.intel.com/browse/ACD-308 Signed-off-by: Peter Lawthers --- drivers/android/sync.h | 21 + 1 file changed, 21 insertions(+) diff --git a/drivers/android/sync.h b/drivers/android/sync.h index d57fa0a..75532d8 100644 --- a/drivers/android/sync.h +++ b/drivers/android/sync.h @@ -345,6 +345,27 @@ int sync_fence_cancel_async(struct sync_fence *fence, */ int sync_fence_wait(struct sync_fence *fence, long timeout); +/** + * sync_fence_is_signaled() - Return an indication if the fence is signaled + * @fence:fence to check + * + * returns 1 if fence is signaled + * returns 0 if fence is not signaled + * returns < 0 if fence is in error state + */ +static inline int +sync_fence_is_signaled(struct sync_fence *fence) +{ +int status; + +status = atomic_read(>status); +if (status == 0) +return 1; +if (status > 0) +return 0; +return status; +} Not so important but could simply return bool, like "return status <= 0"? Since it is called "is_signaled" and it is only used in boolean mode in future patches. There is no point in throwing away the error code unnecessarily. It can be useful in debug output and indeed will show up the scheduler status dump via debugfs. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo
On Mon, Dec 14, 2015 at 01:16:47PM +0200, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > The vma may have been rebound between the last time the cursor was > enabled and now, so skipping the cursor gtt offset deduction is not > safe unless we would also reset cursor_bo to NULL when disabling the > cursor. Just thow cursor_bo to the bin instead since it's lost all > other uses thanks to universal plane support. You could also mention that since updating the cursor is so slow now through the universal-plane support, that a fast path to avoid a few writes/checks is also irrelevant. -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] [RFC libdrm] intel: Add support for softpin
On 14 December 2015 at 09:04, Daniel Vetterwrote: > On Mon, Dec 14, 2015 at 08:41:05AM +, Song, Ruiling wrote: >> >> >> > -Original Message- >> > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel >> > Vetter >> > Sent: Monday, December 14, 2015 4:28 PM >> > To: Song, Ruiling >> > Cc: k...@bitplanet.net; Winiarski, Michal ; >> > mesa-...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Ben >> > Widawsky ; dri-de...@lists.freedesktop.org >> > Subject: Re: [Intel-gfx] [RFC libdrm] intel: Add support for softpin >> > >> > On Mon, Dec 14, 2015 at 07:24:29AM +, Song, Ruiling wrote: >> > > >> > > >> > > > -Original Message- >> > > > From: hoegsb...@gmail.com [mailto:hoegsb...@gmail.com] On Behalf >> > Of >> > > > Kristian H?gsberg >> > > > Sent: Monday, December 14, 2015 1:34 PM >> > > > To: Song, Ruiling >> > > > Cc: Winiarski, Michal ; intel- >> > > > g...@lists.freedesktop.org; mesa-...@lists.freedesktop.org; Ben >> > Widawsky >> > > > ; dri-de...@lists.freedesktop.org >> > > > Subject: Re: [Intel-gfx] [RFC libdrm] intel: Add support for softpin >> > > > >> > > > On Sun, Dec 13, 2015 at 7:17 PM, Song, Ruiling >> > > > wrote: >> > > > >> -Original Message- >> > > > >> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On >> > > > Behalf >> > > > >> Of Micha? Winiarski >> > > > >> Sent: Wednesday, September 9, 2015 10:07 PM >> > > > >> To: intel-gfx@lists.freedesktop.org >> > > > >> Cc: Ben Widawsky ; dri- >> > > > de...@lists.freedesktop.org; >> > > > >> mesa-...@lists.freedesktop.org >> > > > >> Subject: [Intel-gfx] [RFC libdrm] intel: Add support for softpin >> > > > >> >> > > > >> Softpin allows userspace to take greater control of GPU virtual >> > > > >> address >> > > > >> space and eliminates the need of relocations. It can also be used to >> > > > >> mirror addresses between GPU and CPU (shared virtual memory). >> > > > >> Calls to drm_intel_bo_emit_reloc are still required to build the >> > > > >> list of >> > > > >> drm_i915_gem_exec_objects at exec time, but no entries in relocs are >> > > > >> created. Self-relocs don't make any sense for softpinned objects and >> > can >> > > > >> indicate a programming errors, thus are forbidden. Softpinned >> > > > >> objects >> > > > >> are marked by asterisk in debug dumps. >> > > > >> >> > > > >> Cc: Thomas Daniel >> > > > >> Cc: Kristian Høgsberg >> > > > >> Cc: Zou Nanhai >> > > > >> Cc: Michel Thierry >> > > > >> Cc: Ben Widawsky >> > > > >> Cc: Chris Wilson >> > > > >> Signed-off-by: Michał Winiarski >> > > > >> --- >> > > > >> include/drm/i915_drm.h| 4 +- >> > > > >> intel/intel_bufmgr.c | 9 +++ >> > > > >> intel/intel_bufmgr.h | 1 + >> > > > >> intel/intel_bufmgr_gem.c | 176 >> > > > >> -- >> > > > >> intel/intel_bufmgr_priv.h | 7 ++ >> > > > >> 5 files changed, 173 insertions(+), 24 deletions(-) >> > > > > >> > > > > Will anybody help to push the patch to libdrm? Beignet highly depend >> > on >> > > > this to implement ocl2.0 svm. >> > > > >> > > > Is the kernel patch upstream? >> > > >> > > Yes, the kernel patch already merged, see: >> > > http://cgit.freedesktop.org/drm- >> > intel/commit/?id=506a8e87d8d2746b9e9d2433503fe237c54e4750 >> > > >> > > I find below line of code in libdrm does not match the kernel version. >> > > The >> > kernel patch defined as: >> > > "#define EXEC_OBJECT_PINNED (1<<4)", but this patch defined it as (1<<3). >> > >> > Please always regenerate the entire headers from the kernel sources using >> > >> > $ make headers_install >> > >> > Then copy the headers from the kernel's usr/include/drm to libdrm. Never >> > patch i915_drm.h manually. >> >> Thanks for the info. But the problem is libdrm still tracks such kind of >> header files. >> Should this kind of header file be removed from libdrm? Or any document in >> libdrm to make this explicit? > > All other kernel headers are extracted from the kernel directly, but > generally those packages only get update when a new kernel comes out. > Usually it's called linux-headers or similar. And only updating headers > when the release is out is _way_ too slow for drm/gfx. That's why we have > drm headers in libdrm. > That plus hysterical raisins, from when drm was part of userspace ;-) > But yeah we should document this, maybe even script it. Perhaps even just > upgrade headers automatically as soon as the upstream drm-next branch > changes. I'm all for tweaking the make target, although automating to the point of zero developer interaction might not be ideal.
Re: [Intel-gfx] [PATCH 13/13] drm/i915: Cache last IRQ seqno to reduce IRQ overhead
On 11/12/2015 14:28, Tvrtko Ursulin wrote: On 11/12/15 13:12, john.c.harri...@intel.com wrote: From: John HarrisonThe notify function can be called many times without the seqno changing. A large number of duplicates are to prevent races due to the requirement of not enabling interrupts until requested. However, when interrupts are enabled the IRQ handle can be called multiple times without the ring's seqno value changing. This patch reduces the overhead of these extra calls by caching the last processed seqno value and early exiting if it has not changed. v3: New patch for series. For: VIZ-5190 Signed-off-by: John Harrison --- drivers/gpu/drm/i915/i915_gem.c | 14 +++--- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 279d79f..3c88678 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2457,6 +2457,8 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno) for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_seqno); j++) ring->semaphore.sync_seqno[j] = 0; + +ring->last_irq_seqno = 0; } return 0; @@ -2788,11 +2790,14 @@ void i915_gem_request_notify(struct intel_engine_cs *ring, bool fence_locked) return; } -if (!fence_locked) -spin_lock_irqsave(>fence_lock, flags); - seqno = ring->get_seqno(ring, false); trace_i915_gem_request_notify(ring, seqno); +if (seqno == ring->last_irq_seqno) +return; +ring->last_irq_seqno = seqno; Hmmm.. do you want to make the check "seqno <= ring->last_irq_seqno" ? Is there a possibility for some weird timing or caching issue where two callers get in and last_irq_seqno goes backwards? Not sure that it would cause a problem, but pattern is unusual and hard to understand for me. The check is simply to prevent repeat processing of identical seqno values. The 'last_' value is never used for anything more complicated. If there is a very rare race condition where the repeat processing can still happen, it doesn't really matter too much. Also check and the assignment would need to be under the spinlock I think. The whole point is to not grab the spinlock if there is no work to do. Hence the seqno read and test must be done first. The assignment could potentially be done after the lock but if two different threads have made it that far concurrently then it doesn't really matter who does the write first. Most likely they are both processing the same seqno and in the really rare case of two concurrent threads actually reading two different (and both new) seqno values then there is no guarantee about which will take the lock first. So you are into the above situation of it doesn't really matter if there is then a third time around later that finds an 'incorrect' last value and goes through the processing sequence but with no work to do. + +if (!fence_locked) +spin_lock_irqsave(>fence_lock, flags); list_for_each_entry_safe(req, req_next, >fence_signal_list, signal_link) { if (!req->cancelled) { @@ -3163,7 +3168,10 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, * Tidy up anything left over. This includes a call to * i915_gem_request_notify() which will make sure that any requests * that were on the signal pending list get also cleaned up. + * NB: The seqno cache must be cleared otherwise the notify call will + * simply return immediately. */ +ring->last_irq_seqno = 0; i915_gem_retire_requests_ring(ring); /* Having flushed all requests from all queues, we know that all diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 9d09edb..1987abd 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -356,6 +356,7 @@ struct intel_engine_cs { spinlock_t fence_lock; struct list_head fence_signal_list; struct list_head fence_unsignal_list; +uint32_t last_irq_seqno; }; bool intel_ring_initialized(struct intel_engine_cs *ring); Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC libdrm] intel: Add support for softpin
On Mon, Dec 14, 2015 at 08:41:05AM +, Song, Ruiling wrote: > > > > -Original Message- > > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Monday, December 14, 2015 4:28 PM > > To: Song, Ruiling> > Cc: k...@bitplanet.net; Winiarski, Michal ; > > mesa-...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Ben > > Widawsky ; dri-de...@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [RFC libdrm] intel: Add support for softpin > > > > On Mon, Dec 14, 2015 at 07:24:29AM +, Song, Ruiling wrote: > > > > > > > > > > -Original Message- > > > > From: hoegsb...@gmail.com [mailto:hoegsb...@gmail.com] On Behalf > > Of > > > > Kristian H?gsberg > > > > Sent: Monday, December 14, 2015 1:34 PM > > > > To: Song, Ruiling > > > > Cc: Winiarski, Michal ; intel- > > > > g...@lists.freedesktop.org; mesa-...@lists.freedesktop.org; Ben > > Widawsky > > > > ; dri-de...@lists.freedesktop.org > > > > Subject: Re: [Intel-gfx] [RFC libdrm] intel: Add support for softpin > > > > > > > > On Sun, Dec 13, 2015 at 7:17 PM, Song, Ruiling > > > > wrote: > > > > >> -Original Message- > > > > >> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On > > > > Behalf > > > > >> Of Micha? Winiarski > > > > >> Sent: Wednesday, September 9, 2015 10:07 PM > > > > >> To: intel-gfx@lists.freedesktop.org > > > > >> Cc: Ben Widawsky ; dri- > > > > de...@lists.freedesktop.org; > > > > >> mesa-...@lists.freedesktop.org > > > > >> Subject: [Intel-gfx] [RFC libdrm] intel: Add support for softpin > > > > >> > > > > >> Softpin allows userspace to take greater control of GPU virtual > > > > >> address > > > > >> space and eliminates the need of relocations. It can also be used to > > > > >> mirror addresses between GPU and CPU (shared virtual memory). > > > > >> Calls to drm_intel_bo_emit_reloc are still required to build the > > > > >> list of > > > > >> drm_i915_gem_exec_objects at exec time, but no entries in relocs are > > > > >> created. Self-relocs don't make any sense for softpinned objects and > > can > > > > >> indicate a programming errors, thus are forbidden. Softpinned objects > > > > >> are marked by asterisk in debug dumps. > > > > >> > > > > >> Cc: Thomas Daniel > > > > >> Cc: Kristian Høgsberg > > > > >> Cc: Zou Nanhai > > > > >> Cc: Michel Thierry > > > > >> Cc: Ben Widawsky > > > > >> Cc: Chris Wilson > > > > >> Signed-off-by: Michał Winiarski > > > > >> --- > > > > >> include/drm/i915_drm.h| 4 +- > > > > >> intel/intel_bufmgr.c | 9 +++ > > > > >> intel/intel_bufmgr.h | 1 + > > > > >> intel/intel_bufmgr_gem.c | 176 > > > > >> -- > > > > >> intel/intel_bufmgr_priv.h | 7 ++ > > > > >> 5 files changed, 173 insertions(+), 24 deletions(-) > > > > > > > > > > Will anybody help to push the patch to libdrm? Beignet highly depend > > on > > > > this to implement ocl2.0 svm. > > > > > > > > Is the kernel patch upstream? > > > > > > Yes, the kernel patch already merged, see: > > > http://cgit.freedesktop.org/drm- > > intel/commit/?id=506a8e87d8d2746b9e9d2433503fe237c54e4750 > > > > > > I find below line of code in libdrm does not match the kernel version. The > > kernel patch defined as: > > > "#define EXEC_OBJECT_PINNED (1<<4)", but this patch defined it as (1<<3). > > > > Please always regenerate the entire headers from the kernel sources using > > > > $ make headers_install > > > > Then copy the headers from the kernel's usr/include/drm to libdrm. Never > > patch i915_drm.h manually. > > Thanks for the info. But the problem is libdrm still tracks such kind of > header files. > Should this kind of header file be removed from libdrm? Or any document in > libdrm to make this explicit? All other kernel headers are extracted from the kernel directly, but generally those packages only get update when a new kernel comes out. Usually it's called linux-headers or similar. And only updating headers when the release is out is _way_ too slow for drm/gfx. That's why we have drm headers in libdrm. But yeah we should document this, maybe even script it. Perhaps even just upgrade headers automatically as soon as the upstream drm-next branch changes. -Daniel > > Thanks! > Ruiling > > > Thanks, Daniel > > > > > > > > Hello Michal, > > > > > > Could you help to rebase the patch against: > > > [Intel-gfx] [PATCH libdrm v4 0/2] 48-bit virtual address support in > > > i915 > > > I think we need both 48bit & softpin in libdrm. > > > > > > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h > > > index ded43b1..2b99fc6
Re: [Intel-gfx] [PATCH V4 2/2] drm/i915: start adding dp mst audio
On Fri, 11 Dec 2015, Takashi Iwaiwrote: > On Fri, 11 Dec 2015 07:07:53 +0100, > Libin Yang wrote: >> >> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c >> >>> b/drivers/gpu/drm/i915/intel_audio.c >> >>> index 9aa83e7..5ad2e66 100644 >> >>> --- a/drivers/gpu/drm/i915/intel_audio.c >> >>> +++ b/drivers/gpu/drm/i915/intel_audio.c >> >>> @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct >> >>> intel_encoder *encoder) >> >>> tmp |= AUD_CONFIG_N_PROG_ENABLE; >> >>> tmp &= ~AUD_CONFIG_UPPER_N_MASK; >> >>> tmp &= ~AUD_CONFIG_LOWER_N_MASK; >> >>> -if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) >> >>> +if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || >> >>> +intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) >> >>> tmp |= AUD_CONFIG_N_VALUE_INDEX; > > The same check is missing in hsw_audio_codec_enable()? > >> >>> I915_WRITE(HSW_AUD_CFG(pipe), tmp); >> >>> >> >>> @@ -474,7 +475,8 @@ static void ilk_audio_codec_enable(struct >> >>> drm_connector *connector, >> >>> tmp &= ~AUD_CONFIG_N_VALUE_INDEX; >> >>> tmp &= ~AUD_CONFIG_N_PROG_ENABLE; >> >>> tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; >> >>> -if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) >> >>> +if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || >> >>> +intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) >> >>> tmp |= AUD_CONFIG_N_VALUE_INDEX; > > ... and missing for ilk_audio_codec_disable()? > > >> >>> else >> >>> tmp |= audio_config_hdmi_pixel_clock(adjusted_mode); >> >>> @@ -512,7 +514,8 @@ void intel_audio_codec_enable(struct intel_encoder >> >>> *intel_encoder) >> >>> >> >>> /* ELD Conn_Type */ >> >>> connector->eld[5] &= ~(3 << 2); >> >>> -if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) >> >>> +if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || >> >>> +intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST)) > > IMO, it's better to have a macro to cover this two-line check instead > of open-coding at each place. We'll have 5 places in the end. I don't think that's necessary. Only the hsw_* functions need it, the other platforms (using the other functions) don't support DP MST. BR, Jani. > > > thanks, > > Takashi -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v3] gem_flink_race/prime_self_import: Improve test reliability
gem_flink_race and prime_self_import have subtests which read the number of open gem objects from debugfs to determine if objects have leaked during the test. However the test can fail sporadically if the number of gem objects changes due to other process activity. This patch introduces a change to check the number of gem objects several times to filter out any fluctuations. v2: Moved the common code to a library and made the loop android specific (Daniel Vetter) v3: Renamed get_stable_obj_count -> igt_get_stable_obj_count Signed-off-by: Derek Morton--- lib/igt_debugfs.c | 58 +++ lib/igt_debugfs.h | 6 + tests/gem_flink_race.c| 25 +++- tests/prime_self_import.c | 31 + 4 files changed, 73 insertions(+), 47 deletions(-) diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c index 2c3b1cf..4322e8e 100644 --- a/lib/igt_debugfs.c +++ b/lib/igt_debugfs.c @@ -667,3 +667,61 @@ void igt_enable_prefault(void) { igt_prefault_control(true); } + +static int get_object_count(void) +{ + FILE *file; + int ret, scanned; + + igt_drop_caches_set(DROP_RETIRE | DROP_ACTIVE); + + file = igt_debugfs_fopen("i915_gem_objects", "r"); + + scanned = fscanf(file, "%i objects", ); + igt_assert_eq(scanned, 1); + + return ret; +} + +/** + * igt_get_stable_obj_count: + * @driver: fd to drm/i915 GEM driver + * + * This puts the driver into a stable (quiescent) state and then returns the + * current number of gem buffer objects as reported in the i915_gem_objects + * debugFS interface. + */ +int igt_get_stable_obj_count(int driver) +{ + int obj_count; + gem_quiescent_gpu(driver); + obj_count = get_object_count(); + /* The test relies on the system being in the same state before and +* after the test so any difference in the object count is a result of +* leaks during the test. gem_quiescent_gpu() mostly achieves this but +* on android occasionally obj_count can still change briefly. +* The loop ensures obj_count has remained stable over several checks +*/ +#ifdef ANDROID + { + int loop_count = 0; + int prev_obj_count = obj_count; + while (loop_count < 4) { + usleep(20); + gem_quiescent_gpu(driver); + obj_count = get_object_count(); + if (obj_count == prev_obj_count) { + loop_count++; + } else { + igt_debug("loop_count=%d, obj_count=%d, prev_obj_count=%d\n", + loop_count, obj_count, prev_obj_count); + loop_count = 0; + prev_obj_count = obj_count; + } + + } + } +#endif + return obj_count; +} + diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h index bbf7f69..24018eb 100644 --- a/lib/igt_debugfs.h +++ b/lib/igt_debugfs.h @@ -165,4 +165,10 @@ void igt_drop_caches_set(uint64_t val); void igt_disable_prefault(void); void igt_enable_prefault(void); +/* + * Put the driver into a stable (quiescent) state and get the current number of + * gem buffer objects + */ +int igt_get_stable_obj_count(int driver); + #endif /* __IGT_DEBUGFS_H__ */ diff --git a/tests/gem_flink_race.c b/tests/gem_flink_race.c index b17ef85..30e33f6 100644 --- a/tests/gem_flink_race.c +++ b/tests/gem_flink_race.c @@ -44,28 +44,11 @@ IGT_TEST_DESCRIPTION("Check for flink/open vs. gem close races."); * in the flink name and corresponding reference getting leaked. */ -/* We want lockless and I'm to lazy to dig out an atomic libarary. On x86 this +/* We want lockless and I'm to lazy to dig out an atomic library. On x86 this * works, too. */ volatile int pls_die = 0; int fd; -static int get_object_count(void) -{ - FILE *file; - int scanned, ret; - - igt_drop_caches_set(DROP_RETIRE | DROP_ACTIVE); - - file = igt_debugfs_fopen("i915_gem_objects", "r"); - - scanned = fscanf(file, "%i objects", ); - igt_assert_eq(scanned, 1); - igt_debug("Reports %d objects\n", ret); - - return ret; -} - - static void *thread_fn_flink_name(void *p) { struct drm_gem_open open_struct; @@ -164,8 +147,7 @@ static void test_flink_close(void) * up the counts */ fake = drm_open_driver(DRIVER_INTEL); - gem_quiescent_gpu(fake); - obj_count = get_object_count(); + obj_count = igt_get_stable_obj_count(fake); num_threads = sysconf(_SC_NPROCESSORS_ONLN); @@ -190,8 +172,7 @@ static void test_flink_close(void) close(fd); - gem_quiescent_gpu(fake); - obj_count = get_object_count() - obj_count; + obj_count = igt_get_stable_obj_count(fake) - obj_count;
Re: [Intel-gfx] [PATCH V4 2/2] drm/i915: start adding dp mst audio
On Mon, 14 Dec 2015, Takashi Iwaiwrote: > On Mon, 14 Dec 2015 10:33:44 +0100, > Jani Nikula wrote: >> >> On Fri, 11 Dec 2015, Takashi Iwai wrote: >> > On Fri, 11 Dec 2015 07:07:53 +0100, >> > Libin Yang wrote: >> >> >> >> >>> diff --git a/drivers/gpu/drm/i915/intel_audio.c >> >> >>> b/drivers/gpu/drm/i915/intel_audio.c >> >> >>> index 9aa83e7..5ad2e66 100644 >> >> >>> --- a/drivers/gpu/drm/i915/intel_audio.c >> >> >>> +++ b/drivers/gpu/drm/i915/intel_audio.c >> >> >>> @@ -262,7 +262,8 @@ static void hsw_audio_codec_disable(struct >> >> >>> intel_encoder *encoder) >> >> >>> tmp |= AUD_CONFIG_N_PROG_ENABLE; >> >> >>> tmp &= ~AUD_CONFIG_UPPER_N_MASK; >> >> >>> tmp &= ~AUD_CONFIG_LOWER_N_MASK; >> >> >>> - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) >> >> >>> + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || >> >> >>> + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) >> >> >>> tmp |= AUD_CONFIG_N_VALUE_INDEX; >> > >> > The same check is missing in hsw_audio_codec_enable()? >> > >> >> >>> I915_WRITE(HSW_AUD_CFG(pipe), tmp); >> >> >>> >> >> >>> @@ -474,7 +475,8 @@ static void ilk_audio_codec_enable(struct >> >> >>> drm_connector *connector, >> >> >>> tmp &= ~AUD_CONFIG_N_VALUE_INDEX; >> >> >>> tmp &= ~AUD_CONFIG_N_PROG_ENABLE; >> >> >>> tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; >> >> >>> - if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) >> >> >>> + if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) || >> >> >>> + intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST)) >> >> >>> tmp |= AUD_CONFIG_N_VALUE_INDEX; >> > >> > ... and missing for ilk_audio_codec_disable()? >> > >> > >> >> >>> else >> >> >>> tmp |= audio_config_hdmi_pixel_clock(adjusted_mode); >> >> >>> @@ -512,7 +514,8 @@ void intel_audio_codec_enable(struct >> >> >>> intel_encoder *intel_encoder) >> >> >>> >> >> >>> /* ELD Conn_Type */ >> >> >>> connector->eld[5] &= ~(3 << 2); >> >> >>> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) >> >> >>> + if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) || >> >> >>> + intel_pipe_has_type(crtc, INTEL_OUTPUT_DP_MST)) >> > >> > IMO, it's better to have a macro to cover this two-line check instead >> > of open-coding at each place. We'll have 5 places in the end. >> >> I don't think that's necessary. Only the hsw_* functions need it, the >> other platforms (using the other functions) don't support DP MST. > > Then the patch changes the functions. And still three places have > this check. Probably the mistake in the patch was to add the check to ilk_audio_codec_enable when it should have been added to hsw_audio_codec_enable. BR, Jani. > > > thanks, > > Takashi -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/11] drm/i915/bios: rename intel_parse_bios to intel_bios_init
While at it, move the declaration to where everything else is declared. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/intel_bios.c | 4 ++-- drivers/gpu/drm/i915/intel_bios.h | 2 -- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 84e2b202ecb5..9cb71b6aed5d 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -370,7 +370,7 @@ static int i915_load_modeset_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret; - ret = intel_parse_bios(dev); + ret = intel_bios_init(dev); if (ret) DRM_INFO("failed to find VBIOS tables\n"); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 912408539822..a2bd4c6a86a1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3352,6 +3352,9 @@ static inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter) } extern void intel_i2c_reset(struct drm_device *dev); +/* intel_bios.c */ +int intel_bios_init(struct drm_device *dev); + /* intel_opregion.c */ #ifdef CONFIG_ACPI extern int intel_opregion_setup(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 45a90d4a4a81..f2b79b7d12b8 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1282,7 +1282,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) } /** - * intel_parse_bios - find VBT and initialize settings from the BIOS + * intel_bios_init - find VBT and initialize settings from the BIOS * @dev: DRM device * * Loads the Video BIOS and checks that the VBT exists. Sets scratch registers @@ -1291,7 +1291,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) * Returns 0 on success, nonzero on failure. */ int -intel_parse_bios(struct drm_device *dev) +intel_bios_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct pci_dev *pdev = dev->pdev; diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index 7ec8c9aefb84..689eb42f863c 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -588,8 +588,6 @@ struct bdb_psr { struct psr_table psr_table[16]; } __packed; -int intel_parse_bios(struct drm_device *dev); - /* * Driver<->VBIOS interaction occurs through scratch bits in * GR18 & SWF*. -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file
Hasn't been necessary since commit 115719fceaa733d646e39cdce83cc32ddb891a49 Author: Williams, Dan JDate: Mon Oct 12 21:12:57 2015 + i915: switch from acpi_os_ioremap to memremap Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_debugfs.c | 11 ++- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 24318b79bcfc..a9e1f18c36d1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1842,25 +1842,18 @@ static int i915_opregion(struct seq_file *m, void *unused) struct drm_device *dev = node->minor->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_opregion *opregion = _priv->opregion; - void *data = kmalloc(OPREGION_SIZE, GFP_KERNEL); int ret; - if (data == NULL) - return -ENOMEM; - ret = mutex_lock_interruptible(>struct_mutex); if (ret) goto out; - if (opregion->header) { - memcpy(data, opregion->header, OPREGION_SIZE); - seq_write(m, data, OPREGION_SIZE); - } + if (opregion->header) + seq_write(m, opregion->header, OPREGION_SIZE); mutex_unlock(>struct_mutex); out: - kfree(data); return 0; } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB
The RVDA and RVDS (raw VBT data address and size) fields of the ASLE mailbox may specify an alternate location for VBT instead of mailbox #4. Use the alternate location if available and valid, falling back to mailbox #4 otherwise. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_opregion.c | 25 +++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ca8c2a64bc6d..8cfac7398568 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -457,6 +457,7 @@ struct intel_opregion { u32 swsci_gbda_sub_functions; u32 swsci_sbcb_sub_functions; struct opregion_asle *asle; + void *rvda; const void *vbt; u32 vbt_size; u32 *lid_state; diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index e89ee2383fe1..a139889dd45b 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev) /* just clear all opregion memory pointers now */ memunmap(opregion->header); + if (opregion->rvda) { + memunmap(opregion->rvda); + opregion->rvda = NULL; + } opregion->header = NULL; opregion->acpi = NULL; opregion->swsci = NULL; @@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev) DRM_DEBUG_DRIVER("ASLE extension supported\n"); if (!dmi_check_system(intel_no_opregion_vbt)) { - const void *vbt = base + OPREGION_VBT_OFFSET; - u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; + const void *vbt = NULL; + u32 vbt_size = 0; + + if (opregion->header->opregion_ver >= 2 && opregion->asle && + opregion->asle->rvda && opregion->asle->rvds) { + opregion->rvda = memremap(opregion->asle->rvda, + opregion->asle->rvds, + MEMREMAP_WB); + vbt = opregion->rvda; + vbt_size = opregion->asle->rvds; + } if (intel_bios_is_valid_vbt(vbt, vbt_size)) { opregion->vbt = vbt; opregion->vbt_size = vbt_size; + DRM_DEBUG_DRIVER("VBT from RVDA\n"); + } else { + vbt = base + OPREGION_VBT_OFFSET; + vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; + if (intel_bios_is_valid_vbt(vbt, vbt_size)) { + opregion->vbt = vbt; + opregion->vbt_size = vbt_size; + } } } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/11] drm/i915/opregion: make VBT pointer a const
Because we can. It's not to be touched so tell the compiler too. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_opregion.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ef15f1710b50..10e47167c594 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -457,7 +457,7 @@ struct intel_opregion { u32 swsci_gbda_sub_functions; u32 swsci_sbcb_sub_functions; struct opregion_asle *asle; - void *vbt; + const void *vbt; u32 *lid_state; struct work_struct asle_work; }; diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 3c76a8684ff2..f9403b1c8fdd 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -987,7 +987,7 @@ int intel_opregion_setup(struct drm_device *dev) DRM_DEBUG_DRIVER("ASLE extension supported\n"); if (!dmi_check_system(intel_no_opregion_vbt)) { - void *vbt = base + OPREGION_VBT_OFFSET; + const void *vbt = base + OPREGION_VBT_OFFSET; u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; if (intel_bios_is_valid_vbt(vbt, vbt_size)) -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/11] drm/i915/bios: move debug logging about VBT source to intel_parse_bios()
The decision about which source will be used for VBT is done in intel_parse_bios(), not in the VBT validation function. Make the VBT validation function strictly about validation, and move the debug logging to where it logically belongs. This will make even more sense in the future when the validation for ACPI OpRegion based VBT takes place at OpRegion setup time. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/intel_bios.c | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 2fc2a994f395..45a90d4a4a81 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1223,8 +1223,7 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt) static const struct vbt_header *validate_vbt(const void *base, size_t size, -const void *_vbt, -const char *source) +const void *_vbt) { size_t offset = _vbt - base; const struct vbt_header *vbt = _vbt; @@ -1255,8 +1254,6 @@ static const struct vbt_header *validate_vbt(const void *base, return NULL; } - DRM_DEBUG_KMS("Using VBT from %s: %20s\n", - source, vbt->signature); return vbt; } @@ -1276,7 +1273,7 @@ static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) */ void *_bios = (void __force *) bios; - vbt = validate_vbt(_bios, size, _bios + i, "PCI ROM"); + vbt = validate_vbt(_bios, size, _bios + i); break; } } @@ -1309,8 +1306,11 @@ intel_parse_bios(struct drm_device *dev) /* XXX Should this validation be moved to intel_opregion.c? */ vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE, - dev_priv->opregion.vbt, "OpRegion"); - if (!vbt) { + dev_priv->opregion.vbt); + if (vbt) { + DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n", + vbt->signature); + } else { size_t size; bios = pci_map_rom(pdev, ); @@ -1322,6 +1322,9 @@ intel_parse_bios(struct drm_device *dev) pci_unmap_rom(pdev, bios); return -1; } + + DRM_DEBUG_KMS("Using VBT from PCI ROM: %20s\n", + vbt->signature); } bdb = get_bdb_header(vbt); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/11] drm/i915: refactor VBT validation
Make the validation function a boolean operating on a buffer of given size, removing the extra pointer dances. Move the OpRegion based VBT validation to intel_opregion_setup(), only initializing opregion->vbt if it's valid. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 63 ++- drivers/gpu/drm/i915/intel_opregion.c | 9 +++-- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a2bd4c6a86a1..ef15f1710b50 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev); /* intel_bios.c */ int intel_bios_init(struct drm_device *dev); +bool intel_bios_is_valid_vbt(const void *buf, size_t size); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index f2b79b7d12b8..007b2b759600 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt) return _vbt + vbt->bdb_offset; } -static const struct vbt_header *validate_vbt(const void *base, -size_t size, -const void *_vbt) +/** + * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT + * @buf: pointer to a buffer to validate + * @size: size of the buffer + * + * Returns true on valid VBT. + */ +bool intel_bios_is_valid_vbt(const void *buf, size_t size) { - size_t offset = _vbt - base; - const struct vbt_header *vbt = _vbt; + const struct vbt_header *vbt = buf; const struct bdb_header *bdb; if (!vbt) - return NULL; + return false; - if (offset + sizeof(struct vbt_header) > size) { + if (sizeof(struct vbt_header) > size) { DRM_DEBUG_DRIVER("VBT header incomplete\n"); - return NULL; + return false; } if (memcmp(vbt->signature, "$VBT", 4)) { DRM_DEBUG_DRIVER("VBT invalid signature\n"); - return NULL; + return false; } - offset += vbt->bdb_offset; - if (offset + sizeof(struct bdb_header) > size) { + if (vbt->bdb_offset + sizeof(struct bdb_header) > size) { DRM_DEBUG_DRIVER("BDB header incomplete\n"); - return NULL; + return false; } bdb = get_bdb_header(vbt); - if (offset + bdb->bdb_size > size) { + if (vbt->bdb_offset + bdb->bdb_size > size) { DRM_DEBUG_DRIVER("BDB incomplete\n"); - return NULL; + return false; } return vbt; @@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base, static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) { - const struct vbt_header *vbt = NULL; size_t i; /* Scour memory looking for the VBT signature. */ for (i = 0; i + 4 < size; i++) { - if (ioread32(bios + i) == *((const u32 *) "$VBT")) { - /* -* This is the one place where we explicitly discard the -* address space (__iomem) of the BIOS/VBT. From now on -* everything is based on 'base', and treated as regular -* memory. -*/ - void *_bios = (void __force *) bios; + void *vbt; - vbt = validate_vbt(_bios, size, _bios + i); - break; - } + if (ioread32(bios + i) != *((const u32 *) "$VBT")) + continue; + + /* +* This is the one place where we explicitly discard the address +* space (__iomem) of the BIOS/VBT. +*/ + vbt = (void __force *) bios + i; + if (intel_bios_is_valid_vbt(vbt, size - i)) + return vbt; + + break; } - return vbt; + return NULL; } /** @@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct pci_dev *pdev = dev->pdev; - const struct vbt_header *vbt; + const struct vbt_header *vbt = dev_priv->opregion.vbt; const struct bdb_header *bdb; u8 __iomem *bios = NULL; @@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev) init_vbt_defaults(dev_priv); - /* XXX Should this validation be moved to intel_opregion.c? */ - vbt =
[Intel-gfx] [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT
In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, thus unavailable in i915_opregion, so add a separate file for the VBT. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/i915_debugfs.c | 22 ++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_opregion.c | 4 +++- 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index a9e1f18c36d1..aef1393e707f 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1857,6 +1857,27 @@ out: return 0; } +static int i915_vbt(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = m->private; + struct drm_device *dev = node->minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_opregion *opregion = _priv->opregion; + int ret; + + ret = mutex_lock_interruptible(>struct_mutex); + if (ret) + goto out; + + if (opregion->vbt) + seq_write(m, opregion->vbt, opregion->vbt_size); + + mutex_unlock(>struct_mutex); + +out: + return 0; +} + static int i915_gem_framebuffer_info(struct seq_file *m, void *data) { struct drm_info_node *node = m->private; @@ -5325,6 +5346,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_ips_status", i915_ips_status, 0}, {"i915_sr_status", i915_sr_status, 0}, {"i915_opregion", i915_opregion, 0}, + {"i915_vbt", i915_vbt, 0}, {"i915_gem_framebuffer", i915_gem_framebuffer_info, 0}, {"i915_context_status", i915_context_status, 0}, {"i915_dump_lrc", i915_dump_lrc, 0}, diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 10e47167c594..ca8c2a64bc6d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -458,6 +458,7 @@ struct intel_opregion { u32 swsci_sbcb_sub_functions; struct opregion_asle *asle; const void *vbt; + u32 vbt_size; u32 *lid_state; struct work_struct asle_work; }; diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index f9403b1c8fdd..e89ee2383fe1 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -990,8 +990,10 @@ int intel_opregion_setup(struct drm_device *dev) const void *vbt = base + OPREGION_VBT_OFFSET; u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; - if (intel_bios_is_valid_vbt(vbt, vbt_size)) + if (intel_bios_is_valid_vbt(vbt, vbt_size)) { opregion->vbt = vbt; + opregion->vbt_size = vbt_size; + } } return 0; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/11] drm/i915/opregion: make VBT size limit more strict
The VBT in OpRegion should fit in mailbox #4. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/intel_opregion.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 9f992e7c6185..3c76a8684ff2 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -988,7 +988,7 @@ int intel_opregion_setup(struct drm_device *dev) if (!dmi_check_system(intel_no_opregion_vbt)) { void *vbt = base + OPREGION_VBT_OFFSET; - u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET; + u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; if (intel_bios_is_valid_vbt(vbt, vbt_size)) opregion->vbt = vbt; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/11] drm/i915/bios: have functions return vbt, not bdb, header pointer
This will simplify further work. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/intel_bios.c | 41 +-- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 401e1141f55f..2fc2a994f395 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1214,7 +1214,14 @@ init_vbt_defaults(struct drm_i915_private *dev_priv) } } -static const struct bdb_header *validate_vbt(const void *base, +static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt) +{ + const void *_vbt = vbt; + + return _vbt + vbt->bdb_offset; +} + +static const struct vbt_header *validate_vbt(const void *base, size_t size, const void *_vbt, const char *source) @@ -1223,6 +1230,9 @@ static const struct bdb_header *validate_vbt(const void *base, const struct vbt_header *vbt = _vbt; const struct bdb_header *bdb; + if (!vbt) + return NULL; + if (offset + sizeof(struct vbt_header) > size) { DRM_DEBUG_DRIVER("VBT header incomplete\n"); return NULL; @@ -1239,7 +1249,7 @@ static const struct bdb_header *validate_vbt(const void *base, return NULL; } - bdb = base + offset; + bdb = get_bdb_header(vbt); if (offset + bdb->bdb_size > size) { DRM_DEBUG_DRIVER("BDB incomplete\n"); return NULL; @@ -1247,12 +1257,12 @@ static const struct bdb_header *validate_vbt(const void *base, DRM_DEBUG_KMS("Using VBT from %s: %20s\n", source, vbt->signature); - return bdb; + return vbt; } -static const struct bdb_header *find_vbt(void __iomem *bios, size_t size) +static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) { - const struct bdb_header *bdb = NULL; + const struct vbt_header *vbt = NULL; size_t i; /* Scour memory looking for the VBT signature. */ @@ -1266,12 +1276,12 @@ static const struct bdb_header *find_vbt(void __iomem *bios, size_t size) */ void *_bios = (void __force *) bios; - bdb = validate_vbt(_bios, size, _bios + i, "PCI ROM"); + vbt = validate_vbt(_bios, size, _bios + i, "PCI ROM"); break; } } - return bdb; + return vbt; } /** @@ -1288,7 +1298,8 @@ intel_parse_bios(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct pci_dev *pdev = dev->pdev; - const struct bdb_header *bdb = NULL; + const struct vbt_header *vbt; + const struct bdb_header *bdb; u8 __iomem *bios = NULL; if (HAS_PCH_NOP(dev)) @@ -1297,24 +1308,24 @@ intel_parse_bios(struct drm_device *dev) init_vbt_defaults(dev_priv); /* XXX Should this validation be moved to intel_opregion.c? */ - if (dev_priv->opregion.vbt) - bdb = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE, - dev_priv->opregion.vbt, "OpRegion"); - - if (bdb == NULL) { + vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE, + dev_priv->opregion.vbt, "OpRegion"); + if (!vbt) { size_t size; bios = pci_map_rom(pdev, ); if (!bios) return -1; - bdb = find_vbt(bios, size); - if (!bdb) { + vbt = find_vbt(bios, size); + if (!vbt) { pci_unmap_rom(pdev, bios); return -1; } } + bdb = get_bdb_header(vbt); + /* Grab useful general definitions */ parse_general_features(dev_priv, bdb); parse_general_definitions(dev_priv, bdb); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/11] drm/i915: VBT/opregion refactor to support >6 KB VBT
This series refactors the VBT and OpRegion code to support VBT bigger than 6 KB. It's plenty of small, hopefully easy to review steps. This series supersedes patches 2-6 of Deepak's series [1]. I took over because I would not have been able to adequately describe in review what I wanted done. I'll continue with rebasing the rest of the patches on top of this one. BR, Jani. [1] http://mid.gmane.org/1448923632-16760-1-git-send-email-m.dee...@intel.com Deepak M (1): drm/i915: Add Intel opregion mailbox 5 structure Jani Nikula (10): drm/i915: move "no VBT in opregion" quirk to intel_opregion_setup() drm/i915/bios: have functions return vbt, not bdb, header pointer drm/i915/bios: move debug logging about VBT source to intel_parse_bios() drm/i915/bios: rename intel_parse_bios to intel_bios_init drm/i915: refactor VBT validation drm/i915/opregion: make VBT size limit more strict drm/i915/opregion: make VBT pointer a const drm/i915: don't use a temp buffer for opregion debugfs file drm/i915/debugfs: add a separate debugfs file for VBT drm/i915/opregion: handle VBT sizes bigger than 6 KB drivers/gpu/drm/i915/i915_debugfs.c | 31 ++--- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 8 ++- drivers/gpu/drm/i915/intel_bios.c | 117 -- drivers/gpu/drm/i915/intel_bios.h | 2 - drivers/gpu/drm/i915/intel_opregion.c | 66 ++- 6 files changed, 151 insertions(+), 75 deletions(-) -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/11] drm/i915: move "no VBT in opregion" quirk to intel_opregion_setup()
Check the quirk in intel_opregion_setup(), and don't initialize opregion->vbt at all if the quirk says it's not present, hiding the quirk from the rest of the driver. Signed-off-by: Jani Nikula--- drivers/gpu/drm/i915/intel_bios.c | 24 ++-- drivers/gpu/drm/i915/intel_opregion.c | 25 +++-- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 070470fe9a91..401e1141f55f 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -24,7 +24,7 @@ *Eric Anholt * */ -#include + #include #include #include @@ -1214,26 +1214,6 @@ init_vbt_defaults(struct drm_i915_private *dev_priv) } } -static int intel_no_opregion_vbt_callback(const struct dmi_system_id *id) -{ - DRM_DEBUG_KMS("Falling back to manually reading VBT from " - "VBIOS ROM for %s\n", - id->ident); - return 1; -} - -static const struct dmi_system_id intel_no_opregion_vbt[] = { - { - .callback = intel_no_opregion_vbt_callback, - .ident = "ThinkCentre A57", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), - DMI_MATCH(DMI_PRODUCT_NAME, "97027RG"), - }, - }, - { } -}; - static const struct bdb_header *validate_vbt(const void *base, size_t size, const void *_vbt, @@ -1317,7 +1297,7 @@ intel_parse_bios(struct drm_device *dev) init_vbt_defaults(dev_priv); /* XXX Should this validation be moved to intel_opregion.c? */ - if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv->opregion.vbt) + if (dev_priv->opregion.vbt) bdb = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE, dev_priv->opregion.vbt, "OpRegion"); diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index cd97b9a5df57..5b9fc790d300 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -26,6 +26,7 @@ */ #include +#include #include #include @@ -904,6 +905,25 @@ static void swsci_setup(struct drm_device *dev) static inline void swsci_setup(struct drm_device *dev) {} #endif /* CONFIG_ACPI */ +static int intel_no_opregion_vbt_callback(const struct dmi_system_id *id) +{ + DRM_DEBUG_KMS("Falling back to manually reading VBT from " + "VBIOS ROM for %s\n", id->ident); + return 1; +} + +static const struct dmi_system_id intel_no_opregion_vbt[] = { + { + .callback = intel_no_opregion_vbt_callback, + .ident = "ThinkCentre A57", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_NAME, "97027RG"), + }, + }, + { } +}; + int intel_opregion_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -942,8 +962,6 @@ int intel_opregion_setup(struct drm_device *dev) goto err_out; } opregion->header = base; - opregion->vbt = base + OPREGION_VBT_OFFSET; - opregion->lid_state = base + ACPI_CLID; mboxes = opregion->header->mboxes; @@ -968,6 +986,9 @@ int intel_opregion_setup(struct drm_device *dev) if (mboxes & MBOX_ASLE_EXT) DRM_DEBUG_DRIVER("ASLE extension supported\n"); + if (!dmi_check_system(intel_no_opregion_vbt)) + opregion->vbt = base + OPREGION_VBT_OFFSET; + return 0; err_out: -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/11] drm/i915: Add Intel opregion mailbox 5 structure
From: Deepak MMailbox 5 is BIOS to Driver Notification mailbox is intended to support BIOS to Driver event notification or data storage for BIOS to Driver data synchronization purpose. Mailbox 5 is the extension of mailbox 3. v4 by Jani: - don't add asle_ext to dev_priv as it's unused - use u8 for bddc and rsvd fields in asle ext struct - add BUILD_BUG_ON the asle ext struct size - debug logging for asle ext present Cc: Jani Nikula Signed-off-by: Deepak M Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_opregion.c | 13 + 1 file changed, 13 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index 64efedfad879..cd97b9a5df57 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -46,6 +46,7 @@ #define OPREGION_SWSCI_OFFSET 0x200 #define OPREGION_ASLE_OFFSET 0x300 #define OPREGION_VBT_OFFSET0x400 +#define OPREGION_ASLE_EXT_OFFSET 0x1C00 #define OPREGION_SIGNATURE "IntelGraphicsMem" #define MBOX_ACPI (1<<0) @@ -125,6 +126,13 @@ struct opregion_asle { u8 rsvd[58]; } __packed; +/* OpRegion mailbox #5: ASLE ext */ +struct opregion_asle_ext { + u32 phed; /* Panel Header */ + u8 bddc[256]; /* Panel EDID */ + u8 rsvd[764]; +} __packed; + /* Driver readiness indicator */ #define ASLE_ARDY_READY(1 << 0) #define ASLE_ARDY_NOT_READY(0 << 0) @@ -909,6 +917,7 @@ int intel_opregion_setup(struct drm_device *dev) BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100); BUILD_BUG_ON(sizeof(struct opregion_swsci) != 0x100); BUILD_BUG_ON(sizeof(struct opregion_asle) != 0x100); + BUILD_BUG_ON(sizeof(struct opregion_asle_ext) != 0x400); pci_read_config_dword(dev->pdev, PCI_ASLS, ); DRM_DEBUG_DRIVER("graphic opregion physical addr: 0x%x\n", asls); @@ -948,6 +957,7 @@ int intel_opregion_setup(struct drm_device *dev) opregion->swsci = base + OPREGION_SWSCI_OFFSET; swsci_setup(dev); } + if (mboxes & MBOX_ASLE) { DRM_DEBUG_DRIVER("ASLE supported\n"); opregion->asle = base + OPREGION_ASLE_OFFSET; @@ -955,6 +965,9 @@ int intel_opregion_setup(struct drm_device *dev) opregion->asle->ardy = ASLE_ARDY_NOT_READY; } + if (mboxes & MBOX_ASLE_EXT) + DRM_DEBUG_DRIVER("ASLE extension supported\n"); + return 0; err_out: -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Drop the broken cursor base==0 special casing
On Mon, Dec 14, 2015 at 01:16:48PM +0200, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > The cursor code tries to treat base==0 to mean disabled. That fails > when the cursor bo gets bound at ggtt offset 0, and the user is left > looking at an invisible cursor. > > We lose the disabled->disabled optimization, but that seems like > something better handled at a slightly higher level. > > Cc: Takashi Iwai > Cc: Jani Nikula > Signed-off-by: Ville Syrjälä Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Parsing VBT if size of VBT exceeds 6KB
On Mon, Dec 14, 2015 at 05:46:41PM +0530, Deepak M wrote: > Currently the iomap for VBT works only if the size of the > VBT is less than 6KB, but if the size of the VBT exceeds > 6KB than the physical address and the size of the VBT to > be iomapped is specified in the mailbox3 and is iomapped > accordingly. > > v3: -Splitted the patch into small ones > -Handeled memory unmap in intel_opregion_fini > -removed the new file created for opregion macro`s > v4: Moving the vbt assignment after the opregion fields are assigned > > Cc: Mika Kahola> Cc: Jani Nikula > Signed-off-by: Deepak M > --- > > drivers/gpu/drm/i915/intel_opregion.c | 47 > +-- > 1 file changed, 34 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c > b/drivers/gpu/drm/i915/intel_opregion.c > index 7908a1d..5116690 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -856,6 +856,8 @@ void intel_opregion_fini(struct drm_device *dev) > } > > /* just clear all opregion memory pointers now */ > + if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) > + memunmap(opregion->vbt); > memunmap(opregion->header); > opregion->header = NULL; > opregion->acpi = NULL; > @@ -933,7 +935,8 @@ int intel_opregion_setup(struct drm_device *dev) > char buf[sizeof(OPREGION_SIGNATURE)]; > const struct vbt_header *vbt = NULL; > int err = 0; > - void *base; > + void *base, *vbt_base; > + size_t size; > > BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100); > BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100); > @@ -963,19 +966,7 @@ int intel_opregion_setup(struct drm_device *dev) > goto err_out; > } > > - vbt = validate_vbt(base + OPREGION_VBT_OFFSET, > - MAILBOX_4_SIZE, "OpRegion"); > - > - if (vbt == NULL) { > - err = -EINVAL; > - goto err_out; > - } > - > - vbt = (const struct vbt_header *)(base + OPREGION_VBT_OFFSET); > - dev_priv->opregion.vbt_size = vbt->vbt_size; > - > opregion->header = base; > - opregion->vbt = base + OPREGION_VBT_OFFSET; > > opregion->lid_state = base + ACPI_CLID; > opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; > @@ -998,6 +989,36 @@ int intel_opregion_setup(struct drm_device *dev) > opregion->asle->ardy = ASLE_ARDY_NOT_READY; > } > > + /* > + * Non-zero value in rvda field is an indication to driver that a > + * valid Raw VBT is stored in that address and driver should not refer > + * to mailbox4 for getting VBT. > + */ > + if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) { > + size = opregion->asle->rvds; > + vbt_base = memremap(opregion->asle->rvda, > + size, MEMREMAP_WB); > + } else { > + size = MAILBOX_4_SIZE; > + vbt_base = base + OPREGION_VBT_OFFSET; > + } > + > + vbt = validate_vbt(vbt_base, size, "OpRegion"); > + > + if (vbt == NULL) { > + err = -EINVAL; > + goto err_out; > + } > + > + /* Assigning the vbt_size based on the VBT location */ > + if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) > + dev_priv->opregion.vbt_size = opregion->asle->rvds; > + else { > + vbt = (const struct vbt_header *)(base + OPREGION_VBT_OFFSET); i.e. vbt = vbt_base; which is already done by vbt = validate_vbt; > + dev_priv->opregion.vbt_size = vbt->vbt_size; > + } So this reduces down to: /* Explanation why the new method cannot store the size in vbt->vbt_size */ if (vbt != opregion->asle->rvda) size = vbt->vbt_size; dev_priv->opregion.vbt_size = size; opregrion->vbt = vbt; And the vbt_base variable is redundant. Cut out the tautology and reduce the apparent complex interdependence between paths. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] topic/drm-misc
Hi Dave, Last (very likely at least) drm-misc pull for 4.5. 3 big things: - piles of docs for kms vtables. - drm.debug dmesg output prettification from Ville (i915 parts are for 4.6 I think) - connector mode probing/validating/merging cleanup from Ville. From that last pile please cherry-pick commit be8719a610003297c28b140f1ebd4445aef1d613 Author: Ville SyrjäläDate: Thu Dec 3 23:14:09 2015 +0200 drm: Don't overwrite UNVERFIED mode status to OK for drm-fixes like we discussed on irc - it's a regression fix I merged to avoid conflict hilarity. Oh, conflicts: One in i915 with drm-intel-next (new argument vs. error path added to same function call), and a few in vmwgfx (hook removal here vs. set_cursor2 addedin in -fixes). Stephen has the right resolutions posted to dri-devel too. There's one more small series from Nicolas to clean up the interface of drm_dev_set_unique, but that might conflict with new drivers. So wanted to double-check that and wait a bit until I send you that one. Worst case it'll go in right around 4.5-rc1. Wrt 4.5 I'll only send you one more drm-intel pull early next week, because misaligned agina a bit. Is that still ok? Cheers, Daniel The following changes since commit e876b41ab074561d65f213bf5e0fc68cf5bc7380: Back merge tag 'v4.4-rc4' into drm-next (2015-12-08 11:04:26 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/drm-misc-2015-12-14 for you to fetch changes up to d6e6e14fa61dcabbc05092ea124540280573720c: drm: modes: Revert cc344980c767 "replace simple_strtoul by kstrtouint" (2015-12-11 17:13:06 +0100) Daniel Vetter (27): drm: Polish fbdev helper struct docs drm: Move LEAVE/ENTER_ATOMIC_MODESET to fbdev helpers drm: Reorganize helper vtables and their docs drm: Make helper vtable pointers type-safe drm: Merge helper docbook into kerneldoc comments drm/bridge: Improve kerneldoc drm: Update drm_plane_funcs kerneldoc drm/nouveau: Ditch NULL save/restore hook assignments drm/qxl: Drop dummy save/restore hooks drm/virtio: Drop dummy save/restore functions drm/vmwgfx: Drop dummy save/restore hooks drm/gma500: Move to private save/restore hooks drm/nouveau: Use private save/restore hooks for CRTCs drm: Remove crtc/connector->save/restore hooks drm: Move encoder->save/restore into nouveau drm: Document drm_atomic_*_get_property drm: Document drm_connector_funcs drm: connector->dpms is not optional drm: document drm_crtc_funcs drm: Add kerneldoc for drm_framebuffer_funcs drm: Kerneldoc for drm_mode_config_funcs drm: Document drm_plane_helper_funcs drm: Document drm_connector_helper_funcs drm/atomic-helper: Mention the new system/resume helpers the docs drm: Move drm_display_mode an related docs into kerneldoc drm: Document drm_encoder/crtc_helper_funcs drm: Documentation style guide LABBE Corentin (1): drm: modes: Revert cc344980c767 "replace simple_strtoul by kstrtouint" Rodrigo Vivi (1): drm/i2c/tda998x: Remove unused save/restore drm encoder helpers. Ville Syrjälä (14): drm: Pass 'name' to drm_crtc_init_with_planes() drm: Pass 'name' to drm_universal_plane_init() drm: Pass 'name' to drm_encoder_init() drm: Use driver specified encoder name drm: Add crtc->name and use it in debug messages drm: Add plane->name and use it in debug prints drm: Don't overwrite UNVERFIED mode status to OK drm: Rename MODE_UNVERIFIED to MODE_STALE drm: Flatten drm_mode_connector_list_update() a bit drm: Only merge mode type bits between new probed modes drm: Drop drm_helper_probe_single_connector_modes_nomerge() drm/sti: Drop bogus drm_mode_sort() call drm: Allow override_edid to override the firmware EDID drm: Expand the drm_helper_probe_single_connector_modes() docs Documentation/DocBook/gpu.tmpl | 537 +- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h |1 + drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 14 +- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 14 +- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c| 14 +- drivers/gpu/drm/armada/armada_crtc.c |4 +- drivers/gpu/drm/armada/armada_overlay.c |2 +- drivers/gpu/drm/ast/ast_mode.c |2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c |2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c |2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |2 +- drivers/gpu/drm/bochs/bochs_kms.c|2 +- drivers/gpu/drm/cirrus/cirrus_mode.c |2 +- drivers/gpu/drm/drm_atomic.c | 86 +- drivers/gpu/drm/drm_atomic_helper.c | 72 +-
Re: [Intel-gfx] [PATCH 3/9] drm/i915: Clearing buffer objects via CPU/GTT
On Mon, Dec 14, 2015 at 11:16:05AM +0530, ankitprasad.r.sha...@intel.com wrote: > From: Ankitprasad Sharma> > This patch adds support for clearing buffer objects via CPU/GTT. This > is particularly useful for clearing out the non shmem backed objects. > Currently intend to use this only for buffers allocated from stolen > region. > > v2: Added kernel doc for i915_gem_clear_object(), corrected/removed > variable assignments (Tvrtko) > > v3: Map object page by page to the gtt if the pinning of the whole object > to the ggtt fails, Corrected function name (Chris) > > v4: Clear the buffer page by page, and not map the whole object in the gtt > aperture. Use i915 wrapper function in place of drm_mm_insert_node_in_range. > > Testcase: igt/gem_stolen > > Signed-off-by: Ankitprasad Sharma > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 44 > + > 2 files changed, 45 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a10b866..e195fee 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2897,6 +2897,7 @@ int i915_gem_obj_prepare_shmem_read(struct > drm_i915_gem_object *obj, > int *needs_clflush); > > int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > +int i915_gem_object_clear(struct drm_i915_gem_object *obj); > > static inline int __sg_page_count(struct scatterlist *sg) > { > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 46c1e75..e50a91b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5293,3 +5293,47 @@ fail: > drm_gem_object_unreference(>base); > return ERR_PTR(ret); > } > + > +/** > + * i915_gem_object_clear() - Clear buffer object via CPU/GTT > + * @obj: Buffer object to be cleared > + * > + * Return: 0 - success, non-zero - failure > + */ > +int i915_gem_object_clear(struct drm_i915_gem_object *obj) > +{ > + int ret, i; > + char __iomem *base; > + size_t size = obj->base.size; > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > + struct drm_mm_node node; > + > + lockdep_assert_held(>base.dev->struct_mutex); > + memset(, 0, sizeof(node)); > + ret = i915_gem_insert_node_in_range(i915, , 4096, 0, > + 0, i915->gtt.mappable_end); I do not like this wrapper because you have no idea what it is meant to do. > + if (ret) > + goto out; > + > + i915_gem_object_pin_pages(obj); > + base = io_mapping_map_wc(i915->gtt.mappable, node.start); > + for (i = 0; i < size/PAGE_SIZE; i++) { > + wmb(); > + i915->gtt.base.insert_page(>gtt.base, > +i915_gem_object_get_dma_address(obj, > i), > +node.start, > +I915_CACHE_NONE, 0); > + wmb(); > + memset_io(base, 0, 4096); The barriers can just be written as: for (;;) { insert_page(); wmb(); memset_io() wmb(); } -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Parsing VBT if size of VBT exceeds 6KB
On Mon, 14 Dec 2015, Chris Wilsonwrote: > On Mon, Dec 14, 2015 at 05:46:41PM +0530, Deepak M wrote: >> Currently the iomap for VBT works only if the size of the >> VBT is less than 6KB, but if the size of the VBT exceeds >> 6KB than the physical address and the size of the VBT to >> be iomapped is specified in the mailbox3 and is iomapped >> accordingly. >> >> v3: -Splitted the patch into small ones >> -Handeled memory unmap in intel_opregion_fini >> -removed the new file created for opregion macro`s >> v4: Moving the vbt assignment after the opregion fields are assigned >> >> Cc: Mika Kahola >> Cc: Jani Nikula >> Signed-off-by: Deepak M >> --- >> >> drivers/gpu/drm/i915/intel_opregion.c | 47 >> +-- >> 1 file changed, 34 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c >> b/drivers/gpu/drm/i915/intel_opregion.c >> index 7908a1d..5116690 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -856,6 +856,8 @@ void intel_opregion_fini(struct drm_device *dev) >> } >> >> /* just clear all opregion memory pointers now */ >> +if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) >> +memunmap(opregion->vbt); >> memunmap(opregion->header); >> opregion->header = NULL; >> opregion->acpi = NULL; >> @@ -933,7 +935,8 @@ int intel_opregion_setup(struct drm_device *dev) >> char buf[sizeof(OPREGION_SIGNATURE)]; >> const struct vbt_header *vbt = NULL; >> int err = 0; >> -void *base; >> +void *base, *vbt_base; >> +size_t size; >> >> BUILD_BUG_ON(sizeof(struct opregion_header) != 0x100); >> BUILD_BUG_ON(sizeof(struct opregion_acpi) != 0x100); >> @@ -963,19 +966,7 @@ int intel_opregion_setup(struct drm_device *dev) >> goto err_out; >> } >> >> -vbt = validate_vbt(base + OPREGION_VBT_OFFSET, >> -MAILBOX_4_SIZE, "OpRegion"); >> - >> -if (vbt == NULL) { >> -err = -EINVAL; >> -goto err_out; >> -} >> - >> -vbt = (const struct vbt_header *)(base + OPREGION_VBT_OFFSET); >> -dev_priv->opregion.vbt_size = vbt->vbt_size; >> - >> opregion->header = base; >> -opregion->vbt = base + OPREGION_VBT_OFFSET; >> >> opregion->lid_state = base + ACPI_CLID; >> opregion->asle_ext = base + OPREGION_ASLE_EXT_OFFSET; >> @@ -998,6 +989,36 @@ int intel_opregion_setup(struct drm_device *dev) >> opregion->asle->ardy = ASLE_ARDY_NOT_READY; >> } >> >> +/* >> + * Non-zero value in rvda field is an indication to driver that a >> + * valid Raw VBT is stored in that address and driver should not refer >> + * to mailbox4 for getting VBT. >> + */ >> +if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) { >> +size = opregion->asle->rvds; >> +vbt_base = memremap(opregion->asle->rvda, >> +size, MEMREMAP_WB); >> +} else { >> +size = MAILBOX_4_SIZE; >> +vbt_base = base + OPREGION_VBT_OFFSET; >> +} >> + >> +vbt = validate_vbt(vbt_base, size, "OpRegion"); >> + >> +if (vbt == NULL) { >> +err = -EINVAL; >> +goto err_out; >> +} >> + >> +/* Assigning the vbt_size based on the VBT location */ >> +if (opregion->header->opregion_ver >= 2 && opregion->asle->rvda) >> +dev_priv->opregion.vbt_size = opregion->asle->rvds; >> +else { >> +vbt = (const struct vbt_header *)(base + OPREGION_VBT_OFFSET); > i.e. vbt = vbt_base; > > which is already done by vbt = validate_vbt; > >> +dev_priv->opregion.vbt_size = vbt->vbt_size; >> +} > > So this reduces down to: > > /* Explanation why the new method cannot store the size in vbt->vbt_size */ > if (vbt != opregion->asle->rvda) > size = vbt->vbt_size; > dev_priv->opregion.vbt_size = size; > opregrion->vbt = vbt; > > And the vbt_base variable is redundant. > > Cut out the tautology and reduce the apparent complex > interdependence between paths. I rewrote patches 2-6 in this series into a new VBT/opregion refactoring series [1] that should clean this up. BR, Jani. [1] http://mid.gmane.org/cover.1450089383.git.jani.nik...@intel.com -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT
On Mon, Dec 14, 2015 at 12:50:54PM +0200, Jani Nikula wrote: > In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, > thus unavailable in i915_opregion, so add a separate file for the VBT. > > Signed-off-by: Jani Nikula> --- > drivers/gpu/drm/i915/i915_debugfs.c | 22 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_opregion.c | 4 +++- > 3 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index a9e1f18c36d1..aef1393e707f 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1857,6 +1857,27 @@ out: > return 0; > } > > +static int i915_vbt(struct seq_file *m, void *unused) > +{ > + struct drm_info_node *node = m->private; > + struct drm_device *dev = node->minor->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_opregion *opregion = _priv->opregion; > + int ret; > + > + ret = mutex_lock_interruptible(>struct_mutex); The contents of opregion->vbt are not serialised by struct_mutex, right? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/11] drm/i915/debugfs: add a separate debugfs file for VBT
On Mon, Dec 14, 2015 at 01:06:51PM +0200, Jani Nikula wrote: > On Mon, 14 Dec 2015, Chris Wilsonwrote: > > On Mon, Dec 14, 2015 at 12:50:54PM +0200, Jani Nikula wrote: > >> In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, > >> thus unavailable in i915_opregion, so add a separate file for the VBT. > >> > >> Signed-off-by: Jani Nikula > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 22 ++ > >> drivers/gpu/drm/i915/i915_drv.h | 1 + > >> drivers/gpu/drm/i915/intel_opregion.c | 4 +++- > >> 3 files changed, 26 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > >> b/drivers/gpu/drm/i915/i915_debugfs.c > >> index a9e1f18c36d1..aef1393e707f 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -1857,6 +1857,27 @@ out: > >>return 0; > >> } > >> > >> +static int i915_vbt(struct seq_file *m, void *unused) > >> +{ > >> + struct drm_info_node *node = m->private; > >> + struct drm_device *dev = node->minor->dev; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct intel_opregion *opregion = _priv->opregion; > >> + int ret; > >> + > >> + ret = mutex_lock_interruptible(>struct_mutex); > > > > The contents of opregion->vbt are not serialised by struct_mutex, right? > > Cargo culting ftw. I figured it was maybe for suspend/resume paths, but > that doesn't really make sense does it? No, the only thing that we control here are the dev_priv->opregion pointers - and they are init only. So I think we are totally safe to drop the struct_mutex. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915/skl: Do not allow scaling when crtc is disabled.
On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote: > This fixes a warning when the crtc is turned off. In that case fb > will be NULL, and crtc_clock will be 0. Because the crtc is no longer > active this is not a bug, and shouldn't trigger the WARN_ON. > > Also remove handling a null crtc_state, with all transitional helpers > gone this can no longer happen. > Reviewed-by: Mika Kahola> Signed-off-by: Maarten Lankhorst > --- > 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 764eeb05492d..351ecb69e5eb 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13693,7 +13693,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct > intel_crtc_state *crtc_state > struct drm_i915_private *dev_priv; > int crtc_clock, cdclk; > > - if (!intel_crtc || !crtc_state) > + if (!intel_crtc || !crtc_state->base.enable) > return DRM_PLANE_HELPER_NO_SCALING; > > dev = intel_crtc->base.dev; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/13] drm/i915: Add sync framework support to execbuff IOCTL
On Mon, Dec 14, 2015 at 11:46:22AM +, John Harrison wrote: > >>@@ -1341,6 +1375,17 @@ i915_gem_do_execbuffer(struct drm_device > >>*dev, void *data, > >> u32 dispatch_flags; > >> int ret; > >> bool need_relocs; > >>+int fd_fence_complete = -1; > >>+int fd_fence_wait = lower_32_bits(args->rsvd2); > >>+struct sync_fence *sync_fence; > >>+ > >>+/* > >>+ * Make sure an broken fence handle is not returned no matter > >>+ * how early an error might be hit. Note that rsvd2 has to be > >>+ * saved away first because it is also an input parameter! > >>+ */ > > > >Instead of the 2nd sentence maybe say something like "Note that we > >have saved rsvd2 already for later use since it is also in input > >parameter!". Like written I was expecting the code following the > >comment to do that, and then was confused when it didn't. Or maybe > >my attention span is too short. > Will update the comment for those who can't remember what they read > two lines earlier... Honestly, I thought the complaint here would be that the user's input parameter is being modified on the error path breaking the ABI i.e. drmIoctl() will not 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 v2 1/2] drm/i915: Kill intel_crtc->cursor_bo
On Mon, Dec 14, 2015 at 05:35:02PM +0200, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > The vma may have been rebound between the last time the cursor was > enabled and now, so skipping the cursor gtt offset deduction is not > safe unless we would also reset cursor_bo to NULL when disabling the > cursor. Just thow cursor_bo to the bin instead since it's lost all > other uses thanks to universal plane support. > > Chris pointed out that cursor updates are currently too slow > via universal planes that micro optimizations like these wouldn't > even help. > > v2: Add a note about futility of micro optimizations (Chris) > > References: > http://lists.freedesktop.org/archives/intel-gfx/2015-December/082976.html > Cc: Chris Wilson > Cc: Takashi Iwai > Cc: Jani Nikula > Signed-off-by: Ville Syrjälä Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm: Move more framebuffer doc from docbook to kerneldoc
On Tue, Dec 08, 2015 at 09:49:19AM +0100, Daniel Vetter wrote: [...] > @@ -187,6 +189,9 @@ struct drm_framebuffer_funcs { >* copying the current screen contents to a private buffer and blending >* between that and the new contents. >* > + * GEM based drivers should call drm_gem_handle_create() to create the > + * handle. > + * >* RETURNS: >* >* 0 on success or a negative error code on failure. > @@ -1727,6 +1732,17 @@ struct drm_mode_config_funcs { >* requested metadata, but most of that is left to the driver. See >* struct _mode_fb_cmd2 for details. >* > + * If the parameters are deemed valid and the backing storage objects in > + * the underlying memory manager all exists then the drivers to allocate "... all exist, then the driver allocates a new _framebuffer structure ..."? > + * a new _framebuffer structure, subclassed to contain > + * driver-specific information (like the internal native buffer object > + * references). It also needs to fill out all relevant metadata, which > + * should by done by calling drm_helper_mode_fill_fb_struct(). "should be done" > + * > + * The initializing is finalized by calling drm_framebuffer_init(), "The initialization" Other than that, looks good: Reviewed-by: Thierry Redingsignature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/10] drm/i915: Write out crc frame counts in hex
From: Ville SyrjäläThe crc code assumes that the printed frame counter value takes exactly 8 characters. The counter is a 32bit value, so to fit it into 8 characters we need to print it in hex, in decimal it would take 10 characters. This obviously needs a corresponding change in intel-gpu-tools. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 24318b79bcfc..96d6e5de0811 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3518,7 +3518,7 @@ i915_pipe_crc_read(struct file *filep, char __user *user_buf, size_t count, pipe_crc->tail = (pipe_crc->tail + 1) & (INTEL_PIPE_CRC_ENTRIES_NR - 1); bytes_read += snprintf(buf, PIPE_CRC_BUFFER_LEN, - "%8u %8x %8x %8x %8x %8x\n", + "%8x %8x %8x %8x %8x %8x\n", entry->frame, entry->crc[0], entry->crc[1], entry->crc[2], entry->crc[3], entry->crc[4]); -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/10] drm/i915: Wait for pipe to start before sampling vblank timestamps on gen2
From: Ville SyrjäläWe use the vblank timestamps to generate the vblank frame counter value on gen2. That means we need the pipe scanout position to be accurate when we call drm_crtc_vblank_on(), otherwise the frame counter guesstimate may jump when the pipe actually start. What I observed on my 85x is that the DSL initially reads 0, and when the pipe actually starts DSL jumps to vblank_start. On gen2 DSL==0 means actually vtotal-1 (see update_scanline_offset()), so if we initially get vtotal-1, and then very quickly vblank_start (or thereabouts), the scanout position will appear to jump backwards by approximately one vblank length. Which means the frame counter guesstimate will also jump backwards. That's no good, so let's make sure the pipe has started before we call drm_crtc_vblank_on(). Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7dd7200d3ba9..78845d1cfd2e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2152,6 +2152,17 @@ static void intel_enable_pipe(struct intel_crtc *crtc) I915_WRITE(reg, val | PIPECONF_ENABLE); POSTING_READ(reg); + + /* +* Until the pipe starts DSL will read as 0, which would cause +* an apparent vblank timestamp jump, which messes up also the +* frame count when it's derived from the timestamps. So let's +* wait for the pipe to start properly before we call +* drm_crtc_vblank_on() +*/ + if (dev->max_vblank_count == 0 && + wait_for(intel_get_crtc_scanline(crtc) != crtc->scanline_offset, 50)) + DRM_ERROR("pipe %c didn't start\n", pipe_name(pipe)); } /** -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/10] drm/i915: Allow 27 bytes child_dev for VBT <109
From: Ville SyrjäläMy 85x has VBT version 108 which has a child dev size of 27 bytes. Let's allow that without printing an error. We still want to reject the actual parsin since for that we need the child device size to be at least 33 bytes. So we should still check for that, but let's make it print a debug message only instead of an error. While at it, toss in a BUILD_BUG_ON() to verify our struct old_child_dev_config is in fact 33 bytes. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_bios.c | 19 +++ 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 070470fe9a91..770b825dabc0 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1089,7 +1089,10 @@ parse_device_mapping(struct drm_i915_private *dev_priv, DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n"); return; } - if (bdb->version < 195) { + if (bdb->version < 109) { + expected_size = 27; + } else if (bdb->version < 195) { + BUILD_BUG_ON(sizeof(struct old_child_dev_config) != 33); expected_size = sizeof(struct old_child_dev_config); } else if (bdb->version == 195) { expected_size = 37; @@ -1102,18 +1105,18 @@ parse_device_mapping(struct drm_i915_private *dev_priv, bdb->version, expected_size); } - /* The legacy sized child device config is the minimum we need. */ - if (p_defs->child_dev_size < sizeof(struct old_child_dev_config)) { - DRM_ERROR("Child device config size %u is too small.\n", - p_defs->child_dev_size); - return; - } - /* Flag an error for unexpected size, but continue anyway. */ if (p_defs->child_dev_size != expected_size) DRM_ERROR("Unexpected child device config size %u (expected %u for VBT version %u)\n", p_defs->child_dev_size, expected_size, bdb->version); + /* The legacy sized child device config is the minimum we need. */ + if (p_defs->child_dev_size < sizeof(struct old_child_dev_config)) { + DRM_DEBUG_KMS("Child device config size %u is too small.\n", + p_defs->child_dev_size); + return; + } + /* get the block size of general definitions */ block_size = get_blocksize(p_defs); /* get the number of child device */ -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/10] drm/i915: Expect child dev size of 22 bytes for VBT < 106
From: Ville SyrjäläMy 830 has VBT version 105 with child device size of 22 bytes. Let's assume that's correct and adjust our expectations. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_bios.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 770b825dabc0..c99a96989113 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1089,7 +1089,9 @@ parse_device_mapping(struct drm_i915_private *dev_priv, DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n"); return; } - if (bdb->version < 109) { + if (bdb->version < 106) { + expected_size = 22; + } else if (bdb->version < 109) { expected_size = 27; } else if (bdb->version < 195) { BUILD_BUG_ON(sizeof(struct old_child_dev_config) != 33); -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/10] drm/i915: Cleanup phys status page too
From: Ville SyrjäläRestore the lost phys status page cleanup. Fixes the following splat with DMA_API_DEBUG=y: WARNING: CPU: 0 PID: 21615 at ../lib/dma-debug.c:974 dma_debug_device_change+0x190/0x1f0() pci :00:02.0: DMA-API: device driver has pending DMA allocations while released from device [count=1] One of leaked entries details: [device address=0x23163000] [size=4096 bytes] [mapped with DMA_BIDIRECTIONAL] [mapped as coherent] Modules linked in: i915(-) i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm sha256_generic hmac drbg ctr ccm sch_fq_codel binfmt_misc joydev mousedev arc4 ath5k iTCO_wdt mac80211 smsc_ircc2 ath snd_intel8x0m snd_intel8x0 snd_ac97_codec ac97_bus psmouse snd_pcm input_leds i2c_i801 pcspkr snd_timer cfg80211 snd soundcore i2c_core ehci_pci firewire_ohci ehci_hcd firewire_core lpc_ich 8139too rfkill crc_itu_t mfd_core mii usbcore rng_core intel_agp intel_gtt usb_common agpgart irda crc_ccitt fujitsu_laptop led_class parport_pc video parport evdev backlight CPU: 0 PID: 21615 Comm: rmmod Tainted: G U 4.4.0-rc4-mgm-ovl+ #4 Hardware name: FUJITSU SIEMENS LIFEBOOK S6120/FJNB16C, BIOS Version 1.26 05/10/2004 e31a3de0 e31a3de0 e31a3d9c c128d4bd e31a3dd0 c1045a0c c15e00c4 e31a3dfc 546f c15dfad2 03ce c12b3740 03ce c12b3740 0001 f61fb8a0 e31a3de8 c1045a83 0009 e31a3de0 c15e00c4 e31a3dfc e31a3e4c Call Trace: [] dump_stack+0x16/0x19 [] warn_slowpath_common+0x8c/0xd0 [] ? dma_debug_device_change+0x190/0x1f0 [] ? dma_debug_device_change+0x190/0x1f0 [] warn_slowpath_fmt+0x33/0x40 [] dma_debug_device_change+0x190/0x1f0 [] notifier_call_chain+0x59/0x70 [] __blocking_notifier_call_chain+0x3f/0x80 [] blocking_notifier_call_chain+0x1f/0x30 [] __device_release_driver+0xc3/0xf0 [] driver_detach+0x97/0xa0 [] bus_remove_driver+0x40/0x90 [] driver_unregister+0x28/0x60 [] ? trace_hardirqs_on_caller+0x12c/0x1d0 [] pci_unregister_driver+0x18/0x80 [] drm_pci_exit+0x87/0xb0 [drm] [] i915_exit+0x1b/0x1ee [i915] [] SyS_delete_module+0x14c/0x210 [] ? trace_hardirqs_on_caller+0x12c/0x1d0 [] ? fput+0xd/0x10 [] do_fast_syscall_32+0xa4/0x450 [] sysenter_past_esp+0x3b/0x5d ---[ end trace c2ecbc77760f10a0 ]--- Mapped at: [] debug_dma_alloc_coherent+0x33/0x90 [] drm_pci_alloc+0x18c/0x1e0 [drm] [] intel_init_ring_buffer+0x2af/0x490 [i915] [] intel_init_render_ring_buffer+0x130/0x750 [i915] [] i915_gem_init_rings+0x1e/0x110 [i915] Cc: Chris Wilson Fixes: 5c6c600 ("drm/i915: Remove DRI1 ring accessors and API") Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_ringbuffer.c | 22 +++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index eefce9a3e9c8..0c005a3dc57f 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1899,6 +1899,17 @@ i915_dispatch_execbuffer(struct drm_i915_gem_request *req, return 0; } +static void cleanup_phys_status_page(struct intel_engine_cs *ring) +{ + struct drm_i915_private *dev_priv = to_i915(ring->dev); + + if (!dev_priv->status_page_dmah) + return; + + drm_pci_free(ring->dev, dev_priv->status_page_dmah); + ring->status_page.page_addr = NULL; +} + static void cleanup_status_page(struct intel_engine_cs *ring) { struct drm_i915_gem_object *obj; @@ -1915,9 +1926,9 @@ static void cleanup_status_page(struct intel_engine_cs *ring) static int init_status_page(struct intel_engine_cs *ring) { - struct drm_i915_gem_object *obj; + struct drm_i915_gem_object *obj = ring->status_page.obj; - if ((obj = ring->status_page.obj) == NULL) { + if (obj == NULL) { unsigned flags; int ret; @@ -2208,7 +2219,12 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) if (ring->cleanup) ring->cleanup(ring); - cleanup_status_page(ring); + if (I915_NEED_GFX_HWS(ring->dev)) { + cleanup_status_page(ring); + } else { + BUG_ON(ring->id != RCS); + cleanup_phys_status_page(ring); + } i915_cmd_parser_fini_ring(ring); i915_gem_batch_pool_fini(>batch_pool); -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/9] drm/i915: Support for pread/pwrite from/to non shmem backed objects
On Mon, Dec 14, 2015 at 11:16:09AM +0530, ankitprasad.r.sha...@intel.com wrote: > @@ -1150,8 +1252,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void > *data, >* perspective, requiring manual detiling by the client. >*/ > if (obj->tiling_mode == I915_TILING_NONE && > - obj->base.write_domain != I915_GEM_DOMAIN_CPU && > - cpu_write_needs_clflush(obj)) { > + (!obj->base.filp || > + (obj->base.write_domain != I915_GEM_DOMAIN_CPU && > + cpu_write_needs_clflush(obj { This is too confusing. Move the write_domain check to needs_clflush ala: diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8edc56a34caa..fd3c73c8ab77 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -49,6 +49,9 @@ static bool cpu_cache_is_coherent(struct drm_device *dev, static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj) { + if (obj->base.write_domain == I915_GEM_DOMAIN_CPU) + return false; + if (!cpu_cache_is_coherent(obj->base.dev, obj->cache_level)) return true; @@ -1073,7 +1076,6 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, * perspective, requiring manual detiling by the client. */ if (obj->tiling_mode == I915_TILING_NONE && - obj->base.write_domain != I915_GEM_DOMAIN_CPU && cpu_write_needs_clflush(obj)) { ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file); /* Note that the gtt paths might fail with non-page-backed user @@ -3159,9 +3161,7 @@ out: * object is now coherent at its new cache level (with respect * to the access domain). */ - if (obj->cache_dirty && - obj->base.write_domain != I915_GEM_DOMAIN_CPU && - cpu_write_needs_clflush(obj)) { + if (obj->cache_dirty && cpu_write_needs_clflush(obj)) { if (i915_gem_clflush_object(obj, true)) i915_gem_chipset_flush(obj->base.dev); } and the negative (tiling mode) test to i915_gem_gtt_pwrite_fast. Then it reads as if (obj->base.filp == NULL || cpu_write_needs_clflush(obj)) ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file); > /* Note that the gtt paths might fail with non-page-backed user >* pointers (e.g. gtt mappings when moving data between > @@ -1161,7 +1264,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void > *data, > if (ret == -EFAULT || ret == -ENOSPC) { > if (obj->phys_handle) > ret = i915_gem_phys_pwrite(obj, args, file); > - else > + else if (obj->base.filp) > ret = i915_gem_shmem_pwrite(dev, obj, args, file); Forgot else ret = -ENODEV; -- 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] bisected: i915 modeset broken in ac9b8236551d1177fd07b56aef9b565d1864420d
On Mon, Dec 14, 2015 at 03:31:09PM +0200, Meelis Roos wrote: > Between 4.4-rc3 and 4.4-rc4, i915 modesetting broke on my i5-2400 PC. That would seem to be SNB. > Instead of seeing the new dense graphics mode, I see the last VGA text > lines and no X appears either. That's a bit weird. SNB has no power power wells, so only runtime PM could be a factor, but it should not kick in that fast during boot even if you enable it before loading the driver since we set the delay to 10 seconds. And in any case the commit you list shouldn't really change anything for SNB. We used to grab a rpm reference for gmbus via intel_aux_display_runtime_get() and now we get it via the GMBUS power domain instead. I would be more tempted to blame the live status check, except it was already included in 4.4-rc1 I think. We didn't have a power well reference for it until commit 0f5a9be15797 ("drm/i915: take a power domain reference while checking the HDMI live status") but that's already in v4.4-rc4. In any case the live status check was already reported to cause failures (at least on some SNB IIRC). There have been attempts to remedy it since: http://lists.freedesktop.org/archives/intel-gfx/2015-December/082689.html http://lists.freedesktop.org/archives/intel-gfx/2015-December/082932.html So this bisect result is somewhat mysterious. A full dmesg with drm.debug=0xe with and without the offending patch reverted would be helpful. And might be best to attach those into a bug report (https://bugs.freedesktop.org/ -> DRI -> DRM/Intel) so that we don't lose track of them. Oh, are we even talking about HDMI/DVI here, or something else? > > I saw something similar on I865G but have not had time to check if it is > the same issue. > > ac9b8236551d1177fd07b56aef9b565d1864420d is the first bad commit > commit ac9b8236551d1177fd07b56aef9b565d1864420d > Author: Ville Syrjälä> Date: Fri Nov 27 18:55:26 2015 +0200 > > drm/i915: Introduce a gmbus power domain > > Currently the gmbus code uses intel_aux_display_runtime_get/put in an > effort to make sure the hardware is powered up sufficiently for gmbus. > That function only takes the runtime PM reference which on VLV/CHV/BXT > is not enough. We need the disp2d/pipe-a well on VLV/CHV and power well > 2 on BXT. So add a new power domnain for gmbus and kill off the now > unused intel_aux_display_runtime_get/put. And change > intel_hdmi_set_edid() to use the gmbus power domain too since that's all > we need there. > > Also toss in a BUILD_BUG_ON() to catch problems if we run out of > bits for power domains. We're already really close to the limit... > > [Patrik: Add gmbus string to debugfs output] > > Signed-off-by: Ville Syrjälä > Reviewed-by: Patrik Jakobsson > [Cherry-picked from drm-intel-next-queued f0ab43e6 (Imre)] > Signed-off-by: Imre Deak > Link: > http://patchwork.freedesktop.org/patch/msgid/1448643329-18675-3-git-send-email-imre.d...@intel.com > Signed-off-by: Jani Nikula > > :04 04 39379146d7e6dda8a4d5f8781ee3d307cce8c47e > f4f09fae0485ad6263d31d425296fa9cd7de343b M drivers > > > -- > Meelis Roos (mr...@linux.ee) -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/11] drm/i915: Introduce drm_i915_gem_request_node for request tracking
On Mon, Dec 14, 2015 at 03:58:42PM +, Tvrtko Ursulin wrote: > > Hi, > > > On 14/12/15 11:36, Chris Wilson wrote: > >In the next patch, request tracking is made more generic and for that we > >need a new expanded struct and to separate out the logic changes from > >the mechanical churn, we split out the structure renaming into this > >patch. For extra fun, create a new i915_gem_request.h. > > Subject and patch disagree on the new structure name. :) > > Could you draw a nice diagram demonstrating the new design? It is > not straightforward to derive it from the patch series. > > Emphasis on relationships between engines, requests and request_active etc. It's the same as before. Instead of explicitly named functions to call on retiring, you have a list of callbacks. > Also I notice even though you later add vma->last_read, I don't see > that obj->last_read is never removed. Why would it? obj is for the GEM api, vma is for internal - they have different lifetimes and track different state as I thought I explained. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/10] drm/i915: Use drm_vblank_count() on gen2 for crc frame count
From: Ville SyrjäläGen2 doesn't have a hardware frame counter, so let's use the sw counter value instead. Testcase: igt/kms_pipe_crc_basic/read-crc-pipe-?-frame-sequence Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_debugfs.c | 11 +++ drivers/gpu/drm/i915/i915_irq.c | 5 - 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 96d6e5de0811..695c69e85374 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -4021,6 +4021,14 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, if (!entries) return -ENOMEM; + if (dev->max_vblank_count == 0) { + ret = drm_vblank_get(dev, pipe); + if (ret) { + kfree(entries); + return ret; + } + } + /* * When IPS gets enabled, the pipe CRC changes. Since IPS gets * enabled and disabled dynamically based on package C states, @@ -4073,6 +4081,9 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, hsw_trans_edp_pipe_A_crc_wa(dev, false); hsw_enable_ips(crtc); + + if (dev->max_vblank_count == 0) + drm_vblank_put(dev, pipe); } return 0; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 86664d1b3389..37ec8427359a 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1533,7 +1533,10 @@ static void display_pipe_crc_irq_handler(struct drm_device *dev, enum pipe pipe, entry = _crc->entries[head]; - entry->frame = dev->driver->get_vblank_counter(dev, pipe); + if (dev->max_vblank_count == 0) + entry->frame = drm_vblank_count(dev, pipe); + else + entry->frame = dev->driver->get_vblank_counter(dev, pipe); entry->crc[0] = crc0; entry->crc[1] = crc1; entry->crc[2] = crc2; -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/10] drm/i915: Enable vblank_disable_immediate on gen2
From: Ville SyrjäläSince commit 4dfd648 ("drm: Use vblank timestamps to guesstimate how many vblanks were missed") the vblank code can cook up a frame counter value based on the vblank timestamps (as long as they're accurate), so there's no longer any need to keep vblank interrupts enabled on gen2 when no one is interested in them. So let's opt into the immediate disable scheme on gen2 as well. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_irq.c | 9 + 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 37ec8427359a..111edda1e73d 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4437,14 +4437,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) dev->max_vblank_count = 0xff; /* only 24 bits of frame count */ } - /* -* Opt out of the vblank disable timer on everything except gen2. -* Gen2 doesn't have a hardware frame counter and so depends on -* vblank interrupts to produce sane vblank seuquence numbers. -*/ - if (!IS_GEN2(dev_priv)) - dev->vblank_disable_immediate = true; - + dev->vblank_disable_immediate = true; dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/32] drm/i915: Remove the lazy_coherency parameter from request-completed?
On Mon, Dec 14, 2015 at 02:59:30PM +, Tvrtko Ursulin wrote: > > Hi, > > On 11/12/15 11:33, Chris Wilson wrote: > >Now that we have split out the seqno-barrier from the > >engine->get_seqno() callback itself, we can move the users of the > >seqno-barrier to the required callsites simplifying the common code and > >making the required workaround handling much more explicit. > > What bothers me about this patch, and the one preceding it, is that > I don't see a tangible improvement for the programmer who still has > to know when to read the seqno and when to "read it harder, read for > real". In earlier patches, I called it irq_barrier. It's not reading it harder. It's just that there is a ordering issue with receiving an interrupt and the seqno write being visible. > Barrier in this sense has a relation to the state of things but > somehow feels too low level to me when used from the code. But to be > fair I am not sure how to better define it. > > Would ring->get_seqno paired with ring->read_seqno perhaps make > sense? Implementation for ring->read_seqno would just be a flush > followed by ring->get_seqno then. Or maybe keep the barrier and add > ring->read_seqno which would be ring->seqno_barrier + > ring_get_seqno? No. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Kill intel_crtc->cursor_bo
On Mon, Dec 14, 2015 at 11:32:23AM +, Chris Wilson wrote: > On Mon, Dec 14, 2015 at 01:16:47PM +0200, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä> > > > The vma may have been rebound between the last time the cursor was > > enabled and now, so skipping the cursor gtt offset deduction is not > > safe unless we would also reset cursor_bo to NULL when disabling the > > cursor. Just thow cursor_bo to the bin instead since it's lost all > > other uses thanks to universal plane support. > > You could also mention that since updating the cursor is so slow now > through the universal-plane support, that a fast path to avoid a few > writes/checks is also irrelevant. I can add a note. At some point we really should start measuring things and try to get some of the speed back for common stuff like cursor movement and page flips. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Documentation style guide
On Wed, Dec 09, 2015 at 05:08:02PM +0100, Daniel Vetter wrote: > Every time I type or review docs this seems a bit different. Try to > document the common style so we can try to unify at least new docs. > > v2: Spelling fixes from Pierre, Laurent and Jani. > > v3: More spelling fixes from Lukas. > > Cc: Pierre Moreau> Cc: Jani Nikula > Cc: Laurent Pinchart > Cc: Lukas Wunner > Acked-by: Laurent Pinchart > Signed-off-by: Daniel Vetter > Link: > http://patchwork.freedesktop.org/patch/msgid/1449564561-3896-1-git-send-email-daniel.vet...@ffwll.ch > --- > Documentation/DocBook/gpu.tmpl | 37 + > 1 file changed, 37 insertions(+) > > diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl > index 749b8e2f2113..c66d6412f573 100644 > --- a/Documentation/DocBook/gpu.tmpl > +++ b/Documentation/DocBook/gpu.tmpl > @@ -124,6 +124,43 @@ > >[Insert diagram of typical DRM stack here] > > + > +Style Guidelines > + > + For consistency this documentation uses American English. Abbreviations > + are written as all-uppercase, for example: DRM, KMS, IOCTL, CRTC, and > so > + on. To aid in reading, documentations make full use of the markup "..., the documentation makes full use of..." and perhaps "makes use of the full set of markup characters that kerneldoc provides". > + characters kerneldoc provides: @parameter for function parameters, > @member > + for structure members, structure to reference structures and > + function() for functions. These all get automatically hyperlinked if > + kerneldoc for the referenced objects exists. When referencing entries > in > + function vtables please use -vfunc(). Note that kerneldoc does > + not support referencing struct members directly, so please add a > reference > + to the vtable struct somewhere in the same paragraph or at least > section. > + > + > + Except in special situations (to separate locked from unlocked > variants) > + locking requirements for functions aren't documented in the kerneldoc. > + Instead locking should be check at runtime using e.g. "should be checked" > + WARN_ON(!mutex_is_locked(...));. Since it's much easier to > + ignore documentation than runtime noise this provides more value. And > on > + top of that runtime checks do need to be updated when the locking rules > + change, increasing the chances that they're correct. Within the > + documentation the locking rules should be explained in the relevant > + structures: Either in the comment for the lock explaining what it > + protects, or data fields need a note about which lock protects them, or > + both. I think you're supposed to have the "or" only in the final subsentence: "either ... protects, data fields need ..., or both." > + > + > + Functions which have a non-void return value should have a > + section called "Returns" explaining the expected return values in > + different cases and their meanings. Currently there's no consensus > whether > + that section name should be all upper-case or not, and whether it > should > + end in a colon or not. Go with the file-local style. Other common > section I thought the colon was necessary for kerneldoc to turn it into a section? Overall, long overdue, so thanks for writing it up: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 1/2] drm/i915: Kill intel_crtc->cursor_bo
From: Ville SyrjäläThe vma may have been rebound between the last time the cursor was enabled and now, so skipping the cursor gtt offset deduction is not safe unless we would also reset cursor_bo to NULL when disabling the cursor. Just thow cursor_bo to the bin instead since it's lost all other uses thanks to universal plane support. Chris pointed out that cursor updates are currently too slow via universal planes that micro optimizations like these wouldn't even help. v2: Add a note about futility of micro optimizations (Chris) References: http://lists.freedesktop.org/archives/intel-gfx/2015-December/082976.html Cc: Chris Wilson Cc: Takashi Iwai Cc: Jani Nikula Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 5 - drivers/gpu/drm/i915/intel_drv.h | 1 - 2 files changed, 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e9bb860a55dc..f2a0151b3f14 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14105,9 +14105,6 @@ intel_commit_cursor_plane(struct drm_plane *plane, crtc = crtc ? crtc : plane->crtc; intel_crtc = to_intel_crtc(crtc); - if (intel_crtc->cursor_bo == obj) - goto update; - if (!obj) addr = 0; else if (!INTEL_INFO(dev)->cursor_needs_physical) @@ -14116,9 +14113,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, addr = obj->phys_handle->busaddr; intel_crtc->cursor_addr = addr; - intel_crtc->cursor_bo = obj; -update: intel_crtc_update_cursor(crtc, state->visible); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 76dfa286cd95..6324c782d062 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -568,7 +568,6 @@ struct intel_crtc { int adjusted_x; int adjusted_y; - struct drm_i915_gem_object *cursor_bo; uint32_t cursor_addr; uint32_t cursor_cntl; uint32_t cursor_size; -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/atomic-helper: Drop unneeded argument from check_pending_encoder
On Tue, Dec 08, 2015 at 09:49:18AM +0100, Daniel Vetter wrote: > Just a remnant from an old iteration of this patch that I've forgotten > to remove: We only need the encoder to figure out whether it has been > reassigned in this update already or not to figure out whether there's > a conflict or not. > > Reported-by: Thierry Reding> Cc: Thierry Reding > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_atomic_helper.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/11] drm/i915: Introduce drm_i915_gem_request_node for request tracking
Hi, On 14/12/15 11:36, Chris Wilson wrote: In the next patch, request tracking is made more generic and for that we need a new expanded struct and to separate out the logic changes from the mechanical churn, we split out the structure renaming into this patch. For extra fun, create a new i915_gem_request.h. Subject and patch disagree on the new structure name. :) Could you draw a nice diagram demonstrating the new design? It is not straightforward to derive it from the patch series. Emphasis on relationships between engines, requests and request_active etc. Also I notice even though you later add vma->last_read, I don't see that obj->last_read is never removed. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/10] drm/i915: Fixes from my attempt at running igt on gen2
From: Ville SyrjäläIt's been a while since I last ran igt on gen2, so I figured I'd give it a shot. 855 had some failures, 830 no longer worked at all. So I went ahead and fixed them, and here's the result. The first three patches are not even gen2 specific bugs I caught with this effort. The rest is for gen2. I have some fixes for igt as well, which I'll post separately. The good news is that with these patches (and the igt fixes) my 855 completes a full kms_flip run without failures, and the BAT set has only one failure (gem_render_tiled_blits). 830 is fairly good too, but it does have a lot of underruns and pipe_assert() dmesg warnings. Lot of those are due to the pipe enable quirks since we implement those quite haphazardly. The series is available here: git://github.com/vsyrjala/linux.git gen2_igt_fixes Ville Syrjälä (10): drm/i915: Release mmaps on partial ggtt vma unbind drm/i915: Cleanup phys status page too drm/i915: Write out crc frame counts in hex drm/i915: Wait for pipe to start before sampling vblank timestamps on gen2 drm/i915: Use drm_vblank_count() on gen2 for crc frame count drm/i915: Enable vblank_disable_immediate on gen2 drm/i915: Allow 27 bytes child_dev for VBT <109 drm/i915: Expect child dev size of 22 bytes for VBT < 106 drm/i915: Reject < 8 byte batches on 830/845 drm/i915: Use MI_BATCH_BUFFER_START on 830/845 drivers/gpu/drm/i915/i915_debugfs.c| 13 - drivers/gpu/drm/i915/i915_gem.c| 3 +++ drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +++ drivers/gpu/drm/i915/i915_irq.c| 14 +- drivers/gpu/drm/i915/intel_bios.c | 21 drivers/gpu/drm/i915/intel_display.c | 11 +++ drivers/gpu/drm/i915/intel_ringbuffer.c| 31 +++--- 7 files changed, 71 insertions(+), 25 deletions(-) -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/10] drm/i915: Release mmaps on partial ggtt vma unbind
From: Ville SyrjäläWhen a partial ggtt vma gets evicted, we need to zap any CPU mapping to said vma as well. Currently we zap the mappings only when the normal gtt vma gets evicted, but for partial vmas we leave behind stale CPU mappins. And so, if something else gets bound into the same gtt address range, any userspace access into the relevant virtual addresses will go astray. I didn't find anything really suitable in the mm code to zap just the needed mappings (we'd need to know the right CPU side mm and vma etc.), so let's just call i915_gem_release_mmap() for now. Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Chris Wilson Testcase: igt/gem_mmap_gtt Fixes: c5ad54c ("drm/i915: Use partial view in mmap fault handler") Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 66b170598ae6..c29b929f796c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3264,6 +3264,9 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait) ret = i915_gem_object_put_fence(obj); if (ret) return ret; + } else if (i915_is_ggtt(vma->vm) && + vma->ggtt_view.type == I915_GGTT_VIEW_PARTIAL) { + i915_gem_release_mmap(obj); } trace_i915_vma_unbind(vma); -- 2.4.10 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Correct max delay for HDMI hotplug live status checking
The total delay of HDMI hotplug detecting with 30ms have already been split into a resolution of 3 retries of 10ms each, for the worst cases. But it still suffered from only waiting 10ms at most in intel_hdmi_detect(). This patch corrects it by reading hotplug status with 4 times at most for 30ms delay. v2: - straight up to loop execution for more clear in code readability - mdelay will replace with msleep by Daniel's new patch drm/i915: mdelay(10) considered harmful - suggest to re-evaluate try times for being compatible to old HDMI monitor Reviewed-by: Cooper ChiouTested-by: Gary Wang Cc: Jani Nikula Cc: Daniel Vetter Cc: Gavin Hindman Cc: Sonika Jindal Cc: Shashank Sharma Signed-off-by: Gary Wang --- drivers/gpu/drm/i915/intel_hdmi.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) mode change 100644 => 100755 drivers/gpu/drm/i915/intel_hdmi.c diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c old mode 100644 new mode 100755 index 6825543..912d6c3 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1387,17 +1387,18 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct drm_i915_private *dev_priv = to_i915(connector->dev); bool live_status = false; - unsigned int retry = 3; + unsigned int try; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - while (!live_status && --retry) { + for (try = 0; !live_status && try < 4; try++) { + if (try) + mdelay(10); live_status = intel_digital_port_connected(dev_priv, hdmi_to_dig_port(intel_hdmi)); - mdelay(10); } if (!live_status) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/i915: Support for creating Stolen memory backed objects
On Mon, 2015-12-14 at 10:05 +, Chris Wilson wrote: > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index d727b49..ebce8c9 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -357,6 +357,7 @@ typedef struct drm_i915_irq_wait { > > #define I915_PARAM_HAS_GPU_RESET35 > > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > > +#define I915_PARAM_CREATE_VERSION 38 > > > > typedef struct drm_i915_getparam { > > __s32 param; > > @@ -456,6 +457,21 @@ struct drm_i915_gem_create { > > */ > > __u32 handle; > > __u32 pad; > > + /** > > +* Requested flags (currently used for placement > > +* (which memory domain)) > > +* > > +* You can request that the object be created from special memory > > +* rather than regular system pages using this parameter. Such > > +* irregular objects may have certain restrictions (such as CPU > > +* access to a stolen object is verboten). > > +* > > +* This can be used in the future for other purposes too > > +* e.g. specifying tiling/caching/madvise > > +*/ > > + __u32 flags; > > +#define I915_CREATE_PLACEMENT_STOLEN (1<<0) /* Cannot use CPU mmaps > > */ > > +#define __I915_CREATE_UNKNOWN_FLAGS-(I915_CREATE_PLACEMENT_STOLEN > > << 1) > > Alignment. sizeof(drm_i915_gem_create) must be aligned to u64 since we > contain u64 (to keep ABI compat for 32bit). > -Chris Sure, will update __u32 flags to __64 flags Thanks, Ankit > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 10/11] drm/i915/debugfs: add a separate debugfs file for VBT
In the future the VBT might not be in mailbox #4 of the ACPI OpRegion, thus unavailable in i915_opregion, so add a separate file for the VBT. v2: Drop the locking as unneeded (Chris) Cc: Chris WilsonSigned-off-by: Jani Nikula --- drivers/gpu/drm/i915/i915_debugfs.c | 14 ++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_opregion.c | 4 +++- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index a9e1f18c36d1..0fc38bb7276c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1857,6 +1857,19 @@ out: return 0; } +static int i915_vbt(struct seq_file *m, void *unused) +{ + struct drm_info_node *node = m->private; + struct drm_device *dev = node->minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_opregion *opregion = _priv->opregion; + + if (opregion->vbt) + seq_write(m, opregion->vbt, opregion->vbt_size); + + return 0; +} + static int i915_gem_framebuffer_info(struct seq_file *m, void *data) { struct drm_info_node *node = m->private; @@ -5325,6 +5338,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_ips_status", i915_ips_status, 0}, {"i915_sr_status", i915_sr_status, 0}, {"i915_opregion", i915_opregion, 0}, + {"i915_vbt", i915_vbt, 0}, {"i915_gem_framebuffer", i915_gem_framebuffer_info, 0}, {"i915_context_status", i915_context_status, 0}, {"i915_dump_lrc", i915_dump_lrc, 0}, diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 10e47167c594..ca8c2a64bc6d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -458,6 +458,7 @@ struct intel_opregion { u32 swsci_sbcb_sub_functions; struct opregion_asle *asle; const void *vbt; + u32 vbt_size; u32 *lid_state; struct work_struct asle_work; }; diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index f9403b1c8fdd..e89ee2383fe1 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -990,8 +990,10 @@ int intel_opregion_setup(struct drm_device *dev) const void *vbt = base + OPREGION_VBT_OFFSET; u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; - if (intel_bios_is_valid_vbt(vbt, vbt_size)) + if (intel_bios_is_valid_vbt(vbt, vbt_size)) { opregion->vbt = vbt; + opregion->vbt_size = vbt_size; + } } return 0; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/11] drm/i915: don't use a temp buffer for opregion debugfs file
On Mon, Dec 14, 2015 at 12:50:53PM +0200, Jani Nikula wrote: > Hasn't been necessary since > > commit 115719fceaa733d646e39cdce83cc32ddb891a49 > Author: Williams, Dan J> Date: Mon Oct 12 21:12:57 2015 + > > i915: switch from acpi_os_ioremap to memremap > > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/i915_debugfs.c | 11 ++- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 24318b79bcfc..a9e1f18c36d1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1842,25 +1842,18 @@ static int i915_opregion(struct seq_file *m, void > *unused) > struct drm_device *dev = node->minor->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_opregion *opregion = _priv->opregion; > - void *data = kmalloc(OPREGION_SIZE, GFP_KERNEL); > int ret; > > - if (data == NULL) > - return -ENOMEM; > - > ret = mutex_lock_interruptible(>struct_mutex); > if (ret) > goto out; A bit off topic, but I wonder what this locking is supposed to protect? > > - if (opregion->header) { > - memcpy(data, opregion->header, OPREGION_SIZE); > - seq_write(m, data, OPREGION_SIZE); > - } > + if (opregion->header) > + seq_write(m, opregion->header, OPREGION_SIZE); > > mutex_unlock(>struct_mutex); > > out: > - kfree(data); > return 0; > } > > -- > 2.1.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Clean up GPU hang message
On Mon, Dec 14, 2015 at 11:39:47AM +, Chris Wilson wrote: > On Mon, Dec 14, 2015 at 11:28:39AM +, Dave Gordon wrote: > > If this is a test that has disabled error state > > capture (because it's *trying* to hang one or more rings) can we > > still know how many rings have been diagnosed as hung? > > If you have a use case, then you can look at the interface you require. > i915_hangcheck_info should be sufficient in most cases, or at least a > good starting point. But you may want a more specific debugfs to avoid > parsing. Gah, even better is just to use the reset-stats ioctl! -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/32] drm/i915: Slaughter the thundering i915_wait_request herd
On Mon, Dec 14, 2015 at 12:21:32PM +, Tvrtko Ursulin wrote: > >@@ -1229,18 +1219,13 @@ int __i915_wait_request(struct drm_i915_gem_request > >*req, > > s64 *timeout, > > struct intel_rps_client *rps) > > { > >-struct intel_engine_cs *ring = i915_gem_request_get_ring(req); > >-struct drm_device *dev = ring->dev; > >-struct drm_i915_private *dev_priv = dev->dev_private; > >-const bool irq_test_in_progress = > >-ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & > >intel_ring_flag(ring); > > int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; > >-DEFINE_WAIT(wait); > >-unsigned long timeout_expire; > >+struct intel_breadcrumb wait; > >+unsigned long timeout_remain; > > s64 before, now; > >-int ret; > >+int ret = 0; > > > >-WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled"); > >+might_sleep(); > > Why this suddenly? No reason other than reading other patches and thought this would make good annotation for this function. > > /* Wake up __wait_seqno, potentially holding dev->struct_mutex. */ > > for_each_ring(ring, dev_priv, i) > >-wake_up_all(>irq_queue); > >+intel_engine_wakeup(ring); > > This is from process context without a lock or synchronize rcu. I'll > comment on it at the implementation site as well. True. We could just do rcu_read_lock() here. > > > > /* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */ > > wake_up_all(_priv->pending_flip_queue); > >@@ -2986,16 +2985,17 @@ static void i915_hangcheck_elapsed(struct > >work_struct *work) > > if (ring_idle(ring, seqno)) { > > ring->hangcheck.action = HANGCHECK_IDLE; > > > >-if (waitqueue_active(>irq_queue)) { > >+if (intel_engine_has_waiter(ring)) { > > /* Issue a wake-up to catch stuck h/w. > > */ > > if (!test_and_set_bit(ring->id, > > _priv->gpu_error.missed_irq_rings)) { > >-if > >(!(dev_priv->gpu_error.test_irq_rings & intel_ring_flag(ring))) > >+if (!test_bit(ring->id, > >_priv->gpu_error.test_irq_rings)) > > DRM_ERROR("Hangcheck > > timer elapsed... %s idle\n", > > ring->name); > > else > > DRM_INFO("Fake missed > > irq on %s\n", > > ring->name); > >-wake_up_all(>irq_queue); > >+ > >+ > >intel_engine_enable_fake_irq(ring); > > } > > /* Safeguard against driver failure */ > > ring->hangcheck.score += BUSY; > >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c > >b/drivers/gpu/drm/i915/intel_breadcrumbs.c > >new file mode 100644 > >index ..a9a199bca2c2 > >--- /dev/null > >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c > >@@ -0,0 +1,274 @@ > >+/* > >+ * Copyright © 2015 Intel Corporation > >+ * > >+ * Permission is hereby granted, free of charge, to any person obtaining a > >+ * copy of this software and associated documentation files (the > >"Software"), > >+ * to deal in the Software without restriction, including without limitation > >+ * the rights to use, copy, modify, merge, publish, distribute, sublicense, > >+ * and/or sell copies of the Software, and to permit persons to whom the > >+ * Software is furnished to do so, subject to the following conditions: > >+ * > >+ * The above copyright notice and this permission notice (including the next > >+ * paragraph) shall be included in all copies or substantial portions of the > >+ * Software. > >+ * > >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > >OR > >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > >OTHER > >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > >+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > >DEALINGS > >+ * IN THE SOFTWARE. > >+ * > >+ */ > >+ > >+#include "i915_drv.h" > >+ > >+static void intel_breadcrumbs_fake_irq(unsigned long data) > >+{ > >+struct intel_breadcrumbs *b = (struct intel_breadcrumbs *)data; > >+struct task_struct *task; > >+ > >+/* > >+ * The timer persists in case we cannot enable interrupts, > >+ * or if we have previously seen
Re: [Intel-gfx] [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB
On Mon, 14 Dec 2015, Ville Syrjäläwrote: > On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote: >> The RVDA and RVDS (raw VBT data address and size) fields of the ASLE >> mailbox may specify an alternate location for VBT instead of mailbox #4. >> Use the alternate location if available and valid, falling back to >> mailbox #4 otherwise. >> >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_opregion.c | 25 +++-- >> 2 files changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index ca8c2a64bc6d..8cfac7398568 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -457,6 +457,7 @@ struct intel_opregion { >> u32 swsci_gbda_sub_functions; >> u32 swsci_sbcb_sub_functions; >> struct opregion_asle *asle; >> +void *rvda; >> const void *vbt; >> u32 vbt_size; >> u32 *lid_state; >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c >> b/drivers/gpu/drm/i915/intel_opregion.c >> index e89ee2383fe1..a139889dd45b 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev) >> >> /* just clear all opregion memory pointers now */ >> memunmap(opregion->header); >> +if (opregion->rvda) { >> +memunmap(opregion->rvda); >> +opregion->rvda = NULL; >> +} >> opregion->header = NULL; >> opregion->acpi = NULL; >> opregion->swsci = NULL; >> @@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev) >> ` DRM_DEBUG_DRIVER("ASLE extension supported\n"); >> >> if (!dmi_check_system(intel_no_opregion_vbt)) { >> -const void *vbt = base + OPREGION_VBT_OFFSET; >> -u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; >> +const void *vbt = NULL; >> +u32 vbt_size = 0; >> + >> +if (opregion->header->opregion_ver >= 2 && opregion->asle && >> +opregion->asle->rvda && opregion->asle->rvds) { > > Either I'm blind or you didn't actually add rvda/rvds to struct opregion_asle. > > Also the spec seems confused as usual. Some parts if it refer to this > as RVDA others as RVBT. Although RVBT also seems to be what the mbox #4 > contents are called in another place, and to add insult to injury that > place also has the offset and size all wrong. Sigh. > > Anyway, apart from the missing rvda/rvds definititions the rest looks OK. Those are in Deepak's commit I pushed last week: commit c85f6c91ec4218487a29a7d1f918f7f6f0424c75 Author: Deepak M Date: Tue Dec 1 04:17:05 2015 +0530 drm/i915: add VBT address and size fields to ASLE mailbox struct I did build this, and even booted it. ;) BR, Jani. > >> +opregion->rvda = memremap(opregion->asle->rvda, >> + opregion->asle->rvds, >> + MEMREMAP_WB); >> +vbt = opregion->rvda; >> +vbt_size = opregion->asle->rvds; >> +} >> >> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >> opregion->vbt = vbt; >> opregion->vbt_size = vbt_size; >> +DRM_DEBUG_DRIVER("VBT from RVDA\n"); >> +} else { >> +vbt = base + OPREGION_VBT_OFFSET; >> +vbt_size = OPREGION_ASLE_EXT_OFFSET - >> OPREGION_VBT_OFFSET; >> +if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >> +opregion->vbt = vbt; >> +opregion->vbt_size = vbt_size; >> +} >> } >> } >> >> -- >> 2.1.4 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: refactor VBT validation
On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote: > Make the validation function a boolean operating on a buffer of given > size, removing the extra pointer dances. > > Move the OpRegion based VBT validation to intel_opregion_setup(), only > initializing opregion->vbt if it's valid. > > Signed-off-by: Jani Nikula> --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 63 > ++- > drivers/gpu/drm/i915/intel_opregion.c | 9 +++-- > 3 files changed, 40 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a2bd4c6a86a1..ef15f1710b50 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev); > > /* intel_bios.c */ > int intel_bios_init(struct drm_device *dev); > +bool intel_bios_is_valid_vbt(const void *buf, size_t size); > > /* intel_opregion.c */ > #ifdef CONFIG_ACPI > diff --git a/drivers/gpu/drm/i915/intel_bios.c > b/drivers/gpu/drm/i915/intel_bios.c > index f2b79b7d12b8..007b2b759600 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const > struct vbt_header *vbt) > return _vbt + vbt->bdb_offset; > } > > -static const struct vbt_header *validate_vbt(const void *base, > - size_t size, > - const void *_vbt) > +/** > + * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT > + * @buf: pointer to a buffer to validate > + * @size:size of the buffer > + * > + * Returns true on valid VBT. > + */ > +bool intel_bios_is_valid_vbt(const void *buf, size_t size) > { > - size_t offset = _vbt - base; > - const struct vbt_header *vbt = _vbt; > + const struct vbt_header *vbt = buf; > const struct bdb_header *bdb; > > if (!vbt) > - return NULL; > + return false; > > - if (offset + sizeof(struct vbt_header) > size) { > + if (sizeof(struct vbt_header) > size) { > DRM_DEBUG_DRIVER("VBT header incomplete\n"); > - return NULL; > + return false; > } > > if (memcmp(vbt->signature, "$VBT", 4)) { > DRM_DEBUG_DRIVER("VBT invalid signature\n"); > - return NULL; > + return false; > } > > - offset += vbt->bdb_offset; > - if (offset + sizeof(struct bdb_header) > size) { > + if (vbt->bdb_offset + sizeof(struct bdb_header) > size) { > DRM_DEBUG_DRIVER("BDB header incomplete\n"); > - return NULL; > + return false; > } > > bdb = get_bdb_header(vbt); > - if (offset + bdb->bdb_size > size) { > + if (vbt->bdb_offset + bdb->bdb_size > size) { > DRM_DEBUG_DRIVER("BDB incomplete\n"); > - return NULL; > + return false; > } > > return vbt; > @@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const > void *base, > > static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) > { > - const struct vbt_header *vbt = NULL; > size_t i; > > /* Scour memory looking for the VBT signature. */ > for (i = 0; i + 4 < size; i++) { > - if (ioread32(bios + i) == *((const u32 *) "$VBT")) { > - /* > - * This is the one place where we explicitly discard the > - * address space (__iomem) of the BIOS/VBT. From now on > - * everything is based on 'base', and treated as regular > - * memory. > - */ > - void *_bios = (void __force *) bios; > + void *vbt; > > - vbt = validate_vbt(_bios, size, _bios + i); > - break; > - } > + if (ioread32(bios + i) != *((const u32 *) "$VBT")) > + continue; > + > + /* > + * This is the one place where we explicitly discard the address > + * space (__iomem) of the BIOS/VBT. > + */ > + vbt = (void __force *) bios + i; > + if (intel_bios_is_valid_vbt(vbt, size - i)) > + return vbt; > + > + break; > } > > - return vbt; > + return NULL; > } > > /** > @@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct pci_dev *pdev = dev->pdev; > - const struct vbt_header *vbt; > + const struct vbt_header *vbt = dev_priv->opregion.vbt; > const struct bdb_header *bdb; > u8 __iomem *bios = NULL; > > @@ -1304,9 +1308,6 @@
Re: [Intel-gfx] [PATCH] Revert "drm/i915: Clean up associated VMAs on context destruction"
On Mon, Dec 14, 2015 at 01:54:38PM +, Chris Wilson wrote: > This reverts commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae. > > The patch was incomplete, introduced more problems of greater severity > than it solved and worse the solution was known and available at the > time of the patch. Bad send-email, see the tail of the 11/11 series for this in its proper sequence. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/11] drm/i915/opregion: handle VBT sizes bigger than 6 KB
On Mon, Dec 14, 2015 at 12:50:55PM +0200, Jani Nikula wrote: > The RVDA and RVDS (raw VBT data address and size) fields of the ASLE > mailbox may specify an alternate location for VBT instead of mailbox #4. > Use the alternate location if available and valid, falling back to > mailbox #4 otherwise. > > Signed-off-by: Jani Nikula> --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_opregion.c | 25 +++-- > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ca8c2a64bc6d..8cfac7398568 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -457,6 +457,7 @@ struct intel_opregion { > u32 swsci_gbda_sub_functions; > u32 swsci_sbcb_sub_functions; > struct opregion_asle *asle; > + void *rvda; > const void *vbt; > u32 vbt_size; > u32 *lid_state; > diff --git a/drivers/gpu/drm/i915/intel_opregion.c > b/drivers/gpu/drm/i915/intel_opregion.c > index e89ee2383fe1..a139889dd45b 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -837,6 +837,10 @@ void intel_opregion_fini(struct drm_device *dev) > > /* just clear all opregion memory pointers now */ > memunmap(opregion->header); > + if (opregion->rvda) { > + memunmap(opregion->rvda); > + opregion->rvda = NULL; > + } > opregion->header = NULL; > opregion->acpi = NULL; > opregion->swsci = NULL; > @@ -987,12 +991,29 @@ int intel_opregion_setup(struct drm_device *dev) > ` DRM_DEBUG_DRIVER("ASLE extension supported\n"); > > if (!dmi_check_system(intel_no_opregion_vbt)) { > - const void *vbt = base + OPREGION_VBT_OFFSET; > - u32 vbt_size = OPREGION_ASLE_EXT_OFFSET - OPREGION_VBT_OFFSET; > + const void *vbt = NULL; > + u32 vbt_size = 0; > + > + if (opregion->header->opregion_ver >= 2 && opregion->asle && > + opregion->asle->rvda && opregion->asle->rvds) { Either I'm blind or you didn't actually add rvda/rvds to struct opregion_asle. Also the spec seems confused as usual. Some parts if it refer to this as RVDA others as RVBT. Although RVBT also seems to be what the mbox #4 contents are called in another place, and to add insult to injury that place also has the offset and size all wrong. Sigh. Anyway, apart from the missing rvda/rvds definititions the rest looks OK. > + opregion->rvda = memremap(opregion->asle->rvda, > + opregion->asle->rvds, > + MEMREMAP_WB); > + vbt = opregion->rvda; > + vbt_size = opregion->asle->rvds; > + } > > if (intel_bios_is_valid_vbt(vbt, vbt_size)) { > opregion->vbt = vbt; > opregion->vbt_size = vbt_size; > + DRM_DEBUG_DRIVER("VBT from RVDA\n"); > + } else { > + vbt = base + OPREGION_VBT_OFFSET; > + vbt_size = OPREGION_ASLE_EXT_OFFSET - > OPREGION_VBT_OFFSET; > + if (intel_bios_is_valid_vbt(vbt, vbt_size)) { > + opregion->vbt = vbt; > + opregion->vbt_size = vbt_size; > + } > } > } > > -- > 2.1.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/11] drm/i915: refactor VBT validation
On Mon, 14 Dec 2015, Ville Syrjäläwrote: > On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote: >> Make the validation function a boolean operating on a buffer of given >> size, removing the extra pointer dances. >> >> Move the OpRegion based VBT validation to intel_opregion_setup(), only >> initializing opregion->vbt if it's valid. >> >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_bios.c | 63 >> ++- >> drivers/gpu/drm/i915/intel_opregion.c | 9 +++-- >> 3 files changed, 40 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index a2bd4c6a86a1..ef15f1710b50 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev); >> >> /* intel_bios.c */ >> int intel_bios_init(struct drm_device *dev); >> +bool intel_bios_is_valid_vbt(const void *buf, size_t size); >> >> /* intel_opregion.c */ >> #ifdef CONFIG_ACPI >> diff --git a/drivers/gpu/drm/i915/intel_bios.c >> b/drivers/gpu/drm/i915/intel_bios.c >> index f2b79b7d12b8..007b2b759600 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const >> struct vbt_header *vbt) >> return _vbt + vbt->bdb_offset; >> } >> >> -static const struct vbt_header *validate_vbt(const void *base, >> - size_t size, >> - const void *_vbt) >> +/** >> + * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT >> + * @buf:pointer to a buffer to validate >> + * @size: size of the buffer >> + * >> + * Returns true on valid VBT. >> + */ >> +bool intel_bios_is_valid_vbt(const void *buf, size_t size) >> { >> -size_t offset = _vbt - base; >> -const struct vbt_header *vbt = _vbt; >> +const struct vbt_header *vbt = buf; >> const struct bdb_header *bdb; >> >> if (!vbt) >> -return NULL; >> +return false; >> >> -if (offset + sizeof(struct vbt_header) > size) { >> +if (sizeof(struct vbt_header) > size) { >> DRM_DEBUG_DRIVER("VBT header incomplete\n"); >> -return NULL; >> +return false; >> } >> >> if (memcmp(vbt->signature, "$VBT", 4)) { >> DRM_DEBUG_DRIVER("VBT invalid signature\n"); >> -return NULL; >> +return false; >> } >> >> -offset += vbt->bdb_offset; >> -if (offset + sizeof(struct bdb_header) > size) { >> +if (vbt->bdb_offset + sizeof(struct bdb_header) > size) { >> DRM_DEBUG_DRIVER("BDB header incomplete\n"); >> -return NULL; >> +return false; >> } >> >> bdb = get_bdb_header(vbt); >> -if (offset + bdb->bdb_size > size) { >> +if (vbt->bdb_offset + bdb->bdb_size > size) { >> DRM_DEBUG_DRIVER("BDB incomplete\n"); >> -return NULL; >> +return false; >> } >> >> return vbt; >> @@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const >> void *base, >> >> static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) >> { >> -const struct vbt_header *vbt = NULL; >> size_t i; >> >> /* Scour memory looking for the VBT signature. */ >> for (i = 0; i + 4 < size; i++) { >> -if (ioread32(bios + i) == *((const u32 *) "$VBT")) { >> -/* >> - * This is the one place where we explicitly discard the >> - * address space (__iomem) of the BIOS/VBT. From now on >> - * everything is based on 'base', and treated as regular >> - * memory. >> - */ >> -void *_bios = (void __force *) bios; >> +void *vbt; >> >> -vbt = validate_vbt(_bios, size, _bios + i); >> -break; >> -} >> +if (ioread32(bios + i) != *((const u32 *) "$VBT")) >> +continue; >> + >> +/* >> + * This is the one place where we explicitly discard the address >> + * space (__iomem) of the BIOS/VBT. >> + */ >> +vbt = (void __force *) bios + i; >> +if (intel_bios_is_valid_vbt(vbt, size - i)) >> +return vbt; >> + >> +break; >> } >> >> -return vbt; >> +return NULL; >> } >> >> /** >> @@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct pci_dev *pdev = dev->pdev; >> -const struct vbt_header *vbt; >> +const struct
Re: [Intel-gfx] [PATCH 13/13] drm/i915: Cache last IRQ seqno to reduce IRQ overhead
On 14/12/15 11:58, John Harrison wrote: On 11/12/2015 14:28, Tvrtko Ursulin wrote: On 11/12/15 13:12, john.c.harri...@intel.com wrote: From: John HarrisonThe notify function can be called many times without the seqno changing. A large number of duplicates are to prevent races due to the requirement of not enabling interrupts until requested. However, when interrupts are enabled the IRQ handle can be called multiple times without the ring's seqno value changing. This patch reduces the overhead of these extra calls by caching the last processed seqno value and early exiting if it has not changed. v3: New patch for series. For: VIZ-5190 Signed-off-by: John Harrison --- drivers/gpu/drm/i915/i915_gem.c | 14 +++--- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 279d79f..3c88678 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2457,6 +2457,8 @@ i915_gem_init_seqno(struct drm_device *dev, u32 seqno) for (j = 0; j < ARRAY_SIZE(ring->semaphore.sync_seqno); j++) ring->semaphore.sync_seqno[j] = 0; + +ring->last_irq_seqno = 0; } return 0; @@ -2788,11 +2790,14 @@ void i915_gem_request_notify(struct intel_engine_cs *ring, bool fence_locked) return; } -if (!fence_locked) -spin_lock_irqsave(>fence_lock, flags); - seqno = ring->get_seqno(ring, false); trace_i915_gem_request_notify(ring, seqno); +if (seqno == ring->last_irq_seqno) +return; +ring->last_irq_seqno = seqno; Hmmm.. do you want to make the check "seqno <= ring->last_irq_seqno" ? Is there a possibility for some weird timing or caching issue where two callers get in and last_irq_seqno goes backwards? Not sure that it would cause a problem, but pattern is unusual and hard to understand for me. The check is simply to prevent repeat processing of identical seqno values. The 'last_' value is never used for anything more complicated. If there is a very rare race condition where the repeat processing can still happen, it doesn't really matter too much. Also check and the assignment would need to be under the spinlock I think. The whole point is to not grab the spinlock if there is no work to do. Hence the seqno read and test must be done first. The assignment could potentially be done after the lock but if two different threads have made it that far concurrently then it doesn't really matter who does the write first. Most likely they are both processing the same seqno and in the really rare case of two concurrent threads actually reading two different (and both new) seqno values then there is no guarantee about which will take the lock first. So you are into the above situation of it doesn't really matter if there is then a third time around later that finds an 'incorrect' last value and goes through the processing sequence but with no work to do. I think it would be good to put that in the comment then. :) That you don't care about multiple notify processing running if the timing is right, or that you don't care if ring->last_irq_seqno does not reflect the last processed seqno. Etc. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] bisected: i915 modeset broken in ac9b8236551d1177fd07b56aef9b565d1864420d
Between 4.4-rc3 and 4.4-rc4, i915 modesetting broke on my i5-2400 PC. Instead of seeing the new dense graphics mode, I see the last VGA text lines and no X appears either. I saw something similar on I865G but have not had time to check if it is the same issue. ac9b8236551d1177fd07b56aef9b565d1864420d is the first bad commit commit ac9b8236551d1177fd07b56aef9b565d1864420d Author: Ville SyrjäläDate: Fri Nov 27 18:55:26 2015 +0200 drm/i915: Introduce a gmbus power domain Currently the gmbus code uses intel_aux_display_runtime_get/put in an effort to make sure the hardware is powered up sufficiently for gmbus. That function only takes the runtime PM reference which on VLV/CHV/BXT is not enough. We need the disp2d/pipe-a well on VLV/CHV and power well 2 on BXT. So add a new power domnain for gmbus and kill off the now unused intel_aux_display_runtime_get/put. And change intel_hdmi_set_edid() to use the gmbus power domain too since that's all we need there. Also toss in a BUILD_BUG_ON() to catch problems if we run out of bits for power domains. We're already really close to the limit... [Patrik: Add gmbus string to debugfs output] Signed-off-by: Ville Syrjälä Reviewed-by: Patrik Jakobsson [Cherry-picked from drm-intel-next-queued f0ab43e6 (Imre)] Signed-off-by: Imre Deak Link: http://patchwork.freedesktop.org/patch/msgid/1448643329-18675-3-git-send-email-imre.d...@intel.com Signed-off-by: Jani Nikula :04 04 39379146d7e6dda8a4d5f8781ee3d307cce8c47e f4f09fae0485ad6263d31d425296fa9cd7de343b M drivers -- Meelis Roos (mr...@linux.ee) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Force clean compilation with -Werror
Our driver compiles clean (nowadays thanks to 0day) but for me, at least, it would be beneficial if the compiler threw an error rather than a warning when it found a piece of suspect code. (I use this to compile-check patch series and want to break on the first compiler error in order to fix the patch.) v2: Kick off a new "Debugging" submenu for i915.ko v3: Avoid enabling -Werror for allyesconfig/allmodconfig builds, using COMPILE_TEST as a suitable proxy suggested by Andrew Morton. (Damien) Only make the option available for EXPERT to reinforce that the option should not be casually enabled. Signed-off-by: Chris WilsonCc: Jani Nikula Cc: Damien Lespiau --- drivers/gpu/drm/i915/Kconfig | 8 drivers/gpu/drm/i915/Kconfig.debug | 12 drivers/gpu/drm/i915/Makefile | 2 ++ 3 files changed, 22 insertions(+) create mode 100644 drivers/gpu/drm/i915/Kconfig.debug diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index fcd77b27514d..d493ec4c7399 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -48,3 +48,11 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT option changes the default for that module option. If in doubt, say "N". + +menu "DRM i915 Debugging" + +depends on DRM_I915 + +source drivers/gpu/drm/i915/Kconfig.debug + +endmenu diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug new file mode 100644 index ..1f10ee228eda --- /dev/null +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -0,0 +1,12 @@ +config DRM_I915_WERROR + bool "Force GCC to throw an error instead of a warning when compiling" + default n + # As this may inadvertently break the build, only allow the user + # to shoot oneself in the foot iff they aim really hard + depends on EXPERT + # We use the dependency on !COMPILE_TEST to not be enabled in + # allmodconfig or allyesconfig configurations + depends on !COMPILE_TEST + ---help--- + Add -Werror to the build flags for (and only for) i915.ko. + Do not enable this unless you are writing code for the i915.ko module. diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 0851de07bd13..1e9895b9a546 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -2,6 +2,8 @@ # Makefile for the drm device driver. This driver provides support for the # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. +subdir-ccflags-$(CONFIG_DRM_I915_WERROR) := -Werror + # Please keep these build lists sorted! # core driver code -- 2.6.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx