From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm/mdss: correct UBWC programming sequences
Date: Thu, 12 Mar 2026 07:21:52 +1000
Message-ID: <review-patch1-20260311-ubwc-rework-v2-1-69f718f2a1c9@oss.qualcomm.com>
In-Reply-To: <20260311-ubwc-rework-v2-1-69f718f2a1c9@oss.qualcomm.com>
References: <20260311-ubwc-rework-v2-0-69f718f2a1c9@oss.qualcomm.com> <20260311-ubwc-rework-v2-1-69f718f2a1c9@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the most consequential patch in the series. It changes MDSS UBWC setup from dispatching based on `ubwc_dec_version` to dispatching based on the MDSS hardware revision register.

The `MDSS_HW_VER` macro:
```c
#define MDSS_HW_VER(major, minor, step)	\
	((((major) & 0xf) << 28) |	\
	 (((minor) & 0xfff) << 16) |	\
	 ((step) & 0xffff))
```
is a local definition. It would be worth checking that this matches `REG_MDSS_HW_VERSION` format used elsewhere.

The new `msm_mdss_6x_setup_ubwc` consolidates the old `_dec_40` and `_dec_50` functions and adds a UBWC version tag ladder. One concern: the old `msm_mdss_setup_ubwc_dec_20` set `MDSS_UBWC_STATIC_UBWC_BANK_SPREAD` when `data->ubwc_bank_spread` was true, and the new `msm_mdss_4x_setup_ubwc` does not. Is this correct? Looking at the old code, `_dec_20` had bank_spread, but the new `_4x` doesn't. The old `_dec_20` also had `data->ubwc_swizzle` without masking, while the new `_4x` masks with `& 0x1`. The patch description says the old code had some incorrect assumptions - if so, this should be documented more clearly.

Also in this patch, `msm_mdss_6x_setup_ubwc` adds:
```c
if (data->ubwc_enc_version == UBWC_1_0)
    value |= MDSS_UBWC_STATIC_UBWC_MIN_ACC_LEN(1);
```
This wasn't in the old `_dec_40` / `_dec_50` paths. Is there actually a MDSS 6.x SoC that uses UBWC 1.0 encoding? This seems like defensive programming but could be confusing.

The Fixes tag references `d68db6069a8e`, which seems appropriate.

---
Generated by Claude Code Patch Reviewer