On Wed, 21 Jun 2023 at 00:37, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote: > > > > On 6/16/2023 5:37 PM, Dmitry Baryshkov wrote: > > On 17/06/2023 00:10, Abhinav Kumar wrote: > >> > >> > >> On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote: > >>> On 14/06/2023 23:39, Abhinav Kumar wrote: > >>>> > >>>> > >>>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote: > >>>>> > >>>>> > >>>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote: > >>>>>> > >>>>>> > >>>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote: > >>>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote: > >>>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote: > >>>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote: > >>>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data > >>>>>>>>>> to be sent > >>>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> > >>>>>>>>>> --- > >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++- > >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++ > >>>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-) > >>>>>>>>>> > >>>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644 > >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c > >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c > >>>>>>>>>> @@ -103,7 +103,8 @@ > >>>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \ > >>>>>>>>>> BIT(DPU_INTF_TE) | \ > >>>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \ > >>>>>>>>>> - BIT(DPU_DATA_HCTL_EN)) > >>>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \ > >>>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN)) > >>>>>>>>> > >>>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. > >>>>>>>>> In the > >>>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ > >>>>>>>>> only, > >>>>>>>>> meaning DPU and DSI are now desynced, and the output is completely > >>>>>>>>> corrupted. > >>>>>>> > >>>> > >>>> I looked at the internal docs and also this change. This change is > >>>> incorrect because this will try to enable widebus for DPU >= 5.0 and > >>>> DSI >= 2.5 > >>>> > >>>> That was not the intended right condition as thats not what the docs > >>>> say. > >>>> > >>>> We should enable for DPU >= 7.0 and DSI >= 2.5 > >>>> > >>>> Is there any combination where this compatibility is broken? That > >>>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that > >>>> was incorrect) > >>>> > >>>> Part of this confusion is because of catalog macro re-use again. > >>>> > >>>> This series is a good candidate and infact I think we should only do > >>>> core_revision based check on DPU and DSI to avoid bringing the > >>>> catalog mess into this. > >>> > >>> I have just a single request here: can we please have the same > >>> approach for both DSI and DP? I don't mind changing DP code if it > >>> makes it better. If you don't have better reasons, I like the idea of > >>> DSI/DP dictating whether wide bus should be used on the particular > >>> interface. It allows us to handle possible errata or corner cases > >>> there. Another option would be to make DPU tell DSI / DP whether the > >>> wide bus is enabled or not, but I'd say, this is slightly worse > >>> solution. > >>> > >> > >> Today, DP's widebus does not check if DPU supports that or not. > >> > >> DPU encoder queries the DP whether widebus is available and enables it. > >> > >> We can also do the same thing for DSI. > >> > >> So for intf_type of DSI, DPU encoder will query DSI if it supports > >> widebus. > > > > Not if it supports wide bus. But the check is whether enabling wide bus > > is requested by the output driver (DSI/DP). > > Hi Dmitry, > > Can you explain what you mean by "requested by output driver"? FWIW, if > the DSI version supports wide bus && if DSC is enabled, then wide bus > will be enabled in DSI. Like for DP, let DSI select whether a wide bus should be enabled or not, then let DPU get this flag from DSI. > > Thanks, > > Jessica Zhang > > > > >> > >> Then DSI will do its version checks and for DSC it will say yes. > >> > >> This way, we will never check for the DPU's core revision for DSI and > >> purely rely of DP/DSI's hw revisions. > >> > >> Thats fine with me because that way we again just rely on the hw > >> revision to enable the feature. > >> > >> But as a result I am still going to drop this patch which adds widebus > >> to the catalog as a dpu cap which I always wanted to do anyway as we > >> will just rely on the DSI and DP hw revisions. > > > > Yep. > > > >> > >>>> > >>>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also > >>>>>>> corrupted with this series so something else on this series might be > >>>>>>> broken. > >>>>>>> > >>>>> > >>>>> Missed this response. That seems strange. > >>>>> > >>>>> This series was tested on SM8350 HDK with a command mode panel. > >>>>> > >>>>> We will fix the DPU-DSI handshake and post a v2 but your issue > >>>>> needs investigation in parallel. > >>>>> > >>>>> So another bug to track that would be great. > >>>>> > >>>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to > >>>>>>>>> communicate > >>>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it? > >>>>>>>> > >>>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU > >>>>>>>> asks the > >>>>>>>> actual video output driver if widebus is to be enabled. > >>>>>>> > >>>>>> > >>>>>> I was afraid of this. This series was made on an assumption that > >>>>>> the DPU version of widebus and DSI version of widebus would be > >>>>>> compatible but looks like already SM8150 is an outlier. > >>>>>> > >>>>>> Yes, I think we have to go with second approach. > >>>>>> > >>>>>> DPU queries DSI if it supports widebus and enables it. > >>>>>> > >>>>>> Thanks for your responses. We will post a v2. > >>>>>> > >>>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus > >>>>>>> feature, > >>>>>>> but the DSI does not until two revisions later? Or is this > >>>>>>> available on > >>>>>>> every interface, but only for a different (probably DP) encoder > >>>>>>> block? > >>>>>>> > >>>>>> > >>>>>> Yes its strange. > >>>>>> > >>>> > >>>> I have clarified this above. Its not strange but appeared strange > >>>> because we were checking wrong conditions. > >>>> > >>>> > >>>>>>> - Marijn > >>> > > > > -- > > With best wishes > > Dmitry > > -- With best wishes Dmitry
