| Message ID | 20230430235732.3341119-6-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: simplify DPU encoder init | expand |
On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: > The function dpu_encoder_get_wb() returns controller_id if the > corresponding WB is present in the catalog. We can inline this function > and rely on dpu_rm_get_wb() returning NULL for indices for which the > WB is not present on the device. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++------------------- > 1 file changed, 2 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 4c85cbb030e4..507ff3f88c67 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1277,22 +1277,6 @@ static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, > return INTF_MAX; > } > > -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg *catalog, > - enum dpu_intf_type type, u32 controller_id) > -{ > - int i = 0; > - > - if (type != INTF_WB) > - return WB_MAX; > - > - for (i = 0; i < catalog->wb_count; i++) { > - if (catalog->wb[i].id == controller_id) > - return catalog->wb[i].id; > - } > - > - return WB_MAX; > -} > - > void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, > struct dpu_encoder_phys *phy_enc) > { > @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > */ > u32 controller_id = disp_info->h_tile_instance[i]; > enum dpu_intf intf_idx; > - enum dpu_wb wb_idx; > > if (disp_info->num_of_h_tiles > 1) { > if (i == 0) > @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > disp_info->intf_type, > controller_id); > > - wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, > - disp_info->intf_type, controller_id); > - > if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) > phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx); > > - if (wb_idx >= WB_0 && wb_idx < WB_MAX) > - phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx); > + if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) > + phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id); From what I see, with https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 we are dropping those checks from the RM too, so we are going to rely totally on entering the values correctly in catalog from now on? > > if (!phys_params.hw_intf && !phys_params.hw_wb) { > DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at idx: %d\n", i);
On 03/05/2023 02:51, Abhinav Kumar wrote: > > > On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >> The function dpu_encoder_get_wb() returns controller_id if the >> corresponding WB is present in the catalog. We can inline this function >> and rely on dpu_rm_get_wb() returning NULL for indices for which the >> WB is not present on the device. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++------------------- >> 1 file changed, 2 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 4c85cbb030e4..507ff3f88c67 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -1277,22 +1277,6 @@ static enum dpu_intf dpu_encoder_get_intf(const >> struct dpu_mdss_cfg *catalog, >> return INTF_MAX; >> } >> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg >> *catalog, >> - enum dpu_intf_type type, u32 controller_id) >> -{ >> - int i = 0; >> - >> - if (type != INTF_WB) >> - return WB_MAX; >> - >> - for (i = 0; i < catalog->wb_count; i++) { >> - if (catalog->wb[i].id == controller_id) >> - return catalog->wb[i].id; >> - } >> - >> - return WB_MAX; >> -} >> - >> void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >> struct dpu_encoder_phys *phy_enc) >> { >> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> */ >> u32 controller_id = disp_info->h_tile_instance[i]; >> enum dpu_intf intf_idx; >> - enum dpu_wb wb_idx; >> if (disp_info->num_of_h_tiles > 1) { >> if (i == 0) >> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> disp_info->intf_type, >> controller_id); >> - wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, >> - disp_info->intf_type, controller_id); >> - >> if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) >> phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >> intf_idx); >> - if (wb_idx >= WB_0 && wb_idx < WB_MAX) >> - phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx); >> + if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) >> + phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, >> controller_id); > > > From what I see, with > https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 we > are dropping those checks from the RM too, so we are going to rely > totally on entering the values correctly in catalog from now on? Yes. I see no reason to mistrust the kernel data itself. > >> if (!phys_params.hw_intf && !phys_params.hw_wb) { >> DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at >> idx: %d\n", i);
On 5/2/2023 4:54 PM, Dmitry Baryshkov wrote: > On 03/05/2023 02:51, Abhinav Kumar wrote: >> >> >> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >>> The function dpu_encoder_get_wb() returns controller_id if the >>> corresponding WB is present in the catalog. We can inline this function >>> and rely on dpu_rm_get_wb() returning NULL for indices for which the >>> WB is not present on the device. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++------------------- >>> 1 file changed, 2 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 4c85cbb030e4..507ff3f88c67 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -1277,22 +1277,6 @@ static enum dpu_intf >>> dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, >>> return INTF_MAX; >>> } >>> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg >>> *catalog, >>> - enum dpu_intf_type type, u32 controller_id) >>> -{ >>> - int i = 0; >>> - >>> - if (type != INTF_WB) >>> - return WB_MAX; >>> - >>> - for (i = 0; i < catalog->wb_count; i++) { >>> - if (catalog->wb[i].id == controller_id) >>> - return catalog->wb[i].id; >>> - } >>> - >>> - return WB_MAX; >>> -} >>> - >>> void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >>> struct dpu_encoder_phys *phy_enc) >>> { >>> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> */ >>> u32 controller_id = disp_info->h_tile_instance[i]; >>> enum dpu_intf intf_idx; >>> - enum dpu_wb wb_idx; >>> if (disp_info->num_of_h_tiles > 1) { >>> if (i == 0) >>> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> disp_info->intf_type, >>> controller_id); >>> - wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, >>> - disp_info->intf_type, controller_id); >>> - >>> if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) >>> phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >>> intf_idx); >>> - if (wb_idx >= WB_0 && wb_idx < WB_MAX) >>> - phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx); >>> + if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) >>> + phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, >>> controller_id); >> >> >> From what I see, with >> https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 we >> are dropping those checks from the RM too, so we are going to rely >> totally on entering the values correctly in catalog from now on? > > Yes. I see no reason to mistrust the kernel data itself. Alright, if thats the overall plan, this change itself is fine. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> >>> if (!phys_params.hw_intf && !phys_params.hw_wb) { >>> DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at >>> idx: %d\n", i); >
On 03/05/2023 02:58, Abhinav Kumar wrote: > > > On 5/2/2023 4:54 PM, Dmitry Baryshkov wrote: >> On 03/05/2023 02:51, Abhinav Kumar wrote: >>> >>> >>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote: >>>> The function dpu_encoder_get_wb() returns controller_id if the >>>> corresponding WB is present in the catalog. We can inline this function >>>> and rely on dpu_rm_get_wb() returning NULL for indices for which the >>>> WB is not present on the device. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 >>>> ++------------------- >>>> 1 file changed, 2 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index 4c85cbb030e4..507ff3f88c67 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -1277,22 +1277,6 @@ static enum dpu_intf >>>> dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, >>>> return INTF_MAX; >>>> } >>>> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg >>>> *catalog, >>>> - enum dpu_intf_type type, u32 controller_id) >>>> -{ >>>> - int i = 0; >>>> - >>>> - if (type != INTF_WB) >>>> - return WB_MAX; >>>> - >>>> - for (i = 0; i < catalog->wb_count; i++) { >>>> - if (catalog->wb[i].id == controller_id) >>>> - return catalog->wb[i].id; >>>> - } >>>> - >>>> - return WB_MAX; >>>> -} >>>> - >>>> void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, >>>> struct dpu_encoder_phys *phy_enc) >>>> { >>>> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct >>>> dpu_encoder_virt *dpu_enc, >>>> */ >>>> u32 controller_id = disp_info->h_tile_instance[i]; >>>> enum dpu_intf intf_idx; >>>> - enum dpu_wb wb_idx; >>>> if (disp_info->num_of_h_tiles > 1) { >>>> if (i == 0) >>>> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct >>>> dpu_encoder_virt *dpu_enc, >>>> disp_info->intf_type, >>>> controller_id); >>>> - wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, >>>> - disp_info->intf_type, controller_id); >>>> - >>>> if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) >>>> phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, >>>> intf_idx); >>>> - if (wb_idx >= WB_0 && wb_idx < WB_MAX) >>>> - phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx); >>>> + if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) >>>> + phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, >>>> controller_id); >>> >>> >>> From what I see, with >>> https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 >>> we are dropping those checks from the RM too, so we are going to rely >>> totally on entering the values correctly in catalog from now on? >> >> Yes. I see no reason to mistrust the kernel data itself. > > Alright, if thats the overall plan, this change itself is fine. Yes, I think this is what we discussed some time ago for UBWC and QSEED programming. > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >>> >>>> if (!phys_params.hw_intf && !phys_params.hw_wb) { >>>> DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned >>>> at idx: %d\n", i); >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 4c85cbb030e4..507ff3f88c67 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1277,22 +1277,6 @@ static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog, return INTF_MAX; } -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg *catalog, - enum dpu_intf_type type, u32 controller_id) -{ - int i = 0; - - if (type != INTF_WB) - return WB_MAX; - - for (i = 0; i < catalog->wb_count; i++) { - if (catalog->wb[i].id == controller_id) - return catalog->wb[i].id; - } - - return WB_MAX; -} - void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, struct dpu_encoder_phys *phy_enc) { @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, */ u32 controller_id = disp_info->h_tile_instance[i]; enum dpu_intf intf_idx; - enum dpu_wb wb_idx; if (disp_info->num_of_h_tiles > 1) { if (i == 0) @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, disp_info->intf_type, controller_id); - wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, - disp_info->intf_type, controller_id); - if (intf_idx >= INTF_0 && intf_idx < INTF_MAX) phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx); - if (wb_idx >= WB_0 && wb_idx < WB_MAX) - phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx); + if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX) + phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id); if (!phys_params.hw_intf && !phys_params.hw_wb) { DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at idx: %d\n", i);
The function dpu_encoder_get_wb() returns controller_id if the corresponding WB is present in the catalog. We can inline this function and rely on dpu_rm_get_wb() returning NULL for indices for which the WB is not present on the device. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++------------------- 1 file changed, 2 insertions(+), 22 deletions(-)