>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
>Sent: Tuesday, January 17, 2023 10:26 PM
>To: Kalyan Thota (QUIC) <quic_kaly...@quicinc.com>; dri-
>de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicet...@vger.kernel.org
>Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
>diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
><quic_vpoli...@quicinc.com>; Abhinav Kumar (QUIC)
><quic_abhin...@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_kaly...@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
