On Sun, 5 Feb 2023 at 01:20, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > > > On 2/4/2023 1:08 PM, Dmitry Baryshkov wrote: > > On 04/02/2023 20:35, Abhinav Kumar wrote: > >> > >> > >> On 2/4/2023 2:43 AM, Dmitry Baryshkov wrote: > >>> On 04/02/2023 07:10, Abhinav Kumar wrote: > >>>> > >>>> > >>>> On 2/3/2023 8:10 PM, Dmitry Baryshkov wrote: > >>>>> On 04/02/2023 04:43, Abhinav Kumar wrote: > >>>>>> > >>>>>> > >>>>>> On 2/3/2023 6:29 PM, Dmitry Baryshkov wrote: > >>>>>>> On 04/02/2023 01:35, Abhinav Kumar wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: > >>>>>>>>> Downstream driver uses dpu->caps->smart_dma_rev to update > >>>>>>>>> sspp->cap->features with the bit corresponding to the supported > >>>>>>>>> SmartDMA > >>>>>>>>> version. Upstream driver does not do this, resulting in SSPP > >>>>>>>>> subdriver > >>>>>>>>> not enbaling setup_multirect callback. Add corresponding > >>>>>>>>> SmartDMA SSPP > >>>>>>>>> feature bits to dpu hw catalog. > >>>>>>>>> > >>>>>>>> > >>>>>>>> While reviewing this patch, I had a first hand experience of how > >>>>>>>> we are reusing SSPP bitmasks for so many chipsets but I think > >>>>>>>> overall you got them right here :) > >>>>>>>> > >>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > >>>>>>>>> --- > >>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++++--- > >>>>>>>>> 1 file changed, 7 insertions(+), 3 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 cf053e8f081e..fc818b0273e7 100644 > >>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c > >>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c > >>>>>>>>> @@ -21,13 +21,16 @@ > >>>>>>>>> (VIG_MASK | BIT(DPU_SSPP_SCALER_QSEED3)) > >>>>>>>>> #define VIG_SDM845_MASK \ > >>>>>>>>> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | > >>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3)) > >>>>>>>>> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | > >>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3) |\ > >>>>>>>>> + BIT(DPU_SSPP_SMART_DMA_V2)) > >>>>>>>>> #define VIG_SC7180_MASK \ > >>>>>>>>> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | > >>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED4)) > >>>>>>>>> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | > >>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED4) |\ > >>>>>>>>> + BIT(DPU_SSPP_SMART_DMA_V2)) > >>>>>>>>> #define VIG_SM8250_MASK \ > >>>>>>>>> - (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | > >>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE)) > >>>>>>>>> + (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | > >>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE) |\ > >>>>>>>>> + BIT(DPU_SSPP_SMART_DMA_V2)) > >>>>>>>>> #define VIG_QCM2290_MASK (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL)) > >>>>>>>>> @@ -42,6 +45,7 @@ > >>>>>>>>> #define DMA_SDM845_MASK \ > >>>>>>>>> (BIT(DPU_SSPP_SRC) | BIT(DPU_SSPP_QOS) | > >>>>>>>>> BIT(DPU_SSPP_QOS_8LVL) |\ > >>>>>>>>> BIT(DPU_SSPP_TS_PREFILL) | BIT(DPU_SSPP_TS_PREFILL_REC1) |\ > >>>>>>>>> + BIT(DPU_SSPP_SMART_DMA_V2) |\ > >>>>>>>>> BIT(DPU_SSPP_CDP) | BIT(DPU_SSPP_EXCL_RECT)) > >>>>>>>>> #define DMA_CURSOR_SDM845_MASK \ > >>>>>>>> > >>>>>>>> VIG_SDM845_MASK and DMA_SDM845_MASK are used for many other > >>>>>>>> chipsets like 8250, 8450, 8550. > >>>>>>>> > >>>>>>>> At the moment, for visual validation of this series, I only have > >>>>>>>> sc7180/sc7280. We are leaving the rest for CI. > >>>>>>>> > >>>>>>>> Was that an intentional approach? > >>>>>>>> > >>>>>>>> If so, we will need tested-by tags from folks having > >>>>>>>> 8350/8450/8550/sc8280x,qcm2290? > >>>>>>>> > >>>>>>>> I am only owning the visual validation on sc7280 atm. > >>>>>>> > >>>>>>> I'm not quite sure what is your intent here. Are there any SoCs > >>>>>>> after 845 that do not have SmartDMA 2.5? Or do you propose to > >>>>>>> enable SmartDMA only for the chipsets that we can visually test? > >>>>>>> That sounds strange. > >>>>>>> > >>>>>> > >>>>>> Yes I was thinking to enable smartDMA at the moment on chipsets > >>>>>> which we can validate visually that display comes up. But I am not > >>>>>> sure if thats entirely practical. > >>>>>> > >>>>>> But the intent was I just want to make sure basic display does > >>>>>> come up with smartDMA enabled if we are enabling it for all chipsets. > >>>>> > >>>>> I don't think it is practical or logical. We don't require > >>>>> validating other changes on all possible chipsets, so what is so > >>>>> different with this one? > >>>>> > >>>> > >>>> Thats because with smartDMA if the programming of stages goes wrong > >>>> we could potentially just see a blank screen. Its not about other > >>>> changes, this change in particular controls enabling a feature. > >>>> > >>>> But thats just my thought. I am not going to request to ensure this > >>>> or block this for this. > >>>> > >>>> You can still have my > >>>> > >>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> > >>>> > >>>> But think of the validations that have to be done before we merge it. > >>> > >>> The usual way: verify as much as feasible and let anybody else > >>> complain during the development cycle. > >>> > >> > >> Well, our perspective is to enable the feature on devices on which you > >> are able to test and not enable then wait for others to complain. > > > > This would not be really practical. There are plenty of people who can > > test things on obscure platforms, but unfortunately far less amount of > > people who tightly follow the development and can track which new > > feature applies to a particular platform. I hope to be able to fix that > > slightly with the hw catalog rework. However enabling features on other > > platforms definitely requires more knowledge than simply testing the > > kernel. > > > >> > >> I did not say test all devices. My point was to enable smartDMA on > >> devices which we are able to test. > >> > >> There are other examples of this, like inline rotation, writeback etc. > >> which are at the moment enabled only on devices which QC or others > >> have tested on. > > > > But at the time it was added, inline rotation 2.0 could only be > > supported on sc7280. Probably we should expand it not to sc8280xp and > > sm8[345]50. > > > > For WB I don't remember which platforms were supported at the moment it > > was added. But it's also worth expanding support to new platforms. > > > > And, as we speak about testing, is there an easy way to setup the plane > > with UBWC format modifier? Also, did the WB support patches land into > > libdrm? > > > > I will check the compositor code and update you on the UWBC format > modifier as I am not too familiar with it. Ideally it would be nice to support ubwc planes in some simple tool, e.g. modetest. > > libdrm always supported virtual encoder > https://github.com/grate-driver/libdrm/blob/master/include/drm/drm_mode.h#L352 > > What other support patches are needed? Right now we only use IGT to > validate writeback. I remember there was a patchset to make modeset to support using writeback. What was its fate? > > >> So when i said my suggestion was not practical, yes because if you > >> want to go ahead with this change in the current form, you would have > >> to validate all the chipsets as you are enabling smartDMA on all of them. > >> > >> If you enable smartDMA only on the chipsets you OR others can validate > >> and give Tested-by for like I was planning to do for sc7280, then I am > >> not sure why it doesnt sound logical. > >> > >> But like I said, thats my perspective. I will let you decide as you > >> would know how confident you are with this getting enabled for all > >> chipsets upstream. > > > > I'd say, that once tested on some of the platforms and granted that even > > smalled (qcm2290, sm6115) platforms support smartdma, it will be safe to > > enable smart DMA globablly for every SoC >= sdm845. If I remember > > correctly, msm8998 (and sdm660/630) support smartdma/rect only on DMA > > planes. Is it correct? > > > > > Yes thats right msm8998 supports smartdma only on DMA sspps. Good -- With best wishes Dmitry
