On Sun, Jul 03, 2016 at 05:51:35PM +0530, Goel, Akash wrote:
> 
> 
> On 7/3/2016 2:45 PM, Chris Wilson wrote:
> >On Sun, Jul 03, 2016 at 12:21:22AM +0530, akash.g...@intel.com wrote:
> >>+static void guc_read_update_log_buffer(struct drm_device *dev, bool 
> >>capture_all)
> >>+{
> >>+   struct drm_i915_private *dev_priv = dev->dev_private;
> >>+   struct intel_guc *guc = &dev_priv->guc;
> >>+   struct guc_log_buffer_state *log_buffer_state;
> >>+   struct guc_log_buffer_state *log_buffer_copy_state;
> >>+   void *src_ptr, *dst_ptr;
> >>+   u32 num_pages_to_copy;
> >>+   int i;
> >>+
> >>+   if (!guc->log.obj)
> >>+           return;
> >>+
> >>+   num_pages_to_copy = guc->log.obj->base.size / PAGE_SIZE;
> >>+   /* Don't really need to copy crash buffer area in regular cases as there
> >>+    * won't be any unread data there.
> >>+    */
> >>+   if (!capture_all)
> >>+           num_pages_to_copy -= (GUC_LOG_CRASH_PAGES + 1);
> >>+
> >>+   log_buffer_state = src_ptr =
> >>+           kmap_atomic(i915_gem_object_get_page(guc->log.obj, 0));
> >
> >So why not use i915_gem_object_pin_map() from the start?
> >
> >That will cut down on the churn later.
> 
> Fine, will reorder the series and squash the other patch 'drm/i915:
> Use uncached(WC) mapping for accessing the GuC log buffer' with this
> patch.

I would keep the pin_map(false -> true) as a separate step so that it is
clearly documented (and reversible).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to