On 2023-07-29 02:59:32, Dmitry Baryshkov wrote:
> On 27/07/2023 23:03, Marijn Suijten wrote:
> > On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
> >> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
> >> Rather than checking for the flag, check for the presense of the
> >> corresponding interrupt line.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >
> > That's a smart use of the interrupt field. I both like it, and I do
> > not. While we didn't do any validation for consistency previously, this
> > means we now have multiple ways of controlling available "features":
> >
> > - Feature flags on hardware blocks;
> > - Presence of certain IRQs;
> > - DPU core revision.
>
> I hesitated here too. For INTF it is clear that there is no other good
> way to check for the TE feature. For PP we can just check for the DPU
> revision.
For both we could additionally check the DPU revision, and for INTF we
could check for TYPE_DSI. Both would aid in extra validation, if we
require the IRQ to be present or absent under these conditions.
It might also help to document this, either here and on the respective
struct fields so that catalog implementers know when they should supply
or leave out an IRQ?
- Marijn
> > Maybe that is more confusing to follow? Regardless of that I'm
> > convinced that this patch does what it's supposed to and gets rid of
> > some ambiguity. Maybe a comment above the IF explaining the "PP TE"
> > feature could alleviate the above concerns thoo. Hence:
> >
> > Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> >
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> >> index 9298c166b213..912a3bdf8ad4 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> >> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
> >> c->idx = cfg->id;
> >> c->caps = cfg;
> >>
> >> - if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
> >> + if (cfg->intr_rdptr) {
> >> c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
> >> c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
> >> c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
> >> --
> >> 2.39.2
> >>
>
> --
> With best wishes
> Dmitry
>
