| Message ID | 20230211231259.1308718-7-dmitry.baryshkov@linaro.org |
|---|---|
| State | Accepted |
| Commit | d113d267c3bfa07c0c8e9f68068a18fa18970c9c |
| Headers | show |
| Series | drm/msm/dpu: rework HW catalog | expand |
On 13/02/2023 12:16, Dmitry Baryshkov wrote: > On 13/02/2023 12:41, Neil Armstrong wrote: >> On 12/02/2023 00:12, Dmitry Baryshkov wrote: >>> QSEED4 is a newer variant of QSEED3LITE, which should be used on >>> sm8550. Fix the DPU caps structure and used feature masks. >> >> I found nowhere SM8550 uses Qseed4, on downstream DT, it's written: >> qcom,sde-qseed-sw-lib-rev = "qseedv3lite"; >> qcom,sde-qseed-scalar-version = <0x3002>; > > And then the techpack tells us starting from 0x3000 the v3lite is v4: > > https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L59 > > https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L102 OK then: Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > >> >> Neil >>> >>> Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 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 192fff9238f9..c4e45c472685 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c >>> @@ -458,7 +458,7 @@ static const struct dpu_caps sm8450_dpu_caps = { >>> static const struct dpu_caps sm8550_dpu_caps = { >>> .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH, >>> .max_mixer_blendstages = 0xb, >>> - .qseed_type = DPU_SSPP_SCALER_QSEED3LITE, >>> + .qseed_type = DPU_SSPP_SCALER_QSEED4, >>> .smart_dma_rev = DPU_SSPP_SMART_DMA_V2, /* TODO: v2.5 */ >>> .ubwc_version = DPU_HW_UBWC_VER_40, >>> .has_src_split = true, >>> @@ -1301,13 +1301,13 @@ static const struct dpu_sspp_cfg sm8450_sspp[] = { >>> }; >>> static const struct dpu_sspp_sub_blks sm8550_vig_sblk_0 = >>> - _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED3LITE); >>> + _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED4); >>> static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 = >>> - _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED3LITE); >>> + _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED4); >>> static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 = >>> - _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED3LITE); >>> + _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4); >>> static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 = >>> - _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED3LITE); >>> + _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4); >>> static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK("12", 5); >>> static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6); >> >
On 26/02/2023 01:27, Abhinav Kumar wrote: > Hi Dmitry > > On 2/25/2023 3:06 PM, Dmitry Baryshkov wrote: >> On 24/02/2023 22:51, Abhinav Kumar wrote: >>> >>> >>> On 2/13/2023 9:36 AM, neil.armstrong@linaro.org wrote: >>>> On 13/02/2023 12:16, Dmitry Baryshkov wrote: >>>>> On 13/02/2023 12:41, Neil Armstrong wrote: >>>>>> On 12/02/2023 00:12, Dmitry Baryshkov wrote: >>>>>>> QSEED4 is a newer variant of QSEED3LITE, which should be used on >>>>>>> sm8550. Fix the DPU caps structure and used feature masks. >>>>>> >>>>>> I found nowhere SM8550 uses Qseed4, on downstream DT, it's written: >>>>>> qcom,sde-qseed-sw-lib-rev = "qseedv3lite"; >>>>>> qcom,sde-qseed-scalar-version = <0x3002>; >>>>> >>>>> And then the techpack tells us starting from 0x3000 the v3lite is v4: >>>>> >>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L59 >>>>> >>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L102 >>>> >>>> OK then: >>>> >>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> >>>>> >>> >>> This little bit of confusion is because with downstream, the qseed is >>> a separate usermode library having its own revision. So the SW lib >>> version in this case is not exactly correlating with the scalar HW >>> revision. >> >> Can you possibly spend some more words here? I see that sde_hw_utils.c >> programs scalers slightly different depending on the version of the >> scaler. At some point the SDE driver was reading the register to >> determine the revision. Then it switched to the revision specified in >> the DTS (which, as far as I understand, corresponds to the HW register >> contents). >> >> So, where does SW revision come into the play? (and which library are >> we talking about?). Is the 'v3lite' an SW revision? Or is the 0x3002 >> an SW revision? >> > > qcom,sde-qseed-sw-lib-rev is the SW library revision for libscale. > > This is a proprietary library used to calculate the LUTs for the qseed > block. Its not used in the upstream version of the driver. > > For upstream driver, the driver uses default settings for the LUTs which > work for most of the common use-cases we see. Ack, thanks for the explanation. If default settings work, that's good. When you wrote about the proprietary lib, I started wondering if we loose anything (like worse quality of the images, etc). > > You can refer the below property names, there are programmed by the lib > for the downstream driver. > > 3733 msm_property_install_range( > 3734 &psde->property_info, "scaler_v2", > 3735 0x0, 0, ~0, 0, PLANE_PROP_SCALER_V2); > 3736 msm_property_install_blob(&psde->property_info, > 3737 "lut_sep", 0, > 3738 PLANE_PROP_SCALER_LUT_SEP); > > No, 0x3002 is the HW revision of the qseed and thats why this change is > correct because the SW library name/rev doesnt exactly match the qseed > HW revision as its possible that even qseed3lite library can support the > QSEED4 HW. > > So we should be going off qcom,sde-qseed-scalar-version and not > qcom,sde-qseed-sw-lib-rev. Thanks! So, we should further drop the v3lite/v4 from the scaler name/subblock and use qseed3 everywhere. Correct? > >>> >>> Since upstream DPU only cares about the HW revision of the scaler, we >>> should be going off the qcom,sde-qseed-scalar-version. >>> >>> This change LGTM, >>> >>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >>
On 2/25/2023 4:06 PM, Dmitry Baryshkov wrote: > On 26/02/2023 01:27, Abhinav Kumar wrote: >> Hi Dmitry >> >> On 2/25/2023 3:06 PM, Dmitry Baryshkov wrote: >>> On 24/02/2023 22:51, Abhinav Kumar wrote: >>>> >>>> >>>> On 2/13/2023 9:36 AM, neil.armstrong@linaro.org wrote: >>>>> On 13/02/2023 12:16, Dmitry Baryshkov wrote: >>>>>> On 13/02/2023 12:41, Neil Armstrong wrote: >>>>>>> On 12/02/2023 00:12, Dmitry Baryshkov wrote: >>>>>>>> QSEED4 is a newer variant of QSEED3LITE, which should be used on >>>>>>>> sm8550. Fix the DPU caps structure and used feature masks. >>>>>>> >>>>>>> I found nowhere SM8550 uses Qseed4, on downstream DT, it's written: >>>>>>> qcom,sde-qseed-sw-lib-rev = "qseedv3lite"; >>>>>>> qcom,sde-qseed-scalar-version = <0x3002>; >>>>>> >>>>>> And then the techpack tells us starting from 0x3000 the v3lite is v4: >>>>>> >>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L59 >>>>>> >>>>>> >>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L102 >>>>>> >>>>> >>>>> OK then: >>>>> >>>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> >>>>>> >>>> >>>> This little bit of confusion is because with downstream, the qseed >>>> is a separate usermode library having its own revision. So the SW >>>> lib version in this case is not exactly correlating with the scalar >>>> HW revision. >>> >>> Can you possibly spend some more words here? I see that >>> sde_hw_utils.c programs scalers slightly different depending on the >>> version of the scaler. At some point the SDE driver was reading the >>> register to determine the revision. Then it switched to the revision >>> specified in the DTS (which, as far as I understand, corresponds to >>> the HW register contents). >>> >>> So, where does SW revision come into the play? (and which library are >>> we talking about?). Is the 'v3lite' an SW revision? Or is the 0x3002 >>> an SW revision? >>> >> >> qcom,sde-qseed-sw-lib-rev is the SW library revision for libscale. >> >> This is a proprietary library used to calculate the LUTs for the qseed >> block. Its not used in the upstream version of the driver. >> >> For upstream driver, the driver uses default settings for the LUTs >> which work for most of the common use-cases we see. > > Ack, thanks for the explanation. If default settings work, that's good. > When you wrote about the proprietary lib, I started wondering if we > loose anything (like worse quality of the images, etc). > >> >> You can refer the below property names, there are programmed by the >> lib for the downstream driver. >> >> 3733 msm_property_install_range( >> 3734 &psde->property_info, "scaler_v2", >> 3735 0x0, 0, ~0, 0, PLANE_PROP_SCALER_V2); >> 3736 msm_property_install_blob(&psde->property_info, >> 3737 "lut_sep", 0, >> 3738 PLANE_PROP_SCALER_LUT_SEP); >> >> No, 0x3002 is the HW revision of the qseed and thats why this change >> is correct because the SW library name/rev doesnt exactly match the >> qseed HW revision as its possible that even qseed3lite library can >> support the QSEED4 HW. >> >> So we should be going off qcom,sde-qseed-scalar-version and not >> qcom,sde-qseed-sw-lib-rev. > > Thanks! > > So, we should further drop the v3lite/v4 from the scaler name/subblock > and use qseed3 everywhere. Correct? > No, even that wont be correct because as you pointed out anything we need to handle < QSEED4 case differently from others over here 537 if (pdpu->pipe_hw->cap->features & 538 BIT(DPU_SSPP_SCALER_QSEED4)) { 539 scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H; 540 scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V; 541 } else { 542 scale_cfg->preload_x[i] = DPU_QSEED3_DEFAULT_PRELOAD_H; 543 scale_cfg->preload_y[i] = DPU_QSEED3_DEFAULT_PRELOAD_V; 544 } If we want to clean this up more accurately, we should add qseed_rev in the dpu caps or rename qseed_type to that which will hold the 0x3xxx value and then write a small util which would set the the bit correctly based on the qseed rev (0x3xxxx value). >> >>>> >>>> Since upstream DPU only cares about the HW revision of the scaler, >>>> we should be going off the qcom,sde-qseed-scalar-version. >>>> >>>> This change LGTM, >>>> >>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> >>> >
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 192fff9238f9..c4e45c472685 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -458,7 +458,7 @@ static const struct dpu_caps sm8450_dpu_caps = { static const struct dpu_caps sm8550_dpu_caps = { .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH, .max_mixer_blendstages = 0xb, - .qseed_type = DPU_SSPP_SCALER_QSEED3LITE, + .qseed_type = DPU_SSPP_SCALER_QSEED4, .smart_dma_rev = DPU_SSPP_SMART_DMA_V2, /* TODO: v2.5 */ .ubwc_version = DPU_HW_UBWC_VER_40, .has_src_split = true, @@ -1301,13 +1301,13 @@ static const struct dpu_sspp_cfg sm8450_sspp[] = { }; static const struct dpu_sspp_sub_blks sm8550_vig_sblk_0 = - _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED3LITE); + _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED4); static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 = - _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED3LITE); + _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED4); static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 = - _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED3LITE); + _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4); static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 = - _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED3LITE); + _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4); static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK("12", 5); static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6);
QSEED4 is a newer variant of QSEED3LITE, which should be used on sm8550. Fix the DPU caps structure and used feature masks. Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)