| Message ID | 20240125193834.7065-8-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for CDM over DP | expand |
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. > > 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 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/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. > >> >> 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 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 > > > >> > >> 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/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 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. >> >>>>> >>>>>> >>>>>> 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; >>>>>>> >>>>> >>>>> >>>>> >>> >>> >>> > > >
On Tue, 30 Jan 2024 at 06:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > 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 :( What about explicitly disabling it in such a case? I mean something like: if (dpu_core_rev >= 5.0 && !(enc->hw_dsc && !enc->wide_bus_en)) intf_cfg |= INTF_CFG2_HCTL_EN; > > 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; > >>>>>>> > >>>>> > >>>>> > >>>>> > >>> > >>> > >>> > > > > > >
On 1/29/2024 9:28 PM, Dmitry Baryshkov wrote: > On Tue, 30 Jan 2024 at 06:10, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> 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 :( > > What about explicitly disabling it in such a case? > I mean something like: > > if (dpu_core_rev >= 5.0 && !(enc->hw_dsc && !enc->wide_bus_en)) > intf_cfg |= INTF_CFG2_HCTL_EN; > Condition is correct now. But we dont have enc or dpu version in this function. We need to pass a new parameter called compression_en to dpu_hw_intf_timing_params and set it when dsc is used and then do this. We have widebus_en already in dpu_hw_intf_timing_params. This was anyway part of the DSC over DP but we can add that here and then the if (ctx->cap->features & BIT(DPU_DATA_HCTL_EN)) is indicative of dpu version >=5 so we can move this setup there. > >> >> 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(-)