| Message ID | 20240125193834.7065-8-quic_parellan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for CDM over DP | expand |
On Thu, 25 Jan 2024 at 23:26, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 25/01/2024 21:38, Paloma Arellano wrote: > > INTF_CONFIG2 register cannot have widebus enabled when DP format is > > YUV420. Therefore, program the INTF to send 1 ppc. > > I think this is handled in the DP driver, where we disallow wide bus for > YUV 4:2:0 modes. Maybe this needs some explanation from my side: I think it will be better to have separate conditionals for setting HCTL_EN and for DATABUS_WIDEN. > > > > > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > index 6bba531d6dc41..bfb93f02fe7c1 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > > @@ -168,7 +168,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, > > * video timing. It is recommended to enable it for all cases, except > > * if compression is enabled in 1 pixel per clock mode > > */ > > - if (p->wide_bus_en) > > + if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420) > > + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; > > + else if (p->wide_bus_en) > > intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; > > > > data_width = p->width; > > -- > With best wishes > Dmitry > -- With best wishes Dmitry
On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote: > On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> >> >> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote: >>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is >>>> YUV420. Therefore, program the INTF to send 1 ppc. >>> >>> I think this is handled in the DP driver, where we disallow wide bus >>> for YUV 4:2:0 modes. >> Yes we do disallow wide bus for YUV420 modes, but we still need to >> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add >> this check. > > As I wrote in my second email, I'd prefer to have one if which guards > HCTL_EN and another one for WIDEN > Its hard to separate out the conditions just for HCTL_EN . Its more about handling the various pixel per clock combinations. But, here is how I can best summarize it. Lets consider DSI and DP separately: 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ). This is same the same condition as widebus today in msm_dsi_host_is_wide_bus_enabled(). Hence no changes needed for DSI. 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case as they are independent cases. We dont support YUV420 + DSC case. There are other cases which fall outside of this bucket but they are optional ones. We only follow the "required" ones. With this summary in mind, I am fine with what we have except perhaps better documentation above this block. When DSC over DP gets added, I am expecting no changes to this block as it will fall under the widebus_en case. With this information, how else would you like the check? >>> >>>> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> index 6bba531d6dc41..bfb93f02fe7c1 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>> @@ -168,7 +168,9 @@ static void >>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, >>>> * video timing. It is recommended to enable it for all cases, >>>> except >>>> * if compression is enabled in 1 pixel per clock mode >>>> */ >>>> - if (p->wide_bus_en) >>>> + if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420) >>>> + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; >>>> + else if (p->wide_bus_en) >>>> intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; >>>> data_width = p->width; >>> > > >
On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote: > > On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >> > >> > >> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote: > >>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is > >>>> YUV420. Therefore, program the INTF to send 1 ppc. > >>> > >>> I think this is handled in the DP driver, where we disallow wide bus > >>> for YUV 4:2:0 modes. > >> Yes we do disallow wide bus for YUV420 modes, but we still need to > >> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add > >> this check. > > > > As I wrote in my second email, I'd prefer to have one if which guards > > HCTL_EN and another one for WIDEN > > > Its hard to separate out the conditions just for HCTL_EN . Its more > about handling the various pixel per clock combinations. > > But, here is how I can best summarize it. > > Lets consider DSI and DP separately: > > 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ). > > This is same the same condition as widebus today in > msm_dsi_host_is_wide_bus_enabled(). > > Hence no changes needed for DSI. Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being enabled, while you have written that HCTL_EN should be set in all cases on a corresponding platform. > > 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case > as they are independent cases. We dont support YUV420 + DSC case. > > There are other cases which fall outside of this bucket but they are > optional ones. We only follow the "required" ones. > > With this summary in mind, I am fine with what we have except perhaps > better documentation above this block. > > When DSC over DP gets added, I am expecting no changes to this block as > it will fall under the widebus_en case. > > With this information, how else would you like the check? What does this bit really change? > > >>> > >>>> > >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>> --- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++- > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>> index 6bba531d6dc41..bfb93f02fe7c1 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>> @@ -168,7 +168,9 @@ static void > >>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, > >>>> * video timing. It is recommended to enable it for all cases, > >>>> except > >>>> * if compression is enabled in 1 pixel per clock mode > >>>> */ > >>>> - if (p->wide_bus_en) > >>>> + if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420) > >>>> + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; > >>>> + else if (p->wide_bus_en) > >>>> intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; > >>>> data_width = p->width; > >>> > > > > > >
On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote: > > On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote: > >>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >>>> > >>>> > >>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote: > >>>>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is > >>>>>> YUV420. Therefore, program the INTF to send 1 ppc. > >>>>> > >>>>> I think this is handled in the DP driver, where we disallow wide bus > >>>>> for YUV 4:2:0 modes. > >>>> Yes we do disallow wide bus for YUV420 modes, but we still need to > >>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add > >>>> this check. > >>> > >>> As I wrote in my second email, I'd prefer to have one if which guards > >>> HCTL_EN and another one for WIDEN > >>> > >> Its hard to separate out the conditions just for HCTL_EN . Its more > >> about handling the various pixel per clock combinations. > >> > >> But, here is how I can best summarize it. > >> > >> Lets consider DSI and DP separately: > >> > >> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ). > >> > >> This is same the same condition as widebus today in > >> msm_dsi_host_is_wide_bus_enabled(). > >> > >> Hence no changes needed for DSI. > > > > Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being > > enabled, while you have written that HCTL_EN should be set in all > > cases on a corresponding platform. > > > > Agreed. This is true, we should enable HCTL_EN for DSI irrespective of > widebus for the versions I wrote. > > Basically for the non-compressed case. > > I will write something up to fix this for DSI. I think this can go as a > bug fix. > > But that does not change the DP conditions OR in other words, I dont see > anything wrong with this patch yet. > > >> > >> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case > >> as they are independent cases. We dont support YUV420 + DSC case. > >> > >> There are other cases which fall outside of this bucket but they are > >> optional ones. We only follow the "required" ones. > >> > >> With this summary in mind, I am fine with what we have except perhaps > >> better documentation above this block. > >> > >> When DSC over DP gets added, I am expecting no changes to this block as > >> it will fall under the widebus_en case. > >> > >> With this information, how else would you like the check? > > > > What does this bit really change? > > > > This bit basically just tells that the data sent per line is programmed > with INTF_DISPLAY_DATA_HCTL like this cap is suggesting. > > if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) { > DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2); > DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, > display_data_hctl); > DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl); > } > > Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function. Can we enable it unconditionally for DPU >= 5.0? > > >> > >>>>> > >>>>>> > >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++- > >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>>> index 6bba531d6dc41..bfb93f02fe7c1 100644 > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c > >>>>>> @@ -168,7 +168,9 @@ static void > >>>>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, > >>>>>> * video timing. It is recommended to enable it for all cases, > >>>>>> except > >>>>>> * if compression is enabled in 1 pixel per clock mode > >>>>>> */ > >>>>>> - if (p->wide_bus_en) > >>>>>> + if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420) > >>>>>> + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; > >>>>>> + else if (p->wide_bus_en) > >>>>>> intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; > >>>>>> data_width = p->width; > >>>>> > >>> > >>> > >>> > > > > > >
On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote: > On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote: >>> On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote: >>>>> On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> On 1/25/2024 1:26 PM, Dmitry Baryshkov wrote: >>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>>>>>> INTF_CONFIG2 register cannot have widebus enabled when DP format is >>>>>>>> YUV420. Therefore, program the INTF to send 1 ppc. >>>>>>> >>>>>>> I think this is handled in the DP driver, where we disallow wide bus >>>>>>> for YUV 4:2:0 modes. >>>>>> Yes we do disallow wide bus for YUV420 modes, but we still need to >>>>>> program the INTF_CFG2_DATA_HCTL_EN. Therefore, it is necessary to add >>>>>> this check. >>>>> >>>>> As I wrote in my second email, I'd prefer to have one if which guards >>>>> HCTL_EN and another one for WIDEN >>>>> >>>> Its hard to separate out the conditions just for HCTL_EN . Its more >>>> about handling the various pixel per clock combinations. >>>> >>>> But, here is how I can best summarize it. >>>> >>>> Lets consider DSI and DP separately: >>>> >>>> 1) For DSI, for anything > DSI version 2.5 ( DPU version 7 ). >>>> >>>> This is same the same condition as widebus today in >>>> msm_dsi_host_is_wide_bus_enabled(). >>>> >>>> Hence no changes needed for DSI. >>> >>> Not quite. msm_dsi_host_is_wide_bus_enabled() checks for the DSC being >>> enabled, while you have written that HCTL_EN should be set in all >>> cases on a corresponding platform. >>> >> >> Agreed. This is true, we should enable HCTL_EN for DSI irrespective of >> widebus for the versions I wrote. >> >> Basically for the non-compressed case. >> >> I will write something up to fix this for DSI. I think this can go as a >> bug fix. >> >> But that does not change the DP conditions OR in other words, I dont see >> anything wrong with this patch yet. >> >>>> >>>> 2) For DP, whenever widebus is enabled AND YUV420 uncompressed case >>>> as they are independent cases. We dont support YUV420 + DSC case. >>>> >>>> There are other cases which fall outside of this bucket but they are >>>> optional ones. We only follow the "required" ones. >>>> >>>> With this summary in mind, I am fine with what we have except perhaps >>>> better documentation above this block. >>>> >>>> When DSC over DP gets added, I am expecting no changes to this block as >>>> it will fall under the widebus_en case. >>>> >>>> With this information, how else would you like the check? >>> >>> What does this bit really change? >>> >> >> This bit basically just tells that the data sent per line is programmed >> with INTF_DISPLAY_DATA_HCTL like this cap is suggesting. >> >> if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) { >> DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2); >> DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, >> display_data_hctl); >> DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl); >> } >> >> Prior to that it was programmed with INTF_DISPLAY_HCTL in the same function. > > Can we enable it unconditionally for DPU >= 5.0? > There is a corner-case that we should not enable it when compression is enabled without widebus as per the docs :( For DP there will not be a case like that because compression and widebus go together but for DSI, it is possible. So I found that the reset value of this register does cover all cases for DPU >= 7.0 so below fix will address the DSI concern and will fix the issue even for YUV420 cases such as this one for DPU >= 7.0 diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 6bba531d6dc4..cbd5ebd516cd 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -168,6 +168,8 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, * video timing. It is recommended to enable it for all cases, except * if compression is enabled in 1 pixel per clock mode */ + + intf_cfg2 = DPU_REG_READ(c, INTF_CONFIG2); if (p->wide_bus_en) intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; But, this does not still work for DPU < 7.0 such as sc7180 if we try YUV420 over DP on that because its DPU version is 6.2 so we will have to keep this patch for those cases. >> >>>> >>>>>>> >>>>>>>> >>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++- >>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>>> index 6bba531d6dc41..bfb93f02fe7c1 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c >>>>>>>> @@ -168,7 +168,9 @@ static void >>>>>>>> dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, >>>>>>>> * video timing. It is recommended to enable it for all cases, >>>>>>>> except >>>>>>>> * if compression is enabled in 1 pixel per clock mode >>>>>>>> */ >>>>>>>> - if (p->wide_bus_en) >>>>>>>> + if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420) >>>>>>>> + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; >>>>>>>> + else if (p->wide_bus_en) >>>>>>>> intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; >>>>>>>> data_width = p->width; >>>>>>> >>>>> >>>>> >>>>> >>> >>> >>> > > >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 6bba531d6dc41..bfb93f02fe7c1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -168,7 +168,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx, * video timing. It is recommended to enable it for all cases, except * if compression is enabled in 1 pixel per clock mode */ - if (p->wide_bus_en) + if (dp_intf && fmt->base.pixel_format == DRM_FORMAT_YUV420) + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN; + else if (p->wide_bus_en) intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN; data_width = p->width;
INTF_CONFIG2 register cannot have widebus enabled when DP format is YUV420. Therefore, program the INTF to send 1 ppc. Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)