Re: [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Dave Gordon <david.s.gordon@intel.com>]

On 11/12/15 18:15, Daniel Vetter wrote:
> On Wed, Dec 09, 2015 at 07:39:56PM +0000, Dave Gordon wrote:
>> On 09/12/15 16:15, Tvrtko Ursulin wrote:
>>>
>>> Hi,
>>>
>>> On 09/12/15 12:46, ankitprasad.r.sharma@intel.com wrote:
>>>> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>>>>
>>>> This patch adds support for extending the pread/pwrite functionality
>>>> for objects not backed by shmem. The access will be made through
>>>> gtt interface. This will cover objects backed by stolen memory as well
>>>> as other non-shmem backed objects.
>>>>
>>>> v2: Drop locks around slow_user_access, prefault the pages before
>>>> access (Chris)
>>>>
>>>> v3: Rebased to the latest drm-intel-nightly (Ankit)
>>>>
>>>> v4: Moved page base & offset calculations outside the copy loop,
>>>> corrected data types for size and offset variables, corrected if-else
>>>> braces format (Tvrtko/kerneldocs)
>>>>
>>>> v5: Enabled pread/pwrite for all non-shmem backed objects including
>>>> without tiling restrictions (Ankit)
>>>>
>>>> v6: Using pwrite_fast for non-shmem backed objects as well (Chris)
>>>>
>>>> v7: Updated commit message, Renamed i915_gem_gtt_read to
>>>> i915_gem_gtt_copy,
>>>> added pwrite slow path for non-shmem backed objects (Chris/Tvrtko)
>>>>
>>>> v8: Updated v7 commit message, mutex unlock around pwrite slow path for
>>>> non-shmem backed objects (Tvrtko)
>>>>
>>>> Testcase: igt/gem_stolen
>>>>
>>>> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_gem.c | 151
>>>> +++++++++++++++++++++++++++++++++-------
>>>>   1 file changed, 127 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
[snip!]
>>>>   static int
>>>>   i915_gem_shmem_pread(struct drm_device *dev,
>>>>                struct drm_i915_gem_object *obj,
>>>> @@ -737,17 +830,14 @@ i915_gem_pread_ioctl(struct drm_device *dev,
>>>> void *data,
>>>>           goto out;
>>>>       }
>>>>
>>>> -    /* prime objects have no backing filp to GEM pread/pwrite
>>>> -     * pages from.
>>>> -     */
>>>> -    if (!obj->base.filp) {
>>>> -        ret = -EINVAL;
>>>> -        goto out;
>>>> -    }
>>>> -
>>>>       trace_i915_gem_object_pread(obj, args->offset, args->size);
>>>>
>>>> -    ret = i915_gem_shmem_pread(dev, obj, args, file);
>>>> +    /* pread for non shmem backed objects */
>>>> +    if (!obj->base.filp && obj->tiling_mode == I915_TILING_NONE)
>>>> +        ret = i915_gem_gtt_copy(dev, obj, args->size,
>>>> +                    args->offset, args->data_ptr);
>>>> +    else
>>>> +        ret = i915_gem_shmem_pread(dev, obj, args, file);
>>>
>>> Hm, it will end up calling i915_gem_shmem_pread for non-shmem backed
>>> objects if tiling is set. Sounds wrong to me unless I am missing something?
>>
>> Which GEM objects have obj->base.filp set? Is it ONLY regular gtt-type
>
> obj->base.filp is for shmem backed stuff. gtt is irrelevant for backing
> storage, well except if you can't read the shmem stuff directly with the
> cpu the only way is to go through a gtt device mapping.
> -Daniel

So obj->base.filp is set for both phys and shmem (default) object types; 
I called the latter a "gtt" type just because the get/put_pages() 
functions have "gtt" in their names). But I note that the naming of GEM 
object vfuncs (and vfunc tables) isn't consistent :( Maybe they should 
be named "i915_gem_object_{get,put}_pages_shmem()", and the table would 
then be i915_gem_object_shmem_ops :)

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx