[Intel-gfx] [PATCH v4 5/5] i915: add documentation to intel_engine_cs

[Rogovin, Kevin]

HI,


-----Original Message-----
From: Joonas Lahtinen [mailto:joonas.lahtinen at linux.intel.com] 

 - snip -
>>  
>>         void            (*set_default_submission)(struct intel_engine_cs *engine);
>>  
>> +       /* In addition to pinning the context, returns the intel_ringbuffer
>> +        * to which to write commands.

>	/* Pin context and return intel_ring to write commands to. */

I like that since it is shorter :)

>> +
>> +       /* Request room on the ringbuffer of a request in order to write
>> +        * commands for a request; In addition, if necessary, add commands
>> +        * to the buffer so that the i915_gem_context of the request
>> +        * is the one active for the commands.
>> +        */

> "Reserve room from the ringbuffer for commands and emit necessary context switching commands."?

Agreed; reserved is word to use here.

>> +       /* Add a batchbuffer start command; the GPU command is added to
>> +        * the buffer holding the commands of the request (i.e. calling
>> +        * intel_ring_begin() on i915_request::ring).
>> +        */
>>         int             (*emit_bb_start)(struct i915_request *rq,
>>                                          u64 offset, u32 length,
>>                                         unsigned int dispatch_flags);  
>> #define I915_DISPATCH_SECURE BIT(0)  #define I915_DISPATCH_PINNED 
>> BIT(1)
>>  #define I915_DISPATCH_RS     BIT(2)
>> +       /* Add a memory write command that writes the global sequence number
>> +        * (i915_request::global_seqno) and also add an interrupt command;
>> +        * the GPU command is added to the buffer holding the commands of
>> +        * the request (i.e. calling intel_ring_begin() on
>> +        * i915_request::ring).

>This is more about what a breadcrumb is than what this interface is about. "Add commands for triggering a breadcrumb 
> to be picked up" and maybe explain elsewhere what a breadcrumb is.

> So overall, try to make the comments bit less verbose and leave the implementation detail to the implementation functions :)

I am somewhat tempted to just drop this patch or add more documentation. The function pointers are used in the code common
to the legacy way and LRC way of submitting batchbuffers to the GPU, so they should have somekind of contract to what they are
supposed to do... but spelling out that contract might be a bit much...

Opinions?

 -Kevin