On 25.09.2024 10:56, Steven Price wrote: >On 23/09/2024 21:43, Adrián Larumbe wrote: >> Hi Steve, >> >> On 23.09.2024 09:55, Steven Price wrote: >>> On 20/09/2024 23:36, Adrián Larumbe wrote: >>>> Hi Steve, thanks for the review. >>> >>> Hi Adrián, >>> >>>> I've applied all of your suggestions for the next patch series revision, so I'll >>>> only be answering to your question about the calc_profiling_ringbuf_num_slots >>>> function further down below. >>>> >>> >>> [...] >>> >>>>>> @@ -3003,6 +3190,34 @@ static const struct drm_sched_backend_ops panthor_queue_sched_ops = { >>>>>> .free_job = queue_free_job, >>>>>> }; >>>>>> >>>>>> +static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev, >>>>>> + u32 cs_ringbuf_size) >>>>>> +{ >>>>>> + u32 min_profiled_job_instrs = U32_MAX; >>>>>> + u32 last_flag = fls(PANTHOR_DEVICE_PROFILING_ALL); >>>>>> + >>>>>> + /* >>>>>> + * We want to calculate the minimum size of a profiled job's CS, >>>>>> + * because since they need additional instructions for the sampling >>>>>> + * of performance metrics, they might take up further slots in >>>>>> + * the queue's ringbuffer. This means we might not need as many job >>>>>> + * slots for keeping track of their profiling information. What we >>>>>> + * need is the maximum number of slots we should allocate to this end, >>>>>> + * which matches the maximum number of profiled jobs we can place >>>>>> + * simultaneously in the queue's ring buffer. >>>>>> + * That has to be calculated separately for every single job profiling >>>>>> + * flag, but not in the case job profiling is disabled, since unprofiled >>>>>> + * jobs don't need to keep track of this at all. >>>>>> + */ >>>>>> + for (u32 i = 0; i < last_flag; i++) { >>>>>> + if (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) >>>>>> + min_profiled_job_instrs = >>>>>> + min(min_profiled_job_instrs, calc_job_credits(BIT(i))); >>>>>> + } >>>>>> + >>>>>> + return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64)); >>>>>> +} >>>>> >>>>> I may be missing something, but is there a situation where this is >>>>> different to calc_job_credits(0)? AFAICT the infrastructure you've added >>>>> can only add extra instructions to the no-flags case - whereas this >>>>> implies you're thinking that instructions may also be removed (or replaced). >>>>> >>>>> Steve >>>> >>>> Since we create a separate kernel BO to hold the profiling information slot, we >>>> need one that would be able to accomodate as many slots as the maximum number of >>>> profiled jobs we can insert simultaneously into the queue's ring buffer. Because >>>> profiled jobs always take more instructions than unprofiled ones, then we would >>>> usually need fewer slots than the number of unprofiled jobs we could insert at >>>> once in the ring buffer. >>>> >>>> Because we represent profiling metrics with a bit mask, then we need to test the >>>> size of the CS for every single metric enabled in isolation, since enabling more >>>> than one will always mean a bigger CS, and therefore fewer jobs tracked at once >>>> in the queue's ring buffer. >>>> >>>> In our case, calling calc_job_credits(0) would simply tell us the number of >>>> instructions we need for a normal job with no profiled features enabled, which >>>> would always requiere less instructions than profiled ones, and therefore more >>>> slots in the profiling info kernel BO. But we don't need to keep track of >>>> profiling numbers for unprofiled jobs, so there's no point in calculating this >>>> number. >>>> >>>> At first I was simply allocating a profiling info kernel BO as big as the number >>>> of simultaneous unprofiled job slots in the ring queue, but Boris pointed out >>>> that since queue ringbuffers can be as big as 2GiB, a lot of this memory would >>>> be wasted, since profiled jobs always require more slots because they hold more >>>> instructions, so fewer profiling slots in said kernel BO. >>>> >>>> The value of this approach will eventually manifest if we decided to keep track of >>>> more profiling metrics, since this code won't have to change at all, other than >>>> adding new profiling flags in the panthor_device_profiling_flags enum. >>> >>> Thanks for the detailed explanation. I think what I was missing is that >>> the loop is checking each bit flag independently and *not* checking >>> calc_job_credits(0). >>> >>> The check for (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) is probably what >>> confused me - that should be completely redundant. Or at least we need >>> something more intelligent if we have profiling bits which are not >>> mutually compatible. >> >> I thought of an alternative that would only test bits that are actually part of >> the mask: >> >> static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev, >> u32 cs_ringbuf_size) >> { >> u32 min_profiled_job_instrs = U32_MAX; >> u32 profiling_mask = PANTHOR_DEVICE_PROFILING_ALL; >> >> while (profiling_mask) { >> u32 i = ffs(profiling_mask) - 1; >> profiling_mask &= ~BIT(i); >> min_profiled_job_instrs = >> min(min_profiled_job_instrs, calc_job_credits(BIT(i))); >> } >> >> return DIV_ROUND_UP(cs_ringbuf_size, min_profiled_job_instrs * sizeof(u64)); >> } >> >> However, I don't think this would be more efficient, because ffs() is probably >> fetching the first set bit by performing register shifts, and I guess this would >> take somewhat longer than iterating over every single bit from the last one, >> even if also matching them against the whole mask, just in case in future >> additions of performance metrics we decide to leave some of the lower >> significance bits untouched. > >Efficiency isn't very important here - we're not on a fast path, so it's >more about ensuring the code is readable. I don't think the above is >more readable then the original for loop. > >> Regarding your question about mutual compatibility, I don't think that is an >> issue here, because we're testing bits in isolation. If in the future we find >> out that some of the values we're profiling cannot be sampled at once, we can >> add that logic to the sysfs knob handler, to make sure UM cannot set forbidden >> profiling masks. > >My comment about compatibility is because in the original above you were >calculating the top bit of PANTHOR_DEVICE_PROFILING_ALL: > >> u32 last_flag = fls(PANTHOR_DEVICE_PROFILING_ALL); > >then looping between 0 and that bit: > >> for (u32 i = 0; i < last_flag; i++) { > >So the test: > >> if (BIT(i) & PANTHOR_DEVICE_PROFILING_ALL) > >would only fail if PANTHOR_DEVICE_PROFILING_ALL had gaps in the bits >that it set. The only reason I can think for that to be true in the >future is if there is some sort of incompatibility - e.g. maybe there's >an old and new way of doing some form of profiling with the old way >being kept for backwards compatibility. But I suspect if/when that is >required we'll need to revisit this function anyway. So that 'if' >statement seems completely redundant (it's trivially always true). I think you're right about this. Would you be fine with the rest of the patch as it is in revision 8 if I also deleted this bitmask check? >Steve > >>> I'm also not entirely sure that the amount of RAM saved is significant, >>> but you've already written the code so we might as well have the saving ;) >> >> I think this was more evident before Boris suggested we reduce the basic slot >> size to that of a single cache line, because then the minimum profiled job >> might've taken twice as many ringbuffer slots as a nonprofiled one. In that >> case, we would need a half as big BO for holding the sampled data (in case the >> least size profiled job CS would extend over the 16 instruction boundary). >> I still think this is a good idea so that in the future we don't need to worry >> about adjusting the code that deals with preparing the right boilerplate CS, >> since it'll only be a matter of adding new instructions inside prepare_job_instrs(). >> >>> Thanks, >>> Steve >>> >>>> Regards, >>>> Adrian >>>> >>>>>> + >>>>>> static struct panthor_queue * >>>>>> group_create_queue(struct panthor_group *group, >>>>>> const struct drm_panthor_queue_create *args) >>>>>> @@ -3056,9 +3271,35 @@ group_create_queue(struct panthor_group *group, >>>>>> goto err_free_queue; >>>>>> } >>>>>> >>>>>> + queue->profiling.slot_count = >>>>>> + calc_profiling_ringbuf_num_slots(group->ptdev, args->ringbuf_size); >>>>>> + >>>>>> + queue->profiling.slots = >>>>>> + panthor_kernel_bo_create(group->ptdev, group->vm, >>>>>> + queue->profiling.slot_count * >>>>>> + sizeof(struct panthor_job_profiling_data), >>>>>> + DRM_PANTHOR_BO_NO_MMAP, >>>>>> + DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | >>>>>> + DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED, >>>>>> + PANTHOR_VM_KERNEL_AUTO_VA); >>>>>> + >>>>>> + if (IS_ERR(queue->profiling.slots)) { >>>>>> + ret = PTR_ERR(queue->profiling.slots); >>>>>> + goto err_free_queue; >>>>>> + } >>>>>> + >>>>>> + ret = panthor_kernel_bo_vmap(queue->profiling.slots); >>>>>> + if (ret) >>>>>> + goto err_free_queue; >>>>>> + >>>>>> + /* >>>>>> + * Credit limit argument tells us the total number of instructions >>>>>> + * across all CS slots in the ringbuffer, with some jobs requiring >>>>>> + * twice as many as others, depending on their profiling status. >>>>>> + */ >>>>>> ret = drm_sched_init(&queue->scheduler, &panthor_queue_sched_ops, >>>>>> group->ptdev->scheduler->wq, 1, >>>>>> - args->ringbuf_size / (NUM_INSTRS_PER_SLOT * sizeof(u64)), >>>>>> + args->ringbuf_size / sizeof(u64), >>>>>> 0, msecs_to_jiffies(JOB_TIMEOUT_MS), >>>>>> group->ptdev->reset.wq, >>>>>> NULL, "panthor-queue", group->ptdev->base.dev); >>>>>> @@ -3354,6 +3595,7 @@ panthor_job_create(struct panthor_file *pfile, >>>>>> { >>>>>> struct panthor_group_pool *gpool = pfile->groups; >>>>>> struct panthor_job *job; >>>>>> + u32 credits; >>>>>> int ret; >>>>>> >>>>>> if (qsubmit->pad) >>>>>> @@ -3407,9 +3649,16 @@ panthor_job_create(struct panthor_file *pfile, >>>>>> } >>>>>> } >>>>>> >>>>>> + job->profiling.mask = pfile->ptdev->profile_mask; >>>>>> + credits = calc_job_credits(job->profiling.mask); >>>>>> + if (credits == 0) { >>>>>> + ret = -EINVAL; >>>>>> + goto err_put_job; >>>>>> + } >>>>>> + >>>>>> ret = drm_sched_job_init(&job->base, >>>>>> &job->group->queues[job->queue_idx]->entity, >>>>>> - 1, job->group); >>>>>> + credits, job->group); >>>>>> if (ret) >>>>>> goto err_put_job; >>>>>> >>>>