On 1/29/2024 5:43 PM, Dmitry Baryshkov wrote:
On Tue, 30 Jan 2024 at 03:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/29/2024 4:03 PM, Dmitry Baryshkov wrote:
On Tue, 30 Jan 2024 at 01:51, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 1/27/2024 9:33 PM, Dmitry Baryshkov wrote:
On Sun, 28 Jan 2024 at 07:16, Paloma Arellano <quic_parellan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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;
