Re: [PATCH v7 1/5] drm/panthor: introduce job cycle and timestamp accounting

[Steven Price <steven.price@arm.com>]

On 23/09/2024 11:18, Boris Brezillon wrote:
> On Mon, 23 Sep 2024 10:07:14 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>>> +static struct dma_fence *
>>> +queue_run_job(struct drm_sched_job *sched_job)
>>> +{
>>> +	struct panthor_job *job = container_of(sched_job, struct panthor_job, base);
>>> +	struct panthor_group *group = job->group;
>>> +	struct panthor_queue *queue = group->queues[job->queue_idx];
>>> +	struct panthor_device *ptdev = group->ptdev;
>>> +	struct panthor_scheduler *sched = ptdev->scheduler;
>>> +	struct panthor_job_ringbuf_instrs instrs;  
>>
>> instrs isn't initialised...
>>
>>> +	struct panthor_job_cs_params cs_params;
>>> +	struct dma_fence *done_fence;
>>> +	int ret;
>>>  
>>>  	/* Stream size is zero, nothing to do except making sure all previously
>>>  	 * submitted jobs are done before we signal the
>>> @@ -2900,17 +3062,23 @@ queue_run_job(struct drm_sched_job *sched_job)
>>>  		       queue->fence_ctx.id,
>>>  		       atomic64_inc_return(&queue->fence_ctx.seqno));
>>>  
>>> -	memcpy(queue->ringbuf->kmap + ringbuf_insert,
>>> -	       call_instrs, sizeof(call_instrs));
>>> +	job->profiling.slot = queue->profiling.seqno++;
>>> +	if (queue->profiling.seqno == queue->profiling.slot_count)
>>> +		queue->profiling.seqno = 0;
>>> +
>>> +	job->ringbuf.start = queue->iface.input->insert;
>>> +
>>> +	get_job_cs_params(job, &cs_params);
>>> +	prepare_job_instrs(&cs_params, &instrs);  
>>
>> ...but it's passed into prepare_job_instrs() which depends on
>> instrs.count (same bug as was in calc_job_credits()) - sorry I didn't
>> spot it last review.
> 
> Hm, can't we initialize instr::count to zero in prepare_job_instrs()
> instead?

Indeed that would probably be better! I hadn't noticed there were two
places in the previous review.

Steve