Re: [v10] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280

[Marijn Suijten]

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