Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Hello again after easter, I have looked little bit more at sn65* driver and its application to have better background. I miss only info what panel do you have, how it is enabled/power controlled. W dniu 01.04.2021 o 16:57, Doug Anderson pisze: > Hi, > > On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda wrote: >> >> W dniu 31.03.2021 o 16:48, Doug Anderson pisze: >>> Hi, >>> >>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda wrote: >>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: >>>>> eDP panels won't provide their EDID unless they're powered on. Let's >>>>> chain a power-on before we read the EDID. This roughly matches what >>>>> was done in 'parade-ps8640.c'. >>>>> >>>>> NOTE: The old code attempted to call pm_runtime_get_sync() before >>>>> reading the EDID. While that was enough to power the bridge chip on, >>>>> it wasn't enough to talk to the panel for two reasons: >>>>> 1. Since we never ran the bridge chip's pre-enable then we never set >>>>> the bit to ignore HPD. This meant the bridge chip didn't even _try_ >>>>> to go out on the bus and communicate with the panel. >>>>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read >>>>> if the panel wasn't on. >>>>> >>>>> One thing that's a bit odd here is taking advantage of the EDID that >>>>> the core might have cached for us. See the patch ("drm/edid: Use the >>>>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache >>>>> by: >>>>> - Instantly failing aux transfers if we're not powered. >>>>> - If the first read of the EDID fails we try again after powering. >>>>> >>>>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC") >>>>> Signed-off-by: Douglas Anderson >>>>> --- >>>>> Depending on what people think of the other patches in this series, >>>>> some of this could change. >>>>> - If everyone loves the "runtime PM" in the panel driver then we >>>>> could, in theory, put the pre-enable chaining straight in the "aux >>>>> transfer" function. >>>>> - If everyone hates the EDID cache moving to the core then we can >>>>> avoid some of the awkward flow of things and keep the EDID cache in >>>>> the sn65dsi86 driver. >>>> I wonder if this shouldn't be solved in the core - ie caller of >>>> get_modes callback should be responsible for powering up the pipeline, >>>> otherwise we need to repeat this stuff in every bridge/panel driver. >>>> >>>> Any thoughts? >>> Yeah, I did look at this a little bit. Presumably it would only make >>> sense to do it for eDP connections since: >>> >>> a) The concept of reading an EDID doesn't make sense for things like MIPI. >> I guess you mean MIPI DSI > Yes, sorry! I'll try to be more clear. > > >> and yes I agree, more generally it usually(!) >> doesn't make sense for any setup with fixed display panel. >> >> On the other hand there are DSI/HDMI or DSI/DP adapters which usually >> have EDID reading logic. >> >> And the concept makes sense for most connectors accepting external >> displays: HDMI, DP, MHL, VGA... > So, actually, IMO the concept doesn't make sense for anything with an > external connector. Here's the logic for a handful of connectors: > > 1. MIPI DSI: there is no EDID so this doesn't make sense. > > 2. An external connector (HDMI, DP, etc): the display that's plugged > in is externally powered so doesn't need us to power it up to read the > EDID. By definition, when the HPD signal is asserted then it's OK to > read the EDID and we don't even know if a display is plugged in until > HPD is asserted. Thus no special power sequencing is needed to read > the EDID. (Yes, we need to make sure that the eDP controller itself > is powered, but that doesn't seem like it's the core's business). Not true IMO, even if external device is powered on, you must enable EDID-reader logic. I guess it is not uncommon to have different power states for EDID reading and bridge/panel pre-enablement (especially in embedded world). In fact there are setups where EDID-reader is totally different device than the bridge itself, and these devices should be
Re: [PATCH v2 0/8] drm/bridge: Make panel and bridge probe order consistent
Hi Jagan, W dniu 09.08.2021 o 10:00, Jagan Teki pisze: > Hi Andrzej, > > On Wed, Aug 4, 2021 at 7:48 PM a.hajda wrote: >> Hi Maxime, >> >> I have been busy with other tasks, and I did not follow the list last >> time, so sorry for my late response. >> >> On 28.07.2021 15:32, Maxime Ripard wrote: >>> Hi, >>> >>> We've encountered an issue with the RaspberryPi DSI panel that prevented the >>> whole display driver from probing. >>> >>> The issue is described in detail in the commit 7213246a803f ("drm/vc4: dsi: >>> Only register our component once a DSI device is attached"), but the basic >>> idea >>> is that since the panel is probed through i2c, there's no synchronization >>> between its probe and the registration of the MIPI-DSI host it's attached >>> to. >>> >>> We initially moved the component framework registration to the MIPI-DSI Host >>> attach hook to make sure we register our component only when we have a DSI >>> device attached to our MIPI-DSI host, and then use lookup our DSI device in >>> our >>> bind hook. >>> >>> However, all the DSI bridges controlled through i2c are only registering >>> their >>> associated DSI device in their bridge attach hook, meaning with our change >> >> I guess this is incorrect. I have promoted several times the pattern >> that device driver shouldn't expose its interfaces (for example >> component_add, drm_panel_add, drm_bridge_add) until it gathers all >> required dependencies. In this particular case bridges should defer >> probe until DSI bus becomes available. I guess this way the patch you >> reverts would work. >> >> I advised few times this pattern in case of DSI hosts, apparently I >> didn't notice the similar issue can appear in case of bridges. Or there >> is something I have missed??? > Look like Maxime is correct. I2C based DSI bridge will get probe > during bridge_attach which usually called from bridge driver > bridge_attach call. Non-I2C bridges and DSI panels will get probe > during host.attach. > We do get similar situation for dw-mipi-dsi bridges, where icn6211 > bridge is not I2C-based bridge and it gets probed in host_attach and > sn65dsi83 is I2C based bridge and it gets probed in bridge_attach. > > Here is the simple call trace we have observed with dw-mipi-dsi bridge > when all possible DSI device are trying to probe. > > 1. DSI panels and bridges will invoke the host attach > from probe in order to find the panel_or_bridge. > > chipone_probe() > dw_mipi_dsi_host_attach().start > dw_mipi_dsi_panel_or_bridge() > ...found the panel_or_bridge... > > ltdc_encoder_init().start > dw_mipi_dsi_bridge_attach().start > dw_mipi_dsi_host_attach().start > chipone_attach(). start > > chipone_attach(). done > dw_mipi_dsi_host_attach().done > dw_mipi_dsi_bridge_attach(). done > ltdc_encoder_init().done > > 2. I2C based DSI bridge will invoke the drm_bridge_attach > from bridge attach in order to find the panel_or_bridge. > > ltdc_encoder_init().start > dw_mipi_dsi_bridge_attach().start > dw_mipi_dsi_panel_or_bridge() > ...found the panel_or_bridge... > dw_mipi_dsi_host_attach().start > sn65dsi83_attach(). start > > sn65dsi83_attach(). done > dw_mipi_dsi_host_attach().done > dw_mipi_dsi_bridge_attach(). done > ltdc_encoder_init().done > > It is correct that the I2C-based bridges will attach the host via > mipi_dsi_attach in driver bridge API where as it done in probe for > Non-I2C bridges and DSI panels. The call order depends on the registration time of DSI host. In case of dw-mipi-dsi it is called from .component_bind callback (dsi_bind-> dsi_host_init -> mipi_dsi_host_register). And this is "the original sin" :) dw-mipi-dsi calls component_add without prior acquiring its dependency - drm_bridge and before DSI host registration. In such situation bridge author should follow this pattern and perform similar initialization: first drm_bridge_add, then mipi_dsi_attach. And now authors of bridges are in dead end in case they want their bridge/panel drivers cooperate with dw-mipi-dsi host (with pattern look for sink - bridge/panel, then register DSI bus) and with other DSI hosts (most of then use pattern - register DSI bus then look for the sink - panel or bridge). Quick look at the DSI hosts suggests the pattern get-sink-then-register-bus are used only by kirin/dw_drm_dsi.c and msm/dsi. All other DSI hosts uses apparently register-bus-then-get-sink pattern - as I said it was not profound analysis - just few greps of some keywords. >> Anyway there are already eleven(?) bridge drivers using this pattern. I >> wonder if fixing it would be difficult, or if it expose other issues??? >> The patches should be quite straightforward - move >> of_find_mip
Re: [PATCH v3 2/8] drm/bridge: Document the probe issue with MIPI-DSI bridges
Hi Maxime, On 23.08.2021 10:47, Maxime Ripard wrote: > Interactions between bridges, panels, MIPI-DSI host and the component > framework are not trivial and can lead to probing issues when > implementing a display driver. Let's document the various cases we need > too consider, and the solution to support all the cases. > > Signed-off-by: Maxime Ripard > --- > Documentation/gpu/drm-kms-helpers.rst | 6 +++ > drivers/gpu/drm/drm_bridge.c | 58 +++ > 2 files changed, 64 insertions(+) > > diff --git a/Documentation/gpu/drm-kms-helpers.rst > b/Documentation/gpu/drm-kms-helpers.rst > index 10f8df7aecc0..ec2f65b31930 100644 > --- a/Documentation/gpu/drm-kms-helpers.rst > +++ b/Documentation/gpu/drm-kms-helpers.rst > @@ -157,6 +157,12 @@ Display Driver Integration > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > :doc: display driver integration > > +Special Care with MIPI-DSI bridges > +-- > + > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c > + :doc: special care dsi > + > Bridge Operations > - > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index baff74ea4a33..794654233cf5 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -96,6 +96,64 @@ >* documentation of bridge operations for more details). >*/ > > +/** > + * DOC: special care dsi > + * > + * The interaction between the bridges and other frameworks involved in > + * the probing of the display driver and the bridge driver can be > + * challenging. Indeed, there's multiple cases that needs to be > + * considered: > + * > + * - The display driver doesn't use the component framework and isn't a > + * MIPI-DSI host. In this case, the bridge driver will probe at some > + * point and the display driver should try to probe again by returning > + * EPROBE_DEFER as long as the bridge driver hasn't probed. > + * > + * - The display driver doesn't use the component framework, but is a > + * MIPI-DSI host. The bridge device uses the MIPI-DCS commands to be > + * controlled. In this case, the bridge device is a child of the > + * display device and when it will probe it's assured that the display > + * device (and MIPI-DSI host) is present. The display driver will be > + * assured that the bridge driver is connected between the > + * &mipi_dsi_host_ops.attach and &mipi_dsi_host_ops.detach operations. > + * Therefore, it must run mipi_dsi_host_register() in its probe > + * function, and then run drm_bridge_attach() in its > + * &mipi_dsi_host_ops.attach hook. > + * > + * - The display driver uses the component framework and is a MIPI-DSI > + * host. The bridge device uses the MIPI-DCS commands to be > + * controlled. This is the same situation than above, and can run > + * mipi_dsi_host_register() in either its probe or bind hooks. > + * > + * - The display driver uses the component framework and is a MIPI-DSI > + * host. The bridge device uses a separate bus (such as I2C) to be > + * controlled. In this case, there's no correlation between the probe > + * of the bridge and display drivers, so care must be taken to avoid > + * an endless EPROBE_DEFER loop, with each driver waiting for the > + * other to probe. > + * > + * The ideal pattern to cover the last item (and all the others in the > + * display driver case) is to split the operations like this: > + * > + * - In the display driver must run mipi_dsi_host_register() and > + * component_add in its probe hook. It will make sure that the > + * MIPI-DSI host sticks around, and that the driver's bind can be > + * called. I guess component_add is leftover from previous iteration (as you wrote few lines below) component_add should be called from dsi host attach callback. > + * > + * - In its probe hook, the bridge driver must try to find its MIPI-DSI > + * host, register as a MIPI-DSI device and attach the MIPI-DSI device > + * to its host. The bridge driver is now functional. > + * > + * - In its &struct mipi_dsi_host_ops.attach hook, the display driver > + * can now add its component. Its bind hook will now be called and > + * since the bridge driver is attached and registered, we can now look > + * for and attach it. > + * > + * At this point, we're now certain that both the display driver and the > + * bridge driver are functional and we can't have a deadlock-like > + * situation when probing. > + */ > + Beside small mistake the whole patch looks OK for me. Maybe it would be worth to mention what is the real cause of this "special DSI case" - there is mutual dependency between two following entities in display chain: 1. display driver - it provides DSI bus, and requires drm_bridge or drm_panel provided by child device. 2. bridge or panel with DSI transport - it requires DSI bus provided by display driver, and provides drm_bridge or drm_panel interface required by displ
Re: [PATCH v3 1/8] drm/bridge: Add documentation sections
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze: > The bridge documentation overview is quite packed already, and we'll add > some more documentation that isn't part of an overview at all. > > Let's add some sections to the documentation to separate each bits. > > Reviewed-by: Sam Ravnborg > Signed-off-by: Maxime Ripard Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v3 3/8] drm/mipi-dsi: Create devm device registration
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze: > Devices that take their data through the MIPI-DSI bus but are controlled > through a secondary bus like I2C have to register a secondary device on > the MIPI-DSI bus through the mipi_dsi_device_register_full() function. > > At removal or when an error occurs, that device needs to be removed > through a call to mipi_dsi_device_unregister(). > > Let's create a device-managed variant of the registration function that > will automatically unregister the device at unbind. > > Signed-off-by: Maxime Ripard Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v3 4/8] drm/mipi-dsi: Create devm device attachment
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze: > Signed-off-by: Maxime Ripard Missing description. With this fixed: Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v3 6/8] drm/bridge: ps8640: Register and attach our DSI device at probe
W dniu 23.08.2021 o 10:47, Maxime Ripard pisze: > In order to avoid any probe ordering issue, the best practice is to move > the secondary MIPI-DSI device registration and attachment to the > MIPI-DSI host at probe time. Let's do this. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/bridge/parade-ps8640.c | 93 ++ > 1 file changed, 51 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c > b/drivers/gpu/drm/bridge/parade-ps8640.c > index 794c9516b05d..37f7d166a3c6 100644 > --- a/drivers/gpu/drm/bridge/parade-ps8640.c > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c > @@ -213,52 +213,10 @@ static int ps8640_bridge_attach(struct drm_bridge > *bridge, > enum drm_bridge_attach_flags flags) > { > struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); > - struct device *dev = &ps_bridge->page[0]->dev; > - struct device_node *in_ep, *dsi_node; > - struct mipi_dsi_device *dsi; > - struct mipi_dsi_host *host; > - int ret; > - const struct mipi_dsi_device_info info = { .type = "ps8640", > -.channel = 0, > -.node = NULL, > - }; > > if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) > return -EINVAL; > > - /* port@0 is ps8640 dsi input port */ > - in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); > - if (!in_ep) > - return -ENODEV; > - > - dsi_node = of_graph_get_remote_port_parent(in_ep); > - of_node_put(in_ep); > - if (!dsi_node) > - return -ENODEV; > - > - host = of_find_mipi_dsi_host_by_node(dsi_node); > - of_node_put(dsi_node); > - if (!host) > - return -ENODEV; > - > - dsi = devm_mipi_dsi_device_register_full(dev, host, &info); > - if (IS_ERR(dsi)) { > - dev_err(dev, "failed to create dsi device\n"); > - ret = PTR_ERR(dsi); > - return ret; > - } > - > - ps_bridge->dsi = dsi; > - > - dsi->host = host; > - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | > - MIPI_DSI_MODE_VIDEO_SYNC_PULSE; > - dsi->format = MIPI_DSI_FMT_RGB888; > - dsi->lanes = DP_NUM_LANES; > - ret = devm_mipi_dsi_attach(dev, dsi); > - if (ret) > - return ret; > - > /* Attach the panel-bridge to the dsi bridge */ > return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge, >&ps_bridge->bridge, flags); > @@ -305,6 +263,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs > = { > .pre_enable = ps8640_pre_enable, > }; > > +static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 > *ps_bridge) > +{ > + struct device_node *in_ep, *dsi_node; > + struct mipi_dsi_device *dsi; > + struct mipi_dsi_host *host; > + int ret; > + const struct mipi_dsi_device_info info = { .type = "ps8640", > +.channel = 0, > +.node = NULL, > + }; > + > + /* port@0 is ps8640 dsi input port */ > + in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); > + if (!in_ep) > + return -ENODEV; > + > + dsi_node = of_graph_get_remote_port_parent(in_ep); > + of_node_put(in_ep); > + if (!dsi_node) > + return -ENODEV; > + > + host = of_find_mipi_dsi_host_by_node(dsi_node); > + of_node_put(dsi_node); > + if (!host) > + return -EPROBE_DEFER; > + > + dsi = devm_mipi_dsi_device_register_full(dev, host, &info); > + if (IS_ERR(dsi)) { > + dev_err(dev, "failed to create dsi device\n"); > + return PTR_ERR(dsi); > + } > + > + ps_bridge->dsi = dsi; > + > + dsi->host = host; > + dsi->mode_flags = MIPI_DSI_MODE_VIDEO | > + MIPI_DSI_MODE_VIDEO_SYNC_PULSE; > + dsi->format = MIPI_DSI_FMT_RGB888; > + dsi->lanes = DP_NUM_LANES; > + > + ret = devm_mipi_dsi_attach(dev, dsi); > + if (ret) > + return ret; > + > + return 0; > +} > + > static int ps8640_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > @@ -371,6 +376,10 @@ static int ps8640_probe(struct i2c_client *client) > > drm_bridge_add(&ps_bridge->bridge); > > + ret = ps8640_bridge_host_attach(dev, ps_bridge); > + if (ret) > + return ret; I do not see drm_bridge_remove on error path, the same for sn65dsi83. Regards Andrzej > + > return 0; > } >
Re: [PATCH v2 01/14] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi Douglas, W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > The drm_bridge_chain_pre_enable() is not the proper opposite of > drm_bridge_chain_post_disable(). It continues along the chain to > _before_ the starting bridge. Let's fix that. > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > Signed-off-by: Douglas Anderson > --- > > (no changes since v1) > > drivers/gpu/drm/drm_bridge.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 64f0effb52ac..044acd07c153 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge > *bridge) > list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { > if (iter->funcs->pre_enable) > iter->funcs->pre_enable(iter); > + > + if (iter == bridge) > + break; Looking at the bridge chaining code always makes me sick :) but beside this the change looks correct, and follows drm_atomic_bridge_chain_pre_enable. Reviewed-by: Andrzej Hajda Regards Andrzej > } > } > EXPORT_SYMBOL(drm_bridge_chain_pre_enable); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 02/14] drm/bridge: ti-sn65dsi86: Simplify refclk handling
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > The clock framework makes it simple to deal with an optional clock. > You can call clk_get_optional() and if the clock isn't specified it'll > just return NULL without complaint. It's valid to pass NULL to > enable/disable/prepare/unprepare. Let's make use of this to simplify > things a tiny bit. > > Signed-off-by: Douglas Anderson > Reviewed-by: Robert Foss > Reviewed-by: Bjorn Andersson > Reviewed-by: Stephen Boyd > Reviewed-by: Laurent Pinchart Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > Changes in v2: > - Removed 2nd paragraph in commit message. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++ > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 88df4dd0f39d..96fe8f2c0ea9 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1275,14 +1275,9 @@ static int ti_sn_bridge_probe(struct i2c_client > *client, > return ret; > } > > - pdata->refclk = devm_clk_get(pdata->dev, "refclk"); > - if (IS_ERR(pdata->refclk)) { > - ret = PTR_ERR(pdata->refclk); > - if (ret == -EPROBE_DEFER) > - return ret; > - DRM_DEBUG_KMS("refclk not found\n"); > - pdata->refclk = NULL; > - } > + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk"); > + if (IS_ERR(pdata->refclk)) > + return PTR_ERR(pdata->refclk); > > ret = ti_sn_bridge_parse_dsi_host(pdata); > if (ret) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 03/14] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > A random comment inside a function had "/**" in front of it. That > doesn't make sense. Remove. > > Signed-off-by: Douglas Anderson Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 96fe8f2c0ea9..76f43af6735d 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) > /* set dsi clk frequency value */ > ti_sn_bridge_set_dsi_rate(pdata); > > - /** > + /* >* The SN65DSI86 only supports ASSR Display Authentication method and >* this method is enabled by default. An eDP panel must support this >* authentication method. We need to enable this method in the eDP panel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 04/14] drm/bridge: ti-sn65dsi86: Reorder remove()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > Let's make the remove() function strictly the reverse of the probe() > function so it's easier to reason about. > > NOTES: > - The MIPI calls probably belong in detach() but will be moved in a >separate patch. The mipi is incorrectly handled already - mipi devices are searched after bridge registration - it should be reverse, there is comment in the driver that it is due to some dsi hosts, maybe it would be better to fix it there instead of conserve this bad design. > - The cached EDID freeing isn't actually part of probe but needs to be >in remove to avoid orphaning memory until better handling of the >EDID happens. > This patch was created by code inspection and should move us closer to > a proper remove. > > Signed-off-by: Douglas Anderson > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 76f43af6735d..c006678c9921 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client > *client) > if (!pdata) > return -EINVAL; > > - kfree(pdata->edid); > - ti_sn_debugfs_remove(pdata); > - > - of_node_put(pdata->host_node); > - > - pm_runtime_disable(pdata->dev); > - > if (pdata->dsi) { > mipi_dsi_detach(pdata->dsi); > mipi_dsi_device_unregister(pdata->dsi); > } > > + kfree(pdata->edid); > + > + ti_sn_debugfs_remove(pdata); > + > drm_bridge_remove(&pdata->bridge); > > + pm_runtime_disable(pdata->dev); > + > + of_node_put(pdata->host_node); > + Looks good. Reviewed-by: Andrzej Hajda Regards Andrzej > return 0; > } > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 05/14] drm/bridge: ti-sn65dsi86: Move MIPI detach() / unregister() to detach()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > The register() / attach() for MIPI happen in the bridge's > attach(). That means that the inverse belongs in the bridge's > detach(). As I commented in previous patch, it would be better to fix mipi/bridge registration order in host and this driver. Have you considered this? Regards Andrzej > > Signed-off-by: Douglas Anderson > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index c006678c9921..e8e523b3a16b 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -437,7 +437,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, > > static void ti_sn_bridge_detach(struct drm_bridge *bridge) > { > - drm_dp_aux_unregister(&bridge_to_ti_sn_bridge(bridge)->aux); > + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); > + > + > + if (pdata->dsi) { > + mipi_dsi_detach(pdata->dsi); > + mipi_dsi_device_unregister(pdata->dsi); > + } > + > + drm_dp_aux_unregister(&pdata->aux); > } > > static void ti_sn_bridge_disable(struct drm_bridge *bridge) > @@ -1315,11 +1323,6 @@ static int ti_sn_bridge_remove(struct i2c_client > *client) > if (!pdata) > return -EINVAL; > > - if (pdata->dsi) { > - mipi_dsi_detach(pdata->dsi); > - mipi_dsi_device_unregister(pdata->dsi); > - } > - > kfree(pdata->edid); > > ti_sn_debugfs_remove(pdata); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 06/14] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > We prepared the panel in pre_enable() so we should unprepare it in > post_disable() to match. > > This becomes important once we start using pre_enable() and > post_disable() to make sure things are powered on (and then off again) > when reading the EDID. > > Signed-off-by: Douglas Anderson Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index e8e523b3a16b..50a52af8e39f 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -460,8 +460,6 @@ static void ti_sn_bridge_disable(struct drm_bridge > *bridge) > regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0); > /* disable DP PLL */ > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0); > - > - drm_panel_unprepare(pdata->panel); > } > > static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata) > @@ -877,6 +875,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge > *bridge) > { > struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); > > + drm_panel_unprepare(pdata->panel); > + > clk_disable_unprepare(pdata->refclk); > > pm_runtime_put_sync(pdata->dev); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 07/14] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > If we just leave the detect() function as NULL then the upper layers > assume we're always connected. There's no reason for a stub. > > Signed-off-by: Douglas Anderson Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 50a52af8e39f..a0a00dd1187c 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs > ti_sn_bridge_connector_helper_funcs = { > .mode_valid = ti_sn_bridge_connector_mode_valid, > }; > > -static enum drm_connector_status > -ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force) > -{ > - /** > - * TODO: Currently if drm_panel is present, then always > - * return the status as connected. Need to add support to detect > - * device state for hot pluggable scenarios. > - */ > - return connector_status_connected; > -} > - > static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = { > .fill_modes = drm_helper_probe_single_connector_modes, > - .detect = ti_sn_bridge_connector_detect, > .destroy = drm_connector_cleanup, > .reset = drm_atomic_helper_connector_reset, > .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 08/14] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > As of commit 5186421cbfe2 ("drm: Introduce epoch counter to > drm_connector") the drm_get_edid() function calls > drm_connector_update_edid_property() for us. There's no reason for us > to call it again. > > Signed-off-by: Douglas Anderson Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index a0a00dd1187c..9577ebd58c4c 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -270,7 +270,7 @@ static int ti_sn_bridge_connector_get_modes(struct > drm_connector *connector) > { > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); > struct edid *edid = pdata->edid; > - int num, ret; > + int num; > > if (!edid) { > pm_runtime_get_sync(pdata->dev); > @@ -279,12 +279,9 @@ static int ti_sn_bridge_connector_get_modes(struct > drm_connector *connector) > } > > if (edid && drm_edid_is_valid(edid)) { > - ret = drm_connector_update_edid_property(connector, edid); > - if (!ret) { > - num = drm_add_edid_modes(connector, edid); > - if (num) > - return num; > - } > + num = drm_add_edid_modes(connector, edid); > + if (num) > + return num; > } > > return drm_panel_get_modes(pdata->panel, connector); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/14] drm/bridge: ti-sn65dsi86: Stop caching the EDID ourselves
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > Now that we have the patch ("drm/edid: Use the cached EDID in > drm_get_edid() if eDP") we no longer need to maintain our own > cache. Drop this code. > > Signed-off-by: Douglas Anderson Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 +- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 9577ebd58c4c..c0398daaa4a6 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -121,7 +121,6 @@ >* @debugfs: Used for managing our debugfs. >* @host_node:Remote DSI node. >* @dsi: Our MIPI DSI source. > - * @edid: Detected EDID of eDP panel. >* @refclk: Our reference clock. >* @panel:Our panel. >* @enable_gpio: The GPIO we toggle to enable the bridge. > @@ -147,7 +146,6 @@ struct ti_sn_bridge { > struct drm_bridge bridge; > struct drm_connectorconnector; > struct dentry *debugfs; > - struct edid *edid; > struct device_node *host_node; > struct mipi_dsi_device *dsi; > struct clk *refclk; > @@ -269,17 +267,17 @@ connector_to_ti_sn_bridge(struct drm_connector > *connector) > static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) > { > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); > - struct edid *edid = pdata->edid; > - int num; > + struct edid *edid; > + int num = 0; > > - if (!edid) { > - pm_runtime_get_sync(pdata->dev); > - edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc); > - pm_runtime_put(pdata->dev); > - } > + pm_runtime_get_sync(pdata->dev); > + edid = drm_get_edid(connector, &pdata->aux.ddc); > + pm_runtime_put(pdata->dev); > > - if (edid && drm_edid_is_valid(edid)) { > - num = drm_add_edid_modes(connector, edid); > + if (edid) { > + if (drm_edid_is_valid(edid)) > + num = drm_add_edid_modes(connector, edid); > + kfree(edid); > if (num) > return num; > } > @@ -1308,8 +1306,6 @@ static int ti_sn_bridge_remove(struct i2c_client > *client) > if (!pdata) > return -EINVAL; > > - kfree(pdata->edid); > - > ti_sn_debugfs_remove(pdata); > > drm_bridge_remove(&pdata->bridge); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > eDP panels won't provide their EDID unless they're powered on. Let's > chain a power-on before we read the EDID. This roughly matches what > was done in 'parade-ps8640.c'. > > NOTE: The old code attempted to call pm_runtime_get_sync() before > reading the EDID. While that was enough to power the bridge chip on, > it wasn't enough to talk to the panel for two reasons: > 1. Since we never ran the bridge chip's pre-enable then we never set > the bit to ignore HPD. This meant the bridge chip didn't even _try_ > to go out on the bus and communicate with the panel. > 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read > if the panel wasn't on. > > One thing that's a bit odd here is taking advantage of the EDID that > the core might have cached for us. See the patch ("drm/edid: Use the > cached EDID in drm_get_edid() if eDP"). We manage to get at the cache > by: > - Instantly failing aux transfers if we're not powered. > - If the first read of the EDID fails we try again after powering. > > Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC") > Signed-off-by: Douglas Anderson > --- > Depending on what people think of the other patches in this series, > some of this could change. > - If everyone loves the "runtime PM" in the panel driver then we >could, in theory, put the pre-enable chaining straight in the "aux >transfer" function. > - If everyone hates the EDID cache moving to the core then we can >avoid some of the awkward flow of things and keep the EDID cache in >the sn65dsi86 driver. I wonder if this shouldn't be solved in the core - ie caller of get_modes callback should be responsible for powering up the pipeline, otherwise we need to repeat this stuff in every bridge/panel driver. Any thoughts? Regards Andrzej > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 +-- > 1 file changed, 37 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index c0398daaa4a6..673c9f1c2d8e 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -128,6 +128,7 @@ >* @dp_lanes: Count of dp_lanes we're using. >* @ln_assign:Value to program to the LN_ASSIGN register. >* @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG. > + * @pre_enabled: If true then pre_enable() has run. >* >* @gchip:If we expose our GPIOs, this is used. >* @gchip_output: A cache of whether we've set GPIOs to output. This > @@ -155,6 +156,7 @@ struct ti_sn_bridge { > int dp_lanes; > u8 ln_assign; > u8 ln_polrs; > + boolpre_enabled; > > #if defined(CONFIG_OF_GPIO) > struct gpio_chipgchip; > @@ -268,11 +270,33 @@ static int ti_sn_bridge_connector_get_modes(struct > drm_connector *connector) > { > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector); > struct edid *edid; > + bool was_enabled; > int num = 0; > > - pm_runtime_get_sync(pdata->dev); > + /* > + * Try to get the EDID first without anything special. There are > + * three things that could happen with this call. > + * a) It might just return from its cache. > + * b) It might try to initiate an AUX transfer which might work. > + * c) It might try to initiate an AUX transfer which might fail because > + *we're not powered up. > + * > + * If we get a failure we'll assume case c) and try again. NOTE: we > + * don't want to power up every time because that's slow and we don't > + * have visibility into whether the data has already been cached. > + */ > edid = drm_get_edid(connector, &pdata->aux.ddc); > - pm_runtime_put(pdata->dev); > + if (!edid) { > + was_enabled = pdata->pre_enabled; > + > + if (!was_enabled) > + drm_bridge_chain_pre_enable(&pdata->bridge); > + > + edid = drm_get_edid(connector, &pdata->aux.ddc); > + > + if (!was_enabled) > + drm_bridge_chain_post_disable(&pdata->bridge); > + } > > if (edid) { > if (drm_edid_is_valid(edid)) > @@ -852,12 +876,16 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge > *bridge) > HPD_DISABLE); > > drm_panel_prepare(pdata->panel); > + > + pdata->pre_enabled = true; > } > > static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > { > struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge); > > + pdata->pre_enabled = false; > + > drm_panel_unprepare(pdata->panel); > > clk_disable_unprepare(pdata->refclk); > @@ -891,6
Re: [PATCH v4 1/1] drm/bridge: lt8912b: Fix issues found during static analysis
W dniu 31.03.2021 o 13:49, Adrien Grassein pisze: > Some issues where found during static analysis of this driver. Subject should describe what has been fixed, description why. If there is multiple different issues maybe patch split would be better. > > Reported-by: Dan Carpenter > Suggested-by: Dan Carpenter > Signed-off-by: Adrien Grassein > --- > drivers/gpu/drm/bridge/lontium-lt8912b.c | 27 +++- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c > b/drivers/gpu/drm/bridge/lontium-lt8912b.c > index 61491615bad0..4c8d79142262 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c > +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c > @@ -621,7 +621,8 @@ static int lt8912_parse_dt(struct lt8912 *lt) > { > struct gpio_desc *gp_reset; > struct device *dev = lt->dev; > - int ret = 0; > + int ret; > + int data_lanes; > struct device_node *port_node; > struct device_node *endpoint; > > @@ -635,13 +636,16 @@ static int lt8912_parse_dt(struct lt8912 *lt) > lt->gp_reset = gp_reset; > > endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); > - if (IS_ERR(endpoint)) { > - ret = PTR_ERR(endpoint); > - goto end; > - } > + if (!endpoint) > + return -ENODEV; > > - lt->data_lanes = of_property_count_u32_elems(endpoint, "data-lanes"); > + data_lanes = of_property_count_u32_elems(endpoint, "data-lanes"); > of_node_put(endpoint); > + if (data_lanes < 0) { > + dev_err(lt->dev, "%s: Bad data-lanes property\n", __func__); > + return data_lanes; > + } > + lt->data_lanes = data_lanes; > > lt->host_node = of_graph_get_remote_node(dev->of_node, 0, -1); > if (!lt->host_node) { > @@ -658,19 +662,22 @@ static int lt8912_parse_dt(struct lt8912 *lt) > } > > lt->hdmi_port = of_drm_find_bridge(port_node); > - if (IS_ERR(lt->hdmi_port)) { > + if (!lt->hdmi_port) { > dev_err(lt->dev, "%s: Failed to get hdmi port\n", __func__); > - ret = PTR_ERR(lt->hdmi_port); > - of_node_put(lt->host_node); > - goto end; > + ret = -ENODEV; > + of_node_put(port_node); > + goto err_free_host_node; > } > > if (!of_device_is_compatible(port_node, "hdmi-connector")) { > dev_err(lt->dev, "%s: Failed to get hdmi port\n", __func__); > + of_node_put(port_node); > ret = -EINVAL; > + goto err_free_host_node; Maybe better would be to put of_node_put(port_node) after err_free_host_node label - of_node_put(NULL) does nothing. > } > > of_node_put(port_node); > + return 0; > > end: > return ret; This label and code can be removed, am I right? After reading the body I know what the patch does, so I can propose the subject, what about "fix incorrect handling of of_* return values". And please make description more descriptive :) With that fixed you can add my: Reviewed-by: Andrzej Hajda Regards Andrzej ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
W dniu 31.03.2021 o 16:48, Doug Anderson pisze: > Hi, > > On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda wrote: >> >> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: >>> eDP panels won't provide their EDID unless they're powered on. Let's >>> chain a power-on before we read the EDID. This roughly matches what >>> was done in 'parade-ps8640.c'. >>> >>> NOTE: The old code attempted to call pm_runtime_get_sync() before >>> reading the EDID. While that was enough to power the bridge chip on, >>> it wasn't enough to talk to the panel for two reasons: >>> 1. Since we never ran the bridge chip's pre-enable then we never set >>> the bit to ignore HPD. This meant the bridge chip didn't even _try_ >>> to go out on the bus and communicate with the panel. >>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read >>> if the panel wasn't on. >>> >>> One thing that's a bit odd here is taking advantage of the EDID that >>> the core might have cached for us. See the patch ("drm/edid: Use the >>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache >>> by: >>> - Instantly failing aux transfers if we're not powered. >>> - If the first read of the EDID fails we try again after powering. >>> >>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC") >>> Signed-off-by: Douglas Anderson >>> --- >>> Depending on what people think of the other patches in this series, >>> some of this could change. >>> - If everyone loves the "runtime PM" in the panel driver then we >>> could, in theory, put the pre-enable chaining straight in the "aux >>> transfer" function. >>> - If everyone hates the EDID cache moving to the core then we can >>> avoid some of the awkward flow of things and keep the EDID cache in >>> the sn65dsi86 driver. >> >> I wonder if this shouldn't be solved in the core - ie caller of >> get_modes callback should be responsible for powering up the pipeline, >> otherwise we need to repeat this stuff in every bridge/panel driver. >> >> Any thoughts? > Yeah, I did look at this a little bit. Presumably it would only make > sense to do it for eDP connections since: > > a) The concept of reading an EDID doesn't make sense for things like MIPI. I guess you mean MIPI DSI, and yes I agree, more generally it usually(!) doesn't make sense for any setup with fixed display panel. On the other hand there are DSI/HDMI or DSI/DP adapters which usually have EDID reading logic. And the concept makes sense for most connectors accepting external displays: HDMI, DP, MHL, VGA... > > b) For something with an external connector (DP and HDMI) you don't > even know they're inserted unless the EDID is ready to read (these > devices are, essentially, always powered). Usually there are two elements which are not the same: 1. HotPlug signal/wire. 2. EDID reading logic. The logic responsible for reading EDID needs to be enabled only for time required for EDID reading :) So it's power state often must be controlled explicitly by the bridge driver. So even if in many cases pre_enable powers on the logic for EDID reading it does not make it the rule, so I must step back from my claim that it is up to caller :) > > So I started off trying to do this in the core for eDP, but then it > wasn't completely clear how to write this code in a way that was super > generic. Specifically: > > 1. I don't think it's a 100% guarantee that everything is powered on > in pre-enable and powered off in post-disable. In this bridge chip > it's true, but maybe not every eDP driver? Would you want me to just > assume this, or add a flag? Ok, pre_enable should power on the chip, but for performing initialization of video transport layer. Assumption it will power on EDID logic is incorrect, so my claim seems wrong, but also this patch looks incorrect :) In general only device containing EDID logic knows how to power it up. Since I do not know your particular case I can propose few possible ways to investigate: - call bridge.next->get_modes - you leave responsibility for powering up to the downstream device. - ddc driver on i2c request should power up the panel - seems also correct, Regards Andrzej > > 2. It wasn't totally clear to me which state to use for telling if the > bridge had already been pre-enabled so I could avoid double-calling > it. I could dig more if need be but I spent a bit of time looking and >
Re: [PATCH v3 3/3] drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler
Hi Dmitry, W dniu 17.01.2021 o 01:23, Dmitry Baryshkov pisze: > drm hotplug handling code (drm_client_dev_hotplug()) can wait on mutex, > thus delaying further lt9611uxc IRQ events processing. It was observed > occasionally during bootups, when drm_client_modeset_probe() was waiting > for EDID ready event, which was delayed because IRQ handler was stuck > trying to deliver hotplug event. > Move hotplug notifications from IRQ handler to separate work to be able > to process IRQ events without delays. > > Signed-off-by: Dmitry Baryshkov > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") > Reviewed-by: Bjorn Andersson > Signed-off-by: Dmitry Baryshkov > --- > drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 30 +- > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > index b708700e182d..209e39923914 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > > #include > > @@ -36,6 +37,7 @@ struct lt9611uxc { > struct mutex ocm_lock; > > struct wait_queue_head wq; > + struct work_struct work; > > struct device_node *dsi0_node; > struct device_node *dsi1_node; > @@ -52,6 +54,7 @@ struct lt9611uxc { > > bool hpd_supported; > bool edid_read; > + bool hdmi_connected; > uint8_t fw_version; > }; > > @@ -151,15 +154,26 @@ static irqreturn_t lt9611uxc_irq_thread_handler(int > irq, void *dev_id) > } > > if (irq_status & BIT(1)) { > - if (lt9611uxc->connector.dev) > - drm_kms_helper_hotplug_event(lt9611uxc->connector.dev); > - else > - drm_bridge_hpd_notify(<9611uxc->bridge, !!(hpd_status > & BIT(1))); > + lt9611uxc->hdmi_connected = !!(hpd_status & BIT(1)); No need for !!, int->bool implicit conversion will do the thing. > + schedule_work(<9611uxc->work); > } > > return IRQ_HANDLED; > } > > +static void lt9611uxc_hpd_work(struct work_struct *work) > +{ > + struct lt9611uxc *lt9611uxc = container_of(work, struct lt9611uxc, > work); > + > + if (lt9611uxc->connector.dev) > + drm_kms_helper_hotplug_event(lt9611uxc->connector.dev); > + else > + drm_bridge_hpd_notify(<9611uxc->bridge, > + lt9611uxc->hdmi_connected ? > + connector_status_connected : > + connector_status_disconnected); I am little bit worried about accessing lt9611uxc->hdmi_connected - it is set in different thread, and there is no explicit sync code between them. I guess it is possible to loss proper HPD signal, especially if the HPD line is unstable (is there signal debouncing?). > +} > + > static void lt9611uxc_reset(struct lt9611uxc *lt9611uxc) > { > gpiod_set_value_cansleep(lt9611uxc->reset_gpio, 1); > @@ -447,7 +461,7 @@ static enum drm_connector_status > lt9611uxc_bridge_detect(struct drm_bridge *brid > struct lt9611uxc *lt9611uxc = bridge_to_lt9611uxc(bridge); > unsigned int reg_val = 0; > int ret; > - int connected = 1; > + bool connected = true; > > if (lt9611uxc->hpd_supported) { > lt9611uxc_lock(lt9611uxc); > @@ -457,8 +471,9 @@ static enum drm_connector_status > lt9611uxc_bridge_detect(struct drm_bridge *brid > if (ret) > dev_err(lt9611uxc->dev, "failed to read hpd status: > %d\n", ret); > else > - connected = reg_val & BIT(1); > + connected = !!(reg_val & BIT(1)); Again no no need for !!. I saw in v2 there was R-B tags added by Bjorn to other two patches, please do not forgot them next time. Regards Andrzej > } > + lt9611uxc->hdmi_connected = connected; > > return connected ? connector_status_connected : > connector_status_disconnected; > @@ -931,6 +946,8 @@ static int lt9611uxc_probe(struct i2c_client *client, > lt9611uxc->fw_version = ret; > > init_waitqueue_head(<9611uxc->wq); > + INIT_WORK(<9611uxc->work, lt9611uxc_hpd_work); > + > ret = devm_request_threaded_irq(dev, client->irq, NULL, > lt9611uxc_irq_thread_handler, > IRQF_ONESHOT, "lt9611uxc", lt9611uxc); > @@ -967,6 +984,7 @@ static int lt9611uxc_remove(struct i2c_client *client) > struct lt9611uxc *lt9611uxc = i2c_get_clientdata(client); > > disable_irq(client->irq); > + flush_scheduled_work(); > lt9611uxc_audio_exit(lt9611uxc); > drm_bridge_remove(<9611uxc->bridge); > ___ dri-devel
Re: [PATCH v4 1/3] drm/bridge/lontium-lt9611uxc: fix waiting for EDID to become available
W dniu 22.01.2021 o 00:33, Dmitry Baryshkov pisze: > - Call wake_up() when EDID ready event is received to wake >wait_event_interruptible_timeout() > > - Increase waiting timeout, reading EDID can take longer than 100ms, so >let's be on a safe side. > > Signed-off-by: Dmitry Baryshkov > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") > Reviewed-by: Bjorn Andersson Reviewed-by: Andrzej Hajda Regards Andrzej ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/3] drm/bridge/lontium-lt9611uxc: fix get_edid return code
W dniu 22.01.2021 o 00:33, Dmitry Baryshkov pisze: > Return NULL pointer from get_edid() callback rather than ERR_PTR() > pointer, as DRM code does NULL checks rather than IS_ERR(). Also while > we are at it, return NULL if getting EDID timed out. > > Signed-off-by: Dmitry Baryshkov > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") > Reviewed-by: Bjorn Andersson Reviewed-by: Andrzej Hajda Regards Andrzej ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 3/3] drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler
W dniu 22.01.2021 o 00:33, Dmitry Baryshkov pisze: > drm hotplug handling code (drm_client_dev_hotplug()) can wait on mutex, > thus delaying further lt9611uxc IRQ events processing. It was observed > occasionally during bootups, when drm_client_modeset_probe() was waiting > for EDID ready event, which was delayed because IRQ handler was stuck > trying to deliver hotplug event. > Move hotplug notifications from IRQ handler to separate work to be able > to process IRQ events without delays. > > Signed-off-by: Dmitry Baryshkov > Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") Reviewed-by: Andrzej Hajda Let's wait till Monday for other comments, then I can queue the patchset. Regards Andrzej ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6] drm/bridge: add it6505 driver
Hi Allen, Sorry for long delay. W dniu 08.12.2020 o 11:58, allen pisze: > This adds support for the iTE IT6505. > This device can convert DPI signal to DP output. > > From: Allen Chen > Signed-off-by: Jitao Shi > Signed-off-by: Pi-Hsun Shih > Signed-off-by: Yilun Lin > Signed-off-by: Hermes Wu > Signed-off-by: Allen Chen > --- > drivers/gpu/drm/bridge/Kconfig |7 + > drivers/gpu/drm/bridge/Makefile |1 + > drivers/gpu/drm/bridge/ite-it6505.c | 3343 +++ > 3 files changed, 3351 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/ite-it6505.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index e4110d6ca7b3c..25d34d7196004 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -74,6 +74,13 @@ config DRM_LONTIUM_LT9611UXC > HDMI signals > Please say Y if you have such hardware. > > +config DRM_ITE_IT6505 > + tristate "ITE IT6505 DisplayPort bridge" > + depends on OF > + select DRM_KMS_HELPER > + help > + ITE IT6505 DisplayPort bridge chip driver. > + > config DRM_LVDS_CODEC > tristate "Transparent LVDS encoders and decoders support" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 86e7acc76f8d6..2b2f8f0b5b0fa 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o > obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o > obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o > obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o > +obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o Please keep alphabetic order. > obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += > megachips-stdp-ge-b850v3-fw.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > new file mode 100644 > index 0..5e76719a51a4a > --- /dev/null > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -0,0 +1,3343 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +/* > + * Copyright (c) 2020, The Linux Foundation. All rights reserved. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define REG_IC_VER 0x04 > + > +#define REG_RESET_CTRL 0x05 > +#define VIDEO_RESET BIT(0) > +#define AUDIO_RESET BIT(1) > +#define ALL_LOGIC_RESET BIT(2) > +#define AUX_RESET BIT(3) > +#define HDCP_RESET BIT(4) > + > +#define INT_STATUS_01 0x06 > +#define INT_MASK_01 0x09 > +#define INT_HPD_CHANGE BIT(0) > +#define INT_RECEIVE_HPD_IRQ BIT(1) > +#define INT_SCDT_CHANGE BIT(2) > +#define INT_HDCP_FAIL BIT(3) > +#define INT_HDCP_DONE BIT(4) > + > +#define INT_STATUS_02 0x07 > +#define INT_MASK_02 0x0A > +#define INT_AUX_CMD_FAIL BIT(0) > +#define INT_HDCP_KSV_CHECK BIT(1) > +#define INT_AUDIO_FIFO_ERROR BIT(2) > + > +#define INT_STATUS_03 0x08 > +#define INT_MASK_03 0x0B > +#define INT_LINK_TRAIN_FAIL BIT(4) > +#define INT_VID_FIFO_ERROR BIT(5) > +#define INT_IO_LATCH_FIFO_OVERFLOW BIT(7) > + > +#define REG_SYSTEM_STS 0x0D > +#define INT_STS BIT(0) > +#define HPD_STS BIT(1) > +#define VIDEO_STB BIT(2) > + > +#define REG_LINK_TRAIN_STS 0x0E > +#define LINK_STATE_CR BIT(2) > +#define LINK_STATE_EQ BIT(3) > +#define LINK_STATE_NORP BIT(4) > + > +#define REG_BANK_SEL 0x0F > +#define REG_CLK_CTRL0 0x10 > +#define M_PCLK_DELAY 0x03 > + > +#define REG_AUX_OPT 0x11 > +#define AUX_AUTO_RST BIT(0) > +#define AUX_FIX_FREQ BIT(3) > + > +#define REG_DATA_CTRL0 0x12 > +#define VIDEO_LATCH_EDGE BIT(4) > +#define ENABLE_PCLK_COUNTER BIT(7) > + > +#define REG_PCLK_COUNTER_VALUE 0x13 > + > +#define REG_501_FIFO_CTRL 0x15 > +#define RST_501_FIFO BIT(1) > + > +#define REG_TRAIN_CTRL0 0x16 > +#define FORCE_LBR BIT(0) > +#define LANE_COUNT_MASK 0x06 > +#define LANE_SWAP BIT(3) > +#define SPREAD_AMP_5 BIT(4) > +#define FORCE_CR_DONE BIT(5) > +#define FORCE_EQ_DONE BIT(6) > + > +#define REG_TRAIN_CTRL1 0x17 > +#define AUTO_TRAIN BIT(0) > +#define MANUAL_TRAIN BIT(1) > +#define FORCE_RETRAIN BIT(2) > + > +#define REG_AUX_CTRL 0x23 > +#define CLR_EDID_FIFO BIT(0) > +#define AUX_USER_MODE BIT(1) > +#define AUX_NO_SEGMENT_WR BIT(6) > +#define AUX_EN_FIFO_READ BIT(7) > + > +#define REG_AUX_ADR_0_7 0x24 > +#define REG_AUX_ADR_8_15 0x25 > +#define REG_AUX_ADR_16_19 0x26 > +#define REG_AUX_OUT_DATA0 0x27 > + > +#define REG_AUX_CMD_REQ 0x2B > +#define AUX_BUSY BIT(5) > + > +#define REG_AUX_DATA_0_7 0x2C > +#define REG_AUX_DATA_8_15 0x2D > +#define REG_AUX_DATA_1
Re: [PATCH v4 3/3] drm/bridge/lontium-lt9611uxc: move HPD notification out of IRQ handler
W dniu 22.01.2021 o 10:34, Andrzej Hajda pisze: > W dniu 22.01.2021 o 00:33, Dmitry Baryshkov pisze: >> drm hotplug handling code (drm_client_dev_hotplug()) can wait on mutex, >> thus delaying further lt9611uxc IRQ events processing. It was observed >> occasionally during bootups, when drm_client_modeset_probe() was waiting >> for EDID ready event, which was delayed because IRQ handler was stuck >> trying to deliver hotplug event. >> Move hotplug notifications from IRQ handler to separate work to be able >> to process IRQ events without delays. >> >> Signed-off-by: Dmitry Baryshkov >> Fixes: 0cbbd5b1a012 ("drm: bridge: add support for lontium LT9611UXC bridge") > > Reviewed-by: Andrzej Hajda > > > Let's wait till Monday for other comments, then I can queue the patchset. Queued to drm-misc-fixes. I have fixed missing parentheses in the last patch in lt9611uxc_hpd_work, but apparently sth went wrong and I have merged version without these parentheses :(, can be fixed in another future patchset. Regards Andrzej > > > Regards > Andrzej > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/v1/url?k=20f4a2d0-7f6f9b96-20f5299f-0cc47a3003e8-b85b502ae8b34801&q=1&e=194b3466-3374-4717-82f7-d4cec3951dd6&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: anx7625: enable DSI EOTP
W dniu 04.02.2021 o 13:34, Nicolas Boichat pisze: > On Thu, Feb 4, 2021 at 8:07 PM Robert Foss wrote: >> Hi Xin, >> >> Thanks for the patch. >> >> On Thu, 28 Jan 2021 at 12:17, Xin Ji wrote: >>> Enable DSI EOTP feature for fixing some panel screen constance >>> shift issue. >>> Removing MIPI flag MIPI_DSI_MODE_EOT_PACKET to enable DSI EOTP. >> I don't think I quite understand how removing the >> MIPI_DSI_MODE_EOT_PACKET flag will cause DSI EOTP to be enabled. Could >> you extrapolate on this in the commit message? > That confused me as well, but it turns out that's how the flag is defined: > ``` > /* disable EoT packets in HS mode */ > #define MIPI_DSI_MODE_EOT_PACKET BIT(9) > ``` > (https://protect2.fireeye.com/v1/url?k=5bd95ebd-044267fb-5bd8d5f2-0cc47a3003e8-ce9db8ea264d6901&q=1&e=900556dc-d199-4c18-9432-5c3465a98eae&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Fdrm%2Fdrm_mipi_dsi.h%23L129) > > I'm almost tempted to put together a mass patch to rename all of these > flags... Yes that would be good, many of these flags were just copy pasted from some hw datasheet, without good analysis how to adapt them to the framework. Regards Andrzej > >>> Signed-off-by: Xin Ji >>> --- >>> drivers/gpu/drm/bridge/analogix/anx7625.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c >>> b/drivers/gpu/drm/bridge/analogix/anx7625.c >>> index 65cc059..e31eeb1b 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c >>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c >>> @@ -1334,7 +1334,6 @@ static int anx7625_attach_dsi(struct anx7625_data >>> *ctx) >>> dsi->format = MIPI_DSI_FMT_RGB888; >>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>> MIPI_DSI_MODE_VIDEO_SYNC_PULSE | >>> - MIPI_DSI_MODE_EOT_PACKET| >>> MIPI_DSI_MODE_VIDEO_HSE; >>> >>> if (mipi_dsi_attach(dsi) < 0) { >>> -- >>> 2.7.4 >>> > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/v1/url?k=457f3f39-1ae4067f-457eb476-0cc47a3003e8-b702072da729d8c9&q=1&e=900556dc-d199-4c18-9432-5c3465a98eae&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/16] drm/exynos: implement a drm bridge
W dniu 04.02.2021 o 17:05, Daniel Vetter pisze: > On Thu, Feb 04, 2021 at 11:56:32AM +0100, Michael Tretter wrote: >> On Thu, 04 Feb 2021 11:17:49 +0100, Daniel Vetter wrote: >>> On Wed, Feb 3, 2021 at 9:32 PM Michael Tretter >>> wrote: >>>> On Mon, 01 Feb 2021 17:33:14 +0100, Michael Tretter wrote: >>>>> On Tue, 15 Sep 2020 21:40:40 +0200, Andrzej Hajda wrote: >>>>>> W dniu 14.09.2020 o 23:19, Andrzej Hajda pisze: >>>>>>> On 14.09.2020 22:01, Michael Tretter wrote: >>>>>>>> On Mon, 14 Sep 2020 14:31:19 +0200, Marek Szyprowski wrote: >>>>>>>>> On 14.09.2020 10:29, Marek Szyprowski wrote: >>>>>>>>>> On 11.09.2020 15:54, Michael Tretter wrote: >>>>>>>>>>> Make the exynos_dsi driver a full drm bridge that can be found and >>>>>>>>>>> used >>>>>>>>>>> from other drivers. >>>>>>>>>>> >>>>>>>>>>> Other drivers can only attach to the bridge, if a mipi dsi device >>>>>>>>>>> already attached to the bridge. This allows to defer the probe of >>>>>>>>>>> the >>>>>>>>>>> display pipe until the downstream bridges are available, too. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Michael Tretter >>>>>>>>>> This one (and the whole series applied) still fails on Exynos boards: >>>>>>>>>> >>>>>>>>>> [drm] Exynos DRM: using 11c0.fimd device for DMA mapping >>>>>>>>>> operations >>>>>>>>>> exynos-drm exynos-drm: bound 11c0.fimd (ops fimd_component_ops) >>>>>>>>>> OF: graph: no port node found in /soc/dsi@11c8 >>>>>>>>>> 8<--- cut here --- >>>>>>>>>> Unable to handle kernel NULL pointer dereference at virtual address >>>>>>>>>> 0084 >>>>>>>>>> pgd = (ptrval) >>>>>>>>>> [0084] *pgd= >>>>>>>>>> Internal error: Oops: 5 [#1] PREEMPT SMP ARM >>>>>>>>>> Modules linked in: >>>>>>>>>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted >>>>>>>>>> 5.9.0-rc4-next-20200911-00010-g417dc70d70ec #1608 >>>>>>>>>> Hardware name: Samsung Exynos (Flattened Device Tree) >>>>>>>>>> PC is at drm_bridge_attach+0x18/0x164 >>>>>>>>>> LR is at exynos_dsi_bind+0x88/0xa8 >>>>>>>>>> pc : []lr : []psr: 2013 >>>>>>>>>> sp : ef0dfca8 ip : 0002 fp : c13190e0 >>>>>>>>>> r10: r9 : ee46d580 r8 : c13190e0 >>>>>>>>>> r7 : ee438800 r6 : 0018 r5 : ef253810 r4 : ef39e840 >>>>>>>>>> r3 : r2 : 0018 r1 : ef39e888 r0 : ef39e840 >>>>>>>>>> Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none >>>>>>>>>> Control: 10c5387d Table: 4000404a DAC: 0051 >>>>>>>>>> Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) >>>>>>>>>> Stack: (0xef0dfca8 to 0xef0e) >>>>>>>>>> ... >>>>>>>>>> [] (drm_bridge_attach) from [] >>>>>>>>>> (exynos_dsi_bind+0x88/0xa8) >>>>>>>>>> [] (exynos_dsi_bind) from [] >>>>>>>>>> (component_bind_all+0xfc/0x290) >>>>>>>>>> [] (component_bind_all) from [] >>>>>>>>>> (exynos_drm_bind+0xe4/0x19c) >>>>>>>>>> [] (exynos_drm_bind) from [] >>>>>>>>>> (try_to_bring_up_master+0x1e4/0x2c4) >>>>>>>>>> [] (try_to_bring_up_master) from [] >>>>>>>>>> (component_master_add_with_match+0xd4/0x108) >>>>>>>>>> [] (component_master_add_with_match) from [] >>>>>>>>>> (exynos_drm_platform_probe+0xe4/0x110) >>>>>>>>>> [] (exynos_drm_platform_probe) from [] >>>>>>>>>> (platform_drv_probe+0x6c/0xa4) >>>>>>>>>> [] (platform
Re: [PATCH] drm: bridge: adv7511: Add set_jack handler
W dniu 19.01.2021 o 05:41, Jun Nie pisze: > With commit 55c5cc63ab, the hdmi_codec_set_jack() will report unsupport > failure if set_jack handler is missing. Add set_jack handler to resolve > this failure. > > Signed-off-by: Jun Nie > --- > .../gpu/drm/bridge/adv7511/adv7511_audio.c| 27 ++- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > index f101dd2819b5..16de1a8ca7a0 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_audio.c > @@ -217,22 +217,35 @@ static int adv7511_hdmi_i2s_get_dai_id(struct > snd_soc_component *component, > return -EINVAL; > } > > +static int adv7511_hdmi_i2s_hook_plugged_cb(struct device *dev, void *data, > + hdmi_codec_plugged_cb fn, > + struct device *codec_dev) > +{ > + struct adv7511 *adv7511 = data; > + bool plugged = adv7511->connector.status == connector_status_connected; Maybe some locking, to protect access to adv7511->connector.status. > + > + fn(codec_dev, plugged); > + return 0; > +} > + > static const struct hdmi_codec_ops adv7511_codec_ops = { > .hw_params = adv7511_hdmi_hw_params, > .audio_shutdown = audio_shutdown, > .audio_startup = audio_startup, > .get_dai_id = adv7511_hdmi_i2s_get_dai_id, > -}; > - > -static const struct hdmi_codec_pdata codec_data = { > - .ops = &adv7511_codec_ops, > - .max_i2s_channels = 2, > - .i2s = 1, > - .spdif = 1, > + .hook_plugged_cb = adv7511_hdmi_i2s_hook_plugged_cb, > }; > > int adv7511_audio_init(struct device *dev, struct adv7511 *adv7511) > { > + struct hdmi_codec_pdata codec_data = { I think const modifier should be OK here. Regards Andrzej > + .ops = &adv7511_codec_ops, > + .max_i2s_channels = 2, > + .i2s = 1, > + .spdif = 1, > + .data = adv7511, > + }; > + > adv7511->audio_pdev = platform_device_register_data(dev, > HDMI_CODEC_DRV_NAME, > PLATFORM_DEVID_AUTO, ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/16] drm/exynos: implement a drm bridge
Hi Michael, W dniu 18.02.2021 o 09:04, Michael Tretter pisze: > On Wed, 10 Feb 2021 10:10:37 +0100, Frieder Schrempf wrote: >> On 04.02.21 18:46, Daniel Vetter wrote: >>> On Thu, Feb 4, 2021 at 6:26 PM Laurent Pinchart >>> wrote: >>>> On Thu, Feb 04, 2021 at 06:19:22PM +0100, Daniel Vetter wrote: >>>>> On Thu, Feb 4, 2021 at 5:28 PM Andrzej Hajda wrote: >>>>>> W dniu 04.02.2021 o 17:05, Daniel Vetter pisze: >>>>>>> On Thu, Feb 04, 2021 at 11:56:32AM +0100, Michael Tretter wrote: >>>>>>>> On Thu, 04 Feb 2021 11:17:49 +0100, Daniel Vetter wrote: >>>>>>>>> On Wed, Feb 3, 2021 at 9:32 PM Michael Tretter wrote: >>>>>>>>>> On Mon, 01 Feb 2021 17:33:14 +0100, Michael Tretter wrote: >>>>>>>>>>> On Tue, 15 Sep 2020 21:40:40 +0200, Andrzej Hajda wrote: >>>>>>>>>>>> W dniu 14.09.2020 o 23:19, Andrzej Hajda pisze: >>>>>>>>>>>>> On 14.09.2020 22:01, Michael Tretter wrote: >>>>>>>>>>>>>> On Mon, 14 Sep 2020 14:31:19 +0200, Marek Szyprowski wrote: >>>>>>>>>>>>>>> On 14.09.2020 10:29, Marek Szyprowski wrote: >>>>>>>>>>>>>>>> On 11.09.2020 15:54, Michael Tretter wrote: >>>>>>>>>>>>>>>>> Make the exynos_dsi driver a full drm bridge that can be >>>>>>>>>>>>>>>>> found and >>>>>>>>>>>>>>>>> used >>>>>>>>>>>>>>>>> from other drivers. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Other drivers can only attach to the bridge, if a mipi dsi >>>>>>>>>>>>>>>>> device >>>>>>>>>>>>>>>>> already attached to the bridge. This allows to defer the >>>>>>>>>>>>>>>>> probe of the >>>>>>>>>>>>>>>>> display pipe until the downstream bridges are available, too. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Signed-off-by: Michael Tretter >>>>>>>>>>>>>>>> This one (and the whole series applied) still fails on Exynos >>>>>>>>>>>>>>>> boards: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> [drm] Exynos DRM: using 11c0.fimd device for DMA mapping >>>>>>>>>>>>>>>> operations >>>>>>>>>>>>>>>> exynos-drm exynos-drm: bound 11c0.fimd (ops >>>>>>>>>>>>>>>> fimd_component_ops) >>>>>>>>>>>>>>>> OF: graph: no port node found in /soc/dsi@11c8 >>>>>>>>>>>>>>>> 8<--- cut here --- >>>>>>>>>>>>>>>> Unable to handle kernel NULL pointer dereference at virtual >>>>>>>>>>>>>>>> address >>>>>>>>>>>>>>>> 0084 >>>>>>>>>>>>>>>> pgd = (ptrval) >>>>>>>>>>>>>>>> [0084] *pgd= >>>>>>>>>>>>>>>> Internal error: Oops: 5 [#1] PREEMPT SMP ARM >>>>>>>>>>>>>>>> Modules linked in: >>>>>>>>>>>>>>>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted >>>>>>>>>>>>>>>> 5.9.0-rc4-next-20200911-00010-g417dc70d70ec #1608 >>>>>>>>>>>>>>>> Hardware name: Samsung Exynos (Flattened Device Tree) >>>>>>>>>>>>>>>> PC is at drm_bridge_attach+0x18/0x164 >>>>>>>>>>>>>>>> LR is at exynos_dsi_bind+0x88/0xa8 >>>>>>>>>>>>>>>> pc : []lr : []psr: 2013 >>>>>>>>>>>>>>>> sp : ef0dfca8 ip : 0002 fp : c13190e0 >>>>>>>>>>>>>>>>
Re: [PATCH] drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling features
Hi Nicolas, W dniu 22.02.2021 o 06:31, Nicolas Boichat pisze: > On Mon, Feb 22, 2021 at 3:08 AM Laurent Pinchart > wrote: >> Hi Nicolas, >> >> Thank you for the patch. >> >> On Thu, Feb 11, 2021 at 11:33:55AM +0800, Nicolas Boichat wrote: >>> Many of the DSI flags have names opposite to their actual effects, >>> e.g. MIPI_DSI_MODE_EOT_PACKET means that EoT packets will actually >>> be disabled. Fix this by including _NO_ in the flag names, e.g. >>> MIPI_DSI_MODE_NO_EOT_PACKET. >>> >>> Signed-off-by: Nicolas Boichat >> This looks good to me, it increases readability. >> >> Reviewed-by: Laurent Pinchart >> >> Please however see the end of the mail for a comment. Reviewed-by: Andrzej Hajda And comment at the end. >> >>> --- >>> I considered adding _DISABLE_ instead, but that'd make the >>> flag names a big too long. >>> >>> Generated with: >>> flag=MIPI_DSI_MODE_VIDEO_HFP; git grep $flag | cut -f1 -d':' | \ >>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HFP/" {} >>> flag=MIPI_DSI_MODE_VIDEO_HBP; git grep $flag | cut -f1 -d':' | \ >>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HBP/" {} >>> flag=MIPI_DSI_MODE_VIDEO_HSA; git grep $flag | cut -f1 -d':' | \ >>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HSA/" {} >>> flag=MIPI_DSI_MODE_EOT_PACKET; git grep $flag | cut -f1 -d':' | \ >>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_NO_EOT_PACKET/" {} >>> (then minor format changes) >> Ever tried coccinelle ? :-) > Fun project for next time ,-) > >>> drivers/gpu/drm/bridge/adv7511/adv7533.c | 2 +- >>> drivers/gpu/drm/bridge/analogix/anx7625.c| 2 +- >>> drivers/gpu/drm/bridge/cdns-dsi.c| 4 ++-- >>> drivers/gpu/drm/bridge/tc358768.c| 2 +- >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 >>> drivers/gpu/drm/mcde/mcde_dsi.c | 2 +- >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- >>> drivers/gpu/drm/msm/dsi/dsi_host.c | 8 >>> drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 2 +- >>> drivers/gpu/drm/panel/panel-dsi-cm.c | 2 +- >>> drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +- >>> drivers/gpu/drm/panel/panel-khadas-ts050.c | 2 +- >>> drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c | 2 +- >>> drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c | 2 +- >>> drivers/gpu/drm/panel/panel-novatek-nt35510.c| 2 +- >>> drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c | 2 +- >>> drivers/gpu/drm/panel/panel-samsung-s6d16d0.c| 2 +- >>> drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +- >>> drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c| 2 +- >>> drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c| 4 ++-- >>> drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 2 +- >>> drivers/gpu/drm/panel/panel-simple.c | 2 +- >>> drivers/gpu/drm/panel/panel-sony-acx424akp.c | 2 +- >>> drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c | 2 +- >>> include/drm/drm_mipi_dsi.h | 8 >>> 25 files changed, 36 insertions(+), 36 deletions(-) >>> >>> [] >>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h >>> index 360e6377e84b..ba91cf22af51 100644 >>> --- a/include/drm/drm_mipi_dsi.h >>> +++ b/include/drm/drm_mipi_dsi.h >>> @@ -119,15 +119,15 @@ struct mipi_dsi_host >>> *of_find_mipi_dsi_host_by_node(struct device_node *node); >>> /* enable hsync-end packets in vsync-pulse and v-porch area */ >>> #define MIPI_DSI_MODE_VIDEO_HSE BIT(4) >> We're mixing bits that enable a feature and bits that disable a feature. >> Are these bits defined in the DSI spec, or internal to DRM ? In the >> latter case, would it make sense to standardize on one "polarity" ? That >> would be a more intrusive change in drivers though. > Yes, that'd require auditing every single code path and reverse the > logic as needed. I'm not volunteering for that ,-P (hopefully the > current change is still an improvement). > > Hopefully real DSI experts can comment (Andrzej?), I think the default > are sensible settings? Hehe
Re: [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface
On 13.03.2022 20:45, Andi Shyti wrote: Hi Andrzej, I'm sorry, but I'm not fully understanding, +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name) +{ + struct kobject *kobj = &dev->kobj; + + /* +* We are interested at knowing from where the interface +* has been called, whether it's called from gt/ or from +* the parent directory. +* From the interface position it depends also the value of +* the private data. +* If the interface is called from gt/ then private data is +* of the "struct intel_gt *" type, otherwise it's * a +* "struct drm_i915_private *" type. +*/ + if (!is_object_gt(kobj)) { + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + pr_devel_ratelimited(DEPRECATED + "%s (pid %d) is accessing deprecated %s " + "sysfs control, please use gt/gt/%s instead\n", + current->comm, task_pid_nr(current), name, name); + return to_gt(i915); + } + + return kobj_to_gt(kobj); It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully. How would it help? The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference. While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs(). I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops. 'kobj_sysfs_ops' is of the type 'kobj_type'. Yes, but for example kobj_sysfs_ops.show points to function kobj_attr_show, and kobj_attr_show expects that it's attr argument is embedded in kobj_attribute[1], but this is not true in case of 'id' attribute - it is embedded in device_attribute. In short kobj_sysfs_ops should be used only with attrs embeded in kobj_attribute, unless I missed sth. [1]: https://elixir.bootlin.com/linux/latest/source/lib/kobject.c#L836 kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute. That's exactly the approach I use in the next patches for the power management files, I use "struct kobj_gt" wrapped around "struct kobject". But I'm using that only for the GT files. But attributes are still defined using DEVICE_ATTR* macros, ie they are embedded in device_attribute, so the problem is the same - you are using kobj_sysfs_ops with device_attribute. Are you, btw, suggesting to use this same approache also for the legacy files that for now have a pointer to the drm kobject? This way I would need to add more information, like the pointer to i915 and gt_id. This way I wouldn't need the files above that look hacky to you. Is this what you mean? Positive feedback is more difficult :) I am little bit lost in possible solutions, after grepping other drivers I have not good advice about proper handling of such situation, *beside splitting the interface*. For sure attrs used in device/power must be embedded in device_attribute. So if you do not want to split interface, then it implies GTs attrs must be also in device_attribute. Then maybe creating custom sysfs_ops would help??? I am not sure. Regards Andrzej Andi
Re: [Intel-gfx] [PATCH v6 3/7] drm/i915: Prepare for multiple GTs
On 18.03.2022 03:10, Andi Shyti wrote: From: Tvrtko Ursulin On a multi-tile platform, each tile has its own registers + GGTT space, and BAR 0 is extended to cover all of them. Up to four GTs are supported in i915->gt[], with slot zero shadowing the existing i915->gt0 to enable source compatibility with legacy driver paths. A for_each_gt macro is added to iterate over the GTs and will be used by upcoming patches that convert various parts of the driver to be multi-gt aware. Only the primary/root tile is initialized for now; the other tiles will be detected and plugged in by future patches once the necessary infrastructure is in place to handle them. Signed-off-by: Abdiel Janulgue Signed-off-by: Daniele Ceraolo Spurio Signed-off-by: Tvrtko Ursulin Signed-off-by: Matt Roper Signed-off-by: Andi Shyti Cc: Daniele Ceraolo Spurio Cc: Joonas Lahtinen Cc: Matthew Auld Reviewed-by: Matt Roper --- Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [Intel-gfx] [PATCH v6 7/7] drm/i915/gt: Adding new sysfs frequency attributes
On 18.03.2022 03:10, Andi Shyti wrote: From: Sujaritha Sundaresan This patch adds the following new sysfs frequency attributes: - punit_req_freq_mhz - throttle_reason_status - throttle_reason_pl1 - throttle_reason_pl2 - throttle_reason_pl4 - throttle_reason_thermal - throttle_reason_prochot - throttle_reason_ratl - throttle_reason_vr_thermalert - throttle_reason_vr_tdc Signed-off-by: Sujaritha Sundaresan Cc: Dale B Stimson Signed-off-by: Andi Shyti --- Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [Intel-gfx] [PATCH v6 6/7] drm/i915/gt: Create per-tile RPS sysfs interfaces
On 18.03.2022 03:10, Andi Shyti wrote: Now tiles have their own sysfs interfaces under the gt/ directory. Because RPS is a property that can be configured on a tile basis, then each tile should have its own interface The new sysfs structure will have a similar layout for the 4 tile case: /sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms │ │ ├── rps_act_freq_mhz │ │ ├── rps_boost_freq_mhz │ │ ├── rps_cur_freq_mhz │ │ ├── rps_max_freq_mhz │ │ ├── rps_min_freq_mhz │ │ ├── rps_RP0_freq_mhz │ │ ├── rps_RP1_freq_mhz │ │ └── rps_RPn_freq_mhz . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ ├── rps_act_freq_mhz │ ├── rps_boost_freq_mhz │ ├── rps_cur_freq_mhz │ ├── rps_max_freq_mhz │ ├── rps_min_freq_mhz │ ├── rps_RP0_freq_mhz │ ├── rps_RP1_freq_mhz │ └── rps_RPn_freq_mhz ├── gt_act_freq_mhz -+ ├── gt_boost_freq_mhz | ├── gt_cur_freq_mhz|Original interface ├── gt_max_freq_mhz+─-> kept as existing ABI; ├── gt_min_freq_mhz|it points to gt0/ ├── gt_RP0_freq_mhz| ├── gt_RP1_freq_mhz| └── gt_RPn_freq_mhz -+ The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when writing they loop through all the GTs and write the information on each interface. When reading they provide the average value from all the GTs. This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915. Signed-off-by: Andi Shyti Signed-off-by: Lucas De Marchi Cc: Chris Wilson Cc: Joonas Lahtinen Cc: Matt Roper Cc: Sujaritha Sundaresan Cc: Tvrtko Ursulin --- Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [Intel-gfx] [PATCH v6 4/7] drm/i915/gt: create per-tile sysfs interface
On 18.03.2022 03:10, Andi Shyti wrote: Now that we have tiles we want each of them to have its own interface. A directory "gt/" is created under "cardN/" that will contain as many diroctories as the tiles. In the coming patches tile related interfaces will be added. For now the sysfs gt structure simply has an id interface related to the current tile count. The directory structure will follow this scheme: /sys/.../card0 └── gt ├── gt0 │ └── id : : └─- gtN └── id This new set of interfaces will be a basic tool for system managers and administrators when using i915. Signed-off-by: Andi Shyti Cc: Matt Roper Cc: Sujaritha Sundaresan Cc: Tvrtko Ursulin Reviewed-by: Sujaritha Sundaresan --- drivers/gpu/drm/i915/Makefile| 1 + drivers/gpu/drm/i915/gt/intel_gt.c | 2 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 103 +++ drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c| 7 +- drivers/gpu/drm/i915/i915_sysfs.h| 3 + scripts/extract-cert | Bin 0 -> 17952 bytes With above removed. Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH 21/22] drm: Use drm_mode_init() for on-stack modes
On 18.02.2022 11:04, Ville Syrjala wrote: From: Ville Syrjälä Initialize on-stack modes with drm_mode_init() to guarantee no stack garbage in the list head, or that we aren't copying over another mode's list head. Based on the following cocci script, with manual fixups: @decl@ identifier M; expression E; @@ - struct drm_display_mode M = E; + struct drm_display_mode M; @@ identifier decl.M; expression decl.E; statement S, S1; @@ struct drm_display_mode M; ... when != S + drm_mode_init(&M, &E); + S1 @@ expression decl.E; @@ - &*E + E Signed-off-by: Ville Syrjälä --- Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH 22/22] drm: Use drm_mode_copy()
On 18.02.2022 11:04, Ville Syrjala wrote: From: Ville Syrjälä struct drm_display_mode embeds a list head, so overwriting the full struct with another one will corrupt the list (if the destination mode is on a list). Use drm_mode_copy() instead which explicitly preserves the list head of the destination mode. Even if we know the destination mode is not on any list using drm_mode_copy() seems decent as it sets a good example. Bad examples of not using it might eventually get copied into code where preserving the list head actually matters. Obviously one case not covered here is when the mode itself is embedded in a larger structure and the whole structure is copied. But if we are careful when copying into modes embedded in structures I think we can be a little more reassured that bogus list heads haven't been propagated in. @is_mode_copy@ @@ drm_mode_copy(...) { ... } @depends on !is_mode_copy@ struct drm_display_mode *mode; expression E, S; @@ ( - *mode = E + drm_mode_copy(mode, &E) | - memcpy(mode, E, S) + drm_mode_copy(mode, E) ) @depends on !is_mode_copy@ struct drm_display_mode mode; expression E; @@ ( - mode = E + drm_mode_copy(&mode, &E) | - memcpy(&mode, E, S) + drm_mode_copy(&mode, E) ) @@ struct drm_display_mode *mode; @@ - &*mode + mode Signed-off-by: Ville Syrjälä --- Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH 3/9] lib/ref_tracker: __ref_tracker_dir_print improve printing
On 17.02.2022 16:38, Eric Dumazet wrote: On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda wrote: To improve readibility of ref_tracker printing following changes have been performed: - added display name for ref_tracker_dir, - stack trace is printed indented, in the same printk call, - total number of references is printed every time, - print info about dropped references. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 15 --- lib/ref_tracker.c | 28 ++-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index b9c968a716483..090230e5b485d 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -15,18 +15,26 @@ struct ref_tracker_dir { refcount_t untracked; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ + charname[32]; #endif }; #ifdef CONFIG_REF_TRACKER -static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + +// Temporary allow two and three arguments, until consumers are converted +#define ref_tracker_dir_init(_d, _q, args...) _ref_tracker_dir_init(_d, _q, ##args, #_d) +#define _ref_tracker_dir_init(_d, _q, _n, ...) __ref_tracker_dir_init(_d, _q, _n) + +static inline void __ref_tracker_dir_init(struct ref_tracker_dir *dir, + unsigned int quarantine_count, + const char *name) { INIT_LIST_HEAD(&dir->list); INIT_LIST_HEAD(&dir->quarantine); spin_lock_init(&dir->lock); dir->quarantine_avail = quarantine_count; refcount_set(&dir->untracked, 1); + strlcpy(dir->name, name, sizeof(dir->name)); stack_depot_init(); } @@ -47,7 +55,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, #else /* CONFIG_REF_TRACKER */ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + unsigned int quarantine_count, + ...) { } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 0e9c7d2828ccb..943cff08110e3 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,4 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later + +#define pr_fmt(fmt) "ref_tracker: " fmt + #include #include #include @@ -7,6 +10,7 @@ #include #define REF_TRACKER_STACK_ENTRIES 16 +#define STACK_BUF_SIZE 1024 struct ref_tracker { struct list_headhead; /* anchor into dir->list or dir->quarantine */ @@ -26,31 +30,43 @@ static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct l void __ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { - unsigned int i = 0, count = 0; + unsigned int i = 0, count = 0, total = 0; struct ref_tracker *tracker; depot_stack_handle_t stack; + char *sbuf; lockdep_assert_held(&dir->lock); if (list_empty(&dir->list)) return; + sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT); + + list_for_each_entry(tracker, &dir->list, head) + ++total; Another iteration over a potential long list. You can count the @skipped number in the following iteration just fine. Skipped is already in 'count', but you are right with double looping> I can count total in the same loop, just forgot to merge loops during code evolution. Will be fixed. Regards Andrzej int skipped = 0; + list_sort(NULL, &dir->list, ref_tracker_cmp); list_for_each_entry(tracker, &dir->list, head) { - if (i++ >= display_limit) - break; if (!count++) stack = tracker->alloc_stack_handle; if (stack == tracker->alloc_stack_handle && !list_is_last(&tracker->head, &dir->list)) continue; + if (i++ >= display_limit) skipped++; + continue; - pr_err("leaked %d references.\n", count); - if (stack) - stack_depot_print(stack); + if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4)) + sbuf[0] = 0; + pr_err("%s@%pK has %d/%d users at\n%s\n", + dir->name, dir, count, total, sbuf); count = 0; }
Re: [PATCH 5/9] lib/ref_tracker: improve allocation flags
On 17.02.2022 16:13, Eric Dumazet wrote: On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda wrote: Library can be called in non-sleeping context, so it should not use __GFP_NOFAIL. Instead it should calmly handle allocation fails, for this __GFP_NOWARN has been added as well. Your commit changelog is misleading . The GFP_NOFAIL issue has been fixed already in commit c12837d1bb31032bead9060dec99ef310d5b9fb7 ("ref_tracker: use __GFP_NOFAIL more carefully") I based the patchset on drm-tip, which do not have this commit, I will take a look how to keep drm-tip base (to allow intel CI tests) and take patch above into account - maybe simple cherry-picking? Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 7b00bca300043..c8441ffbb058a 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -59,7 +59,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir, if (list_empty(&dir->list)) return; - sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT); + sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT | __GFP_NOWARN); This belongs to patch 3 in your series. OK, again historical reason. list_for_each_entry(tracker, &dir->list, head) ++total; @@ -154,11 +154,11 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, unsigned long entries[REF_TRACKER_STACK_ENTRIES]; struct ref_tracker *tracker; unsigned int nr_entries; - gfp_t gfp_mask = gfp; Simply change this line to : gfp_t gfp_mask = gfp | __GFP_NOFAIL; and "| __GFP_NOWARN". + gfp_t gfp_mask; unsigned long flags; Then leave all this code as is ? I find current code more readable. Yep you are right. - if (gfp & __GFP_DIRECT_RECLAIM) - gfp_mask |= __GFP_NOFAIL; + gfp |= __GFP_NOWARN; + gfp_mask = (gfp & __GFP_DIRECT_RECLAIM) ? (gfp | __GFP_NOFAIL) : gfp; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); if (unlikely(!tracker)) { pr_err_once("memory allocation failure, unreliable refcount tracker.\n"); @@ -191,7 +191,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); nr_entries = filter_irq_stacks(entries, nr_entries); lib/ref_tracker.c got patches in net-next, your patch series is going to add conflicts. git log --oneline 5740d0689096..4d449bdc5b26 --no-merges -- lib/ref_tracker.c c2d1e3df4af59261777b39c2e47476acd4d1cbeb ref_tracker: remove filter_irq_stacks() call 8fd5522f44dcd7f05454ddc4f16d0f821b676cd9 ref_tracker: add a count of untracked references e3ececfe668facd87d920b608349a32607060e66 ref_tracker: implement use-after-free detection So I will cherry-pick these patches into next version of patchset, with "NO MERGE" annotation (to allow testing), and if my ref_track patches will be accepted then they can go via net-dev tree and intel patches will wait till update of drm-tip. Is this scenario OK? Regards Andrzej - stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); + stack_handle = stack_depot_save(entries, nr_entries, + GFP_NOWAIT | __GFP_NOWARN); This is fine. spin_lock_irqsave(&dir->lock, flags); if (tracker->dead) { -- 2.25.1
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Separate wakeref tracking
On 17.02.2022 15:48, Ville Syrjälä wrote: On Thu, Feb 17, 2022 at 03:04:38PM +0100, Andrzej Hajda wrote: -static noinline depot_stack_handle_t +static intel_wakeref_t track_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - depot_stack_handle_t stack, *stacks; - unsigned long flags; - - if (rpm->no_wakeref_tracking) - return -1; - - stack = __save_depot_stack(); - if (!stack) + if (!rpm->available) return -1; no_wakeref_tracking != available ups I missed this change, will be more careful in next iteration. Regards Andrzej
Re: [PATCH 2/9] lib/ref_tracker: compact stacktraces before printing
On 17.02.2022 16:23, Eric Dumazet wrote: On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda wrote: In cases references are taken alternately on multiple exec paths leak report can grow substantially, sorting and grouping leaks by stack_handle allows to compact it. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- lib/ref_tracker.c | 35 +++ 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 1b0c6d645d64a..0e9c7d2828ccb 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include +#include #include #include #include @@ -14,23 +15,41 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; +static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct list_head *b) +{ + const struct ref_tracker *ta = list_entry(a, const struct ref_tracker, head); + const struct ref_tracker *tb = list_entry(b, const struct ref_tracker, head); + + return ta->alloc_stack_handle - tb->alloc_stack_handle; +} + void __ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { + unsigned int i = 0, count = 0; struct ref_tracker *tracker; - unsigned int i = 0; + depot_stack_handle_t stack; lockdep_assert_held(&dir->lock); + if (list_empty(&dir->list)) + return; + + list_sort(NULL, &dir->list, ref_tracker_cmp); What is going to be the cost of sorting a list with 1,000,000 items in it ? Do we really have such cases? I just want to make sure we do not trade printing at most ~10 references (from netdev_wait_allrefs()) to a soft lockup :/ with no useful info if something went terribly wrong. I suggest that you do not sort a potential big list, and instead attempt to allocate an array of @display_limits 'struct stack_counts' I suspect @display_limits will always be kept to a reasonable value (less than 100 ?) I though rather about 16 :) In theory everything is possible, but do we have real case examples which could lead to 100 stack traces? Maybe some frameworks used by multiple consumers (drivers) ??? struct stack_counts { depot_stack_handle_t stack_handle; unsigned int count; } Then, iterating the list and update the array (that you can keep sorted by ->stack_handle) Then after iterating, print the (at_most) @display_limits handles found in the temp array. OK, could be faster and less invasive. Other solution would be keeping the array in dir and update in every tracker alloc/free, this way we avoid iteration over potentially big list, but it would cost memory and since printing is rather rare I am not sure if it is worth. I will try your proposition. Regards Andrzej + list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { + if (i++ >= display_limit) break; - } + if (!count++) + stack = tracker->alloc_stack_handle; + if (stack == tracker->alloc_stack_handle && + !list_is_last(&tracker->head, &dir->list)) + continue; + + pr_err("leaked %d references.\n", count); + if (stack) + stack_depot_print(stack); + count = 0; } } EXPORT_SYMBOL(__ref_tracker_dir_print); -- 2.25.1
Re: [PATCH 01/22] drm: Add drm_mode_init()
On 18.02.2022 11:03, Ville Syrjala wrote: From: Ville Syrjälä Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_modes.c | 17 + include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy); +/** + * drm_mode_init - initialize the mode from another mode + * @dst: mode to overwrite + * @src: mode to copy + * + * Copy an existing mode into another mode, zeroing the + * list head of the destination mode. Typically used + * to guarantee the list head is not left with stack + * garbage in on-stack modes. + */ +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{ + memset(dst, 0, sizeof(*dst)); Why not just clear the list head? Or maybe poison it? It would be more cleaner. I wonder why there is no such helper in list.h. Regards Andrzej + drm_mode_copy(dst, src); +} +EXPORT_SYMBOL(drm_mode_init); + /** * drm_mode_duplicate - allocate and duplicate an existing mode * @dev: drm_device to allocate the duplicated mode for diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 29ba4adf0c53..e6e5a588fab1 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -484,6 +484,8 @@ void drm_mode_set_crtcinfo(struct drm_display_mode *p, int adjust_flags); void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src); +void drm_mode_init(struct drm_display_mode *dst, + const struct drm_display_mode *src); struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, const struct drm_display_mode *mode); bool drm_mode_match(const struct drm_display_mode *mode1,
Re: [PATCH 06/22] drm/bridge: Use drm_mode_copy()
On 18.02.2022 11:03, Ville Syrjala wrote: From: Ville Syrjälä struct drm_display_mode embeds a list head, so overwriting the full struct with another one will corrupt the list (if the destination mode is on a list). Use drm_mode_copy() instead which explicitly preserves the list head of the destination mode. Even if we know the destination mode is not on any list using drm_mode_copy() seems decent as it sets a good example. Bad examples of not using it might eventually get copied into code where preserving the list head actually matters. Obviously one case not covered here is when the mode itself is embedded in a larger structure and the whole structure is copied. But if we are careful when copying into modes embedded in structures I think we can be a little more reassured that bogus list heads haven't been propagated in. @is_mode_copy@ @@ drm_mode_copy(...) { ... } @depends on !is_mode_copy@ struct drm_display_mode *mode; expression E, S; @@ ( - *mode = E + drm_mode_copy(mode, &E) | - memcpy(mode, E, S) + drm_mode_copy(mode, E) ) @depends on !is_mode_copy@ struct drm_display_mode mode; expression E; @@ ( - mode = E + drm_mode_copy(&mode, &E) | - memcpy(&mode, E, S) + drm_mode_copy(&mode, E) ) @@ struct drm_display_mode *mode; @@ - &*mode + mode Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Signed-off-by: Ville Syrjälä Reviewed-by: Andrzej Hajda Regards Andrzej --- drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/bridge/tc358767.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index 963a6794735f..881cf338d5cf 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -857,7 +857,7 @@ nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, /* Save the new desired phy config */ memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); - memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode)); + drm_mode_copy(&dsi->mode, adjusted_mode); drm_mode_debug_printmodeline(adjusted_mode); if (pm_runtime_resume_and_get(dev) < 0) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 4befc104d220..a563460f8d20 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2830,7 +2830,7 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, mutex_lock(&hdmi->mutex); /* Store the display mode for plugin/DKMS poweron events */ - memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode)); + drm_mode_copy(&hdmi->previous_mode, mode); mutex_unlock(&hdmi->mutex); } diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index c23e0abc65e8..7f9574b17caa 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1312,7 +1312,7 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, { struct tc_data *tc = bridge_to_tc(bridge); - tc->mode = *mode; + drm_mode_copy(&tc->mode, mode); } static struct edid *tc_get_edid(struct drm_bridge *bridge,
Re: [PATCH 01/22] drm: Add drm_mode_init()
On 18.02.2022 12:56, Ville Syrjälä wrote: On Fri, Feb 18, 2022 at 12:22:44PM +0100, Andrzej Hajda wrote: On 18.02.2022 11:03, Ville Syrjala wrote: From: Ville Syrjälä Add a variant of drm_mode_copy() that explicitly clears out the list head of the destination mode. Helpful to guarantee we don't have stack garbage left in there for on-stack modes. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/drm_modes.c | 17 + include/drm/drm_modes.h | 2 ++ 2 files changed, 19 insertions(+) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 96b13e36293c..40d4ce4a1da4 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -892,6 +892,23 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode * } EXPORT_SYMBOL(drm_mode_copy); +/** + * drm_mode_init - initialize the mode from another mode + * @dst: mode to overwrite + * @src: mode to copy + * + * Copy an existing mode into another mode, zeroing the + * list head of the destination mode. Typically used + * to guarantee the list head is not left with stack + * garbage in on-stack modes. + */ +void drm_mode_init(struct drm_display_mode *dst, const struct drm_display_mode *src) +{ + memset(dst, 0, sizeof(*dst)); Why not just clear the list head? Or maybe poison it? It would be more cleaner. Then we have two places that need to be updated if some other field gets introduced that needs preserving. With a full memset() we only have to care about drm_mode_copy(). Don't see much point in micro-optimizing this thing. In such case DOC should be modified to avoid updating it "if some other field..." :) Anyway: Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH 0/2] DSI host and peripheral initialisation ordering
Hi, On 16.02.2022 17:59, Dave Stevenson wrote: Hi All Hopefully I've cc'ed all those that have bashed this problem around previously, or are otherwise linked to DRM bridges. There have been numerous discussions around how DSI support is currently broken as it doesn't support initialising the PHY to LP-11 and potentially the clock lane to HS prior to configuring the DSI peripheral. There is no op where the interface is initialised but HS video isn't also being sent. Currently you have: - peripheral pre_enable (host not initialised yet) - host pre_enable - encoder enable - host enable - peripheral enable (video already running) vc4 and exynos currently implement the DSI host as an encoder, and split the bridge_chain. This fails if you want to switch to being a bridge and/or use atomic calls as the state of all the elements split off are not added by drm_atomic_add_encoder_bridges. dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so the bridge/panel pre_enable can send commands. In their post_disable they then call the downstream bridge/panel post_disable op manually so that shutdown commands can be sent before shutting down the PHY. Nothing handles that fact, so the framework then continues down the bridge chain and calls the post_disable again, so we get unbalanced panel prepare/unprepare calls being reported [3]. There have been patches[4] proposing reversing the entire direction of pre_enable and post_disable, but that risks driving voltage into devices that have yet to be powered up. There have been discussions about adding either a pre_pre_enable, or adding a DSI host_op to initialise the host[5]. Both require significant reworking to all existing drivers in moving initialisation phases. We have patches that look like they may well be addressing race conditions in starting up a DSI peripheral[6]. This patch takes a hybrid of the two: an optional reversing of the order for specific links within the bridge chain within pre_enable and post_disable done within the drm_bridge framework. I'm more than happy to move where the flag exists in structures (currently as DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does this solve the problem posed? If not, then can you describe the actual scenario it doesn't cover? A DSI peripheral can set the flag to get the DSI host initialised first, and therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral can still send shutdown commands prior to the DSI host being shut down in post_disable. It also handles the case where there are multiple devices in the chain that all want their upstream bridge enabled first, so should there be a DSI mux between host and peripheral, then it can still get the host to the correct state. An example tree is at [7] which is drm-misc-next with these patches and then a conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed once we're over this hurdle). It is working happily with the Toshiba TC358762 on a Raspberry Pi 7" panel. The same approach but on our vendor 5.15 tree[8] has also been tested successfully on a TI SN65DSI83 and LVDS panel. Whilst here, I've also documented the expected behaviour of DSI hosts and peripherals to aid those who come along after. Good summary, of multiple attempts of solving the issue (however I still could add some more :) ). I think the main issue is that we try to squeeze different hardware protocol requirements into one quite restrictive framework - whole crtc->encoder->bridges->(panel ||connector) is managed directly by drm core. No place to negotiate configuration directly between players (bridges/panels). This patchset slightly looses the restrictions, so hopefully will help for some time, but still every developer needs to solve riddles what to put into callbacks, to allow driver working in different pipelines. Ideally I would like to drop idea of the bridge/panel and build abstraction on data links. So for example DSI/EDP bridge during probe would register DSI sink with their ops, and EDP source with their ops or just look for EDP sink (what will suit better). To establish data link they could use their ops and helpers to provide two-way conversation. This way if we need add support for new data link type or extend existing one we do not need to touch whole framework and pray to not break some strange bridge, or to add ops which will not be used by most of users. Putting dreams off, I think this patchset can add some value, at the price of call chain complication. Lets see opinion of others. Regards Andrzej Thanks Dave [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L940 [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html [5] https://lists.
[PATCH v2 00/11] drm/i915: use ref_tracker library for tracking wakerefs
Hi, Appearance of ref_tracker library allows to drop custom solution for wakeref tracking used in i915 and reuse the library. For this few adjustements has been made to ref_tracker, details in patches. I hope changes are OK for original author. The patchset has been rebased on top of drm-tip to allow test changes by CI. Patches marked "[DO NOT MERGE]" are cherry-picked from linux-next (they are not yet in drm-tip), to allow build and run CI on the patchset (it works only on drm-tip tree). Added CC to netdev as the only user of the library atm. v2: - replaced list_sort with ref_tracker_dir_stats, to avoid potentially extensive sorting, if number of reports is expected to be big enough (???) we can replace linear search in ref_tracker_dir_stats.stacks with binary heap (min_heap), - refactored gfp flags, - fixed i915 handling of no-tracking flag. Regards Andrzej Andrzej Hajda (6): lib/ref_tracker: add unlocked leak print helper lib/ref_tracker: __ref_tracker_dir_print improve printing lib/ref_tracker: add printing to memory buffer lib/ref_tracker: remove warnings in case of allocation failure drm/i915: Correct type of wakeref variable drm/i915: replace Intel internal tracker with kernel core ref_tracker Chris Wilson (2): drm/i915: Separate wakeref tracking drm/i915: Track leaked gt->wakerefs Eric Dumazet (3): [DO NOT MERGE] ref_tracker: implement use-after-free detection [DO NOT MERGE] ref_tracker: add a count of untracked references [DO NOT MERGE] ref_tracker: remove filter_irq_stacks() call drivers/gpu/drm/i915/Kconfig.debug| 19 ++ drivers/gpu/drm/i915/Makefile | 1 + .../drm/i915/display/intel_display_power.c| 2 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 7 +- .../i915/gem/selftests/i915_gem_coherency.c | 10 +- .../drm/i915/gem/selftests/i915_gem_mman.c| 14 +- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 13 +- .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 3 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 6 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 + .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 +- drivers/gpu/drm/i915/gt/intel_gt_pm.h | 36 ++- drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 20 +- drivers/gpu/drm/i915/gt/selftest_gt_pm.c | 5 +- drivers/gpu/drm/i915/gt/selftest_reset.c | 10 +- drivers/gpu/drm/i915/gt/selftest_rps.c| 17 +- drivers/gpu/drm/i915/gt/selftest_slpc.c | 10 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 +- drivers/gpu/drm/i915/i915_pmu.c | 16 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 239 ++ drivers/gpu/drm/i915/intel_runtime_pm.h | 10 +- drivers/gpu/drm/i915/intel_wakeref.c | 10 +- drivers/gpu/drm/i915/intel_wakeref.h | 112 +++- include/linux/ref_tracker.h | 35 ++- lib/ref_tracker.c | 198 --- 27 files changed, 480 insertions(+), 344 deletions(-) -- 2.25.1
[PATCH v2 01/11] [DO NOT MERGE] ref_tracker: implement use-after-free detection
From: Eric Dumazet Whenever ref_tracker_dir_init() is called, mark the struct ref_tracker_dir as dead. Test the dead status from ref_tracker_alloc() and ref_tracker_free() This should detect buggy dev_put()/dev_hold() happening too late in netdevice dismantle process. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 2 ++ lib/ref_tracker.c | 5 + 2 files changed, 7 insertions(+) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 60f3453be23e6..a443abda937d8 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -13,6 +13,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned intquarantine_avail; refcount_t untracked; + booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ #endif @@ -26,6 +27,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, INIT_LIST_HEAD(&dir->quarantine); spin_lock_init(&dir->lock); dir->quarantine_avail = quarantine_count; + dir->dead = false; refcount_set(&dir->untracked, 1); stack_depot_init(); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index a6789c0c626b0..32ff6bd497f8e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -20,6 +20,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) unsigned long flags; bool leak = false; + dir->dead = true; spin_lock_irqsave(&dir->lock, flags); list_for_each_entry_safe(tracker, n, &dir->quarantine, head) { list_del(&tracker->head); @@ -72,6 +73,8 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, gfp_t gfp_mask = gfp; unsigned long flags; + WARN_ON_ONCE(dir->dead); + if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); @@ -100,6 +103,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, unsigned int nr_entries; unsigned long flags; + WARN_ON_ONCE(dir->dead); + if (!tracker) { refcount_dec(&dir->untracked); return -EEXIST; -- 2.25.1
[PATCH v2 1/9] lib/ref_tracker: add unlocked leak print helper
To have reliable detection of leaks, caller must be able to check under the same lock both: tracked counter and the leaks. dir.lock is natural candidate for such lock and unlocked print helper can be called with this lock taken. As a bonus we can reuse this helper in ref_tracker_dir_exit. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 8 + lib/ref_tracker.c | 66 + 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 60f3453be23e6..b9c968a716483 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -32,6 +32,9 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, void ref_tracker_dir_exit(struct ref_tracker_dir *dir); +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit); + void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit); @@ -52,6 +55,11 @@ static inline void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { } +static inline void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ +} + static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index a6789c0c626b0..1b0c6d645d64a 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -14,6 +14,38 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + struct ref_tracker *tracker; + unsigned int i = 0; + + lockdep_assert_held(&dir->lock); + + list_for_each_entry(tracker, &dir->list, head) { + if (i < display_limit) { + pr_err("leaked reference.\n"); + if (tracker->alloc_stack_handle) + stack_depot_print(tracker->alloc_stack_handle); + i++; + } else { + break; + } + } +} +EXPORT_SYMBOL(__ref_tracker_dir_print); + +void ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + unsigned long flags; + + spin_lock_irqsave(&dir->lock, flags); + __ref_tracker_dir_print(dir, display_limit); + spin_unlock_irqrestore(&dir->lock, flags); +} +EXPORT_SYMBOL(ref_tracker_dir_print); + void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { struct ref_tracker *tracker, *n; @@ -26,13 +58,13 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) kfree(tracker); dir->quarantine_avail++; } - list_for_each_entry_safe(tracker, n, &dir->list, head) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); + if (!list_empty(&dir->list)) { + __ref_tracker_dir_print(dir, 16); leak = true; - list_del(&tracker->head); - kfree(tracker); + list_for_each_entry_safe(tracker, n, &dir->list, head) { + list_del(&tracker->head); + kfree(tracker); + } } spin_unlock_irqrestore(&dir->lock, flags); WARN_ON_ONCE(leak); @@ -40,28 +72,6 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) } EXPORT_SYMBOL(ref_tracker_dir_exit); -void ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) -{ - struct ref_tracker *tracker; - unsigned long flags; - unsigned int i = 0; - - spin_lock_irqsave(&dir->lock, flags); - list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { - break; - } - } - spin_unlock_irqrestore(&dir->lock, flags); -} -EXPORT_SYMBOL(ref_tracker_dir_print); - int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp) -- 2.25.1
[PATCH v2 01/11] ref_tracker: implement use-after-free detection
From: Eric Dumazet Whenever ref_tracker_dir_init() is called, mark the struct ref_tracker_dir as dead. Test the dead status from ref_tracker_alloc() and ref_tracker_free() This should detect buggy dev_put()/dev_hold() happening too late in netdevice dismantle process. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 2 ++ lib/ref_tracker.c | 5 + 2 files changed, 7 insertions(+) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 60f3453be23e6..a443abda937d8 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -13,6 +13,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned intquarantine_avail; refcount_t untracked; + booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ #endif @@ -26,6 +27,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, INIT_LIST_HEAD(&dir->quarantine); spin_lock_init(&dir->lock); dir->quarantine_avail = quarantine_count; + dir->dead = false; refcount_set(&dir->untracked, 1); stack_depot_init(); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index a6789c0c626b0..32ff6bd497f8e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -20,6 +20,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) unsigned long flags; bool leak = false; + dir->dead = true; spin_lock_irqsave(&dir->lock, flags); list_for_each_entry_safe(tracker, n, &dir->quarantine, head) { list_del(&tracker->head); @@ -72,6 +73,8 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, gfp_t gfp_mask = gfp; unsigned long flags; + WARN_ON_ONCE(dir->dead); + if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); @@ -100,6 +103,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, unsigned int nr_entries; unsigned long flags; + WARN_ON_ONCE(dir->dead); + if (!tracker) { refcount_dec(&dir->untracked); return -EEXIST; -- 2.25.1
[PATCH v2 02/11] [DO NOT MERGE] ref_tracker: add a count of untracked references
From: Eric Dumazet We are still chasing a netdev refcount imbalance, and we suspect we have one rogue dev_put() that is consuming a reference taken from a dev_hold_track() To detect this case, allow ref_tracker_alloc() and ref_tracker_free() to be called with a NULL @trackerp parameter, and use a dedicated refcount_t just for them. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 2 ++ lib/ref_tracker.c | 12 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index a443abda937d8..9ca353ab712b5 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -13,6 +13,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned intquarantine_avail; refcount_t untracked; + refcount_t no_tracker; booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ @@ -29,6 +30,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, dir->quarantine_avail = quarantine_count; dir->dead = false; refcount_set(&dir->untracked, 1); + refcount_set(&dir->no_tracker, 1); stack_depot_init(); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 32ff6bd497f8e..9c0c2e09df666 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -38,6 +38,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) spin_unlock_irqrestore(&dir->lock, flags); WARN_ON_ONCE(leak); WARN_ON_ONCE(refcount_read(&dir->untracked) != 1); + WARN_ON_ONCE(refcount_read(&dir->no_tracker) != 1); } EXPORT_SYMBOL(ref_tracker_dir_exit); @@ -75,6 +76,10 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, WARN_ON_ONCE(dir->dead); + if (!trackerp) { + refcount_inc(&dir->no_tracker); + return 0; + } if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); @@ -98,13 +103,18 @@ int ref_tracker_free(struct ref_tracker_dir *dir, struct ref_tracker **trackerp) { unsigned long entries[REF_TRACKER_STACK_ENTRIES]; - struct ref_tracker *tracker = *trackerp; depot_stack_handle_t stack_handle; + struct ref_tracker *tracker; unsigned int nr_entries; unsigned long flags; WARN_ON_ONCE(dir->dead); + if (!trackerp) { + refcount_dec(&dir->no_tracker); + return 0; + } + tracker = *trackerp; if (!tracker) { refcount_dec(&dir->untracked); return -EEXIST; -- 2.25.1
[PATCH v2 2/9] lib/ref_tracker: compact stacktraces before printing
In cases references are taken alternately on multiple exec paths leak report can grow substantially, sorting and grouping leaks by stack_handle allows to compact it. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- lib/ref_tracker.c | 35 +++ 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 1b0c6d645d64a..0e9c7d2828ccb 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include +#include #include #include #include @@ -14,23 +15,41 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; +static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct list_head *b) +{ + const struct ref_tracker *ta = list_entry(a, const struct ref_tracker, head); + const struct ref_tracker *tb = list_entry(b, const struct ref_tracker, head); + + return ta->alloc_stack_handle - tb->alloc_stack_handle; +} + void __ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { + unsigned int i = 0, count = 0; struct ref_tracker *tracker; - unsigned int i = 0; + depot_stack_handle_t stack; lockdep_assert_held(&dir->lock); + if (list_empty(&dir->list)) + return; + + list_sort(NULL, &dir->list, ref_tracker_cmp); + list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { + if (i++ >= display_limit) break; - } + if (!count++) + stack = tracker->alloc_stack_handle; + if (stack == tracker->alloc_stack_handle && + !list_is_last(&tracker->head, &dir->list)) + continue; + + pr_err("leaked %d references.\n", count); + if (stack) + stack_depot_print(stack); + count = 0; } } EXPORT_SYMBOL(__ref_tracker_dir_print); -- 2.25.1
[PATCH v2 02/11] ref_tracker: add a count of untracked references
From: Eric Dumazet We are still chasing a netdev refcount imbalance, and we suspect we have one rogue dev_put() that is consuming a reference taken from a dev_hold_track() To detect this case, allow ref_tracker_alloc() and ref_tracker_free() to be called with a NULL @trackerp parameter, and use a dedicated refcount_t just for them. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 2 ++ lib/ref_tracker.c | 12 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index a443abda937d8..9ca353ab712b5 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -13,6 +13,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned intquarantine_avail; refcount_t untracked; + refcount_t no_tracker; booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ @@ -29,6 +30,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, dir->quarantine_avail = quarantine_count; dir->dead = false; refcount_set(&dir->untracked, 1); + refcount_set(&dir->no_tracker, 1); stack_depot_init(); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 32ff6bd497f8e..9c0c2e09df666 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -38,6 +38,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) spin_unlock_irqrestore(&dir->lock, flags); WARN_ON_ONCE(leak); WARN_ON_ONCE(refcount_read(&dir->untracked) != 1); + WARN_ON_ONCE(refcount_read(&dir->no_tracker) != 1); } EXPORT_SYMBOL(ref_tracker_dir_exit); @@ -75,6 +76,10 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, WARN_ON_ONCE(dir->dead); + if (!trackerp) { + refcount_inc(&dir->no_tracker); + return 0; + } if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); @@ -98,13 +103,18 @@ int ref_tracker_free(struct ref_tracker_dir *dir, struct ref_tracker **trackerp) { unsigned long entries[REF_TRACKER_STACK_ENTRIES]; - struct ref_tracker *tracker = *trackerp; depot_stack_handle_t stack_handle; + struct ref_tracker *tracker; unsigned int nr_entries; unsigned long flags; WARN_ON_ONCE(dir->dead); + if (!trackerp) { + refcount_dec(&dir->no_tracker); + return 0; + } + tracker = *trackerp; if (!tracker) { refcount_dec(&dir->untracked); return -EEXIST; -- 2.25.1
[PATCH v2 03/11] [DO NOT MERGE] ref_tracker: remove filter_irq_stacks() call
From: Eric Dumazet After commit e94006608949 ("lib/stackdepot: always do filter_irq_stacks() in stack_depot_save()") it became unnecessary to filter the stack before calling stack_depot_save(). Signed-off-by: Eric Dumazet Cc: Marco Elver Cc: Alexander Potapenko Cc: Dmitry Vyukov Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 9c0c2e09df666..dc7b14aa3431e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -89,7 +89,6 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, return -ENOMEM; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - nr_entries = filter_irq_stacks(entries, nr_entries); tracker->alloc_stack_handle = stack_depot_save(entries, nr_entries, gfp); spin_lock_irqsave(&dir->lock, flags); @@ -120,7 +119,6 @@ int ref_tracker_free(struct ref_tracker_dir *dir, return -EEXIST; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - nr_entries = filter_irq_stacks(entries, nr_entries); stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); spin_lock_irqsave(&dir->lock, flags); -- 2.25.1
[PATCH v2 3/9] lib/ref_tracker: __ref_tracker_dir_print improve printing
To improve readibility of ref_tracker printing following changes have been performed: - added display name for ref_tracker_dir, - stack trace is printed indented, in the same printk call, - total number of references is printed every time, - print info about dropped references. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 15 --- lib/ref_tracker.c | 28 ++-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index b9c968a716483..090230e5b485d 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -15,18 +15,26 @@ struct ref_tracker_dir { refcount_t untracked; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ + charname[32]; #endif }; #ifdef CONFIG_REF_TRACKER -static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + +// Temporary allow two and three arguments, until consumers are converted +#define ref_tracker_dir_init(_d, _q, args...) _ref_tracker_dir_init(_d, _q, ##args, #_d) +#define _ref_tracker_dir_init(_d, _q, _n, ...) __ref_tracker_dir_init(_d, _q, _n) + +static inline void __ref_tracker_dir_init(struct ref_tracker_dir *dir, + unsigned int quarantine_count, + const char *name) { INIT_LIST_HEAD(&dir->list); INIT_LIST_HEAD(&dir->quarantine); spin_lock_init(&dir->lock); dir->quarantine_avail = quarantine_count; refcount_set(&dir->untracked, 1); + strlcpy(dir->name, name, sizeof(dir->name)); stack_depot_init(); } @@ -47,7 +55,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, #else /* CONFIG_REF_TRACKER */ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + unsigned int quarantine_count, + ...) { } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 0e9c7d2828ccb..943cff08110e3 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,4 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later + +#define pr_fmt(fmt) "ref_tracker: " fmt + #include #include #include @@ -7,6 +10,7 @@ #include #define REF_TRACKER_STACK_ENTRIES 16 +#define STACK_BUF_SIZE 1024 struct ref_tracker { struct list_headhead; /* anchor into dir->list or dir->quarantine */ @@ -26,31 +30,43 @@ static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct l void __ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { - unsigned int i = 0, count = 0; + unsigned int i = 0, count = 0, total = 0; struct ref_tracker *tracker; depot_stack_handle_t stack; + char *sbuf; lockdep_assert_held(&dir->lock); if (list_empty(&dir->list)) return; + sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT); + + list_for_each_entry(tracker, &dir->list, head) + ++total; + list_sort(NULL, &dir->list, ref_tracker_cmp); list_for_each_entry(tracker, &dir->list, head) { - if (i++ >= display_limit) - break; if (!count++) stack = tracker->alloc_stack_handle; if (stack == tracker->alloc_stack_handle && !list_is_last(&tracker->head, &dir->list)) continue; + if (i++ >= display_limit) + continue; - pr_err("leaked %d references.\n", count); - if (stack) - stack_depot_print(stack); + if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4)) + sbuf[0] = 0; + pr_err("%s@%pK has %d/%d users at\n%s\n", + dir->name, dir, count, total, sbuf); count = 0; } + if (i > display_limit) + pr_err("%s@%pK skipped %d/%d reports with %d unique stacks.\n", + dir->name, dir, count, total, i - display_limit); + + kfree(sbuf); } EXPORT_SYMBOL(__ref_tracker_dir_print); -- 2.25.1
[PATCH v2 4/9] lib/ref_tracker: add printing to memory buffer
In case one wants to show stats via debugfs. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 8 ++ lib/ref_tracker.c | 52 - 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 090230e5b485d..6d2634590ee5a 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -46,6 +46,8 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit); +int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size); + int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp); @@ -74,6 +76,12 @@ static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir, { } +static inline int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, + char *buf, size_t size) +{ + return 0; +} + static inline int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 943cff08110e3..7b00bca300043 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -27,8 +27,27 @@ static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct l return ta->alloc_stack_handle - tb->alloc_stack_handle; } -void __ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) +struct ostream { + char *buf; + int size, used; +}; + +#define pr_ostream(stream, fmt, args...) \ +({ \ + struct ostream *_s = (stream); \ +\ + if (!_s->buf) { \ + pr_err(fmt, ##args); \ + } else { \ + int ret, len = _s->size - _s->used; \ + ret = snprintf(_s->buf + _s->used, len, pr_fmt(fmt), ##args); \ + _s->used += min(ret, len); \ + } \ +}) + +static void +__ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir, +unsigned int display_limit, struct ostream *s) { unsigned int i = 0, count = 0, total = 0; struct ref_tracker *tracker; @@ -58,16 +77,24 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4)) sbuf[0] = 0; - pr_err("%s@%pK has %d/%d users at\n%s\n", - dir->name, dir, count, total, sbuf); + pr_ostream(s, "%s@%pK has %d/%d users at\n%s\n", + dir->name, dir, count, total, sbuf); count = 0; } if (i > display_limit) - pr_err("%s@%pK skipped %d/%d reports with %d unique stacks.\n", - dir->name, dir, count, total, i - display_limit); + pr_ostream(s, "%s@%pK skipped %d/%d reports with %d unique stacks.\n", + dir->name, dir, count, total, i - display_limit); kfree(sbuf); } + +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + struct ostream os = {}; + + __ref_tracker_dir_pr_ostream(dir, display_limit, &os); +} EXPORT_SYMBOL(__ref_tracker_dir_print); void ref_tracker_dir_print(struct ref_tracker_dir *dir, @@ -81,6 +108,19 @@ void ref_tracker_dir_print(struct ref_tracker_dir *dir, } EXPORT_SYMBOL(ref_tracker_dir_print); +int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size) +{ + struct ostream os = { .buf = buf, .size = size }; + unsigned long flags; + + spin_lock_irqsave(&dir->lock, flags); + __ref_tracker_dir_pr_ostream(dir, 16, &os); + spin_unlock_irqrestore(&dir->lock, flags); + + return os.used; +} +EXPORT_SYMBOL(ref_tracker_dir_snprint); + void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { struct ref_tracker *tracker, *n; -- 2.25.1
[PATCH v2 03/11] ref_tracker: remove filter_irq_stacks() call
From: Eric Dumazet After commit e94006608949 ("lib/stackdepot: always do filter_irq_stacks() in stack_depot_save()") it became unnecessary to filter the stack before calling stack_depot_save(). Signed-off-by: Eric Dumazet Cc: Marco Elver Cc: Alexander Potapenko Cc: Dmitry Vyukov Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 9c0c2e09df666..dc7b14aa3431e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -89,7 +89,6 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, return -ENOMEM; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - nr_entries = filter_irq_stacks(entries, nr_entries); tracker->alloc_stack_handle = stack_depot_save(entries, nr_entries, gfp); spin_lock_irqsave(&dir->lock, flags); @@ -120,7 +119,6 @@ int ref_tracker_free(struct ref_tracker_dir *dir, return -EEXIST; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - nr_entries = filter_irq_stacks(entries, nr_entries); stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); spin_lock_irqsave(&dir->lock, flags); -- 2.25.1
[PATCH v2 05/11] lib/ref_tracker: __ref_tracker_dir_print improve printing
To improve readibility of ref_tracker printing following changes have been performed: - reports are printed per stack_handle - log is more compact, - added display name for ref_tracker_dir, - stack trace is printed indented, in the same printk call, - total number of references is printed every time, - print info about dropped references. Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 15 +-- lib/ref_tracker.c | 90 - 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 3e9e9df2a41f5..a2cf1f6309adb 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -17,12 +17,19 @@ struct ref_tracker_dir { booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ + charname[32]; #endif }; #ifdef CONFIG_REF_TRACKER -static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + +// Temporary allow two and three arguments, until consumers are converted +#define ref_tracker_dir_init(_d, _q, args...) _ref_tracker_dir_init(_d, _q, ##args, #_d) +#define _ref_tracker_dir_init(_d, _q, _n, ...) __ref_tracker_dir_init(_d, _q, _n) + +static inline void __ref_tracker_dir_init(struct ref_tracker_dir *dir, + unsigned int quarantine_count, + const char *name) { INIT_LIST_HEAD(&dir->list); INIT_LIST_HEAD(&dir->quarantine); @@ -31,6 +38,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, dir->dead = false; refcount_set(&dir->untracked, 1); refcount_set(&dir->no_tracker, 1); + strlcpy(dir->name, name, sizeof(dir->name)); stack_depot_init(); } @@ -51,7 +59,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, #else /* CONFIG_REF_TRACKER */ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + unsigned int quarantine_count, + ...) { } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 5e9f90bbf771b..ab1253fde244e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,11 +1,16 @@ // SPDX-License-Identifier: GPL-2.0-or-later + +#define pr_fmt(fmt) "ref_tracker: " fmt + #include +#include #include #include #include #include #define REF_TRACKER_STACK_ENTRIES 16 +#define STACK_BUF_SIZE 1024 struct ref_tracker { struct list_headhead; /* anchor into dir->list or dir->quarantine */ @@ -14,24 +19,87 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; -void __ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) +struct ref_tracker_dir_stats { + int total; + int count; + struct { + depot_stack_handle_t stack_handle; + unsigned int count; + } stacks[]; +}; + +static struct ref_tracker_dir_stats * +ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit) { + struct ref_tracker_dir_stats *stats; struct ref_tracker *tracker; - unsigned int i = 0; - lockdep_assert_held(&dir->lock); + stats = kmalloc(struct_size(stats, stacks, limit), + GFP_NOWAIT | __GFP_NOWARN); + if (!stats) + return ERR_PTR(-ENOMEM); + stats->total = 0; + stats->count = 0; list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { - break; + depot_stack_handle_t stack = tracker->alloc_stack_handle; + int i; + + ++stats->total; + for (i = 0; i < stats->count; ++i) + if (stats->stacks[i].stack_handle == stack) + break; + if (i >= limit) + continue; + if (i >= stats->count) { + stats->stacks[i].stack_handle = stack; + stats->stacks[i].count = 0; + ++stats->count; } + ++stats->stacks[i].count; + } + + return stats; +} + +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, +
[PATCH v2 04/11] lib/ref_tracker: add unlocked leak print helper
To have reliable detection of leaks, caller must be able to check under the same lock both: tracked counter and the leaks. dir.lock is natural candidate for such lock and unlocked print helper can be called with this lock taken. As a bonus we can reuse this helper in ref_tracker_dir_exit. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 8 + lib/ref_tracker.c | 66 + 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 9ca353ab712b5..3e9e9df2a41f5 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -36,6 +36,9 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, void ref_tracker_dir_exit(struct ref_tracker_dir *dir); +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit); + void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit); @@ -56,6 +59,11 @@ static inline void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { } +static inline void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ +} + static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index dc7b14aa3431e..5e9f90bbf771b 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -14,6 +14,38 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + struct ref_tracker *tracker; + unsigned int i = 0; + + lockdep_assert_held(&dir->lock); + + list_for_each_entry(tracker, &dir->list, head) { + if (i < display_limit) { + pr_err("leaked reference.\n"); + if (tracker->alloc_stack_handle) + stack_depot_print(tracker->alloc_stack_handle); + i++; + } else { + break; + } + } +} +EXPORT_SYMBOL(__ref_tracker_dir_print); + +void ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + unsigned long flags; + + spin_lock_irqsave(&dir->lock, flags); + __ref_tracker_dir_print(dir, display_limit); + spin_unlock_irqrestore(&dir->lock, flags); +} +EXPORT_SYMBOL(ref_tracker_dir_print); + void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { struct ref_tracker *tracker, *n; @@ -27,13 +59,13 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) kfree(tracker); dir->quarantine_avail++; } - list_for_each_entry_safe(tracker, n, &dir->list, head) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); + if (!list_empty(&dir->list)) { + __ref_tracker_dir_print(dir, 16); leak = true; - list_del(&tracker->head); - kfree(tracker); + list_for_each_entry_safe(tracker, n, &dir->list, head) { + list_del(&tracker->head); + kfree(tracker); + } } spin_unlock_irqrestore(&dir->lock, flags); WARN_ON_ONCE(leak); @@ -42,28 +74,6 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) } EXPORT_SYMBOL(ref_tracker_dir_exit); -void ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) -{ - struct ref_tracker *tracker; - unsigned long flags; - unsigned int i = 0; - - spin_lock_irqsave(&dir->lock, flags); - list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { - break; - } - } - spin_unlock_irqrestore(&dir->lock, flags); -} -EXPORT_SYMBOL(ref_tracker_dir_print); - int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp) -- 2.25.1
[PATCH v2 5/9] lib/ref_tracker: improve allocation flags
Library can be called in non-sleeping context, so it should not use __GFP_NOFAIL. Instead it should calmly handle allocation fails, for this __GFP_NOWARN has been added as well. Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 7b00bca300043..c8441ffbb058a 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -59,7 +59,7 @@ __ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir, if (list_empty(&dir->list)) return; - sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT); + sbuf = kmalloc(STACK_BUF_SIZE, GFP_NOWAIT | __GFP_NOWARN); list_for_each_entry(tracker, &dir->list, head) ++total; @@ -154,11 +154,11 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, unsigned long entries[REF_TRACKER_STACK_ENTRIES]; struct ref_tracker *tracker; unsigned int nr_entries; - gfp_t gfp_mask = gfp; + gfp_t gfp_mask; unsigned long flags; - if (gfp & __GFP_DIRECT_RECLAIM) - gfp_mask |= __GFP_NOFAIL; + gfp |= __GFP_NOWARN; + gfp_mask = (gfp & __GFP_DIRECT_RECLAIM) ? (gfp | __GFP_NOFAIL) : gfp; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); if (unlikely(!tracker)) { pr_err_once("memory allocation failure, unreliable refcount tracker.\n"); @@ -191,7 +191,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); nr_entries = filter_irq_stacks(entries, nr_entries); - stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); + stack_handle = stack_depot_save(entries, nr_entries, + GFP_NOWAIT | __GFP_NOWARN); spin_lock_irqsave(&dir->lock, flags); if (tracker->dead) { -- 2.25.1
[PATCH v2 6/9] drm/i915: Separate wakeref tracking
From: Chris Wilson Extract the callstack tracking of intel_runtime_pm.c into its own utility so that that we can reuse it for other online debugging of scoped wakerefs. Signed-off-by: Chris Wilson Reviewed-by: Andrzej Hajda Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/Kconfig.debug | 9 + drivers/gpu/drm/i915/Makefile| 4 + drivers/gpu/drm/i915/intel_runtime_pm.c | 244 +++ drivers/gpu/drm/i915/intel_runtime_pm.h | 10 +- drivers/gpu/drm/i915/intel_wakeref.h | 6 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 ++ drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 ++ 7 files changed, 355 insertions(+), 228 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c create mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index e7fd3e76f8a20..8b1973146e848 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -33,6 +33,7 @@ config DRM_I915_DEBUG select PREEMPT_COUNT select I2C_CHARDEV select STACKDEPOT + select STACKTRACE select DRM_DP_AUX_CHARDEV select X86_MSR # used by igt/pm_rpm select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks) @@ -45,6 +46,7 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO + select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST @@ -235,11 +237,18 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". +config DRM_I915_TRACK_WAKEREF + depends on STACKDEPOT + depends on STACKTRACE + bool + config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n select STACKDEPOT + select STACKTRACE + select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3dc..88a403d3294cb 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -75,6 +75,10 @@ i915-$(CONFIG_DEBUG_FS) += \ i915_debugfs_params.o \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o + +i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ + intel_wakeref_tracker.o + i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 6ed5786bcd299..7bd10efa56bf3 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -52,182 +52,37 @@ #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) -#include - -#define STACKDEPTH 8 - -static noinline depot_stack_handle_t __save_depot_stack(void) -{ - unsigned long entries[STACKDEPTH]; - unsigned int n; - - n = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - return stack_depot_save(entries, n, GFP_NOWAIT | __GFP_NOWARN); -} - static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - spin_lock_init(&rpm->debug.lock); - stack_depot_init(); + intel_wakeref_tracker_init(&rpm->debug); } -static noinline depot_stack_handle_t +static intel_wakeref_t track_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - depot_stack_handle_t stack, *stacks; - unsigned long flags; - - if (rpm->no_wakeref_tracking) - return -1; - - stack = __save_depot_stack(); - if (!stack) + if (!rpm->available) return -1; - spin_lock_irqsave(&rpm->debug.lock, flags); - - if (!rpm->debug.count) - rpm->debug.last_acquire = stack; - - stacks = krealloc(rpm->debug.owners, - (rpm->debug.count + 1) * sizeof(*stacks), - GFP_NOWAIT | __GFP_NOWARN); - if (stacks) { - stacks[rpm->debug.count++] = stack; - rpm->debug.owners = stacks; - } else { - stack = -1; - } - - spin_unlock_irqrestore(&rpm->debug.lock, flags); - - return stack; + return intel_wakeref_tracker_add(&rpm->debug); } static void untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm, -depot_stack_handle_t stack) +intel_wakeref_t wakeref) { - struct drm_i915_private *i915 = conta
[PATCH v2 06/11] lib/ref_tracker: add printing to memory buffer
In case one wants to show stats via debugfs. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 8 ++ lib/ref_tracker.c | 56 +++-- 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index a2cf1f6309adb..2fdbfd2e14797 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -50,6 +50,8 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit); +int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size); + int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp); @@ -78,6 +80,12 @@ static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir, { } +static inline int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, + char *buf, size_t size) +{ + return 0; +} + static inline int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index ab1253fde244e..2ef4596b6b36f 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -62,8 +62,27 @@ ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit) return stats; } -void __ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) +struct ostream { + char *buf; + int size, used; +}; + +#define pr_ostream(stream, fmt, args...) \ +({ \ + struct ostream *_s = (stream); \ +\ + if (!_s->buf) { \ + pr_err(fmt, ##args); \ + } else { \ + int ret, len = _s->size - _s->used; \ + ret = snprintf(_s->buf + _s->used, len, pr_fmt(fmt), ##args); \ + _s->used += min(ret, len); \ + } \ +}) + +static void +__ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir, +unsigned int display_limit, struct ostream *s) { struct ref_tracker_dir_stats *stats; unsigned int i = 0, skipped; @@ -77,8 +96,8 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, stats = ref_tracker_get_stats(dir, display_limit); if (IS_ERR(stats)) { - pr_err("%s@%pK: couldn't get stats, error %pe\n", - dir->name, dir, stats); + pr_ostream(s, "%s@%pK: couldn't get stats, error %pe\n", + dir->name, dir, stats); return; } @@ -88,19 +107,27 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, stack = stats->stacks[i].stack_handle; if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4)) sbuf[0] = 0; - pr_err("%s@%pK has %d/%d users at\n%s\n", dir->name, dir, - stats->stacks[i].count, stats->total, sbuf); + pr_ostream(s, "%s@%pK has %d/%d users at\n%s\n", dir->name, dir, + stats->stacks[i].count, stats->total, sbuf); skipped -= stats->stacks[i].count; } if (skipped) - pr_err("%s@%pK skipped reports about %d/%d users.\n", - dir->name, dir, skipped, stats->total); + pr_ostream(s, "%s@%pK skipped reports about %d/%d users.\n", + dir->name, dir, skipped, stats->total); kfree(sbuf); kfree(stats); } + +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + struct ostream os = {}; + + __ref_tracker_dir_pr_ostream(dir, display_limit, &os); +} EXPORT_SYMBOL(__ref_tracker_dir_print); void ref_tracker_dir_print(struct ref_tracker_dir *dir, @@ -114,6 +141,19 @@ void ref_tracker_dir_print(struct ref_tracker_dir *dir, } EXPORT_SYMBOL(ref_tracker_dir_print); +int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size) +{ + struct ostream os = { .buf = buf, .size = size }; + unsigned long flags; + + spin_lock_irqsave(&dir->lock, flags); + __ref_tracker_dir_pr_ostream(dir, 16, &os); + spin_unlock_irqrestore(&dir->lock, flags); + + return os.used; +} +EXPORT_SYMBOL(ref_tracker_dir_snprint); + void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { struct ref_tracker *tracker, *n; -- 2.25.1
[PATCH v2 7/9] drm/i915: Track leaked gt->wakerefs
From: Chris Wilson Track every intel_gt_pm_get() until its corresponding release in intel_gt_pm_put() by returning a cookie to the caller for acquire that must be passed by on rleased. When there is an imbalance, we can see who either tried to free a stale wakeref, or who forgot to free theirs. v2: Rebase from backporting wakeref leak (Umesh) Signed-off-by: Chris Wilson Reviewed-by: Andrzej Hajda Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/Kconfig.debug| 15 +++ .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 7 ++-- .../i915/gem/selftests/i915_gem_coherency.c | 10 +++-- .../drm/i915/gem/selftests/i915_gem_mman.c| 14 --- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 13 -- .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 3 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 4 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 + .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 10 +++-- drivers/gpu/drm/i915/gt/intel_gt_pm.h | 36 drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 20 + drivers/gpu/drm/i915/gt/selftest_gt_pm.c | 5 ++- drivers/gpu/drm/i915/gt/selftest_reset.c | 10 +++-- drivers/gpu/drm/i915/gt/selftest_rps.c| 17 drivers/gpu/drm/i915/gt/selftest_slpc.c | 10 +++-- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 9 ++-- drivers/gpu/drm/i915/i915_pmu.c | 16 +++ drivers/gpu/drm/i915/intel_wakeref.c | 4 ++ drivers/gpu/drm/i915/intel_wakeref.h | 42 +++ 21 files changed, 182 insertions(+), 71 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 8b1973146e848..3bdc73f30a9e1 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -48,6 +48,7 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_MMIO select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM + select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST select BROKEN # for prototype uAPI @@ -257,3 +258,17 @@ config DRM_I915_DEBUG_RUNTIME_PM Recommended for driver developers only. If in doubt, say "N" + +config DRM_I915_DEBUG_WAKEREF + bool "Enable extra tracking for wakerefs" + depends on DRM_I915 + default n + select STACKDEPOT + select STACKTRACE + select DRM_I915_TRACK_WAKEREF + help + Choose this option to turn on extra state checking and usage + tracking for the wakerefPM functionality. This may introduce + overhead during driver runtime. + + If in doubt, say "N" diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 13c975da77474..4b6c144f706da 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -252,6 +252,7 @@ struct i915_execbuffer { struct intel_gt *gt; /* gt for the execbuf */ struct intel_context *context; /* logical state for the request */ struct i915_gem_context *gem_context; /** caller's context */ + intel_wakeref_t wakeref; /** our requests to build */ struct i915_request *requests[MAX_ENGINE_INSTANCE + 1]; @@ -2679,7 +2680,7 @@ eb_select_engine(struct i915_execbuffer *eb) for_each_child(ce, child) intel_context_get(child); - intel_gt_pm_get(ce->engine->gt); + eb->wakeref = intel_gt_pm_get(ce->engine->gt); if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) { err = intel_context_alloc_state(ce); @@ -2713,7 +2714,7 @@ eb_select_engine(struct i915_execbuffer *eb) return err; err: - intel_gt_pm_put(ce->engine->gt); + intel_gt_pm_put(ce->engine->gt, eb->wakeref); for_each_child(ce, child) intel_context_put(child); intel_context_put(ce); @@ -2725,7 +2726,7 @@ eb_put_engine(struct i915_execbuffer *eb) { struct intel_context *child; - intel_gt_pm_put(eb->gt); + intel_gt_pm_put(eb->context->engine->gt, eb->wakeref); for_each_child(eb->context, child) intel_context_put(child); intel_context_put(eb->context); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c index 13b088cc787eb..553f2730c2a76 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c @@ -85,6 +85,7 @@ static int cpu_get(struct context *ctx, unsigned long offset, u32 *v) static int gtt_set(struct context *ctx, unsigned long offset,
[PATCH v2 8/9] drm/i915: Correct type of wakeref variable
Wakeref has dedicated type. Assumption it will be int compatible forever is incorrect. Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 7799939c38945..b308dd0866eaf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2797,7 +2797,7 @@ static void destroyed_worker_func(struct work_struct *w) struct intel_guc *guc = container_of(w, struct intel_guc, submission_state.destroyed_worker); struct intel_gt *gt = guc_to_gt(guc); - int tmp; + intel_wakeref_t tmp; with_intel_gt_pm(gt, tmp) deregister_destroyed_contexts(guc); -- 2.25.1
[PATCH v2 07/11] lib/ref_tracker: remove warnings in case of allocation failure
Library can handle allocation failures. To avoid allocation warnings __GFP_NOWARN has been added everywhere. Moreover GFP_ATOMIC has been replaced with GFP_NOWAIT in case of stack allocation on tracker free call. Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 2ef4596b6b36f..cae4498fcfd70 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -189,7 +189,7 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, unsigned long entries[REF_TRACKER_STACK_ENTRIES]; struct ref_tracker *tracker; unsigned int nr_entries; - gfp_t gfp_mask = gfp; + gfp_t gfp_mask = gfp | __GFP_NOWARN; unsigned long flags; WARN_ON_ONCE(dir->dead); @@ -237,7 +237,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, return -EEXIST; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); + stack_handle = stack_depot_save(entries, nr_entries, + GFP_NOWAIT | __GFP_NOWARN); spin_lock_irqsave(&dir->lock, flags); if (tracker->dead) { -- 2.25.1
[PATCH v2 08/11] drm/i915: Separate wakeref tracking
From: Chris Wilson Extract the callstack tracking of intel_runtime_pm.c into its own utility so that that we can reuse it for other online debugging of scoped wakerefs. Signed-off-by: Chris Wilson Reviewed-by: Andrzej Hajda Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/Kconfig.debug | 9 + drivers/gpu/drm/i915/Makefile| 4 + drivers/gpu/drm/i915/intel_runtime_pm.c | 244 +++ drivers/gpu/drm/i915/intel_runtime_pm.h | 10 +- drivers/gpu/drm/i915/intel_wakeref.h | 6 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 ++ drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 ++ 7 files changed, 355 insertions(+), 228 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c create mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index e7fd3e76f8a20..8b1973146e848 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -33,6 +33,7 @@ config DRM_I915_DEBUG select PREEMPT_COUNT select I2C_CHARDEV select STACKDEPOT + select STACKTRACE select DRM_DP_AUX_CHARDEV select X86_MSR # used by igt/pm_rpm select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks) @@ -45,6 +46,7 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO + select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST @@ -235,11 +237,18 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". +config DRM_I915_TRACK_WAKEREF + depends on STACKDEPOT + depends on STACKTRACE + bool + config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n select STACKDEPOT + select STACKTRACE + select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3dc..88a403d3294cb 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -75,6 +75,10 @@ i915-$(CONFIG_DEBUG_FS) += \ i915_debugfs_params.o \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o + +i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ + intel_wakeref_tracker.o + i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 6ed5786bcd299..7bd10efa56bf3 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -52,182 +52,37 @@ #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) -#include - -#define STACKDEPTH 8 - -static noinline depot_stack_handle_t __save_depot_stack(void) -{ - unsigned long entries[STACKDEPTH]; - unsigned int n; - - n = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - return stack_depot_save(entries, n, GFP_NOWAIT | __GFP_NOWARN); -} - static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - spin_lock_init(&rpm->debug.lock); - stack_depot_init(); + intel_wakeref_tracker_init(&rpm->debug); } -static noinline depot_stack_handle_t +static intel_wakeref_t track_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - depot_stack_handle_t stack, *stacks; - unsigned long flags; - - if (rpm->no_wakeref_tracking) - return -1; - - stack = __save_depot_stack(); - if (!stack) + if (!rpm->available) return -1; - spin_lock_irqsave(&rpm->debug.lock, flags); - - if (!rpm->debug.count) - rpm->debug.last_acquire = stack; - - stacks = krealloc(rpm->debug.owners, - (rpm->debug.count + 1) * sizeof(*stacks), - GFP_NOWAIT | __GFP_NOWARN); - if (stacks) { - stacks[rpm->debug.count++] = stack; - rpm->debug.owners = stacks; - } else { - stack = -1; - } - - spin_unlock_irqrestore(&rpm->debug.lock, flags); - - return stack; + return intel_wakeref_tracker_add(&rpm->debug); } static void untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm, -depot_stack_handle_t stack) +intel_wakeref_t wakeref) { - struct drm_i915_private *i915 = conta
[PATCH v2 9/9] drm/i915: replace Intel internal tracker with kernel core ref_tracker
Beside reusing existing code, the main advantage of ref_tracker is tracking per instance of wakeref. It allows also to catch double put. On the other side we lose information about the first acquire and the last release, but the advantages outweigh it. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/Kconfig.debug| 11 +- drivers/gpu/drm/i915/Makefile | 3 - .../drm/i915/display/intel_display_power.c| 2 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +- drivers/gpu/drm/i915/intel_runtime_pm.h | 2 +- drivers/gpu/drm/i915/intel_wakeref.c | 8 +- drivers/gpu/drm/i915/intel_wakeref.h | 72 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 -- drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 -- 11 files changed, 86 insertions(+), 349 deletions(-) delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 3bdc73f30a9e1..6c57f3e265f20 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,7 @@ config DRM_I915_DEBUG select DEBUG_FS select PREEMPT_COUNT select I2C_CHARDEV + select REF_TRACKER select STACKDEPOT select STACKTRACE select DRM_DP_AUX_CHARDEV @@ -46,7 +47,6 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO - select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS @@ -238,18 +238,13 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". -config DRM_I915_TRACK_WAKEREF - depends on STACKDEPOT - depends on STACKTRACE - bool - config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during @@ -263,9 +258,9 @@ config DRM_I915_DEBUG_WAKEREF bool "Enable extra tracking for wakerefs" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking and usage tracking for the wakerefPM functionality. This may introduce diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 88a403d3294cb..1f8d71430e2e6 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -76,9 +76,6 @@ i915-$(CONFIG_DEBUG_FS) += \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o -i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ - intel_wakeref_tracker.o - i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 9ebae7ac32356..0e1bf724f89b5 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -2107,7 +2107,7 @@ print_async_put_domains_state(struct i915_power_domains *power_domains) struct drm_i915_private, power_domains); - drm_dbg(&i915->drm, "async_put_wakeref %u\n", + drm_dbg(&i915->drm, "async_put_wakeref %lu\n", power_domains->async_put_wakeref); print_power_domains(power_domains, "async_put_domains[0]", diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 52e46e7830ff5..cf8cc348942cb 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -273,7 +273,7 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) { struct intel_runtime_pm *rpm = engine->uncore->rpm; - intel_wakeref_init(&engine->wakeref, rpm, &wf_ops); + intel_wakeref_init(&engine->wakeref, rpm, &wf_ops, engine->name); intel_engine_init_heartbeat(engine); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index 7ee65a93f926f..01a055d0d0989 100644 --- a/drivers/gpu/drm/i915/
[PATCH v2 09/11] drm/i915: Track leaked gt->wakerefs
From: Chris Wilson Track every intel_gt_pm_get() until its corresponding release in intel_gt_pm_put() by returning a cookie to the caller for acquire that must be passed by on rleased. When there is an imbalance, we can see who either tried to free a stale wakeref, or who forgot to free theirs. v2: Rebase from backporting wakeref leak (Umesh) Signed-off-by: Chris Wilson Reviewed-by: Andrzej Hajda Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/Kconfig.debug| 15 +++ .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 7 ++-- .../i915/gem/selftests/i915_gem_coherency.c | 10 +++-- .../drm/i915/gem/selftests/i915_gem_mman.c| 14 --- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 13 -- .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 3 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 4 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 + .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 10 +++-- drivers/gpu/drm/i915/gt/intel_gt_pm.h | 36 drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 20 + drivers/gpu/drm/i915/gt/selftest_gt_pm.c | 5 ++- drivers/gpu/drm/i915/gt/selftest_reset.c | 10 +++-- drivers/gpu/drm/i915/gt/selftest_rps.c| 17 drivers/gpu/drm/i915/gt/selftest_slpc.c | 10 +++-- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 9 ++-- drivers/gpu/drm/i915/i915_pmu.c | 16 +++ drivers/gpu/drm/i915/intel_wakeref.c | 4 ++ drivers/gpu/drm/i915/intel_wakeref.h | 42 +++ 21 files changed, 182 insertions(+), 71 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 8b1973146e848..3bdc73f30a9e1 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -48,6 +48,7 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_MMIO select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM + select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST select BROKEN # for prototype uAPI @@ -257,3 +258,17 @@ config DRM_I915_DEBUG_RUNTIME_PM Recommended for driver developers only. If in doubt, say "N" + +config DRM_I915_DEBUG_WAKEREF + bool "Enable extra tracking for wakerefs" + depends on DRM_I915 + default n + select STACKDEPOT + select STACKTRACE + select DRM_I915_TRACK_WAKEREF + help + Choose this option to turn on extra state checking and usage + tracking for the wakerefPM functionality. This may introduce + overhead during driver runtime. + + If in doubt, say "N" diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 13c975da77474..4b6c144f706da 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -252,6 +252,7 @@ struct i915_execbuffer { struct intel_gt *gt; /* gt for the execbuf */ struct intel_context *context; /* logical state for the request */ struct i915_gem_context *gem_context; /** caller's context */ + intel_wakeref_t wakeref; /** our requests to build */ struct i915_request *requests[MAX_ENGINE_INSTANCE + 1]; @@ -2679,7 +2680,7 @@ eb_select_engine(struct i915_execbuffer *eb) for_each_child(ce, child) intel_context_get(child); - intel_gt_pm_get(ce->engine->gt); + eb->wakeref = intel_gt_pm_get(ce->engine->gt); if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) { err = intel_context_alloc_state(ce); @@ -2713,7 +2714,7 @@ eb_select_engine(struct i915_execbuffer *eb) return err; err: - intel_gt_pm_put(ce->engine->gt); + intel_gt_pm_put(ce->engine->gt, eb->wakeref); for_each_child(ce, child) intel_context_put(child); intel_context_put(ce); @@ -2725,7 +2726,7 @@ eb_put_engine(struct i915_execbuffer *eb) { struct intel_context *child; - intel_gt_pm_put(eb->gt); + intel_gt_pm_put(eb->context->engine->gt, eb->wakeref); for_each_child(eb->context, child) intel_context_put(child); intel_context_put(eb->context); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c index 13b088cc787eb..553f2730c2a76 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c @@ -85,6 +85,7 @@ static int cpu_get(struct context *ctx, unsigned long offset, u32 *v) static int gtt_set(struct context *ctx, unsigned long offset,
[PATCH v2 10/11] drm/i915: Correct type of wakeref variable
Wakeref has dedicated type. Assumption it will be int compatible forever is incorrect. Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 7799939c38945..b308dd0866eaf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2797,7 +2797,7 @@ static void destroyed_worker_func(struct work_struct *w) struct intel_guc *guc = container_of(w, struct intel_guc, submission_state.destroyed_worker); struct intel_gt *gt = guc_to_gt(guc); - int tmp; + intel_wakeref_t tmp; with_intel_gt_pm(gt, tmp) deregister_destroyed_contexts(guc); -- 2.25.1
[PATCH v2 11/11] drm/i915: replace Intel internal tracker with kernel core ref_tracker
Beside reusing existing code, the main advantage of ref_tracker is tracking per instance of wakeref. It allows also to catch double put. On the other side we lose information about the first acquire and the last release, but the advantages outweigh it. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/Kconfig.debug| 11 +- drivers/gpu/drm/i915/Makefile | 3 - .../drm/i915/display/intel_display_power.c| 2 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 25 +- drivers/gpu/drm/i915/intel_runtime_pm.h | 2 +- drivers/gpu/drm/i915/intel_wakeref.c | 8 +- drivers/gpu/drm/i915/intel_wakeref.h | 72 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 -- drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 -- 11 files changed, 87 insertions(+), 350 deletions(-) delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 3bdc73f30a9e1..6c57f3e265f20 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,7 @@ config DRM_I915_DEBUG select DEBUG_FS select PREEMPT_COUNT select I2C_CHARDEV + select REF_TRACKER select STACKDEPOT select STACKTRACE select DRM_DP_AUX_CHARDEV @@ -46,7 +47,6 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO - select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS @@ -238,18 +238,13 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". -config DRM_I915_TRACK_WAKEREF - depends on STACKDEPOT - depends on STACKTRACE - bool - config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during @@ -263,9 +258,9 @@ config DRM_I915_DEBUG_WAKEREF bool "Enable extra tracking for wakerefs" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking and usage tracking for the wakerefPM functionality. This may introduce diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 88a403d3294cb..1f8d71430e2e6 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -76,9 +76,6 @@ i915-$(CONFIG_DEBUG_FS) += \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o -i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ - intel_wakeref_tracker.o - i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 9ebae7ac32356..0e1bf724f89b5 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -2107,7 +2107,7 @@ print_async_put_domains_state(struct i915_power_domains *power_domains) struct drm_i915_private, power_domains); - drm_dbg(&i915->drm, "async_put_wakeref %u\n", + drm_dbg(&i915->drm, "async_put_wakeref %lu\n", power_domains->async_put_wakeref); print_power_domains(power_domains, "async_put_domains[0]", diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 52e46e7830ff5..cf8cc348942cb 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -273,7 +273,7 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) { struct intel_runtime_pm *rpm = engine->uncore->rpm; - intel_wakeref_init(&engine->wakeref, rpm, &wf_ops); + intel_wakeref_init(&engine->wakeref, rpm, &wf_ops, engine->name); intel_engine_init_heartbeat(engine); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index 7ee65a93f926f..01a055d0d0989 100644 --- a/drivers/gpu/drm/i915/
Re: [PATCH v2 00/11] drm/i915: use ref_tracker library for tracking wakerefs
On 22.02.2022 00:16, Andrzej Hajda wrote: Hi, Appearance of ref_tracker library allows to drop custom solution for wakeref tracking used in i915 and reuse the library. For this few adjustements has been made to ref_tracker, details in patches. I hope changes are OK for original author. The patchset has been rebased on top of drm-tip to allow test changes by CI. Patches marked "[DO NOT MERGE]" are cherry-picked from linux-next (they are not yet in drm-tip), to allow build and run CI on the patchset (it works only on drm-tip tree). Added CC to netdev as the only user of the library atm. v2: - replaced list_sort with ref_tracker_dir_stats, to avoid potentially extensive sorting, if number of reports is expected to be big enough (???) we can replace linear search in ref_tracker_dir_stats.stacks with binary heap (min_heap), - refactored gfp flags, - fixed i915 handling of no-tracking flag. Regards Andrzej Sorry for the mess, sth wrong happened to my scripts and I've messed patches, I will resend it properly. Regards Andrzej
[PATCH v3 00/11] drm/i915: use ref_tracker library for tracking wakerefs
Hi, Appearance of ref_tracker library allows to drop custom solution for wakeref tracking used in i915 and reuse the library. For this few adjustements has been made to ref_tracker, details in patches. I hope changes are OK for original author. The patchset has been rebased on top of drm-tip to allow test changes by CI. Patches marked "[DO NOT MERGE]" are cherry-picked from linux-next (they are not yet in drm-tip), to allow build and run CI on the patchset (it works only on drm-tip tree). Added CC to netdev as the only user of the library atm. v2: - replaced list_sort with ref_tracker_dir_stats, to avoid potentially extensive sorting, if number of reports is expected to be big enough (???) we can replace linear search in ref_tracker_dir_stats.stacks with binary heap (min_heap), - refactored gfp flags, - fixed i915 handling of no-tracking flag. v3: - fixed mess with duplicated mails Regards Andrzej Andrzej Hajda (6): lib/ref_tracker: add unlocked leak print helper lib/ref_tracker: __ref_tracker_dir_print improve printing lib/ref_tracker: add printing to memory buffer lib/ref_tracker: remove warnings in case of allocation failure drm/i915: Correct type of wakeref variable drm/i915: replace Intel internal tracker with kernel core ref_tracker Chris Wilson (2): drm/i915: Separate wakeref tracking drm/i915: Track leaked gt->wakerefs Eric Dumazet (3): [DO NOT MERGE] ref_tracker: implement use-after-free detection [DO NOT MERGE] ref_tracker: add a count of untracked references [DO NOT MERGE] ref_tracker: remove filter_irq_stacks() call drivers/gpu/drm/i915/Kconfig.debug| 19 ++ drivers/gpu/drm/i915/Makefile | 1 + .../drm/i915/display/intel_display_power.c| 2 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 7 +- .../i915/gem/selftests/i915_gem_coherency.c | 10 +- .../drm/i915/gem/selftests/i915_gem_mman.c| 14 +- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 13 +- .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 3 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 6 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 + .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 +- drivers/gpu/drm/i915/gt/intel_gt_pm.h | 36 ++- drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 20 +- drivers/gpu/drm/i915/gt/selftest_gt_pm.c | 5 +- drivers/gpu/drm/i915/gt/selftest_reset.c | 10 +- drivers/gpu/drm/i915/gt/selftest_rps.c| 17 +- drivers/gpu/drm/i915/gt/selftest_slpc.c | 10 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 +- drivers/gpu/drm/i915/i915_pmu.c | 16 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 239 ++ drivers/gpu/drm/i915/intel_runtime_pm.h | 10 +- drivers/gpu/drm/i915/intel_wakeref.c | 10 +- drivers/gpu/drm/i915/intel_wakeref.h | 112 +++- include/linux/ref_tracker.h | 35 ++- lib/ref_tracker.c | 198 --- 27 files changed, 480 insertions(+), 344 deletions(-) -- 2.25.1
[PATCH v3 01/11] [DO NOT MERGE] ref_tracker: implement use-after-free detection
From: Eric Dumazet Whenever ref_tracker_dir_init() is called, mark the struct ref_tracker_dir as dead. Test the dead status from ref_tracker_alloc() and ref_tracker_free() This should detect buggy dev_put()/dev_hold() happening too late in netdevice dismantle process. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 2 ++ lib/ref_tracker.c | 5 + 2 files changed, 7 insertions(+) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 60f3453be23e6..a443abda937d8 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -13,6 +13,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned intquarantine_avail; refcount_t untracked; + booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ #endif @@ -26,6 +27,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, INIT_LIST_HEAD(&dir->quarantine); spin_lock_init(&dir->lock); dir->quarantine_avail = quarantine_count; + dir->dead = false; refcount_set(&dir->untracked, 1); stack_depot_init(); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index a6789c0c626b0..32ff6bd497f8e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -20,6 +20,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) unsigned long flags; bool leak = false; + dir->dead = true; spin_lock_irqsave(&dir->lock, flags); list_for_each_entry_safe(tracker, n, &dir->quarantine, head) { list_del(&tracker->head); @@ -72,6 +73,8 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, gfp_t gfp_mask = gfp; unsigned long flags; + WARN_ON_ONCE(dir->dead); + if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); @@ -100,6 +103,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, unsigned int nr_entries; unsigned long flags; + WARN_ON_ONCE(dir->dead); + if (!tracker) { refcount_dec(&dir->untracked); return -EEXIST; -- 2.25.1
[PATCH v3 02/11] [DO NOT MERGE] ref_tracker: add a count of untracked references
From: Eric Dumazet We are still chasing a netdev refcount imbalance, and we suspect we have one rogue dev_put() that is consuming a reference taken from a dev_hold_track() To detect this case, allow ref_tracker_alloc() and ref_tracker_free() to be called with a NULL @trackerp parameter, and use a dedicated refcount_t just for them. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 2 ++ lib/ref_tracker.c | 12 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index a443abda937d8..9ca353ab712b5 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -13,6 +13,7 @@ struct ref_tracker_dir { spinlock_t lock; unsigned intquarantine_avail; refcount_t untracked; + refcount_t no_tracker; booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ @@ -29,6 +30,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, dir->quarantine_avail = quarantine_count; dir->dead = false; refcount_set(&dir->untracked, 1); + refcount_set(&dir->no_tracker, 1); stack_depot_init(); } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 32ff6bd497f8e..9c0c2e09df666 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -38,6 +38,7 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) spin_unlock_irqrestore(&dir->lock, flags); WARN_ON_ONCE(leak); WARN_ON_ONCE(refcount_read(&dir->untracked) != 1); + WARN_ON_ONCE(refcount_read(&dir->no_tracker) != 1); } EXPORT_SYMBOL(ref_tracker_dir_exit); @@ -75,6 +76,10 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, WARN_ON_ONCE(dir->dead); + if (!trackerp) { + refcount_inc(&dir->no_tracker); + return 0; + } if (gfp & __GFP_DIRECT_RECLAIM) gfp_mask |= __GFP_NOFAIL; *trackerp = tracker = kzalloc(sizeof(*tracker), gfp_mask); @@ -98,13 +103,18 @@ int ref_tracker_free(struct ref_tracker_dir *dir, struct ref_tracker **trackerp) { unsigned long entries[REF_TRACKER_STACK_ENTRIES]; - struct ref_tracker *tracker = *trackerp; depot_stack_handle_t stack_handle; + struct ref_tracker *tracker; unsigned int nr_entries; unsigned long flags; WARN_ON_ONCE(dir->dead); + if (!trackerp) { + refcount_dec(&dir->no_tracker); + return 0; + } + tracker = *trackerp; if (!tracker) { refcount_dec(&dir->untracked); return -EEXIST; -- 2.25.1
[PATCH v3 03/11] [DO NOT MERGE] ref_tracker: remove filter_irq_stacks() call
From: Eric Dumazet After commit e94006608949 ("lib/stackdepot: always do filter_irq_stacks() in stack_depot_save()") it became unnecessary to filter the stack before calling stack_depot_save(). Signed-off-by: Eric Dumazet Cc: Marco Elver Cc: Alexander Potapenko Cc: Dmitry Vyukov Signed-off-by: David S. Miller Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 9c0c2e09df666..dc7b14aa3431e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -89,7 +89,6 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, return -ENOMEM; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - nr_entries = filter_irq_stacks(entries, nr_entries); tracker->alloc_stack_handle = stack_depot_save(entries, nr_entries, gfp); spin_lock_irqsave(&dir->lock, flags); @@ -120,7 +119,6 @@ int ref_tracker_free(struct ref_tracker_dir *dir, return -EEXIST; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - nr_entries = filter_irq_stacks(entries, nr_entries); stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); spin_lock_irqsave(&dir->lock, flags); -- 2.25.1
[PATCH v3 04/11] lib/ref_tracker: add unlocked leak print helper
To have reliable detection of leaks, caller must be able to check under the same lock both: tracked counter and the leaks. dir.lock is natural candidate for such lock and unlocked print helper can be called with this lock taken. As a bonus we can reuse this helper in ref_tracker_dir_exit. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 8 + lib/ref_tracker.c | 66 + 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 9ca353ab712b5..3e9e9df2a41f5 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -36,6 +36,9 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, void ref_tracker_dir_exit(struct ref_tracker_dir *dir); +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit); + void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit); @@ -56,6 +59,11 @@ static inline void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { } +static inline void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ +} + static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit) { diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index dc7b14aa3431e..5e9f90bbf771b 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -14,6 +14,38 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + struct ref_tracker *tracker; + unsigned int i = 0; + + lockdep_assert_held(&dir->lock); + + list_for_each_entry(tracker, &dir->list, head) { + if (i < display_limit) { + pr_err("leaked reference.\n"); + if (tracker->alloc_stack_handle) + stack_depot_print(tracker->alloc_stack_handle); + i++; + } else { + break; + } + } +} +EXPORT_SYMBOL(__ref_tracker_dir_print); + +void ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + unsigned long flags; + + spin_lock_irqsave(&dir->lock, flags); + __ref_tracker_dir_print(dir, display_limit); + spin_unlock_irqrestore(&dir->lock, flags); +} +EXPORT_SYMBOL(ref_tracker_dir_print); + void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { struct ref_tracker *tracker, *n; @@ -27,13 +59,13 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) kfree(tracker); dir->quarantine_avail++; } - list_for_each_entry_safe(tracker, n, &dir->list, head) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); + if (!list_empty(&dir->list)) { + __ref_tracker_dir_print(dir, 16); leak = true; - list_del(&tracker->head); - kfree(tracker); + list_for_each_entry_safe(tracker, n, &dir->list, head) { + list_del(&tracker->head); + kfree(tracker); + } } spin_unlock_irqrestore(&dir->lock, flags); WARN_ON_ONCE(leak); @@ -42,28 +74,6 @@ void ref_tracker_dir_exit(struct ref_tracker_dir *dir) } EXPORT_SYMBOL(ref_tracker_dir_exit); -void ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) -{ - struct ref_tracker *tracker; - unsigned long flags; - unsigned int i = 0; - - spin_lock_irqsave(&dir->lock, flags); - list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { - break; - } - } - spin_unlock_irqrestore(&dir->lock, flags); -} -EXPORT_SYMBOL(ref_tracker_dir_print); - int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp) -- 2.25.1
[PATCH v3 05/11] lib/ref_tracker: __ref_tracker_dir_print improve printing
To improve readibility of ref_tracker printing following changes have been performed: - reports are printed per stack_handle - log is more compact, - added display name for ref_tracker_dir, - stack trace is printed indented, in the same printk call, - total number of references is printed every time, - print info about dropped references. Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 15 +-- lib/ref_tracker.c | 90 - 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 3e9e9df2a41f5..a2cf1f6309adb 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -17,12 +17,19 @@ struct ref_tracker_dir { booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ + charname[32]; #endif }; #ifdef CONFIG_REF_TRACKER -static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + +// Temporary allow two and three arguments, until consumers are converted +#define ref_tracker_dir_init(_d, _q, args...) _ref_tracker_dir_init(_d, _q, ##args, #_d) +#define _ref_tracker_dir_init(_d, _q, _n, ...) __ref_tracker_dir_init(_d, _q, _n) + +static inline void __ref_tracker_dir_init(struct ref_tracker_dir *dir, + unsigned int quarantine_count, + const char *name) { INIT_LIST_HEAD(&dir->list); INIT_LIST_HEAD(&dir->quarantine); @@ -31,6 +38,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, dir->dead = false; refcount_set(&dir->untracked, 1); refcount_set(&dir->no_tracker, 1); + strlcpy(dir->name, name, sizeof(dir->name)); stack_depot_init(); } @@ -51,7 +59,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, #else /* CONFIG_REF_TRACKER */ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + unsigned int quarantine_count, + ...) { } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 5e9f90bbf771b..ab1253fde244e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,11 +1,16 @@ // SPDX-License-Identifier: GPL-2.0-or-later + +#define pr_fmt(fmt) "ref_tracker: " fmt + #include +#include #include #include #include #include #define REF_TRACKER_STACK_ENTRIES 16 +#define STACK_BUF_SIZE 1024 struct ref_tracker { struct list_headhead; /* anchor into dir->list or dir->quarantine */ @@ -14,24 +19,87 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; -void __ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) +struct ref_tracker_dir_stats { + int total; + int count; + struct { + depot_stack_handle_t stack_handle; + unsigned int count; + } stacks[]; +}; + +static struct ref_tracker_dir_stats * +ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit) { + struct ref_tracker_dir_stats *stats; struct ref_tracker *tracker; - unsigned int i = 0; - lockdep_assert_held(&dir->lock); + stats = kmalloc(struct_size(stats, stacks, limit), + GFP_NOWAIT | __GFP_NOWARN); + if (!stats) + return ERR_PTR(-ENOMEM); + stats->total = 0; + stats->count = 0; list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { - break; + depot_stack_handle_t stack = tracker->alloc_stack_handle; + int i; + + ++stats->total; + for (i = 0; i < stats->count; ++i) + if (stats->stacks[i].stack_handle == stack) + break; + if (i >= limit) + continue; + if (i >= stats->count) { + stats->stacks[i].stack_handle = stack; + stats->stacks[i].count = 0; + ++stats->count; } + ++stats->stacks[i].count; + } + + return stats; +} + +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, +
[PATCH v3 06/11] lib/ref_tracker: add printing to memory buffer
In case one wants to show stats via debugfs. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- include/linux/ref_tracker.h | 8 ++ lib/ref_tracker.c | 56 +++-- 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index a2cf1f6309adb..2fdbfd2e14797 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -50,6 +50,8 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, void ref_tracker_dir_print(struct ref_tracker_dir *dir, unsigned int display_limit); +int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size); + int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp); @@ -78,6 +80,12 @@ static inline void ref_tracker_dir_print(struct ref_tracker_dir *dir, { } +static inline int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, + char *buf, size_t size) +{ + return 0; +} + static inline int ref_tracker_alloc(struct ref_tracker_dir *dir, struct ref_tracker **trackerp, gfp_t gfp) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index ab1253fde244e..2ef4596b6b36f 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -62,8 +62,27 @@ ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit) return stats; } -void __ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) +struct ostream { + char *buf; + int size, used; +}; + +#define pr_ostream(stream, fmt, args...) \ +({ \ + struct ostream *_s = (stream); \ +\ + if (!_s->buf) { \ + pr_err(fmt, ##args); \ + } else { \ + int ret, len = _s->size - _s->used; \ + ret = snprintf(_s->buf + _s->used, len, pr_fmt(fmt), ##args); \ + _s->used += min(ret, len); \ + } \ +}) + +static void +__ref_tracker_dir_pr_ostream(struct ref_tracker_dir *dir, +unsigned int display_limit, struct ostream *s) { struct ref_tracker_dir_stats *stats; unsigned int i = 0, skipped; @@ -77,8 +96,8 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, stats = ref_tracker_get_stats(dir, display_limit); if (IS_ERR(stats)) { - pr_err("%s@%pK: couldn't get stats, error %pe\n", - dir->name, dir, stats); + pr_ostream(s, "%s@%pK: couldn't get stats, error %pe\n", + dir->name, dir, stats); return; } @@ -88,19 +107,27 @@ void __ref_tracker_dir_print(struct ref_tracker_dir *dir, stack = stats->stacks[i].stack_handle; if (sbuf && !stack_depot_snprint(stack, sbuf, STACK_BUF_SIZE, 4)) sbuf[0] = 0; - pr_err("%s@%pK has %d/%d users at\n%s\n", dir->name, dir, - stats->stacks[i].count, stats->total, sbuf); + pr_ostream(s, "%s@%pK has %d/%d users at\n%s\n", dir->name, dir, + stats->stacks[i].count, stats->total, sbuf); skipped -= stats->stacks[i].count; } if (skipped) - pr_err("%s@%pK skipped reports about %d/%d users.\n", - dir->name, dir, skipped, stats->total); + pr_ostream(s, "%s@%pK skipped reports about %d/%d users.\n", + dir->name, dir, skipped, stats->total); kfree(sbuf); kfree(stats); } + +void __ref_tracker_dir_print(struct ref_tracker_dir *dir, + unsigned int display_limit) +{ + struct ostream os = {}; + + __ref_tracker_dir_pr_ostream(dir, display_limit, &os); +} EXPORT_SYMBOL(__ref_tracker_dir_print); void ref_tracker_dir_print(struct ref_tracker_dir *dir, @@ -114,6 +141,19 @@ void ref_tracker_dir_print(struct ref_tracker_dir *dir, } EXPORT_SYMBOL(ref_tracker_dir_print); +int ref_tracker_dir_snprint(struct ref_tracker_dir *dir, char *buf, size_t size) +{ + struct ostream os = { .buf = buf, .size = size }; + unsigned long flags; + + spin_lock_irqsave(&dir->lock, flags); + __ref_tracker_dir_pr_ostream(dir, 16, &os); + spin_unlock_irqrestore(&dir->lock, flags); + + return os.used; +} +EXPORT_SYMBOL(ref_tracker_dir_snprint); + void ref_tracker_dir_exit(struct ref_tracker_dir *dir) { struct ref_tracker *tracker, *n; -- 2.25.1
[PATCH v3 09/11] drm/i915: Track leaked gt->wakerefs
From: Chris Wilson Track every intel_gt_pm_get() until its corresponding release in intel_gt_pm_put() by returning a cookie to the caller for acquire that must be passed by on rleased. When there is an imbalance, we can see who either tried to free a stale wakeref, or who forgot to free theirs. v2: Rebase from backporting wakeref leak (Umesh) Signed-off-by: Chris Wilson Reviewed-by: Andrzej Hajda Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/Kconfig.debug| 15 +++ .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 7 ++-- .../i915/gem/selftests/i915_gem_coherency.c | 10 +++-- .../drm/i915/gem/selftests/i915_gem_mman.c| 14 --- drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 13 -- .../gpu/drm/i915/gt/intel_breadcrumbs_types.h | 3 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 4 +- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 + .../drm/i915/gt/intel_execlists_submission.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 10 +++-- drivers/gpu/drm/i915/gt/intel_gt_pm.h | 36 drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 4 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 20 + drivers/gpu/drm/i915/gt/selftest_gt_pm.c | 5 ++- drivers/gpu/drm/i915/gt/selftest_reset.c | 10 +++-- drivers/gpu/drm/i915/gt/selftest_rps.c| 17 drivers/gpu/drm/i915/gt/selftest_slpc.c | 10 +++-- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 9 ++-- drivers/gpu/drm/i915/i915_pmu.c | 16 +++ drivers/gpu/drm/i915/intel_wakeref.c | 4 ++ drivers/gpu/drm/i915/intel_wakeref.h | 42 +++ 21 files changed, 182 insertions(+), 71 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 8b1973146e848..3bdc73f30a9e1 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -48,6 +48,7 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_MMIO select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM + select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST select BROKEN # for prototype uAPI @@ -257,3 +258,17 @@ config DRM_I915_DEBUG_RUNTIME_PM Recommended for driver developers only. If in doubt, say "N" + +config DRM_I915_DEBUG_WAKEREF + bool "Enable extra tracking for wakerefs" + depends on DRM_I915 + default n + select STACKDEPOT + select STACKTRACE + select DRM_I915_TRACK_WAKEREF + help + Choose this option to turn on extra state checking and usage + tracking for the wakerefPM functionality. This may introduce + overhead during driver runtime. + + If in doubt, say "N" diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 13c975da77474..4b6c144f706da 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -252,6 +252,7 @@ struct i915_execbuffer { struct intel_gt *gt; /* gt for the execbuf */ struct intel_context *context; /* logical state for the request */ struct i915_gem_context *gem_context; /** caller's context */ + intel_wakeref_t wakeref; /** our requests to build */ struct i915_request *requests[MAX_ENGINE_INSTANCE + 1]; @@ -2679,7 +2680,7 @@ eb_select_engine(struct i915_execbuffer *eb) for_each_child(ce, child) intel_context_get(child); - intel_gt_pm_get(ce->engine->gt); + eb->wakeref = intel_gt_pm_get(ce->engine->gt); if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) { err = intel_context_alloc_state(ce); @@ -2713,7 +2714,7 @@ eb_select_engine(struct i915_execbuffer *eb) return err; err: - intel_gt_pm_put(ce->engine->gt); + intel_gt_pm_put(ce->engine->gt, eb->wakeref); for_each_child(ce, child) intel_context_put(child); intel_context_put(ce); @@ -2725,7 +2726,7 @@ eb_put_engine(struct i915_execbuffer *eb) { struct intel_context *child; - intel_gt_pm_put(eb->gt); + intel_gt_pm_put(eb->context->engine->gt, eb->wakeref); for_each_child(eb->context, child) intel_context_put(child); intel_context_put(eb->context); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c index 13b088cc787eb..553f2730c2a76 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c @@ -85,6 +85,7 @@ static int cpu_get(struct context *ctx, unsigned long offset, u32 *v) static int gtt_set(struct context *ctx, unsigned long offset,
[PATCH v3 08/11] drm/i915: Separate wakeref tracking
From: Chris Wilson Extract the callstack tracking of intel_runtime_pm.c into its own utility so that that we can reuse it for other online debugging of scoped wakerefs. Signed-off-by: Chris Wilson Reviewed-by: Andrzej Hajda Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/Kconfig.debug | 9 + drivers/gpu/drm/i915/Makefile| 4 + drivers/gpu/drm/i915/intel_runtime_pm.c | 244 +++ drivers/gpu/drm/i915/intel_runtime_pm.h | 10 +- drivers/gpu/drm/i915/intel_wakeref.h | 6 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 ++ drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 ++ 7 files changed, 355 insertions(+), 228 deletions(-) create mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c create mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index e7fd3e76f8a20..8b1973146e848 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -33,6 +33,7 @@ config DRM_I915_DEBUG select PREEMPT_COUNT select I2C_CHARDEV select STACKDEPOT + select STACKTRACE select DRM_DP_AUX_CHARDEV select X86_MSR # used by igt/pm_rpm select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks) @@ -45,6 +46,7 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO + select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST @@ -235,11 +237,18 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". +config DRM_I915_TRACK_WAKEREF + depends on STACKDEPOT + depends on STACKTRACE + bool + config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n select STACKDEPOT + select STACKTRACE + select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3dc..88a403d3294cb 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -75,6 +75,10 @@ i915-$(CONFIG_DEBUG_FS) += \ i915_debugfs_params.o \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o + +i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ + intel_wakeref_tracker.o + i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 6ed5786bcd299..7bd10efa56bf3 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -52,182 +52,37 @@ #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_RUNTIME_PM) -#include - -#define STACKDEPTH 8 - -static noinline depot_stack_handle_t __save_depot_stack(void) -{ - unsigned long entries[STACKDEPTH]; - unsigned int n; - - n = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - return stack_depot_save(entries, n, GFP_NOWAIT | __GFP_NOWARN); -} - static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - spin_lock_init(&rpm->debug.lock); - stack_depot_init(); + intel_wakeref_tracker_init(&rpm->debug); } -static noinline depot_stack_handle_t +static intel_wakeref_t track_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - depot_stack_handle_t stack, *stacks; - unsigned long flags; - - if (rpm->no_wakeref_tracking) - return -1; - - stack = __save_depot_stack(); - if (!stack) + if (!rpm->available) return -1; - spin_lock_irqsave(&rpm->debug.lock, flags); - - if (!rpm->debug.count) - rpm->debug.last_acquire = stack; - - stacks = krealloc(rpm->debug.owners, - (rpm->debug.count + 1) * sizeof(*stacks), - GFP_NOWAIT | __GFP_NOWARN); - if (stacks) { - stacks[rpm->debug.count++] = stack; - rpm->debug.owners = stacks; - } else { - stack = -1; - } - - spin_unlock_irqrestore(&rpm->debug.lock, flags); - - return stack; + return intel_wakeref_tracker_add(&rpm->debug); } static void untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm, -depot_stack_handle_t stack) +intel_wakeref_t wakeref) { - struct drm_i915_private *i915 = conta
[PATCH v3 07/11] lib/ref_tracker: remove warnings in case of allocation failure
Library can handle allocation failures. To avoid allocation warnings __GFP_NOWARN has been added everywhere. Moreover GFP_ATOMIC has been replaced with GFP_NOWAIT in case of stack allocation on tracker free call. Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 2ef4596b6b36f..cae4498fcfd70 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -189,7 +189,7 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, unsigned long entries[REF_TRACKER_STACK_ENTRIES]; struct ref_tracker *tracker; unsigned int nr_entries; - gfp_t gfp_mask = gfp; + gfp_t gfp_mask = gfp | __GFP_NOWARN; unsigned long flags; WARN_ON_ONCE(dir->dead); @@ -237,7 +237,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, return -EEXIST; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); + stack_handle = stack_depot_save(entries, nr_entries, + GFP_NOWAIT | __GFP_NOWARN); spin_lock_irqsave(&dir->lock, flags); if (tracker->dead) { -- 2.25.1
[PATCH v3 10/11] drm/i915: Correct type of wakeref variable
Wakeref has dedicated type. Assumption it will be int compatible forever is incorrect. Signed-off-by: Andrzej Hajda --- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 7799939c38945..b308dd0866eaf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2797,7 +2797,7 @@ static void destroyed_worker_func(struct work_struct *w) struct intel_guc *guc = container_of(w, struct intel_guc, submission_state.destroyed_worker); struct intel_gt *gt = guc_to_gt(guc); - int tmp; + intel_wakeref_t tmp; with_intel_gt_pm(gt, tmp) deregister_destroyed_contexts(guc); -- 2.25.1
[PATCH v3 11/11] drm/i915: replace Intel internal tracker with kernel core ref_tracker
Beside reusing existing code, the main advantage of ref_tracker is tracking per instance of wakeref. It allows also to catch double put. On the other side we lose information about the first acquire and the last release, but the advantages outweigh it. Signed-off-by: Andrzej Hajda Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/Kconfig.debug| 11 +- drivers/gpu/drm/i915/Makefile | 3 - .../drm/i915/display/intel_display_power.c| 2 +- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 25 +- drivers/gpu/drm/i915/intel_runtime_pm.h | 2 +- drivers/gpu/drm/i915/intel_wakeref.c | 8 +- drivers/gpu/drm/i915/intel_wakeref.h | 72 +- drivers/gpu/drm/i915/intel_wakeref_tracker.c | 234 -- drivers/gpu/drm/i915/intel_wakeref_tracker.h | 76 -- 11 files changed, 87 insertions(+), 350 deletions(-) delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.c delete mode 100644 drivers/gpu/drm/i915/intel_wakeref_tracker.h diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 3bdc73f30a9e1..6c57f3e265f20 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,7 @@ config DRM_I915_DEBUG select DEBUG_FS select PREEMPT_COUNT select I2C_CHARDEV + select REF_TRACKER select STACKDEPOT select STACKTRACE select DRM_DP_AUX_CHARDEV @@ -46,7 +47,6 @@ config DRM_I915_DEBUG select DRM_I915_DEBUG_GEM select DRM_I915_DEBUG_GEM_ONCE select DRM_I915_DEBUG_MMIO - select DRM_I915_TRACK_WAKEREF select DRM_I915_DEBUG_RUNTIME_PM select DRM_I915_DEBUG_WAKEREF select DRM_I915_SW_FENCE_DEBUG_OBJECTS @@ -238,18 +238,13 @@ config DRM_I915_DEBUG_VBLANK_EVADE If in doubt, say "N". -config DRM_I915_TRACK_WAKEREF - depends on STACKDEPOT - depends on STACKTRACE - bool - config DRM_I915_DEBUG_RUNTIME_PM bool "Enable extra state checking for runtime PM" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking for the runtime PM functionality. This may introduce overhead during @@ -263,9 +258,9 @@ config DRM_I915_DEBUG_WAKEREF bool "Enable extra tracking for wakerefs" depends on DRM_I915 default n + select REF_TRACKER select STACKDEPOT select STACKTRACE - select DRM_I915_TRACK_WAKEREF help Choose this option to turn on extra state checking and usage tracking for the wakerefPM functionality. This may introduce diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 88a403d3294cb..1f8d71430e2e6 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -76,9 +76,6 @@ i915-$(CONFIG_DEBUG_FS) += \ display/intel_display_debugfs.o \ display/intel_pipe_crc.o -i915-$(CONFIG_DRM_I915_TRACK_WAKEREF) += \ - intel_wakeref_tracker.o - i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # "Graphics Technology" (aka we talk to the gpu) diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 9ebae7ac32356..0e1bf724f89b5 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -2107,7 +2107,7 @@ print_async_put_domains_state(struct i915_power_domains *power_domains) struct drm_i915_private, power_domains); - drm_dbg(&i915->drm, "async_put_wakeref %u\n", + drm_dbg(&i915->drm, "async_put_wakeref %lu\n", power_domains->async_put_wakeref); print_power_domains(power_domains, "async_put_domains[0]", diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index 52e46e7830ff5..cf8cc348942cb 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -273,7 +273,7 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) { struct intel_runtime_pm *rpm = engine->uncore->rpm; - intel_wakeref_init(&engine->wakeref, rpm, &wf_ops); + intel_wakeref_init(&engine->wakeref, rpm, &wf_ops, engine->name); intel_engine_init_heartbeat(engine); } diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index 7ee65a93f926f..01a055d0d0989 100644 --- a/drivers/gpu/drm/i915/
Re: [Intel-gfx] [PATCH v3 08/11] drm/i915: Separate wakeref tracking
On 22.02.2022 08:12, Ville Syrjälä wrote: On Tue, Feb 22, 2022 at 12:25:39AM +0100, Andrzej Hajda wrote: -static noinline depot_stack_handle_t +static intel_wakeref_t track_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm) { - depot_stack_handle_t stack, *stacks; - unsigned long flags; - - if (rpm->no_wakeref_tracking) - return -1; - - stack = __save_depot_stack(); - if (!stack) + if (!rpm->available) return -1; Still not the same. It was fixed but in wrong place - patch 11. I will move the change here. Regards Andrzej
Re: [PATCH v3 05/11] lib/ref_tracker: __ref_tracker_dir_print improve printing
On 22.02.2022 01:08, Eric Dumazet wrote: On Mon, Feb 21, 2022 at 3:26 PM Andrzej Hajda wrote: To improve readibility of ref_tracker printing following changes readability have been performed: - reports are printed per stack_handle - log is more compact, - added display name for ref_tracker_dir, - stack trace is printed indented, in the same printk call, - total number of references is printed every time, - print info about dropped references. Signed-off-by: Andrzej Hajda --- include/linux/ref_tracker.h | 15 +-- lib/ref_tracker.c | 90 - 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/include/linux/ref_tracker.h b/include/linux/ref_tracker.h index 3e9e9df2a41f5..a2cf1f6309adb 100644 --- a/include/linux/ref_tracker.h +++ b/include/linux/ref_tracker.h @@ -17,12 +17,19 @@ struct ref_tracker_dir { booldead; struct list_headlist; /* List of active trackers */ struct list_headquarantine; /* List of dead trackers */ + charname[32]; #endif }; #ifdef CONFIG_REF_TRACKER -static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + +// Temporary allow two and three arguments, until consumers are converted +#define ref_tracker_dir_init(_d, _q, args...) _ref_tracker_dir_init(_d, _q, ##args, #_d) +#define _ref_tracker_dir_init(_d, _q, _n, ...) __ref_tracker_dir_init(_d, _q, _n) + +static inline void __ref_tracker_dir_init(struct ref_tracker_dir *dir, + unsigned int quarantine_count, + const char *name) { INIT_LIST_HEAD(&dir->list); INIT_LIST_HEAD(&dir->quarantine); @@ -31,6 +38,7 @@ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, dir->dead = false; refcount_set(&dir->untracked, 1); refcount_set(&dir->no_tracker, 1); + strlcpy(dir->name, name, sizeof(dir->name)); stack_depot_init(); } @@ -51,7 +59,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, #else /* CONFIG_REF_TRACKER */ static inline void ref_tracker_dir_init(struct ref_tracker_dir *dir, - unsigned int quarantine_count) + unsigned int quarantine_count, + ...) { } diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 5e9f90bbf771b..ab1253fde244e 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -1,11 +1,16 @@ // SPDX-License-Identifier: GPL-2.0-or-later + +#define pr_fmt(fmt) "ref_tracker: " fmt + #include +#include #include #include #include #include #define REF_TRACKER_STACK_ENTRIES 16 +#define STACK_BUF_SIZE 1024 struct ref_tracker { struct list_headhead; /* anchor into dir->list or dir->quarantine */ @@ -14,24 +19,87 @@ struct ref_tracker { depot_stack_handle_tfree_stack_handle; }; -void __ref_tracker_dir_print(struct ref_tracker_dir *dir, - unsigned int display_limit) +struct ref_tracker_dir_stats { + int total; + int count; + struct { + depot_stack_handle_t stack_handle; + unsigned int count; + } stacks[]; +}; + +static struct ref_tracker_dir_stats * +ref_tracker_get_stats(struct ref_tracker_dir *dir, unsigned int limit) { + struct ref_tracker_dir_stats *stats; struct ref_tracker *tracker; - unsigned int i = 0; - lockdep_assert_held(&dir->lock); + stats = kmalloc(struct_size(stats, stacks, limit), + GFP_NOWAIT | __GFP_NOWARN); I would be more comfortable if the allocation was done by the caller, possibly using GFP_KERNEL and evenutally kvmalloc(), instead of under dir->lock ? I though also about it, but decided to left this change to another patch as the change can be substantial and could open another discussion. I am not sure what you mean by 'caller' but it could be even external user of the API: 1. alloc data for ref_tracker_dir_stats. 2. take locks, if necessary. 3. gather stats (ref_tracker_get_stats) atomically. 4. release taken locks. 5. print stats. This way, allocation and printing would happen outside locks. + if (!stats) + return ERR_PTR(-ENOMEM); + stats->total = 0; + stats->count = 0; list_for_each_entry(tracker, &dir->list, head) { - if (i < display_limit) { - pr_err("leaked reference.\n"); - if (tracker->alloc_stack_handle) - stack_depot_print(tracker->alloc_stack_handle); - i++; - } else { -
Re: [PATCH v3 07/11] lib/ref_tracker: remove warnings in case of allocation failure
On 22.02.2022 00:54, Eric Dumazet wrote: On Mon, Feb 21, 2022 at 3:26 PM Andrzej Hajda wrote: Library can handle allocation failures. To avoid allocation warnings __GFP_NOWARN has been added everywhere. Moreover GFP_ATOMIC has been replaced with GFP_NOWAIT in case of stack allocation on tracker free call. Signed-off-by: Andrzej Hajda --- lib/ref_tracker.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c index 2ef4596b6b36f..cae4498fcfd70 100644 --- a/lib/ref_tracker.c +++ b/lib/ref_tracker.c @@ -189,7 +189,7 @@ int ref_tracker_alloc(struct ref_tracker_dir *dir, unsigned long entries[REF_TRACKER_STACK_ENTRIES]; struct ref_tracker *tracker; unsigned int nr_entries; - gfp_t gfp_mask = gfp; + gfp_t gfp_mask = gfp | __GFP_NOWARN; SGTM unsigned long flags; WARN_ON_ONCE(dir->dead); @@ -237,7 +237,8 @@ int ref_tracker_free(struct ref_tracker_dir *dir, return -EEXIST; } nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 1); - stack_handle = stack_depot_save(entries, nr_entries, GFP_ATOMIC); + stack_handle = stack_depot_save(entries, nr_entries, + GFP_NOWAIT | __GFP_NOWARN); Last time I looked at this, __GFP_NOWARN was enforced in __stack_depot_save() You are right, however I am not sure if we should count on unexpected (at least for me) and undocumented behavior. Currently we do not need to rely on some hidden feature. Regards Andrzej spin_lock_irqsave(&dir->lock, flags); if (tracker->dead) { -- 2.25.1
Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
On 22.02.2022 09:43, Dave Stevenson wrote: Hi Laurent. Thanks for the review. On Tue, 22 Feb 2022 at 06:34, Laurent Pinchart wrote: Hi Dave, Thank you for the patch. On Wed, Feb 16, 2022 at 04:59:43PM +, Dave Stevenson wrote: DSI sink devices typically want the DSI host powered up and configured before they are powered up. pre_enable is the place this would normally happen, but they are called in reverse order from panel/connector towards the encoder, which is the "wrong" order. Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set to swap the order of pre_enable (and post_disable) so that any upstream bridges are called first to create the desired state. eg: - Panel - Bridge 1 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST - Bridge 3 - Encoder Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3, Bridge 2. If there was a Bridge 4 between Bridge 3 and Encoder, would it be Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2 ? I'd capture that here, to be explicit. No. - Panel - Bridge 1 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST - Bridge 3 - Bridge 4 - Encoder Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 4, Encoder. ie it only swaps the order of bridges 2 & 3. - Panel - Bridge 1 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST - Bridge 3 DRM_BRIDGE_OP_UPSTREAM_FIRST - Bridge 4 - Encoder Would result in pre_enable's being called as Panel, Bridge 1, Bridge 4, Bridge 3, Bridge 2, Encoder. (Bridge 2&3 have asked for upstream to be enabled first, which means bridge 4. Bridge 2 wants upstream enabled first, which means bridge 3). - Panel - Bridge 1 - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST - Bridge 3 - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST - Bridge 5 - Encoder Would result in Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge 4, Encoder. So we only reverse the order whilst the bridges request that they want upstream enabled first, but we can do that multiple times within the chain. I hope that makes sense. Signed-off-by: Dave Stevenson --- drivers/gpu/drm/drm_bridge.c | 197 +-- include/drm/drm_bridge.h | 8 ++ 2 files changed, 180 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7c24e8340efa 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable); * Calls &drm_bridge_funcs.post_disable op for all the bridges in the * encoder chain, starting from the first bridge to the last. These are called * after completing the encoder's prepare op. Missing blank line, as well as in three locations below. + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for + * that bridge will be called before the previous one to reverse the pre_enable + * calling direction. * * Note: the bridge passed should be the one closest to the encoder */ void drm_bridge_chain_post_disable(struct drm_bridge *bridge) { struct drm_encoder *encoder; + struct drm_bridge *next, *limit; if (!bridge) return; encoder = bridge->encoder; list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { + limit = NULL; + + if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) { + next = list_next_entry(bridge, chain_node); + + if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) { + limit = next; + + list_for_each_entry_from(next, &encoder->bridge_chain, + chain_node) { + if (!(next->ops & + DRM_BRIDGE_OP_UPSTREAM_FIRST)) { + next = list_prev_entry(next, chain_node); + limit = next; + break; + } + } + + list_for_each_entry_from_reverse(next, &encoder->bridge_chain, + chain_node) { + if (next == bridge) + break; + + if (next->funcs->post_disable) + next->funcs->post_disable(next); + } + } + } + if (bridge->funcs->post_disable) bridge->funcs->post_disable(bridge); + + if (limit) + bridge = limit; } + } EXPORT_SYMBOL(drm_bridge_chain_post_disable); @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set
Re: [PATCH v5 2/7] drm/i915: Prepare for multiple GTs
On 17.02.2022 15:41, Andi Shyti wrote: From: Tvrtko Ursulin On a multi-tile platform, each tile has its own registers + GGTT space, and BAR 0 is extended to cover all of them. Up to four GTs are supported in i915->gt[], with slot zero shadowing the existing i915->gt0 to enable source compatibility with legacy driver paths. A for_each_gt macro is added to iterate over the GTs and will be used by upcoming patches that convert various parts of the driver to be multi-gt aware. Only the primary/root tile is initialized for now; the other tiles will be detected and plugged in by future patches once the necessary infrastructure is in place to handle them. Signed-off-by: Abdiel Janulgue Signed-off-by: Daniele Ceraolo Spurio Signed-off-by: Tvrtko Ursulin Signed-off-by: Matt Roper Signed-off-by: Andi Shyti Cc: Daniele Ceraolo Spurio Cc: Joonas Lahtinen Cc: Matthew Auld Reviewed-by: Matt Roper --- drivers/gpu/drm/i915/gt/intel_gt.c| 135 -- drivers/gpu/drm/i915/gt/intel_gt.h| 16 ++- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 9 +- drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 + drivers/gpu/drm/i915/i915_driver.c| 29 ++-- drivers/gpu/drm/i915/i915_drv.h | 6 + drivers/gpu/drm/i915/intel_memory_region.h| 3 + drivers/gpu/drm/i915/intel_uncore.c | 12 +- drivers/gpu/drm/i915/intel_uncore.h | 3 +- .../gpu/drm/i915/selftests/mock_gem_device.c | 7 +- 10 files changed, 182 insertions(+), 45 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index db171e85f4df..8c64b81e9ec9 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -29,7 +29,7 @@ #include "intel_uncore.h" #include "shmem_utils.h" -void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +static void __intel_gt_init_early(struct intel_gt *gt) { spin_lock_init(>->irq_lock); @@ -51,19 +51,29 @@ void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) intel_rps_init_early(>->rps); } -void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915) +/* Preliminary initialization of Tile 0 */ +void intel_root_gt_init_early(struct drm_i915_private *i915) { + struct intel_gt *gt = to_gt(i915); + gt->i915 = i915; gt->uncore = &i915->uncore; + + __intel_gt_init_early(gt); } -int intel_gt_probe_lmem(struct intel_gt *gt) +static int intel_gt_probe_lmem(struct intel_gt *gt) { struct drm_i915_private *i915 = gt->i915; + unsigned int instance = gt->info.id; struct intel_memory_region *mem; int id; int err; + id = INTEL_REGION_LMEM_0 + instance; + if (drm_WARN_ON(&i915->drm, id >= INTEL_REGION_STOLEN_SMEM)) Do we need to check id correctness? wouldn't be enough to check it on initialization of gt->info.id. If yes, maybe (id > INTEL_REGION_LMEM_3) would be more readable, or (info.id < MAX_GT), up to you. + return -ENODEV; + mem = intel_gt_setup_lmem(gt); if (mem == ERR_PTR(-ENODEV)) mem = intel_gt_setup_fake_lmem(gt); @@ -78,9 +88,8 @@ int intel_gt_probe_lmem(struct intel_gt *gt) return err; } - id = INTEL_REGION_LMEM_0; - mem->id = id; + mem->instance = instance; intel_memory_region_set_name(mem, "local%u", mem->instance); @@ -795,16 +804,21 @@ void intel_gt_driver_release(struct intel_gt *gt) intel_gt_fini_buffer_pool(gt); } -void intel_gt_driver_late_release(struct intel_gt *gt) +void intel_gt_driver_late_release(struct drm_i915_private *i915) { + struct intel_gt *gt; + unsigned int id; + /* We need to wait for inflight RCU frees to release their grip */ rcu_barrier(); - intel_uc_driver_late_release(>->uc); - intel_gt_fini_requests(gt); - intel_gt_fini_reset(gt); - intel_gt_fini_timelines(gt); - intel_engines_free(gt); + for_each_gt(gt, i915, id) { + intel_uc_driver_late_release(>->uc); + intel_gt_fini_requests(gt); + intel_gt_fini_reset(gt); + intel_gt_fini_timelines(gt); + intel_engines_free(gt); + } } /** @@ -913,6 +927,105 @@ u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg) return intel_uncore_read_fw(gt->uncore, reg); } +static int intel_gt_tile_setup(struct intel_gt *gt, phys_addr_t phys_addr) +{ + unsigned int id = gt->info.id; + int ret; + + if (id) { + struct intel_uncore_mmio_debug *mmio_debug; + struct intel_uncore *uncore; + + uncore = kzalloc(sizeof(*uncore), GFP_KERNEL); + if (!gt->uncore) + return -ENOMEM; s/gt->uncore/uncore/ + + mmio_debug = kza
Re: [PATCH v5 1/7] drm/i915: Rename INTEL_REGION_LMEM with INTEL_REGION_LMEM_0
On 17.02.2022 15:41, Andi Shyti wrote: With the upcoming multitile support each tile will have its own local memory. Mark the current LMEM with the suffix '0' to emphasise that it belongs to the root tile. Suggested-by: Michal Wajdeczko Signed-off-by: Andi Shyti Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [Intel-gfx] [PATCH v5 3/7] drm/i915/gt: add gt_is_root() helper
On 28.02.2022 21:02, Michal Wajdeczko wrote: On 17.02.2022 15:41, Andi Shyti wrote: The "gt_is_root(struct intel_gt *gt)" helper return true if the gt is the root gt, which means that its id is 0. Return false otherwise. Suggested-by: Michal Wajdeczko Signed-off-by: Andi Shyti --- drivers/gpu/drm/i915/gt/intel_gt.h | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h index 915d6192079b..f17f51e2d394 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.h +++ b/drivers/gpu/drm/i915/gt/intel_gt.h @@ -19,6 +19,11 @@ struct drm_printer; ##__VA_ARGS__); \ } while (0) +static inline bool gt_is_root(struct intel_gt *gt) +{ + return !gt->info.id; +} + we could squash this patch with prev one, where it can be used in: intel_gt_tile_cleanup(struct intel_gt *gt) { intel_uncore_cleanup_mmio(gt->uncore); - if (gt->info.id) { + if (!gt_is_root(gt)) { kfree(gt->uncore); kfree(gt); } } It can be used in intel_gt_tile_setup as well, and then you can remove id var. or just use it this way in this patch, with that: Reviewed-by: Michal Wajdeczko Accordingly: Reviewed-by: Andrzej Hajda Regards Andrzej static inline struct intel_gt *uc_to_gt(struct intel_uc *uc) { return container_of(uc, struct intel_gt, uc);
Re: [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface
On 17.02.2022 15:41, Andi Shyti wrote: Now that we have tiles we want each of them to have its own interface. A directory "gt/" is created under "cardN/" that will contain as many diroctories as the tiles. In the coming patches tile related interfaces will be added. For now the sysfs gt structure simply has an id interface related to the current tile count. The directory structure will follow this scheme: /sys/.../card0 └── gt ├── gt0 │ └── id : : └─- gtN └── id This new set of interfaces will be a basic tool for system managers and administrators when using i915. Signed-off-by: Andi Shyti Cc: Matt Roper Cc: Sujaritha Sundaresan Cc: Tvrtko Ursulin Reviewed-by: Sujaritha Sundaresan --- drivers/gpu/drm/i915/Makefile| 1 + drivers/gpu/drm/i915/gt/intel_gt.c | 2 + drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 118 +++ drivers/gpu/drm/i915/gt/intel_gt_sysfs.h | 34 +++ drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c| 12 ++- drivers/gpu/drm/i915/i915_sysfs.h| 3 + 7 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.c create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_sysfs.h diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 9d588d936e3d..277064b00afd 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -105,6 +105,7 @@ gt-y += \ gt/intel_gt_pm_debugfs.o \ gt/intel_gt_pm_irq.o \ gt/intel_gt_requests.o \ + gt/intel_gt_sysfs.o \ gt/intel_gtt.o \ gt/intel_llc.o \ gt/intel_lrc.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 8c64b81e9ec9..0f080bbad043 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -26,6 +26,7 @@ #include "intel_rc6.h" #include "intel_renderstate.h" #include "intel_rps.h" +#include "intel_gt_sysfs.h" #include "intel_uncore.h" #include "shmem_utils.h" @@ -458,6 +459,7 @@ void intel_gt_driver_register(struct intel_gt *gt) intel_rps_driver_register(>->rps); intel_gt_debugfs_register(gt); + intel_gt_sysfs_register(gt); } static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c new file mode 100644 index ..0206e9aa4867 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2022 Intel Corporation + */ + +#include +#include +#include +#include +#include + +#include "i915_drv.h" +#include "i915_sysfs.h" +#include "intel_gt.h" +#include "intel_gt_sysfs.h" +#include "intel_gt_types.h" +#include "intel_rc6.h" + +bool is_object_gt(struct kobject *kobj) +{ + return !strncmp(kobj->name, "gt", 2); +} It looks quite fragile, at the moment I do not have better idea:) maybe after reviewing the rest of the patches. + +static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{ + return container_of(kobj, struct kobj_gt, base)->gt; +} + +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name) +{ + struct kobject *kobj = &dev->kobj; + + /* +* We are interested at knowing from where the interface +* has been called, whether it's called from gt/ or from +* the parent directory. +* From the interface position it depends also the value of +* the private data. +* If the interface is called from gt/ then private data is +* of the "struct intel_gt *" type, otherwise it's * a +* "struct drm_i915_private *" type. +*/ + if (!is_object_gt(kobj)) { + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + pr_devel_ratelimited(DEPRECATED + "%s (pid %d) is accessing deprecated %s " + "sysfs control, please use gt/gt/%s instead\n", + current->comm, task_pid_nr(current), name, name); + return to_gt(i915); + } + + return kobj_to_gt(kobj); It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully. +} + +static ssize_t id_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct intel_gt *gt = intel_
Re: [PATCH v5 5/7] drm/i915/gt: Create per-tile RC6 sysfs interface
const struct attribute_group *grp) +{ + return is_object_gt(kobj) ? + sysfs_create_group(kobj, &grp[0]) : + sysfs_merge_group(kobj, &grp[1]); +} Merging handling "gt/gt#/*" and "power/*" attributes into the same helpers seems unnatural to me, in many functions we have two branches based on value of is_object_gt, with the most hacky intel_gt_sysfs_get_drvdata. Splitting handling these attributes would allow to drop fragile is_object_gt helper and make functions more straightforward, probably at the cost of few lines more. On the other side I am not sure if it is worth to change everything to just address code purity concerns :) So with variable type adjustement: Reviewed-by: Andrzej Hajda Regards Andrzej Regards Andrzej + +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ + int ret; + + if (!HAS_RC6(gt->i915)) + return; + + ret = __intel_gt_sysfs_create_group(kobj, rc6_attr_group); + if (ret) + drm_warn(>->i915->drm, +"failed to create gt%u RC6 sysfs files\n", +gt->info.id); + + /* +* cannot use the is_visible() attribute because +* the upper object inherits from the parent group. +*/ + if (HAS_RC6p(gt->i915)) { + ret = __intel_gt_sysfs_create_group(kobj, rc6p_attr_group); + if (ret) + drm_warn(>->i915->drm, +"failed to create gt%u RC6p sysfs files\n", +gt->info.id); + } + + if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) { + ret = __intel_gt_sysfs_create_group(kobj, media_rc6_attr_group); + if (ret) + drm_warn(>->i915->drm, +"failed to create media %u RC6 sysfs files\n", +gt->info.id); + } +} +#else +static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) +{ +} +#endif /* CONFIG_PM */ + +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj) +{ + intel_sysfs_rc6_init(gt, kobj); +} diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h new file mode 100644 index ..f567105a4a89 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __SYSFS_GT_PM_H__ +#define __SYSFS_GT_PM_H__ + +#include + +#include "intel_gt_types.h" + +void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj); + +#endif /* SYSFS_RC6_H */ diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 3643efde52ca..b08a959e5ccc 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -45,107 +45,6 @@ struct drm_i915_private *kdev_minor_to_i915(struct device *kdev) return to_i915(minor->dev); } -#ifdef CONFIG_PM -static u32 calc_residency(struct drm_i915_private *dev_priv, - i915_reg_t reg) -{ - intel_wakeref_t wakeref; - u64 res = 0; - - with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref) - res = intel_rc6_residency_us(&to_gt(dev_priv)->rc6, reg); - - return DIV_ROUND_CLOSEST_ULL(res, 1000); -} - -static ssize_t rc6_enable_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - unsigned int mask; - - mask = 0; - if (HAS_RC6(dev_priv)) - mask |= BIT(0); - if (HAS_RC6p(dev_priv)) - mask |= BIT(1); - if (HAS_RC6pp(dev_priv)) - mask |= BIT(2); - - return sysfs_emit(buf, "%x\n", mask); -} - -static ssize_t rc6_residency_ms_show(struct device *kdev, -struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6); - return sysfs_emit(buf, "%u\n", rc6_residency); -} - -static ssize_t rc6p_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); - u32 rc6p_residency = calc_residency(dev_priv, GEN6_GT_GFX_RC6p); - return sysfs_emit(buf, "%u\n", rc6p_residency); -} - -static ssize_t rc6pp_residency_ms_show(struct device *kdev, - struct device_attribute *attr, char *buf) -{ - struct drm_i915_private *dev_p
Re: [PATCH v5 6/7] drm/i915/gt: Create per-tile RPS sysfs interfaces
On 17.02.2022 15:41, Andi Shyti wrote: Now tiles have their own sysfs interfaces under the gt/ directory. Because RPS is a property that can be configured on a tile basis, then each tile should have its own interface The new sysfs structure will have a similar layout for the 4 tile case: /sys/.../card0 ├── gt │ ├── gt0 │ │ ├── id │ │ ├── rc6_enable │ │ ├── rc6_residency_ms │ │ ├── rps_act_freq_mhz │ │ ├── rps_boost_freq_mhz │ │ ├── rps_cur_freq_mhz │ │ ├── rps_max_freq_mhz │ │ ├── rps_min_freq_mhz │ │ ├── rps_RP0_freq_mhz │ │ ├── rps_RP1_freq_mhz │ │ └── rps_RPn_freq_mhz . . . . . . │ └── gtN │ ├── id │ ├── rc6_enable │ ├── rc6_residency_ms │ ├── rps_act_freq_mhz │ ├── rps_boost_freq_mhz │ ├── rps_cur_freq_mhz │ ├── rps_max_freq_mhz │ ├── rps_min_freq_mhz │ ├── rps_RP0_freq_mhz │ ├── rps_RP1_freq_mhz │ └── rps_RPn_freq_mhz ├── gt_act_freq_mhz -+ ├── gt_boost_freq_mhz | ├── gt_cur_freq_mhz|Original interface ├── gt_max_freq_mhz+─-> kept as existing ABI; ├── gt_min_freq_mhz|it points to gt0/ ├── gt_RP0_freq_mhz| ├── gt_RP1_freq_mhz| └── gt_RPn_freq_mhz -+ The existing interfaces have been kept in their original location to preserve the existing ABI. They act on all the GTs: when writing they loop through all the GTs and write the information on each interface. When reading they provide the average value from all the GTs. This patch is not really adding exposing new interfaces (new ABI) other than adapting the existing one to more tiles. In any case this new set of interfaces will be a basic tool for system managers and administrators when using i915. Signed-off-by: Andi Shyti Signed-off-by: Lucas De Marchi Cc: Chris Wilson Cc: Joonas Lahtinen Cc: Matt Roper Cc: Sujaritha Sundaresan Cc: Tvrtko Ursulin --- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 276 drivers/gpu/drm/i915/i915_sysfs.c | 177 - 2 files changed, 276 insertions(+), 177 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 27d93a36894a..8e86b8f675f1 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -14,8 +14,33 @@ #include "intel_gt_sysfs.h" #include "intel_gt_sysfs_pm.h" #include "intel_rc6.h" +#include "intel_rps.h" #ifdef CONFIG_PM +static int +sysfs_gt_attribute_w_func(struct device *dev, struct device_attribute *attr, + int (func)(struct intel_gt *gt, u32 val), u32 val) +{ + struct intel_gt *gt; + int ret; + + if (!is_object_gt(&dev->kobj)) { + int i; + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + for_each_gt(gt, i915, i) { + ret = func(gt, val); + if (ret) + break; + } + } else { + gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + ret = func(gt, val); + } + + return ret; +} + static s64 sysfs_gt_attribute_r_func(struct device *dev, struct device_attribute *attr, s64 (func)(struct intel_gt *gt)) @@ -214,7 +239,258 @@ static void intel_sysfs_rc6_init(struct intel_gt *gt, struct kobject *kobj) } #endif /* CONFIG_PM */ +static s64 __act_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_read_actual_frequency(>->rps); +} + +static ssize_t act_freq_mhz_show(struct device *dev, +struct device_attribute *attr, char *buff) +{ + s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr, + __act_freq_mhz_show); + + return sysfs_emit(buff, "%u\n", (u32) actual_freq); Again, variable can be just u32. +} + +static s64 __cur_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_get_requested_frequency(>->rps); +} + +static ssize_t cur_freq_mhz_show(struct device *dev, +struct device_attribute *attr, char *buff) +{ + s64 cur_freq = sysfs_gt_attribute_r_func(dev, attr, +__cur_freq_mhz_show); + + return sysfs_emit(buff, "%u\n", (u32) cur_freq); +} + +static s64 __boost_freq_mhz_show(struct intel_gt *gt) +{ + return intel_rps_get_boost_frequency(>->rps); +} + +static ssize_t boost_freq_mhz_show(struct device *dev, + struct device_attr
Re: [PATCH v5 7/7] drm/i915/gt: Adding new sysfs frequency attributes
On 17.02.2022 15:41, Andi Shyti wrote: From: Sujaritha Sundaresan This patch adds the following new sysfs frequency attributes; - punit_req_freq_mhz - throttle_reason_status - throttle_reason_pl1 - throttle_reason_pl2 - throttle_reason_pl4 - throttle_reason_thermal - throttle_reason_prochot - throttle_reason_ratl - throttle_reason_vr_thermalert - throttle_reason_vr_tdc Signed-off-by: Sujaritha Sundaresan Signed-off-by: Andi Shyti Cc: Dale B Stimson --- drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 142 drivers/gpu/drm/i915/gt/intel_rps.c | 83 drivers/gpu/drm/i915/gt/intel_rps.h | 10 ++ drivers/gpu/drm/i915/i915_reg.h | 11 ++ 4 files changed, 246 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c index 8e86b8f675f1..8be676cd1607 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c @@ -463,6 +463,141 @@ static ssize_t rps_rp_mhz_show(struct device *dev, static const struct attribute * const gen6_rps_attrs[] = GEN6_RPS_ATTR; static const struct attribute * const gen6_gt_attrs[] = GEN6_GT_ATTR; +static ssize_t punit_req_freq_mhz_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + u32 preq = intel_rps_read_punit_req_frequency(rps); + + return scnprintf(buff, PAGE_SIZE, "%d\n", preq); +} + +static ssize_t throttle_reason_status_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool status = !!intel_rps_read_throttle_reason_status(rps); !! is not necessary if you assign to bool variable, here and below. + + return scnprintf(buff, PAGE_SIZE, "%u\n", status); +} + +static ssize_t throttle_reason_pl1_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool pl1 = !!intel_rps_read_throttle_reason_pl1(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", pl1); +} + +static ssize_t throttle_reason_pl2_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool pl2 = !!intel_rps_read_throttle_reason_pl2(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", pl2); +} + +static ssize_t throttle_reason_pl4_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool pl4 = !!intel_rps_read_throttle_reason_pl4(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", pl4); +} + +static ssize_t throttle_reason_thermal_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool thermal = !!intel_rps_read_throttle_reason_thermal(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", thermal); +} + +static ssize_t throttle_reason_prochot_show(struct device *dev, + struct device_attribute *attr, + char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool prochot = !!intel_rps_read_throttle_reason_prochot(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", prochot); +} + +static ssize_t throttle_reason_ratl_show(struct device *dev, +struct device_attribute *attr, +char *buff) +{ + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name); + struct intel_rps *rps = >->rps; + bool ratl = !!intel_rps_read_throttle_reason_ratl(rps); + + return scnprintf(buff, PAGE_SIZE, "%u\n", ratl); +} + +static ssize_t throttle_reason_vr_thermalert_show(struct device *dev, +
Re: [PATCH v5 4/7] drm/i915/gt: create per-tile sysfs interface
On 07.03.2022 00:04, Andi Shyti wrote: Hi Andrzej, [...] +bool is_object_gt(struct kobject *kobj) +{ + return !strncmp(kobj->name, "gt", 2); +} It looks quite fragile, at the moment I do not have better idea:) maybe after reviewing the rest of the patches. yeah... it's not pretty, I agree, but I couldn't come up with a better way of doing it. +static struct intel_gt *kobj_to_gt(struct kobject *kobj) +{ + return container_of(kobj, struct kobj_gt, base)->gt; +} + +struct intel_gt *intel_gt_sysfs_get_drvdata(struct device *dev, + const char *name) +{ + struct kobject *kobj = &dev->kobj; + + /* +* We are interested at knowing from where the interface +* has been called, whether it's called from gt/ or from +* the parent directory. +* From the interface position it depends also the value of +* the private data. +* If the interface is called from gt/ then private data is +* of the "struct intel_gt *" type, otherwise it's * a +* "struct drm_i915_private *" type. +*/ + if (!is_object_gt(kobj)) { + struct drm_i915_private *i915 = kdev_minor_to_i915(dev); + + pr_devel_ratelimited(DEPRECATED + "%s (pid %d) is accessing deprecated %s " + "sysfs control, please use gt/gt/%s instead\n", + current->comm, task_pid_nr(current), name, name); + return to_gt(i915); + } + + return kobj_to_gt(kobj); It took some time for me to understand what is going on here. We have dev argument which sometimes can point to "struct device", sometimes to "struct kobj_gt", but it's type suggests differently, quite ugly. I wonder if wouldn't be better to use __ATTR instead of DEVICE_ATTR* as in case of intel_engines_add_sysfs. This way abstractions would look better, hopefully. How would it help? The difference is that I'm adding twice different interfaces with the same name and different location (i.e. different object). The legacy intrefaces inherit the object from drm and I'm preserving that reference. While the new objects would derive from the previous and they are pretty much like intel_engines_add_sysfs(). I was not clear on the issue. Here in case of 'id' attribute it is defined as device_attribute, but in kobj_type.sysfs_ops you assign formally incompatible &kobj_sysfs_ops. kobj_sysfs_ops expects kobj_attribute! Fortunately kobj_attribute is 'binary compatible' with device_attribute and kobj is at beginning of struct device as well, so it does not blow up, but I wouldn't say it is clean solution :) If you look at intel_engines_add_sysfs you can see that all attributes are defined as kobj_attribute. Regards Andrzej [...] +struct kobject * +intel_gt_create_kobj(struct intel_gt *gt, struct kobject *dir, const char *name) +{ + struct kobj_gt *kg; + + kg = kzalloc(sizeof(*kg), GFP_KERNEL); + if (!kg) + return NULL; + + kobject_init(&kg->base, &kobj_gt_type); + kg->gt = gt; + + /* xfer ownership to sysfs tree */ + if (kobject_add(&kg->base, dir, "%s", name)) { + kobject_put(&kg->base); + return NULL; + } + + return &kg->base; /* borrowed ref */ +} + +void intel_gt_sysfs_register(struct intel_gt *gt) +{ + struct kobject *dir; + char name[80]; + + snprintf(name, sizeof(name), "gt%d", gt->info.id); + + dir = intel_gt_create_kobj(gt, gt->i915->sysfs_gt, name); + if (!dir) { + drm_warn(>->i915->drm, +"failed to initialize %s sysfs root\n", name); + return; + } +} Squashing intel_gt_create_kobj into intel_gt_sysfs_register would simplify code and allows drop snprintf to local array. right! +static struct kobject *i915_setup_gt_sysfs(struct kobject *parent) +{ + return kobject_create_and_add("gt", parent); +} + void i915_setup_sysfs(struct drm_i915_private *dev_priv) { struct device *kdev = dev_priv->drm.primary->kdev; @@ -538,6 +543,11 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv) if (ret) drm_err(&dev_priv->drm, "RPS sysfs setup failed\n"); + dev_priv->sysfs_gt = i915_setup_gt_sysfs(&kdev->kobj); Why not directly kobject_create_and_add("gt", parent) ? up to you. of course! [...] Thanks a lot for the review, Andi
Re: [PATCH] drm/bridge: cdns: Make use of the helper function devm_platform_ioremap_resource()
W dniu 31.08.2021 o 15:50, Cai Huoqing pisze: > Use the devm_platform_ioremap_resource() helper instead of > calling platform_get_resource() and devm_ioremap_resource() > separately > > Signed-off-by: Cai Huoqing Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v3 00/16] eDP: Support probing eDP panels dynamically instead of hardcoding
Removed most CC: SMTP server protested. On 01.09.2021 22:19, Douglas Anderson wrote: > The goal of this patch series is to move away from hardcoding exact > eDP panels in device tree files. As discussed in the various patches > in this series (I'm not repeating everything here), most eDP panels > are 99% probable and we can get that last 1% by allowing two "power > up" delays to be specified in the device tree file and then using the > panel ID (found in the EDID) to look up additional power sequencing > delays for the panel. > > This patch series is the logical contiunation of a previous patch > series where I proposed solving this problem by adding a > board-specific compatible string [1]. In the discussion that followed > it sounded like people were open to something like the solution > proposed in this new series. > > In version 2 I got rid of the idea that we could have a "fallback" > compatible string that we'd use if we didn't recognize the ID in the > EDID. This simplifies the bindings a lot and the implementation > somewhat. As a result of not having a "fallback", though, I'm not > confident in transitioning any existing boards over to this since > we'll have to fallback to very conservative timings if we don't > recognize the ID from the EDID and I can't guarantee that I've seen > every panel that might have shipped on an existing product. The plan > is to use "edp-panel" only on new boards or new revisions of old > boards where we can guarantee that every EDID that ships out of the > factory has an ID in the table. > > Version 3 of this series now splits out all eDP panels to their own > driver and adds the generic eDP panel support to this new driver. I > believe this is what Sam was looking for [2]. > > [1] https://lore.kernel.org/r/yfkqaxomowyye...@google.com/ > [2] https://lore.kernel.org/r/yrtsfntn%2ft8fl...@ravnborg.org/ > I like the idea - if something can be configured dynamically lets do it. But I have few questions: 1. Have you read different real panels id's? In many cases (MIPI DSI, LVDS with EDID) manufacturers often forgot to set proper id fields. I do not know how EDID's ids are reliable in case of edp panels. 2. You are working with edp panels - beside EDID they have DPCD which contains things like IEEE_OUI and "Device Identification String", I guess they could be also used for detecting panels, have you considered it? I think DPCD Id should be assigned to EDP-Sink interface, and EDID Id to the actual panel behind it. With this assumption one could consider which timings should be property of which entity. Regards Andrzej
Re: [PATCH v3 06/16] ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP
On 02.09.2021 01:10, Doug Anderson wrote: > Hi, > > On Wed, Sep 1, 2021 at 2:12 PM Olof Johansson wrote: >> >> On Wed, Sep 1, 2021 at 1:20 PM Douglas Anderson >> wrote: >>> >>> In the patch ("drm/panel-simple-edp: Split eDP panels out of >>> panel-simple") we split the PANEL_SIMPLE driver in 2. By default let's >>> give everyone who had the old driver enabled the new driver too. If >>> folks want to opt-out of one or the other they always can later. >>> >>> Signed-off-by: Douglas Anderson >> >> Isn't this a case where the new option should just have had the old >> option as the default value to avoid this kind of churn and possibly >> broken platforms? > > I'm happy to go either way. I guess I didn't do that originally > because logically there's not any reason to link the two drivers going > forward. Said another way, someone enabling the "simple panel" driver > for non-eDP panels wouldn't expect that the "simple panel" driver for > DP panels would also get enabled by default. They really have nothing > to do with one another. Enabling by default for something like this > also seems like it would lead to bloat. I could have sworn that > periodically people get yelled at for marking drivers on by default > when it doesn't make sense. > > ...that being said, I'm happy to change the default as you suggest. > Just let me know. I guess this is just misunderstanding. Symbol names: CONFIG_DRM_PANEL_SIMPLE=y CONFIG_DRM_PANEL_SIMPLE_EDP=y suggests that CONFIG_DRM_PANEL_SIMPLE_EDP is an 'suboption' of CONFIG_DRM_PANEL_SIMPLE, but these symbols are independent - old symbol has been split into two independent new symbols. So Doug's approach seems correct to me. Maybe one could change names of symbols to avoid confusion(?). One more thing, I suspect previous patch can break platforms with EDP panels. Even if this patch fixes it, maybe it would be better to squash these patches? Or add temporal solution to save bisecatability. Regards Andrzej > > -Doug >
Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge
On 07.09.2021 04:39, Marek Vasut wrote: > In rare cases, the bridge may not start up correctly, which usually > leads to no display output. In case this happens, warn about it in > the kernel log. > > Signed-off-by: Marek Vasut > Cc: Jagan Teki > Cc: Laurent Pinchart > Cc: Linus Walleij > Cc: Robert Foss > Cc: Sam Ravnborg > Cc: dri-devel@lists.freedesktop.org > --- > NOTE: See the following: > https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video > https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533 > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index a32f70bc68ea4..4ea71d7f0bfbc 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct drm_bridge > *bridge, > /* Clear all errors that got asserted during initialization. */ > regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > regmap_write(ctx->regmap, REG_IRQ_STAT, pval); It does not look as correct error handling, maybe it would be good to analyze and optionally report 'unexpected' errors here as well. > + > + usleep_range(1, 12000); > + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); > + if (pval) > + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); I am not sure what is the case here but it looks like 'we do not know what is going on, so let's add some diagnostic messages to gather info and figure it out later'. Whole driver lacks IRQ handler which IMO could perform better diagnosis, and I guess it could also help in recovery, but this is just my guess. So if this patch is enough for now you can add: Reviewed-by: Andrzej Hajda Regards Andrzej > } > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, >
Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge
W dniu 07.09.2021 o 16:25, Marek Vasut pisze: > On 9/7/21 9:31 AM, Andrzej Hajda wrote: >> On 07.09.2021 04:39, Marek Vasut wrote: >>> In rare cases, the bridge may not start up correctly, which usually >>> leads to no display output. In case this happens, warn about it in >>> the kernel log. >>> >>> Signed-off-by: Marek Vasut >>> Cc: Jagan Teki >>> Cc: Laurent Pinchart >>> Cc: Linus Walleij >>> Cc: Robert Foss >>> Cc: Sam Ravnborg >>> Cc: dri-devel@lists.freedesktop.org >>> --- >>> NOTE: See the following: >>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video >>> >>> >>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533 >>> >>> >>> --- >>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 + >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>> index a32f70bc68ea4..4ea71d7f0bfbc 100644 >>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct >>> drm_bridge *bridge, >>> /* Clear all errors that got asserted during initialization. */ >>> regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); >>> regmap_write(ctx->regmap, REG_IRQ_STAT, pval); >> >> >> It does not look as correct error handling, maybe it would be good to >> analyze and optionally report 'unexpected' errors here as well. > > The above is correct -- it clears the status register because the > setup might've set random bits in that register. Then we wait a bit, > let the link run, and read them again to get the real link status in > this new piece of code below, hence the usleep_range there. And then > if the link indicates a problem, we know it is a problem. Usually such registers are cleared on very beginning of the initialization, and tested (via irq handler, or via reading), during initalization, if initialization phase goes well. If it is not the case forgive me. > >>> + >>> + usleep_range(1, 12000); >>> + regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); >>> + if (pval) >>> + dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); >> >> >> I am not sure what is the case here but it looks like 'we do not know >> what is going on, so let's add some diagnostic messages to gather info >> and figure it out later'. > > That's pretty much the case, see the two links above in the NOTE > section. If something goes wrong, we print the value for the user > (usually developer) so they can fix their problems. We cannot do much > better in the attach callback. > > The issue I ran into (and where this would be helpful information to > me during debugging, since the issue happened real seldom, see also > the NOTE links above) is that the DSI controller driver started > streaming video on the data lanes before the DSI83 had a chance to > initialize. This worked most of the time, except for a few exceptions > here and there, where the video didn't start. This does set link > status bits consistently. In the meantime, I fixed the controller > driver (so far downstream, due to ongoing discussion). Maybe drm_connector_set_link_status_property(conn, DRM_MODE_LINK_STATUS_BAD) would be usefule here. > >> Whole driver lacks IRQ handler which IMO could perform better diagnosis, >> and I guess it could also help in recovery, but this is just my guess. >> So if this patch is enough for now you can add: > > No, IRQ won't help you here, because by the time you get the IRQ, the > DSI host already started streaming video on data lanes and you won't > be able to correctly reinit the DSI83 unless you communicate to the > DSI host that it should switch the data lanes back to LP11. > > And for that, there is a bigger chunk missing really. What needs to be > added is a way for the DSI bridge / panel to communicate its needs to > the DSI host -- things like "I need DSI clock lane frequency f MHz, I > need clock lane in HS mode and data lanes in LP11 mode". If you look > at the way DSI hosts and bridges/panels work out the DSI link > parameters, you will notice they basically do it each on their own, > there is no such API or communication channel.
Re: [PATCH] drm/bridge: ti-sn65dsi83: Check link status register after enabling the bridge
W dniu 08.09.2021 o 13:11, Dave Stevenson pisze: > Hi Marek and Andrzej > > On Tue, 7 Sept 2021 at 22:24, Marek Vasut wrote: >> On 9/7/21 7:29 PM, Andrzej Hajda wrote: >>> W dniu 07.09.2021 o 16:25, Marek Vasut pisze: >>>> On 9/7/21 9:31 AM, Andrzej Hajda wrote: >>>>> On 07.09.2021 04:39, Marek Vasut wrote: >>>>>> In rare cases, the bridge may not start up correctly, which usually >>>>>> leads to no display output. In case this happens, warn about it in >>>>>> the kernel log. >>>>>> >>>>>> Signed-off-by: Marek Vasut >>>>>> Cc: Jagan Teki >>>>>> Cc: Laurent Pinchart >>>>>> Cc: Linus Walleij >>>>>> Cc: Robert Foss >>>>>> Cc: Sam Ravnborg >>>>>> Cc: dri-devel@lists.freedesktop.org >>>>>> --- >>>>>> NOTE: See the following: >>>>>> https://e2e.ti.com/support/interface-group/interface/f/interface-forum/942005/sn65dsi83-dsi83-lvds-bridge---sporadic-behavior---no-video >>>>>> >>>>>> https://community.nxp.com/t5/i-MX-Processors/i-MX8M-MIPI-DSI-Interface-LVDS-Bridge-Initialization/td-p/1156533 >>>>>> >>>>>> --- >>>>>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 5 + >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> index a32f70bc68ea4..4ea71d7f0bfbc 100644 >>>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> @@ -520,6 +520,11 @@ static void sn65dsi83_atomic_enable(struct >>>>>> drm_bridge *bridge, >>>>>> /* Clear all errors that got asserted during initialization. */ >>>>>> regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); >>>>>> regmap_write(ctx->regmap, REG_IRQ_STAT, pval); >>>>> >>>>> It does not look as correct error handling, maybe it would be good to >>>>> analyze and optionally report 'unexpected' errors here as well. >>>> The above is correct -- it clears the status register because the >>>> setup might've set random bits in that register. Then we wait a bit, >>>> let the link run, and read them again to get the real link status in >>>> this new piece of code below, hence the usleep_range there. And then >>>> if the link indicates a problem, we know it is a problem. >>> >>> Usually such registers are cleared on very beginning of the >>> initialization, and tested (via irq handler, or via reading), during >>> initalization, if initialization phase goes well. If it is not the case >>> forgive me. >> The init just flips the bit at random in the IRQ_STAT register, so no, >> that's not really viable here. That's why we clear them at the end, and >> then wait a bit, and then check whether something new appeared in them. >> >> If not, all is great. >> >> Sure, we could generate an IRQ, but then IRQ line is not always >> connected to this chip on all hardware I have available. So this gives >> the user at least some indication that something is wrong with their HW. >> >>>>>> + >>>>>> +usleep_range(1, 12000); >>>>>> +regmap_read(ctx->regmap, REG_IRQ_STAT, &pval); >>>>>> +if (pval) >>>>>> +dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval); >>>>> >>>>> I am not sure what is the case here but it looks like 'we do not know >>>>> what is going on, so let's add some diagnostic messages to gather info >>>>> and figure it out later'. >>>> That's pretty much the case, see the two links above in the NOTE >>>> section. If something goes wrong, we print the value for the user >>>> (usually developer) so they can fix their problems. We cannot do much >>>> better in the attach callback. >>>> >>>> The issue I ran into (and where this would be helpful information to >>>> me during debugging, since the issue happened real seldom, see also >>>> the NOTE links above) is that the DSI controller driver started >>>> streaming video on the data lanes before the DSI83 had a