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

Dave Gordon
2014-12-10T16:39:25Z

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.