Re: linux-next: build failure after merge of the drm-misc tree
Hi all, On Mon, 1 Nov 2021 19:42:23 +1100 Stephen Rothwell wrote: > > On Fri, 15 Oct 2021 20:26:48 +1100 Stephen Rothwell > wrote: > > > > After merging the drm-misc tree, today's linux-next build (arm > > multi_v7_defconfig) failed like this: > > > > drivers/gpu/drm/drm_modeset_lock.c:111:29: error: conflicting types for > > '__stack_depot_save' > > 111 | static depot_stack_handle_t __stack_depot_save(void) > > | ^~ > > In file included from include/linux/page_ext.h:7, > > from include/linux/mm.h:25, > > from include/linux/kallsyms.h:13, > > from include/linux/bpf.h:20, > > from include/linux/bpf-cgroup.h:5, > > from include/linux/cgroup-defs.h:22, > > from include/linux/cgroup.h:28, > > from include/linux/memcontrol.h:13, > > from include/linux/swap.h:9, > > from include/linux/suspend.h:5, > > from include/linux/regulator/consumer.h:35, > > from include/linux/i2c.h:18, > > from include/drm/drm_crtc.h:28, > > from include/drm/drm_atomic.h:31, > > from drivers/gpu/drm/drm_modeset_lock.c:24: > > include/linux/stackdepot.h:18:22: note: previous declaration of > > '__stack_depot_save' was here > >18 | depot_stack_handle_t __stack_depot_save(unsigned long *entries, > > | ^~ > > > > Caused by commit > > > > cd06ab2fd48f ("drm/locking: add backtrace for locking contended locks > > without backoff") > > > > This may only have been revealed because of another fix I have had to > > apply today. > > > > I have applied the following patch for today. > > > > From: Stephen Rothwell > > Date: Fri, 15 Oct 2021 20:17:52 +1100 > > Subject: [PATCH] drm/locking: fix for name conflict > > > > Fixes: cd06ab2fd48f ("drm/locking: add backtrace for locking contended > > locks without backoff") > > Signed-off-by: Stephen Rothwell > > --- > > drivers/gpu/drm/drm_modeset_lock.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_modeset_lock.c > > b/drivers/gpu/drm/drm_modeset_lock.c > > index 4d32b61fa1fd..ee36dd20900d 100644 > > --- a/drivers/gpu/drm/drm_modeset_lock.c > > +++ b/drivers/gpu/drm/drm_modeset_lock.c > > @@ -79,7 +79,7 @@ > > static DEFINE_WW_CLASS(crtc_ww_class); > > > > #if IS_ENABLED(CONFIG_DRM_DEBUG_MODESET_LOCK) > > -static noinline depot_stack_handle_t __stack_depot_save(void) > > +static noinline depot_stack_handle_t __drm_stack_depot_save(void) > > { > > unsigned long entries[8]; > > unsigned int n; > > @@ -108,7 +108,7 @@ static void __stack_depot_print(depot_stack_handle_t > > stack_depot) > > kfree(buf); > > } > > #else /* CONFIG_DRM_DEBUG_MODESET_LOCK */ > > -static depot_stack_handle_t __stack_depot_save(void) > > +static depot_stack_handle_t __drm_stack_depot_save(void) > > { > > return 0; > > } > > @@ -317,7 +317,7 @@ static inline int modeset_lock(struct drm_modeset_lock > > *lock, > > ret = 0; > > } else if (ret == -EDEADLK) { > > ctx->contended = lock; > > - ctx->stack_depot = __stack_depot_save(); > > + ctx->stack_depot = __drm_stack_depot_save(); > > } > > > > return ret; > > This has reappeared today. I don't know what happened to the drm-misc > tree over the weeked :-( > > I have reapplied the above fix. So the above drm-misc commit is now in the drm tree, but its fix up commit vanished from the drm-misc tree over the past weekend :-( -- Cheers, Stephen Rothwell pgp7tciiPUGNB.pgp Description: OpenPGP digital signature
Re: [PATCH] drm/msm/devfreq: Fix OPP refcnt leak
On 11/4/21 5:28 PM, Rob Clark wrote: From: Rob Clark Reported-by: Douglas Anderson Fixes: 9bc95570175a ("drm/msm: Devfreq tuning") Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_gpu_devfreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c index d32b729b4616..9bf8600b6eea 100644 --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c @@ -20,8 +20,6 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq, struct msm_gpu *gpu = dev_to_gpu(dev); struct dev_pm_opp *opp; - opp = devfreq_recommended_opp(dev, freq, flags); - /* * If the GPU is idle, devfreq is not aware, so just ignore * it's requests @@ -31,6 +29,8 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq, return 0; } + opp = devfreq_recommended_opp(dev, freq, flags); + if (IS_ERR(opp)) return PTR_ERR(opp); Testing this here on the Lenovo Yoga C630, and I'm starting to see in my dmesg output [ 36.337061] devfreq 500.gpu: Couldn't update frequency transition information. [ 36.388122] devfreq 500.gpu: Couldn't update frequency transition information. [ 36.810941] wcd934x-codec wcd934x-codec.3.auto: Port Closed RX port 1, value 4 [ 36.811914] wcd934x-codec wcd934x-codec.3.auto: Port Closed RX port 2, value 4 [ 198.794946] devfreq 500.gpu: Couldn't update frequency transition information. [ 198.845698] devfreq 500.gpu: Couldn't update frequency transition information. [ 502.285421] devfreq 500.gpu: Couldn't update frequency transition information. [ 502.339427] devfreq 500.gpu: Couldn't update frequency transition information. [ 503.361469] devfreq 500.gpu: Couldn't update frequency transition information. [ 503.412757] devfreq 500.gpu: Couldn't update frequency transition information. [ 503.871480] devfreq 500.gpu: Couldn't update frequency transition information. [ 503.922712] devfreq 500.gpu: Couldn't update frequency transition information. [ 503.974474] devfreq 500.gpu: Couldn't update frequency transition information. [ 504.025501] devfreq 500.gpu: Couldn't update frequency transition information. [ 505.923563] devfreq 500.gpu: Couldn't update frequency transition information. [ 505.974513] devfreq 500.gpu: Couldn't update frequency transition information. [ 510.313052] usb 3-1.3: USB disconnect, device number 4 [ 519.677148] usb 3-1.3: new high-speed USB device number 5 using xhci-hcd [ 519.793394] usb 3-1.3: New USB device found, idVendor=5986, idProduct=2115, bcdDevice=54.20 [ 519.793441] usb 3-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [ 519.793472] usb 3-1.3: Product: Integrated Camera [ 519.793495] usb 3-1.3: Manufacturer: SunplusIT Inc [ 519.861020] usb 3-1.3: Found UVC 1.00 device Integrated Camera (5986:2115) [ 519.892879] input: Integrated Camera: Integrated C as /devices/platform/soc@0/a8f8800.usb/a80.dwc3/xhci-hcd.1.auto/usb3/3-1/3-1.3/3-1.3:1.0/input/input27 [ 520.283839] devfreq 500.gpu: Couldn't update frequency transition information. [ 520.335854] devfreq 500.gpu: Couldn't update frequency transition information. Is this intended?
[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, +
[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
Re: [PATCH v12 0/4] Add MIPI rx DPI support
On Thu, Nov 04, 2021 at 11:20:21AM +0100, Robert Foss wrote: > Hey Xin, > > Applied to drm-misc-next. > > The way this series was submitted to the mailing list is not correct > and is breaking a lot of tooling. It seems like you used git > send-email, but the individual patches of the series are not connected > properly and both b4 and the patchwork tools are not able to handle > this series properly. > > Please try to use git send-email along the lines of this: > git send-email -$NBR_PATCHES_IN_SERIES -v$VERSION_OF_SERIES --annotate > --to= Hi Rob, thanks for the comment, sorry about that. I'll submit v12 by your command. Xin > > > Rob.
[PATCH v4 14/14] drm/msm: Implement HDCP 1.x using the new drm HDCP helpers
From: Sean Paul This patch adds HDCP 1.x support to msm DP connectors using the new HDCP helpers. Cc: Stephen Boyd Cc: Abhinav Kumar Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-15-s...@poorly.run #v3 Changes in v2: -Squash [1] into this patch with the following changes (Stephen) -Update the sc7180 dtsi file -Remove resource names and just use index (Stephen) Changes in v3: -Split out the dtsi change from v2 (Stephen) -Fix set-but-unused warning identified by 0-day -Fix up a couple of style nits (Stephen) -Store HDCP key directly in dp_hdcp struct (Stephen) -Remove wmb in HDCP key initialization, move an_seed (Stephen) -Use FIELD_PREP for bstatus/bcaps (Stephen) -#define read_poll_timeout values (Stephen) -Remove unnecessary parentheses in dp_hdcp_store_ksv_fifo (Stephen) -Add compatible string for hdcp (Stephen) -Rename dp_hdcp_write_* functions (Abhinav) -Add 1us delay between An reads (Abhinav) -Delete unused dp_hdcp_read_* functions Changes in v4: -Rebase on Bjorn's multi-dp patchset [1] https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-14-s...@poorly.run --- drivers/gpu/drm/msm/Makefile| 1 + drivers/gpu/drm/msm/dp/dp_debug.c | 46 ++- drivers/gpu/drm/msm/dp/dp_debug.h | 6 +- drivers/gpu/drm/msm/dp/dp_display.c | 46 ++- drivers/gpu/drm/msm/dp/dp_display.h | 5 + drivers/gpu/drm/msm/dp/dp_drm.c | 68 +++- drivers/gpu/drm/msm/dp/dp_drm.h | 5 + drivers/gpu/drm/msm/dp/dp_hdcp.c| 462 drivers/gpu/drm/msm/dp/dp_hdcp.h| 27 ++ drivers/gpu/drm/msm/dp/dp_parser.c | 20 +- drivers/gpu/drm/msm/dp/dp_parser.h | 4 + drivers/gpu/drm/msm/dp/dp_reg.h | 32 +- drivers/gpu/drm/msm/msm_atomic.c| 15 + 13 files changed, 729 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 40577f8856d8..fb3411a74e61 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -108,6 +108,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \ dp/dp_ctrl.o \ dp/dp_display.o \ dp/dp_drm.o \ + dp/dp_hdcp.o \ dp/dp_hpd.o \ dp/dp_link.o \ dp/dp_panel.o \ diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c index da4323556ef3..c16fce17d096 100644 --- a/drivers/gpu/drm/msm/dp/dp_debug.c +++ b/drivers/gpu/drm/msm/dp/dp_debug.c @@ -8,6 +8,7 @@ #include #include #include +#include #include "dp_parser.h" #include "dp_catalog.h" @@ -15,6 +16,7 @@ #include "dp_ctrl.h" #include "dp_debug.h" #include "dp_display.h" +#include "dp_hdcp.h" #define DEBUG_NAME "msm_dp" @@ -25,6 +27,7 @@ struct dp_debug_private { struct dp_link *link; struct dp_panel *panel; struct drm_connector *connector; + struct dp_hdcp *hdcp; struct device *dev; struct drm_device *drm_dev; @@ -198,6 +201,35 @@ static int dp_test_active_open(struct inode *inode, inode->i_private); } +static ssize_t dp_hdcp_key_write(struct file *file, const char __user *ubuf, +size_t len, loff_t *offp) +{ + char *input_buffer; + int ret; + struct dp_debug_private *debug = file->private_data; + + if (len != (DRM_HDCP_KSV_LEN + DP_HDCP_NUM_KEYS * DP_HDCP_KEY_LEN)) + return -EINVAL; + + if (!debug->hdcp) + return -ENOENT; + + input_buffer = memdup_user_nul(ubuf, len); + if (IS_ERR(input_buffer)) + return PTR_ERR(input_buffer); + + ret = dp_hdcp_ingest_key(debug->hdcp, input_buffer, len); + + kfree(input_buffer); + if (ret < 0) { + DRM_ERROR("Could not ingest HDCP key, ret=%d\n", ret); + return ret; + } + + *offp += len; + return len; +} + static const struct file_operations test_active_fops = { .owner = THIS_MODULE, .open = dp_test_active_open, @@ -207,6 +239,12 @@ static const struct file_operations test_active_fops = { .write = dp_test_active_write }; +static const struct file_operations dp_hdcp_key_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .write = dp_hdcp_key_write, +}; + static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor *minor) { int rc = 0; @@ -228,6 +266,10 @@ static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor *minor) minor->debugfs_root, debug, &dp_test_type_fops); + debugfs_create_file("msm_dp_hdcp_key", 0222, + minor->debugfs_r
[PATCH v4 13/14] arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller
From: Sean Paul This patch adds the register ranges required for HDCP key injection and HDCP TrustZone interaction as described in the dt-bindings for the sc7180 dp controller. Now that these are supported, change the compatible string to "dp-hdcp". Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-14-s...@poorly.run #v3 Changes in v3: -Split off into a new patch containing just the dts change (Stephen) -Add hdcp compatible string (Stephen) Changes in v4: -Rebase on Bjorn's multi-dp patchset --- arch/arm64/boot/dts/qcom/sc7180.dtsi | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index c8921e2d6480..838270f70b62 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -3088,7 +3088,13 @@ mdss_dp: displayport-controller@ae9 { compatible = "qcom,sc7180-dp"; status = "disabled"; - reg = <0 0x0ae9 0 0x1400>; + reg = <0 0x0ae9 0 0x200>, + <0 0x0ae90200 0 0x200>, + <0 0x0ae90400 0 0xc00>, + <0 0x0ae91000 0 0x400>, + <0 0x0ae91400 0 0x400>, + <0 0x0aed1000 0 0x175>, + <0 0x0aee1000 0 0x2c>; interrupt-parent = <&mdss>; interrupts = <12>; -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
From: Sean Paul This patch adds the bindings for the MSM DisplayPort HDCP registers which are required to write the HDCP key into the display controller as well as the registers to enable HDCP authentication/key exchange/encryption. We'll use a new compatible string for this since the fields are optional. Cc: Rob Herring Cc: Stephen Boyd Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-13-s...@poorly.run #v3 Changes in v2: -Drop register range names (Stephen) -Fix yaml errors (Rob) Changes in v3: -Add new compatible string for dp-hdcp -Add descriptions to reg -Add minItems/maxItems to reg -Make reg depend on the new hdcp compatible string Changes in v4: -Rebase on Bjorn's multi-dp patchset --- .../devicetree/bindings/display/msm/dp-controller.yaml| 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index b36d74c1da7c..f6e4b102373a 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -21,12 +21,16 @@ properties: - qcom,sc8180x-edp reg: +minItems: 5 +maxItems: 7 items: - description: ahb register block - description: aux register block - description: link register block - description: p0 register block - description: p1 register block + - description: (Optional) Registers for HDCP device key injection + - description: (Optional) Registers for HDCP TrustZone interaction interrupts: maxItems: 1 @@ -111,7 +115,9 @@ examples: <0xae90200 0x200>, <0xae90400 0xc00>, <0xae91000 0x400>, - <0xae91400 0x400>; + <0xae91400 0x400>, + <0x0aed1000 0x174>, + <0x0aee1000 0x2c>; interrupt-parent = <&mdss>; interrupts = <12>; clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 11/14] drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules
From: Sean Paul Audio is initialized last, it should be de-initialized first to match the order in dp_init_sub_modules(). Reviewed-by: Abhinav Kumar Reviewed-by: Stephen Boyd Reviewed-by: Dmitry Baryshkov Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-12-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-12-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-12-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/msm/dp/dp_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index aba8aa47ed76..79412a8fbaff 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -707,9 +707,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data) static void dp_display_deinit_sub_modules(struct dp_display_private *dp) { dp_debug_put(dp->debug); + dp_audio_put(dp->audio); dp_panel_put(dp->panel); dp_aux_put(dp->aux); - dp_audio_put(dp->audio); } static int dp_init_sub_modules(struct dp_display_private *dp) -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 10/14] drm/msm/dpu: Remove encoder->enable() hack
From: Sean Paul encoder->commit() was being misused because there were some global resources which needed to be tweaked in encoder->enable() which were not accessible in dpu_encoder.c. That is no longer true and the redirect serves no purpose any longer. So remove the indirection. Tested-by: Stephen Boyd Reviewed-by: Stephen Boyd Reviewed-by: Abhinav Kumar Reviewed-by: Dmitry Baryshkov Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-11-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-11-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-11-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 - drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 -- drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 4 4 files changed, 1 insertion(+), 32 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index cc57c615be67..c83bfda6a1ee 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -2116,11 +2116,8 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { .mode_set = dpu_encoder_virt_mode_set, .disable = dpu_encoder_virt_disable, - .enable = dpu_kms_encoder_enable, + .enable = dpu_encoder_virt_enable, .atomic_check = dpu_encoder_virt_atomic_check, - - /* This is called by dpu_kms_encoder_enable */ - .commit = dpu_encoder_virt_enable, }; static const struct drm_encoder_funcs dpu_encoder_funcs = { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 66b7df7daa6a..891faf8d6e21 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -384,28 +384,6 @@ static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask) } } -/* - * Override the encoder enable since we need to setup the inline rotator and do - * some crtc magic before enabling any bridge that might be present. - */ -void dpu_kms_encoder_enable(struct drm_encoder *encoder) -{ - const struct drm_encoder_helper_funcs *funcs = encoder->helper_private; - struct drm_device *dev = encoder->dev; - struct drm_crtc *crtc; - - /* Forward this enable call to the commit hook */ - if (funcs && funcs->commit) - funcs->commit(encoder); - - drm_for_each_crtc(crtc, dev) { - if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder))) - continue; - - trace_dpu_kms_enc_enable(DRMID(crtc)); - } -} - static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask) { struct dpu_kms *dpu_kms = to_dpu_kms(kms); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 775bcbda860f..0707b2cb43c8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -235,8 +235,6 @@ void *dpu_debugfs_get_root(struct dpu_kms *dpu_kms); int dpu_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc); void dpu_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc); -void dpu_kms_encoder_enable(struct drm_encoder *encoder); - /** * dpu_kms_get_clk_rate() - get the clock rate * @dpu_kms: pointer to dpu_kms structure diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h index 37bba57675a8..54d74341e690 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h @@ -266,10 +266,6 @@ DEFINE_EVENT(dpu_drm_obj_template, dpu_crtc_complete_commit, TP_PROTO(uint32_t drm_id), TP_ARGS(drm_id) ); -DEFINE_EVENT(dpu_drm_obj_template, dpu_kms_enc_enable, - TP_PROTO(uint32_t drm_id), - TP_ARGS(drm_id) -); DEFINE_EVENT(dpu_drm_obj_template, dpu_kms_commit, TP_PROTO(uint32_t drm_id), TP_ARGS(drm_id) -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 09/14] drm/msm/dpu: Remove useless checks in dpu_encoder
From: Sean Paul A couple more useless checks to remove in dpu_encoder. Reviewed-by: Stephen Boyd Reviewed-by: Abhinav Kumar Reviewed-by: Dmitry Baryshkov Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-10-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-10-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-10-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index e7ee4cfb8461..cc57c615be67 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1148,10 +1148,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) struct msm_drm_private *priv; struct drm_display_mode *cur_mode = NULL; - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } dpu_enc = to_dpu_encoder_virt(drm_enc); mutex_lock(&dpu_enc->enc_lock); @@ -1197,14 +1193,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) struct msm_drm_private *priv; int i = 0; - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } else if (!drm_enc->dev) { - DPU_ERROR("invalid dev\n"); - return; - } - dpu_enc = to_dpu_encoder_virt(drm_enc); DPU_DEBUG_ENC(dpu_enc, "\n"); -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 08/14] drm/msm/dpu_kms: Re-order dpu includes
From: Sean Paul Make includes alphabetical in dpu_kms.c Reviewed-by: Abhinav Kumar Reviewed-by: Stephen Boyd Reviewed-by: Dmitry Baryshkov Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-9-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-9-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-9-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index a15b26428280..66b7df7daa6a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -21,14 +21,14 @@ #include "msm_gem.h" #include "disp/msm_disp_snapshot.h" -#include "dpu_kms.h" #include "dpu_core_irq.h" +#include "dpu_crtc.h" +#include "dpu_encoder.h" #include "dpu_formats.h" #include "dpu_hw_vbif.h" -#include "dpu_vbif.h" -#include "dpu_encoder.h" +#include "dpu_kms.h" #include "dpu_plane.h" -#include "dpu_crtc.h" +#include "dpu_vbif.h" #define CREATE_TRACE_POINTS #include "dpu_trace.h" -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 07/14] drm/i915/hdcp: Use HDCP helpers for i915
From: Sean Paul Now that all of the HDCP 1.x logic has been migrated to the central HDCP helpers, use it in the i915 driver. The majority of the driver code for HDCP 1.x will live in intel_hdcp.c, however there are a few helper hooks which are connector-specific and need to be partially or fully implemented in the intel_dp_hdcp.c or intel_hdmi.c. We'll leave most of the HDCP 2.x code alone since we don't have another implementation of HDCP 2.x to use as reference for what should and should not live in the drm helpers. The helper will call the overly general enable/disable/is_capable HDCP 2.x callbacks and leave the interesting stuff for the driver. Once we have another HDCP 2.x implementation, we should do a similar migration. Acked-by: Jani Nikula Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-8-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-8-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-8-s...@poorly.run #v3 Changes in v2: -Fix mst helper function pointer reported by 0-day Changes in v3: -Add forward declaration for drm_atomic_state in intel_hdcp.h identified by 0-day Changes in v4: -None --- drivers/gpu/drm/i915/display/intel_ddi.c | 29 +- .../drm/i915/display/intel_display_debugfs.c | 6 +- .../drm/i915/display/intel_display_types.h| 58 +- drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 345 +++ drivers/gpu/drm/i915/display/intel_dp_mst.c | 17 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 935 +++--- drivers/gpu/drm/i915/display/intel_hdcp.h | 31 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++--- 8 files changed, 418 insertions(+), 1259 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 145d51ac43a3..bfdf586207d2 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -26,6 +26,7 @@ */ #include +#include #include "i915_drv.h" #include "intel_audio.h" @@ -3016,6 +3017,9 @@ static void intel_enable_ddi(struct intel_atomic_state *state, const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state) { + struct intel_connector *connector = to_intel_connector(conn_state->connector); + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); + drm_WARN_ON(state->base.dev, crtc_state->has_pch_encoder); if (!crtc_state->bigjoiner_slave) @@ -3032,12 +3036,10 @@ static void intel_enable_ddi(struct intel_atomic_state *state, else intel_enable_ddi_dp(state, encoder, crtc_state, conn_state); - /* Enable hdcp if it's desired */ - if (conn_state->content_protection == - DRM_MODE_CONTENT_PROTECTION_DESIRED) - intel_hdcp_enable(to_intel_connector(conn_state->connector), - crtc_state, - (u8)conn_state->hdcp_content_type); + if (connector->hdcp_helper_data) + drm_hdcp_helper_atomic_commit(connector->hdcp_helper_data, + &state->base, + &dig_port->hdcp_mutex); } static void intel_disable_ddi_dp(struct intel_atomic_state *state, @@ -3088,7 +3090,13 @@ static void intel_disable_ddi(struct intel_atomic_state *state, const struct intel_crtc_state *old_crtc_state, const struct drm_connector_state *old_conn_state) { - intel_hdcp_disable(to_intel_connector(old_conn_state->connector)); + struct intel_connector *connector = to_intel_connector(old_conn_state->connector); + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); + + if (connector->hdcp_helper_data) + drm_hdcp_helper_atomic_commit(connector->hdcp_helper_data, + &state->base, + &dig_port->hdcp_mutex); if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) intel_disable_ddi_hdmi(state, encoder, old_crtc_state, @@ -3118,13 +3126,18 @@ void intel_ddi_update_pipe(struct intel_atomic_state *state, const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state) { + struct intel_connector *connector = to_intel_connector(conn_state->connector); + struct intel_digital_port *dig_port = enc_to_dig_port(encoder); if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) && !intel_encoder_is_mst(encoder)) intel_ddi_update_pipe_dp(state, encoder, crtc_state, conn_state); - intel_hdcp_update_pipe(state, encoder, crt
[PATCH v4 06/14] drm/i915/hdcp: Retain hdcp_capable return codes
From: Sean Paul The shim functions return error codes, but they are discarded in intel_hdcp.c. This patch plumbs the return codes through so they are properly handled. Acked-by: Jani Nikula Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-7-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-7-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-7-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- .../drm/i915/display/intel_display_debugfs.c | 9 +++- drivers/gpu/drm/i915/display/intel_hdcp.c | 51 ++- drivers/gpu/drm/i915/display/intel_hdcp.h | 4 +- 3 files changed, 37 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index d7d6dde518a3..ef3039fb1e0c 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c @@ -643,6 +643,7 @@ static void intel_panel_info(struct seq_file *m, struct intel_panel *panel) static void intel_hdcp_info(struct seq_file *m, struct intel_connector *intel_connector) { + int ret; bool hdcp_cap, hdcp2_cap; if (!intel_connector->hdcp.shim) { @@ -650,8 +651,12 @@ static void intel_hdcp_info(struct seq_file *m, goto out; } - hdcp_cap = intel_hdcp_capable(intel_connector); - hdcp2_cap = intel_hdcp2_capable(intel_connector); + ret = intel_hdcp_capable(intel_connector, &hdcp_cap); + if (ret) + hdcp_cap = false; + ret = intel_hdcp2_capable(intel_connector, &hdcp2_cap); + if (ret) + hdcp2_cap = false; if (hdcp_cap) seq_puts(m, "HDCP1.4 "); diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 8fc830e38311..ac05d2c6d3e7 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -153,50 +153,49 @@ int intel_hdcp_read_valid_bksv(struct intel_digital_port *dig_port, } /* Is HDCP1.4 capable on Platform and Sink */ -bool intel_hdcp_capable(struct intel_connector *connector) +int intel_hdcp_capable(struct intel_connector *connector, bool *capable) { struct intel_digital_port *dig_port = intel_attached_dig_port(connector); const struct intel_hdcp_shim *shim = connector->hdcp.shim; - bool capable = false; u8 bksv[5]; + *capable = false; + if (!shim) - return capable; + return 0; - if (shim->hdcp_capable) { - shim->hdcp_capable(dig_port, &capable); - } else { - if (!intel_hdcp_read_valid_bksv(dig_port, shim, bksv)) - capable = true; - } + if (shim->hdcp_capable) + return shim->hdcp_capable(dig_port, capable); + + if (!intel_hdcp_read_valid_bksv(dig_port, shim, bksv)) + *capable = true; - return capable; + return 0; } /* Is HDCP2.2 capable on Platform and Sink */ -bool intel_hdcp2_capable(struct intel_connector *connector) +int intel_hdcp2_capable(struct intel_connector *connector, bool *capable) { struct intel_digital_port *dig_port = intel_attached_dig_port(connector); struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct intel_hdcp *hdcp = &connector->hdcp; - bool capable = false; + + *capable = false; /* I915 support for HDCP2.2 */ if (!hdcp->hdcp2_supported) - return false; + return 0; /* MEI interface is solid */ mutex_lock(&dev_priv->hdcp_comp_mutex); if (!dev_priv->hdcp_comp_added || !dev_priv->hdcp_master) { mutex_unlock(&dev_priv->hdcp_comp_mutex); - return false; + return 0; } mutex_unlock(&dev_priv->hdcp_comp_mutex); /* Sink's capability for HDCP2.2 */ - hdcp->shim->hdcp_2_2_capable(dig_port, &capable); - - return capable; + return hdcp->shim->hdcp_2_2_capable(dig_port, capable); } static bool intel_hdcp_in_use(struct drm_i915_private *dev_priv, @@ -2332,6 +2331,7 @@ int intel_hdcp_enable(struct intel_connector *connector, struct intel_digital_port *dig_port = intel_attached_dig_port(connector); struct intel_hdcp *hdcp = &connector->hdcp; unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS; + bool capable; int ret = -EINVAL; if (!hdcp->shim) @@ -2350,21 +2350,27 @@ int intel_hdcp_enable(struct intel_connector *connector, * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup * is capable of HDCP2.2, it is preferred to use HDCP2.2. */ -
[PATCH v4 05/14] drm/i915/hdcp: Consolidate HDCP setup/state cache
From: Sean Paul Stick all of the setup for HDCP into a dedicated function. No functional change, but this will facilitate moving HDCP logic into helpers. Acked-by: Jani Nikula Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-6-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-6-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-6-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/i915/display/intel_hdcp.c | 52 +++ 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index f12790697e2d..8fc830e38311 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2167,6 +2167,37 @@ static enum mei_fw_tc intel_get_mei_fw_tc(enum transcoder cpu_transcoder) } } +static int +_intel_hdcp_setup(struct intel_connector *connector, + const struct intel_crtc_state *pipe_config, u8 content_type) +{ + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct intel_hdcp *hdcp = &connector->hdcp; + int ret = 0; + + if (!connector->encoder) { + drm_err(&dev_priv->drm, "[%s:%d] encoder is not initialized\n", + connector->base.name, connector->base.base.id); + return -ENODEV; + } + + hdcp->content_type = content_type; + + if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) { + hdcp->cpu_transcoder = pipe_config->mst_master_transcoder; + hdcp->stream_transcoder = pipe_config->cpu_transcoder; + } else { + hdcp->cpu_transcoder = pipe_config->cpu_transcoder; + hdcp->stream_transcoder = INVALID_TRANSCODER; + } + + if (DISPLAY_VER(dev_priv) >= 12) + dig_port->hdcp_port_data.fw_tc = intel_get_mei_fw_tc(hdcp->cpu_transcoder); + + return ret; +} + static int initialize_hdcp_port_data(struct intel_connector *connector, struct intel_digital_port *dig_port, const struct intel_hdcp_shim *shim) @@ -2306,28 +2337,14 @@ int intel_hdcp_enable(struct intel_connector *connector, if (!hdcp->shim) return -ENOENT; - if (!connector->encoder) { - drm_err(&dev_priv->drm, "[%s:%d] encoder is not initialized\n", - connector->base.name, connector->base.base.id); - return -ENODEV; - } - mutex_lock(&hdcp->mutex); mutex_lock(&dig_port->hdcp_mutex); drm_WARN_ON(&dev_priv->drm, hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED); - hdcp->content_type = content_type; - - if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) { - hdcp->cpu_transcoder = pipe_config->mst_master_transcoder; - hdcp->stream_transcoder = pipe_config->cpu_transcoder; - } else { - hdcp->cpu_transcoder = pipe_config->cpu_transcoder; - hdcp->stream_transcoder = INVALID_TRANSCODER; - } - if (DISPLAY_VER(dev_priv) >= 12) - dig_port->hdcp_port_data.fw_tc = intel_get_mei_fw_tc(hdcp->cpu_transcoder); + ret = _intel_hdcp_setup(connector, pipe_config, content_type); + if (ret) + goto out; /* * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup @@ -2355,6 +2372,7 @@ int intel_hdcp_enable(struct intel_connector *connector, true); } +out: mutex_unlock(&dig_port->hdcp_mutex); mutex_unlock(&hdcp->mutex); return ret; -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 04/14] drm/hdcp: Expand HDCP helper library for enable/disable/check
From: Sean Paul This patch expands upon the HDCP helper library to manage HDCP enable, disable, and check. Previous to this patch, the majority of the state management and sink interaction is tucked inside the Intel driver with the understanding that once a new platform supported HDCP we could make good decisions about what should be centralized. With the addition of HDCP support for Qualcomm, it's time to migrate the protocol-specific bits of HDCP authentication, key exchange, and link checks to the HDCP helper. In terms of functionality, this migration is 1:1 with the Intel driver, however things are laid out a bit differently than with intel_hdcp.c, which is why this is a separate patch from the i915 transition to the helper. On i915, the shim vtable is used to account for HDMI vs. DP vs. DP-MST differences whereas the helper library uses a LUT to account for the register offsets and a remote read function to route the messages. On i915, storing the sink information in the source is done inline whereas now we use the new drm_hdcp_helper_funcs vtable to store and fetch information to/from source hw. Finally, instead of calling enable/disable directly from the driver, we'll leave that decision to the helper and by calling drm_hdcp_helper_atomic_commit() from the driver. All told, this will centralize the protocol and state handling in the helper, ensuring we collect all of our bugs^Wlogic in one place. Cc: Abhinav Kumar Acked-by: Jani Nikula Reviewed-by: Abhinav Kumar Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-5-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-5-s...@poorly.run #v3 Changes in v2: -Fixed set-but-unused variable identified by 0-day Changes in v3: -Fixed uninitialized variable warning identified by 0-day Changes in v4: -None --- drivers/gpu/drm/drm_hdcp.c | 1103 include/drm/drm_hdcp.h | 191 +++ 2 files changed, 1294 insertions(+) diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 8c851d40cd45..2bfa07fc3fbc 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -6,15 +6,20 @@ * Ramalingam C */ +#include #include #include #include +#include +#include #include #include #include +#include #include #include +#include #include #include #include @@ -513,3 +518,1101 @@ bool drm_hdcp_atomic_check(struct drm_connector *connector, return old_hdcp != new_hdcp; } EXPORT_SYMBOL(drm_hdcp_atomic_check); + +struct drm_hdcp_helper_data { + struct mutex mutex; + struct mutex *driver_mutex; + + struct drm_connector *connector; + const struct drm_hdcp_helper_funcs *funcs; + + u64 value; + unsigned int enabled_type; + + struct delayed_work check_work; + struct work_struct prop_work; + + struct drm_dp_aux *aux; + const struct drm_hdcp_hdcp1_receiver_reg_lut *hdcp1_lut; +}; + +struct drm_hdcp_hdcp1_receiver_reg_lut { + unsigned int bksv; + unsigned int ri; + unsigned int aksv; + unsigned int an; + unsigned int ainfo; + unsigned int v[5]; + unsigned int bcaps; + unsigned int bcaps_mask_repeater_present; + unsigned int bstatus; +}; + +static const struct drm_hdcp_hdcp1_receiver_reg_lut drm_hdcp_hdcp1_ddc_lut = { + .bksv = DRM_HDCP_DDC_BKSV, + .ri = DRM_HDCP_DDC_RI_PRIME, + .aksv = DRM_HDCP_DDC_AKSV, + .an = DRM_HDCP_DDC_AN, + .ainfo = DRM_HDCP_DDC_AINFO, + .v = { DRM_HDCP_DDC_V_PRIME(0), DRM_HDCP_DDC_V_PRIME(1), + DRM_HDCP_DDC_V_PRIME(2), DRM_HDCP_DDC_V_PRIME(3), + DRM_HDCP_DDC_V_PRIME(4) }, + .bcaps = DRM_HDCP_DDC_BCAPS, + .bcaps_mask_repeater_present = DRM_HDCP_DDC_BCAPS_REPEATER_PRESENT, + .bstatus = DRM_HDCP_DDC_BSTATUS, +}; + +static const struct drm_hdcp_hdcp1_receiver_reg_lut drm_hdcp_hdcp1_dpcd_lut = { + .bksv = DP_AUX_HDCP_BKSV, + .ri = DP_AUX_HDCP_RI_PRIME, + .aksv = DP_AUX_HDCP_AKSV, + .an = DP_AUX_HDCP_AN, + .ainfo = DP_AUX_HDCP_AINFO, + .v = { DP_AUX_HDCP_V_PRIME(0), DP_AUX_HDCP_V_PRIME(1), + DP_AUX_HDCP_V_PRIME(2), DP_AUX_HDCP_V_PRIME(3), + DP_AUX_HDCP_V_PRIME(4) }, + .bcaps = DP_AUX_HDCP_BCAPS, + .bcaps_mask_repeater_present = DP_BCAPS_REPEATER_PRESENT, + + /* +* For some reason the HDMI and DP HDCP specs call this register +* definition by different names. In the HDMI spec, it's called BSTATUS, +* but in DP it's called BINFO. +*/ + .bstatus = DP_AUX_HDCP_BINFO, +}; + +static int drm_hdcp_remote_ddc_read(struct i2c_adapter *i2c, + unsigned int offset, u8 *value, size_t len) +{ + int ret; + u8 start = offset
[PATCH v4 03/14] drm/hdcp: Update property value on content type and user changes
From: Sean Paul This patch updates the connector's property value in 2 cases which were previously missed: 1- Content type changes. The value should revert back to DESIRED from ENABLED in case the driver must re-authenticate the link due to the new content type. 2- Userspace sets value to DESIRED while ENABLED. In this case, the value should be reset immediately to ENABLED since the link is actively being encrypted. To accommodate these changes, I've split up the conditionals to make things a bit more clear (as much as one can with this mess of state). Acked-by: Jani Nikula Reviewed-by: Abhinav Kumar Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-4-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -Fixed indentation issue identified by 0-day Changes in v4: -None --- drivers/gpu/drm/drm_hdcp.c | 26 +- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index dd8fa91c51d6..8c851d40cd45 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector *connector, return true; /* -* Nothing to do if content type is unchanged and one of: -* - state didn't change +* Content type changes require an HDCP disable/enable cycle. +*/ + if (new_conn_state->hdcp_content_type != old_conn_state->hdcp_content_type) { + new_conn_state->content_protection = + DRM_MODE_CONTENT_PROTECTION_DESIRED; + return true; + } + + /* +* Ignore meaningless state changes: * - HDCP was activated since the last commit -* - attempting to set to desired while already enabled +* - Attempting to set to desired while already enabled */ - if (old_hdcp == new_hdcp || - (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && + if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) || (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { - if (old_conn_state->hdcp_content_type == - new_conn_state->hdcp_content_type) - return false; + new_conn_state->content_protection = + DRM_MODE_CONTENT_PROTECTION_ENABLED; + return false; } - return true; + /* Finally, if state changes, we need action */ + return old_hdcp != new_hdcp; } EXPORT_SYMBOL(drm_hdcp_atomic_check); -- Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v4 02/14] drm/hdcp: Avoid changing crtc state in hdcp atomic check
From: Sean Paul Instead of forcing a modeset in the hdcp atomic check, simply return true if the content protection value is changing and let the driver decide whether a modeset is required or not. Acked-by: Jani Nikula Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-3-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-3-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-3-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/drm_hdcp.c | 33 +++-- drivers/gpu/drm/i915/display/intel_atomic.c | 5 ++-- include/drm/drm_hdcp.h | 2 +- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 522326b03e66..dd8fa91c51d6 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -430,11 +430,14 @@ EXPORT_SYMBOL(drm_hdcp_update_content_protection); * @connector: drm_connector on which content protection state needs an update * * This function can be used by display drivers to perform an atomic check on the - * hdcp state elements. If hdcp state has changed, this function will set - * mode_changed on the crtc driving the connector so it can update its hardware - * to match the hdcp state. + * hdcp state elements. If hdcp state has changed in a manner which requires the + * driver to enable or disable content protection, this function will return + * true. + * + * Returns: + * true if the driver must enable/disable hdcp, false otherwise */ -void drm_hdcp_atomic_check(struct drm_connector *connector, +bool drm_hdcp_atomic_check(struct drm_connector *connector, struct drm_atomic_state *state) { struct drm_connector_state *new_conn_state, *old_conn_state; @@ -452,10 +455,12 @@ void drm_hdcp_atomic_check(struct drm_connector *connector, * If the connector is being disabled with CP enabled, mark it * desired so it's re-enabled when the connector is brought back */ - if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) + if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) { new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED; - return; + return true; + } + return false; } new_crtc_state = drm_atomic_get_new_crtc_state(state, @@ -467,9 +472,19 @@ void drm_hdcp_atomic_check(struct drm_connector *connector, */ if (drm_atomic_crtc_needs_modeset(new_crtc_state) && (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && -new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)) +new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)) { new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED; + return true; + } + + /* +* Coming back from disable or changing CRTC with DESIRED state requires +* that the driver try CP enable. +*/ + if (new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && + new_conn_state->crtc != old_conn_state->crtc) + return true; /* * Nothing to do if content type is unchanged and one of: @@ -484,9 +499,9 @@ void drm_hdcp_atomic_check(struct drm_connector *connector, new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { if (old_conn_state->hdcp_content_type == new_conn_state->hdcp_content_type) - return; + return false; } - new_crtc_state->mode_changed = true; + return true; } EXPORT_SYMBOL(drm_hdcp_atomic_check); diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index 1e306e8427ec..c7b5470c40aa 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -122,8 +122,6 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, to_intel_digital_connector_state(old_state); struct drm_crtc_state *crtc_state; - drm_hdcp_atomic_check(conn, state); - if (!new_state->crtc) return 0; @@ -139,7 +137,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || new_conn_state->base.content_type != old_conn_state->base.content_type || new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode || - !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) +
[PATCH v4 01/14] drm/hdcp: Add drm_hdcp_atomic_check()
From: Sean Paul This patch moves the hdcp atomic check from i915 to drm_hdcp so other drivers can use it. No functional changes, just cleaned up some of the code when moving it over. Acked-by: Jani Nikula Acked-by: Jani Nikula Reviewed-by: Abhinav Kumar Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-2-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-2-s...@poorly.run #v2 Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-2-s...@poorly.run #v3 Changes in v2: -None Changes in v3: -None Changes in v4: -None --- drivers/gpu/drm/drm_hdcp.c | 71 - drivers/gpu/drm/i915/display/intel_atomic.c | 4 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 47 -- drivers/gpu/drm/i915/display/intel_hdcp.h | 3 - include/drm/drm_hdcp.h | 3 + 5 files changed, 75 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index ca9b8f697202..522326b03e66 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -13,13 +13,14 @@ #include #include +#include +#include #include #include #include #include #include #include -#include #include "drm_internal.h" @@ -421,3 +422,71 @@ void drm_hdcp_update_content_protection(struct drm_connector *connector, dev->mode_config.content_protection_property); } EXPORT_SYMBOL(drm_hdcp_update_content_protection); + +/** + * drm_hdcp_atomic_check - Helper for drivers to call during connector->atomic_check + * + * @state: pointer to the atomic state being checked + * @connector: drm_connector on which content protection state needs an update + * + * This function can be used by display drivers to perform an atomic check on the + * hdcp state elements. If hdcp state has changed, this function will set + * mode_changed on the crtc driving the connector so it can update its hardware + * to match the hdcp state. + */ +void drm_hdcp_atomic_check(struct drm_connector *connector, + struct drm_atomic_state *state) +{ + struct drm_connector_state *new_conn_state, *old_conn_state; + struct drm_crtc_state *new_crtc_state; + u64 old_hdcp, new_hdcp; + + old_conn_state = drm_atomic_get_old_connector_state(state, connector); + old_hdcp = old_conn_state->content_protection; + + new_conn_state = drm_atomic_get_new_connector_state(state, connector); + new_hdcp = new_conn_state->content_protection; + + if (!new_conn_state->crtc) { + /* +* If the connector is being disabled with CP enabled, mark it +* desired so it's re-enabled when the connector is brought back +*/ + if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) + new_conn_state->content_protection = + DRM_MODE_CONTENT_PROTECTION_DESIRED; + return; + } + + new_crtc_state = drm_atomic_get_new_crtc_state(state, + new_conn_state->crtc); + /* + * Fix the HDCP uapi content protection state in case of modeset. + * FIXME: As per HDCP content protection property uapi doc, an uevent() + * need to be sent if there is transition from ENABLED->DESIRED. + */ + if (drm_atomic_crtc_needs_modeset(new_crtc_state) && + (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && +new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)) + new_conn_state->content_protection = + DRM_MODE_CONTENT_PROTECTION_DESIRED; + + /* +* Nothing to do if content type is unchanged and one of: +* - state didn't change +* - HDCP was activated since the last commit +* - attempting to set to desired while already enabled +*/ + if (old_hdcp == new_hdcp || + (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED && +new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) || + (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED && +new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) { + if (old_conn_state->hdcp_content_type == + new_conn_state->hdcp_content_type) + return; + } + + new_crtc_state->mode_changed = true; +} +EXPORT_SYMBOL(drm_hdcp_atomic_check); diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index b4e7ac51aa31..1e306e8427ec 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -32,13 +32,13 @@ #include #include #include +#include #include #include "intel_atomic.h" #include "intel_cdclk.h" #include "intel_display_types.h" #include "
[PATCH v4 00/14] drm/hdcp: Pull HDCP auth/exchange/check into helpers
From: Sean Paul Just me with another revision of HDCP support for msm. This v4 patch series is mostly a retread of v3 with the following changes: - rebased on Bjorn's displayport-controller register refactor - another change to the dt bindings to remove the compatible string added in v3 - updated review tags I'm missing reviews on the core, i915 patches, and the final patch. It would be fantastic to get some feedback on these before the set once again drifts too far from -tip and I need a painful rebase :-) Thank you to the reviewers for their feedback thus far! Please take a look, Sean Link: https://patchwork.freedesktop.org/series/94623/ #v1 Link: https://patchwork.freedesktop.org/series/94713/ #v2 Link: https://patchwork.freedesktop.org/series/94712/ #v3 Sean Paul (14): drm/hdcp: Add drm_hdcp_atomic_check() drm/hdcp: Avoid changing crtc state in hdcp atomic check drm/hdcp: Update property value on content type and user changes drm/hdcp: Expand HDCP helper library for enable/disable/check drm/i915/hdcp: Consolidate HDCP setup/state cache drm/i915/hdcp: Retain hdcp_capable return codes drm/i915/hdcp: Use HDCP helpers for i915 drm/msm/dpu_kms: Re-order dpu includes drm/msm/dpu: Remove useless checks in dpu_encoder drm/msm/dpu: Remove encoder->enable() hack drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules dt-bindings: msm/dp: Add bindings for HDCP registers arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller drm/msm: Implement HDCP 1.x using the new drm HDCP helpers .../bindings/display/msm/dp-controller.yaml |8 +- arch/arm64/boot/dts/qcom/sc7180.dtsi |8 +- drivers/gpu/drm/drm_hdcp.c| 1197 - drivers/gpu/drm/i915/display/intel_atomic.c |7 +- drivers/gpu/drm/i915/display/intel_ddi.c | 29 +- .../drm/i915/display/intel_display_debugfs.c | 11 +- .../drm/i915/display/intel_display_types.h| 58 +- drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 345 ++--- drivers/gpu/drm/i915/display/intel_dp_mst.c | 17 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 1011 +++--- drivers/gpu/drm/i915/display/intel_hdcp.h | 36 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++-- drivers/gpu/drm/msm/Makefile |1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 30 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |2 - drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |4 - drivers/gpu/drm/msm/dp/dp_debug.c | 46 +- drivers/gpu/drm/msm/dp/dp_debug.h |6 +- drivers/gpu/drm/msm/dp/dp_display.c | 48 +- drivers/gpu/drm/msm/dp/dp_display.h |5 + drivers/gpu/drm/msm/dp/dp_drm.c | 68 +- drivers/gpu/drm/msm/dp/dp_drm.h |5 + drivers/gpu/drm/msm/dp/dp_hdcp.c | 462 +++ drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 + drivers/gpu/drm/msm/dp/dp_parser.c| 20 +- drivers/gpu/drm/msm/dp/dp_parser.h|4 + drivers/gpu/drm/msm/dp/dp_reg.h | 32 +- drivers/gpu/drm/msm/msm_atomic.c | 15 + include/drm/drm_hdcp.h| 194 +++ 30 files changed, 2592 insertions(+), 1377 deletions(-) create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h -- Sean Paul, Software Engineer, Google / Chromium OS
Re: linux-next: manual merge of the char-misc tree with the drm-intel tree
Hi all, On Thu, 28 Oct 2021 18:27:53 +1100 Stephen Rothwell wrote: > > Today's linux-next merge of the char-misc tree got a conflict in: > > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > between commit: > > 5740211ea442 ("drm/i915/dmabuf: fix broken build") > > from the drm-intel tree and commit: > > 16b0314aa746 ("dma-buf: move dma-buf symbols into the DMA_BUF module > namespace") > > from the char-misc tree. > > I fixed it up (see below) and can carry the fix as necessary. This > is now fixed as far as linux-next is concerned, but any non trivial > conflicts should be mentioned to your upstream maintainer when your tree > is submitted for merging. You may also want to consider cooperating > with the maintainer of the conflicting tree to minimise any particularly > complex conflicts. > > -- > Cheers, > Stephen Rothwell > > diff --cc drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index a45d0ec2c5b6,abb854281347.. > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@@ -12,13 -13,8 +13,15 @@@ > #include "i915_gem_object.h" > #include "i915_scatterlist.h" > > +#if defined(CONFIG_X86) > +#include > +#else > +#define wbinvd_on_all_cpus() \ > +pr_warn(DRIVER_NAME ": Missing cache flush in %s\n", __func__) > +#endif > + > + MODULE_IMPORT_NS(DMA_BUF); > + > I915_SELFTEST_DECLARE(static bool force_different_devices;) > > static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf) This is now a conflict between the drm-intel tree and Linux' tree. -- Cheers, Stephen Rothwell pgpbztGTzrT4J.pgp Description: OpenPGP digital signature
Re: [Freedreno] [PATCH] drm/msm/mdp5: drop vdd regulator
On 11/3/2021 5:34 PM, Dmitry Baryshkov wrote: 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 Reviewed-by: Abhinav Kumar --- 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; }
Re: [PATCH v7 18/20] drm/mediatek: add ovl_adaptor support for MT8195
Hi, Nancy: Nancy.Lin 於 2021年10月29日 週五 下午3:52寫道: > > Add ovl_adaptor driver for MT8195. > Ovl_adaptor is an encapsulated module and designed for simplified > DRM control flow. This module is composed of 8 RDMAs, 4 MERGEs and > an ETHDR. Two RDMAs merge into one layer, so this module support 4 > layers. > > Signed-off-by: Nancy.Lin > --- > drivers/gpu/drm/mediatek/Makefile | 1 + > drivers/gpu/drm/mediatek/mtk_disp_drv.h | 16 + > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 436 ++ > drivers/gpu/drm/mediatek/mtk_drm_drv.h| 1 + > 4 files changed, 454 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > > diff --git a/drivers/gpu/drm/mediatek/Makefile > b/drivers/gpu/drm/mediatek/Makefile > index fb158a1e7f06..3abd27d7c91d 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -6,6 +6,7 @@ mediatek-drm-y := mtk_disp_aal.o \ > mtk_disp_gamma.o \ > mtk_disp_merge.o \ > mtk_disp_ovl.o \ > + mtk_disp_ovl_adaptor.o \ > mtk_disp_rdma.o \ > mtk_drm_crtc.o \ > mtk_drm_ddp_comp.o \ > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h > index 224a710bb537..cd9827402626 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h > @@ -112,6 +112,22 @@ void mtk_rdma_enable_vblank(struct device *dev, > void *vblank_cb_data); > void mtk_rdma_disable_vblank(struct device *dev); > > +int mtk_ovl_adaptor_clk_enable(struct device *dev); > +void mtk_ovl_adaptor_clk_disable(struct device *dev); > +void mtk_ovl_adaptor_config(struct device *dev, unsigned int w, > + unsigned int h, unsigned int vrefresh, > + unsigned int bpc, struct cmdq_pkt *cmdq_pkt); > +void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx, > + struct mtk_plane_state *state, > + struct cmdq_pkt *cmdq_pkt); > +void mtk_ovl_adaptor_enable_vblank(struct device *dev, > + void (*vblank_cb)(void *), > + void *vblank_cb_data); > +void mtk_ovl_adaptor_disable_vblank(struct device *dev); > +void mtk_ovl_adaptor_start(struct device *dev); > +void mtk_ovl_adaptor_stop(struct device *dev); > +unsigned int mtk_ovl_adaptor_layer_nr(struct device *dev); > + > int mtk_mdp_rdma_clk_enable(struct device *dev); > void mtk_mdp_rdma_clk_disable(struct device *dev); > void mtk_mdp_rdma_start(struct device *dev, struct cmdq_pkt *cmdq_pkt); > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > new file mode 100644 > index ..148322597fa8 > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > @@ -0,0 +1,436 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2021 MediaTek Inc. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "mtk_disp_drv.h" > +#include "mtk_drm_crtc.h" > +#include "mtk_drm_ddp_comp.h" > +#include "mtk_drm_drv.h" > +#include "mtk_ethdr.h" > + > +#define MTK_OVL_ADAPTOR_RDMA_MAX_WIDTH 1920 > +#define MTK_OVL_ADAPTOR_LAYER_NUM 4 > + > +enum mtk_ovl_adaptor_comp_type { > + OVL_ADAPTOR_TYPE_RDMA = 0, > + OVL_ADAPTOR_TYPE_MERGE, > + OVL_ADAPTOR_TYPE_ETHDR, > + OVL_ADAPTOR_TYPE_NUM, > +}; > + > +enum mtk_ovl_adaptor_comp_id { > + OVL_ADAPTOR_MDP_RDMA0, > + OVL_ADAPTOR_MDP_RDMA1, > + OVL_ADAPTOR_MDP_RDMA2, > + OVL_ADAPTOR_MDP_RDMA3, > + OVL_ADAPTOR_MDP_RDMA4, > + OVL_ADAPTOR_MDP_RDMA5, > + OVL_ADAPTOR_MDP_RDMA6, > + OVL_ADAPTOR_MDP_RDMA7, > + OVL_ADAPTOR_MERGE0, > + OVL_ADAPTOR_MERGE1, > + OVL_ADAPTOR_MERGE2, > + OVL_ADAPTOR_MERGE3, > + OVL_ADAPTOR_ETHDR0, > + OVL_ADAPTOR_ID_MAX > +}; > + > +struct ovl_adaptor_comp_match { > + enum mtk_ovl_adaptor_comp_type type; > + int alias_id; > +}; > + > +struct mtk_disp_ovl_adaptor { > + struct device *ovl_adaptor_comp[OVL_ADAPTOR_ID_MAX]; > + struct device *mmsys_dev; > +}; > + > +static const char * const private_comp_stem[OVL_ADAPTOR_TYPE_NUM] = { > + [OVL_ADAPTOR_TYPE_RDMA] = "vdo1_rdma", > + [OVL_ADAPTOR_TYPE_MERGE]= "merge", > + [OVL_ADAPTOR_TYPE_ETHDR]= "ethdr", > +}; > + > +static const struct ovl_adaptor_comp_match comp_matches[OVL_ADAPTOR_ID_MAX] > = { > + [OVL_ADAPTOR_MDP_RDMA0] = { OVL_ADAPTOR_TYPE_RDMA, 0 }, > + [OVL_ADAPTOR_MDP_RDMA1] = { OVL_ADAPTOR_TYPE_RDMA, 1 }, > + [OVL_ADAPTOR_MDP_RDMA2] = { OVL_ADAPTOR_TYPE_RDMA, 2 }, > + [OV
Re: [PATCH] drm/msm/devfreq: Fix OPP refcnt leak
Hi, On Thu, Nov 4, 2021 at 3:23 PM Rob Clark wrote: > > From: Rob Clark > > Reported-by: Douglas Anderson > Fixes: 9bc95570175a ("drm/msm: Devfreq tuning") > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Douglas Anderson
Re: [PATCH v7 17/20] drm/mediatek: add mediatek-drm plane color encoding info
Hi, Nancy: Nancy.Lin 於 2021年10月29日 週五 下午3:52寫道: > > Add plane color encoding information for color space conversion. > It's a preparation for adding support for mt8195 ovl_adaptor mdp_rdma > csc control. Reviewed-by: Chun-Kuang Hu > > Signed-off-by: Nancy.Lin > --- > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 1 + > drivers/gpu/drm/mediatek/mtk_drm_plane.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 734a1fb052df..81bd5d6e8df5 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -137,6 +137,7 @@ static void mtk_plane_update_new_state(struct > drm_plane_state *new_state, > mtk_plane_state->pending.width = drm_rect_width(&new_state->dst); > mtk_plane_state->pending.height = drm_rect_height(&new_state->dst); > mtk_plane_state->pending.rotation = new_state->rotation; > + mtk_plane_state->pending.color_encoding = new_state->color_encoding; > } > > static void mtk_plane_atomic_async_update(struct drm_plane *plane, > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h > b/drivers/gpu/drm/mediatek/mtk_drm_plane.h > index d454bece9535..2d5ec66e3df1 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h > @@ -24,6 +24,7 @@ struct mtk_plane_pending_state { > booldirty; > boolasync_dirty; > boolasync_config; > + enum drm_color_encoding color_encoding; > }; > > struct mtk_plane_state { > -- > 2.18.0 >
Re: [PATCH v7 16/20] drm/mediatek: add ETHDR support for MT8195
Hi, Nancy: Nancy.Lin 於 2021年10月29日 週五 下午3:52寫道: > > ETHDR is a part of ovl_adaptor. > ETHDR is designed for HDR video and graphics conversion in the external > display path. It handles multiple HDR input types and performs tone > mapping, color space/color format conversion, and then combine > different layers, output the required HDR or SDR signal to the > subsequent display path. > > Signed-off-by: Nancy.Lin > --- > drivers/gpu/drm/mediatek/Makefile| 1 + > drivers/gpu/drm/mediatek/mtk_ethdr.c | 399 +++ > drivers/gpu/drm/mediatek/mtk_ethdr.h | 25 ++ > 3 files changed, 425 insertions(+) > create mode 100644 drivers/gpu/drm/mediatek/mtk_ethdr.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_ethdr.h > > diff --git a/drivers/gpu/drm/mediatek/Makefile > b/drivers/gpu/drm/mediatek/Makefile > index 6e604a933ed0..fb158a1e7f06 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -14,6 +14,7 @@ mediatek-drm-y := mtk_disp_aal.o \ > mtk_drm_plane.o \ > mtk_dsi.o \ > mtk_dpi.o \ > + mtk_ethdr.o \ > mtk_mdp_rdma.o > > obj-$(CONFIG_DRM_MEDIATEK) += mediatek-drm.o > diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c > b/drivers/gpu/drm/mediatek/mtk_ethdr.c > new file mode 100644 > index ..86b584e9cf2c > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c > @@ -0,0 +1,399 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2021 MediaTek Inc. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "mtk_drm_crtc.h" > +#include "mtk_drm_ddp_comp.h" > +#include "mtk_drm_drv.h" > +#include "mtk_ethdr.h" > + > +#define MIX_INTEN 0x4 > +#define MIX_FME_CPL_INTEN BIT(1) > +#define MIX_INTSTA 0x8 > +#define MIX_EN 0xc > +#define MIX_RST0x14 > +#define MIX_ROI_SIZE 0x18 > +#define MIX_DATAPATH_CON 0x1c > +#define OUTPUT_NO_RND BIT(3) > +#define SOURCE_RGB_SEL BIT(7) > +#define BACKGROUND_RELAY (4 << 9) > +#define MIX_ROI_BGCLR 0x20 > +#define BGCLR_BLACK0xff00 > +#define MIX_SRC_CON0x24 > +#define MIX_SRC_L0_EN BIT(0) > +#define MIX_L_SRC_CON(n) (0x28 + 0x18 * (n)) > +#define NON_PREMULTI_SOURCE(2 << 12) > +#define MIX_L_SRC_SIZE(n) (0x30 + 0x18 * (n)) > +#define MIX_L_SRC_OFFSET(n)(0x34 + 0x18 * (n)) > +#define MIX_FUNC_DCM0 0x120 > +#define MIX_FUNC_DCM1 0x124 > +#define MIX_FUNC_DCM_ENABLE0x > + > +#define HDR_VDO_FE_0804_HDR_DM_FE 0x804 > +#define HDR_VDO_FE_0804_BYPASS_ALL 0xfd > +#define HDR_GFX_FE_0204_GFX_HDR_FE 0x204 > +#define HDR_GFX_FE_0204_BYPASS_ALL 0xfd > +#define HDR_VDO_BE_0204_VDO_DM_BE 0x204 > +#define HDR_VDO_BE_0204_BYPASS_ALL 0x7e > + > +#define MIXER_INX_MODE_BYPASS 0 > +#define MIXER_INX_MODE_EVEN_EXTEND 1 > +#define DEFAULT_9BIT_ALPHA 0x100 > +#defineMIXER_ALPHA_AEN BIT(8) > +#defineMIXER_ALPHA 0xff > +#define ETHDR_CLK_NUM 13 > + > +enum mtk_ethdr_comp_id { > + ETHDR_MIXER, > + ETHDR_VDO_FE0, > + ETHDR_VDO_FE1, > + ETHDR_GFX_FE0, > + ETHDR_GFX_FE1, > + ETHDR_VDO_BE, > + ETHDR_ADL_DS, > + ETHDR_ID_MAX > +}; > + > +struct mtk_ethdr_comp { > + struct device *dev; > + void __iomem*regs; > + struct cmdq_client_reg cmdq_base; > +}; > + > +struct mtk_ethdr { > + struct mtk_ethdr_comp ethdr_comp[ETHDR_ID_MAX]; > + struct clk_bulk_dataethdr_clk[ETHDR_CLK_NUM]; > + struct device *mmsys_dev; > + spinlock_t lock; /* protects vblank_cb and > vblank_cb_data */ > + void(*vblank_cb)(void *data); > + void*vblank_cb_data; > + int irq; > +}; > + > +static const char * const ethdr_comp_str[] = { > + "ETHDR_MIXER", > + "ETHDR_VDO_FE0", > + "ETHDR_VDO_FE1", > + "ETHDR_GFX_FE0", > + "ETHDR_GFX_FE1", > + "ETHDR_VDO_BE", > + "ETHDR_ADL_DS", > + "ETHDR_ID_MAX" > +}; > + > +static const char * const ethdr_clk_str[] = { > + "ethdr_top", > + "mixer", > + "vdo_fe0", > + "vdo_fe1", > + "gfx_fe0", > + "gfx_fe1", > + "vdo_be", > + "adl_ds", > + "vdo_fe0_async", > + "vdo_fe1_async", > + "gfx_fe0_
[PATCH] drm/msm/devfreq: Fix OPP refcnt leak
From: Rob Clark Reported-by: Douglas Anderson Fixes: 9bc95570175a ("drm/msm: Devfreq tuning") Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_gpu_devfreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c index d32b729b4616..9bf8600b6eea 100644 --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c @@ -20,8 +20,6 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq, struct msm_gpu *gpu = dev_to_gpu(dev); struct dev_pm_opp *opp; - opp = devfreq_recommended_opp(dev, freq, flags); - /* * If the GPU is idle, devfreq is not aware, so just ignore * it's requests @@ -31,6 +29,8 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq, return 0; } + opp = devfreq_recommended_opp(dev, freq, flags); + if (IS_ERR(opp)) return PTR_ERR(opp); -- 2.31.1
[PATCH] drm/virtio: Fix NULL dereference error in virtio_gpu_poll
When virgl is not enabled, vfpriv pointer would not be allocated. Therefore, check for a valid value before dereferencing. Reported-by: Christian Zigotzky Cc: Gurchetan Singh Cc: Gerd Hoffmann Signed-off-by: Vivek Kasireddy --- drivers/gpu/drm/virtio/virtgpu_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index 749db18dcfa2..d86e1ad4a972 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -163,10 +163,11 @@ static __poll_t virtio_gpu_poll(struct file *filp, struct drm_file *drm_file = filp->private_data; struct virtio_gpu_fpriv *vfpriv = drm_file->driver_priv; struct drm_device *dev = drm_file->minor->dev; + struct virtio_gpu_device *vgdev = dev->dev_private; struct drm_pending_event *e = NULL; __poll_t mask = 0; - if (!vfpriv->ring_idx_mask) + if (!vgdev->has_virgl_3d || !vfpriv || !vfpriv->ring_idx_mask) return drm_poll(filp, wait); poll_wait(filp, &drm_file->event_wait, wait); -- 2.31.1
Re: [PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
On Thu, Nov 4, 2021 at 4:03 PM Mark Yacoub wrote: > > From: Mark Yacoub > > [Why] > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma > or Degamma props in the new CRTC state, allowing any invalid size to > be passed on. > 2. Each driver has its own LUT size, which could also be different for > legacy users. > > [How] > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes > assigned by the driver when it's initializing its color and CTM > management. > 2. Create drm_atomic_helper_check_crtc which is called by > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that > they match the sizes in the new CRTC state. > 3. As the LUT size check now happens in drm_atomic_helper_check, remove > the lut check in intel_color.c > > Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK > Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL) > > v3: > 1. Check for lut pointer inside drm_check_lut_size. > > v2: > 1. Remove the rename to a parent commit. > 2. Create a drm drm_check_lut_size instead of intel only function. > > v1: > 1. Fix typos > 2. Remove the LUT size check from intel driver > 3. Rename old LUT check to indicate it's a channel change > > Signed-off-by: Mark Yacoub > --- > drivers/gpu/drm/drm_atomic_helper.c| 52 ++ > drivers/gpu/drm/drm_color_mgmt.c | 19 > drivers/gpu/drm/i915/display/intel_color.c | 32 ++--- > include/drm/drm_atomic_helper.h| 1 + > include/drm/drm_color_mgmt.h | 3 ++ > include/drm/drm_crtc.h | 11 + > 6 files changed, 99 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index bc3487964fb5e..548e5d8221fb4 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_helper_check_planes); > > +/** > + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes > + * @state: the driver state object > + * > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new > + * state holds them. > + * > + * RETURNS: > + * Zero for success or -errno > + */ > +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state) > +{ > + struct drm_crtc *crtc; > + struct drm_crtc_state *new_crtc_state; > + int i; > + > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { > + if (!new_crtc_state->color_mgmt_changed) > + continue; > + > + if (drm_check_lut_size(new_crtc_state->gamma_lut, > + crtc->gamma_lut_size) || > + drm_check_lut_size(new_crtc_state->gamma_lut, > + crtc->gamma_size)) { > + drm_dbg_state( > + state->dev, > + "Invalid Gamma LUT size. Expected %u/%u, got > %u.\n", > + crtc->gamma_lut_size, crtc->gamma_size, > + > drm_color_lut_size(new_crtc_state->gamma_lut)); > + return -EINVAL; > + } > + > + if (drm_check_lut_size(new_crtc_state->degamma_lut, > + crtc->degamma_lut_size)) { > + drm_dbg_state( > + state->dev, > + "Invalid Degamma LUT size. Expected %u, got > %u.\n", > + crtc->degamma_lut_size, > + drm_color_lut_size( > + new_crtc_state->degamma_lut)); > + return -EINVAL; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs); > + > /** > * drm_atomic_helper_check - validate state object > * @dev: DRM device > @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev, > if (ret) > return ret; > > + ret = drm_atomic_helper_check_crtcs(state); > + if (ret) > + return ret; > + > if (state->legacy_cursor_update) > state->async_update = !drm_atomic_helper_async_check(dev, > state); > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c > b/drivers/gpu/drm/drm_color_mgmt.c > index 16a07f84948f3..c85094223b535 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > struct drm_mode_config *config = &dev->mode_config; > > if (degamma_lut_size) { > + crtc->degamma_lut_size = degamma_lut_size; > drm_object_attach_property(&crtc->base, >
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Jani, On 11/4/21 20:57, Jani Nikula wrote: > On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: >> +/** >> + * 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. >> + * >> + * Return: 0 on success or a negative error code on failure. >> + */ >> +int drm_drv_enabled(const struct drm_driver *driver) >> +{ >> +if (vgacon_text_force()) { >> +DRM_INFO("%s driver is disabled\n", driver->name); >> +return -ENODEV; >> +} >> + >> +return 0; >> +} >> +EXPORT_SYMBOL(drm_drv_enabled); > > The name implies a bool return, but it's not. > > if (drm_drv_enabled(...)) { > /* surprise, it's disabled! */ > } > It used to return a bool in v2 but Thomas suggested an int instead to have consistency on the errno code that was returned by the callers. I should probably name that function differently to avoid confusion. But I think you are correct and this change is caused too much churn for not that much benefit, specially since is unclear that there might be another condition to prevent a DRM driver to load besides nomodeset. I'll just drop this patch and post only #2 but making drivers to test using the drm_check_modeset() function (which doesn't have a name that implies a bool return). > > BR, > Jani. > > > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v5 3/3] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
From: Mark Yacoub [Why] drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT sizes. There is no need to check it within amdgpu_dm_atomic_check. [How] Remove the local call to verify LUT sizes and use DRM Core function instead. Tested on ChromeOS Zork. v1: Remove amdgpu_dm_verify_lut_sizes everywhere. Signed-off-by: Mark Yacoub Reviewed-by: Sean Paul --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 - .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 35 --- 3 files changed, 4 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index f74663b6b046e..47f8de1cfc3a5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10244,6 +10244,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, } } #endif + ret = drm_atomic_helper_check_crtcs(state); + if (ret) + return ret; + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { dm_old_crtc_state = to_dm_crtc_state(old_crtc_state); @@ -10253,10 +10257,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, dm_old_crtc_state->dsc_force_changed == false) continue; - ret = amdgpu_dm_verify_lut_sizes(new_crtc_state); - if (ret) - goto fail; - if (!new_crtc_state->enable) continue; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index fcb9c4a629c32..22730e5542092 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -617,7 +617,6 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device *dev); #define MAX_COLOR_LEGACY_LUT_ENTRIES 256 void amdgpu_dm_init_color_mod(void); -int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state); int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc); int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc, struct dc_plane_state *dc_plane_state); diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c index a022e5bb30a5c..319f8a8a89835 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c @@ -284,37 +284,6 @@ static int __set_input_tf(struct dc_transfer_func *func, return res ? 0 : -ENOMEM; } -/** - * Verifies that the Degamma and Gamma LUTs attached to the |crtc_state| are of - * the expected size. - * Returns 0 on success. - */ -int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state) -{ - const struct drm_color_lut *lut = NULL; - uint32_t size = 0; - - lut = __extract_blob_lut(crtc_state->degamma_lut, &size); - if (lut && size != MAX_COLOR_LUT_ENTRIES) { - DRM_DEBUG_DRIVER( - "Invalid Degamma LUT size. Should be %u but got %u.\n", - MAX_COLOR_LUT_ENTRIES, size); - return -EINVAL; - } - - lut = __extract_blob_lut(crtc_state->gamma_lut, &size); - if (lut && size != MAX_COLOR_LUT_ENTRIES && - size != MAX_COLOR_LEGACY_LUT_ENTRIES) { - DRM_DEBUG_DRIVER( - "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n", - MAX_COLOR_LUT_ENTRIES, MAX_COLOR_LEGACY_LUT_ENTRIES, - size); - return -EINVAL; - } - - return 0; -} - /** * amdgpu_dm_update_crtc_color_mgmt: Maps DRM color management to DC stream. * @crtc: amdgpu_dm crtc state @@ -348,10 +317,6 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc) bool is_legacy; int r; - r = amdgpu_dm_verify_lut_sizes(&crtc->base); - if (r) - return r; - degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, °amma_size); regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, ®amma_size); -- 2.34.0.rc0.344.g81b53c2807-goog
[PATCH v5 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
From: Mark Yacoub [Why] 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma or Degamma props in the new CRTC state, allowing any invalid size to be passed on. 2. Each driver has its own LUT size, which could also be different for legacy users. [How] 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes assigned by the driver when it's initializing its color and CTM management. 2. Create drm_atomic_helper_check_crtc which is called by drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that they match the sizes in the new CRTC state. 3. As the LUT size check now happens in drm_atomic_helper_check, remove the lut check in intel_color.c Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL) v3: 1. Check for lut pointer inside drm_check_lut_size. v2: 1. Remove the rename to a parent commit. 2. Create a drm drm_check_lut_size instead of intel only function. v1: 1. Fix typos 2. Remove the LUT size check from intel driver 3. Rename old LUT check to indicate it's a channel change Signed-off-by: Mark Yacoub --- drivers/gpu/drm/drm_atomic_helper.c| 52 ++ drivers/gpu/drm/drm_color_mgmt.c | 19 drivers/gpu/drm/i915/display/intel_color.c | 32 ++--- include/drm/drm_atomic_helper.h| 1 + include/drm/drm_color_mgmt.h | 3 ++ include/drm/drm_crtc.h | 11 + 6 files changed, 99 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc3487964fb5e..548e5d8221fb4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_planes); +/** + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes + * @state: the driver state object + * + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new + * state holds them. + * + * RETURNS: + * Zero for success or -errno + */ +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *new_crtc_state; + int i; + + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { + if (!new_crtc_state->color_mgmt_changed) + continue; + + if (drm_check_lut_size(new_crtc_state->gamma_lut, + crtc->gamma_lut_size) || + drm_check_lut_size(new_crtc_state->gamma_lut, + crtc->gamma_size)) { + drm_dbg_state( + state->dev, + "Invalid Gamma LUT size. Expected %u/%u, got %u.\n", + crtc->gamma_lut_size, crtc->gamma_size, + drm_color_lut_size(new_crtc_state->gamma_lut)); + return -EINVAL; + } + + if (drm_check_lut_size(new_crtc_state->degamma_lut, + crtc->degamma_lut_size)) { + drm_dbg_state( + state->dev, + "Invalid Degamma LUT size. Expected %u, got %u.\n", + crtc->degamma_lut_size, + drm_color_lut_size( + new_crtc_state->degamma_lut)); + return -EINVAL; + } + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs); + /** * drm_atomic_helper_check - validate state object * @dev: DRM device @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret; + ret = drm_atomic_helper_check_crtcs(state); + if (ret) + return ret; + if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(dev, state); diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 16a07f84948f3..c85094223b535 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, struct drm_mode_config *config = &dev->mode_config; if (degamma_lut_size) { + crtc->degamma_lut_size = degamma_lut_size; drm_object_attach_property(&crtc->base, config->degamma_lut_property, 0); drm_object_attach_property(&crtc->base, @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, config->ctm_property, 0); if (gamma_lut_size) { + crtc->ga
[PATCH v5 1/3] drm: Move drm_color_lut_check implementation internal to intel_color
From: Mark Yacoub [Why] The tests of LUT_EQUAL_CHANNELS and LUT_NON_DECREASING are currently unique to i915 driver. Freeing up the function name for the more generic LUT checks to folllow Tested on Eldrid ChromeOS (TGL). v2: 1. Convert the enum to #define. 2. Add INTEL_COLOR_ prefix. v1: Stuff the test function from DRM to intel driver. Signed-off-by: Mark Yacoub --- drivers/gpu/drm/drm_color_mgmt.c | 43 -- drivers/gpu/drm/i915/display/intel_color.c | 43 +++--- drivers/gpu/drm/i915/display/intel_color.h | 16 drivers/gpu/drm/i915/i915_pci.c| 28 -- include/drm/drm_color_mgmt.h | 27 -- 5 files changed, 71 insertions(+), 86 deletions(-) diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..16a07f84948f3 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -583,46 +583,3 @@ int drm_plane_create_color_properties(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_color_properties); - -/** - * drm_color_lut_check - check validity of lookup table - * @lut: property blob containing LUT to check - * @tests: bitmask of tests to run - * - * Helper to check whether a userspace-provided lookup table is valid and - * satisfies hardware requirements. Drivers pass a bitmask indicating which of - * the tests in &drm_color_lut_tests should be performed. - * - * Returns 0 on success, -EINVAL on failure. - */ -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests) -{ - const struct drm_color_lut *entry; - int i; - - if (!lut || !tests) - return 0; - - entry = lut->data; - for (i = 0; i < drm_color_lut_size(lut); i++) { - if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { - if (entry[i].red != entry[i].blue || - entry[i].red != entry[i].green) { - DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n"); - return -EINVAL; - } - } - - if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { - if (entry[i].red < entry[i - 1].red || - entry[i].green < entry[i - 1].green || - entry[i].blue < entry[i - 1].blue) { - DRM_DEBUG_KMS("LUT entries must never decrease.\n"); - return -EINVAL; - } - } - } - - return 0; -} -EXPORT_SYMBOL(drm_color_lut_check); diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index dab892d2251ba..1469871d21ff9 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1279,13 +1279,46 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) return 0; } +static int test_luts(const struct drm_property_blob *lut, u32 tests) +{ + const struct drm_color_lut *entry; + int i; + + if (!lut || !tests) + return 0; + + entry = lut->data; + for (i = 0; i < drm_color_lut_size(lut); i++) { + if (tests & INTEL_COLOR_LUT_EQUAL_CHANNELS) { + if (entry[i].red != entry[i].blue || + entry[i].red != entry[i].green) { + DRM_DEBUG_KMS( + "All LUT entries must have equal r/g/b\n"); + return -EINVAL; + } + } + + if (i > 0 && tests & INTEL_COLOR_LUT_NON_DECREASING) { + if (entry[i].red < entry[i - 1].red || + entry[i].green < entry[i - 1].green || + entry[i].blue < entry[i - 1].blue) { + DRM_DEBUG_KMS( + "LUT entries must never decrease.\n"); + return -EINVAL; + } + } + } + + return 0; +} + static int check_luts(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut; const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut; int gamma_length, degamma_length; - u32 gamma_tests, degamma_tests; + u32 gamma_channels_tests, degamma_channels_tests; /* Always allow legacy gamma LUT with no further checking. */ if (crtc_state_is_legacy_gamma(crtc_state)) @@ -1300,15 +1333,15 @@ static int check_luts(const struct intel_crtc_state *crtc_state) degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_siz
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: > +/** > + * 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. > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +int drm_drv_enabled(const struct drm_driver *driver) > +{ > + if (vgacon_text_force()) { > + DRM_INFO("%s driver is disabled\n", driver->name); > + return -ENODEV; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_drv_enabled); The name implies a bool return, but it's not. if (drm_drv_enabled(...)) { /* surprise, it's disabled! */ } BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Thu, 04 Nov 2021, Sam Ravnborg wrote: > Hi Javier, > >> >> >>> >> >>> -if (vgacon_text_force() && i915_modparams.modeset == -1) >> >>> +ret = drm_drv_enabled(&driver); >> >> >> >> You pass the local driver variable here - which looks wrong as this is >> >> not the same as the driver variable declared in another file. >> > >> >> Yes, Jani mentioned it already. I got confused and thought that the driver >> variable was also defined in the same compilation unit... >> >> Maybe I could squash the following change? >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index b18a250e5d2e..b8f399b76363 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -89,7 +89,7 @@ >> #include "intel_region_ttm.h" >> #include "vlv_suspend.h" >> >> -static const struct drm_driver driver; >> +const struct drm_driver driver; > No, variables with such a generic name will clash. > > Just add a > const drm_driver * i915_drm_driver(void) > { > return &driver; > } > > And then use this function to access the drm_driver variable. Agreed on the general principle of exposing interfaces over data. But... why? I'm still at a loss what problem this solves. We pass &driver to exactly one place, devm_drm_dev_alloc(), and it's neatly hidden away. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Javier, > > >>> > >>> - if (vgacon_text_force() && i915_modparams.modeset == -1) > >>> + ret = drm_drv_enabled(&driver); > >> > >> You pass the local driver variable here - which looks wrong as this is > >> not the same as the driver variable declared in another file. > > > > Yes, Jani mentioned it already. I got confused and thought that the driver > variable was also defined in the same compilation unit... > > Maybe I could squash the following change? > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index b18a250e5d2e..b8f399b76363 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -89,7 +89,7 @@ > #include "intel_region_ttm.h" > #include "vlv_suspend.h" > > -static const struct drm_driver driver; > +const struct drm_driver driver; No, variables with such a generic name will clash. Just add a const drm_driver * i915_drm_driver(void) { return &driver; } And then use this function to access the drm_driver variable. Sam
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Sam, On 11/4/21 18:57, Jani Nikula wrote: > On Thu, 04 Nov 2021, Sam Ravnborg wrote: >> Hi Javier, >> >> On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote: >>> Some DRM drivers check the vgacon_text_force() function return value as an >>> indication on whether they should be allowed to be enabled or not. >>> >>> This function returns true if the nomodeset kernel command line parameter >>> was set. But there may be other conditions besides this to determine if a >>> driver should be enabled. >>> >>> Let's add a drm_drv_enabled() helper function to encapsulate that logic so >>> can be later extended if needed, without having to modify all the drivers. >>> >>> Also, while being there do some cleanup. The vgacon_text_force() function >>> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it. >>> >>> Suggested-by: Thomas Zimmermann >>> Signed-off-by: Javier Martinez Canillas >>> --- >>> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index 8214a0b1ab7f..3fb567d62881 100644 >>> --- a/drivers/gpu/drm/drm_drv.c >>> +++ b/drivers/gpu/drm/drm_drv.c >>> @@ -975,6 +975,26 @@ 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. >>> + * >>> + * Return: 0 on success or a negative error code on failure. >>> + */ >>> +int drm_drv_enabled(const struct drm_driver *driver) >>> +{ >>> + if (vgacon_text_force()) { >>> + DRM_INFO("%s driver is disabled\n", driver->name); >> >> DRM_INFO is deprecated, please do not use it in new code. >> Also other users had an error message and not just info - is info >> enough? >> Thanks, I didn't know that. Right, they had an error but I do wonder if that was correct though. After all isn't an error but an explicit disable due "nomodeset" being set in the kernel command line. [snip] >>> >>> - if (vgacon_text_force() && i915_modparams.modeset == -1) >>> + ret = drm_drv_enabled(&driver); >> >> You pass the local driver variable here - which looks wrong as this is >> not the same as the driver variable declared in another file. > Yes, Jani mentioned it already. I got confused and thought that the driver variable was also defined in the same compilation unit... Maybe I could squash the following change? diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b18a250e5d2e..b8f399b76363 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -89,7 +89,7 @@ #include "intel_region_ttm.h" #include "vlv_suspend.h" -static const struct drm_driver driver; +const struct drm_driver driver; static int i915_get_bridge_dev(struct drm_i915_private *dev_priv) { @@ -1777,7 +1777,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = { DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl, DRM_RENDER_ALLOW), }; -static const struct drm_driver driver = { +const struct drm_driver driver = { /* Don't use MTRRs here; the Xserver or userspace app should * deal with them for Intel hardware. */ diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c index c890c1ca20c4..88f770920324 100644 --- a/drivers/gpu/drm/i915/i915_module.c +++ b/drivers/gpu/drm/i915/i915_module.c @@ -16,7 +16,7 @@ #include "i915_selftest.h" #include "i915_vma.h" -static const struct drm_driver driver; +extern const struct drm_driver driver; static int i915_check_nomodeset(void) { That should work and I actually got a laptop with an i915 and tested the change both when CONFIG_DRM_I915=m and CONFIG_DRM_I915=y is set. Another option is to declare it in i915_drv.h and not make the definition static. > Indeed. > >> Maybe move the check to new function you can add to init_funcs, >> and locate the new function in i915_drv - so it has access to driver? > > We don't really want that, though. This check is pretty much as early as > it can be, and there's a ton of useless initialization that would happen > if we waited until drm_driver is available. > Agreed. > From my POV, drm_drv_enabled() is a solution that creates a worse > problem for us than it solves. > I don't have a strong opinion on this. I could just do patch #2 without adding a level of indirection through drm_drv_enabled(). But Thomas and Daniel Vetter suggested that we should do this change before. That is, the drivers could just check if should be enabled by calling to the drm_check_modeset() function directly if people agree that encapsulating that logic in a drm_drv_enabled() is not needed. > > BR, > Jani. > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v6] 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 V5 -> V6: Fix duty cycle rounding and set period outside conditional loop --- 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..8f893cf0cfde 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); + } else { + pwm_get_state(lp->pwm, &state); } - pwm_config(lp->pwm, duty, period); - if (duty) - pwm_enable(lp->pwm); - else - pwm_disable(lp->pwm); + state.period = lp->pdata->period_ns; + state.duty_cycle = div_u64(br * state.period, 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 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Thu, 04 Nov 2021, Sam Ravnborg wrote: > Hi Javier, > > On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote: >> Some DRM drivers check the vgacon_text_force() function return value as an >> indication on whether they should be allowed to be enabled or not. >> >> This function returns true if the nomodeset kernel command line parameter >> was set. But there may be other conditions besides this to determine if a >> driver should be enabled. >> >> Let's add a drm_drv_enabled() helper function to encapsulate that logic so >> can be later extended if needed, without having to modify all the drivers. >> >> Also, while being there do some cleanup. The vgacon_text_force() function >> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it. >> >> Suggested-by: Thomas Zimmermann >> Signed-off-by: Javier Martinez Canillas >> --- >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index 8214a0b1ab7f..3fb567d62881 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -975,6 +975,26 @@ 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. >> + * >> + * Return: 0 on success or a negative error code on failure. >> + */ >> +int drm_drv_enabled(const struct drm_driver *driver) >> +{ >> +if (vgacon_text_force()) { >> +DRM_INFO("%s driver is disabled\n", driver->name); > > DRM_INFO is deprecated, please do not use it in new code. > Also other users had an error message and not just info - is info > enough? > > >> +return -ENODEV; >> +} >> + >> +return 0; >> +} >> +EXPORT_SYMBOL(drm_drv_enabled); >> + >> /* >> * DRM Core >> * The DRM core module initializes all global DRM objects and makes them >> diff --git a/drivers/gpu/drm/i915/i915_module.c >> b/drivers/gpu/drm/i915/i915_module.c >> index ab2295dd4500..45cb3e540eff 100644 >> --- a/drivers/gpu/drm/i915/i915_module.c >> +++ b/drivers/gpu/drm/i915/i915_module.c >> @@ -18,9 +18,12 @@ >> #include "i915_selftest.h" >> #include "i915_vma.h" >> >> +static const struct drm_driver driver; > Hmmm... > >> + >> static int i915_check_nomodeset(void) >> { >> bool use_kms = true; >> +int ret; >> >> /* >> * Enable KMS by default, unless explicitly overriden by >> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void) >> if (i915_modparams.modeset == 0) >> use_kms = false; >> >> -if (vgacon_text_force() && i915_modparams.modeset == -1) >> +ret = drm_drv_enabled(&driver); > > You pass the local driver variable here - which looks wrong as this is > not the same as the driver variable declared in another file. Indeed. > Maybe move the check to new function you can add to init_funcs, > and locate the new function in i915_drv - so it has access to driver? We don't really want that, though. This check is pretty much as early as it can be, and there's a ton of useless initialization that would happen if we waited until drm_driver is available. >From my POV, drm_drv_enabled() is a solution that creates a worse problem for us than it solves. BR, Jani. > > > Sam -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v4 1/3] drm: Move drm_color_lut_check implementation internal to intel_color
On Thu, 04 Nov 2021, Mark Yacoub wrote: > From: Mark Yacoub > > [Why] > The tests of LUT_EQUAL_CHANNELS and LUT_NON_DECREASING are currently > unique to i915 driver. > Freeing up the function name for the more generic LUT checks to folllow > > Tested on Eldrid ChromeOS (TGL). > > v1: > Stuff the test function from DRM to intel driver. > > Signed-off-by: Mark Yacoub > --- > drivers/gpu/drm/drm_color_mgmt.c | 43 -- > drivers/gpu/drm/i915/display/intel_color.c | 43 +++--- > drivers/gpu/drm/i915/display/intel_color.h | 27 ++ > drivers/gpu/drm/i915/i915_pci.c| 27 -- > include/drm/drm_color_mgmt.h | 27 -- > 5 files changed, 81 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c > b/drivers/gpu/drm/drm_color_mgmt.c > index bb14f488c8f6c..16a07f84948f3 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -583,46 +583,3 @@ int drm_plane_create_color_properties(struct drm_plane > *plane, > return 0; > } > EXPORT_SYMBOL(drm_plane_create_color_properties); > - > -/** > - * drm_color_lut_check - check validity of lookup table > - * @lut: property blob containing LUT to check > - * @tests: bitmask of tests to run > - * > - * Helper to check whether a userspace-provided lookup table is valid and > - * satisfies hardware requirements. Drivers pass a bitmask indicating which > of > - * the tests in &drm_color_lut_tests should be performed. > - * > - * Returns 0 on success, -EINVAL on failure. > - */ > -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests) > -{ > - const struct drm_color_lut *entry; > - int i; > - > - if (!lut || !tests) > - return 0; > - > - entry = lut->data; > - for (i = 0; i < drm_color_lut_size(lut); i++) { > - if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { > - if (entry[i].red != entry[i].blue || > - entry[i].red != entry[i].green) { > - DRM_DEBUG_KMS("All LUT entries must have equal > r/g/b\n"); > - return -EINVAL; > - } > - } > - > - if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { > - if (entry[i].red < entry[i - 1].red || > - entry[i].green < entry[i - 1].green || > - entry[i].blue < entry[i - 1].blue) { > - DRM_DEBUG_KMS("LUT entries must never > decrease.\n"); > - return -EINVAL; > - } > - } > - } > - > - return 0; > -} > -EXPORT_SYMBOL(drm_color_lut_check); > diff --git a/drivers/gpu/drm/i915/display/intel_color.c > b/drivers/gpu/drm/i915/display/intel_color.c > index dab892d2251ba..bde98a155c9f3 100644 > --- a/drivers/gpu/drm/i915/display/intel_color.c > +++ b/drivers/gpu/drm/i915/display/intel_color.c > @@ -1279,13 +1279,46 @@ static int check_lut_size(const struct > drm_property_blob *lut, int expected) > return 0; > } > > +static int test_luts(const struct drm_property_blob *lut, u32 tests) > +{ > + const struct drm_color_lut *entry; > + int i; > + > + if (!lut || !tests) > + return 0; > + > + entry = lut->data; > + for (i = 0; i < drm_color_lut_size(lut); i++) { > + if (tests & LUT_EQUAL_CHANNELS) { > + if (entry[i].red != entry[i].blue || > + entry[i].red != entry[i].green) { > + DRM_DEBUG_KMS( > + "All LUT entries must have equal > r/g/b\n"); > + return -EINVAL; > + } > + } > + > + if (i > 0 && tests & LUT_NON_DECREASING) { > + if (entry[i].red < entry[i - 1].red || > + entry[i].green < entry[i - 1].green || > + entry[i].blue < entry[i - 1].blue) { > + DRM_DEBUG_KMS( > + "LUT entries must never decrease.\n"); > + return -EINVAL; > + } > + } > + } > + > + return 0; > +} > + > static int check_luts(const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut; > const struct drm_property_blob *degamma_lut = > crtc_state->hw.degamma_lut; > int gamma_length, degamma_length; > - u32 gamma_tests, degamma_tests; > + u32 gamma_channels_tests, degamma_channels_tests; > > /* Always allow legacy gamma LUT with no further checking. */ > if (crtc_state_is_legacy_gamma(crtc_state)) > @@ -1300,15 +1333,15 @@ static int check_luts(const s
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Javier, On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote: > Some DRM drivers check the vgacon_text_force() function return value as an > indication on whether they should be allowed to be enabled or not. > > This function returns true if the nomodeset kernel command line parameter > was set. But there may be other conditions besides this to determine if a > driver should be enabled. > > Let's add a drm_drv_enabled() helper function to encapsulate that logic so > can be later extended if needed, without having to modify all the drivers. > > Also, while being there do some cleanup. The vgacon_text_force() function > is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it. > > Suggested-by: Thomas Zimmermann > Signed-off-by: Javier Martinez Canillas > --- > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 8214a0b1ab7f..3fb567d62881 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -975,6 +975,26 @@ 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. > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +int drm_drv_enabled(const struct drm_driver *driver) > +{ > + if (vgacon_text_force()) { > + DRM_INFO("%s driver is disabled\n", driver->name); DRM_INFO is deprecated, please do not use it in new code. Also other users had an error message and not just info - is info enough? > + return -ENODEV; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_drv_enabled); > + > /* > * DRM Core > * The DRM core module initializes all global DRM objects and makes them > diff --git a/drivers/gpu/drm/i915/i915_module.c > b/drivers/gpu/drm/i915/i915_module.c > index ab2295dd4500..45cb3e540eff 100644 > --- a/drivers/gpu/drm/i915/i915_module.c > +++ b/drivers/gpu/drm/i915/i915_module.c > @@ -18,9 +18,12 @@ > #include "i915_selftest.h" > #include "i915_vma.h" > > +static const struct drm_driver driver; Hmmm... > + > static int i915_check_nomodeset(void) > { > bool use_kms = true; > + int ret; > > /* >* Enable KMS by default, unless explicitly overriden by > @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void) > if (i915_modparams.modeset == 0) > use_kms = false; > > - if (vgacon_text_force() && i915_modparams.modeset == -1) > + ret = drm_drv_enabled(&driver); You pass the local driver variable here - which looks wrong as this is not the same as the driver variable declared in another file. Maybe move the check to new function you can add to init_funcs, and locate the new function in i915_drv - so it has access to driver? Sam
[Bug 214859] drm-amdgpu-init-iommu~fd-device-init.patch introduce bug
https://bugzilla.kernel.org/show_bug.cgi?id=214859 --- Comment #7 from t...@siduction.org --- With linux 5.14.17-rc1 and 5.15.1-rc1 the problem is gone. So i think, that bug is resolved. -- 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 v4 3/3] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
From: Mark Yacoub [Why] drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT sizes. There is no need to check it within amdgpu_dm_atomic_check. [How] Remove the local call to verify LUT sizes and use DRM Core function instead. Tested on ChromeOS Zork. v1: Remove amdgpu_dm_verify_lut_sizes everywhere. Signed-off-by: Mark Yacoub Reviewed-by: Sean Paul --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 - .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 35 --- 3 files changed, 4 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index f74663b6b046e..47f8de1cfc3a5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -10244,6 +10244,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, } } #endif + ret = drm_atomic_helper_check_crtcs(state); + if (ret) + return ret; + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { dm_old_crtc_state = to_dm_crtc_state(old_crtc_state); @@ -10253,10 +10257,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, dm_old_crtc_state->dsc_force_changed == false) continue; - ret = amdgpu_dm_verify_lut_sizes(new_crtc_state); - if (ret) - goto fail; - if (!new_crtc_state->enable) continue; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h index fcb9c4a629c32..22730e5542092 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -617,7 +617,6 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device *dev); #define MAX_COLOR_LEGACY_LUT_ENTRIES 256 void amdgpu_dm_init_color_mod(void); -int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state); int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc); int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc, struct dc_plane_state *dc_plane_state); diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c index a022e5bb30a5c..319f8a8a89835 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c @@ -284,37 +284,6 @@ static int __set_input_tf(struct dc_transfer_func *func, return res ? 0 : -ENOMEM; } -/** - * Verifies that the Degamma and Gamma LUTs attached to the |crtc_state| are of - * the expected size. - * Returns 0 on success. - */ -int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state) -{ - const struct drm_color_lut *lut = NULL; - uint32_t size = 0; - - lut = __extract_blob_lut(crtc_state->degamma_lut, &size); - if (lut && size != MAX_COLOR_LUT_ENTRIES) { - DRM_DEBUG_DRIVER( - "Invalid Degamma LUT size. Should be %u but got %u.\n", - MAX_COLOR_LUT_ENTRIES, size); - return -EINVAL; - } - - lut = __extract_blob_lut(crtc_state->gamma_lut, &size); - if (lut && size != MAX_COLOR_LUT_ENTRIES && - size != MAX_COLOR_LEGACY_LUT_ENTRIES) { - DRM_DEBUG_DRIVER( - "Invalid Gamma LUT size. Should be %u (or %u for legacy) but got %u.\n", - MAX_COLOR_LUT_ENTRIES, MAX_COLOR_LEGACY_LUT_ENTRIES, - size); - return -EINVAL; - } - - return 0; -} - /** * amdgpu_dm_update_crtc_color_mgmt: Maps DRM color management to DC stream. * @crtc: amdgpu_dm crtc state @@ -348,10 +317,6 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc) bool is_legacy; int r; - r = amdgpu_dm_verify_lut_sizes(&crtc->base); - if (r) - return r; - degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, °amma_size); regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, ®amma_size); -- 2.34.0.rc0.344.g81b53c2807-goog
[PATCH v4 2/3] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
From: Mark Yacoub [Why] 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma or Degamma props in the new CRTC state, allowing any invalid size to be passed on. 2. Each driver has its own LUT size, which could also be different for legacy users. [How] 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes assigned by the driver when it's initializing its color and CTM management. 2. Create drm_atomic_helper_check_crtc which is called by drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that they match the sizes in the new CRTC state. 3. As the LUT size check now happens in drm_atomic_helper_check, remove the lut check in intel_color.c Resolves: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK Tested on Zork (amdgpu) and Jacuzzi (mediatek), volteer (TGL) v3: 1. Check for lut pointer inside drm_check_lut_size. v2: 1. Remove the rename to a parent commit. 2. Create a drm drm_check_lut_size instead of intel only function. v1: 1. Fix typos 2. Remove the LUT size check from intel driver 3. Rename old LUT check to indicate it's a channel change Signed-off-by: Mark Yacoub --- drivers/gpu/drm/drm_atomic_helper.c| 52 ++ drivers/gpu/drm/drm_color_mgmt.c | 19 drivers/gpu/drm/i915/display/intel_color.c | 32 ++--- include/drm/drm_atomic_helper.h| 1 + include/drm/drm_color_mgmt.h | 3 ++ include/drm/drm_crtc.h | 11 + 6 files changed, 99 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc3487964fb5e..548e5d8221fb4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -929,6 +929,54 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_planes); +/** + * drm_atomic_helper_check_crtcs - validate state object for CRTC changes + * @state: the driver state object + * + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the new + * state holds them. + * + * RETURNS: + * Zero for success or -errno + */ +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *new_crtc_state; + int i; + + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) { + if (!new_crtc_state->color_mgmt_changed) + continue; + + if (drm_check_lut_size(new_crtc_state->gamma_lut, + crtc->gamma_lut_size) || + drm_check_lut_size(new_crtc_state->gamma_lut, + crtc->gamma_size)) { + drm_dbg_state( + state->dev, + "Invalid Gamma LUT size. Expected %u/%u, got %u.\n", + crtc->gamma_lut_size, crtc->gamma_size, + drm_color_lut_size(new_crtc_state->gamma_lut)); + return -EINVAL; + } + + if (drm_check_lut_size(new_crtc_state->degamma_lut, + crtc->degamma_lut_size)) { + drm_dbg_state( + state->dev, + "Invalid Degamma LUT size. Expected %u, got %u.\n", + crtc->degamma_lut_size, + drm_color_lut_size( + new_crtc_state->degamma_lut)); + return -EINVAL; + } + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_check_crtcs); + /** * drm_atomic_helper_check - validate state object * @dev: DRM device @@ -974,6 +1022,10 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret; + ret = drm_atomic_helper_check_crtcs(state); + if (ret) + return ret; + if (state->legacy_cursor_update) state->async_update = !drm_atomic_helper_async_check(dev, state); diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 16a07f84948f3..c85094223b535 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, struct drm_mode_config *config = &dev->mode_config; if (degamma_lut_size) { + crtc->degamma_lut_size = degamma_lut_size; drm_object_attach_property(&crtc->base, config->degamma_lut_property, 0); drm_object_attach_property(&crtc->base, @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, config->ctm_property, 0); if (gamma_lut_size) { + crtc->ga
[PATCH v4 1/3] drm: Move drm_color_lut_check implementation internal to intel_color
From: Mark Yacoub [Why] The tests of LUT_EQUAL_CHANNELS and LUT_NON_DECREASING are currently unique to i915 driver. Freeing up the function name for the more generic LUT checks to folllow Tested on Eldrid ChromeOS (TGL). v1: Stuff the test function from DRM to intel driver. Signed-off-by: Mark Yacoub --- drivers/gpu/drm/drm_color_mgmt.c | 43 -- drivers/gpu/drm/i915/display/intel_color.c | 43 +++--- drivers/gpu/drm/i915/display/intel_color.h | 27 ++ drivers/gpu/drm/i915/i915_pci.c| 27 -- include/drm/drm_color_mgmt.h | 27 -- 5 files changed, 81 insertions(+), 86 deletions(-) diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index bb14f488c8f6c..16a07f84948f3 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -583,46 +583,3 @@ int drm_plane_create_color_properties(struct drm_plane *plane, return 0; } EXPORT_SYMBOL(drm_plane_create_color_properties); - -/** - * drm_color_lut_check - check validity of lookup table - * @lut: property blob containing LUT to check - * @tests: bitmask of tests to run - * - * Helper to check whether a userspace-provided lookup table is valid and - * satisfies hardware requirements. Drivers pass a bitmask indicating which of - * the tests in &drm_color_lut_tests should be performed. - * - * Returns 0 on success, -EINVAL on failure. - */ -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests) -{ - const struct drm_color_lut *entry; - int i; - - if (!lut || !tests) - return 0; - - entry = lut->data; - for (i = 0; i < drm_color_lut_size(lut); i++) { - if (tests & DRM_COLOR_LUT_EQUAL_CHANNELS) { - if (entry[i].red != entry[i].blue || - entry[i].red != entry[i].green) { - DRM_DEBUG_KMS("All LUT entries must have equal r/g/b\n"); - return -EINVAL; - } - } - - if (i > 0 && tests & DRM_COLOR_LUT_NON_DECREASING) { - if (entry[i].red < entry[i - 1].red || - entry[i].green < entry[i - 1].green || - entry[i].blue < entry[i - 1].blue) { - DRM_DEBUG_KMS("LUT entries must never decrease.\n"); - return -EINVAL; - } - } - } - - return 0; -} -EXPORT_SYMBOL(drm_color_lut_check); diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c index dab892d2251ba..bde98a155c9f3 100644 --- a/drivers/gpu/drm/i915/display/intel_color.c +++ b/drivers/gpu/drm/i915/display/intel_color.c @@ -1279,13 +1279,46 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected) return 0; } +static int test_luts(const struct drm_property_blob *lut, u32 tests) +{ + const struct drm_color_lut *entry; + int i; + + if (!lut || !tests) + return 0; + + entry = lut->data; + for (i = 0; i < drm_color_lut_size(lut); i++) { + if (tests & LUT_EQUAL_CHANNELS) { + if (entry[i].red != entry[i].blue || + entry[i].red != entry[i].green) { + DRM_DEBUG_KMS( + "All LUT entries must have equal r/g/b\n"); + return -EINVAL; + } + } + + if (i > 0 && tests & LUT_NON_DECREASING) { + if (entry[i].red < entry[i - 1].red || + entry[i].green < entry[i - 1].green || + entry[i].blue < entry[i - 1].blue) { + DRM_DEBUG_KMS( + "LUT entries must never decrease.\n"); + return -EINVAL; + } + } + } + + return 0; +} + static int check_luts(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); const struct drm_property_blob *gamma_lut = crtc_state->hw.gamma_lut; const struct drm_property_blob *degamma_lut = crtc_state->hw.degamma_lut; int gamma_length, degamma_length; - u32 gamma_tests, degamma_tests; + u32 gamma_channels_tests, degamma_channels_tests; /* Always allow legacy gamma LUT with no further checking. */ if (crtc_state_is_legacy_gamma(crtc_state)) @@ -1300,15 +1333,15 @@ static int check_luts(const struct intel_crtc_state *crtc_state) degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size; gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size; - degam
Re: Questions about KMS flip
+Nick It looks to be the old drm_plane_state->fb holds that reference. See dm_plane_helper_cleanup_fb() in amdgpu_dm.c. Harry On 2021-11-04 08:51, Christian König wrote: > Hi guys, > > adding the usual suspects which might know that of hand: When we do a KMS > page flip, who keeps the reference to the BO while it is scanned out? > > We are running into warning backtraces from TTM which look more than odd. > > Thanks, > Christian.
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On 11/4/21 17:24, Jani Nikula wrote: [snip] >> index ab2295dd4500..45cb3e540eff 100644 >> --- a/drivers/gpu/drm/i915/i915_module.c >> +++ b/drivers/gpu/drm/i915/i915_module.c >> @@ -18,9 +18,12 @@ >> #include "i915_selftest.h" >> #include "i915_vma.h" >> >> +static const struct drm_driver driver; >> + > > No, this makes absolutely no sense, and will also oops on nomodeset. > Ups, sorry about that. For some reason I thought that it was defined in the same compilation unit, but I noticed now that it is in i915_drv.c. > BR, > Jani. > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] drm/fb-helper: Fix restore_fbdev when there are pending delayed hotplug
When using Xorg/Logind and an external monitor connected with an MST dock. After disconnecting the external monitor, switching to VT may not work, the (internal) monitor sill display Xorg, and you can't see what you are typing in the VT. This is related to commit e2809c7db818df6bbd0edf843e1beb2fbc9d8541 which introduced delayed hotplug for MST, and commit dc5bdb68b5b369d5bc7d1de96fa64cc1737a6320 which introduced a workaround for Xorg and logind, and add a force parameter to __drm_fb_helper_restore_fbdev_mode_unlocked() in this case. When switching to VT, with Xorg and logind, if there are pending hotplug event (like MST unplugged), the hotplug path may not be fulfilled, because logind may drop the master a bit later. It leads to the console not showing up on the monitor. So in this case, forward the "force" parameter to the hotplug event, and ignore if there is a drm master in this case. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_helper.c | 66 ++--- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 8e7a124d6c5a..c07080f661b1 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -82,6 +82,8 @@ MODULE_PARM_DESC(drm_leak_fbdev_smem, static LIST_HEAD(kernel_fb_helper_list); static DEFINE_MUTEX(kernel_fb_helper_lock); +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force); + /** * DOC: fbdev helpers * @@ -258,7 +260,7 @@ __drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper, mutex_unlock(&fb_helper->lock); if (do_delayed) - drm_fb_helper_hotplug_event(fb_helper); + __drm_fb_helper_hotplug_event(fb_helper, force); return ret; } @@ -1930,6 +1932,38 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) } EXPORT_SYMBOL(drm_fb_helper_initial_config); +static int __drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper, bool force) +{ + int err = 0; + + if (!drm_fbdev_emulation || !fb_helper) + return 0; + + mutex_lock(&fb_helper->lock); + if (fb_helper->deferred_setup) { + err = __drm_fb_helper_initial_config_and_unlock(fb_helper, + fb_helper->preferred_bpp); + return err; + } + if (!force) { + if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { + fb_helper->delayed_hotplug = true; + mutex_unlock(&fb_helper->lock); + return err; + } + drm_master_internal_release(fb_helper->dev); + } + drm_dbg_kms(fb_helper->dev, "\n"); + + drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); + drm_setup_crtcs_fb(fb_helper); + mutex_unlock(&fb_helper->lock); + + drm_fb_helper_set_par(fb_helper->fbdev); + + return 0; +} + /** * drm_fb_helper_hotplug_event - respond to a hotplug notification by * probing all the outputs attached to the fb @@ -1953,35 +1987,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); */ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { - int err = 0; - - if (!drm_fbdev_emulation || !fb_helper) - return 0; - - mutex_lock(&fb_helper->lock); - if (fb_helper->deferred_setup) { - err = __drm_fb_helper_initial_config_and_unlock(fb_helper, - fb_helper->preferred_bpp); - return err; - } - - if (!fb_helper->fb || !drm_master_internal_acquire(fb_helper->dev)) { - fb_helper->delayed_hotplug = true; - mutex_unlock(&fb_helper->lock); - return err; - } - - drm_master_internal_release(fb_helper->dev); - - drm_dbg_kms(fb_helper->dev, "\n"); - - drm_client_modeset_probe(&fb_helper->client, fb_helper->fb->width, fb_helper->fb->height); - drm_setup_crtcs_fb(fb_helper); - mutex_unlock(&fb_helper->lock); - - drm_fb_helper_set_par(fb_helper->fbdev); - - return 0; + return __drm_fb_helper_hotplug_event(fb_helper, false); } EXPORT_SYMBOL(drm_fb_helper_hotplug_event); -- 2.33.1
Re: [RFC v2 02/22] drm: Add Enhanced Gamma and color lut range attributes
On 2021-11-04 04:38, Pekka Paalanen wrote: > On Wed, 3 Nov 2021 11:08:13 -0400 > Harry Wentland wrote: > >> 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. > > > Hi Harry, > > I think you just would not. Conceptually an SDR plane gets its very own > LUT that converts the FB [0.0, 1.0] range to any appropriate [a >= 0.0, > b <= 1.0] range in HDR. This is purely conceptual, in the terms of the > abstract KMS color pipeline, where [0.0, 1.0] is always the full > dynamic range at any point of the pipeline, meaning it is relative to > it
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Thu, 04 Nov 2021, Javier Martinez Canillas wrote: > Some DRM drivers check the vgacon_text_force() function return value as an > indication on whether they should be allowed to be enabled or not. > > This function returns true if the nomodeset kernel command line parameter > was set. But there may be other conditions besides this to determine if a > driver should be enabled. > > Let's add a drm_drv_enabled() helper function to encapsulate that logic so > can be later extended if needed, without having to modify all the drivers. > > Also, while being there do some cleanup. The vgacon_text_force() function > is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it. > > Suggested-by: Thomas Zimmermann > Signed-off-by: Javier Martinez Canillas > --- > > Changes in v2: > - Squash patch to add drm_drv_enabled() and make drivers use it. > - Make the drivers changes before moving nomodeset logic to DRM. > - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset. > - Remove debug and error messages in drivers. > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++ > drivers/gpu/drm/ast/ast_drv.c | 7 +-- > drivers/gpu/drm/drm_drv.c | 20 > drivers/gpu/drm/i915/i915_module.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +-- > drivers/gpu/drm/nouveau/nouveau_drm.c | 5 - > drivers/gpu/drm/qxl/qxl_drv.c | 7 +-- > drivers/gpu/drm/radeon/radeon_drv.c | 6 -- > drivers/gpu/drm/tiny/bochs.c| 7 +-- > drivers/gpu/drm/tiny/cirrus.c | 8 ++-- > drivers/gpu/drm/vboxvideo/vbox_drv.c| 9 + > drivers/gpu/drm/virtio/virtgpu_drv.c| 5 +++-- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +++-- > include/drm/drm_drv.h | 1 + > 14 files changed, 74 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index c718fb5f3f8a..7fde40d06181 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > @@ -2514,10 +2514,9 @@ static int __init amdgpu_init(void) > { > int r; > > - if (vgacon_text_force()) { > - DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n"); > - return -EINVAL; > - } > + r = drm_drv_enabled(&amdgpu_kms_driver) > + if (r) > + return r; > > r = amdgpu_sync_init(); > if (r) > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c > index 86d5cd7b6318..802063279b86 100644 > --- a/drivers/gpu/drm/ast/ast_drv.c > +++ b/drivers/gpu/drm/ast/ast_drv.c > @@ -233,8 +233,11 @@ static struct pci_driver ast_pci_driver = { > > static int __init ast_init(void) > { > - if (vgacon_text_force() && ast_modeset == -1) > - return -EINVAL; > + int ret; > + > + ret = drm_drv_enabled(&ast_driver); > + if (ret && ast_modeset == -1) > + return ret; > > if (ast_modeset == 0) > return -EINVAL; > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 8214a0b1ab7f..3fb567d62881 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -975,6 +975,26 @@ 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. > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +int drm_drv_enabled(const struct drm_driver *driver) > +{ > + if (vgacon_text_force()) { > + DRM_INFO("%s driver is disabled\n", driver->name); > + return -ENODEV; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_drv_enabled); > + > /* > * DRM Core > * The DRM core module initializes all global DRM objects and makes them > diff --git a/drivers/gpu/drm/i915/i915_module.c > b/drivers/gpu/drm/i915/i915_module.c > index ab2295dd4500..45cb3e540eff 100644 > --- a/drivers/gpu/drm/i915/i915_module.c > +++ b/drivers/gpu/drm/i915/i915_module.c > @@ -18,9 +18,12 @@ > #include "i915_selftest.h" > #include "i915_vma.h" > > +static const struct drm_driver driver; > + No, this makes absolutely no sense, and will also oops on nomodeset. BR, Jani. > static int i915_check_nomodeset(void) > { > bool use_kms = true; > + int ret; > > /* >* Enable KMS by default, unless explicitly overriden by > @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void) > if (i915_modparams.modeset == 0) > use_kms = false; > > - if (vgacon_text_force() && i915_modparams.modeset == -1) > + ret = drm_drv_enabled(&driver); > + if (ret && i915_modparams.modeset == -
[PATCH v2 2/2] drm: Move nomodeset kernel parameter 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 the vgacon_text_force() function and related logic to the DRM subsystem. While doing that, rename the function to drm_check_modeset() which better reflects what the function is really used to test for. Suggested-by: Daniel Vetter Signed-off-by: Javier Martinez Canillas --- Changes in v2: - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set. - Squash patches to move nomodeset logic to DRM and do the renaming. - Name the function drm_check_modeset() and make it return -ENODEV. drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 - drivers/gpu/drm/ast/ast_drv.c | 1 - drivers/gpu/drm/drm_drv.c | 9 + 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 -- 18 files changed, 39 insertions(+), 44 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1c41156deb5f..c74810c285af 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-$(CONFIG_VGA_CONSOLE) += 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 7fde40d06181..b4b6993861e6 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 diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index 802063279b86..6222082c3082 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_drv.c b/drivers/gpu/drm/drm_drv.c index 3fb567d62881..80b85b8ea776 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique); */ int drm_drv_enabled(const struct drm_driver *driver) { - if (vgacon_text_force()) { + int ret; + + ret = drm_check_modeset(); + if (ret) DRM_INFO("%s driver is disabled\n", driver->name); - return -ENODEV; - } - return 0; + return ret; } EXPORT_SYMBOL(drm_drv_enabled); diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c new file mode 100644 index ..6683e396d2c5 --- /dev/null +++ b/drivers/gpu/drm/drm_nomodeset.c @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include + +static bool drm_nomodeset; + +int drm_check_modeset(void) +{ + return drm_nomodeset ? -ENODEV : 0; +} +EXPORT_SYMBOL(drm_check_modeset); + +static int __init disable_modeset(char *str) +{ + 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"); + pr_warn("Unless you actually understand what nomodeset does, you should reboot without enabling it\n"); + + return 1; +} + +/* 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 45cb3e540eff..c890c1ca20c4 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 2a581094ba2b..8e000cac11ba 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@
[PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Some DRM drivers check the vgacon_text_force() function return value as an indication on whether they should be allowed to be enabled or not. This function returns true if the nomodeset kernel command line parameter was set. But there may be other conditions besides this to determine if a driver should be enabled. Let's add a drm_drv_enabled() helper function to encapsulate that logic so can be later extended if needed, without having to modify all the drivers. Also, while being there do some cleanup. The vgacon_text_force() function is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it. Suggested-by: Thomas Zimmermann Signed-off-by: Javier Martinez Canillas --- Changes in v2: - Squash patch to add drm_drv_enabled() and make drivers use it. - Make the drivers changes before moving nomodeset logic to DRM. - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset. - Remove debug and error messages in drivers. drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++ drivers/gpu/drm/ast/ast_drv.c | 7 +-- drivers/gpu/drm/drm_drv.c | 20 drivers/gpu/drm/i915/i915_module.c | 6 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 5 - drivers/gpu/drm/qxl/qxl_drv.c | 7 +-- drivers/gpu/drm/radeon/radeon_drv.c | 6 -- drivers/gpu/drm/tiny/bochs.c| 7 +-- drivers/gpu/drm/tiny/cirrus.c | 8 ++-- drivers/gpu/drm/vboxvideo/vbox_drv.c| 9 + drivers/gpu/drm/virtio/virtgpu_drv.c| 5 +++-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +++-- include/drm/drm_drv.h | 1 + 14 files changed, 74 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index c718fb5f3f8a..7fde40d06181 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -2514,10 +2514,9 @@ static int __init amdgpu_init(void) { int r; - if (vgacon_text_force()) { - DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n"); - return -EINVAL; - } + r = drm_drv_enabled(&amdgpu_kms_driver) + if (r) + return r; r = amdgpu_sync_init(); if (r) diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c index 86d5cd7b6318..802063279b86 100644 --- a/drivers/gpu/drm/ast/ast_drv.c +++ b/drivers/gpu/drm/ast/ast_drv.c @@ -233,8 +233,11 @@ static struct pci_driver ast_pci_driver = { static int __init ast_init(void) { - if (vgacon_text_force() && ast_modeset == -1) - return -EINVAL; + int ret; + + ret = drm_drv_enabled(&ast_driver); + if (ret && ast_modeset == -1) + return ret; if (ast_modeset == 0) return -EINVAL; diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 8214a0b1ab7f..3fb567d62881 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -975,6 +975,26 @@ 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. + * + * Return: 0 on success or a negative error code on failure. + */ +int drm_drv_enabled(const struct drm_driver *driver) +{ + if (vgacon_text_force()) { + DRM_INFO("%s driver is disabled\n", driver->name); + return -ENODEV; + } + + return 0; +} +EXPORT_SYMBOL(drm_drv_enabled); + /* * DRM Core * The DRM core module initializes all global DRM objects and makes them diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c index ab2295dd4500..45cb3e540eff 100644 --- a/drivers/gpu/drm/i915/i915_module.c +++ b/drivers/gpu/drm/i915/i915_module.c @@ -18,9 +18,12 @@ #include "i915_selftest.h" #include "i915_vma.h" +static const struct drm_driver driver; + static int i915_check_nomodeset(void) { bool use_kms = true; + int ret; /* * Enable KMS by default, unless explicitly overriden by @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void) if (i915_modparams.modeset == 0) use_kms = false; - if (vgacon_text_force() && i915_modparams.modeset == -1) + ret = drm_drv_enabled(&driver); + if (ret && 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 6b9243713b3c..2a581094ba2b 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -378,8 +378,11 @@ static struct pci_driver mgag
[PATCH v2 0/2] Cleanups for the nomodeset kernel command line parameter logic
There is a lot of historical baggage on this parameter. It is defined in the vgacon driver as nomodeset, but its set function is called text_mode() and the value 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 should 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. This is a v2 of the patches, that address the issues pointed out by Thomas Zimmermann and Jani Nikula in v1: https://lore.kernel.org/lkml/5b4e4534-4786-d231-e331-78fdb5d84...@redhat.com/T/ Patch #1 adds a drm_drv_enabled() function that could be used by drivers to check if these could be enabled, instead of just using vgacon_text_force(). Patch #2 moves the nomodeset logic to the DRM subsystem and renames the functions and variables to better explain what these actually do. Changes in v2: - Squash patch to add drm_drv_enabled() and make drivers use it. - Make the drivers changes before moving nomodeset logic to DRM. - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset. - Remove debug and error messages in drivers. - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set. - Squash patches to move nomodeset logic to DRM and do the renaming. - Name the function drm_check_modeset() and make it return -ENODEV. Javier Martinez Canillas (2): drm: Add a drm_drv_enabled() to check if drivers should be enabled drm: Move nomodeset kernel parameter to the DRM subsystem drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 +++- drivers/gpu/drm/ast/ast_drv.c | 8 +--- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 8 +--- drivers/gpu/drm/mgag200/mgag200_drv.c | 8 +--- drivers/gpu/drm/nouveau/nouveau_drm.c | 6 -- drivers/gpu/drm/qxl/qxl_drv.c | 8 +--- drivers/gpu/drm/radeon/radeon_drv.c | 7 --- drivers/gpu/drm/tiny/bochs.c| 8 +--- drivers/gpu/drm/tiny/cirrus.c | 9 ++--- drivers/gpu/drm/vboxvideo/vbox_drv.c| 10 +- drivers/gpu/drm/virtio/virtgpu_drv.c| 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 +++--- 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, 109 insertions(+), 66 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
Re: [PATCH] drm/i915/selftests: Use clear_and_wake_up_bit() for the per-engine reset bitlocks
On Thu, Nov 04, 2021 at 01:58:44PM +0100, Thomas Hellström wrote: > Some selftests assume that nothing will attempt to grab these bitlocks > while they are held by the selftests. With GuC, for example, that is > not true because the hanging workloads may cause the GuC code to attempt > to grab them for a global reset, and that may cause it to end up > sleeping on the bit never waking up. Regardless whether that will be > the final solution for GuC, use clear_and_wake_up_bit() pending a more > thorough investigation on how this should be handled moving forward. > > Signed-off-by: Thomas Hellström This series will also fix the CI crash: https://patchwork.freedesktop.org/series/96406/ Regardless of the above series this one looks correct and needed. With that: Reviewed-by: Matthew Brost > --- > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 8 > drivers/gpu/drm/i915/selftests/igt_reset.c | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > index 7e2d99dd012d..8590419be4c6 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > @@ -528,7 +528,7 @@ static int igt_reset_nop_engine(void *arg) > break; > } > } while (time_before(jiffies, end_time)); > - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); > + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); > st_engine_heartbeat_enable(engine); > > pr_info("%s(%s): %d resets\n", __func__, engine->name, count); > @@ -679,7 +679,7 @@ static int igt_reset_fail_engine(void *arg) > out: > pr_info("%s(%s): %d resets\n", __func__, engine->name, count); > skip: > - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); > + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); > st_engine_heartbeat_enable(engine); > intel_context_put(ce); > > @@ -824,7 +824,7 @@ static int __igt_reset_engine(struct intel_gt *gt, bool > active) > if (err) > break; > } while (time_before(jiffies, end_time)); > - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); > + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); > st_engine_heartbeat_enable(engine); > pr_info("%s: Completed %lu %s resets\n", > engine->name, count, active ? "active" : "idle"); > @@ -1165,7 +1165,7 @@ static int __igt_reset_engines(struct intel_gt *gt, > if (err) > break; > } while (time_before(jiffies, end_time)); > - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); > + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); > st_engine_heartbeat_enable_no_pm(engine); > > pr_info("i915_reset_engine(%s:%s): %lu resets\n", > diff --git a/drivers/gpu/drm/i915/selftests/igt_reset.c > b/drivers/gpu/drm/i915/selftests/igt_reset.c > index 9f8590b868a9..a2838c65f8a5 100644 > --- a/drivers/gpu/drm/i915/selftests/igt_reset.c > +++ b/drivers/gpu/drm/i915/selftests/igt_reset.c > @@ -36,7 +36,7 @@ void igt_global_reset_unlock(struct intel_gt *gt) > enum intel_engine_id id; > > for_each_engine(engine, gt, id) > - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); > + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); > > clear_bit(I915_RESET_BACKOFF, >->reset.flags); > wake_up_all(>->reset.queue); > -- > 2.31.1 >
Re: [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
On Thu, Nov 04, 2021 at 09:48:41AM +0100, Maxime Ripard wrote: > Hi Ville, > > On Wed, Nov 03, 2021 at 08:05:16PM +0200, Ville Syrjälä wrote: > > 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. > > Patch 2 introduces something along those lines. > > It doesn't cover everything though, we're using this define in vc4 to > limit the available modes in mode_valid on HDMI controllers not > 4k-capable I wouldn't want to use this kind of define for those kinds of checks anyway. If the hardware has specific limits in what kind of clocks it can generate (or what it was validated for) IMO you should spell those out explicitly instead of assuming they happen to match some standard defined max value. -- Ville Syrjälä Intel
Re: [PATCH 02/13] drm/connector: Add helper to check if a mode requires scrambling
On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote: > --- a/include/drm/drm_modes.h > +++ b/include/drm/drm_modes.h > @@ -424,6 +424,21 @@ static inline bool drm_mode_is_stereo(const struct > drm_display_mode *mode) > return mode->flags & DRM_MODE_FLAG_3D_MASK; > } > > +/** > + * drm_mode_hdmi_requires_scrambling - Checks if a mode requires HDMI > Scrambling > + * @mode: DRM display mode > + * > + * Checks if a given display mode requires the scrambling to be enabled. > + * > + * Returns: > + * A boolean stating whether it's required or not. > + */ > +static inline bool > +drm_mode_hdmi_requires_scrambling(const struct drm_display_mode *mode) > +{ > + return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ; > +} That's only correct for 8bpc. The actual limit is on the TMDS clock (or rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4 magic for the actually transmitted TMDS clock). -- Ville Syrjälä Intel
RE: refactor the i915 GVT support and move to the modern mdev API v2
Hi Joonas and Christoph: We were testing the patch series since Monday and planning to reply after we get the test result. Mostly, we are concerned about patch 6 and how it would affect the test result. Patch 6 changes the timing of loading GVT-g. According to the discussion in the last email, this will break our design of golden MMIO snapshot. Also moving GVT-g code into kvmgt.ko requires the discussion of defining and shrinking the interfaces between i915 and kvmgt. I guess the ideal way to take Christoph's patch is: 1) We have to figure out how to deal with golden MMIO snapshot. It's a little bit hard to take the re-factor patch before settling this down. In the previous discussion, we would like to find a way to do the snapshot in intel_gvt.c 2) Shrink and refine the exported interfaces because of moving the code into kvmgt.ko 3) Get patches reviewed and merged. For 1) I was thinking to separated the MMIO handler table from handlers.c and let it build different data structures depends on where it got referenced. 2) That's might require some more discussion. Is it possible to separate the refactor part from the using new mdev API stuff? So that the design opens in the re-factor patches wouldn’t block the process of mdev API improvement? Thanks, Zhi. -Original Message- From: Joonas Lahtinen Sent: Thursday, November 4, 2021 2:59 PM To: Christoph Hellwig ; Jani Nikula ; Vivi, Rodrigo ; Zhenyu Wang ; Wang, Zhi A Cc: Jason Gunthorpe ; intel-...@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org Subject: Re: refactor the i915 GVT support and move to the modern mdev API v2 Hi Zhenyu and Zhi, Can you have somebody from the GVT team to review the patches that are fully contained in gvt/ ? I also started discussion on patch 6 which is about defining the interface between the modules. I remember there is prior work to shrink the interface. Do you have links to such patches? The minimal we should do is to eliminate the double underscore prefixed functions. But I would prefer to have the symbol exports by default so that we can enable the functionality just by loading the module. Regards, Joonas Quoting Christoph Hellwig (2021-11-02 09:05:32) > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors > various bits to clean that up. The main user visible change is that > almost all of the GVT code moves out of the main i915 driver and into > the kvmgt module. > > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-g > vt > > Changes since v1: > - rebased on Linux 5.15 > - allow the kvmgvt module to be loaded at any time and thus solve >the deadlock when both i915 amd kvmgvt are modular > - include the conversion to the modern mdev API > > Note that I do expect to rebased this again against 5.16-rc1 once > released, but I'd like to get this out for review ASAP. > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 33 > b/drivers/gpu/drm/i915/Makefile | 31 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c |4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c | 36 - > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c| 55 + > b/drivers/gpu/drm/i915/gvt/gvt.h| 125 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 + > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 1099 > +++- > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 > b/drivers/gpu/drm/i915/gvt/page_track.c |8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 - > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.c |7 > b/drivers/gpu/drm/i915/i915_drv.h |1 > b/drivers/gpu/drm/i915/i915_trace.h |1 > b/drivers/gpu/drm/i915/intel_gvt.c | 162 +++- > b/drivers/gpu/drm/i915/intel_gvt.h | 17 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/gvt.c | 340 - > drivers/gpu/drm/i915/gvt/hypercall.h| 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 --- > 25 files changed, 929 insertions(+), 1833 deletions(-)
[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 #17 from Lang Yu (lang...@amd.com) --- Created attachment 299441 --> https://bugzilla.kernel.org/attachment.cgi?id=299441&action=edit test patch to find who pinned amdgpu dmabuf dmesg may be too large, please add log_buf_len=1024M into kernel cmdline. Thanks for you help! -- 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 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 #16 from Alex Deucher (alexdeuc...@gmail.com) --- (In reply to Lang Yu from comment #15) > Many thanks for your help! I made a test patch to find who pinned amdgpu > dmabuf. Did you forget to attach it? -- 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 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 #15 from Lang Yu (lang...@amd.com) --- (In reply to Erhard F. from comment #14) > (In reply to Lang Yu from comment #12) > > The warning was just merged into mainline 5.15.0 on Tue Nov 2 16:47:49 > > 2021(commit 56d33754481f). Not sure Erhard F.'s build contains this > warning. > I applied your patch on top of v5.15 after its' release which was 2021-10-31 > not on git master. Many thanks for your help! I made a test patch to find who pinned amdgpu dmabuf. Could you please apply it on latest(commit 7ddb58cb0ecae8e8b6181d736a87667cc9ab8389) mainline 5.15.0, then reproduce the warning and collect full dmesg? As I still didn't reproduce it on my machine... -- 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 0/5] Add support for Wanchanglong panel used in px30-evb v11
Hi Sam On Sat, Oct 16, 2021 at 2:27 PM Sam Ravnborg wrote: > > Hi Michael, > > On Sat, Oct 16, 2021 at 10:22:27AM +, Michael Trimarchi wrote: > > This patch series add support for W552946ABA panel. This panel is used > > in px30-evb v11. All the patches can be applied on top of drm-fixes > > branch. The last patch is suppose to fix a race when the panel is built > > in. Tested on px30 evb > > > > Michael Trimarchi (5): > > dt-bindings: vendor-prefix: add Wanchanglong Electronics Technology > > drm/panel: ilitek-ili9881d: add support for Wanchanglong W552946ABA > > panel > > dt-bindings: ili9881c: add compatible string for Wanchanglong > > w552946aba > > drm/panel: ilitek-ili9881c: Make gpio-reset optional > The four patches has been applied to drm-misc-next. > I sent another fix on the same panel. Are those patches queued on some tree? > > drm/bridge: dw-mipi-dsi: Fix dsi registration during drm probing > This patch looks like it does not belong in this series. > Anyway - commented on it as I did not understand the code. > > Sam Michael
[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 #14 from Erhard F. (erhar...@mailbox.org) --- (In reply to Lang Yu from comment #12) > The warning was just merged into mainline 5.15.0 on Tue Nov 2 16:47:49 > 2021(commit 56d33754481f). Not sure Erhard F.'s build contains this warning. I applied your patch on top of v5.15 after its' release which was 2021-10-31 not on git master. -- 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: refactor the i915 GVT support and move to the modern mdev API v2
Hi Zhenyu and Zhi, Can you have somebody from the GVT team to review the patches that are fully contained in gvt/ ? I also started discussion on patch 6 which is about defining the interface between the modules. I remember there is prior work to shrink the interface. Do you have links to such patches? The minimal we should do is to eliminate the double underscore prefixed functions. But I would prefer to have the symbol exports by default so that we can enable the functionality just by loading the module. Regards, Joonas Quoting Christoph Hellwig (2021-11-02 09:05:32) > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors various > bits to clean that up. The main user visible change is that almost all > of the GVT code moves out of the main i915 driver and into the kvmgt > module. > > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-gvt > > Changes since v1: > - rebased on Linux 5.15 > - allow the kvmgvt module to be loaded at any time and thus solve >the deadlock when both i915 amd kvmgvt are modular > - include the conversion to the modern mdev API > > Note that I do expect to rebased this again against 5.16-rc1 once > released, but I'd like to get this out for review ASAP. > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 33 > b/drivers/gpu/drm/i915/Makefile | 31 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c |4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c | 36 - > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c| 55 + > b/drivers/gpu/drm/i915/gvt/gvt.h| 125 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 + > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 1099 > +++- > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 > b/drivers/gpu/drm/i915/gvt/page_track.c |8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 - > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.c |7 > b/drivers/gpu/drm/i915/i915_drv.h |1 > b/drivers/gpu/drm/i915/i915_trace.h |1 > b/drivers/gpu/drm/i915/intel_gvt.c | 162 +++- > b/drivers/gpu/drm/i915/intel_gvt.h | 17 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/gvt.c | 340 - > drivers/gpu/drm/i915/gvt/hypercall.h| 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 --- > 25 files changed, 929 insertions(+), 1833 deletions(-)
[PATCH] drm/i915/selftests: Use clear_and_wake_up_bit() for the per-engine reset bitlocks
Some selftests assume that nothing will attempt to grab these bitlocks while they are held by the selftests. With GuC, for example, that is not true because the hanging workloads may cause the GuC code to attempt to grab them for a global reset, and that may cause it to end up sleeping on the bit never waking up. Regardless whether that will be the final solution for GuC, use clear_and_wake_up_bit() pending a more thorough investigation on how this should be handled moving forward. Signed-off-by: Thomas Hellström --- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 8 drivers/gpu/drm/i915/selftests/igt_reset.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c index 7e2d99dd012d..8590419be4c6 100644 --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c @@ -528,7 +528,7 @@ static int igt_reset_nop_engine(void *arg) break; } } while (time_before(jiffies, end_time)); - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); st_engine_heartbeat_enable(engine); pr_info("%s(%s): %d resets\n", __func__, engine->name, count); @@ -679,7 +679,7 @@ static int igt_reset_fail_engine(void *arg) out: pr_info("%s(%s): %d resets\n", __func__, engine->name, count); skip: - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); st_engine_heartbeat_enable(engine); intel_context_put(ce); @@ -824,7 +824,7 @@ static int __igt_reset_engine(struct intel_gt *gt, bool active) if (err) break; } while (time_before(jiffies, end_time)); - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); st_engine_heartbeat_enable(engine); pr_info("%s: Completed %lu %s resets\n", engine->name, count, active ? "active" : "idle"); @@ -1165,7 +1165,7 @@ static int __igt_reset_engines(struct intel_gt *gt, if (err) break; } while (time_before(jiffies, end_time)); - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); st_engine_heartbeat_enable_no_pm(engine); pr_info("i915_reset_engine(%s:%s): %lu resets\n", diff --git a/drivers/gpu/drm/i915/selftests/igt_reset.c b/drivers/gpu/drm/i915/selftests/igt_reset.c index 9f8590b868a9..a2838c65f8a5 100644 --- a/drivers/gpu/drm/i915/selftests/igt_reset.c +++ b/drivers/gpu/drm/i915/selftests/igt_reset.c @@ -36,7 +36,7 @@ void igt_global_reset_unlock(struct intel_gt *gt) enum intel_engine_id id; for_each_engine(engine, gt, id) - clear_bit(I915_RESET_ENGINE + id, >->reset.flags); + clear_and_wake_up_bit(I915_RESET_ENGINE + id, >->reset.flags); clear_bit(I915_RESET_BACKOFF, >->reset.flags); wake_up_all(>->reset.queue); -- 2.31.1
Re: [PATCH 06/29] drm/i915/gvt: move the gvt code into kvmgt.ko
+ Thomas, Maarten and Matt (Also, Zhi and Zhenyu, please see down) Quoting Christoph Hellwig (2021-11-02 09:05:38) > Instead of having an option to build the gvt code into the main i915 > module, just move it into the kvmgt.ko module. This only requires > a new struct with three entries that the KVMGT modules needs to register > with the main i915 module, and a proper list of GVT-enabled devices > instead of global device pointer. > > Signed-off-by: Christoph Hellwig > +/* > + * Exported here so that the exports only get created when GVT support is > + * actually enabled. > + */ > +EXPORT_SYMBOL_NS_GPL(i915_gem_object_alloc, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_object_create_shmem, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_object_init, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_object_ggtt_pin_ww, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_object_pin_map, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_object_set_to_cpu_domain, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(__i915_gem_object_flush_map, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(__i915_gem_object_set_pages, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_gtt_insert, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_prime_export, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_ww_ctx_init, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_ww_ctx_backoff, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_gem_ww_ctx_fini, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_ppgtt_create, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_request_add, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_request_create, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_request_wait, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_reserve_fence, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_unreserve_fence, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_vm_release, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(i915_vma_move_to_active, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_context_create, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(__intel_context_do_pin, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(__intel_context_do_unpin, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_ring_begin, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_runtime_pm_get, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_runtime_pm_put_unchecked, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_uncore_forcewake_for_reg, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_uncore_forcewake_get, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(intel_uncore_forcewake_put, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(shmem_pin_map, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(shmem_unpin_map, I915_GVT); > +EXPORT_SYMBOL_NS_GPL(__px_dma, I915_GVT); This is where the module conversion got stuck last time. Conditionally exporting is kind of a partial remedy. Previously the intention for the rewrite was to define a clear interface between the two modules which would be well defined enough that we could avoid frequent clashes and hopefully get GVT into drm-tip, too. The absolute minimum requirement is not to have any of the double underscore symbols in this list. As that convention is specifically used to mark functions which are expected to have reduced error checking because of the exact point they are being called from. With that done we should be where we can enable the exports by default and modprobe would be all that is required. I think Zhenyu and Zhi took an AR back in time to see how small they could shrink this list. Would we have some followup patches available to shrink this interface? Also, the golden MMIO state capture remains something that leaks the hypervisor state into the guests. For example the fact which bug W/A are applied in hypervisor, and I would rather have that code path isolated before enabling by default. Regards, Joonas
Questions about KMS flip
Hi guys, adding the usual suspects which might know that of hand: When we do a KMS page flip, who keeps the reference to the BO while it is scanned out? We are running into warning backtraces from TTM which look more than odd. Thanks, Christian.
Re: [PATCH] drm/virtio: delay pinning the pages till first use
Hi Maksym, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm/drm-next] [also build test ERROR on next-20211104] [cannot apply to v5.15] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Maksym-Wezdecki/drm-virtio-delay-pinning-the-pages-till-first-use/20211102-193430 base: git://anongit.freedesktop.org/drm/drm drm-next config: i386-buildonly-randconfig-r002-20211101 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 264d3b6d4e08401c5b50a85bd76e80b3461d77e6) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/1795d2fd78a334a37a02dba76ac1e314cf122467 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Maksym-Wezdecki/drm-virtio-delay-pinning-the-pages-till-first-use/20211102-193430 git checkout 1795d2fd78a334a37a02dba76ac1e314cf122467 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): >> drivers/gpu/drm/virtio/virtgpu_object.c:254:11: error: variable 'ents' is >> uninitialized when used here [-Werror,-Wuninitialized] ents, nents); ^~~~ drivers/gpu/drm/virtio/virtgpu_object.c:219:35: note: initialize the variable 'ents' to silence this warning struct virtio_gpu_mem_entry *ents; ^ = NULL >> drivers/gpu/drm/virtio/virtgpu_object.c:254:17: error: variable 'nents' is >> uninitialized when used here [-Werror,-Wuninitialized] ents, nents); ^ drivers/gpu/drm/virtio/virtgpu_object.c:220:20: note: initialize the variable 'nents' to silence this warning unsigned int nents; ^ = 0 2 errors generated. vim +/ents +254 drivers/gpu/drm/virtio/virtgpu_object.c 2f2aa13724d568 Gerd Hoffmann 2020-02-07 210 dc5698e80cf724 Dave Airlie 2013-09-09 211 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev, 4441235f9566e6 Gerd Hoffmann 2019-03-18 212 struct virtio_gpu_object_params *params, 530b28426a94b8 Gerd Hoffmann 2019-03-18 213 struct virtio_gpu_object **bo_ptr, 530b28426a94b8 Gerd Hoffmann 2019-03-18 214 struct virtio_gpu_fence *fence) dc5698e80cf724 Dave Airlie 2013-09-09 215 { e2324300f427ff Gerd Hoffmann 2019-08-29 216 struct virtio_gpu_object_array *objs = NULL; c66df701e783bc Gerd Hoffmann 2019-08-29 217 struct drm_gem_shmem_object *shmem_obj; dc5698e80cf724 Dave Airlie 2013-09-09 218 struct virtio_gpu_object *bo; 2f2aa13724d568 Gerd Hoffmann 2020-02-07 219 struct virtio_gpu_mem_entry *ents; 2f2aa13724d568 Gerd Hoffmann 2020-02-07 220 unsigned int nents; dc5698e80cf724 Dave Airlie 2013-09-09 221 int ret; dc5698e80cf724 Dave Airlie 2013-09-09 222 dc5698e80cf724 Dave Airlie 2013-09-09 223 *bo_ptr = NULL; dc5698e80cf724 Dave Airlie 2013-09-09 224 c66df701e783bc Gerd Hoffmann 2019-08-29 225 params->size = roundup(params->size, PAGE_SIZE); c66df701e783bc Gerd Hoffmann 2019-08-29 226 shmem_obj = drm_gem_shmem_create(vgdev->ddev, params->size); c66df701e783bc Gerd Hoffmann 2019-08-29 227 if (IS_ERR(shmem_obj)) c66df701e783bc Gerd Hoffmann 2019-08-29 228 return PTR_ERR(shmem_obj); c66df701e783bc Gerd Hoffmann 2019-08-29 229 bo = gem_to_virtio_gpu_obj(&shmem_obj->base); dc5698e80cf724 Dave Airlie 2013-09-09 230 556c62e85f9b97 Matthew Wilcox 2018-10-30 231 ret = virtio_gpu_resource_id_get(vgdev, &bo->hw_res_handle); e2324300f427ff Gerd Hoffmann 2019-08-29 232 if (ret < 0) e2324300f427ff Gerd Hoffmann 2019-08-29 233 goto err_free_gem; e2324300f427ff Gerd Hoffmann 2019-08-29 234 530b28426a94b8 Gerd Hoffmann 2019-03-18 235 bo->dumb = params->dumb; 530b28426a94b8 Gerd Hoffmann 2019-03-18 236 e2324300f427ff Gerd Hoffmann
[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 #13 from Christian König (christian.koe...@amd.com) --- (In reply to Lang Yu from comment #12) > The warning was just merged into mainline 5.15.0 on Tue Nov 2 16:47:49 > 2021(commit 56d33754481f). Not sure Erhard F.'s build contains this warning. Good point. > And we can also add a debug WARN() into amdgpu_dma_buf_pin() to see who > pinned dma_buf. I thought about that as well, the problem is that we call this function very often (e.g. 60 times a second if we play a 60fps video or similar). Saying that I could as well be misguided by the dma_buf_release() function in the call stack. This could potentially also be a bug in DAL/DC where we forget to unpin a BO in some situation. -- 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 02/29] drm/i915/gvt: integrate into the main Makefile
On Thu, Nov 04, 2021 at 02:30:20PM +0200, Joonas Lahtinen wrote: > Quoting Christoph Hellwig (2021-11-02 09:05:34) > > Remove the separately included Makefile and just use the relative > > reference from the main i915 Makefile as for source files in other > > subdirectories. > > The thinking behind the split is to avoid any merge conflicts as the > gvt/ subdirectory is handled through separate pull request flow and > are note part of drm-tip. Oh? In that case can we eventually move the VFIO mdev driver to drivers/vfio/mdev/intel_gvt/ please? Jason
Re: [PATCH 02/29] drm/i915/gvt: integrate into the main Makefile
Quoting Christoph Hellwig (2021-11-02 09:05:34) > Remove the separately included Makefile and just use the relative > reference from the main i915 Makefile as for source files in other > subdirectories. The thinking behind the split is to avoid any merge conflicts as the gvt/ subdirectory is handled through separate pull request flow and are note part of drm-tip. The other subdirectories are part of drm-intel-next/drm-intel-gt-next and are part of drm-tip. So I would rather still see the Makefile live in gvt/ directory. Regards, Joonas > Signed-off-by: Christoph Hellwig > --- > drivers/gpu/drm/i915/Makefile | 29 - > drivers/gpu/drm/i915/gvt/Makefile | 9 - > drivers/gpu/drm/i915/gvt/trace.h | 2 +- > 3 files changed, 25 insertions(+), 15 deletions(-) > delete mode 100644 drivers/gpu/drm/i915/gvt/Makefile > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 335ba9f43d8f7..63523032eea26 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -295,11 +295,30 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \ > > # virtual gpu code > i915-y += i915_vgpu.o > - > -ifeq ($(CONFIG_DRM_I915_GVT),y) > -i915-y += intel_gvt.o > -include $(src)/gvt/Makefile > -endif > +i915-$(CONFIG_DRM_I915_GVT) += \ > + intel_gvt.o \ > + gvt/gvt.o \ > + gvt/aperture_gm.o \ > + gvt/handlers.o \ > + gvt/vgpu.o \ > + gvt/trace_points.o \ > + gvt/firmware.o \ > + gvt/interrupt.o \ > + gvt/gtt.o \ > + gvt/cfg_space.o \ > + gvt/opregion.o \ > + gvt/mmio.o \ > + gvt/display.o \ > + gvt/edid.o \ > + gvt/execlist.o \ > + gvt/scheduler.o \ > + gvt/sched_policy.o \ > + gvt/mmio_context.o \ > + gvt/cmd_parser.o \ > + gvt/debugfs.o \ > + gvt/fb_decoder.o \ > + gvt/dmabuf.o \ > + gvt/page_track.o > > obj-$(CONFIG_DRM_I915) += i915.o > obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o > diff --git a/drivers/gpu/drm/i915/gvt/Makefile > b/drivers/gpu/drm/i915/gvt/Makefile > deleted file mode 100644 > index ea8324abc784a..0 > --- a/drivers/gpu/drm/i915/gvt/Makefile > +++ /dev/null > @@ -1,9 +0,0 @@ > -# SPDX-License-Identifier: GPL-2.0 > -GVT_DIR := gvt > -GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o > firmware.o \ > - interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ > - execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o > debugfs.o \ > - fb_decoder.o dmabuf.o page_track.o > - > -ccflags-y += -I $(srctree)/$(src) -I > $(srctree)/$(src)/$(GVT_DIR)/ > -i915-y += $(addprefix $(GVT_DIR)/, > $(GVT_SOURCE)) > diff --git a/drivers/gpu/drm/i915/gvt/trace.h > b/drivers/gpu/drm/i915/gvt/trace.h > index 6d787750d279f..348f57f8301db 100644 > --- a/drivers/gpu/drm/i915/gvt/trace.h > +++ b/drivers/gpu/drm/i915/gvt/trace.h > @@ -379,5 +379,5 @@ TRACE_EVENT(render_mmio, > #undef TRACE_INCLUDE_PATH > #define TRACE_INCLUDE_PATH . > #undef TRACE_INCLUDE_FILE > -#define TRACE_INCLUDE_FILE trace > +#define TRACE_INCLUDE_FILE gvt/trace > #include > -- > 2.30.2 >
[PATCH] kernel/locking: Use a pointer in ww_mutex_trylock().
mutex_acquire_nest() expects a pointer, pass the pointer. Fixes: 12235da8c80a1 ("kernel/locking: Add context to ww_mutex_trylock()") Signed-off-by: Sebastian Andrzej Siewior --- Not sure why I haven't seen this earlier… kernel/locking/ww_rt_mutex.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/locking/ww_rt_mutex.c b/kernel/locking/ww_rt_mutex.c index 0e00205cf467a..d1473c624105c 100644 --- a/kernel/locking/ww_rt_mutex.c +++ b/kernel/locking/ww_rt_mutex.c @@ -26,7 +26,7 @@ int ww_mutex_trylock(struct ww_mutex *lock, struct ww_acquire_ctx *ww_ctx) if (__rt_mutex_trylock(&rtm->rtmutex)) { ww_mutex_set_context_fastpath(lock, ww_ctx); - mutex_acquire_nest(&rtm->dep_map, 0, 1, ww_ctx->dep_map, _RET_IP_); + mutex_acquire_nest(&rtm->dep_map, 0, 1, &ww_ctx->dep_map, _RET_IP_); return 1; } -- 2.33.1
[PATCH v3] media: mtk-vcodec: Align width and height to 64 bytes
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 Acked-by: Nicolas Dufresne Tested-by: Steve Cho --- 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_DISABLED 0x10 #define VCODEC_DEC_4K_CODED_WIDTH 4096U #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 = -- 2.25.1
[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 #12 from Lang Yu (lang...@amd.com) --- (In reply to Christian König from comment #11) > Well it's really appreciated that you are looking into this. > > One thing we might want to do is to move the warning in dma_buf_release(): > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 3f63d58bf68a..6ecc01585cf4 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -75,6 +75,7 @@ static void dma_buf_release(struct dentry *dentry) > * dma-buf while still having pending operation to the buffer. > */ > BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); > + WARN_ON(!list_empty(&dmabuf->attachments)); > > dma_buf_stats_teardown(dmabuf); > dmabuf->ops->release(dmabuf); > @@ -82,7 +83,6 @@ static void dma_buf_release(struct dentry *dentry) > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > - WARN_ON(!list_empty(&dmabuf->attachments)); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf); > > This way users get the dma-buf warning first and maybe a bit less confused. The warning was just merged into mainline 5.15.0 on Tue Nov 2 16:47:49 2021(commit 56d33754481f). Not sure Erhard F.'s build contains this warning. And we can also add a debug WARN() into amdgpu_dma_buf_pin() to see who pinned dma_buf. -- 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 4/5] drm: Add a drm_drv_enabled() helper function
Hello Jani, On 11/4/21 12:10, Jani Nikula wrote: > On Wed, 03 Nov 2021, Thomas Zimmermann wrote: >> 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). > > Where and how exactly? This is why we need to see the patch using the > function. A patch is worth a thousand words. ;) > Thomas suggested to squash patches #4 and #5 so I'll do that when posting v2. > See current vgacon_text_force() usage in i915/i915_module.c. It happens > way before anything related to pci or drm driver. Why bother with the > complicated setup and teardown of stuff if you can bail out earlier? > Yes, most drivers use vgacon_text_force() in their module init callback. The ones that do in their probe function are drivers that don't have a module init/exit but just use the module_platform_driver() macro. I won't modify that and will keep the bail in the same place that the drivers already do. I hope to have time and post a new revision today. > > BR, > Jani. > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] video: udlfb: replace snprintf in show functions with sysfs_emit
From: Jing Yao coccicheck complains about the use of snprintf() in sysfs show functions: WARNING use scnprintf or sprintf Use sysfs_emit instead of scnprintf, snprintf or sprintf makes more sense. Reported-by: Zeal Robot Signed-off-by: Jing Yao --- drivers/video/fbdev/udlfb.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c index b9cdd02c1000..90f48b71fd8f 100644 --- a/drivers/video/fbdev/udlfb.c +++ b/drivers/video/fbdev/udlfb.c @@ -1426,7 +1426,7 @@ static ssize_t metrics_bytes_rendered_show(struct device *fbdev, struct device_attribute *a, char *buf) { struct fb_info *fb_info = dev_get_drvdata(fbdev); struct dlfb_data *dlfb = fb_info->par; - return snprintf(buf, PAGE_SIZE, "%u\n", + return sysfs_emit(buf, "%u\n", atomic_read(&dlfb->bytes_rendered)); } @@ -1434,7 +1434,7 @@ static ssize_t metrics_bytes_identical_show(struct device *fbdev, struct device_attribute *a, char *buf) { struct fb_info *fb_info = dev_get_drvdata(fbdev); struct dlfb_data *dlfb = fb_info->par; - return snprintf(buf, PAGE_SIZE, "%u\n", + return sysfs_emit(buf, "%u\n", atomic_read(&dlfb->bytes_identical)); } @@ -1442,7 +1442,7 @@ static ssize_t metrics_bytes_sent_show(struct device *fbdev, struct device_attribute *a, char *buf) { struct fb_info *fb_info = dev_get_drvdata(fbdev); struct dlfb_data *dlfb = fb_info->par; - return snprintf(buf, PAGE_SIZE, "%u\n", + return sysfs_emit(buf, "%u\n", atomic_read(&dlfb->bytes_sent)); } @@ -1450,7 +1450,7 @@ static ssize_t metrics_cpu_kcycles_used_show(struct device *fbdev, struct device_attribute *a, char *buf) { struct fb_info *fb_info = dev_get_drvdata(fbdev); struct dlfb_data *dlfb = fb_info->par; - return snprintf(buf, PAGE_SIZE, "%u\n", + return sysfs_emit(buf, "%u\n", atomic_read(&dlfb->cpu_kcycles_used)); } -- 2.25.1
Re: [RESEND PATCH 4/5] drm: Add a drm_drv_enabled() helper function
On Wed, 03 Nov 2021, Thomas Zimmermann wrote: > 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). Where and how exactly? This is why we need to see the patch using the function. A patch is worth a thousand words. ;) See current vgacon_text_force() usage in i915/i915_module.c. It happens way before anything related to pci or drm driver. Why bother with the complicated setup and teardown of stuff if you can bail out earlier? BR, Jani. > > 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 >> -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v3] drm/bridge: analogix_dp: Make PSR-exit block less
HI Brian, On Wed, 3 Nov 2021 at 21:52, 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 > 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 > Applied to drm-misc-next
[PATCH] drm/bridge: parade-ps8640: Assign drm_device to dp aux channel
As it was done with other bridge drivers and to solve a warning coming from drm_dp_aux_register(), add a backpointer to drm_device in the drm_dp_aux that we're registering. Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/bridge/parade-ps8640.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 191cc196c9d1..20d8e606d543 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -455,6 +455,7 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, goto err_dsi_attach; } + ps_bridge->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&ps_bridge->aux); if (ret) { dev_err(dev, "failed to register DP AUX channel: %d\n", ret); -- 2.33.1
Re: [PATCH v12 0/4] Add MIPI rx DPI support
Hey Xin, Applied to drm-misc-next. The way this series was submitted to the mailing list is not correct and is breaking a lot of tooling. It seems like you used git send-email, but the individual patches of the series are not connected properly and both b4 and the patchwork tools are not able to handle this series properly. Please try to use git send-email along the lines of this: git send-email -$NBR_PATCHES_IN_SERIES -v$VERSION_OF_SERIES --annotate --to= Rob.
[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 #11 from Christian König (christian.koe...@amd.com) --- Well it's really appreciated that you are looking into this. One thing we might want to do is to move the warning in dma_buf_release(): diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 3f63d58bf68a..6ecc01585cf4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -75,6 +75,7 @@ static void dma_buf_release(struct dentry *dentry) * dma-buf while still having pending operation to the buffer. */ BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); + WARN_ON(!list_empty(&dmabuf->attachments)); dma_buf_stats_teardown(dmabuf); dmabuf->ops->release(dmabuf); @@ -82,7 +83,6 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); - WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); This way users get the dma-buf warning first and maybe a bit less confused. -- 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 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) v5: - Fix an issue where we, if the dependency was already signaled, might end up waiting for a memcpy fence that would never signal. Signed-off-by: Thomas Hellström Reviewed-by: Matthew Auld --- drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 324 +++--- drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 5 + .../drm/i915/gem/selftests/i915_gem_migrate.c | 24 +- 3 files changed, 297 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..e9b1c23cacc0 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); +
[PATCH v5 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 v5 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) v5: - Fix an issue where we might end up waiting for a fence that would never signal. 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 | 522 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 43 ++ .../drm/i915/gem/selftests/i915_gem_migrate.c | 24 +- 6 files changed, 672 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
[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 #10 from Lang Yu (lang...@amd.com) --- (In reply to Christian König from comment #9) > (In reply to Lang Yu from comment #8) > > (In reply to Christian König from comment #7) > > > Yeah, that won't work. As far as I can see the problem is not inside > > amdgpu, > > > but rather inside the driver which is importing buffers from amdgpu. > > > > At least, we should call drm_prime_gem_destroy() to detach dma-buf(if > > exists) before WARN_ON_ONCE(bo->pin_count). > > Nope, that's incorrect. You are mixing things up here. > > This is for the case when amdgpu imports a buffer, but the warning happens > when amdgpu exports a buffer. > > And on import you indeed only want to drop the attachment after the BO is > really destroyed or not when the GEM handle is destroyed. Otherwise you > could potentially unmap memory while it is still used by the hardware. > > > And do you think if clients don't unmap/detach amdgpu dma-buf properly, > > should amdgpu do that work? Thanks! > > No. That rather looks like the importer is messing up some reference count > and forgets to destroy the attachment before the dma-buf. There is > absolutely nothing the exporter can do in that situation. > > There is the slightly chance that the bug is indeed somewhere inside amdgpu > or the dma-buf framework itself (Michel and I are huntin a similar issue at > the moment), but it does work with other driver combinations. Thanks for your clarification. Seems hard to reproduce the issue. -- 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 v8 00/10] drm/vc4: hdmi: Support the 4k @ 60Hz modes
On Mon, 25 Oct 2021 17:28:53 +0200, Maxime Ripard wrote: > Here is a series that enables the higher resolutions on the HDMI0 Controller > found in the BCM2711 (RPi4). > > In order to work it needs a few adjustments to config.txt, most notably to > enable the enable_hdmi_4kp60 option. > > Let me know what you think, > Maxime > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: [PATCH 10/10] drm/i915: Add privacy-screen support (v3)
Hi, On 11/4/21 00:16, Rajat Jain wrote: > Hello Hans, > > Thanks a lot for working on this diligently and getting almost all of > it finally merged! > > On Wed, Oct 13, 2021 at 2:59 PM Ville Syrjälä > wrote: >> >> On Tue, Oct 05, 2021 at 10:23:22PM +0200, Hans de Goede wrote: >>> Add support for eDP panels with a built-in privacy screen using the >>> new drm_privacy_screen class. >>> >>> Changes in v3: >>> - Move drm_privacy_screen_get() call to intel_ddi_init_dp_connector() >>> >>> Changes in v2: >>> - Call drm_connector_update_privacy_screen() from >>> intel_enable_ddi_dp() / intel_ddi_update_pipe_dp() instead of adding a >>> for_each_new_connector_in_state() loop to intel_atomic_commit_tail() >>> - Move the probe-deferral check to the intel_modeset_probe_defer() helper >>> >>> Signed-off-by: Hans de Goede >>> --- >>> drivers/gpu/drm/i915/display/intel_atomic.c | 1 + >>> drivers/gpu/drm/i915/display/intel_ddi.c | 16 >>> drivers/gpu/drm/i915/display/intel_display.c | 10 ++ >>> 3 files changed, 27 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c >>> b/drivers/gpu/drm/i915/display/intel_atomic.c >>> index b4e7ac51aa31..a62550711e98 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_atomic.c >>> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c >>> @@ -139,6 +139,7 @@ int intel_digital_connector_atomic_check(struct >>> drm_connector *conn, >>> new_conn_state->base.picture_aspect_ratio != >>> old_conn_state->base.picture_aspect_ratio || >>> new_conn_state->base.content_type != >>> old_conn_state->base.content_type || >>> new_conn_state->base.scaling_mode != >>> old_conn_state->base.scaling_mode || >>> + new_conn_state->base.privacy_screen_sw_state != >>> old_conn_state->base.privacy_screen_sw_state || >>> !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) >>> crtc_state->mode_changed = true; >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c >>> b/drivers/gpu/drm/i915/display/intel_ddi.c >>> index 0d4cf7fa8720..272714e07cc6 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c >>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >>> @@ -25,6 +25,7 @@ >>> * >>> */ >>> >>> +#include >>> #include >>> >>> #include "i915_drv.h" >>> @@ -2946,6 +2947,7 @@ static void intel_enable_ddi_dp(struct >>> intel_atomic_state *state, >>> if (port == PORT_A && DISPLAY_VER(dev_priv) < 9) >>> intel_dp_stop_link_train(intel_dp, crtc_state); >>> >>> + drm_connector_update_privacy_screen(conn_state); >>> intel_edp_backlight_on(crtc_state, conn_state); >>> >>> if (!dig_port->lspcon.active || dig_port->dp.has_hdmi_sink) >>> @@ -3161,6 +3163,7 @@ static void intel_ddi_update_pipe_dp(struct >>> intel_atomic_state *state, >>> intel_drrs_update(intel_dp, crtc_state); >>> >>> intel_backlight_update(state, encoder, crtc_state, conn_state); >>> + drm_connector_update_privacy_screen(conn_state); >>> } >>> >>> void intel_ddi_update_pipe(struct intel_atomic_state *state, >>> @@ -3979,6 +3982,19 @@ intel_ddi_init_dp_connector(struct >>> intel_digital_port *dig_port) >>> return NULL; >>> } >>> >>> + if (dig_port->base.type == INTEL_OUTPUT_EDP) { >> >> Connector type check would be a bit more consistent with what this is >> about I think. But there's is 1:1 correspondence with the encoder type >> for eDP so not a particularly important point. >> >> Reviewed-by: Ville Syrjälä > > I see only 8 out of 10 patches in this series were applied to drm-tip. > I'm curious if there is any reason for which the last 2 patches were > not applied: > > [Patch 9/10]: drm/i915: Add intel_modeset_probe_defer() helper > [Patch 10/10]: drm/i915: Add privacy-screen support (v3) > > I look forward to getting them merged so that I can use them. The main-parts of the patch-set were merged through drm-misc-next, the 2 i915 patches had a conflict there since the series was based on drm-tip and some of the surrounding i915 code had some small changes in drm-intel-next which was not in drm-misc-next yet. Once drm-intel-next merges in recent drm-misc-next changes (after the merge window closes) I will push the remaining 2 patches through drm-intel-next and then everything will be in drm-tip and on its way to 5.17 . Regards, Hans >>> + struct drm_device *dev = dig_port->base.base.dev; >>> + struct drm_privacy_screen *privacy_screen; >>> + >>> + privacy_screen = drm_privacy_screen_get(dev->dev, NULL); >>> + if (!IS_ERR(privacy_screen)) { >>> + >>> drm_connector_attach_privacy_screen_provider(&connector->base, >>> + >>> privacy_screen); >>> + } else if (PTR_ERR(privacy_screen) != -ENODEV) { >>> + drm_warn(dev, "Error getting privacy-screen\n"); >>>
Re: [PATCH 8/8] drm/amdgpu: add drm buddy support to amdgpu
Am 04.11.21 um 09:49 schrieb Matthew Auld: On 04/11/2021 07:34, Christian König wrote: Am 03.11.21 um 20:25 schrieb Matthew Auld: 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? Nope it isn't, but does that function really calls kmalloc()? It calls kmem_cache_zalloc(..., GFP_KERNEL) Oh that's bad. In this case we either need a mutex here or some other approach to avoid the allocation. Christian. Christian. + 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 3/3] backlight: lp855x: Add support ACPI enumeration
Hi, On 11/4/21 09:29, Lee Jones wrote: > On Wed, 03 Nov 2021, 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). > > Also the merge-window is open, so this is headed for v5.17. Right, I didn't expect anything else. Regards, Hans
Re: [PATCH 8/8] drm/amdgpu: add drm buddy support to amdgpu
On 04/11/2021 07:34, Christian König wrote: Am 03.11.21 um 20:25 schrieb Matthew Auld: 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? Nope it isn't, but does that function really calls kmalloc()? It calls kmem_cache_zalloc(..., GFP_KERNEL) Christian. + 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 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate
Hi Ville, On Wed, Nov 03, 2021 at 08:05:16PM +0200, Ville Syrjälä wrote: > 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. Patch 2 introduces something along those lines. It doesn't cover everything though, we're using this define in vc4 to limit the available modes in mode_valid on HDMI controllers not 4k-capable We could probably do better on the name, but I still believe a define like this would be valuable. Maxime signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table
John Harrison writes: > On 11/3/2021 14:38, Jordan Justen wrote: >> 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. So, guc literally reads this info from the hardware verbatim? Then gives it to i915 so i915 can give it to UMDs? Otherwise, it seems like a table in software. Anyway... >> 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. Obviously not what should be done, but apparently all i915 is willing to do. >> 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. So, why can't i915 define this extremely simple (apparently unchangeable) blob format, and thereby guarantee that it will have to insulate UMDs if the format changes by making a different query item? It ought to be made painful for everyone if this blob format changes. Hopefully the format will basically never change. (Even if new keys/values might be added.) Further, it seems there is an implication that the keys will always be backward compatible. Is that true, and if so, how can there be harm in i915 enumerating the "known" ones? >> 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. i915 can't/won't say anything about it, but look at this unmerged IGT test? In the next sentence you'll say, but don't count on that because IGT really has no control over it. :) > 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? :) Apparently all the data for this spec is "available" (in an unmerged IGT patch), but am I correct in assuming that no actual spec timeframe is defined? -Jordan
Re: [RFC v2 02/22] drm: Add Enhanced Gamma and color lut range attributes
On Wed, 3 Nov 2021 11:08:13 -0400 Harry Wentland wrote: > 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. Hi Harry, I think you just would not. Conceptually an SDR plane gets its very own LUT that converts the FB [0.0, 1.0] range to any appropriate [a >= 0.0, b <= 1.0] range in HDR. This is purely conceptual, in the terms of the abstract KMS color pipeline, where [0.0, 1.0] is always the full dynamic range at any point of the pipeline, meaning it is relative to its placement in the pipeline. If you want to use values >1.0 in hw, you can do so under the hood. At lea
Re: Re: [PATCH] drm: mxsfb: Check NULL pointer
On 11/3/21 8:58 AM, Marek Vasut 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 ? As fas as I am concerned, 'connector_list' in the drm_connector_list_iter_next() is initialized in the drm_mode_config_init(), which is 'connector_list->next = connector_list'. And therefore, the check in drm_connector_list_iter_next() that 'if (lhead->next == &config->connector_list)' is directly satisfied and returns NULL. I am not sure wheter it is designed on purpose. If not, please fix it.
Re: [PATCH v2 3/3] backlight: lp855x: Add support ACPI enumeration
On Wed, 03 Nov 2021, 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). Also the merge-window is open, so this is headed for v5.17. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [Linaro-mm-sig] [PATCH] dma-buf/poll: Get a file reference for outstanding fence callbacks
Am 03.11.21 um 15:50 schrieb Michel Dänzer: 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.gnome.org%2FGNOME%2Fmutter%2F-%2Fmerge_requests%2F1880&data=04%7C01%7Cchristian.koenig%40amd.com%7C8d930ab39011481a839c08d99ed95755%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637715479787056688%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=SjxSZIsWkP7ru1iHyL0IY9hN9882ENv7Cy38vzOtqyc%3D&reserved=0 . 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? I've just pushed it to drm-misc-next-fixes since it won't even apply to drm-misc-fixes. Could be that we get requests to backport this because of the CC stable. Christian.
[Bug 214921] amdgpu hangs HP Laptop on shutdown
https://bugzilla.kernel.org/show_bug.cgi?id=214921 spassw...@web.de changed: What|Removed |Added Regression|No |Yes --- Comment #1 from spassw...@web.de --- It turns out that ressuming from suspend has been broken a long time on the above hardware. The last Kernel where it works is 5.12. The first commit where resuming from suspend leads to screen corruption is 4588f7b7dd5f09e70b6e223490a0d054c3d64071 -- 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 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 #9 from Christian König (christian.koe...@amd.com) --- (In reply to Lang Yu from comment #8) > (In reply to Christian König from comment #7) > > Yeah, that won't work. As far as I can see the problem is not inside > amdgpu, > > but rather inside the driver which is importing buffers from amdgpu. > > At least, we should call drm_prime_gem_destroy() to detach dma-buf(if > exists) before WARN_ON_ONCE(bo->pin_count). Nope, that's incorrect. You are mixing things up here. This is for the case when amdgpu imports a buffer, but the warning happens when amdgpu exports a buffer. And on import you indeed only want to drop the attachment after the BO is really destroyed or not when the GEM handle is destroyed. Otherwise you could potentially unmap memory while it is still used by the hardware. > And do you think if clients don't unmap/detach amdgpu dma-buf properly, > should amdgpu do that work? Thanks! No. That rather looks like the importer is messing up some reference count and forgets to destroy the attachment before the dma-buf. There is absolutely nothing the exporter can do in that situation. There is the slightly chance that the bug is indeed somewhere inside amdgpu or the dma-buf framework itself (Michel and I are huntin a similar issue at the moment), but it does work with other driver combinations. -- 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 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 #8 from Lang Yu (lang...@amd.com) --- (In reply to Christian König from comment #7) > Yeah, that won't work. As far as I can see the problem is not inside amdgpu, > but rather inside the driver which is importing buffers from amdgpu. At least, we should call drm_prime_gem_destroy() to detach dma-buf(if exists) before WARN_ON_ONCE(bo->pin_count). And do you think if clients don't unmap/detach amdgpu dma-buf properly, should amdgpu do that work? Thanks! -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.