[PATCH v2 1/6] drm/msm/dpu: don't set DPU_INTF_TE globally

[Marijn Suijten]

On 2023-05-23 01:01:50, Dmitry Baryshkov wrote:
> On 23/05/2023 00:56, Marijn Suijten wrote:
> > Title suggestion: s/globally/on non-TE/DSI (INTF) blocks
> > 
> > On 2023-05-23 00:45:22, Dmitry Baryshkov wrote:
> >> Using BIT(DPU_INTF_TE) in INTF_SC7180_MASK (and by extension in
> >> INTF_SC7280_MASK) results in this bit (and corrsponding operations)
> >> being enabled for all interfaces, even the ones which do not have TE
> >> block. Move this bit setting to INTF_DSI_TE(), so that it is only
> >> enabled for those INTF blocks which have TE support.
> >>
> >> Fixes: 152c1d430992 ("drm/msm/dpu: Add TEAR-READ-pointer interrupt to INTF block")
> >> Reviewed-by: Neil Armstrong <neil.armstrong at linaro.org>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > 
> > We've always been setting flags globally but I guess it makes sense to
> > not only restrict this flag to DPU >= 5.0.0 but also just the few
> > hardware blocks that actually have these in their *enlarged* register
> > space (and have the interrupt).
> > 
> > Reviewed-by: Marijn Suijten <marijn.suijten at somainline.org>
> > 
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> index 1dee5ba2b312..162141cb5c83 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> @@ -101,7 +101,6 @@
> >>   
> >>   #define INTF_SC7180_MASK \
> >>   	(BIT(DPU_INTF_INPUT_CTRL) | \
> >> -	 BIT(DPU_INTF_TE) | \
> >>   	 BIT(DPU_INTF_STATUS_SUPPORTED) | \
> >>   	 BIT(DPU_DATA_HCTL_EN))
> >>   
> >> @@ -544,7 +543,7 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
> >>   	{\
> >>   	.name = _name, .id = _id, \
> >>   	.base = _base, .len = _len, \
> >> -	.features = _features, \
> >> +	.features = _features | BIT(DPU_INTF_TE), \
> > 
> > Now that we're more broadly switching to this pattern, should we do the
> > same for PP_BLK() with and without TE block?  That way we can also
> > forcefully initialize intr_rdptr=-1 similar to what I did for
> > intr_tear_rd_ptr in INTF_BLK() (vs INTF_BLK_DSI_TE) here, instead of
> > having the -1's floating around the catalog when I added them in commit
> > 7952f5180eb3e ("drm/msm/dpu: Remove intr_rdptr from DPU >= 5.0.0
> > pingpong config").
> 
> If we are going to expand the macros, then hiding -1 probably doesn't 
> make sense as it will reappear soon.
> 
> Probably it makes sense to do another thing (which would play better 
> with the expanded macros): increase IRQ indices by 1, making 'NO IRQ' 
> equal to 0 instead of -1. This way all non-existing interrupts can be 
> omitted during macros expansion. WDYT?

I'm fine explicitly setting it to -1 to clarify it is missing.  On the
other hand, default struct initialization might accidentally initialize
it to the first interrupt on MDP_SSPP_TOP0_INTR (when users forget to
write the member), where it makes sense to start at 1 instead.  Need to
think about this for a bit.
(The forced number of arguments is an advantage of the macro, even if we
 now have too many numeric constants to know which field it sets)

- Marijn

> > - Marijn
> > 
> >>   	.type = _type, \
> >>   	.controller_id = _ctrl_id, \
> >>   	.prog_fetch_lines_worst_case = _progfetch, \
> >> -- 
> >> 2.39.2
> >>
> 
> -- 
> With best wishes
> Dmitry
>