date:20230609

This is a series of changes for DSI to enable command mode support
for DSC v1.2.

This includes:

1) Rounding up `hdisplay / 3` in dsc_timing_setup()
2) Adjusting pclk_rate to account for compression
3) Fixing incorrect uses of slice_count in DSI DSC calculations
4) Setting the DATA_COMPRESS bit when DSC is enabled

With these changes (and the dependency below), DSC v1.2 should work over
DSI in command mode.

Note: Changes that add DSC v1.2 support for video mode will be posted
with the DP support changes.

Depends-on:
 - "add DSC 1.2 dpu supports" [1]
 - "Introduce MSM-specific DSC helpers" [2]
 - "drm/msm/dsi: use mult_frac for pclk_bpp calculation" [3]

[1] https://patchwork.freedesktop.org/series/116789/
[2] https://patchwork.freedesktop.org/series/115833/
[3] https://patchwork.freedesktop.org/patch/538273/?series=118072=1

Signed-off-by: Jessica Zhang 
---
Changes in v6:
- "Adjust" --> "Reduce" in pclk patch title (Marijn)
- dsi_adjust_compressed_pclk() --> dsi_adjust_pclk_for_compression()
  (Marijn)
- Moved dsi_adjust_pclk_for_compression() to before is_bonded_dsi pclk
  adjustment (Dmitry)
- Added documentation comments explaining the pclk_rate and hdisplay
  adjustments related to DSC (Dmitry)
- Only set DATA_COMPRESS bit if DSC is enabled (Abhinav)
- Rebased on top of latest msm-next-lumag branch
- Link to v5: 
https://lore.kernel.org/r/20230405-add-dsc-support-v5-0-028c10850...@quicinc.com

Changes in v5:
- Added newline before enable_compression() function pointer definition
- Rebased on top of "drm/msm/dsi: use mult_frac for pclk_bpp calculation"
- Reworded commit messages for clarity
- Dropped mentions of "soft slice" in commit messages
- "slice_per_packet" -> "slice_per_pkt"
- Picked up reviewed-by tags
- Link to v4: 
https://lore.kernel.org/r/20230405-add-dsc-support-v4-0-15daf84f8...@quicinc.com

Changes in v4:
- Clarified slice_per_pkt comment regarding pkt_per_line calculations
- Reworded commit message for "drm/msm/dsi: Remove incorrect references
  to slice_count"
- Wrapped INTF_SC7280_MASK macro definition in parentheses
- Fixed incorrect commit hash in "msm/drm/dsi: Round up DSC hdisplay
  calculation"
- Picked up Reviewed-by tag
- Link to v3: 
https://lore.kernel.org/r/20230405-add-dsc-support-v3-0-6e1d35a20...@quicinc.com

Changes in v3:
- Added fix to round up hdisplay DSC adjustment
- Fixed inconsistent whitespace in dpu_hw_intf_ops comment doc
- Moved placement of dpu_hw_intf_enable_compression
- Picked up "drm/msm/dsi: Fix calculation for pkt_per_line" patch and
  squashed all slice_count fixes into a single patch
- Use drm_mode_vrefresh() to calculate adjusted pclk rate
- Moved compressed pclk adjustment to dsi_adjust_compressed_pclk() helper
- Rebased changes on top of updated dependencies
- Reworded commit message for "drm/msm/dpu: Set DATA_COMPRESS for
  command mode" for clarity
- Removed revision changelog in commit messages
- Link to v2: 
https://lore.kernel.org/r/20230405-add-dsc-support-v2-0-1072c70e9...@quicinc.com

Changes in v2:
- Changed has_data_compress dpu_cap to a DATA_COMPRESS INTF feature flag
- Changed pclk math to only divide hdisplay by compression ratio
- Reworded word count TODO comment
- Make DATA_COMPRESS an INTF flag
- Read INTF_CONFIG2 before writing to DATA_COMPRESS bit
- Fixed whitespace issue in macro definition
- Removed `inline` from dpu_hw_intf_enable_compression declaration
- Only set dpu_hw_intf_ops.data_compress if DATA_COMPRESS feature is set
- Reworded commit messages and cover letter for clarity
- Link to v1: 
https://lore.kernel.org/r/20230405-add-dsc-support-v1-0-6bc6f03ae...@quicinc.com

---
Jessica Zhang (6):
  msm/drm/dsi: Round up DSC hdisplay calculation
  drm/msm/dsi: Reduce pclk rate for compression
  drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0
  drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2
  drm/msm/dsi: Remove incorrect references to slice_count
  drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations

 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  3 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 13 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h|  3 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c | 59 +-
 6 files changed, 67 insertions(+), 15 deletions(-)
---
base-commit: 8dc20f06b90af85c083866df73dae69236183b62
change-id: 20230405-add-dsc-support-fe130ba49841

Best regards,
-- 
Jessica Zhang

[PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations

Add documentation comments explaining the pclk_rate and hdisplay math
related to DSC.

Signed-off-by: Jessica Zhang 
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index fb1d3a25765f..aeaadc18bc7b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
 static unsigned long dsi_adjust_pclk_for_compression(const struct 
drm_display_mode *mode,
const struct drm_dsc_config *dsc)
 {
+   /*
+* Adjust the pclk rate by calculating a new hdisplay proportional to
+* the compression ratio such that:
+* new_hdisplay = old_hdisplay * target_bpp / source_bpp
+*
+* Porches need not be adjusted during compression.
+*/
int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * 
drm_dsc_get_bpp_int(dsc),
dsc->bits_per_component * 3);
 
@@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, 
bool is_bonded_dsi)
 
/* Divide the display by 3 but keep back/font porch and
 * pulse width same
+*
+* hdisplay will be divided by 3 here to account for the fact
+* that DPU sends 3 bytes per pclk cycle to DSI.
 */
h_total -= hdisplay;
hdisplay = 
DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);

-- 
2.40.1

[PATCH v6 3/6] drm/msm/dpu: Add DPU_INTF_DATA_COMPRESS feature flag for DPU >= 7.0

In DPU 7.x and later, DSC/DCE enablement registers have been moved from
PINGPONG to INTF. Thus, add a DPU_INTF_DATA_COMPRESS feature flag that will
be set if the DATA_COMPRESS register is in the INTF block.

Reviewed-by: Marijn Suijten 
Signed-off-by: Jessica Zhang 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 008df60b00f0..36ba3f58dcdf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -105,7 +105,7 @@
 BIT(DPU_INTF_STATUS_SUPPORTED) | \
 BIT(DPU_DATA_HCTL_EN))
 
-#define INTF_SC7280_MASK (INTF_SC7180_MASK)
+#define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
 
 #define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
 BIT(DPU_WB_UBWC) | \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index d3598dd9d448..b860784ade72 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -181,6 +181,7 @@ enum {
  * @DPU_DATA_HCTL_ENAllows data to be transferred at different 
rate
  *  than video timing
  * @DPU_INTF_STATUS_SUPPORTED   INTF block has INTF_STATUS register
+ * @DPU_INTF_DATA_COMPRESS  INTF block has DATA_COMPRESS register
  * @DPU_INTF_MAX
  */
 enum {
@@ -188,6 +189,7 @@ enum {
DPU_INTF_TE,
DPU_DATA_HCTL_EN,
DPU_INTF_STATUS_SUPPORTED,
+   DPU_INTF_DATA_COMPRESS,
DPU_INTF_MAX
 };
 

-- 
2.40.1

[PATCH v6 2/6] drm/msm/dsi: Reduce pclk rate for compression

Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
is enabled.

Signed-off-by: Jessica Zhang 
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a448931af804..98ea1da492c7 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -561,12 +561,27 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
*msm_host)
clk_disable_unprepare(msm_host->byte_clk);
 }
 
-static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, 
bool is_bonded_dsi)
+static unsigned long dsi_adjust_pclk_for_compression(const struct 
drm_display_mode *mode,
+   const struct drm_dsc_config *dsc)
+{
+   int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * 
drm_dsc_get_bpp_int(dsc),
+   dsc->bits_per_component * 3);
+
+   int new_htotal = mode->htotal - mode->hdisplay + new_hdisplay;
+
+   return new_htotal * mode->vtotal * drm_mode_vrefresh(mode);
+}
+
+static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
+   const struct drm_dsc_config *dsc, bool is_bonded_dsi)
 {
unsigned long pclk_rate;
 
pclk_rate = mode->clock * 1000;
 
+   if (dsc)
+   pclk_rate = dsi_adjust_pclk_for_compression(mode, dsc);
+
/*
 * For bonded DSI mode, the current DRM mode has the complete width of 
the
 * panel. Since, the complete panel is driven by two DSI controllers,
@@ -585,7 +600,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host 
*host, bool is_bonded_d
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
u8 lanes = msm_host->lanes;
u32 bpp = dsi_get_bpp(msm_host->format);
-   unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
+   unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, 
is_bonded_dsi);
unsigned long pclk_bpp;
 
if (lanes == 0) {
@@ -604,7 +619,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host 
*host, bool is_bonded_d
 
 static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 {
-   msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
is_bonded_dsi);
+   msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
msm_host->dsc, is_bonded_dsi);
msm_host->byte_clk_rate = dsi_byte_clk_get_rate(_host->base, 
is_bonded_dsi,
msm_host->mode);
 

-- 
2.40.1

[PATCH v6 4/6] drm/msm/dpu: Set DATA_COMPRESS on command mode for DCE/DSC 1.2

Add a DPU INTF op to set the DCE_DATA_COMPRESS bit to enable the
DCE/DSC 1.2 datapath

Note: For now, this op is called for command mode encoders only. Changes to
set DATA_COMPRESS for video mode encoders will be posted along with DSC
v1.2 support for DP.

Signed-off-by: Jessica Zhang 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  3 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c  | 13 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h  |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 63ba0082b6ee..b856c6286c85 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -67,6 +67,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
phys_enc->hw_intf->ops.bind_pingpong_blk(
phys_enc->hw_intf,
phys_enc->hw_pp->idx);
+
+   if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
+   phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
 }
 
 static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
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 530f82e34c1e..5b0f6627e29b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -91,6 +91,7 @@
 
 #define INTF_CFG2_DATABUS_WIDENBIT(0)
 #define INTF_CFG2_DATA_HCTL_EN BIT(4)
+#define INTF_CFG2_DCE_DATA_COMPRESS BIT(12)
 
 
 static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
@@ -512,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct 
dpu_hw_intf *intf,
 
 }
 
+static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+{
+   u32 intf_cfg2 = DPU_REG_READ(>hw, INTF_CONFIG2);
+
+   intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+
+   DPU_REG_WRITE(>hw, INTF_CONFIG2, intf_cfg2);
+}
+
 static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
unsigned long cap)
 {
@@ -532,6 +542,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
ops->vsync_sel = dpu_hw_intf_vsync_sel;
ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
}
+
+   if (cap & BIT(DPU_INTF_DATA_COMPRESS))
+   ops->enable_compression = dpu_hw_intf_enable_compression;
 }
 
 struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 33895eca1211..99e21c4137f9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -70,6 +70,7 @@ struct intf_status {
  * @get_autorefresh:Retrieve autorefresh config from hardware
  *  Return: 0 on success, -ETIMEDOUT on timeout
  * @vsync_sel:  Select vsync signal for tear-effect 
configuration
+ * @enable_compression: Enable data compression
  */
 struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -106,6 +107,8 @@ struct dpu_hw_intf_ops {
 * Disable autorefresh if enabled
 */
void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t 
encoder_id, u16 vdisplay);
+
+   void (*enable_compression)(struct dpu_hw_intf *intf);
 };
 
 struct dpu_hw_intf {

-- 
2.40.1

[PATCH v6 5/6] drm/msm/dsi: Remove incorrect references to slice_count

Currently, slice_count is being used to calculate word count and
pkt_per_line. Instead, these values should be calculated using slice per
packet, which is not the same as slice_count.

Slice count represents the number of slices per interface, and its value
will not always match that of slice per packet. For example, it is possible
to have cases where there are multiple slices per interface but the panel
specifies only one slice per packet.

Thus, use the default value of one slice per packet and remove slice_count
from the aforementioned calculations.

Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Fixes: bc6b6ff8135c ("drm/msm/dsi: Use DSC slice(s) packet size to compute word 
count")
Reviewed-by: Marijn Suijten 
Signed-off-by: Jessica Zhang 
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 98ea1da492c7..fb1d3a25765f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -863,18 +863,17 @@ static void dsi_update_dsc_timing(struct msm_dsi_host 
*msm_host, bool is_cmd_mod
 */
slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
 
-   /*
-* If slice_count is greater than slice_per_intf
-* then default to 1. This can happen during partial
-* update.
-*/
-   if (dsc->slice_count > slice_per_intf)
-   dsc->slice_count = 1;
-
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
 
eol_byte_num = total_bytes_per_intf % 3;
-   pkt_per_line = slice_per_intf / dsc->slice_count;
+
+   /*
+* Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
+*
+* Since the current driver only supports slice_per_pkt = 1,
+* pkt_per_line will be equal to slice per intf for now.
+*/
+   pkt_per_line = slice_per_intf;
 
if (is_cmd_mode) /* packet data type */
reg = 
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -998,7 +997,14 @@ static void dsi_timing_setup(struct msm_dsi_host 
*msm_host, bool is_bonded_dsi)
if (!msm_host->dsc)
wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
else
-   wc = msm_host->dsc->slice_chunk_size * 
msm_host->dsc->slice_count + 1;
+   /*
+* When DSC is enabled, WC = slice_chunk_size * 
slice_per_pkt + 1.
+* Currently, the driver only supports default value of 
slice_per_pkt = 1
+*
+* TODO: Expand mipi_dsi_device struct to hold 
slice_per_pkt info
+*   and adjust DSC math to account for 
slice_per_pkt.
+*/
+   wc = msm_host->dsc->slice_chunk_size + 1;
 
dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |

-- 
2.40.1

[PATCH v6 1/6] msm/drm/dsi: Round up DSC hdisplay calculation

Currently, when compression is enabled, hdisplay is reduced via integer
division. This causes issues for modes where the original hdisplay is
not a multiple of 3.

To fix this, use DIV_ROUND_UP to divide hdisplay.

Suggested-by: Marijn Suijten 
Fixes: 08802f515c3cf ("drm/msm/dsi: Add support for DSC configuration")
Reviewed-by: Marijn Suijten 
Signed-off-by: Jessica Zhang 
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1a99d75025dc..a448931af804 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -949,7 +949,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, 
bool is_bonded_dsi)
 * pulse width same
 */
h_total -= hdisplay;
-   hdisplay = msm_dsc_get_bytes_per_line(msm_host->dsc) / 3;
+   hdisplay = 
DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
h_total += hdisplay;
ha_end = ha_start + hdisplay;
}

-- 
2.40.1

Re: [PATCH v8 10/18] drm/msm/a6xx: Introduce GMU wrapper support

2023-06-09 Thread Akhil P Oommen

On Mon, May 29, 2023 at 03:52:29PM +0200, Konrad Dybcio wrote:
> 
> Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
> but don't implement the associated GMUs. This is due to the fact that
> the GMU directly pokes at RPMh. Sadly, this means we have to take care
> of enabling & scaling power rails, clocks and bandwidth ourselves.
> 
> Reuse existing Adreno-common code and modify the deeply-GMU-infused
> A6XX code to facilitate these GPUs. This involves if-ing out lots
> of GMU callbacks and introducing a new type of GMU - GMU wrapper (it's
> the actual name that Qualcomm uses in their downstream kernels).
> 
> This is essentially a register region which is convenient to model
> as a device. We'll use it for managing the GDSCs. The register
> layout matches the actual GMU_CX/GX regions on the "real GMU" devices
> and lets us reuse quite a bit of gmu_read/write/rmw calls.
> 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c   |  72 +-
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 211 
> 
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h   |   1 +
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c |  14 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c |   8 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h |   6 +
>  6 files changed, 277 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 5ba8cba69383..385ca3a12462 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -1437,6 +1437,7 @@ static int a6xx_gmu_get_irq(struct a6xx_gmu *gmu, 
> struct platform_device *pdev,
>  
>  void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>  {
> + struct adreno_gpu *adreno_gpu = _gpu->base;
>   struct a6xx_gmu *gmu = _gpu->gmu;
>   struct platform_device *pdev = to_platform_device(gmu->dev);
>  
> @@ -1462,10 +1463,12 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>   gmu->mmio = NULL;
>   gmu->rscc = NULL;
>  
> - a6xx_gmu_memory_free(gmu);
> + if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> + a6xx_gmu_memory_free(gmu);
>  
> - free_irq(gmu->gmu_irq, gmu);
> - free_irq(gmu->hfi_irq, gmu);
> + free_irq(gmu->gmu_irq, gmu);
> + free_irq(gmu->hfi_irq, gmu);
> + }
>  
>   /* Drop reference taken in of_find_device_by_node */
>   put_device(gmu->dev);
> @@ -1484,6 +1487,69 @@ static int cxpd_notifier_cb(struct notifier_block *nb,
>   return 0;
>  }
>  
> +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node 
> *node)
> +{
> + struct platform_device *pdev = of_find_device_by_node(node);
> + struct a6xx_gmu *gmu = _gpu->gmu;
> + int ret;
> +
> + if (!pdev)
> + return -ENODEV;
> +
> + gmu->dev = >dev;
> +
> + of_dma_configure(gmu->dev, node, true);
> +
> + pm_runtime_enable(gmu->dev);
> +
> + /* Mark legacy for manual SPTPRAC control */
> + gmu->legacy = true;
> +
> + /* Map the GMU registers */
> + gmu->mmio = a6xx_gmu_get_mmio(pdev, "gmu");
> + if (IS_ERR(gmu->mmio)) {
> + ret = PTR_ERR(gmu->mmio);
> + goto err_mmio;
> + }
> +
> + gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
> + if (IS_ERR(gmu->cxpd)) {
> + ret = PTR_ERR(gmu->cxpd);
> + goto err_mmio;
> + }
> +
> + if (!device_link_add(gmu->dev, gmu->cxpd, DL_FLAG_PM_RUNTIME)) {
> + ret = -ENODEV;
> + goto detach_cxpd;
> + }
> +
> + init_completion(>pd_gate);
> + complete_all(>pd_gate);
> + gmu->pd_nb.notifier_call = cxpd_notifier_cb;
> +
> + /* Get a link to the GX power domain to reset the GPU */
> + gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
> + if (IS_ERR(gmu->gxpd)) {
> + ret = PTR_ERR(gmu->gxpd);
> + goto err_mmio;
> + }
> +
> + gmu->initialized = true;
> +
> + return 0;
> +
> +detach_cxpd:
> + dev_pm_domain_detach(gmu->cxpd, false);
> +
> +err_mmio:
> + iounmap(gmu->mmio);
> +
> + /* Drop reference taken in of_find_device_by_node */
> + put_device(gmu->dev);
> +
> + return ret;
> +}
> +
>  int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
>  {
>   struct adreno_gpu *adreno_gpu = _gpu->base;
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 58bf405b85d8..0a44762dbb6d 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -21,7 +21,7 @@ static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
>   struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>  
>   /* Check that the GMU is idle */
> - if (!a6xx_gmu_isidle(_gpu->gmu))
> + if (!adreno_has_gmu_wrapper(adreno_gpu) && 
> !a6xx_gmu_isidle(_gpu->gmu))
>   return false;
>  
>   /* Check tha

[PATCH] drm/i915/guc/slpc: Apply min softlimit correctly

2023-06-09 Thread Vinay Belgaumkar

We were skipping when min_softlimit was equal to RPn. We need to apply
it rergardless as efficient frequency will push the SLPC min to RPe.
This will break scenarios where user sets a min softlimit < RPe before
reset and then performs a GT reset.

Fixes: 95ccf312a1e4 ("drm/i915/guc/slpc: Allow SLPC to use efficient frequency")

Signed-off-by: Vinay Belgaumkar 
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
index 01b75529311c..ee9f83af7cf6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
@@ -606,7 +606,7 @@ static int slpc_set_softlimits(struct intel_guc_slpc *slpc)
if (unlikely(ret))
return ret;
slpc_to_gt(slpc)->defaults.min_freq = slpc->min_freq_softlimit;
-   } else if (slpc->min_freq_softlimit != slpc->min_freq) {
+   } else {
return intel_guc_slpc_set_min_freq(slpc,
   slpc->min_freq_softlimit);
}
-- 
2.38.1

Re: [PATCH] drm/amd/amdgpu: enable W=1 for amdgpu

2023-06-09 Thread Nathan Chancellor

+ Masahiro and linux-kbuild

On Fri, Jun 09, 2023 at 12:42:06PM -0400, Hamza Mahfooz wrote:
> We have a clean build with W=1 as of
> commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move
> SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable
> these checks unconditionally for the entire module to catch these errors
> during development.
> 
> Cc: Alex Deucher 
> Cc: Nathan Chancellor 
> Signed-off-by: Hamza Mahfooz 

I think this is fine, especially since it will help catch issues in
amdgpu quickly and hopefully encourage developers to fix their problems
before they make it to a tree with wider impact lika -next.

However, this is now the third place that W=1 has been effectively
enabled (i915 and btrfs are the other two I know of) and it would be
nice if this was a little more unified, especially since it is not
uncommon for the warnings under W=1 to shift around and keeping them
unified will make maintainence over the longer term a little easier. I
am not sure if this has been brought up in the past and I don't want to
hold up this change but I suspect this sentiment of wanting to enable
W=1 on a per-subsystem basis is going to continue to grow.

Regardless, for clang 11.1.0 to 16.0.5, I see no warnings when building
drivers/gpu/drm/amd/amdgpu/ with Arch Linux's configuration or
allmodconfig.

Reviewed-by: Nathan Chancellor 
Tested-by: Nathan Chancellor 

> ---
>  drivers/gpu/drm/amd/amdgpu/Makefile | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 86b833085f19..8d16f280b695 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
>   -I$(FULL_AMD_PATH)/amdkfd
>  
>  subdir-ccflags-y := -Wextra
> -subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
> +subdir-ccflags-y += -Wunused
> +subdir-ccflags-y += -Wmissing-prototypes
> +subdir-ccflags-y += -Wmissing-declarations
> +subdir-ccflags-y += -Wmissing-include-dirs
> +subdir-ccflags-y += -Wold-style-definition
> +subdir-ccflags-y += -Wmissing-format-attribute
> +# Need this to avoid recursive variable evaluation issues
> +cond-flags := $(call cc-option, -Wunused-but-set-variable) \
> + $(call cc-option, -Wunused-const-variable) \
> + $(call cc-option, -Wstringop-truncation) \
> + $(call cc-option, -Wpacked-not-aligned)
> +subdir-ccflags-y += $(cond-flags)
>  subdir-ccflags-y += -Wno-unused-parameter
>  subdir-ccflags-y += -Wno-type-limits
>  subdir-ccflags-y += -Wno-sign-compare
> -- 
> 2.40.1
>

Re: [PATCH] drm: etnaviv: Replace of_platform.h with explicit includes

2023-06-09 Thread Rob Herring

On Mon, Apr 10, 2023 at 5:26 PM Rob Herring  wrote:
>
> Etnaviv doesn't use anything from of_platform.h, but depends on
> of.h, of_device.h, and platform_device.h which are all implicitly
> included, but that is going to be removed soon.
>
> Signed-off-by: Rob Herring 
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Ping!

>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 44ca803237a5..c68e83ed5a23 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -6,7 +6,9 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
> +#include 
> +#include 
>  #include 
>
>  #include 
> --
> 2.39.2
>

Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices

2023-06-09 Thread Bjorn Helgaas

On Sat, Jun 10, 2023 at 02:07:58AM +0800, Sui Jingfeng wrote:
> On 2023/6/10 01:52, Bjorn Helgaas wrote:
> > On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote:
> > > On 2023/6/9 01:32, Bjorn Helgaas wrote:
> > > > On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote:
> > > > > From: Sui Jingfeng 
> > > > > 
> > > > > This patch adds PCI driver support on top of what we already have. 
> > > > > Take
> > > > > the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI 
> > > > > device
> > > > > driver. There is only one GPU core for the GC1000 in the LS7A1000 and
> > > > > LS2K1000. Therefore, component frameworks can be avoided.
> > > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
> > > > > 0},
> > > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 
> > > > > 0},
> > > > PCI_VDEVICE()
> > > This make it impossible to hook device-specific data in the future.
> > > 
> > > But currently there no device specific data associated with the
> > > 0x7a05 and 0x7a15,
> > > 
> > > so it's acceptable for now. Thanks.
> > Haha, ISTR having this conversation before, sorry for repeating it.
> > 
> > Indeed, it's fine as-is.  But PCI_VDEVICE() actually *does* allow for
> > vendor-specific data because it doesn't include the data element,
> > which defaults to zero if you don't specify it.
> > 
> > So for example, drivers/net/ethernet/realtek/r8169_main.c has this:
> > 
> >{ PCI_VDEVICE(REALTEK, 0x8129) },
> >{ PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT },
> > 
> > where 0x8129 has no driver_data (it defaults to zero), but 0x8136
> > does.
> 
> PCI_VDEVICE macro end with two zero. (I thought it was three)

No worries, I thought the same thing the first five times I read it :)

Re: [PATCH] drm/radeon: Disable outputs when releasing fbdev client

Applied.  Thanks!

Alex

On Fri, Jun 9, 2023 at 1:40 PM Borislav Petkov  wrote:
>
> On Fri, Jun 09, 2023 at 04:03:56PM +0200, Thomas Zimmermann wrote:
> > Disable the modesetting pipeline before release the radeon's fbdev
> > client. Fixes the following error:
> >
> > [   17.217408] WARNING: CPU: 5 PID: 1464 at 
> > drivers/gpu/drm/ttm/ttm_bo.c:326 ttm_bo_release+0x27e/0x2d0 [ttm]
> > [   17.217418] Modules linked in: edac_mce_amd radeon(+) drm_ttm_helper ttm 
> > video drm_suballoc_helper drm_display_helper kvm irqbypass drm_kms_helper 
> > syscopyarea crc32_pclmul sysfillrect sha512_ssse3 sysimgblt sha512_generic 
> > cfbfillrect cfbimgblt wmi_bmof aesni_intel cfbcopyarea crypto_simd cryptd 
> > k10temp acpi_cpufreq wmi dm_mod
> > [   17.217432] CPU: 5 PID: 1464 Comm: systemd-udevd Not tainted 6.4.0-rc4+ 
> > #1
> > [   17.217436] Hardware name: Micro-Star International Co., Ltd. 
> > MS-7A38/B450M PRO-VDH MAX (MS-7A38), BIOS B.G0 07/26/2022
> > [   17.217438] RIP: 0010:ttm_bo_release+0x27e/0x2d0 [ttm]
> > [   17.217444] Code: 48 89 43 38 48 89 43 40 48 8b 5c 24 30 48 8b b5 40 08 
> > 00 00 48 8b 6c 24 38 48 83 c4 58 e9 7a 49 f7 e0 48 89 ef e9 6c fe ff ff 
> > <0f> 0b 48 83 7b 20 00 0f 84 b7 fd ff ff 0f 0b 0f 1f 00 e9 ad fd ff
> > [   17.217448] RSP: 0018:c995fbb0 EFLAGS: 00010202
> > [   17.217451] RAX: 0001 RBX: 8881052c8de0 RCX: 
> > 
> > [   17.217453] RDX: 0001 RSI:  RDI: 
> > 8881052c8de0
> > [   17.217455] RBP: 888104a66e00 R08: 8881052c8de0 R09: 
> > 888104a7cf08
> > [   17.217457] R10: c995fbe0 R11: c995fbe8 R12: 
> > 8881052c8c78
> > [   17.217458] R13: 8881052c8c78 R14: dead0100 R15: 
> > 88810528b108
> > [   17.217460] FS:  7f319fcbb8c0() GS:1a54() 
> > knlGS:
> > [   17.217463] CS:  0010 DS:  ES:  CR0: 80050033
> > [   17.217464] CR2: 55dc8b0224a0 CR3: 00010373d000 CR4: 
> > 00750ee0
> > [   17.217466] PKRU: 5554
> > [   17.217468] Call Trace:
> > [   17.217470]  
> > [   17.217472]  ? __warn+0x97/0x160
> > [   17.217476]  ? ttm_bo_release+0x27e/0x2d0 [ttm]
> > [   17.217481]  ? report_bug+0x1ec/0x200
> > [   17.217487]  ? handle_bug+0x3c/0x70
> > [   17.217490]  ? exc_invalid_op+0x1f/0x90
> > [   17.217493]  ? preempt_count_sub+0xb5/0x100
> > [   17.217496]  ? asm_exc_invalid_op+0x16/0x20
> > [   17.217500]  ? ttm_bo_release+0x27e/0x2d0 [ttm]
> > [   17.217505]  ? ttm_resource_move_to_lru_tail+0x1ab/0x1d0 [ttm]
> > [   17.217511]  radeon_bo_unref+0x1a/0x30 [radeon]
> > [   17.217547]  radeon_gem_object_free+0x20/0x30 [radeon]
> > [   17.217579]  radeon_fbdev_fb_destroy+0x57/0x90 [radeon]
> > [   17.217616]  unregister_framebuffer+0x72/0x110
> > [   17.217620]  drm_client_dev_unregister+0x6d/0xe0
> > [   17.217623]  drm_dev_unregister+0x2e/0x90
> > [   17.217626]  drm_put_dev+0x26/0x90
> > [   17.217628]  pci_device_remove+0x44/0xc0
> > [   17.217631]  really_probe+0x257/0x340
> > [   17.217635]  __driver_probe_device+0x73/0x120
> > [   17.217638]  driver_probe_device+0x2c/0xb0
> > [   17.217641]  __driver_attach+0xa0/0x150
> > [   17.217643]  ? __pfx___driver_attach+0x10/0x10
> > [   17.217646]  bus_for_each_dev+0x67/0xa0
> > [   17.217649]  bus_add_driver+0x10e/0x210
> > [   17.217651]  driver_register+0x5c/0x120
> > [   17.217653]  ? __pfx_radeon_module_init+0x10/0x10 [radeon]
> > [   17.217681]  do_one_initcall+0x44/0x220
> > [   17.217684]  ? kmalloc_trace+0x37/0xc0
> > [   17.217688]  do_init_module+0x64/0x240
> > [   17.217691]  __do_sys_finit_module+0xb2/0x100
> > [   17.217694]  do_syscall_64+0x3b/0x90
> > [   17.217697]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> > [   17.217700] RIP: 0033:0x7f319feaa5a9
> > [   17.217702] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 
> > 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 
> > <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 27 08 0d 00 f7 d8 64 89 01 48
> > [   17.217706] RSP: 002b:7ffc6bf3e7f8 EFLAGS: 0246 ORIG_RAX: 
> > 0139
> > [   17.217709] RAX: ffda RBX: 5607204f3170 RCX: 
> > 7f319feaa5a9
> > [   17.217710] RDX:  RSI: 7f31a002eefd RDI: 
> > 0018
> > [   17.217712] RBP: 7f31a002eefd R08:  R09: 
> > 5607204f1860
> > [   17.217714] R10: 0018 R11: 0246 R12: 
> > 0002
> > [   17.217716] R13:  R14: 560720522450 R15: 
> > 560720255899
> > [   17.217718]  
> > [   17.217719] ---[ end trace  ]---
> >
> > The buffer object backing the fbdev emulation got pinned twice: by the
> > fb_probe helper radeon_fbdev_create_pinned_object() and the modesetting
> > code when the framebuffer got displayed. It only got unpinned once by
> > the fbdev helper radeon_fbdev_destroy_pinned_object(). Hence TTM's BO-
> > release function complains about the pin

Re: [RESEND 15/15] drm/amd/amdgpu/sdma_v6_0: Demote a bunch of half-completed function headers

These have already been fixed up.

Thanks!

Alex

On Fri, Jun 9, 2023 at 4:18 AM Lee Jones  wrote:
>
> Fixes the following W=1 kernel build warning(s):
>
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:248: warning: Function parameter or 
> member 'job' not described in 'sdma_v6_0_ring_emit_ib'
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:248: warning: Function parameter or 
> member 'flags' not described in 'sdma_v6_0_ring_emit_ib'
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:945: warning: Function parameter or 
> member 'timeout' not described in 'sdma_v6_0_ring_test_ib'
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:1124: warning: Function parameter or 
> member 'ring' not described in 'sdma_v6_0_ring_pad_ib'
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:1175: warning: Function parameter or 
> member 'vmid' not described in 'sdma_v6_0_ring_emit_vm_flush'
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c:1175: warning: Function parameter or 
> member 'pd_addr' not described in 'sdma_v6_0_ring_emit_vm_flush'
>
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "Pan, Xinhui" 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Sumit Semwal 
> Cc: Stanley Yang 
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-me...@vger.kernel.org
> Cc: linaro-mm-...@lists.linaro.org
> Signed-off-by: Lee Jones 
> ---
>  drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c 
> b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> index 3b03dda854fdc..8cd7abe74e6c4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> @@ -233,7 +233,7 @@ static void sdma_v6_0_ring_insert_nop(struct amdgpu_ring 
> *ring, uint32_t count)
> amdgpu_ring_write(ring, ring->funcs->nop);
>  }
>
> -/**
> +/*
>   * sdma_v6_0_ring_emit_ib - Schedule an IB on the DMA engine
>   *
>   * @ring: amdgpu ring pointer
> @@ -936,7 +936,7 @@ static int sdma_v6_0_ring_test_ring(struct amdgpu_ring 
> *ring)
> return r;
>  }
>
> -/**
> +/*
>   * sdma_v6_0_ring_test_ib - test an IB on the DMA engine
>   *
>   * @ring: amdgpu_ring structure holding ring information
> @@ -1118,7 +1118,7 @@ static void sdma_v6_0_vm_set_pte_pde(struct amdgpu_ib 
> *ib,
> ib->ptr[ib->length_dw++] = count - 1; /* number of entries */
>  }
>
> -/**
> +/*
>   * sdma_v6_0_ring_pad_ib - pad the IB
>   * @ib: indirect buffer to fill with padding
>   * @ring: amdgpu ring pointer
> @@ -1167,7 +1167,7 @@ static void sdma_v6_0_ring_emit_pipeline_sync(struct 
> amdgpu_ring *ring)
>   SDMA_PKT_POLL_REGMEM_DW5_INTERVAL(4)); /* retry 
> count, poll interval */
>  }
>
> -/**
> +/*
>   * sdma_v6_0_ring_emit_vm_flush - vm flush using sDMA
>   *
>   * @ring: amdgpu_ring pointer
> --
> 2.41.0.162.gfafddb0af9-goog
>

Re: [RESEND 14/15] drm/radeon/radeon_ttm: Remove unused variable 'rbo' from radeon_bo_move()

This patch is no longer applicable.

Alex

On Fri, Jun 9, 2023 at 4:18 AM Lee Jones  wrote:
>
> Fixes the following W=1 kernel build warning(s):
>
>  drivers/gpu/drm/radeon/radeon_ttm.c: In function ‘radeon_bo_move’:
>  drivers/gpu/drm/radeon/radeon_ttm.c:201:27: warning: variable ‘rbo’ set but 
> not used [-Wunused-but-set-variable]
>
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "Pan, Xinhui" 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Jerome Glisse 
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Lee Jones 
> ---
>  drivers/gpu/drm/radeon/radeon_ttm.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c 
> b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 4eb83ccc4906a..de4e6d78f1e12 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -197,7 +197,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
> bool evict,
>  {
> struct ttm_resource *old_mem = bo->resource;
> struct radeon_device *rdev;
> -   struct radeon_bo *rbo;
> int r;
>
> if (new_mem->mem_type == TTM_PL_TT) {
> @@ -210,7 +209,6 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, 
> bool evict,
> if (r)
> return r;
>
> -   rbo = container_of(bo, struct radeon_bo, tbo);
> rdev = radeon_get_rdev(bo->bdev);
> if (!old_mem || (old_mem->mem_type == TTM_PL_SYSTEM &&
>  bo->ttm == NULL)) {
> --
> 2.41.0.162.gfafddb0af9-goog
>

Re: [PATCH v8 09/18] drm/msm/a6xx: Extend and explain UBWC config

2023-06-09 Thread Akhil P Oommen

On Mon, May 29, 2023 at 03:52:28PM +0200, Konrad Dybcio wrote:
> 
> Rename lower_bit to hbb_lo and explain what it signifies.
> Add explanations (wherever possible to other tunables).
> 
> Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
> 
> Reviewed-by: Rob Clark 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 39 
> +++
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index dfde5fb65eed..58bf405b85d8 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -786,10 +786,25 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>  {
>   struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> - u32 lower_bit = 2;
> - u32 amsbc = 0;
> + /* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */
>   u32 rgb565_predicator = 0;
> + /* Unknown, introduced with A650 family */
>   u32 uavflagprd_inv = 0;
> + /* Whether the minimum access length is 64 bits */
> + u32 min_acc_len = 0;
> + /* Entirely magic, per-GPU-gen value */
> + u32 ubwc_mode = 0;
> + /*
> +  * The Highest Bank Bit value represents the bit of the highest DDR 
> bank.
> +  * We then subtract 13 from it (13 is the minimum value allowed by hw) 
> and
> +  * write the lowest two bits of the remaining value as hbb_lo and the
> +  * one above it as hbb_hi to the hardware. This should ideally use DRAM
> +  * type detection.
> +  */
> + u32 hbb_hi = 0;
> + u32 hbb_lo = 2;
> + /* Unknown, introduced with A640/680 */
> + u32 amsbc = 0;
>  
>   /* a618 is using the hw default values */
>   if (adreno_is_a618(adreno_gpu))
> @@ -800,25 +815,31 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>  
>   if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>   /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
> - lower_bit = 3;
> + hbb_lo = 3;
>   amsbc = 1;
>   rgb565_predicator = 1;
>   uavflagprd_inv = 2;
>   }
>  
>   if (adreno_is_7c3(adreno_gpu)) {
> - lower_bit = 1;
> + hbb_lo = 1;
>   amsbc = 1;
>   rgb565_predicator = 1;
>   uavflagprd_inv = 2;
>   }
>  
>   gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
> - rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
> - gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
> - gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
> - uavflagprd_inv << 4 | lower_bit << 1);
> - gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
> +   rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 |
> +   min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
> +
> + gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 |
> +   min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
> +
> + gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 |
> +   uavflagprd_inv << 4 | min_acc_len << 3 |
> +   hbb_lo << 1 | ubwc_mode);
> +
> + gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << 
> 21);
>  }
>  
>  static int a6xx_cp_init(struct msm_gpu *gpu)
> 

Reviewed-by: Akhil P Oommen 

-Akhil
> -- 
> 2.40.1
>

Re: [PATCH v8 08/18] drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init

2023-06-09 Thread Konrad Dybcio




On 9.06.2023 20:25, Akhil P Oommen wrote:
> On Mon, May 29, 2023 at 03:52:27PM +0200, Konrad Dybcio wrote:
>>
>> Currently we're only deasserting REG_A6XX_RBBM_GBIF_HALT, but we also
>> need REG_A6XX_GBIF_HALT to be set to 0.
>>
>> This is typically done automatically on successful GX collapse, but in
>> case that fails, we should take care of it.
>>
>> Also, add a memory barrier to ensure it's gone through before jumping
>> to further initialization.
>>
>> Reviewed-by: Dmitry Baryshkov 
>> Signed-off-by: Konrad Dybcio 
>> ---
>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
>> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index 083ccb5bcb4e..dfde5fb65eed 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -1003,8 +1003,12 @@ static int hw_init(struct msm_gpu *gpu)
>>  a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_GPU_SET);
>>  
>>  /* Clear GBIF halt in case GX domain was not collapsed */
>> -if (a6xx_has_gbif(adreno_gpu))
>> +if (a6xx_has_gbif(adreno_gpu)) {
>> +gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
>>  gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
>> +/* Let's make extra sure that the GPU can access the memory.. */
>> +mb();
> This barrier is unnecessary because writel transactions are ordered and
> we don't expect a traffic from GPU immediately after this.
> 
> -Akhil
Right, let's remove it!

Konrad
>> +}
>>  
>>  gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0);
>>  
>>
>> -- 
>> 2.40.1
>>

Re: [PATCH v8 08/18] drm/msm/a6xx: Remove both GBIF and RBBM GBIF halt on hw init

2023-06-09 Thread Akhil P Oommen

On Mon, May 29, 2023 at 03:52:27PM +0200, Konrad Dybcio wrote:
> 
> Currently we're only deasserting REG_A6XX_RBBM_GBIF_HALT, but we also
> need REG_A6XX_GBIF_HALT to be set to 0.
> 
> This is typically done automatically on successful GX collapse, but in
> case that fails, we should take care of it.
> 
> Also, add a memory barrier to ensure it's gone through before jumping
> to further initialization.
> 
> Reviewed-by: Dmitry Baryshkov 
> Signed-off-by: Konrad Dybcio 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 083ccb5bcb4e..dfde5fb65eed 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1003,8 +1003,12 @@ static int hw_init(struct msm_gpu *gpu)
>   a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_GPU_SET);
>  
>   /* Clear GBIF halt in case GX domain was not collapsed */
> - if (a6xx_has_gbif(adreno_gpu))
> + if (a6xx_has_gbif(adreno_gpu)) {
> + gpu_write(gpu, REG_A6XX_GBIF_HALT, 0);
>   gpu_write(gpu, REG_A6XX_RBBM_GBIF_HALT, 0);
> + /* Let's make extra sure that the GPU can access the memory.. */
> + mb();
This barrier is unnecessary because writel transactions are ordered and
we don't expect a traffic from GPU immediately after this.

-Akhil
> + }
>  
>   gpu_write(gpu, REG_A6XX_RBBM_SECVID_TSB_CNTL, 0);
>  
> 
> -- 
> 2.40.1
>

Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices


Hi,

On 2023/6/10 01:52, Bjorn Helgaas wrote:

On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote:

On 2023/6/9 01:32, Bjorn Helgaas wrote:

On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote:

From: Sui Jingfeng 

This patch adds PCI driver support on top of what we already have. Take
the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device
driver. There is only one GPU core for the GC1000 in the LS7A1000 and
LS2K1000. Therefore, component frameworks can be avoided.
+   {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
+   {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},

PCI_VDEVICE()

This make it impossible to hook device-specific data in the future.

But currently there no device specific data associated with the
0x7a05 and 0x7a15,

so it's acceptable for now. Thanks.

Haha, ISTR having this conversation before, sorry for repeating it.

Indeed, it's fine as-is.  But PCI_VDEVICE() actually *does* allow for
vendor-specific data because it doesn't include the data element,
which defaults to zero if you don't specify it.

So for example, drivers/net/ethernet/realtek/r8169_main.c has this:

   { PCI_VDEVICE(REALTEK, 0x8129) },
   { PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT },

where 0x8129 has no driver_data (it defaults to zero), but 0x8136
does.


Yeah, I'm wrong.

PCI_VDEVICE macro end with two zero. (I thought it was three)

Thanks for the education.

With those lessons learned, I somewhat know how to create patch.

It should meet community's requirement before sending.

I'm too naive in the before.

Thanks a lot, really.


Bjorn


--
Jingfeng

[PATCH v2] drm/sched: Make sure we wait for all dependencies in kill_jobs_cb()

2023-06-09 Thread Boris Brezillon

drm_sched_entity_kill_jobs_cb() logic is omitting the last fence popped
from the dependency array that was waited upon before
drm_sched_entity_kill() was called (drm_sched_entity::dependency field),
so we're basically waiting for all dependencies except one.

In theory, this wait shouldn't be needed because resources should have
their users registered to the dma_resv object, thus guaranteeing that
future jobs wanting to access these resources wait on all the previous
users (depending on the access type, of course). But we want to keep
these explicit waits in the kill entity path just in case.

Let's make sure we keep all dependencies in the array in
drm_sched_job_dependency(), so we can iterate over the array and wait
in drm_sched_entity_kill_jobs_cb().

Signed-off-by: Boris Brezillon 
Suggested-by: "Christian König" 
Cc: Frank Binns 
Cc: Sarah Walker 
Cc: Donald Robson 
Cc: Luben Tuikov 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Sumit Semwal 
Cc: "Christian König" 
---
 drivers/gpu/drm/scheduler/sched_entity.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index 68e807ae136a..e1b437e66f3c 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -176,13 +176,14 @@ static void drm_sched_entity_kill_jobs_cb(struct 
dma_fence *f,
 {
struct drm_sched_job *job = container_of(cb, struct drm_sched_job,
 finish_cb);
+   unsigned long index;
int r;
 
dma_fence_put(f);
 
/* Wait for all dependencies to avoid data corruptions */
-   while (!xa_empty(>dependencies)) {
-   f = xa_erase(>dependencies, job->last_dependency++);
+   xa_for_each(>dependencies, index, f) {
+   xa_erase(>dependencies, index);
r = dma_fence_add_callback(f, >finish_cb,
   drm_sched_entity_kill_jobs_cb);
if (!r)
@@ -415,8 +416,17 @@ static struct dma_fence *
 drm_sched_job_dependency(struct drm_sched_job *job,
 struct drm_sched_entity *entity)
 {
-   if (!xa_empty(>dependencies))
-   return xa_erase(>dependencies, job->last_dependency++);
+   struct dma_fence *f;
+
+   /* We keep the fence around, so we can iterate over all dependencies
+* in drm_sched_entity_kill_jobs_cb() to make all deps are signaled
+* before killing the job.
+*/
+   f = xa_load(>dependencies, job->last_dependency);
+   if (f) {
+   job->last_dependency++;
+   return dma_fence_get(f);
+   }
 
if (job->sched->ops->prepare_job)
return job->sched->ops->prepare_job(job, entity);
-- 
2.40.1

Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices

2023-06-09 Thread Bjorn Helgaas

On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote:
> On 2023/6/9 01:32, Bjorn Helgaas wrote:
> > On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote:
> > > From: Sui Jingfeng 
> > > 
> > > This patch adds PCI driver support on top of what we already have. Take
> > > the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device
> > > driver. There is only one GPU core for the GC1000 in the LS7A1000 and
> > > LS2K1000. Therefore, component frameworks can be avoided.

> > > + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> > > + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> >
> > PCI_VDEVICE()
> 
> This make it impossible to hook device-specific data in the future.
> 
> But currently there no device specific data associated with the
> 0x7a05 and 0x7a15,
> 
> so it's acceptable for now. Thanks.

Haha, ISTR having this conversation before, sorry for repeating it.

Indeed, it's fine as-is.  But PCI_VDEVICE() actually *does* allow for
vendor-specific data because it doesn't include the data element,
which defaults to zero if you don't specify it.

So for example, drivers/net/ethernet/realtek/r8169_main.c has this:

  { PCI_VDEVICE(REALTEK, 0x8129) },
  { PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT },

where 0x8129 has no driver_data (it defaults to zero), but 0x8136
does.

Bjorn

[pull] amdgpu, amdkfd, radeon, ttm, drm, UAPI drm-next-6.5

Hi Dave, Daniel,

New stuff for 6.5.  Includes last week's PR with fixed sob tag, plus a few
new things.

The following changes since commit e82c98f2ca439356d5595ba8c9cd782f993f6f8c:

  Merge tag 'amd-drm-next-6.4-2023-04-14' of 
https://gitlab.freedesktop.org/agd5f/linux into drm-next (2023-04-17 10:54:59 
+1000)

are available in the Git repository at:

  https://gitlab.freedesktop.org/agd5f/linux.git 
tags/amd-drm-next-6.5-2023-06-09

for you to fetch changes up to 3b718dcaf163d17fe907ea098c8449e0cd6bc271:

  drm/amd/display: Filter out AC mode frequencies on DC mode systems 
(2023-06-09 12:50:55 -0400)


amd-drm-next-6.5-2023-06-02:

amdgpu:
- SR-IOV fixes
- Warning fixes
- Misc code cleanups and spelling fixes
- DCN 3.2 updates
- Improved DC FAMS support for better power management
- Improved DC SubVP support for better power management
- DCN 3.1.x fixes
- Max IB size query
- DC GPU reset fixes
- RAS updates
- DCN 3.0.x fixes
- S/G display fixes
- CP shadow buffer support
- Implement connector force callback
- Z8 power improvements
- PSP 13.0.10 vbflash support
- Mode2 reset fixes
- Store MQDs in VRAM to improve queue switch latency
- VCN 3.x fixes
- JPEG 3.x fixes
- Enable DC_FP on LoongArch
- GFXOFF fixes
- GC 9.4.3 partition support
- SDMA 4.4.2 partition support
- VCN/JPEG 4.0.3 partition support
- VCN 4.0.3 updates
- NBIO 7.9 updates
- GC 9.4.3 updates
- Take NUMA into account when allocating memory
- Handle NUMA for partitions
- SMU 13.0.6 updates
- GC 9.4.3 RAS updates
- Stop including unused swiotlb.h
- SMU 13.0.7 fixes
- Fix clock output ordering on some APUs
- Clean up DC FPGA code
- GFX9 preemption fixes
- Misc irq fixes
- S0ix fixes
- Add new DRM_AMDGPU_WERROR config parameter to help with CI
- PCIe fix for RDNA2
- kdoc fixes
- Documentation updates

amdkfd:
- Query TTM mem limit rather than hardcoding it
- GC 9.4.3 partition support
- Handle NUMA for partitions

radeon:
- Fix possible double free
- Stop including unused swiotlb.h
- Fix possible division by zero

ttm:
- Add query for TTM mem limit
- Add NUMA awareness to pools
- Export ttm_pool_fini()

UAPI:
- Add new ctx query flag to better handle GPU resets
  Mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22290
- Add new interface to query and set shadow buffer for RDNA3
  Mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21986
- Add new INFO query for max IB size
  Proposed userspace: 
https://gitlab.freedesktop.org/bnieuwenhuizen/mesa/-/commits/ib-rejection-v3

amd-drm-next-6.5-2023-06-09:

amdgpu:
- S0ix fixes
- Initial SMU13 Overdrive support
- kdoc fixes
- Misc clode cleanups
- Flexible array fixes
- Display OTG fixes
- SMU 13.0.6 updates
- Revert some broken clock counter updates
- Misc display fixes
- GFX9 preemption fixes
- Add support for newer EEPROM bad page table format
- Add missing radeon secondary id
- Add support for new colorspace KMS API
- CSA fix
- Stable pstate fixes for APUs
- make vbl interface admin only
- Handle PCI accelerator class

amdkfd:
- Add debugger support for gdb

radeon:
- Fix possible UAF

drm:
- Add Colorspace functionality

UAPI:
- Add debugger interface for enabling gdb
  Proposed userspace: 
https://github.com/ROCm-Developer-Tools/ROCdbgapi/tree/wip-dbgapi
- Add KMS colorspace API
  Discussion: 
https://lists.freedesktop.org/archives/dri-devel/2023-June/408128.html


Alan Liu (4):
  drm/amd/display: Fix in disabling secure display
  drm/amdgpu: Fix desktop freezed after gpu-reset
  drm/amd/display: Fix in secure display context creation
  drm/amd/display: Fix warning in disabling vblank irq

Alex Deucher (37):
  drm/amdgpu/gfx11: add FW version check for new CP GFX shadow feature
  drm/amdgpu/gfx11: check the CP FW version CP GFX shadow support
  drm/amdgpu/UAPI: add new CS chunk for GFX shadow buffers
  drm/amdgpu: don't require a job for cond_exec and shadow
  drm/amdgpu: add UAPI to query GFX shadow sizes
  drm/amdgpu: add gfx shadow callback
  drm/amdgpu: add get_gfx_shadow_info callback for gfx11
  drm/amdgpu: add support for new GFX shadow size query
  drm/amdgpu: bump driver version number for CP GFX shadow
  drm/amdgpu: track MQD size for gfx and compute
  drm/amdgpu: add debugfs interface for reading MQDs
  drm/amdgpu/gfx11: update gpu_clock_counter logic
  drm/amdgpu/gfx11: drop old bring up code
  drm/amdgpu/gfx10: drop old bring up code
  drm/amdgpu: add [en/dis]able_kgq() functions
  drm/amdgpu/gfx10: use generic [en/dis]able_kgq() helpers
  drm/amdgpu/gfx11: use generic [en/dis]able_kgq() helpers
  drm/amdgpu/gfx10: drop unused variable
  drm/amdgpu/gfx11: drop unused variable
  drm/amdgpu/gfx8: always restore kcq MQDs
  drm/amdgpu/gfx9: always restore kcq MQDs
  drm/amdgpu/gfx10: always restore kcq/kgq MQDs

Re: [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register




On 2023/6/10 00:48, Bjorn Helgaas wrote:

On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote:

On 2023/6/9 03:19, Bjorn Helgaas wrote:

On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:

From: Sui Jingfeng 

The vga_is_firmware_default() function is arch-dependent, which doesn't
sound right. At least, it also works on the Mips and LoongArch platforms.
Tested with the drm/amdgpu and drm/radeon drivers. However, it's difficult
to enumerate all arch-driver combinations. I'm wrong if there is only one
exception.

With the observation that device drivers typically have better knowledge
about which PCI bar contains the firmware framebuffer, which could avoid
the need to iterate all of the PCI BARs.

But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is
probably not suitable to make such an optimization for a specific device.

There are PCI display controllers that don't have a dedicated VRAM bar,
this function will lose its effectiveness in such a case. Luckily, the
device driver can provide an accurate workaround.

Therefore, this patch introduces a callback that allows the device driver
to tell the VGAARB if the device is the default boot device. This patch
only intends to introduce the mechanism, while the implementation is left
to the device driver authors. Also honor the comment: "Clients have two
callback mechanisms they can use"

s/bar/BAR/ (several)

Nothing here uses the callback.  I don't want to merge this until we
have a user.

This is chicken and egg question.

If you could help get this merge first, I will show you the first user.


I'm not sure why the device driver should know whether its device is
the default boot device.

It's not that the device driver should know,

but it's about that the device driver has the right to override.

Device driver may have better approach to identify the default boot
device.

The way we usually handle this is to include the new callback in the
same series as the first user of it.  That has two benefits:
(1) everybody can review the whole picture and possibly suggest
different approaches, and (2) when we merge the infrastructure,
we also merge a user of it at the same time, so the whole thing can be
tested and we don't end up with unused code.


OK, acceptable

I will try to prepare the user code of this callback and respin the patch.

I may resend it with another patch set in the future, this series 
already drop it,


see v5[1]

[1] https://patchwork.freedesktop.org/series/119134/


Bjorn

Re: [PATCH] drm/radeon: Disable outputs when releasing fbdev client

2023-06-09 Thread Borislav Petkov

On Fri, Jun 09, 2023 at 04:03:56PM +0200, Thomas Zimmermann wrote:
> Disable the modesetting pipeline before release the radeon's fbdev
> client. Fixes the following error:
> 
> [   17.217408] WARNING: CPU: 5 PID: 1464 at drivers/gpu/drm/ttm/ttm_bo.c:326 
> ttm_bo_release+0x27e/0x2d0 [ttm]
> [   17.217418] Modules linked in: edac_mce_amd radeon(+) drm_ttm_helper ttm 
> video drm_suballoc_helper drm_display_helper kvm irqbypass drm_kms_helper 
> syscopyarea crc32_pclmul sysfillrect sha512_ssse3 sysimgblt sha512_generic 
> cfbfillrect cfbimgblt wmi_bmof aesni_intel cfbcopyarea crypto_simd cryptd 
> k10temp acpi_cpufreq wmi dm_mod
> [   17.217432] CPU: 5 PID: 1464 Comm: systemd-udevd Not tainted 6.4.0-rc4+ #1
> [   17.217436] Hardware name: Micro-Star International Co., Ltd. 
> MS-7A38/B450M PRO-VDH MAX (MS-7A38), BIOS B.G0 07/26/2022
> [   17.217438] RIP: 0010:ttm_bo_release+0x27e/0x2d0 [ttm]
> [   17.217444] Code: 48 89 43 38 48 89 43 40 48 8b 5c 24 30 48 8b b5 40 08 00 
> 00 48 8b 6c 24 38 48 83 c4 58 e9 7a 49 f7 e0 48 89 ef e9 6c fe ff ff <0f> 0b 
> 48 83 7b 20 00 0f 84 b7 fd ff ff 0f 0b 0f 1f 00 e9 ad fd ff
> [   17.217448] RSP: 0018:c995fbb0 EFLAGS: 00010202
> [   17.217451] RAX: 0001 RBX: 8881052c8de0 RCX: 
> 
> [   17.217453] RDX: 0001 RSI:  RDI: 
> 8881052c8de0
> [   17.217455] RBP: 888104a66e00 R08: 8881052c8de0 R09: 
> 888104a7cf08
> [   17.217457] R10: c995fbe0 R11: c995fbe8 R12: 
> 8881052c8c78
> [   17.217458] R13: 8881052c8c78 R14: dead0100 R15: 
> 88810528b108
> [   17.217460] FS:  7f319fcbb8c0() GS:1a54() 
> knlGS:
> [   17.217463] CS:  0010 DS:  ES:  CR0: 80050033
> [   17.217464] CR2: 55dc8b0224a0 CR3: 00010373d000 CR4: 
> 00750ee0
> [   17.217466] PKRU: 5554
> [   17.217468] Call Trace:
> [   17.217470]  
> [   17.217472]  ? __warn+0x97/0x160
> [   17.217476]  ? ttm_bo_release+0x27e/0x2d0 [ttm]
> [   17.217481]  ? report_bug+0x1ec/0x200
> [   17.217487]  ? handle_bug+0x3c/0x70
> [   17.217490]  ? exc_invalid_op+0x1f/0x90
> [   17.217493]  ? preempt_count_sub+0xb5/0x100
> [   17.217496]  ? asm_exc_invalid_op+0x16/0x20
> [   17.217500]  ? ttm_bo_release+0x27e/0x2d0 [ttm]
> [   17.217505]  ? ttm_resource_move_to_lru_tail+0x1ab/0x1d0 [ttm]
> [   17.217511]  radeon_bo_unref+0x1a/0x30 [radeon]
> [   17.217547]  radeon_gem_object_free+0x20/0x30 [radeon]
> [   17.217579]  radeon_fbdev_fb_destroy+0x57/0x90 [radeon]
> [   17.217616]  unregister_framebuffer+0x72/0x110
> [   17.217620]  drm_client_dev_unregister+0x6d/0xe0
> [   17.217623]  drm_dev_unregister+0x2e/0x90
> [   17.217626]  drm_put_dev+0x26/0x90
> [   17.217628]  pci_device_remove+0x44/0xc0
> [   17.217631]  really_probe+0x257/0x340
> [   17.217635]  __driver_probe_device+0x73/0x120
> [   17.217638]  driver_probe_device+0x2c/0xb0
> [   17.217641]  __driver_attach+0xa0/0x150
> [   17.217643]  ? __pfx___driver_attach+0x10/0x10
> [   17.217646]  bus_for_each_dev+0x67/0xa0
> [   17.217649]  bus_add_driver+0x10e/0x210
> [   17.217651]  driver_register+0x5c/0x120
> [   17.217653]  ? __pfx_radeon_module_init+0x10/0x10 [radeon]
> [   17.217681]  do_one_initcall+0x44/0x220
> [   17.217684]  ? kmalloc_trace+0x37/0xc0
> [   17.217688]  do_init_module+0x64/0x240
> [   17.217691]  __do_sys_finit_module+0xb2/0x100
> [   17.217694]  do_syscall_64+0x3b/0x90
> [   17.217697]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [   17.217700] RIP: 0033:0x7f319feaa5a9
> [   17.217702] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 89 
> f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 
> 01 f0 ff ff 73 01 c3 48 8b 0d 27 08 0d 00 f7 d8 64 89 01 48
> [   17.217706] RSP: 002b:7ffc6bf3e7f8 EFLAGS: 0246 ORIG_RAX: 
> 0139
> [   17.217709] RAX: ffda RBX: 5607204f3170 RCX: 
> 7f319feaa5a9
> [   17.217710] RDX:  RSI: 7f31a002eefd RDI: 
> 0018
> [   17.217712] RBP: 7f31a002eefd R08:  R09: 
> 5607204f1860
> [   17.217714] R10: 0018 R11: 0246 R12: 
> 0002
> [   17.217716] R13:  R14: 560720522450 R15: 
> 560720255899
> [   17.217718]  
> [   17.217719] ---[ end trace  ]---
> 
> The buffer object backing the fbdev emulation got pinned twice: by the
> fb_probe helper radeon_fbdev_create_pinned_object() and the modesetting
> code when the framebuffer got displayed. It only got unpinned once by
> the fbdev helper radeon_fbdev_destroy_pinned_object(). Hence TTM's BO-
> release function complains about the pin counter. Forcing the outputs
> off also undoes the modesettings pin increment.
> 
> Reported-by: Borislav Petkov 
> Closes: 
> https://lore.kernel.org/dri-devel/20230603174814.GCZHt83pN+wNjf63sC@fat_crate.local/
> Signed-off-by: Thomas Zimmermann 
> Fixes: e317a69fe891

Re: [PATCH] drm/edid: Add quirk for OSVR HDK 2.0

2023-06-09 Thread Ralph Campbell




On 6/9/23 02:03, Jani Nikula wrote:

On Thu, 08 Jun 2023, Ralph Campbell  wrote:

The OSVR virtual reality headset HDK 2.0 uses a different EDID
vendor and device identifier than the HDK 1.1 - 1.4 headsets.
Add the HDK 2.0 vendor and device identifier to the quirks table so
that window managers do not try to display the desktop screen on the
headset display.

At some point in time we requested bugs to be filed about quirks, with
EDIDs attached, so we could look at them later, and maybe remove the
quirks.

The headset non-desktop thing started off as a quirk, but since then
we've added both Microsoft VSDB and DisplayID primary use as ways to
indicate this without quirks.

BR,
Jani.


If you want me to file a bug, I can do that and I have the EDID too.
Where would I file it?

I did see the DisplayID 2.0 code. This headset is no longer being
manufactured so updating the EDID is not practical.


Signed-off-by: Ralph Campbell 
Tested-by: Ralph Campbell 
---
  drivers/gpu/drm/drm_edid.c | 1 +
  1 file changed, 1 insertion(+)

I don't know how many of these VR headsets are still around but I have a
working one and I saw and entry for HDK 1.x so I thought it would be good
to add HDK 2.0.

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 0454da505687..3b8cc1fe05e8 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -230,6 +230,7 @@ static const struct edid_quirk {
  
  	/* OSVR HDK and HDK2 VR Headsets */

EDID_QUIRK('S', 'V', 'R', 0x1019, EDID_QUIRK_NON_DESKTOP),
+   EDID_QUIRK('A', 'O', 'U', 0x, EDID_QUIRK_NON_DESKTOP),
  };
  
  /*

Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression





On 6/9/2023 9:58 AM, Dmitry Baryshkov wrote:

On 08/06/2023 23:36, Marijn Suijten wrote:

Same title suggestion as earlier: s/adjust/reduce

On 2023-05-22 18:08:56, Jessica Zhang wrote:
Adjust the pclk rate to divide hdisplay by the compression ratio when 
DSC

is enabled.

Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++---
  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c

index a448931af804..88f370dd2ea1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -561,7 +561,18 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
*msm_host)

  clk_disable_unprepare(msm_host->byte_clk);
  }
-static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
*mode, bool is_bonded_dsi)
+static unsigned long dsi_adjust_compressed_pclk(const struct 
drm_display_mode *mode,


Nit: adjust_pclk_for_compression

As discussed before we realized that this change is more-or-less a hack,
since downstream calculates pclk quite differently - at least for
command-mode panels.  Do you still intend to land this patch this way,
or go the proper route by introducing the right math from the get-go?
Or is the math at least correct for video-mode panels?


Can we please postpone the cmd-vs-video discussion? Otherwise I will 
reserve myself a right to push a patch dropping CMD mode support until 
somebody comes with a proper way to handle CMD clock calculation.



It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel 
timings are a kind of a hack and should be improved. No, we can not do 
this as a part of this series. I think everybody agrees that with the 
current way of calculating CMD panel timings, this function does some 
kind of a trick.




This function requires a documentation comment to explain that all.


+    const struct drm_dsc_config *dsc)
+{
+    int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * 
drm_dsc_get_bpp_int(dsc),


This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
bits_per_component==8 is assumed.  In fact, it then becomes identical
to the following line in dsi_host.c which you added previously:

hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 
3);


This would imply a simple /3, but as far as I understand it is not 
correct here.




If not, what is the difference between these two calculations?  Maybe
they both need to be in a properly-named helper.


+    dsc->bits_per_component * 3);


I hope to see a documentation patch to be posted, telling that this 
scales hdisplay and thus pclk by the factor of compressed_bpp / 
uncompressed_bpp.


This is not how it is usually done, but I would accept a separate 
documentation patch going over the calculation here and in 
dsi_timing_setup (and maybe other unobvious cases, if there is anything 
left).




As we established in the drm/msm issue [2] there is currently a
confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
ratio between dsi_get_bpp() and dsc->bpp or something else.  Can you
clarify that with constants and comments?

[2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24


+
+    return (new_hdisplay + (mode->htotal - mode->hdisplay))
+    * mode->vtotal * drm_mode_vrefresh(mode);


As clarified in [1] I was not necessarily suggesting to move this math
to a separate helper, but to also use a few more properly-named
intermediate variables to not have multi-line math and self-documenting
code.  These lines could be split to be much more clear.


I think it's fine more or less. One pair of parenthesis is unnecessary, 
but that's mostly it. Maybe `new_htotal' variable would make some sense.


Also, please excuse me if this was discussed somewhere. This calculation 
means that only the displayed data is compressed, but porches are not 
touched. Correct?


Hi Dmitry,

Correct, we will apply the compression ratio to only the hdisplay but 
keep the porches as is.






[1]: 
https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/



+}
+
+static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
*mode,

+    const struct drm_dsc_config *dsc, bool is_bonded_dsi)
  {
  unsigned long pclk_rate;
@@ -576,6 +587,10 @@ static unsigned long dsi_get_pclk_rate(const 
struct drm_display_mode *mode, bool

  if (is_bonded_dsi)
  pclk_rate /= 2;
+    /* If DSC is enabled, divide hdisplay by compression ratio */
+    if (dsc)
+    pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);


Looking for the perfection, I'd also move the pclk adjustment to come 
before the is_bonded_dsi check.


Acked.

Thanks,

Jessica Zhang





The confusion with this comment (and the reason the aforementioned
discussion [2] carried on so long) stems from the fact a division makes
sense for a bit/byte clock, but not for a pixel clock: we

Re: [PATCH 0/2] accel/qaic fixes for 6.4 part 2

2023-06-09 Thread Jeffrey Hugo


On 6/2/2023 3:04 PM, Jeffrey Hugo wrote:

Two additional fixes for corner cases found during development when
buggy userspace or firmware ends up subjecting the KMD to error
scenarios.

Carl Vanderlip (1):
   accel/qaic: Free user handle on interrupted mutex

Jeffrey Hugo (1):
   accel/qaic: Fix NULL pointer deref in qaic_destroy_drm_device()

  drivers/accel/qaic/qaic_drv.c | 4 
  1 file changed, 4 insertions(+)



Pushed to drm-misc-fixes

Re: [PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj

2023-06-09 Thread Rob Clark

On Fri, Jun 9, 2023 at 7:12 AM Tvrtko Ursulin
 wrote:
>
>
> On 09/06/2023 13:44, Iddamsetty, Aravind wrote:
> > On 09-06-2023 17:41, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin 
> >>
> >> I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which
> >> will return a reference to a newly created GEM objects (if created), in
> >> order to enable tracking of imported i915 GEM objects in the following
> >> patch.
> >
> > instead of this what if we implement the open call back in i915
> >
> > struct drm_gem_object_funcs {
> >
> >  /**
> >   * @open:
> >   *
> >   * Called upon GEM handle creation.
> >   *
> >   * This callback is optional.
> >   */
> >  int (*open)(struct drm_gem_object *obj, struct drm_file *file);
> >
> > which gets called whenever a handle(drm_gem_handle_create_tail) is
> > created and in the open we can check if to_intel_bo(obj)->base.dma_buf
> > then it is imported if not it is owned or created by it.
>
> I wanted to track as much memory usage as we have which is buffer object
> backed, including page tables and contexts. And since those are not user
> visible (they don't have handles), they wouldn't be covered by the
> obj->funcs->open() callback.
>
> If we wanted to limit to objects with handles we could simply do what
> Rob proposed and that is to walk the handles idr. But that does not feel
> like the right direction to me. Open for discussion I guess.

I guess you just have a few special case objects per context?
Wouldn't it be easier to just track _those_ specially and append them
to the results after doing the normal idr table walk?

(Also, doing something special for dma-buf smells a bit odd..
considering that we also have legacy flink name based sharing as
well.)

BR,
-R

[PATCH v2 5/5] drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()

The driver only supports OLED controllers that have a native DRM_FORMAT_C1
pixel format and that is why it has harcoded a division of the width by 8.

But the driver might be extended to support devices that have a different
pixel format. So it's better to use the struct drm_format_info helpers to
compute the size of the buffer, used to store the pixels in native format.

Signed-off-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
---

(no changes since v1)

 drivers/gpu/drm/solomon/ssd130x.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index 0be3b476dc60..b3dc1ca9dc10 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -150,9 +150,16 @@ static int ssd130x_buf_alloc(struct ssd130x_device 
*ssd130x)
 {
unsigned int page_height = ssd130x->device_info->page_height;
unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+   const struct drm_format_info *fi;
+   unsigned int pitch;
 
-   ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8),
- ssd130x->height, GFP_KERNEL);
+   fi = drm_format_info(DRM_FORMAT_C1);
+   if (!fi)
+   return -EINVAL;
+
+   pitch = drm_format_info_min_pitch(fi, 0, ssd130x->width);
+
+   ssd130x->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
if (!ssd130x->buffer)
return -ENOMEM;
 
-- 
2.40.1

[PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update

The resolutions for these panels are fixed and defined in the Device Tree,
so there's no point to allocate the buffers on each plane update and that
can just be done once.

Let's do the allocation and free on the encoder enable and disable helpers
since that's where others initialization and teardown operations are done.

Signed-off-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
---

(no changes since v1)

 drivers/gpu/drm/solomon/ssd130x.c | 88 +++
 drivers/gpu/drm/solomon/ssd130x.h |  3 ++
 2 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index 5cac1149e34e..0be3b476dc60 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -146,6 +146,31 @@ static inline struct ssd130x_device *drm_to_ssd130x(struct 
drm_device *drm)
return container_of(drm, struct ssd130x_device, drm);
 }
 
+static int ssd130x_buf_alloc(struct ssd130x_device *ssd130x)
+{
+   unsigned int page_height = ssd130x->device_info->page_height;
+   unsigned int pages = DIV_ROUND_UP(ssd130x->height, page_height);
+
+   ssd130x->buffer = kcalloc(DIV_ROUND_UP(ssd130x->width, 8),
+ ssd130x->height, GFP_KERNEL);
+   if (!ssd130x->buffer)
+   return -ENOMEM;
+
+   ssd130x->data_array = kcalloc(ssd130x->width, pages, GFP_KERNEL);
+   if (!ssd130x->data_array) {
+   kfree(ssd130x->buffer);
+   return -ENOMEM;
+   }
+
+   return 0;
+}
+
+static void ssd130x_buf_free(struct ssd130x_device *ssd130x)
+{
+   kfree(ssd130x->data_array);
+   kfree(ssd130x->buffer);
+}
+
 /*
  * Helper to write data (SSD130X_DATA) to the device.
  */
@@ -434,11 +459,12 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
 SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
 }
 
-static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
-  struct drm_rect *rect)
+static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect 
*rect)
 {
unsigned int x = rect->x1;
unsigned int y = rect->y1;
+   u8 *buf = ssd130x->buffer;
+   u8 *data_array = ssd130x->data_array;
unsigned int width = drm_rect_width(rect);
unsigned int height = drm_rect_height(rect);
unsigned int line_length = DIV_ROUND_UP(width, 8);
@@ -447,14 +473,9 @@ static int ssd130x_update_rect(struct ssd130x_device 
*ssd130x, u8 *buf,
struct drm_device *drm = >drm;
u32 array_idx = 0;
int ret, i, j, k;
-   u8 *data_array = NULL;
 
drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
 
-   data_array = kcalloc(width, pages, GFP_KERNEL);
-   if (!data_array)
-   return -ENOMEM;
-
/*
 * The screen is divided in pages, each having a height of 8
 * pixels, and the width of the screen. When sending a byte of
@@ -488,11 +509,11 @@ static int ssd130x_update_rect(struct ssd130x_device 
*ssd130x, u8 *buf,
/* Set address range for horizontal addressing mode */
ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, 
width);
if (ret < 0)
-   goto out_free;
+   return ret;
 
ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y 
/ 8, pages);
if (ret < 0)
-   goto out_free;
+   return ret;
}
 
for (i = 0; i < pages; i++) {
@@ -522,11 +543,11 @@ static int ssd130x_update_rect(struct ssd130x_device 
*ssd130x, u8 *buf,
   ssd130x->page_offset + i,
   ssd130x->col_offset + x);
if (ret < 0)
-   goto out_free;
+   return ret;
 
ret = ssd130x_write_data(ssd130x, data_array, width);
if (ret < 0)
-   goto out_free;
+   return ret;
 
array_idx = 0;
}
@@ -536,14 +557,11 @@ static int ssd130x_update_rect(struct ssd130x_device 
*ssd130x, u8 *buf,
if (!ssd130x->page_address_mode)
ret = ssd130x_write_data(ssd130x, data_array, width * pages);
 
-out_free:
-   kfree(data_array);
return ret;
 }
 
 static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
 {
-   u8 *buf = NULL;
struct drm_rect fullscreen = {
.x1 = 0,
.x2 = ssd130x->width,
@@ -551,14 +569,7 @@ static void ssd130x_clear_screen(struct ssd130x_device 
*ssd130x)
.y2 = ssd130x->height,
};
 
-   buf = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), ssd130x->height,
-

[PATCH v2 2/5] dt-bindings: display: ssd1307fb: Remove default width and height values

A default resolution in the ssd130x driver isn't set to an arbitrary 96x16
anymore. Instead is set to a width and height that's controller dependent.

The datasheets for the chips describes the following display resolutions:

 - SH1106:  132 x 64 Dot Matrix OLED/PLED
 - SSD1305: 132 x 64 Dot Matrix OLED/PLED
 - SSD1306: 128 x 64 Dot Matrix OLED/PLED
 - SSD1307: 128 x 39 Dot Matrix OLED/PLED
 - SSD1309: 128 x 64 Dot Matrix OLED/PLED

Update DT schema to reflect what the driver does and make its users aware.

Signed-off-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
---

Changes in v2:
- List per controller default width/height values in DT schema (Maxime Ripard).

 .../bindings/display/solomon,ssd1307fb.yaml   | 28 ---
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml 
b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
index 94bb5ef567c6..20e2bd15d4d2 100644
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
@@ -49,15 +49,15 @@ properties:
 
   solomon,height:
 $ref: /schemas/types.yaml#/definitions/uint32
-default: 16
 description:
-  Height in pixel of the screen driven by the controller
+  Height in pixel of the screen driven by the controller.
+  The default value is controller-dependent.
 
   solomon,width:
 $ref: /schemas/types.yaml#/definitions/uint32
-default: 96
 description:
-  Width in pixel of the screen driven by the controller
+  Width in pixel of the screen driven by the controller.
+  The default value is controller-dependent.
 
   solomon,page-offset:
 $ref: /schemas/types.yaml#/definitions/uint32
@@ -157,6 +157,10 @@ allOf:
 const: sinowealth,sh1106
 then:
   properties:
+width:
+  default: 132
+height:
+  default: 64
 solomon,dclk-div:
   default: 1
 solomon,dclk-frq:
@@ -171,6 +175,10 @@ allOf:
   - solomon,ssd1305
 then:
   properties:
+width:
+  default: 132
+height:
+  default: 64
 solomon,dclk-div:
   default: 1
 solomon,dclk-frq:
@@ -185,6 +193,10 @@ allOf:
   - solomon,ssd1306
 then:
   properties:
+width:
+  default: 128
+height:
+  default: 64
 solomon,dclk-div:
   default: 1
 solomon,dclk-frq:
@@ -199,6 +211,10 @@ allOf:
   - solomon,ssd1307
 then:
   properties:
+width:
+  default: 128
+height:
+  default: 39
 solomon,dclk-div:
   default: 2
 solomon,dclk-frq:
@@ -215,6 +231,10 @@ allOf:
   - solomon,ssd1309
 then:
   properties:
+width:
+  default: 128
+height:
+  default: 64
 solomon,dclk-div:
   default: 1
 solomon,dclk-frq:
-- 
2.40.1

[PATCH v2 3/5] drm/ssd130x: Set the page height value in the device info data

The driver only supports OLED controllers that have a page height of 8 but
there are devices that have different page heights. So it is better to not
hardcode this value and instead have it as a per controller data value.

Signed-off-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
---

(no changes since v1)

 drivers/gpu/drm/solomon/ssd130x.c | 15 +++
 drivers/gpu/drm/solomon/ssd130x.h |  1 +
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index a0e5e26c0bc9..5cac1149e34e 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -102,6 +102,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_width = 132,
.default_height = 64,
.page_mode_only = 1,
+   .page_height = 8,
},
[SSD1305_ID] = {
.default_vcomh = 0x34,
@@ -109,6 +110,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_dclk_frq = 7,
.default_width = 132,
.default_height = 64,
+   .page_height = 8,
},
[SSD1306_ID] = {
.default_vcomh = 0x20,
@@ -117,6 +119,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.need_chargepump = 1,
.default_width = 128,
.default_height = 64,
+   .page_height = 8,
},
[SSD1307_ID] = {
.default_vcomh = 0x20,
@@ -125,6 +128,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.need_pwm = 1,
.default_width = 128,
.default_height = 39,
+   .page_height = 8,
},
[SSD1309_ID] = {
.default_vcomh = 0x34,
@@ -132,6 +136,7 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_dclk_frq = 10,
.default_width = 128,
.default_height = 64,
+   .page_height = 8,
}
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -437,7 +442,8 @@ static int ssd130x_update_rect(struct ssd130x_device 
*ssd130x, u8 *buf,
unsigned int width = drm_rect_width(rect);
unsigned int height = drm_rect_height(rect);
unsigned int line_length = DIV_ROUND_UP(width, 8);
-   unsigned int pages = DIV_ROUND_UP(height, 8);
+   unsigned int page_height = ssd130x->device_info->page_height;
+   unsigned int pages = DIV_ROUND_UP(height, page_height);
struct drm_device *drm = >drm;
u32 array_idx = 0;
int ret, i, j, k;
@@ -559,16 +565,17 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer 
*fb, const struct iosys_m
struct drm_rect *rect)
 {
struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+   unsigned int page_height = ssd130x->device_info->page_height;
struct iosys_map dst;
unsigned int dst_pitch;
int ret = 0;
u8 *buf = NULL;
 
/* Align y to display page boundaries */
-   rect->y1 = round_down(rect->y1, 8);
-   rect->y2 = min_t(unsigned int, round_up(rect->y2, 8), ssd130x->height);
+   rect->y1 = round_down(rect->y1, page_height);
+   rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), 
ssd130x->height);
 
-   dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+   dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), page_height);
buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL);
if (!buf)
return -ENOMEM;
diff --git a/drivers/gpu/drm/solomon/ssd130x.h 
b/drivers/gpu/drm/solomon/ssd130x.h
index a2bc8d75078b..87968b3e7fb8 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -39,6 +39,7 @@ struct ssd130x_deviceinfo {
u32 default_dclk_frq;
u32 default_width;
u32 default_height;
+   u32 page_height;
int need_pwm;
int need_chargepump;
bool page_mode_only;
-- 
2.40.1

[PATCH v2 1/5] drm/ssd130x: Make default width and height to be controller dependent

Currently the driver hardcodes the default values to 96x16 pixels but this
default resolution depends on the controller. The datasheets for the chips
describes the following display controller resolutions:

 - SH1106:  132 x 64 Dot Matrix OLED/PLED
 - SSD1305: 132 x 64 Dot Matrix OLED/PLED
 - SSD1306: 128 x 64 Dot Matrix OLED/PLED
 - SSD1307: 128 x 39 Dot Matrix OLED/PLED
 - SSD1309: 128 x 64 Dot Matrix OLED/PLED

Add this information to the devices' info structures, and use it set as a
default if not defined in DT rather than hardcoding to an arbitrary value.

Signed-off-by: Javier Martinez Canillas 
Reviewed-by: Thomas Zimmermann 
---

(no changes since v1)

 drivers/gpu/drm/solomon/ssd130x.c | 14 --
 drivers/gpu/drm/solomon/ssd130x.h |  2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c 
b/drivers/gpu/drm/solomon/ssd130x.c
index 8cbf5aa66e19..a0e5e26c0bc9 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -99,29 +99,39 @@ const struct ssd130x_deviceinfo ssd130x_variants[] = {
.default_vcomh = 0x40,
.default_dclk_div = 1,
.default_dclk_frq = 5,
+   .default_width = 132,
+   .default_height = 64,
.page_mode_only = 1,
},
[SSD1305_ID] = {
.default_vcomh = 0x34,
.default_dclk_div = 1,
.default_dclk_frq = 7,
+   .default_width = 132,
+   .default_height = 64,
},
[SSD1306_ID] = {
.default_vcomh = 0x20,
.default_dclk_div = 1,
.default_dclk_frq = 8,
.need_chargepump = 1,
+   .default_width = 128,
+   .default_height = 64,
},
[SSD1307_ID] = {
.default_vcomh = 0x20,
.default_dclk_div = 2,
.default_dclk_frq = 12,
.need_pwm = 1,
+   .default_width = 128,
+   .default_height = 39,
},
[SSD1309_ID] = {
.default_vcomh = 0x34,
.default_dclk_div = 1,
.default_dclk_frq = 10,
+   .default_width = 128,
+   .default_height = 64,
}
 };
 EXPORT_SYMBOL_NS_GPL(ssd130x_variants, DRM_SSD130X);
@@ -798,10 +808,10 @@ static void ssd130x_parse_properties(struct 
ssd130x_device *ssd130x)
struct device *dev = ssd130x->dev;
 
if (device_property_read_u32(dev, "solomon,width", >width))
-   ssd130x->width = 96;
+   ssd130x->width = ssd130x->device_info->default_width;
 
if (device_property_read_u32(dev, "solomon,height", >height))
-   ssd130x->height = 16;
+   ssd130x->height = ssd130x->device_info->default_height;
 
if (device_property_read_u32(dev, "solomon,page-offset", 
>page_offset))
ssd130x->page_offset = 1;
diff --git a/drivers/gpu/drm/solomon/ssd130x.h 
b/drivers/gpu/drm/solomon/ssd130x.h
index db03ee5db392..a2bc8d75078b 100644
--- a/drivers/gpu/drm/solomon/ssd130x.h
+++ b/drivers/gpu/drm/solomon/ssd130x.h
@@ -37,6 +37,8 @@ struct ssd130x_deviceinfo {
u32 default_vcomh;
u32 default_dclk_div;
u32 default_dclk_frq;
+   u32 default_width;
+   u32 default_height;
int need_pwm;
int need_chargepump;
bool page_mode_only;
-- 
2.40.1

[PATCH v2 0/5] drm/ssd130x: A few enhancements and cleanups

Hello,

While working on adding support for the SSD132X family of 4-bit grayscale
Solomon OLED panel controllers, I noticed a few things in the driver that
can be improved and make extending to support other chip families easier.

I've split the preparatory patches in this series and will post the actual
SSD132X support as a separate patch-set once this one is merged.

Best regards,
Javier

Changes in v2:
- List per controller default width/height values in DT schema (Maxime Ripard).

Javier Martinez Canillas (5):
  drm/ssd130x: Make default width and height to be controller dependent
  dt-bindings: display: ssd1307fb: Remove default width and height
values
  drm/ssd130x: Set the page height value in the device info data
  drm/ssd130x: Don't allocate buffers on each plane update
  drm/ssd130x: Remove hardcoded bits-per-pixel in ssd130x_buf_alloc()

 .../bindings/display/solomon,ssd1307fb.yaml   |  28 +++-
 drivers/gpu/drm/solomon/ssd130x.c | 124 --
 drivers/gpu/drm/solomon/ssd130x.h |   6 +
 3 files changed, 113 insertions(+), 45 deletions(-)

-- 
2.40.1

Re: [PATCH v5 2/5] drm/msm/dsi: Adjust pclk rate for compression

2023-06-09 Thread Dmitry Baryshkov


On 08/06/2023 23:36, Marijn Suijten wrote:

Same title suggestion as earlier: s/adjust/reduce

On 2023-05-22 18:08:56, Jessica Zhang wrote:

Adjust the pclk rate to divide hdisplay by the compression ratio when DSC
is enabled.

Signed-off-by: Jessica Zhang 
---
  drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++---
  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a448931af804..88f370dd2ea1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -561,7 +561,18 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
clk_disable_unprepare(msm_host->byte_clk);
  }
  
-static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)

+static unsigned long dsi_adjust_compressed_pclk(const struct drm_display_mode 
*mode,


Nit: adjust_pclk_for_compression

As discussed before we realized that this change is more-or-less a hack,
since downstream calculates pclk quite differently - at least for
command-mode panels.  Do you still intend to land this patch this way,
or go the proper route by introducing the right math from the get-go?
Or is the math at least correct for video-mode panels?


Can we please postpone the cmd-vs-video discussion? Otherwise I will 
reserve myself a right to push a patch dropping CMD mode support until 
somebody comes with a proper way to handle CMD clock calculation.



It is off-topic for the sake of DSC 1.2 support. Yes, all CMD panel 
timings are a kind of a hack and should be improved. No, we can not do 
this as a part of this series. I think everybody agrees that with the 
current way of calculating CMD panel timings, this function does some 
kind of a trick.




This function requires a documentation comment to explain that all.


+   const struct drm_dsc_config *dsc)
+{
+   int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * 
drm_dsc_get_bpp_int(dsc),


This sounds like a prime candidate for msm_dsc_get_bytes_per_line(), if
bits_per_component==8 is assumed.  In fact, it then becomes identical
to the following line in dsi_host.c which you added previously:

hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);


This would imply a simple /3, but as far as I understand it is not 
correct here.




If not, what is the difference between these two calculations?  Maybe
they both need to be in a properly-named helper.


+   dsc->bits_per_component * 3);


I hope to see a documentation patch to be posted, telling that this 
scales hdisplay and thus pclk by the factor of compressed_bpp / 
uncompressed_bpp.


This is not how it is usually done, but I would accept a separate 
documentation patch going over the calculation here and in 
dsi_timing_setup (and maybe other unobvious cases, if there is anything 
left).




As we established in the drm/msm issue [2] there is currently a
confusion whether this /3 (and the /3 in dsi_timing_setup) come from the
ratio between dsi_get_bpp() and dsc->bpp or something else.  Can you
clarify that with constants and comments?

[2]: https://gitlab.freedesktop.org/drm/msm/-/issues/24


+
+   return (new_hdisplay + (mode->htotal - mode->hdisplay))
+   * mode->vtotal * drm_mode_vrefresh(mode);


As clarified in [1] I was not necessarily suggesting to move this math
to a separate helper, but to also use a few more properly-named
intermediate variables to not have multi-line math and self-documenting
code.  These lines could be split to be much more clear.


I think it's fine more or less. One pair of parenthesis is unnecessary, 
but that's mostly it. Maybe `new_htotal' variable would make some sense.


Also, please excuse me if this was discussed somewhere. This calculation 
means that only the displayed data is compressed, but porches are not 
touched. Correct?




[1]: 
https://lore.kernel.org/linux-arm-msm/u4x2vldkzsokfcpbkz3dtwcllbdk4ljcz6kzuaxt5frx6g76o5@uku6abewvye7/


+}
+
+static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
+   const struct drm_dsc_config *dsc, bool is_bonded_dsi)
  {
unsigned long pclk_rate;
  
@@ -576,6 +587,10 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool

if (is_bonded_dsi)
pclk_rate /= 2;
  
+	/* If DSC is enabled, divide hdisplay by compression ratio */

+   if (dsc)
+   pclk_rate = dsi_adjust_compressed_pclk(mode, dsc);


Looking for the perfection, I'd also move the pclk adjustment to come 
before the is_bonded_dsi check.




The confusion with this comment (and the reason the aforementioned
discussion [2] carried on so long) stems from the fact a division makes
sense for a bit/byte clock, but not for a pixel clock: we still intend
to send the same number of pixels, just spending less bytes on them.  So
as you clarify the /3 above, can you also

[linux-next:master] BUILD REGRESSION 53ab6975c12d1ad86c599a8927e8c698b144d669

2023-06-09 Thread kernel test robot

tree/branch: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch HEAD: 53ab6975c12d1ad86c599a8927e8c698b144d669  Add linux-next specific 
files for 20230609

Error/Warning reports:

https://lore.kernel.org/oe-kbuild-all/202306081708.gtvacxsh-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306082341.uqtcm8po-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306100035.vtusnhm4-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202306100056.z3g9guft-...@intel.com

Error/Warning: (recently discovered and may have been fixed)

drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:113:17: warning: format specifies 
type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:147:17: warning: format specifies 
type 'long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
drivers/leds/leds-cht-wcove.c:144:21: warning: no previous prototype for 
'cht_wc_leds_brightness_get' [-Wmissing-prototypes]
drivers/scsi/FlashPoint.c:1712:12: warning: stack frame size (1056) exceeds 
limit (1024) in 'FlashPoint_HandleInterrupt' [-Wframe-larger-than]
include/drm/drm_print.h:456:39: error: format '%ld' expects argument of type 
'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} 
[-Werror=format=]
include/drm/drm_print.h:456:39: warning: format '%ld' expects argument of type 
'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
uvdevice.c:(.init.text+0x2e): undefined reference to `uv_info'

Unverified Error/Warning (likely false positive, please contact us if 
interested):

drivers/net/ethernet/emulex/benet/be_main.c:2460 be_rx_compl_process_gro() 
error: buffer overflow '((skb_end_pointer(skb)))->frags' 17 <= u16max
drivers/usb/cdns3/cdns3-starfive.c:23: warning: expecting prototype for 
cdns3(). Prototype was for USB_STRAP_HOST() instead
fs/btrfs/volumes.c:6407 btrfs_map_block() error: we previously assumed 
'mirror_num_ret' could be null (see line 6245)
kernel/events/uprobes.c:478 uprobe_write_opcode() warn: passing zero to 
'PTR_ERR'

Error/Warning ids grouped by kconfigs:

gcc_recent_errors
|-- i386-allyesconfig
|   |-- 
drivers-leds-leds-cht-wcove.c:warning:no-previous-prototype-for-cht_wc_leds_brightness_get
|   `-- 
include-drm-drm_print.h:error:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int
|-- i386-randconfig-i004-20230608
|   `-- 
include-drm-drm_print.h:error:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int
|-- i386-randconfig-i062-20230608
|   `-- 
include-drm-drm_print.h:warning:format-ld-expects-argument-of-type-long-int-but-argument-has-type-size_t-aka-unsigned-int
|-- riscv-allmodconfig
|   `-- 
drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead
|-- riscv-allyesconfig
|   `-- 
drivers-usb-cdns3-cdns3-starfive.c:warning:expecting-prototype-for-cdns3().-Prototype-was-for-USB_STRAP_HOST()-instead
|-- s390-randconfig-r044-20230609
|   `-- uvdevice.c:(.init.text):undefined-reference-to-uv_info
|-- x86_64-allyesconfig
|   `-- 
drivers-leds-leds-cht-wcove.c:warning:no-previous-prototype-for-cht_wc_leds_brightness_get
`-- x86_64-randconfig-m001-20230608
|-- 
drivers-net-ethernet-emulex-benet-be_main.c-be_rx_compl_process_gro()-error:buffer-overflow-((skb_end_pointer(skb)))-frags-u16max
|-- 
fs-btrfs-volumes.c-btrfs_map_block()-error:we-previously-assumed-mirror_num_ret-could-be-null-(see-line-)
`-- 
kernel-events-uprobes.c-uprobe_write_opcode()-warn:passing-zero-to-PTR_ERR
clang_recent_errors
|-- i386-randconfig-i014-20230608
|   `-- 
drivers-gpu-drm-i915-pxp-intel_pxp_gsccs.c:warning:format-specifies-type-long-but-the-argument-has-type-size_t-(aka-unsigned-int-)
|-- i386-randconfig-r026-20230608
|   `-- 
drivers-gpu-drm-i915-pxp-intel_pxp_gsccs.c:warning:format-specifies-type-long-but-the-argument-has-type-size_t-(aka-unsigned-int-)
`-- powerpc-allmodconfig
`-- 
drivers-scsi-FlashPoint.c:warning:stack-frame-size-()-exceeds-limit-()-in-FlashPoint_HandleInterrupt

elapsed time: 729m

configs tested: 149
configs skipped: 6

tested configs:
alphaallyesconfig   gcc  
alpha   defconfig   gcc  
arc  allyesconfig   gcc  
arc defconfig   gcc  
arc haps_hs_defconfig   gcc  
arc  randconfig-r011-20230608   gcc  
arc  randconfig-r033-20230608   gcc  
arc  randconfig-r043-20230608   gcc  
arm  allmodconfig   gcc  
arm  allyesconfig   gcc  
arm   aspeed_g4_defconfig   clang
arm defconfig   gcc  
arm  footbridge_defconfig   gcc  
armhisi_defconfig   gcc

Re: [PATCH v2] drm/i915/gsc: take a wakeref for the proxy-init-completion check

2023-06-09 Thread Ceraolo Spurio, Daniele





On 6/8/2023 4:07 PM, Alan Previn wrote:

Ensure intel_gsc_uc_fw_init_done and intel_gsc_uc_fw_proxy_init
takes a wakeref before reading GSC Shim registers.

NOTE: another patch in review also adds a call from selftest to
this same function. (https://patchwork.freedesktop.org/series/117713/)
which is why i am adding the wakeref inside the callee, not the
caller.

v2: - add a helper, 'gsc_uc_get_fw_status' for both callers
   (Daniele Ceraolo)

Fixes: 99afb7cc8c44 ("drm/i915/pxp: Add ARB session creation and cleanup")
Signed-off-by: Alan Previn 


Reviewed-by: Daniele Ceraolo Spurio 

Daniele


---
  drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 22 ++
  1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c 
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
index f46eb17a7a98..60e9c6c9e775 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -24,21 +24,27 @@ static bool gsc_is_in_reset(struct intel_uncore *uncore)
   GSC_FW_CURRENT_STATE_RESET;
  }
  
-bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)

+static u32 gsc_uc_get_fw_status(struct intel_uncore *uncore)
  {
-   struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
-   u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+   intel_wakeref_t wakeref;
+   u32 fw_status = 0;
  
-	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==

+   with_intel_runtime_pm(uncore->rpm, wakeref)
+   fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+
+   return fw_status;
+}
+
+bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
+{
+   return REG_FIELD_GET(GSC_FW_CURRENT_STATE,
+gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore)) ==
   GSC_FW_PROXY_STATE_NORMAL;
  }
  
  bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)

  {
-   struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
-   u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
-
-   return fw_status & GSC_FW_INIT_COMPLETE_BIT;
+   return gsc_uc_get_fw_status(gsc_uc_to_gt(gsc)->uncore) & 
GSC_FW_INIT_COMPLETE_BIT;
  }
  
  static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)


base-commit: 27187d09511e1d47dbaaf91c7332319551a8edab

Re: [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register

2023-06-09 Thread Bjorn Helgaas

On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote:
> On 2023/6/9 03:19, Bjorn Helgaas wrote:
> > On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:
> > > From: Sui Jingfeng 
> > > 
> > > The vga_is_firmware_default() function is arch-dependent, which doesn't
> > > sound right. At least, it also works on the Mips and LoongArch platforms.
> > > Tested with the drm/amdgpu and drm/radeon drivers. However, it's difficult
> > > to enumerate all arch-driver combinations. I'm wrong if there is only one
> > > exception.
> > > 
> > > With the observation that device drivers typically have better knowledge
> > > about which PCI bar contains the firmware framebuffer, which could avoid
> > > the need to iterate all of the PCI BARs.
> > > 
> > > But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is
> > > probably not suitable to make such an optimization for a specific device.
> > > 
> > > There are PCI display controllers that don't have a dedicated VRAM bar,
> > > this function will lose its effectiveness in such a case. Luckily, the
> > > device driver can provide an accurate workaround.
> > > 
> > > Therefore, this patch introduces a callback that allows the device driver
> > > to tell the VGAARB if the device is the default boot device. This patch
> > > only intends to introduce the mechanism, while the implementation is left
> > > to the device driver authors. Also honor the comment: "Clients have two
> > > callback mechanisms they can use"
> > s/bar/BAR/ (several)
> > 
> > Nothing here uses the callback.  I don't want to merge this until we
> > have a user.
> 
> This is chicken and egg question.
> 
> If you could help get this merge first, I will show you the first user.
> 
> > I'm not sure why the device driver should know whether its device is
> > the default boot device.
> 
> It's not that the device driver should know,
> 
> but it's about that the device driver has the right to override.
> 
> Device driver may have better approach to identify the default boot
> device.

The way we usually handle this is to include the new callback in the
same series as the first user of it.  That has two benefits:
(1) everybody can review the whole picture and possibly suggest
different approaches, and (2) when we merge the infrastructure,
we also merge a user of it at the same time, so the whole thing can be
tested and we don't end up with unused code.

Bjorn

[PATCH] drm/amd/amdgpu: enable W=1 for amdgpu

2023-06-09 Thread Hamza Mahfooz

We have a clean build with W=1 as of
commit 12a15dd589ac ("drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move
SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef"). So, let's enable
these checks unconditionally for the entire module to catch these errors
during development.

Cc: Alex Deucher 
Cc: Nathan Chancellor 
Signed-off-by: Hamza Mahfooz 
---
 drivers/gpu/drm/amd/amdgpu/Makefile | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
b/drivers/gpu/drm/amd/amdgpu/Makefile
index 86b833085f19..8d16f280b695 100644
--- a/drivers/gpu/drm/amd/amdgpu/Makefile
+++ b/drivers/gpu/drm/amd/amdgpu/Makefile
@@ -40,7 +40,18 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
-I$(FULL_AMD_PATH)/amdkfd
 
 subdir-ccflags-y := -Wextra
-subdir-ccflags-y += $(call cc-option, -Wunused-but-set-variable)
+subdir-ccflags-y += -Wunused
+subdir-ccflags-y += -Wmissing-prototypes
+subdir-ccflags-y += -Wmissing-declarations
+subdir-ccflags-y += -Wmissing-include-dirs
+subdir-ccflags-y += -Wold-style-definition
+subdir-ccflags-y += -Wmissing-format-attribute
+# Need this to avoid recursive variable evaluation issues
+cond-flags := $(call cc-option, -Wunused-but-set-variable) \
+   $(call cc-option, -Wunused-const-variable) \
+   $(call cc-option, -Wstringop-truncation) \
+   $(call cc-option, -Wpacked-not-aligned)
+subdir-ccflags-y += $(cond-flags)
 subdir-ccflags-y += -Wno-unused-parameter
 subdir-ccflags-y += -Wno-type-limits
 subdir-ccflags-y += -Wno-sign-compare
-- 
2.40.1

Re: [RFC] Plane color pipeline KMS uAPI

2023-06-09 Thread Simon Ser

Hi Christopher,

On Friday, June 9th, 2023 at 17:52, Christopher Braga  
wrote:

> > The new COLOROP objects also expose a number of KMS properties. Each has a
> > type, a reference to the next COLOROP object in the linked list, and other
> > type-specific properties. Here is an example for a 1D LUT operation:
> >
> >  Color operation 42
> >  ├─ "type": enum {Bypass, 1D curve} = 1D curve
> >  ├─ "1d_curve_type": enum {LUT, sRGB, PQ, BT.709, HLG, …} = LUT
> The options sRGB / PQ / BT.709 / HLG would select hard-coded 1D
> curves? Will different hardware be allowed to expose a subset of these
> enum values?

Yes. Only hardcoded LUTs supported by the HW are exposed as enum entries.

> >  ├─ "lut_size": immutable range = 4096
> >  ├─ "lut_data": blob
> >  └─ "next": immutable color operation ID = 43
> >
> Some hardware has per channel 1D LUT values, while others use the same
> LUT for all channels.  We will definitely need to expose this in the
> UAPI in some form.

Hm, I was assuming per-channel 1D LUTs here, just like the existing GAMMA_LUT/
DEGAMMA_LUT properties work. If some hardware can't support that, it'll need
to get exposed as another color operation block.

> > To configure this hardware block, user-space can fill a KMS blob with
> > 4096 u32
> > entries, then set "lut_data" to the blob ID. Other color operation types
> > might
> > have different properties.
> >
> The bit-depth of the LUT is an important piece of information we should
> include by default. Are we assuming that the DRM driver will always
> reduce the input values to the resolution supported by the pipeline?
> This could result in differences between the hardware behavior
> and the shader behavior.
> 
> Additionally, some pipelines are floating point while others are fixed.
> How would user space know if it needs to pack 32 bit integer values vs
> 32 bit float values?

Again, I'm deferring to the existing GAMMA_LUT/DEGAMMA_LUT. These use a common
definition of LUT blob (u16 elements) and it's up to the driver to convert.

Using a very precise format for the uAPI has the nice property of making the
uAPI much simpler to use. User-space sends high precision data and it's up to
drivers to map that to whatever the hardware accepts.

Exposing the actual hardware precision is something we've talked about during
the hackfest. It'll probably be useful to some extent, but will require some
discussion to figure out how to design the uAPI. Maybe a simple property is
enough, maybe not (e.g. fully describing the precision of segmented LUTs would
probably be trickier).

I'd rather keep things simple for the first pass, we can always add more
properties for bit depth etc later on.

> > Here is another example with a 3D LUT:
> >
> >  Color operation 42
> >  ├─ "type": enum {Bypass, 3D LUT} = 3D LUT
> >  ├─ "lut_size": immutable range = 33
> >  ├─ "lut_data": blob
> >  └─ "next": immutable color operation ID = 43
> >
> We are going to need to expose the packing order here to avoid any
> programming uncertainty. I don't think we can safely assume all hardware
> is equivalent.

The driver can easily change the layout of the matrix and do any conversion
necessary when programming the hardware. We do need to document what layout is
used in the uAPI for sure.

> > And one last example with a matrix:
> >
> >  Color operation 42
> >  ├─ "type": enum {Bypass, Matrix} = Matrix
> >  ├─ "matrix_data": blob
> >  └─ "next": immutable color operation ID = 43
> >
> It is unclear to me what the default sizing of this matrix is. Any
> objections to exposing these details with an additional property?

The existing CTM property uses 9 uint64 (S31.32) values. Is there a case where
that wouldn't be enough?

> Dithering logic exists in some pipelines. I think we need a plan to
> expose that here as well.

Hm, I'm not too familiar with dithering. Do you think it would make sense to
expose as an additional colorop block? Do you think it would have more
consequences on the design?

I want to re-iterate that we don't need to ship all features from day 1. We
just need to come up with a uAPI design on which new features can be built on.

> > [Simon note: an alternative would be to split the color pipeline into
> > two, by
> > having two plane properties ("color_pipeline_pre_scale" and
> > "color_pipeline_post_scale") instead of a single one. This would be
> > similar to
> > the way we want to split pre-blending and post-blending. This could be less
> > expressive for drivers, there may be hardware where there are dependencies
> > between the pre- and post-scaling pipeline?]
> >
> As others have noted, breaking up the pipeline with immutable blocks
> makes the most sense to me here. This way we don't have to predict ahead
> of time every type of block that maybe affected by pipeline ordering.
> Splitting the pipeline into two properties now means future
> logical splits would require introduction of further plane

Re: [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers

2023-06-09 Thread Rob Herring

On Wed, Jun 07, 2023 at 02:49:26PM -0700, Douglas Anderson wrote:
> Inform fw_devlink of the fact that a panel follower (like a
> touchscreen) is effectively a consumer of the panel from the purposes
> of fw_devlink.
> 
> NOTE: this patch isn't required for correctness but instead optimizes
> probe order / helps avoid deferrals.
> 
> Signed-off-by: Douglas Anderson 
> ---
> Since this is so small, I'd presume it's OK for it to go through a DRM
> tree with the proper Ack. That being said, this patch is just an
> optimization and thus it could land completely separately from the
> rest and they could all meet up in mainline.
> 
> Changes in v2:
> - ("Add a devlink for panel followers") new for v2.
> 
>  drivers/of/property.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring

[PATCH v2 1/2] of: module: Export of_device_uevent()

The content of of_device_uevent() is currently hardcoded in a driver
that can be compiled as a module. Nothing prevents of_device_uevent() to
be exported to modules, most of the other helpers in of/device.c
actually are. The reason why this helper was not exported is because it
has been so far only useful in drivers/base, which is built-in anyway.

With the idea of getting rid of the hardcoded implementation of
of_device_uevent() in other places in the kernel, let's export it to GPL
modules (very much like its cousins in the same file).

Signed-off-by: Miquel Raynal 
---
 drivers/of/device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 0f00f1b80708..90131de6d75b 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -312,6 +312,7 @@ void of_device_uevent(const struct device *dev, struct 
kobj_uevent_env *env)
}
mutex_unlock(_mutex);
 }
+EXPORT_SYMBOL_GPL(of_device_uevent);
 
 int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env 
*env)
 {
-- 
2.34.1

[PATCH v2 2/2] gpu: host1x: Stop open-coding of_device_uevent()

There is apparently no reasons to open-code of_device_uevent() besides:
- The helper receives a struct device while we want to use the of_node
  member of the struct device *parent*.
- of_device_uevent() could not be called by modules because of a missing
  EXPORT_SYMBOL*().

In practice, the former point is not very constraining, just calling
of_device_uevent(dev->parent, ...) would have made the trick.

The latter point is more an observation rather than a real blocking
point because nothing prevented of_uevent() (called by the inline
function of_device_uevent()) to be exported to modules. In practice,
this helper is now exported, so nothing prevent us from using
of_device_uevent() anymore.

Let's use the core helper directly instead of open-coding it.

Cc: Thierry Reding 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Mikko Perttunen 
Cc: Rob Herring 
Cc: Frank Rowand 
Signed-off-by: Miquel Raynal 

---

This patch depends on the changes performed earlier in the series under
the drivers/of/ folder.
---
 drivers/gpu/host1x/bus.c | 29 ++---
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 4d16a3396c4a..dae589b83be1 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -338,32 +338,15 @@ static int host1x_device_match(struct device *dev, struct 
device_driver *drv)
return strcmp(dev_name(dev), drv->name) == 0;
 }
 
+/*
+ * Note that this is really only needed for backwards compatibility
+ * with libdrm, which parses this information from sysfs and will
+ * fail if it can't find the OF_FULLNAME, specifically.
+ */
 static int host1x_device_uevent(const struct device *dev,
struct kobj_uevent_env *env)
 {
-   struct device_node *np = dev->parent->of_node;
-   unsigned int count = 0;
-   struct property *p;
-   const char *compat;
-
-   /*
-* This duplicates most of of_device_uevent(), but the latter cannot
-* be called from modules and operates on dev->of_node, which is not
-* available in this case.
-*
-* Note that this is really only needed for backwards compatibility
-* with libdrm, which parses this information from sysfs and will
-* fail if it can't find the OF_FULLNAME, specifically.
-*/
-   add_uevent_var(env, "OF_NAME=%pOFn", np);
-   add_uevent_var(env, "OF_FULLNAME=%pOF", np);
-
-   of_property_for_each_string(np, "compatible", p, compat) {
-   add_uevent_var(env, "OF_COMPATIBLE_%u=%s", count, compat);
-   count++;
-   }
-
-   add_uevent_var(env, "OF_COMPATIBLE_N=%u", count);
+   of_device_uevent((const struct device *)>parent, env);
 
return 0;
 }
-- 
2.34.1

[PATCH v2 0/2] Small of/device cleanup

My previous attempt to slightly clean the OF core wrt device structures
was rather unsuccessful as the idea behind the discussed cleanup was
more impacting than what I thought, leading to most of the previous
series to be dropped. However, aside, two patches seemed actually
relevant, so here they are, alone.

Link: https://lore.kernel.org/all/20230608184903.ga3200973-r...@kernel.org/

Thanks,
Miquèl

Changes in v2:
* Dropped all the of_device.h/of_module.h changes
* Directly used of_device_uevent() from the host1x driver

Miquel Raynal (2):
  of: module: Export of_device_uevent()
  gpu: host1x: Stop open-coding of_device_uevent()

 drivers/gpu/host1x/bus.c | 29 ++---
 drivers/of/device.c  |  1 +
 2 files changed, 7 insertions(+), 23 deletions(-)

-- 
2.34.1

Re: [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens

2023-06-09 Thread Krzysztof Kozlowski

On 07/06/2023 23:49, Douglas Anderson wrote:
> As talked about in the patch ("drm/panel: Add a way for other devices
> to follow panel state"), touchscreens that are connected to panels are
> generally expected to be power sequenced together with the panel
> they're attached to. Today, nothing provides information allowing you
> to find out that a touchscreen is connected to a panel. Let's add a
> phandle for this.
> 
> The proerty is added to the generic touchscreen bindings and then
> enabled in the bindings for the i2c-hid backed devices. This can and
> should be added for other touchscreens in the future, but for now
> let's start small.
> 
> Signed-off-by: Douglas Anderson 
> ---
> 
> Changes in v2:
> - Move the description to the generic touchscreen.yaml.
> - Update the desc to make it clearer it's only for integrated devices.


Reviewed-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

Re: [RFC] Plane color pipeline KMS uAPI

2023-06-09 Thread Christopher Braga


Hi all,

The goal of this RFC is to expose a generic KMS uAPI to configure the color
pipeline before blending, ie. after a pixel is tapped from a plane's
framebuffer and before it's blended with other planes. With this new 
uAPI we

aim to reduce the battery life impact of color management and HDR on mobile
devices, to improve performance and to decrease latency by skipping
composition on the 3D engine. This proposal is the result of discussions at
the Red Hat HDR hackfest [1] which took place a few days ago. Engineers
familiar with the AMD, Intel and NVIDIA hardware have participated in the
discussion.

This proposal takes a prescriptive approach instead of a descriptive 
approach.

Drivers describe the available hardware blocks in terms of low-level
mathematical operations, then user-space configures each block. We decided
against a descriptive approach where user-space would provide a high-level
description of the colorspace and other parameters: we want to give more
control and flexibility to user-space, e.g. to be able to replicate 
exactly the

color pipeline with shaders and switch between shaders and KMS pipelines
seamlessly, and to avoid forcing user-space into a particular color 
management

policy.


Thanks for posting this Simon! This overview does a great job of
breaking down the proposal. A few questions inline below.


We've decided against mirroring the existing CRTC properties
DEGAMMA_LUT/CTM/GAMMA_LUT onto KMS planes. Indeed, the color management
pipeline can significantly differ between vendors and this approach cannot
accurately abstract all hardware. In particular, the availability, 
ordering and
capabilities of hardware blocks is different on each display engine. So, 
we've

decided to go for a highly detailed hardware capability discovery.

This new uAPI should not be in conflict with existing standard KMS 
properties,

since there are none which control the pre-blending color pipeline at the
moment. It does conflict with any vendor-specific properties like
NV_INPUT_COLORSPACE or the patches on the mailing list adding AMD-specific
properties. Drivers will need to either reject atomic commits 
configuring both
uAPIs, or alternatively we could add a DRM client cap which hides the 
vendor

properties and shows the new generic properties when enabled.

To use this uAPI, first user-space needs to discover hardware 
capabilities via
KMS objects and properties, then user-space can configure the hardware 
via an

atomic commit. This works similarly to the existing KMS uAPI, e.g. planes.

Our proposal introduces a new "color_pipeline" plane property, and a new 
KMS
object type, "COLOROP" (short for color operation). The "color_pipeline" 
plane
property is an enum, each enum entry represents a color pipeline 
supported by

the hardware. The special zero entry indicates that the pipeline is in
"bypass"/"no-op" mode. For instance, the following plane properties 
describe a

primary plane with 2 supported pipelines but currently configured in bypass
mode:

     Plane 10
     ├─ "type": immutable enum {Overlay, Primary, Cursor} = Primary
     ├─ …
     └─ "color_pipeline": enum {0, 42, 52} = 0

The non-zero entries describe color pipelines as a linked list of 
COLOROP KMS

objects. The entry value is an object ID pointing to the head of the linked
list (the first operation in the color pipeline).

The new COLOROP objects also expose a number of KMS properties. Each has a
type, a reference to the next COLOROP object in the linked list, and other
type-specific properties. Here is an example for a 1D LUT operation:

     Color operation 42
     ├─ "type": enum {Bypass, 1D curve} = 1D curve
     ├─ "1d_curve_type": enum {LUT, sRGB, PQ, BT.709, HLG, …} = LUT

The options sRGB / PQ / BT.709 / HLG would select hard-coded 1D
curves? Will different hardware be allowed to expose a subset of these 
enum values?



     ├─ "lut_size": immutable range = 4096
     ├─ "lut_data": blob
     └─ "next": immutable color operation ID = 43

Some hardware has per channel 1D LUT values, while others use the same 
LUT for all channels.  We will definitely need to expose this in the 
UAPI in some form.


To configure this hardware block, user-space can fill a KMS blob with 
4096 u32
entries, then set "lut_data" to the blob ID. Other color operation types 
might

have different properties.


The bit-depth of the LUT is an important piece of information we should
include by default. Are we assuming that the DRM driver will always
reduce the input values to the resolution supported by the pipeline? 
This could result in differences between the hardware behavior

and the shader behavior.

Additionally, some pipelines are floating point while others are fixed. 
How would user space know if it needs to pack 32 bit integer values vs

32 bit float values?


Here is another example with a 3D LUT:

     Color operation 42
     ├─ "type": enum {Bypass, 3D LUT} = 3D LUT
     ├─ "lut_size": immutable range = 33
     ├─ "lut_data":

Re: [PATCH v4 1/8] dt-bindings: display: mediatek: add MT8195 hdmi bindings

2023-06-09 Thread Guillaume Ranquet

On Thu, 08 Jun 2023 23:05, Rob Herring  wrote:
>On Mon, May 29, 2023 at 04:30:58PM +0200, Guillaume Ranquet wrote:
>> Add mt8195 SoC bindings for hdmi and hdmi-ddc
>>
>> On mt8195 the ddc i2c controller is part of the hdmi IP block and thus has no
>> specific register range, power domain or interrupt, making it simpler
>> than the legacy "mediatek,hdmi-ddc" binding.
>>
>> Signed-off-by: Guillaume Ranquet 
>> ---
>>  .../bindings/display/mediatek/mediatek,hdmi.yaml   | 59 
>> ++
>>  .../display/mediatek/mediatek,mt8195-hdmi-ddc.yaml | 45 +
>>  2 files changed, 93 insertions(+), 11 deletions(-)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml 
>> b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
>> index b90b6d18a828..4f62e6b94048 100644
>> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
>> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.yaml
>> @@ -21,6 +21,7 @@ properties:
>>- mediatek,mt7623-hdmi
>>- mediatek,mt8167-hdmi
>>- mediatek,mt8173-hdmi
>> +  - mediatek,mt8195-hdmi
>>
>>reg:
>>  maxItems: 1
>> @@ -29,18 +30,10 @@ properties:
>>  maxItems: 1
>>
>>clocks:
>> -items:
>> -  - description: Pixel Clock
>> -  - description: HDMI PLL
>> -  - description: Bit Clock
>> -  - description: S/PDIF Clock
>> +maxItems: 4
>>
>>clock-names:
>> -items:
>> -  - const: pixel
>> -  - const: pll
>> -  - const: bclk
>> -  - const: spdif
>> +maxItems: 4
>>
>>phys:
>>  maxItems: 1
>> @@ -58,6 +51,9 @@ properties:
>>  description: |
>>phandle link and register offset to the system configuration 
>> registers.
>>
>> +  power-domains:
>> +maxItems: 1
>> +
>>ports:
>>  $ref: /schemas/graph.yaml#/properties/ports
>>
>> @@ -86,9 +82,50 @@ required:
>>- clock-names
>>- phys
>>- phy-names
>> -  - mediatek,syscon-hdmi
>>- ports
>>
>> +allOf:
>> +  - if:
>> +  properties:
>> +compatible:
>> +  contains:
>> +const: mediatek,mt8195-hdmi
>> +then:
>> +  properties:
>> +clocks:
>> +  items:
>> +- description: APB
>> +- description: HDCP
>> +- description: HDCP 24M
>> +- description: Split HDMI
>> +clock-names:
>> +  items:
>> +- const: hdmi_apb_sel
>> +- const: hdcp_sel
>> +- const: hdcp24_sel
>> +- const: split_hdmi
>> +
>> +  required:
>> +- power-domains
>> +else:
>> +  properties:
>> +clocks:
>> +  items:
>> +- description: Pixel Clock
>> +- description: HDMI PLL
>> +- description: Bit Clock
>> +- description: S/PDIF Clock
>> +
>> +clock-names:
>> +  items:
>> +- const: pixel
>> +- const: pll
>> +- const: bclk
>> +- const: spdif
>
>I don't understand how the same h/w block can have completely different
>clocks. If not the same h/w or evolution of the same h/w, then do a
>separate schema.
>

Hi Rob,

I'm not entirely sure what's the best approach here.
The IPs are different enough to warrant a separate schema IMHO.
Though CK asked me to merge both IPs together (for both schema and code).

CK might want to chime in and advocate his point of view?

>> +
>> +  required:
>> +- mediatek,syscon-hdmi
>> +
>>  additionalProperties: false
>>
>>  examples:
>> diff --git 
>> a/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml
>>  
>> b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml
>> new file mode 100644
>> index ..84c096835b47
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: 
>> http://devicetree.org/schemas/display/mediatek/mediatek,mt8195-hdmi-ddc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Mediatek HDMI DDC for mt8195
>> +
>> +maintainers:
>> +  - CK Hu 
>> +  - Jitao shi 
>> +
>> +description: |
>> +  The HDMI DDC i2c controller is used to interface with the HDMI DDC pins.
>> +
>> +properties:
>> +  compatible:
>> +enum:
>> +  - mediatek,mt8195-hdmi-ddc
>> +
>> +  clocks:
>> +maxItems: 1
>> +
>> +  mediatek,hdmi:
>> +$ref: /schemas/types.yaml#/definitions/phandle
>> +description:
>> +  A phandle to the mt8195 hdmi controller
>> +
>> +required:
>> +  - compatible
>> +  - clocks
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +#include 
>> +#include 
>> +hdmiddc0: i2c {
>> +  compatible = "mediatek,mt8195-hdmi-ddc";
>> +  mediatek,hdmi = <>;
>> +  clocks = <>;
>
>How does

Re: [PATCH 0/6] drm: Add mouse cursor hotspot support to atomic KMS

2023-06-09 Thread Albert Esteve

On 6/10/22 10:59, Daniel Vetter wrote:

On Fri, Jun 10, 2022 at 10:41:05AM +0200, Daniel Vetter wrote:

Hi all,

Kinda top post because the thread is sprawling and I think we need a
summary/restart. I think there's at least 3 issues here:

- lack of hotspot property support, which means compositors can't really
support hotspot with atomic. Which isn't entirely true, because you
totally can use atomic for the primary planes/crtcs and the legacy
cursor ioctls, but I understand that people might find that a bit silly :-)

Anyway this problme is solved by the patch set here, and I think results
in some nice cleanups to boot.

- the fact that cursors for virtual drivers are not planes, but really
special things. Which just breaks the universal plane kms uapi. That
part isn't solved, and I do agree with Simon and Pekka that we really
should solve this before we unleash even more compositors onto the
atomic paths of virtual drivers.

I think the simplest solution for this is:
1. add a new DRM_PLANE_TYPE_VIRTUAL_CURSOR, and set that for these
special cursor planes on all virtual drivers
2. add the new "I understand virtual cursors planes" setparam, filter
virtual cursor planes for userspace which doesn't set this (like we do
right now if userspace doesn't set the universal plane mode)
3. backport the above patches to all stable kernels
4. make sure the hotspot property is only set on VIRTUAL_CURSOR planes
and nothing else in the rebased patch series

Simon also mentioned on irc that these special planes must not expose the
CRTC_X/Y property, since that doesn't really do much at all. Or is our
understanding of how this all works for commandeered cursors wrong?

- third issue: These special virtual display properties arent documented.
Aside from hotspot there's also suggested X/Y and maybe other stuff. I
have no idea what suggested X/Y does and what userspace should do with
it. I think we need a new section for virtualized drivers which:
- documents all the properties involved
- documents the new cap for enabling virtual cursor planes
- documents some of the key flows that compositors should implement for
best experience
- documents how exactly the user experience will degrade if compositors
pretend it's just a normal kms driver (maybe put that into each of the
special flows that a compositor ideally supports)
- whatever other comments and gaps I've missed, I'm sure
Simon/Pekka/others will chime in once the patch exists.

Great bonus would be an igt which demonstrates these flows. With the
interactive debug breakpoints to wait for resizing or whatever this should
be all neatly possible.
-Daniel

Hi all,

We have been testing the v2 of this patch and it works correctly for us.

First we tested with a patched Mutter, the mouse clicks were correct,
and behavior was as expected.

But I've also added an IGT test as suggested above:
https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/compare/master...modeset-cursor-hotspot-test?from_project_id=1274

I could post the IGT patch upstream once Zack's patches land.

Hope that helps!

-Albert

There's a bit of fixing oopsies (virtualized drivers really shouldn't have
enabled universal planes for their cursors) and debt (all these properties
predate the push to document stuff so we need to fix that), but I don't
think it's too much. And I think, from reading the threads, that this
should cover everything?

Anything I've missed? Or got completely wrong?

Cheers, Daniel

On Fri, Jun 03, 2022 at 02:14:59PM +, Simon Ser wrote:

Hi,

Please, read this thread:
https://lists.freedesktop.org/archives/dri-devel/2020-March/thread.html#259615

It has a lot of information about the pitfalls of cursor hotspot and
other things done by VM software.

In particular: since the driver will ignore the KMS cursor plane
position set by user-space, I don't think it's okay to just expose
without opt-in from user-space (e.g. with a DRM_CLIENT_CAP).

cc wayland-devel and Pekka for user-space feedback.

On Thursday, June 2nd, 2022 at 17:42, Zack Rusin wrote:

- all userspace code needs to hardcore a list of drivers which require
hotspots because there's no way to query from drm "does this driver
require hotspot"

Can you elaborate? I'm not sure I understand what you mean here.

Thanks,

Simon

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v17 1/1] drm/i915: Allow user to set cache at BO creation

2023-06-09 Thread Yang, Fei

> Hi Carl,
>
 besides this, ask a dumb question.
 How we retrieve the pat_index from a shared resource though dma_buf fd?
 maybe we need to know whether it could be CPU cached if we want map it.
 Of course, looks there are no real usage to access it though CPU.
 Just use it directly without any pat related options ?
>>>
>>> I am not understanding. Do you want to ask the PAT table to the driver? Are
>>> you referring to the CPU PAT index?
>>>
>>> In any case, if I understood correctly, you don't necessarily always need to
>>> set the PAT options and the cache options will fall into the default values.
>>>
>>> Please let me know if I haven't answered the question.
>>>
>>
>> If mesa create a resource , then use DRM_IOCTL_PRIME_HANDLE_TO_FD convert it 
>> to a dma fd.
>> Then share it to media, media use DRM_IOCTL_PRIME_FD_TO_HANDLE convert it to 
>> a gem bo.
>> But media does not know the PAT index , because mesa create it and set it.
>> So, if media want to call DRM_IOCTL_I915_GEM_MMAP_OFFSET, media does not 
>> know whether it could be WB.
>
> That's a good point. To be honest I am not really sure how this
> is handled.
>
> Fei, Jordan? Do you have suggestion here?

Is it possible to pass the PAT setting when sharing the fd?
Or perhaps we should have kept the get_caching ioctl functional?
Joonas, could you chime in here?

> Andi

[PATCH] drm: atmel-hlcdc: Support inverting the pixel clock polarity

On the SoC host controller, the pixel clock can be:
* standard: data is launched on the rising edge
* inverted: data is launched on the falling edge

Some panels may need the inverted option to be used so let's support
this DRM flag.

Signed-off-by: Miquel Raynal 
---

Hello, this change was tested on a Sama5d36 based custom board with a
panel not behaving correctly if the pixel clock was not inverted.
Cheers, Miquèl

 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 25 +++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 58184cd6ab0b..cc5cf4c2faf7 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -68,7 +68,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc 
*c)
struct atmel_hlcdc_crtc *crtc = drm_crtc_to_atmel_hlcdc_crtc(c);
struct regmap *regmap = crtc->dc->hlcdc->regmap;
struct drm_display_mode *adj = >state->adjusted_mode;
+   struct drm_encoder *encoder = NULL, *en_iter;
+   struct drm_connector *connector = NULL;
struct atmel_hlcdc_crtc_state *state;
+   struct drm_device *ddev = c->dev;
+   struct drm_connector_list_iter iter;
unsigned long mode_rate;
struct videomode vm;
unsigned long prate;
@@ -76,6 +80,23 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc 
*c)
unsigned int cfg = 0;
int div, ret;
 
+   /* get encoder from crtc */
+   drm_for_each_encoder(en_iter, ddev) {
+   if (en_iter->crtc == c) {
+   encoder = en_iter;
+   break;
+   }
+   }
+
+   if (encoder) {
+   /* Get the connector from encoder */
+   drm_connector_list_iter_begin(ddev, );
+   drm_for_each_connector_iter(connector, )
+   if (connector->encoder == encoder)
+   break;
+   drm_connector_list_iter_end();
+   }
+
ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk);
if (ret)
return;
@@ -134,6 +155,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc 
*c)
 
cfg |= ATMEL_HLCDC_CLKDIV(div);
 
+   if (connector &&
+   connector->display_info.bus_flags & 
DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+   cfg |= ATMEL_HLCDC_CLKPOL;
+
regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0), mask, cfg);
 
state = drm_crtc_state_to_atmel_hlcdc_crtc_state(c->state);
-- 
2.34.1

Re: [RESEND 11/15] drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef

2023-06-09 Thread Hamza Mahfooz


On 6/9/23 04:17, Lee Jones wrote:

Fixes the following W=1 kernel build warning(s):

  drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:48:22: 
warning: ‘SYNAPTICS_DEVICE_ID’ defined but not used [-Wunused-const-variable=]

Cc: Harry Wentland 
Cc: Leo Li 
Cc: Rodrigo Siqueira 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: amd-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Lee Jones 


Applied, thanks!


---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 09e056a647087..cd20cfc049969 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -44,9 +44,6 @@
  #include "dm_helpers.h"
  #include "ddc_service_types.h"
  
-/* MST Dock */

-static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA";
-
  /* dm_helpers_parse_edid_caps
   *
   * Parse edid caps
@@ -702,6 +699,9 @@ static void apply_synaptics_fifo_reset_wa(struct drm_dp_aux 
*aux)
DC_LOG_DC("Done apply_synaptics_fifo_reset_wa\n");
  }
  
+/* MST Dock */

+static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA";
+
  static uint8_t write_dsc_enable_synaptics_non_virtual_dpcd_mst(
struct drm_dp_aux *aux,
const struct dc_stream_state *stream,

--
Hamza

Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower

2023-06-09 Thread Doug Anderson

Hi,

On Fri, Jun 9, 2023 at 2:27 AM Benjamin Tissoires
 wrote:
>
> > I suspect that it's not worth it, but I'll do this if you feel
> > strongly about it.
> >
> > I guess the simplest way I can think of to move this to its own file
> > would be to put the whole private data structure (struct i2c_hid) in a
> > private header file and then add prototypes for i2c_hid_core_resume()
> > and i2c_hid_core_suspend() there. Then I could add something like
> > i2c_hid_core_handle_panel_follower() that would have all the
> > registration logic. I'd still need special cases in the core
> > suspend/resume/remove code unless I add a level of abstraction. While
> > the level of abstraction is more "pure", it also would make the code
> > harder to follow.
> >
> > Unless I hear a strong opinion (or if this series changes
> > significantly), I'll plan to keep things in the same file and just use
> > a Kconfig.
> >
>
> Right, a separate file might not be the best then :(
>
> Do you envision this to be used on the ACPI side of i2c-hid? Because
> if this is OF only, then maybe it would be interesting to put it there
> (in i2c-hid-of.c), instead of having it in the core. IIRC i2c-hid-of
> also has ways to set up/down regulators, so maybe it'll be better
> there?

There is no reason why this problem would be limited to devices using
devicetree. Even if ACPI could somehow magically power sequence the
touchscreen and panel together, doing it behind the back of the kernel
driver would be a bad idea anyway so folks using ACPI would need the
same code. I don't have tons of experience with ACPI nor how to hook
this up there, but I purposely made the API for registering the panel
follower such that the client doesn't pass anything devicetree
specific. If someone could figure out how to detect a link between a
panel and a touchscreen for ACPI and add this code to
drm_panel_add_follower() then it would automatically work for the ACPI
case as well.

-Doug

[PATCH 7/7] drm/panel: sitronix-st7789v: Check display ID

A very basic debugging rule when a device is connected for the first
time is to access a read-only register which contains known data in
order to ensure the communication protocol is properly working. This
driver lacked any read helper which is often a critical peace for
fastening bring-ups.

Add a read helper and use it to verify the communication with the panel
is working as soon as possible in order to fail early if this is not the
case.

Signed-off-by: Miquel Raynal 
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c| 78 +++
 1 file changed, 78 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c 
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 7de192a3a8aa..fb21d52a7a63 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -113,6 +113,9 @@
return val; \
} while (0)
 
+#define ST7789V_IDS { 0x85, 0x85, 0x52 }
+#define ST7789V_IDS_SIZE 3
+
 struct st7789v_panel_info {
const struct drm_display_mode *display_mode;
u16 width_mm;
@@ -165,6 +168,77 @@ static int st7789v_write_data(struct st7789v *ctx, u8 cmd)
return st7789v_spi_write(ctx, ST7789V_DATA, cmd);
 }
 
+static int st7789v_read_data(struct st7789v *ctx, u8 cmd, u8 *buf,
+unsigned int len)
+{
+   struct spi_transfer xfer[2] = { };
+   struct spi_message msg;
+   u16 txbuf = ((ST7789V_COMMAND & 1) << 8) | cmd;
+   u16 rxbuf[4] = {};
+   u8 bit9 = 0;
+   int ret, i;
+
+   switch (len) {
+   case 1:
+   case 3:
+   case 4:
+   break;
+   default:
+   return -EOPNOTSUPP;
+   }
+
+   spi_message_init();
+
+   xfer[0].tx_buf = 
+   xfer[0].len = sizeof(txbuf);
+   spi_message_add_tail([0], );
+
+   xfer[1].rx_buf = rxbuf;
+   xfer[1].len = len * 2;
+   spi_message_add_tail([1], );
+
+   ret = spi_sync(ctx->spi, );
+   if (ret)
+   return ret;
+
+   for (i = 0; i < len; i++) {
+   buf[i] = rxbuf[i] >> i | (bit9 << (9 - i));
+   if (i)
+   bit9 = rxbuf[i] & GENMASK(i - 1, 0);
+   }
+
+   return 0;
+}
+
+static int st7789v_check_id(struct drm_panel *panel)
+{
+   const u8 st7789v_ids[ST7789V_IDS_SIZE] = ST7789V_IDS;
+   struct st7789v *ctx = panel_to_st7789v(panel);
+   bool invalid_ids = false;
+   int ret, i;
+   u8 ids[3];
+
+   ret = st7789v_read_data(ctx, MIPI_DCS_GET_DISPLAY_ID, ids, 
ST7789V_IDS_SIZE);
+   if (ret) {
+   dev_err(panel->dev, "Failed to read IDs\n");
+   return ret;
+   }
+
+   for (i = 0; i < ST7789V_IDS_SIZE; i++) {
+   if (ids[i] != st7789v_ids[i]) {
+   invalid_ids = true;
+   break;
+   }
+   }
+
+   if (invalid_ids) {
+   dev_err(panel->dev, "Unrecognized panel IDs");
+   return -EIO;
+   }
+
+   return 0;
+}
+
 static const struct drm_display_mode default_mode = {
.clock = 7000,
.hdisplay = 240,
@@ -237,6 +311,10 @@ static int st7789v_prepare(struct drm_panel *panel)
gpiod_set_value(ctx->reset, 0);
msleep(120);
 
+   ret = st7789v_check_id(panel);
+   if (ret)
+   return ret;
+
ST7789V_TEST(ret, st7789v_write_command(ctx, MIPI_DCS_EXIT_SLEEP_MODE));
 
/* We need to wait 120ms after a sleep out command */
-- 
2.34.1

[PATCH 2/7] drm/panel: sitronix-st7789v: Use 9 bits per spi word by default

The Sitronix controller expects 9-bit words, provide this as default at
probe time rather than specifying this in each and every access.

Signed-off-by: Miquel Raynal 
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c 
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index c67b9adb157f..e9ca7ebb458a 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -138,7 +138,6 @@ static int st7789v_spi_write(struct st7789v *ctx, enum 
st7789v_prefix prefix,
spi_message_init();
 
xfer.tx_buf = 
-   xfer.bits_per_word = 9;
xfer.len = sizeof(txbuf);
 
spi_message_add_tail(, );
@@ -365,6 +364,13 @@ static int st7789v_probe(struct spi_device *spi)
spi_set_drvdata(spi, ctx);
ctx->spi = spi;
 
+   spi->bits_per_word = 9;
+   ret = spi_setup(spi);
+   if (ret < 0) {
+   dev_err(>dev, "spi_setup failed: %d\n", ret);
+   return ret;
+   }
+
drm_panel_init(>panel, >dev, _drm_funcs,
   DRM_MODE_CONNECTOR_DPI);
 
-- 
2.34.1

[PATCH 3/7] drm/panel: sitronix-st7789v: Specify the expected bus format

The LCD controller supports RGB444, RGB565 and RGB888. The value that is
written in the COLMOD register indicates using RGB888, so let's clearly
specify the in-use bus format.

Signed-off-by: Miquel Raynal 
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c 
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index e9ca7ebb458a..0abb45bea18d 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -6,6 +6,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -170,6 +171,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
 struct drm_connector *connector)
 {
struct drm_display_mode *mode;
+   u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 
mode = drm_mode_duplicate(connector->dev, _mode);
if (!mode) {
@@ -186,6 +188,8 @@ static int st7789v_get_modes(struct drm_panel *panel,
 
connector->display_info.width_mm = 61;
connector->display_info.height_mm = 103;
+   drm_display_info_set_bus_formats(>display_info,
+_format, 1);
 
return 1;
 }
-- 
2.34.1

[PATCH 5/7] dt-bindings: display: st7789v: Add the edt, et028013dma panel compatible

The ST7789V LCD controller is also embedded in the ET028013DMA panel. In
fact, "sitronix,st7789v" might not be totally relevant alone as most of
the time -if not all- the LCD controller will always be packaged into a
display with its own physical properties.

Let's keep "sitronix,st7789v" valid alone for backward compatibility,
but we should definitely provide two compatibles to fully describe such
panel, so let's expect to have both when describing a panel such as the
EDT ET028013DMA.

Signed-off-by: Miquel Raynal 
---
 .../bindings/display/panel/sitronix,st7789v.yaml   | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git 
a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml 
b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
index d984b59daa4a..d4c8af9a973d 100644
--- a/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
+++ b/Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml
@@ -15,7 +15,12 @@ allOf:
 
 properties:
   compatible:
-const: sitronix,st7789v
+oneOf:
+  - items:
+  - enum:
+  - edt,et028013dma
+  - const: sitronix,st7789v
+  - const: sitronix,st7789v
 
   reg: true
   reset-gpios: true
-- 
2.34.1

[PATCH 6/7] drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support

This panel from Emerging Display Technologies Corporation features an
ST7789V2 panel inside which is almost identical to what the Sitronix
panel driver supports.

In practice, the module physical size is specific, and experiments show
that the display will malfunction if any of the following situation
occurs:
* Pixel clock is above 3MHz
* Pixel clock is not inverted
I could not properly identify the reasons behind these failures, scope
captures show valid input signals.

Signed-off-by: Miquel Raynal 
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c| 34 +--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c 
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 212bccc31804..7de192a3a8aa 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -30,7 +30,8 @@
 #define ST7789V_RGBCTRL_RCM(n) (((n) & 3) << 5)
 #define ST7789V_RGBCTRL_VSYNC_HIGH BIT(3)
 #define ST7789V_RGBCTRL_HSYNC_HIGH BIT(2)
-#define ST7789V_RGBCTRL_PCLK_HIGH  BIT(1)
+#define ST7789V_RGBCTRL_PCLK_FALLING   BIT(1)
+#define ST7789V_RGBCTRL_PCLK_RISING0
 #define ST7789V_RGBCTRL_VBP(n) ((n) & 0x7f)
 #define ST7789V_RGBCTRL_HBP(n) ((n) & 0x1f)
 
@@ -117,6 +118,7 @@ struct st7789v_panel_info {
u16 width_mm;
u16 height_mm;
u32 bus_format;
+   u32 bus_flags;
 };
 
 struct st7789v {
@@ -175,6 +177,18 @@ static const struct drm_display_mode default_mode = {
.vtotal = 320 + 8 + 4 + 4,
 };
 
+static const struct drm_display_mode slow_mode = {
+   .clock = 3000,
+   .hdisplay = 240,
+   .hsync_start = 240 + 38,
+   .hsync_end = 240 + 38 + 10,
+   .htotal = 240 + 38 + 10 + 10,
+   .vdisplay = 320,
+   .vsync_start = 320 + 8,
+   .vsync_end = 320 + 8 + 4,
+   .vtotal = 320 + 8 + 4 + 4,
+};
+
 static int st7789v_get_modes(struct drm_panel *panel,
 struct drm_connector *connector)
 {
@@ -197,6 +211,7 @@ static int st7789v_get_modes(struct drm_panel *panel,
 
connector->display_info.width_mm = panel_info->width_mm;
connector->display_info.height_mm = panel_info->height_mm;
+   connector->display_info.bus_flags = panel_info->bus_flags;
drm_display_info_set_bus_formats(>display_info,
 _info->bus_format, 1);
 
@@ -206,8 +221,13 @@ static int st7789v_get_modes(struct drm_panel *panel,
 static int st7789v_prepare(struct drm_panel *panel)
 {
struct st7789v *ctx = panel_to_st7789v(panel);
+   const struct st7789v_panel_info *panel_info = ctx->panel_info;
+   u8 pck = ST7789V_RGBCTRL_PCLK_FALLING;
int ret;
 
+   if (panel_info->bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
+   pck = ST7789V_RGBCTRL_PCLK_RISING;
+
ret = regulator_enable(ctx->power);
if (ret)
return ret;
@@ -321,7 +341,7 @@ static int st7789v_prepare(struct drm_panel *panel)
 ST7789V_RGBCTRL_RCM(2) |
 ST7789V_RGBCTRL_VSYNC_HIGH |
 ST7789V_RGBCTRL_HSYNC_HIGH |
-ST7789V_RGBCTRL_PCLK_HIGH));
+pck));
ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_HBP(20)));
 
@@ -422,14 +442,24 @@ static const struct st7789v_panel_info st7789v_info = {
.bus_format = MEDIA_BUS_FMT_RGB666_1X18,
 };
 
+static const struct st7789v_panel_info et028013dma_info = {
+   .display_mode = _mode,
+   .width_mm = 43,
+   .height_mm = 58,
+   .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+   .bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+};
+
 static const struct of_device_id st7789v_of_match[] = {
{ .compatible = "sitronix,st7789v", .data = _info },
+   { .compatible = "edt,et028013dma", .data = _info },
{ }
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
 
 static const struct spi_device_id st7789v_ids[] = {
{ "st7789v", },
+   { "et028013dma", },
{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, st7789v_ids);
-- 
2.34.1

[PATCH 1/7] drm/panel: sitronix-st7789v: Prevent core spi warnings

The spi core warns us about using an of_device_id table without a
spi_device_id table aside for module utilities in orter to not rely on
OF modaliases. Just add this table using the device name without the
vendor prefix (as it is usually done).

Signed-off-by: Miquel Raynal 
---
 drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c 
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index bbc4569cbcdc..c67b9adb157f 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -400,9 +400,16 @@ static const struct of_device_id st7789v_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
 
+static const struct spi_device_id st7789v_ids[] = {
+   { "st7789v", },
+   { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, st7789v_ids);
+
 static struct spi_driver st7789v_driver = {
.probe = st7789v_probe,
.remove = st7789v_remove,
+   .id_table = st7789v_ids,
.driver = {
.name = "st7789v",
.of_match_table = st7789v_of_match,
-- 
2.34.1

[PATCH 4/7] drm/panel: sitronix-st7789v: Use platform data

The Sitronix ST7789V LCD controller is actually packaged in a number of
different panels with slightly different properties. Before introducing
the support for another pannel using this same LCD controller, let's
move all the panel-specific information into a dedicated structure that
is available as platform data.

There is no functional change.

Signed-off-by: Miquel Raynal 
---
 .../gpu/drm/panel/panel-sitronix-st7789v.c| 30 +++
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c 
b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
index 0abb45bea18d..212bccc31804 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
@@ -112,11 +112,19 @@
return val; \
} while (0)
 
+struct st7789v_panel_info {
+   const struct drm_display_mode *display_mode;
+   u16 width_mm;
+   u16 height_mm;
+   u32 bus_format;
+};
+
 struct st7789v {
struct drm_panel panel;
struct spi_device *spi;
struct gpio_desc *reset;
struct regulator *power;
+   const struct st7789v_panel_info *panel_info;
 };
 
 enum st7789v_prefix {
@@ -170,10 +178,11 @@ static const struct drm_display_mode default_mode = {
 static int st7789v_get_modes(struct drm_panel *panel,
 struct drm_connector *connector)
 {
+   struct st7789v *ctx = panel_to_st7789v(panel);
+   const struct st7789v_panel_info *panel_info = ctx->panel_info;
struct drm_display_mode *mode;
-   u32 bus_format = MEDIA_BUS_FMT_RGB666_1X18;
 
-   mode = drm_mode_duplicate(connector->dev, _mode);
+   mode = drm_mode_duplicate(connector->dev, panel_info->display_mode);
if (!mode) {
dev_err(panel->dev, "failed to add mode %ux%ux@%u\n",
default_mode.hdisplay, default_mode.vdisplay,
@@ -186,10 +195,10 @@ static int st7789v_get_modes(struct drm_panel *panel,
mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
drm_mode_probed_add(connector, mode);
 
-   connector->display_info.width_mm = 61;
-   connector->display_info.height_mm = 103;
+   connector->display_info.width_mm = panel_info->width_mm;
+   connector->display_info.height_mm = panel_info->height_mm;
drm_display_info_set_bus_formats(>display_info,
-_format, 1);
+_info->bus_format, 1);
 
return 1;
 }
@@ -365,6 +374,8 @@ static int st7789v_probe(struct spi_device *spi)
if (!ctx)
return -ENOMEM;
 
+   ctx->panel_info = device_get_match_data(>dev);
+
spi_set_drvdata(spi, ctx);
ctx->spi = spi;
 
@@ -404,8 +415,15 @@ static void st7789v_remove(struct spi_device *spi)
drm_panel_remove(>panel);
 }
 
+static const struct st7789v_panel_info st7789v_info = {
+   .display_mode = _mode,
+   .width_mm = 64,
+   .height_mm = 103,
+   .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
 static const struct of_device_id st7789v_of_match[] = {
-   { .compatible = "sitronix,st7789v" },
+   { .compatible = "sitronix,st7789v", .data = _info },
{ }
 };
 MODULE_DEVICE_TABLE(of, st7789v_of_match);
-- 
2.34.1

[PATCH 0/7] drm/panel: sitronix-st7789v: Support ET028013DMA panel

Hello,

The aim of this series is to add support for the EDT ET028013DMA
panel. This panel features a Sitronix ST7789V2 LCD controller, which is
already supported mainline (or very close to the ST7789V for which
Maxime added support years ago).

The EDT panel is slightly different on the geometry and appears not to
support refresh rates higher than 30fps (above, glitches are visible,
despite the incoming signals being rather clean). While I was working on
this panel, I found quite inconvenient to not be able to read anything
back as it is a great tool for debugging purposes. So the last patch
actually adds a read helper and uses it to perform a sanity check at
probe time by verifying the Sitronix controller IDs. If deemed
irrelevant, this patch may be discarded.

Thanks,
Miquèl

Miquel Raynal (7):
  drm/panel: sitronix-st7789v: Prevent core spi warnings
  drm/panel: sitronix-st7789v: Use 9 bits per spi word by default
  drm/panel: sitronix-st7789v: Specify the expected bus format
  drm/panel: sitronix-st7789v: Use platform data
  dt-bindings: display: st7789v: Add the edt,et028013dma panel
compatible
  drm/panel: sitronix-st7789v: Add EDT ET028013DMA panel support
  drm/panel: sitronix-st7789v: Check display ID

 .../display/panel/sitronix,st7789v.yaml   |   7 +-
 .../gpu/drm/panel/panel-sitronix-st7789v.c| 157 +-
 2 files changed, 156 insertions(+), 8 deletions(-)

-- 
2.34.1

Re: [RESEND 11/15] drm/amd/display/amdgpu_dm/amdgpu_dm_helpers: Move SYNAPTICS_DEVICE_ID into CONFIG_DRM_AMD_DC_DCN ifdef

2023-06-09 Thread Harry Wentland

On 6/9/23 04:17, Lee Jones wrote:
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:48:22: 
> warning: ‘SYNAPTICS_DEVICE_ID’ defined but not used [-Wunused-const-variable=]
> 
> Cc: Harry Wentland 
> Cc: Leo Li 
> Cc: Rodrigo Siqueira 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "Pan, Xinhui" 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Lee Jones 

Reviewed-by: Harry Wentland 

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 09e056a647087..cd20cfc049969 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -44,9 +44,6 @@
>  #include "dm_helpers.h"
>  #include "ddc_service_types.h"
>  
> -/* MST Dock */
> -static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA";
> -
>  /* dm_helpers_parse_edid_caps
>   *
>   * Parse edid caps
> @@ -702,6 +699,9 @@ static void apply_synaptics_fifo_reset_wa(struct 
> drm_dp_aux *aux)
>   DC_LOG_DC("Done apply_synaptics_fifo_reset_wa\n");
>  }
>  
> +/* MST Dock */
> +static const uint8_t SYNAPTICS_DEVICE_ID[] = "SYNA";
> +
>  static uint8_t write_dsc_enable_synaptics_non_virtual_dpcd_mst(
>   struct drm_dp_aux *aux,
>   const struct dc_stream_state *stream,

Re: Adreno devfreq lockdep splat with 6.3-rc2

2023-06-09 Thread Rob Clark

On Thu, Jun 8, 2023 at 11:17 PM Johan Hovold  wrote:
>
> On Thu, Jun 08, 2023 at 02:17:45PM -0700, Rob Clark wrote:
> > On Thu, Jun 8, 2023 at 7:12 AM Johan Hovold  wrote:
>
> > > Have you had a chance to look at this regression yet? It prevents us
> > > from using lockdep on the X13s as it is disabled as soon as we start
> > > the GPU.
> >
> > Hmm, curious what is different between x13s and sc7180/sc7280 things?
>
> It seems like lockdep needs to hit the tear down path in order to
> detect the circular lock dependency. Perhaps you don't hit that on your
> sc7180/sc7280?
>
> It is due to the fact that the panel is looked up way too late so that
> bind fails unless the panel driver is already loaded when the msm drm
> driver probes.

Oh, this seems likely

> Manually loading the panel driver before msm makes the splat go away.
>
> > Or did lockdep recently get more clever (or more annotation)?
>
> I think this is indeed a new problem related to some of the devfreq work
> you did in 6.3-rc1 (e.g. fadcc3ab1302 ("drm/msm/gpu: Bypass PM QoS
> constraint for idle clamp")).
>
> > I did spend some time a while back trying to bring some sense to
> > devfreq/pm-qos/icc locking:
> > https://patchwork.freedesktop.org/series/115028/
> >
> > but haven't had time to revisit that for a while
>
> That's the series I link to below, but IIRC it did not look directly
> applicable to the splat I see on X13s (e.g. does not involve
> fs_reclaim).

Ahh, right, sorry I've not had time to do more than glance at the
thread.. and yeah, that one is mostly just trying to solve the reclaim
problem by moving allocations out from under the big-pm-qos-lock.

As far as fadcc3ab1302 ("drm/msm/gpu: Bypass PM QoS constraint for
idle clamp"), it should be just taking the lock that
dev_pm_qos_update_request() would have indirectly, although I guess
without some intervening lock?  We can't really avoid taking the
devfreq lock, I don't think.  But I'd have to spend time I don't have
right now digging into it..

BR,
-R

> > > On Wed, Mar 15, 2023 at 10:19:21AM +0100, Johan Hovold wrote:
> > > >
> > > > Since 6.3-rc2 (or possibly -rc1), I'm now seeing the below
> > > > devfreq-related lockdep splat.
> > > >
> > > > I noticed that you posted a fix for something similar here:
> > > >
> > > >   
> > > > https://lore.kernel.org/r/20230312204150.1353517-9-robdcl...@gmail.com
> > > >
> > > > but that particular patch makes no difference.
> > > >
> > > > From skimming the calltraces below and qos/devfreq related changes in
> > > > 6.3-rc1 it seems like this could be related to:
> > > >
> > > >   fadcc3ab1302 ("drm/msm/gpu: Bypass PM QoS constraint for idle 
> > > > clamp")
>
> Johan

Re: [PATCH v8 2/2] soc: mediatek: remove DDP_DOMPONENT_DITHER from enum

2023-06-09 Thread Matthias Brugger





On 31/05/2023 09:43, Chen-Yu Tsai wrote:

Hi Matthias,

On Mon, Mar 6, 2023 at 4:07 PM Jason-JH.Lin  wrote:


After mmsys and drm change DITHER enum to DDP_COMPONENT_DITHER0,
mmsys header can remove the useless DDP_COMPONENT_DITHER enum.

Signed-off-by: Jason-JH.Lin 
Reviewed-by: AngeloGioacchino Del Regno 

Reviewed-by: Rex-BC Chen 
Acked-by: Matthias Brugger 


CK didn't pick up this patch. Since the other patch already got picked up
in v6.4-rc1, could you merge this for v6.5?



Yes, I gave an acked-by as I thought that CK will take both of them. Anyway 
applied now.


Matthias



Thanks
ChenYu



---
  include/linux/soc/mediatek/mtk-mmsys.h | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/soc/mediatek/mtk-mmsys.h 
b/include/linux/soc/mediatek/mtk-mmsys.h
index dc2963a0a0f7..8eb5846985b4 100644
--- a/include/linux/soc/mediatek/mtk-mmsys.h
+++ b/include/linux/soc/mediatek/mtk-mmsys.h
@@ -27,8 +27,7 @@ enum mtk_ddp_comp_id {
 DDP_COMPONENT_CCORR,
 DDP_COMPONENT_COLOR0,
 DDP_COMPONENT_COLOR1,
-   DDP_COMPONENT_DITHER,
-   DDP_COMPONENT_DITHER0 = DDP_COMPONENT_DITHER,
+   DDP_COMPONENT_DITHER0,
 DDP_COMPONENT_DITHER1,
 DDP_COMPONENT_DP_INTF0,
 DDP_COMPONENT_DP_INTF1,
--
2.18.0

Re: [RESEND 05/15] drm/mediatek/mtk_disp_ccorr: Remove half completed incorrect struct header

2023-06-09 Thread Matthias Brugger





On 09/06/2023 10:17, Lee Jones wrote:

Fixes the following W=1 kernel build warning(s):

  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or 
member 'clk' not described in 'mtk_disp_ccorr'
  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or 
member 'regs' not described in 'mtk_disp_ccorr'
  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or 
member 'cmdq_reg' not described in 'mtk_disp_ccorr'
  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:47: warning: Function parameter or 
member 'data' not described in 'mtk_disp_ccorr'

Cc: Chun-Kuang Hu 
Cc: Philipp Zabel 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Matthias Brugger 
Cc: AngeloGioacchino Del Regno 
Cc: dri-devel@lists.freedesktop.org
Cc: linux-media...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Signed-off-by: Lee Jones 
---
  drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 5 -
  1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c 
b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
index 1773379b24398..720f3c7ef7b4f 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
@@ -34,11 +34,6 @@ struct mtk_disp_ccorr_data {
u32 matrix_bits;
  };
  
-/**

- * struct mtk_disp_ccorr - DISP_CCORR driver structure
- * @ddp_comp - structure containing type enum and hardware resources
- * @crtc - associated crtc to report irq events to
- */


That surely suppress the warning but I think the correct to do here is to fix 
the documentation instead of deleting it.


Regards,
Matthias


  struct mtk_disp_ccorr {
struct clk *clk;
void __iomem *regs;

Re: [RFC PATCH] drm/sched: Wait for the currently popped dependency in kill_jobs_cb()

2023-06-09 Thread Christian König


Am 09.06.23 um 16:10 schrieb Boris Brezillon:

Hello Christian,

On Fri, 9 Jun 2023 13:53:59 +0200
Christian König  wrote:


Am 08.06.23 um 08:55 schrieb Boris Brezillon:

If I understand correctly, drm_sched_entity_kill_jobs_cb() is supposed
to wait on all the external dependencies (those added to
drm_sched_job::dependencies) before signaling the job finished fence.
This is done this way to prevent jobs depending on these canceled jobs
from considering the resources they want to access as ready, when
they're actually still used by other components, thus leading to
potential data corruptions.

The problem is, the kill_jobs logic is omitting the last fence popped
from the dependencies array that was waited upon before
drm_sched_entity_kill() was called (drm_sched_entity::dependency field),
so we're basically waiting for all dependencies except one.

This is an attempt at fixing that, but also an opportunity to make sure
I understood the drm_sched_entity_kill(), because I'm not 100% sure if
skipping this currently popped dependency was intentional or not. I can't
see a good reason why we'd want to skip that one, but I might be missing
something.

Signed-off-by: Boris Brezillon 
Cc: Frank Binns 
Cc: Sarah Walker 
Cc: Donald Robson 
Cc: Luben Tuikov 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Sumit Semwal 
Cc: "Christian König" 
---
Stumbled across this issue while working on native dependency support
with Donald (which will be posted soon). Flagged as RFC, because I'm
not sure this is legit, and also not sure we want to fix it this way.
I tried re-using drm_sched_entity::dependency, but it's a bit of a mess
because of the asynchronousity of the wait, and the fact we use
drm_sched_entity::dependency to know if we have a clear_dep()
callback registered, so we can easily reset it without removing the
callback.

Well yes, that's a known problem. But this is really not the right
approach to fixing this.

Trying to wait for all the dependencies before killing jobs was added
because of the way we kept track of dma_fences in dma_resv objects.
Basically adding exclusive fences removed all other fences leading to a
bit fragile memory management.

Okay.


This handling was removed by now and so the workaround for waiting for
dependencies is not really necessary any more, but I think it is still
better to do so.

The right approach of getting this waiting for dependencies completely
straight is also not to touch entity->dependency in any way, but to stop
removing them from the XA in drm_sched_job_dependency(). Otherwise you
don't catch the pipeline optimized ones either.

Do you want me to post a v2 doing that, or should I forget about it?
If we decide to keep things like that, it might be good to at least
add a comment explaining why we don't care.


Well, if you have time fixing this fully and keeping the dependency 
handling is certainly be preferred.


Regards,
Christian.



Regards,

Boris

Re: [PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj




On 09/06/2023 13:44, Iddamsetty, Aravind wrote:

On 09-06-2023 17:41, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which
will return a reference to a newly created GEM objects (if created), in
order to enable tracking of imported i915 GEM objects in the following
patch.


instead of this what if we implement the open call back in i915

struct drm_gem_object_funcs {

 /**
  * @open:
  *
  * Called upon GEM handle creation.
  *
  * This callback is optional.
  */
 int (*open)(struct drm_gem_object *obj, struct drm_file *file);

which gets called whenever a handle(drm_gem_handle_create_tail) is
created and in the open we can check if to_intel_bo(obj)->base.dma_buf
then it is imported if not it is owned or created by it.


I wanted to track as much memory usage as we have which is buffer object 
backed, including page tables and contexts. And since those are not user 
visible (they don't have handles), they wouldn't be covered by the 
obj->funcs->open() callback.


If we wanted to limit to objects with handles we could simply do what 
Rob proposed and that is to walk the handles idr. But that does not feel 
like the right direction to me. Open for discussion I guess.


Regards,

Tvrtko

Re: [RFC PATCH] drm/sched: Wait for the currently popped dependency in kill_jobs_cb()

2023-06-09 Thread Boris Brezillon

Hello Christian,

On Fri, 9 Jun 2023 13:53:59 +0200
Christian König  wrote:

> Am 08.06.23 um 08:55 schrieb Boris Brezillon:
> > If I understand correctly, drm_sched_entity_kill_jobs_cb() is supposed
> > to wait on all the external dependencies (those added to
> > drm_sched_job::dependencies) before signaling the job finished fence.
> > This is done this way to prevent jobs depending on these canceled jobs
> > from considering the resources they want to access as ready, when
> > they're actually still used by other components, thus leading to
> > potential data corruptions.
> >
> > The problem is, the kill_jobs logic is omitting the last fence popped
> > from the dependencies array that was waited upon before
> > drm_sched_entity_kill() was called (drm_sched_entity::dependency field),
> > so we're basically waiting for all dependencies except one.
> >
> > This is an attempt at fixing that, but also an opportunity to make sure
> > I understood the drm_sched_entity_kill(), because I'm not 100% sure if
> > skipping this currently popped dependency was intentional or not. I can't
> > see a good reason why we'd want to skip that one, but I might be missing
> > something.
> >
> > Signed-off-by: Boris Brezillon 
> > Cc: Frank Binns 
> > Cc: Sarah Walker 
> > Cc: Donald Robson 
> > Cc: Luben Tuikov 
> > Cc: David Airlie 
> > Cc: Daniel Vetter 
> > Cc: Sumit Semwal 
> > Cc: "Christian König" 
> > ---
> > Stumbled across this issue while working on native dependency support
> > with Donald (which will be posted soon). Flagged as RFC, because I'm
> > not sure this is legit, and also not sure we want to fix it this way.
> > I tried re-using drm_sched_entity::dependency, but it's a bit of a mess
> > because of the asynchronousity of the wait, and the fact we use
> > drm_sched_entity::dependency to know if we have a clear_dep()
> > callback registered, so we can easily reset it without removing the
> > callback.  
> 
> Well yes, that's a known problem. But this is really not the right 
> approach to fixing this.
> 
> Trying to wait for all the dependencies before killing jobs was added 
> because of the way we kept track of dma_fences in dma_resv objects. 
> Basically adding exclusive fences removed all other fences leading to a 
> bit fragile memory management.

Okay.

> 
> This handling was removed by now and so the workaround for waiting for 
> dependencies is not really necessary any more, but I think it is still 
> better to do so.
> 
> The right approach of getting this waiting for dependencies completely 
> straight is also not to touch entity->dependency in any way, but to stop 
> removing them from the XA in drm_sched_job_dependency(). Otherwise you 
> don't catch the pipeline optimized ones either.

Do you want me to post a v2 doing that, or should I forget about it?
If we decide to keep things like that, it might be good to at least
add a comment explaining why we don't care.

Regards,

Boris

[PATCH] dt-bindings: gpu: drop unneeded quotes

2023-06-09 Thread Krzysztof Kozlowski

Cleanup bindings dropping unneeded quotes. Once all these are fixed,
checking for this can be enabled in yamllint.

Signed-off-by: Krzysztof Kozlowski 
---
 Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 2 +-
 Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml 
b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
index 0400a361875d..e796a1ff8c82 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
@@ -86,7 +86,7 @@ properties:
 const: 2
 
   dynamic-power-coefficient:
-$ref: '/schemas/types.yaml#/definitions/uint32'
+$ref: /schemas/types.yaml#/definitions/uint32
 description:
   A u32 value that represents the running time dynamic
   power coefficient in units of uW/MHz/V^2. The
diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml 
b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml
index 2a25384ca3ef..ca02baba5526 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml
@@ -92,7 +92,7 @@ properties:
   dma-coherent: true
 
   dynamic-power-coefficient:
-$ref: '/schemas/types.yaml#/definitions/uint32'
+$ref: /schemas/types.yaml#/definitions/uint32
 description:
   A u32 value that represents the running time dynamic
   power coefficient in units of uW/MHz/V^2. The
-- 
2.34.1

Re: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled




On 09/06/2023 13:52, Christian König wrote:

Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin:


On 09/06/2023 07:32, Christian König wrote:

Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin:

From: Tvrtko Ursulin 

For dma_fence_is_signaled signaling critical path annotations are an
annoying cause of false positives when using dma_fence_is_signaled and
indirectly higher level helpers such as dma_resv_test_signaled etc.

Drop the critical path annotation since the "is signaled" API does not
guarantee to ever change the signaled status anyway.

We do that by adding a low level _dma_fence_signal helper and use it 
from

dma_fence_is_signaled.


I have been considering dropping the signaling from the 
dma_fence_is_signaled() function altogether.


Doing this together with the spin locking we have in the dma_fence is 
just utterly nonsense since the purpose of the external spinlock is 
to keep the signaling in order while this here defeats that.


The quick check is ok I think, but signaling the dma_fence and 
issuing the callbacks should always come from the interrupt handler.


What do you think is broken with regards to ordering with the current 
code? The unlocked fast check?


Well it's not broken, the complexity just doesn't make sense.

The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t 
itself. That was done to make sure that all dma_fence objects from a 
single context (or in other words hardware device) signal in the order 
of their sequence number, e.g. 1 first, then 2, then 3 etc...


But when somebody uses the dma_fence_is_signaled() function it's 
perfectly possible that this races with an interrupt handler which wants 
to signal fences on another CPU.


In other words we get:
CPU A:
dma_fence_is_signaled(fence with seqno=3)
fence->ops->signaled() returns true
dma_fence_signal()
spin_lock_irqsave(fence->lock)
signal seqno=3
...

CPU B:
device_driver_interrupt_handler()
spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete
signal seqno=1
signal seqno=2
seqno=3 is already signaled.
signal seqno=4
...


Right I see. However hm.. would the order of be guaranteed anyway, if 
someone would be observing what CPU B is doing via the 
dma_fence_is_signaled->test_bit? And in which scenarios would it matter 
if out of order signaled status could be observed?


Either fence->lock should not be a pointer or we should not signal the 
fence from dma_fence_is_signaled().


I strongly think that later should be the way to go.


Despite having wrote the above, I don't have any objections to removing 
this either. I don't see anything in the contract that requires it, but 
it was probably a bit before my time to know why it was added so I don't 
know if it could cause any subtle issues. It should be okay to try and see.


Regards,

Tvrtko

[PATCH] drm/radeon: Disable outputs when releasing fbdev client

2023-06-09 Thread Thomas Zimmermann

Disable the modesetting pipeline before release the radeon's fbdev
client. Fixes the following error:

[   17.217408] WARNING: CPU: 5 PID: 1464 at drivers/gpu/drm/ttm/ttm_bo.c:326 
ttm_bo_release+0x27e/0x2d0 [ttm]
[   17.217418] Modules linked in: edac_mce_amd radeon(+) drm_ttm_helper ttm 
video drm_suballoc_helper drm_display_helper kvm irqbypass drm_kms_helper 
syscopyarea crc32_pclmul sysfillrect sha512_ssse3 sysimgblt sha512_generic 
cfbfillrect cfbimgblt wmi_bmof aesni_intel cfbcopyarea crypto_simd cryptd 
k10temp acpi_cpufreq wmi dm_mod
[   17.217432] CPU: 5 PID: 1464 Comm: systemd-udevd Not tainted 6.4.0-rc4+ #1
[   17.217436] Hardware name: Micro-Star International Co., Ltd. MS-7A38/B450M 
PRO-VDH MAX (MS-7A38), BIOS B.G0 07/26/2022
[   17.217438] RIP: 0010:ttm_bo_release+0x27e/0x2d0 [ttm]
[   17.217444] Code: 48 89 43 38 48 89 43 40 48 8b 5c 24 30 48 8b b5 40 08 00 
00 48 8b 6c 24 38 48 83 c4 58 e9 7a 49 f7 e0 48 89 ef e9 6c fe ff ff <0f> 0b 48 
83 7b 20 00 0f 84 b7 fd ff ff 0f 0b 0f 1f 00 e9 ad fd ff
[   17.217448] RSP: 0018:c995fbb0 EFLAGS: 00010202
[   17.217451] RAX: 0001 RBX: 8881052c8de0 RCX: 
[   17.217453] RDX: 0001 RSI:  RDI: 8881052c8de0
[   17.217455] RBP: 888104a66e00 R08: 8881052c8de0 R09: 888104a7cf08
[   17.217457] R10: c995fbe0 R11: c995fbe8 R12: 8881052c8c78
[   17.217458] R13: 8881052c8c78 R14: dead0100 R15: 88810528b108
[   17.217460] FS:  7f319fcbb8c0() GS:1a54() 
knlGS:
[   17.217463] CS:  0010 DS:  ES:  CR0: 80050033
[   17.217464] CR2: 55dc8b0224a0 CR3: 00010373d000 CR4: 00750ee0
[   17.217466] PKRU: 5554
[   17.217468] Call Trace:
[   17.217470]  
[   17.217472]  ? __warn+0x97/0x160
[   17.217476]  ? ttm_bo_release+0x27e/0x2d0 [ttm]
[   17.217481]  ? report_bug+0x1ec/0x200
[   17.217487]  ? handle_bug+0x3c/0x70
[   17.217490]  ? exc_invalid_op+0x1f/0x90
[   17.217493]  ? preempt_count_sub+0xb5/0x100
[   17.217496]  ? asm_exc_invalid_op+0x16/0x20
[   17.217500]  ? ttm_bo_release+0x27e/0x2d0 [ttm]
[   17.217505]  ? ttm_resource_move_to_lru_tail+0x1ab/0x1d0 [ttm]
[   17.217511]  radeon_bo_unref+0x1a/0x30 [radeon]
[   17.217547]  radeon_gem_object_free+0x20/0x30 [radeon]
[   17.217579]  radeon_fbdev_fb_destroy+0x57/0x90 [radeon]
[   17.217616]  unregister_framebuffer+0x72/0x110
[   17.217620]  drm_client_dev_unregister+0x6d/0xe0
[   17.217623]  drm_dev_unregister+0x2e/0x90
[   17.217626]  drm_put_dev+0x26/0x90
[   17.217628]  pci_device_remove+0x44/0xc0
[   17.217631]  really_probe+0x257/0x340
[   17.217635]  __driver_probe_device+0x73/0x120
[   17.217638]  driver_probe_device+0x2c/0xb0
[   17.217641]  __driver_attach+0xa0/0x150
[   17.217643]  ? __pfx___driver_attach+0x10/0x10
[   17.217646]  bus_for_each_dev+0x67/0xa0
[   17.217649]  bus_add_driver+0x10e/0x210
[   17.217651]  driver_register+0x5c/0x120
[   17.217653]  ? __pfx_radeon_module_init+0x10/0x10 [radeon]
[   17.217681]  do_one_initcall+0x44/0x220
[   17.217684]  ? kmalloc_trace+0x37/0xc0
[   17.217688]  do_init_module+0x64/0x240
[   17.217691]  __do_sys_finit_module+0xb2/0x100
[   17.217694]  do_syscall_64+0x3b/0x90
[   17.217697]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[   17.217700] RIP: 0033:0x7f319feaa5a9
[   17.217702] Code: 08 89 e8 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 90 48 89 
f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 
f0 ff ff 73 01 c3 48 8b 0d 27 08 0d 00 f7 d8 64 89 01 48
[   17.217706] RSP: 002b:7ffc6bf3e7f8 EFLAGS: 0246 ORIG_RAX: 
0139
[   17.217709] RAX: ffda RBX: 5607204f3170 RCX: 7f319feaa5a9
[   17.217710] RDX:  RSI: 7f31a002eefd RDI: 0018
[   17.217712] RBP: 7f31a002eefd R08:  R09: 5607204f1860
[   17.217714] R10: 0018 R11: 0246 R12: 0002
[   17.217716] R13:  R14: 560720522450 R15: 560720255899
[   17.217718]  
[   17.217719] ---[ end trace  ]---

The buffer object backing the fbdev emulation got pinned twice: by the
fb_probe helper radeon_fbdev_create_pinned_object() and the modesetting
code when the framebuffer got displayed. It only got unpinned once by
the fbdev helper radeon_fbdev_destroy_pinned_object(). Hence TTM's BO-
release function complains about the pin counter. Forcing the outputs
off also undoes the modesettings pin increment.

Reported-by: Borislav Petkov 
Closes: 
https://lore.kernel.org/dri-devel/20230603174814.GCZHt83pN+wNjf63sC@fat_crate.local/
Signed-off-by: Thomas Zimmermann 
Fixes: e317a69fe891 ("drm/radeon: Implement client-based fbdev emulation")
Cc: Alex Deucher 
Cc: Thomas Zimmermann 
Cc: "Christian König" 
Cc: "Pan, Xinhui" 
Cc: amd-...@lists.freedesktop.org
---
 drivers/gpu/drm/radeon/radeon_fbdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git

[PATCH v2 1/4] drm/todo: Add atomic modesetting references

The section about converting existing KMS drivers to atomic modesetting
mentions the existence of a conversion guide, but does not reference it.
While the guide is old and rusty, it still contains useful information,
so add a link to it.  Also link to the LWN.net articles that give an
overview about the atomic mode setting design.

While at it, remove the reference to unconverted virtual HW drivers, as
they've been converted.

Signed-off-by: Geert Uytterhoeven 
Reviewed-by: Javier Martinez Canillas 
Reviewed-by: Laurent Pinchart 
---
v2:
  - Add Reviewed-by,
  - Drop double space after full stop,
  - Use footnotes for references,
  - Remore reference to unconverted virtual HW drivers.
---
 Documentation/gpu/todo.rst | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 68bdafa0284f55f6..6c328613c049fc1d 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -49,14 +49,18 @@ converted over. Modern compositors like Wayland or 
Surfaceflinger on Android
 really want an atomic modeset interface, so this is all about the bright
 future.
 
-There is a conversion guide for atomic and all you need is a GPU for a
-non-converted driver (again virtual HW drivers for KVM are still all
-suitable).
+There is a conversion guide for atomic [1]_ and all you need is a GPU for a
+non-converted driver.  The "Atomic mode setting design overview" series [2]_
+[3]_ at LWN.net can also be helpful.
 
 As part of this drivers also need to convert to universal plane (which means
 exposing primary & cursor as proper plane objects). But that's much easier to
 do by directly using the new atomic helper driver callbacks.
 
+  .. [1] 
https://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html
+  .. [2] https://lwn.net/Articles/653071/
+  .. [3] https://lwn.net/Articles/653466/
+
 Contact: Daniel Vetter, respective driver maintainers
 
 Level: Advanced
-- 
2.34.1

[PATCH v2 0/4] drm: Atomic modesetting doc and comment improvements

Hi all,

This patch series contains various improvements to the documentation and
comments related to atomic modesetting.  Hopefully, it will ease the job
of DRM novice who want to tackle the daunting task of converting a
legacy DRM driver to atomic modesetting.

Changes compared to 1:
  - Add Reviewed-by,
  - Drop double space after full stop,
  - Use footnotes for references,
  - Remore reference to unconverted virtual HW drivers,
  - New patch [2/4],
  - Drop "first part" in drivers/gpu/drm/drm_plane_helper.c.

Thanks for your comments!

Geert Uytterhoeven (4):
  drm/todo: Add atomic modesetting references
  drm/todo: Convert list of fbconv links to footnotes
  drm: Remove references to removed transitional helpers
  drm: Fix references to drm_plane_helper_check_state()

 Documentation/gpu/todo.rst   | 20 ++
 drivers/gpu/drm/drm_plane_helper.c   | 12 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c  |  3 +-
 drivers/gpu/drm/tidss/tidss_plane.c  |  3 +-
 include/drm/drm_crtc.h   |  5 ---
 include/drm/drm_modeset_helper_vtables.h | 48 +++-
 6 files changed, 39 insertions(+), 52 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH v2 3/4] drm: Remove references to removed transitional helpers

The transitional helpers were removed a long time ago, but some
references stuck.  Remove them.

Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
Signed-off-by: Geert Uytterhoeven 
---
v2:
  - Drop "first part" in drivers/gpu/drm/drm_plane_helper.c.
---
 drivers/gpu/drm/drm_plane_helper.c   | 12 +-
 include/drm/drm_crtc.h   |  5 ---
 include/drm/drm_modeset_helper_vtables.h | 48 +++-
 3 files changed, 23 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c 
b/drivers/gpu/drm/drm_plane_helper.c
index c91e454eba097942..5e95089676ff81ed 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -40,8 +40,8 @@
 /**
  * DOC: overview
  *
- * This helper library has two parts. The first part has support to implement
- * primary plane support on top of the normal CRTC configuration interface.
+ * This helper library contains helpers to implement primary plane support on
+ * top of the normal CRTC configuration interface.
  * Since the legacy _mode_config_funcs.set_config interface ties the 
primary
  * plane together with the CRTC state this does not allow userspace to disable
  * the primary plane itself. The default primary plane only expose XRBG and
@@ -51,14 +51,6 @@
  * planes, and newly merged drivers must not rely upon these transitional
  * helpers.
  *
- * The second part also implements transitional helpers which allow drivers to
- * gradually switch to the atomic helper infrastructure for plane updates. Once
- * that switch is complete drivers shouldn't use these any longer, instead 
using
- * the proper legacy implementations for update and disable plane hooks 
provided
- * by the atomic helpers.
- *
- * Again drivers are strongly urged to switch to the new interfaces.
- *
  * The plane helpers share the function table structures with other helpers,
  * specifically also the atomic helpers. See  drm_plane_helper_funcs for
  * the details.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8e1cbc75143ef216..8b48a1974da3143c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -77,11 +77,6 @@ struct drm_plane_helper_funcs;
  * intended to indicate whether a full modeset is needed, rather than strictly
  * describing what has changed in a commit. See also:
  * drm_atomic_crtc_needs_modeset()
- *
- * WARNING: Transitional helpers (like drm_helper_crtc_mode_set() or
- * drm_helper_crtc_mode_set_base()) do not maintain many of the derived control
- * state like @plane_mask so drivers not converted over to atomic helpers 
should
- * not rely on these being accurate!
  */
 struct drm_crtc_state {
/** @crtc: backpointer to the CRTC */
diff --git a/include/drm/drm_modeset_helper_vtables.h 
b/include/drm/drm_modeset_helper_vtables.h
index 965faf082a6d1acb..e3c3ac615909474b 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -59,8 +59,8 @@ enum mode_set_atomic {
 /**
  * struct drm_crtc_helper_funcs - helper operations for CRTCs
  *
- * These hooks are used by the legacy CRTC helpers, the transitional plane
- * helpers and the new atomic modesetting helpers.
+ * These hooks are used by the legacy CRTC helpers and the new atomic
+ * modesetting helpers.
  */
 struct drm_crtc_helper_funcs {
/**
@@ -216,9 +216,7 @@ struct drm_crtc_helper_funcs {
 *
 * This callback is used to update the display mode of a CRTC without
 * changing anything of the primary plane configuration. This fits the
-* requirement of atomic and hence is used by the atomic helpers. It is
-* also used by the transitional plane helpers to implement a
-* @mode_set hook in drm_helper_crtc_mode_set().
+* requirement of atomic and hence is used by the atomic helpers.
 *
 * Note that the display pipe is completely off when this function is
 * called. Atomic drivers which need hardware to be running before they
@@ -333,8 +331,8 @@ struct drm_crtc_helper_funcs {
 * all updated. Again the recommendation is to just call check helpers
 * until a maximal configuration is reached.
 *
-* This callback is used by the atomic modeset helpers and by the
-* transitional plane helpers, but it is optional.
+* This callback is used by the atomic modeset helpers, but it is
+* optional.
 *
 * NOTE:
 *
@@ -373,8 +371,8 @@ struct drm_crtc_helper_funcs {
 * has picked. See drm_atomic_helper_commit_planes() for a discussion of
 * the tradeoffs and variants of plane commit helpers.
 *
-* This callback is used by the atomic modeset helpers and by the
-* transitional plane helpers, but it is optional.
+* This callback is used by the atomic modeset helpers, but it is
+* optional.
 */
void (*atomic_begin)(struct drm_crtc *crtc,

[PATCH v2 4/4] drm: Fix references to drm_plane_helper_check_state()

As of commit a01cb8ba3f628293 ("drm: Move drm_plane_helper_check_state()
into drm_atomic_helper.c"), drm_plane_helper_check_state() no longer
exists, but is part of drm_atomic_helper_check_plane_state().

Signed-off-by: Geert Uytterhoeven 
Reviewed-by: Laurent Pinchart 
---
v2:
  - Add Reviewed-by.
---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++-
 drivers/gpu/drm/tidss/tidss_plane.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c 
b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index d759e019218181ce..e445fac8e0b46c21 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -600,7 +600,8 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
if (!state->crtc) {
/*
 * The visible field is not reset by the DRM core but only
-* updated by drm_plane_helper_check_state(), set it manually.
+* updated by drm_atomic_helper_check_plane_state(), set it
+* manually.
 */
state->visible = false;
*format = NULL;
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c 
b/drivers/gpu/drm/tidss/tidss_plane.c
index 6bdd6e4a955ab3cc..e1c0ef0c3894c855 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -38,7 +38,8 @@ static int tidss_plane_atomic_check(struct drm_plane *plane,
if (!new_plane_state->crtc) {
/*
 * The visible field is not reset by the DRM core but only
-* updated by drm_plane_helper_check_state(), set it manually.
+* updated by drm_atomic_helper_check_plane_state(), set it
+* manually.
 */
new_plane_state->visible = false;
return 0;
-- 
2.34.1

[PATCH v2 2/4] drm/todo: Convert list of fbconv links to footnotes

Convert the references to fbconv links to footnotes, so they can be
navigated.

Signed-off-by: Geert Uytterhoeven 
---
v2:
  - New.
---
 Documentation/gpu/todo.rst | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 6c328613c049fc1d..ce1d4e22c327063b 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -753,16 +753,16 @@ existing hardware. The new driver's call-back functions 
are filled from
 existing fbdev code.
 
 More complex fbdev drivers can be refactored step-by-step into a DRM
-driver with the help of the DRM fbconv helpers. [1] These helpers provide
+driver with the help of the DRM fbconv helpers [4]_. These helpers provide
 the transition layer between the DRM core infrastructure and the fbdev
 driver interface. Create a new DRM driver on top of the fbconv helpers,
 copy over the fbdev driver, and hook it up to the DRM code. Examples for
-several fbdev drivers are available at [1] and a tutorial of this process
-available at [2]. The result is a primitive DRM driver that can run X11
+several fbdev drivers are available at [4]_ and a tutorial of this process
+available at [5]_. The result is a primitive DRM driver that can run X11
 and Weston.
 
- - [1] https://gitlab.freedesktop.org/tzimmermann/linux/tree/fbconv
- - [2] 
https://gitlab.freedesktop.org/tzimmermann/linux/blob/fbconv/drivers/gpu/drm/drm_fbconv_helper.c
+ .. [4] https://gitlab.freedesktop.org/tzimmermann/linux/tree/fbconv
+ .. [5] 
https://gitlab.freedesktop.org/tzimmermann/linux/blob/fbconv/drivers/gpu/drm/drm_fbconv_helper.c
 
 Contact: Thomas Zimmermann 
 
-- 
2.34.1

Re: [PATCH] accel/habanalabs: add more debugfs stub helpers

2023-06-09 Thread Tomer Tayar

On 09/06/2023 15:06, Arnd Bergmann wrote:
> From: Arnd Bergmann 
>
> Two functions got added with normal prototypes for debugfs, but not
> alternative when building without it:
>
> drivers/accel/habanalabs/common/device.c: In function 'hl_device_init':
> drivers/accel/habanalabs/common/device.c:2177:14: error: implicit declaration 
> of function 'hl_debugfs_device_init'; did you mean 'hl_debugfs_init'? 
> [-Werror=implicit-function-declaration]
> drivers/accel/habanalabs/common/device.c:2305:9: error: implicit declaration 
> of function 'hl_debugfs_device_fini'; did you mean 'hl_debugfs_remove_file'? 
> [-Werror=implicit-function-declaration]
>
> Add stubs for these as well.
>
> Fixes: 553311fc7b76e ("accel/habanalabs: expose debugfs files later")
> Signed-off-by: Arnd Bergmann 

Thanks,
Reviewed-by: Tomer Tayar 

> ---
>   drivers/accel/habanalabs/common/habanalabs.h | 9 +
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/accel/habanalabs/common/habanalabs.h 
> b/drivers/accel/habanalabs/common/habanalabs.h
> index d92ba2e30e310..2f027d5a82064 100644
> --- a/drivers/accel/habanalabs/common/habanalabs.h
> +++ b/drivers/accel/habanalabs/common/habanalabs.h
> @@ -3980,6 +3980,15 @@ static inline void hl_debugfs_fini(void)
>   {
>   }
>   
> +static inline int hl_debugfs_device_init(struct hl_device *hdev)
> +{
> + return 0;
> +}
> +
> +static inline void hl_debugfs_device_fini(struct hl_device *hdev)
> +{
> +}
> +
>   static inline void hl_debugfs_add_device(struct hl_device *hdev)
>   {
>   }

Re: [PATCH v10 9/9] drm: Remove superfluous print statements in DRM core

2023-06-09 Thread Siddh Raman Pant

On Wed, 07 Jun 2023 00:00:10 +0530, Siddh Raman Pant wrote:
> There are a couple of superfluous print statements using the drm_*
> macros, which do stuff like printing newlines, print OOM messages
> (OOM while allocating memory is already supposed to be noisy), and
> printing strings like "Initialised" with no extra info whatsoever.
> 
> Thus, remove a couple of these superfluous strings.
> 
> Suggested-by: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com>
> Signed-off-by: Siddh Raman Pant c...@siddh.me>

This patch is the only one introducing these changes additionally,
no other patch content has been changed from v9 and sent for merge.
I should have clarified this in the cover.

This patch may or may not be dropped, courtesy follow-up discussion on
v9 regarding it: https://lore.kernel.org/lkml/87jzwfu1wf@intel.com/

Thanks,
Siddh

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

2023-06-09 Thread Simon Ser

Hi,

On Friday, June 9th, 2023 at 13:31, Thomas Zimmermann  
wrote:

> Is there a v3 of this patchset? It was Acked with the one errno code
> changed.

Since this was a minor change, I did it locally and pushed the patch
to drm-misc-next already.

Simon

Re: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled

2023-06-09 Thread Christian König


Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin:


On 09/06/2023 07:32, Christian König wrote:

Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin:

From: Tvrtko Ursulin 

For dma_fence_is_signaled signaling critical path annotations are an
annoying cause of false positives when using dma_fence_is_signaled and
indirectly higher level helpers such as dma_resv_test_signaled etc.

Drop the critical path annotation since the "is signaled" API does not
guarantee to ever change the signaled status anyway.

We do that by adding a low level _dma_fence_signal helper and use it 
from

dma_fence_is_signaled.


I have been considering dropping the signaling from the 
dma_fence_is_signaled() function altogether.


Doing this together with the spin locking we have in the dma_fence is 
just utterly nonsense since the purpose of the external spinlock is 
to keep the signaling in order while this here defeats that.


The quick check is ok I think, but signaling the dma_fence and 
issuing the callbacks should always come from the interrupt handler.


What do you think is broken with regards to ordering with the current 
code? The unlocked fast check?


Well it's not broken, the complexity just doesn't make sense.

The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t 
itself. That was done to make sure that all dma_fence objects from a 
single context (or in other words hardware device) signal in the order 
of their sequence number, e.g. 1 first, then 2, then 3 etc...


But when somebody uses the dma_fence_is_signaled() function it's 
perfectly possible that this races with an interrupt handler which wants 
to signal fences on another CPU.


In other words we get:
CPU A:
dma_fence_is_signaled(fence with seqno=3)
fence->ops->signaled() returns true
dma_fence_signal()
spin_lock_irqsave(fence->lock)
signal seqno=3
...

CPU B:
device_driver_interrupt_handler()
spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete
signal seqno=1
signal seqno=2
seqno=3 is already signaled.
signal seqno=4
...

Either fence->lock should not be a pointer or we should not signal the 
fence from dma_fence_is_signaled().


I strongly think that later should be the way to go.

Regards,
Christian.



Regards,

Tvrtko



Christian.



Signed-off-by: Tvrtko Ursulin 
Cc: Christian König 
Cc: Daniel Vetter 
---
  drivers/dma-buf/dma-fence.c | 26 --
  include/linux/dma-fence.h   |  3 ++-
  2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index f177c56269bb..ace1415f0566 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -444,6 +444,25 @@ int dma_fence_signal_locked(struct dma_fence 
*fence)

  }
  EXPORT_SYMBOL(dma_fence_signal_locked);
+/**
+ * __dma_fence_signal - signal completion of a fence bypassing 
critical section annotation

+ * @fence: the fence to signal
+ *
+ * This low-level helper should not be used by code external to 
dma-fence.h|c!

+ */
+int _dma_fence_signal(struct dma_fence *fence)
+{
+    unsigned long flags;
+    int ret;
+
+    spin_lock_irqsave(fence->lock, flags);
+    ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
+    spin_unlock_irqrestore(fence->lock, flags);
+
+    return ret;
+}
+EXPORT_SYMBOL(_dma_fence_signal);
+
  /**
   * dma_fence_signal - signal completion of a fence
   * @fence: the fence to signal
@@ -459,7 +478,6 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
   */
  int dma_fence_signal(struct dma_fence *fence)
  {
-    unsigned long flags;
  int ret;
  bool tmp;
@@ -467,11 +485,7 @@ int dma_fence_signal(struct dma_fence *fence)
  return -EINVAL;
  tmp = dma_fence_begin_signalling();
-
-    spin_lock_irqsave(fence->lock, flags);
-    ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
-    spin_unlock_irqrestore(fence->lock, flags);
-
+    ret = _dma_fence_signal(fence);
  dma_fence_end_signalling(tmp);
  return ret;
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index d54b595a0fe0..d94768ad70e4 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -387,6 +387,7 @@ static inline void dma_fence_end_signalling(bool 
cookie) {}

  static inline void __dma_fence_might_wait(void) {}
  #endif
+int _dma_fence_signal(struct dma_fence *fence);
  int dma_fence_signal(struct dma_fence *fence);
  int dma_fence_signal_locked(struct dma_fence *fence);
  int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t 
timestamp);

@@ -452,7 +453,7 @@ dma_fence_is_signaled(struct dma_fence *fence)
  return true;
  if (fence->ops->signaled && fence->ops->signaled(fence)) {
-    dma_fence_signal(fence);
+    _dma_fence_signal(fence);
  return true;
  }

Re: [PATCH 2/3] drm: Remove references to removed transitional helpers

On Fri, Jun 2, 2023 at 1:17 PM Geert Uytterhoeven  wrote:
> On Fri, Jun 2, 2023 at 1:05 PM Laurent Pinchart
>  wrote:
> > On Fri, Jun 02, 2023 at 11:11:35AM +0200, Geert Uytterhoeven wrote:
> > > The transitional helpers were removed a long time ago, but some
> > > references stuck.  Remove them.
> > >
> > > Fixes: 21ebe615c16994f3 ("drm: Remove transitional helpers")
> > > Signed-off-by: Geert Uytterhoeven 
>
> > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > @@ -51,14 +51,6 @@
> > >   * planes, and newly merged drivers must not rely upon these transitional
> > >   * helpers.
> > >   *
> >
> > The first paragraph starts with "This helper library has two parts.". As
> > you're dropping the mention of the second part, I think you should
> > rework the first paragraph too.
>
> That was my initial thought, too.
> However, the code still has a second part, not related to the topic of
> the first part (primary plane support).

Upon deeper investigation, all of this is related to primary plane
support. Will update...

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj

2023-06-09 Thread Iddamsetty, Aravind




On 09-06-2023 17:41, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which
> will return a reference to a newly created GEM objects (if created), in
> order to enable tracking of imported i915 GEM objects in the following
> patch.

instead of this what if we implement the open call back in i915

struct drm_gem_object_funcs {

/**
 * @open:
 *
 * Called upon GEM handle creation.
 *
 * This callback is optional.
 */
int (*open)(struct drm_gem_object *obj, struct drm_file *file);

which gets called whenever a handle(drm_gem_handle_create_tail) is
created and in the open we can check if to_intel_bo(obj)->base.dma_buf
then it is imported if not it is owned or created by it.

Thanks,
Aravind.

> 
> Minor code reshuffule and only trivial additions on top of
> drm_gem_prime_fd_to_handle.
> 
> Signed-off-by: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/drm_prime.c | 41 -
>  include/drm/drm_prime.h |  4 
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index d29dafce9bb0..ef75f67e3057 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -284,11 +284,12 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
>  EXPORT_SYMBOL(drm_gem_dmabuf_release);
>  
>  /**
> - * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> + * drm_gem_prime_fd_to_handle_obj - PRIME import function for GEM drivers
>   * @dev: drm_device to import into
>   * @file_priv: drm file-private structure
>   * @prime_fd: fd id of the dma-buf which should be imported
>   * @handle: pointer to storage for the handle of the imported buffer object
> + * @objp: optional pointer in which reference to created GEM object can be 
> returned
>   *
>   * This is the PRIME import function which must be used mandatorily by GEM
>   * drivers to ensure correct lifetime management of the underlying GEM 
> object.
> @@ -297,9 +298,10 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
>   *
>   * Returns 0 on success or a negative error code on failure.
>   */
> -int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> -struct drm_file *file_priv, int prime_fd,
> -uint32_t *handle)
> +int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev,
> +struct drm_file *file_priv, int prime_fd,
> +uint32_t *handle,
> +struct drm_gem_object **objp)
>  {
>   struct dma_buf *dma_buf;
>   struct drm_gem_object *obj;
> @@ -336,7 +338,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  
>   /* _handle_create_tail unconditionally unlocks dev->object_name_lock. */
>   ret = drm_gem_handle_create_tail(file_priv, obj, handle);
> - drm_gem_object_put(obj);
> + if (!objp)
> + drm_gem_object_put(obj);
>   if (ret)
>   goto out_put;
>  
> @@ -348,6 +351,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  
>   dma_buf_put(dma_buf);
>  
> + if (objp)
> + *objp = obj;
> +
>   return 0;
>  
>  fail:
> @@ -356,6 +362,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>*/
>   drm_gem_handle_delete(file_priv, *handle);
>   dma_buf_put(dma_buf);
> + if (objp)
> + drm_gem_object_put(obj);
>   return ret;
>  
>  out_unlock:
> @@ -365,6 +373,29 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>   dma_buf_put(dma_buf);
>   return ret;
>  }
> +EXPORT_SYMBOL(drm_gem_prime_fd_to_handle_obj);
> +
> +/**
> + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> + * @dev: drm_device to import into
> + * @file_priv: drm file-private structure
> + * @prime_fd: fd id of the dma-buf which should be imported
> + * @handle: pointer to storage for the handle of the imported buffer object
> + *
> + * This is the PRIME import function which must be used mandatorily by GEM
> + * drivers to ensure correct lifetime management of the underlying GEM 
> object.
> + * The actual importing of GEM object from the dma-buf is done through the
> + * _driver.gem_prime_import driver callback.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> +struct drm_file *file_priv, int prime_fd,
> +uint32_t *handle)
> +{
> + return drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, handle,
> +   NULL);
> +}
>  EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
>  
>  int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 2a1d01e5b56b..10d145ce6586 100644
>

Re: [PATCH v5 3/4] PCI/VGA: Tidy up the code and comment format


Hi,

On 2023/6/9 20:00, Andi Shyti wrote:

Hi Sui,

On Fri, Jun 09, 2023 at 07:24:16PM +0800, Sui Jingfeng wrote:

This patch replaces the leading space with a tab and removes the double
blank line, no functional change.

You mainly fixed comment style, though and it's not written here.

No need to resend for me as you also wrote it in the title. But
next time please list all the changes in the commit log.


OK, I remember this.

This patch is trivial, that's fine.

There are probably more complex patch in future, probably will send to 
you if the checkpatch.pl script  mandate it.


 :-)

Thanks a lot really.


Signed-off-by: Sui Jingfeng 

Reviewed-by: Andi Shyti 

Andi


---
  drivers/pci/vgaarb.c   | 108 -
  include/linux/vgaarb.h |   4 +-
  2 files changed, 65 insertions(+), 47 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 22a505e877dc..ceb914245383 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -61,7 +61,6 @@ static bool vga_arbiter_used;
  static DEFINE_SPINLOCK(vga_lock);
  static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
  
-

  static const char *vga_iostate_to_str(unsigned int iostate)
  {
/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
@@ -79,8 +78,10 @@ static const char *vga_iostate_to_str(unsigned int iostate)
  
  static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)

  {
-   /* we could in theory hand out locks on IO and mem
-* separately to userspace but it can cause deadlocks */
+   /*
+* We could in theory hand out locks on IO and mem
+* separately to userspace but it can cause deadlocks
+*/
if (strncmp(buf, "none", 4) == 0) {
*io_state = VGA_RSRC_NONE;
return 1;
@@ -99,7 +100,7 @@ static int vga_str_to_iostate(char *buf, int str_size, 
unsigned int *io_state)
return 1;
  }
  
-/* this is only used a cookie - it should not be dereferenced */

+/* This is only used as cookie, it should not be dereferenced */
  static struct pci_dev *vga_default;
  
  /* Find somebody in our list */

@@ -193,14 +194,17 @@ int vga_remove_vgacon(struct pci_dev *pdev)
  #endif
  EXPORT_SYMBOL(vga_remove_vgacon);
  
-/* If we don't ever use VGA arb we should avoid

-   turning off anything anywhere due to old X servers getting
-   confused about the boot device not being VGA */
+/*
+ * If we don't ever use VGA arb we should avoid
+ * turning off anything anywhere due to old X servers getting
+ * confused about the boot device not being VGA
+ */
  static void vga_check_first_use(void)
  {
-   /* we should inform all GPUs in the system that
-* VGA arb has occurred and to try and disable resources
-* if they can */
+   /*
+* We should inform all GPUs in the system that
+* vgaarb has occurred and to try and disable resources if they can
+*/
if (!vga_arbiter_used) {
vga_arbiter_used = true;
vga_arbiter_notify_clients();
@@ -216,7 +220,8 @@ static struct vga_device *__vga_tryget(struct vga_device 
*vgadev,
unsigned int pci_bits;
u32 flags = 0;
  
-	/* Account for "normal" resources to lock. If we decode the legacy,

+   /*
+* Account for "normal" resources to lock. If we decode the legacy,
 * counterpart, we need to request it as well
 */
if ((rsrc & VGA_RSRC_NORMAL_IO) &&
@@ -236,7 +241,8 @@ static struct vga_device *__vga_tryget(struct vga_device 
*vgadev,
if (wants == 0)
goto lock_them;
  
-	/* We don't need to request a legacy resource, we just enable

+   /*
+* We don't need to request a legacy resource, we just enable
 * appropriate decoding and go
 */
legacy_wants = wants & VGA_RSRC_LEGACY_MASK;
@@ -252,7 +258,8 @@ static struct vga_device *__vga_tryget(struct vga_device 
*vgadev,
if (vgadev == conflict)
continue;
  
-		/* We have a possible conflict. before we go further, we must

+   /*
+* We have a possible conflict. before we go further, we must
 * check if we sit on the same bus as the conflicting device.
 * if we don't, then we must tie both IO and MEM resources
 * together since there is only a single bit controlling
@@ -263,13 +270,15 @@ static struct vga_device *__vga_tryget(struct vga_device 
*vgadev,
lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
}
  
-		/* Check if the guy has a lock on the resource. If he does,

+   /*
+* Check if the guy has a lock on the resource. If he does,
 * return the conflicting entry
 */
if (conflict->locks & lwants)
return conflict;
  
-		/* Ok, now check if it owns the resource we want.  We can

+   /*
+

Re: [PATCH v5 1/4] PCI/VGA: Use unsigned type for the io_state variable


Hi,

On 2023/6/9 19:48, Andi Shyti wrote:

Hi Sui,

On Fri, Jun 09, 2023 at 07:24:14PM +0800, Sui Jingfeng wrote:

The io_state variable in the vga_arb_write() function is declared with
unsigned int type, while the vga_str_to_iostate() function takes int *
type. To keep them consistent, replace the third argument of
vga_str_to_iostate() function with the unsigned int * type.

Signed-off-by: Sui Jingfeng 

Reviewed-by: Andi Shyti 

Thanks a lot.

Andi


---
  drivers/pci/vgaarb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 5a696078b382..c1bc6c983932 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -77,7 +77,7 @@ static const char *vga_iostate_to_str(unsigned int iostate)
return "none";
  }
  
-static int vga_str_to_iostate(char *buf, int str_size, int *io_state)

+static int vga_str_to_iostate(char *buf, int str_size, unsigned int *io_state)
  {
/* we could in theory hand out locks on IO and mem
 * separately to userspace but it can cause deadlocks */
--
2.25.1


--
Jingfeng

Re: [Intel-gfx] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices


Hi,

On 2023/6/9 03:12, Bjorn Helgaas wrote:

Start with verb and capitalize to match ("Deal only with ...")

On Thu, Jun 08, 2023 at 07:43:21PM +0800, Sui Jingfeng wrote:

From: Sui Jingfeng 

vgaarb only deal with the VGA devcie(pdev->class == 0x0300), so replace the
pci_get_subsys() function with pci_get_class(). Filter the non pci display
device out. There no need to process the non display PCI device.

s/non pci/non-PCI/
s/non display/non-display/

This is fine, and I'll merge this, but someday I would like to get rid
of the bus_register_notifier() and call vga_arbiter_add_pci_device()
and vga_arbiter_del_pci_device() directly from the PCI core.


Nice idea!


But I'm wondering there are traps in this.

The pci_notifier in vgaarb.c is still need on Mips platform.

Because of loading order problems.

On MIPS system, PCI devices are enumerated by pcibios_init(),

which runs after vga_arb_device_init(). This is true until now.

When vga_arb_device_init() function get called, it will capture nothing.

On that time, the system have no PCI device enumerated.

Because of this, there are various problems in the past.

This is the reason we still need the notifier,

we need a way to capture the PCI display device after vgaarb already 
loaded(finished).



On complex case, there are ASpeed BMC, loongson integrated display 
controller,


and radeon discrete video card co-exist on Loongson 3B4000 server.

I have fixed various bug at our downstream(linux-4.19) environment.


I have fixed a bug on downstream involved with aspeed bmc, by workaround[1].

Chen Huacai grabbing my patch[1] and rewrite it, submit together with 
him-self's.


Its fine,  but those patch still look paleness in the front of Loongson 
integrated display controller.


Loongson integrated display controller don't has a dedicated VRAM bar.

vga_is_firmware_default() will lose its effectiveness then.

This is the reason we reveal our patch(0004 in this series) to face 
upstream.


Its not only for loongson integrated display controller through.

Its not uncommon that ARM servers have A aspeed BMC and discrete amdgpu 
or old radeon card.


Let the device drivers gave us a hint probably can help to resolve 
multi-video card co-exist


problem.

Consider that sometime user want to use integrate gpu, sometime user 
want to use discrete gpu.


Also, during driver developing(or debugging),

driver writer may want override the default boot device.


vgaarb probable shouldn't make the decision

without giving the device driver a chance to override.


Beside,  vga_is_firmware_default() only apply for PCI display device.

On ARM64 system, there are a lot of platform device.

If we move this function back to the device driver,

it probably applicable for a platform display controller + one/two PCI 
display devices case.



We find a method at downstream during we get efifb works LoongArch platform.

We can utilize the screen_info, screen_info say where's the firmware 
framebuffer is located at.


Drivers for specific hardware platform perhaps know more clearly than 
vgaarb.


if it is the default boot device.


[1] 
https://lore.kernel.org/all/20210514080025.1828197-6-chenhua...@loongson.cn/



Or if you wanted to do that now, that would be even better :)

Bjorn


Signed-off-by: Sui Jingfeng 
---
  drivers/pci/vgaarb.c | 22 --
  1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 7f0347f4f6fd..b0bf4952a95d 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -756,10 +756,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev 
*pdev)
struct pci_dev *bridge;
u16 cmd;
  
-	/* Only deal with VGA class devices */

-   if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
-   return false;
-
/* Allocate structure */
vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
if (vgadev == NULL) {
@@ -1499,7 +1495,9 @@ static int pci_notify(struct notifier_block *nb, unsigned 
long action,
struct pci_dev *pdev = to_pci_dev(dev);
bool notify = false;
  
-	vgaarb_dbg(dev, "%s\n", __func__);

+   /* Only deal with VGA class devices */
+   if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+   return 0;
  
  	/* For now we're only intereted in devices added and removed. I didn't

 * test this thing here, so someone needs to double check for the
@@ -1509,6 +1507,8 @@ static int pci_notify(struct notifier_block *nb, unsigned 
long action,
else if (action == BUS_NOTIFY_DEL_DEVICE)
notify = vga_arbiter_del_pci_device(pdev);
  
+	vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);

+
if (notify)
vga_arbiter_notify_clients();
return 0;
@@ -1533,8 +1533,8 @@ static struct miscdevice vga_arb_device = {
  
  static int __init vga_arb_device_init(void)

  {
+   struct pci_dev *pdev = NULL;
int rc;
-   struct pci_dev

[PATCH 8/8] drm/i915: Implement fdinfo memory stats printing

From: Tvrtko Ursulin 

Use the newly added drm_print_memory_stats helper to show memory
utilisation of our objects in drm/driver specific fdinfo output.

To collect the stats we walk the per memory regions object lists
and accumulate object size into the respective drm_memory_stats
categories.

Objects with multiple possible placements are reported in multiple
regions for total and shared sizes, while other categories are
counted only for the currently active region.

Signed-off-by: Tvrtko Ursulin 
Cc: Aravind Iddamsetty 
Cc: Rob Clark 
---
 drivers/gpu/drm/i915/i915_drm_client.c | 64 ++
 1 file changed, 64 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drm_client.c 
b/drivers/gpu/drm/i915/i915_drm_client.c
index 31316edbf30b..596de36ee09c 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -48,6 +48,68 @@ void __i915_drm_client_free(struct kref *kref)
 }
 
 #ifdef CONFIG_PROC_FS
+static void
+obj_meminfo(struct drm_i915_gem_object *obj,
+   struct drm_memory_stats stats[INTEL_REGION_UNKNOWN])
+{
+   struct intel_memory_region *mr;
+   u64 sz = obj->base.size;
+   enum intel_region_id id;
+   unsigned int i;
+
+   /* Attribute size and shared to all possible memory regions. */
+   for (i = 0; i < obj->mm.n_placements; i++) {
+   mr = obj->mm.placements[i];
+   id = mr->id;
+
+   if (obj->base.handle_count > 1)
+   stats[id].shared += sz;
+   else
+   stats[id].private += sz;
+   }
+
+   /* Attribute other categories to only the current region. */
+   mr = obj->mm.region;
+   if (mr)
+   id = mr->id;
+   else
+   id = INTEL_REGION_SMEM;
+
+   if (i915_gem_object_has_pages(obj)) {
+   stats[id].resident += sz;
+
+   if (!dma_resv_test_signaled(obj->base.resv,
+   dma_resv_usage_rw(true)))
+   stats[id].active += sz;
+   else if (i915_gem_object_is_shrinkable(obj) &&
+obj->mm.madv == I915_MADV_DONTNEED)
+   stats[id].purgeable += sz;
+   }
+}
+
+static void show_meminfo(struct drm_printer *p, struct drm_file *file)
+{
+   struct drm_memory_stats stats[INTEL_REGION_UNKNOWN] = {};
+   struct drm_i915_file_private *fpriv = file->driver_priv;
+   struct i915_drm_client *client = fpriv->client;
+   struct drm_i915_private *i915 = fpriv->i915;
+   struct drm_i915_gem_object *obj;
+   struct intel_memory_region *mr;
+   unsigned int id;
+
+   spin_lock_irq(>objects_lock);
+   list_for_each_entry(obj, >objects_list, client_link)
+   obj_meminfo(obj, stats);
+   spin_unlock_irq(>objects_lock);
+
+   for_each_memory_region(mr, i915, id)
+   drm_print_memory_stats(p,
+  [id],
+  DRM_GEM_OBJECT_RESIDENT |
+  DRM_GEM_OBJECT_PURGEABLE,
+  mr->name);
+}
+
 static const char * const uabi_class_names[] = {
[I915_ENGINE_CLASS_RENDER] = "render",
[I915_ENGINE_CLASS_COPY] = "copy",
@@ -109,6 +171,8 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct 
drm_file *file)
 * **
 */
 
+   show_meminfo(p, file);
+
if (GRAPHICS_VER(i915) < 8)
return;
 
-- 
2.39.2

[PATCH 6/8] drm: Add drm_gem_prime_fd_to_handle_obj

From: Tvrtko Ursulin 

I need a new flavour of the drm_gem_prime_fd_to_handle helper, one which
will return a reference to a newly created GEM objects (if created), in
order to enable tracking of imported i915 GEM objects in the following
patch.

Minor code reshuffule and only trivial additions on top of
drm_gem_prime_fd_to_handle.

Signed-off-by: Tvrtko Ursulin 
---
 drivers/gpu/drm/drm_prime.c | 41 -
 include/drm/drm_prime.h |  4 
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index d29dafce9bb0..ef75f67e3057 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -284,11 +284,12 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
 EXPORT_SYMBOL(drm_gem_dmabuf_release);
 
 /**
- * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
+ * drm_gem_prime_fd_to_handle_obj - PRIME import function for GEM drivers
  * @dev: drm_device to import into
  * @file_priv: drm file-private structure
  * @prime_fd: fd id of the dma-buf which should be imported
  * @handle: pointer to storage for the handle of the imported buffer object
+ * @objp: optional pointer in which reference to created GEM object can be 
returned
  *
  * This is the PRIME import function which must be used mandatorily by GEM
  * drivers to ensure correct lifetime management of the underlying GEM object.
@@ -297,9 +298,10 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
  *
  * Returns 0 on success or a negative error code on failure.
  */
-int drm_gem_prime_fd_to_handle(struct drm_device *dev,
-  struct drm_file *file_priv, int prime_fd,
-  uint32_t *handle)
+int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev,
+  struct drm_file *file_priv, int prime_fd,
+  uint32_t *handle,
+  struct drm_gem_object **objp)
 {
struct dma_buf *dma_buf;
struct drm_gem_object *obj;
@@ -336,7 +338,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
/* _handle_create_tail unconditionally unlocks dev->object_name_lock. */
ret = drm_gem_handle_create_tail(file_priv, obj, handle);
-   drm_gem_object_put(obj);
+   if (!objp)
+   drm_gem_object_put(obj);
if (ret)
goto out_put;
 
@@ -348,6 +351,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
dma_buf_put(dma_buf);
 
+   if (objp)
+   *objp = obj;
+
return 0;
 
 fail:
@@ -356,6 +362,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 */
drm_gem_handle_delete(file_priv, *handle);
dma_buf_put(dma_buf);
+   if (objp)
+   drm_gem_object_put(obj);
return ret;
 
 out_unlock:
@@ -365,6 +373,29 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
dma_buf_put(dma_buf);
return ret;
 }
+EXPORT_SYMBOL(drm_gem_prime_fd_to_handle_obj);
+
+/**
+ * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
+ * @dev: drm_device to import into
+ * @file_priv: drm file-private structure
+ * @prime_fd: fd id of the dma-buf which should be imported
+ * @handle: pointer to storage for the handle of the imported buffer object
+ *
+ * This is the PRIME import function which must be used mandatorily by GEM
+ * drivers to ensure correct lifetime management of the underlying GEM object.
+ * The actual importing of GEM object from the dma-buf is done through the
+ * _driver.gem_prime_import driver callback.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_gem_prime_fd_to_handle(struct drm_device *dev,
+  struct drm_file *file_priv, int prime_fd,
+  uint32_t *handle)
+{
+   return drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd, handle,
+ NULL);
+}
 EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
 
 int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 2a1d01e5b56b..10d145ce6586 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -69,6 +69,10 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
 
 int drm_gem_prime_fd_to_handle(struct drm_device *dev,
   struct drm_file *file_priv, int prime_fd, 
uint32_t *handle);
+int drm_gem_prime_fd_to_handle_obj(struct drm_device *dev,
+  struct drm_file *file_priv, int prime_fd,
+  uint32_t *handle,
+  struct drm_gem_object **obj);
 int drm_gem_prime_handle_to_fd(struct drm_device *dev,
   struct drm_file *file_priv, uint32_t handle, 
uint32_t flags,
   int *prime_fd);
-- 
2.39.2

[PATCH 7/8] drm/i915: Track imported dma-buf objects in memory stats

From: Tvrtko Ursulin 

We want to be able to show memory usage of imported dma-buf opjects in the
fdinfo stats.

To achieve this we wrap drm_gem_prime_fd_to_handle(_obj) in
i915_gem_prime_fd_to_handle and append some client management at the end.

Signed-off-by: Tvrtko Ursulin 
Cc: Aravind Iddamsetty 
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 32 ++
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h |  7 +
 drivers/gpu/drm/i915/i915_driver.c |  2 +-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c 
b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index fd556a076d05..2e2d9d7c1992 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -11,7 +11,10 @@
 
 #include 
 
+#include 
+
 #include "gem/i915_gem_dmabuf.h"
+#include "i915_drm_client.h"
 #include "i915_drv.h"
 #include "i915_gem_object.h"
 #include "i915_scatterlist.h"
@@ -344,6 +347,35 @@ struct drm_gem_object *i915_gem_prime_import(struct 
drm_device *dev,
return ERR_PTR(ret);
 }
 
+int i915_gem_prime_fd_to_handle(struct drm_device *dev,
+   struct drm_file *file_priv, int prime_fd,
+   uint32_t *handle)
+{
+   struct drm_gem_object *gem_obj = NULL;
+   int ret;
+
+   if (IS_ENABLED(CONFIG_PROC_FS))
+   ret = drm_gem_prime_fd_to_handle_obj(dev, file_priv, prime_fd,
+handle, _obj);
+   else
+   ret = drm_gem_prime_fd_to_handle(dev, file_priv, prime_fd,
+handle);
+   if (ret)
+   return ret;
+
+   if (gem_obj) {
+   struct drm_i915_file_private *fpriv = file_priv->driver_priv;
+   struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
+
+   /* Really imported and not just alias? */
+   if (obj->ops == _gem_object_dmabuf_ops)
+   i915_drm_client_add_object(fpriv->client, obj);
+   i915_gem_object_put(obj);
+   }
+
+   return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_dmabuf.c"
 #include "selftests/i915_gem_dmabuf.c"
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h 
b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h
index 6e0405d47ce1..63635c221c7c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.h
@@ -6,8 +6,11 @@
 #ifndef __I915_GEM_DMABUF_H__
 #define __I915_GEM_DMABUF_H__
 
+#include 
+
 struct drm_gem_object;
 struct drm_device;
+struct drm_file;
 struct dma_buf;
 
 struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
@@ -15,4 +18,8 @@ struct drm_gem_object *i915_gem_prime_import(struct 
drm_device *dev,
 
 struct dma_buf *i915_gem_prime_export(struct drm_gem_object *gem_obj, int 
flags);
 
+int i915_gem_prime_fd_to_handle(struct drm_device *dev,
+   struct drm_file *file_priv, int prime_fd,
+   uint32_t *handle);
+
 #endif /* __I915_GEM_DMABUF_H__ */
diff --git a/drivers/gpu/drm/i915/i915_driver.c 
b/drivers/gpu/drm/i915/i915_driver.c
index ace8534b6cc5..03f3157371bf 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1806,7 +1806,7 @@ static const struct drm_driver i915_drm_driver = {
.show_fdinfo = i915_drm_client_fdinfo,
 
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
-   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
+   .prime_fd_to_handle = i915_gem_prime_fd_to_handle,
.gem_prime_import = i915_gem_prime_import,
 
.dumb_create = i915_gem_dumb_create,
-- 
2.39.2

[PATCH 4/8] drm/i915: Track page table backing store usage

From: Tvrtko Ursulin 

Account page table backing store against the owning client memory usage
stats.

Signed-off-by: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/gt/intel_gtt.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c 
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 2f6a9be0ffe6..126269a0d728 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -58,6 +58,9 @@ struct drm_i915_gem_object *alloc_pt_lmem(struct 
i915_address_space *vm, int sz)
if (!IS_ERR(obj)) {
obj->base.resv = i915_vm_resv_get(vm);
obj->shares_resv_from = vm;
+
+   if (vm->fpriv)
+   i915_drm_client_add_object(vm->fpriv->client, obj);
}
 
return obj;
@@ -79,6 +82,9 @@ struct drm_i915_gem_object *alloc_pt_dma(struct 
i915_address_space *vm, int sz)
if (!IS_ERR(obj)) {
obj->base.resv = i915_vm_resv_get(vm);
obj->shares_resv_from = vm;
+
+   if (vm->fpriv)
+   i915_drm_client_add_object(vm->fpriv->client, obj);
}
 
return obj;
-- 
2.39.2

[PATCH 5/8] drm/i915: Account ring buffer and context state storage