| Message ID | 20220211202728.6146-7-alyssa.rosenzweig@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Initial Valhall support | expand |
On 11/02/2022 20:27, alyssa.rosenzweig@collabora.com wrote: > From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > > 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; ----8<---- Steve > --- > drivers/gpu/drm/panfrost/panfrost_features.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h > index 36fadcf9634e..1a8bdebc86a3 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_features.h > +++ b/drivers/gpu/drm/panfrost/panfrost_features.h > @@ -21,6 +21,7 @@ enum panfrost_hw_feature { > HW_FEATURE_TLS_HASHING, > HW_FEATURE_THREAD_GROUP_SPLIT, > HW_FEATURE_IDVS_GROUP_SIZE, > + HW_FEATURE_CLEAN_ONLY_SAFE, > HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG, > }; >
> > 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.
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
diff --git a/drivers/gpu/drm/panfrost/panfrost_features.h b/drivers/gpu/drm/panfrost/panfrost_features.h index 36fadcf9634e..1a8bdebc86a3 100644 --- a/drivers/gpu/drm/panfrost/panfrost_features.h +++ b/drivers/gpu/drm/panfrost/panfrost_features.h @@ -21,6 +21,7 @@ enum panfrost_hw_feature { HW_FEATURE_TLS_HASHING, HW_FEATURE_THREAD_GROUP_SPLIT, HW_FEATURE_IDVS_GROUP_SIZE, + HW_FEATURE_CLEAN_ONLY_SAFE, HW_FEATURE_3BIT_EXT_RW_L2_MMU_CONFIG, };