date:20230712

The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.

Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
Signed-off-by: Gaosheng Cui 
Reviewed-by: Abhinav Kumar 
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a99310b68793..a499e3b350fc 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -89,7 +89,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct 
msm_gem_submit *submit
 * since we've already mapped it once in
 * submit_reloc()
 */
-   if (WARN_ON(!ptr))
+   if (WARN_ON(IS_ERR(ptr)))
return;
 
for (i = 0; i < dwords; i++) {
-- 
2.25.1

[PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()

The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.

Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
Signed-off-by: Gaosheng Cui 
---
 drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index 3276a3e82c62..e9c92439398d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component 
*c,
u32 avail_scalers;
 
pipe_st = komeda_pipeline_get_state(c->pipeline, state);
-   if (!pipe_st)
+   if (IS_ERR(pipe_st))
return NULL;
 
avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
-- 
2.25.1

[PATCH v2 1/3] drm/panel: Fix IS_ERR() vs NULL check in nt35950_probe()

The mipi_dsi_device_register_full() returns an ERR_PTR() on failure,
we should use IS_ERR() to check the return value.

Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC 
panels")
Signed-off-by: Gaosheng Cui 
---
 drivers/gpu/drm/panel/panel-novatek-nt35950.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c 
b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
index 8b108ac80b55..4903bbf1df55 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
@@ -571,7 +571,7 @@ static int nt35950_probe(struct mipi_dsi_device *dsi)
}
 
nt->dsi[1] = mipi_dsi_device_register_full(dsi_r_host, info);
-   if (!nt->dsi[1]) {
+   if (IS_ERR(nt->dsi[1])) {
dev_err(dev, "Cannot get secondary DSI node\n");
return -ENODEV;
}
-- 
2.25.1

[PATCH v2 0/3] Fix IS_ERR() vs NULL check for drm

v2:
- I'm sorry I missed some emails, these patches were submitted last year,
  now let me resend it with the following changes:
  1. remove the patch: "drm/msm: Fix IS_ERR_OR_NULL() vs NULL check in 
msm_icc_get()"
  2. remove the patch: "drm/vc4: kms: Fix IS_ERR() vs NULL check for vc4_kms"
  3. add "Reviewed-by: Abhinav Kumar " to the second 
patch.

  Thanks!

v1:
- This series contains a few fixup patches, to fix IS_ERR() vs NULL check
  for drm, and avoid a potential null-ptr-defer issue, too. Thanks!

Gaosheng Cui (3):
  drm/panel: Fix IS_ERR() vs NULL check in nt35950_probe()
  drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()
  drm/komeda: Fix IS_ERR() vs NULL check in
komeda_component_get_avail_scaler()

 drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c  | 2 +-
 drivers/gpu/drm/panel/panel-novatek-nt35950.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.25.1

RE: [PATCH v2] drm/amd/pm: Vangogh: Add new gpu_metrics_v2_4 to acquire gpu_metrics

2023-07-12 Thread Yang, WenYou

[AMD Official Use Only - General]

Any comments? Any advice?

Best Regards,
Wenyou

> -Original Message-
> From: Wenyou Yang 
> Sent: Wednesday, June 21, 2023 2:32 PM
> To: Deucher, Alexander ; Limonciello, Mario
> ; Koenig, Christian ;
> Pan, Xinhui ; Quan, Evan 
> Cc: Yuan, Perry ; Liang, Richard qi
> ; amd-...@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Yang, WenYou
> 
> Subject: [PATCH v2] drm/amd/pm: Vangogh: Add new gpu_metrics_v2_4 to
> acquire gpu_metrics
>
> To acquire the voltage and current info from gpu_metrics interface, but
> gpu_metrics_v2_3 doesn't contain them, and to be backward compatible, add
> new gpu_metrics_v2_4 structure.
>
> Reviewed-by: Mario Limonciello 
> Acked-by: Evan Quan 
> Signed-off-by: Wenyou Yang 
> ---
>  .../gpu/drm/amd/include/kgd_pp_interface.h|  69 +++
>  .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c  | 109 -
> -
>  drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c|   3 +
>  3 files changed, 172 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> index 9f542f6e19ed..90989405eddc 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -892,4 +892,73 @@ struct gpu_metrics_v2_3 {
>   uint16_taverage_temperature_core[8]; //
> average CPU core temperature on APUs
>   uint16_taverage_temperature_l3[2];
>  };
> +
> +struct gpu_metrics_v2_4 {
> + struct metrics_table_header common_header;
> +
> + /* Temperature (unit: centi-Celsius) */
> + uint16_ttemperature_gfx;
> + uint16_ttemperature_soc;
> + uint16_ttemperature_core[8];
> + uint16_ttemperature_l3[2];
> +
> + /* Utilization (unit: centi) */
> + uint16_taverage_gfx_activity;
> + uint16_taverage_mm_activity;
> +
> + /* Driver attached timestamp (in ns) */
> + uint64_tsystem_clock_counter;
> +
> + /* Power/Energy (unit: mW) */
> + uint16_taverage_socket_power;
> + uint16_taverage_cpu_power;
> + uint16_taverage_soc_power;
> + uint16_taverage_gfx_power;
> + uint16_taverage_core_power[8];
> +
> + /* Average clocks (unit: MHz) */
> + uint16_taverage_gfxclk_frequency;
> + uint16_taverage_socclk_frequency;
> + uint16_taverage_uclk_frequency;
> + uint16_taverage_fclk_frequency;
> + uint16_taverage_vclk_frequency;
> + uint16_taverage_dclk_frequency;
> +
> + /* Current clocks (unit: MHz) */
> + uint16_tcurrent_gfxclk;
> + uint16_tcurrent_socclk;
> + uint16_tcurrent_uclk;
> + uint16_tcurrent_fclk;
> + uint16_tcurrent_vclk;
> + uint16_tcurrent_dclk;
> + uint16_tcurrent_coreclk[8];
> + uint16_tcurrent_l3clk[2];
> +
> + /* Throttle status (ASIC dependent) */
> + uint32_tthrottle_status;
> +
> + /* Fans */
> + uint16_tfan_pwm;
> +
> + uint16_tpadding[3];
> +
> + /* Throttle status (ASIC independent) */
> + uint64_tindep_throttle_status;
> +
> + /* Average Temperature (unit: centi-Celsius) */
> + uint16_taverage_temperature_gfx;
> + uint16_taverage_temperature_soc;
> + uint16_taverage_temperature_core[8];
> + uint16_taverage_temperature_l3[2];
> +
> + /* Power/Voltage (unit: mV) */
> + uint16_taverage_cpu_voltage;
> + uint16_taverage_soc_voltage;
> + uint16_taverage_gfx_voltage;
> +
> + /* Power/Current (unit: mA) */
> + uint16_taverage_cpu_current;
> + uint16_taverage_soc_current;
> + uint16_taverage_gfx_current;
> +};
>  #endif
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> index 067b4e0b026c..185d0b50ee8e 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> @@ -1854,6 +1854,86 @@ static ssize_t vangogh_get_gpu_metrics_v2_3(struct
> smu_context *smu,
>

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

2023-07-12 Thread Adam Ford

On Wed, Jul 12, 2023 at 5:34 PM Tim Harvey  wrote:
>
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf  wrote:
> >
> > From: Frieder Schrempf 
> >
> > According to the documentation [1] the proper enable flow is:
> >
> > 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> > 2. Disable stop state to bring data lanes into HS mode
> >
> > Currently we do this all at once within enable(), which doesn't
> > allow to meet the requirements of some downstream bridges.
> >
> > To fix this we now enable the DSI in pre_enable() and force it
> > into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> > register until enable() is called where we reset the bit.
> >
> > We currently do this only for i.MX8M as Exynos uses a different
> > init flow where samsung_dsim_init() is called from
> > samsung_dsim_host_transfer().
> >
> > [1] 
> > https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >
> > Signed-off-by: Frieder Schrempf 
> > ---
> > Changes for v2:
> > * Drop RFC
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
> > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index e0a402a85787..9775779721d9 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim 
> > *dsi)
> > reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > reg &= ~DSIM_STOP_STATE_CNT_MASK;
> > reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> > +
> > +   if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> > +   reg |= DSIM_FORCE_STOP_STATE;
> > +
> > samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >
> > reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
> > @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct 
> > drm_bridge *bridge,
> > ret = samsung_dsim_init(dsi);
> > if (ret)
> > return;
> > +
> > +   samsung_dsim_set_display_mode(dsi);
> > +   samsung_dsim_set_display_enable(dsi, true);
> > }
> >  }
> >
> > @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct 
> > drm_bridge *bridge,
> >struct drm_bridge_state 
> > *old_bridge_state)
> >  {
> > struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > +   u32 reg;
> >
> > -   samsung_dsim_set_display_mode(dsi);
> > -   samsung_dsim_set_display_enable(dsi, true);
> > +   if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +   samsung_dsim_set_display_mode(dsi);
> > +   samsung_dsim_set_display_enable(dsi, true);
> > +   } else {
> > +   reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +   reg &= ~DSIM_FORCE_STOP_STATE;
> > +   samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +   }
> >
> > dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >  }
> > @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct 
> > drm_bridge *bridge,
> > struct drm_bridge_state 
> > *old_bridge_state)
> >  {
> > struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > +   u32 reg;
> >
> > if (!(dsi->state & DSIM_STATE_ENABLED))
> > return;
> >
> > +   if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +   reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +   reg |= DSIM_FORCE_STOP_STATE;
> > +   samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +   }
> > +
> > dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >  }
> >
> > --
> > 2.40.0
> >
>
> Hi Frieder,
>
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
>
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
>
> The issue appears as:
> [6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)
>
> Instead of
> [1.011729] samsung-dsim 32e1.dsi: supply vddcore not found,
> using dummy regulator
> [1.019829] samsung-dsim 32e1.dsi: supply vddio not found,
> using dummy regulator
> [5.649928] samsung-dsim 32e1.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
>
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
>
> I'm

Re: [PATCH 4/4] vgacon, arch/*: remove unused screen_info definitions

2023-07-12 Thread Guo Ren

csky:
Acked-by: Guo Ren 

On Fri, Jul 7, 2023 at 5:56 AM Arnd Bergmann  wrote:
>
> From: Arnd Bergmann 
>
> A number of architectures either kept the screen_info definition for
> historical purposes as it used to be required by the generic VT code, or
> they copied it from another architecture in order to build the VGA
> console driver in an allmodconfig build.
>
> Now that vgacon no longer builds on these architectures, remove the
> stale definitions.
>
> Signed-off-by: Arnd Bergmann 
> ---
>  arch/csky/kernel/setup.c  | 12 
>  arch/hexagon/kernel/Makefile  |  2 --
>  arch/hexagon/kernel/screen_info.c |  3 ---
>  arch/nios2/kernel/setup.c |  5 -
>  arch/sh/kernel/setup.c|  5 -
>  arch/sparc/kernel/setup_32.c  | 13 -
>  arch/sparc/kernel/setup_64.c  | 13 -
>  arch/xtensa/kernel/setup.c| 12 
>  8 files changed, 65 deletions(-)
>  delete mode 100644 arch/hexagon/kernel/screen_info.c
>
> diff --git a/arch/csky/kernel/setup.c b/arch/csky/kernel/setup.c
> index 106fbf0b6f3b4..51012e90780d6 100644
> --- a/arch/csky/kernel/setup.c
> +++ b/arch/csky/kernel/setup.c
> @@ -8,22 +8,10 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
>
> -#ifdef CONFIG_DUMMY_CONSOLE
> -struct screen_info screen_info = {
> -   .orig_video_lines   = 30,
> -   .orig_video_cols= 80,
> -   .orig_video_mode= 0,
> -   .orig_video_ega_bx  = 0,
> -   .orig_video_isVGA   = 1,
> -   .orig_video_points  = 8
> -};
> -#endif
> -
>  static void __init csky_memblock_init(void)
>  {
> unsigned long lowmem_size = PFN_DOWN(LOWMEM_LIMIT - 
> PHYS_OFFSET_OFFSET);
> diff --git a/arch/hexagon/kernel/Makefile b/arch/hexagon/kernel/Makefile
> index e73cb321630ec..3fdf937eb572e 100644
> --- a/arch/hexagon/kernel/Makefile
> +++ b/arch/hexagon/kernel/Makefile
> @@ -17,5 +17,3 @@ obj-y += vm_vectors.o
>  obj-$(CONFIG_HAS_DMA) += dma.o
>
>  obj-$(CONFIG_STACKTRACE) += stacktrace.o
> -
> -obj-$(CONFIG_VGA_CONSOLE) += screen_info.o
> diff --git a/arch/hexagon/kernel/screen_info.c 
> b/arch/hexagon/kernel/screen_info.c
> deleted file mode 100644
> index 1e1ceb18bafe7..0
> --- a/arch/hexagon/kernel/screen_info.c
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -#include 
> -
> -struct screen_info screen_info;
> diff --git a/arch/nios2/kernel/setup.c b/arch/nios2/kernel/setup.c
> index 8582ed9658447..da122a5fa43b2 100644
> --- a/arch/nios2/kernel/setup.c
> +++ b/arch/nios2/kernel/setup.c
> @@ -19,7 +19,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>
>  #include 
>  #include 
> @@ -36,10 +35,6 @@ static struct pt_regs fake_regs = { 0, 0, 0, 0, 0, 0, 0, 
> 0, 0, 0, 0, 0, 0, 0, 0,
> 0, 0, 0, 0, 0, 0,
> 0};
>
> -#ifdef CONFIG_VT
> -struct screen_info screen_info;
> -#endif
> -
>  /* Copy a short hook instruction sequence to the exception address */
>  static inline void copy_exception_handler(unsigned int addr)
>  {
> diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c
> index b3da2757faaf3..3d80515298d26 100644
> --- a/arch/sh/kernel/setup.c
> +++ b/arch/sh/kernel/setup.c
> @@ -7,7 +7,6 @@
>   *  Copyright (C) 1999  Niibe Yutaka
>   *  Copyright (C) 2002 - 2010 Paul Mundt
>   */
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -69,10 +68,6 @@ EXPORT_SYMBOL(cpu_data);
>  struct sh_machine_vector sh_mv = { .mv_name = "generic", };
>  EXPORT_SYMBOL(sh_mv);
>
> -#ifdef CONFIG_VT
> -struct screen_info screen_info;
> -#endif
> -
>  extern int root_mountflags;
>
>  #define RAMDISK_IMAGE_START_MASK   0x07FF
> diff --git a/arch/sparc/kernel/setup_32.c b/arch/sparc/kernel/setup_32.c
> index 34ef7febf0d56..e3b72a7b46d37 100644
> --- a/arch/sparc/kernel/setup_32.c
> +++ b/arch/sparc/kernel/setup_32.c
> @@ -17,7 +17,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -51,18 +50,6 @@
>
>  #include "kernel.h"
>
> -struct screen_info screen_info = {
> -   0, 0,   /* orig-x, orig-y */
> -   0,  /* unused */
> -   0,  /* orig-video-page */
> -   0,  /* orig-video-mode */
> -   128,/* orig-video-cols */
> -   0,0,0,  /* ega_ax, ega_bx, ega_cx */
> -   54, /* orig-video-lines */
> -   0,  /* orig-video-isVGA */
> -   16  /* orig-video-points */
> -};
> -
>  /* Typing sync at the prom prompt calls the function pointed to by
>   * romvec->pv_synchook which I set to the following function.
>   * This should sync all filesystems and return, for now it just
> diff --git a/arch/sparc/kernel/setup_64.c b/arch/sparc/kernel/setup_64.c
> index 6546ca9d4d3f1..6a4797dec34b4 100644
> --- a/arch/sparc/kernel/setup_64.c
> +++

RE: [PATCH] drm/exynos: fix a possible null-pointer dereference due to data race in exynos_drm_crtc_atomic_disable()

2023-07-12 Thread SR




> -Original Message-
> From: Tuo Li 
> Sent: Friday, June 30, 2023 11:19 AM
> To: inki@samsung.com; sw0312@samsung.com;
> kyungmin.p...@samsung.com; airl...@gmail.com; dan...@ffwll.ch;
> krzysztof.kozlow...@linaro.org; alim.akh...@samsung.com
> Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org;
> baijiaju1...@outlook.com; Tuo Li ; BassCheck
> 
> Subject: [PATCH] drm/exynos: fix a possible null-pointer dereference due
> to data race in exynos_drm_crtc_atomic_disable()
> 
> The variable crtc->state->event is often protected by the lock
> crtc->dev->event_lock when is accessed. However, it is accessed as a
> condition of an if statement in exynos_drm_crtc_atomic_disable() without
> holding the lock:
> 
>   if (crtc->state->event && !crtc->state->active)
> 
> However, if crtc->state->event is changed to NULL by another thread right
> after the conditions of the if statement is checked to be true, a
> null-pointer dereference can occur in drm_crtc_send_vblank_event():
> 
>   e->pipe = pipe;
> 
> To fix this possible null-pointer dereference caused by data race, the
> spin lock coverage is extended to protect the if statement as well as the
> function call to drm_crtc_send_vblank_event().
> 
> Reported-by: BassCheck 
> Signed-off-by: Tuo Li 

Applied.

Thanks,
Inki Dae

> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 4153f302de7c..d19e796c2061 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -39,13 +39,12 @@ static void exynos_drm_crtc_atomic_disable(struct
> drm_crtc *crtc,
>   if (exynos_crtc->ops->atomic_disable)
>   exynos_crtc->ops->atomic_disable(exynos_crtc);
> 
> + spin_lock_irq(>dev->event_lock);
>   if (crtc->state->event && !crtc->state->active) {
> - spin_lock_irq(>dev->event_lock);
>   drm_crtc_send_vblank_event(crtc, crtc->state->event);
> - spin_unlock_irq(>dev->event_lock);
> -
>   crtc->state->event = NULL;
>   }
> + spin_unlock_irq(>dev->event_lock);
>  }
> 
>  static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
> --
> 2.34.1

linux-next: manual merge of the drm-misc tree with Linus' tree

2023-07-12 Thread Stephen Rothwell

Hi all,

Today's linux-next merge of the drm-misc tree got a conflict in:

  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c

between commit:

  570b295248b0 ("drm/amdgpu: fix number of fence calculations")

from Linus' tree and commit:

  ca6c1e210aa7 ("drm/amdgpu: use the new drm_exec object for CS v3")

from the drm-misc tree.

I fixed it up (the latter removed the lines modified by the former, so
I just did that) and can carry the fix as necessary. This is now fixed
as far as linux-next is concerned, but any non trivial conflicts should
be mentioned to your upstream maintainer when your tree is submitted for
merging.  You may also want to consider cooperating with the maintainer
of the conflicting tree to minimise any particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell


pgpEX8jZMJXuV.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/5] drm/msm: Fix IS_ERR() vs NULL check in a5xx_submit_in_rb()





On 11/10/2022 1:44 AM, Gaosheng Cui wrote:

The msm_gem_get_vaddr() returns an ERR_PTR() on failure, we should
use IS_ERR() to check the return value.

Fixes: 6a8bd08d0465 ("drm/msm: add sudo flag to submit ioctl")
Signed-off-by: Gaosheng Cui 
---
  drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Abhinav Kumar

Re: [PATCH] drm: Replace drm_framebuffer plane size functions with its equivalents

2023-07-12 Thread André Almeida


Hi Carlos,

Em 27/06/2023 15:22, Carlos Eduardo Gallo Filho escreveu:
[...]


So, replace each drm_framebuffer_plane_{width,height} and
fb_plane_{width,height} call to drm_format_info_plane_{width,height}
and remove them.



I see that with this replace, there's a small code change from

return DIV_ROUND_UP(width, format->hsub);

to
return width / info->hsub;

is there any case that the replaced function will give different results?

Re: [PATCH V6 1/9] drivers core: Add support for Wifi band RF mitigations

2023-07-12 Thread Mario Limonciello


On 7/12/23 18:12, Andrew Lunn wrote:

+/**
+ * wbrf_supported_producer - Determine if the device can report frequencies
+ *
+ * @dev: device pointer
+ *
+ * WBRF is used to mitigate devices that cause harmonic interference.
+ * This function will determine if this device needs to report such 
frequencies.


How is the WBRF core supposed to answer this question? That it knows
there is at least one device which has registered with WBRF saying it
can change its behaviour to avoid causing interference?


Potential producers are supposed to be the ones asking the question.

Rather than "Determine if the device can report frequencies" should it be
"Determine if the device should report frequencies"

Agree.


A WiFi device can always report frequencies, since it knows what
frequency is it currently using. However, it is pointless making such
reports if there is no device which can actually make use of the
information.


Which is why this function exists.

With the AMD ACPI implementation the platform will dictate this information.

If a future device tree implementation is added it would work similarly.




+bool wbrf_supported_producer(struct device *dev)
+{
+   return true;
+}


I found the default implementation of true being odd. It makes me
wounder, what is the point of this call. I would expect this to see if
a linked list is empty or not.


The point is a lot clearer when you look at the description for the 
Kconfig included in patch 2.


+ Ideally it is the hardware designer/provider who should provide a
+ solution for the possible RF interference issue. Since they know
+ well whether there could be RF interference issue with their
+ platforms.
+
+ Say Y to enable this generic WBRF solution for diagnosing potential
+ interference issues on systems without the ACPI mechanism and
+ developing solutions.

WBRF_AMD_ACPI and WBRF_GENERIC are mutually exclusive.  I don't expect 
the average user to enable WBRF_GENERIC, but there isn't anything to 
stop them from doing so.


It may also aide in developing a WBRF_DEVICE_TREE or similar.




+/**
+ * wbrf_supported_consumer - Determine if the device can react to frequencies


This again seems odd. A device should know if it can react to
frequencies or not. WBRF core should not need to tell it. What makes
more sense to me is that this call is about a device telling the WBRF
core it is able to react to frequencies. The WBRF core then can give a
good answer to wbrf_supported_producer(), yes, i know of some other
device who might be able to do something to avoid causing interference
to you, so please do tell me about frequencies you want to use.

What is missing here in this API is policy information. The WBRF core
knows it has zero or more devices which can report what frequencies
they are using, and it has zero or more devices which maybe can do
something. But then you need policy to say this particular board needs
any registered devices to actually do something because of poor
shielding. Should this policy be as simple as a bool, or should it
actually say the board has shielding issues for a list of frequencies?
I think the answer to what will depend on the cost of taking action
when no action is actually required.


Again, look at patch 2 and the purpose of WBRF_GENERIC.  I suppose an 
argument can be made to bring WBRF_GENERIC into patch 1.





+ * wbrf_register_notifier - Register for notifications of frequency changes
+ *
+ * @nb: driver notifier block
+ *
+ * WBRF is used to mitigate devices that cause harmonic interference.
+ * This function will allow consumers to register for frequency notifications.
+ */
+int wbrf_register_notifier(struct notifier_block *nb)
+{
+   return blocking_notifier_chain_register(_chain_head, nb);
+}


What are the timing requirements for the handler? Should the handler
block until the device has finished doing what it needs to do and the
frequency response has settled? We don't want the WiFi device doing a
SNR measurement until we know local noise is at a minimum. I think it
would be good to document things like this here.


+struct wbrf_ranges_out {
+   u32 num_of_ranges;
+   struct exclusion_range  band_list[MAX_NUM_OF_WBRF_RANGES];
+} __packed;


Seems odd using packed here. It is the only structure which is
packed. I would also move the u32 after the struct so it is naturally
aligned on 64 bit systems.

Andrew

Re: [PATCH V6 1/9] drivers core: Add support for Wifi band RF mitigations

2023-07-12 Thread Andrew Lunn

> +/**
> + * wbrf_supported_producer - Determine if the device can report frequencies
> + *
> + * @dev: device pointer
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will determine if this device needs to report such 
> frequencies.

How is the WBRF core supposed to answer this question? That it knows
there is at least one device which has registered with WBRF saying it
can change its behaviour to avoid causing interference?

Rather than "Determine if the device can report frequencies" should it be
"Determine if the device should report frequencies"

A WiFi device can always report frequencies, since it knows what
frequency is it currently using. However, it is pointless making such
reports if there is no device which can actually make use of the
information. 

> +bool wbrf_supported_producer(struct device *dev)
> +{
> + return true;
> +}

I found the default implementation of true being odd. It makes me
wounder, what is the point of this call. I would expect this to see if
a linked list is empty or not.

> +/**
> + * wbrf_supported_consumer - Determine if the device can react to frequencies

This again seems odd. A device should know if it can react to
frequencies or not. WBRF core should not need to tell it. What makes
more sense to me is that this call is about a device telling the WBRF
core it is able to react to frequencies. The WBRF core then can give a
good answer to wbrf_supported_producer(), yes, i know of some other
device who might be able to do something to avoid causing interference
to you, so please do tell me about frequencies you want to use.

What is missing here in this API is policy information. The WBRF core
knows it has zero or more devices which can report what frequencies
they are using, and it has zero or more devices which maybe can do
something. But then you need policy to say this particular board needs
any registered devices to actually do something because of poor
shielding. Should this policy be as simple as a bool, or should it
actually say the board has shielding issues for a list of frequencies?
I think the answer to what will depend on the cost of taking action
when no action is actually required.

> + * wbrf_register_notifier - Register for notifications of frequency changes
> + *
> + * @nb: driver notifier block
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will allow consumers to register for frequency 
> notifications.
> + */
> +int wbrf_register_notifier(struct notifier_block *nb)
> +{
> + return blocking_notifier_chain_register(_chain_head, nb);
> +}

What are the timing requirements for the handler? Should the handler
block until the device has finished doing what it needs to do and the
frequency response has settled? We don't want the WiFi device doing a
SNR measurement until we know local noise is at a minimum. I think it
would be good to document things like this here.

> +struct wbrf_ranges_out {
> + u32 num_of_ranges;
> + struct exclusion_range  band_list[MAX_NUM_OF_WBRF_RANGES];
> +} __packed;

Seems odd using packed here. It is the only structure which is
packed. I would also move the u32 after the struct so it is naturally
aligned on 64 bit systems.

Andrew

[PATCH v5] drm/i915/selftest/gsc: Ensure GSC Proxy init completes before selftests

2023-07-12 Thread Alan Previn

On MTL, if the GSC Proxy init flows haven't completed, submissions to the
GSC engine will fail. Those init flows are dependent on the mei's
gsc_proxy component that is loaded in parallel with i915 and a
worker that could potentially start after i915 driver init is done.

That said, all subsytems that access the GSC engine today does check
for such init flow completion before using the GSC engine. However,
selftests currently don't wait on anything before starting.

To fix this, add a waiter function at the start of __run_selftests
that waits for gsc-proxy init flows to complete.

Difference from prior versions:
   v5: - Move the call to __wait_gsc_proxy_completed from common
 __run_selftests dispatcher to the group-level selftest
 function (Trvtko).
   - change the pr_info to pr_warn if we hit the timeout.
   v4: - Remove generalized waiters function table framework (Tvrtko).
   - Remove mention of CI-framework-timeout from comments (Tvrtko).
   v3: - Rebase to latest drm-tip.
   v2: - Based on internal testing, increase the timeout for gsc-proxy
 specific case to 8 seconds.

Signed-off-by: Alan Previn 
---
 .../gpu/drm/i915/selftests/i915_selftest.c| 26 +++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c 
b/drivers/gpu/drm/i915/selftests/i915_selftest.c
index 39da0fb0d6d2..b03d03eac3d6 100644
--- a/drivers/gpu/drm/i915/selftests/i915_selftest.c
+++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c
@@ -24,6 +24,8 @@
 #include 
 
 #include "gt/intel_gt_pm.h"
+#include "gt/uc/intel_gsc_fw.h"
+
 #include "i915_driver.h"
 #include "i915_drv.h"
 #include "i915_selftest.h"
@@ -127,6 +129,26 @@ static void set_default_test_all(struct selftest *st, 
unsigned int count)
st[i].enabled = true;
 }
 
+static void
+__wait_gsc_proxy_completed(struct drm_i915_private *i915)
+{
+   bool need_to_wait = (IS_ENABLED(CONFIG_INTEL_MEI_GSC_PROXY) &&
+i915->media_gt &&
+HAS_ENGINE(i915->media_gt, GSC0) &&
+
intel_uc_fw_is_loadable(>media_gt->uc.gsc.fw));
+   /*
+* The gsc proxy component depends on the kernel component driver load 
ordering
+* and in corner cases (the first time after an IFWI flash), 
init-completion
+* firmware flows take longer.
+*/
+   unsigned long timeout_ms = 8000;
+
+   if (need_to_wait &&
+   (wait_for(intel_gsc_uc_fw_proxy_init_done(>media_gt->uc.gsc, 
true),
+   timeout_ms)))
+   pr_warn(DRIVER_NAME "Timed out waiting for 
gsc_proxy_completion!\n");
+}
+
 static int __run_selftests(const char *name,
   struct selftest *st,
   unsigned int count,
@@ -206,6 +228,8 @@ int i915_live_selftests(struct pci_dev *pdev)
if (!i915_selftest.live)
return 0;
 
+   __wait_gsc_proxy_completed(pdev_to_i915(pdev));
+
err = run_selftests(live, pdev_to_i915(pdev));
if (err) {
i915_selftest.live = err;
@@ -227,6 +251,8 @@ int i915_perf_selftests(struct pci_dev *pdev)
if (!i915_selftest.perf)
return 0;
 
+   __wait_gsc_proxy_completed(pdev_to_i915(pdev));
+
err = run_selftests(perf, pdev_to_i915(pdev));
if (err) {
i915_selftest.perf = err;

base-commit: 57ea1a97c50c63c77e3bfa46ee486e8a451be5e7
-- 
2.39.0

[RFC PATCH 2/3] drm/virtio: new fence for every plane update

Having a fence linked to a virtio_gpu_framebuffer in plane update sequence
would cause conflict when several planes referencing the same framebuffer
especially when those planes are updated concurrently (e.g. Xorg screen
covering multi-displays configured for an extended mode). So it is better
for the fence to be created for every plane update event then link it to
the plane state since each plane update comes with a new plane state obj.

The plane state for virtio-gpu, "struct virtio_gpu_plane_state" is added for
this. This structure represents drm_plane_state and it contains the reference
to virtio_gpu_fence, which was previously in
"struct virtio_gpu_framebuffer".

"virtio_gpu_plane_duplicate_state" and "virtio_gpu_plane_destroy_state" were
added as well to manage virtio_gpu_plane_state.

Several drm helpers were slightly modified accordingly to use the fence in new
plane state structure. virtio_gpu_plane_cleanup_fb was completely removed as
none of code in the function are not required.

Also, the condition for adding fence, (plane->state->fb != new_state->fb) was
removed for the sychronous FB update even when the same FB is flushed again
consecutively.

Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 +++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 76 +++---
 2 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 4126c384286b..61fd37f95fbd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -191,6 +191,13 @@ struct virtio_gpu_framebuffer {
 #define to_virtio_gpu_framebuffer(x) \
container_of(x, struct virtio_gpu_framebuffer, base)
 
+struct virtio_gpu_plane_state {
+   struct drm_plane_state base;
+   struct virtio_gpu_fence *fence;
+};
+#define to_virtio_gpu_plane_state(x) \
+   container_of(x, struct virtio_gpu_plane_state, base)
+
 struct virtio_gpu_queue {
struct virtqueue *vq;
spinlock_t qlock;
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c 
b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a2e045f3a000..a063f06ab6c5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -66,12 +66,36 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
return format;
 }
 
+static struct
+drm_plane_state *virtio_gpu_plane_duplicate_state(struct drm_plane *plane)
+{
+   struct virtio_gpu_plane_state *new;
+
+   if (WARN_ON(!plane->state))
+   return NULL;
+
+   new = kzalloc(sizeof(*new), GFP_KERNEL);
+   if (!new)
+   return NULL;
+
+   __drm_atomic_helper_plane_duplicate_state(plane, >base);
+
+   return >base;
+}
+
+static void virtio_gpu_plane_destroy_state(struct drm_plane *plane,
+  struct drm_plane_state *state)
+{
+   __drm_atomic_helper_plane_destroy_state(state);
+   kfree(to_virtio_gpu_plane_state(state));
+}
+
 static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
.update_plane   = drm_atomic_helper_update_plane,
.disable_plane  = drm_atomic_helper_disable_plane,
.reset  = drm_atomic_helper_plane_reset,
-   .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
-   .atomic_destroy_state   = drm_atomic_helper_plane_destroy_state,
+   .atomic_duplicate_state = virtio_gpu_plane_duplicate_state,
+   .atomic_destroy_state   = virtio_gpu_plane_destroy_state,
 };
 
 static int virtio_gpu_plane_atomic_check(struct drm_plane *plane,
@@ -128,11 +152,13 @@ static void virtio_gpu_resource_flush(struct drm_plane 
*plane,
struct drm_device *dev = plane->dev;
struct virtio_gpu_device *vgdev = dev->dev_private;
struct virtio_gpu_framebuffer *vgfb;
+   struct virtio_gpu_plane_state *vgplane_st;
struct virtio_gpu_object *bo;
 
vgfb = to_virtio_gpu_framebuffer(plane->state->fb);
+   vgplane_st = to_virtio_gpu_plane_state(plane->state);
bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
-   if (vgfb->fence) {
+   if (vgplane_st->fence) {
struct virtio_gpu_object_array *objs;
 
objs = virtio_gpu_array_alloc(1);
@@ -141,13 +167,12 @@ static void virtio_gpu_resource_flush(struct drm_plane 
*plane,
virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
virtio_gpu_array_lock_resv(objs);
virtio_gpu_cmd_resource_flush(vgdev, bo->hw_res_handle, x, y,
- width, height, objs, vgfb->fence);
+ width, height, objs,
+ vgplane_st->fence);
virtio_gpu_notify(vgdev);
-
-   dma_fence_wait_timeout(>fence->f, true,
+

[RFC PATCH 3/3] drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

This helper is needed for framebuffer synchronization. Old framebuffer data
is often displayed on the guest display without this helper.

Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c 
b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a063f06ab6c5..e197299489ce 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "virtgpu_drv.h"
 
@@ -271,6 +272,9 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane 
*plane,
vgfb = to_virtio_gpu_framebuffer(new_state->fb);
vgplane_st = to_virtio_gpu_plane_state(new_state);
bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
+
+   drm_gem_plane_helper_prepare_fb(plane, new_state);
+
if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
return 0;
 
-- 
2.20.1

[RFC PATCH 1/3] drm/virtio: .release ops for virtgpu fence release

virtio_gpu_fence_release is added to free virtio-gpu-fence
upon release of dma_fence.

Cc: Gerd Hoffmann 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c 
b/drivers/gpu/drm/virtio/virtgpu_fence.c
index f28357dbde35..ba659ac2a51d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -63,12 +63,20 @@ static void virtio_gpu_timeline_value_str(struct dma_fence 
*f, char *str,
 (u64)atomic64_read(>drv->last_fence_id));
 }
 
+static void virtio_gpu_fence_release(struct dma_fence *f)
+{
+   struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
+
+   kfree(fence);
+}
+
 static const struct dma_fence_ops virtio_gpu_fence_ops = {
.get_driver_name = virtio_gpu_get_driver_name,
.get_timeline_name   = virtio_gpu_get_timeline_name,
.signaled= virtio_gpu_fence_signaled,
.fence_value_str = virtio_gpu_fence_value_str,
.timeline_value_str  = virtio_gpu_timeline_value_str,
+   .release = virtio_gpu_fence_release,
 };
 
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device 
*vgdev,
-- 
2.20.1

[PFC PATCH 0/3] drm/virtio: synchronous guest framebuffer update

"Resubmission"

This series is for fixing issues regarding scanout synchronization with
host (e.g. QEMU/KVM) that uses virtio-gpu. This was submitted a while ago
but didn't get enough feedback/reviews so I am trying it again. This is a
rebased version. And the previous version is at
https://lists.freedesktop.org/archives/dri-devel/2022-September/373782.html

And very first version that has some feedbacks can be found here:
https://www.spinics.net/lists/dri-devel/msg349641.html

Dongwon Kim (3):
  drm/virtio: .release ops for virtgpu fence release
  drm/virtio: new fence for every plane update
  drm/virtio: drm_gem_plane_helper_prepare_fb for obj synchronization

 drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 +++
 drivers/gpu/drm/virtio/virtgpu_fence.c |  8 +++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 80 +++---
 3 files changed, 63 insertions(+), 32 deletions(-)

-- 
2.20.1

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf  wrote:
>
> From: Frieder Schrempf 
>
> According to the documentation [1] the proper enable flow is:
>
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
>
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
>
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
>
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
>
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>
> Signed-off-by: Frieder Schrempf 
> ---
> Changes for v2:
> * Drop RFC
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..9775779721d9 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim 
> *dsi)
> reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> reg &= ~DSIM_STOP_STATE_CNT_MASK;
> reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +   if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +   reg |= DSIM_FORCE_STOP_STATE;
> +
> samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>
> reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0x);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct 
> drm_bridge *bridge,
> ret = samsung_dsim_init(dsi);
> if (ret)
> return;
> +
> +   samsung_dsim_set_display_mode(dsi);
> +   samsung_dsim_set_display_enable(dsi, true);
> }
>  }
>
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct 
> drm_bridge *bridge,
>struct drm_bridge_state 
> *old_bridge_state)
>  {
> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +   u32 reg;
>
> -   samsung_dsim_set_display_mode(dsi);
> -   samsung_dsim_set_display_enable(dsi, true);
> +   if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +   samsung_dsim_set_display_mode(dsi);
> +   samsung_dsim_set_display_enable(dsi, true);
> +   } else {
> +   reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +   reg &= ~DSIM_FORCE_STOP_STATE;
> +   samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +   }
>
> dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct 
> drm_bridge *bridge,
> struct drm_bridge_state 
> *old_bridge_state)
>  {
> struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +   u32 reg;
>
> if (!(dsi->state & DSIM_STATE_ENABLED))
> return;
>
> +   if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +   reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +   reg |= DSIM_FORCE_STOP_STATE;
> +   samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +   }
> +
> dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }
>
> --
> 2.40.0
>

Hi Frieder,

I found this patch to break mipi-dsi display on my board which has:
 - FocalTech FT5406 10pt touch controller (with no interrupt)
 - Powertip PH800480T013-IDF02 compatible panel
 - Toshiba TC358762 compatible DSI to DBI bridge
 - ATTINY based regulator used for backlight controller and panel enable

I enable this via a dt overlay in a pending patch
imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
6.5-rc1 which has this patch.

The issue appears as:
[6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
64 01 05 00 00 00
[6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)

Instead of
[1.011729] samsung-dsim 32e1.dsi: supply vddcore not found,
using dummy regulator
[1.019829] samsung-dsim 32e1.dsi: supply vddio not found,
using dummy regulator
[5.649928] samsung-dsim 32e1.dsi:
[drm:samsung_dsim_host_attach] Attached tc358762 device

I'm curious what board/panel were you needing this for and do you have
any ideas why it broke my setup?

I'm also curious what board/panel Alexander tested this with and if
Adam or Jagan (or others) have tested this with their hardware?

best regards,

Tim
[1] 
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.2127186-1-thar...@gateworks.com/

Re: 6.5-rc1 breakage in samsung-dsim

On Wed, Jul 12, 2023 at 2:43 PM Marek Vasut  wrote:
>
> On 7/12/23 20:52, Tim Harvey wrote:
> > Greetings,
>
> Tim,
>
> > I've noticed a regression in 6.5-rc1 that I'm having trouble bisecting
> > between 6.4 with regards to imx8mm MIPI DSI.
> >
> > I'm testing on an imx8mm-venice-gw72xx-0x with the following display:
> >   - Powertip PH800480T013-IDF02 compatible panel
> >   - Toshiba TC358762 compatible DSI to DBI bridge
> >   - ATTINY based regulator used for backlight controller and panel enable
>
> You mean RPi 7" display wired to non-RPi hardware like many people do ? ;-)

yup good source of readily available cheap expansion hardware :)

>
> > I'm using a dt overlay to support this [1] which works on 6.4 but on
> > 6.5-rc1 I get the following error:
> > [6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
> > 64 01 05 00 00 00
> > [6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)
> >
> > I'm trying to bisect this for some reason. Does anyone have any idea
> > what may be causing this or how I can debug it?
>
> Try and do something like ...
>
> git revert --no-edit v6.4..v6.5-rc1 -- drivers/gpu/drm/bridge/tc358762.c
> drivers/gpu/drm/bridge/samsung-dsim.c drivers/regulator/*attiny*.c

$ git revert --no-edit v6.4...v6.5-rc1 --
drivers/gpu/drm/bridge/tc358762.c
drivers/gpu/drm/bridge/samsung-dsim.c drivers/regulator/*attiny*.c
fatal: bad revision 'drivers/gpu/drm/bridge/tc358762.c'

your filenames look correct to me however:
$ ls drivers/gpu/drm/bridge/tc358762.c
drivers/gpu/drm/bridge/samsung-dsim.c drivers/regulator/*attiny*.c
drivers/gpu/drm/bridge/samsung-dsim.c
drivers/gpu/drm/bridge/tc358762.c
drivers/regulator/rpi-panel-attiny-regulator.c

is that intended to revert all patches between v6.4 and v6.5-rc1 in
files matching that file pattern? That would be very useful indeed if
I can figure out the syntax!

I did by hand revert the patches to samsung-dsim by hand and found the
breakage to occur with commit 0c14d3130654 ("drm: bridge:
samsung-dsim: Fix i.MX8M enable flow to meet spec"). Reverting that
works for me.

I'll respond to that thread directly but I would love to know how to
revert a patches to a series of files like you suggested if you know
what's wrong with the syntax.

thanks,

Tim



>
> (I might have the filenames wrong)
>
> Does that start working afterward ?
>
> If so, you can reverse-bisect on the reverts.
>
> I wouldn't be surprised if this was somehow related to the non-burst
> mode delay calculation again, sigh.
>
> [...]

[PATCH] drm/msm: Fix hw_fence error path cleanup

2023-07-12 Thread Rob Clark

From: Rob Clark 

In an error path where the submit is free'd without the job being run,
the hw_fence pointer is simply a kzalloc'd block of memory.  In this
case we should just kfree() it, rather than trying to decrement it's
reference count.  Fortunately we can tell that this is the case by
checking for a zero refcount, since if the job was run, the submit would
be holding a reference to the hw_fence.

Fixes: f94e6a51e17c ("drm/msm: Pre-allocate hw_fence")
Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/msm_fence.c  |  6 ++
 drivers/gpu/drm/msm/msm_gem_submit.c | 14 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index 96599ec3eb78..1a5d4f1c8b42 100644
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -191,6 +191,12 @@ msm_fence_init(struct dma_fence *fence, struct 
msm_fence_context *fctx)
 
f->fctx = fctx;
 
+   /*
+* Until this point, the fence was just some pre-allocated memory,
+* no-one should have taken a reference to it yet.
+*/
+   WARN_ON(kref_read(>refcount));
+
dma_fence_init(>base, _fence_ops, >spinlock,
   fctx->context, ++fctx->last_fence);
 }
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c 
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3f1aa4de3b87..9d66498cdc04 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -86,7 +86,19 @@ void __msm_gem_submit_destroy(struct kref *kref)
}
 
dma_fence_put(submit->user_fence);
-   dma_fence_put(submit->hw_fence);
+
+   /*
+* If the submit is freed before msm_job_run(), then hw_fence is
+* just some pre-allocated memory, not a reference counted fence.
+* Once the job runs and the hw_fence is initialized, it will
+* have a refcount of at least one, since the submit holds a ref
+* to the hw_fence.
+*/
+   if (kref_read(>hw_fence->refcount) == 0) {
+   kfree(submit->hw_fence);
+   } else {
+   dma_fence_put(submit->hw_fence);
+   }
 
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
-- 
2.41.0

Re: [PATCH v5 3/5] drm/msm/dpu: rename all hw_intf structs to have dpu_hw prefix


On 12/07/2023 04:20, Abhinav Kumar wrote:

dpu_hw_intf has a few instances of structs which do not have
the dpu_hw prefix. Lets fix this by renaming those structs
and updating the usage of those accordingly.

Signed-off-by: Abhinav Kumar 
---
  .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 18 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c|  6 +++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 12 ++--
  3 files changed, 18 insertions(+), 18 deletions(-)


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

Re: [PATCH v5 4/5] drm/msm/dpu: rename enable_compression() to program_intf_cmd_cfg()


On 12/07/2023 04:20, Abhinav Kumar wrote:

Rename the intf's enable_compression() op to program_intf_cmd_cfg()
and allow it to accept a struct intf_cmd_mode_cfg to program
all the bits at once. This can be re-used by widebus later on as
well as it touches the same register.

changes in v5:
- rename struct intf_cmd_mode_cfg to dpu_hw_intf_cmd_mode_cfg
- remove couple of comments

Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 ++--
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c  | 8 +---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h  | 9 +++--
  3 files changed, 18 insertions(+), 7 deletions(-)


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

[PATCH v4 05/11] drm/msm/dpu: rework indentation in dpu_core_perf

dpu_core_perf.c contains several multi-line conditions which are hard to
comprehent because of the indentation. Rework the identation of these
conditions to make it easier to understand them.

Reviewed-by: Abhinav Kumar 
Acked-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index f9f44cfcfbf2..841e1fc0c6a7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -173,8 +173,8 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 
drm_for_each_crtc(tmp_crtc, crtc->dev) {
if (tmp_crtc->enabled &&
-   (dpu_crtc_get_client_type(tmp_crtc) ==
-   curr_client_type) && (tmp_crtc != crtc)) {
+   dpu_crtc_get_client_type(tmp_crtc) == curr_client_type &&
+   tmp_crtc != crtc) {
struct dpu_crtc_state *tmp_cstate =
to_dpu_crtc_state(tmp_crtc->state);
 
@@ -365,10 +365,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
update_bus = true;
}
 
-   if ((params_changed &&
-   (new->core_clk_rate > old->core_clk_rate)) ||
-   (!params_changed &&
-   (new->core_clk_rate < old->core_clk_rate))) {
+   if ((params_changed && new->core_clk_rate > old->core_clk_rate) 
||
+   (!params_changed && new->core_clk_rate < 
old->core_clk_rate)) {
old->core_clk_rate = new->core_clk_rate;
update_clk = true;
}
-- 
2.39.2

[PATCH v4 06/11] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

The stop_req is true only in the dpu_crtc_disable() case, when
crtc->enable has already been set to false. This renders the stop_req
argument useless. Remove it completely.

Reviewed-by: Abhinav Kumar 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  6 +++---
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 841e1fc0c6a7..3b3c2659297d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -315,7 +315,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
 }
 
 int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
-   int params_changed, bool stop_req)
+ int params_changed)
 {
struct dpu_core_perf_params *new, *old;
bool update_bus = false, update_clk = false;
@@ -339,13 +339,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
dpu_crtc = to_dpu_crtc(crtc);
dpu_cstate = to_dpu_crtc_state(crtc->state);
 
-   DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
-   crtc->base.id, stop_req, kms->perf.core_clk_rate);
+   DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
+   crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
 
old = _crtc->cur_perf;
new = _cstate->new_perf;
 
-   if (crtc->enabled && !stop_req) {
+   if (crtc->enabled) {
/*
 * cases for bus bandwidth update.
 * 1. new bandwidth vote - "ab or ib vote" is higher
@@ -378,7 +378,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
}
 
trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
-   new->core_clk_rate, stop_req, update_bus, update_clk);
+   new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
 
if (update_bus) {
ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
@@ -398,7 +398,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 
DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
 
-   trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
+   trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, 
clk_rate);
 
clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
ret = dev_pm_opp_set_rate(>pdev->dev, clk_rate);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index c965dfbc3007..c0097b67f9dd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -75,11 +75,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
  * dpu_core_perf_crtc_update - update performance of the given crtc
  * @crtc: Pointer to crtc
  * @params_changed: true if crtc parameters are modified
- * @stop_req: true if this is a stop request
  * return: zero if success, or error code otherwise
  */
 int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
-   int params_changed, bool stop_req);
+ int params_changed);
 
 /**
  * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 1edf2b6b0a26..8ce7586e2ddf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
 void dpu_crtc_complete_commit(struct drm_crtc *crtc)
 {
trace_dpu_crtc_complete_commit(DRMID(crtc));
-   dpu_core_perf_crtc_update(crtc, 0, false);
+   dpu_core_perf_crtc_update(crtc, 0);
_dpu_crtc_complete_flip(crtc);
 }
 
@@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
return;
 
/* update performance setting before crtc kickoff */
-   dpu_core_perf_crtc_update(crtc, 1, false);
+   dpu_core_perf_crtc_update(crtc, 1);
 
/*
 * Final plane updates: Give each plane a chance to complete all
@@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
atomic_set(_crtc->frame_pending, 0);
}
 
-   dpu_core_perf_crtc_update(crtc, 0, true);
+   dpu_core_perf_crtc_update(crtc, 0);
 
drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
-- 
2.39.2

[PATCH v4 08/11] drm/msm/dpu: remove unused fields from struct dpu_core_perf

Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
are not used by the code.

Reviewed-by: Konrad Dybcio 
Reviewed-by: Abhinav Kumar 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 3 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 2 +-
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 27a0312bd140..d8c88ce5190d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -497,15 +497,12 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
 
perf->max_core_clk_rate = 0;
perf->core_clk = NULL;
-   perf->dev = NULL;
 }
 
 int dpu_core_perf_init(struct dpu_core_perf *perf,
-   struct drm_device *dev,
const struct dpu_perf_cfg *perf_cfg,
struct clk *core_clk)
 {
-   perf->dev = dev;
perf->perf_cfg = perf_cfg;
perf->core_clk = core_clk;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index f4b84e67138c..e718d523ff30 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -36,8 +36,6 @@ struct dpu_core_perf_tune {
 
 /**
  * struct dpu_core_perf - definition of core performance context
- * @dev: Pointer to drm device
- * @debugfs_root: top level debug folder
  * @perf_cfg: Platform-specific performance configuration
  * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
@@ -49,8 +47,6 @@ struct dpu_core_perf_tune {
  * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
  */
 struct dpu_core_perf {
-   struct drm_device *dev;
-   struct dentry *debugfs_root;
const struct dpu_perf_cfg *perf_cfg;
struct clk *core_clk;
u64 core_clk_rate;
@@ -95,12 +91,10 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
 /**
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
- * @dev: Pointer to drm device
  * @perf_cfg: Pointer to platform performance configuration
  * @core_clk: pointer to core clock
  */
 int dpu_core_perf_init(struct dpu_core_perf *perf,
-   struct drm_device *dev,
const struct dpu_perf_cfg *perf_cfg,
struct clk *core_clk);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 074c032cd24e..80e08302680c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1156,7 +1156,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms->hw_vbif[vbif->id] = hw;
}
 
-   rc = dpu_core_perf_init(_kms->perf, dev, dpu_kms->catalog->perf,
+   rc = dpu_core_perf_init(_kms->perf, dpu_kms->catalog->perf,
msm_clk_bulk_get_clock(dpu_kms->clocks, 
dpu_kms->num_clocks, "core"));
if (rc) {
DPU_ERROR("failed to init perf %d\n", rc);
-- 
2.39.2

[PATCH v4 11/11] drm/msm/dpu: drop dpu_core_perf_destroy()

This function does nothing, just clears one struct field. Drop it now.

Acked-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 10 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  1 -
 3 files changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 16a4d6c67f4d..31813a322afd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -486,16 +486,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, 
struct dentry *parent)
 }
 #endif
 
-void dpu_core_perf_destroy(struct dpu_core_perf *perf)
-{
-   if (!perf) {
-   DPU_ERROR("invalid parameters\n");
-   return;
-   }
-
-   perf->max_core_clk_rate = 0;
-}
-
 int dpu_core_perf_init(struct dpu_core_perf *perf,
const struct dpu_perf_cfg *perf_cfg,
unsigned long max_core_clk_rate)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 8cc55752db5e..4186977390bd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -78,12 +78,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
  */
 void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
 
-/**
- * dpu_core_perf_destroy - destroy the given core performance context
- * @perf: Pointer to core performance context
- */
-void dpu_core_perf_destroy(struct dpu_core_perf *perf);
-
 /**
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 5bfea4868e43..76ba86d3e436 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1212,7 +1212,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
return 0;
 
 drm_obj_init_err:
-   dpu_core_perf_destroy(_kms->perf);
 hw_intr_init_err:
 perf_err:
 power_error:
-- 
2.39.2

[PATCH v4 03/11] drm/msm/dpu: core_perf: bail earlier if there are no ICC paths

Skip bandwidth aggregation and return early if there are no interconnect
paths defined for the DPU device.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 333dcfe57800..05d340aa18c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -237,11 +237,11 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms 
*kms,
int i, ret = 0;
u64 avg_bw;
 
-   dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), 
);
-
if (!kms->num_paths)
return 0;
 
+   dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), 
);
+
avg_bw = perf.bw_ctl;
do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
 
-- 
2.39.2

[PATCH v4 10/11] drm/msm/dpu: move max clock decision to dpu_kms.

dpu_core_perf should not make decisions on the maximum possible core
clock rate. Pass the value from dpu_kms_hw_init().

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 11 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 13 +++--
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 896e87b13dbe..16a4d6c67f4d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -494,21 +494,14 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
}
 
perf->max_core_clk_rate = 0;
-   perf->core_clk = NULL;
 }
 
 int dpu_core_perf_init(struct dpu_core_perf *perf,
const struct dpu_perf_cfg *perf_cfg,
-   struct clk *core_clk)
+   unsigned long max_core_clk_rate)
 {
perf->perf_cfg = perf_cfg;
-   perf->core_clk = core_clk;
-
-   perf->max_core_clk_rate = clk_get_rate(core_clk);
-   if (!perf->max_core_clk_rate) {
-   DPU_DEBUG("optional max core clk rate, use default\n");
-   perf->max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
-   }
+   perf->max_core_clk_rate = max_core_clk_rate;
 
return 0;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e718d523ff30..8cc55752db5e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -12,8 +12,6 @@
 
 #include "dpu_hw_catalog.h"
 
-#defineDPU_PERF_DEFAULT_MAX_CORE_CLK_RATE  41250
-
 /**
  * struct dpu_core_perf_params - definition of performance parameters
  * @max_per_pipe_ib: maximum instantaneous bandwidth request
@@ -37,7 +35,6 @@ struct dpu_core_perf_tune {
 /**
  * struct dpu_core_perf - definition of core performance context
  * @perf_cfg: Platform-specific performance configuration
- * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
  * @max_core_clk_rate: maximum allowable core clock rate
  * @perf_tune: debug control for performance tuning
@@ -48,7 +45,6 @@ struct dpu_core_perf_tune {
  */
 struct dpu_core_perf {
const struct dpu_perf_cfg *perf_cfg;
-   struct clk *core_clk;
u64 core_clk_rate;
u64 max_core_clk_rate;
struct dpu_core_perf_tune perf_tune;
@@ -92,11 +88,11 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
  * @perf_cfg: Pointer to platform performance configuration
- * @core_clk: pointer to core clock
+ * @max_core_clk_rate: Maximum core clock rate
  */
 int dpu_core_perf_init(struct dpu_core_perf *perf,
const struct dpu_perf_cfg *perf_cfg,
-   struct clk *core_clk);
+   unsigned long max_core_clk_rate);
 
 struct dpu_kms;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 80e08302680c..5bfea4868e43 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1051,11 +1051,14 @@ unsigned long dpu_kms_get_clk_rate(struct dpu_kms 
*dpu_kms, char *clock_name)
return clk_get_rate(clk);
 }
 
+#defineDPU_PERF_DEFAULT_MAX_CORE_CLK_RATE  41250
+
 static int dpu_kms_hw_init(struct msm_kms *kms)
 {
struct dpu_kms *dpu_kms;
struct drm_device *dev;
int i, rc = -EINVAL;
+   unsigned long max_core_clk_rate;
u32 core_rev;
 
if (!kms) {
@@ -1156,8 +1159,14 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms->hw_vbif[vbif->id] = hw;
}
 
-   rc = dpu_core_perf_init(_kms->perf, dpu_kms->catalog->perf,
-   msm_clk_bulk_get_clock(dpu_kms->clocks, 
dpu_kms->num_clocks, "core"));
+   /* TODO: use the same max_freq as in dpu_kms_hw_init */
+   max_core_clk_rate = dpu_kms_get_clk_rate(dpu_kms, "core");
+   if (!max_core_clk_rate) {
+   DPU_DEBUG("max core clk rate not determined, using default\n");
+   max_core_clk_rate = DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE;
+   }
+
+   rc = dpu_core_perf_init(_kms->perf, dpu_kms->catalog->perf, 
max_core_clk_rate);
if (rc) {
DPU_ERROR("failed to init perf %d\n", rc);
goto perf_err;
-- 
2.39.2

[PATCH v4 09/11] drm/msm/dpu: remove extra clk_round_rate() call

The dev_pm_opp_set_rate() already contains a call for clk_round_rate for
the passed value. Stop calling it manually from
_dpu_core_perf_get_core_clk_rate(). It is slightly incorrect to call it
this way, as we should round the final calculated clock rate rather than
rounding all the intermediate values.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index d8c88ce5190d..896e87b13dbe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -299,8 +299,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
dpu_cstate = to_dpu_crtc_state(crtc->state);
clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
clk_rate);
-   clk_rate = clk_round_rate(kms->perf.core_clk,
-   clk_rate);
}
}
 
-- 
2.39.2

[PATCH v4 07/11] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
full-featured catalog data.

Acked-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 73 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  2 +-
 3 files changed, 35 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 3b3c2659297d..27a0312bd140 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -33,11 +33,11 @@ enum dpu_perf_mode {
 
 /**
  * _dpu_core_perf_calc_bw() - to calculate BW per crtc
- * @kms:  pointer to the dpu_kms
+ * @perf_cfg: performance configuration
  * @crtc: pointer to a crtc
  * Return: returns aggregated BW for all planes in crtc.
  */
-static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
+static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
struct drm_crtc *crtc)
 {
struct drm_plane *plane;
@@ -53,7 +53,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
crtc_plane_bw += pstate->plane_fetch_bw;
}
 
-   bw_factor = kms->catalog->perf->bw_inefficiency_factor;
+   bw_factor = perf_cfg->bw_inefficiency_factor;
if (bw_factor) {
crtc_plane_bw *= bw_factor;
do_div(crtc_plane_bw, 100);
@@ -64,12 +64,12 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
 
 /**
  * _dpu_core_perf_calc_clk() - to calculate clock per crtc
- * @kms:  pointer to the dpu_kms
+ * @perf_cfg: performance configuration
  * @crtc: pointer to a crtc
  * @state: pointer to a crtc state
  * Return: returns max clk for all planes in crtc.
  */
-static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
+static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg,
struct drm_crtc *crtc, struct drm_crtc_state *state)
 {
struct drm_plane *plane;
@@ -90,7 +90,7 @@ static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
crtc_clk = max(pstate->plane_clk, crtc_clk);
}
 
-   clk_factor = kms->catalog->perf->clk_inefficiency_factor;
+   clk_factor = perf_cfg->clk_inefficiency_factor;
if (clk_factor) {
crtc_clk *= clk_factor;
do_div(crtc_clk, 100);
@@ -106,30 +106,32 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc 
*crtc)
return to_dpu_kms(priv->kms);
 }
 
-static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
-   struct drm_crtc *crtc,
-   struct drm_crtc_state *state,
-   struct dpu_core_perf_params *perf)
+static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf,
+struct drm_crtc *crtc,
+struct drm_crtc_state *state,
+struct dpu_core_perf_params *perf)
 {
-   if (!kms || !kms->catalog || !crtc || !state || !perf) {
+   const struct dpu_perf_cfg *perf_cfg = core_perf->perf_cfg;
+
+   if (!perf_cfg || !crtc || !state || !perf) {
DPU_ERROR("invalid parameters\n");
return;
}
 
memset(perf, 0, sizeof(struct dpu_core_perf_params));
 
-   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
+   if (core_perf->perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
perf->bw_ctl = 0;
perf->max_per_pipe_ib = 0;
perf->core_clk_rate = 0;
-   } else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
-   perf->bw_ctl = kms->perf.fix_core_ab_vote;
-   perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
-   perf->core_clk_rate = kms->perf.fix_core_clk_rate;
+   } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) {
+   perf->bw_ctl = core_perf->fix_core_ab_vote;
+   perf->max_per_pipe_ib = core_perf->fix_core_ib_vote;
+   perf->core_clk_rate = core_perf->fix_core_clk_rate;
} else {
-   perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
-   perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
-   perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
+   perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+   perf->max_per_pipe_ib = perf_cfg->min_dram_ib;
+   perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, 
state);
}
 
DRM_DEBUG_ATOMIC(
@@ -154,10 +156,6 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
}
 
kms = _dpu_crtc_get_kms(crtc);
-   if (!kms->catalog) {
-   DPU_ERROR("invalid parameters\n");
-   return 0;
-   }
 
/* we only need bandwidth check on real-time clients (interfaces) */
if

[PATCH v4 02/11] drm/msm/dpu: core_perf: extract bandwidth aggregation function

In preparation to refactoring the dpu_core_perf debugfs interface,
extract the bandwidth aggregation function from
_dpu_core_perf_crtc_update_bus().

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 39 +++
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 1d9d83d7b99e..333dcfe57800 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -206,33 +206,38 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
return 0;
 }
 
-static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
-   struct drm_crtc *crtc)
+static void dpu_core_perf_aggregate(struct drm_device *ddev,
+   enum dpu_crtc_client_type curr_client_type,
+   struct dpu_core_perf_params *perf)
 {
-   struct dpu_core_perf_params perf = { 0 };
-   enum dpu_crtc_client_type curr_client_type
-   = dpu_crtc_get_client_type(crtc);
-   struct drm_crtc *tmp_crtc;
struct dpu_crtc_state *dpu_cstate;
-   int i, ret = 0;
-   u64 avg_bw;
+   struct drm_crtc *tmp_crtc;
 
-   drm_for_each_crtc(tmp_crtc, crtc->dev) {
+   drm_for_each_crtc(tmp_crtc, ddev) {
if (tmp_crtc->enabled &&
-   curr_client_type ==
-   dpu_crtc_get_client_type(tmp_crtc)) {
+   curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
 
-   perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
-   dpu_cstate->new_perf.max_per_pipe_ib);
+   perf->max_per_pipe_ib = max(perf->max_per_pipe_ib,
+   
dpu_cstate->new_perf.max_per_pipe_ib);
 
-   perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
+   perf->bw_ctl += dpu_cstate->new_perf.bw_ctl;
 
-   DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
- tmp_crtc->base.id,
- dpu_cstate->new_perf.bw_ctl, kms->num_paths);
+   DRM_DEBUG_ATOMIC("crtc=%d bw=%llu\n",
+tmp_crtc->base.id,
+dpu_cstate->new_perf.bw_ctl);
}
}
+}
+
+static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
+   struct drm_crtc *crtc)
+{
+   struct dpu_core_perf_params perf = { 0 };
+   int i, ret = 0;
+   u64 avg_bw;
+
+   dpu_core_perf_aggregate(crtc->dev, dpu_crtc_get_client_type(crtc), 
);
 
if (!kms->num_paths)
return 0;
-- 
2.39.2

[PATCH v4 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides

The values in struct dpu_core_perf_tune are fixed per the core perf
mode. Drop the 'tune' values and substitute them with known values when
performing perf management.

Note: min_bus_vote was not used at all, so it is just silently dropped.

Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 27 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  4 ---
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 05d340aa18c5..f9f44cfcfbf2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
 
 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 {
-   u64 clk_rate = kms->perf.perf_tune.min_core_clk;
+   u64 clk_rate;
struct drm_crtc *crtc;
struct dpu_crtc_state *dpu_cstate;
 
+   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
+   return kms->perf.fix_core_clk_rate;
+
+   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
+   return kms->perf.max_core_clk_rate;
+
drm_for_each_crtc(crtc, kms->dev) {
if (crtc->enabled) {
dpu_cstate = to_dpu_crtc_state(crtc->state);
@@ -305,11 +311,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
}
}
 
-   if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
-   clk_rate = kms->perf.fix_core_clk_rate;
-
-   DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
-
return clk_rate;
 }
 
@@ -397,6 +398,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
if (update_clk) {
clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
 
+   DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
+
trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
 
clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
@@ -418,7 +421,6 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file,
const char __user *user_buf, size_t count, loff_t *ppos)
 {
struct dpu_core_perf *perf = file->private_data;
-   const struct dpu_perf_cfg *cfg = perf->catalog->perf;
u32 perf_mode = 0;
int ret;
 
@@ -433,14 +435,9 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file,
DRM_INFO("fix performance mode\n");
} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
/* run the driver with max clk and BW vote */
-   perf->perf_tune.min_core_clk = perf->max_core_clk_rate;
-   perf->perf_tune.min_bus_vote =
-   (u64) cfg->max_bw_high * 1000;
DRM_INFO("minimum performance mode\n");
} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
/* reset the perf tune params to 0 */
-   perf->perf_tune.min_core_clk = 0;
-   perf->perf_tune.min_bus_vote = 0;
DRM_INFO("normal performance mode\n");
}
perf->perf_tune.mode = perf_mode;
@@ -456,10 +453,8 @@ static ssize_t _dpu_core_perf_mode_read(struct file *file,
char buf[128];
 
len = scnprintf(buf, sizeof(buf),
-   "mode %d min_mdp_clk %llu min_bus_vote %llu\n",
-   perf->perf_tune.mode,
-   perf->perf_tune.min_core_clk,
-   perf->perf_tune.min_bus_vote);
+   "mode %d\n",
+   perf->perf_tune.mode);
 
return simple_read_from_buffer(buff, count, ppos, buf, len);
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 29bb8ee2bc26..c965dfbc3007 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -29,13 +29,9 @@ struct dpu_core_perf_params {
 /**
  * struct dpu_core_perf_tune - definition of performance tuning control
  * @mode: performance mode
- * @min_core_clk: minimum core clock
- * @min_bus_vote: minimum bus vote
  */
 struct dpu_core_perf_tune {
u32 mode;
-   u64 min_core_clk;
-   u64 min_bus_vote;
 };
 
 /**
-- 
2.39.2

[PATCH v4 01/11] drm/msm/dpu: drop enum dpu_core_perf_data_bus_id

Drop the leftover of bus-client -> interconnect conversion, the enum
dpu_core_perf_data_bus_id.

Fixes: cb88482e2570 ("drm/msm/dpu: clean up references of DPU custom bus 
scaling")
Reviewed-by: Konrad Dybcio 
Reviewed-by: Abhinav Kumar 
Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 13 -
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e3795995e145..29bb8ee2bc26 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -14,19 +14,6 @@
 
 #defineDPU_PERF_DEFAULT_MAX_CORE_CLK_RATE  41250
 
-/**
- * enum dpu_core_perf_data_bus_id - data bus identifier
- * @DPU_CORE_PERF_DATA_BUS_ID_MNOC: DPU/MNOC data bus
- * @DPU_CORE_PERF_DATA_BUS_ID_LLCC: MNOC/LLCC data bus
- * @DPU_CORE_PERF_DATA_BUS_ID_EBI: LLCC/EBI data bus
- */
-enum dpu_core_perf_data_bus_id {
-   DPU_CORE_PERF_DATA_BUS_ID_MNOC,
-   DPU_CORE_PERF_DATA_BUS_ID_LLCC,
-   DPU_CORE_PERF_DATA_BUS_ID_EBI,
-   DPU_CORE_PERF_DATA_BUS_ID_MAX,
-};
-
 /**
  * struct dpu_core_perf_params - definition of performance parameters
  * @max_per_pipe_ib: maximum instantaneous bandwidth request
-- 
2.39.2

[PATCH v4 00/11] drm/msm/dpu: cleanup dpu_core_perf module

Apply several cleanups to the DPU's core_perf module.

Changes since v3:
- Dropped avg_bw type change (Abhinav)
- Removed core_perf from the commit cubject (Abhinav)

Changes since v2:
- Dropped perf tuning patches for now (Abhinav)
- Restored kms variable assignment in dpu_core_perf_crtc_release_bw
  (LKP)
- Fixed description for the last patch (Abhinav)

Changes since v1:
- Reworked overrides for the perf parameters instead of completely
  dropping them. Abhinav described why these overrides are useful.
- Moved max clock rate determination to dpu_kms.c

Dmitry Baryshkov (11):
  drm/msm/dpu: drop enum dpu_core_perf_data_bus_id
  drm/msm/dpu: core_perf: extract bandwidth aggregation function
  drm/msm/dpu: core_perf: bail earlier if there are no ICC paths
  drm/msm/dpu: drop separate dpu_core_perf_tune overrides
  drm/msm/dpu: rework indentation in dpu_core_perf
  drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param
  drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code
  drm/msm/dpu: remove unused fields from struct dpu_core_perf
  drm/msm/dpu: remove extra clk_round_rate() call
  drm/msm/dpu: move max clock decision to dpu_kms.
  drm/msm/dpu: drop dpu_core_perf_destroy()

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 187 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  48 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  14 +-
 4 files changed, 96 insertions(+), 159 deletions(-)

-- 
2.39.2

Re: [PATCH v2 2/8] drm/msm/mdss: correct UBWC programming for SM8550


On 13/07/2023 01:02, Abhinav Kumar wrote:



On 7/12/2023 5:11 AM, Dmitry Baryshkov wrote:

The SM8550 platform employs newer UBWC decoder, which requires slightly
different programming.

Fixes: a2f33995c19d ("drm/msm: mdss: add support for SM8550")
Signed-off-by: Dmitry Baryshkov 


Reviewed-by: Abhinav Kumar 

Do we also need another fixes tag

Fixes: d68db6069a8e ("drm/msm/mdss: convert UBWC setup to use match data")


No. We do not need to fix the conversion. Only the sm8550 setup.



Also, this was previously part of 
https://patchwork.freedesktop.org/series/118074/ .


In this one its after the bindings change.

For easier picking into -fixes, will you be moving this ahead of the 
bindings change and OR do you want to keep this part of the old series 
as it seems better suited there.


I think even if I pick this for -fixes, rest of this series can be 
rebased without issues. But let me know what you would prefer.



I had rejects with the original series (and 0 reviews), so it was easier 
to push that as a part of this series too.


I'm fine with you pulling either of the patches into -fixes.

--
With best wishes
Dmitry

Re: [PATCH v2 2/8] drm/msm/mdss: correct UBWC programming for SM8550





On 7/12/2023 5:11 AM, Dmitry Baryshkov wrote:

The SM8550 platform employs newer UBWC decoder, which requires slightly
different programming.

Fixes: a2f33995c19d ("drm/msm: mdss: add support for SM8550")
Signed-off-by: Dmitry Baryshkov 


Reviewed-by: Abhinav Kumar 

Do we also need another fixes tag

Fixes: d68db6069a8e ("drm/msm/mdss: convert UBWC setup to use match data")

Also, this was previously part of 
https://patchwork.freedesktop.org/series/118074/ .


In this one its after the bindings change.

For easier picking into -fixes, will you be moving this ahead of the 
bindings change and OR do you want to keep this part of the old series 
as it seems better suited there.


I think even if I pick this for -fixes, rest of this series can be 
rebased without issues. But let me know what you would prefer.

Re: 6.5-rc1 breakage in samsung-dsim

2023-07-12 Thread Marek Vasut


On 7/12/23 20:52, Tim Harvey wrote:

Greetings,


Tim,


I've noticed a regression in 6.5-rc1 that I'm having trouble bisecting
between 6.4 with regards to imx8mm MIPI DSI.

I'm testing on an imx8mm-venice-gw72xx-0x with the following display:
  - Powertip PH800480T013-IDF02 compatible panel
  - Toshiba TC358762 compatible DSI to DBI bridge
  - ATTINY based regulator used for backlight controller and panel enable


You mean RPi 7" display wired to non-RPi hardware like many people do ? ;-)


I'm using a dt overlay to support this [1] which works on 6.4 but on
6.5-rc1 I get the following error:
[6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
64 01 05 00 00 00
[6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)

I'm trying to bisect this for some reason. Does anyone have any idea
what may be causing this or how I can debug it?


Try and do something like ...

git revert --no-edit v6.4..v6.5-rc1 -- drivers/gpu/drm/bridge/tc358762.c 
drivers/gpu/drm/bridge/samsung-dsim.c drivers/regulator/*attiny*.c


(I might have the filenames wrong)

Does that start working afterward ?

If so, you can reverse-bisect on the reverts.

I wouldn't be surprised if this was somehow related to the non-burst 
mode delay calculation again, sigh.


[...]

Re: [PATCH v2 01/15] drm/msm/dsi: Drop unused regulators from QCM2290 14nm DSI PHY config





On 6/27/2023 1:14 PM, Marijn Suijten wrote:

The regulator setup was likely copied from other SoCs by mistake.  Just
like SM6125 the DSI PHY on this platform is not getting power from a
regulator but from the MX power domain.

Fixes: 572e9fd6d14a ("drm/msm/dsi: Add phy configuration for QCM2290")
Signed-off-by: Marijn Suijten 
---
  drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 2 --
  1 file changed, 2 deletions(-)



Reviewed-by: Abhinav Kumar

Re: 6.5-rc1 breakage in samsung-dsim

On Wed, Jul 12, 2023 at 12:24 PM Jagan Teki  wrote:
>
> On Thu, Jul 13, 2023 at 12:22 AM Tim Harvey  wrote:
> >
> > Greetings,
> >
> > I've noticed a regression in 6.5-rc1 that I'm having trouble bisecting
> > between 6.4 with regards to imx8mm MIPI DSI.
> >
> > I'm testing on an imx8mm-venice-gw72xx-0x with the following display:
> >  - Powertip PH800480T013-IDF02 compatible panel
> >  - Toshiba TC358762 compatible DSI to DBI bridge
> >  - ATTINY based regulator used for backlight controller and panel enable
> >
> > I'm using a dt overlay to support this [1] which works on 6.4 but on
> > 6.5-rc1 I get the following error:
> > [6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
> > 64 01 05 00 00 00
> > [6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)
> >
> > I'm trying to bisect this for some reason. Does anyone have any idea
> > what may be causing this or how I can debug it?
>
> What if you can revert or drop the dynamic PHY configuration (HEAD ...
> a617b33f7e51)? can you check it?
>
> Thanks,
> Jagan.

Hi Jagan,

That's one of the first things I tried but it didn't revert cleanly so
I simply did a git checkout of 1a56fcf08ae4 (the patch before
a617b33f7e51) as that was the before the 6 samsung-dsim patches I
suspected. This version also failed but then I did a git log and
noticed only 90 patches before the Linux 6.4-rc1 tag. This of course
predates Linux 6.4 yet a617b33f7e51 was not in 6.4 which is why I got
confused when a git bisect between v6.4 and v6.5-rc1 started bisecting
to patches that pre-dated v6.4. I'm so confused at this point :)

best regards,

Tim

Re: [Nouveau] [PATCH v2] drm/nouveau: bring back blit subchannel for pre nv50 GPUs

2023-07-12 Thread Ben Skeggs

On Fri, 26 May 2023 at 19:11, Karol Herbst  wrote:
>
> 1ba6113a90a0 removed a lot of the kernel GPU channel, but method 0x128
> was important as otherwise the GPU spams us with `CACHE_ERROR` messages.
>
> We use the blit subchannel inside our vblank handling, so we should keep
> at least this part.
>
> v2: Only do it for NV11+ GPUs
>
> Closes: https://gitlab.freedesktop.org/drm/nouveau/-/issues/201
> Fixes: 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
> Signed-off-by: Karol Herbst 
Reviewed-by: Ben Skeggs 

> ---
>  drivers/gpu/drm/nouveau/nouveau_chan.c |  1 +
>  drivers/gpu/drm/nouveau/nouveau_chan.h |  1 +
>  drivers/gpu/drm/nouveau/nouveau_drm.c  | 20 +---
>  3 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.c 
> b/drivers/gpu/drm/nouveau/nouveau_chan.c
> index e648ecd0c1a0..3dfbc374478e 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.c
> @@ -90,6 +90,7 @@ nouveau_channel_del(struct nouveau_channel **pchan)
> if (cli)
> nouveau_svmm_part(chan->vmm->svmm, chan->inst);
>
> +   nvif_object_dtor(>blit);
> nvif_object_dtor(>nvsw);
> nvif_object_dtor(>gart);
> nvif_object_dtor(>vram);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_chan.h 
> b/drivers/gpu/drm/nouveau/nouveau_chan.h
> index e06a8ffed31a..bad7466bd0d5 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_chan.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_chan.h
> @@ -53,6 +53,7 @@ struct nouveau_channel {
> u32 user_put;
>
> struct nvif_object user;
> +   struct nvif_object blit;
>
> struct nvif_event kill;
> atomic_t killed;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index cc7c5b4a05fd..9512f1c2f871 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -369,15 +369,29 @@ nouveau_accel_gr_init(struct nouveau_drm *drm)
> ret = nvif_object_ctor(>channel->user, "drmNvsw",
>NVDRM_NVSW, nouveau_abi16_swclass(drm),
>NULL, 0, >channel->nvsw);
> +
> +   if (ret == 0 && device->info.chipset >= 0x11) {
> +   ret = nvif_object_ctor(>channel->user, "drmBlit",
> +  0x005f, 0x009f,
> +  NULL, 0, >channel->blit);
> +   }
> +
> if (ret == 0) {
> struct nvif_push *push = drm->channel->chan.push;
> -   ret = PUSH_WAIT(push, 2);
> -   if (ret == 0)
> +   ret = PUSH_WAIT(push, 8);
> +   if (ret == 0) {
> +   if (device->info.chipset >= 0x11) {
> +   PUSH_NVSQ(push, NV05F, 0x, 
> drm->channel->blit.handle);
> +   PUSH_NVSQ(push, NV09F, 0x0120, 0,
> +  0x0124, 1,
> +  0x0128, 2);
> +   }
> PUSH_NVSQ(push, NV_SW, 0x, 
> drm->channel->nvsw.handle);
> +   }
> }
>
> if (ret) {
> -   NV_ERROR(drm, "failed to allocate sw class, %d\n", 
> ret);
> +   NV_ERROR(drm, "failed to allocate sw or blit class, 
> %d\n", ret);
> nouveau_accel_gr_fini(drm);
> return;
> }
> --
> 2.40.1
>

Re: [Nouveau] [PATCH] drm/nouveau/acr: Abort loading ACR if no firmware was found

2023-07-12 Thread Ben Skeggs

On Thu, 13 Jul 2023 at 05:31, Dave Airlie  wrote:
>
> On Tue, 23 May 2023 at 19:37, Karol Herbst  wrote:
> >
> > On Mon, May 22, 2023 at 10:18 PM Karol Herbst  wrote:
> > >
> > > This fixes a NULL pointer access inside nvkm_acr_oneinit in case necessary
> > > firmware files couldn't be loaded.
> > >
> > > Closes: https://gitlab.freedesktop.org/drm/nouveau/-/issues/212
> > > Fixes: 4b569ded09fd ("drm/nouveau/acr/ga102: initial support")
> > > Signed-off-by: Karol Herbst 
> > > ---
> > >  drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c 
> > > b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> > > index 795f3a649b12..6388234c352c 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> > > @@ -224,7 +224,7 @@ nvkm_acr_oneinit(struct nvkm_subdev *subdev)
> > > u64 falcons;
> > > int ret, i;
> > >
> > > -   if (list_empty(>hsfw)) {
> > > +   if (list_empty(>hsfw) || !acr->func->wpr_layout) {
> >
> > Now thinking about this, it should probably also check acr->func...
>
> with that fixed if you think you need it,
I don't *think* you do.  I believe modprobe will fail for any case it
can be NULL.

>
> Reviewed-by: Dave Airlie 
>
> >
> > > nvkm_debug(subdev, "No HSFW(s)\n");
> > > nvkm_acr_cleanup(acr);
> > > return 0;
> > > --
> > > 2.40.1
> > >
> >

Re: [PATCH v3 09/11] drm/msm/dpu: core_perf: remove extra clk_round_rate() call





On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:

The dev_pm_opp_set_rate() already contains a call for clk_round_rate for
the passed value. Stop calling it manually from
_dpu_core_perf_get_core_clk_rate(). It is slightly incorrect to call it
this way, as we should round the final calculated clock rate rather than
rounding all the intermediate values.



Change is alright but do we really need a core_perf tag on the subject line?


Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index afd75750380c..a570810c9254 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -299,8 +299,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms 
*kms)
dpu_cstate = to_dpu_crtc_state(crtc->state);
clk_rate = max(dpu_cstate->new_perf.core_clk_rate,
clk_rate);
-   clk_rate = clk_round_rate(kms->perf.core_clk,
-   clk_rate);
}
}

Re: [PATCH v3 08/11] drm/msm/dpu: remove unused fields from struct dpu_core_perf





On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:

Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
are not used by the code.

Reviewed-by: Konrad Dybcio 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 3 ---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 6 --
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   | 2 +-
  3 files changed, 1 insertion(+), 10 deletions(-)



Reviewed-by: Abhinav Kumar

Re: [PATCH v3 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides





On 7/10/2023 7:34 PM, Dmitry Baryshkov wrote:

On 11/07/2023 05:31, Abhinav Kumar wrote:



On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:

The values in struct dpu_core_perf_tune are fixed per the core perf
mode. Drop the 'tune' values and substitute them with known values when
performing perf management.

Note: min_bus_vote was not used at all, so it is just silently dropped.



Interesting . should bring this back properly. Will take it up.


Ack, thanks.




Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ---
  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  4 ---
  2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c

index 05d340aa18c5..348550ac7e51 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -235,7 +235,7 @@ static int _dpu_core_perf_crtc_update_bus(struct 
dpu_kms *kms,

  {
  struct dpu_core_perf_params perf = { 0 };
  int i, ret = 0;
-    u64 avg_bw;
+    u32 avg_bw;


avg_bw seems unused in this patch, so unrelated change?


  if (!kms->num_paths)
  return 0;
@@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct 
drm_crtc *crtc)

  static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
  {
-    u64 clk_rate = kms->perf.perf_tune.min_core_clk;
+    u64 clk_rate;
  struct drm_crtc *crtc;
  struct dpu_crtc_state *dpu_cstate;
+    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
+    return kms->perf.fix_core_clk_rate;
+
+    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
+    return kms->perf.max_core_clk_rate;
+
  drm_for_each_crtc(crtc, kms->dev) {
  if (crtc->enabled) {
  dpu_cstate = to_dpu_crtc_state(crtc->state);
@@ -305,11 +311,6 @@ static u64 
_dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)

  }
  }
-    if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
-    clk_rate = kms->perf.fix_core_clk_rate;
-
-    DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
-


Why dont you move both FIXED and MINIMUM handling below instead of above.

So that they will just override the clk_rate and you can keep this 
useful log here and it matches where the function is.


I can keep the log in the next version. The logic was quite simple: 
there is no need to loop over CRTCs if we know that we are overriding 
the value.




Yes I understood that part. I wanted to keep the log and the function 
together that way we are logging whats the value its going to return.


This patch is logging it at the caller. Thats the only difference.

I am fine either way. Once the avg_bw change is removed, I can ack this.



This chunk looks better that way.

Re: [PATCH] drm/nouveau/acr: Abort loading ACR if no firmware was found

2023-07-12 Thread Dave Airlie

On Tue, 23 May 2023 at 19:37, Karol Herbst  wrote:
>
> On Mon, May 22, 2023 at 10:18 PM Karol Herbst  wrote:
> >
> > This fixes a NULL pointer access inside nvkm_acr_oneinit in case necessary
> > firmware files couldn't be loaded.
> >
> > Closes: https://gitlab.freedesktop.org/drm/nouveau/-/issues/212
> > Fixes: 4b569ded09fd ("drm/nouveau/acr/ga102: initial support")
> > Signed-off-by: Karol Herbst 
> > ---
> >  drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c 
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> > index 795f3a649b12..6388234c352c 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/base.c
> > @@ -224,7 +224,7 @@ nvkm_acr_oneinit(struct nvkm_subdev *subdev)
> > u64 falcons;
> > int ret, i;
> >
> > -   if (list_empty(>hsfw)) {
> > +   if (list_empty(>hsfw) || !acr->func->wpr_layout) {
>
> Now thinking about this, it should probably also check acr->func...

with that fixed if you think you need it,

Reviewed-by: Dave Airlie 

>
> > nvkm_debug(subdev, "No HSFW(s)\n");
> > nvkm_acr_cleanup(acr);
> > return 0;
> > --
> > 2.40.1
> >
>

Re: [PATCH v2 1/8] dt-bindings: display/msm: Add reg bus and rotator interconnects

2023-07-12 Thread Krzysztof Kozlowski

On 12/07/2023 14:11, Dmitry Baryshkov wrote:
> From: Konrad Dybcio 
> 
> Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there are
> other connection paths:
> - a path that connects rotator block to the DDR.
> - a path that needs to be handled to ensure MDSS register access
>   functions properly, namely the "reg bus", a.k.a the CPU-MDSS CFG
>   interconnect.
> 
> Describe these paths bindings to allow using them in device trees and in
> the driver
> 
> Signed-off-by: Konrad Dybcio 
> Signed-off-by: Dmitry Baryshkov 


Acked-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

Re: 6.5-rc1 breakage in samsung-dsim

2023-07-12 Thread Jagan Teki

On Thu, Jul 13, 2023 at 12:22 AM Tim Harvey  wrote:
>
> Greetings,
>
> I've noticed a regression in 6.5-rc1 that I'm having trouble bisecting
> between 6.4 with regards to imx8mm MIPI DSI.
>
> I'm testing on an imx8mm-venice-gw72xx-0x with the following display:
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
>
> I'm using a dt overlay to support this [1] which works on 6.4 but on
> 6.5-rc1 I get the following error:
> [6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)
>
> I'm trying to bisect this for some reason. Does anyone have any idea
> what may be causing this or how I can debug it?

What if you can revert or drop the dynamic PHY configuration (HEAD ...
a617b33f7e51)? can you check it?

Thanks,
Jagan.

Re: [PATCH RFC v1 04/52] drm/armada: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev

2023-07-12 Thread Russell King (Oracle)

On Wed, Jul 12, 2023 at 11:46:14AM +0200, Uwe Kleine-König wrote:
> Prepare dropping the alias "dev" for struct drm_crtc::drm_dev. "drm_dev"
> is the better name as "dev" is usually a struct device pointer.
> 
> No semantic changes.
> 
> Signed-off-by: Uwe Kleine-König 

Reviewed-by: Russell King (Oracle) 

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

6.5-rc1 breakage in samsung-dsim

Greetings,

I've noticed a regression in 6.5-rc1 that I'm having trouble bisecting
between 6.4 with regards to imx8mm MIPI DSI.

I'm testing on an imx8mm-venice-gw72xx-0x with the following display:
 - Powertip PH800480T013-IDF02 compatible panel
 - Toshiba TC358762 compatible DSI to DBI bridge
 - ATTINY based regulator used for backlight controller and panel enable

I'm using a dt overlay to support this [1] which works on 6.4 but on
6.5-rc1 I get the following error:
[6.110585] samsung-dsim 32e6.dsi: xfer timed out: 29 06 00 00
64 01 05 00 00 00
[6.326588] tc358762 32e6.dsi.0: error initializing bridge (-110)

I'm trying to bisect this for some reason. Does anyone have any idea
what may be causing this or how I can debug it?

Best regards,

Tim
[1] 
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.2127186-1-thar...@gateworks.com/

[pull] amdgpu drm-fixes-6.5

2023-07-12 Thread Alex Deucher

Hi Dave, Daniel,

Fixes for 6.5.

The following changes since commit 6725f33228077902ddac2a05e0ab361dee36e4ba:

  Merge tag 'drm-misc-next-fixes-2023-07-06' of 
git://anongit.freedesktop.org/drm/drm-misc into drm-next (2023-07-07 11:05:16 
+1000)

are available in the Git repository at:

  https://gitlab.freedesktop.org/agd5f/linux.git 
tags/amd-drm-fixes-6.5-2023-07-12

for you to fetch changes up to e701156ccc6c7a5f104a968dda74cd6434178712:

  drm/amd: Align SMU11 SMU_MSG_OverridePcieParameters implementation with SMU13 
(2023-07-12 12:21:23 -0400)


amd-drm-fixes-6.5-2023-07-12:

amdgpu:
- SMU i2c locking fix
- Fix a possible deadlock in process restoration for ROCm apps
- Disable PCIe lane/speed switching on Intel platforms (the platforms don't 
support it)


Evan Quan (1):
  drm/amd/pm: share the code around SMU13 pcie parameters update

Mario Limonciello (3):
  drm/amd/pm: conditionally disable pcie lane/speed switching for SMU13
  drm/amd: Move helper for dynamic speed switch check out of smu13
  drm/amd: Align SMU11 SMU_MSG_OverridePcieParameters implementation with 
SMU13

Yang Wang (1):
  drm/amd/pm: fix smu i2c data read risk

gaba (1):
  drm/amdgpu: avoid restore process run into dead loop.

 drivers/gpu/drm/amd/amdgpu/amdgpu.h|  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   |  3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 19 +
 drivers/gpu/drm/amd/pm/swsmu/inc/smu_v13_0.h   |  4 +
 drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c  |  2 +-
 drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c|  2 +-
 .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c| 91 +-
 drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c |  2 +-
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 48 
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c   | 35 +
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c   |  2 +-
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_7_ppt.c   | 33 +---
 12 files changed, 101 insertions(+), 141 deletions(-)

Re: [PATCH] drm/doc: document that PRIME import/export is always supported

2023-07-12 Thread Jeffrey Hugo

On 7/12/2023 12:32 PM, Simon Ser wrote:

Since commit 6b85aa68d9d5 ("drm: Enable PRIME import/export for all
drivers"), import/export is always supported. Document this so that
user-space knows what to expect.

Signed-off-by: Simon Ser 
Cc: Thomas Zimmermann 
Cc: Alex Deucher 
Cc: Jeffrey Hugo 
Cc: Daniel Vetter 

Seems reasonable to me.
Reviewed-by: Jeffrey Hugo

[PATCH] drm/doc: document that PRIME import/export is always supported

2023-07-12 Thread Simon Ser

Since commit 6b85aa68d9d5 ("drm: Enable PRIME import/export for all
drivers"), import/export is always supported. Document this so that
user-space knows what to expect.

Signed-off-by: Simon Ser 
Cc: Thomas Zimmermann 
Cc: Alex Deucher 
Cc: Jeffrey Hugo 
Cc: Daniel Vetter 
---
 include/uapi/drm/drm.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index a87ca2d4..56c194df527e 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -673,6 +673,9 @@ struct drm_gem_open {
  * Bitfield of supported PRIME sharing capabilities. See _PRIME_CAP_IMPORT
  * and _PRIME_CAP_EXPORT.
  *
+ * Starting from kernel version 6.6, both _PRIME_CAP_IMPORT and
+ * _PRIME_CAP_EXPORT are always advertised.
+ *
  * PRIME buffers are exposed as dma-buf file descriptors. See
  * Documentation/gpu/drm-mm.rst, section "PRIME Buffer Sharing".
  */
@@ -682,6 +685,8 @@ struct drm_gem_open {
  *
  * If this bit is set in _CAP_PRIME, the driver supports importing PRIME
  * buffers via the _IOCTL_PRIME_FD_TO_HANDLE ioctl.
+ *
+ * Starting from kernel version 6.6, this bit is always set in _CAP_PRIME.
  */
 #define  DRM_PRIME_CAP_IMPORT  0x1
 /**
@@ -689,6 +694,8 @@ struct drm_gem_open {
  *
  * If this bit is set in _CAP_PRIME, the driver supports exporting PRIME
  * buffers via the _IOCTL_PRIME_HANDLE_TO_FD ioctl.
+ *
+ * Starting from kernel version 6.6, this bit is always set in _CAP_PRIME.
  */
 #define  DRM_PRIME_CAP_EXPORT  0x2
 /**
-- 
2.41.0

Re: [Intel-gfx] [PATCH v4] drm/i915/selftest/gsc: Ensure GSC Proxy init completes before selftests

2023-07-12 Thread Teres Alexis, Alan Previn

On Wed, 2023-07-12 at 10:19 +0100, Tvrtko Ursulin wrote:
> On 11/07/2023 23:02, Alan Previn wrote:
> > On MTL, if the GSC Proxy init flows haven't completed, submissions to the
> > GSC engine will fail. Those init flows are dependent on the mei's
> > gsc_proxy component that is loaded in parallel with i915 and a
> > worker that could potentially start after i915 driver init is done.
> > 
> > That said, all subsytems that access the GSC engine today does check
> > for such init flow completion before using the GSC engine. However,
> > selftests currently don't wait on anything before starting.
> > 
> > 
> > 
alan:snip

> > +   /*
> > +* The gsc proxy component depends on the kernel component driver load 
> > ordering
> > +* and in corner cases (the first time after an IFWI flash), 
> > init-completion
> > +* firmware flows take longer.
> > +*/
> > +   unsigned long timeout_ms = 8000;
> > +
> > +   if (need_to_wait &&
> > +   (wait_for(intel_gsc_uc_fw_proxy_init_done(>media_gt->uc.gsc, 
> > true),
> > +   timeout_ms)))
> > +   pr_info(DRIVER_NAME "Timed out waiting for 
> > gsc_proxy_completion!\n");
> 
> Would it make sense to error out here? Or at least upgrade to pr_warn or 
> something?
alan: agree on pr_warn (especially since need_for_wait being true and we tried 
for 8 secs - this should never happen).

> 
> I didn't quite understand the points Daniele raised about engine loops 
> and resets - in my mind GSC engine is this special thing exercised for 
> highly specialized operations and not touched in random for_each_engine 
> loop tests, but I also did not really look so might be totally wrong.

alan: after consulting with Daniele further, the concern in the case of
having gsc-proxy-init mid-execution while other selttests
are running (when thinking of tests that have nothing to do with GSC
but has indirect effect like memory-pressure, engine-resets,
etc) is that the proxy-init will bail-out print an error but
that would result in CI reporting a false-negative. We can't skip
that error since CONFIG_INTEL_MEI_GSC_PROXY tells us that the kernel
owner wants GSC-PROXY working.

> 
> In any case, v4 reads clear - no confusing comments and not 
> over-engineered so is acceptable to me.
> 
alan: thanks Tvrtko.


> P.S. Maybe the check *could* be moved to i915_live_selftests, where hw 
> dependencies conceptually fit better, and maybe i915_perf_selftests 
> would need it too then (?), but it is up to you.
alan: i can do this quickly as a rev5 (after a bit of manual check if perf 
needs it)

> 
> Maybe even in the array selftests/i915_live_selftests.h if we could add 
> a facility to make unskippable tests and add this one after the sanity 
> check. Which would then achieve the same generalized thing you had in 
> the previous version without needing to add a new array/loop.
alan: i rather not attempt this as part of the current patch but will
consider it as a separate patch if you are okay with that?

Re: [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet

2023-07-12 Thread Marek Vasut

On 7/9/23 03:03, Abhinav Kumar wrote:

On 7/7/2023 1:47 AM, Neil Armstrong wrote:

On 07/07/2023 09:18, Neil Armstrong wrote:

Hi,

On 06/07/2023 11:20, Amit Pundir wrote:

On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov
wrote:

[Adding freedreno@ to cc list]

On Wed, 5 Jul 2023 at 08:31, Jagan Teki
wrote:

Hi Amit,

On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir
wrote:

Hi Marek,

On Wed, 5 Jul 2023 at 01:48, Marek Vasut wrote:

Do not generate the HS front and back porch gaps, the HSA gap and
EOT packet, as these packets are not required. This makes the
bridge

work with Samsung DSIM on i.MX8MM and i.MX8MP.

This patch broke display on Dragonboard 845c (SDM845) devboard
running

AOSP. This is what I see
https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg.
Reverting this patch fixes this regression for me.

Might be msm dsi host require proper handling on these updated
mode_flags? did they?

The msm DSI host supports those flags. Also, I'd like to point out
that the patch didn't change the rest of the driver code. So even if
drm/msm ignored some of the flags, it should not have caused the
issue. Most likely the issue is on the lt9611 side. I's suspect that
additional programming is required to make it work with these flags.

I spent some time today on smoke testing these flags (individually and
in limited combination) on DB845c, to narrow down this breakage to one
or more flag(s) triggering it. Here are my observations in limited
testing done so far.

There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled
alone and system boots to UI as usual.

MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in the
screenshot[1] shared earlier as well.

Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and
MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired
with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken
display as reported.

In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags
added in this commit break the display on DB845c one way or another.

I think the investigation would be to understand why samsung-dsim
requires
such flags and/or what are the difference in behavior between MSM DSI
and samsung DSIM

for those flags ?

If someone has access to the lt9611 datasheet, so it requires
HSA/HFP/HBP to be

skipped ? and does MSM DSI and samsung DSIM skip them in the same way ?

I think there's a mismatch, where on one side this flags sets the link
in LP-11 while
in HSA/HFP/HPB while on the other it completely removes those blanking
packets.

The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not LP-11
while HPB.

the registers used in both controllers are different:
- samsung-dsim: DSIM_HBP_DISABLE_MODE
- msm dsi: DSI_VID_CFG0_HBP_POWER_STOP

The first one suggest removing the packet, while the second one
suggests powering

off the line while in the blanking packet period.

@Abhinav, can you comment on that ?

I dont get what it means by completely removes blanking packets in DSIM.

MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM.

Maybe there is a need for new set of flags which differentiate between
HBP skipped (i.e. NO HBP) and HBP LP11 ?

It should be replacing those periods with LP11 too.

The traffic mode being used on this bridge is
MIPI_DSI_MODE_VIDEO_SYNC_PULSE which is "Non-Burst Mode with Sync Pulses".

As per this traffic mode in the DSI spec,

"Normally, periods shown as HSA (Horizontal Sync Active), HBP
(Horizontal Back Porch) and HFP (Horizontal Front Porch) are filled by
Blanking Packets, with lengths (including packet overhead) calculated
to match the period specified by the peripheral’s data sheet.
Alternatively, if there is sufficient time to transition from HS to LP
mode and back again, a timed interval in LP mode may substitute for a
Blanking Packet, thus saving power. During HSA, HBP and HFP periods, the
bus should stay in the LP-11 state."

So we can either send the blanking packets or transition to LP state and
those 3 flags are controlling exactly that during those periods for MSM
driver.

If you stop sending the blanking packets, you need to replace that gap
with LP.

I don't think that's what MIPI_DSI_MODE_VIDEO_NO_HBP means, the way I
understand MIPI_DSI_MODE_VIDEO_NO_HBP is that it skips the HBP
completely. So if you want HBP, then do not set
MIPI_DSI_MODE_VIDEO_NO_HBP . And if you want LP11 during HBP, that is I
think up to the controller (or maybe another new flag?).

One reason I can think of why this could break with MSM is perhaps we do
not have sufficient time in those periods for the LP-HS transition like
the spec has written.

1) What is the resolution which is getting broken on db845c with this? I
would like to know the full drm_display_mode for it to see how much time
we have in those periods. Is any resolution working or all are broken.

2) I also do not completely

Re: [PATCH 5/5] drm/debugfs: rework drm_debugfs_create_files implementation

2023-07-12 Thread Tomer Tayar

On 11/07/2023 17:04, Christian König wrote:
> Use managed memory allocation for this. That allows us to not keep
> track of all the files any more.
>
> Signed-off-by: Christian König 
> ---
>   drivers/accel/drm_accel.c  |  2 -
>   drivers/gpu/drm/drm_debugfs.c  | 75 +-
>   drivers/gpu/drm/drm_drv.c  |  2 -
>   drivers/gpu/drm/drm_internal.h |  5 ---
>   drivers/gpu/drm/tegra/dc.c |  9 +++-
>   drivers/gpu/drm/tegra/dsi.c|  1 +
>   drivers/gpu/drm/tegra/hdmi.c   |  3 +-
>   drivers/gpu/drm/tegra/sor.c|  1 +
>   include/drm/drm_debugfs.h  |  4 +-
>   include/drm/drm_file.h |  4 --
>   10 files changed, 34 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> index 82c5fcbbc164..02ccf6520d27 100644
> --- a/drivers/accel/drm_accel.c
> +++ b/drivers/accel/drm_accel.c
> @@ -100,8 +100,6 @@ void accel_debugfs_register(struct drm_device *dev)
>   {
>   struct drm_minor *minor = dev->accel;
>   
> - INIT_LIST_HEAD(>debugfs_list);
> - mutex_init(>debugfs_lock);
>   minor->debugfs_root = dev->debugfs_root;
>   
>   drm_debugfs_create_files(accel_debugfs_list, ACCEL_DEBUGFS_ENTRIES,
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index d723143852e3..7aea06cb6be9 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -208,7 +208,7 @@ void drm_debugfs_create_files(const struct drm_info_list 
> *files, int count,

Need to remove the "These files will be removed automatically on 
drm_debugfs_cleanup()" sentence from the description of 
drm_debugfs_create_files(),  because drm_debugfs_cleanup() is removed in 
this patch.

>   if (features && !drm_core_check_all_features(dev, features))
>   continue;
>   
> - tmp = kmalloc(sizeof(struct drm_info_node), GFP_KERNEL);
> + tmp = drmm_kzalloc(dev, sizeof(*tmp), GFP_KERNEL);
>   if (tmp == NULL)
>   continue;
>   
> @@ -217,14 +217,28 @@ void drm_debugfs_create_files(const struct 
> drm_info_list *files, int count,
>   0444, root, tmp,
>   _debugfs_fops);
>   tmp->info_ent = [i];
> -
> - mutex_lock(>debugfs_lock);
> - list_add(>list, >debugfs_list);
> - mutex_unlock(>debugfs_lock);
>   }
>   }
>   EXPORT_SYMBOL(drm_debugfs_create_files);
>   
> +int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> +  struct dentry *root, struct drm_minor *minor)
> +{
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + struct dentry *dent = debugfs_lookup(files[i].name, root);
> +
> + if (!dent)
> + continue;
> +
> + drmm_kfree(minor->dev, d_inode(dent)->i_private);
> + debugfs_remove(dent);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_debugfs_remove_files);
> +
>   /**
>* drm_debugfs_dev_init - create debugfs directory for the device
>* @dev: the device which we want to create the directory for
> @@ -267,11 +281,8 @@ int drm_debugfs_register(struct drm_minor *minor, int 
> minor_id,
>   struct drm_device *dev = minor->dev;
>   char name[64];
>   
> - INIT_LIST_HEAD(>debugfs_list);
> - mutex_init(>debugfs_lock);
>   sprintf(name, "%d", minor_id);
> - minor->debugfs_symlink = debugfs_create_symlink(name, root,
> - dev->unique);
> + debugfs_create_symlink(name, root, dev->unique);

Is it okay to not track the symlink for a later removal? Will it be 
removed as part of the new drm_debugfs_remove_files()?

Thanks,
Tomer

>   
>   /* TODO: Only for compatibility with drivers */
>   minor->debugfs_root = dev->debugfs_root;
> @@ -282,52 +293,6 @@ int drm_debugfs_register(struct drm_minor *minor, int 
> minor_id,
>   return 0;
>   }
>   
> -int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> -  struct drm_minor *minor)
> -{
> - struct list_head *pos, *q;
> - struct drm_info_node *tmp;
> - int i;
> -
> - mutex_lock(>debugfs_lock);
> - for (i = 0; i < count; i++) {
> - list_for_each_safe(pos, q, >debugfs_list) {
> - tmp = list_entry(pos, struct drm_info_node, list);
> - if (tmp->info_ent == [i]) {
> - debugfs_remove(tmp->dent);
> - list_del(pos);
> - kfree(tmp);
> - }
> - }
> - }
> - mutex_unlock(>debugfs_lock);
> - return 0;
> -}
> -EXPORT_SYMBOL(drm_debugfs_remove_files);
> -
> -static void drm_debugfs_remove_all_files(struct drm_minor *minor)
> -{
> - struct drm_info_node *node, *tmp;
> -
> -

Re: [PATCH 3/5] drm/debugfs: rework debugfs directory creation v4

2023-07-12 Thread Tomer Tayar

On 11/07/2023 17:04, Christian König wrote:
> Instead of the per minor directories only create a single debugfs
> directory for the whole device directly when the device is initialized.
>
> For DRM devices each minor gets a symlink to the per device directory
> for now until we can be sure that this isn't useful any more in any way.
>
> Accel devices create only the per device directory and also drops the mid
> layer callback to create driver specific files.
>
> v2: cleanup accel component as well
> v3: fix typo when debugfs is disabled
> v4: call drm_debugfs_dev_fini() during release as well,
>  some kerneldoc typos fixed
>
> Signed-off-by: Christian König 
> ---
>   drivers/accel/drm_accel.c   | 27 +++
>   drivers/gpu/drm/drm_atomic.c|  4 +-
>   drivers/gpu/drm/drm_client.c|  4 +-
>   drivers/gpu/drm/drm_crtc_internal.h |  2 +-
>   drivers/gpu/drm/drm_debugfs.c   | 73 -
>   drivers/gpu/drm/drm_drv.c   | 21 +++--
>   drivers/gpu/drm/drm_framebuffer.c   |  4 +-
>   drivers/gpu/drm/drm_internal.h  | 20 ++--
>   include/drm/drm_accel.h |  9 +++-
>   include/drm/drm_client.h|  2 +-
>   include/drm/drm_device.h|  7 +++
>   include/drm/drm_drv.h   |  8 
>   include/drm/drm_file.h  |  1 +
>   13 files changed, 130 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> index 01edf2c00b5a..82c5fcbbc164 100644
> --- a/drivers/accel/drm_accel.c
> +++ b/drivers/accel/drm_accel.c
> @@ -79,26 +79,33 @@ static const struct drm_info_list accel_debugfs_list[] = {
>   #define ACCEL_DEBUGFS_ENTRIES ARRAY_SIZE(accel_debugfs_list)
>   
>   /**
> - * accel_debugfs_init() - Initialize debugfs for accel minor
> + * accel_debugfs_init() - Initialize debugfs for device
> + * @dev: Pointer to the device instance.
> + *
> + * This function creates a root directory for the device in debugfs.
> + */
> +void accel_debugfs_init(struct drm_device *dev)
> +{
> + drm_debugfs_dev_init(dev, accel_debugfs_root);
> +}
> +
> +/**
> + * accel_debugfs_register() - Register debugfs for device
>* @minor: Pointer to the drm_minor instance.
>* @minor_id: The minor's id

Sorry that I missed that in the previous review, but 'minor' and 
'minor_id' should be replaced with 'dev'.

Thanks,
Tomer

>*
> - * This function initializes the drm minor's debugfs members and creates
> - * a root directory for the minor in debugfs. It also creates common files
> - * for accelerators and calls the driver's debugfs init callback.
> + * Creates common files for accelerators.
>*/
> -void accel_debugfs_init(struct drm_minor *minor, int minor_id)
> +void accel_debugfs_register(struct drm_device *dev)
>   {
> - struct drm_device *dev = minor->dev;
> - char name[64];
> + struct drm_minor *minor = dev->accel;
>   
>   INIT_LIST_HEAD(>debugfs_list);
>   mutex_init(>debugfs_lock);
> - sprintf(name, "%d", minor_id);
> - minor->debugfs_root = debugfs_create_dir(name, accel_debugfs_root);
> + minor->debugfs_root = dev->debugfs_root;
>   
>   drm_debugfs_create_files(accel_debugfs_list, ACCEL_DEBUGFS_ENTRIES,
> -  minor->debugfs_root, minor);
> +  dev->debugfs_root, minor);
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2c454568a607..affce6a8851f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1832,9 +1832,9 @@ static const struct drm_debugfs_info 
> drm_atomic_debugfs_list[] = {
>   {"state", drm_state_info, 0},
>   };
>   
> -void drm_atomic_debugfs_init(struct drm_minor *minor)
> +void drm_atomic_debugfs_init(struct drm_device *dev)
>   {
> - drm_debugfs_add_files(minor->dev, drm_atomic_debugfs_list,
> + drm_debugfs_add_files(dev, drm_atomic_debugfs_list,
> ARRAY_SIZE(drm_atomic_debugfs_list));
>   }
>   #endif
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index f6292ba0e6fc..a91132276f21 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -514,9 +514,9 @@ static const struct drm_debugfs_info 
> drm_client_debugfs_list[] = {
>   { "internal_clients", drm_client_debugfs_internal_clients, 0 },
>   };
>   
> -void drm_client_debugfs_init(struct drm_minor *minor)
> +void drm_client_debugfs_init(struct drm_device *dev)
>   {
> - drm_debugfs_add_files(minor->dev, drm_client_debugfs_list,
> + drm_debugfs_add_files(dev, drm_client_debugfs_list,
> ARRAY_SIZE(drm_client_debugfs_list));
>   }
>   #endif
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
> b/drivers/gpu/drm/drm_crtc_internal.h
> index 501a10edd0e1..8556c3b3ff88 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -232,7 +232,7 @@

Re: [PATCH v4 3/3] drm/panel-fannal-c3004: Add fannal c3004 DSI panel

2023-07-12 Thread Marek Vasut


On 7/12/23 17:10, Paulo Pavacic wrote:

Hi,

[...]


Or whether it makes sense to outright have a separate driver. The later
would introduce duplication, but maybe that much duplication is OK.


I would like to create new driver because panel-st7701 seems to be
outdated and is using non-standard macro (ST7701_WRITE()


There is no such macro:

$ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l
0

There never was such a macro used in the driver either, are you sure you
are not using some hacked up patched downstream fork of the driver ?


I meant ST7701_DSI() macro; It can be replaced with
mipi_dsi_generic_write_seq from kernel 6.3. Sorry for the confusion.


OK


$ git log -p next/master --
drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l
0


) and for me
it is crashing kernel 5.15.


Have you based all the aforementioned discussion and argumentation on
year and half old Linux 5.15.y code base too ?

If so, you are missing many patches:

$ git log --oneline --no-merges v5.15..next/master --
drivers/gpu/drm/panel/panel-sitronix-st7701.c
5a2854e577dc2 drm: panel: Add orientation support for st7701
e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver
c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI
attach failure
49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection
c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation
57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron
DMT028VGHMCMI-1A TFT
42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences
83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and
timing
de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel
count from TFT mode
82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control
bitfield name
1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count
from TFT mode
779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT
specific
7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies
common to ST7701
a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode,
LPM, non-continuous clock
6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags
common to ST7701
79abca2b39900 drm/mipi-dsi: Make remove callback return void


I will try backporting those patches to 5.15 and applying them to see
whether it will then work with initialization sequences provided in
this merge request just to be sure not to have duplication. We are
still working on transitioning to newer kernel so for the time being
I'm using mostly 5.15.

On 5.15 kernel I have following kernel panic only with st7701 from the
panel drivers I have tried:

[   20.255322] Kernel panic - not syncing: Asynchronous SError Interrupt
[   20.255326] CPU: 1 PID: 36 Comm: kworker/1:1 Tainted: G   O
  5.15.77-5.15.77-2.1.0 #1


The latest 5.15.y is 5.15.120 , can you re-test on that version ?


[   20.255330] Hardware name: XXX i.MX8XX board:XXX (DT)


Is this some NXP downstream kernel fork with thousands of extra patches?
The version string 2.1.0 looks very much like NXP versioning scheme ...

[...]

Re: [Intel-gfx] [PATCH] drm/i915/huc: check HuC and GuC version compatibility on MTL

2023-07-12 Thread Ceraolo Spurio, Daniele





On 7/12/2023 3:03 AM, Andrzej Hajda wrote:

On 11.07.2023 22:31, Daniele Ceraolo Spurio wrote:

Due to a change in the auth flow on MTL, GuC 70.7.0 and newer will only
be able to authenticate HuC 8.5.1 and newer. The plan is to update the 2
binaries sinchronously in linux-firmware so that the fw repo always has
a matching pair that works; still, it's better to check in the kernel so
we can print an error message and abort HuC loading if the binaries are
out of sync instead of failing the authentication.

Signed-off-by: Daniele Ceraolo Spurio 
Cc: John Harrison 
---
  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 42 
  1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c 
b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c

index 08e16017584b..f0cc5bb47fa0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -803,11 +803,53 @@ static int try_firmware_load(struct intel_uc_fw 
*uc_fw, const struct firmware **

  return 0;
  }
  +static int check_mtl_huc_guc_compatibility(struct intel_gt *gt,
+   struct intel_uc_fw_file *huc_selected)
+{
+    struct intel_uc_fw_file *guc_selected = 
>uc.guc.fw.file_selected;

+    struct intel_uc_fw_ver *huc_ver = _selected->ver;
+    struct intel_uc_fw_ver *guc_ver = _selected->ver;
+    bool new_huc;
+    bool new_guc;
+
+    /* we can only do this check after having fetched both GuC and 
HuC */

+    GEM_BUG_ON(!huc_selected->path || !guc_selected->path);
+
+    /*
+ * Due to changes in the authentication flow for MTL, HuC 8.5.1 
or newer
+ * requires GuC 70.7.0 or newer. Older HuC binaries will instead 
require

+ * GuC < 70.7.0.
+ */
+    new_huc = huc_ver->major > 8 ||
+  (huc_ver->major == 8 && huc_ver->minor > 5) ||
+  (huc_ver->major == 8 && huc_ver->minor == 5 && 
huc_ver->patch >= 1);

+
+    new_guc = guc_ver->major > 70 ||
+  (guc_ver->major == 70 && guc_ver->minor >= 7);


Wouldn't be more readable to define sth like UC_VER_FULL(v)
then use UC_VER_FULL(huc_ver) >= IP_VER_FULL(8, 5, 1).
I am not sure if it is worth for two checks.


We've been trying to avoid those kind of macros because the version 
would need to be a u64 under the hood (each version number is a u16) and 
therefore type casting would be required to make all the shifting work, 
which makes the macro nasty to look at and as you said IMO not worth it 
for just 2 checks. Note that the GuC is the exception because it 
guarantees its version fits in a u32, so there is some macro use in the 
GuC-specific code.






+
+    if (new_huc != new_guc) {
+    UNEXPECTED(gt, "HuC %u.%u.%u is incompatible with GuC 
%u.%u.%u\n",

+   huc_ver->major, huc_ver->minor, huc_ver->patch,
+   guc_ver->major, guc_ver->minor, guc_ver->patch);
+    gt_info(gt, "MTL GuC 70.7.0+ and HuC 8.5.1+ don't work with 
older releases\n");

+    return -ENOEXEC;
+    }
+
+    return 0;
+}
+
  int intel_uc_check_file_version(struct intel_uc_fw *uc_fw, bool 
*old_ver)

  {
  struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
  struct intel_uc_fw_file *wanted = _fw->file_wanted;
  struct intel_uc_fw_file *selected = _fw->file_selected;
+    int ret;
+
+    if (IS_METEORLAKE(gt->i915) && uc_fw->type == 
INTEL_UC_FW_TYPE_HUC) {


Moving this check inside check function would make it more generic, up 
to you.


This will hopefully never apply to any other platform. This is a light 
breach of the HuC compatibility contract, so I really don't want to have 
a generic function to handle it. I want it to be clear from a higher 
level that this is an exception for a specific platform. Maybe worth 
adding a comment? Would something like the following make things clearer?


/*
 * MTL has some compatibility issues with early GuC/HuC binaries
 * not working with newer ones. This is specific to MTL and we
 * don't expect it to extend to other platforms.
 */

Daniele



Reviewed-by: Andrzej Hajda 

Regards
Andrzej



+    ret = check_mtl_huc_guc_compatibility(gt, selected);
+    if (ret)
+    return ret;
+    }
    if (!wanted->ver.major || !selected->ver.major)
  return 0;

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

2023-07-12 Thread Melissa Wen

On 07/12, Aurabindo Pillai wrote:
> 
> 
> On 7/12/2023 12:24 PM, Melissa Wen wrote:
> > On 07/12, Pillai, Aurabindo wrote:
> > > [Public]
> > > 
> > > Hi Guilherme,
> > > 
> > > Sorry there was one more patch which I missed to attach. Please add this 
> > > 3rd patch and retry.
> > > 
> > > Reverting that patch would cause high power consumption on Navi2x GPU 
> > > also cause hangs on certain multi monitor configurations. With these 3 
> > > patches, you're getting the same effect as reverting the aforementioned 
> > > patches, but it makes the reverted sequence available only for Steam deck 
> > > hardware.
> > > 
> > 
> > Hi Jay,
> > 
> > Thanks for looking at this issue.
> > 
> > You mention power consumption and multi-monitor configuration issues
> > that can affect a driver if we revert this OTG change, and both sounds
> > quite relevant to me. Can they not affect DCN301 too? Is there something
> > that needs further work so the DCN301 can benefit from this improvement
> > as well?
> > 
> > Also, let us know if we can contribute in any way.
> > 
> 
> Hi Melissa,
> 
> Unfortunately, DCN301 does not support Firmware Assisted Memory Clock
> Switching, which is the feature that gets blocked on Navi2x if we revert the
> patch in question.  This is the feature that enables lower power consumption
> on some multi monitor configurations and high refresh rate single monitor
> configurations.
> 
> Navi2x is configured to use FAMS in the driver, but without this change,
> firmware wont be able to actually enable the feature in DMCUB, which puts
> the driver in a unexpected state. On DCN301, this unexpected state will not
> occur, because there is no FAMS support in driver nor firmware.

Oh, got it. Sounds fine.
Many thanks for explaning the context of this change.

Best Regards,

Melissa

> 
> --
> Jay
> 
> 
> 


signature.asc
Description: PGP signature

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

2023-07-12 Thread Aurabindo Pillai





On 7/12/2023 12:24 PM, Melissa Wen wrote:

On 07/12, Pillai, Aurabindo wrote:

[Public]

Hi Guilherme,

Sorry there was one more patch which I missed to attach. Please add this 3rd 
patch and retry.

Reverting that patch would cause high power consumption on Navi2x GPU also 
cause hangs on certain multi monitor configurations. With these 3 patches, 
you're getting the same effect as reverting the aforementioned patches, but it 
makes the reverted sequence available only for Steam deck hardware.



Hi Jay,

Thanks for looking at this issue.

You mention power consumption and multi-monitor configuration issues
that can affect a driver if we revert this OTG change, and both sounds
quite relevant to me. Can they not affect DCN301 too? Is there something
that needs further work so the DCN301 can benefit from this improvement
as well?

Also, let us know if we can contribute in any way.



Hi Melissa,

Unfortunately, DCN301 does not support Firmware Assisted Memory Clock 
Switching, which is the feature that gets blocked on Navi2x if we revert 
the patch in question.  This is the feature that enables lower power 
consumption on some multi monitor configurations and high refresh rate 
single monitor configurations.


Navi2x is configured to use FAMS in the driver, but without this change, 
firmware wont be able to actually enable the feature in DMCUB, which 
puts the driver in a unexpected state. On DCN301, this unexpected state 
will not occur, because there is no FAMS support in driver nor firmware.


--
Jay

Re: [PATCH v2 00/13] Add Inanbo T28CP45TN89 panel support

2023-07-12 Thread Miquel Raynal

Hi Sebastian,

+ Thomas

s...@kernel.org wrote on Sat, 22 Apr 2023 22:49:59 +0200:

> Hi,
> 
> This adds panel support for Inanbo T28CP45TN89, which I found inside of a
> handheld thermal camera. The panel is based on the st7789v controller. All
> information is based on reverse engineering.

I haven't seen another version for this series so I assume it is still
the one to look at. As you already know, I also want to add support for
a panel based on the st7789 display controller. As discussed, I rebased
my changes on top of yours as you actually sent them upstream
much earlier than I did.

As a single minor comment was made to this version of the series, I
would like to know if you wanted to send a new version soon? Or if it
would make sense to send a bigger series with all our common patches in
one single shot (mine should apply cleanly without further work on
yours). Let me know how we can move forward.

For the record, here are my patches.

Link: 
https://lore.kernel.org/all/20230619155958.3119181-1-miquel.ray...@bootlin.com/

Thanks a lot,
Miquèl

> 
> Changes since PATCHv1:
>  * https://lore.kernel.org/all/20230317232355.1554980-1-...@kernel.org/
>  * Apply DT binding changes requested by Krzysztof Kozlowski and his Ack
>  * I changed the driver patches to avoid code duplication and splitted
>the code a bit more
> 
> -- Sebastian
> 
> Sebastian Reichel (13):
>   dt-bindings: vendor-prefixes: add Inanbo
>   dt-bindings: display: st7789v: add Inanbo T28CP45TN89
>   drm/panel: sitronix-st7789v: add SPI ID table
>   drm/panel: sitronix-st7789v: remove unused constants
>   drm/panel: sitronix-st7789v: make reset GPIO optional
>   drm/panel: sitronix-st7789v: simplify st7789v_spi_write
>   drm/panel: sitronix-st7789v: improve error handling
>   drm/panel: sitronix-st7789v: avoid hardcoding mode info
>   drm/panel: sitronix-st7789v: avoid hardcoding panel size
>   drm/panel: sitronix-st7789v: add media bus format
>   drm/panel: sitronix-st7789v: avoid hardcoding invert mode
>   drm/panel: sitronix-st7789v: avoid hardcoding polarity info
>   drm/panel: sitronix-st7789v: add Inanbo T28CP45TN89 support
> 
>  .../display/panel/sitronix,st7789v.yaml   |   5 +-
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  .../gpu/drm/panel/panel-sitronix-st7789v.c| 148 ++
>  3 files changed, 120 insertions(+), 35 deletions(-)
>

[PATCH][next] drm/nouveau/bios/therm: make read-only array duty_lut static const

2023-07-12 Thread Colin Ian King

Don't populate the read-only array on the stack, instead make it static
const.

Signed-off-by: Colin Ian King 
---
 drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c
index 5babc5a7c7d5..31fb20cc5a9b 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c
@@ -156,8 +156,10 @@ nvbios_therm_fan_parse(struct nvkm_bios *bios, struct 
nvbios_therm_fan *fan)
u8 ver, len, i;
u32 entry;
 
-   uint8_t duty_lut[] = { 0, 0, 25, 0, 40, 0, 50, 0,
-   75, 0, 85, 0, 100, 0, 100, 0 };
+   static const uint8_t duty_lut[] = {
+   0, 0, 25, 0, 40, 0, 50, 0,
+   75, 0, 85, 0, 100, 0, 100, 0
+   };
 
i = 0;
fan->nr_fan_trip = 0;
-- 
2.39.2

Re: [PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

2023-07-12 Thread Felix Kuehling

Allocations in the heap and stack tend to be small, with several 
allocations sharing the same page. Sharing the same page for different 
allocations with different access patterns leads to thrashing when we 
migrate data back and forth on GPU and CPU access. To avoid this we 
disable HMM migrations for head and stack VMAs.


Regards,
  Felix


Am 2023-07-12 um 10:42 schrieb Christoph Hellwig:

On Wed, Jul 12, 2023 at 10:38:29PM +0800, Kefeng Wang wrote:

Use the helpers to simplify code.

Nothing against your addition of a helper, but a GPU driver really
should have no business even looking at this information..

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

2023-07-12 Thread Melissa Wen

On 07/12, Pillai, Aurabindo wrote:
> [Public]
> 
> Hi Guilherme,
> 
> Sorry there was one more patch which I missed to attach. Please add this 3rd 
> patch and retry.
> 
> Reverting that patch would cause high power consumption on Navi2x GPU also 
> cause hangs on certain multi monitor configurations. With these 3 patches, 
> you're getting the same effect as reverting the aforementioned patches, but 
> it makes the reverted sequence available only for Steam deck hardware.
> 

Hi Jay,

Thanks for looking at this issue.

You mention power consumption and multi-monitor configuration issues
that can affect a driver if we revert this OTG change, and both sounds
quite relevant to me. Can they not affect DCN301 too? Is there something
that needs further work so the DCN301 can benefit from this improvement
as well?

Also, let us know if we can contribute in any way.

Best Regards,

Melissa


> --
> 
> Regards,
> Jay
> 
> From: Guilherme G. Piccoli 
> Sent: Tuesday, July 11, 2023 7:15 PM
> To: Pillai, Aurabindo ; Deucher, Alexander 
> 
> Cc: amd-...@lists.freedesktop.org ; Koenig, 
> Christian ; Pan, Xinhui ; 
> dri-devel@lists.freedesktop.org ; 
> kernel-...@igalia.com ; 
> cristian.ciocal...@collabora.com ; André 
> Almeida ; Melissa Wen ; Siqueira, 
> Rodrigo 
> Subject: Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max 
> selectors unconditionally for DCN1+"
> 
> On 11/07/2023 15:22, Aurabindo Pillai wrote:
> > [...]
> > Hi,
> >
> > Sorry for the delayed response, this patch went unnoticed. This revert 
> > would break asics. Could you try the attached patch without reverting this 
> > one ?
> 
> Hi Aurabindo, thanks for your response!
> 
> I've tried kernel 6.5-rc1, and it seems the issue is present, due to the
> patch being merged on Linus tree [as 1598fc576420 ("drm/amd/display:
> Program OTG vtotal min/max selectors unconditionally for DCN1+")].
> 
> Then, I tried both your attached patches on top of that, and
> unfortunately, the behavior is the same: Steam Deck doesn't boot with
> graphics, and we can see the single error "amdgpu :04:00.0: [drm]
> *ERROR* [CRTC:67:crtc-0] flip_done timed out" on dmesg.
> 
> Do you / Alex think we could get this revert for 6.5-rc2, so at least we
> could boot mainline there while the issue is handled? It would be an
> intermediate fix. You mentioned it breaks some asics, but did they work
> until now, without your patch?
> 
> Thanks,
> 
> 
> Guilherme




signature.asc
Description: PGP signature

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Sui Jingfeng


Hi,


Thanks for you patch, I'm here join to the discussion.

I will express my opinion, educate me if I'm wrong.


My understanding is that drm_device is not really a device like 
pci_device and platform_device.


the name 'dev' is generally refer to 'struct device *'.


So I think, the name 'drm' or 'ddev' is more better than 'drm_dev', and 
easy to type.


'drm_dev' is looks ugly.


Try to find a best name you love and replace all.

No union please, that is just a grammar sugar.  Personally I don't want it.


On 2023/7/12 21:38, Uwe Kleine-König wrote:

Hello Maxime,

On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:

On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:

Background is that this makes merge conflicts easier to handle and detect.

Really?

FWIW, I agree with Christian here.


Each file (apart from include/drm/drm_crtc.h) is only touched once. So
unless I'm missing something you don't get less or easier conflicts by
doing it all in a single patch. But you gain the freedom to drop a
patch for one driver without having to drop the rest with it.

Not really, because the last patch removed the union anyway. So you have
to revert both the last patch, plus that driver one. And then you need
to add a TODO to remove that union eventually.

Yes, with a single patch you have only one revert (but 194 files changed,
1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
bigger). (And maybe you get away with just reverting the last patch.)

With a single patch the TODO after a revert is "redo it all again (and
prepare for a different set of conflicts)" while with the split series
it's only "fix that one driver that was forgotten/borked" + reapply that
10 line patch. As the one who gets that TODO, I prefer the latter.

So in sum: If your metric is "small count of reverted commits", you're
right. If however your metric is: Better get 95% of this series' change
in than maybe 0%, the split series is the way to do it.

With me having spend ~3h on this series' changes, it's maybe
understandable that I did it the way I did.

FTR: This series was created on top of v6.5-rc1.



I think,  this series should be able applied to drm-tip or drm-misc.

And should pass the compile test.



  If you apply it to
drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
be the responsible maintainer who applies this series, I like being able
to just do git am --skip then.

FTR#2: In drm-misc-next is a new driver
(drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
now might indeed be a good idea.



No, that driver should be nuked together if this series is going to be 
applied(after get acked-by and/or reviewed-by).


Please don't leave that driver alone there.



So I still like the split version better, but I'm open to a more
verbose reasoning from your side.

You're doing only one thing here, really: you change the name of a
structure field. If it was shared between multiple maintainers, then
sure, splitting that up is easier for everyone, but this will go through
drm-misc, so I can't see the benefit it brings.

I see your argument, but I think mine weights more.

Best regards
Uwe

[PATCH][next][V2] video: fbdev: kyro: make some const read-only arrays static and reduce type size

2023-07-12 Thread Colin Ian King

Don't populate the const read-only arrays on the stack but instead
make them static const. Use smaller types to use less storage for
the arrays.  Also makes the object code a little smaller.

Signed-off-by: Colin Ian King 
---

V2: Use smaller int types, kudos to Helge Deller for suggesting this

---
 drivers/video/fbdev/kyro/STG4000InitDevice.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/kyro/STG4000InitDevice.c 
b/drivers/video/fbdev/kyro/STG4000InitDevice.c
index edfa0a04854d..79886a246638 100644
--- a/drivers/video/fbdev/kyro/STG4000InitDevice.c
+++ b/drivers/video/fbdev/kyro/STG4000InitDevice.c
@@ -83,11 +83,11 @@ volatile u32 i,count=0; \
 static u32 InitSDRAMRegisters(volatile STG4000REG __iomem *pSTGReg,
  u32 dwSubSysID, u32 dwRevID)
 {
-   u32 adwSDRAMArgCfg0[] = { 0xa0, 0x80, 0xa0, 0xa0, 0xa0 };
-   u32 adwSDRAMCfg1[] = { 0x8732, 0x8732, 0xa732, 0xa732, 0x8732 };
-   u32 adwSDRAMCfg2[] = { 0x87d2, 0x87d2, 0xa7d2, 0x87d2, 0xa7d2 };
-   u32 adwSDRAMRsh[] = { 36, 39, 40 };
-   u32 adwChipSpeed[] = { 110, 120, 125 };
+   static const u8 adwSDRAMArgCfg0[] = { 0xa0, 0x80, 0xa0, 0xa0, 0xa0 };
+   static const u16 adwSDRAMCfg1[] = { 0x8732, 0x8732, 0xa732, 0xa732, 
0x8732 };
+   static const u16 adwSDRAMCfg2[] = { 0x87d2, 0x87d2, 0xa7d2, 0x87d2, 
0xa7d2 };
+   static const u8 adwSDRAMRsh[] = { 36, 39, 40 };
+   static const u8 adwChipSpeed[] = { 110, 120, 125 };
u32 dwMemTypeIdx;
u32 dwChipSpeedIdx;
 
-- 
2.39.2

Re: [PATCH v3 0/2] Small of/device cleanup

2023-07-12 Thread Miquel Raynal

Hello,

miquel.ray...@bootlin.com wrote on Thu, 22 Jun 2023 23:32:12 +0200:

> 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/

I expect this small series to go through the drm tree, but as I actually
sent it right before the beginning of the merge window and because I am
not an experienced drm contributor, I would like to know if I am
required to resend the patches or if they are fine as-is (I don't
expect any conflicts with v6.5-rc1).

Just let me know if a re-send is expected.

Cheers,
Miquèl

> 
> Thanks,
> Miquèl
> 
> Changes in v3:
> * Fixed the dev->parent referencing in the host1x driver.
> * Collected Rob's Acked-by.
> 
> 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(-)
>

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

2023-07-12 Thread Miquel Raynal

Hello,

s...@ravnborg.org wrote on Sat, 10 Jun 2023 22:05:15 +0200:

> On Fri, Jun 09, 2023 at 04:48:43PM +0200, Miquel Raynal wrote:
> > 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   
> 
> Hi Miquel,
> 
> the patch is:
> Reviewed-by: Sam Ravnborg 
> 
> I hope someone else can pick it up and apply it to drm-misc as
> my drm-misc setup is hopelessly outdated atm.

Looks like nobody picked this up yet, can someone take it? Let me know
if you want me to send it again.

Thanks,
Miquèl

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

2023-07-12 Thread Aurabindo Pillai





On 7/12/2023 11:50 AM, Guilherme G. Piccoli wrote:

On 12/07/2023 11:47, Pillai, Aurabindo wrote:

Hi Guilherme,

Sorry there was one more patch which I missed to attach. Please add this
3^rd  patch and retry.

Reverting that patch would cause high power consumption on Navi2x GPU
also cause hangs on certain multi monitor configurations. With these 3
patches, you're getting the same effect as reverting the aforementioned
patches, but it makes the reverted sequence available only for Steam
deck hardware.



Thanks a lot for your detailed explanation, and the 3rd patch! Indeed,
amdgpu works fine on Deck with that - great =)

Feel free to add my:
Tested-by: Guilherme G. Piccoli  #Steam Deck

Oh, a fixes tag would also makes sense, I guess.
BTW, if possible to submit the 3 patches in a proper series to get it
merged on 6.5-rc cycle (the sooner the better), I'd really appreciate!



Thanks for confirmation! I'll add the fixes tag so that it gets picked up.

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

2023-07-12 Thread Guilherme G. Piccoli

On 12/07/2023 11:47, Pillai, Aurabindo wrote:
> Hi Guilherme,
> 
> Sorry there was one more patch which I missed to attach. Please add this
> 3^rd  patch and retry.
> 
> Reverting that patch would cause high power consumption on Navi2x GPU
> also cause hangs on certain multi monitor configurations. With these 3
> patches, you're getting the same effect as reverting the aforementioned
> patches, but it makes the reverted sequence available only for Steam
> deck hardware.
> 

Thanks a lot for your detailed explanation, and the 3rd patch! Indeed,
amdgpu works fine on Deck with that - great =)

Feel free to add my:
Tested-by: Guilherme G. Piccoli  #Steam Deck

Oh, a fixes tag would also makes sense, I guess.
BTW, if possible to submit the 3 patches in a proper series to get it
merged on 6.5-rc cycle (the sooner the better), I'd really appreciate!

Cheers,

Guilherme

Re: [PATCH][next] video: fbdev: kyro: make some const read-only arrays static

2023-07-12 Thread Helge Deller


On 7/12/23 16:57, Colin Ian King wrote:

Don't populate the const read-only arrays on the stack but instead
make them static const. Also makes the object code a little smaller.


Looks good, but you can optimze even further...


Signed-off-by: Colin Ian King 
---
  drivers/video/fbdev/kyro/STG4000InitDevice.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/kyro/STG4000InitDevice.c 
b/drivers/video/fbdev/kyro/STG4000InitDevice.c
index edfa0a04854d..bf1ee3addbd0 100644
--- a/drivers/video/fbdev/kyro/STG4000InitDevice.c
+++ b/drivers/video/fbdev/kyro/STG4000InitDevice.c
@@ -83,11 +83,11 @@ volatile u32 i,count=0; \
  static u32 InitSDRAMRegisters(volatile STG4000REG __iomem *pSTGReg,
  u32 dwSubSysID, u32 dwRevID)
  {
-   u32 adwSDRAMArgCfg0[] = { 0xa0, 0x80, 0xa0, 0xa0, 0xa0 };
-   u32 adwSDRAMCfg1[] = { 0x8732, 0x8732, 0xa732, 0xa732, 0x8732 };
-   u32 adwSDRAMCfg2[] = { 0x87d2, 0x87d2, 0xa7d2, 0x87d2, 0xa7d2 };
-   u32 adwSDRAMRsh[] = { 36, 39, 40 };
-   u32 adwChipSpeed[] = { 110, 120, 125 };> +   static const u32 
adwSDRAMArgCfg0[] = { 0xa0, 0x80, 0xa0, 0xa0, 0xa0 };


Make this u8 instead of u32 (saves 5*3 = 15 bytes):
+   static const u8 adwSDRAMArgCfg0[] = { 0xa0, 0x80, 0xa0, 0xa0, 0xa0 };


+   static const u32 adwSDRAMCfg1[] = { 0x8732, 0x8732, 0xa732, 0xa732, 
0x8732 };


u16


+   static const u32 adwSDRAMCfg2[] = { 0x87d2, 0x87d2, 0xa7d2, 0x87d2, 
0xa7d2 };


u16


+   static const u32 adwSDRAMRsh[] = { 36, 39, 40 };


u8


+   static const u32 adwChipSpeed[] = { 110, 120, 125 };


u8...

Can you change that (test compile) and resend?

Helge

Re: [PATCH 5/7] drm/panfrost: switch to using drm_exec

2023-07-12 Thread Steven Price

On 12/07/2023 13:47, Christian König wrote:
> Just a straightforward conversion without any optimization.
> 
> Only compile tested for now.
> 
> Signed-off-by: Christian König 
> Cc: Rob Herring 
> Cc: Tomeu Vizoso 
> Cc: Steven Price 
> Cc: Alyssa Rosenzweig 
> ---
>  drivers/gpu/drm/panfrost/Kconfig|  1 +
>  drivers/gpu/drm/panfrost/panfrost_job.c | 12 +++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/Kconfig 
> b/drivers/gpu/drm/panfrost/Kconfig
> index e6403a9d66ad..e86a1a2fd8e1 100644
> --- a/drivers/gpu/drm/panfrost/Kconfig
> +++ b/drivers/gpu/drm/panfrost/Kconfig
> @@ -7,6 +7,7 @@ config DRM_PANFROST
>   depends on !GENERIC_ATOMIC64# for IOMMU_IO_PGTABLE_LPAE
>   depends on MMU
>   select DRM_SCHED
> + select DRM_EXEC
>   select IOMMU_SUPPORT
>   select IOMMU_IO_PGTABLE_LPAE
>   select DRM_GEM_SHMEM_HELPER
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c 
> b/drivers/gpu/drm/panfrost/panfrost_job.c
> index dbc597ab46fb..8b9206e910b5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -8,6 +8,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -275,13 +276,14 @@ static void panfrost_attach_object_fences(struct 
> drm_gem_object **bos,
>  int panfrost_job_push(struct panfrost_job *job)
>  {
>   struct panfrost_device *pfdev = job->pfdev;
> - struct ww_acquire_ctx acquire_ctx;
> + struct drm_exec exec;
>   int ret = 0;
>  
> - ret = drm_gem_lock_reservations(job->bos, job->bo_count,
> - _ctx);
> + drm_exec_init(, DRM_EXEC_INTERRUPTIBLE_WAIT);
> + drm_exec_until_all_locked()
> + ret = drm_exec_prepare_array(, job->bos, job->bo_count, 1);

This loop bothers me - the return value is ignored until
drm_exec_until_all_locked() decides we can exit. It also reads badly
without the braces and with the "if" unindented below. The documentation
("a typical usage pattern") suggests this should be something like:

drm_exec_until_all_locked() {
ret = drm_exec_prepare_array(...);
drm_exec_retry_on_contention();
if (ret)
goto unlock;
}

Although I'm not sure if that's actually different because if there's no
contention that drm_exec_until_all_locked() looks like it should exit.

Perhaps it's just the name drm_exec_until_all_locked() which perhaps
should be drm_exec_until_not_contended()...?

Anyway I gave it a quick spin on a Firefly-RK3288 and didn't see any
problems, but I don't think I've got any tests which would create
contention on the BOs.

Steve

>   if (ret)
> - return ret;
> + goto unlock;
>  
>   mutex_lock(>sched_lock);
>   drm_sched_job_arm(>base);
> @@ -305,7 +307,7 @@ int panfrost_job_push(struct panfrost_job *job)
> job->render_done_fence);
>  
>  unlock:
> - drm_gem_unlock_reservations(job->bos, job->bo_count, _ctx);
> + drm_exec_fini();
>  
>   return ret;
>  }

Re: [PATCH] drm/ast: Fix default resolution when no monitor is connected on DP

2023-07-12 Thread Jocelyn Falempe

On 12/07/2023 17:05, Sui Jingfeng wrote:

Hi,

I'm here join to the discussion. Because I know a little about aspeed BMC.

On 2023/7/10 16:07, Jocelyn Falempe wrote:

On 07/07/2023 09:30, Thomas Zimmermann wrote:

Am 06.07.23 um 18:37 schrieb Jocelyn Falempe:
[...]

You could out-comment the VGA code in the ast driver for testing.

Oh, Thanks for the idea, I will try that.

The result is that I get a black screen on the remote BMC. So maybe
adding a remote/bmc connector will solve that.

Could work. That would be a connector and encoder of type 'virtual'?

Not all ast devices have a BMC. Do you know how to detect its presence?

Hum, I though all ast devices have a BMC,

No, Thomas is right, not all ast devices have a BMC.

I have two discrete AST BMC cards, see [1] for reference.

I generally using the ast2400 BMC cards to testing patches and study
drm/ast driver.

It seems that this two cards are pure 2D display card, because they
don't have a net interface(can not plug-in the net cable).

[1]
https://github.com/loongson-gfx/loongson_boards/blob/main/ast_bmc_cards/ast1400_and_ast2400.jpg

Thanks for this picture, I didn't know about this discrete graphic
cards, with PCIe connector.

and I don't see a way to detect that BMC is active or present.

I think we better find one, then if the BMC is active (present).

we could create a virtual encoder and connector safely.

(It would be even better to know the browser's size, and automatically
resize, like when using a VM. But I'm not sure the hardware/firmware
is able to do this).

I think it is not difficult, it just that need the firmware of your
board to set a value to a register,

(a scratch register) or something like that.

But this really need you have the firmware (source code) to support this.

Yes, that's the difficult part.

Or you are luckily, if there somebody already done this for you.

On the other hand, are there any drawback to present a BMC connector
even when the hardware doesn't have it ?

If not properly setting up, I think you will create two encoder and two
connector in the system.

Yes, but I think it won't have any visible effect for the end-user.

Jocelyn

Best regards
Thomas

Best regards,

[PATCH v5 1/2] drm/ast: Add BMC virtual connector

2023-07-12 Thread Jocelyn Falempe

Most aspeed devices have a BMC, which allows to remotely see the screen.
Also in the common use case, those servers don't have a display connected.
So add a Virtual connector, to reflect that even if no display is
connected, the framebuffer can still be seen remotely.
This prepares the work to implement a detect_ctx() for the Display port
connector.

v4: call drm_add_modes_noedid() with 4096x4096 (Thomas Zimmermann)
remove useless struct field init to 0 (Thomas Zimmermann)
don't use drm_simple_encoder_init() (Thomas Zimmermann)
inline ast_bmc_connector_init() (Thomas Zimmermann)

Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
Signed-off-by: Jocelyn Falempe 
---
 drivers/gpu/drm/ast/ast_drv.h  |  4 +++
 drivers/gpu/drm/ast/ast_mode.c | 58 ++
 2 files changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 3f6e0c984523..c9659e72002f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -214,6 +214,10 @@ struct ast_device {
struct drm_encoder encoder;
struct drm_connector connector;
} astdp;
+   struct {
+   struct drm_encoder encoder;
+   struct drm_connector connector;
+   } bmc;
} output;
 
bool support_wide_screen;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index f711d592da52..1a8293162fec 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1735,6 +1735,61 @@ static int ast_astdp_output_init(struct ast_device *ast)
return 0;
 }
 
+/*
+ * BMC virtual Connector
+ */
+
+static int ast_bmc_connector_helper_get_modes(struct drm_connector *connector)
+{
+   return drm_add_modes_noedid(connector, 4096, 4096);
+}
+
+static const struct drm_connector_helper_funcs ast_bmc_connector_helper_funcs 
= {
+   .get_modes = ast_bmc_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs ast_bmc_connector_funcs = {
+   .reset = drm_atomic_helper_connector_reset,
+   .fill_modes = drm_helper_probe_single_connector_modes,
+   .destroy = drm_connector_cleanup,
+   .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+   .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static const struct drm_encoder_funcs ast_bmc_encoder_funcs = {
+   .destroy = drm_encoder_cleanup,
+};
+
+static int ast_bmc_output_init(struct ast_device *ast)
+{
+   struct drm_device *dev = >base;
+   struct drm_crtc *crtc = >crtc;
+   struct drm_encoder *encoder = >output.bmc.encoder;
+   struct drm_connector *connector = >output.bmc.connector;
+   int ret;
+
+
+   ret = drm_encoder_init(dev, encoder,
+   _bmc_encoder_funcs,
+   DRM_MODE_ENCODER_VIRTUAL, "ast_bmc");
+   if (ret)
+   return ret;
+   encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+   ret = drm_connector_init(dev, connector, _bmc_connector_funcs,
+DRM_MODE_CONNECTOR_VIRTUAL);
+   if (ret)
+   return ret;
+
+   drm_connector_helper_add(connector, _bmc_connector_helper_funcs);
+
+   ret = drm_connector_attach_encoder(connector, encoder);
+   if (ret)
+   return ret;
+
+   return 0;
+}
+
 /*
  * Mode config
  */
@@ -1842,6 +1897,9 @@ int ast_mode_config_init(struct ast_device *ast)
if (ret)
return ret;
}
+   ret = ast_bmc_output_init(ast);
+   if (ret)
+   return ret;
 
drm_mode_config_reset(dev);
 

base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2
-- 
2.41.0

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello Maxime,

On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
> On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> > > Background is that this makes merge conflicts easier to handle and detect.
> > 
> > Really?
> 
> FWIW, I agree with Christian here.
> 
> > Each file (apart from include/drm/drm_crtc.h) is only touched once. So
> > unless I'm missing something you don't get less or easier conflicts by
> > doing it all in a single patch. But you gain the freedom to drop a
> > patch for one driver without having to drop the rest with it.
> 
> Not really, because the last patch removed the union anyway. So you have
> to revert both the last patch, plus that driver one. And then you need
> to add a TODO to remove that union eventually.

Yes, with a single patch you have only one revert (but 194 files changed,
1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
bigger). (And maybe you get away with just reverting the last patch.)

With a single patch the TODO after a revert is "redo it all again (and
prepare for a different set of conflicts)" while with the split series
it's only "fix that one driver that was forgotten/borked" + reapply that
10 line patch. As the one who gets that TODO, I prefer the latter.

So in sum: If your metric is "small count of reverted commits", you're
right. If however your metric is: Better get 95% of this series' change
in than maybe 0%, the split series is the way to do it.

With me having spend ~3h on this series' changes, it's maybe
understandable that I did it the way I did.

FTR: This series was created on top of v6.5-rc1. If you apply it to
drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
be the responsible maintainer who applies this series, I like being able
to just do git am --skip then. 

FTR#2: In drm-misc-next is a new driver
(drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
now might indeed be a good idea.

> > So I still like the split version better, but I'm open to a more
> > verbose reasoning from your side.
> 
> You're doing only one thing here, really: you change the name of a
> structure field. If it was shared between multiple maintainers, then
> sure, splitting that up is easier for everyone, but this will go through
> drm-misc, so I can't see the benefit it brings.

I see your argument, but I think mine weights more.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |

signature.asc
Description: PGP signature

[PATCH 3/5] drm/amdkfd: use vma_is_stack() and vma_is_heap()

Use the helpers to simplify code.

Signed-off-by: Kefeng Wang 
---
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 479c4f66afa7..19ce68a7e1a8 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -2623,10 +2623,7 @@ svm_range_get_range_boundaries(struct kfd_process *p, 
int64_t addr,
return -EFAULT;
}
 
-   *is_heap_stack = (vma->vm_start <= vma->vm_mm->brk &&
- vma->vm_end >= vma->vm_mm->start_brk) ||
-(vma->vm_start <= vma->vm_mm->start_stack &&
- vma->vm_end >= vma->vm_mm->start_stack);
+   *is_heap_stack = vma_is_heap(vma) || vma_is_stack(vma);
 
start_limit = max(vma->vm_start >> PAGE_SHIFT,
  (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
-- 
2.41.0

[PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!

Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
  1 struct drm_device *adev_to_drm
  1 struct drm_device *drm_
  1 struct drm_device  *drm_dev
  1 struct drm_device*drm_dev
  1 struct drm_device *pdev
  1 struct drm_device *rdev
  1 struct drm_device *vdev
  2 struct drm_device *dcss_drv_dev_to_drm
  2 struct drm_device **ddev
  2 struct drm_device *drm_dev_alloc
  2 struct drm_device *mock
  2 struct drm_device *p_ddev
  5 struct drm_device *device
  9 struct drm_device * dev
 25 struct drm_device *d
 95 struct drm_device *
216 struct drm_device *ddev
234 struct drm_device *drm_dev
611 struct drm_device *drm
   4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?

The series was compile tested for arm, arm64, powerpc and amd64 using an
allmodconfig (though I only build drivers/gpu/).

Best regards
Uwe

Uwe Kleine-König (52):
  drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
  drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/armada: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/exynos: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/gma500: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/meson: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/pl111: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/radeon: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/renesas: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/rockchip: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/solomon: Use struct drm_crtc::drm_dev instead of struct
drm_crtc::dev
  drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
  drm/sun4i: Use struct

[PATCH 2/5] mm: use vma_is_stack() and vma_is_heap()

Use the helpers to simplify code.

Signed-off-by: Kefeng Wang 
---
 fs/proc/task_mmu.c   | 24 
 fs/proc/task_nommu.c | 15 +--
 2 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index cfab855fe7e9..05e9893552ce 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -236,21 +236,6 @@ static int do_maps_open(struct inode *inode, struct file 
*file,
sizeof(struct proc_maps_private));
 }
 
-/*
- * Indicate if the VMA is a stack for the given task; for
- * /proc/PID/maps that is the stack of the main task.
- */
-static int is_stack(struct vm_area_struct *vma)
-{
-   /*
-* We make no effort to guess what a given thread considers to be
-* its "stack".  It's not even well-defined for programs written
-* languages like Go.
-*/
-   return vma->vm_start <= vma->vm_mm->start_stack &&
-   vma->vm_end >= vma->vm_mm->start_stack;
-}
-
 static void show_vma_header_prefix(struct seq_file *m,
   unsigned long start, unsigned long end,
   vm_flags_t flags, unsigned long long pgoff,
@@ -327,13 +312,12 @@ show_map_vma(struct seq_file *m, struct vm_area_struct 
*vma)
goto done;
}
 
-   if (vma->vm_start <= mm->brk &&
-   vma->vm_end >= mm->start_brk) {
+   if (vma_is_heap(vma)) {
name = "[heap]";
goto done;
}
 
-   if (is_stack(vma)) {
+   if (vma_is_stack(vma)) {
name = "[stack]";
goto done;
}
@@ -1974,9 +1958,9 @@ static int show_numa_map(struct seq_file *m, void *v)
if (file) {
seq_puts(m, " file=");
seq_file_path(m, file, "\n\t= ");
-   } else if (vma->vm_start <= mm->brk && vma->vm_end >= mm->start_brk) {
+   } else if (vma_is_heap(vma)) {
seq_puts(m, " heap");
-   } else if (is_stack(vma)) {
+   } else if (vma_is_stack(vma)) {
seq_puts(m, " stack");
}
 
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 2c8b62265981..f42c84172b9e 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -121,19 +121,6 @@ unsigned long task_statm(struct mm_struct *mm,
return size;
 }
 
-static int is_stack(struct vm_area_struct *vma)
-{
-   struct mm_struct *mm = vma->vm_mm;
-
-   /*
-* We make no effort to guess what a given thread considers to be
-* its "stack".  It's not even well-defined for programs written
-* languages like Go.
-*/
-   return vma->vm_start <= mm->start_stack &&
-   vma->vm_end >= mm->start_stack;
-}
-
 /*
  * display a single VMA to a sequenced file
  */
@@ -171,7 +158,7 @@ static int nommu_vma_show(struct seq_file *m, struct 
vm_area_struct *vma)
if (file) {
seq_pad(m, ' ');
seq_file_path(m, file, "");
-   } else if (mm && is_stack(vma)) {
+   } else if (mm && vma_is_stack(vma)) {
seq_pad(m, ' ');
seq_puts(m, "[stack]");
}
-- 
2.41.0

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Paul Kocialkowski

Hi Uwe,

On Wed 12 Jul 23, 11:46, Uwe Kleine-König wrote:
> Hello,
> 
> while I debugged an issue in the imx-lcdc driver I was constantly
> irritated about struct drm_device pointer variables being named "dev"
> because with that name I usually expect a struct device pointer.

Well personally I usually expect that the "dev" member of a subsystem-specific
struct refers to a device of that subsystem, so for me having "dev" refer to
a drm_device for e.g. drm_crtc makes good sense.

I would only expect dev to refer to a struct device in the subsystem-specific
device structure (drm_device). I don't think it makes much sense to carry
the struct device in any other subsystem-specific structure anyway.

So IMO things are fine as-is but this is not a very strong opinion either.

> I think there is a big benefit when these are all renamed to "drm_dev".
> I have no strong preference here though, so "drmdev" or "drm" are fine
> for me, too. Let the bikesheding begin!

I would definitely prefer "drm_dev" over "drmdev" (hard to read, feels like
aborted camelcase, pretty ugly) or "drm" (too vague).

Cheers,

Paul

> Some statistics:
> 
> $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c 
> | sort -n
>   1 struct drm_device *adev_to_drm
>   1 struct drm_device *drm_
>   1 struct drm_device  *drm_dev
>   1 struct drm_device*drm_dev
>   1 struct drm_device *pdev
>   1 struct drm_device *rdev
>   1 struct drm_device *vdev
>   2 struct drm_device *dcss_drv_dev_to_drm
>   2 struct drm_device **ddev
>   2 struct drm_device *drm_dev_alloc
>   2 struct drm_device *mock
>   2 struct drm_device *p_ddev
>   5 struct drm_device *device
>   9 struct drm_device * dev
>  25 struct drm_device *d
>  95 struct drm_device *
> 216 struct drm_device *ddev
> 234 struct drm_device *drm_dev
> 611 struct drm_device *drm
>4190 struct drm_device *dev
> 
> This series starts with renaming struct drm_crtc::dev to drm_dev. If
> it's not only me and others like the result of this effort it should be
> followed up by adapting the other structs and the individual usages in
> the different drivers.
> 
> To make this series a bit easier handleable, I first added an alias for
> drm_crtc::dev, then converted the drivers one after another and the last
> patch drops the "dev" name. This has the advantage of being easier to
> review, and if I should have missed an instance only the last patch must
> be dropped/reverted. Also this series might conflict with other patches,
> in this case the remaining patches can still go in (apart from the last
> one of course). Maybe it also makes sense to delay applying the last
> patch by one development cycle?
> 
> The series was compile tested for arm, arm64, powerpc and amd64 using an
> allmodconfig (though I only build drivers/gpu/).
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (52):
>   drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
>   drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/armada: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/exynos: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/gma500: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/meson: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
>   drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
> drm_crtc::dev
>   drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
>

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Thomas Zimmermann


Hi

Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".


If you rename drm_crtc.dev, you should also address *all* other data 
structures.



I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!


We've discussed this to death. IIRC 'drm' would be the prefered choice.

Best regards
Thomas



Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
   1 struct drm_device *adev_to_drm
   1 struct drm_device *drm_
   1 struct drm_device  *drm_dev
   1 struct drm_device*drm_dev
   1 struct drm_device *pdev
   1 struct drm_device *rdev
   1 struct drm_device *vdev
   2 struct drm_device *dcss_drv_dev_to_drm
   2 struct drm_device **ddev
   2 struct drm_device *drm_dev_alloc
   2 struct drm_device *mock
   2 struct drm_device *p_ddev
   5 struct drm_device *device
   9 struct drm_device * dev
  25 struct drm_device *d
  95 struct drm_device *
 216 struct drm_device *ddev
 234 struct drm_device *drm_dev
 611 struct drm_device *drm
4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?

The series was compile tested for arm, arm64, powerpc and amd64 using an
allmodconfig (though I only build drivers/gpu/).

Best regards
Uwe

Uwe Kleine-König (52):
   drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
   drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/armada: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/exynos: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gma500: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/meson: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/pl111: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/radeon: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/renesas: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/rockchip: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/solomon: Use

[PATCH 4/5] selinux: use vma_is_stack() and vma_is_heap()

Use the helpers to simplify code.

Signed-off-by: Kefeng Wang 
---
 security/selinux/hooks.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4e46cf3d67b6..289ef2d6a427 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3775,13 +3775,10 @@ static int selinux_file_mprotect(struct vm_area_struct 
*vma,
if (default_noexec &&
(prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
int rc = 0;
-   if (vma->vm_start >= vma->vm_mm->start_brk &&
-   vma->vm_end <= vma->vm_mm->brk) {
+   if (vma_is_heap(vma)) {
rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
  PROCESS__EXECHEAP, NULL);
-   } else if (!vma->vm_file &&
-  ((vma->vm_start <= vma->vm_mm->start_stack &&
-vma->vm_end >= vma->vm_mm->start_stack) ||
+   } else if (!vma->vm_file && vma_is_stack(vma) ||
vma_is_stack_for_current(vma))) {
rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
  PROCESS__EXECSTACK, NULL);
-- 
2.41.0

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Julia Lawall



On Wed, 12 Jul 2023, Uwe Kleine-König wrote:

> On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:
> > Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > > Hello,
> > >
> > > while I debugged an issue in the imx-lcdc driver I was constantly
> > > irritated about struct drm_device pointer variables being named "dev"
> > > because with that name I usually expect a struct device pointer.
> > >
> > > I think there is a big benefit when these are all renamed to "drm_dev".
> > > I have no strong preference here though, so "drmdev" or "drm" are fine
> > > for me, too. Let the bikesheding begin!
> > >
> > > Some statistics:
> > >
> > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > > -c | sort -n
> > >1 struct drm_device *adev_to_drm
> > >1 struct drm_device *drm_
> > >1 struct drm_device  *drm_dev
> > >1 struct drm_device*drm_dev
> > >1 struct drm_device *pdev
> > >1 struct drm_device *rdev
> > >1 struct drm_device *vdev
> > >2 struct drm_device *dcss_drv_dev_to_drm
> > >2 struct drm_device **ddev
> > >2 struct drm_device *drm_dev_alloc
> > >2 struct drm_device *mock
> > >2 struct drm_device *p_ddev
> > >5 struct drm_device *device
> > >9 struct drm_device * dev
> > >   25 struct drm_device *d
> > >   95 struct drm_device *
> > >  216 struct drm_device *ddev
> > >  234 struct drm_device *drm_dev
> > >  611 struct drm_device *drm
> > > 4190 struct drm_device *dev
> > >
> > > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > > it's not only me and others like the result of this effort it should be
> > > followed up by adapting the other structs and the individual usages in
> > > the different drivers.
> > >
> > > To make this series a bit easier handleable, I first added an alias for
> > > drm_crtc::dev, then converted the drivers one after another and the last
> > > patch drops the "dev" name. This has the advantage of being easier to
> > > review, and if I should have missed an instance only the last patch must
> > > be dropped/reverted. Also this series might conflict with other patches,
> > > in this case the remaining patches can still go in (apart from the last
> > > one of course). Maybe it also makes sense to delay applying the last
> > > patch by one development cycle?
> >
> > When you automatically generate the patch (with cocci for example) I usually
> > prefer a single patch instead.
>
> Maybe I'm too stupid, but only parts of this patch were created by
> coccinelle. I failed to convert code like
>
> -   spin_lock_irq(>dev->event_lock);
> +   spin_lock_irq(>drm_dev->event_lock);
>
> Added Julia to Cc, maybe she has a hint?!

A priori, I see no reason why the rule below should not apply to the above
code.  Is there a parsing problem in the containing function?  You can run

spatch --parse-c file.c

If there is a paring problem, please let me know and i will try to fix it
so the while thing can be done automatically.

julia

>
> (Up to now it's only
>
> @@
> struct drm_crtc *crtc;
> @@
> -crtc->dev
> +crtc->drm_dev
>
> )
>
> > Background is that this makes merge conflicts easier to handle and detect.
>
> Really? Each file (apart from include/drm/drm_crtc.h) is only touched
> once. So unless I'm missing something you don't get less or easier
> conflicts by doing it all in a single patch. But you gain the freedom to
> drop a patch for one driver without having to drop the rest with it. So
> I still like the split version better, but I'm open to a more verbose
> reasoning from your side.
>
> > When you have multiple patches and a merge conflict because of some added
> > lines using the old field the build breaks only on the last patch which
> > removes the old field.
>
> Then you can revert/drop the last patch without having to respin the
> whole single patch and thus caring for still more conflicts that arise
> until the new version is sent.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
>

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Maxime Ripard

On Wed, Jul 12, 2023 at 03:38:03PM +0200, Uwe Kleine-König wrote:
> Hello Maxime,
> 
> On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
> > On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> > > > Background is that this makes merge conflicts easier to handle and 
> > > > detect.
> > > 
> > > Really?
> > 
> > FWIW, I agree with Christian here.
> > 
> > > Each file (apart from include/drm/drm_crtc.h) is only touched once. So
> > > unless I'm missing something you don't get less or easier conflicts by
> > > doing it all in a single patch. But you gain the freedom to drop a
> > > patch for one driver without having to drop the rest with it.
> > 
> > Not really, because the last patch removed the union anyway. So you have
> > to revert both the last patch, plus that driver one. And then you need
> > to add a TODO to remove that union eventually.
> 
> Yes, with a single patch you have only one revert (but 194 files changed,
> 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
> file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
> bigger). (And maybe you get away with just reverting the last patch.)
> 
> With a single patch the TODO after a revert is "redo it all again (and
> prepare for a different set of conflicts)" while with the split series
> it's only "fix that one driver that was forgotten/borked" + reapply that
> 10 line patch. As the one who gets that TODO, I prefer the latter.
> 
> So in sum: If your metric is "small count of reverted commits", you're
> right. If however your metric is: Better get 95% of this series' change
> in than maybe 0%, the split series is the way to do it.

I guess that's where we disagree: I don't see the point of having 95% of
it, either 0 or 100.

> With me having spend ~3h on this series' changes, it's maybe
> understandable that I did it the way I did.

I'm sorry, but that's never been an argument? I'm sure you and I both
have had series that took much longer dropped because it wasn't the
right approach.

> FTR: This series was created on top of v6.5-rc1. If you apply it to
> drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
> be the responsible maintainer who applies this series, I like being able
> to just do git am --skip then. 

Or we can ask that the driver is based on drm-misc-next ...

> FTR#2: In drm-misc-next is a new driver
> (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
> now might indeed be a good idea.

... which is going to fix that one too.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Christian König


Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!

Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
   1 struct drm_device *adev_to_drm
   1 struct drm_device *drm_
   1 struct drm_device  *drm_dev
   1 struct drm_device*drm_dev
   1 struct drm_device *pdev
   1 struct drm_device *rdev
   1 struct drm_device *vdev
   2 struct drm_device *dcss_drv_dev_to_drm
   2 struct drm_device **ddev
   2 struct drm_device *drm_dev_alloc
   2 struct drm_device *mock
   2 struct drm_device *p_ddev
   5 struct drm_device *device
   9 struct drm_device * dev
  25 struct drm_device *d
  95 struct drm_device *
 216 struct drm_device *ddev
 234 struct drm_device *drm_dev
 611 struct drm_device *drm
4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?


When you automatically generate the patch (with cocci for example) I 
usually prefer a single patch instead.


Background is that this makes merge conflicts easier to handle and detect.

When you have multiple patches and a merge conflict because of some 
added lines using the old field the build breaks only on the last patch 
which removes the old field.


In such cases reviewing the patch just means automatically re-generating 
it and double checking that you don't see anything funky.


Apart from that I honestly absolutely don't care what the name is.

Cheers,
Christian.



The series was compile tested for arm, arm64, powerpc and amd64 using an
allmodconfig (though I only build drivers/gpu/).

Best regards
Uwe

Uwe Kleine-König (52):
   drm/crtc: Start renaming struct drm_crtc::dev to drm_dev
   drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/armada: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/aspeed: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/exynos: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gma500: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/hyperv: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/ingenic: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/logicvc: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mediatek: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/meson: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/mgag200: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev
   drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/nouveau: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct
 drm_crtc::dev
   drm/pl111: Use struct

[PATCH 1/5] mm: introduce vma_is_stack() and vma_is_heap()

Introduce the two helpers for general use.

Signed-off-by: Kefeng Wang 
---
 include/linux/mm.h | 12 
 1 file changed, 12 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1462cf15badf..0bbeb31ac750 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -926,6 +926,18 @@ static inline bool vma_is_anonymous(struct vm_area_struct 
*vma)
return !vma->vm_ops;
 }
 
+static inline bool vma_is_heap(struct vm_area_struct *vma)
+{
+   return vma->vm_start <= vma->vm_mm->brk &&
+   vma->vm_end >= vma->vm_mm->start_brk;
+}
+
+static inline bool vma_is_stack(struct vm_area_struct *vma)
+{
+   return vma->vm_start <= vma->vm_mm->start_stack &&
+  vma->vm_end >= vma->vm_mm->start_stack;
+}
+
 static inline bool vma_is_temporary_stack(struct vm_area_struct *vma)
 {
int maybe_stack = vma->vm_flags & (VM_GROWSDOWN | VM_GROWSUP);
-- 
2.41.0

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Christian König


Am 12.07.23 um 15:38 schrieb Uwe Kleine-König:

Hello Maxime,

On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:

On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:

Background is that this makes merge conflicts easier to handle and detect.

Really?

FWIW, I agree with Christian here.


Each file (apart from include/drm/drm_crtc.h) is only touched once. So
unless I'm missing something you don't get less or easier conflicts by
doing it all in a single patch. But you gain the freedom to drop a
patch for one driver without having to drop the rest with it.

Not really, because the last patch removed the union anyway. So you have
to revert both the last patch, plus that driver one. And then you need
to add a TODO to remove that union eventually.

Yes, with a single patch you have only one revert (but 194 files changed,
1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
bigger). (And maybe you get away with just reverting the last patch.)

With a single patch the TODO after a revert is "redo it all again (and
prepare for a different set of conflicts)" while with the split series
it's only "fix that one driver that was forgotten/borked" + reapply that
10 line patch.


Yeah, but for a maintainer the size of the patches doesn't matter. 
That's only interesting if you need to manually review the patch, which 
you hopefully doesn't do in case of something auto-generated.


In other words if the patch is auto-generated re-applying it completely 
is less work than fixing things up individually.



  As the one who gets that TODO, I prefer the latter.


Yeah, but your personal preferences are not a technical relevant 
argument to a maintainer.


At the end of the day Dave or Daniel need to decide, because they need 
to live with it.


Regards,
Christian.



So in sum: If your metric is "small count of reverted commits", you're
right. If however your metric is: Better get 95% of this series' change
in than maybe 0%, the split series is the way to do it.

With me having spend ~3h on this series' changes, it's maybe
understandable that I did it the way I did.

FTR: This series was created on top of v6.5-rc1. If you apply it to
drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
be the responsible maintainer who applies this series, I like being able
to just do git am --skip then.

FTR#2: In drm-misc-next is a new driver
(drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
now might indeed be a good idea.


So I still like the split version better, but I'm open to a more
verbose reasoning from your side.

You're doing only one thing here, really: you change the name of a
structure field. If it was shared between multiple maintainers, then
sure, splitting that up is easier for everyone, but this will go through
drm-misc, so I can't see the benefit it brings.

I see your argument, but I think mine weights more.

Best regards
Uwe

[PATCH 5/5] perf/core: use vma_is_stack() and vma_is_heap()

Use the helpers to simplify code, also kill unneeded goto cpy_name.

Signed-off-by: Kefeng Wang 
---
 kernel/events/core.c | 22 +++---
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 78ae7b6f90fd..cb271f449b81 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8685,22 +8685,14 @@ static void perf_event_mmap_event(struct 
perf_mmap_event *mmap_event)
}
 
name = (char *)arch_vma_name(vma);
-   if (name)
-   goto cpy_name;
-
-   if (vma->vm_start <= vma->vm_mm->start_brk &&
-   vma->vm_end >= vma->vm_mm->brk) {
-   name = "[heap]";
-   goto cpy_name;
+   if (!name) {
+   if (vma_is_heap(vma))
+   name = "[heap]";
+   else if (vma_is_stack(vma))
+   name = "[stack]";
+   else
+   name = "//anon";
}
-   if (vma->vm_start <= vma->vm_mm->start_stack &&
-   vma->vm_end >= vma->vm_mm->start_stack) {
-   name = "[stack]";
-   goto cpy_name;
-   }
-
-   name = "//anon";
-   goto cpy_name;
}
 
 cpy_name:
-- 
2.41.0

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:
> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > Hello,
> > 
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> > 
> > I think there is a big benefit when these are all renamed to "drm_dev".
> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> > 
> > Some statistics:
> > 
> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq 
> > -c | sort -n
> >1 struct drm_device *adev_to_drm
> >1 struct drm_device *drm_
> >1 struct drm_device  *drm_dev
> >1 struct drm_device*drm_dev
> >1 struct drm_device *pdev
> >1 struct drm_device *rdev
> >1 struct drm_device *vdev
> >2 struct drm_device *dcss_drv_dev_to_drm
> >2 struct drm_device **ddev
> >2 struct drm_device *drm_dev_alloc
> >2 struct drm_device *mock
> >2 struct drm_device *p_ddev
> >5 struct drm_device *device
> >9 struct drm_device * dev
> >   25 struct drm_device *d
> >   95 struct drm_device *
> >  216 struct drm_device *ddev
> >  234 struct drm_device *drm_dev
> >  611 struct drm_device *drm
> > 4190 struct drm_device *dev
> > 
> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > it's not only me and others like the result of this effort it should be
> > followed up by adapting the other structs and the individual usages in
> > the different drivers.
> > 
> > To make this series a bit easier handleable, I first added an alias for
> > drm_crtc::dev, then converted the drivers one after another and the last
> > patch drops the "dev" name. This has the advantage of being easier to
> > review, and if I should have missed an instance only the last patch must
> > be dropped/reverted. Also this series might conflict with other patches,
> > in this case the remaining patches can still go in (apart from the last
> > one of course). Maybe it also makes sense to delay applying the last
> > patch by one development cycle?
> 
> When you automatically generate the patch (with cocci for example) I usually
> prefer a single patch instead.

Maybe I'm too stupid, but only parts of this patch were created by
coccinelle. I failed to convert code like

-   spin_lock_irq(>dev->event_lock);
+   spin_lock_irq(>drm_dev->event_lock);

Added Julia to Cc, maybe she has a hint?!

(Up to now it's only 

@@
struct drm_crtc *crtc;
@@
-crtc->dev
+crtc->drm_dev

)

> Background is that this makes merge conflicts easier to handle and detect.

Really? Each file (apart from include/drm/drm_crtc.h) is only touched
once. So unless I'm missing something you don't get less or easier
conflicts by doing it all in a single patch. But you gain the freedom to
drop a patch for one driver without having to drop the rest with it. So
I still like the split version better, but I'm open to a more verbose
reasoning from your side.

> When you have multiple patches and a merge conflict because of some added
> lines using the old field the build breaks only on the last patch which
> removes the old field.

Then you can revert/drop the last patch without having to respin the
whole single patch and thus caring for still more conflicts that arise
until the new version is sent.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Andrzej Hajda





On 12.07.2023 13:07, Julia Lawall wrote:


On Wed, 12 Jul 2023, Uwe Kleine-König wrote:


On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:

Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:

Hello,

while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.

I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!

Some statistics:

$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | 
sort -n
1 struct drm_device *adev_to_drm
1 struct drm_device *drm_
1 struct drm_device  *drm_dev
1 struct drm_device*drm_dev
1 struct drm_device *pdev
1 struct drm_device *rdev
1 struct drm_device *vdev
2 struct drm_device *dcss_drv_dev_to_drm
2 struct drm_device **ddev
2 struct drm_device *drm_dev_alloc
2 struct drm_device *mock
2 struct drm_device *p_ddev
5 struct drm_device *device
9 struct drm_device * dev
   25 struct drm_device *d
   95 struct drm_device *
  216 struct drm_device *ddev
  234 struct drm_device *drm_dev
  611 struct drm_device *drm
 4190 struct drm_device *dev

This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.

To make this series a bit easier handleable, I first added an alias for
drm_crtc::dev, then converted the drivers one after another and the last
patch drops the "dev" name. This has the advantage of being easier to
review, and if I should have missed an instance only the last patch must
be dropped/reverted. Also this series might conflict with other patches,
in this case the remaining patches can still go in (apart from the last
one of course). Maybe it also makes sense to delay applying the last
patch by one development cycle?

When you automatically generate the patch (with cocci for example) I usually
prefer a single patch instead.

Maybe I'm too stupid, but only parts of this patch were created by
coccinelle. I failed to convert code like

-   spin_lock_irq(>dev->event_lock);
+   spin_lock_irq(>drm_dev->event_lock);

Added Julia to Cc, maybe she has a hint?!

A priori, I see no reason why the rule below should not apply to the above
code.  Is there a parsing problem in the containing function?  You can run

spatch --parse-c file.c

If there is a paring problem, please let me know and i will try to fix it
so the while thing can be done automatically.


I guess some clever macros can fool spatch, at least I observe such 
things in i915 which often uses custom iterators.


Regards
Andrzej



julia


(Up to now it's only

@@
struct drm_crtc *crtc;
@@
-crtc->dev
+crtc->drm_dev

)


Background is that this makes merge conflicts easier to handle and detect.

Really? Each file (apart from include/drm/drm_crtc.h) is only touched
once. So unless I'm missing something you don't get less or easier
conflicts by doing it all in a single patch. But you gain the freedom to
drop a patch for one driver without having to drop the rest with it. So
I still like the split version better, but I'm open to a more verbose
reasoning from your side.


When you have multiple patches and a merge conflict because of some added
lines using the old field the build breaks only on the last patch which
removes the old field.

Then you can revert/drop the last patch without having to respin the
whole single patch and thus caring for still more conflicts that arise
until the new version is sent.

Best regards
Uwe

--
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |

>

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

2023-07-12 Thread Jani Nikula

On Wed, 12 Jul 2023, Uwe Kleine-König  wrote:
> Hello,
>
> while I debugged an issue in the imx-lcdc driver I was constantly
> irritated about struct drm_device pointer variables being named "dev"
> because with that name I usually expect a struct device pointer.
>
> I think there is a big benefit when these are all renamed to "drm_dev".
> I have no strong preference here though, so "drmdev" or "drm" are fine
> for me, too. Let the bikesheding begin!
>
> Some statistics:
>
> $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c 
> | sort -n
>   1 struct drm_device *adev_to_drm
>   1 struct drm_device *drm_
>   1 struct drm_device  *drm_dev
>   1 struct drm_device*drm_dev
>   1 struct drm_device *pdev
>   1 struct drm_device *rdev
>   1 struct drm_device *vdev
>   2 struct drm_device *dcss_drv_dev_to_drm
>   2 struct drm_device **ddev
>   2 struct drm_device *drm_dev_alloc
>   2 struct drm_device *mock
>   2 struct drm_device *p_ddev
>   5 struct drm_device *device
>   9 struct drm_device * dev
>  25 struct drm_device *d
>  95 struct drm_device *
> 216 struct drm_device *ddev
> 234 struct drm_device *drm_dev
> 611 struct drm_device *drm
>4190 struct drm_device *dev
>
> This series starts with renaming struct drm_crtc::dev to drm_dev. If
> it's not only me and others like the result of this effort it should be
> followed up by adapting the other structs and the individual usages in
> the different drivers.

I think this is an unnecessary change. In drm, a dev is usually a drm
device, i.e. struct drm_device *. As shown by the numbers above.

If folks insist on following through with this anyway, I'm firmly in the
camp the name should be "drm" and nothing else.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

Hello Thomas,

On Wed, Jul 12, 2023 at 12:19:37PM +0200, Thomas Zimmermann wrote:
> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > Hello,
> > 
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> > 
> > I think there is a big benefit when these are all renamed to "drm_dev".
> 
> If you rename drm_crtc.dev, you should also address *all* other data
> structures.

Yes. Changing drm_crtc::dev was some effort, so I thought to send that
one out before doing the same to

drm_dp_mst_topology_mgr
drm_atomic_state
drm_master
drm_bridge
drm_client_dev
drm_connector
drm_debugfs_entry
drm_encoder
drm_fb_helper
drm_minor
drm_framebuffer
drm_gem_object
drm_plane
drm_property
drm_property_blob
drm_vblank_crtc

when in the end the intention isn't welcome.

> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> 
> We've discussed this to death. IIRC 'drm' would be the prefered choice.

"drm" at least has the advantage to be the 2nd most common name. With
Paul Kocialkowski prefering "drm_dev" there is no clear favourite yet.
Maybe all the other people with strong opinions are dead if this was
"discussed to death" before? :-)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |


signature.asc
Description: PGP signature

[PATCH 0/5] mm: convert to vma_is_heap/stack()