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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 14 Dec 2022 22:05:16 +0100
From:   Marijn Suijten <marijn.suijten@...ainline.org>
To:     Kalyan Thota <kalyant@....qualcomm.com>
Cc:     "Kalyan Thota (QUIC)" <quic_kalyant@...cinc.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "freedreno@...ts.freedesktop.org" <freedreno@...ts.freedesktop.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robdclark@...omium.org" <robdclark@...omium.org>,
        "dianders@...omium.org" <dianders@...omium.org>,
        "swboyd@...omium.org" <swboyd@...omium.org>,
        "Vinod Polimera (QUIC)" <quic_vpolimer@...cinc.com>,
        "dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>,
        "Abhinav Kumar (QUIC)" <quic_abhinavk@...cinc.com>
Subject: Re: [v10] drm/msm/disp/dpu1: add support for dspp sub block flush in
 sc7280

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