On 2022-12-12 11:35:15, Kalyan Thota wrote: > [..] > >> + if (ctx->pending_dspp_flush_mask[dspp - DSPP_0]) > >> + DPU_REG_WRITE(&ctx->hw, CTL_DSPP_n_FLUSH(dspp - > >> DSPP_0), > >> + ctx->pending_dspp_flush_mask[dspp - > >> + DSPP_0]); > > > >Shouldn't this loop as a whole check if _any_ DSPP flush is requested via > >`pending_flush_mask & BIT(29)`? The other flushes don't check the per-block > >mask value either (and could write zero that way) but only base this check > >on the > >presence of a global flush mask for that block. > > > BIT(29) enables dspp flush only from DPU rev 7.x.x where hierarchal flush is > introduced. For other targets that supports CTL_ACTIVE, it's a NOP.
The only way this patch ever writes pending_dspp_flush_mask is followed by unconditionally setting BIT(29) in pending_flush_mask. I was under the assumption that pending_dspp_flush_mask should be considered invalid or irrelevant unless BIT(29) is set. > With the check "pending_flush_mask & BIT(29)", unintended DSPP registers for > that CTL path will be programmed to "0" which is not correct IMO. You can also keep the second `if` to guard against that; as said the code above does exactly this though, but I think we could assume that if a pending sub-block flush is set, pending_dspp_flush_mask is nonzero? > Secondly "pending_flush_mask & BIT(29)" although will not be true for DPU > 6.x.x versions but can be confusing w.r.t code readability. > Let me know your thoughts. Ack, it is /super/ confusing that BIT(29) is used for DSPP (sub-block) flush, but also to flash INTF_2?? In fact there are many overlapping flush bits used for different components. Only few are clarified via a #define. Can you confirm whether this is correct? And whether these should all be pulled out into numerically-sorted defines to improve readability and document intentional overlap? - Marijn
