| Message ID | 20230525-add-widebus-support-v1-1-c7069f2efca1@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] drm/msm/dpu: Add DPU_INTF_DATABUS_WIDEN feature flag for DPU >= 5.0 | expand |
On 2023-06-14 14:23:38, Marijn Suijten wrote: <snip> > 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. Never mind, this was a bad conflict-resolve. Jessica's original BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was only writing, and the way I conflict-resolved that caused the write of BURST_MODE to overwrite the RMW DATABUS_WIDEN. If both are moved to dsi_ctrl_config(), we could do a read, add both flags in conditionally, and write. - Marijn
On 2023-06-16 14:06:47, Abhinav Kumar wrote: > On 6/14/2023 3:49 PM, Marijn Suijten wrote: > > On 2023-06-14 14:23:38, Marijn Suijten wrote: > > <snip> > >> 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. > > > > Never mind, this was a bad conflict-resolve. Jessica's original > > BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was > > only writing, and the way I conflict-resolved that caused the write of > > BURST_MODE to overwrite the RMW DATABUS_WIDEN. > > > > If both are moved to dsi_ctrl_config(), we could do a read, add both > > flags in conditionally, and write. > > > > So just to confirm, there is no issue on your 8350 setup with widebus > enabled right? After properly conflict-resolving both series so that they either both RMW, or combine the two bit-sets (the same you just discussed _not_ doing on the DPU side for DATABUS_WIDEN|COMPRESSION...) before a single write, it works fine! - Marijn
On Wed, 21 Jun 2023 at 00:37, Jessica Zhang <quic_jesszhan@quicinc.com> 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@quicinc.com> > >>>>>>>>>> --- > >>>>>>>>>> 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 > >
On 6/20/2023 3:11 PM, Dmitry Baryshkov wrote: > On Wed, 21 Jun 2023 at 00:37, Jessica Zhang <quic_jesszhan@quicinc.com> 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@quicinc.com> >>>>>>>>>>>> --- >>>>>>>>>>>> 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. Got it -- so basically have DSI do the version check *and* the DSC check in some `msm_dsi_is_widebus_enabled()` helper and have DPU use that helper to check if widebus is enabled. I think that should be fine. Thanks, Jessica Zhang > >> >> 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
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)) #define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS)) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index b860784ade72..b9939e00f5e0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -182,6 +182,7 @@ enum { * than video timing * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register + * @DPU_INTF_DATABUS_WIDEN INTF block has DATABUS_WIDEN register * @DPU_INTF_MAX */ enum { @@ -190,6 +191,7 @@ enum { DPU_DATA_HCTL_EN, DPU_INTF_STATUS_SUPPORTED, DPU_INTF_DATA_COMPRESS, + DPU_INTF_DATABUS_WIDEN, DPU_INTF_MAX };
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@quicinc.com> --- 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(-)