[Intel-gfx] [PATCH 1/1 RFC] drivers/gpu/drm/i915:Documentation for batchbuffer submission

[Rogovin, Kevin]

Hi,

> These documentation improvements are much welcome, here are a few comments from me.

Thankyou!

>Quoting kevin.rogovin at intel.com (2018-02-16 16:04:22)
>> +Intel GPU Basics
>> +----------------
>> +
>> +An Intel GPU has multiple engines. There are several engine types.
>> +
>> +- RCS engine is for rendering 3D and performing compute, this is named `I915_EXEC_DEFAULT` in user space.

>I'd call out I915_EXEC_RENDER existence here and introduce I915_EXEC_DEFAULT as its own line.

I agree; though what exactly is I915_EXECI_DEFAULT supposed to mean? it appears (to me) as just an alias
for I915_EXEC_RENDER.

>> +- BCS is a blitting (copy) engine, this is named `I915_EXEC_BLT` in user space.
> +- VCS is a video encode and decode engine, this is named 
> +`I915_EXEC_BSD` in user space
>> +- VECS is video enhancement engine, this is named `I915_EXEC_VEBOX` in user space.
>> +
>> +The Intel GPU family is a familiy of integrated GPU's using Unified 
>> +Memory Access. For having the GPU "do work", user space will feed the 
>> +GPU batch buffers via one of the ioctls 
>> +`DRM_IOCTL_I915_GEM_EXECBUFFER`, `DRM_IOCTL_I915_GEM_EXECBUFFER2` or 
>> +`DRM_IOCTL_I915_GEM_EXECBUFFER2_WR`. Most such batchbuffers will 
>> +instruct the

> I'd also call out DRM_IOCTL_I915_GEM_EXECBUFFER to be legacy submission method and primarily mention I915_GEM_EXECBUFFER2_WR.

Agreed. 

>> +GPU to perform work (for example rendering) and that work needs 
>> +memory from which to read and memory to which to write. All memory is 
>> +encapsulated within GEM buffer objects (usually created with the ioctl DRM_IOCTL_I915_GEM_CREATE).
>> +An ioctl providing a batchbuffer for the GPU to create will also list 
>> +all GEM buffer objects that the batchbuffer reads and/or writes.
>> +

>In chronological order, maybe first introduce the hardware contexts?
>Only then go to PPGTT.

Sure.

- snip (quoting patch) -

>> +Common Code
>> +~~~~~~~~~~~
>> +
>> +.. kernel-doc:: drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +   :doc: User command execution
>> +
>> +.. kernel-doc:: drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +   :functions: i915_gem_do_execbuffer

>I'm not sure about referring to internal functions as they're bound to change often. No strong feeling on this, I just see this will be easy to miss when changing the related code.

I can place the text in the source file itself and have the .rst reference it, does this sound good?

>> +
>> +Batchbuffer Submission Varieties
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +As stated before, there are several varieties in how to submit 
>> +batchbuffers to the GPU; which one in use is controlled by function 
>> +pointer values in the c-struct intel_engine_cs (defined in 
>> +drivers/gpu/drm/i915/intel_ringbuffer.h)
>> +
>> +- request_alloc
>> +- submit_request

> Same here. Due to the being here in a separate file, I'm not sure if this level of detail is going to be kept up when changing the actual code?

I can place the text in the source file as well; one of the goals I have is that the documentation has sufficient details within
it so that a new person can not only have an idea of what the driver is doing, but where the code is located.

>> +
>> +The three varieties for submitting batchbuffer to the GPU are the following.
>> +
>> +1. Batchbuffers are subbmitted directly to a ring buffer; this is the most basic way to submit batchbuffers to the GPU and is for generations strictly before Gen8. When batchbuffers are submitted this older way, their contents are checked via Batchbuffer Parsing, see `Batchbuffer Parsing`_.

> Just for editing and reading pleasure, there must be a way of cutting long lines in lists.

This was something that I had troubles with; when I cut the long lines, the produced html output would not make it a list;
I had tried each of the following to cut the long lines:

  1. just cut the long lines to multiple lines

  2. add a \ between line-breaks

  3. cut long line and add white space to force alignment

Each of these resulted in just one item instead of a list. Any advice/pointers are greatly appreciated.

> But more importantly, do refer to Command Parser/Parsing as the code uses cmd parser aka. command parser extensively.

The item (3) has a link to the section "Batchbuffer Parsing" whose contents are the information on the command parser. Would you 
like me to rename the section "Batchbuffer Parsing" to "Command Parser" as well?

And again, thankyou for the comments (as this is just an RFC); I will be posting more (hopefully) by the end of the week with
all feedback taken into use and additional content (namely how/when those various function pointers are called and the entire
IRQ dance for submission and other details,  (for example) GuC submission).

-Kevin