Re: [PATCH 6/9] drm/panfrost: Add "clean only safe" feature bit

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

On 14/02/2022 17:01, Alyssa Rosenzweig wrote:
>>> Add the HW_FEATURE_CLEAN_ONLY_SAFE bit based on kbase. When I actually
>>> tried to port the logic from kbase, trivial jobs raised Data Invalid
>>> Faults, so this may depend on other coherency details. It's still useful
>>> to have the bit to record the feature bit when adding new models.
>>>
>>> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
>>
>> Sadly I don't have the hardware to try this out on, but it should be a
>> simple case of the below (untested):
>>
>> ----8<----
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>> index 908d79520853..602e51c4966e 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>> @@ -212,9 +212,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>>          * start */
>>         cfg |= JS_CONFIG_THREAD_PRI(8) |
>>                 JS_CONFIG_START_FLUSH_CLEAN_INVALIDATE |
>> -               JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE |
>>                 panfrost_get_job_chain_flag(job);
>>  
>> +       if (panfrost_has_hw_feature(pfdev, HW_FEATURE_CLEAN_ONLY_SAFE))
>> +               cfg |= JS_CONFIG_END_FLUSH_CLEAN;
>> +       else
>> +               cfg |= JS_CONFIG_END_FLUSH_CLEAN_INVALIDATE;
>> +
>>         if (panfrost_has_hw_feature(pfdev, HW_FEATURE_FLUSH_REDUCTION))
>>                 cfg |= JS_CONFIG_ENABLE_FLUSH_REDUCTION;
> 
> Yes, this is the patch I typed out... causes DATA_INVALID_FAULTs for me
> with Mesa. Which makes me wonder if userspace needs to respect some
> extra rules for this to be safe.

Odd - the invalidate at the end of the job shouldn't be needed to read
the job descriptors from userspace only the one at the beginning.

However I'm wondering if there's something fishy happening with the
flush reduction. That allows skipping the cache maintenance at the
beginning of a job if there has already been one for other reasons. But
I can't immediately see any difference in the way kbase handles this.

Steve