| Message ID | 1673972488-30140-3-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: > Allow dspps to be populated as a requirement for all the encoder > types it need not be just DSI. If for any encoder the dspp > allocation doesn't go through then there can be an option to > fallback for color features. > > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 9c6817b..e39b345 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc) > static struct msm_display_topology dpu_encoder_get_topology( > struct dpu_encoder_virt *dpu_enc, > struct dpu_kms *dpu_kms, > - struct drm_display_mode *mode) > + struct drm_display_mode *mode, > + struct drm_crtc_state *crtc_state) Is this new argument used at all? > { > struct msm_display_topology topology = {0}; > int i, intf_count = 0; > @@ -563,8 +564,9 @@ static struct msm_display_topology dpu_encoder_get_topology( > * 1 LM, 1 INTF > * 2 LM, 1 INTF (stream merge to support high resolution interfaces) > * > - * Adding color blocks only to primary interface if available in > - * sufficient number > + * dspp blocks are made optional. If RM manager cannot allocate > + * dspp blocks, then reservations will still go through with non dspp LM's > + * so as to allow color management support via composer fallbacks > */ No, this is not the way to go. First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required. Right now your patch makes it possible to allocate LMs, that have DSPP attached, for non-CTM-enabled encoder and later fail allocation of DSPP for the CRTC which has CTM blob attached. Second, the decision on using DSPPs should come from dpu_crtc_atomic_check(). Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail if the need_dspp constraint can't be fulfilled. > if (intf_count == 2) > topology.num_lm = 2; > @@ -573,11 +575,9 @@ static struct msm_display_topology dpu_encoder_get_topology( > else > topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1; > > - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { > - if (dpu_kms->catalog->dspp && > - (dpu_kms->catalog->dspp_count >= topology.num_lm)) > - topology.num_dspp = topology.num_lm; > - } > + if (dpu_kms->catalog->dspp && > + (dpu_kms->catalog->dspp_count >= topology.num_lm)) > + topology.num_dspp = topology.num_lm; > > topology.num_enc = 0; > topology.num_intf = intf_count; > @@ -643,7 +643,7 @@ static int dpu_encoder_virt_atomic_check( > } > } > > - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); > + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state); > > /* Reserve dynamic resources now. */ > if (!ret) {
>-----Original Message----- >From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >Sent: Tuesday, January 17, 2023 10:26 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 2/3] drm/msm/disp/dpu1: allow dspp selection for all the >interfaces > >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:21, Kalyan Thota wrote: >> Allow dspps to be populated as a requirement for all the encoder types >> it need not be just DSI. If for any encoder the dspp allocation >> doesn't go through then there can be an option to fallback for color >> features. >> >> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 9c6817b..e39b345 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder >*drm_enc) >> static struct msm_display_topology dpu_encoder_get_topology( >> struct dpu_encoder_virt *dpu_enc, >> struct dpu_kms *dpu_kms, >> - struct drm_display_mode *mode) >> + struct drm_display_mode *mode, >> + struct drm_crtc_state *crtc_state) > >Is this new argument used at all? > >> { >> struct msm_display_topology topology = {0}; >> int i, intf_count = 0; >> @@ -563,8 +564,9 @@ static struct msm_display_topology >dpu_encoder_get_topology( >> * 1 LM, 1 INTF >> * 2 LM, 1 INTF (stream merge to support high resolution interfaces) >> * >> - * Adding color blocks only to primary interface if available in >> - * sufficient number >> + * dspp blocks are made optional. If RM manager cannot allocate >> + * dspp blocks, then reservations will still go through with non dspp LM's >> + * so as to allow color management support via composer >> + fallbacks >> */ > >No, this is not the way to go. > >First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required. >Right now your patch makes it possible to allocate LMs, that have DSPP attached, >for non-CTM-enabled encoder and later fail allocation of DSPP for the CRTC >which has CTM blob attached. > >Second, the decision on using DSPPs should come from dpu_crtc_atomic_check(). >Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail if the >need_dspp constraint can't be fulfilled. > We may not get color_mgmt_changed property set during modeset commit, where as our resource allocation happens during modeset. With this approach, dspps will get allocated on first come first serve basis @Rob, is it possible to send color management property during modeset, in that case, we can use it for dspp allocation to the datapath. The current approach doesn't assume it. On chrome compositor, I see that color property was being set in the subsequent commits but not in modeset. > >> if (intf_count == 2) >> topology.num_lm = 2; >> @@ -573,11 +575,9 @@ static struct msm_display_topology >dpu_encoder_get_topology( >> else >> topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) >> ? 2 : 1; >> >> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { >> - if (dpu_kms->catalog->dspp && >> - (dpu_kms->catalog->dspp_count >= topology.num_lm)) >> - topology.num_dspp = topology.num_lm; >> - } >> + if (dpu_kms->catalog->dspp && >> + (dpu_kms->catalog->dspp_count >= topology.num_lm)) >> + topology.num_dspp = topology.num_lm; >> >> topology.num_enc = 0; >> topology.num_intf = intf_count; >> @@ -643,7 +643,7 @@ static int dpu_encoder_virt_atomic_check( >> } >> } >> >> - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); >> + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, >> + crtc_state); >> >> /* Reserve dynamic resources now. */ >> if (!ret) { > >-- >With best wishes >Dmitry
On 18/01/2023 05:30, Kalyan Thota wrote: > > >> -----Original Message----- >> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Sent: Tuesday, January 17, 2023 10:26 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 2/3] drm/msm/disp/dpu1: allow dspp selection for all the >> interfaces >> >> 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:21, Kalyan Thota wrote: >>> Allow dspps to be populated as a requirement for all the encoder types >>> it need not be just DSI. If for any encoder the dspp allocation >>> doesn't go through then there can be an option to fallback for color >>> features. >>> >>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++--------- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 9c6817b..e39b345 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder >> *drm_enc) >>> static struct msm_display_topology dpu_encoder_get_topology( >>> struct dpu_encoder_virt *dpu_enc, >>> struct dpu_kms *dpu_kms, >>> - struct drm_display_mode *mode) >>> + struct drm_display_mode *mode, >>> + struct drm_crtc_state *crtc_state) >> >> Is this new argument used at all? >> >>> { >>> struct msm_display_topology topology = {0}; >>> int i, intf_count = 0; >>> @@ -563,8 +564,9 @@ static struct msm_display_topology >> dpu_encoder_get_topology( >>> * 1 LM, 1 INTF >>> * 2 LM, 1 INTF (stream merge to support high resolution interfaces) >>> * >>> - * Adding color blocks only to primary interface if available in >>> - * sufficient number >>> + * dspp blocks are made optional. If RM manager cannot allocate >>> + * dspp blocks, then reservations will still go through with non dspp LM's >>> + * so as to allow color management support via composer >>> + fallbacks >>> */ >> >> No, this is not the way to go. >> >> First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required. >> Right now your patch makes it possible to allocate LMs, that have DSPP attached, >> for non-CTM-enabled encoder and later fail allocation of DSPP for the CRTC >> which has CTM blob attached. >> >> Second, the decision on using DSPPs should come from dpu_crtc_atomic_check(). >> Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail if the >> need_dspp constraint can't be fulfilled. >> > We may not get color_mgmt_changed property set during modeset commit, where as our resource allocation happens during modeset. So, you have to fix the conditions to perform LM reallocation if CTM usage has changed (note, color_mgmt_changed is not a correct one here). > With this approach, dspps will get allocated on first come first serve basis I don't think that this is what we have agreed upon. > @Rob, is it possible to send color management property during modeset, in that case, we can use it for dspp allocation to the datapath. The current approach doesn't assume it. > On chrome compositor, I see that color property was being set in the subsequent commits but not in modeset. > >> >>> if (intf_count == 2) >>> topology.num_lm = 2; >>> @@ -573,11 +575,9 @@ static struct msm_display_topology >> dpu_encoder_get_topology( >>> else >>> topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) >>> ? 2 : 1; >>> >>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { >>> - if (dpu_kms->catalog->dspp && >>> - (dpu_kms->catalog->dspp_count >= topology.num_lm)) >>> - topology.num_dspp = topology.num_lm; >>> - } >>> + if (dpu_kms->catalog->dspp && >>> + (dpu_kms->catalog->dspp_count >= topology.num_lm)) >>> + topology.num_dspp = topology.num_lm; >>> >>> topology.num_enc = 0; >>> topology.num_intf = intf_count; >>> @@ -643,7 +643,7 @@ static int dpu_encoder_virt_atomic_check( >>> } >>> } >>> >>> - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); >>> + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, >>> + crtc_state); >>> >>> /* Reserve dynamic resources now. */ >>> if (!ret) { >> >> -- >> With best wishes >> Dmitry >
>-----Original Message----- >From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >Sent: Wednesday, January 18, 2023 9:06 AM >To: Kalyan Thota <kalyant@qti.qualcomm.com>; 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; Abhinav Kumar (QUIC) ><quic_abhinavk@quicinc.com>; Doug Anderson <dianders@google.com> >Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org; >dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC) ><quic_vpolimer@quicinc.com> >Subject: Re: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the >interfaces > >WARNING: This email originated from outside of Qualcomm. Please be wary of >any links or attachments, and do not enable macros. > >On 18/01/2023 05:30, Kalyan Thota wrote: >> >> >>> -----Original Message----- >>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> Sent: Tuesday, January 17, 2023 10:26 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 2/3] drm/msm/disp/dpu1: allow dspp selection for >>> all the interfaces >>> >>> 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:21, Kalyan Thota wrote: >>>> Allow dspps to be populated as a requirement for all the encoder >>>> types it need not be just DSI. If for any encoder the dspp >>>> allocation doesn't go through then there can be an option to >>>> fallback for color features. >>>> >>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++--------- >>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> index 9c6817b..e39b345 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct >>>> drm_encoder >>> *drm_enc) >>>> static struct msm_display_topology dpu_encoder_get_topology( >>>> struct dpu_encoder_virt *dpu_enc, >>>> struct dpu_kms *dpu_kms, >>>> - struct drm_display_mode *mode) >>>> + struct drm_display_mode *mode, >>>> + struct drm_crtc_state *crtc_state) >>> >>> Is this new argument used at all? >>> >>>> { >>>> struct msm_display_topology topology = {0}; >>>> int i, intf_count = 0; >>>> @@ -563,8 +564,9 @@ static struct msm_display_topology >>> dpu_encoder_get_topology( >>>> * 1 LM, 1 INTF >>>> * 2 LM, 1 INTF (stream merge to support high resolution interfaces) >>>> * >>>> - * Adding color blocks only to primary interface if available in >>>> - * sufficient number >>>> + * dspp blocks are made optional. If RM manager cannot allocate >>>> + * dspp blocks, then reservations will still go through with non dspp LM's >>>> + * so as to allow color management support via composer >>>> + fallbacks >>>> */ >>> >>> No, this is not the way to go. >>> >>> First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required. >>> Right now your patch makes it possible to allocate LMs, that have >>> DSPP attached, for non-CTM-enabled encoder and later fail allocation >>> of DSPP for the CRTC which has CTM blob attached. >>> >>> Second, the decision on using DSPPs should come from >dpu_crtc_atomic_check(). >>> Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail >>> if the need_dspp constraint can't be fulfilled. >>> >> We may not get color_mgmt_changed property set during modeset commit, >where as our resource allocation happens during modeset. > >So, you have to fix the conditions to perform LM reallocation if CTM usage has >changed (note, color_mgmt_changed is not a correct one here). > If I take the above approach, where should I update the new reservations as we won't be getting atomic_mode_set callback as the change is only w.r.t color management. Sequence : 1) atomic_check on encoder Received a request with no ctm enabled (1LM, 0dspp, 1 intf) Rm will reserve 1LM and 1 intf 2) atomic_modeset on encoder Update the state with reservations. 3) Commit with ctm enabled ( 1 LM, 1 dspp, 1 intf) Here as the topology has changed, I need to re - reserve the resource freeing the earlier ones. But where should I update them to the state ? shall I do it as part of atomic_check for this case ? >> With this approach, dspps will get allocated on first come first serve >> basis > >I don't think that this is what we have agreed upon. > >> @Rob, is it possible to send color management property during modeset, in >that case, we can use it for dspp allocation to the datapath. The current approach >doesn't assume it. >> On chrome compositor, I see that color property was being set in the >subsequent commits but not in modeset. >> >>> >>>> if (intf_count == 2) >>>> topology.num_lm = 2; >>>> @@ -573,11 +575,9 @@ static struct msm_display_topology >>> dpu_encoder_get_topology( >>>> else >>>> topology.num_lm = (mode->hdisplay > >>>> MAX_HDISPLAY_SPLIT) ? 2 : 1; >>>> >>>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { >>>> - if (dpu_kms->catalog->dspp && >>>> - (dpu_kms->catalog->dspp_count >= topology.num_lm)) >>>> - topology.num_dspp = topology.num_lm; >>>> - } >>>> + if (dpu_kms->catalog->dspp && >>>> + (dpu_kms->catalog->dspp_count >= topology.num_lm)) >>>> + topology.num_dspp = topology.num_lm; >>>> >>>> topology.num_enc = 0; >>>> topology.num_intf = intf_count; @@ -643,7 +643,7 @@ static >>>> int dpu_encoder_virt_atomic_check( >>>> } >>>> } >>>> >>>> - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); >>>> + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, >>>> + adj_mode, crtc_state); >>>> >>>> /* Reserve dynamic resources now. */ >>>> if (!ret) { >>> >>> -- >>> With best wishes >>> Dmitry >> > >-- >With best wishes >Dmitry
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 9c6817b..e39b345 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc) static struct msm_display_topology dpu_encoder_get_topology( struct dpu_encoder_virt *dpu_enc, struct dpu_kms *dpu_kms, - struct drm_display_mode *mode) + struct drm_display_mode *mode, + struct drm_crtc_state *crtc_state) { struct msm_display_topology topology = {0}; int i, intf_count = 0; @@ -563,8 +564,9 @@ static struct msm_display_topology dpu_encoder_get_topology( * 1 LM, 1 INTF * 2 LM, 1 INTF (stream merge to support high resolution interfaces) * - * Adding color blocks only to primary interface if available in - * sufficient number + * dspp blocks are made optional. If RM manager cannot allocate + * dspp blocks, then reservations will still go through with non dspp LM's + * so as to allow color management support via composer fallbacks */ if (intf_count == 2) topology.num_lm = 2; @@ -573,11 +575,9 @@ static struct msm_display_topology dpu_encoder_get_topology( else topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1; - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) { - if (dpu_kms->catalog->dspp && - (dpu_kms->catalog->dspp_count >= topology.num_lm)) - topology.num_dspp = topology.num_lm; - } + if (dpu_kms->catalog->dspp && + (dpu_kms->catalog->dspp_count >= topology.num_lm)) + topology.num_dspp = topology.num_lm; topology.num_enc = 0; topology.num_intf = intf_count; @@ -643,7 +643,7 @@ static int dpu_encoder_virt_atomic_check( } } - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); + topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state); /* Reserve dynamic resources now. */ if (!ret) {
Allow dspps to be populated as a requirement for all the encoder types it need not be just DSI. If for any encoder the dspp allocation doesn't go through then there can be an option to fallback for color features. Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)