Re: [PATCH v5 01/10] drm/msm: serialise framebuffer pin state

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 10/24/26 1:25 AM, Dmitry Baryshkov wrote:
> prepare_count, iova[] and the pins they describe are updated locklessly,
> but drm_atomic_helper_commit() prepares a new state in parallel with the
> completion of the previous one -- stall_checks() only stalls on the second
> previous commit.  Commit N+1's ->prepare_fb() thus runs while commit N is
> in ->cleanup_fb(), and for a double-buffered flip that is the same
> framebuffer:
> 
>   cleanup: prepare_count 1 -> 0
>   prepare: prepare_count 0 -> 1, pins, stores iova[]
>   cleanup: memset(iova, 0)
> 
> leaving the plane programmed with a NULL base address:
> 
>   arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
>       iova=0x00000100, fsynr=0x3e0023, cbfrsynra=0x1c00, cb=11
> 
> The opposite order is broken too since commit 8ac37c88f991 ("drm/msm:
> Refcount framebuffer pins"): a prepare which finds the count non-zero
> returns at once, assuming iova[] is populated.

The clanker thinks this is correct and was able to reproduce by doing
something similar with a test program

It also left the following comment:

The mutex approach in patch 1 has a direct precedent in omapdrm, so it's
a sound choice. msm is unusual in caching iova[] per framebuffer and
then zeroing it, which is why it needs the lock at all. If you want a
fix with less state, the amdgpu approach of not clearing the cached
address on cleanup would also avoid the NULL-base hazard. I'd keep
the mutex, though, since patch 2's count-leak fix sits on top of it.

any which way:

Acked-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad