On Tue, 2015-12-22 at 10:44 +0000, Tvrtko Ursulin wrote:
> Hi,
>
> On 22/12/15 06:20, ankitprasad.r.sharma@xxxxxxxxx wrote:
> > From: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> >
> > 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)
> >
> > v5: Renamed wrapper function for drm_mm_insert_node_in_range (Chris)
> >
> > v5: Added wrapper for drm_mm_remove_node() (Chris)
> >
> > v6: Added get_pages call before pinning the pages (Tvrtko)
> > Added remove_mappable_node() wrapper for drm_mm_remove_node() (Chris)
> >
> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 94 +++++++++++++++++++++++++++++++----------
> > 1 file changed, 72 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index bf7f203..f11ec89 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -61,6 +61,23 @@ static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
> > return obj->pin_display;
> > }
> >
> > +static int
> > +insert_mappable_node(struct drm_i915_private *i915,
> > + struct drm_mm_node *node)
> > +{
> > + return drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm, node,
> > + PAGE_SIZE, 0, 0,
>
> It does not look ideal that the function name is saying it will insert a
> node and then it only inserts one page.
>
> In that respect previous version looked better to me but since it is
> static and single use - whatever - this has been dragging for too long.
We can take size as well, as a parameter passed to the wrapper.
>
> > + 0, i915->gtt.mappable_end,
> > + DRM_MM_SEARCH_DEFAULT,
> > + DRM_MM_CREATE_DEFAULT);
> > +}
> > +
> > +static void
> > +remove_mappable_node(struct drm_mm_node *node)
> > +{
> > + drm_mm_remove_node(node);
> > +}
> > +
> > /* some bookkeeping */
> > static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
> > size_t size)
> > @@ -760,20 +777,34 @@ fast_user_write(struct io_mapping *mapping,
> > * user into the GTT, uncached.
> > */
> > static int
> > -i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> > +i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
> > struct drm_i915_gem_object *obj,
> > struct drm_i915_gem_pwrite *args,
> > struct drm_file *file)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - ssize_t remain;
> > - loff_t offset, page_base;
> > + struct drm_mm_node node;
> > + uint64_t remain, offset;
> > char __user *user_data;
> > - int page_offset, page_length, ret;
> > + int ret;
> >
> > ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
> > - if (ret)
> > - goto out;
> > + if (ret) {
> > + memset(&node, 0, sizeof(node));
> > + ret = insert_mappable_node(i915, &node);
> > + if (ret)
> > + goto out;
> > +
> > + ret = i915_gem_object_get_pages(obj);
> > + if (ret) {
> > + remove_mappable_node(&node);
> > + goto out;
> > + }
> > +
> > + i915_gem_object_pin_pages(obj);
> > + } else {
> > + node.start = i915_gem_obj_ggtt_offset(obj);
> > + node.allocated = false;
> > + }
> >
> > ret = i915_gem_object_set_to_gtt_domain(obj, true);
> > if (ret)
> > @@ -783,31 +814,39 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> > if (ret)
> > goto out_unpin;
> >
> > - user_data = to_user_ptr(args->data_ptr);
> > - remain = args->size;
> > -
> > - offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
> > -
> > intel_fb_obj_invalidate(obj, ORIGIN_GTT);
> > + obj->dirty = true;
> >
> > - while (remain > 0) {
> > + user_data = to_user_ptr(args->data_ptr);
> > + offset = args->offset;
> > + remain = args->size;
> > + while (remain) {
> > /* Operation in this page
> > *
> > * page_base = page offset within aperture
> > * page_offset = offset within page
> > * page_length = bytes to copy for this page
> > */
> > - page_base = offset & PAGE_MASK;
> > - page_offset = offset_in_page(offset);
> > - page_length = remain;
> > - if ((page_offset + remain) > PAGE_SIZE)
> > - page_length = PAGE_SIZE - page_offset;
> > -
> > + u32 page_base = node.start;
>
> Compiler does not complain about possible truncation here? I would
> change it to a type of equivalent width just in case. Even before it was
> loff_t.
Never saw a compiler warning related to this truncation. Though this is
not going to affect the functionality, I will update it to u64.
Thanks,
Ankit
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
