[PATCH v2 0/9] drm/bridge: imx: Add i.MX93 MIPI DSI support
Hi, This series aims to add MIPI DSI support for Freescale i.MX93 SoC. There is a Synopsys DesignWare MIPI DSI host controller and a Synopsys Designware MIPI DPHY embedded in i.MX93. Some configurations and extensions to them are controlled by i.MX93 media blk-ctrl. Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI bridge helpers and implementing i.MX93 MIPI DSI specific extensions. Note that since this series touches the dw-mipi-dsi driver, tests are needed to be done for meson, rockchip and stm. Patch 1 ~ 7 do preparation work for adding i.MX93 MIPI DSI DRM bridge driver. Patch 8 adds DT-binding documentation for i.MX93 MIPI DSI. Patch 9 adds i.MX93 MIPI DSI DRM bridge. v1->v2: * Add Rob's R-b tag on patch 8. * Use dev_err_probe() to replace DRM_DEV_ERROR() in patch 9. (Sam and Alexander) * Use dev_*() to replace DRM_*() in patch 9. (Sam) * Fix build for arm architecture in patch 9. (Reported-by: kernel test robot ) * Improve error messages for imx93_dsi_phy_init() in patch 9. Liu Ying (9): drm/bridge: synopsys: dw-mipi-dsi: Add dw_mipi_dsi_get_bridge() helper drm/bridge: synopsys: dw-mipi-dsi: Add input bus format negotiation support drm/bridge: synopsys: dw-mipi-dsi: Force input bus flags drm/bridge: synopsys: dw-mipi-dsi: Add mode fixup support drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles for HSA and HBP drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI drm/bridge: imx: Add i.MX93 MIPI DSI support .../display/bridge/fsl,imx93-mipi-dsi.yaml| 115 +++ drivers/gpu/drm/bridge/imx/Kconfig| 10 + drivers/gpu/drm/bridge/imx/Makefile | 1 + drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 917 ++ drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 91 +- include/drm/bridge/dw_mipi_dsi.h | 16 + 6 files changed, 1146 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c -- 2.37.1
[PATCH v2 1/9] drm/bridge: synopsys: dw-mipi-dsi: Add dw_mipi_dsi_get_bridge() helper
Add dw_mipi_dsi_get_bridge() helper so that it can be used by vendor
drivers which implement vendor specific extensions to Synopsys DW MIPI DSI.
Signed-off-by: Liu Ying
---
v1->v2:
* No change.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 ++
include/drm/bridge/dw_mipi_dsi.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index b2efecf7d160..57eae0fdd970 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -1207,6 +1207,12 @@ void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi,
struct dw_mipi_dsi *slave)
}
EXPORT_SYMBOL_GPL(dw_mipi_dsi_set_slave);
+struct drm_bridge *dw_mipi_dsi_get_bridge(struct dw_mipi_dsi *dsi)
+{
+ return &dsi->bridge;
+}
+EXPORT_SYMBOL_GPL(dw_mipi_dsi_get_bridge);
+
/*
* Probe/remove API, used from platforms based on the DRM bridge API.
*/
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index 5286a53a1875..f54621b17a69 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -11,6 +11,7 @@
#include
+#include
#include
struct drm_display_mode;
@@ -68,5 +69,6 @@ void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder);
void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave);
+struct drm_bridge *dw_mipi_dsi_get_bridge(struct dw_mipi_dsi *dsi);
#endif /* __DW_MIPI_DSI__ */
--
2.37.1
[PATCH v2 2/9] drm/bridge: synopsys: dw-mipi-dsi: Add input bus format negotiation support
Introduce ->get_input_bus_fmts() callback to struct dw_mipi_dsi_plat_data
so that vendor drivers can implement specific methods to get input bus
formats for Synopsys DW MIPI DSI.
While at it, implement a generic callback for ->atomic_get_input_bus_fmts(),
where we try to get the input bus formats through pdata->get_input_bus_fmts()
first. If it's unavailable, fall back to the only format - MEDIA_BUS_FMT_FIXED,
which matches the default behavior if ->atomic_get_input_bus_fmts() is not
implemented as ->atomic_get_input_bus_fmts()'s kerneldoc indicates.
Signed-off-by: Liu Ying
---
v1->v2:
* No change.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 30 +++
include/drm/bridge/dw_mipi_dsi.h | 11 +++
2 files changed, 41 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 57eae0fdd970..8580b8a97fb1 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -536,6 +537,34 @@ static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops
= {
.transfer = dw_mipi_dsi_host_transfer,
};
+static u32 *
+dw_mipi_dsi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+struct drm_bridge_state
*bridge_state,
+struct drm_crtc_state *crtc_state,
+struct drm_connector_state
*conn_state,
+u32 output_fmt,
+unsigned int *num_input_fmts)
+{
+ struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
+ const struct dw_mipi_dsi_plat_data *pdata = dsi->plat_data;
+ u32 *input_fmts;
+
+ if (pdata->get_input_bus_fmts)
+ return pdata->get_input_bus_fmts(pdata->priv_data,
+bridge, bridge_state,
+crtc_state, conn_state,
+output_fmt, num_input_fmts);
+
+ /* Fall back to MEDIA_BUS_FMT_FIXED as the only input format. */
+ input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+ input_fmts[0] = MEDIA_BUS_FMT_FIXED;
+ *num_input_fmts = 1;
+
+ return input_fmts;
+}
+
static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
{
u32 val;
@@ -1003,6 +1032,7 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge
*bridge,
static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_get_input_bus_fmts =
dw_mipi_dsi_bridge_atomic_get_input_bus_fmts,
.atomic_reset = drm_atomic_helper_bridge_reset,
.atomic_enable = dw_mipi_dsi_bridge_atomic_enable,
.atomic_post_disable= dw_mipi_dsi_bridge_post_atomic_disable,
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index f54621b17a69..246650f2814f 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -11,7 +11,10 @@
#include
+#include
#include
+#include
+#include
#include
struct drm_display_mode;
@@ -56,6 +59,14 @@ struct dw_mipi_dsi_plat_data {
unsigned long mode_flags,
u32 lanes, u32 format);
+ u32 *(*get_input_bus_fmts)(void *priv_data,
+ struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts);
+
const struct dw_mipi_dsi_phy_ops *phy_ops;
const struct dw_mipi_dsi_host_ops *host_ops;
--
2.37.1
[PATCH v2 3/9] drm/bridge: synopsys: dw-mipi-dsi: Force input bus flags
The DATAEN_ACTIVE_LOW bit in DSI_DPI_CFG_POL register is set to zero,
so set the DRM_BUS_FLAG_DE_HIGH flag in input_bus_cfg.flags. It appears
that the DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE flag also makes sense, so
set it in input_bus_cfg.flags too. With this patch, the flags set by
drm_atomic_bridge_propagate_bus_flags() are overridden (see comment in
that function) in case any downstream bridges propagates invalid flags
to this bridge. A real problematic case is to connect a RM67191 MIPI
DSI panel whose driver sets DRM_BUS_FLAG_DE_LOW and
DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE bus flags.
Signed-off-by: Liu Ying
---
v1->v2:
* No change.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 8580b8a97fb1..8cd89a63b5f2 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -565,6 +566,17 @@ dw_mipi_dsi_bridge_atomic_get_input_bus_fmts(struct
drm_bridge *bridge,
return input_fmts;
}
+static int dw_mipi_dsi_bridge_atomic_check(struct drm_bridge *bridge,
+ struct drm_bridge_state
*bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state
*conn_state)
+{
+ bridge_state->input_bus_cfg.flags =
+ DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
+
+ return 0;
+}
+
static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
{
u32 val;
@@ -1033,6 +1045,7 @@ static const struct drm_bridge_funcs
dw_mipi_dsi_bridge_funcs = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_get_input_bus_fmts =
dw_mipi_dsi_bridge_atomic_get_input_bus_fmts,
+ .atomic_check = dw_mipi_dsi_bridge_atomic_check,
.atomic_reset = drm_atomic_helper_bridge_reset,
.atomic_enable = dw_mipi_dsi_bridge_atomic_enable,
.atomic_post_disable= dw_mipi_dsi_bridge_post_atomic_disable,
--
2.37.1
[PATCH v2 4/9] drm/bridge: synopsys: dw-mipi-dsi: Add mode fixup support
Vendor drivers may need to fixup mode due to pixel clock tree limitation,
so introduce the ->mode_fixup() callcack to struct dw_mipi_dsi_plat_data
and call it at atomic check stage if available.
Signed-off-by: Liu Ying
---
v1->v2:
* No change.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 14 ++
include/drm/bridge/dw_mipi_dsi.h | 3 +++
2 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 8cd89a63b5f2..c754d55f71d1 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -571,9 +571,23 @@ static int dw_mipi_dsi_bridge_atomic_check(struct
drm_bridge *bridge,
struct drm_crtc_state *crtc_state,
struct drm_connector_state
*conn_state)
{
+ struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
+ const struct dw_mipi_dsi_plat_data *pdata = dsi->plat_data;
+ bool ret;
+
bridge_state->input_bus_cfg.flags =
DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
+ if (pdata->mode_fixup) {
+ ret = pdata->mode_fixup(pdata->priv_data, &crtc_state->mode,
+ &crtc_state->adjusted_mode);
+ if (!ret) {
+ DRM_DEBUG_DRIVER("failed to fixup mode " DRM_MODE_FMT
"\n",
+DRM_MODE_ARG(&crtc_state->mode));
+ return -EINVAL;
+ }
+ }
+
return 0;
}
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index 246650f2814f..65d5e68065e3 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -59,6 +59,9 @@ struct dw_mipi_dsi_plat_data {
unsigned long mode_flags,
u32 lanes, u32 format);
+ bool (*mode_fixup)(void *priv_data, const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode);
+
u32 *(*get_input_bus_fmts)(void *priv_data,
struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,
--
2.37.1
[PATCH v2 5/9] drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc
To get better accuration, use pixel clock rate to calculate lbcc instead of
lane_mbps since the pixel clock rate is in KHz while lane_mbps is in MHz.
Without this, distorted image can be seen on a HDMI monitor connected with
i.MX93 11x11 EVK through ADV7535 DSI to HDMI bridge in 1920x1080p@60 video
mode.
Signed-off-by: Liu Ying
---
v1->v2:
* No change.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index c754d55f71d1..332388fd86da 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -762,8 +763,15 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct
dw_mipi_dsi *dsi,
u32 hcomponent)
{
u32 frac, lbcc;
+ int bpp;
- lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
+ bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+ if (bpp < 0) {
+ dev_err(dsi->dev, "failed to get bpp\n");
+ return 0;
+ }
+
+ lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
frac = lbcc % mode->clock;
lbcc = lbcc / mode->clock;
--
2.37.1
[PATCH v2 6/9] drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles for HSA and HBP
According to Synopsys support channel, each region of HSA, HBP and HFP must
have minimum number of 10 bytes where constant 4 bytes are for HSS or HSE
and 6 bytes are for blanking packet(header + CRC). Hence, the below table
comes in.
++--+---+
| data lanes | min lbcc | bytes |
++--+---+
| 1 |10| 1*10 |
++--+---+
| 2 |5 | 2*5 |
++--+---+
| 3 |4 | 3*4 |
++--+---+
| 4 |3 | 4*3 |
++--+---+
Implement the minimum lbcc numbers to make sure that the values programmed
into DSI_VID_HSA_TIME and DSI_VID_HBP_TIME registers meet the minimum
number requirement. For DSI_VID_HLINE_TIME register, it seems that the
value programmed should be based on mode->htotal as-is, instead of sum up
HSA, HBP, HFP and HDISPLAY.
This helps the case where Raydium RM67191 DSI panel is connected, since
it's video timing for hsync length is only 2 pixels and without this patch
the programmed value for DSI_VID_HSA_TIME is only 2 with 4 data lanes.
Signed-off-by: Liu Ying
---
v1->v2:
* No change.
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 332388fd86da..536306ccea5a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -757,12 +757,19 @@ static void dw_mipi_dsi_command_mode_config(struct
dw_mipi_dsi *dsi)
dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
}
+static const u32 minimum_lbccs[] = {10, 5, 4, 3};
+
+static inline u32 dw_mipi_dsi_get_minimum_lbcc(struct dw_mipi_dsi *dsi)
+{
+ return minimum_lbccs[dsi->lanes - 1];
+}
+
/* Get lane byte clock cycles. */
static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
const struct drm_display_mode *mode,
u32 hcomponent)
{
- u32 frac, lbcc;
+ u32 frac, lbcc, minimum_lbcc;
int bpp;
bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
@@ -778,6 +785,11 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct
dw_mipi_dsi *dsi,
if (frac)
lbcc++;
+ minimum_lbcc = dw_mipi_dsi_get_minimum_lbcc(dsi);
+
+ if (lbcc < minimum_lbcc)
+ lbcc = minimum_lbcc;
+
return lbcc;
}
--
2.37.1
[PATCH v2 7/9] drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check
According to Synopsys DW MIPI DSI host databook, HSTX and LPRX timeout contention detections are measured in TO_CLK_DIVISION cycles. However, the current driver programs magic values to TO_CLK_DIVISION, HSTX_TO_CNT and LPRX_TO_CNT register fields, which makes timeout error event wrongly happen for some video modes, at least for the typical 1920x1080p@60 video mode read from a HDMI monitor driven by ADV7535 DSI to HDMI bridge. While at it, the current driver doesn't enable interrupt to handle or complain about the error status, so true error just happens silently except for display distortions by visual check. Disable the timeout check by setting those timeout register fields to zero for now until someone comes along with better computations for the timeout values. Although the databook doesn't mention what happens when they are set to zero, it turns out the false error doesn't happen for the 1920x1080p@60 video mode at least. Signed-off-by: Liu Ying --- v1->v2: * No change. drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 536306ccea5a..0fcadf99e783 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -684,7 +684,7 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) * timeout clock division should be computed with the * high speed transmission counter timeout and byte lane... */ - dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) | + dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(0) | TX_ESC_CLK_DIVISION(esc_clk_division)); } @@ -747,7 +747,7 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi) * compute high speed transmission counter timeout according * to the timeout clock division (TO_CLK_DIVISION) and byte lane... */ - dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000)); + dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(0) | LPRX_TO_CNT(0)); /* * TODO dw drv improvements * the Bus-Turn-Around Timeout Counter should be computed -- 2.37.1
[PATCH v2 8/9] dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI
Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
controller and a Synopsys Designware MIPI DPHY. Some configurations
and extensions to them are controlled by i.MX93 media blk-ctrl.
Signed-off-by: Liu Ying
Reviewed-by: Rob Herring
---
v1->v2:
* Add Rob's R-b tag.
.../display/bridge/fsl,imx93-mipi-dsi.yaml| 115 ++
1 file changed, 115 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
diff --git
a/Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
b/Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
new file mode 100644
index ..d6e51d0cf546
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/fsl,imx93-mipi-dsi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX93 specific extensions to Synopsys Designware MIPI DSI
+
+maintainers:
+ - Liu Ying
+
+description: |
+ There is a Synopsys Designware MIPI DSI Host Controller and a Synopsys
+ Designware MIPI DPHY embedded in Freescale i.MX93 SoC. Some configurations
+ and extensions to them are controlled by i.MX93 media blk-ctrl.
+
+allOf:
+ - $ref: snps,dw-mipi-dsi.yaml#
+
+properties:
+ compatible:
+const: fsl,imx93-mipi-dsi
+
+ clocks:
+items:
+ - description: apb clock
+ - description: pixel clock
+ - description: PHY configuration clock
+ - description: PHY reference clock
+
+ clock-names:
+items:
+ - const: pclk
+ - const: pix
+ - const: phy_cfg
+ - const: phy_ref
+
+ interrupts:
+maxItems: 1
+
+ fsl,media-blk-ctrl:
+$ref: /schemas/types.yaml#/definitions/phandle
+description:
+ i.MX93 media blk-ctrl, as a syscon, controls pixel component bit map
+ configurations from LCDIF display controller to the MIPI DSI host
+ controller and MIPI DPHY PLL related configurations through PLL SoC
+ interface.
+
+ power-domains:
+maxItems: 1
+
+required:
+ - compatible
+ - interrupts
+ - fsl,media-blk-ctrl
+ - power-domains
+
+unevaluatedProperties: false
+
+examples:
+ - |
+#include
+#include
+#include
+#include
+
+dsi@4ae1 {
+compatible = "fsl,imx93-mipi-dsi";
+reg = <0x4ae1 0x1>;
+interrupts = ;
+clocks = <&clk IMX93_CLK_MIPI_DSI_GATE>,
+ <&clk IMX93_CLK_MEDIA_DISP_PIX>,
+ <&clk IMX93_CLK_MIPI_PHY_CFG>,
+ <&clk IMX93_CLK_24M>;
+clock-names = "pclk", "pix", "phy_cfg", "phy_ref";
+fsl,media-blk-ctrl = <&media_blk_ctrl>;
+power-domains = <&media_blk_ctrl IMX93_MEDIABLK_PD_MIPI_DSI>;
+#address-cells = <1>;
+#size-cells = <0>;
+
+panel@0 {
+compatible = "raydium,rm67191";
+reg = <0>;
+reset-gpios = <&adp5585gpio 6 GPIO_ACTIVE_LOW>;
+dsi-lanes = <4>;
+video-mode = <2>;
+
+port {
+panel_in: endpoint {
+remote-endpoint = <&dsi_out>;
+};
+};
+};
+
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+
+port@0 {
+reg = <0>;
+
+dsi_to_lcdif: endpoint {
+remote-endpoint = <&lcdif_to_dsi>;
+};
+};
+
+port@1 {
+reg = <1>;
+
+dsi_out: endpoint {
+remote-endpoint = <&panel_in>;
+};
+};
+};
+};
--
2.37.1
[PATCH v2 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support
Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host controller and a Synopsys Designware MIPI DPHY. Some configurations and extensions to them are controlled by i.MX93 media blk-ctrl. Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI bridge helpers and implementing i.MX93 MIPI DSI specific extensions. Signed-off-by: Liu Ying --- v1->v2: * Use dev_err_probe() to replace DRM_DEV_ERROR(). (Sam and Alexander) * Use dev_*() to replace DRM_*(). (Sam) * Fix build for arm architecture. (Reported-by: kernel test robot ) * Improve error messages for imx93_dsi_phy_init(). drivers/gpu/drm/bridge/imx/Kconfig | 10 + drivers/gpu/drm/bridge/imx/Makefile | 1 + drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 917 3 files changed, 928 insertions(+) create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig index 9fae28db6aa7..5182298c7182 100644 --- a/drivers/gpu/drm/bridge/imx/Kconfig +++ b/drivers/gpu/drm/bridge/imx/Kconfig @@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI Choose this to enable pixel link to display pixel interface(PXL2DPI) found in Freescale i.MX8qxp processor. +config DRM_IMX93_MIPI_DSI + tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI DSI" + depends on OF + depends on COMMON_CLK + select DRM_DW_MIPI_DSI + select GENERIC_PHY_MIPI_DPHY + help + Choose this to enable MIPI DSI controller found in Freescale i.MX93 + processor. + endif # ARCH_MXC || COMPILE_TEST diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile index 8e2ebf3399a1..2b0c2e44aa1b 100644 --- a/drivers/gpu/drm/bridge/imx/Makefile +++ b/drivers/gpu/drm/bridge/imx/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o +obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c new file mode 100644 index ..3ff30ce80c5b --- /dev/null +++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c @@ -0,0 +1,917 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/* + * Copyright 2022,2023 NXP + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +/* DPHY PLL configuration registers */ +#define DSI_REG0x4c +#define CFGCLKFREQRANGE_MASK GENMASK(5, 0) +#define CFGCLKFREQRANGE(x)FIELD_PREP(CFGCLKFREQRANGE_MASK, (x)) +#define CLKSEL_MASK GENMASK(7, 6) +#define CLKSEL_STOP FIELD_PREP(CLKSEL_MASK, 0) +#define CLKSEL_GENFIELD_PREP(CLKSEL_MASK, 1) +#define CLKSEL_EXTFIELD_PREP(CLKSEL_MASK, 2) +#define HSFREQRANGE_MASK GENMASK(14, 8) +#define HSFREQRANGE(x)FIELD_PREP(HSFREQRANGE_MASK, (x)) +#define UPDATE_PLLBIT(17) +#define SHADOW_CLRBIT(18) +#define CLK_EXT BIT(19) + +#define DSI_WRITE_REG0 0x50 +#define M_MASKGENMASK(9, 0) +#define M(x) FIELD_PREP(M_MASK, ((x) - 2)) +#define N_MASKGENMASK(13, 10) +#define N(x) FIELD_PREP(N_MASK, ((x) - 1)) +#define VCO_CTRL_MASK GENMASK(19, 14) +#define VCO_CTRL(x) FIELD_PREP(VCO_CTRL_MASK, (x)) +#define PROP_CTRL_MASKGENMASK(25, 20) +#define PROP_CTRL(x) FIELD_PREP(PROP_CTRL_MASK, (x)) +#define INT_CTRL_MASK GENMASK(31, 26) +#define INT_CTRL(x) FIELD_PREP(INT_CTRL_MASK, (x)) + +#define DSI_WRITE_REG1 0x54 +#define GMP_CTRL_MASK GENMASK(1, 0) +#define GMP_CTRL(x) FIELD_PREP(GMP_CTRL_MASK, (x)) +#define CPBIAS_CTRL_MASK GENMASK(8, 2) +#define CPBIAS_CTRL(x)FIELD_PREP(CPBIAS_CTRL_MASK, (x)) +#define PLL_SHADOW_CTRL BIT(9) + +/* display mux control register */ +#define DISPLAY_MUX0x60 +#define MIPI_DSI_RGB666_MAP_CFG GENMASK(7, 6) +#define RGB666_CONFIG1 FIELD_PREP(MIPI_DSI_RGB666_MAP_CFG, 0) +#define RGB666_CONFIG2 FIELD_PREP(MIPI_DSI_RGB666_MAP_CFG, 1) +#define MIPI_DSI_RGB565_MAP_CFG GENMASK(5, 4) +#define RGB565_CONFIG1 FIELD_PREP(MIPI_DSI_RGB565_MAP_CFG, 0) +#define RGB565_CONFIG2
Re: [PATCH v4] drm: adv7511: Fix low refresh rate register for ADV7533/5
On Wed, Jul 19, 2023 at 9:02 AM Alexandru Ardelean wrote:
>
> From: Bogdan Togorean
>
> For ADV7533 and ADV7535 low refresh rate is selected using
> bits [3:2] of 0x4a main register.
> So depending on ADV model write 0xfb or 0x4a register.
>
> Fixes: 2437e7cd88e8 ("drm/bridge: adv7533: Initial support for ADV7533")
> Reviewed-by: Robert Foss
> Reviewed-by: Nuno Sa
> Signed-off-by: Bogdan Togorean
> Signed-off-by: Alexandru Ardelean
> ---
Right.
I forgot a changelog here.
Apologies
Changelog v3 -> v4:
* Ran ./scripts/checkpatch.pl --strict
* Added Reviewed-by: Robert Foss
Changelog v2 -> v3:
*
https://lore.kernel.org/dri-devel/1c3fde3a873b0f948d3c4d37107c5bb67dc9f7bb.ca...@gmail.com/T/#u
* Added my S-o-b tag back
Changelog v1 -> v2:
*
https://lore.kernel.org/dri-devel/20190716131005.761-1-bogdan.togor...@analog.com/
* added Fixes: tag
* added Reviewed-by: tag for Nuno
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index ddceafa7b637..8d6c93296503 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -786,8 +786,13 @@ static void adv7511_mode_set(struct adv7511 *adv7511,
> else
> low_refresh_rate = ADV7511_LOW_REFRESH_RATE_NONE;
>
> - regmap_update_bits(adv7511->regmap, 0xfb,
> - 0x6, low_refresh_rate << 1);
> + if (adv7511->type == ADV7511)
> + regmap_update_bits(adv7511->regmap, 0xfb,
> + 0x6, low_refresh_rate << 1);
> + else
> + regmap_update_bits(adv7511->regmap, 0x4a,
> + 0xc, low_refresh_rate << 2);
> +
> regmap_update_bits(adv7511->regmap, 0x17,
> 0x60, (vsync_polarity << 6) | (hsync_polarity << 5));
>
> --
> 2.41.0
>
[PATCH] drm/i915: fix application of sizeof to pointer
The coccinelle check report: ./drivers/scsi/csiostor/csio_mb.c:1554:46-52: ERROR: application of sizeof to pointer Signed-off-by: Ran Sun --- drivers/gpu/drm/i915/i915_syncmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_syncmap.c b/drivers/gpu/drm/i915/i915_syncmap.c index 60404dbb2e9f..4eeaf12be72d 100644 --- a/drivers/gpu/drm/i915/i915_syncmap.c +++ b/drivers/gpu/drm/i915/i915_syncmap.c @@ -282,7 +282,7 @@ static noinline int __sync_set(struct i915_syncmap **root, u64 id, u32 seqno) unsigned int above; /* Insert a join above the current layer */ - next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(next), + next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(*next), GFP_KERNEL); if (unlikely(!next)) return -ENOMEM;
Re: [PATCH v3 02/15] arm64: dts: qcom: sm6125: Sort spmi_bus node numerically by reg
Il 19/07/23 23:54, Marijn Suijten ha scritto:
On 2023-07-19 01:02:56, Dmitry Baryshkov wrote:
On 19/07/2023 00:24, Marijn Suijten wrote:
This node has always resided in the wrong spot, making it somewhat
harder to contribute new node entries while maintaining proper sorting
around it. Move the node up to sit after hsusb_phy1 where it maintains
proper numerical sorting on the (first of its many) reg address
property.
Fixes: cff4bbaf2a2d ("arm64: dts: qcom: Add support for SM6125")
Reviewed-by: Konrad Dybcio
Signed-off-by: Marijn Suijten
---
arch/arm64/boot/dts/qcom/sm6125.dtsi | 38
++--
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi
b/arch/arm64/boot/dts/qcom/sm6125.dtsi
index 6937c7ebdb81..cfd0901d4555 100644
--- a/arch/arm64/boot/dts/qcom/sm6125.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi
@@ -684,6 +684,24 @@ hsusb_phy1: phy@1613000 {
status = "disabled";
};
+ spmi_bus: spmi@1c4 {
+ compatible = "qcom,spmi-pmic-arb";
+ reg = <0x01c4 0x1100>,
+ <0x01e0 0x200>,
+ <0x03e0 0x10>,
+ <0x03f0 0xa>,
+ <0x01c0a000 0x26000>;
+ reg-names = "core", "chnls", "obsrvr", "intr", "cnfg";
+ interrupt-names = "periph_irq";
+ interrupts = ;
+ qcom,ee = <0>;
+ qcom,channel = <0>;
+ #address-cells = <2>;
+ #size-cells = <0>;
+ interrupt-controller;
+ #interrupt-cells = <4>;
+ };
+
rpm_msg_ram: sram@45f {
compatible = "qcom,rpm-msg-ram";
reg = <0x045f 0x7000>;
@@ -1189,27 +1207,9 @@ sram@469 {
reg = <0x0469 0x1>;
};
- spmi_bus: spmi@1c4 {
- compatible = "qcom,spmi-pmic-arb";
- reg = <0x01c4 0x1100>,
- <0x01e0 0x200>,
- <0x03e0 0x10>,
- <0x03f0 0xa>,
- <0x01c0a000 0x26000>;
- reg-names = "core", "chnls", "obsrvr", "intr", "cnfg";
- interrupt-names = "periph_irq";
- interrupts = ;
- qcom,ee = <0>;
- qcom,channel = <0>;
- #address-cells = <2>;
- #size-cells = <0>;
- interrupt-controller;
- #interrupt-cells = <4>;
- };
-
apps_smmu: iommu@c60 {
compatible = "qcom,sm6125-smmu-500", "qcom,smmu-500",
"arm,mmu-500";
- reg = <0xc60 0x8>;
+ reg = <0x0c60 0x8>;
Irrelevant, please split.
This was already here in v1, and it is what likely contributed to the
sorting mismatch in the first place.
But will split it and send a v4 for just this...
I agree in that it is irrelevant, but anyway, for the next time: you should at
least mention "the other change" in your commit message ;-)
Also, remember that this commit has a Fixes tag :-)
Cheers,
Angelo
RE: [RFC v1 1/3] mm/mmu_notifier: Add a new notifier for mapping updates (new pages)
Hi Alistair,
>
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 64a3239b6407..1f2f0209101a 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6096,8 +6096,12 @@ vm_fault_t hugetlb_fault(struct mm_struct
> *mm, struct vm_area_struct *vma,
> > * hugetlb_no_page will drop vma lock and hugetlb fault
> > * mutex internally, which make us return immediately.
> > */
> > - return hugetlb_no_page(mm, vma, mapping, idx, address,
> ptep,
> > + ret = hugetlb_no_page(mm, vma, mapping, idx, address,
> ptep,
> > entry, flags);
> > + if (!ret)
> > + mmu_notifier_update_mapping(vma->vm_mm,
> address,
> > + pte_pfn(*ptep));
>
> The next patch ends up calling pfn_to_page() on the result of
> pte_pfn(*ptep). I don't think that's safe because couldn't the PTE have
> already changed and/or the new page have been freed?
Yeah, that might be possible; I believe the right thing to do would be:
- return hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
+ ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
entry, flags);
+ if (!ret) {
+ ptl = huge_pte_lock(h, mm, ptep);
+ mmu_notifier_update_mapping(vma->vm_mm, address,
+pte_pfn(*ptep));
+ spin_unlock(ptl);
+ }
In which case I'd need to make a similar change in the shmem path as well.
And, also redo (or eliminate) the locking in udmabuf (patch) which seems a
bit excessive on a second look given our use-case (where reads and writes do
not happen simultaneously due to fence synchronization in the guest driver).
Thanks,
Vivek
>
> > + return ret;
> >
> > ret = 0;
> >
> > @@ -6223,6 +6227,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm,
> struct vm_area_struct *vma,
> > */
> > if (need_wait_lock)
> > folio_wait_locked(folio);
> > + if (!ret)
> > + mmu_notifier_update_mapping(vma->vm_mm, address,
> > + pte_pfn(*ptep));
> > return ret;
> > }
> >
> > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> > index 50c0dde1354f..6421405334b9 100644
> > --- a/mm/mmu_notifier.c
> > +++ b/mm/mmu_notifier.c
> > @@ -441,6 +441,23 @@ void __mmu_notifier_change_pte(struct
> mm_struct *mm, unsigned long address,
> > srcu_read_unlock(&srcu, id);
> > }
> >
> > +void __mmu_notifier_update_mapping(struct mm_struct *mm, unsigned
> long address,
> > + unsigned long pfn)
> > +{
> > + struct mmu_notifier *subscription;
> > + int id;
> > +
> > + id = srcu_read_lock(&srcu);
> > + hlist_for_each_entry_rcu(subscription,
> > +&mm->notifier_subscriptions->list, hlist,
> > +srcu_read_lock_held(&srcu)) {
> > + if (subscription->ops->update_mapping)
> > + subscription->ops->update_mapping(subscription,
> mm,
> > + address, pfn);
> > + }
> > + srcu_read_unlock(&srcu, id);
> > +}
> > +
> > static int mn_itree_invalidate(struct mmu_notifier_subscriptions
> *subscriptions,
> >const struct mmu_notifier_range *range)
> > {
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 2f2e0e618072..e59eb5fafadb 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -77,6 +77,7 @@ static struct vfsmount *shm_mnt;
> > #include
> > #include
> > #include
> > +#include
> > #include
> >
> > #include
> > @@ -2164,8 +2165,12 @@ static vm_fault_t shmem_fault(struct vm_fault
> *vmf)
> > gfp, vma, vmf, &ret);
> > if (err)
> > return vmf_error(err);
> > - if (folio)
> > + if (folio) {
> > vmf->page = folio_file_page(folio, vmf->pgoff);
> > + if (ret == VM_FAULT_LOCKED)
> > + mmu_notifier_update_mapping(vma->vm_mm, vmf-
> >address,
> > + page_to_pfn(vmf->page));
> > + }
> > return ret;
> > }
Re: [PATCH v4] drm: adv7511: Fix low refresh rate register for ADV7533/5
On 19.07.23 08:01, Alexandru Ardelean wrote:
> From: Bogdan Togorean
>
> For ADV7533 and ADV7535 low refresh rate is selected using
> bits [3:2] of 0x4a main register.
> So depending on ADV model write 0xfb or 0x4a register.
>
> Fixes: 2437e7cd88e8 ("drm/bridge: adv7533: Initial support for ADV7533")
> Reviewed-by: Robert Foss
> Reviewed-by: Nuno Sa
> Signed-off-by: Bogdan Togorean
> Signed-off-by: Alexandru Ardelean
Reviewed-by: Frieder Schrempf
Re: [PATCH v2] drm/mediatek: fix uninitialized symbol
[PULL] drm-misc-fixes
Hi, Here's this week drm-misc-fixes PR Maxime drm-misc-fixes-2023-07-20: Memory leak fixes in drm/client, memory access/leak fixes for accel/qaic, another leak fix in dma-buf and three nouveau fixes around hotplugging. The following changes since commit 835a65f51790e1f72b1ab106ec89db9ac15b47d6: drm/nouveau: bring back blit subchannel for pre nv50 GPUs (2023-07-12 22:38:41 +0200) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2023-07-20 for you to fetch changes up to ea293f823a8805735d9e00124df81a8f448ed1ae: drm/nouveau/kms/nv50-: init hpd_irq_lock for PIOR DP (2023-07-19 11:08:47 +0200) Memory leak fixes in drm/client, memory access/leak fixes for accel/qaic, another leak fix in dma-buf and three nouveau fixes around hotplugging. Ben Skeggs (3): drm/nouveau/i2c: fix number of aux event slots drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts drm/nouveau/kms/nv50-: init hpd_irq_lock for PIOR DP Dan Carpenter (4): accel/qaic: tighten bounds checking in encode_message() accel/qaic: tighten bounds checking in decode_message() accel/qaic: Add consistent integer overflow checks accel/qaic: Fix a leak in map_user_pages() Jocelyn Falempe (2): drm/client: Fix memory leak in drm_client_target_cloned drm/client: Fix memory leak in drm_client_modeset_probe Ville Syrjälä (1): dma-buf/dma-resv: Stop leaking on krealloc() failure drivers/accel/qaic/qaic_control.c | 39 +++ drivers/dma-buf/dma-resv.c| 13 +--- drivers/gpu/drm/drm_client_modeset.c | 6 drivers/gpu/drm/nouveau/dispnv50/disp.c | 4 +++ drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h | 4 +-- drivers/gpu/drm/nouveau/nvkm/engine/disp/uconn.c | 27 ++-- drivers/gpu/drm/nouveau/nvkm/subdev/i2c/base.c| 11 +-- 7 files changed, 73 insertions(+), 31 deletions(-) signature.asc Description: PGP signature
Re: [PATCH 00/11] drm: kunit: Switch to kunit actions
Hi, On Mon, Jul 17, 2023 at 11:24:13PM +0800, suijingfeng wrote: > On 2023/7/10 15:47, Maxime Ripard wrote: > > Hi, > > > > Since v6.5-rc1, kunit gained a devm/drmm-like mechanism that makes tests > > resources much easier to cleanup. > > > > This series converts the existing tests to use those new actions were > > relevant. > > Is the word 'were' here means that 'where' relevant ? Yes :) > Or it is means that it were relevant, after applied you patch it is not > relevant anymore ? > > > Let me know what you think, > > Maxime > > > > Signed-off-by: Maxime Ripard > > --- > > Maxime Ripard (11): > >drm/tests: helpers: Switch to kunit actions > >drm/tests: client-modeset: Remove call to > > drm_kunit_helper_free_device() > >drm/tests: modes: Remove call to drm_kunit_helper_free_device() > >drm/tests: probe-helper: Remove call to > > drm_kunit_helper_free_device() > >drm/tests: helpers: Create an helper to allocate a locking ctx > >drm/tests: helpers: Create an helper to allocate an atomic state > > a helper or an helper ? > > Should this two patch be re-titled as following ? > > I search it on the internet[1], mostly using a helper. > > > drm/tests: helpers: Create a helper to allocate a locking ctx > drm/tests: helpers: Create a helper to allocate an atomic state > > [1] https://www.a-or-an.com/a_an/helper > > Sorry about the noise if I'm wrong. You're right, I'll fix it Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH v2,2/2] drm/mediatek: dp: Add the audio control to mtk_dp_data struct
On Thu, 2023-07-06 at 12:26 +0200, AngeloGioacchino Del Regno wrote:
External email : Please do not click links or open attachments until you have
verified the sender or the content.
Il 06/07/23 04:14, Shuijing Li ha scritto:
> Mainly add the following two flag:
Hi Angelo
This patch is for MT8188 hw design change, MT8195 has not changed, so MT8195
does not need modify.
Best Regards,
Shuijing
> 1.The audio packet arrangement function is to only arrange audio
> packets into the Hblanking area. In order to align with the HW
> default setting of g1200, this function needs to be turned off.
>
> 2.Due to the difference of HW, different dividers need to be set.
>
> Signed-off-by: Shuijing Li
> Signed-off-by: Jitao Shi
> ---
> Changes in v2:
> - change the variables' name to be more descriptive
> - add a comment that describes the function of mtk_dp_audio_sample_arrange
> - reduce indentation by doing the inverse check
> - add a definition of some bits
> - add support for mediatek, mt8188-edp-tx
> per suggestion from the previous thread:
> https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/
> ---
> drivers/gpu/drm/mediatek/mtk_dp.c | 47 ++-
> drivers/gpu/drm/mediatek/mtk_dp_reg.h | 6
> 2 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
> b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 64eee77452c0..8e1a13ab2ba2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -139,6 +139,8 @@ struct mtk_dp_data {
> unsigned int smc_cmd;
> const struct mtk_dp_efuse_fmt *efuse_fmt;
> bool audio_supported;
> +bool audio_pkt_in_hblank_area;
> +u16 audio_m_div2_bit;
> };
>
> static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] =
> {
> @@ -647,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct
> mtk_dp *mtk_dp,
> static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
> {
> mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
> - AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> + mtk_dp->data->audio_m_div2_bit,
> AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
> }
>
> @@ -1362,6 +1364,18 @@ static void
> mtk_dp_sdp_set_down_cnt_init_in_hblank(struct mtk_dp *mtk_dp)
> SDP_DOWN_CNT_INIT_IN_HBLANK_DP_ENC1_P0_MASK);
> }
>
> +static void mtk_dp_audio_sample_arrange(struct mtk_dp *mtk_dp)
> +{
> +/* arrange audio packets into the Hblanking and Vblanking area */
> +if (!mtk_dp->data->audio_pkt_in_hblank_area)
> +return;
There's only one remaining question: why is this done for MT8188 but *not* for
MT8195?
Thanks,
Angelo
> +
> +mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3374, 0,
> + SDP_ASP_INSERT_IN_HBLANK_DP_ENC1_P0_MASK);
> +mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3374, 0,
> + SDP_DOWN_ASP_CNT_INIT_DP_ENC1_P0_MASK);
> +}
> +
> static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
> {
> u32 sram_read_start = min_t(u32, MTK_DP_TBC_BUF_READ_START_ADDR,
> @@ -1371,6 +1385,7 @@ static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
> MTK_DP_PIX_PER_ADDR);
> mtk_dp_set_sram_read_start(mtk_dp, sram_read_start);
> mtk_dp_setup_encoder(mtk_dp);
> +mtk_dp_audio_sample_arrange(mtk_dp);
> mtk_dp_sdp_set_down_cnt_init_in_hblank(mtk_dp);
> mtk_dp_sdp_set_down_cnt_init(mtk_dp, sram_read_start);
> }
> @@ -2616,11 +2631,31 @@ static int mtk_dp_resume(struct device *dev)
>
> static SIMPLE_DEV_PM_OPS(mtk_dp_pm_ops, mtk_dp_suspend, mtk_dp_resume);
>
> +static const struct mtk_dp_data mt8188_edp_data = {
> +.bridge_type = DRM_MODE_CONNECTOR_eDP,
> +.smc_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE,
> +.efuse_fmt = mt8195_edp_efuse_fmt,
> +.audio_supported = false,
> +.audio_pkt_in_hblank_area = false,
> +.audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> +};
> +
> +static const struct mtk_dp_data mt8188_dp_data = {
> +.bridge_type = DRM_MODE_CONNECTOR_DisplayPort,
> +.smc_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE,
> +.efuse_fmt = mt8195_dp_efuse_fmt,
> +.audio_supported = true,
> +.audio_pkt_in_hblank_area = true,
> +.audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> +};
> +
> static const struct mtk_dp_data mt8195_edp_data = {
> .bridge_type = DRM_MODE_CONNECTOR_eDP,
> .smc_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE,
> .efuse_fmt = mt8195_edp_efuse_fmt,
> .audio_supported = false,
> +.audio_pkt_in_hblank_area = false,
> +.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> };
>
> static const struct mtk_dp_data mt8195_dp_data = {
> @@ -2628,9 +2663,19 @@ static const struct mtk_dp_data mt8195_dp_data = {
> .smc_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE,
> .efuse_fmt = mt8195_dp_efuse_fmt,
> .audio_supported = true,
> +.audio_pkt_in_hblank_area = false,
> +.audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
> };
>
> static const
[PULL] drm-intel-fixes
Hi Dave, Daniel, Only two fixes for the 6.5 rc window this week - one perf/OA use after free on Xe_HP platforms and one defconfig build fix for GCC versions older than 8. Regards, Tvrtko drm-intel-fixes-2023-07-20: - Add sentinel to xehp_oa_b_counters [perf] (Andrzej Hajda) - Revert "drm/i915: use localized __diag_ignore_all() instead of per file" (Jani Nikula) The following changes since commit fdf0eaf11452d72945af31804e2a1048ee1b574c: Linux 6.5-rc2 (2023-07-16 15:10:37 -0700) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-fixes-2023-07-20 for you to fetch changes up to 2c27770a7bc88ef7f6614d11d96d8e62017d0b78: Revert "drm/i915: use localized __diag_ignore_all() instead of per file" (2023-07-17 13:39:04 +0100) - Add sentinel to xehp_oa_b_counters [perf] (Andrzej Hajda) - Revert "drm/i915: use localized __diag_ignore_all() instead of per file" (Jani Nikula) Andrzej Hajda (1): drm/i915/perf: add sentinel to xehp_oa_b_counters Jani Nikula (1): Revert "drm/i915: use localized __diag_ignore_all() instead of per file" drivers/gpu/drm/i915/Makefile | 5 + drivers/gpu/drm/i915/display/intel_display_device.c | 5 - drivers/gpu/drm/i915/display/intel_fbdev.c | 5 - drivers/gpu/drm/i915/i915_pci.c | 5 - drivers/gpu/drm/i915/i915_perf.c| 1 + 5 files changed, 6 insertions(+), 15 deletions(-)
Re: [PATCH v2, 2/2] drm/mediatek: dp: Add the audio control to mtk_dp_data struct
[PATCH v3,0/3] Add compatible to increase MT8188 audio control
Add dt-binding documentation of dp-tx for MediaTek MT8188 SoC. Mainly add the following two flag: 1.The audio packet arrangement function is to only arrange audio packets into the Hblanking area. In order to align with the HW default setting of g1200, this function needs to be turned off. 2.Due to the difference of HW, different dividers need to be set. Base on the branch of linus/master v6.4. Shuijing Li (3): dt-bindings: display: mediatek: dp: Add compatible for MediaTek MT8188 drm/mediatek: dp: Add the audio control to mtk_dp_data struct drm/mediatek: dp: Add the audio divider to mtk_dp_data struct .../display/mediatek/mediatek,dp.yaml | 2 + drivers/gpu/drm/mediatek/mtk_dp.c | 47 ++- drivers/gpu/drm/mediatek/mtk_dp_reg.h | 6 +++ 3 files changed, 54 insertions(+), 1 deletion(-) -- 2.40.1
[PATCH v3, 1/3] dt-bindings: display: mediatek: dp: Add compatible for MediaTek MT8188
Add dt-binding documentation of dp-tx for MediaTek MT8188 SoC. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi Reviewed-by: AngeloGioacchino Del Regno Acked-by: Krzysztof Kozlowski --- Changes in v2: add a mediatek,mt8188-edp-tx compatible per suggestion from the previous thread: https://lore.kernel.org/lkml/c4a4a900-c80d-b110-f10e-7fa2dae8b...@collabora.com/ --- .../devicetree/bindings/display/mediatek/mediatek,dp.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml index ff781f2174a0..2aef1eb32e11 100644 --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml @@ -21,6 +21,8 @@ description: | properties: compatible: enum: + - mediatek,mt8188-dp-tx + - mediatek,mt8188-edp-tx - mediatek,mt8195-dp-tx - mediatek,mt8195-edp-tx -- 2.40.1
[PATCH v3, 3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
Due to the difference of HW, different dividers need to be set.
Signed-off-by: Shuijing Li
Signed-off-by: Jitao Shi
---
Changes in v3:
Separate these two things into two different patches.
per suggestion from the previous thread:
https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/
Changes in v2:
- change the variables' name to be more descriptive
- add a comment that describes the function of mtk_dp_audio_sample_arrange
- reduce indentation by doing the inverse check
- add a definition of some bits
- add support for mediatek, mt8188-edp-tx
per suggestion from the previous thread:
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/
---
drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++-
drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
b/drivers/gpu/drm/mediatek/mtk_dp.c
index d8cda83d6fef..8e1a13ab2ba2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -140,6 +140,7 @@ struct mtk_dp_data {
const struct mtk_dp_efuse_fmt *efuse_fmt;
bool audio_supported;
bool audio_pkt_in_hblank_area;
+ u16 audio_m_div2_bit;
};
static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
@@ -648,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp
*mtk_dp,
static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
{
mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
- AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+ mtk_dp->data->audio_m_div2_bit,
AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
}
@@ -2636,6 +2637,7 @@ static const struct mtk_dp_data mt8188_edp_data = {
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8188_dp_data = {
@@ -2644,6 +2646,7 @@ static const struct mtk_dp_data mt8188_dp_data = {
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
.audio_pkt_in_hblank_area = true,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_edp_data = {
@@ -2652,6 +2655,7 @@ static const struct mtk_dp_data mt8195_edp_data = {
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_dp_data = {
@@ -2660,6 +2664,7 @@ static const struct mtk_dp_data mt8195_dp_data = {
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct of_device_id mtk_dp_of_match[] = {
diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
index f38d6ff12afe..6d7f0405867e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
@@ -162,6 +162,7 @@
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 (1 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 (2 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 (3 << 8)
+#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (5 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 (6 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 (7 << 8)
--
2.40.1
[PATCH v3, 2/3] drm/mediatek: dp: Add the audio packet flag to mtk_dp_data struct
The audio packet arrangement function is to only arrange audio.
packets into the Hblanking area. In order to align with the HW
default setting of mt8195, this function needs to be turned off.
Signed-off-by: Shuijing Li
Signed-off-by: Jitao Shi
---
Changes in v3:
Separate these two things into two different patches.
per suggestion from the previous thread:
https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/
Changes in v2:
- change the variables' name to be more descriptive
- add a comment that describes the function of mtk_dp_audio_sample_arrange
- reduce indentation by doing the inverse check
- add a definition of some bits
- add support for mediatek, mt8188-edp-tx
per suggestion from the previous thread:
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/
---
drivers/gpu/drm/mediatek/mtk_dp.c | 40 +++
drivers/gpu/drm/mediatek/mtk_dp_reg.h | 5
2 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
b/drivers/gpu/drm/mediatek/mtk_dp.c
index 64eee77452c0..d8cda83d6fef 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -139,6 +139,7 @@ struct mtk_dp_data {
unsigned int smc_cmd;
const struct mtk_dp_efuse_fmt *efuse_fmt;
bool audio_supported;
+ bool audio_pkt_in_hblank_area;
};
static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
@@ -1362,6 +1363,18 @@ static void
mtk_dp_sdp_set_down_cnt_init_in_hblank(struct mtk_dp *mtk_dp)
SDP_DOWN_CNT_INIT_IN_HBLANK_DP_ENC1_P0_MASK);
}
+static void mtk_dp_audio_sample_arrange(struct mtk_dp *mtk_dp)
+{
+ /* arrange audio packets into the Hblanking and Vblanking area */
+ if (!mtk_dp->data->audio_pkt_in_hblank_area)
+ return;
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3374, 0,
+ SDP_ASP_INSERT_IN_HBLANK_DP_ENC1_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3374, 0,
+ SDP_DOWN_ASP_CNT_INIT_DP_ENC1_P0_MASK);
+}
+
static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
{
u32 sram_read_start = min_t(u32, MTK_DP_TBC_BUF_READ_START_ADDR,
@@ -1371,6 +1384,7 @@ static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
MTK_DP_PIX_PER_ADDR);
mtk_dp_set_sram_read_start(mtk_dp, sram_read_start);
mtk_dp_setup_encoder(mtk_dp);
+ mtk_dp_audio_sample_arrange(mtk_dp);
mtk_dp_sdp_set_down_cnt_init_in_hblank(mtk_dp);
mtk_dp_sdp_set_down_cnt_init(mtk_dp, sram_read_start);
}
@@ -2616,11 +2630,28 @@ static int mtk_dp_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(mtk_dp_pm_ops, mtk_dp_suspend, mtk_dp_resume);
+static const struct mtk_dp_data mt8188_edp_data = {
+ .bridge_type = DRM_MODE_CONNECTOR_eDP,
+ .smc_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE,
+ .efuse_fmt = mt8195_edp_efuse_fmt,
+ .audio_supported = false,
+ .audio_pkt_in_hblank_area = false,
+};
+
+static const struct mtk_dp_data mt8188_dp_data = {
+ .bridge_type = DRM_MODE_CONNECTOR_DisplayPort,
+ .smc_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE,
+ .efuse_fmt = mt8195_dp_efuse_fmt,
+ .audio_supported = true,
+ .audio_pkt_in_hblank_area = true,
+};
+
static const struct mtk_dp_data mt8195_edp_data = {
.bridge_type = DRM_MODE_CONNECTOR_eDP,
.smc_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE,
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
+ .audio_pkt_in_hblank_area = false,
};
static const struct mtk_dp_data mt8195_dp_data = {
@@ -2628,9 +2659,18 @@ static const struct mtk_dp_data mt8195_dp_data = {
.smc_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE,
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
+ .audio_pkt_in_hblank_area = false,
};
static const struct of_device_id mtk_dp_of_match[] = {
+ {
+ .compatible = "mediatek,mt8188-edp-tx",
+ .data = &mt8188_edp_data,
+ },
+ {
+ .compatible = "mediatek,mt8188-dp-tx",
+ .data = &mt8188_dp_data,
+ },
{
.compatible = "mediatek,mt8195-edp-tx",
.data = &mt8195_edp_data,
diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
index 84e38cef03c2..f38d6ff12afe 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
@@ -228,6 +228,11 @@
VIDEO_STABLE_CNT_THRD_DP_ENC1_P0 | \
SDP_DP13_EN_DP_ENC1_P0
| \
BS2BS_MODE_DP_ENC1_P0)
+
+#define MTK_DP_ENC1_P0_33740x3374
+#define SDP_ASP_INSERT_IN_HBLANK_DP_ENC1_P0_MASK BIT(12)
+#define SDP_DOWN_ASP_CNT_INIT_DP_ENC1_P0
Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()
On 2023/7/19 23:25, Paul Moore wrote:
On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang wrote:
On 2023/7/19 17:02, Christian Göttsche wrote:
On Wed, 19 Jul 2023 at 09:40, Kefeng Wang wrote:
Use the helpers to simplify code.
Cc: Paul Moore
Cc: Stephen Smalley
Cc: Eric Paris
Acked-by: Paul Moore
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 d06e350fedee..ee8575540a8e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3762,13 +3762,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_initial_heap(vma)) {
This seems to change the condition from
vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk
to
vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk
(or AND arguments swapped)
vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk
Is this intended?
The new condition is to check whether there is intersection between
[startbrk,brk] and [vm_start,vm_end], it contains orignal check, so
I think it is ok, but for selinux check, I am not sure if there is
some other problem.
This particular SELinux vma check is see if the vma falls within the
heap; can you confirm that this change preserves this?
Yes, within is one case of new vma scope check.
Re: [PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
On 18/07/2023 22:44, Nathan Chancellor wrote:
A proposed update to clang's -Wconstant-logical-operand to warn when the
left hand side is a constant shows the following instance in
nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ,
such as CONFIG_HZ=300:
drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical
'&&' with constant operand [-Wconstant-logical-operand]
189 | if (NSEC_PER_SEC % HZ &&
| ~ ^
drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise
operation
189 | if (NSEC_PER_SEC % HZ &&
| ^~
| &
drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to
silence this warning
1 warning generated.
Turn this into an explicit comparison against zero to make the
expression a boolean to make it clear this should be a logical check,
not a bitwise one.
So -Wconstant-logical-operand only triggers when it is a constant but
not zero constant? Why does that make sense is not a kludge to avoid too
much noise?
Personally, it all feels a bit over the top as a warning, since code in
both cases should optimise away. And we may end up papering over it if
it becomes a default.
Then again this patch IMO does make the code more readable, so I am
happy to take this one via our tree. Or either give ack to bring it in
via drm-misc-next:
Acked-by: Tvrtko Ursulin
Let me know which route works best.
Regards,
Tvrtko
Link: https://reviews.llvm.org/D142609
Signed-off-by: Nathan Chancellor
---
drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 4a33ad2d122b..d4b918fb11ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj,
static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
{
/* nsecs_to_jiffies64() does not guard against overflow */
- if (NSEC_PER_SEC % HZ &&
+ if ((NSEC_PER_SEC % HZ) != 0 &&
div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
return MAX_JIFFY_OFFSET;
Re: [PATCH v5 9/9] drm: Introduce documentation for hotspot properties
Simon Ser writes: Hello Simon, > On Thursday, July 20th, 2023 at 07:03, Zack Rusin wrote: > >> I'll give this series a few more hours on the list and if no one objects >> I'll push >> it to drm-misc later today. Thanks! > > Sorry, but this doesn't seem to be enough to satisfy the DRM merge > requirements. This introduces a new uAPI but is missing user-space > patches and IGT. See [1] and [2]. > > [1]: https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#requirements > [2]: > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > Albert (Cc'ed) wrote IGT tests for this new uAPI and was waiting for Zack's patches to land to post them. I believe his branch is [0] but he can correct me if I'm wrong on that. Zack also has mutter patches and Albert has been testing those too. [0]: https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/commits/modeset-cursor-hotspot-test/ -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 01/11] drm/tests: helpers: Switch to kunit actions
Hi Javier,
Thanks for reviewing the series
On Wed, Jul 19, 2023 at 08:42:51PM +0200, Javier Martinez Canillas wrote:
> > diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > index 4df47071dc88..38211fea9ae6 100644
> > --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> > @@ -35,8 +35,8 @@ static struct platform_driver fake_platform_driver = {
> > * able to leverage the usual infrastructure and most notably the
> > * device-managed resources just like a "real" device.
> > *
> > - * Callers need to make sure drm_kunit_helper_free_device() on the
> > - * device when done.
> > + * Resources will be cleaned up automatically, but the removal can be
> > + * forced using @drm_kunit_helper_free_device.
> > *
> > * Returns:
> > * A pointer to the new device, or an ERR_PTR() otherwise.
> > @@ -49,12 +49,31 @@ struct device *drm_kunit_helper_alloc_device(struct
> > kunit *test)
> > ret = platform_driver_register(&fake_platform_driver);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > + ret = kunit_add_action_or_reset(test,
> > + (kunit_action_t
> > *)platform_driver_unregister,
> > + &fake_platform_driver);
> > + KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> >
> > + ret = kunit_add_action_or_reset(test,
> > + (kunit_action_t *)platform_device_put,
> > + pdev);
> > + KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > ret = platform_device_add(pdev);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > + kunit_remove_action(test,
> > + (kunit_action_t *)platform_device_put,
> > + pdev);
> > +
>
> I understand that this action removal is because platform_device_put() is
> not needed anymore after the platform_device_unregister() remove action is
> registered since that already takes care of the platform_device_put().
It's not so much that it's not needed after platform_device_unregister,
but rather that once you've called paltform_device_add my understanding
was that you didn't need it anymore.
I can't find where I got that from though, so I might be wrong. It also
looks like I could just use platform_device_del directly and not cancel
platform_device_put which will make it more obvious.
> But maybe add a comment to make more clear for someone who is not familiar
> with these details of the platform core ?
>
> > EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
> > @@ -70,8 +89,13 @@ void drm_kunit_helper_free_device(struct kunit *test,
> > struct device *dev)
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> >
> > - platform_device_unregister(pdev);
> > - platform_driver_unregister(&fake_platform_driver);
> > + kunit_release_action(test,
> > +(kunit_action_t *)platform_device_unregister,
> > +pdev);
> > +
> > + kunit_release_action(test,
> > +(kunit_action_t *)platform_driver_unregister,
> > +&fake_platform_driver);
> > }
> > EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
> >
>
> I thought the point of using the kunit cleanup actions was that you could
> just make the kunit framework handle the release of resources and not do
> it manually?
You're right
> Can you just remove this function helper or is still needed in some cases?
We still need it for the drmm execution test where we would force the
removal of the device. All other tests at the moment shouldn't call it.
Maxime
signature.asc
Description: PGP signature
Re: [RFC v1 1/3] mm/mmu_notifier: Add a new notifier for mapping updates (new pages)
"Kasireddy, Vivek" writes:
> Hi Alistair,
>
>>
>> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> > index 64a3239b6407..1f2f0209101a 100644
>> > --- a/mm/hugetlb.c
>> > +++ b/mm/hugetlb.c
>> > @@ -6096,8 +6096,12 @@ vm_fault_t hugetlb_fault(struct mm_struct
>> *mm, struct vm_area_struct *vma,
>> > * hugetlb_no_page will drop vma lock and hugetlb fault
>> > * mutex internally, which make us return immediately.
>> > */
>> > - return hugetlb_no_page(mm, vma, mapping, idx, address,
>> ptep,
>> > + ret = hugetlb_no_page(mm, vma, mapping, idx, address,
>> ptep,
>> > entry, flags);
>> > + if (!ret)
>> > + mmu_notifier_update_mapping(vma->vm_mm,
>> address,
>> > + pte_pfn(*ptep));
>>
>> The next patch ends up calling pfn_to_page() on the result of
>> pte_pfn(*ptep). I don't think that's safe because couldn't the PTE have
>> already changed and/or the new page have been freed?
> Yeah, that might be possible; I believe the right thing to do would be:
> - return hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
> + ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
> entry, flags);
> + if (!ret) {
> + ptl = huge_pte_lock(h, mm, ptep);
> + mmu_notifier_update_mapping(vma->vm_mm, address,
> +pte_pfn(*ptep));
> + spin_unlock(ptl);
> + }
Yes, although obviously as I think you point out below you wouldn't be
able to take any sleeping locks in mmu_notifier_update_mapping().
> In which case I'd need to make a similar change in the shmem path as well.
> And, also redo (or eliminate) the locking in udmabuf (patch) which seems a
> bit excessive on a second look given our use-case (where reads and writes do
> not happen simultaneously due to fence synchronization in the guest driver).
I'm not at all familiar with the udmabuf use case but that sounds
brittle and effectively makes this notifier udmabuf specific right?
The name gives the impression it is more general though. I have
contemplated adding a notifier for PTE updates for drivers using
hmm_range_fault() as it would save some expensive device faults and it
this could be useful for that.
So if we're adding a notifier for PTE updates I think it would be good
if it covered all cases and was robust enough to allow mirroring of the
correct PTE value (ie. by being called under PTL or via some other
synchronisation like hmm_range_fault()).
Thanks.
> Thanks,
> Vivek
>
>>
>> > + return ret;
>> >
>> >ret = 0;
>> >
>> > @@ -6223,6 +6227,9 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm,
>> struct vm_area_struct *vma,
>> > */
>> >if (need_wait_lock)
>> >folio_wait_locked(folio);
>> > + if (!ret)
>> > + mmu_notifier_update_mapping(vma->vm_mm, address,
>> > + pte_pfn(*ptep));
>> >return ret;
>> > }
>> >
>> > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
>> > index 50c0dde1354f..6421405334b9 100644
>> > --- a/mm/mmu_notifier.c
>> > +++ b/mm/mmu_notifier.c
>> > @@ -441,6 +441,23 @@ void __mmu_notifier_change_pte(struct
>> mm_struct *mm, unsigned long address,
>> >srcu_read_unlock(&srcu, id);
>> > }
>> >
>> > +void __mmu_notifier_update_mapping(struct mm_struct *mm, unsigned
>> long address,
>> > + unsigned long pfn)
>> > +{
>> > + struct mmu_notifier *subscription;
>> > + int id;
>> > +
>> > + id = srcu_read_lock(&srcu);
>> > + hlist_for_each_entry_rcu(subscription,
>> > + &mm->notifier_subscriptions->list, hlist,
>> > + srcu_read_lock_held(&srcu)) {
>> > + if (subscription->ops->update_mapping)
>> > + subscription->ops->update_mapping(subscription,
>> mm,
>> > +address, pfn);
>> > + }
>> > + srcu_read_unlock(&srcu, id);
>> > +}
>> > +
>> > static int mn_itree_invalidate(struct mmu_notifier_subscriptions
>> *subscriptions,
>> > const struct mmu_notifier_range *range)
>> > {
>> > diff --git a/mm/shmem.c b/mm/shmem.c
>> > index 2f2e0e618072..e59eb5fafadb 100644
>> > --- a/mm/shmem.c
>> > +++ b/mm/shmem.c
>> > @@ -77,6 +77,7 @@ static struct vfsmount *shm_mnt;
>> > #include
>> > #include
>> > #include
>> > +#include
>> > #include
>> >
>> > #include
>> > @@ -2164,8 +2165,12 @@ static vm_fault_t shmem_fault(struct vm_fault
>> *vmf)
>> > gfp, vma, vmf, &ret);
>> >if (err)
>> >return vmf_error(err);
>> > - if (folio)
>> > + if (folio) {
>> >vmf->page = folio_file_page(folio, vmf->pgoff);
>> > + if (ret == VM_FAULT_LOCKED)
>> > +
Re: [PATCH v5 9/9] drm: Introduce documentation for hotspot properties
On Thursday, July 20th, 2023 at 10:50, Javier Martinez Canillas wrote: > > On Thursday, July 20th, 2023 at 07:03, Zack Rusin za...@vmware.com wrote: > > > > > I'll give this series a few more hours on the list and if no one objects > > > I'll push > > > it to drm-misc later today. Thanks! > > > > Sorry, but this doesn't seem to be enough to satisfy the DRM merge > > requirements. This introduces a new uAPI but is missing user-space > > patches and IGT. See 1 and 2. > > > Albert (Cc'ed) wrote IGT tests for this new uAPI and was waiting for > Zack's patches to land to post them. I believe his branch is [0] but > he can correct me if I'm wrong on that. > > Zack also has mutter patches and Albert has been testing those too. > > [0]: > https://gitlab.freedesktop.org/aesteve/igt-gpu-tools/-/commits/modeset-cursor-hotspot-test/ Ah, nice. Please do post all of these (without merging them) and include links to them in the commit message. Posting is important to make sure there are no gaps/mistakes in the tests and user-space impl.
Re: [PATCH 06/19] drm/i915/display: Account for DSC not split case while computing cdclk
On Thu, Jul 13, 2023 at 04:03:33PM +0530, Ankit Nautiyal wrote:
> Currently we assume 2 Pixels Per Clock (PPC) while computing
> plane cdclk and min_cdlck. In cases where DSC single engine
> is used the throughput is 1 PPC.
>
> So account for the above case, while computing cdclk.
>
> v2: Use helper to get the adjusted pixel rate.
>
> Signed-off-by: Ankit Nautiyal
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 2 +-
> drivers/gpu/drm/i915/display/intel_vdsc.c | 12
> drivers/gpu/drm/i915/display/intel_vdsc.h | 2 ++
> drivers/gpu/drm/i915/display/skl_universal_plane.c | 4 ++--
> 4 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index dcc1f6941b60..701909966545 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2508,7 +2508,7 @@ static int intel_pixel_rate_to_cdclk(const struct
> intel_crtc_state *crtc_state)
> int pixel_rate = crtc_state->pixel_rate;
>
> if (DISPLAY_VER(dev_priv) >= 10)
> - return DIV_ROUND_UP(pixel_rate, 2);
> + return intel_dsc_get_adjusted_pixel_rate(crtc_state,
> pixel_rate);
> else if (DISPLAY_VER(dev_priv) == 9 ||
>IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> return pixel_rate;
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 9d76c2756784..bbfdbf06da68 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -1038,3 +1038,15 @@ void intel_dsc_get_config(struct intel_crtc_state
> *crtc_state)
> out:
> intel_display_power_put(dev_priv, power_domain, wakeref);
> }
> +
> +int intel_dsc_get_adjusted_pixel_rate(const struct intel_crtc_state
> *crtc_state, int pixel_rate)
> +{
> + /*
> + * If single VDSC engine is used, it uses one pixel per clock
> + * otherwise we use two pixels per clock.
> + */
> + if (crtc_state->dsc.compression_enable && !crtc_state->dsc.dsc_split)
> + return pixel_rate;
> +
> + return DIV_ROUND_UP(pixel_rate, 2);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.h
> b/drivers/gpu/drm/i915/display/intel_vdsc.h
> index 2cc41ff08909..3bb4b1980b6b 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.h
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h
> @@ -28,4 +28,6 @@ void intel_dsc_dsi_pps_write(struct intel_encoder *encoder,
> void intel_dsc_dp_pps_write(struct intel_encoder *encoder,
> const struct intel_crtc_state *crtc_state);
>
> +int intel_dsc_get_adjusted_pixel_rate(const struct intel_crtc_state
> *crtc_state, int pixel_rate);
> +
> #endif /* __INTEL_VDSC_H__ */
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 6b01a0b68b97..9eeb25ec4be9 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -17,6 +17,7 @@
> #include "intel_fb.h"
> #include "intel_fbc.h"
> #include "intel_psr.h"
> +#include "intel_vdsc.h"
> #include "skl_scaler.h"
> #include "skl_universal_plane.h"
> #include "skl_watermark.h"
> @@ -263,8 +264,7 @@ static int icl_plane_min_cdclk(const struct
> intel_crtc_state *crtc_state,
> {
> unsigned int pixel_rate = intel_plane_pixel_rate(crtc_state,
> plane_state);
>
> - /* two pixels per clock */
> - return DIV_ROUND_UP(pixel_rate, 2);
> + return intel_dsc_get_adjusted_pixel_rate(crtc_state, pixel_rate);
Hi Ankit,
I think the thing what you are taking of is already handled here in
intel_cdclk.c:
/*
* When we decide to use only one VDSC engine, since
* each VDSC operates with 1 ppc throughput, pixel clock
* cannot be higher than the VDSC clock (cdclk)
* If there 2 VDSC engines, then pixel clock can't be higher than
* VDSC clock(cdclk) * 2 and so on.
*/
if (crtc_state->dsc.compression_enable) {
int num_vdsc_instances =
intel_dsc_get_num_vdsc_instances(crtc_state);
min_cdclk = max_t(int, min_cdclk,
DIV_ROUND_UP(crtc_state->pixel_rate,
num_vdsc_instances));
}
Also even if something still have to be done here, I think we should preferrably
deal with anything related to DSC in a single place, to prevent any kind of
confusion(when those checks are scattered in different places, it is way more
easy to forget/not notice something)
I think intel_pixel_rate_to_cdclk isn't supposed to know anything about DSC or
any other specifics like audio checks and etc - it is
just dealing with the "default" uncompressed case.
Any other additional limitations or checks we apply after those,
Re: [PATCH v3 0/9] PCI/VGA: Improve the default VGA device selection
Hi, On 2023/7/20 03:32, Bjorn Helgaas wrote: drm/amdgpu: Implement the is_primary_gpu callback of vga_client_register() drm/radeon: Add an implement for the is_primary_gpu function callback drm/i915: Add an implement for the is_primary_gpu hook drm/ast: Register as a vga client to vgaarb by calling vga_client_register() drm/loongson: Add an implement for the is_primary_gpu function callback There's unnecessary variation in the subject lines (and the commit logs) of these patches. If they all do the same thing but in different drivers, it's useful if the patches all*look* the same. You might be able to write these subjects as "Implement .is_primary_gpu() callback" for brevity. This is a very good suggestion, I will adopt this. Thanks, really.
Re: [PATCH v5 2/9] drm/atomic: Add support for mouse hotspots
On Wednesday, July 19th, 2023 at 03:42, Zack Rusin wrote:
> From: Zack Rusin
>
> Atomic modesetting code lacked support for specifying mouse cursor
> hotspots. The legacy kms DRM_IOCTL_MODE_CURSOR2 had support for setting
> the hotspot but the functionality was not implemented in the new atomic
> paths.
>
> Due to the lack of hotspots in the atomic paths userspace compositors
> completely disable atomic modesetting for drivers that require it (i.e.
> all paravirtualized drivers).
>
> This change adds hotspot properties to the atomic codepaths throughtout
> the DRM core and will allow enabling atomic modesetting for virtualized
> drivers in the userspace.
>
> Signed-off-by: Zack Rusin
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Reviewed-by: Javier Martinez Canillas
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 14 +++
> drivers/gpu/drm/drm_atomic_uapi.c | 20 +
> drivers/gpu/drm/drm_plane.c | 50 +++
> include/drm/drm_plane.h | 14 +++
> 4 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 784e63d70a42..54975de44a0e 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -275,6 +275,20 @@ void __drm_atomic_helper_plane_state_reset(struct
> drm_plane_state *plane_state,
> plane_state->normalized_zpos = val;
> }
> }
> +
> + if (plane->hotspot_x_property) {
> + if (!drm_object_property_get_default_value(&plane->base,
> +
> plane->hotspot_x_property,
> +&val))
> + plane_state->hotspot_x = val;
> + }
> +
> + if (plane->hotspot_y_property) {
> + if (!drm_object_property_get_default_value(&plane->base,
> +
> plane->hotspot_y_property,
> +&val))
> + plane_state->hotspot_y = val;
> + }
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_state_reset);
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 98d3b10c08ae..07a7b3f18df2 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -593,6 +593,22 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
> } else if (plane->funcs->atomic_set_property) {
> return plane->funcs->atomic_set_property(plane, state,
> property, val);
> + } else if (property == plane->hotspot_x_property) {
> + if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> + drm_dbg_atomic(plane->dev,
> +"[PLANE:%d:%s] is not a cursor plane:
> 0x%llx\n",
> +plane->base.id, plane->name, val);
> + return -EINVAL;
> + }
Hm, it sounds a bit weird to catch this case here. Wouldn't it be a driver bug
to attach the hotspot props to a plane which isn't a cursor? Wouldn't it make
more sense to WARN in drm_plane_create_hotspot_properties() if a driver
attempts to do so?
> + state->hotspot_x = val;
> + } else if (property == plane->hotspot_y_property) {
> + if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> + drm_dbg_atomic(plane->dev,
> +"[PLANE:%d:%s] is not a cursor plane:
> 0x%llx\n",
> +plane->base.id, plane->name, val);
> + return -EINVAL;
> + }
> + state->hotspot_y = val;
> } else {
> drm_dbg_atomic(plane->dev,
> "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
> @@ -653,6 +669,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> *val = state->scaling_filter;
> } else if (plane->funcs->atomic_get_property) {
> return plane->funcs->atomic_get_property(plane, state,
> property, val);
> + } else if (property == plane->hotspot_x_property) {
> + *val = state->hotspot_x;
> + } else if (property == plane->hotspot_y_property) {
> + *val = state->hotspot_y;
> } else {
> drm_dbg_atomic(dev,
> "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index c6bbb0c209f4..eaca367bdc7e 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -230,6 +230,47 @@ static int create_in_format_blob(struct drm_device *dev,
> struct drm_plane *plane
> return 0;
> }
>
> +/**
Re: [PATCH 07/19] drm/i915/intel_cdclk: Add vdsc with bigjoiner constraints on min_cdlck
On Thu, Jul 13, 2023 at 04:03:34PM +0530, Ankit Nautiyal wrote:
> As per Bsepc:49259, Bigjoiner BW check puts restriction on the
> compressed bpp for a given CDCLK, pixelclock in cases where
> Bigjoiner + DSC are used.
>
> Currently compressed bpp is computed first, and it is ensured that
> the bpp will work at least with the max CDCLK freq.
>
> Since the CDCLK is computed later, lets account for Bigjoiner BW
> check while calculating Min CDCLK.
>
> v2: Use pixel clock in the bw calculations. (Ville)
>
> Signed-off-by: Ankit Nautiyal
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 61 +-
> 1 file changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 701909966545..788dba576294 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2533,6 +2533,51 @@ static int intel_planes_min_cdclk(const struct
> intel_crtc_state *crtc_state)
> return min_cdclk;
> }
>
> +static int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + int num_vdsc_instances = intel_dsc_get_num_vdsc_instances(crtc_state);
> + int min_cdclk = 0;
> +
> + /*
> + * When we decide to use only one VDSC engine, since
> + * each VDSC operates with 1 ppc throughput, pixel clock
> + * cannot be higher than the VDSC clock (cdclk)
> + * If there 2 VDSC engines, then pixel clock can't be higher than
> + * VDSC clock(cdclk) * 2 and so on.
> + */
> + min_cdclk = max_t(int, min_cdclk,
> + DIV_ROUND_UP(crtc_state->pixel_rate,
> num_vdsc_instances));
> +
> + if (crtc_state->bigjoiner_pipes) {
> + int pixel_clock = crtc_state->hw.adjusted_mode.clock;
> +
> + /*
> + * According to Bigjoiner bw check:
> + * compressed_bpp <= PPC * CDCLK * Big joiner Interface bits /
> Pixel clock
> + *
> + * We have already computed compressed_bpp, so now compute the
> min CDCLK that
> + * is required to support this compressed_bpp.
> + *
> + * => CDCLK >= compressed_bpp * Pixel clock / (PPC * Bigjoiner
> Interface bits)
> + *
> + * Since PPC = 2 with bigjoiner
> + * => CDCLK >= compressed_bpp * Pixel clock / 2 * Bigjoiner
> Interface bits
> + *
> + * #TODO Bspec mentions to account for FEC overhead while using
> pixel clock.
> + * Check if we need to use FEC overhead in the above
> calculations.
There is already some function used in intel_dp.c:
intel_dp_mode_to_fec_clock(mode_clock) => Should you may be just use that one,
to account FEC
overhead?
> + */
> + int bigjoiner_interface_bits = DISPLAY_VER(i915) > 13 ? 36 : 24;
> + int min_cdclk_bj = (crtc_state->dsc.compressed_bpp *
> pixel_clock) /
> +(2 * bigjoiner_interface_bits);
I would use "num_vdsc_instances" instead of 2, since we even get those
explicitly.
> +
> + min_cdclk = max(min_cdclk, min_cdclk_bj);
> + }
> +
> + return min_cdclk;
> +}
> +
> int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> {
> struct drm_i915_private *dev_priv =
> @@ -2604,20 +2649,8 @@ int intel_crtc_compute_min_cdclk(const struct
> intel_crtc_state *crtc_state)
> /* Account for additional needs from the planes */
> min_cdclk = max(intel_planes_min_cdclk(crtc_state), min_cdclk);
>
> - /*
> - * When we decide to use only one VDSC engine, since
> - * each VDSC operates with 1 ppc throughput, pixel clock
> - * cannot be higher than the VDSC clock (cdclk)
> - * If there 2 VDSC engines, then pixel clock can't be higher than
> - * VDSC clock(cdclk) * 2 and so on.
> - */
> - if (crtc_state->dsc.compression_enable) {
> - int num_vdsc_instances =
> intel_dsc_get_num_vdsc_instances(crtc_state);
> -
> - min_cdclk = max_t(int, min_cdclk,
> - DIV_ROUND_UP(crtc_state->pixel_rate,
> -num_vdsc_instances));
> - }
> + if (crtc_state->dsc.compression_enable)
> + min_cdclk = max(min_cdclk, intel_vdsc_min_cdclk(crtc_state));
With notes above taken care of:
Reviewed-by: Stanislav Lisovskiy
>
> /*
>* HACK. Currently for TGL/DG2 platforms we calculate
> --
> 2.40.1
>
Re: [PATCH v3,2/3] drm/mediatek: dp: Add the audio packet flag to mtk_dp_data struct
Il 20/07/23 10:26, Shuijing Li ha scritto: The audio packet arrangement function is to only arrange audio. packets into the Hblanking area. In order to align with the HW default setting of mt8195, this function needs to be turned off. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
Il 20/07/23 10:26, Shuijing Li ha scritto: Due to the difference of HW, different dividers need to be set. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH 05/19] drm/i915/dp: Update Bigjoiner interface bits for computing compressed bpp
On Thu, Jul 13, 2023 at 04:03:32PM +0530, Ankit Nautiyal wrote:
> In Bigjoiner check for DSC, bigjoiner interface bits for DP for
> DISPLAY > 13 is 36 (Bspec: 49259).
>
> v2: Corrected Display ver to 13.
>
> v3: Follow convention for conditional statement. (Ville)
>
> v4: Fix check for display ver. (Ville)
>
> Signed-off-by: Ankit Nautiyal
> Reviewed-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 19768ac658ba..c1fd448d80e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -802,8 +802,9 @@ u16 intel_dp_dsc_get_max_compressed_bpp(struct
> drm_i915_private *i915,
> bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
>
> if (bigjoiner) {
> + int bigjoiner_interface_bits = DISPLAY_VER(i915) >= 14 ? 36 :
> 24;
> u32 max_bpp_bigjoiner =
> - i915->display.cdclk.max_cdclk_freq * 48 /
> + i915->display.cdclk.max_cdclk_freq * 2 *
> bigjoiner_interface_bits /
Probably "num_vdsc_instances = intel_dsc_get_num_vdsc_instances(crtc_state);"
again,
instead of "2"?
With that clarified,
Reviewed-by: Stanislav Lisovskiy
> intel_dp_mode_to_fec_clock(mode_clock);
>
> bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
> --
> 2.40.1
>
Re: [PATCH 13/19] drm/i915/dp: Avoid left shift of DSC output bpp by 4
On Thu, Jul 13, 2023 at 04:03:40PM +0530, Ankit Nautiyal wrote: > To make way for fractional bpp support, avoid left shifting the > output_bpp by 4 in helper intel_dp_dsc_get_output_bpp. > > Signed-off-by: Ankit Nautiyal > --- > drivers/gpu/drm/i915/display/intel_dp.c | 10 +++--- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +- > 2 files changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 9d2d05da594b..a7d58eb914c6 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -812,11 +812,7 @@ u16 intel_dp_dsc_get_max_compressed_bpp(struct > drm_i915_private *i915, > > bits_per_pixel = intel_dp_dsc_nearest_valid_bpp(i915, bits_per_pixel, > pipe_bpp); > > - /* > - * Compressed BPP in U6.4 format so multiply by 16, for Gen 11, > - * fractional part is 0 > - */ > - return bits_per_pixel << 4; > + return bits_per_pixel; > } > > u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp, > @@ -1206,7 +1202,7 @@ intel_dp_mode_valid(struct drm_connector *_connector, > > mode->hdisplay, > bigjoiner, > > output_format, > - pipe_bpp, > 64) >> 4; > + pipe_bpp, > 64); > dsc_slice_count = > intel_dp_dsc_get_slice_count(intel_dp, >target_clock, > @@ -1812,7 +1808,7 @@ int intel_dp_dsc_compute_config(struct intel_dp > *intel_dp, > > pipe_config->pipe_bpp); > > pipe_config->dsc.compressed_bpp = min_t(u16, > - > dsc_max_compressed_bpp >> 4, > + > dsc_max_compressed_bpp, > output_bpp); > } > pipe_config->dsc.slice_count = dsc_dp_slice_count; > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index dff4717edbd0..4895d6242915 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c > @@ -982,7 +982,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector > *connector, > > mode->hdisplay, > bigjoiner, > > INTEL_OUTPUT_FORMAT_RGB, > - pipe_bpp, > 64) >> 4; > + pipe_bpp, > 64); Reviewed-by: Stanislav Lisovskiy > dsc_slice_count = > intel_dp_dsc_get_slice_count(intel_dp, >target_clock, > -- > 2.40.1 >
Re: [PATCH v3,2/3] drm/mediatek: dp: Add the audio packet flag to mtk_dp_data struct
Reviewed-by: Alexandre Mergnat On 20/07/2023 10:26, Shuijing Li wrote: The audio packet arrangement function is to only arrange audio. packets into the Hblanking area. In order to align with the HW default setting of mt8195, this function needs to be turned off. -- Regards, Alexandre
[PATCH v3] drm/i915: Fix premature release of request's reusable memory
Infinite waits for completion of GPU activity have been observed in CI,
mostly inside __i915_active_wait(), triggered by igt@gem_barrier_race or
igt@perf@stress-open-close. Root cause analysis, based of ftrace dumps
generated with a lot of extra trace_printk() calls added to the code,
revealed loops of request dependencies being accidentally built,
preventing the requests from being processed, each waiting for completion
of another one's activity.
After we substitute a new request for a last active one tracked on a
timeline, we set up a dependency of our new request to wait on completion
of current activity of that previous one. While doing that, we must take
care of keeping the old request still in memory until we use its
attributes for setting up that await dependency, or we can happen to set
up the await dependency on an unrelated request that already reuses the
memory previously allocated to the old one, already released. Combined
with perf adding consecutive kernel context remote requests to different
user context timelines, unresolvable loops of await dependencies can be
built, leading do infinite waits.
We obtain a pointer to the previous request to wait upon when we
substitute it with a pointer to our new request in an active tracker,
e.g. in intel_timeline.last_request. In some processing paths we protect
that old request from being freed before we use it by getting a reference
to it under RCU protection, but in others, e.g. __i915_request_commit()
-> __i915_request_add_to_timeline() -> __i915_request_ensure_ordering(),
we don't. But anyway, since the requests' memory is SLAB_FAILSAFE_BY_RCU,
that RCU protection is not sufficient against reuse of memory.
We could protect i915_request's memory from being prematurely reused by
calling its release function via call_rcu() and using rcu_read_lock()
consequently, as proposed in v1. However, that approach leads to
significant (up to 10 times) increase of SLAB utilization by i915_request
SLAB cache. Another potential approach is to take a reference to the
previous active fence.
When updating an active fence tracker, we first lock the new fence,
substitute a pointer of the current active fence with the new one, then we
lock the substituted fence. With this approach, there is a time window
after the substitution and before the lock when the request can be
concurrently released by an interrupt handler and its memory reused, then
we may happen to lock and return a new, unrelated request.
Always get a reference to the current active fence first, before
replacing it with a new one. Having it protected from premature release
and reuse, lock it and then replace with the new one but only if not
yet signalled via a potential concurrent interrupt nor replaced with
another one by a potential concurrent thread, otherwise retry, starting
from getting a reference to the new current one. Adjust users to not
get a reference to the previous active fence themselves and always put the
reference got by __i915_active_fence_set() when no longer needed.
v3: Fix lockdep splat reports and other issues caused by incorrect use of
try_cmpxchg() (use (cmpxchg() != prev) instead)
v2: Protect request's memory by getting a reference to it in favor of
delegating its release to call_rcu() (Chris)
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8211
Fixes: df9f85d8582e ("drm/i915: Serialise i915_active_fence_set() with itself")
Suggested-by: Chris Wilson
Signed-off-by: Janusz Krzysztofik
Cc: # v5.6+
---
drivers/gpu/drm/i915/i915_active.c | 99 -
drivers/gpu/drm/i915/i915_request.c | 11
2 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_active.c
b/drivers/gpu/drm/i915/i915_active.c
index 8ef93889061a6..5ec293011d990 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -449,8 +449,11 @@ int i915_active_add_request(struct i915_active *ref,
struct i915_request *rq)
}
} while (unlikely(is_barrier(active)));
- if (!__i915_active_fence_set(active, fence))
+ fence = __i915_active_fence_set(active, fence);
+ if (!fence)
__i915_active_acquire(ref);
+ else
+ dma_fence_put(fence);
out:
i915_active_release(ref);
@@ -469,13 +472,9 @@ __i915_active_set_fence(struct i915_active *ref,
return NULL;
}
- rcu_read_lock();
prev = __i915_active_fence_set(active, fence);
- if (prev)
- prev = dma_fence_get_rcu(prev);
- else
+ if (!prev)
__i915_active_acquire(ref);
- rcu_read_unlock();
return prev;
}
@@ -1019,10 +1018,11 @@ void i915_request_add_active_barriers(struct
i915_request *rq)
*
* Records the new @fence as the last active fence along its timeline in
* this active tracker, moving the tracking callbacks from the previous
- * fence onto this one. Returns the
Re: [PATCH] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
On Wednesday, July 19th, 2023 at 19:05, Erik Kurzinger
wrote:
> These new ioctls perform a task similar to
> DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
> IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
> timeline point to import or export the fence to or from on a timeline
> syncobj.
>
> This eliminates the need to use a temporary binary syncobj along with
> DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
> technique userspace has had to employ up to this point. While that does
> work, it is rather awkward from the programmer's perspective. Since DRM
> syncobjs have been proposed as the basis for display server explicit
> synchronization protocols, e.g. [1] and [2], providing a more
> streamlined interface now seems worthwhile.
This looks like a good idea to me! The patch looks good as well, apart
from one tricky issue, see below...
> [1]
> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
> [2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
>
> Accompanying userspace patches...
> IGT:
> https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/241e7f379aeaa9b22a32277e77ad4011c8717a57
> libdrm:
> https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/b3961a592fc6f8b05f7e3a12413fb58eca2dbfa2
(Unfortunately this isn't enough when it comes to user-space patches: the
kernel rules require a "real" user of the new IOCTL, not just a libdr IOCTL
wrapper. I will post a patch to make use of this from wlroots if that helps.)
> Signed-off-by: Erik Kurzinger
> ---
> drivers/gpu/drm/drm_internal.h | 4 +++
> drivers/gpu/drm/drm_ioctl.c| 4 +++
> drivers/gpu/drm/drm_syncobj.c | 60 ++
> include/uapi/drm/drm.h | 9 +
> 4 files changed, 71 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index d7e023bbb0d5..64a28ed26a16 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -253,6 +253,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device
> *dev, void *data,
> struct drm_file *file_private);
> int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private);
> +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file_private);
> +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file_private);
>
> /* drm_framebuffer.c */
> void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7c9d66ee917d..0344e8e447bc 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -710,6 +710,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
> DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE,
> drm_syncobj_import_sync_file_ioctl,
> + DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE,
> drm_syncobj_export_sync_file_ioctl,
> + DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl,
> 0),
> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
> drm_crtc_queue_sequence_ioctl, 0),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl,
> DRM_MASTER),
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 0c2be8360525..bf0c1eae353a 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -181,6 +181,13 @@
> * Note that if you want to transfer a struct &dma_fence_chain from a given
> * point on a timeline syncobj from/into a binary syncobj, you can use the
> * point 0 to mean take/replace the fence in the syncobj.
> + *
> + * &DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and
> &DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
> + * let the client import or export the struct &dma_fence_chain of a syncobj
> + * at a particular timeline point from or to a sync file.
> + * These behave similarly to &DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
> + * and &DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above,
> except
> + * that they accommodate timeline syncobjs in addition to binary syncobjs.
> */
>
> #include
> @@ -682,10 +689,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file
> *file_private,
> }
>
> static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> - int fd, int handle)
> + int fd, u64 point, int handle)
Nit: can we spe
Re: [PATCH v5 1/9] drm: Disable the cursor plane on atomic contexts with virtualized drivers
On Wednesday, July 19th, 2023 at 03:42, Zack Rusin wrote:
> From: Zack Rusin
>
> Cursor planes on virtualized drivers have special meaning and require
> that the clients handle them in specific ways, e.g. the cursor plane
> should react to the mouse movement the way a mouse cursor would be
> expected to and the client is required to set hotspot properties on it
> in order for the mouse events to be routed correctly.
>
> This breaks the contract as specified by the "universal planes". Fix it
> by disabling the cursor planes on virtualized drivers while adding
> a foundation on top of which it's possible to special case mouse cursor
> planes for clients that want it.
>
> Disabling the cursor planes makes some kms compositors which were broken,
> e.g. Weston, fallback to software cursor which works fine or at least
> better than currently while having no effect on others, e.g. gnome-shell
> or kwin, which put virtualized drivers on a deny-list when running in
> atomic context to make them fallback to legacy kms and avoid this issue.
>
> Signed-off-by: Zack Rusin
> Fixes: 681e7ec73044 ("drm: Allow userspace to ask for universal plane list
> (v2)")
> Cc: # v5.4+
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Dave Airlie
> Cc: Gerd Hoffmann
> Cc: Hans de Goede
> Cc: Gurchetan Singh
> Cc: Chia-I Wu
> Cc: dri-devel@lists.freedesktop.org
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: spice-de...@lists.freedesktop.org
> Acked-by: Pekka Paalanen
> Reviewed-by: Javier Martinez Canillas
Nit: I think it would be better to reflect the name of the DRM client cap in
the supports_virtualized_cursor_plane field.
Regardless:
Reviewed-by: Simon Ser
Re: [PATCH v5 9/9] drm: Introduce documentation for hotspot properties
On Wednesday, July 19th, 2023 at 03:42, Zack Rusin wrote:
> +/**
> + * DOC: hotspot properties
> + *
> + * HOTSPOT_X: property to set mouse hotspot x offset.
> + * HOTSPOT_Y: property to set mouse hotspot y offset.
> + *
> + * When the plane is being used as a cursor image to display a mouse pointer,
> + * the "hotspot" is the offset within the cursor image where mouse events
> + * are expected to go.
> + *
> + * Positive values move the hotspot from the top-left corner of the cursor
> + * plane towards the right and bottom.
> + *
> + * Most display drivers do not need this information because the
> + * hotspot is not actually connected to anything visible on screen.
> + * However, this is necessary for display drivers like the para-virtualized
> + * drivers (eg qxl, vbox, virtio, vmwgfx), that are attached to a user
> console
> + * with a mouse pointer. Since these consoles are often being remoted over a
> + * network, they would otherwise have to wait to display the pointer
> movement to
> + * the user until a full network round-trip has occurred. New mouse events
> have
> + * to be sent from the user's console, over the network to the virtual input
> + * devices, forwarded to the desktop for processing, and then the cursor
> plane's
> + * position can be updated and sent back to the user's console over the
> network.
> + * Instead, with the hotspot information, the console can anticipate the new
> + * location, and draw the mouse cursor there before the confirmation comes
> in.
> + * To do that correctly, the user's console must be able predict how the
> + * desktop will process mouse events, which normally requires the desktop's
> + * mouse topology information, ie where each CRTC sits in the mouse
> coordinate
> + * space. This is typically sent to the para-virtualized drivers using some
> + * driver-specific method, and the driver then forwards it to the console by
> + * way of the virtual display device or hypervisor.
> + *
> + * The assumption is generally made that there is only one cursor plane being
> + * used this way at a time, and that the desktop is feeding all mouse devices
> + * into the same global pointer. Para-virtualized drivers that require this
> + * should only be exposing a single cursor plane, or find some other way
> + * to coordinate with a userspace desktop that supports multiple pointers.
> + * If the hotspot properties are set, the cursor plane is therefore assumed
> to be
> + * used only for displaying a mouse cursor image, and the position of the
> combined
> + * cursor plane + offset can therefore be used for coordinating with input
> from a
> + * mouse device.
> + *
> + * The cursor will then be drawn either at the location of the plane in the
> CRTC
> + * console, or as a free-floating cursor plane on the user's console
> + * corresponding to their desktop mouse position.
> + *
> + * DRM clients which would like to work correctly on drivers which expose
> + * hotspot properties should advertise DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT.
Nit: an ampersand ("&") can be used to linkify that cap.
> + * Setting this property on drivers which do not special case
> + * cursor planes will return EOPNOTSUPP, which can be used by userspace to
> + * gauge requirements of the hardware/drivers they're running on. Advertising
> + * DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT implies that the userspace client
> will be
> + * correctly setting the hotspot properties.
Thanks a lot for writing these docs! It's super helpful!
Reviewed-by: Simon Ser
Re: [PATCH v5 8/9] drm: Introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT
On Wednesday, July 19th, 2023 at 03:42, Zack Rusin wrote: > From: Zack Rusin > > Virtualized drivers place additional restrictions on the cursor plane > which breaks the contract of universal planes. To allow atomic > modesettings with virtualized drivers the clients need to advertise > that they're capable of dealing with those extra restrictions. > > To do that introduce DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT which > lets DRM know that the client is aware of and capable of dealing with > the extra restrictions on the virtual cursor plane. > > Setting this option to true makes DRM expose the cursor plane on > virtualized drivers. The userspace is expected to set the hotspots > and handle mouse events on that plane. > > Signed-off-by: Zack Rusin > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: dri-devel@lists.freedesktop.org > Acked-by: Pekka Paalanen > Reviewed-by: Javier Martinez Canillas Looks good! Reviewed-by: Simon Ser
Re: [PATCH 4/9] drm/verisilicon: Add gem driver for JH7110 SoC
On 2023/6/19 21:18, Thomas Zimmermann wrote:
>
>
> Am 02.06.23 um 09:40 schrieb Keith Zhao:
>> This patch implements gem related APIs for JH7100 SoC.
>>
>> Signed-off-by: Keith Zhao
>> ---
>> drivers/gpu/drm/verisilicon/Makefile | 3 +-
>> drivers/gpu/drm/verisilicon/vs_drv.c | 6 +
>> drivers/gpu/drm/verisilicon/vs_gem.c | 372 +++
>> drivers/gpu/drm/verisilicon/vs_gem.h | 72 ++
>> 4 files changed, 452 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/verisilicon/vs_gem.c
>> create mode 100644 drivers/gpu/drm/verisilicon/vs_gem.h
>>
>> diff --git a/drivers/gpu/drm/verisilicon/Makefile
>> b/drivers/gpu/drm/verisilicon/Makefile
>> index 64ce1b26546c..30360e370e47 100644
>> --- a/drivers/gpu/drm/verisilicon/Makefile
>> +++ b/drivers/gpu/drm/verisilicon/Makefile
>> @@ -1,6 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0
>> -vs_drm-objs := vs_drv.o
>> +vs_drm-objs := vs_drv.o \
>> + vs_gem.o
>> obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o
>> diff --git a/drivers/gpu/drm/verisilicon/vs_drv.c
>> b/drivers/gpu/drm/verisilicon/vs_drv.c
>> index 24d333598477..e0a2fc43b55f 100644
>> --- a/drivers/gpu/drm/verisilicon/vs_drv.c
>> +++ b/drivers/gpu/drm/verisilicon/vs_drv.c
>> @@ -30,6 +30,7 @@
>> #include
>> #include "vs_drv.h"
>> +#include "vs_gem.h"
>> #define DRV_NAME "starfive"
>> #define DRV_DESC "Starfive DRM driver"
>> @@ -47,6 +48,7 @@ static const struct file_operations fops = {
>> .compat_ioctl = drm_compat_ioctl,
>> .poll = drm_poll,
>> .read = drm_read,
>> + .mmap = vs_gem_mmap,
>> };
>> static struct drm_driver vs_drm_driver = {
>> @@ -54,6 +56,10 @@ static struct drm_driver vs_drm_driver = {
>> .lastclose = drm_fb_helper_lastclose,
>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> + .gem_prime_import = vs_gem_prime_import,
>> + .gem_prime_import_sg_table = vs_gem_prime_import_sg_table,
>> + .gem_prime_mmap = vs_gem_prime_mmap,
>> + .dumb_create = vs_gem_dumb_create,
>> .fops = &fops,
>> .name = DRV_NAME,
>> .desc = DRV_DESC,
>> diff --git a/drivers/gpu/drm/verisilicon/vs_gem.c
>> b/drivers/gpu/drm/verisilicon/vs_gem.c
>> new file mode 100644
>> index ..3f963471c1ab
>> --- /dev/null
>> +++ b/drivers/gpu/drm/verisilicon/vs_gem.c
>> @@ -0,0 +1,372 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#include "vs_drv.h"
>> +#include "vs_gem.h"
>> +
>> +static const struct drm_gem_object_funcs vs_gem_default_funcs;
>> +
>> +static int vs_gem_alloc_buf(struct vs_gem_object *vs_obj)
>> +{
>> + struct drm_device *dev = vs_obj->base.dev;
>> + unsigned int nr_pages;
>> + struct sg_table sgt;
>> + int ret = -ENOMEM;
>> +
>> + if (vs_obj->dma_addr) {
>> + DRM_DEV_DEBUG_KMS(dev->dev, "already allocated.\n");
>> + return 0;
>> + }
>> +
>> + vs_obj->dma_attrs = DMA_ATTR_WRITE_COMBINE | DMA_ATTR_FORCE_CONTIGUOUS
>> + | DMA_ATTR_NO_KERNEL_MAPPING;
>> +
>> + nr_pages = vs_obj->size >> PAGE_SHIFT;
>> +
>> + vs_obj->pages = kvmalloc_array(nr_pages, sizeof(struct page *),
>> + GFP_KERNEL | __GFP_ZERO);
>> + if (!vs_obj->pages) {
>> + DRM_DEV_ERROR(dev->dev, "failed to allocate pages.\n");
>> + return -ENOMEM;
>> + }
>> +
>> + vs_obj->cookie = dma_alloc_attrs(to_dma_dev(dev), vs_obj->size,
>> + &vs_obj->dma_addr, GFP_KERNEL,
>> + vs_obj->dma_attrs);
>> +
>> + if (!vs_obj->cookie) {
>> + DRM_DEV_ERROR(dev->dev, "failed to allocate buffer.\n");
>> + goto err_free;
>> + }
>> +
>> + vs_obj->iova = vs_obj->dma_addr;
>> +
>> + ret = dma_get_sgtable_attrs(to_dma_dev(dev), &sgt,
>> + vs_obj->cookie, vs_obj->dma_addr,
>> + vs_obj->size, vs_obj->dma_attrs);
>> + if (ret < 0) {
>> + DRM_DEV_ERROR(dev->dev, "failed to get sgtable.\n");
>> + goto err_mem_free;
>> + }
>> +
>> + if (drm_prime_sg_to_page_array(&sgt, vs_obj->pages, nr_pages)) {
>> + DRM_DEV_ERROR(dev->dev, "invalid sgtable.\n");
>> + ret = -EINVAL;
>> + goto err_sgt_free;
>> + }
>> +
>> + sg_free_table(&sgt);
>> +
>> + return 0;
>> +
>> +err_sgt_free:
>> + sg_free_table(&sgt);
>> +err_mem_free:
>> + dma_free_attrs(to_dma_dev(dev), vs_obj->size, vs_obj->cookie,
>> + vs_obj->dma_addr, vs_obj->dma_attrs);
>> +err_free:
>> + kvfree(vs_obj->pages);
>> +
>> + return ret;
>> +}
>> +
>> +static void vs_gem_free_buf(struct vs_gem_object *vs_obj)
>> +{
>> + struct drm_device *dev = vs_obj->base.dev;
>> +
>> + if (!vs_obj->dma_addr) {
>> +
Re: [PATCH v3] drm/ssd130x: Fix an oops when attempting to update a disabled plane
Hi,
On Mon, Jul 17, 2023 at 06:30:22PM +0200, Javier Martinez Canillas wrote:
> Geert reports that the following NULL pointer dereference happens for him
> after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each
> plane update"):
>
> [drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
> ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1)
> and format(R1 little-endian (0x20203152))
> Unable to handle kernel NULL pointer dereference at virtual address
>
> Oops [#1]
> CPU: 0 PID: 1 Comm: swapper Not tainted
> 6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565
> epc : ssd130x_update_rect.isra.0+0x13c/0x340
> ra : ssd130x_update_rect.isra.0+0x2bc/0x340
> ...
> status: 0120 badaddr: cause: 000f
> [] ssd130x_update_rect.isra.0+0x13c/0x340
> [] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> [] drm_atomic_helper_commit_planes+0xfc/0x27c
> [] drm_atomic_helper_commit_tail+0x5c/0xb4
> [] commit_tail+0x190/0x1b8
> [] drm_atomic_helper_commit+0x194/0x1c0
> [] drm_atomic_commit+0xa4/0xe4
> [] drm_client_modeset_commit_atomic+0x244/0x278
> [] drm_client_modeset_commit_locked+0x7c/0x1bc
> [] drm_client_modeset_commit+0x34/0x64
> [] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8
> [] drm_fb_helper_set_par+0x38/0x58
> [] fbcon_init+0x294/0x534
> ...
>
> The problem is that fbcon calls fbcon_init() which triggers a DRM modeset
> and this leads to drm_atomic_helper_commit_planes() attempting to commit
> the atomic state for all planes, even the ones whose CRTC is not enabled.
>
> Since the primary plane buffer is allocated in the encoder .atomic_enable
> callback, this happens after that initial modeset commit and leads to the
> mentioned NULL pointer dereference.
I think that's where the problem lies: you must not allocate a buffer in
atomic_enable.
After drm_atomic_helper_swap_state(), the new commit is being applied
and you aren't allowed to fail, and an allocation can fail.
Everything needs to be prepared by the time _swap_state is called, and
it's one of the point of atomic_check.
So you need to allocate your buffer there, and use it in whatever
atomic_commit related hook you need it in.
The typical way of doing this would be to create a custom state
structure that embeds the global one, create your own reset,
atomic_duplicate_state and atomic_destroy_state implementations, and
store the buffer pointer there.
Maxime
signature.asc
Description: PGP signature
Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx
On Wed, Jul 19, 2023 at 09:12:14PM +0200, Javier Martinez Canillas wrote:
> suijingfeng writes:
>
> > Hi,
> >
> > On 2023/7/10 15:47, Maxime Ripard wrote:
>
> [...]
>
> >> +
> >> +/**
> >> + * drm_kunit_helper_context_alloc - Allocates an acquire context
> >> + * @test: The test context object
> >> + *
> >> + * Allocates and initializes a modeset acquire context.
> >> + *
> >> + * The context is tied to the kunit test context, so we must not call
> >> + * drm_modeset_acquire_fini() on it, it will be done so automatically.
> >> + *
> >> + * Returns:
> >> + * An ERR_PTR on error, a pointer to the newly allocated context otherwise
> >> + */
> >> +struct drm_modeset_acquire_ctx *
> >> +drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
> >> +{
> >> + struct drm_modeset_acquire_ctx *ctx;
> >> + int ret;
> >> +
> >> + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> >> + KUNIT_ASSERT_NOT_NULL(test, ctx);
> >> +
> >> + drm_modeset_acquire_init(ctx, 0);
> >> +
> >> + ret = kunit_add_action_or_reset(test,
> >> + action_drm_release_context,
> >> + ctx);
> >> + if (ret)
> >> + return ERR_PTR(ret);
> >> +
> >> + return ctx;
> >> +}
> >> +EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
> >> +
> >
> > I think all of the patch inside this series are quite well.
> >
> > Personally, I can't find problems in it.
> >
> >
> > But I still want to ask a question:
> >
> > Should the managed functions you introduced be prefixed with drmm_
> > (instead of drm_) ?
> >
>
> That's a good question. But personally I think that the drmm_ prefix
> should be reserved for drm_device managed resources and helpers.
Yeah, to me drmm functions are meant for resources that are tied to the
DRM device lifetime. These resources are tied to the test lifetime, so
they shouldn't share the same prefix.
> > As mindless programmer may still want to call drm_modeset_acquire_fini()
> > on the pointer returned by
> >
> > drm_kunit_helper_acquire_ctx_alloc()?
> >
>
> The function kernel-doc already mentions that there's no need to do that
> and that will be done automatically by kunit. So shouldn't be different of
> other functions helper where the programmer didn't read the documentation.
Right
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
On 20/07/2023 10:26, Shuijing Li wrote:
Due to the difference of HW, different dividers need to be set.
Signed-off-by: Shuijing Li
Signed-off-by: Jitao Shi
---
Changes in v3:
Separate these two things into two different patches.
per suggestion from the previous thread:
https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/
Changes in v2:
- change the variables' name to be more descriptive
- add a comment that describes the function of mtk_dp_audio_sample_arrange
- reduce indentation by doing the inverse check
- add a definition of some bits
- add support for mediatek, mt8188-edp-tx
per suggestion from the previous thread:
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/
---
drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++-
drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
b/drivers/gpu/drm/mediatek/mtk_dp.c
index d8cda83d6fef..8e1a13ab2ba2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -140,6 +140,7 @@ struct mtk_dp_data {
const struct mtk_dp_efuse_fmt *efuse_fmt;
bool audio_supported;
bool audio_pkt_in_hblank_area;
+ u16 audio_m_div2_bit;
};
static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
@@ -648,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp
*mtk_dp,
static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
{
mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
- AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+ mtk_dp->data->audio_m_div2_bit,
AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
}
@@ -2636,6 +2637,7 @@ static const struct mtk_dp_data mt8188_edp_data = {
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8188_dp_data = {
@@ -2644,6 +2646,7 @@ static const struct mtk_dp_data mt8188_dp_data = {
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
.audio_pkt_in_hblank_area = true,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_edp_data = {
@@ -2652,6 +2655,7 @@ static const struct mtk_dp_data mt8195_edp_data = {
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_dp_data = {
@@ -2660,6 +2664,7 @@ static const struct mtk_dp_data mt8195_dp_data = {
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct of_device_id mtk_dp_of_match[] = {
diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
index f38d6ff12afe..6d7f0405867e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
@@ -162,6 +162,7 @@
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2(1 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4(2 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8(3 << 8)
+#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8)
IMO, it's a bit weird to have SoC specific define in the generic header.
Are you sure this bit is only available for MT8188 ?
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2(5 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4(6 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8(7 << 8)
Reviewed-by: Alexandre Mergnat
--
Regards,
Alexandre
Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx
Hi,
On 2023/7/20 18:07, Maxime Ripard wrote:
On Wed, Jul 19, 2023 at 09:12:14PM +0200, Javier Martinez Canillas wrote:
suijingfeng writes:
Hi,
On 2023/7/10 15:47, Maxime Ripard wrote:
[...]
+
+/**
+ * drm_kunit_helper_context_alloc - Allocates an acquire context
+ * @test: The test context object
+ *
+ * Allocates and initializes a modeset acquire context.
+ *
+ * The context is tied to the kunit test context, so we must not call
+ * drm_modeset_acquire_fini() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated context otherwise
+ */
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
+{
+ struct drm_modeset_acquire_ctx *ctx;
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+ drm_modeset_acquire_init(ctx, 0);
+
+ ret = kunit_add_action_or_reset(test,
+ action_drm_release_context,
+ ctx);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+
I think all of the patch inside this series are quite well.
Personally, I can't find problems in it.
But I still want to ask a question:
Should the managed functions you introduced be prefixed with drmm_
(instead of drm_) ?
That's a good question. But personally I think that the drmm_ prefix
should be reserved for drm_device managed resources and helpers.
Yeah, to me drmm functions are meant for resources that are tied to the
DRM device lifetime. These resources are tied to the test lifetime, so
they shouldn't share the same prefix.
Okay then,
Thanks for reply.
As mindless programmer may still want to call drm_modeset_acquire_fini()
on the pointer returned by
drm_kunit_helper_acquire_ctx_alloc()?
The function kernel-doc already mentions that there's no need to do that
and that will be done automatically by kunit. So shouldn't be different of
other functions helper where the programmer didn't read the documentation.
Right
Maxime
Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()
On Wed, 19 Jul 2023 at 09:40, Kefeng Wang wrote:
>
> Use the helpers to simplify code.
>
> Cc: Paul Moore
> Cc: Stephen Smalley
> Cc: Eric Paris
> Acked-by: Paul Moore
> 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 d06e350fedee..ee8575540a8e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3762,13 +3762,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_initial_heap(vma)) {
This seems to change the condition from
vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk
to
vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk
(or AND arguments swapped)
vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <= vma->vm_mm->brk
Is this intended?
> 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_initial_stack(vma) ||
> vma_is_stack_for_current(vma))) {
> rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
> PROCESS__EXECSTACK, NULL);
> --
> 2.27.0
>
[PATCH 1/2] dt-bindings: ili9881c: Add TDO TL050HDV35 LCD panel
Add support for TDO TL050HDV35-H1311A LCD panel. Signed-off-by: Matus Gajdos --- .../devicetree/bindings/display/panel/ilitek,ili9881c.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml index c5d1df680858..e7ab6224b52e 100644 --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9881c.yaml @@ -18,6 +18,7 @@ properties: - enum: - bananapi,lhr050h41 - feixin,k101-im2byl02 + - tdo,tl050hdv35 - wanchanglong,w552946aba - const: ilitek,ili9881c -- 2.25.1
[PATCH 2/2] drm/panel: ilitek-ili9881c: Add TDO TL050HDV35 LCD panel
Add support for TDO TL050HDV35-H1311A LCD panel.
Signed-off-by: Matus Gajdos
---
drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 194 ++
1 file changed, 194 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
index 1ec696adf9de..78ac57224689 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c
@@ -455,6 +455,174 @@ static const struct ili9881c_instr k101_im2byl02_init[] =
{
ILI9881C_COMMAND_INSTR(0xD3, 0x3F), /* VN0 */
};
+static const struct ili9881c_instr tl050hdv35_init[] = {
+ ILI9881C_SWITCH_PAGE_INSTR(3),
+ ILI9881C_COMMAND_INSTR(0x01, 0x00),
+ ILI9881C_COMMAND_INSTR(0x02, 0x00),
+ ILI9881C_COMMAND_INSTR(0x03, 0x73),
+ ILI9881C_COMMAND_INSTR(0x04, 0x00),
+ ILI9881C_COMMAND_INSTR(0x05, 0x00),
+ ILI9881C_COMMAND_INSTR(0x06, 0x0a),
+ ILI9881C_COMMAND_INSTR(0x07, 0x00),
+ ILI9881C_COMMAND_INSTR(0x08, 0x00),
+ ILI9881C_COMMAND_INSTR(0x09, 0x01),
+ ILI9881C_COMMAND_INSTR(0x0a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0c, 0x01),
+ ILI9881C_COMMAND_INSTR(0x0d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x0f, 0x1d),
+ ILI9881C_COMMAND_INSTR(0x10, 0x1d),
+ ILI9881C_COMMAND_INSTR(0x15, 0x00),
+ ILI9881C_COMMAND_INSTR(0x16, 0x00),
+ ILI9881C_COMMAND_INSTR(0x17, 0x00),
+ ILI9881C_COMMAND_INSTR(0x18, 0x00),
+ ILI9881C_COMMAND_INSTR(0x19, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x1e, 0x40),
+ ILI9881C_COMMAND_INSTR(0x1f, 0x80),
+ ILI9881C_COMMAND_INSTR(0x20, 0x06),
+ ILI9881C_COMMAND_INSTR(0x21, 0x02),
+ ILI9881C_COMMAND_INSTR(0x28, 0x33),
+ ILI9881C_COMMAND_INSTR(0x29, 0x03),
+ ILI9881C_COMMAND_INSTR(0x2a, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2b, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x2f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x35, 0x00),
+ ILI9881C_COMMAND_INSTR(0x36, 0x00),
+ ILI9881C_COMMAND_INSTR(0x37, 0x00),
+ ILI9881C_COMMAND_INSTR(0x38, 0x3C),
+ ILI9881C_COMMAND_INSTR(0x39, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3a, 0x40),
+ ILI9881C_COMMAND_INSTR(0x3b, 0x40),
+ ILI9881C_COMMAND_INSTR(0x3c, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3d, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3e, 0x00),
+ ILI9881C_COMMAND_INSTR(0x3f, 0x00),
+ ILI9881C_COMMAND_INSTR(0x40, 0x00),
+ ILI9881C_COMMAND_INSTR(0x41, 0x00),
+ ILI9881C_COMMAND_INSTR(0x42, 0x00),
+ ILI9881C_COMMAND_INSTR(0x43, 0x00),
+ ILI9881C_COMMAND_INSTR(0x44, 0x00),
+ ILI9881C_COMMAND_INSTR(0x55, 0xab),
+ ILI9881C_COMMAND_INSTR(0x5a, 0x89),
+ ILI9881C_COMMAND_INSTR(0x5b, 0xab),
+ ILI9881C_COMMAND_INSTR(0x5c, 0xcd),
+ ILI9881C_COMMAND_INSTR(0x5d, 0xef),
+ ILI9881C_COMMAND_INSTR(0x5e, 0x11),
+ ILI9881C_COMMAND_INSTR(0x5f, 0x01),
+ ILI9881C_COMMAND_INSTR(0x60, 0x00),
+ ILI9881C_COMMAND_INSTR(0x61, 0x15),
+ ILI9881C_COMMAND_INSTR(0x62, 0x14),
+ ILI9881C_COMMAND_INSTR(0x63, 0x0e),
+ ILI9881C_COMMAND_INSTR(0x64, 0x0f),
+ ILI9881C_COMMAND_INSTR(0x65, 0x0c),
+ ILI9881C_COMMAND_INSTR(0x66, 0x0d),
+ ILI9881C_COMMAND_INSTR(0x67, 0x06),
+ ILI9881C_COMMAND_INSTR(0x68, 0x02),
+ ILI9881C_COMMAND_INSTR(0x69, 0x07),
+ ILI9881C_COMMAND_INSTR(0x6a, 0x02),
+ ILI9881C_COMMAND_INSTR(0x6b, 0x02),
+ ILI9881C_COMMAND_INSTR(0x6c, 0x02),
+ ILI9881C_COMMAND_INSTR(0x6d, 0x02),
+ ILI9881C_COMMAND_INSTR(0x6e, 0x02),
+ ILI9881C_COMMAND_INSTR(0x6f, 0x02),
+ ILI9881C_COMMAND_INSTR(0x70, 0x02),
+ ILI9881C_COMMAND_INSTR(0x71, 0x02),
+ ILI9881C_COMMAND_INSTR(0x72, 0x02),
+ ILI9881C_COMMAND_INSTR(0x73, 0x02),
+ ILI9881C_COMMAND_INSTR(0x74, 0x02),
+ ILI9881C_COMMAND_INSTR(0x75, 0x01),
+ ILI9881C_COMMAND_INSTR(0x76, 0x00),
+ ILI9881C_COMMAND_INSTR(0x77, 0x14),
+ ILI9881C_COMMAND_INSTR(0x78, 0x15),
+ ILI9881C_COMMAND_INSTR(0x79, 0x0e),
+ ILI9881C_COMMAND_INSTR(0x7a, 0x0f),
+ ILI9881C_COMMAND_INSTR(0x7b, 0x0c),
+ ILI9881C_COMMAND_INSTR(0x7c, 0x0d),
+ ILI9881C_COMMAND_INSTR(0x7d, 0x06),
+ ILI9881C_COMMAND_INSTR(0x7e, 0x02),
+ ILI9881C_COMMAND_INSTR(0x7f, 0x07),
+ ILI9881C_COMMAND_INSTR(0x88, 0x02),
+ ILI9881C_COMMAND_INSTR(0x89, 0x02),
+ ILI9881C_COMMAND_INSTR(0x8A, 0x02),
+ ILI9881C_SWITCH_PAGE_INSTR(4),
+ ILI9881C_COMMAND_INSTR(0x38, 0x01),
+ ILI9881C_COMMAND_INSTR(0x39, 0x00),
+ I
Re: [PATCH v2 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap()
On Wed, Jul 19, 2023 at 6:23 AM Kefeng Wang wrote:
> On 2023/7/19 17:02, Christian Göttsche wrote:
> > On Wed, 19 Jul 2023 at 09:40, Kefeng Wang
> > wrote:
> >>
> >> Use the helpers to simplify code.
> >>
> >> Cc: Paul Moore
> >> Cc: Stephen Smalley
> >> Cc: Eric Paris
> >> Acked-by: Paul Moore
> >> 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 d06e350fedee..ee8575540a8e 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -3762,13 +3762,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_initial_heap(vma)) {
> >
> > This seems to change the condition from
> >
> > vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <=
> > vma->vm_mm->brk
> >
> > to
> >
> > vma->vm_start <= vma->vm_mm->brk && vma->vm_end >=
> > vma->vm_mm->start_brk
> >
> > (or AND arguments swapped)
> >
> > vma->vm_end >= vma->vm_mm->start_brk && vma->vm_start <=
> > vma->vm_mm->brk
> >
> > Is this intended?
>
> The new condition is to check whether there is intersection between
> [startbrk,brk] and [vm_start,vm_end], it contains orignal check, so
> I think it is ok, but for selinux check, I am not sure if there is
> some other problem.
This particular SELinux vma check is see if the vma falls within the
heap; can you confirm that this change preserves this?
--
paul-moore.com
[PATCH 0/2] drm/panel: ilitek-ili9881c: Add TDO TL050HDV35-H1311A LCD panel
The first patch updates the DT documentation and the second is the update of the ilitek ili9881c driver. Matus Gajdos (2): dt-bindings: ili9881c: Add TDO TL050HDV35 LCD panel drm/panel: ilitek-ili9881c: Add TDO TL050HDV35 LCD panel .../display/panel/ilitek,ili9881c.yaml| 1 + drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 194 ++ 2 files changed, 195 insertions(+) -- 2.25.1
Re: [PATCH 3/3] drm/scheduler: Clean up jobs when the scheduler is torn down.
July 18, 2023 at 1:14 AM, "Luben Tuikov" wrote: > > > Not sure about other drivers--they can speak for themselves and the CC > > > list > > > should include them--please use "dim add-missing-cc" and make sure > > > that the Git commit description contains the Cc tags--then git send-email > > > will populate the SMTP CC. Feel free to add more Cc tags on top of that. > > > > I use `b4 prep -c` which I think does the same thing? I just ran it > > again and it only added 'linaro-mm-...@lists.linaro.org', not sure why > > that one wasn't there. Am I missing anything else? > > Not sure about "b4 prep -c"--using "git send-email" instead, but what is > important is to add the Cc: tags as part of the commit message. A "git log" of > drm-misc-next shows the proper format. Then maintainers add Link: > tag to the correct email thread, which is usually completely automated > by "dim" or by "git am", or both. It's useful to note here that this is not standard practice across the entirety of the Linux tree. In general, Cc: trailers are added to individual commits when get_maintainer.pl wouldn't otherwise include someone in the recipient list. The "dim" tool mentioned here is specific to the DRM subsystem (the "d" stands for "DRM"). Since both tools work on git series, you can use it alongside b4. DRM folks, if get_maintainer.pl isn't finding someone who should be included on a series of patches, should the MAINTAINERS file be updated to make it easier to submit valid patches without needing to know of "dim"? Regards, -K
[PATCH v2 0/2] drm/mipi-dbi: Allow using same the D/C GPIO for multiple displays
When multiple displays are connected to the same SPI bus, the data/command switch is sometimes considered part of the bus and is shared among the displays. This series adds the GPIO_FLAGS_BIT_NONEXCLUSIVE flag for this GPIO and SPI bus locking to the panel-mipi-dbi/drm_mipi_dbi drivers to support this hardware setup. --- Changes in v2: - fix uses of mipi_dbi_spi_transfer outside drm_mipi_dbi.c (errors reported by kernel test robot) - remove the is_locked argument introduced in v1 which was always set to true Otto Pflüger (2): drm/mipi-dbi: Lock SPI bus before setting D/C GPIO drm/tiny: panel-mipi-dbi: Allow sharing the D/C GPIO drivers/gpu/drm/drm_mipi_dbi.c| 17 + drivers/gpu/drm/tiny/ili9225.c| 7 ++- drivers/gpu/drm/tiny/ili9486.c| 4 drivers/gpu/drm/tiny/panel-mipi-dbi.c | 3 ++- 4 files changed, 25 insertions(+), 6 deletions(-) base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252 -- 2.39.1
[PATCH v2 1/2] drm/mipi-dbi: Lock SPI bus before setting D/C GPIO
Multiple displays may be connected to the same bus and share a D/C GPIO,
so the display driver needs exclusive access to the bus to ensure that
it can control the D/C GPIO safely.
Signed-off-by: Otto Pflüger
---
drivers/gpu/drm/drm_mipi_dbi.c | 17 +
drivers/gpu/drm/tiny/ili9225.c | 7 ++-
drivers/gpu/drm/tiny/ili9486.c | 4
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index c871d9f096b8..e90f0bf895b3 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1140,10 +1140,13 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi
*dbi, u8 *cmd,
return -ENOMEM;
tr[1].rx_buf = buf;
+
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 0);
spi_message_init_with_transfers(&m, tr, ARRAY_SIZE(tr));
- ret = spi_sync(spi, &m);
+ ret = spi_sync_locked(spi, &m);
+ spi_bus_unlock(spi->controller);
if (ret)
goto err_free;
@@ -1177,19 +1180,24 @@ static int mipi_dbi_typec3_command(struct mipi_dbi
*dbi, u8 *cmd,
MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 0);
speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, cmd, 1);
+ spi_bus_unlock(spi->controller);
if (ret || !num)
return ret;
if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !dbi->swap_bytes)
bpw = 16;
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 1);
speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
+ ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, par, num);
+ spi_bus_unlock(spi->controller);
- return mipi_dbi_spi_transfer(spi, speed_hz, bpw, par, num);
+ return ret;
}
/**
@@ -1271,7 +1279,8 @@ EXPORT_SYMBOL(mipi_dbi_spi_init);
* @len: Buffer length
*
* This SPI transfer helper breaks up the transfer of @buf into chunks which
- * the SPI controller driver can handle.
+ * the SPI controller driver can handle. The SPI bus must be locked when
+ * calling this.
*
* Returns:
* Zero on success, negative error code on failure.
@@ -1305,7 +1314,7 @@ int mipi_dbi_spi_transfer(struct spi_device *spi, u32
speed_hz,
buf += chunk;
len -= chunk;
- ret = spi_sync(spi, &m);
+ ret = spi_sync_locked(spi, &m);
if (ret)
return ret;
}
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
index 077c6ff5a2e1..4ceb68ffac4b 100644
--- a/drivers/gpu/drm/tiny/ili9225.c
+++ b/drivers/gpu/drm/tiny/ili9225.c
@@ -316,19 +316,24 @@ static int ili9225_dbi_command(struct mipi_dbi *dbi, u8
*cmd, u8 *par,
u32 speed_hz;
int ret;
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 0);
speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, cmd, 1);
+ spi_bus_unlock(spi->controller);
if (ret || !num)
return ret;
if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !dbi->swap_bytes)
bpw = 16;
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 1);
speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
+ ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, par, num);
+ spi_bus_unlock(spi->controller);
- return mipi_dbi_spi_transfer(spi, speed_hz, bpw, par, num);
+ return ret;
}
static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = {
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
index 02265c898816..938bceed5999 100644
--- a/drivers/gpu/drm/tiny/ili9486.c
+++ b/drivers/gpu/drm/tiny/ili9486.c
@@ -59,9 +59,11 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd,
u8 *par,
* before being transferred as 8-bit on the big endian SPI bus.
*/
buf[0] = cpu_to_be16(*cmd);
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(mipi->dc, 0);
speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 2);
ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, buf, 2);
+ spi_bus_unlock(spi->controller);
if (ret || !num)
goto free;
@@ -79,9 +81,11 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd,
u8 *par,
if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
bpw = 16;
+ spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(mipi->dc, 1);
speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, data, num);
+ spi_bus_unlock(spi->controller);
free:
kfree(buf);
--
2.39.1
[PATCH v2 2/2] drm/tiny: panel-mipi-dbi: Allow sharing the D/C GPIO
Displays that are connected to the same SPI bus may share the D/C GPIO. Use GPIOD_FLAGS_BIT_NONEXCLUSIVE to allow access to the same GPIO for multiple panel-mipi-dbi instances. Exclusive access to the GPIO during transfers is ensured by the locking in drm_mipi_dbi.c. Signed-off-by: Otto Pflüger --- drivers/gpu/drm/tiny/panel-mipi-dbi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c index eb9f13f18a02..e616e3890043 100644 --- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c @@ -307,7 +307,8 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi) if (IS_ERR(dbi->reset)) return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n"); - dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW); + dc = devm_gpiod_get_optional(dev, "dc", +GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE); if (IS_ERR(dc)) return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n"); -- 2.39.1
Re: [PATCH] accel/habanalabs: add more debugfs stub helpers
On Sun, Jun 11, 2023 at 12:50:31PM +0300, Oded Gabbay wrote:
> On Fri, Jun 9, 2023 at 4:37 PM Tomer Tayar wrote:
> >
> > On 09/06/2023 15:06, Arnd Bergmann wrote:
> > > From: Arnd Bergmann
> > >
> > > Two functions got added with normal prototypes for debugfs, but not
> > > alternative when building without it:
> > >
> > > drivers/accel/habanalabs/common/device.c: In function 'hl_device_init':
> > > drivers/accel/habanalabs/common/device.c:2177:14: error: implicit
> > > declaration of function 'hl_debugfs_device_init'; did you mean
> > > 'hl_debugfs_init'? [-Werror=implicit-function-declaration]
> > > drivers/accel/habanalabs/common/device.c:2305:9: error: implicit
> > > declaration of function 'hl_debugfs_device_fini'; did you mean
> > > 'hl_debugfs_remove_file'? [-Werror=implicit-function-declaration]
> > >
> > > Add stubs for these as well.
> > >
> > > Fixes: 553311fc7b76e ("accel/habanalabs: expose debugfs files later")
> > > Signed-off-by: Arnd Bergmann
> >
> > Thanks,
> > Reviewed-by: Tomer Tayar
>
> Thanks,
> Applied to -fixes.
As requested applied to drm-fixes, hopeful for the next one your drm-misc
account issue is fixed.
-Daniel
> Oded
> >
> > > ---
> > > drivers/accel/habanalabs/common/habanalabs.h | 9 +
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/accel/habanalabs/common/habanalabs.h
> > > b/drivers/accel/habanalabs/common/habanalabs.h
> > > index d92ba2e30e310..2f027d5a82064 100644
> > > --- a/drivers/accel/habanalabs/common/habanalabs.h
> > > +++ b/drivers/accel/habanalabs/common/habanalabs.h
> > > @@ -3980,6 +3980,15 @@ static inline void hl_debugfs_fini(void)
> > > {
> > > }
> > >
> > > +static inline int hl_debugfs_device_init(struct hl_device *hdev)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > +static inline void hl_debugfs_device_fini(struct hl_device *hdev)
> > > +{
> > > +}
> > > +
> > > static inline void hl_debugfs_add_device(struct hl_device *hdev)
> > > {
> > > }
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH] drm/ioctl: turn on -Woverride-init for IOCTL table
Recently two patches [1] [2] have hit a case of mistakenly picking
an IOCTL number which was already in-use. This is hard to debug
because the last definition wins and there is no indication that
there is a conflict.
Fix this by enabling -Werror=override-init for the IOCTL table.
When there is a duplicate entry, the compiler now errors out:
CC [M] drivers/gpu/drm/drm_ioctl.o
drivers/gpu/drm/drm_ioctl.c:555:33: error: initialized field overwritten
[-Werror=override-init]
555 | [DRM_IOCTL_NR(ioctl)] = { \
| ^
drivers/gpu/drm/drm_ioctl.c:708:9: note: in expansion of macro
‘DRM_IOCTL_DEF’
708 | DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EVENTFD,
drm_syncobj_reset_ioctl,
| ^
drivers/gpu/drm/drm_ioctl.c:555:33: note: (near initialization for
‘drm_ioctls[207]’)
555 | [DRM_IOCTL_NR(ioctl)] = { \
| ^
drivers/gpu/drm/drm_ioctl.c:708:9: note: in expansion of macro
‘DRM_IOCTL_DEF’
708 | DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EVENTFD,
drm_syncobj_reset_ioctl,
| ^
cc1: some warnings being treated as errors
[1]:
https://lore.kernel.org/dri-devel/20230714111257.11940-1-cont...@emersion.fr/
[2]:
https://lore.kernel.org/dri-devel/vVFDBgHpdcB0vOwnl02QPOFmAZPEbIV56E_wQ8B012K2GZ-fAGyG0JMbSrMu3-IcKYVf0JpJyrf71e6KFHfeMoSPJlYRACxlxy91eW9c6Fc=@emersion.fr/
Signed-off-by: Simon Ser
Cc: Erik Kurzinger
Cc: Daniel Vetter
Cc: Thomas Zimmermann
Cc: Christian König
---
drivers/gpu/drm/drm_ioctl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f03ffbacfe9b..cd485eb54d2a 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -566,6 +566,8 @@ static int drm_ioctl_permit(u32 flags, struct drm_file
*file_priv)
#endif
/* Ioctl table */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic error "-Woverride-init"
static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_VERSION, drm_version, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_GET_UNIQUE, drm_getunique, 0),
@@ -718,6 +720,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl,
DRM_MASTER),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl,
DRM_MASTER),
};
+#pragma GCC diagnostic pop
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE(drm_ioctls)
--
2.41.0
Re: [PATCH] accel/habanalabs: add more debugfs stub helpers
On Thu, Jul 20, 2023 at 1:29 PM Daniel Vetter wrote:
>
> On Sun, Jun 11, 2023 at 12:50:31PM +0300, Oded Gabbay wrote:
> > On Fri, Jun 9, 2023 at 4:37 PM Tomer Tayar wrote:
> > >
> > > On 09/06/2023 15:06, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann
> > > >
> > > > Two functions got added with normal prototypes for debugfs, but not
> > > > alternative when building without it:
> > > >
> > > > drivers/accel/habanalabs/common/device.c: In function 'hl_device_init':
> > > > drivers/accel/habanalabs/common/device.c:2177:14: error: implicit
> > > > declaration of function 'hl_debugfs_device_init'; did you mean
> > > > 'hl_debugfs_init'? [-Werror=implicit-function-declaration]
> > > > drivers/accel/habanalabs/common/device.c:2305:9: error: implicit
> > > > declaration of function 'hl_debugfs_device_fini'; did you mean
> > > > 'hl_debugfs_remove_file'? [-Werror=implicit-function-declaration]
> > > >
> > > > Add stubs for these as well.
> > > >
> > > > Fixes: 553311fc7b76e ("accel/habanalabs: expose debugfs files later")
> > > > Signed-off-by: Arnd Bergmann
> > >
> > > Thanks,
> > > Reviewed-by: Tomer Tayar
> >
> > Thanks,
> > Applied to -fixes.
>
> As requested applied to drm-fixes, hopeful for the next one your drm-misc
> account issue is fixed.
> -Daniel
Thanks Daniel for the help.
I also hope it will be fixed :)
Oded
>
> > Oded
> > >
> > > > ---
> > > > drivers/accel/habanalabs/common/habanalabs.h | 9 +
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/accel/habanalabs/common/habanalabs.h
> > > > b/drivers/accel/habanalabs/common/habanalabs.h
> > > > index d92ba2e30e310..2f027d5a82064 100644
> > > > --- a/drivers/accel/habanalabs/common/habanalabs.h
> > > > +++ b/drivers/accel/habanalabs/common/habanalabs.h
> > > > @@ -3980,6 +3980,15 @@ static inline void hl_debugfs_fini(void)
> > > > {
> > > > }
> > > >
> > > > +static inline int hl_debugfs_device_init(struct hl_device *hdev)
> > > > +{
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static inline void hl_debugfs_device_fini(struct hl_device *hdev)
> > > > +{
> > > > +}
> > > > +
> > > > static inline void hl_debugfs_add_device(struct hl_device *hdev)
> > > > {
> > > > }
> > >
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Re: [PATCH v3] drm/ssd130x: Fix an oops when attempting to update a disabled plane
Maxime Ripard writes:
Hello Maxime,
Thanks a lot for your feedback.
> Hi,
>
> On Mon, Jul 17, 2023 at 06:30:22PM +0200, Javier Martinez Canillas wrote:
>> Geert reports that the following NULL pointer dereference happens for him
>> after commit 49d7d581ceaf ("drm/ssd130x: Don't allocate buffers on each
>> plane update"):
>>
>> [drm] Initialized ssd130x 1.0.0 20220131 for 0-003c on minor 0
>> ssd130x-i2c 0-003c: [drm] surface width(128), height(32), bpp(1)
>> and format(R1 little-endian (0x20203152))
>> Unable to handle kernel NULL pointer dereference at virtual address
>>
>> Oops [#1]
>> CPU: 0 PID: 1 Comm: swapper Not tainted
>> 6.5.0-rc1-orangecrab-02219-g0a529a1e4bf4 #565
>> epc : ssd130x_update_rect.isra.0+0x13c/0x340
>> ra : ssd130x_update_rect.isra.0+0x2bc/0x340
>> ...
>> status: 0120 badaddr: cause: 000f
>> [] ssd130x_update_rect.isra.0+0x13c/0x340
>> [] ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
>> [] drm_atomic_helper_commit_planes+0xfc/0x27c
>> [] drm_atomic_helper_commit_tail+0x5c/0xb4
>> [] commit_tail+0x190/0x1b8
>> [] drm_atomic_helper_commit+0x194/0x1c0
>> [] drm_atomic_commit+0xa4/0xe4
>> [] drm_client_modeset_commit_atomic+0x244/0x278
>> [] drm_client_modeset_commit_locked+0x7c/0x1bc
>> [] drm_client_modeset_commit+0x34/0x64
>> [] __drm_fb_helper_restore_fbdev_mode_unlocked+0xc4/0xe8
>> [] drm_fb_helper_set_par+0x38/0x58
>> [] fbcon_init+0x294/0x534
>> ...
>>
>> The problem is that fbcon calls fbcon_init() which triggers a DRM modeset
>> and this leads to drm_atomic_helper_commit_planes() attempting to commit
>> the atomic state for all planes, even the ones whose CRTC is not enabled.
>>
>> Since the primary plane buffer is allocated in the encoder .atomic_enable
>> callback, this happens after that initial modeset commit and leads to the
>> mentioned NULL pointer dereference.
>
> I think that's where the problem lies: you must not allocate a buffer in
> atomic_enable.
>
> After drm_atomic_helper_swap_state(), the new commit is being applied
> and you aren't allowed to fail, and an allocation can fail.
>
> Everything needs to be prepared by the time _swap_state is called, and
> it's one of the point of atomic_check.
>
> So you need to allocate your buffer there, and use it in whatever
> atomic_commit related hook you need it in.
>
> The typical way of doing this would be to create a custom state
> structure that embeds the global one, create your own reset,
> atomic_duplicate_state and atomic_destroy_state implementations, and
> store the buffer pointer there.
>
I see. That makes totally sense to me. I'll do that in v4 then!
> Maxime
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH drm-misc-next v8 01/12] drm: manager to keep track of GPUs VA mappings
On 20/07/2023 01:14, Danilo Krummrich wrote:
> Add infrastructure to keep track of GPU virtual address (VA) mappings
> with a decicated VA space manager implementation.
>
> New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers
> start implementing, allow userspace applications to request multiple and
> arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is
> intended to serve the following purposes in this context.
>
> 1) Provide infrastructure to track GPU VA allocations and mappings,
>making using an interval tree (RB-tree).
>
> 2) Generically connect GPU VA mappings to their backing buffers, in
>particular DRM GEM objects.
>
> 3) Provide a common implementation to perform more complex mapping
>operations on the GPU VA space. In particular splitting and merging
>of GPU VA mappings, e.g. for intersecting mapping requests or partial
>unmap requests.
>
> Acked-by: Thomas Hellström
> Acked-by: Matthew Brost
> Reviewed-by: Boris Brezillon
> Tested-by: Matthew Brost
> Tested-by: Donald Robson
> Suggested-by: Dave Airlie
> Signed-off-by: Danilo Krummrich
[...]
> diff --git a/drivers/gpu/drm/drm_gpuva_mgr.c b/drivers/gpu/drm/drm_gpuva_mgr.c
> new file mode 100644
> index ..dee2235530d6
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gpuva_mgr.c
[...]
> +static bool
> +drm_gpuva_check_overflow(u64 addr, u64 range)
> +{
> + u64 end;
> +
> + return WARN(check_add_overflow(addr, range, &end),
> + "GPUVA address limited to %lu bytes.\n", sizeof(end));
> +}
This produces a warning on 32 bit systems as sizeof() isn't necessarily
an unsigned long. The fix below silences the warning.
Thanks,
Steve
---8<-
>From 9c7356580362b6ac4673724f18ea6e8453b52913 Mon Sep 17 00:00:00 2001
From: Steven Price
Date: Thu, 20 Jul 2023 10:58:09 +0100
Subject: [PATCH] drm: manager: Fix printk format for size_t
sizeof() returns a size_t which may be different to an unsigned long.
Use the correct format specifier of '%zu' to prevent compiler warnings.
Fixes: e6303f323b1a ("drm: manager to keep track of GPUs VA mappings")
Signed-off-by: Steven Price
---
drivers/gpu/drm/drm_gpuva_mgr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gpuva_mgr.c b/drivers/gpu/drm/drm_gpuva_mgr.c
index 0b80177592a6..f86bfad74ff8 100644
--- a/drivers/gpu/drm/drm_gpuva_mgr.c
+++ b/drivers/gpu/drm/drm_gpuva_mgr.c
@@ -619,7 +619,7 @@ drm_gpuva_check_overflow(u64 addr, u64 range)
u64 end;
return WARN(check_add_overflow(addr, range, &end),
- "GPUVA address limited to %lu bytes.\n", sizeof(end));
+ "GPUVA address limited to %zu bytes.\n", sizeof(end));
}
static bool
--
2.39.2
Re: [PATCH drm-misc-next v8 02/12] drm: debugfs: provide infrastructure to dump a DRM GPU VA space
On 20/07/2023 01:14, Danilo Krummrich wrote:
> This commit adds a function to dump a DRM GPU VA space and a macro for
> drivers to register the struct drm_info_list 'gpuvas' entry.
>
> Most likely, most drivers might maintain one DRM GPU VA space per struct
> drm_file, but there might also be drivers not having a fixed relation
> between DRM GPU VA spaces and a DRM core infrastructure, hence we need the
> indirection via the driver iterating it's maintained DRM GPU VA spaces.
>
> Reviewed-by: Boris Brezillon
> Signed-off-by: Danilo Krummrich
> ---
> drivers/gpu/drm/drm_debugfs.c | 40 +++
> include/drm/drm_debugfs.h | 25 ++
> 2 files changed, 65 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 4855230ba2c6..c90dbcffa0dc 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -39,6 +39,7 @@
> #include
> #include
> #include
> +#include
>
> #include "drm_crtc_internal.h"
> #include "drm_internal.h"
> @@ -175,6 +176,45 @@ static const struct file_operations drm_debugfs_fops = {
> .release = single_release,
> };
>
> +/**
> + * drm_debugfs_gpuva_info - dump the given DRM GPU VA space
> + * @m: pointer to the &seq_file to write
> + * @mgr: the &drm_gpuva_manager representing the GPU VA space
> + *
> + * Dumps the GPU VA mappings of a given DRM GPU VA manager.
> + *
> + * For each DRM GPU VA space drivers should call this function from their
> + * &drm_info_list's show callback.
> + *
> + * Returns: 0 on success, -ENODEV if the &mgr is not initialized
> + */
> +int drm_debugfs_gpuva_info(struct seq_file *m,
> +struct drm_gpuva_manager *mgr)
> +{
> + struct drm_gpuva *va, *kva = &mgr->kernel_alloc_node;
> +
> + if (!mgr->name)
> + return -ENODEV;
> +
> + seq_printf(m, "DRM GPU VA space (%s) [0x%016llx;0x%016llx]\n",
> +mgr->name, mgr->mm_start, mgr->mm_start + mgr->mm_range);
> + seq_printf(m, "Kernel reserved node [0x%016llx;0x%016llx]\n",
> +kva->va.addr, kva->va.addr + kva->va.range);
> + seq_puts(m, "\n");
> + seq_puts(m, " VAs | start | range | end
> | object | object offset\n");
> + seq_puts(m,
> "-\n");
> + drm_gpuva_for_each_va(va, mgr) {
> + if (unlikely(va == kva))
> + continue;
> +
> + seq_printf(m, " | 0x%016llx | 0x%016llx | 0x%016llx |
> 0x%016llx | 0x%016llx\n",
> +va->va.addr, va->va.range, va->va.addr +
> va->va.range,
> +(u64)va->gem.obj, va->gem.offset);
Casting a pointer to u64 generates a warning on 32 bit systems:
drivers/gpu/drm/drm_debugfs.c:212:7: error: cast from pointer to integer of
different size [-Werror=pointer-to-int-cast]
212 | (u64)va->gem.obj, va->gem.offset);
| ^
Adding an extra cast to uintptr_t shuts the compiler up (alternatively a
proper pointer type like %px could be used in the format string, but
that does make the table layout harder).
The patch below fixes the warning for me.
Steve
8<
>From bea59724d68fec0b3a56f82a42d0e4e59c514565 Mon Sep 17 00:00:00 2001
From: Steven Price
Date: Thu, 20 Jul 2023 11:45:11 +0100
Subject: [PATCH] drm: debugfs: Silence warning from cast
Casting a pointer to an integer of a different size generates a warning
from the compiler. First cast the pointer to a pointer-sized type to
keep the compiler happy.
Fixes: 4f66feeab173 ("drm: debugfs: provide infrastructure to dump a DRM GPU VA
space")
Signed-off-by: Steven Price
---
drivers/gpu/drm/drm_debugfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index c90dbcffa0dc..a3a488205009 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -209,7 +209,7 @@ int drm_debugfs_gpuva_info(struct seq_file *m,
seq_printf(m, " | 0x%016llx | 0x%016llx | 0x%016llx |
0x%016llx | 0x%016llx\n",
va->va.addr, va->va.range, va->va.addr +
va->va.range,
- (u64)va->gem.obj, va->gem.offset);
+ (u64)(uintptr_t)va->gem.obj, va->gem.offset);
}
return 0;
--
2.39.2
Re: [RFC v5 00/17] DRM cgroup controller with scheduling control and memory stats
Hi, On 19/07/2023 21:31, T.J. Mercier wrote: On Wed, Jul 12, 2023 at 4:47 AM Tvrtko Ursulin wrote: drm.memory.stat A nested file containing cumulative memory statistics for the whole sub-hierarchy, broken down into separate GPUs and separate memory regions supported by the latter. For example:: $ cat drm.memory.stat card0 region=system total=12898304 shared=0 active=0 resident=12111872 purgeable=167936 card0 region=stolen-system total=0 shared=0 active=0 resident=0 purgeable=0 Card designation corresponds to the DRM device names and multiple line entries can be present per card. Memory region names should be expected to be driver specific with the exception of 'system' which is standardised and applicable for GPUs which can operate on system memory buffers. Sub-keys 'resident' and 'purgeable' are optional. Per category region usage is reported in bytes. * Feedback from people interested in drm.active_us and drm.memory.stat is required to understand the use cases and their usefulness (of the fields). Memory stats are something which was easy to add to my series, since I was already working on the fdinfo memory stats patches, but the question is how useful it is. Hi Tvrtko, I think this style of driver-defined categories for reporting of memory could potentially allow us to eliminate the GPU memory tracking tracepoint used on Android (gpu_mem_total). This would involve reading drm.memory.stat at the root cgroup (I see it's currently disabled on I can put it available under root too, don't think there is any technical reason to not have it. In fact, now that I look at it again, memory.stat is present on root so that would align with my general guideline to keep the two as similar as possible. the root), which means traversing the whole cgroup tree under the cgroup lock to generate the values on-demand. This would be done rarely, but I still wonder what the cost of that would turn out to be. Yeah that's ugly. I could eliminate cgroup_lock by being a bit smarter. Just didn't think it worth it for the RFC. Basically to account memory stats for any sub-tree I need the equivalent one struct drm_memory_stats per DRM device present in the hierarchy. So I could pre-allocate a few and restart if run out of spares, or something. They are really small so pre-allocating a good number, based on past state or something, should would good enough. Or even total number of DRM devices in a system as a pessimistic and safe option for most reasonable deployments. The drm_memory_stats categories in the output don't seem like a big value-add for this use-case, but no real objection to them being You mean the fact there are different categories is not a value add for your use case because you would only use one? The idea was to align 1:1 with DRM memory stats fdinfo and somewhat emulate how memory.stat also offers a breakdown. there. I know it's called the DRM cgroup controller, but it'd be nice if there were a way to make the mem tracking part work for any driver that wishes to participate as many of our devices don't use a DRM driver. But making that work doesn't look like it would fit very Ah that would be a challenge indeed to which I don't have any answers right now. Hm if you have a DRM device somewhere in the chain memory stats would still show up. Like if you had a dma-buf producer which is not a DRM driver, but then that buffer was imported by a DRM driver, it would show up in a cgroup. Or vice-versa. But if there aren't any in the whole chain then it would not. cleanly into this controller, so I'll just shut up now. Not all all, good feedback! Regards, Tvrtko
[PATCH] accel/habanalabs/gaudi2: prepare to remove cpu_rst_status
From: Igor Grinberg
The soft reset has transitioned to CPUCP packet instead of plain
register write and is about to be removed from the struct cpu_dyn_regs.
As a preparation for removing the cpu_rst_status field from
struct cpu_dyn_regs, switch to use the plain macro - this keeps the
backward compatibility.
Signed-off-by: Igor Grinberg
Reviewed-by: Oded Gabbay
Signed-off-by: Oded Gabbay
---
drivers/accel/habanalabs/gaudi2/gaudi2.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/accel/habanalabs/gaudi2/gaudi2.c
b/drivers/accel/habanalabs/gaudi2/gaudi2.c
index 222310bf1098..dc96e34b72ef 100644
--- a/drivers/accel/habanalabs/gaudi2/gaudi2.c
+++ b/drivers/accel/habanalabs/gaudi2/gaudi2.c
@@ -6253,16 +6253,12 @@ static int gaudi2_get_soft_rst_done_indication(struct
hl_device *hdev, u32 poll_
static int gaudi2_execute_soft_reset(struct hl_device *hdev, bool
driver_performs_reset,
u32 poll_timeout_us)
{
- struct cpu_dyn_regs *dyn_regs =
&hdev->fw_loader.dynamic_loader.comm_desc.cpu_dyn_regs;
- int rc = 0;
+ int rc;
if (!driver_performs_reset) {
if (hl_is_fw_sw_ver_below(hdev, 1, 10)) {
/* set SP to indicate reset request sent to FW */
- if (dyn_regs->cpu_rst_status)
- WREG32(le32_to_cpu(dyn_regs->cpu_rst_status),
CPU_RST_STATUS_NA);
- else
- WREG32(mmCPU_RST_STATUS_TO_HOST,
CPU_RST_STATUS_NA);
+ WREG32(mmCPU_RST_STATUS_TO_HOST, CPU_RST_STATUS_NA);
WREG32(mmGIC_HOST_SOFT_RST_IRQ_POLL_REG,
gaudi2_irq_map_table[GAUDI2_EVENT_CPU_SOFT_RESET].cpu_id);
--
2.34.1
Re: [09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init
On Tue, Jul 18, 2023 at 01:24:29AM +0800, suijingfeng wrote:
> On 2023/7/10 15:47, Maxime Ripard wrote:
> > The new helper to init the locking context allows to remove some
> > boilerplate.
> >
> > Signed-off-by: Maxime Ripard
> > ---
> > drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 42
> > --
> > 1 file changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> > b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> > index 776a7b01608f..ff1deaed0cab 100644
> > --- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> > +++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
> > @@ -20,7 +20,6 @@
> > struct pv_muxing_priv {
> > struct vc4_dev *vc4;
> > - struct drm_modeset_acquire_ctx ctx;
> > struct drm_atomic_state *state;
> > };
> > @@ -725,6 +724,7 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit
> > *test)
> > static int vc4_pv_muxing_test_init(struct kunit *test)
> > {
> > const struct pv_muxing_param *params = test->param_value;
> > + struct drm_modeset_acquire_ctx *ctx;
> > struct drm_atomic_state *state;
> > struct pv_muxing_priv *priv;
> > struct drm_device *drm;
> > @@ -738,13 +738,14 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
> > priv->vc4 = vc4;
> > - drm_modeset_acquire_init(&priv->ctx, 0);
> > + ctx = drm_kunit_helper_acquire_ctx_alloc(test);
>
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
>
> The pointer returned by drm_kunit_helper_acquire_ctx_alloc() function can't
> be NULL,
>
> if ctx is NULL, the current kthread will be terminated by the
> KUNIT_ASSERT_NOT_NULL() in the drm_kunit_helper_acquire_ctx_alloc().
>
> so only a PTR_ERR is possible, right?
>
> If so, probably invent a KUNIT_ASSERT_NOT_ERR() function to call is enough.
>
> I'm fine with this patch, but I feel the checking if the ctx is NULL is
> redundant.
I guess, but we're still reference that pointer later on, so making sure
that it's a valid pointer still makes sense.
Maxime
signature.asc
Description: PGP signature
[PATCH v2 00/11] drm: kunit: Switch to kunit actions
Hi, Since v6.5-rc1, kunit gained a devm/drmm-like mechanism that makes tests resources much easier to cleanup. This series converts the existing tests to use those new actions where relevant. Let me know what you think, Maxime Signed-off-by: Maxime Ripard --- Changes in v2: - Fix some typos - Use plaltform_device_del instead of removing the call to platform_device_put after calling platform_device_add - Link to v1: https://lore.kernel.org/r/20230710-kms-kunit-actions-rework-v1-0-722c58d72...@kernel.org --- Maxime Ripard (11): drm/tests: helpers: Switch to kunit actions drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device() drm/tests: modes: Remove call to drm_kunit_helper_free_device() drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device() drm/tests: helpers: Create a helper to allocate a locking ctx drm/tests: helpers: Create a helper to allocate an atomic state drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device() drm/vc4: tests: mock: Use a kunit action to unregister DRM device drm/vc4: tests: pv-muxing: Switch to managed locking init drm/vc4: tests: Switch to atomic state allocation helper drm/vc4: tests: pv-muxing: Document test scenario drivers/gpu/drm/tests/drm_client_modeset_test.c | 8 -- drivers/gpu/drm/tests/drm_kunit_helpers.c | 108 +- drivers/gpu/drm/tests/drm_modes_test.c | 8 -- drivers/gpu/drm/tests/drm_probe_helper_test.c | 8 -- drivers/gpu/drm/vc4/tests/vc4_mock.c| 5 ++ drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 115 +--- include/drm/drm_kunit_helpers.h | 7 ++ 7 files changed, 158 insertions(+), 101 deletions(-) --- base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252 change-id: 20230710-kms-kunit-actions-rework-5d163762c93b Best regards, -- Maxime Ripard
[PATCH v2 01/11] drm/tests: helpers: Switch to kunit actions
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 28
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 4df47071dc88..5856beb7f7d7 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -35,8 +35,8 @@ static struct platform_driver fake_platform_driver = {
* able to leverage the usual infrastructure and most notably the
* device-managed resources just like a "real" device.
*
- * Callers need to make sure drm_kunit_helper_free_device() on the
- * device when done.
+ * Resources will be cleaned up automatically, but the removal can be
+ * forced using @drm_kunit_helper_free_device.
*
* Returns:
* A pointer to the new device, or an ERR_PTR() otherwise.
@@ -49,12 +49,27 @@ struct device *drm_kunit_helper_alloc_device(struct kunit
*test)
ret = platform_driver_register(&fake_platform_driver);
KUNIT_ASSERT_EQ(test, ret, 0);
+ ret = kunit_add_action_or_reset(test,
+ (kunit_action_t
*)platform_driver_unregister,
+ &fake_platform_driver);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+ ret = kunit_add_action_or_reset(test,
+ (kunit_action_t *)platform_device_put,
+ pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
ret = platform_device_add(pdev);
KUNIT_ASSERT_EQ(test, ret, 0);
+ ret = kunit_add_action_or_reset(test,
+ (kunit_action_t *)platform_device_del,
+ pdev);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
return &pdev->dev;
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
@@ -70,8 +85,13 @@ void drm_kunit_helper_free_device(struct kunit *test, struct
device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- platform_device_unregister(pdev);
- platform_driver_unregister(&fake_platform_driver);
+ kunit_release_action(test,
+(kunit_action_t *)platform_device_unregister,
+pdev);
+
+ kunit_release_action(test,
+(kunit_action_t *)platform_driver_unregister,
+&fake_platform_driver);
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
--
2.41.0
[PATCH v2 02/11] drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device()
Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.
Remove it.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_client_modeset_test.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c
b/drivers/gpu/drm/tests/drm_client_modeset_test.c
index 416a279b6dae..7516f6cb36e4 100644
--- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -82,13 +82,6 @@ static int drm_client_modeset_test_init(struct kunit *test)
return 0;
}
-static void drm_client_modeset_test_exit(struct kunit *test)
-{
- struct drm_client_modeset_test_priv *priv = test->priv;
-
- drm_kunit_helper_free_device(test, priv->dev);
-}
-
static void drm_test_pick_cmdline_res_1920_1080_60(struct kunit *test)
{
struct drm_client_modeset_test_priv *priv = test->priv;
@@ -188,7 +181,6 @@ static struct kunit_case drm_test_pick_cmdline_tests[] = {
static struct kunit_suite drm_test_pick_cmdline_test_suite = {
.name = "drm_test_pick_cmdline",
.init = drm_client_modeset_test_init,
- .exit = drm_client_modeset_test_exit,
.test_cases = drm_test_pick_cmdline_tests
};
--
2.41.0
[PATCH v2 04/11] drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device()
Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.
Remove it.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_probe_helper_test.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_probe_helper_test.c
b/drivers/gpu/drm/tests/drm_probe_helper_test.c
index 0ee65828623e..1a2044070a6c 100644
--- a/drivers/gpu/drm/tests/drm_probe_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_probe_helper_test.c
@@ -60,13 +60,6 @@ static int drm_probe_helper_test_init(struct kunit *test)
return 0;
}
-static void drm_probe_helper_test_exit(struct kunit *test)
-{
- struct drm_probe_helper_test_priv *priv = test->priv;
-
- drm_kunit_helper_free_device(test, priv->dev);
-}
-
typedef struct drm_display_mode *(*expected_mode_func_t)(struct drm_device *);
struct drm_connector_helper_tv_get_modes_test {
@@ -208,7 +201,6 @@ static struct kunit_case
drm_test_connector_helper_tv_get_modes_tests[] = {
static struct kunit_suite drm_test_connector_helper_tv_get_modes_suite = {
.name = "drm_connector_helper_tv_get_modes",
.init = drm_probe_helper_test_init,
- .exit = drm_probe_helper_test_exit,
.test_cases = drm_test_connector_helper_tv_get_modes_tests,
};
--
2.41.0
[PATCH v2 03/11] drm/tests: modes: Remove call to drm_kunit_helper_free_device()
Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.
Remove it.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_modes_test.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/tests/drm_modes_test.c
b/drivers/gpu/drm/tests/drm_modes_test.c
index bc4aa2ce78be..1e9f63fbfead 100644
--- a/drivers/gpu/drm/tests/drm_modes_test.c
+++ b/drivers/gpu/drm/tests/drm_modes_test.c
@@ -36,13 +36,6 @@ static int drm_test_modes_init(struct kunit *test)
return 0;
}
-static void drm_test_modes_exit(struct kunit *test)
-{
- struct drm_test_modes_priv *priv = test->priv;
-
- drm_kunit_helper_free_device(test, priv->dev);
-}
-
static void drm_test_modes_analog_tv_ntsc_480i(struct kunit *test)
{
struct drm_test_modes_priv *priv = test->priv;
@@ -148,7 +141,6 @@ static struct kunit_case drm_modes_analog_tv_tests[] = {
static struct kunit_suite drm_modes_analog_tv_test_suite = {
.name = "drm_modes_analog_tv",
.init = drm_test_modes_init,
- .exit = drm_test_modes_exit,
.test_cases = drm_modes_analog_tv_tests,
};
--
2.41.0
[PATCH v2 05/11] drm/tests: helpers: Create a helper to allocate a locking ctx
As we get more and more tests, the locking context initialisation
creates more and more boilerplate, both at creation and destruction.
Let's create a helper that will allocate, initialise a context, and
register kunit actions to clean up once the test is done.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++
include/drm/drm_kunit_helpers.h | 2 ++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 5856beb7f7d7..5130d4553262 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -120,5 +120,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct
kunit *test,
}
EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
+static void action_drm_release_context(void *ptr)
+{
+ struct drm_modeset_acquire_ctx *ctx = ptr;
+
+ drm_modeset_drop_locks(ctx);
+ drm_modeset_acquire_fini(ctx);
+}
+
+/**
+ * drm_kunit_helper_context_alloc - Allocates an acquire context
+ * @test: The test context object
+ *
+ * Allocates and initializes a modeset acquire context.
+ *
+ * The context is tied to the kunit test context, so we must not call
+ * drm_modeset_acquire_fini() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated context otherwise
+ */
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
+{
+ struct drm_modeset_acquire_ctx *ctx;
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+ drm_modeset_acquire_init(ctx, 0);
+
+ ret = kunit_add_action_or_reset(test,
+ action_drm_release_context,
+ ctx);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+
MODULE_AUTHOR("Maxime Ripard ");
MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index ed013fdcc1ff..4ba5e10653c6 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
sizeof(_type),
\
offsetof(_type, _member),
\
_feat))
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
#endif // DRM_KUNIT_HELPERS_H_
--
2.41.0
[PATCH v2 09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init
The new helper to init the locking context allows to remove some
boilerplate.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 42 --
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index 776a7b01608f..ff1deaed0cab 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -20,7 +20,6 @@
struct pv_muxing_priv {
struct vc4_dev *vc4;
- struct drm_modeset_acquire_ctx ctx;
struct drm_atomic_state *state;
};
@@ -725,6 +724,7 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit
*test)
static int vc4_pv_muxing_test_init(struct kunit *test)
{
const struct pv_muxing_param *params = test->param_value;
+ struct drm_modeset_acquire_ctx *ctx;
struct drm_atomic_state *state;
struct pv_muxing_priv *priv;
struct drm_device *drm;
@@ -738,13 +738,14 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
priv->vc4 = vc4;
- drm_modeset_acquire_init(&priv->ctx, 0);
+ ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = &vc4->base;
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = &priv->ctx;
+ state->acquire_ctx = ctx;
priv->state = state;
@@ -757,8 +758,6 @@ static void vc4_pv_muxing_test_exit(struct kunit *test)
struct drm_atomic_state *state = priv->state;
drm_atomic_state_put(state);
- drm_modeset_drop_locks(&priv->ctx);
- drm_modeset_acquire_fini(&priv->ctx);
}
static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -798,7 +797,7 @@ static struct kunit_suite vc5_pv_muxing_test_suite = {
*/
static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit
*test)
{
- struct drm_modeset_acquire_ctx ctx;
+ struct drm_modeset_acquire_ctx *ctx;
struct drm_atomic_state *state;
struct vc4_crtc_state *new_vc4_crtc_state;
struct vc4_hvs_state *new_hvs_state;
@@ -811,13 +810,14 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
vc4 = vc5_mock_device(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
- drm_modeset_acquire_init(&ctx, 0);
+ ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = &vc4->base;
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = &ctx;
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -844,7 +844,7 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = &ctx;
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -866,13 +866,11 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
drm_atomic_state_put(state);
- drm_modeset_drop_locks(&ctx);
- drm_modeset_acquire_fini(&ctx);
}
static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
{
- struct drm_modeset_acquire_ctx ctx;
+ struct drm_modeset_acquire_ctx *ctx;
struct drm_atomic_state *state;
struct vc4_crtc_state *new_vc4_crtc_state;
struct vc4_hvs_state *new_hvs_state;
@@ -885,13 +883,14 @@ static void
drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
vc4 = vc5_mock_device(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
- drm_modeset_acquire_init(&ctx, 0);
+ ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = &vc4->base;
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = &ctx;
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -929,7 +928,7 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = &ctx;
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_del_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -954,14 +953,12 @@ st
[PATCH v2 06/11] drm/tests: helpers: Create a helper to allocate an atomic state
As we gain more tests, boilerplate to allocate an atomic state and free
it starts to be there more and more as well.
In order to reduce the allocation boilerplate, we can create a helper
to create that atomic state, and call an action when the test is done.
This will also clean up the exit path.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 39 +++
include/drm/drm_kunit_helpers.h | 5
2 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 5130d4553262..d3b2c6b48163 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+#include
#include
#include
#include
@@ -161,5 +162,43 @@ drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+/**
+ * drm_kunit_helper_atomic_state_alloc - Allocates an atomic state
+ * @test: The test context object
+ * @drm: The device to alloc the state for
+ * @ctx: Locking context for that atomic update
+ *
+ * Allocates a empty atomic state.
+ *
+ * The state is tied to the kunit test context, so we must not call
+ * drm_atomic_state_put() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated state otherwise
+ */
+struct drm_atomic_state *
+drm_kunit_helper_atomic_state_alloc(struct kunit *test,
+ struct drm_device *drm,
+ struct drm_modeset_acquire_ctx *ctx)
+{
+ struct drm_atomic_state *state;
+ int ret;
+
+ state = drm_atomic_state_alloc(drm);
+ if (!state)
+ return ERR_PTR(-ENOMEM);
+
+ ret = kunit_add_action_or_reset(test,
+ (kunit_action_t *)drm_atomic_state_put,
+ state);
+ if (ret)
+ return ERR_PTR(ret);
+
+ state->acquire_ctx = ctx;
+
+ return state;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_atomic_state_alloc);
+
MODULE_AUTHOR("Maxime Ripard ");
MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index 4ba5e10653c6..514c8a7a32f0 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -90,4 +90,9 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
struct drm_modeset_acquire_ctx *
drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
+struct drm_atomic_state *
+drm_kunit_helper_atomic_state_alloc(struct kunit *test,
+ struct drm_device *drm,
+ struct drm_modeset_acquire_ctx *ctx);
+
#endif // DRM_KUNIT_HELPERS_H_
--
2.41.0
[PATCH v2 08/11] drm/vc4: tests: mock: Use a kunit action to unregister DRM device
The *_mock_device functions allocate a DRM device that needs to be
released using drm_dev_unregister.
Now that we have a kunit release action API, we can switch to it and
don't require any kind of garbage collection from the caller.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/tests/vc4_mock.c | 5 +
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 6 --
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.c
b/drivers/gpu/drm/vc4/tests/vc4_mock.c
index a4bed26af32f..00825ddc52f0 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.c
@@ -186,6 +186,11 @@ static struct vc4_dev *__mock_device(struct kunit *test,
bool is_vc5)
ret = drm_dev_register(drm, 0);
KUNIT_ASSERT_EQ(test, ret, 0);
+ ret = kunit_add_action_or_reset(test,
+ (kunit_action_t *)drm_dev_unregister,
+ drm);
+ KUNIT_ASSERT_EQ(test, ret, 0);
+
return vc4;
}
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index 6c982e72cae8..776a7b01608f 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -754,14 +754,11 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
static void vc4_pv_muxing_test_exit(struct kunit *test)
{
struct pv_muxing_priv *priv = test->priv;
- struct vc4_dev *vc4 = priv->vc4;
- struct drm_device *drm = &vc4->base;
struct drm_atomic_state *state = priv->state;
drm_atomic_state_put(state);
drm_modeset_drop_locks(&priv->ctx);
drm_modeset_acquire_fini(&priv->ctx);
- drm_dev_unregister(drm);
}
static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -871,7 +868,6 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
drm_atomic_state_put(state);
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
- drm_dev_unregister(drm);
}
static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
@@ -960,7 +956,6 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
drm_atomic_state_put(state);
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
- drm_dev_unregister(drm);
}
static void
@@ -1013,7 +1008,6 @@
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
drm_atomic_state_put(state);
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
- drm_dev_unregister(drm);
}
static struct kunit_case vc5_pv_muxing_bugs_tests[] = {
--
2.41.0
[PATCH v2 10/11] drm/vc4: tests: Switch to atomic state allocation helper
Now that we have a helper that takes care of an atomic state allocation
and cleanup, we can migrate to it to simplify our tests.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 55 --
1 file changed, 8 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index ff1deaed0cab..5f9f5626329d 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -725,7 +725,6 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
{
const struct pv_muxing_param *params = test->param_value;
struct drm_modeset_acquire_ctx *ctx;
- struct drm_atomic_state *state;
struct pv_muxing_priv *priv;
struct drm_device *drm;
struct vc4_dev *vc4;
@@ -742,24 +741,12 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = &vc4->base;
- state = drm_atomic_state_alloc(drm);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
-
- state->acquire_ctx = ctx;
-
- priv->state = state;
+ priv->state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->state);
return 0;
}
-static void vc4_pv_muxing_test_exit(struct kunit *test)
-{
- struct pv_muxing_priv *priv = test->priv;
- struct drm_atomic_state *state = priv->state;
-
- drm_atomic_state_put(state);
-}
-
static struct kunit_case vc4_pv_muxing_tests[] = {
KUNIT_CASE_PARAM(drm_vc4_test_pv_muxing,
vc4_test_pv_muxing_gen_params),
@@ -771,7 +758,6 @@ static struct kunit_case vc4_pv_muxing_tests[] = {
static struct kunit_suite vc4_pv_muxing_test_suite = {
.name = "vc4-pv-muxing-combinations",
.init = vc4_pv_muxing_test_init,
- .exit = vc4_pv_muxing_test_exit,
.test_cases = vc4_pv_muxing_tests,
};
@@ -786,7 +772,6 @@ static struct kunit_case vc5_pv_muxing_tests[] = {
static struct kunit_suite vc5_pv_muxing_test_suite = {
.name = "vc5-pv-muxing-combinations",
.init = vc4_pv_muxing_test_init,
- .exit = vc4_pv_muxing_test_exit,
.test_cases = vc5_pv_muxing_tests,
};
@@ -814,11 +799,9 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = &vc4->base;
- state = drm_atomic_state_alloc(drm);
+ state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = ctx;
-
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -839,13 +822,9 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
ret = drm_atomic_helper_swap_state(state, false);
KUNIT_ASSERT_EQ(test, ret, 0);
- drm_atomic_state_put(state);
-
- state = drm_atomic_state_alloc(drm);
+ state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = ctx;
-
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -864,8 +843,6 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_ASSERT_TRUE(test,
new_hvs_state->fifo_state[hdmi1_channel].in_use);
KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
-
- drm_atomic_state_put(state);
}
static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
@@ -887,11 +864,9 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = &vc4->base;
- state = drm_atomic_state_alloc(drm);
+ state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = ctx;
-
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -923,13 +898,9 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
ret = drm_atomic_helper_swap_state(state, false);
KUNIT_ASSERT_EQ(test, ret, 0);
- drm_atomic_state_put(state);
-
- state = drm_atomic_state_alloc(drm);
+ state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = ctx;
-
ret = vc4_mock_atomic_del_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -951,8 +922,6 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
KUNIT_EXPECT_EQ(test, old_hdmi1_channel, h
[PATCH v2 07/11] drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device()
Calling drm_kunit_helper_free_device() to clean up the resources
allocated by drm_kunit_helper_alloc_device() is now optional and not
needed in most cases.
Remove it.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index ae0bd0f81698..6c982e72cae8 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -762,7 +762,6 @@ static void vc4_pv_muxing_test_exit(struct kunit *test)
drm_modeset_drop_locks(&priv->ctx);
drm_modeset_acquire_fini(&priv->ctx);
drm_dev_unregister(drm);
- drm_kunit_helper_free_device(test, vc4->dev);
}
static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -873,7 +872,6 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
drm_dev_unregister(drm);
- drm_kunit_helper_free_device(test, vc4->dev);
}
static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
@@ -963,7 +961,6 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
drm_dev_unregister(drm);
- drm_kunit_helper_free_device(test, vc4->dev);
}
static void
@@ -1017,7 +1014,6 @@
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct ku
drm_modeset_drop_locks(&ctx);
drm_modeset_acquire_fini(&ctx);
drm_dev_unregister(drm);
- drm_kunit_helper_free_device(test, vc4->dev);
}
static struct kunit_case vc5_pv_muxing_bugs_tests[] = {
--
2.41.0
[PATCH v2 11/11] drm/vc4: tests: pv-muxing: Document test scenario
We've had a couple of tests that weren't really obvious, nor did they
document what they were supposed to test. Document that to make it
hopefully more obvious.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index 5f9f5626329d..61622e951031 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -845,6 +845,13 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
}
+/*
+ * This test makes sure that we never change the FIFO of an active HVS
+ * channel if we disable a FIFO with a lower index.
+ *
+ * Doing so would result in a FIFO stall and would disrupt an output
+ * supposed to be unaffected by the commit.
+ */
static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
{
struct drm_modeset_acquire_ctx *ctx;
@@ -924,6 +931,21 @@ static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct
kunit *test)
}
}
+/*
+ * Test that if we affect a single output, only the CRTC state of that
+ * output will be pulled in the global atomic state.
+ *
+ * This is relevant for two things:
+ *
+ * - If we don't have that state at all, we are unlikely to affect the
+ * FIFO muxing. This is somewhat redundant with
+ * drm_test_vc5_pv_muxing_bugs_stable_fifo()
+ *
+ * - KMS waits for page flips to occur on all the CRTC found in the
+ * CRTC state. Since the CRTC is unaffected, we would over-wait, but
+ * most importantly run into corner cases like waiting on an
+ * inactive CRTC that never completes.
+ */
static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable_too_many_crtc_state(struct
kunit *test)
{
--
2.41.0
Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state
On Thu, Jul 20, 2023 at 06:06:27AM +, Ying Liu wrote: > Bootloader may leave gpio direction as input and gpio value as logical low. > It hints that initial backlight power state should be FB_BLANK_POWERDOWN > since the gpio value is literally logical low. To be honest this probably "hints" that the bootloader simply didn't consider the backlight at all :-) . I'd rather the patch description focus on what circumstances lead to the current code making a bad decision. More like: If the GPIO pin is in the input state but the backlight is currently off due to default pull downs then ... > So, let's drop output gpio > direction check and only check gpio value to set the initial power state. This check was specifically added by Bartosz so I'd be interested in his opinion of this change (especially since he is now a GPIO maintainer)! What motivates (or motivated) the need to check the direction rather than just read that current logic level on the pin? Daniel. [I'm done but since Bartosz and Linus were not on copy of the original thread I've left the rest of the patch below as a convenience ;-) ] > Signed-off-by: Liu Ying > --- > drivers/video/backlight/gpio_backlight.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/video/backlight/gpio_backlight.c > b/drivers/video/backlight/gpio_backlight.c > index d3bea42407f1..d28c30b2a35d 100644 > --- a/drivers/video/backlight/gpio_backlight.c > +++ b/drivers/video/backlight/gpio_backlight.c > @@ -87,8 +87,7 @@ static int gpio_backlight_probe(struct platform_device > *pdev) > /* Not booted with device tree or no phandle link to the node */ > bl->props.power = def_value ? FB_BLANK_UNBLANK > : FB_BLANK_POWERDOWN; > - else if (gpiod_get_direction(gbl->gpiod) == 0 && > - gpiod_get_value_cansleep(gbl->gpiod) == 0) > + else if (gpiod_get_value_cansleep(gbl->gpiod) == 0) > bl->props.power = FB_BLANK_POWERDOWN; > else > bl->props.power = FB_BLANK_UNBLANK; > -- > 2.37.1 >
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
Il 20/07/23 12:14, Alexandre Mergnat ha scritto:
On 20/07/2023 10:26, Shuijing Li wrote:
Due to the difference of HW, different dividers need to be set.
Signed-off-by: Shuijing Li
Signed-off-by: Jitao Shi
---
Changes in v3:
Separate these two things into two different patches.
per suggestion from the previous thread:
https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/
Changes in v2:
- change the variables' name to be more descriptive
- add a comment that describes the function of mtk_dp_audio_sample_arrange
- reduce indentation by doing the inverse check
- add a definition of some bits
- add support for mediatek, mt8188-edp-tx
per suggestion from the previous thread:
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/
---
drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++-
drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
b/drivers/gpu/drm/mediatek/mtk_dp.c
index d8cda83d6fef..8e1a13ab2ba2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -140,6 +140,7 @@ struct mtk_dp_data {
const struct mtk_dp_efuse_fmt *efuse_fmt;
bool audio_supported;
bool audio_pkt_in_hblank_area;
+ u16 audio_m_div2_bit;
};
static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
@@ -648,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp
*mtk_dp,
static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
{
mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
- AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+ mtk_dp->data->audio_m_div2_bit,
AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
}
@@ -2636,6 +2637,7 @@ static const struct mtk_dp_data mt8188_edp_data = {
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8188_dp_data = {
@@ -2644,6 +2646,7 @@ static const struct mtk_dp_data mt8188_dp_data = {
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
.audio_pkt_in_hblank_area = true,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_edp_data = {
@@ -2652,6 +2655,7 @@ static const struct mtk_dp_data mt8195_edp_data = {
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_dp_data = {
@@ -2660,6 +2664,7 @@ static const struct mtk_dp_data mt8195_dp_data = {
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
.audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct of_device_id mtk_dp_of_match[] = {
diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
index f38d6ff12afe..6d7f0405867e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
@@ -162,6 +162,7 @@
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 (1 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 (2 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 (3 << 8)
+#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8)
IMO, it's a bit weird to have SoC specific define in the generic header.
Are you sure this bit is only available for MT8188 ?
Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (5 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 (6 << 8)
#define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 (7 << 8)
Reviewed-by: Alexandre Mergnat
[LINUX KERNEL PATCH v3 1/1] virtgpu: init vq during resume and notify qemu guest status
This patch solves two problem:
First, when we suspended guest VM, it called into Qemu to call
virtio_reset->__virtio_queue_reset, this cleared all virtuqueue
information of virtgpu on Qemu end. As a result, after guest
resumed, guest sended ctrl/cursor requests to Qemu through
virtqueue, but Qemu can't get requests from the virtqueue now.
In function virtio_queue_notify, vq->vring.desc is NULL.
So, this patch add freeze and restore function for virtgpu driver.
In freeze function, it flushes all virtqueue works and deletes
virtqueues. In restore function, it initializes virtqueues. And
then, Qemu and guest can communicate normally.
Second, when we suspended guest VM, it called into Qemu to call
virtio_reset->virtio_gpu_gl_reset, this destroyed resources and
reset renderer which were used for display. As a result, after
guest resumed, the display can't come back and we only saw a black
screen.
So, this patch add a new ctrl message VIRTIO_GPU_CMD_SET_FREEZE_MODE.
When guest is during suspending, we set freeze mode to freeze_S3 to
notify Qemu that guest entered suspending, and then Qemu will not
destroy resources. When guest is during resuming, we set freeze mode
to unfreeze to notify Qemu that guest exited suspending, and then
Qemu will keep its origin actions. As a result, the display can come
back and everything of guest can come back to the time when guest
was suspended.
Due to this implemention needs cooperation with host Qemu, so it
added a new feature flag VIRTIO_GPU_F_FREEZE_S3, so that guest and
host can negotiate whenever freeze_S3 is supported or not.
Signed-off-by: Jiqian Chen
---
drivers/gpu/drm/virtio/virtgpu_debugfs.c | 1 +
drivers/gpu/drm/virtio/virtgpu_drv.c | 39
drivers/gpu/drm/virtio/virtgpu_drv.h | 5 +++
drivers/gpu/drm/virtio/virtgpu_kms.c | 36 --
drivers/gpu/drm/virtio/virtgpu_vq.c | 16 ++
include/uapi/linux/virtio_gpu.h | 19
6 files changed, 107 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index 853dd9aa397e..c84fd6d7f5f3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -55,6 +55,7 @@ static int virtio_gpu_features(struct seq_file *m, void *data)
virtio_gpu_add_bool(m, "blob resources", vgdev->has_resource_blob);
virtio_gpu_add_bool(m, "context init", vgdev->has_context_init);
+ virtio_gpu_add_bool(m, "freeze_S3", vgdev->has_freeze_S3);
virtio_gpu_add_int(m, "cap sets", vgdev->num_capsets);
virtio_gpu_add_int(m, "scanouts", vgdev->num_scanouts);
if (vgdev->host_visible_region.len) {
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index add075681e18..83ad0ac82b94 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -130,6 +130,40 @@ static void virtio_gpu_config_changed(struct virtio_device
*vdev)
schedule_work(&vgdev->config_changed_work);
}
+#ifdef CONFIG_PM
+static int virtio_gpu_freeze(struct virtio_device *dev)
+{
+ struct drm_device *ddev = dev->priv;
+ struct virtio_gpu_device *vgdev = ddev->dev_private;
+ int ret = 0;
+
+ if (vgdev->has_freeze_S3) {
+ ret = virtio_gpu_cmd_set_freeze_mode(vgdev,
+ VIRTIO_GPU_FREEZE_MODE_FREEZE_S3);
+ }
+ if (!ret) {
+ flush_work(&vgdev->ctrlq.dequeue_work);
+ flush_work(&vgdev->cursorq.dequeue_work);
+ vgdev->vdev->config->del_vqs(vgdev->vdev);
+ }
+ return ret;
+}
+
+static int virtio_gpu_restore(struct virtio_device *dev)
+{
+ struct drm_device *ddev = dev->priv;
+ struct virtio_gpu_device *vgdev = ddev->dev_private;
+ int ret;
+
+ ret = virtio_gpu_init_vqs(dev);
+ if (!ret && vgdev->has_freeze_S3) {
+ ret = virtio_gpu_cmd_set_freeze_mode(vgdev,
+ VIRTIO_GPU_FREEZE_MODE_UNFREEZE);
+ }
+ return ret;
+}
+#endif
+
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_GPU, VIRTIO_DEV_ANY_ID },
{ 0 },
@@ -148,6 +182,7 @@ static unsigned int features[] = {
VIRTIO_GPU_F_RESOURCE_UUID,
VIRTIO_GPU_F_RESOURCE_BLOB,
VIRTIO_GPU_F_CONTEXT_INIT,
+ VIRTIO_GPU_F_FREEZE_S3,
};
static struct virtio_driver virtio_gpu_driver = {
.feature_table = features,
@@ -156,6 +191,10 @@ static struct virtio_driver virtio_gpu_driver = {
.driver.owner = THIS_MODULE,
.id_table = id_table,
.probe = virtio_gpu_probe,
+#ifdef CONFIG_PM
+ .freeze = virtio_gpu_freeze,
+ .restore = virtio_gpu_restore,
+#endif
.remove = virtio_gpu_remove,
.config_changed = virtio_gpu_config_changed
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio
[LINUX KERNEL PATCH v3 0/1] add S3 support for virtgpu
v3: Hi all, Thanks for Gerd Hoffmann's advice. V3 makes below changes: * Use enum for freeze mode, so this can be extended with more modes in the future. * Rename functions and paratemers with "_S3" postfix. And no functional changes. Best regards, Jiqian Chen. v2: Hi all, Thanks to Marc-André Lureau, Robert Beckett and Gerd Hoffmann for their advice and guidance. V2 makes below changes: * Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000) * Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and host can negotiate whenever freezing is supported or not. V2 of Qemu patch: https://lore.kernel.org/qemu-devel/20230630070016.841459-1-jiqian.c...@amd.com/T/#t v1: link, https://lore.kernel.org/lkml/20230608063857.1677973-1-jiqian.c...@amd.com/ Hi all, I am working to implement virtgpu S3 function on Xen. Currently on Xen, if we start a guest who enables virtgpu, and then run "echo mem > /sys/power/state" to suspend guest. And run "sudo xl trigger s3resume" to resume guest. We can find that the guest kernel comes back, but the display doesn't. It just shows a black screen. In response to the above phenomenon, I have found two problems. First, if we move mouse on the black screen, guest kernel still sends a cursor request to Qemu, but Qemu doesn't response. Because when guest is suspending, it calls device_suspend, and then call into Qemu to call virtio_reset->__virtio_queue_reset. In __virtio_queue_reset, it clears all virtqueue information on Qemu end. So, after guest resumes, Qemu can't get message from virtqueue. Second, the reason why display can't come back is that when guest is suspending, it calls into Qemu to call virtio_reset->virtio_gpu_gl_reset. In virtio_gpu_gl_reset, it destroys all resources and resets renderer, which are used for display. So after guest resumes, the display can't come back to the status when guest is suspended. This patch initializes virtqueue when guest is resuming to solve first problem. And it notifies Qemu that guest is suspending to prevent Qemu destroying resources, this is to solve second problem. And then, I can bring the display back, and everything continues their actions after guest resumes. Modifications on Qemu end: https://lore.kernel.org/qemu-devel/20230608025655.1674357-2-jiqian.c...@amd.com/ Jiqian Chen (1): virtgpu: init vq during resume and notify qemu guest status drivers/gpu/drm/virtio/virtgpu_debugfs.c | 1 + drivers/gpu/drm/virtio/virtgpu_drv.c | 39 drivers/gpu/drm/virtio/virtgpu_drv.h | 5 +++ drivers/gpu/drm/virtio/virtgpu_kms.c | 36 -- drivers/gpu/drm/virtio/virtgpu_vq.c | 16 ++ include/uapi/linux/virtio_gpu.h | 19 6 files changed, 107 insertions(+), 9 deletions(-) -- 2.34.1
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote: Il 20/07/23 12:14, Alexandre Mergnat ha scritto: On 20/07/2023 10:26, Shuijing Li wrote: Due to the difference of HW, different dividers need to be set. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi --- Changes in v3: Separate these two things into two different patches. per suggestion from the previous thread: https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/ Changes in v2: - change the variables' name to be more descriptive - add a comment that describes the function of mtk_dp_audio_sample_arrange - reduce indentation by doing the inverse check - add a definition of some bits - add support for mediatek, mt8188-edp-tx per suggestion from the previous thread: https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/ --- drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++- drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) ... b/drivers/gpu/drm/mediatek/mtk_dp_reg.h index f38d6ff12afe..6d7f0405867e 100644 --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h @@ -162,6 +162,7 @@ #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 (1 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 (2 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 (3 << 8) +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8) IMO, it's a bit weird to have SoC specific define in the generic header. Are you sure this bit is only available for MT8188 ? Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-) Ok then, to avoid this kind of issue for other SoCs in the future, is that make sense for you to do a SoC specific header file beside the generic one? #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (5 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 (6 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 (7 << 8) Reviewed-by: Alexandre Mergnat -- Regards, Alexandre
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
Il 20/07/23 14:07, Alexandre Mergnat ha scritto: On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote: Il 20/07/23 12:14, Alexandre Mergnat ha scritto: On 20/07/2023 10:26, Shuijing Li wrote: Due to the difference of HW, different dividers need to be set. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi --- Changes in v3: Separate these two things into two different patches. per suggestion from the previous thread: https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/ Changes in v2: - change the variables' name to be more descriptive - add a comment that describes the function of mtk_dp_audio_sample_arrange - reduce indentation by doing the inverse check - add a definition of some bits - add support for mediatek, mt8188-edp-tx per suggestion from the previous thread: https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/ --- drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++- drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) ... b/drivers/gpu/drm/mediatek/mtk_dp_reg.h index f38d6ff12afe..6d7f0405867e 100644 --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h @@ -162,6 +162,7 @@ #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 (1 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 (2 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 (3 << 8) +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8) IMO, it's a bit weird to have SoC specific define in the generic header. Are you sure this bit is only available for MT8188 ? Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-) Ok then, to avoid this kind of issue for other SoCs in the future, is that make sense for you to do a SoC specific header file beside the generic one? For just one definition? That's a bit overkill :-) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (5 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 (6 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 (7 << 8) Reviewed-by: Alexandre Mergnat
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
On 20/07/2023 14:08, AngeloGioacchino Del Regno wrote: Il 20/07/23 14:07, Alexandre Mergnat ha scritto: On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote: Il 20/07/23 12:14, Alexandre Mergnat ha scritto: On 20/07/2023 10:26, Shuijing Li wrote: Due to the difference of HW, different dividers need to be set. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi --- Changes in v3: Separate these two things into two different patches. per suggestion from the previous thread: https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/ Changes in v2: - change the variables' name to be more descriptive - add a comment that describes the function of mtk_dp_audio_sample_arrange - reduce indentation by doing the inverse check - add a definition of some bits - add support for mediatek, mt8188-edp-tx per suggestion from the previous thread: https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/ --- drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++- drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) ... b/drivers/gpu/drm/mediatek/mtk_dp_reg.h index f38d6ff12afe..6d7f0405867e 100644 --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h @@ -162,6 +162,7 @@ #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 (1 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 (2 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 (3 << 8) +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8) IMO, it's a bit weird to have SoC specific define in the generic header. Are you sure this bit is only available for MT8188 ? Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-) Ok then, to avoid this kind of issue for other SoCs in the future, is that make sense for you to do a SoC specific header file beside the generic one? For just one definition? That's a bit overkill :-) You're right, but we must start somewhere ^^, and show the proper way for future patches. Actually, I gave my Reviewed-by because it's only one definition. This will be fixed later (I hope). #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (5 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 (6 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 (7 << 8) Reviewed-by: Alexandre Mergnat -- Regards, Alexandre
Re: [PATCH v6 5/9] drm/i915/gt: Refactor intel_emit_pipe_control_cs() in a single function
Some of the patches are needed to be backported to v5.8 so I wonder if
you should do the refactoring later in the series.
I guess best way is to check if you can apply the series in v5.8
On 7/19/2023 1:07 PM, Andi Shyti wrote:
Just a trivial refactoring for reducing the number of code
duplicate. This will come at handy in the next commits.
Signed-off-by: Andi Shyti
---
drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 44 +---
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
index 7566c89d9def3..1b1dadacfbf42 100644
--- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
@@ -177,23 +177,31 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32
*cs, const i915_reg_t inv
return cs;
}
+static u32 *intel_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0,
+ u32 bit_group_1, u32 offset)
+{
+ u32 *cs;
+
+ cs = intel_ring_begin(rq, 6);
+ if (IS_ERR(cs))
+ return cs;
+
+ cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
+LRC_PPHWSP_SCRATCH_ADDR);
+ intel_ring_advance(rq, cs);
+
+ return cs;
+}
+
static int mtl_dummy_pipe_control(struct i915_request *rq)
{
/* Wa_14016712196 */
if (IS_MTL_GRAPHICS_STEP(rq->engine->i915, M, STEP_A0, STEP_B0) ||
- IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0)) {
- u32 *cs;
-
- /* dummy PIPE_CONTROL + depth flush */
- cs = intel_ring_begin(rq, 6);
- if (IS_ERR(cs))
- return PTR_ERR(cs);
- cs = gen12_emit_pipe_control(cs,
-0,
-PIPE_CONTROL_DEPTH_CACHE_FLUSH,
-LRC_PPHWSP_SCRATCH_ADDR);
- intel_ring_advance(rq, cs);
- }
+ IS_MTL_GRAPHICS_STEP(rq->engine->i915, P, STEP_A0, STEP_B0))
+ intel_emit_pipe_control_cs(rq,
+ 0,
+ PIPE_CONTROL_DEPTH_CACHE_FLUSH,
+ LRC_PPHWSP_SCRATCH_ADDR);
return 0;
}
@@ -210,7 +218,6 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
u32 bit_group_0 = 0;
u32 bit_group_1 = 0;
int err;
- u32 *cs;
err = mtl_dummy_pipe_control(rq);
if (err)
@@ -237,13 +244,8 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
else if (engine->class == COMPUTE_CLASS)
bit_group_1 &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
- cs = intel_ring_begin(rq, 6);
- if (IS_ERR(cs))
- return PTR_ERR(cs);
-
- cs = gen12_emit_pipe_control(cs, bit_group_0, bit_group_1,
-LRC_PPHWSP_SCRATCH_ADDR);
- intel_ring_advance(rq, cs);
+ intel_emit_pipe_control_cs(rq, bit_group_0, bit_group_1,
+ LRC_PPHWSP_SCRATCH_ADDR);
}
if (mode & EMIT_INVALIDATE) {
Re: [PATCH v6 6/9] drm/i915/gt: Ensure memory quiesced before invalidation for all engines
Hi Andi,
On 7/19/2023 1:07 PM, Andi Shyti wrote:
Commit af9e423a8aae ("drm/i915/gt: Ensure memory quiesced before
invalidation") has made sure that the memory is quiesced before
invalidating the AUX CCS table. Do it for all the other engines
and not just RCS.
Signed-off-by: Andi Shyti
Cc: Jonathan Cavitt
Cc: Matt Roper
---
drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 46 ++--
1 file changed, 36 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
index 1b1dadacfbf42..3bedab8d61db1 100644
--- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
@@ -309,19 +309,45 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32
mode)
int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode)
{
intel_engine_mask_t aux_inv = 0;
- u32 cmd, *cs;
+ u32 cmd = 4;
+ u32 *cs;
- cmd = 4;
- if (mode & EMIT_INVALIDATE) {
+ if (mode & EMIT_INVALIDATE)
cmd += 2;
- if (HAS_AUX_CCS(rq->engine->i915) &&
- (rq->engine->class == VIDEO_DECODE_CLASS ||
-rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) {
- aux_inv = rq->engine->mask &
- ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0);
- if (aux_inv)
- cmd += 4;
+ if (HAS_AUX_CCS(rq->engine->i915))
+ aux_inv = rq->engine->mask &
+ ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0);
+
+ /*
+* On Aux CCS platforms the invalidation of the Aux
+* table requires quiescing memory traffic beforehand
+*/
+ if (aux_inv) {
+ u32 bit_group_0 = 0;
+ u32 bit_group_1 = 0;
+
+ cmd += 4;
+
+ bit_group_0 |= PIPE_CONTROL0_HDC_PIPELINE_FLUSH;
+
+ switch (rq->engine->class) {
+ case VIDEO_DECODE_CLASS:
+ bit_group_1 |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH;
+ bit_group_1 |= PIPE_CONTROL_DEPTH_CACHE_FLUSH;
+ bit_group_1 |= PIPE_CONTROL_DC_FLUSH_ENABLE;
+ bit_group_1 |= PIPE_CONTROL_FLUSH_L3;
+ bit_group_1 |= PIPE_CONTROL_CS_STALL;
+
+ intel_emit_pipe_control_cs(rq, bit_group_0, bit_group_1,
+ LRC_PPHWSP_SCRATCH_ADDR);
I think pipe control is only for compute and render engines.
+
+ break;
+
+ case VIDEO_ENHANCEMENT_CLASS:
+ case COMPUTE_CLASS:
Don't think gen12_emit_flush_xcs() will get called for compute engine.
intel_guc_submission_setup() --> rcs_submission_override() replaces
gen12_emit_flush_xcs() with |gen12_emit_flush_rcs()|
|for compute and render.|
|
|
|Regards,|
|Nirmoy
|
+ case COPY_ENGINE_CLASS:
+ break;
}
}
[PATCH next] drm/loongson: Fix error handling in lsdc_pixel_pll_setup()
There are two problems in lsdc_pixel_pll_setup()
1. If kzalloc() fails then call iounmap() to release the resources.
2. Both kzalloc and ioremap doesnot return error pointers on failure, so
using IS_ERR_OR_NULL() checks is a bit confusing and not very right,
fix this by changing those to NULL checks instead.
Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Signed-off-by: Harshit Mogalapalli
---
This is found with static analysis with smacth and only compile tested.
---
drivers/gpu/drm/loongson/lsdc_pixpll.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/loongson/lsdc_pixpll.c
b/drivers/gpu/drm/loongson/lsdc_pixpll.c
index 04c15b4697e2..2609a2256da4 100644
--- a/drivers/gpu/drm/loongson/lsdc_pixpll.c
+++ b/drivers/gpu/drm/loongson/lsdc_pixpll.c
@@ -120,12 +120,14 @@ static int lsdc_pixel_pll_setup(struct lsdc_pixpll *
const this)
struct lsdc_pixpll_parms *pparms;
this->mmio = ioremap(this->reg_base, this->reg_size);
- if (IS_ERR_OR_NULL(this->mmio))
+ if (!this->mmio)
return -ENOMEM;
pparms = kzalloc(sizeof(*pparms), GFP_KERNEL);
- if (IS_ERR_OR_NULL(pparms))
+ if (!pparms) {
+ iounmap(this->mmio);
return -ENOMEM;
+ }
pparms->ref_clock = LSDC_PLL_REF_CLK_KHZ;
--
2.39.3
[PATCH v3 1/2] dt-bindings: display: add bindings for pcd8544 displays
Add device tree binding documentation for PCD8544 LCD display controller.
Signed-off-by: Viktar Simanenka
---
v3:add a little more description to the exposed vendor properties
add commit message finally
v2 link:
https://lore.kernel.org/linux-devicetree/20230719154450.620410-1-viteo...@gmail.com/
.../bindings/display/nxp,pcd8544.yaml | 95 +++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/nxp,pcd8544.yaml
diff --git a/Documentation/devicetree/bindings/display/nxp,pcd8544.yaml
b/Documentation/devicetree/bindings/display/nxp,pcd8544.yaml
new file mode 100644
index ..bacdeff9776e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/nxp,pcd8544.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/nxp,pcd8544.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Philips Semiconductors PCD8544 LCD Display Controller
+
+maintainers:
+ - Viktar Simanenka
+
+description: |
+ Philips Semiconductors PCD8544 LCD Display Controller with SPI control bus.
+ Designed to drive a graphic display of 48 rows and 84 columns,
+ such as Nokia 5110/3310 LCDs.
+
+allOf:
+ - $ref: panel/panel-common.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+enum:
+ - nxp,pcd8544
+
+ dc-gpios:
+maxItems: 1
+description: Data/Command selection pin (D/CX)
+
+ reset-gpios:
+maxItems: 1
+description: Display Reset pin (RST)
+
+ nxp,inverted:
+type: boolean
+description: Display color inversion
+
+ nxp,voltage-op:
+$ref: /schemas/types.yaml#/definitions/uint32
+minimum: 0
+maximum: 127
+description: |
+ Liquid crystall voltage operation coefficient. Determines the LCD
+ controlling voltage on display segments. Should be adjusted according
+ to the ambient temperature. Adjusts the contrast of the display.
+
+ nxp,temperature-coeff:
+$ref: /schemas/types.yaml#/definitions/uint32
+minimum: 0
+maximum: 3
+description: |
+ Display temperature compensation coefficient. Increases LCD controlling
+ voltage at lower temperatures to maintain optimum contrast.
+
+ nxp,bias:
+$ref: /schemas/types.yaml#/definitions/uint32
+minimum: 0
+maximum: 7
+description: |
+ Display bias system coefficient. Should only be changed if an external
+ oscillator is used for the display.
+
+required:
+ - compatible
+ - reg
+ - dc-gpios
+ - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+ - |
+#include
+
+spi {
+#address-cells = <1>;
+#size-cells = <0>;
+
+display@0 {
+compatible = "nxp,pcd8544";
+reg = <0>;
+spi-max-frequency = <800>;
+
+dc-gpios = <&pio 0 3 GPIO_ACTIVE_HIGH>; /* DC=PA3 */
+reset-gpios = <&pio 0 1 GPIO_ACTIVE_HIGH>; /* RESET=PA1 */
+backlight = <&backlight>;
+
+nxp,inverted;
+nxp,voltage-op = <0>;
+nxp,bias = <4>;
+nxp,temperature-coeff = <0>;
+};
+};
+
+...
--
2.34.1
[PATCH v3 2/2] drm/tiny: add display driver for philips pcd8544 display controller
Add support for monochrome LCD SPI displays (such as Nokia 5110/3310 LCD)
based on PCD8544 LCD display controller.
Signed-off-by: Viktar Simanenka
---
v3:nothing changed from v2
v2:checked and fixed with sparse and smatch
changed param prefixes
v2 link:
https://lore.kernel.org/linux-devicetree/20230719154450.620410-2-viteo...@gmail.com/
drivers/gpu/drm/tiny/Kconfig | 11 +
drivers/gpu/drm/tiny/Makefile | 1 +
drivers/gpu/drm/tiny/pcd8544.c | 506 +
3 files changed, 518 insertions(+)
create mode 100644 drivers/gpu/drm/tiny/pcd8544.c
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649bc1..10caa0818253 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -172,6 +172,17 @@ config TINYDRM_MI0283QT
DRM driver for the Multi-Inno MI0283QT display panel
If M is selected the module will be called mi0283qt.
+config TINYDRM_PCD8544
+ tristate "DRM support for PCD8544 displays"
+ depends on DRM && SPI
+ select DRM_KMS_HELPER
+ select DRM_GEM_DMA_HELPER
+ select BACKLIGHT_CLASS_DEVICE
+ help
+ DRM driver for PCD8544 (Nokia 5110/3310) 84x48 LCD displays.
+
+ If M is selected the module will be called pcd8544.
+
config TINYDRM_REPAPER
tristate "DRM support for Pervasive Displays RePaper panels (V231)"
depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a044b..75bc112a02f9 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
+obj-$(CONFIG_TINYDRM_PCD8544) += pcd8544.o
obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
obj-$(CONFIG_TINYDRM_ST7586) += st7586.o
obj-$(CONFIG_TINYDRM_ST7735R) += st7735r.o
diff --git a/drivers/gpu/drm/tiny/pcd8544.c b/drivers/gpu/drm/tiny/pcd8544.c
new file mode 100644
index ..73958b302a36
--- /dev/null
+++ b/drivers/gpu/drm/tiny/pcd8544.c
@@ -0,0 +1,506 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * DRM driver for Philips PCD8544 LCD controller/driver.
+ * Compatible with Nokia 5110/3310 84x48 LCD displays.
+ *
+ * Copyright 2023 Viktar Simanenka
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/*
+ * The display is monochrome, every bit in buffer is a pixel.
+ * Display RAM divided into 6 banks along y-axis, each bank 84 bytes along
x-axis.
+ * Driver uses horizontal addressing.
+ */
+
+#define PCD8544_FUNCTIONSET 0x20
+#define PCD8544_DISPLAYCONTROL 0x08
+#define PCD8544_SETYADDR0x40
+#define PCD8544_SETXADDR0x80
+#define PCD8544_SETBIAS 0x10
+#define PCD8544_SETTEMPCOEF 0x04
+#define PCD8544_SETVOP 0x80
+
+#define PCD8544_EXTENDED_INSTRUCTION0x01
+#define PCD8544_VERTICAL_ADDRESSING 0x02
+#define PCD8544_DISPLAYNORMAL 0x04
+#define PCD8544_DISPLAYINVERTED 0x05
+
+struct pcd8544_device {
+ struct drm_device drm;
+ struct drm_simple_display_pipe pipe;
+ struct drm_connector connector;
+ struct drm_display_mode mode;
+ struct spi_device *spi;
+
+ u32 width;
+ u32 height;
+ u8 *tx_buf; // Buffer used for transfer
+ size_t tx_buflen;
+
+ struct backlight_device *backlight;
+ struct gpio_desc *reset;
+ struct gpio_desc *dc;
+
+ u32 inverted;
+ u32 temperature_coeff;
+ u32 bias;
+ u32 voltage_op;
+};
+
+MODULE_PARM_DESC(inverted, "Invert display colors: 1 - enable, 0 - disable");
+MODULE_PARM_DESC(voltage_op, "Vop[6:0] LCD voltage operation coefficient:
0-127 (default: 0)");
+MODULE_PARM_DESC(temperature_coeff, "TC[1:0] Temperature compensation
coefficient: 0-3 (default: 0)");
+MODULE_PARM_DESC(bias, "BS[2:0] Bias system coefficient: 0-7 (default: 4)");
+
+#define drm_to_dev(__dev) container_of(__dev, struct pcd8544_device, drm)
+
+static int pcd8544_spi_transfer(struct spi_device *spi, const void *buf,
size_t len)
+{
+ size_t max_chunk = spi_max_transfer_size(spi);
+ struct spi_transfer tr = {
+ .bits_per_word = 8,
+ .speed_hz = 0,
+ };
+ struct spi_message m;
+ size_t chunk;
+ int ret;
+
+ max_chunk = ALIGN_DOWN(max_chunk, 2);
+
+ spi_message_init_with_transfers(&m, &tr, 1);
+
+ while (len) {
+ chunk = min(len, max_chunk);
+
+ tr.tx_buf = buf;
+ tr.len = chunk;
+ buf += chunk;
+ len -= chunk;
+
+ ret = spi_sync(spi, &m);
+
Re: [PATCH v3,3/3] drm/mediatek: dp: Add the audio divider to mtk_dp_data struct
Il 20/07/23 14:29, Alexandre Mergnat ha scritto: On 20/07/2023 14:08, AngeloGioacchino Del Regno wrote: Il 20/07/23 14:07, Alexandre Mergnat ha scritto: On 20/07/2023 13:54, AngeloGioacchino Del Regno wrote: Il 20/07/23 12:14, Alexandre Mergnat ha scritto: On 20/07/2023 10:26, Shuijing Li wrote: Due to the difference of HW, different dividers need to be set. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi --- Changes in v3: Separate these two things into two different patches. per suggestion from the previous thread: https://lore.kernel.org/lkml/e2ad22bcba31797f38a12a488d4246a01bf0cb2e.ca...@mediatek.com/ Changes in v2: - change the variables' name to be more descriptive - add a comment that describes the function of mtk_dp_audio_sample_arrange - reduce indentation by doing the inverse check - add a definition of some bits - add support for mediatek, mt8188-edp-tx per suggestion from the previous thread: https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/ --- drivers/gpu/drm/mediatek/mtk_dp.c | 7 ++- drivers/gpu/drm/mediatek/mtk_dp_reg.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) ... b/drivers/gpu/drm/mediatek/mtk_dp_reg.h index f38d6ff12afe..6d7f0405867e 100644 --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h @@ -162,6 +162,7 @@ #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_2 (1 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_4 (2 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MUL_8 (3 << 8) +#define MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (4 << 8) IMO, it's a bit weird to have SoC specific define in the generic header. Are you sure this bit is only available for MT8188 ? Eh, the P0_DIV2 bit is 5<<8 for MT8195, while for 8188 it's 4<<8, clearly :-) Ok then, to avoid this kind of issue for other SoCs in the future, is that make sense for you to do a SoC specific header file beside the generic one? For just one definition? That's a bit overkill :-) You're right, but we must start somewhere ^^, and show the proper way for future patches. Actually, I gave my Reviewed-by because it's only one definition. This will be fixed later (I hope). I'm confident that *if* and when "a bunch" of SoC-specific definitions will appear, those will be splitted in different headers. :-) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2 (5 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_4 (6 << 8) #define AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_8 (7 << 8) Reviewed-by: Alexandre Mergnat
[PATCH v2 3/5] drm/i915: Fix HPD polling, reenabling the output poll work as needed
After the commit in the Fixes: line below, HPD polling stopped working
on i915, since after that change calling drm_kms_helper_poll_enable()
doesn't restart drm_mode_config::output_poll_work if the work was
stopped (no connectors needing polling) and enabling polling for a
connector (during runtime suspend or detecting an HPD IRQ storm).
After the above change calling drm_kms_helper_poll_enable() is a nop
after it's been called already and polling for some connectors was
disabled/re-enabled.
Fix this by calling drm_kms_helper_poll_reschedule() added in the
previous patch instead, which reschedules the work whenever expected.
Fixes: d33a54e3991d ("drm/probe_helper: sort out poll_running vs poll_enabled")
Cc: Dmitry Baryshkov
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/display/intel_hotplug.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
b/drivers/gpu/drm/i915/display/intel_hotplug.c
index dd7eb9fc78610..d9f0ab1d953b9 100644
--- a/drivers/gpu/drm/i915/display/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
@@ -212,7 +212,7 @@ intel_hpd_irq_storm_switch_to_polling(struct
drm_i915_private *dev_priv)
/* Enable polling and queue hotplug re-enabling. */
if (hpd_disabled) {
- drm_kms_helper_poll_enable(&dev_priv->drm);
+ drm_kms_helper_poll_reschedule(&dev_priv->drm);
mod_delayed_work(dev_priv->unordered_wq,
&dev_priv->display.hotplug.reenable_work,
msecs_to_jiffies(HPD_STORM_REENABLE_DELAY));
@@ -676,7 +676,7 @@ static void i915_hpd_poll_init_work(struct work_struct
*work)
drm_connector_list_iter_end(&conn_iter);
if (enabled)
- drm_kms_helper_poll_enable(&dev_priv->drm);
+ drm_kms_helper_poll_reschedule(&dev_priv->drm);
mutex_unlock(&dev_priv->drm.mode_config.mutex);
--
2.37.2
[PATCH v2 2/5] drm: Add an HPD poll helper to reschedule the poll work
Add a helper to reschedule drm_mode_config::output_poll_work after
polling has been enabled for a connector (and needing a reschedule,
since previously polling was disabled for all connectors and hence
output_poll_work was not running).
This is needed by the next patch fixing HPD polling on i915.
Cc: Dmitry Baryshkov
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak
---
drivers/gpu/drm/drm_probe_helper.c | 68 --
include/drm/drm_probe_helper.h | 1 +
2 files changed, 47 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 2fb9bf901a2cc..3f479483d7d80 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -262,6 +262,26 @@ static bool drm_kms_helper_enable_hpd(struct drm_device
*dev)
}
#define DRM_OUTPUT_POLL_PERIOD (10*HZ)
+static void reschedule_output_poll_work(struct drm_device *dev)
+{
+ unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
+
+ if (dev->mode_config.delayed_event)
+ /*
+* FIXME:
+*
+* Use short (1s) delay to handle the initial delayed event.
+* This delay should not be needed, but Optimus/nouveau will
+* fail in a mysterious way if the delayed event is handled as
+* soon as possible like it is done in
+* drm_helper_probe_single_connector_modes() in case the poll
+* was enabled before.
+*/
+ delay = HZ;
+
+ schedule_delayed_work(&dev->mode_config.output_poll_work, delay);
+}
+
/**
* drm_kms_helper_poll_enable - re-enable output polling.
* @dev: drm_device
@@ -279,37 +299,41 @@ static bool drm_kms_helper_enable_hpd(struct drm_device
*dev)
*/
void drm_kms_helper_poll_enable(struct drm_device *dev)
{
- bool poll = false;
- unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
-
if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll ||
dev->mode_config.poll_running)
return;
- poll = drm_kms_helper_enable_hpd(dev);
-
- if (dev->mode_config.delayed_event) {
- /*
-* FIXME:
-*
-* Use short (1s) delay to handle the initial delayed event.
-* This delay should not be needed, but Optimus/nouveau will
-* fail in a mysterious way if the delayed event is handled as
-* soon as possible like it is done in
-* drm_helper_probe_single_connector_modes() in case the poll
-* was enabled before.
-*/
- poll = true;
- delay = HZ;
- }
-
- if (poll)
- schedule_delayed_work(&dev->mode_config.output_poll_work,
delay);
+ if (drm_kms_helper_enable_hpd(dev) ||
+ dev->mode_config.delayed_event)
+ reschedule_output_poll_work(dev);
dev->mode_config.poll_running = true;
}
EXPORT_SYMBOL(drm_kms_helper_poll_enable);
+/**
+ * drm_kms_helper_poll_reschedule - reschedule the output polling work
+ * @dev: drm_device
+ *
+ * This function reschedules the output polling work, after polling for a
+ * connector has been enabled.
+ *
+ * Drivers must call this helper after enabling polling for a connector by
+ * setting %DRM_CONNECTOR_POLL_CONNECT / %DRM_CONNECTOR_POLL_DISCONNECT flags
+ * in drm_connector::polled. Note that after disabling polling by clearing
these
+ * flags for a connector will stop the output polling work automatically if
+ * the polling is disabled for all other connectors as well.
+ *
+ * The function can be called only after polling has been enabled by calling
+ * drm_kms_helper_poll_init() / drm_kms_helper_poll_enable().
+ */
+void drm_kms_helper_poll_reschedule(struct drm_device *dev)
+{
+ if (dev->mode_config.poll_running)
+ reschedule_output_poll_work(dev);
+}
+EXPORT_SYMBOL(drm_kms_helper_poll_reschedule);
+
static enum drm_connector_status
drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
{
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 4977e0ab72dbb..fad3c4003b2b5 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -25,6 +25,7 @@ void drm_kms_helper_connector_hotplug_event(struct
drm_connector *connector);
void drm_kms_helper_poll_disable(struct drm_device *dev);
void drm_kms_helper_poll_enable(struct drm_device *dev);
+void drm_kms_helper_poll_reschedule(struct drm_device *dev);
bool drm_kms_helper_is_poll_worker(void);
enum drm_mode_status drm_crtc_helper_mode_valid_fixed(struct drm_crtc *crtc,
--
2.37.2
Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state
On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
wrote:
>
> On Thu, Jul 20, 2023 at 06:06:27AM +, Ying Liu wrote:
> > Bootloader may leave gpio direction as input and gpio value as logical low.
> > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > since the gpio value is literally logical low.
>
> To be honest this probably "hints" that the bootloader simply didn't
> consider the backlight at all :-) . I'd rather the patch description
> focus on what circumstances lead to the current code making a bad
> decision. More like:
>
> If the GPIO pin is in the input state but the backlight is currently
> off due to default pull downs then ...
>
> > So, let's drop output gpio
> > direction check and only check gpio value to set the initial power state.
>
> This check was specifically added by Bartosz so I'd be interested in his
> opinion of this change (especially since he is now a GPIO maintainer)!
>
> What motivates (or motivated) the need to check the direction rather
> than just read that current logic level on the pin?
>
>
> Daniel.
> [I'm done but since Bartosz and Linus were not on copy of the original
> thread I've left the rest of the patch below as a convenience ;-) ]
>
This was done in commit: 706dc68102bc ("backlight: gpio: Explicitly
set the direction of the GPIO").
Let me quote myself from it:
--
The GPIO backlight driver currently requests the line 'as is', without
actively setting its direction. This can lead to problems: if the line
is in input mode by default, we won't be able to drive it later when
updating the status and also reading its initial value doesn't make
sense for backlight setting.
--
I agree with Thomas that it's highly unlikely the bootloader "hints"
at any specific backlight settings. That being said, the change itself
looks correct to me. The other branch of that if will always unblank
the backlight if the GPIO is in input mode which may not be desirable.
I don't see any obvious problem with this change, just make sure the
commit message makes more sense.
Bartosz
Re: [PATCH v6 06/11] drm/mediatek: dp: Enable event interrupt only when bridge attached
On 17/07/2023 16:14, AngeloGioacchino Del Regno wrote: It is useless and error-prone to enable the DisplayPort event interrupt before finishing to probe and install the driver, as the DP training cannot happen before the entire pipeline is correctly set up, as the interrupt handler also requires the full hardware to be initialized by mtk_dp_bridge_attach(). Anyway, depending in which state the controller is left from the bootloader, this may cause an interrupt storm and consequently hang the kernel during boot, so, avoid enabling the interrupt until we reach a clean state by adding the IRQ_NOAUTOEN flag before requesting it at probe time and manage the enablement of the ISR in the .attach() and .detach() handlers for the DP bridge. Reviewed-by: Alexandre Mergnat -- Regards, Alexandre
Re: [PATCH v6 07/11] drm/mediatek: dp: Avoid mutex locks if audio is not supported/enabled
On 17/07/2023 16:14, AngeloGioacchino Del Regno wrote: If a controller (usually, eDP!) does not support audio, or audio is not enabled because the endpoint has no audio support, it's useless to lock a mutex only to unlock it right after because there's no .plugged_cb(). Check if the audio is supported and enabled before locking the mutex in mtk_dp_update_plugged_status(): if not, we simply return immediately. While at it, since the update_plugged_status_lock mutex would not be used if the controller doesn't support audio at all, initialize it only if `audio_supported` is true. Reviewed-by: Alexandre Mergnat -- Regards, Alexandre
Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state
On Thu, Jul 20, 2023 at 02:56:32PM +0200, Bartosz Golaszewski wrote:
> On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
> wrote:
> >
> > On Thu, Jul 20, 2023 at 06:06:27AM +, Ying Liu wrote:
> > > Bootloader may leave gpio direction as input and gpio value as logical
> > > low.
> > > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > > since the gpio value is literally logical low.
> >
> > To be honest this probably "hints" that the bootloader simply didn't
> > consider the backlight at all :-) . I'd rather the patch description
> > focus on what circumstances lead to the current code making a bad
> > decision. More like:
> >
> > If the GPIO pin is in the input state but the backlight is currently
> > off due to default pull downs then ...
> >
> > > So, let's drop output gpio
> > > direction check and only check gpio value to set the initial power state.
> >
> > This check was specifically added by Bartosz so I'd be interested in his
> > opinion of this change (especially since he is now a GPIO maintainer)!
> >
> > What motivates (or motivated) the need to check the direction rather
> > than just read that current logic level on the pin?
> >
> >
> > Daniel.
> > [I'm done but since Bartosz and Linus were not on copy of the original
> > thread I've left the rest of the patch below as a convenience ;-) ]
> >
>
> This was done in commit: 706dc68102bc ("backlight: gpio: Explicitly
> set the direction of the GPIO").
>
> Let me quote myself from it:
> --
> The GPIO backlight driver currently requests the line 'as is', without
> actively setting its direction. This can lead to problems: if the line
> is in input mode by default, we won't be able to drive it later when
> updating the status and also reading its initial value doesn't make
> sense for backlight setting.
> --
You are perhaps quoting the wrong bit here ;-). The currently proposed
patch leaves the code to put the pin into output mode unmodified. However
there was an extra line at the bottom of your commit message:
--
Also: check the current direction and only read the value if it's output.
--
This was the bit I wanted to check on, since the proposed patch
literally reverses this!
However...
> I agree with Thomas that it's highly unlikely the bootloader "hints"
> at any specific backlight settings. That being said, the change itself
> looks correct to me. The other branch of that if will always unblank
> the backlight if the GPIO is in input mode which may not be desirable.
... if you're happy the proposed change is OK then I'm happy too!
I came to the same conclusion after reviewing the GPIO code this morning,
however I copied you in because I was worried I might have overlooked
something.
> I don't see any obvious problem with this change, just make sure the
> commit message makes more sense.
Agreed.
Daniel.
Re: [PATCH] backlight: gpio_backlight: Drop output gpio direction check for initial power state
On Thu, Jul 20, 2023 at 3:10 PM Daniel Thompson
wrote:
>
> On Thu, Jul 20, 2023 at 02:56:32PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Jul 20, 2023 at 1:27 PM Daniel Thompson
> > wrote:
> > >
> > > On Thu, Jul 20, 2023 at 06:06:27AM +, Ying Liu wrote:
> > > > Bootloader may leave gpio direction as input and gpio value as logical
> > > > low.
> > > > It hints that initial backlight power state should be FB_BLANK_POWERDOWN
> > > > since the gpio value is literally logical low.
> > >
> > > To be honest this probably "hints" that the bootloader simply didn't
> > > consider the backlight at all :-) . I'd rather the patch description
> > > focus on what circumstances lead to the current code making a bad
> > > decision. More like:
> > >
> > > If the GPIO pin is in the input state but the backlight is currently
> > > off due to default pull downs then ...
> > >
> > > > So, let's drop output gpio
> > > > direction check and only check gpio value to set the initial power
> > > > state.
> > >
> > > This check was specifically added by Bartosz so I'd be interested in his
> > > opinion of this change (especially since he is now a GPIO maintainer)!
> > >
> > > What motivates (or motivated) the need to check the direction rather
> > > than just read that current logic level on the pin?
> > >
> > >
> > > Daniel.
> > > [I'm done but since Bartosz and Linus were not on copy of the original
> > > thread I've left the rest of the patch below as a convenience ;-) ]
> > >
> >
> > This was done in commit: 706dc68102bc ("backlight: gpio: Explicitly
> > set the direction of the GPIO").
> >
> > Let me quote myself from it:
> > --
> > The GPIO backlight driver currently requests the line 'as is', without
> > actively setting its direction. This can lead to problems: if the line
> > is in input mode by default, we won't be able to drive it later when
> > updating the status and also reading its initial value doesn't make
> > sense for backlight setting.
> > --
>
> You are perhaps quoting the wrong bit here ;-). The currently proposed
> patch leaves the code to put the pin into output mode unmodified. However
> there was an extra line at the bottom of your commit message:
> --
> Also: check the current direction and only read the value if it's output.
> --
Yeah I'm no longer sure why I did this. The commit doesn't look harmful though.
Bart
>
> This was the bit I wanted to check on, since the proposed patch
> literally reverses this!
>
> However...
>
>
> > I agree with Thomas that it's highly unlikely the bootloader "hints"
> > at any specific backlight settings. That being said, the change itself
> > looks correct to me. The other branch of that if will always unblank
> > the backlight if the GPIO is in input mode which may not be desirable.
>
> ... if you're happy the proposed change is OK then I'm happy too!
> I came to the same conclusion after reviewing the GPIO code this morning,
> however I copied you in because I was worried I might have overlooked
> something.
>
>
> > I don't see any obvious problem with this change, just make sure the
> > commit message makes more sense.
>
> Agreed.
>
>
> Daniel.
