Re: [PATCH 07/17] drm/msm/dpu: disallow widebus en in INTF_CONFIG2 when DP is YUV420

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

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@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
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,
         * 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;

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux