| Message ID | 1673972488-30140-2-git-send-email-quic_kalyant@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow composer fallbacks for color features | expand |
On 17/01/2023 18:21, Kalyan Thota wrote: > if any topology requests for dspps and catalogue doesn't have the > allocation, avoid failing the reservation. > > This can pave way to build logic allowing composer fallbacks > for all the color features that are handled in dspp. > > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 73b3442..c8899ae 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -343,7 +343,13 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm, > return true; > > idx = lm_cfg->dspp - DSPP_0; > - if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) { > + > + if (idx < 0) { The change doesn't correspond to commit message. > + DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", lm_cfg->dspp); > + return true; > + } > + > + if (idx >= ARRAY_SIZE(rm->dspp_blks)) { > DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp); > return false; > } If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check, I'd suggest dropping the second one
On 17/01/2023 18:35, Dmitry Baryshkov wrote: > On 17/01/2023 18:21, Kalyan Thota wrote: >> if any topology requests for dspps and catalogue doesn't have the >> allocation, avoid failing the reservation. >> >> This can pave way to build logic allowing composer fallbacks >> for all the color features that are handled in dspp. >> >> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> index 73b3442..c8899ae 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> @@ -343,7 +343,13 @@ static bool >> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm, >> return true; >> idx = lm_cfg->dspp - DSPP_0; >> - if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) { >> + >> + if (idx < 0) { > > The change doesn't correspond to commit message. > >> + DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", >> lm_cfg->dspp); >> + return true; >> + } >> + >> + if (idx >= ARRAY_SIZE(rm->dspp_blks)) { >> DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp); >> return false; >> } > > If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check, I'd > suggest dropping the second one > I've misread the patch. However I don't see, why would one request DSPP_NONE while specifying topology->num_dspp. I think that you are trying to put additional logic into a function that should just check for the available resources.
>-----Original Message----- >From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >Sent: Tuesday, January 17, 2023 10:10 PM >To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri- >devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; >freedreno@lists.freedesktop.org; devicetree@vger.kernel.org >Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org; >dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC) ><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC) ><quic_abhinavk@quicinc.com> >Subject: Re: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are >not available. > >WARNING: This email originated from outside of Qualcomm. Please be wary of >any links or attachments, and do not enable macros. > >On 17/01/2023 18:35, Dmitry Baryshkov wrote: >> On 17/01/2023 18:21, Kalyan Thota wrote: >>> if any topology requests for dspps and catalogue doesn't have the >>> allocation, avoid failing the reservation. >>> >>> This can pave way to build logic allowing composer fallbacks for all >>> the color features that are handled in dspp. >>> >>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> index 73b3442..c8899ae 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> @@ -343,7 +343,13 @@ static bool >>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm, >>> return true; >>> idx = lm_cfg->dspp - DSPP_0; >>> - if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) { >>> + >>> + if (idx < 0) { >> >> The change doesn't correspond to commit message. >> >>> + DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", >>> lm_cfg->dspp); >>> + return true; >>> + } >>> + >>> + if (idx >= ARRAY_SIZE(rm->dspp_blks)) { >>> DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp); >>> return false; >>> } >> >> If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check, >> I'd suggest dropping the second one >> > >I've misread the patch. However I don't see, why would one request DSPP_NONE >while specifying topology->num_dspp. I think that you are trying to put additional >logic into a function that should just check for the available resources. > The link is specified in the catalogue. For example: LM_BLK("lm_0", LM_0, 0x44000, MIXER_SC7180_MASK, &sc7180_lm_sblk, PINGPONG_0, 0, DSPP_0), --> This LM has DSPP attached LM_BLK("lm_2", LM_2, 0x46000, MIXER_SC7180_MASK, &sc7180_lm_sblk, PINGPONG_2, LM_3, 0), --> no DSPP LM_BLK("lm_3", LM_3, 0x47000, MIXER_SC7180_MASK, &sc7180_lm_sblk, PINGPONG_3, LM_2, 0), --> no DSPP For the above example, num_dspps will be 1 which is nonzero. But if a request comes on second interface and if there are no dspps then we are not failing the reservation of data path as color features can be offloaded to GPU. Idx for LM_2 and LM_3 will be -1 for above case hence the check not to fail reservation. topology->num_dspp previously was filled based on encoder type, since we want to move away from encoder checks, we are now passing it same as LM number. If there are dspps available we will allocate, in case of non-availability then we are not failing the datapath reservation so that composer fallbacks can be implemented. >-- >With best wishes >Dmitry
On 18/01/2023 05:24, Kalyan Thota wrote: > > >> -----Original Message----- >> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Sent: Tuesday, January 17, 2023 10:10 PM >> To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri- >> devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; >> freedreno@lists.freedesktop.org; devicetree@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org; >> dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC) >> <quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC) >> <quic_abhinavk@quicinc.com> >> Subject: Re: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are >> not available. >> >> WARNING: This email originated from outside of Qualcomm. Please be wary of >> any links or attachments, and do not enable macros. >> >> On 17/01/2023 18:35, Dmitry Baryshkov wrote: >>> On 17/01/2023 18:21, Kalyan Thota wrote: >>>> if any topology requests for dspps and catalogue doesn't have the >>>> allocation, avoid failing the reservation. >>>> >>>> This can pave way to build logic allowing composer fallbacks for all >>>> the color features that are handled in dspp. >>>> >>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>>> index 73b3442..c8899ae 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>>> @@ -343,7 +343,13 @@ static bool >>>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm, >>>> return true; >>>> idx = lm_cfg->dspp - DSPP_0; >>>> - if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) { >>>> + >>>> + if (idx < 0) { >>> >>> The change doesn't correspond to commit message. >>> >>>> + DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", >>>> lm_cfg->dspp); >>>> + return true; >>>> + } >>>> + >>>> + if (idx >= ARRAY_SIZE(rm->dspp_blks)) { >>>> DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp); >>>> return false; >>>> } >>> >>> If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check, >>> I'd suggest dropping the second one >>> >> >> I've misread the patch. However I don't see, why would one request DSPP_NONE >> while specifying topology->num_dspp. I think that you are trying to put additional >> logic into a function that should just check for the available resources. >> > > The link is specified in the catalogue. > For example: > > LM_BLK("lm_0", LM_0, 0x44000, MIXER_SC7180_MASK, > &sc7180_lm_sblk, PINGPONG_0, 0, DSPP_0), --> This LM has DSPP attached > LM_BLK("lm_2", LM_2, 0x46000, MIXER_SC7180_MASK, > &sc7180_lm_sblk, PINGPONG_2, LM_3, 0), --> no DSPP > LM_BLK("lm_3", LM_3, 0x47000, MIXER_SC7180_MASK, > &sc7180_lm_sblk, PINGPONG_3, LM_2, 0), --> no DSPP > > For the above example, num_dspps will be 1 which is nonzero. But if a request comes on second interface and if there are no dspps then we are not failing the reservation of data path as color features can be offloaded to GPU. > Idx for LM_2 and LM_3 will be -1 for above case hence the check not to fail reservation. > > topology->num_dspp previously was filled based on encoder type, since we want to move away from encoder checks, we are now passing it same as LM number. If there are dspps available we will allocate, > in case of non-availability then we are not failing the datapath reservation so that composer fallbacks can be implemented. As I wrote, num_dspps should be filled correctly (by the dpu_get_topology) to request DSPP for CTM-enabled CRTCs and to set the field to 0 if CRTC doesn't have CTM enabled. Then RM code should adhere to the num_dspps passed. It must return an error if it can not fulfil the requirements. Also if no DSPPs are requested, RM should prefer non-DSPP-enabled LMs.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 73b3442..c8899ae 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -343,7 +343,13 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm, return true; idx = lm_cfg->dspp - DSPP_0; - if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) { + + if (idx < 0) { + DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", lm_cfg->dspp); + return true; + } + + if (idx >= ARRAY_SIZE(rm->dspp_blks)) { DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp); return false; }
if any topology requests for dspps and catalogue doesn't have the allocation, avoid failing the reservation. This can pave way to build logic allowing composer fallbacks for all the color features that are handled in dspp. Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)