Re: [PATCH v5] backlight: lp855x: Switch to atomic PWM API
On Wed, Nov 03, 2021 at 02:38:05PM -0300, Maíra Canal wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
>
> Signed-off-by: Maíra Canal
> ---
> V1 -> V2: Initialize variable and simplify conditional loop
> V2 -> V3: Fix assignment of NULL variable
> V3 -> V4: Replace division for pwm_set_relative_duty_cycle
> V4 -> V5: Fix overwrite of state.period
> ---
> drivers/video/backlight/lp855x_bl.c | 21 +
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/backlight/lp855x_bl.c
> b/drivers/video/backlight/lp855x_bl.c
> index e94932c69f54..e70a7b72dcf3 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
>
> static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> {
> - unsigned int period = lp->pdata->period_ns;
> - unsigned int duty = br * period / max_br;
> struct pwm_device *pwm;
> + struct pwm_state state;
>
> /* request pwm device with the consumer name */
> if (!lp->pwm) {
> @@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br,
> int max_br)
>
> lp->pwm = pwm;
>
> - /*
> - * FIXME: pwm_apply_args() should be removed when switching to
> - * the atomic PWM API.
> - */
> - pwm_apply_args(pwm);
> + pwm_init_state(lp->pwm, &state);
> + state.period = lp->pdata->period_ns;
> + } else {
> + pwm_get_state(lp->pwm, &state);
> }
>
> - pwm_config(lp->pwm, duty, period);
> - if (duty)
> - pwm_enable(lp->pwm);
> - else
> - pwm_disable(lp->pwm);
> + pwm_set_relative_duty_cycle(&state, br, max_br);
> + state.enabled = state.duty_cycle;
> +
> + pwm_apply_state(lp->pwm, &state);
Looks mostly right, but only on a deeper look into the driver. The
reason is that in the unmodified code there is always explicitly
period=lp->pdata->period_ns; while after your change the period is unset
(and so the previously set period is used).
So either mention in the change log that this driver doesn't modify the
pwm settings in other places or preferably pick an equivalent conversion
(plus maybe an optimisation in a separate patch).
If you go the "equivalent conversion" path, please note that
pwm_set_relative_duty_cycle() isn't equivalent to br * period / max_br,
as it implements a different rounding.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH v9 10/10] drm: use DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES bitmap to tracefs
On Wed, Nov 3, 2021 at 9:58 AM Jason Baron wrote:
>
>
>
> On 10/27/21 12:36 AM, Jim Cromie wrote:
> > Use new macro to create a sysfs control bitmap knob to control
> > print-to-trace in: /sys/module/drm/parameters/trace
> >
> > todo: reconsider this api, ie a single macro expecting both debug &
> > trace terms (2 each), followed by a single description and the
> > bitmap-spec::
> >
> > Good: declares bitmap once for both interfaces
> >
> > Bad: arg-type/count handling (expecting 4 args) is ugly,
> > especially preceding the bitmap-init var-args.
> >
>
> Hi Jim,
>
> I agree having the bitmap declared twice seems redundant. But I like having
> fewer args and not necessarily combining the trace/log variants of
> DEBUG_CATEGORIES. hmmm...what if the DEFINE_DYNAMIC_DEBUG_CATEGORIES() took a
> pointer to the array of struct dyndbg_bitdesc map[] directly as the
> final argument instead of the __VA_ARGS__? Then, we could just declare the
> map once?
>
indeed. that seems obvious in retrospect,
thanks for the nudge.
also, Im inclined to (uhm, have now done) bikeshed the API in patch 1,
and change _CATEGORIES to something else,
maybe _FMTGRPS
or _BITGRPS < -- this one
ISTM better to be explicit wrt the underlying mechanisms, (least surprise)
let users decide the meaning of "CATEGORIES"
also, HEAD~1 added DEFINE_DYNAMIC_DEBUG_CATEGORIES_FLAGS
which could be used directly for both purposes (after a rename).
TLDR: flags exposes the shared nature of the decorator flags,
the trace and syslog customers of pr_debug traffic should agree on their use.
redoing now...
> Thanks,
>
> -Jason
>
> > Signed-off-by: Jim Cromie
> > ---
> > drivers/gpu/drm/drm_print.c | 19 +++
> > 1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index ce662d0f339b..7b49fbc5e21d 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -73,6 +73,25 @@ DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
static mumble-map
> > [7] = { DRM_DBG_CAT_LEASE },
> > [8] = { DRM_DBG_CAT_DP },
> > [9] = { DRM_DBG_CAT_DRMRES });
> > +
> > +#ifdef CONFIG_TRACING
> > +unsigned long __drm_trace;
> > +EXPORT_SYMBOL(__drm_trace);
> > +DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES(trace, __drm_trace,
> > + DRM_DEBUG_DESC,
mumble-map)
[PATCH v12 4/4] drm/bridge: anx7625: add HDMI audio function
Add audio HDMI codec function support, enable it through device true
flag "analogix,audio-enable".
Reviewed-by: Robert Foss
Signed-off-by: Xin Ji
---
drivers/gpu/drm/bridge/analogix/anx7625.c | 226 ++
drivers/gpu/drm/bridge/analogix/anx7625.h | 5 +
2 files changed, 231 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index f7c3386c8929..001fb39d9919 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -33,6 +33,7 @@
#include
#include
+#include
#include
#include "anx7625.h"
@@ -153,6 +154,20 @@ static int anx7625_write_and(struct anx7625_data *ctx,
return anx7625_reg_write(ctx, client, offset, (val & (mask)));
}
+static int anx7625_write_and_or(struct anx7625_data *ctx,
+ struct i2c_client *client,
+ u8 offset, u8 and_mask, u8 or_mask)
+{
+ int val;
+
+ val = anx7625_reg_read(ctx, client, offset);
+ if (val < 0)
+ return val;
+
+ return anx7625_reg_write(ctx, client,
+offset, (val & and_mask) | (or_mask));
+}
+
static int anx7625_config_bit_matrix(struct anx7625_data *ctx)
{
int i, ret;
@@ -1353,6 +1368,9 @@ static int anx7625_parse_dt(struct device *dev,
else
DRM_DEV_DEBUG_DRIVER(dev, "found MIPI DSI host node.\n");
+ if (of_property_read_bool(np, "analogix,audio-enable"))
+ pdata->audio_en = 1;
+
ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
if (ret < 0) {
if (ret == -ENODEV)
@@ -1423,6 +1441,208 @@ static enum drm_connector_status
anx7625_sink_detect(struct anx7625_data *ctx)
connector_status_disconnected;
}
+static int anx7625_audio_hw_params(struct device *dev, void *data,
+ struct hdmi_codec_daifmt *fmt,
+ struct hdmi_codec_params *params)
+{
+ struct anx7625_data *ctx = dev_get_drvdata(dev);
+ int wl, ch, rate;
+ int ret = 0;
+
+ if (fmt->fmt != HDMI_DSP_A) {
+ DRM_DEV_ERROR(dev, "only supports DSP_A\n");
+ return -EINVAL;
+ }
+
+ DRM_DEV_DEBUG_DRIVER(dev, "setting %d Hz, %d bit, %d channels\n",
+params->sample_rate, params->sample_width,
+params->cea.channels);
+
+ ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
+ AUDIO_CHANNEL_STATUS_6,
+ ~I2S_SLAVE_MODE,
+ TDM_SLAVE_MODE);
+
+ /* Word length */
+ switch (params->sample_width) {
+ case 16:
+ wl = AUDIO_W_LEN_16_20MAX;
+ break;
+ case 18:
+ wl = AUDIO_W_LEN_18_20MAX;
+ break;
+ case 20:
+ wl = AUDIO_W_LEN_20_20MAX;
+ break;
+ case 24:
+ wl = AUDIO_W_LEN_24_24MAX;
+ break;
+ default:
+ DRM_DEV_DEBUG_DRIVER(dev, "wordlength: %d bit not support",
+params->sample_width);
+ return -EINVAL;
+ }
+ ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
+ AUDIO_CHANNEL_STATUS_5,
+ 0xf0, wl);
+
+ /* Channel num */
+ switch (params->cea.channels) {
+ case 2:
+ ch = I2S_CH_2;
+ break;
+ case 4:
+ ch = TDM_CH_4;
+ break;
+ case 6:
+ ch = TDM_CH_6;
+ break;
+ case 8:
+ ch = TDM_CH_8;
+ break;
+ default:
+ DRM_DEV_DEBUG_DRIVER(dev, "channel number: %d not support",
+params->cea.channels);
+ return -EINVAL;
+ }
+ ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
+ AUDIO_CHANNEL_STATUS_6, 0x1f, ch << 5);
+ if (ch > I2S_CH_2)
+ ret |= anx7625_write_or(ctx, ctx->i2c.tx_p2_client,
+ AUDIO_CHANNEL_STATUS_6, AUDIO_LAYOUT);
+ else
+ ret |= anx7625_write_and(ctx, ctx->i2c.tx_p2_client,
+ AUDIO_CHANNEL_STATUS_6, ~AUDIO_LAYOUT);
+
+ /* FS */
+ switch (params->sample_rate) {
+ case 32000:
+ rate = AUDIO_FS_32K;
+ break;
+ case 44100:
+ rate = AUDIO_FS_441K;
+ break;
+ case 48000:
+ rate = AUDIO_FS_48K;
+ break;
+ case 88200:
+ rate = AUDIO_FS_882K;
+ break;
+ case 96000:
+ rate = AUDIO_FS_96K;
+ break;
+ case 1
[PATCH v12 3/4] drm/bridge: anx7625: add MIPI DPI input feature
The basic anx7625 driver only support MIPI DSI rx signal input.
This patch add MIPI DPI rx input configuration support, after apply
this patch, the driver can support DSI rx or DPI rx by adding
'bus-type' in DT.
Reviewed-by: Robert Foss
Signed-off-by: Xin Ji
---
drivers/gpu/drm/bridge/analogix/anx7625.c | 247 --
drivers/gpu/drm/bridge/analogix/anx7625.h | 18 +-
2 files changed, 205 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index f48e91134c20..f7c3386c8929 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -32,6 +32,7 @@
#include
#include
+#include
#include
#include "anx7625.h"
@@ -152,18 +153,18 @@ static int anx7625_write_and(struct anx7625_data *ctx,
return anx7625_reg_write(ctx, client, offset, (val & (mask)));
}
-static int anx7625_write_and_or(struct anx7625_data *ctx,
- struct i2c_client *client,
- u8 offset, u8 and_mask, u8 or_mask)
+static int anx7625_config_bit_matrix(struct anx7625_data *ctx)
{
- int val;
+ int i, ret;
- val = anx7625_reg_read(ctx, client, offset);
- if (val < 0)
- return val;
+ ret = anx7625_write_or(ctx, ctx->i2c.tx_p2_client,
+ AUDIO_CONTROL_REGISTER, 0x80);
+ for (i = 0; i < 13; i++)
+ ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
+VIDEO_BIT_MATRIX_12 + i,
+0x18 + i);
- return anx7625_reg_write(ctx, client,
-offset, (val & and_mask) | (or_mask));
+ return ret;
}
static int anx7625_read_ctrl_status_p0(struct anx7625_data *ctx)
@@ -221,38 +222,6 @@ static int anx7625_video_mute_control(struct anx7625_data
*ctx,
return ret;
}
-static int anx7625_config_audio_input(struct anx7625_data *ctx)
-{
- struct device *dev = &ctx->client->dev;
- int ret;
-
- /* Channel num */
- ret = anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
- AUDIO_CHANNEL_STATUS_6, I2S_CH_2 << 5);
-
- /* FS */
- ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
- AUDIO_CHANNEL_STATUS_4,
- 0xf0, AUDIO_FS_48K);
- /* Word length */
- ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
- AUDIO_CHANNEL_STATUS_5,
- 0xf0, AUDIO_W_LEN_24_24MAX);
- /* I2S */
- ret |= anx7625_write_or(ctx, ctx->i2c.tx_p2_client,
- AUDIO_CHANNEL_STATUS_6, I2S_SLAVE_MODE);
- ret |= anx7625_write_and(ctx, ctx->i2c.tx_p2_client,
-AUDIO_CONTROL_REGISTER, ~TDM_TIMING_MODE);
- /* Audio change flag */
- ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
- AP_AV_STATUS, AP_AUDIO_CHG);
-
- if (ret < 0)
- DRM_DEV_ERROR(dev, "fail to config audio.\n");
-
- return ret;
-}
-
/* Reduction of fraction a/b */
static void anx7625_reduction_of_a_fraction(unsigned long *a, unsigned long *b)
{
@@ -431,7 +400,7 @@ static int anx7625_dsi_video_timing_config(struct
anx7625_data *ctx)
ret |= anx7625_write_and(ctx, ctx->i2c.rx_p1_client,
MIPI_LANE_CTRL_0, 0xfc);
ret |= anx7625_write_or(ctx, ctx->i2c.rx_p1_client,
- MIPI_LANE_CTRL_0, 3);
+ MIPI_LANE_CTRL_0, ctx->pdata.mipi_lanes - 1);
/* Htotal */
htotal = ctx->dt.hactive.min + ctx->dt.hfront_porch.min +
@@ -615,6 +584,76 @@ static int anx7625_dsi_config(struct anx7625_data *ctx)
return ret;
}
+static int anx7625_api_dpi_config(struct anx7625_data *ctx)
+{
+ struct device *dev = &ctx->client->dev;
+ u16 freq = ctx->dt.pixelclock.min / 1000;
+ int ret;
+
+ /* configure pixel clock */
+ ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
+ PIXEL_CLOCK_L, freq & 0xFF);
+ ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
+PIXEL_CLOCK_H, (freq >> 8));
+
+ /* set DPI mode */
+ /* set to DPI PLL module sel */
+ ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
+MIPI_DIGITAL_PLL_9, 0x20);
+ /* power down MIPI */
+ ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
+MIPI_LANE_CTRL_10, 0x08);
+ /* enable DPI mode */
+ ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
+MIPI_DIGITAL_PLL_18, 0x1C);
+ /* set first edge */
+ ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
+
Re: [PATCH v7 1/3] drm/dsi: transer dsi hs packet aligned
Hi sirs
Pls help to review this change.
Best Regards
Jitao.
On Tue, 2021-10-05 at 07:53 +0800, Chun-Kuang Hu wrote:
> Hi, Jitao:
>
> Jitao Shi 於 2021年9月16日 週四 上午6:31寫道：
> >
> > Some DSI devices reqire the hs packet starting and ending
> > at same time on all dsi lanes. So use a flag to those devices.
> >
>
> Reviewed-by: Chun-Kuang Hu
>
> > Signed-off-by: Jitao Shi
> > ---
> > include/drm/drm_mipi_dsi.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/include/drm/drm_mipi_dsi.h
> > b/include/drm/drm_mipi_dsi.h
> > index af7ba8071eb0..8e8563792682 100644
> > --- a/include/drm/drm_mipi_dsi.h
> > +++ b/include/drm/drm_mipi_dsi.h
> > @@ -177,6 +177,7 @@ struct mipi_dsi_device_info {
> > * @lp_rate: maximum lane frequency for low power mode in hertz,
> > this should
> > * be set to the real limits of the hardware, zero is only
> > accepted for
> > * legacy drivers
> > + * @hs_packet_end_aligned: transfer dsi hs packet ending aligned
> > */
> > struct mipi_dsi_device {
> > struct mipi_dsi_host *host;
> > @@ -189,6 +190,7 @@ struct mipi_dsi_device {
> > unsigned long mode_flags;
> > unsigned long hs_rate;
> > unsigned long lp_rate;
> > + bool hs_packet_end_aligned;
> > };
> >
> > #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
> > --
> > 2.25.1
[PATCH v12 2/4] drm/bridge: anx7625: fix not correct return value
At some time, the original code may return non zero value, force return 0
if operation finished.
Reviewed-by: Robert Foss
Signed-off-by: Xin Ji
---
drivers/gpu/drm/bridge/analogix/anx7625.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index d0317651cd75..f48e91134c20 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -191,10 +191,10 @@ static int wait_aux_op_finish(struct anx7625_data *ctx)
AP_AUX_CTRL_STATUS);
if (val < 0 || (val & 0x0F)) {
DRM_DEV_ERROR(dev, "aux status %02x\n", val);
- val = -EIO;
+ return -EIO;
}
- return val;
+ return 0;
}
static int anx7625_video_mute_control(struct anx7625_data *ctx,
--
2.25.1
[PATCH v12 1/4] dt-bindings:drm/bridge:anx7625:add vendor define
Add 'bus-type' and 'data-lanes' define for port0. Add DP tx lane0,
lane1 swing register setting array, and audio enable flag.
The device which cannot pass DP tx PHY CTS caused by long PCB trace or
embedded MUX, adjusting ANX7625 PHY parameters can pass the CTS test. The
adjusting type include Pre-emphasis, Vp-p, Rterm(Resistor Termination)
and Rsel(Driven Strength). Each lane has maximum 20 registers for
these settings.
Signed-off-by: Xin Ji
Reviewed-by: Rob Herring
---
.../display/bridge/analogix,anx7625.yaml | 65 ++-
1 file changed, 63 insertions(+), 2 deletions(-)
diff --git
a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
index ab48ab2f4240..1d3e88daca04 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
@@ -43,14 +43,70 @@ properties:
vdd33-supply:
description: Regulator that provides the supply 3.3V power.
+ analogix,lane0-swing:
+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 1
+maxItems: 20
+description:
+ an array of swing register setting for DP tx lane0 PHY.
+ Registers 0~9 are Swing0_Pre0, Swing1_Pre0, Swing2_Pre0,
+ Swing3_Pre0, Swing0_Pre1, Swing1_Pre1, Swing2_Pre1, Swing0_Pre2,
+ Swing1_Pre2, Swing0_Pre3, they are for [Boost control] and
+ [Swing control] setting.
+ Registers 0~9, bit 3:0 is [Boost control], these bits control
+ post cursor manual, increase the [Boost control] to increase
+ Pre-emphasis value.
+ Registers 0~9, bit 6:4 is [Swing control], these bits control
+ swing manual, increase [Swing control] setting to add Vp-p value
+ for each Swing, Pre.
+ Registers 10~19 are Swing0_Pre0, Swing1_Pre0, Swing2_Pre0,
+ Swing3_Pre0, Swing0_Pre1, Swing1_Pre1, Swing2_Pre1, Swing0_Pre2,
+ Swing1_Pre2, Swing0_Pre3, they are for [R select control] and
+ [R Termination control] setting.
+ Registers 10~19, bit 4:0 is [R select control], these bits are
+ compensation manual, increase it can enhance IO driven strength
+ and Vp-p.
+ Registers 10~19, bit 5:6 is [R termination control], these bits
+ adjust 50ohm impedance of DP tx termination. 00:55 ohm,
+ 01:50 ohm(default), 10:45 ohm, 11:40 ohm.
+
+ analogix,lane1-swing:
+$ref: /schemas/types.yaml#/definitions/uint8-array
+minItems: 1
+maxItems: 20
+description:
+ an array of swing register setting for DP tx lane1 PHY.
+ DP TX lane1 swing register setting same with lane0
+ swing, please refer lane0-swing property description.
+
+ analogix,audio-enable:
+type: boolean
+description: let the driver enable audio HDMI codec function or not.
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
properties:
port@0:
-$ref: /schemas/graph.yaml#/properties/port
+$ref: /schemas/graph.yaml#/$defs/port-base
+unevaluatedProperties: false
description:
- Video port for MIPI DSI input.
+ MIPI DSI/DPI input.
+
+properties:
+ endpoint:
+$ref: /schemas/media/video-interfaces.yaml#
+type: object
+additionalProperties: false
+
+properties:
+ remote-endpoint: true
+
+ bus-type:
+enum: [1, 5]
+default: 1
+
+ data-lanes: true
port@1:
$ref: /schemas/graph.yaml#/properties/port
@@ -87,6 +143,9 @@ examples:
vdd10-supply = <&pp1000_mipibrdg>;
vdd18-supply = <&pp1800_mipibrdg>;
vdd33-supply = <&pp3300_mipibrdg>;
+analogix,audio-enable;
+analogix,lane0-swing = /bits/ 8 <0x14 0x54 0x64 0x74>;
+analogix,lane1-swing = /bits/ 8 <0x14 0x54 0x64 0x74>;
ports {
#address-cells = <1>;
@@ -96,6 +155,8 @@ examples:
reg = <0>;
anx7625_in: endpoint {
remote-endpoint = <&mipi_dsi>;
+bus-type = <5>;
+data-lanes = <0 1 2 3>;
};
};
--
2.25.1
[PATCH v12 0/4] Add MIPI rx DPI support
Hi all, this patch series implement MIPI rx DPI feature. Please help to review. This is the v12 version, rebase all patches on the drm-misc-next. Any mistakes, please let me know, I'll fix it in the next series. Change history: v12: Fix Robert Foss comment - Apply code on drm-misc-next branch v11: Fix Rob Herring comment - Move swing register description in property. - Remove additional property. v10: Fix Rob Herring and Laurent Pinchart comments - Add more description about lane swing configuration in commit message. v9: Fix Neil Amstrong comment - use macro define 'V4L2_FWNODE_BUS_TYPE_PARALLEL' instead of fixing value. v8: Fix Laurent Pinchart comment - Expand the commit message. v7: - Rebase DT on the latest branch 'drm-misc-next'. - Remove HDCP patch. v6: Fix kernel robot compile warning v5: Fix Rob Herring, Hsin-Yi, Robert Foss comments - Rebase code on the branch 'drm-misc-next', refer video-interfaces.yaml - Seprate HDCP function to a new patch - Fix driver not correctly get 'bus-type' 'data-lanes' - Add audio HDMI codec function support v4: Fix Rob Herring comment - Rebase code on the branch 'drm-misc-next' - Change 'analogix,hdcp-support' type to boolean v3: Fix Rob Herring, Dan Carpenter, Nicolas comment - Split the patch, fix not correct return data - Fix several coding format - Split DP tx swing register setting to two property - Add HDCP support vender flag - remove 'analogix,swing-setting' and 'analogix,mipi-dpi-in' property v2: Fix Rob Herring comment - Fix yamllint warnings/errors in analogix,anx7625.yaml - Fix kernel robot compile warning v1: initial MIPI rx DPI feature support Xin Ji (4): dt-bindings:drm/bridge:anx7625:add vendor define drm/bridge: anx7625: fix not correct return value drm/bridge: anx7625: add MIPI DPI input feature drm/bridge: anx7625: add HDMI audio function .../display/bridge/analogix,anx7625.yaml | 65 ++- drivers/gpu/drm/bridge/analogix/anx7625.c | 459 -- drivers/gpu/drm/bridge/analogix/anx7625.h | 23 +- 3 files changed, 492 insertions(+), 55 deletions(-) -- 2.25.1
[pull] amdgpu, amdkfd drm-fixes-5.16
Hi Dave, Daniel, Fixes for 5.16. The following changes since commit d9bd054177fbd2c4762546aec40fc3071bfe4cc0: Merge tag 'amd-drm-next-5.16-2021-10-29' of https://gitlab.freedesktop.org/agd5f/linux into drm-next (2021-11-02 12:40:58 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-fixes-5.16-2021-11-03 for you to fetch changes up to 78469728809b8604dc37ae4e6b12ae12decac5be: drm/amd/display: 3.2.160 (2021-11-03 12:32:34 -0400) amd-drm-fixes-5.16-2021-11-03: amdgpu: - GPU reset fix - Aldebaran fix - Yellow Carp fixes - DCN2.1 DMCUB fix - IOMMU regression fix for Picasso - DSC display fixes - BPC display calculation fixes - Other misc display fixes amdkfd: - SVM fixes - Fix gfx version for renoir Aaron Liu (1): drm/amdgpu: update RLC_PG_DELAY_3 Value to 200us for yellow carp Anson Jacob (1): drm/amd/display: Fix dcn10_log_hubp_states printf format string Anthony Koo (1): drm/amd/display: [FW Promotion] Release 0.0.91 Aric Cyr (1): drm/amd/display: 3.2.160 Aurabindo Pillai (1): drm/amd/display: add condition check for dmub notification Bing Guo (1): drm/amd/display: Fix bpc calculation for specific encodings Felipe Clark (1): drm/amd/display: Fix dummy p-state hang on monitors with extreme timing Felix Kuehling (3): drm/amdkfd: Fix SVM_ATTR_PREFERRED_LOC drm/amdkfd: Avoid thrashing of stack and heap drm/amdkfd: Handle incomplete migration to system memory Graham Sider (1): drm/amdkfd: update gfx target version for Renoir Hersen Wu (1): drm/amd/display: dsc engine not disabled after unplug dsc mst hub Jake Wang (3): drm/amd/display: Added HPO HW control shutdown support drm/amd/display: Add MPC meory shutdown support drm/amd/display: Added new DMUB boot option for power optimization James Zhu (1): drm/amdgpu: remove duplicated kfd_resume_iommu Jimmy Kizito (1): drm/amd/display: Clear encoder assignments when state cleared. Jingwen Chen (1): drm/amd/amdgpu: fix bad job hw_fence use after free in advance tdr Mario Limonciello (6): drm/amdgpu: Convert SMU version to decimal in debugfs drm/amdgpu/pm: drop pp_power_profile_mode support for yellow carp drm/amd/pm: Add missing mutex for pp_get_power_profile_mode drm/amd/pm: Adjust returns when power_profile_mode is not supported drm/amdgpu/pm: Don't show pp_power_profile_mode for unsupported devices drm/amd/display: Look at firmware version to determine using dmub on dcn21 Oak Zeng (1): drm/amdgpu: use correct register mask to extract field Roman Li (1): drm/amd/display: Force disable planes on any pipe split change Wenjing Liu (1): drm/amd/display: fix register write sequence for LINK_SQUARE_PATTERN Yu-ting Shen (1): drm/amd/display: avoid link loss short pulse stuck the system drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 +- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c| 9 +- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 5 +- drivers/gpu/drm/amd/amdgpu/gfxhub_v1_1.c | 18 ++- drivers/gpu/drm/amd/amdkfd/kfd_device.c| 2 +- drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 45 +-- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 44 -- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 -- .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c| 150 - drivers/gpu/drm/amd/display/dc/core/dc.c | 8 +- drivers/gpu/drm/amd/display/dc/core/dc_link.c | 2 + drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c | 8 ++ .../gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c | 22 +++ drivers/gpu/drm/amd/display/dc/dc.h| 3 +- drivers/gpu/drm/amd/display/dc/dc_dp_types.h | 3 + drivers/gpu/drm/amd/display/dc/dce/dce_hwseq.h | 4 +- .../amd/display/dc/dce110/dce110_hw_sequencer.c| 6 + .../drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 2 +- drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c | 3 + drivers/gpu/drm/amd/display/dc/dcn30/dcn30_mpc.c | 7 +- .../gpu/drm/amd/display/dc/dcn30/dcn30_resource.c | 7 +- drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.c | 78 +++ drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hwseq.h | 1 + drivers/gpu/drm/amd/display/dc/dcn31/dcn31_init.c | 1 + .../gpu/drm/amd/display/dc/dcn31/dcn31_resource.c | 6 +- .../amd/display/dc/dml/dcn30/display_mode_vba_30.c | 13 +- .../amd/display/dc/dml/dcn31/display_mode_vba_31.c | 14 +- drivers/gpu/drm/amd/display/dc/inc/hw/mpc.h| 1 + .../drm/amd/display/dc/inc/hw_sequencer_private.h | 1 + drivers/gpu/drm/amd/display/dmub/dmub_srv.h| 1 + drivers/gpu/drm/amd/display/dmub/inc/dmub_cmd.h| 4 +- drivers/gpu/dr
Re: [PATCH v11 4/4] drm/bridge: anx7625: add HDMI audio function
On Wed, Nov 03, 2021 at 04:04:00PM +0100, Robert Foss wrote: > Hey Xin, > > This series does not apply on drm-misc-next. Please fix this and > resend. Make sure that checkpatch --strict passes as well. OK, I'll apply on drm-misc-next, thanks! Xin > > On Wed, 3 Nov 2021 at 15:20, Dan Carpenter wrote: > > > > This is a super awkward way to resend a patch series. Next time, just > > start a new thread and put [PATCH RESEND] in the subject. > > > > I am sorry that no one responded to your thread. :/ > > > > regards, > > dan carpenter
Re: [PATCH v11 4/4] drm/bridge: anx7625: add HDMI audio function
On Wed, Nov 03, 2021 at 05:20:03PM +0300, Dan Carpenter wrote: > This is a super awkward way to resend a patch series. Next time, just > start a new thread and put [PATCH RESEND] in the subject. OK, thanks! Xin > > I am sorry that no one responded to your thread. :/ > > regards, > dan carpenter
Re: [PATCH 3/3] drm/i915/guc/slpc: Update boost sysfs hooks for SLPC
On Mon, 01 Nov 2021 18:26:08 -0700, Vinay Belgaumkar wrote: > > Add a helper to sort through the SLPC/RPS paths of get/set methods. > Boost frequency will be modified as long as it is within the constraints > of RP0 and if it is different from the existing one. We will set min > freq to boost only if there is at least one active waiter. > > v2: Add num_boosts to guc_slpc_info and changes for worker function > v3: Review comments (Ashutosh) Reviewed-by: Ashutosh Dixit
Re: [PATCH 3/3] drm/i915/guc/slpc: Update boost sysfs hooks for SLPC
On Mon, 01 Nov 2021 13:28:14 -0700, Dixit, Ashutosh wrote: > > On Sun, 31 Oct 2021 21:39:37 -0700, Belgaumkar, Vinay wrote: > > > > +static int set_boost_freq(struct intel_rps *rps, u32 val) > > Since this is legacy rps code path maybe change function name to > rps_set_boost_freq? Not being able to find v3 of this patch so giving a R-b on v2 but the R-b applies to v3: Reviewed-by: Ashutosh Dixit
[PATCH] drm/msm/mdp5: drop vdd regulator
The "vdd" regulator was used by the mdp5 driver only on downstream
kernels, where the GDSC is represented as a regulator. On all current
kernels the MDSS_GDSC is implemented as the power domain, removing the
need for this regulator. Remove it from the mdp5 driver.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c | 24 ++-
1 file changed, 2 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
index 2f4895bcb0b0..2ac8fd37c76b 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_mdss.c
@@ -16,8 +16,6 @@ struct mdp5_mdss {
void __iomem *mmio, *vbif;
- struct regulator *vdd;
-
struct clk *ahb_clk;
struct clk *axi_clk;
struct clk *vsync_clk;
@@ -189,8 +187,6 @@ static void mdp5_mdss_destroy(struct drm_device *dev)
irq_domain_remove(mdp5_mdss->irqcontroller.domain);
mdp5_mdss->irqcontroller.domain = NULL;
- regulator_disable(mdp5_mdss->vdd);
-
pm_runtime_disable(dev->dev);
}
@@ -238,31 +234,17 @@ int mdp5_mdss_init(struct drm_device *dev)
goto fail;
}
- /* Regulator to enable GDSCs in downstream kernels */
- mdp5_mdss->vdd = devm_regulator_get(dev->dev, "vdd");
- if (IS_ERR(mdp5_mdss->vdd)) {
- ret = PTR_ERR(mdp5_mdss->vdd);
- goto fail;
- }
-
- ret = regulator_enable(mdp5_mdss->vdd);
- if (ret) {
- DRM_DEV_ERROR(dev->dev, "failed to enable regulator vdd: %d\n",
- ret);
- goto fail;
- }
-
ret = devm_request_irq(dev->dev, platform_get_irq(pdev, 0),
mdss_irq, 0, "mdss_isr", mdp5_mdss);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to init irq: %d\n", ret);
- goto fail_irq;
+ goto fail;
}
ret = mdss_irq_domain_init(mdp5_mdss);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to init sub-block irqs: %d\n",
ret);
- goto fail_irq;
+ goto fail;
}
mdp5_mdss->base.funcs = &mdss_funcs;
@@ -271,8 +253,6 @@ int mdp5_mdss_init(struct drm_device *dev)
pm_runtime_enable(dev->dev);
return 0;
-fail_irq:
- regulator_disable(mdp5_mdss->vdd);
fail:
return ret;
}
--
2.33.0
Re: [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality for SLPC
On Mon, 01 Nov 2021 18:26:07 -0700, Vinay Belgaumkar wrote: > > Add helper in RPS code for handling SLPC and non-SLPC paths. > When boost is requested in the SLPC path, we can ask GuC to ramp > up the frequency req by setting the minimum frequency to boost freq. > Reset freq back to the min softlimit when there are no more waiters. > > v2: Schedule a worker thread which can boost freq from within > an interrupt context as well. > > v3: No need to check against requested freq before scheduling boost > work (Ashutosh) Reviewed-by: Ashutosh Dixit
Re: [PATCH 1/3] drm/i915/guc/slpc: Define and initialize boost frequency
On Mon, 01 Nov 2021 18:26:06 -0700, Vinay Belgaumkar wrote: > > Define helpers and struct members required to record boost info. > Boost frequency is initialized to RP0 at SLPC init. Also define num_waiters > which can track the pending boost requests. > > Boost will be done by scheduling a worker thread. This will avoid > the need to make H2G calls inside an interrupt context. Initialize the > worker function during SLPC init as well. Had to move intel_guc_slpc_init > a few lines below to accomodate this. > > v2: Add a workqueue to handle waitboost > v3: Code review comments (Ashutosh) Reviewed-by: Ashutosh Dixit
[PATCH] Revert "drm/imx: Annotate dma-fence critical section in commit path"
This reverts commit f4b34faa08428d813fc3629f882c503487f94a12.
Since commit f4b34faa0842 ("drm/imx: Annotate dma-fence critical section in
commit path") the following possible circular dependency is detected:
[5.001811] ==
[5.001817] WARNING: possible circular locking dependency detected
[5.001824] 5.14.9-01225-g45da36cc6fcc-dirty #1 Tainted: GW
[5.001833] --
[5.001838] kworker/u8:0/7 is trying to acquire lock:
[5.001848] c1752080 (regulator_list_mutex){+.+.}-{3:3}, at:
regulator_lock_dependent+0x40/0x294
[5.001903]
[5.001903] but task is already holding lock:
[5.001909] c176df78 (dma_fence_map){}-{0:0}, at:
imx_drm_atomic_commit_tail+0x10/0x160
[5.001957]
[5.001957] which lock already depends on the new lock.
...
Revert it for now.
Tested on a imx6q-sabresd.
Fixes: f4b34faa0842 ("drm/imx: Annotate dma-fence critical section in commit
path")
Signed-off-by: Fabio Estevam
---
drivers/gpu/drm/imx/imx-drm-core.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
b/drivers/gpu/drm/imx/imx-drm-core.c
index 9558e9e1b431..cb685fe2039b 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -81,7 +81,6 @@ static void imx_drm_atomic_commit_tail(struct
drm_atomic_state *state)
struct drm_plane_state *old_plane_state, *new_plane_state;
bool plane_disabling = false;
int i;
- bool fence_cookie = dma_fence_begin_signalling();
drm_atomic_helper_commit_modeset_disables(dev, state);
@@ -112,7 +111,6 @@ static void imx_drm_atomic_commit_tail(struct
drm_atomic_state *state)
}
drm_atomic_helper_commit_hw_done(state);
- dma_fence_end_signalling(fence_cookie);
}
static const struct drm_mode_config_helper_funcs imx_drm_mode_config_helpers =
{
--
2.25.1
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table
On 11/3/2021 14:38, Jordan Justen wrote:
John Harrison writes:
On 11/1/2021 08:39, Jordan Justen wrote:
writes:
From: Rodrigo Vivi
GuC contains a consolidated table with a bunch of information about the
current device.
Previously, this information was spread and hardcoded to all the components
including GuC, i915 and various UMDs. The goal here is to consolidate
the data into GuC in a way that all interested components can grab the
very latest and synchronized information using a simple query.
As per most of the other queries, this one can be called twice.
Once with item.length=0 to determine the exact buffer size, then
allocate the user memory and call it again for to retrieve the
table data. For example:
struct drm_i915_query_item item = {
.query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
};
query.items_ptr = (int64_t) &item;
query.num_items = 1;
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
if (item.length <= 0)
return -ENOENT;
data = malloc(item.length);
item.data_ptr = (int64_t) &data;
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
// Parse the data as appropriate...
The returned array is a simple and flexible KLV (Key/Length/Value)
formatted table. For example, it could be just:
enum device_attr {
ATTR_SOME_VALUE = 0,
ATTR_SOME_MASK = 1,
};
static const u32 hwconfig[] = {
ATTR_SOME_VALUE,
1, // Value Length in DWords
8, // Value
ATTR_SOME_MASK,
3,
0x00, 0x, 0xFF00,
};
Seems simple enough, so why doesn't i915 define the format of the
returned hwconfig blob in i915_drm.h?
Because the definition is nothing to do with i915. This table comes from
the hardware spec. It is not defined by the KMD and it is not currently
used by the KMD. So there is no reason for the KMD to be creating
structures for it in the same way that the KMD does not document,
define, struct, etc. every other feature of the hardware that the UMDs
might use.
So, i915 wants to wash it's hands completely of the format? There is
obviously a difference between hardware features and a blob coming from
closed source software. (Which i915 just happens to be passing along.)
The hardware is a lot more difficult to change...
Actually, no. The table is not "coming from closed source software". The
table is defined by hardware specs. It is a table of hardware specific
values. It is not being invented by the GuC just for fun or as a way to
subvert the universe into the realms of closed source software. As per
KMD, GuC is merely passing the table through. The table is only
supported on newer hardware platforms and all GuC does is provide a
mechanism for the KMD to retrieve it because the KMD cannot access it
directly. The table contents are defined by hardware architects same as
all the other aspects of the hardware.
It seems like these details should be dropped from the i915 patch commit
message since i915 wants nothing to do with it.
Sure. Can remove comments.
I would think it'd be preferable for i915 to stand behind the basic blob
format as is (even if the keys/values can't be defined), and make a new
query item if the closed source software changes the format.
Close source software is not allowed to change the format because closed
source software has no say in defining the format. The format is
officially defined as being fixed in the spec. New key values can be
added to the key enumeration but existing values cannot be deprecated
and re-purposed. The table must be stable across all OSs and all
platforms. No software can arbitrarily decide to change it.
Of course, it'd be even better if i915 could define some keys/values as
well. (Or if a spec could be released to help document / tie down the
format.)
See the corresponding IGT test that details all the currently defined keys.
struct drm_i915_hwconfig {
uint32_t key;
uint32_t length;
uint32_t values[];
};
It sounds like the kernel depends on the closed source guc being loaded
to return this information. Is that right? Will i915 also become
dependent on some of this data such that it won't be able to initialize
without the firmware being loaded?
At the moment, the KMD does not use the table at all. We merely provide
a mechanism for the UMDs to retrieve it from the hardware.
In terms of future direction, that is something you need to take up with
the hardware architects.
Why do you keep saying hardware, when only software is involved?
See above - because the table is defined by hardware. No software,
closed or open, has any say in the specification of the table.
John.
The attribute ids are defined in a hardware spec.
Which spec?
Unfortunately, it is not one that is currently public. We are pushing
the relevant people to get it included in the public bspec / HRM. It is
a slow process :(.
Right. I take it no progress has bee
[PATCH] drm/msm/a6xx: Allocate enough space for GMU registers
In commit 142639a52a01 ("drm/msm/a6xx: fix crashstate capture for
A650") we changed a6xx_get_gmu_registers() to read 3 sets of
registers. Unfortunately, we didn't change the memory allocation for
the array. That leads to a KASAN warning (this was on the chromeos-5.4
kernel, which has the problematic commit backported to it):
BUG: KASAN: slab-out-of-bounds in _a6xx_get_gmu_registers+0x144/0x430
Write of size 8 at addr ff80c89432b0 by task A618-worker/209
CPU: 5 PID: 209 Comm: A618-worker Tainted: GW 5.4.156-lockdep
#22
Hardware name: Google Lazor Limozeen without Touchscreen (rev5 - rev8) (DT)
Call trace:
dump_backtrace+0x0/0x248
show_stack+0x20/0x2c
dump_stack+0x128/0x1ec
print_address_description+0x88/0x4a0
__kasan_report+0xfc/0x120
kasan_report+0x10/0x18
__asan_report_store8_noabort+0x1c/0x24
_a6xx_get_gmu_registers+0x144/0x430
a6xx_gpu_state_get+0x330/0x25d4
msm_gpu_crashstate_capture+0xa0/0x84c
recover_worker+0x328/0x838
kthread_worker_fn+0x32c/0x574
kthread+0x2dc/0x39c
ret_from_fork+0x10/0x18
Allocated by task 209:
__kasan_kmalloc+0xfc/0x1c4
kasan_kmalloc+0xc/0x14
kmem_cache_alloc_trace+0x1f0/0x2a0
a6xx_gpu_state_get+0x164/0x25d4
msm_gpu_crashstate_capture+0xa0/0x84c
recover_worker+0x328/0x838
kthread_worker_fn+0x32c/0x574
kthread+0x2dc/0x39c
ret_from_fork+0x10/0x18
Fixes: 142639a52a01 ("drm/msm/a6xx: fix crashstate capture for A650")
Signed-off-by: Douglas Anderson
---
I don't actually know how to trigger a GPU crash. I just happened to
trigger one by getting "lucky" and hitting a timeout after being in
kdb. Thus this is just compile tested. However, it looks pretty sane
to me. ;-)
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index 7501849ed15d..6e90209cd543 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -777,12 +777,12 @@ static void a6xx_get_gmu_registers(struct msm_gpu *gpu,
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
a6xx_state->gmu_registers = state_kcalloc(a6xx_state,
- 2, sizeof(*a6xx_state->gmu_registers));
+ 3, sizeof(*a6xx_state->gmu_registers));
if (!a6xx_state->gmu_registers)
return;
- a6xx_state->nr_gmu_registers = 2;
+ a6xx_state->nr_gmu_registers = 3;
/* Get the CX GMU registers from AHB */
_a6xx_get_gmu_registers(gpu, a6xx_state, &a6xx_gmu_reglist[0],
--
2.33.1.1089.g2158813163f-goog
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table
John Harrison writes:
> On 11/1/2021 08:39, Jordan Justen wrote:
>> writes:
>>
>>> From: Rodrigo Vivi
>>>
>>> GuC contains a consolidated table with a bunch of information about the
>>> current device.
>>>
>>> Previously, this information was spread and hardcoded to all the components
>>> including GuC, i915 and various UMDs. The goal here is to consolidate
>>> the data into GuC in a way that all interested components can grab the
>>> very latest and synchronized information using a simple query.
>>>
>>> As per most of the other queries, this one can be called twice.
>>> Once with item.length=0 to determine the exact buffer size, then
>>> allocate the user memory and call it again for to retrieve the
>>> table data. For example:
>>>struct drm_i915_query_item item = {
>>> .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>>>};
>>>query.items_ptr = (int64_t) &item;
>>>query.num_items = 1;
>>>
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>if (item.length <= 0)
>>> return -ENOENT;
>>>
>>>data = malloc(item.length);
>>>item.data_ptr = (int64_t) &data;
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>// Parse the data as appropriate...
>>>
>>> The returned array is a simple and flexible KLV (Key/Length/Value)
>>> formatted table. For example, it could be just:
>>>enum device_attr {
>>> ATTR_SOME_VALUE = 0,
>>> ATTR_SOME_MASK = 1,
>>>};
>>>
>>>static const u32 hwconfig[] = {
>>>ATTR_SOME_VALUE,
>>>1, // Value Length in DWords
>>>8, // Value
>>>
>>>ATTR_SOME_MASK,
>>>3,
>>>0x00, 0x, 0xFF00,
>>>};
>> Seems simple enough, so why doesn't i915 define the format of the
>> returned hwconfig blob in i915_drm.h?
> Because the definition is nothing to do with i915. This table comes from
> the hardware spec. It is not defined by the KMD and it is not currently
> used by the KMD. So there is no reason for the KMD to be creating
> structures for it in the same way that the KMD does not document,
> define, struct, etc. every other feature of the hardware that the UMDs
> might use.
So, i915 wants to wash it's hands completely of the format? There is
obviously a difference between hardware features and a blob coming from
closed source software. (Which i915 just happens to be passing along.)
The hardware is a lot more difficult to change...
It seems like these details should be dropped from the i915 patch commit
message since i915 wants nothing to do with it.
I would think it'd be preferable for i915 to stand behind the basic blob
format as is (even if the keys/values can't be defined), and make a new
query item if the closed source software changes the format.
Of course, it'd be even better if i915 could define some keys/values as
well. (Or if a spec could be released to help document / tie down the
format.)
>>
>> struct drm_i915_hwconfig {
>> uint32_t key;
>> uint32_t length;
>> uint32_t values[];
>> };
>>
>> It sounds like the kernel depends on the closed source guc being loaded
>> to return this information. Is that right? Will i915 also become
>> dependent on some of this data such that it won't be able to initialize
>> without the firmware being loaded?
> At the moment, the KMD does not use the table at all. We merely provide
> a mechanism for the UMDs to retrieve it from the hardware.
>
> In terms of future direction, that is something you need to take up with
> the hardware architects.
>
Why do you keep saying hardware, when only software is involved?
>
>>> The attribute ids are defined in a hardware spec.
>> Which spec?
>
> Unfortunately, it is not one that is currently public. We are pushing
> the relevant people to get it included in the public bspec / HRM. It is
> a slow process :(.
>
Right. I take it no progress has been made in the 1.5 months since you
posted this, so it'll probably finally be documented 6~12 months after
hardware is available? :)
-Jordan
RE: [PATCH 2/3] drm/i915/dg2: Add initial gt/ctx/engine workarounds
> -Original Message-
> From: Roper, Matthew D
> Sent: Tuesday, November 2, 2021 3:25 PM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Roper, Matthew D
> ; Srivatsa, Anusha
>
> Subject: [PATCH 2/3] drm/i915/dg2: Add initial gt/ctx/engine workarounds
>
> Bspec: 54077,68173,54833
> Cc: Anusha Srivatsa
> Signed-off-by: Matt Roper
Reviewed-by: Anusha Srivatsa
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 278 +++-
> drivers/gpu/drm/i915/i915_reg.h | 94 +--
> drivers/gpu/drm/i915/intel_pm.c | 21 +-
> 3 files changed, 372 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 4aaa210fc003..37fd541a9719 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -644,6 +644,42 @@ static void dg1_ctx_workarounds_init(struct
> intel_engine_cs *engine,
>DG1_HZ_READ_SUPPRESSION_OPTIMIZATION_DISABLE);
> }
>
> +static void dg2_ctx_workarounds_init(struct intel_engine_cs *engine,
> + struct i915_wa_list *wal)
> +{
> + gen12_ctx_gt_tuning_init(engine, wal);
> +
> + /* Wa_16011186671:dg2_g11 */
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0)) {
> + wa_masked_dis(wal, VFLSKPD,
> DIS_MULT_MISS_RD_SQUASH);
> + wa_masked_en(wal, VFLSKPD, DIS_OVER_FETCH_CACHE);
> + }
> +
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_B0)) {
> + /* Wa_14010469329:dg2_g10 */
> + wa_masked_en(wal, GEN11_COMMON_SLICE_CHICKEN3,
> + XEHP_DUAL_SIMD8_SEQ_MERGE_DISABLE);
> +
> + /*
> + * Wa_22010465075:dg2_g10
> + * Wa_22010613112:dg2_g10
> + * Wa_14010698770:dg2_g10
> + */
> + wa_masked_en(wal, GEN11_COMMON_SLICE_CHICKEN3,
> + GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> + }
> +
> + /* Wa_16013271637:dg2 */
> + wa_masked_en(wal, SLICE_COMMON_ECO_CHICKEN1,
> + MSC_MSAA_REODER_BUF_BYPASS_DISABLE);
> +
> + /* Wa_22012532006:dg2 */
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_C0) ||
> + IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0))
> + wa_masked_en(wal, GEN9_HALF_SLICE_CHICKEN7,
> +
> DG2_DISABLE_ROUND_ENABLE_ALLOW_FOR_SSLA);
> +}
> +
> static void fakewa_disable_nestedbb_mode(struct intel_engine_cs *engine,
>struct i915_wa_list *wal)
> {
> @@ -730,7 +766,9 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs
> *engine,
> if (engine->class != RENDER_CLASS)
> goto done;
>
> - if (IS_XEHPSDV(i915))
> + if (IS_DG2(i915))
> + dg2_ctx_workarounds_init(engine, wal);
> + else if (IS_XEHPSDV(i915))
> ; /* noop; none at this time */
> else if (IS_DG1(i915))
> dg1_ctx_workarounds_init(engine, wal); @@ -1343,12
> +1381,117 @@ xehpsdv_gt_workarounds_init(struct intel_gt *gt, struct
> i915_wa_list *wal)
> GLOBAL_INVALIDATION_MODE);
> }
>
> +static void
> +dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> +{
> + struct intel_engine_cs *engine;
> + int id;
> +
> + xehp_init_mcr(gt, wal);
> +
> + /* Wa_14011060649:dg2 */
> + wa_14011060649(gt, wal);
> +
> + /*
> + * Although there are per-engine instances of these registers,
> + * they technically exist outside the engine itself and are not
> + * impacted by engine resets. Furthermore, they're part of the
> + * GuC blacklist so trying to treat them as engine workarounds
> + * will result in GuC initialization failure and a wedged GPU.
> + */
> + for_each_engine(engine, gt, id) {
> + if (engine->class != VIDEO_DECODE_CLASS)
> + continue;
> +
> + /* Wa_16010515920:dg2_g10 */
> + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0,
> STEP_B0))
> + wa_write_or(wal, VDBOX_CGCTL3F18(engine-
> >mmio_base),
> + ALNUNIT_CLKGATE_DIS);
> + }
> +
> + if (IS_DG2_G10(gt->i915)) {
> + /* Wa_22010523718:dg2 */
> + wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE,
> + CG3DDISCFEG_CLKGATE_DIS);
> +
> + /* Wa_14011006942:dg2 */
> + wa_write_or(wal, SUBSLICE_UNIT_LEVEL_CLKGATE,
> + DSS_ROUTER_CLKGATE_DIS);
> + }
> +
> + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_B0)) {
> + /* Wa_14010680813:dg2_g10 */
> + wa_write_or(wal, GEN12_GAMSTLB_CTRL,
> CONTROL_BLOCK_CLKGATE_DIS |
> + EGRESS_BLOCK_CLKGATE_DIS |
> TAG_BLOCK_CLKGATE_DIS);
> +
> + /*
[PATCH] drm/msm: Hangcheck timer fixes
From: Rob Clark
Cancel the timer when the GPU is idle, but also remember to restart it
in the recover path if we've re-submitted submits following the one that
hung.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 0d56699297c7..367f0c698b40 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -16,6 +16,8 @@
#include
#include
+static void hangcheck_timer_reset(struct msm_gpu *gpu);
+
/*
* Power Management:
*/
@@ -450,6 +452,10 @@ static void recover_worker(struct kthread_work *work)
gpu->funcs->submit(gpu, submit);
spin_unlock_irqrestore(&ring->submit_lock, flags);
}
+
+ hangcheck_timer_reset(gpu);
+ } else {
+ del_timer(&gpu->hangcheck_timer);
}
mutex_unlock(&dev->struct_mutex);
@@ -721,6 +727,10 @@ static void retire_worker(struct kthread_work *work)
struct msm_gpu *gpu = container_of(work, struct msm_gpu, retire_work);
retire_submits(gpu);
+
+ if (!msm_gpu_active(gpu)) {
+ del_timer(&gpu->hangcheck_timer);
+ }
}
/* call from irq handler to schedule work to retire bo's */
--
2.31.1
[PATCH 2/2] drm/msm/gpu: Respect PM QoS constraints
From: Rob Clark
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index b24e5475cafb..427c55002f4d 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -158,6 +158,33 @@ void msm_devfreq_suspend(struct msm_gpu *gpu)
devfreq_suspend_device(gpu->devfreq.devfreq);
}
+static void set_target(struct msm_gpu *gpu, unsigned long freq)
+{
+ struct msm_gpu_devfreq *df = &gpu->devfreq;
+ unsigned long min_freq, max_freq;
+ u32 flags = 0;
+
+ /*
+* When setting the target freq internally, we need to apply PM QoS
+* constraints (such as cooling):
+*/
+ min_freq = dev_pm_qos_read_value(df->devfreq->dev.parent,
+DEV_PM_QOS_MIN_FREQUENCY);
+ max_freq = dev_pm_qos_read_value(df->devfreq->dev.parent,
+DEV_PM_QOS_MAX_FREQUENCY);
+
+ if (freq < min_freq) {
+ freq = min_freq;
+ flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
+ }
+ if (freq > max_freq) {
+ freq = max_freq;
+ flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
+ }
+
+ msm_devfreq_target(&gpu->pdev->dev, &freq, flags);
+}
+
void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
{
struct msm_gpu_devfreq *df = &gpu->devfreq;
@@ -173,7 +200,7 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
freq *= factor;
- msm_devfreq_target(&gpu->pdev->dev, &freq, 0);
+ set_target(gpu, freq);
mutex_unlock(&df->devfreq->lock);
}
@@ -212,7 +239,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
df->idle_freq = 0;
- msm_devfreq_target(&gpu->pdev->dev, &target_freq, 0);
+ set_target(gpu, target_freq);
/*
* Reset the polling interval so we aren't inconsistent
--
2.31.1
[PATCH 1/2] drm/msm/devfreq: Add some locking asserts
From: Rob Clark
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index 47b3cf2df230..b24e5475cafb 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -20,6 +20,8 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
struct msm_gpu *gpu = dev_to_gpu(dev);
struct dev_pm_opp *opp;
+ WARN_ON(!mutex_is_locked(&gpu->devfreq.devfreq->lock));
+
opp = devfreq_recommended_opp(dev, freq, flags);
/*
@@ -63,6 +65,8 @@ static int msm_devfreq_get_dev_status(struct device *dev,
struct msm_gpu *gpu = dev_to_gpu(dev);
ktime_t time;
+ WARN_ON(!mutex_is_locked(&gpu->devfreq.devfreq->lock));
+
status->current_frequency = get_freq(gpu);
status->busy_time = gpu->funcs->gpu_busy(gpu);
@@ -75,7 +79,11 @@ static int msm_devfreq_get_dev_status(struct device *dev,
static int msm_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
{
- *freq = get_freq(dev_to_gpu(dev));
+ struct msm_gpu *gpu = dev_to_gpu(dev);
+
+ WARN_ON(!mutex_is_locked(&gpu->devfreq.devfreq->lock));
+
+ *freq = get_freq(gpu);
return 0;
}
--
2.31.1
[PATCH v3] drm/bridge: analogix_dp: Make PSR-exit block less
Prior to commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR"),
"PSR exit" used non-blocking analogix_dp_send_psr_spd(). The refactor
started using the blocking variant, for a variety of reasons -- quoting
Sean Paul's potentially-faulty memory:
"""
- To avoid racing a subsequent PSR entry (if exit takes a long time)
- To avoid racing disable/modeset
- We're not displaying new content while exiting PSR anyways, so there
is minimal utility in allowing frames to be submitted
- We're lying to userspace telling them frames are on the screen when
we're just dropping them on the floor
"""
However, I'm finding that this blocking transition is causing upwards of
60+ ms of unneeded latency on PSR-exit, to the point that initial cursor
movements when leaving PSR are unbearably jumpy.
It turns out that we need to meet in the middle somewhere: Sean is right
that we were "lying to userspace" with a non-blocking PSR-exit, but the
new blocking behavior is also waiting too long:
According to the eDP specification, the sink device must support PSR
entry transitions from both state 4 (ACTIVE_RESYNC) and state 0
(INACTIVE). It also states that in ACTIVE_RESYNC, "the Sink device must
display the incoming active frames from the Source device with no
visible glitches and/or artifacts."
Thus, for our purposes, we only need to wait for ACTIVE_RESYNC before
moving on; we are ready to display video, and subsequent PSR-entry is
safe.
Tested on a Samsung Chromebook Plus (i.e., Rockchip RK3399 Gru Kevin),
where this saves about 60ms of latency, for PSR-exit that used to
take about 80ms.
Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
Cc:
Cc: Zain Wang
Cc: Tomasz Figa
Cc: Heiko Stuebner
Cc: Sean Paul
Signed-off-by: Brian Norris
Reviewed-by: Sean Paul
---
CC list is partially constructed from the commit message of the Fixed
commit
Changes in v3:
- Fix the exiting/entering comment (a reviewer noticed off-mailing-list
that I got it backwards)
- Add Reviewed-by
Changes in v2:
- retitled subject (previous: "drm/bridge: analogix_dp: Make
PSR-disable non-blocking")
- instead of completely non-blocking, make this "less"-blocking
- more background (thanks Sean!)
- more specification details
drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index cab6c8b92efd..6a4f20fccf84 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -998,11 +998,21 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device
*dp,
if (!blocking)
return 0;
+ /*
+* db[1]!=0: entering PSR, wait for fully active remote frame buffer.
+* db[1]==0: exiting PSR, wait for either
+* (a) ACTIVE_RESYNC - the sink "must display the
+* incoming active frames from the Source device with no visible
+* glitches and/or artifacts", even though timings may still be
+* re-synchronizing; or
+* (b) INACTIVE - the transition is fully complete.
+*/
ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
psr_status >= 0 &&
((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
- (!vsc->db[1] && psr_status == DP_PSR_SINK_INACTIVE)), 1500,
- DP_TIMEOUT_PSR_LOOP_MS * 1000);
+ (!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
+psr_status == DP_PSR_SINK_INACTIVE))),
+ 1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
if (ret) {
dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
return ret;
--
2.34.0.rc0.344.g81b53c2807-goog
Re: [PATCH] drm/msm/dp: employ bridge mechanism for display enable and disable
Quoting Kuogee Hsieh (2021-11-02 15:13:44)
> Current display mode_set, enable and disable functions are implemented
> as function called directly from drm encoder. This patch have display
> mode_set, enable and disable be implemented as callback function of drm
> bridge.
Why is it important? Please add those details here.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 --
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 ++
> drivers/gpu/drm/msm/dp/dp_display.c | 26 +++
> drivers/gpu/drm/msm/dp/dp_display.h | 8 ++
> drivers/gpu/drm/msm/dp/dp_drm.c | 113
>
> drivers/gpu/drm/msm/msm_drv.h | 29 ++-
> 6 files changed, 147 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 27d98b5..3bbd09c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -557,6 +557,14 @@ static int _dpu_kms_initialize_displayport(struct
> drm_device *dev,
> encoder->base.id, rc);
> return rc;
> }
> +
> + rc = msm_dp_bridge_init(priv->dp[i], dev, encoder);
Use tabs?
> +if (rc) {
> + DPU_ERROR("failed to setup DPU bridge %d: rc:%d\n",
> + encoder->base.id, rc);
> + return rc;
> +}
> +
> }
>
> return rc;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index e41dd40..317f963 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -569,8 +569,8 @@ static int dp_hpd_plug_handle(struct dp_display_private
> *dp, u32 data)
> return 0;
> };
>
> -static int dp_display_enable(struct dp_display_private *dp, u32 data);
> -static int dp_display_disable(struct dp_display_private *dp, u32 data);
> +static int __dp_display_enable(struct dp_display_private *dp, u32 data);
> +static int __dp_display_disable(struct dp_display_private *dp, u32 data);
>
> static int dp_connect_pending_timeout(struct dp_display_private *dp, u32
> data)
> {
> @@ -855,7 +855,7 @@ static int dp_display_prepare(struct msm_dp *dp_display)
> return 0;
> }
>
> -static int dp_display_enable(struct dp_display_private *dp, u32 data)
> +static int __dp_display_enable(struct dp_display_private *dp, u32 data)
> {
> int rc = 0;
>
> @@ -898,7 +898,7 @@ static int dp_display_post_enable(struct msm_dp
> *dp_display)
> return 0;
> }
>
> -static int dp_display_disable(struct dp_display_private *dp, u32 data)
> +static int __dp_display_disable(struct dp_display_private *dp, u32 data)
> {
> struct msm_dp *dp_display = &dp->dp_display;
>
> @@ -1533,7 +1533,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
> struct drm_device *dev,
> return 0;
> }
>
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> {
> int rc = 0;
> struct dp_display_private *dp_display;
> @@ -1569,12 +1569,12 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
> drm_encoder *encoder)
> if (state == ST_DISPLAY_OFF)
> dp_display_host_init(dp_display, true);
>
> - dp_display_enable(dp_display, 0);
> + __dp_display_enable(dp_display, 0);
>
> rc = dp_display_post_enable(dp);
> if (rc) {
> DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
> - dp_display_disable(dp_display, 0);
> + __dp_display_disable(dp_display, 0);
> dp_display_unprepare(dp);
> }
>
> @@ -1590,7 +1590,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
> drm_encoder *encoder)
> return rc;
> }
>
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder
> *encoder)
> +int dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> {
> struct dp_display_private *dp_display;
>
> @@ -1601,7 +1601,7 @@ int msm_dp_display_pre_disable(struct msm_dp *dp,
> struct drm_encoder *encoder)
> return 0;
> }
>
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> {
> int rc = 0;
> u32 state;
> @@ -1614,7 +1614,7 @@ int msm_dp_display_disable(struct msm_dp *dp, struct
> drm_encoder *encoder)
> /* stop sentinel checking */
> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>
> - dp_display_disable(dp_display, 0);
> + __dp_display_disable(dp_display, 0);
>
> rc = dp_display_unprepare(dp);
> if (rc)
> @@ -1632,9 +1632,9 @@ int msm_dp_display_disable(struct msm_dp *dp, struct
Re: [PATCH v2] drm/bridge: analogix_dp: Make PSR-exit block less
On Fri, Oct 29, 2021 at 04:50:55PM -0700, Brian Norris wrote:
> Prior to commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR"),
> "PSR exit" used non-blocking analogix_dp_send_psr_spd(). The refactor
> started using the blocking variant, for a variety of reasons -- quoting
> Sean Paul's potentially-faulty memory:
>
> """
> - To avoid racing a subsequent PSR entry (if exit takes a long time)
> - To avoid racing disable/modeset
> - We're not displaying new content while exiting PSR anyways, so there
>is minimal utility in allowing frames to be submitted
> - We're lying to userspace telling them frames are on the screen when
>we're just dropping them on the floor
> """
>
> However, I'm finding that this blocking transition is causing upwards of
> 60+ ms of unneeded latency on PSR-exit, to the point that initial cursor
> movements when leaving PSR are unbearably jumpy.
>
> It turns out that we need to meet in the middle somewhere: Sean is right
> that we were "lying to userspace" with a non-blocking PSR-exit, but the
> new blocking behavior is also waiting too long:
>
> According to the eDP specification, the sink device must support PSR
> entry transitions from both state 4 (ACTIVE_RESYNC) and state 0
> (INACTIVE). It also states that in ACTIVE_RESYNC, "the Sink device must
> display the incoming active frames from the Source device with no
> visible glitches and/or artifacts."
>
> Thus, for our purposes, we only need to wait for ACTIVE_RESYNC before
> moving on; we are ready to display video, and subsequent PSR-entry is
> safe.
>
> Tested on a Samsung Chromebook Plus (i.e., Rockchip RK3399 Gru Kevin),
> where this saves about 60ms of latency, for PSR-exit that used to
> take about 80ms.
>
> Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
> Cc:
> Cc: Zain Wang
> Cc: Tomasz Figa
> Cc: Heiko Stuebner
> Cc: Sean Paul
Thank you for revising this!
Reviewed-by: Sean Paul
> Signed-off-by: Brian Norris
> ---
> CC list is partially constructed from the commit message of the Fixed
> commit
>
> Changes in v2:
> - retitled subject (previous: "drm/bridge: analogix_dp: Make
>PSR-disable non-blocking")
> - instead of completely non-blocking, make this "less"-blocking
> - more background (thanks Sean!)
> - more specification details
>
> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index cab6c8b92efd..f8e119e84ae2 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -998,11 +998,21 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device
> *dp,
> if (!blocking)
> return 0;
>
> + /*
> + * db[1]==0: entering PSR, wait for fully active remote frame buffer.
> + * db[1]!=0: exiting PSR, wait for either
> + * (a) ACTIVE_RESYNC - the sink "must display the
> + * incoming active frames from the Source device with no visible
> + * glitches and/or artifacts", even though timings may still be
> + * re-synchronizing; or
> + * (b) INACTIVE - the transition is fully complete.
> + */
> ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
> psr_status >= 0 &&
> ((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
> - (!vsc->db[1] && psr_status == DP_PSR_SINK_INACTIVE)), 1500,
> - DP_TIMEOUT_PSR_LOOP_MS * 1000);
> + (!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
> + psr_status == DP_PSR_SINK_INACTIVE))),
> + 1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
> if (ret) {
> dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
> return ret;
> --
> 2.33.1.1089.g2158813163f-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 8/8] drm/amdgpu: add drm buddy support to amdgpu
On 25/10/2021 14:00, Arunpravin wrote:
- Remove drm_mm references and replace with drm buddy functionalities
- Add res cursor support for drm buddy
Signed-off-by: Arunpravin
+ spin_lock(&mgr->lock);
+ r = drm_buddy_alloc(mm, (uint64_t)place->fpfn << PAGE_SHIFT,
+ (uint64_t)lpfn << PAGE_SHIFT,
+ (uint64_t)n_pages << PAGE_SHIFT,
+min_page_size, &node->blocks,
+node->flags);
Is spinlock + GFP_KERNEL allowed?
+ spin_unlock(&mgr->lock);
+
+ if (unlikely(r))
+ goto error_free_blocks;
+
pages_left -= pages;
++i;
if (pages > pages_left)
pages = pages_left;
}
- spin_unlock(&mgr->lock);
+
+ /* Free unused pages for contiguous allocation */
+ if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
+ uint64_t actual_size = (uint64_t)node->base.num_pages <<
PAGE_SHIFT;
+
+ r = drm_buddy_free_unused_pages(mm,
+ actual_size,
+ &node->blocks);
Needs some locking.
Re: [PATCH v2 4/8] drm: vkms: Add fb information to `vkms_writeback_job`
Hi Thomas,
On 11/3/21 12:45 PM, Thomas Zimmermann wrote:
Hi
Am 26.10.21 um 13:34 schrieb Igor Torrente:
This commit is the groundwork to introduce new formats to the planes and
writeback buffer. As part of it, a new buffer metadata field is added to
`vkms_writeback_job`, this metadata is represented by the `vkms_composer`
struct.
This will allow us, in the future, to have different compositing and wb
format types.
Signed-off-by: Igor Torrente
---
V2: Change the code to get the drm_framebuffer reference and not copy its
contents(Thomas Zimmermann).
---
drivers/gpu/drm/vkms/vkms_composer.c | 4 ++--
drivers/gpu/drm/vkms/vkms_drv.h | 12 ++--
drivers/gpu/drm/vkms/vkms_plane.c | 10 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++---
4 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index 82f79e508f81..383ca657ddf7 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -153,7 +153,7 @@ static void compose_plane(struct vkms_composer
*primary_composer,
struct vkms_composer *plane_composer,
void *vaddr_out)
{
- struct drm_framebuffer *fb = &plane_composer->fb;
+ struct drm_framebuffer *fb = plane_composer->fb;
void *vaddr;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
@@ -174,7 +174,7 @@ static int compose_active_planes(void **vaddr_out,
struct vkms_composer *primary_composer,
struct vkms_crtc_state *crtc_state)
{
- struct drm_framebuffer *fb = &primary_composer->fb;
+ struct drm_framebuffer *fb = primary_composer->fb;
struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
const void *vaddr;
int i;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 64e62993b06f..9e4c1e95bbb1 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -20,13 +20,8 @@
#define XRES_MAX 8192
#define YRES_MAX 8192
-struct vkms_writeback_job {
- struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
- struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
-};
-
struct vkms_composer {
- struct drm_framebuffer fb;
+ struct drm_framebuffer *fb;
struct drm_rect src, dst;
struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
unsigned int offset;
@@ -34,6 +29,11 @@ struct vkms_composer {
unsigned int cpp;
};
+struct vkms_writeback_job {
+ struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
+ struct vkms_composer composer;
+};
+
/**
* vkms_plane_state - Driver specific plane state
* @base: base plane state
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c
b/drivers/gpu/drm/vkms/vkms_plane.c
index 32409e15244b..0a28cb7a85e2 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -50,12 +50,12 @@ static void vkms_plane_destroy_state(struct drm_plane
*plane,
struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
struct drm_crtc *crtc = vkms_state->base.base.crtc;
- if (crtc) {
+ if (crtc && vkms_state->composer->fb) {
/* dropping the reference we acquired in
* vkms_primary_plane_update()
*/
- if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
- drm_framebuffer_put(&vkms_state->composer->fb);
+ if (drm_framebuffer_read_refcount(vkms_state->composer->fb))
+ drm_framebuffer_put(vkms_state->composer->fb);
}
kfree(vkms_state->composer);
@@ -110,9 +110,9 @@ static void vkms_plane_atomic_update(struct drm_plane
*plane,
composer = vkms_plane_state->composer;
memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
- memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
+ composer->fb = fb;
memcpy(&composer->map, &shadow_plane_state->data,
sizeof(composer->map));
- drm_framebuffer_get(&composer->fb);
+ drm_framebuffer_get(composer->fb);
composer->offset = fb->offsets[0];
composer->pitch = fb->pitches[0];
composer->cpp = fb->format->cpp[0];
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
b/drivers/gpu/drm/vkms/vkms_writeback.c
index 8694227f555f..32734cdbf6c2 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -75,12 +75,15 @@ static int vkms_wb_prepare_job(struct
drm_writeback_connector *wb_connector,
if (!vkmsjob)
return -ENOMEM;
- ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data);
+ ret = drm_gem_fb_vmap(job->fb, vkmsjob->composer.map, vkmsjob->da
Re: [PATCH 6/8] drm/i915: add free_unused_pages support to i915
On 25/10/2021 14:00, Arunpravin wrote:
add drm_buddy_free_unused_pages() support on
contiguous allocation
Signed-off-by: Arunpravin
---
drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index 963468228392..162947af8e04 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -98,6 +98,14 @@ static int i915_ttm_buddy_man_alloc(struct
ttm_resource_manager *man,
if (unlikely(err))
goto err_free_blocks;
+ if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
+ err = drm_buddy_free_unused_pages(mm, (uint64_t)n_pages <<
PAGE_SHIFT,
+ &bman_res->blocks);
+
+ if (unlikely(err))
+ goto err_free_blocks;
That needs some locking, mark_free/mark_split are all globally visible.
Some concurrent user might steal the block, or worse.
+ }
+
*res = &bman_res->base;
return 0;
Re: [PATCH 5/8] drm: Implement method to free unused pages
On 25/10/2021 14:00, Arunpravin wrote:
On contiguous allocation, we round up the size
to the *next* power of 2, implement a function
to free the unused pages after the newly allocate block.
Signed-off-by: Arunpravin
Ideally this gets added with some user, so we can see it in action?
Maybe squash the next patch here?
---
drivers/gpu/drm/drm_buddy.c | 103
include/drm/drm_buddy.h | 4 ++
2 files changed, 107 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 9d3547bcc5da..0da8510736eb 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -284,6 +284,109 @@ static inline bool contains(u64 s1, u64 e1, u64 s2, u64
e2)
return s1 <= s2 && e1 >= e2;
}
+/**
+ * drm_buddy_free_unused_pages - free unused pages
+ *
+ * @mm: DRM buddy manager
+ * @actual_size: original size requested
+ * @blocks: output list head to add allocated blocks
+ *
+ * For contiguous allocation, we round up the size to the nearest
+ * power of two value, drivers consume *actual* size, so remaining
+ * portions are unused and it can be freed.
+ *
+ * Returns:
+ * 0 on success, error code on failure.
+ */
+int drm_buddy_free_unused_pages(struct drm_buddy_mm *mm,
drm_buddy_block_trim?
+ u64 actual_size,
new_size?
+ struct list_head *blocks)
+{
+ struct drm_buddy_block *block;
+ struct drm_buddy_block *buddy;
+ u64 actual_start;
+ u64 actual_end;
+ LIST_HEAD(dfs);
+ u64 count = 0;
+ int err;
+
+ if (!list_is_singular(blocks))
+ return -EINVAL;
+
+ block = list_first_entry_or_null(blocks,
+struct drm_buddy_block,
+link);
+
+ if (!block)
+ return -EINVAL;
list_is_singular() already ensures that I guess?
+
+ if (actual_size > drm_buddy_block_size(mm, block))
+ return -EINVAL;
+
+ if (actual_size == drm_buddy_block_size(mm, block))
+ return 0;
Probably need to check the alignment of the actual_size, and also check
that it is non-zero?
+
+ list_del(&block->link);
+
+ actual_start = drm_buddy_block_offset(block);
+ actual_end = actual_start + actual_size - 1;
+
+ if (drm_buddy_block_is_allocated(block))
That should rather be a programmer error.
+ mark_free(mm, block);
+
+ list_add(&block->tmp_link, &dfs);
+
+ while (1) {
+ block = list_first_entry_or_null(&dfs,
+struct drm_buddy_block,
+tmp_link);
+
+ if (!block)
+ break;
+
+ list_del(&block->tmp_link);
+
+ if (count == actual_size)
+ return 0;
Check for overlaps somewhere here to avoid needless searching and splitting?
+
+ if (contains(actual_start, actual_end,
drm_buddy_block_offset(block),
+ (drm_buddy_block_offset(block) +
drm_buddy_block_size(mm, block) - 1))) {
Could maybe record the start/end for better readability?
+ BUG_ON(!drm_buddy_block_is_free(block));
+
+ /* Allocate only required blocks */
+ mark_allocated(block);
+ mm->avail -= drm_buddy_block_size(mm, block);
+ list_add_tail(&block->link, blocks);
+ count += drm_buddy_block_size(mm, block);
+ continue;
+ }
+
+ if (drm_buddy_block_order(block) == 0)
+ continue;
Should be impossible with overlaps check added.
+
+ if (!drm_buddy_block_is_split(block)) {
That should always be true.
+ err = split_block(mm, block);
+
+ if (unlikely(err))
+ goto err_undo;
+ }
+
+ list_add(&block->right->tmp_link, &dfs);
+ list_add(&block->left->tmp_link, &dfs);
+ }
+
+ return -ENOSPC;
Would it make sense to factor out part of the alloc_range for this? It
looks roughly the same.
+
+err_undo:
+ buddy = get_buddy(block);
+ if (buddy &&
+ (drm_buddy_block_is_free(block) &&
+drm_buddy_block_is_free(buddy)))
+ __drm_buddy_free(mm, block);
+ return err;
Where do we add the block back to the original list? Did we not just
leak it?
+}
+EXPORT_SYMBOL(drm_buddy_free_unused_pages);
+
static struct drm_buddy_block *
alloc_range(struct drm_buddy_mm *mm,
u64 start, u64 end,
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index cd8021d2d6e7..1dfc80c88e1f 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@
Re: [PATCH 3/8] drm: implement top-down allocation method
On 25/10/2021 14:00, Arunpravin wrote:
Implemented a function which walk through the order list,
compares the offset and returns the maximum offset block,
this method is unpredictable in obtaining the high range
address blocks which depends on allocation and deallocation.
for instance, if driver requests address at a low specific
range, allocator traverses from the root block and splits
the larger blocks until it reaches the specific block and
in the process of splitting, lower orders in the freelist
are occupied with low range address blocks and for the
subsequent TOPDOWN memory request we may return the low
range blocks.To overcome this issue, we may go with the
below approach.
The other approach, sorting each order list entries in
ascending order and compares the last entry of each
order list in the freelist and return the max block.
This creates sorting overhead on every drm_buddy_free()
request and split up of larger blocks for a single page
request.
Signed-off-by: Arunpravin
---
drivers/gpu/drm/drm_buddy.c | 42 +++--
include/drm/drm_buddy.h | 1 +
2 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 406e3d521903..9d3547bcc5da 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -362,6 +362,27 @@ alloc_range(struct drm_buddy_mm *mm,
return ERR_PTR(err);
}
+static struct drm_buddy_block *
+get_maxblock(struct list_head *head)
+{
+ struct drm_buddy_block *max_block = NULL, *node;
+
+ max_block = list_first_entry_or_null(head,
+struct drm_buddy_block,
+link);
+
+ if (!max_block)
+ return NULL;
+
+ list_for_each_entry(node, head, link) {
+ if (drm_buddy_block_offset(node) >
+ drm_buddy_block_offset(max_block))
Alignment.
+ max_block = node;
+ }
I suppose there will be pathological cases where this will unnecessarily
steal the mappable portion? But in practice maybe this is good enough?
+
+ return max_block;
+}
+
static struct drm_buddy_block *
alloc_from_freelist(struct drm_buddy_mm *mm,
unsigned int order,
@@ -372,13 +393,22 @@ alloc_from_freelist(struct drm_buddy_mm *mm,
int err;
for (i = order; i <= mm->max_order; ++i) {
- if (!list_empty(&mm->free_list[i])) {
- block = list_first_entry_or_null(&mm->free_list[i],
-struct drm_buddy_block,
-link);
+ if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
+ if (!list_empty(&mm->free_list[i])) {
AFAIK no need to keep checking list_empty(), below also.
+ block = get_maxblock(&mm->free_list[i]);
- if (block)
- break;
+ if (block)
+ break;
+ }
+ } else {
+ if (!list_empty(&mm->free_list[i])) {
+ block =
list_first_entry_or_null(&mm->free_list[i],
+struct
drm_buddy_block,
+link);
+
+ if (block)
+ break;
+ }
}
}
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index c7bb5509a7ad..cd8021d2d6e7 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@ -28,6 +28,7 @@
})
#define DRM_BUDDY_RANGE_ALLOCATION (1 << 0)
+#define DRM_BUDDY_TOPDOWN_ALLOCATION (1 << 1)
struct drm_buddy_block {
#define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
Re: [PATCH v2 2/8] drm: improve drm_buddy_alloc function
On 25/10/2021 14:00, Arunpravin wrote:
- Make drm_buddy_alloc a single function to handle
range allocation and non-range allocation demands
- Implemented a new function alloc_range() which allocates
the requested power-of-two block comply with range limitations
- Moved order computation and memory alignment logic from
i915 driver to drm buddy
V2:
merged below changes to keep the build unbroken
- drm_buddy_alloc_range() becomes obsolete and may be removed
- enable ttm range allocation (fpfn / lpfn) support in i915 driver
- apply enhanced drm_buddy_alloc() function to i915 driver
Signed-off-by: Arunpravin
---
drivers/gpu/drm/drm_buddy.c | 265 +++---
drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 67 ++---
drivers/gpu/drm/i915/i915_ttm_buddy_manager.h | 2 +
include/drm/drm_buddy.h | 22 +-
4 files changed, 207 insertions(+), 149 deletions(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 39eb1d224bec..406e3d521903 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -274,63 +274,6 @@ void drm_buddy_free_list(struct drm_buddy_mm *mm, struct
list_head *objects)
}
EXPORT_SYMBOL(drm_buddy_free_list);
-/**
- * drm_buddy_alloc - allocate power-of-two blocks
- *
- * @mm: DRM buddy manager to allocate from
- * @order: size of the allocation
- *
- * The order value here translates to:
- *
- * 0 = 2^0 * mm->chunk_size
- * 1 = 2^1 * mm->chunk_size
- * 2 = 2^2 * mm->chunk_size
- *
- * Returns:
- * allocated ptr to the &drm_buddy_block on success
- */
-struct drm_buddy_block *
-drm_buddy_alloc(struct drm_buddy_mm *mm, unsigned int order)
-{
- struct drm_buddy_block *block = NULL;
- unsigned int i;
- int err;
-
- for (i = order; i <= mm->max_order; ++i) {
- block = list_first_entry_or_null(&mm->free_list[i],
-struct drm_buddy_block,
-link);
- if (block)
- break;
- }
-
- if (!block)
- return ERR_PTR(-ENOSPC);
-
- BUG_ON(!drm_buddy_block_is_free(block));
-
- while (i != order) {
- err = split_block(mm, block);
- if (unlikely(err))
- goto out_free;
-
- /* Go low */
- block = block->left;
- i--;
- }
-
- mark_allocated(block);
- mm->avail -= drm_buddy_block_size(mm, block);
- kmemleak_update_trace(block);
- return block;
-
-out_free:
- if (i != order)
- __drm_buddy_free(mm, block);
- return ERR_PTR(err);
-}
-EXPORT_SYMBOL(drm_buddy_alloc);
-
static inline bool overlaps(u64 s1, u64 e1, u64 s2, u64 e2)
{
return s1 <= e2 && e1 >= s2;
@@ -341,52 +284,22 @@ static inline bool contains(u64 s1, u64 e1, u64 s2, u64
e2)
return s1 <= s2 && e1 >= e2;
}
-/**
- * drm_buddy_alloc_range - allocate range
- *
- * @mm: DRM buddy manager to allocate from
- * @blocks: output list head to add allocated blocks
- * @start: start of the allowed range for this block
- * @size: size of the allocation
- *
- * Intended for pre-allocating portions of the address space, for example to
- * reserve a block for the initial framebuffer or similar, hence the
expectation
- * here is that drm_buddy_alloc() is still the main vehicle for
- * allocations, so if that's not the case then the drm_mm range allocator is
- * probably a much better fit, and so you should probably go use that instead.
- *
- * Note that it's safe to chain together multiple alloc_ranges
- * with the same blocks list
- *
- * Returns:
- * 0 on success, error code on failure.
- */
-int drm_buddy_alloc_range(struct drm_buddy_mm *mm,
- struct list_head *blocks,
- u64 start, u64 size)
+static struct drm_buddy_block *
+alloc_range(struct drm_buddy_mm *mm,
+ u64 start, u64 end,
+ unsigned int order)
{
struct drm_buddy_block *block;
struct drm_buddy_block *buddy;
- LIST_HEAD(allocated);
LIST_HEAD(dfs);
- u64 end;
int err;
int i;
- if (size < mm->chunk_size)
- return -EINVAL;
-
- if (!IS_ALIGNED(size | start, mm->chunk_size))
- return -EINVAL;
-
- if (range_overflows(start, size, mm->size))
- return -EINVAL;
+ end = end - 1;
for (i = 0; i < mm->n_roots; ++i)
list_add_tail(&mm->roots[i]->tmp_link, &dfs);
- end = start + size - 1;
-
do {
u64 block_start;
u64 block_end;
@@ -394,31 +307,32 @@ int drm_buddy_alloc_range(struct drm_buddy_mm *mm,
block = list_first_entry_or_null(&dfs,
struct drm_buddy_block,
tmp_link);
+
No need
Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation
Hi Thomas,
On 11/3/21 12:37 PM, Thomas Zimmermann wrote:
Hi
Am 03.11.21 um 16:11 schrieb Leandro Ribeiro:
Hi,
On 11/3/21 12:03, Igor Torrente wrote:
Hi Leandro,
On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
Hi,
On 10/26/21 08:34, Igor Torrente wrote:
Add a helper function to validate the connector configuration receive in
the encoder atomic_check by the drivers.
So the drivers don't need do these common validations themselves.
Signed-off-by: Igor Torrente
---
V2: Move the format verification to a new helper at the
drm_atomic_helper.c
(Thomas Zimmermann).
---
drivers/gpu/drm/drm_atomic_helper.c | 47 +++
drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++--
include/drm/drm_atomic_helper.h | 3 ++
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 2c0c6ec92820..c2653b9824b5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct
drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
+/**
+ * drm_atomic_helper_check_wb_connector_state() - Check writeback
encoder state
+ * @encoder: encoder state to check
+ * @conn_state: connector state to check
+ *
+ * Checks if the wriback connector state is valid, and returns a
'writeback'
'an error'
erros if it
'error'
+ * isn't.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int
+drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
+ struct drm_connector_state *conn_state)
+{
+ struct drm_writeback_job *wb_job = conn_state->writeback_job;
+ struct drm_property_blob *pixel_format_blob;
+ bool format_supported = false;
+ struct drm_framebuffer *fb;
+ int i, n_formats;
Just 'nformats'.
Please make both variables 'size_t'.
I Will correct all these minor issues.
+ u32 *formats;
+
+ if (!wb_job || !wb_job->fb)
+ return 0;
I think that this should be removed and that this functions should
assume that (wb_job && wb_job->fb) == true.
Ok.
In regular atomic check for planes, there can be planes with no attached
framebuffer. Helpers handle this situation. [1] I don't know if this is
possible in writeback code, but for consistency, it would make sense to
keep this test here. Not sure though.
@Leandro, do you know if it is possible to have a wb_job without a fb
attached?
Actually, it's weird to have conn_state as argument and only use it to
get the wb_job. Instead, this function could receive wb_job directly.
In the Thomas review of v1, he said that maybe other things could be
tested in this helper. I'm not sure what these additional checks could
be, so I tried to design the function signature expecting more things
to be added after his review.
As you can see, the helper is receiving the `drm_encoder` and doing
nothing with it.
If we, eventually, don't find anything else that this helper can do, I
will revert to something very similar (if not equal) to your proposal.
I just want to wait for Thomas's review first.
Sure, that makes sense.
We had many helper functions for atomic modesetting that took various
arguments for whatever they required. Extending such a function with new
functionality/arguments required required touching many drivers and made
the parameter list hard to read. At some point, Maxime went through most
of the code and unified it all to pass full state > So please keep the
connector state. I think it's how we do things ATM.to the helpers.
So please keep the connector state. I think it's how we do things ATM.
OK, I will keep then.
Thanks,
Leandro Ribeiro
Of course, its name/description would have to change.
+
+ pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
+ n_formats = pixel_format_blob->length / sizeof(u32);
+ formats = pixel_format_blob->data;
+ fb = wb_job->fb;
+
+ for (i = 0; i < n_formats; i++) {
+ if (fb->format->format == formats[i]) {
+ format_supported = true;
+ break;
+ }
+ }
+
+ if (!format_supported) {
+ DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
+ &fb->format->format);
Please use drm_dgb_kms() instead. There's a 100-character-per-line
limit. The comment probably fits onto a single line.(?)
I will improve that. This code came from the vkms, which follows the 80
chars limit. If I'm not mistaken.
+ return -EINVAL;
+ }
+
+ return 0;
If you do this, you can get rid of the format_supported flag:
for(...) {
if (fb->format->format == formats[i])
return 0;
}
DRM_DEBUG_KMS(...);
return -EINVAL;
Indeed. Thanks!
Yes, that looks nicer.
Thanks,
Leandro Ribeiro
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
+
/**
* drm_atomic_helper_check_plane_state() - Check plane sta
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct
> > drm_connector *connector,
> > u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > struct drm_scdc *scdc = &hdmi->scdc;
> >
> > - if (max_tmds_clock > 34) {
> > + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > display->max_tmds_clock = max_tmds_clock;
> > DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > display->max_tmds_clock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index d2e61f6c6e08..0666203d52b7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder
> > *encoder,
> > if (scdc->scrambling.low_rates)
> > pipe_config->hdmi_scrambling = true;
> >
> > - if (pipe_config->port_clock > 34) {
> > + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > pipe_config->hdmi_scrambling = true;
> > pipe_config->hdmi_high_tmds_clock_ratio = true;
> > }
>
> All of that is HDMI 2.0 stuff. So this just makes it all super
> confusing IMO. Nak.
So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
of upper limit for the physical cable. But nowhere else is that number
really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
Mcsc limit in various places.
I wonder what people would think of a couple of helpers like:
- drm_hdmi_{can,must}_use_scrambling()
- drm_hdmi_is_high_tmds_clock_ratio()
or something along those lines? At least with those the code would
read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
clock limit really is.
--
Ville Syrjälä
Intel
[Bug 214621] WARNING: CPU: 3 PID: 521 at drivers/gpu/drm/ttm/ttm_bo.c:409 ttm_bo_release+0xb64/0xe40 [ttm]
https://bugzilla.kernel.org/show_bug.cgi?id=214621 --- Comment #6 from Erhard F. (erhar...@mailbox.org) --- (In reply to Lang Yu from comment #5) > Created attachment 299383 [details] > fix a potential dma-buf release warning > > Please have a try with attached patch. Thanks! Thanks! Applied the patch on top of v5.15 but still get: [...] [ cut here ] WARNING: CPU: 2 PID: 519 at drivers/gpu/drm/ttm/ttm_bo.c:409 ttm_bo_release+0xb64/0xe40 [ttm] Modules linked in: rfkill dm_crypt nhpoly1305_sse2 nhpoly1305 chacha_generic chacha_x86_64 libchacha adiantum libpoly1305 algif_skcipher joydev input_leds led_class hid_generic usbhid dm_mod hid ohci_pci raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx evdev f2fs crc32_generic lz4hc_compress lz4_compress lz4_decompress crc32_pclmul amdgpu md_mod aesni_intel libaes crypto_simd cryptd ext4 crc16 mbcache fam15h_power snd_hda_codec_hdmi jbd2 k10temp ehci_pci ohci_hcd snd_hda_intel ehci_hcd snd_intel_dspcfg xhci_pci drm_ttm_helper i2c_piix4 snd_hda_codec ttm mfd_core snd_hwdep snd_hda_core gpu_sched xhci_hcd i2c_algo_bit snd_pcm drm_kms_helper usbcore snd_timer syscopyarea sysfillrect snd sysimgblt usb_common fb_sys_fops soundcore acpi_cpufreq video processor button zram zsmalloc nfsd nct6775 hwmon_vid hwmon auth_rpcgss drm lockd grace fuse drm_panel_orientation_quirks backlight configfs sunrpc efivarfs CPU: 2 PID: 519 Comm: X Not tainted 5.15.0-bdver3+ #3 Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./A88M-G/3.1, BIOS P1.40C 11/21/2016 RIP: 0010:ttm_bo_release+0xb64/0xe40 [ttm] Code: c1 ea 03 80 3c 02 00 0f 85 77 01 00 00 48 8b bb f0 fe ff ff b9 28 23 00 00 31 d2 be 01 00 00 00 e8 81 c9 54 da e9 d3 fe ff ff <0f> 0b e9 1c f5 ff ff 4c 89 e7 e8 4d 5a 54 da e9 26 fc ff ff be 03 RSP: 0018:c900018afb18 EFLAGS: 00010202 RAX: 0007 RBX: 88813d2a7298 RCX: 001c RDX: RSI: 0004 RDI: 88813d2a7298 RBP: 88813d2a7000 R08: c0b63689 R09: 88813d2a729b R10: ed1027a54e53 R11: 0001 R12: dc00 R13: 8881748d03a8 R14: 8881748d03f0 R15: 88810b138b40 FS: 7fa8bfe7adc0() GS:8883d170() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 562b379a2098 CR3: 00014297 CR4: 000506e0 Call Trace: ? fsnotify_grab_connector+0xcc/0x190 amdgpu_bo_unref+0x2c/0x60 [amdgpu] amdgpu_gem_object_free+0xc0/0x100 [amdgpu] ? amdgpu_gem_object_mmap+0xe0/0xe0 [amdgpu] ? call_rcu+0x37f/0x730 ? trace_hardirqs_on+0x1c/0x110 drm_gem_dmabuf_release+0x82/0xb0 [drm] dma_buf_release+0x127/0x230 __dentry_kill+0x376/0x550 ? dma_buf_file_release+0x177/0x200 __fput+0x2c0/0x8c0 task_work_run+0xc5/0x150 do_exit+0x799/0x20c0 ? mm_update_next_owner+0x6d0/0x6d0 do_group_exit+0xe7/0x290 __x64_sys_exit_group+0x35/0x40 do_syscall_64+0x66/0x90 ? do_syscall_64+0xe/0x90 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7fa8bf6fc2f9 Code: Unable to access opcode bytes at RIP 0x7fa8bf6fc2cf. RSP: 002b:7ffc95722778 EFLAGS: 0246 ORIG_RAX: 00e7 RAX: ffda RBX: 7fa8bf7e4920 RCX: 7fa8bf6fc2f9 RDX: 003c RSI: 00e7 RDI: RBP: 7fa8bf7e4920 R08: fd40 R09: 0098b190 R10: 7fa8bef086b8 R11: 0246 R12: R13: R14: 066a R15: irq event stamp: 887428545 hardirqs last enabled at (887428551): [] vprintk_emit+0x2dc/0x310 hardirqs last disabled at (887428556): [] vprintk_emit+0x28b/0x310 softirqs last enabled at (887427644): [] __irq_exit_rcu+0xe5/0x120 softirqs last disabled at (887427625): [] __irq_exit_rcu+0xe5/0x120 ---[ end trace 1b4ae7cf543ff5f4 ]--- [...] It does not trigger on every reboot though, the machine needs to have been running for a few hrs. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 214029] [bisected] [amdgpu] Several memory leaks in amdgpu and ttm
https://bugzilla.kernel.org/show_bug.cgi?id=214029 --- Comment #25 from Erhard F. (erhar...@mailbox.org) --- (In reply to Jan Steffens from comment #24) > Looks like this was mistakenly picked into LTS 5.10, which does not contain > d02117f8efaa5fbc37437df1ae955a147a2a424a. As Christian wrote in comment 21 the patches bisected didn't cause the memleak, but rather just made it more likely to appear. So the patch (0db55f9a1bafbe3dac750ea669de9134922389b5) most probably wandered correctly in 5.10 LTS and 5.4 LTS. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[PATCH v5] backlight: lp855x: Switch to atomic PWM API
Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
replace it for the atomic PWM API.
Signed-off-by: Maíra Canal
---
V1 -> V2: Initialize variable and simplify conditional loop
V2 -> V3: Fix assignment of NULL variable
V3 -> V4: Replace division for pwm_set_relative_duty_cycle
V4 -> V5: Fix overwrite of state.period
---
drivers/video/backlight/lp855x_bl.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index e94932c69f54..e70a7b72dcf3 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
{
- unsigned int period = lp->pdata->period_ns;
- unsigned int duty = br * period / max_br;
struct pwm_device *pwm;
+ struct pwm_state state;
/* request pwm device with the consumer name */
if (!lp->pwm) {
@@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br,
int max_br)
lp->pwm = pwm;
- /*
-* FIXME: pwm_apply_args() should be removed when switching to
-* the atomic PWM API.
-*/
- pwm_apply_args(pwm);
+ pwm_init_state(lp->pwm, &state);
+ state.period = lp->pdata->period_ns;
+ } else {
+ pwm_get_state(lp->pwm, &state);
}
- pwm_config(lp->pwm, duty, period);
- if (duty)
- pwm_enable(lp->pwm);
- else
- pwm_disable(lp->pwm);
+ pwm_set_relative_duty_cycle(&state, br, max_br);
+ state.enabled = state.duty_cycle;
+
+ pwm_apply_state(lp->pwm, &state);
}
static int lp855x_bl_update_status(struct backlight_device *bl)
--
2.31.1
Re: [PATCH v2 3/3] backlight: lp855x: Add support ACPI enumeration
Hi, On 11/3/21 18:31, Daniel Thompson wrote: > On Wed, Nov 03, 2021 at 06:28:15PM +0100, Hans de Goede wrote: >> Hi, >> >> On 11/3/21 18:17, Daniel Thompson wrote: >>> On Tue, Nov 02, 2021 at 11:55:04PM +0100, Hans de Goede wrote: The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight controller for its LCD-panel, with a Xiaomi specific ACPI HID of "XMCC0001", add support for this. Note the new "if (id)" check also fixes a NULL pointer deref when a user tries to manually bind the driver from sysfs. When CONFIG_ACPI is disabled acpi_match_device() will always return NULL, so the lp855x_parse_acpi() call will get optimized away. Signed-off-by: Hans de Goede >>> >>> Reviewed-by: Daniel Thompson >> >> Thank you. >> >> So what is the process for upstreaming backlight patches, >> do these go through drm-misc-next (in that case I can push >> the series myself), or will you pick these up ? > > Lee Jones gathers up the backlight patches and sends a PR (but, except > in exceptional cases, treats my R-b as a pre-requisite before doing so). Ok, thanks. Regards, Hans
Re: [PATCH v2 3/3] backlight: lp855x: Add support ACPI enumeration
On Wed, Nov 03, 2021 at 06:28:15PM +0100, Hans de Goede wrote: > Hi, > > On 11/3/21 18:17, Daniel Thompson wrote: > > On Tue, Nov 02, 2021 at 11:55:04PM +0100, Hans de Goede wrote: > >> The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight > >> controller for its LCD-panel, with a Xiaomi specific ACPI HID of > >> "XMCC0001", add support for this. > >> > >> Note the new "if (id)" check also fixes a NULL pointer deref when a user > >> tries to manually bind the driver from sysfs. > >> > >> When CONFIG_ACPI is disabled acpi_match_device() will always return NULL, > >> so the lp855x_parse_acpi() call will get optimized away. > >> > >> Signed-off-by: Hans de Goede > > > > Reviewed-by: Daniel Thompson > > Thank you. > > So what is the process for upstreaming backlight patches, > do these go through drm-misc-next (in that case I can push > the series myself), or will you pick these up ? Lee Jones gathers up the backlight patches and sends a PR (but, except in exceptional cases, treats my R-b as a pre-requisite before doing so). Daniel.
Re: [PATCH v2 3/3] backlight: lp855x: Add support ACPI enumeration
Hi, On 11/3/21 18:17, Daniel Thompson wrote: > On Tue, Nov 02, 2021 at 11:55:04PM +0100, Hans de Goede wrote: >> The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight >> controller for its LCD-panel, with a Xiaomi specific ACPI HID of >> "XMCC0001", add support for this. >> >> Note the new "if (id)" check also fixes a NULL pointer deref when a user >> tries to manually bind the driver from sysfs. >> >> When CONFIG_ACPI is disabled acpi_match_device() will always return NULL, >> so the lp855x_parse_acpi() call will get optimized away. >> >> Signed-off-by: Hans de Goede > > Reviewed-by: Daniel Thompson Thank you. So what is the process for upstreaming backlight patches, do these go through drm-misc-next (in that case I can push the series myself), or will you pick these up ? Regards, Hans
Re: [PATCH v2 3/3] backlight: lp855x: Add support ACPI enumeration
On Tue, Nov 02, 2021 at 11:55:04PM +0100, Hans de Goede wrote: > The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight > controller for its LCD-panel, with a Xiaomi specific ACPI HID of > "XMCC0001", add support for this. > > Note the new "if (id)" check also fixes a NULL pointer deref when a user > tries to manually bind the driver from sysfs. > > When CONFIG_ACPI is disabled acpi_match_device() will always return NULL, > so the lp855x_parse_acpi() call will get optimized away. > > Signed-off-by: Hans de Goede Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 6/6] drm/radeon: use dma_resv_wait_timeout() instead of manually waiting
Acked-by: Nirmoy Das
On 10/28/2021 3:26 PM, Christian König wrote:
Don't touch the exclusive fence manually here, but rather use the
general dma_resv function. We did that for better hw reset handling but
this doesn't necessary work correctly.
Signed-off-by: Christian König
---
drivers/gpu/drm/radeon/radeon_uvd.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c
b/drivers/gpu/drm/radeon/radeon_uvd.c
index 2ea86919d953..377f9cdb5b53 100644
--- a/drivers/gpu/drm/radeon/radeon_uvd.c
+++ b/drivers/gpu/drm/radeon/radeon_uvd.c
@@ -469,7 +469,6 @@ static int radeon_uvd_cs_msg(struct radeon_cs_parser *p,
struct radeon_bo *bo,
{
int32_t *msg, msg_type, handle;
unsigned img_size = 0;
- struct dma_fence *f;
void *ptr;
int i, r;
@@ -479,13 +478,11 @@ static int radeon_uvd_cs_msg(struct radeon_cs_parser *p,
struct radeon_bo *bo,
return -EINVAL;
}
- f = dma_resv_excl_fence(bo->tbo.base.resv);
- if (f) {
- r = radeon_fence_wait((struct radeon_fence *)f, false);
- if (r) {
- DRM_ERROR("Failed waiting for UVD message (%d)!\n", r);
- return r;
- }
+ r = dma_resv_wait_timeout(bo->tbo.base.resv, false, false,
+ MAX_SCHEDULE_TIMEOUT);
+ if (r <= 0) {
+ DRM_ERROR("Failed waiting for UVD message (%d)!\n", r);
+ return r ? r : -ETIME;
}
r = radeon_bo_kmap(bo, &ptr);
Re: [PATCH v9 10/10] drm: use DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES bitmap to tracefs
On 10/27/21 12:36 AM, Jim Cromie wrote:
> Use new macro to create a sysfs control bitmap knob to control
> print-to-trace in: /sys/module/drm/parameters/trace
>
> todo: reconsider this api, ie a single macro expecting both debug &
> trace terms (2 each), followed by a single description and the
> bitmap-spec::
>
> Good: declares bitmap once for both interfaces
>
> Bad: arg-type/count handling (expecting 4 args) is ugly,
> especially preceding the bitmap-init var-args.
>
Hi Jim,
I agree having the bitmap declared twice seems redundant. But I like having
fewer args and not necessarily combining the trace/log variants of
DEBUG_CATEGORIES. hmmm...what if the DEFINE_DYNAMIC_DEBUG_CATEGORIES() took a
pointer to the array of struct dyndbg_bitdesc map[] directly as the
final argument instead of the __VA_ARGS__? Then, we could just declare the map
once?
Thanks,
-Jason
> Signed-off-by: Jim Cromie
> ---
> drivers/gpu/drm/drm_print.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index ce662d0f339b..7b49fbc5e21d 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -73,6 +73,25 @@ DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
> [7] = { DRM_DBG_CAT_LEASE },
> [8] = { DRM_DBG_CAT_DP },
> [9] = { DRM_DBG_CAT_DRMRES });
> +
> +#ifdef CONFIG_TRACING
> +unsigned long __drm_trace;
> +EXPORT_SYMBOL(__drm_trace);
> +DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES(trace, __drm_trace,
> + DRM_DEBUG_DESC,
> + [0] = { DRM_DBG_CAT_CORE },
> + [1] = { DRM_DBG_CAT_DRIVER },
> + [2] = { DRM_DBG_CAT_KMS },
> + [3] = { DRM_DBG_CAT_PRIME },
> + [4] = { DRM_DBG_CAT_ATOMIC },
> + [5] = { DRM_DBG_CAT_VBL },
> + [6] = { DRM_DBG_CAT_STATE },
> + [7] = { DRM_DBG_CAT_LEASE },
> + [8] = { DRM_DBG_CAT_DP },
> + [9] = { DRM_DBG_CAT_DRMRES });
> +
> +struct trace_array *trace_arr;
> +#endif
> #endif
>
> void __drm_puts_coredump(struct drm_printer *p, const char *str)
>
Re: [PATCH v2 4/8] drm: vkms: Add fb information to `vkms_writeback_job`
Hi
Am 26.10.21 um 13:34 schrieb Igor Torrente:
This commit is the groundwork to introduce new formats to the planes and
writeback buffer. As part of it, a new buffer metadata field is added to
`vkms_writeback_job`, this metadata is represented by the `vkms_composer`
struct.
This will allow us, in the future, to have different compositing and wb
format types.
Signed-off-by: Igor Torrente
---
V2: Change the code to get the drm_framebuffer reference and not copy its
contents(Thomas Zimmermann).
---
drivers/gpu/drm/vkms/vkms_composer.c | 4 ++--
drivers/gpu/drm/vkms/vkms_drv.h | 12 ++--
drivers/gpu/drm/vkms/vkms_plane.c | 10 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 21 ++---
4 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
b/drivers/gpu/drm/vkms/vkms_composer.c
index 82f79e508f81..383ca657ddf7 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -153,7 +153,7 @@ static void compose_plane(struct vkms_composer
*primary_composer,
struct vkms_composer *plane_composer,
void *vaddr_out)
{
- struct drm_framebuffer *fb = &plane_composer->fb;
+ struct drm_framebuffer *fb = plane_composer->fb;
void *vaddr;
void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
@@ -174,7 +174,7 @@ static int compose_active_planes(void **vaddr_out,
struct vkms_composer *primary_composer,
struct vkms_crtc_state *crtc_state)
{
- struct drm_framebuffer *fb = &primary_composer->fb;
+ struct drm_framebuffer *fb = primary_composer->fb;
struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
const void *vaddr;
int i;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 64e62993b06f..9e4c1e95bbb1 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -20,13 +20,8 @@
#define XRES_MAX 8192
#define YRES_MAX 8192
-struct vkms_writeback_job {
- struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
- struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
-};
-
struct vkms_composer {
- struct drm_framebuffer fb;
+ struct drm_framebuffer *fb;
struct drm_rect src, dst;
struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
unsigned int offset;
@@ -34,6 +29,11 @@ struct vkms_composer {
unsigned int cpp;
};
+struct vkms_writeback_job {
+ struct dma_buf_map data[DRM_FORMAT_MAX_PLANES];
+ struct vkms_composer composer;
+};
+
/**
* vkms_plane_state - Driver specific plane state
* @base: base plane state
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c
b/drivers/gpu/drm/vkms/vkms_plane.c
index 32409e15244b..0a28cb7a85e2 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -50,12 +50,12 @@ static void vkms_plane_destroy_state(struct drm_plane
*plane,
struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
struct drm_crtc *crtc = vkms_state->base.base.crtc;
- if (crtc) {
+ if (crtc && vkms_state->composer->fb) {
/* dropping the reference we acquired in
* vkms_primary_plane_update()
*/
- if (drm_framebuffer_read_refcount(&vkms_state->composer->fb))
- drm_framebuffer_put(&vkms_state->composer->fb);
+ if (drm_framebuffer_read_refcount(vkms_state->composer->fb))
+ drm_framebuffer_put(vkms_state->composer->fb);
}
kfree(vkms_state->composer);
@@ -110,9 +110,9 @@ static void vkms_plane_atomic_update(struct drm_plane
*plane,
composer = vkms_plane_state->composer;
memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
- memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
+ composer->fb = fb;
memcpy(&composer->map, &shadow_plane_state->data,
sizeof(composer->map));
- drm_framebuffer_get(&composer->fb);
+ drm_framebuffer_get(composer->fb);
composer->offset = fb->offsets[0];
composer->pitch = fb->pitches[0];
composer->cpp = fb->format->cpp[0];
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
b/drivers/gpu/drm/vkms/vkms_writeback.c
index 8694227f555f..32734cdbf6c2 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -75,12 +75,15 @@ static int vkms_wb_prepare_job(struct
drm_writeback_connector *wb_connector,
if (!vkmsjob)
return -ENOMEM;
- ret = drm_gem_fb_vmap(job->fb, vkmsjob->map, vkmsjob->data);
+ ret = drm_gem_fb_vmap(job->fb, vkmsjob->composer.map, vkmsjob->data);
if (ret) {
DRM_ERROR("vmap failed: %d\n", ret);
Re: [PATCH v2 3/8] drm: vkms: Replace hardcoded value of `vkms_composer.map` to DRM_FORMAT_MAX_PLANES
Hi
Am 26.10.21 um 13:34 schrieb Igor Torrente:
The `map` vector at `vkms_composer` uses a hardcoded value to define its
size.
If someday the maximum number of planes increases, this hardcoded value
can be a problem.
This value is being replaced with the DRM_FORMAT_MAX_PLANES macro.
Signed-off-by: Igor Torrente
Acked-by: Thomas Zimmermann
We can merge that immediately.
Best regards
Thomas
---
drivers/gpu/drm/vkms/vkms_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index d48c23d40ce5..64e62993b06f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -28,7 +28,7 @@ struct vkms_writeback_job {
struct vkms_composer {
struct drm_framebuffer fb;
struct drm_rect src, dst;
- struct dma_buf_map map[4];
+ struct dma_buf_map map[DRM_FORMAT_MAX_PLANES];
unsigned int offset;
unsigned int pitch;
unsigned int cpp;
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation
Hi
Am 03.11.21 um 16:11 schrieb Leandro Ribeiro:
Hi,
On 11/3/21 12:03, Igor Torrente wrote:
Hi Leandro,
On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
Hi,
On 10/26/21 08:34, Igor Torrente wrote:
Add a helper function to validate the connector configuration receive in
the encoder atomic_check by the drivers.
So the drivers don't need do these common validations themselves.
Signed-off-by: Igor Torrente
---
V2: Move the format verification to a new helper at the
drm_atomic_helper.c
(Thomas Zimmermann).
---
drivers/gpu/drm/drm_atomic_helper.c | 47 +++
drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++--
include/drm/drm_atomic_helper.h | 3 ++
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 2c0c6ec92820..c2653b9824b5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct
drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
+/**
+ * drm_atomic_helper_check_wb_connector_state() - Check writeback
encoder state
+ * @encoder: encoder state to check
+ * @conn_state: connector state to check
+ *
+ * Checks if the wriback connector state is valid, and returns a
'writeback'
'an error'
erros if it
'error'
+ * isn't.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int
+drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
+ struct drm_connector_state *conn_state)
+{
+ struct drm_writeback_job *wb_job = conn_state->writeback_job;
+ struct drm_property_blob *pixel_format_blob;
+ bool format_supported = false;
+ struct drm_framebuffer *fb;
+ int i, n_formats;
Just 'nformats'.
Please make both variables 'size_t'.
+ u32 *formats;
+
+ if (!wb_job || !wb_job->fb)
+ return 0;
I think that this should be removed and that this functions should
assume that (wb_job && wb_job->fb) == true.
Ok.
In regular atomic check for planes, there can be planes with no attached
framebuffer. Helpers handle this situation. [1] I don't know if this is
possible in writeback code, but for consistency, it would make sense to
keep this test here. Not sure though.
Actually, it's weird to have conn_state as argument and only use it to
get the wb_job. Instead, this function could receive wb_job directly.
In the Thomas review of v1, he said that maybe other things could be
tested in this helper. I'm not sure what these additional checks could
be, so I tried to design the function signature expecting more things
to be added after his review.
As you can see, the helper is receiving the `drm_encoder` and doing
nothing with it.
If we, eventually, don't find anything else that this helper can do, I
will revert to something very similar (if not equal) to your proposal.
I just want to wait for Thomas's review first.
Sure, that makes sense.
We had many helper functions for atomic modesetting that took various
arguments for whatever they required. Extending such a function with new
functionality/arguments required required touching many drivers and made
the parameter list hard to read. At some point, Maxime went through most
of the code and unified it all to pass full state to the helpers.
So please keep the connector state. I think it's how we do things ATM.
Thanks,
Leandro Ribeiro
Of course, its name/description would have to change.
+
+ pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
+ n_formats = pixel_format_blob->length / sizeof(u32);
+ formats = pixel_format_blob->data;
+ fb = wb_job->fb;
+
+ for (i = 0; i < n_formats; i++) {
+ if (fb->format->format == formats[i]) {
+ format_supported = true;
+ break;
+ }
+ }
+
+ if (!format_supported) {
+ DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
+ &fb->format->format);
Please use drm_dgb_kms() instead. There's a 100-character-per-line
limit. The comment probably fits onto a single line.(?)
+ return -EINVAL;
+ }
+
+ return 0;
If you do this, you can get rid of the format_supported flag:
for(...) {
if (fb->format->format == formats[i])
return 0;
}
DRM_DEBUG_KMS(...);
return -EINVAL;
Indeed. Thanks!
Yes, that looks nicer.
Thanks,
Leandro Ribeiro
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
+
/**
* drm_atomic_helper_check_plane_state() - Check plane state for
validity
* @plane_state: plane state to check
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
b/drivers/gpu/drm/vkms/vkms_writeback.c
index 32734cdbf6c2..42f3396c523a 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct
drm_encoder *encoder,
{
struct drm_framebuffer *fb
Re: [PATCH v2] media: mtk-vcodec: Align width and height to 64 bytes
Le mercredi 03 novembre 2021 à 11:37 +0800, Yunfei Dong a écrit :
> Width and height need to 64 bytes aligned when setting the format.
> Need to make sure all is 64 bytes align when use width and height to
> calculate buffer size.
>
> Signed-off-by: Yunfei Dong
> Change-Id: I39886b1a6b433c92565ddbf297eb193456eec1d2
Perhaps avoid this tag later ? Another perhaps, there is a tag to indicate which
patch introduce that bug, if you add this tag, the patch will be automatically
backported into relevant stable kernel. The format is:
> Fixes: ("
> ---
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h| 1 +
> drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c | 4 ++--
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> index e30806c1faea..66cd6d2242c3 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> @@ -11,6 +11,7 @@
> #include
> #include
>
> +#define VCODEC_DEC_ALIGNED_64 64
> #define VCODEC_CAPABILITY_4K_DISABLED0x10
> #define VCODEC_DEC_4K_CODED_WIDTH4096U
> #define VCODEC_DEC_4K_CODED_HEIGHT 2304U
> diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> index d402fc4bda69..e1a3011772a9 100644
> --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> @@ -562,8 +562,8 @@ static void get_pic_info(struct vdec_h264_slice_inst
> *inst,
> {
> struct mtk_vcodec_ctx *ctx = inst->ctx;
>
> - ctx->picinfo.buf_w = (ctx->picinfo.pic_w + 15) & 0xFFF0;
> - ctx->picinfo.buf_h = (ctx->picinfo.pic_h + 31) & 0xFFE0;
> + ctx->picinfo.buf_w = ALIGN(ctx->picinfo.pic_w, VCODEC_DEC_ALIGNED_64);
> + ctx->picinfo.buf_h = ALIGN(ctx->picinfo.pic_h, VCODEC_DEC_ALIGNED_64);
> ctx->picinfo.fb_sz[0] = ctx->picinfo.buf_w * ctx->picinfo.buf_h;
> ctx->picinfo.fb_sz[1] = ctx->picinfo.fb_sz[0] >> 1;
> inst->vsi_ctx.dec.cap_num_planes =
Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation
Hi,
On 11/3/21 12:03, Igor Torrente wrote:
> Hi Leandro,
>
> On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
>> Hi,
>>
>> On 10/26/21 08:34, Igor Torrente wrote:
>>> Add a helper function to validate the connector configuration receive in
>>> the encoder atomic_check by the drivers.
>>>
>>> So the drivers don't need do these common validations themselves.
>>>
>>> Signed-off-by: Igor Torrente
>>> ---
>>> V2: Move the format verification to a new helper at the
>>> drm_atomic_helper.c
>>> (Thomas Zimmermann).
>>> ---
>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++
>>> drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++--
>>> include/drm/drm_atomic_helper.h | 3 ++
>>> 3 files changed, 54 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 2c0c6ec92820..c2653b9824b5 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct
>>> drm_device *dev,
>>> }
>>> EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
>>> +/**
>>> + * drm_atomic_helper_check_wb_connector_state() - Check writeback
>>> encoder state
>>> + * @encoder: encoder state to check
>>> + * @conn_state: connector state to check
>>> + *
>>> + * Checks if the wriback connector state is valid, and returns a
>>> erros if it
>>> + * isn't.
>>> + *
>>> + * RETURNS:
>>> + * Zero for success or -errno
>>> + */
>>> +int
>>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
>>> + struct drm_connector_state *conn_state)
>>> +{
>>> + struct drm_writeback_job *wb_job = conn_state->writeback_job;
>>> + struct drm_property_blob *pixel_format_blob;
>>> + bool format_supported = false;
>>> + struct drm_framebuffer *fb;
>>> + int i, n_formats;
>>> + u32 *formats;
>>> +
>>> + if (!wb_job || !wb_job->fb)
>>> + return 0;
>>
>> I think that this should be removed and that this functions should
>> assume that (wb_job && wb_job->fb) == true.
>
> Ok.
>
>>
>> Actually, it's weird to have conn_state as argument and only use it to
>> get the wb_job. Instead, this function could receive wb_job directly.
>
> In the Thomas review of v1, he said that maybe other things could be
> tested in this helper. I'm not sure what these additional checks could
> be, so I tried to design the function signature expecting more things
> to be added after his review.
>
> As you can see, the helper is receiving the `drm_encoder` and doing
> nothing with it.
>
> If we, eventually, don't find anything else that this helper can do, I
> will revert to something very similar (if not equal) to your proposal.
> I just want to wait for Thomas's review first.
>
Sure, that makes sense.
Thanks,
Leandro Ribeiro
>>
>> Of course, its name/description would have to change.
>>
>>> +
>>> + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
>>> + n_formats = pixel_format_blob->length / sizeof(u32);
>>> + formats = pixel_format_blob->data;
>>> + fb = wb_job->fb;
>>> +
>>> + for (i = 0; i < n_formats; i++) {
>>> + if (fb->format->format == formats[i]) {
>>> + format_supported = true;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!format_supported) {
>>> + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
>>> + &fb->format->format);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>
>> If you do this, you can get rid of the format_supported flag:
>>
>> for(...) {
>> if (fb->format->format == formats[i])
>> return 0;
>> }
>>
>>
>> DRM_DEBUG_KMS(...);
>> return -EINVAL;
>>
>
> Indeed. Thanks!
>
>> Thanks,
>> Leandro Ribeiro
>>
>>> +}
>>> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
>>> +
>>> /**
>>> * drm_atomic_helper_check_plane_state() - Check plane state for
>>> validity
>>> * @plane_state: plane state to check
>>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
>>> b/drivers/gpu/drm/vkms/vkms_writeback.c
>>> index 32734cdbf6c2..42f3396c523a 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
>>> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct
>>> drm_encoder *encoder,
>>> {
>>> struct drm_framebuffer *fb;
>>> const struct drm_display_mode *mode = &crtc_state->mode;
>>> + int ret;
>>> if (!conn_state->writeback_job ||
>>> !conn_state->writeback_job->fb)
>>> return 0;
>>> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct
>>> drm_encoder *encoder,
>>> return -EINVAL;
>>> }
>>> - if (fb->format->format != vkms_wb_formats[0]) {
>>> - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
>>> - &fb->format->format);
>>> - return -EINVAL;
>>> - }
>>> + ret = drm_atomic_helper_check_wb_encode
Re: [RFC v2 05/22] drm/i915/xelpd: Define Degamma Lut range struct for HDR planes
On 2021-09-06 17:38, Uma Shankar wrote:
> Define the structure with XE_LPD degamma lut ranges. HDR and SDR
> planes have different capabilities, implemented respective
> structure for the HDR planes.
>
> Signed-off-by: Uma Shankar
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 52 ++
> 1 file changed, 52 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index afcb4bf3826c..6403bd74324b 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -2092,6 +2092,58 @@ static void icl_read_luts(struct intel_crtc_state
> *crtc_state)
> }
> }
>
> + /* FIXME input bpc? */
> +__maybe_unused
> +static const struct drm_color_lut_range d13_degamma_hdr[] = {
> + /* segment 1 */
> + {
> + .flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_NON_DECREASING),
> + .count = 128,
Is the distribution of the 128 entries uniform? If so, is a
uniform distribution of 128 points across most of the LUT
good enough for HDR with 128 entries?
> + .input_bpc = 24, .output_bpc = 16,
> + .start = 0, .end = (1 << 24) - 1,
> + .min = 0, .max = (1 << 24) - 1,
> + },
> + /* segment 2 */
> + {
> + .flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_REUSE_LAST |
> + DRM_MODE_LUT_NON_DECREASING),
> + .count = 1,
> + .input_bpc = 24, .output_bpc = 16,
> + .start = (1 << 24) - 1, .end = 1 << 24,
.start and .end are only a single entry apart. Is this correct?
Harry
> + .min = 0, .max = (1 << 27) - 1,
> + },
> + /* Segment 3 */
> + {
> + .flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_REUSE_LAST |
> + DRM_MODE_LUT_NON_DECREASING),
> + .count = 1,
> + .input_bpc = 24, .output_bpc = 16,
> + .start = 1 << 24, .end = 3 << 24,
> + .min = 0, .max = (1 << 27) - 1,
> + },
> + /* Segment 4 */
> + {
> + .flags = (DRM_MODE_LUT_GAMMA |
> + DRM_MODE_LUT_REFLECT_NEGATIVE |
> + DRM_MODE_LUT_INTERPOLATE |
> + DRM_MODE_LUT_REUSE_LAST |
> + DRM_MODE_LUT_NON_DECREASING),
> + .count = 1,
> + .input_bpc = 24, .output_bpc = 16,
> + .start = 3 << 24, .end = 7 << 24,
> + .min = 0, .max = (1 << 27) - 1,
> + },
> +};
> +
> void intel_color_init(struct intel_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>
Re: [RFC v2 02/22] drm: Add Enhanced Gamma and color lut range attributes
On 2021-09-06 17:38, Uma Shankar wrote:
> Existing LUT precision structure is having only 16 bit
> precision. This is not enough for upcoming enhanced hardwares
> and advance usecases like HDR processing. Hence added a new
> structure with 32 bit precision values.
>
> This also defines a new structure to define color lut ranges,
> along with related macro definitions and enums. This will help
> describe multi segmented lut ranges in the hardware.
>
> Signed-off-by: Uma Shankar
> ---
> include/uapi/drm/drm_mode.h | 58 +
> 1 file changed, 58 insertions(+)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 90c55383f1ee..1079794c86c3 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -903,6 +903,64 @@ struct hdr_output_metadata {
> };
> };
>
> +/*
> + * DRM_MODE_LUT_GAMMA|DRM_MODE_LUT_DEGAMMA is legal and means the LUT
> + * can be used for either purpose, but not simultaneously. To expose
> + * modes that support gamma and degamma simultaneously the gamma mode
> + * must declare distinct DRM_MODE_LUT_GAMMA and DRM_MODE_LUT_DEGAMMA
> + * ranges.
> + */
> +/* LUT is for gamma (after CTM) */
> +#define DRM_MODE_LUT_GAMMA BIT(0)
> +/* LUT is for degamma (before CTM) */
> +#define DRM_MODE_LUT_DEGAMMA BIT(1)
> +/* linearly interpolate between the points */
> +#define DRM_MODE_LUT_INTERPOLATE BIT(2)
> +/*
> + * the last value of the previous range is the
> + * first value of the current range.
> + */
> +#define DRM_MODE_LUT_REUSE_LAST BIT(3)
> +/* the curve must be non-decreasing */
> +#define DRM_MODE_LUT_NON_DECREASING BIT(4)
> +/* the curve is reflected across origin for negative inputs */
> +#define DRM_MODE_LUT_REFLECT_NEGATIVE BIT(5)
> +/* the same curve (red) is used for blue and green channels as well */
> +#define DRM_MODE_LUT_SINGLE_CHANNEL BIT(6)
> +
> +struct drm_color_lut_range {
> + /* DRM_MODE_LUT_* */
> + __u32 flags;
> + /* number of points on the curve */
> + __u16 count;
> + /* input/output bits per component */
> + __u8 input_bpc, output_bpc;
> + /* input start/end values */
> + __s32 start, end;
> + /* output min/max values */
> + __s32 min, max;
> +};
> +
> +enum lut_type {
> + LUT_TYPE_DEGAMMA = 0,
> + LUT_TYPE_GAMMA = 1,
> +};
> +
> +/*
> + * Creating 64 bit palette entries for better data
> + * precision. This will be required for HDR and
> + * similar color processing usecases.
> + */
> +struct drm_color_lut_ext {
> + /*
> + * Data is U32.32 fixed point format.
> + */
> + __u64 red;
> + __u64 green;
> + __u64 blue;
> + __u64 reserved;
> +};
I've been drawing out examples of drm_color_lut_range defined PWLs
and understand a bit better what you and Ville are trying to accomplish
with it. It actually makes a lot of sense and would allow for a generic
way to populate different PWL definitions with a generic function.
But I still have some key questions that either are not answered in these
patch sets or that I somehow overlooked.
Can you explain how the U32.32 fixed point format relates to the input_bpc
and output_bpc in drm_color_lut_range, as we as to the pixel coming in from
the framebuffer.
E.g. if we have ARGB2101010 what happens to a 0x3ff red value (assuming alpha
is non-multiplied)?
If the drm_color_lut_range segments are defined with input_bpc of 24 bpc will
0x3ff be zero-expanded to 24-bit? Is the 24 bpc an integer? I.e. would our 3xff
be interpreted as 0x3ff << (24-10)?
Assuming the output_bpc is 16 bpc and the programmed LUT makes this 1-to-1 would
the output value be 0x3ff << (16-10)?
On AMD HW the pipe-internal format is a custom floating point format. We could
probably express that in terms of input/output_bpc and do the translation in our
driver between that and the internal floating point format, depending on the
framebuffer format, but there is the added complication of the magnitude of the
pixel data and correlating HDR with SDR planes.
E.g. any SDR data would map from 0.0 to 1.0 floating point, while HDR content
would
map from 0.0 to some value larger than 1.0. I don't (yet) have a clear picture
how
to represent that with the drm_color_lut_range definition.
If some of these questions should be obvious I apologize for being a bit dense,
though it helps to make this accessible to the lowest common denominator
to ensure not only the smartest devs can work with this.
Harry
> +
> #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>
[Bug 214029] [bisected] [amdgpu] Several memory leaks in amdgpu and ttm
https://bugzilla.kernel.org/show_bug.cgi?id=214029 Jan Steffens (jan.steff...@gmail.com) changed: What|Removed |Added CC||jan.steff...@gmail.com --- Comment #24 from Jan Steffens (jan.steff...@gmail.com) --- Looks like this was mistakenly picked into LTS 5.10, which does not contain d02117f8efaa5fbc37437df1ae955a147a2a424a. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v11 4/4] drm/bridge: anx7625: add HDMI audio function
Hey Xin, This series does not apply on drm-misc-next. Please fix this and resend. Make sure that checkpatch --strict passes as well. On Wed, 3 Nov 2021 at 15:20, Dan Carpenter wrote: > > This is a super awkward way to resend a patch series. Next time, just > start a new thread and put [PATCH RESEND] in the subject. > > I am sorry that no one responded to your thread. :/ > > regards, > dan carpenter
Re: [PATCH v2 5/8] drm: drm_atomic_helper: Add a new helper to deal with the writeback connector validation
Hi Leandro,
On 10/28/21 6:38 PM, Leandro Ribeiro wrote:
Hi,
On 10/26/21 08:34, Igor Torrente wrote:
Add a helper function to validate the connector configuration receive in
the encoder atomic_check by the drivers.
So the drivers don't need do these common validations themselves.
Signed-off-by: Igor Torrente
---
V2: Move the format verification to a new helper at the drm_atomic_helper.c
(Thomas Zimmermann).
---
drivers/gpu/drm/drm_atomic_helper.c | 47 +++
drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++--
include/drm/drm_atomic_helper.h | 3 ++
3 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 2c0c6ec92820..c2653b9824b5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_helper_check_modeset);
+/**
+ * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder state
+ * @encoder: encoder state to check
+ * @conn_state: connector state to check
+ *
+ * Checks if the wriback connector state is valid, and returns a erros if it
+ * isn't.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int
+drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder,
+struct drm_connector_state *conn_state)
+{
+ struct drm_writeback_job *wb_job = conn_state->writeback_job;
+ struct drm_property_blob *pixel_format_blob;
+ bool format_supported = false;
+ struct drm_framebuffer *fb;
+ int i, n_formats;
+ u32 *formats;
+
+ if (!wb_job || !wb_job->fb)
+ return 0;
I think that this should be removed and that this functions should
assume that (wb_job && wb_job->fb) == true.
Ok.
Actually, it's weird to have conn_state as argument and only use it to
get the wb_job. Instead, this function could receive wb_job directly.
In the Thomas review of v1, he said that maybe other things could be
tested in this helper. I'm not sure what these additional checks could
be, so I tried to design the function signature expecting more things
to be added after his review.
As you can see, the helper is receiving the `drm_encoder` and doing
nothing with it.
If we, eventually, don't find anything else that this helper can do, I
will revert to something very similar (if not equal) to your proposal.
I just want to wait for Thomas's review first.
Of course, its name/description would have to change.
+
+ pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr;
+ n_formats = pixel_format_blob->length / sizeof(u32);
+ formats = pixel_format_blob->data;
+ fb = wb_job->fb;
+
+ for (i = 0; i < n_formats; i++) {
+ if (fb->format->format == formats[i]) {
+ format_supported = true;
+ break;
+ }
+ }
+
+ if (!format_supported) {
+ DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
+ &fb->format->format);
+ return -EINVAL;
+ }
+
+ return 0;
If you do this, you can get rid of the format_supported flag:
for(...) {
if (fb->format->format == formats[i])
return 0;
}
DRM_DEBUG_KMS(...);
return -EINVAL;
Indeed. Thanks!
Thanks,
Leandro Ribeiro
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state);
+
/**
* drm_atomic_helper_check_plane_state() - Check plane state for validity
* @plane_state: plane state to check
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c
b/drivers/gpu/drm/vkms/vkms_writeback.c
index 32734cdbf6c2..42f3396c523a 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder
*encoder,
{
struct drm_framebuffer *fb;
const struct drm_display_mode *mode = &crtc_state->mode;
+ int ret;
if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
return 0;
@@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder
*encoder,
return -EINVAL;
}
- if (fb->format->format != vkms_wb_formats[0]) {
- DRM_DEBUG_KMS("Invalid pixel format %p4cc\n",
- &fb->format->format);
- return -EINVAL;
- }
+ ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state);
+ if (ret < 0)
+ return ret;
return 0;
}
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4045e2507e11..3fbf695da60f 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -40,6 +40,9 @@ struct drm_private_state;
int drm_atomic_helper_check_m
Re: [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
Hello Thomas, On 11/3/21 14:01, Thomas Zimmermann wrote: [snip] >> >> >> Javier Martinez Canillas (5): >>drm/i915: Fix comment about modeset parameters >>drm: Move nomodeset kernel parameter handler to the DRM subsystem >>drm: Rename vgacon_text_force() function to drm_modeset_disabled() >>drm: Add a drm_drv_enabled() helper function >>drm: Use drm_drv_enabled() instead of drm_modeset_disabled() > > There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And > I'd put patch (4+5) first, so you have the drivers out of the way. After > that patch (2+3) should only modify drm_drv_enabled(). > Sure, I'm happy with less patches. Thanks for your feedback. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/amd/display: Fix error handling on waiting for completion
[ Adding dri-devel ] On 2021-11-01 16:00, Wang, Chao-kai (Stylon) wrote: > > The problem with -ERESTARTSYS is the same half-baked atomic state with > modifications we made in the interrupted atomic check, is reused in the next > retry and fails the atomic check. What we expect in the next retry is with > the original atomic state. I am going to dig deeper and see if at DRM side we > can go back to use to the original atomic state in the retry. I suspect either DC/DM needs to be able to handle the modified state on retry, or it needs to restore the original state before it returns -ERESTARTSYS. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [Linaro-mm-sig] [PATCH] dma-buf/poll: Get a file reference for outstanding fence callbacks
On 2021-07-23 10:22, Christian König wrote:
> Am 23.07.21 um 10:19 schrieb Michel Dänzer:
>> On 2021-07-23 10:04 a.m., Christian König wrote:
>>> Am 23.07.21 um 09:58 schrieb Michel Dänzer:
From: Michel Dänzer
This makes sure we don't hit the
BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
in dma_buf_release, which could be triggered by user space closing the
dma-buf file description while there are outstanding fence callbacks
from dma_buf_poll.
>>> I was also wondering the same thing while working on this, but then thought
>>> that the poll interface would take care of this.
>> I was able to hit the BUG_ON with
>> https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1880 .
>>
>>
Cc: sta...@vger.kernel.org
Signed-off-by: Michel Dänzer
---
drivers/dma-buf/dma-buf.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 6c520c9bd93c..ec25498a971f 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -65,12 +65,9 @@ static void dma_buf_release(struct dentry *dentry)
BUG_ON(dmabuf->vmapping_counter);
/*
- * Any fences that a dma-buf poll can wait on should be signaled
- * before releasing dma-buf. This is the responsibility of each
- * driver that uses the reservation objects.
- *
- * If you hit this BUG() it means someone dropped their ref to the
- * dma-buf while still having pending operation to the buffer.
+ * If you hit this BUG() it could mean:
+ * * There's a file reference imbalance in dma_buf_poll /
dma_buf_poll_cb or somewhere else
+ * * dmabuf->cb_in/out.active are non-0 despite no pending fence
callback
*/
BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active);
@@ -196,6 +193,7 @@ static loff_t dma_buf_llseek(struct file *file,
loff_t offset, int whence)
static void dma_buf_poll_cb(struct dma_fence *fence, struct
dma_fence_cb *cb)
{
struct dma_buf_poll_cb_t *dcb = (struct dma_buf_poll_cb_t *)cb;
+ struct dma_buf *dmabuf = container_of(dcb->poll, struct dma_buf,
poll);
unsigned long flags;
spin_lock_irqsave(&dcb->poll->lock, flags);
@@ -203,6 +201,8 @@ static void dma_buf_poll_cb(struct dma_fence *fence,
struct dma_fence_cb *cb)
dcb->active = 0;
spin_unlock_irqrestore(&dcb->poll->lock, flags);
dma_fence_put(fence);
+ /* Paired with get_file in dma_buf_poll */
+ fput(dmabuf->file);
>>> Is calling fput() in interrupt context ok? IIRC that could potentially
>>> sleep.
>> Looks fine AFAICT: It has
>>
>> if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
>>
>> and as a fallback for that, it adds the file to a lock-less
>> delayed_fput_list which is processed by a workqueue.
>
> Ah, yes that makes sense.
>
> Fell free to add Reviewed-by: Christian König
Thanks! AFAICT this fix can be merged now for 5.16?
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
[Bug 214859] drm-amdgpu-init-iommu~fd-device-init.patch introduce bug
https://bugzilla.kernel.org/show_bug.cgi?id=214859 --- Comment #6 from James Zhu (jam...@amd.com) --- Created attachment 299437 --> https://bugzilla.kernel.org/attachment.cgi?id=299437&action=edit analysis for this issue Linux 5.14.15 + afd1818 can fix the issue. Linux 5.15rc7 re-apply "init iommu after amdkfd device init" and "move iommu_resume before ip init/resume" which overwrote afd1818 caused the issue again. 714d9e4 drm/amdgpu: init iommu after amdkfd device init f02abeb drm/amdgpu: move iommu_resume before ip init/resume afd1818 drm/amdkfd: fix boot failure when iommu is disabled in Picasso. 286826d drm/amdgpu: init iommu after amdkfd device init 9cec53c drm/amdgpu: move iommu_resume before ip init/resume -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
On 11/3/21 13:57, Thomas Zimmermann wrote:
[snip]
>>
>> -if (vgacon_text_force()) {
>> +if (drm_modeset_disabled()) {
>> DRM_ERROR("amdgpu kernel modesetting disabled.\n");
>
> Please remove all such error messages from drivers.
> drm_modeset_disabled() should print a unified message instead.
>
Agreed.
>> -static bool vgacon_text_mode_force;
>> +static bool drm_nomodeset;
>>
>> -bool vgacon_text_force(void)
>> +bool drm_modeset_disabled(void)
>
> I suggest to rename this function to drm_check_modeset() and have it
> return a negative errno code on failure. This gives maximum flexibility
> and reduces errors in drivers. Right now the drivers return something
> like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.
>
Good idea. I'll do it in v2 as well.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v11 4/4] drm/bridge: anx7625: add HDMI audio function
This is a super awkward way to resend a patch series. Next time, just start a new thread and put [PATCH RESEND] in the subject. I am sorry that no one responded to your thread. :/ regards, dan carpenter
Re: [RESEND PATCH 4/5] drm: Add a drm_drv_enabled() helper function
Hi
Am 03.11.21 um 14:41 schrieb Jani Nikula:
On Wed, 03 Nov 2021, Javier Martinez Canillas wrote:
DRM drivers can use this to determine whether they can be enabled or not.
For now it's just a wrapper around drm_modeset_disabled() but having the
indirection level will allow to add other conditions besides "nomodeset".
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
Can't see i915 trivially using this due to the drm_driver
parameter. Please let's not merge helpers like this without actual
users.
Can you explain?
This would be called on the top of the PCI probe function. The parameter
would allow to decide on a per-driver base (e.g., to ignore generic
drivers).
Best regards
Thomas
BR,
Jani.
---
drivers/gpu/drm/drm_drv.c | 21 +
include/drm/drm_drv.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..70ef08941e06 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -975,6 +975,27 @@ int drm_dev_set_unique(struct drm_device *dev, const char
*name)
}
EXPORT_SYMBOL(drm_dev_set_unique);
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Returns:
+ * True if the DRM driver is enabled and false otherwise.
+ */
+bool drm_drv_enabled(const struct drm_driver *driver)
+{
+ if (drm_modeset_disabled()) {
+ DRM_INFO("%s driver is disabled\n", driver->name);
+ return false;
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
+
/*
* DRM Core
* The DRM core module initializes all global DRM objects and makes them
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 0cd95953cdf5..48f2b6eec012 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -598,5 +598,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct
drm_device *dev)
int drm_dev_set_unique(struct drm_device *dev, const char *name);
+bool drm_drv_enabled(const struct drm_driver *driver);
#endif
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [RESEND PATCH 4/5] drm: Add a drm_drv_enabled() helper function
On Wed, 03 Nov 2021, Javier Martinez Canillas wrote:
> DRM drivers can use this to determine whether they can be enabled or not.
>
> For now it's just a wrapper around drm_modeset_disabled() but having the
> indirection level will allow to add other conditions besides "nomodeset".
>
> Suggested-by: Thomas Zimmermann
> Signed-off-by: Javier Martinez Canillas
Can't see i915 trivially using this due to the drm_driver
parameter. Please let's not merge helpers like this without actual
users.
BR,
Jani.
> ---
>
> drivers/gpu/drm/drm_drv.c | 21 +
> include/drm/drm_drv.h | 1 +
> 2 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..70ef08941e06 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -975,6 +975,27 @@ int drm_dev_set_unique(struct drm_device *dev, const
> char *name)
> }
> EXPORT_SYMBOL(drm_dev_set_unique);
>
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Returns:
> + * True if the DRM driver is enabled and false otherwise.
> + */
> +bool drm_drv_enabled(const struct drm_driver *driver)
> +{
> + if (drm_modeset_disabled()) {
> + DRM_INFO("%s driver is disabled\n", driver->name);
> + return false;
> + }
> +
> + return true;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
> +
> /*
> * DRM Core
> * The DRM core module initializes all global DRM objects and makes them
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 0cd95953cdf5..48f2b6eec012 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -598,5 +598,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct
> drm_device *dev)
>
> int drm_dev_set_unique(struct drm_device *dev, const char *name);
>
> +bool drm_drv_enabled(const struct drm_driver *driver);
>
> #endif
--
Jani Nikula, Intel Open Source Graphics Center
Re: [RESEND PATCH 1/5] drm/i915: Fix comment about modeset parameters
On Wed, 03 Nov 2021, Javier Martinez Canillas wrote: > The comment mentions that the KMS is enabled by default unless either the > i915.modeset module parameter or vga_text_mode_force boot option are used. > > But the latter does not exist and instead the nomodeset option was meant. > > Signed-off-by: Javier Martinez Canillas Thanks for the patch. I've picked this up to drm-intel-next as a non-functional change independent from the rest of the series. BR, Jani. > --- > > drivers/gpu/drm/i915/i915_module.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_module.c > b/drivers/gpu/drm/i915/i915_module.c > index ab2295dd4500..c7507266aa83 100644 > --- a/drivers/gpu/drm/i915/i915_module.c > +++ b/drivers/gpu/drm/i915/i915_module.c > @@ -24,8 +24,8 @@ static int i915_check_nomodeset(void) > > /* >* Enable KMS by default, unless explicitly overriden by > - * either the i915.modeset prarameter or by the > - * vga_text_mode_force boot option. > + * either the i915.modeset parameter or by the > + * nomodeset boot option. >*/ > > if (i915_modparams.modeset == 0) -- Jani Nikula, Intel Open Source Graphics Center
Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
Hello Jani,
On 11/3/21 13:56, Jani Nikula wrote:
[snip]
>>
>> +obj-y += drm_nomodeset.o
>
> This is a subtle functional change. With this, you'll always have
> __setup("nomodeset", text_mode) builtin and the parameter available. And
> using nomodeset will print out the pr_warn() splat from text_mode(). But
> removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
> leads to vgacon_text_force() always returning false.
>
Yes, that's what I decided at the end to make it unconditional. That
way the same behaviour is preserved (even when only DRM drivers are
using the exported symbol).
> To not make functional changes, this should be:
>
> obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>
Right, that should work.
> Now, going with the cleanup in this series, maybe we should make the
> functional change, and break the connection to CONFIG_VGA_CONSOLE
> altogether, also in the header?
>
> (Maybe we'll also need a proxy drm kconfig option to only have
> drm_modeset.o builtin when CONFIG_DRM != n.)
>
See my other email. I believe the issue is drivers/gpu/drm always
being included even when CONFIG_DRM is not set.
Best regards, --
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
Hello Thomas, On 11/3/21 13:41, Thomas Zimmermann wrote: > Hi > > Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas: >> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver >> but the exported vgacon_text_force() symbol is only used by DRM drivers. >> >> It makes much more sense for the parameter logic to be in the subsystem >> of the drivers that are making use of it. Let's move that to DRM. >> >> Suggested-by: Daniel Vetter >> Signed-off-by: Javier Martinez Canillas >> --- >> >> drivers/gpu/drm/Makefile| 2 ++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +-- >> drivers/gpu/drm/ast/ast_drv.c | 1 - >> drivers/gpu/drm/drm_nomodeset.c | 26 + >> drivers/gpu/drm/i915/i915_module.c | 2 -- >> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 - >> drivers/gpu/drm/nouveau/nouveau_drm.c | 1 - >> drivers/gpu/drm/qxl/qxl_drv.c | 1 - >> drivers/gpu/drm/radeon/radeon_drv.c | 1 - >> drivers/gpu/drm/tiny/bochs.c| 1 - >> drivers/gpu/drm/tiny/cirrus.c | 1 - >> drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 - >> drivers/gpu/drm/virtio/virtgpu_drv.c| 1 - >> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 - >> drivers/video/console/vgacon.c | 21 >> include/drm/drm_mode_config.h | 6 ++ >> include/linux/console.h | 6 -- >> 17 files changed, 35 insertions(+), 41 deletions(-) >> create mode 100644 drivers/gpu/drm/drm_nomodeset.c >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 1c41156deb5f..0e2d60ea93ca 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o >> drm_privacy_screen_x86. >> >> obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o >> >> +obj-y += drm_nomodeset.o > > Repeating my other comment, should this rather be protected by a > separate config symbol that is selected by CONFIG_DRM? > I actually thought about that and my opinion is that obj-y reflects what we really want here or do you envision this getting disabled in some cases ? Probably the problem is Kbuild descending into the drivers/gpu dir even when CONFIG_DRM is not set. Maybe what we want is something like the following instead? diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile index 835c88318cec..ef12ee05ba6e 100644 --- a/drivers/gpu/Makefile +++ b/drivers/gpu/Makefile @@ -3,6 +3,7 @@ # taken to initialize them in the correct order. Link order is the only way # to ensure this currently. obj-$(CONFIG_TEGRA_HOST1X) += host1x/ -obj-y += drm/ vga/ +obj-$(CONFIG_DRM) += drm/ +obj-y += vga/ obj-$(CONFIG_IMX_IPUV3_CORE) += ipu-v3/ obj-$(CONFIG_TRACE_GPU_MEM)+= trace/ Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
Hi Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas: [ resend with all relevant people as Cc now, sorry to others for the spam ] There is a lot of historical baggage on this parameter. It's defined in the vgacon driver as a "nomodeset" parameter, but it's handler function is called text_mode() that sets a variable named vgacon_text_mode_force whose value is queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver could be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. There is also another aspect that could be improved, and is the fact that drivers are checking for the nomodeset being set as an indication if have to be loaded. But there may be other reasons why this could be the case, so it is better to encapsulate the logic in a separate function to make clear what's about. Patch #1 is just a trivial fix for a comment that isn't referring to the correct kernel parameter. Patch #2 moves the nomodeset logic to the DRM subsystem. Patch #3 renames the vgacon_text_force() function and accompaning logic as drm_modeset_disabled(), which is what this function is really about. Patch #4 adds a drm_drv_enabled() function that could be used by drivers to check if could be enabled. Patch #5 uses the drm_drv_enabled() function to check this instead of just checking if nomodeset has been set. Javier Martinez Canillas (5): drm/i915: Fix comment about modeset parameters drm: Move nomodeset kernel parameter handler to the DRM subsystem drm: Rename vgacon_text_force() function to drm_modeset_disabled() drm: Add a drm_drv_enabled() helper function drm: Use drm_drv_enabled() instead of drm_modeset_disabled() There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And I'd put patch (4+5) first, so you have the drivers out of the way. After that patch (2+3) should only modify drm_drv_enabled(). Best regards Thomas drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 ++--- drivers/gpu/drm/ast/ast_drv.c | 3 +-- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 10 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- drivers/gpu/drm/qxl/qxl_drv.c | 3 +-- drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- drivers/gpu/drm/tiny/bochs.c| 3 +-- drivers/gpu/drm/tiny/cirrus.c | 3 +-- drivers/gpu/drm/vboxvideo/vbox_drv.c| 5 + drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-- drivers/video/console/vgacon.c | 21 include/drm/drm_drv.h | 1 + include/drm/drm_mode_config.h | 6 ++ include/linux/console.h | 6 -- 19 files changed, 73 insertions(+), 57 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
Hi
Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:
This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.
But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.
Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.
Suggested-by: Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/drm_nomodeset.c | 16
drivers/gpu/drm/i915/i915_module.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
drivers/gpu/drm/tiny/bochs.c| 2 +-
drivers/gpu/drm/tiny/cirrus.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c| 4 +---
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
include/drm/drm_mode_config.h | 4 ++--
14 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
+ if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");
Please remove all such error messages from drivers.
drm_modeset_disabled() should print a unified message instead.
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
+ if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
if (ast_modeset == 0)
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
#include
#include
-static bool vgacon_text_mode_force;
+static bool drm_nomodeset;
-bool vgacon_text_force(void)
+bool drm_modeset_disabled(void)
I suggest to rename this function to drm_check_modeset() and have it
return a negative errno code on failure. This gives maximum flexibility
and reduces errors in drivers. Right now the drivers return something
like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.
{
- return vgacon_text_mode_force;
+ return drm_nomodeset;
}
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
-static int __init text_mode(char *str)
+static int __init disable_modeset(char *str)
{
- vgacon_text_mode_force = true;
+ drm_nomodeset = true;
pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");
pr_warn("Any video related functionality will be severely degraded, and you
may not even be able to suspend the system properly\n");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
}
-/* force text mode - used by kernel modesetting */
-__setup("nomodeset", text_mode);
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
- if (vgacon_text_force() && i915_modparams.modeset == -1)
+ if (drm_modeset_disabled() && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
static int __init mgag200_init(void)
{
- if (vgacon_text_force() && mgag200_modeset == -1)
+ if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
if (mgag200_modeset == 0)
diff --git a/drivers/gpu/drm/nouvea
Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
On Wed, 03 Nov 2021, Javier Martinez Canillas wrote:
> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
> but the exported vgacon_text_force() symbol is only used by DRM drivers.
>
> It makes much more sense for the parameter logic to be in the subsystem
> of the drivers that are making use of it. Let's move that to DRM.
>
> Suggested-by: Daniel Vetter
> Signed-off-by: Javier Martinez Canillas
> ---
>
> drivers/gpu/drm/Makefile| 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
> drivers/gpu/drm/ast/ast_drv.c | 1 -
> drivers/gpu/drm/drm_nomodeset.c | 26 +
> drivers/gpu/drm/i915/i915_module.c | 2 --
> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
> drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
> drivers/gpu/drm/qxl/qxl_drv.c | 1 -
> drivers/gpu/drm/radeon/radeon_drv.c | 1 -
> drivers/gpu/drm/tiny/bochs.c| 1 -
> drivers/gpu/drm/tiny/cirrus.c | 1 -
> drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
> drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
> drivers/video/console/vgacon.c | 21
> include/drm/drm_mode_config.h | 6 ++
> include/linux/console.h | 6 --
> 17 files changed, 35 insertions(+), 41 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c41156deb5f..0e2d60ea93ca 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
> drm_privacy_screen_x86.
>
> obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>
> +obj-y += drm_nomodeset.o
This is a subtle functional change. With this, you'll always have
__setup("nomodeset", text_mode) builtin and the parameter available. And
using nomodeset will print out the pr_warn() splat from text_mode(). But
removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
leads to vgacon_text_force() always returning false.
To not make functional changes, this should be:
obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
Now, going with the cleanup in this series, maybe we should make the
functional change, and break the connection to CONFIG_VGA_CONSOLE
altogether, also in the header?
(Maybe we'll also need a proxy drm kconfig option to only have
drm_modeset.o builtin when CONFIG_DRM != n.)
> +
> drm_cma_helper-y := drm_gem_cma_helper.o
> obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index c718fb5f3f8a..2680a2aaa877 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -31,7 +31,6 @@
> #include "amdgpu_drv.h"
>
> #include
> -#include
> #include
> #include
> #include
> @@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
> int r;
>
> if (vgacon_text_force()) {
> - DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> + DRM_ERROR("amdgpu kernel modesetting disabled.\n");
> return -EINVAL;
> }
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 86d5cd7b6318..048be607b182 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -26,7 +26,6 @@
> * Authors: Dave Airlie
> */
>
> -#include
> #include
> #include
>
> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
> new file mode 100644
> index ..1ac9a8d5a8fe
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_nomodeset.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +
> +static bool vgacon_text_mode_force;
> +
> +bool vgacon_text_force(void)
> +{
> + return vgacon_text_mode_force;
> +}
> +EXPORT_SYMBOL(vgacon_text_force);
> +
> +static int __init text_mode(char *str)
> +{
> + vgacon_text_mode_force = true;
> +
> + pr_warn("You have booted with nomodeset. This means your GPU drivers
> are DISABLED\n");
> + pr_warn("Any video related functionality will be severely degraded, and
> you may not even be able to suspend the system properly\n");
> + pr_warn("Unless you actually understand what nomodeset does, you should
> reboot without enabling it\n");
> +
> + return 1;
> +}
> +
> +/* force text mode - used by kernel modesetting */
> +__setup("nomodeset", text_mode);
> diff --git a/drivers/gpu/drm/i915/i915_module.c
> b/drivers/gpu/drm/i915/i915_module.c
> index c7507266aa83..14a59226519d 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -4,8 +4,6 @@
> * Copyright © 2021 Intel Corporation
> */
>
> -#include
> -
> #include "gem/i915_gem_context.h"
> #include "gem/i915_gem_object.h"
> #include
Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
Hi
Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it. Let's move that to DRM.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
17 files changed, 35 insertions(+), 41 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..0e2d60ea93ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-y += drm_nomodeset.o
Repeating my other comment, should this rather be protected by a
separate config symbol that is selected by CONFIG_DRM?
Best regards
Thomas
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..2680a2aaa877 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
@@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
int r;
if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+ DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..048be607b182 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..1ac9a8d5a8fe
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool vgacon_text_mode_force;
+
+bool vgacon_text_force(void)
+{
+ return vgacon_text_mode_force;
+}
+EXPORT_SYMBOL(vgacon_text_force);
+
+static int __init text_mode(char *str)
+{
+ vgacon_text_mode_force = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers are
DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and you
may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+
+ return 1;
+}
+
+/* force text mode - used by kernel modesetting */
+__setup("nomodeset", text_mode);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c7507266aa83..14a59226519d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
* Copyright © 2021 Intel Corporation
*/
-#include
-
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_object.h"
#include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..685e766db6a4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -6,7 +6,6 @@
* Dave Airlie
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..029997f50d1a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,7 +22,6 @@
* Authors: Ben Skeggs
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index
[RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it. Let's move that to DRM.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
17 files changed, 35 insertions(+), 41 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..0e2d60ea93ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-y += drm_nomodeset.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..2680a2aaa877 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
@@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
int r;
if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+ DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..048be607b182 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..1ac9a8d5a8fe
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool vgacon_text_mode_force;
+
+bool vgacon_text_force(void)
+{
+ return vgacon_text_mode_force;
+}
+EXPORT_SYMBOL(vgacon_text_force);
+
+static int __init text_mode(char *str)
+{
+ vgacon_text_mode_force = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+
+ return 1;
+}
+
+/* force text mode - used by kernel modesetting */
+__setup("nomodeset", text_mode);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c7507266aa83..14a59226519d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
* Copyright © 2021 Intel Corporation
*/
-#include
-
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_object.h"
#include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..685e766db6a4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -6,7 +6,6 @@
* Dave Airlie
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..029997f50d1a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,7 +22,6 @@
* Authors: Ben Skeggs
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index fc47b0deb021..3cd6bd9f059d 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -29,7 +29,6 @@
#include "qxl_drv.h"
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
b/drivers/gpu/drm/radeon
[RESEND PATCH 4/5] drm: Add a drm_drv_enabled() helper function
DRM drivers can use this to determine whether they can be enabled or not.
For now it's just a wrapper around drm_modeset_disabled() but having the
indirection level will allow to add other conditions besides "nomodeset".
Suggested-by: Thomas Zimmermann
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/drm_drv.c | 21 +
include/drm/drm_drv.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..70ef08941e06 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -975,6 +975,27 @@ int drm_dev_set_unique(struct drm_device *dev, const char
*name)
}
EXPORT_SYMBOL(drm_dev_set_unique);
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Returns:
+ * True if the DRM driver is enabled and false otherwise.
+ */
+bool drm_drv_enabled(const struct drm_driver *driver)
+{
+ if (drm_modeset_disabled()) {
+ DRM_INFO("%s driver is disabled\n", driver->name);
+ return false;
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
+
/*
* DRM Core
* The DRM core module initializes all global DRM objects and makes them
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 0cd95953cdf5..48f2b6eec012 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -598,5 +598,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct
drm_device *dev)
int drm_dev_set_unique(struct drm_device *dev, const char *name);
+bool drm_drv_enabled(const struct drm_driver *driver);
#endif
--
2.33.1
Re: [PATCH v3] backlight: lp855x: Switch to atomic PWM API
Em ter., 2 de nov. de 2021 às 05:39, Geert Uytterhoeven
escreveu:
>
> Hi Maíra,
>
> On Sat, Oct 30, 2021 at 3:35 PM Maíra Canal wrote:
> > Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> > replace it for the atomic PWM API.
> >
> > Signed-off-by: Maíra Canal
>
> Thanks for your patch!
>
> > --- a/drivers/video/backlight/lp855x_bl.c
> > +++ b/drivers/video/backlight/lp855x_bl.c
> > @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
> >
> > static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> > {
> > - unsigned int period = lp->pdata->period_ns;
> > - unsigned int duty = br * period / max_br;
> > - struct pwm_device *pwm;
> > + struct pwm_device *pwm = NULL;
> > + struct pwm_state state;
> >
> > /* request pwm device with the consumer name */
> > if (!lp->pwm) {
> > @@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int
> > br, int max_br)
> >
> > lp->pwm = pwm;
> >
> > - /*
> > -* FIXME: pwm_apply_args() should be removed when switching
> > to
> > -* the atomic PWM API.
> > -*/
> > - pwm_apply_args(pwm);
> > + pwm_init_state(lp->pwm, &state);
> > + state.period = lp->pdata->period_ns;
> > }
> >
> > - pwm_config(lp->pwm, duty, period);
> > - if (duty)
> > - pwm_enable(lp->pwm);
> > - else
> > - pwm_disable(lp->pwm);
> > + pwm_get_state(lp->pwm, &state);
> > +
> > + state.duty_cycle = br * state.period / max_br;
>
> Above is a 64-by-32 division, which should not be open-coded, as
> that will cause link failures on 32-bit platform (cfr. "undefined
> reference to `__udivdi3'", as reported by the kernel test robot).
> Please use div_u64() instead.
Hi Geert,
Thank you for the suggestion! I fixed this problem a bit differently
and submitted the v4. I made use of the PWM API and applied the
pwm_set_relative_duty_cycle function, which solved this division
problem.
>
> > + state.enabled = state.duty_cycle;
> > +
> > + pwm_apply_state(lp->pwm, &state);
> > }
> >
> > static int lp855x_bl_update_status(struct backlight_device *bl)
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
[RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.
But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.
Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.
Suggested-by: Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/drm_nomodeset.c | 16
drivers/gpu/drm/i915/i915_module.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
drivers/gpu/drm/tiny/bochs.c| 2 +-
drivers/gpu/drm/tiny/cirrus.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c| 4 +---
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
include/drm/drm_mode_config.h | 4 ++--
14 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
+ if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
+ if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
if (ast_modeset == 0)
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
#include
#include
-static bool vgacon_text_mode_force;
+static bool drm_nomodeset;
-bool vgacon_text_force(void)
+bool drm_modeset_disabled(void)
{
- return vgacon_text_mode_force;
+ return drm_nomodeset;
}
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
-static int __init text_mode(char *str)
+static int __init disable_modeset(char *str)
{
- vgacon_text_mode_force = true;
+ drm_nomodeset = true;
pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
}
-/* force text mode - used by kernel modesetting */
-__setup("nomodeset", text_mode);
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
- if (vgacon_text_force() && i915_modparams.modeset == -1)
+ if (drm_modeset_disabled() && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
static int __init mgag200_init(void)
{
- if (vgacon_text_force() && mgag200_modeset == -1)
+ if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
if (mgag200_modeset == 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 029997f50d1a..903d0e626954 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1321,7 +1321,7 @@ nouveau_drm_init(void)
nouveau_display_options();
if (nouveau_modeset == -1) {
- if (vgacon_text_force())
+ if (drm_modeset_disabled())
nouveau_modeset = 0;
}
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/dri
[RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
[ resend with all relevant people as Cc now, sorry to others for the spam ] There is a lot of historical baggage on this parameter. It's defined in the vgacon driver as a "nomodeset" parameter, but it's handler function is called text_mode() that sets a variable named vgacon_text_mode_force whose value is queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver could be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. There is also another aspect that could be improved, and is the fact that drivers are checking for the nomodeset being set as an indication if have to be loaded. But there may be other reasons why this could be the case, so it is better to encapsulate the logic in a separate function to make clear what's about. Patch #1 is just a trivial fix for a comment that isn't referring to the correct kernel parameter. Patch #2 moves the nomodeset logic to the DRM subsystem. Patch #3 renames the vgacon_text_force() function and accompaning logic as drm_modeset_disabled(), which is what this function is really about. Patch #4 adds a drm_drv_enabled() function that could be used by drivers to check if could be enabled. Patch #5 uses the drm_drv_enabled() function to check this instead of just checking if nomodeset has been set. Javier Martinez Canillas (5): drm/i915: Fix comment about modeset parameters drm: Move nomodeset kernel parameter handler to the DRM subsystem drm: Rename vgacon_text_force() function to drm_modeset_disabled() drm: Add a drm_drv_enabled() helper function drm: Use drm_drv_enabled() instead of drm_modeset_disabled() drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 ++--- drivers/gpu/drm/ast/ast_drv.c | 3 +-- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 10 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- drivers/gpu/drm/qxl/qxl_drv.c | 3 +-- drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- drivers/gpu/drm/tiny/bochs.c| 3 +-- drivers/gpu/drm/tiny/cirrus.c | 3 +-- drivers/gpu/drm/vboxvideo/vbox_drv.c| 5 + drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-- drivers/video/console/vgacon.c | 21 include/drm/drm_drv.h | 1 + include/drm/drm_mode_config.h | 6 ++ include/linux/console.h | 6 -- 19 files changed, 73 insertions(+), 57 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
[RESEND PATCH 1/5] drm/i915: Fix comment about modeset parameters
The comment mentions that the KMS is enabled by default unless either the i915.modeset module parameter or vga_text_mode_force boot option are used. But the latter does not exist and instead the nomodeset option was meant. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/i915/i915_module.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c index ab2295dd4500..c7507266aa83 100644 --- a/drivers/gpu/drm/i915/i915_module.c +++ b/drivers/gpu/drm/i915/i915_module.c @@ -24,8 +24,8 @@ static int i915_check_nomodeset(void) /* * Enable KMS by default, unless explicitly overriden by -* either the i915.modeset prarameter or by the -* vga_text_mode_force boot option. +* either the i915.modeset parameter or by the +* nomodeset boot option. */ if (i915_modparams.modeset == 0) -- 2.33.1
Re: dri/drm/kms question with regards to minor faults
On 01/11/2021 05:20, Bert Schiettecatte wrote: > Hi John > >> Coincidentally, I've been looking at Panfrost on RK3288 this week as >> well! I'm testing it with a project that has been using the binary blob >> driver for several years and unfortunately Panfrost seems to use ~15% >> more CPU. >> Like you, I see a huge number of minor faults (~500/second compared with >> ~3/second on libmali). It seems that Panfrost is mmap'ing and >> munmap'ing buffers on every frame which doesn't happen when the same >> application is using the binary driver. > > Thanks for confirming you are seeing the same issue. > >> Panfrost experts, is there a missed opportunity for optimisation here? >> Or is there something applications should be doing differently to avoid >> repeatedly mapping & unmapping the same buffers? > > Panfrost team - any update on this? I was hoping Alyssa would comment since she's much more familiar with Mesa than I am! On the first point of libmali not performing mmap()s very often - I'll just note that this was a specific design goal and for example the kbase kernel driver provides ioctl()s to do CPU cache maintenance for this to work on arm platforms (i.e. it's not a portable solution). So short answer: yes there is room for optimisation here. However things get tricky when fitting into a portable framework. The easiest way of ensuring cache coherency is to ensure there is a clear owner - so the usual approach is mmap(), read/write some data on the CPU, munmap(), GPU accesses data, repeat. The DMA framework in the kernel will then ensure that any cache maintenance/bounce buffering or other quirks are dealt with. Having said that we know that existing platforms don't require these 'quirks' (because libmali works on them) so in theory it should be possible for Mesa to avoid the mmap()/munmap() dance in many cases (where the memory is coherent with the GPU[1]). But this is where my knowledge of Mesa is lacking as I've no idea how to go about that. Regards, Steve [1] I think this should actually be true all the time with Panfrost as the buffer is mapped write-combining on the CPU if the GPU isn't fully coherent. But I haven't double checked this.
RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer dereference on drm_dp_dpcd_access
On Wed, 03 Nov 2021, "Yuan, Perry" wrote: > [AMD Official Use Only] > > Hi Jani: > >> -Original Message- >> From: Jani Nikula >> Sent: Tuesday, November 2, 2021 4:40 PM >> To: Yuan, Perry ; Maarten Lankhorst >> ; Maxime Ripard ; >> Thomas Zimmermann ; David Airlie ; >> Daniel Vetter >> Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; Huang, >> Shimmer ; Huang, Ray >> Subject: RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer dereference on >> drm_dp_dpcd_access >> >> [CAUTION: External Email] >> >> On Tue, 02 Nov 2021, "Yuan, Perry" wrote: >> > [AMD Official Use Only] >> > >> > Hi Jani: >> > Thanks for your comments. >> > >> >> -Original Message- >> >> From: Jani Nikula >> >> Sent: Monday, November 1, 2021 9:07 PM >> >> To: Yuan, Perry ; Maarten Lankhorst >> >> ; Maxime Ripard >> >> ; Thomas Zimmermann ; >> David >> >> Airlie ; Daniel Vetter >> >> Cc: Yuan, Perry ; >> >> dri-devel@lists.freedesktop.org; linux- ker...@vger.kernel.org; >> >> Huang, Shimmer ; Huang, Ray >> >> >> Subject: Re: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer >> >> dereference on drm_dp_dpcd_access >> >> >> >> [CAUTION: External Email] >> >> >> >> On Mon, 01 Nov 2021, Perry Yuan wrote: >> >> > Fix below crash by adding a check in the drm_dp_dpcd_access which >> >> > ensures that aux->transfer was actually initialized earlier. >> >> >> >> Gut feeling says this is papering over a real usage issue somewhere >> >> else. Why is the aux being used for transfers before ->transfer has >> >> been set? Why should the dp helper be defensive against all kinds of >> misprogramming? >> >> >> >> >> >> BR, >> >> Jani. >> >> >> > >> > The issue was found by Intel IGT test suite, graphic by pass test case. >> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl >> > ab.freedesktop.org%2Fdrm%2Figt-gpu- >> tools&data=04%7C01%7CPerry.Yuan >> > %40amd.com%7C83d011acfe65437c0fa808d99ddc65b0%7C3dd8961fe4884e6 >> 08e11a8 >> > >> 2d994e183d%7C0%7C0%7C637714392203200313%7CUnknown%7CTWFpbGZsb >> 3d8eyJWIj >> > >> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100 >> 0&am >> > >> p;sdata=snPpRYLGeJtTpNGle1YHZAvevcABbgLkgOsffiNzQPw%3D&reserved >> =0 >> > normally use case will not see the issue. >> > To avoid this issue happy again when we run the test case , it will be >> > nice to >> add a check before the transfer is called. >> > And we can see that it really needs to have a check here to make ITG >> > &kernel >> happy. >> >> You're missing my point. What is the root cause? Why do you have the aux >> device or connector registered before ->transfer function is initialized. I >> don't >> think you should do that. >> >> BR, >> Jani. >> > > One potential IGT fix patch to resolve the test case failure is: > > tests/amdgpu/amd_bypass.c > data->pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe_id, >- AMDGPU_PIPE_CRC_SOURCE_DPRX); >+ INTEL_PIPE_CRC_SOURCE_AUTO); > The kernel panic error gone after change "dprx" to "auto" in the IGT test. > > In my view ,the IGT amdgpu bypass test will do some common setup work > including crc piple, source. > When the IGT sets up a new CRC pipe capture source for amdgpu bypass test, > the SOURCE was set as "dprx" instead of "auto" > It makes "amdgpu_dm_crtc_set_crc_source()" failed to set correct AUX and > it's transfer function invalid . > The system I tested use HDMI port connected to monitor . > > amdgpu_dm_crtc_set_crc_source->(aux = (aconn->port) ? &aconn->port->aux : > &aconn->dm_dp_aux.aux;) >drm_dp_start_crc -> > drm_dp_dpcd_readb-> aux->transfer is NULL, issue here. > The fix will use the "auto" keyword, which will let the driver select a > default source of frame CRCs for this CRTC. > > Correct me if have some wrong points. Apparently I'm completely failing to communicate my POV to you. If you have a kernel oops, the fix needs to be in the kernel, not IGT. The question is, why is it possible for IGT (or any userspace) to trigger AUX communication when the ->transfer function is not set? In my opinion, the kernel driver should not have exposed the interface at all if the ->transfer function is not set. The interface is useless without the ->transfer function. IMO, that's the bug. BR, Jani. > > Thank you! > Perry. > >> >> > >> > Perry. >> > >> >> >> >> > >> >> > BUG: kernel NULL pointer dereference, address: PGD >> >> > 0 P4D 0 >> >> > Oops: 0010 [#1] SMP NOPTI >> >> > RIP: 0010:0x0 >> >> > Code: Unable to access opcode bytes at RIP 0xffd6. >> >> > RSP: 0018:a8d64225bab8 EFLAGS: 00010246 >> >> > RAX: RBX: 0020 RCX: a8d64225bb5e >> >> > RDX: 93151d921880 RSI: a8d64225bac8 RDI: 931511a1a9d8 >> >> > RBP: a8d64225bb10 R08: 0001 R09: a8d64225ba60 >> >> > R10: 0002 R11: 000d R1
[PATCH 2/2] drm/virtio: introduce lazy pinning
From: mwezdeck
Userspace can opt-in to not pin pages during resource
create ioctl.
In transfer_*_host and map ioctls check if memory is pinned.
If pages are not pinned, pin it. Otherwise, do nothing.
This change is transparent to userspace.
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 +
drivers/gpu/drm/virtio/virtgpu_object.c | 27 -
include/uapi/drm/virtgpu_drm.h | 9 +
3 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index f6a3a760c32d..c01c5c15701c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -103,6 +103,8 @@ static int virtio_gpu_map_ioctl(struct drm_device *dev,
void *data,
struct virtio_gpu_device *vgdev = dev->dev_private;
struct drm_virtgpu_map *virtio_gpu_map = data;
+ virtio_gpu_object_pin(file, vgdev, virtio_gpu_map->handle);
+
return virtio_gpu_mode_dumb_mmap(file, vgdev->ddev,
virtio_gpu_map->handle,
&virtio_gpu_map->offset);
@@ -292,6 +294,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device
*dev, void *data,
case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
value = vgdev->capset_id_mask;
break;
+ case VIRTGPU_PARAM_PIN_ON_DEMAND:
+ value = 1;
+ break;
default:
return -EINVAL;
}
@@ -414,6 +419,8 @@ static int virtio_gpu_transfer_from_host_ioctl(struct
drm_device *dev,
goto err_put_free;
}
+ virtio_gpu_object_pin(file, vgdev, args->bo_handle);
+
if (!bo->host3d_blob && (args->stride || args->layer_stride)) {
ret = -EINVAL;
goto err_put_free;
@@ -465,6 +472,8 @@ static int virtio_gpu_transfer_to_host_ioctl(struct
drm_device *dev, void *data,
goto err_put_free;
}
+ virtio_gpu_object_pin(file, vgdev, args->bo_handle);
+
if (!vgdev->has_virgl_3d) {
virtio_gpu_cmd_transfer_to_host_2d
(vgdev, offset,
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
b/drivers/gpu/drm/virtio/virtgpu_object.c
index 064c50cb9846..183e57ef10e8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -219,7 +219,7 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
struct virtio_gpu_mem_entry *ents;
unsigned int nents;
int ret;
-
+ uint32_t backup_flags = params->flags;
*bo_ptr = NULL;
params->size = roundup(params->size, PAGE_SIZE);
@@ -246,13 +246,19 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
goto err_put_objs;
}
- ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
- if (ret != 0) {
- virtio_gpu_array_put_free(objs);
- virtio_gpu_free_object(&shmem_obj->base);
- return ret;
+ if (!(backup_flags & VIRTGPU_NOT_PIN_FLAG)) {
+ ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
+ if (ret != 0) {
+ virtio_gpu_array_put_free(objs);
+ virtio_gpu_free_object(&shmem_obj->base);
+ return ret;
+ }
}
+ // turn off these bits, as renderer doesn't support such bits
+ if (params->flags & VIRTGPU_NOT_PIN_FLAG)
+ params->flags &= ~(VIRTGPU_NOT_PIN_FLAG);
+
if (params->blob) {
if (params->blob_mem == VIRTGPU_BLOB_MEM_GUEST)
bo->guest_blob = true;
@@ -262,11 +268,13 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
} else if (params->virgl) {
virtio_gpu_cmd_resource_create_3d(vgdev, bo, params,
objs, fence);
- virtio_gpu_object_attach(vgdev, bo, ents, nents);
+ if (!(backup_flags & VIRTGPU_NOT_PIN_FLAG))
+ virtio_gpu_object_attach(vgdev, bo, ents, nents);
} else {
virtio_gpu_cmd_create_resource(vgdev, bo, params,
objs, fence);
- virtio_gpu_object_attach(vgdev, bo, ents, nents);
+ if (!(backup_flags & VIRTGPU_NOT_PIN_FLAG))
+ virtio_gpu_object_attach(vgdev, bo, ents, nents);
}
*bo_ptr = bo;
@@ -305,9 +313,8 @@ int virtio_gpu_object_pin(struct drm_file *file,
if (!shmem->pages) {
ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
- if (ret != 0) {
+ if (ret != 0)
return -EFAULT;
- }
virtio_gpu_object_attach(vgdev, bo, ents, nents);
[PATCH 1/2] drm/virtio: introduce ioctl for pinning pages
From: mwezdeck
Pinning pages happens in virtio_gpu_object_shmem_init()
function.
This ioctl allows to pin pages if it was not done earlier.
Signed-off-by: mwezdeck
---
drivers/gpu/drm/virtio/virtgpu_drv.h| 5 +++-
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 11
drivers/gpu/drm/virtio/virtgpu_object.c | 34 +
include/uapi/drm/virtgpu_drm.h | 9 +++
4 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index e0265fe74aa5..232933919b91 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -278,7 +278,7 @@ struct virtio_gpu_fpriv {
};
/* virtgpu_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 12
+#define DRM_VIRTIO_NUM_IOCTLS 13
extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
@@ -455,6 +455,9 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
struct virtio_gpu_object **bo_ptr,
struct virtio_gpu_fence *fence);
+int virtio_gpu_object_pin(struct drm_file *file,
+ struct virtio_gpu_device *vgdev, uint32_t handle);
+
bool virtio_gpu_is_shmem(struct virtio_gpu_object *bo);
int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 0007e423d885..f6a3a760c32d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -836,6 +836,14 @@ static int virtio_gpu_context_init_ioctl(struct drm_device
*dev,
return ret;
}
+static int virtio_gpu_pin_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct virtio_gpu_device *vgdev = dev->dev_private;
+ struct drm_virtgpu_pin *virtio_gpu_pin = data;
+ return virtio_gpu_object_pin(file, vgdev, virtio_gpu_pin->handle);
+}
+
struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
DRM_RENDER_ALLOW),
@@ -875,4 +883,7 @@ struct drm_ioctl_desc
virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_CONTEXT_INIT, virtio_gpu_context_init_ioctl,
DRM_RENDER_ALLOW),
+
+ DRM_IOCTL_DEF_DRV(VIRTGPU_PIN, virtio_gpu_pin_ioctl,
+ DRM_RENDER_ALLOW),
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
b/drivers/gpu/drm/virtio/virtgpu_object.c
index f648b0e24447..064c50cb9846 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -280,3 +280,37 @@ int virtio_gpu_object_create(struct virtio_gpu_device
*vgdev,
drm_gem_shmem_free_object(&shmem_obj->base);
return ret;
}
+
+int virtio_gpu_object_pin(struct drm_file *file,
+ struct virtio_gpu_device *vgdev, uint32_t handle)
+{
+ int ret;
+ struct drm_gem_object *gem;
+ struct virtio_gpu_object *bo;
+ struct virtio_gpu_object_shmem *shmem;
+ struct virtio_gpu_mem_entry *ents;
+ unsigned int nents;
+
+ gem = drm_gem_object_lookup(file, handle);
+ if (gem == NULL)
+ return -ENOENT;
+
+ bo = gem_to_virtio_gpu_obj(gem);
+ if (bo == NULL)
+ return -ENOENT;
+
+ shmem = to_virtio_gpu_shmem(bo);
+ if (shmem == NULL)
+ return -ENOENT;
+
+ if (!shmem->pages) {
+ ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
+ if (ret != 0) {
+ return -EFAULT;
+ }
+
+ virtio_gpu_object_attach(vgdev, bo, ents, nents);
+ }
+
+ return 0;
+}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index a13e20cc66b4..bb661d53c0e9 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -48,6 +48,7 @@ extern "C" {
#define DRM_VIRTGPU_GET_CAPS 0x09
#define DRM_VIRTGPU_RESOURCE_CREATE_BLOB 0x0a
#define DRM_VIRTGPU_CONTEXT_INIT 0x0b
+#define DRM_VIRTGPU_PIN 0x0c
#define VIRTGPU_EXECBUF_FENCE_FD_IN0x01
#define VIRTGPU_EXECBUF_FENCE_FD_OUT 0x02
@@ -196,6 +197,10 @@ struct drm_virtgpu_context_init {
__u64 ctx_set_params;
};
+struct drm_virtgpu_pin {
+ __u32 handle;
+};
+
#define DRM_IOCTL_VIRTGPU_MAP \
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_MAP, struct drm_virtgpu_map)
@@ -239,6 +244,10 @@ struct drm_virtgpu_context_init {
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_CONTEXT_INIT, \
struct drm_virtgpu_context_init)
+#define DRM_IOCTL_VIRTGPU_PIN \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_PIN,\
+ struct dr
[PATCH] drm/virtio: new ioctl for pinning and lazy pinning
First patch implements new ioctl. If there are no pages pinned to bo object, then pin it. Second patch implements concepts of lazy pin. Userspace must opt-in for not pinning pages. I would like to record this work here and investigate another possibility. Mainly, problem statement for this two patches is to reduce memory usage consumed by Guest. If we can avoid pinning pages for such resources like textures, then we can use staging buffer to upload texture data to host.
[PATCH v4 2/2] drm/i915/ttm: Failsafe migration blits
If the initial fill blit or copy blit of an object fails, the old
content of the data might be exposed and read as soon as either CPU- or
GPU PTEs are set up to point at the pages.
Intercept the blit fence with an async callback that checks the
blit fence for errors and if there are errors performs an async cpu blit
instead. If there is a failure to allocate the async dma_fence_work,
allocate it on the stack and sync wait for the blit to complete.
Add selftests that simulate gpu blit failures and failure to allocate
the async dma_fence_work.
A previous version of this pach used dma_fence_work, now that's
opencoded which adds more code but might lower the latency
somewhat in the common non-error case.
v3:
- Style fixes (Matthew Auld)
v4:
- Use "#if IS_ENABLED()" instead of #ifdef (Matthew Auld)
Signed-off-by: Thomas Hellström
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 322 +++---
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 5 +
.../drm/i915/gem/selftests/i915_gem_migrate.c | 24 +-
3 files changed, 295 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index 0ed6b7f2b95f..1747e9ff97e7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -18,6 +18,29 @@
#include "gt/intel_gt.h"
#include "gt/intel_migrate.h"
+/**
+ * DOC: Selftest failure modes for failsafe migration:
+ *
+ * For fail_gpu_migration, the gpu blit scheduled is always a clear blit
+ * rather than a copy blit, and then we force the failure paths as if
+ * the blit fence returned an error.
+ *
+ * For fail_work_allocation we fail the kmalloc of the async worker, we
+ * sync the gpu blit. If it then fails, or fail_gpu_migration is set to
+ * true, then a memcpy operation is performed sync.
+ */
+#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
+static bool fail_gpu_migration;
+static bool fail_work_allocation;
+
+void i915_ttm_migrate_set_failure_modes(bool gpu_migration,
+ bool work_allocation)
+{
+ fail_gpu_migration = gpu_migration;
+ fail_work_allocation = work_allocation;
+}
+#endif
+
static enum i915_cache_level
i915_ttm_cache_level(struct drm_i915_private *i915, struct ttm_resource *res,
struct ttm_tt *ttm)
@@ -129,11 +152,11 @@ int i915_ttm_move_notify(struct ttm_buffer_object *bo)
return 0;
}
-static int i915_ttm_accel_move(struct ttm_buffer_object *bo,
- bool clear,
- struct ttm_resource *dst_mem,
- struct ttm_tt *dst_ttm,
- struct sg_table *dst_st)
+static struct dma_fence *i915_ttm_accel_move(struct ttm_buffer_object *bo,
+bool clear,
+struct ttm_resource *dst_mem,
+struct ttm_tt *dst_ttm,
+struct sg_table *dst_st)
{
struct drm_i915_private *i915 = container_of(bo->bdev, typeof(*i915),
bdev);
@@ -144,30 +167,29 @@ static int i915_ttm_accel_move(struct ttm_buffer_object
*bo,
int ret;
if (!i915->gt.migrate.context || intel_gt_is_wedged(&i915->gt))
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
+
+ /* With fail_gpu_migration, we always perform a GPU clear. */
+ if (I915_SELFTEST_ONLY(fail_gpu_migration))
+ clear = true;
dst_level = i915_ttm_cache_level(i915, dst_mem, dst_ttm);
if (clear) {
- if (bo->type == ttm_bo_type_kernel)
- return -EINVAL;
+ if (bo->type == ttm_bo_type_kernel &&
+ !I915_SELFTEST_ONLY(fail_gpu_migration))
+ return ERR_PTR(-EINVAL);
intel_engine_pm_get(i915->gt.migrate.context->engine);
ret = intel_context_migrate_clear(i915->gt.migrate.context,
NULL,
dst_st->sgl, dst_level,
i915_ttm_gtt_binds_lmem(dst_mem),
0, &rq);
-
- if (!ret && rq) {
- i915_request_wait(rq, 0, MAX_SCHEDULE_TIMEOUT);
- i915_request_put(rq);
- }
- intel_engine_pm_put(i915->gt.migrate.context->engine);
} else {
struct i915_refct_sgt *src_rsgt =
i915_ttm_resource_get_st(obj, bo->resource);
if (IS_ERR(src_rsgt))
- return PTR_ERR(src_rsgt);
+ return ERR_CAST(src_rsgt);
src_level = i915_ttm_cache_level(i915, bo->resource, src_ttm);
intel_engi
[PATCH v4 1/2] drm/i915/ttm: Reorganize the ttm move code
We are about to introduce failsafe- and asynchronous migration and
ttm moves.
This will add complexity and code to the TTM move code so it makes sense
to split it out to a separate file to make the i915 TTM code easer to
digest.
Split the i915 TTM move code out and since we will have to change the name
of the gpu_binds_iomem() and cpu_maps_iomem() functions anyway,
we alter the name of gpu_binds_iomem() to i915_ttm_gtt_binds_lmem() which
is more reflecting what it is used for.
With this we also add some more documentation. Otherwise there should be
no functional change.
Signed-off-by: Thomas Hellström
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/i915/Makefile| 1 +
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 328 +++
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 35 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 308 +
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 38 +++
5 files changed, 430 insertions(+), 280 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 467872cca027..7d0d0b814670 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -154,6 +154,7 @@ gem-y += \
gem/i915_gem_throttle.o \
gem/i915_gem_tiling.o \
gem/i915_gem_ttm.o \
+ gem/i915_gem_ttm_move.o \
gem/i915_gem_ttm_pm.o \
gem/i915_gem_userptr.o \
gem/i915_gem_wait.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 6a05369e2705..6369fb9b2455 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -14,13 +14,9 @@
#include "gem/i915_gem_object.h"
#include "gem/i915_gem_region.h"
#include "gem/i915_gem_ttm.h"
+#include "gem/i915_gem_ttm_move.h"
#include "gem/i915_gem_ttm_pm.h"
-
-#include "gt/intel_engine_pm.h"
-#include "gt/intel_gt.h"
-#include "gt/intel_migrate.h"
-
#define I915_TTM_PRIO_PURGE 0
#define I915_TTM_PRIO_NO_PAGES 1
#define I915_TTM_PRIO_HAS_PAGES 2
@@ -108,28 +104,6 @@ static int i915_ttm_err_to_gem(int err)
return err;
}
-static bool gpu_binds_iomem(struct ttm_resource *mem)
-{
- return mem->mem_type != TTM_PL_SYSTEM;
-}
-
-static bool cpu_maps_iomem(struct ttm_resource *mem)
-{
- /* Once / if we support GGTT, this is also false for cached ttm_tts */
- return mem->mem_type != TTM_PL_SYSTEM;
-}
-
-static enum i915_cache_level
-i915_ttm_cache_level(struct drm_i915_private *i915, struct ttm_resource *res,
-struct ttm_tt *ttm)
-{
- return ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(res) &&
- ttm->caching == ttm_cached) ? I915_CACHE_LLC :
- I915_CACHE_NONE;
-}
-
-static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
-
static enum ttm_caching
i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
{
@@ -370,23 +344,14 @@ static void i915_ttm_evict_flags(struct ttm_buffer_object
*bo,
*placement = i915_sys_placement;
}
-static int i915_ttm_move_notify(struct ttm_buffer_object *bo)
-{
- struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- int ret;
-
- ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
- if (ret)
- return ret;
-
- ret = __i915_gem_object_put_pages(obj);
- if (ret)
- return ret;
-
- return 0;
-}
-
-static void i915_ttm_free_cached_io_rsgt(struct drm_i915_gem_object *obj)
+/**
+ * i915_ttm_free_cached_io_rsgt - Free object cached LMEM information
+ * @obj: The GEM object
+ * This function frees any LMEM-related information that is cached on
+ * the object. For example the radix tree for fast page lookup and the
+ * cached refcounted sg-table
+ */
+void i915_ttm_free_cached_io_rsgt(struct drm_i915_gem_object *obj)
{
struct radix_tree_iter iter;
void __rcu **slot;
@@ -403,56 +368,16 @@ static void i915_ttm_free_cached_io_rsgt(struct
drm_i915_gem_object *obj)
obj->ttm.cached_io_rsgt = NULL;
}
-static void
-i915_ttm_adjust_domains_after_move(struct drm_i915_gem_object *obj)
-{
- struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
-
- if (cpu_maps_iomem(bo->resource) || bo->ttm->caching != ttm_cached) {
- obj->write_domain = I915_GEM_DOMAIN_WC;
- obj->read_domains = I915_GEM_DOMAIN_WC;
- } else {
- obj->write_domain = I915_GEM_DOMAIN_CPU;
- obj->read_domains = I915_GEM_DOMAIN_CPU;
- }
-}
-
-static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
-{
- struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
- unsigned int cache_level;
- unsigned int i;
-
- /*
-* If object was moved to an allowable region, update the object
-
[PATCH v4 0/2] drm/i915: Failsafe migration blits
This patch series introduces failsafe migration blits. The reason for this seemingly strange concept is that if the initial clearing or readback of LMEM fails for some reason[1], and we then set up either GPU- or CPU ptes to the allocated LMEM, we can expose old contents from other clients. So after each migration blit to LMEM, attach a dma-fence callback that checks the migration fence error value and if it's an error, performs a memcpy blit, instead. Patch 1 splits out the TTM move code into separate files Patch 2 implements the failsafe blits and related self-tests [1] There are at least two ways we could trigger exposure of uninitialized LMEM assuming the migration blits themselves never trigger a gpu hang. a) A gpu operation preceding a pipelined eviction blit resets and sets the error fence to -EIO, and the error is propagated across the TTM manager to the clear / swapin blit of a newly allocated TTM resource. It aborts and leaves the memory uninitialized. b) Something wedges the GT while a migration blit is submitted. It ends up never executed and TTM can fault user-space cpu-ptes into uninitialized memory. v3: - Style fixes in second patch (Matthew Auld) v4: - More style fixes in second patch (Matthew Auld) Thomas Hellström (2): drm/i915/ttm: Reorganize the ttm move code drm/i915/ttm: Failsafe migration blits drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 328 ++- drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 35 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 520 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 43 ++ .../drm/i915/gem/selftests/i915_gem_migrate.c | 24 +- 6 files changed, 670 insertions(+), 281 deletions(-) create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h -- 2.31.1
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct
> drm_connector *connector,
> u32 max_tmds_clock = hf_vsdb[5] * 5000;
> struct drm_scdc *scdc = &hdmi->scdc;
>
> - if (max_tmds_clock > 34) {
> + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> display->max_tmds_clock = max_tmds_clock;
> DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder
> *encoder,
> if (scdc->scrambling.low_rates)
> pipe_config->hdmi_scrambling = true;
>
> - if (pipe_config->port_clock > 34) {
> + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> pipe_config->hdmi_scrambling = true;
> pipe_config->hdmi_high_tmds_clock_ratio = true;
> }
All of that is HDMI 2.0 stuff. So this just makes it all super
confusing IMO. Nak.
--
Ville Syrjälä
Intel
RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer dereference on drm_dp_dpcd_access
[AMD Official Use Only] Hi Jani: > -Original Message- > From: Jani Nikula > Sent: Tuesday, November 2, 2021 4:40 PM > To: Yuan, Perry ; Maarten Lankhorst > ; Maxime Ripard ; > Thomas Zimmermann ; David Airlie ; > Daniel Vetter > Cc: dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; Huang, > Shimmer ; Huang, Ray > Subject: RE: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer dereference on > drm_dp_dpcd_access > > [CAUTION: External Email] > > On Tue, 02 Nov 2021, "Yuan, Perry" wrote: > > [AMD Official Use Only] > > > > Hi Jani: > > Thanks for your comments. > > > >> -Original Message- > >> From: Jani Nikula > >> Sent: Monday, November 1, 2021 9:07 PM > >> To: Yuan, Perry ; Maarten Lankhorst > >> ; Maxime Ripard > >> ; Thomas Zimmermann ; > David > >> Airlie ; Daniel Vetter > >> Cc: Yuan, Perry ; > >> dri-devel@lists.freedesktop.org; linux- ker...@vger.kernel.org; > >> Huang, Shimmer ; Huang, Ray > > >> Subject: Re: [PATCH v2] drm/dp: Fix aux->transfer NULL pointer > >> dereference on drm_dp_dpcd_access > >> > >> [CAUTION: External Email] > >> > >> On Mon, 01 Nov 2021, Perry Yuan wrote: > >> > Fix below crash by adding a check in the drm_dp_dpcd_access which > >> > ensures that aux->transfer was actually initialized earlier. > >> > >> Gut feeling says this is papering over a real usage issue somewhere > >> else. Why is the aux being used for transfers before ->transfer has > >> been set? Why should the dp helper be defensive against all kinds of > misprogramming? > >> > >> > >> BR, > >> Jani. > >> > > > > The issue was found by Intel IGT test suite, graphic by pass test case. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitl > > ab.freedesktop.org%2Fdrm%2Figt-gpu- > tools&data=04%7C01%7CPerry.Yuan > > %40amd.com%7C83d011acfe65437c0fa808d99ddc65b0%7C3dd8961fe4884e6 > 08e11a8 > > > 2d994e183d%7C0%7C0%7C637714392203200313%7CUnknown%7CTWFpbGZsb > 3d8eyJWIj > > > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100 > 0&am > > > p;sdata=snPpRYLGeJtTpNGle1YHZAvevcABbgLkgOsffiNzQPw%3D&reserved > =0 > > normally use case will not see the issue. > > To avoid this issue happy again when we run the test case , it will be nice > > to > add a check before the transfer is called. > > And we can see that it really needs to have a check here to make ITG &kernel > happy. > > You're missing my point. What is the root cause? Why do you have the aux > device or connector registered before ->transfer function is initialized. I > don't > think you should do that. > > BR, > Jani. > One potential IGT fix patch to resolve the test case failure is: tests/amdgpu/amd_bypass.c data->pipe_crc = igt_pipe_crc_new(data->drm_fd, data->pipe_id, - AMDGPU_PIPE_CRC_SOURCE_DPRX); + INTEL_PIPE_CRC_SOURCE_AUTO); The kernel panic error gone after change "dprx" to "auto" in the IGT test. In my view ,the IGT amdgpu bypass test will do some common setup work including crc piple, source. When the IGT sets up a new CRC pipe capture source for amdgpu bypass test, the SOURCE was set as "dprx" instead of "auto" It makes "amdgpu_dm_crtc_set_crc_source()" failed to set correct AUX and it's transfer function invalid . The system I tested use HDMI port connected to monitor . amdgpu_dm_crtc_set_crc_source->(aux = (aconn->port) ? &aconn->port->aux : &aconn->dm_dp_aux.aux;) drm_dp_start_crc -> drm_dp_dpcd_readb-> aux->transfer is NULL, issue here. The fix will use the "auto" keyword, which will let the driver select a default source of frame CRCs for this CRTC. Correct me if have some wrong points. Thank you! Perry. > > > > > Perry. > > > >> > >> > > >> > BUG: kernel NULL pointer dereference, address: PGD > >> > 0 P4D 0 > >> > Oops: 0010 [#1] SMP NOPTI > >> > RIP: 0010:0x0 > >> > Code: Unable to access opcode bytes at RIP 0xffd6. > >> > RSP: 0018:a8d64225bab8 EFLAGS: 00010246 > >> > RAX: RBX: 0020 RCX: a8d64225bb5e > >> > RDX: 93151d921880 RSI: a8d64225bac8 RDI: 931511a1a9d8 > >> > RBP: a8d64225bb10 R08: 0001 R09: a8d64225ba60 > >> > R10: 0002 R11: 000d R12: 0001 > >> > R13: R14: a8d64225bb5e R15: 931511a1a9d8 > >> > FS: 7ff8ea7fa9c0() GS:9317fe6c() > >> > knlGS: > >> > CS: 0010 DS: ES: CR0: 80050033 > >> > CR2: ffd6 CR3: 00010d5a4000 CR4: 00750ee0 > >> > PKRU: 5554 > >> > Call Trace: > >> > drm_dp_dpcd_access+0x72/0x110 [drm_kms_helper] > >> > drm_dp_dpcd_read+0xb7/0xf0 [drm_kms_helper] > >> > drm_dp_start_crc+0x38/0xb0 [drm_kms_helper] > >> > amdgpu_dm_crtc_set_crc_source+0x1ae/0x3e0 [amdgpu] > >> > crtc_crc_open+0x174/0x220 [drm] > >> > full_proxy_open+0x168/0x1f0 > >> > ? open_proxy_open+0x100/0
[Bug 214029] [bisected] [amdgpu] Several memory leaks in amdgpu and ttm
https://bugzilla.kernel.org/show_bug.cgi?id=214029 Erhard F. (erhar...@mailbox.org) changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |CODE_FIX --- Comment #23 from Erhard F. (erhar...@mailbox.org) --- The fix landed in kernel 5.15, 5.14.16 and affected LTS kernels. Closing. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On 02/11/2021 15:59, Maxime Ripard wrote:
> A lot of drivers open-code the HDMI 1.4 maximum pixel rate in their
> driver to test whether the resolutions are supported or if the
> scrambling needs to be enabled.
>
> Let's create a common define for everyone to use it.
>
> Cc: Alex Deucher
> Cc: amd-...@lists.freedesktop.org
> Cc: Andrzej Hajda
> Cc: Benjamin Gaignard
> Cc: "Christian König"
> Cc: Emma Anholt
> Cc: intel-...@lists.freedesktop.org
> Cc: Jani Nikula
> Cc: Jernej Skrabec
> Cc: Jerome Brunet
> Cc: Jonas Karlman
> Cc: Jonathan Hunter
> Cc: Joonas Lahtinen
> Cc: Kevin Hilman
> Cc: Laurent Pinchart
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> Cc: Martin Blumenstingl
> Cc: Neil Armstrong
> Cc: "Pan, Xinhui"
> Cc: Robert Foss
> Cc: Rodrigo Vivi
> Cc: Thierry Reding
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 ++--
> drivers/gpu/drm/drm_edid.c | 2 +-
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_encoders.c | 2 +-
> drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c | 2 +-
> drivers/gpu/drm/tegra/sor.c| 8
> drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
> include/drm/drm_connector.h| 2 ++
> 9 files changed, 16 insertions(+), 14 deletions(-)
For meson & bridge/synopsys/dw-hdmi:
Acked-by: Neil Armstrong
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 62ae63565d3a..3a58db357be0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -46,7 +46,7 @@
> /* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
> #define SCDC_MIN_SOURCE_VERSION 0x1
>
> -#define HDMI14_MAX_TMDSCLK 34000
> +#define HDMI14_MAX_TMDSCLK (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
>
> enum hdmi_datamap {
> RGB444_8B = 0x01,
> @@ -1264,7 +1264,7 @@ static bool dw_hdmi_support_scdc(struct dw_hdmi *hdmi,
>* for low rates is not supported either
>*/
> if (!display->hdmi.scdc.scrambling.low_rates &&
> - display->max_tmds_clock <= 34)
> + display->max_tmds_clock <= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
> return false;
>
> return true;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7aa2a56a71c8..ec8fb2d098ae 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct
> drm_connector *connector,
> u32 max_tmds_clock = hf_vsdb[5] * 5000;
> struct drm_scdc *scdc = &hdmi->scdc;
>
> - if (max_tmds_clock > 34) {
> + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> display->max_tmds_clock = max_tmds_clock;
> DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder
> *encoder,
> if (scdc->scrambling.low_rates)
> pipe_config->hdmi_scrambling = true;
>
> - if (pipe_config->port_clock > 34) {
> + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> pipe_config->hdmi_scrambling = true;
> pipe_config->hdmi_high_tmds_clock_ratio = true;
> }
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 0afbd1e70bfc..8078667aea0e 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -434,7 +434,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void
> *data,
> readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>
> DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
> - mode->clock > 34 ? 40 : 10);
> + mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ ? 40 : 10);
>
> /* Enable clocks */
> regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0x, 0x100);
> @@ -457,7 +457,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void
> *data,
> dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>
> /* TMDS pattern setup */
> - if (mode->clock > 34 &&
> + if (mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ &&
> dw_hdmi->output_bus_fmt == MEDIA_BUS_FMT_YUV8_1X24) {
> dw_hdmi->data->top_write(dw_hdmi, HDMITX_
Re: [PATCH] drm/prime: Fix use after free in mmap with drm_gem_ttm_mmap
Hi
Am 03.11.21 um 01:12 schrieb Anand K. Mistry:
Any movement on merging this patch? Not urgent on my end (we have this
patch in our tree), but I think other amd users might run into this
UAF.
Thanks for reminding. I've merged your patch into drm-misc-fixes.
Best regards
Thomas
On Thu, 30 Sept 2021 at 17:21, Thomas Zimmermann wrote:
Hi
Am 30.09.21 um 01:00 schrieb Anand K Mistry:
drm_gem_ttm_mmap() drops a reference to the gem object on success. If
the gem object's refcount == 1 on entry to drm_gem_prime_mmap(), that
drop will free the gem object, and the subsequent drm_gem_object_get()
will be a UAF. Fix by grabbing a reference before calling the mmap
helper.
This issue was forseen when the reference dropping was adding in
commit 9786b65bc61ac ("drm/ttm: fix mmap refcounting"):
"For that to work properly the drm_gem_object_get() call in
drm_gem_ttm_mmap() must be moved so it happens before calling
obj->funcs->mmap(), otherwise the gem refcount would go down
to zero."
Signed-off-by: Anand K Mistry
Acked-by: Thomas Zimmermann
This looks fine to me, but it affects many drivers. Let's maybe wait a
bit if more reviews come it.
Best regards
Thomas
---
drivers/gpu/drm/drm_prime.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 2a54f86856af..e1854fd24bb0 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -719,11 +719,13 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct
vm_area_struct *vma)
if (obj->funcs && obj->funcs->mmap) {
vma->vm_ops = obj->funcs->vm_ops;
+ drm_gem_object_get(obj);
ret = obj->funcs->mmap(obj, vma);
- if (ret)
+ if (ret) {
+ drm_gem_object_put(obj);
return ret;
+ }
vma->vm_private_data = obj;
- drm_gem_object_get(obj);
return 0;
}
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
[PATCH] drm: mxsfb: Check NULL pointer
As we see in the drm_connector_list_iter_next(), it could return
NULL. In order to avoid the use of the NULL pointer, it may be
better to check the return value.
Fixes: c42001e ("drm: mxsfb: Use drm_panel_bridge")
Signed-off-by: Jiasheng Jiang
---
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 6da9355..b875c11 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -145,6 +145,8 @@ static int mxsfb_attach_bridge(struct mxsfb_drm_private
*mxsfb)
*/
drm_connector_list_iter_begin(drm, &iter);
mxsfb->connector = drm_connector_list_iter_next(&iter);
+ if (!mxsfb->connector)
+ return 1;
drm_connector_list_iter_end(&iter);
return 0;
--
2.7.4
Re: [PATCH] drm: mxsfb: Check NULL pointer
On 11/3/21 8:48 AM, Jiasheng Jiang wrote:
As we see in the drm_connector_list_iter_next(), it could return
NULL. In order to avoid the use of the NULL pointer, it may be
better to check the return value.
Fixes: c42001e ("drm: mxsfb: Use drm_panel_bridge")
Signed-off-by: Jiasheng Jiang
---
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 6da9355..b875c11 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -145,6 +145,8 @@ static int mxsfb_attach_bridge(struct mxsfb_drm_private
*mxsfb)
*/
drm_connector_list_iter_begin(drm, &iter);
mxsfb->connector = drm_connector_list_iter_next(&iter);
+ if (!mxsfb->connector)
+ return 1;
In which case does this happen failure happen ?
What is the test case ?
Re: [PATCH 6/6] drm/radeon: use dma_resv_wait_timeout() instead of manually waiting
Ping, Alex do you have a moment for that one here?
Am 28.10.21 um 15:26 schrieb Christian König:
Don't touch the exclusive fence manually here, but rather use the
general dma_resv function. We did that for better hw reset handling but
this doesn't necessary work correctly.
Signed-off-by: Christian König
---
drivers/gpu/drm/radeon/radeon_uvd.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c
b/drivers/gpu/drm/radeon/radeon_uvd.c
index 2ea86919d953..377f9cdb5b53 100644
--- a/drivers/gpu/drm/radeon/radeon_uvd.c
+++ b/drivers/gpu/drm/radeon/radeon_uvd.c
@@ -469,7 +469,6 @@ static int radeon_uvd_cs_msg(struct radeon_cs_parser *p,
struct radeon_bo *bo,
{
int32_t *msg, msg_type, handle;
unsigned img_size = 0;
- struct dma_fence *f;
void *ptr;
int i, r;
@@ -479,13 +478,11 @@ static int radeon_uvd_cs_msg(struct radeon_cs_parser *p,
struct radeon_bo *bo,
return -EINVAL;
}
- f = dma_resv_excl_fence(bo->tbo.base.resv);
- if (f) {
- r = radeon_fence_wait((struct radeon_fence *)f, false);
- if (r) {
- DRM_ERROR("Failed waiting for UVD message (%d)!\n", r);
- return r;
- }
+ r = dma_resv_wait_timeout(bo->tbo.base.resv, false, false,
+ MAX_SCHEDULE_TIMEOUT);
+ if (r <= 0) {
+ DRM_ERROR("Failed waiting for UVD message (%d)!\n", r);
+ return r ? r : -ETIME;
}
r = radeon_bo_kmap(bo, &ptr);
Re: [PATCH] drm/virtio: delay pinning the pages till first use
On Tue, Nov 02, 2021 at 08:58:55AM -0700, Chia-I Wu wrote: > On Tue, Nov 2, 2021 at 6:07 AM Gerd Hoffmann wrote: > > > > On Tue, Nov 02, 2021 at 12:31:39PM +0100, Maksym Wezdecki wrote: > > > From: mwezdeck > > > > > > The idea behind the commit: > > > 1. not pin the pages during resource_create ioctl > > > 2. pin the pages on the first use during: > > > - transfer_*_host ioctl > > > - map ioctl > > > > i.e. basically lazy pinning. Approach looks sane to me. > > > > > 3. introduce new ioctl for pinning pages on demand > > > > What is the use case for this ioctl? > > In any case this should be a separate patch. > > Lazy pinning can be a nice optimization that userspace does not > necessarily need to know about. This patch however skips pinning for > execbuffer ioctl and introduces a new pin ioctl instead. That is a > red flag. Ah, so the pin ioctl is for buffers which need a pin for execbuffer. Yep, that isn't going to fly that way, it'll break old userspace. Lazy pinning must be opt-in, so new userspace which knows about the pin ioctl can enable lazy pinning. One possible way would be to add a flag for the VIRTGPU_RESOURCE_CREATE ioctl, so lazy pinning can be enabled per resource. take care, Gerd
