Re: [PATCH 03/10] drm/i915: Updating assorted register and status page definitions

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

On 10/12/14 10:40, Daniel Vetter wrote:
> On Tue, Dec 09, 2014 at 12:59:06PM +0000, John.C.Harrison@Intel.com wrote:
>> From: Dave Gordon <david.s.gordon@intel.com>
>>
>> Added various definitions that will be useful for the scheduler in general and
>> pre-emptive context switching in particular.
>>
>> Change-Id: Ica805b94160426def51f5d520f5ce51c60864a98
>> For: VIZ-1587
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h         |   30 ++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_ringbuffer.h |   40 +++++++++++++++++++++++++++++--
>>  2 files changed, 67 insertions(+), 3 deletions(-)

[snip]

>> +/*
>> + * Tracking; these are updated by the GPU at the beginning and/or end of every
>> + * batch. One pair for regular buffers, the other for preemptive ones.
>> + */
>> +#define I915_BATCH_DONE_SEQNO		0x30  /* Completed batch seqno        */
>> +#define I915_BATCH_ACTIVE_SEQNO		0x31  /* In progress batch seqno      */
>> +#define I915_PREEMPTIVE_DONE_SEQNO	0x32  /* Completed preemptive batch   */
>> +#define I915_PREEMPTIVE_ACTIVE_SEQNO	0x33  /* In progress preemptive batch */
> 
> This are software define and currently not yet used I think. Imo better to
> add them together with the scheduler code that uses them. Splitting out
> #define patches only makes sense if they can be reviewed separately (e.g.
> registers with Bspec). Just adding software stuff (structs, enums, offsets
> not enforced by hw) should be added with the actual code using it.

OK

>> +
>> +/*
>> + * Preemption; these are used by the GPU to save important registers
>> + */
>> +#define I915_SAVE_PREEMPTED_RING_PTR	0x34  /* HEAD before preemption     */
>> +#define I915_SAVE_PREEMPTED_BB_PTR	0x35  /* BB ptr before preemption   */
>> +#define I915_SAVE_PREEMPTED_SBB_PTR	0x36  /* SBB before preemption      */
>> +#define I915_SAVE_PREEMPTED_UHPTR	0x37  /* UHPTR after preemption     */
>> +#define I915_SAVE_PREEMPTED_HEAD	0x38  /* HEAD after preemption      */
>> +#define I915_SAVE_PREEMPTED_TAIL	0x39  /* TAIL after preemption      */
>> +#define I915_SAVE_PREEMPTED_STATUS	0x3A  /* RS preemption status       */
>> +#define I915_SAVE_PREEMPTED_NOPID	0x3B  /* Dummy                      */
> 
> I guess this is hardcoded in hw? In that case a HWS instead of I915 prefix
> sounds better to me to make it clear this is not something i915/gem code
> invented. Also comment should state whether this is execlist or gen8+ or
> something like that.
> -Daniel

No, these are s/w too, so they can be added later if that's preferred.

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