Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König wrote: > > hello Sean, > > On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul wrote: > > I'd really prefer this patch (series or single) is not accepted. This > > will cause problems for everyone cherry-picking patches to a > > downstream kernel (LTS or distro tree). I usually wouldn't expect > > sympathy here, but the questionable benefit does not outweigh the cost > > IM[biased]O. > > I agree that for backports this isn't so nice. However with the split > approach (that was argumented against here) it's not soo bad. Patch #1 > (and similar changes for the other affected structures) could be > trivially backported and with that it doesn't matter if you write dev or > drm (or whatever name is chosen in the end); both work in the same way. Patch #1 avoids the need to backport the entire set, however every change occuring after the rename patches will cause conflicts on future cherry-picks. Downstream kernels will have to backport the whole set. Backporting the entire set will create an epoch in downstream kernels where cherry-picking patches preceding this set will need to undergo conflict resolution as well. As mentioned in my previous email, I don't expect sympathy here, it's part of maintaining a downstream kernel, but there is a real cost to kernel consumers. > > But even with the one-patch-per-rename approach I'd consider the > renaming a net win, because ease of understanding code has a big value. > It's value is not so easy measurable as "conflicts when backporting", > but it also matters in say two years from now, while backporting > shouldn't be an issue then any more. You've rightly identified the conjecture in your statement. I've been on both sides of the argument, having written/maintained drm code upstream and cherry-picked changes to a downstream kernel. Perhaps it's because drm's definition of dev is ingrained in my muscle memory, or maybe it's because I don't do a lot of upstream development these days, but I just have a hard time seeing the benefit here. I appreciate your engagement on the topic, thank you! Sean > > Thanks for your input, best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König| > Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, Jul 12, 2023 at 10:52 AM Jani Nikula wrote: > > On Wed, 12 Jul 2023, Uwe Kleine-König wrote: > > Hello, > > > > while I debugged an issue in the imx-lcdc driver I was constantly > > irritated about struct drm_device pointer variables being named "dev" > > because with that name I usually expect a struct device pointer. > > > > I think there is a big benefit when these are all renamed to "drm_dev". > > I have no strong preference here though, so "drmdev" or "drm" are fine > > for me, too. Let the bikesheding begin! > > > > Some statistics: > > > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq > > -c | sort -n > > 1 struct drm_device *adev_to_drm > > 1 struct drm_device *drm_ > > 1 struct drm_device *drm_dev > > 1 struct drm_device*drm_dev > > 1 struct drm_device *pdev > > 1 struct drm_device *rdev > > 1 struct drm_device *vdev > > 2 struct drm_device *dcss_drv_dev_to_drm > > 2 struct drm_device **ddev > > 2 struct drm_device *drm_dev_alloc > > 2 struct drm_device *mock > > 2 struct drm_device *p_ddev > > 5 struct drm_device *device > > 9 struct drm_device * dev > > 25 struct drm_device *d > > 95 struct drm_device * > > 216 struct drm_device *ddev > > 234 struct drm_device *drm_dev > > 611 struct drm_device *drm > >4190 struct drm_device *dev > > > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > > it's not only me and others like the result of this effort it should be > > followed up by adapting the other structs and the individual usages in > > the different drivers. > > I think this is an unnecessary change. In drm, a dev is usually a drm > device, i.e. struct drm_device *. As shown by the numbers above. > I'd really prefer this patch (series or single) is not accepted. This will cause problems for everyone cherry-picking patches to a downstream kernel (LTS or distro tree). I usually wouldn't expect sympathy here, but the questionable benefit does not outweigh the cost IM[biased]O. Sean > If folks insist on following through with this anyway, I'm firmly in the > camp the name should be "drm" and nothing else. > > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center
[PATCH] drm/amd: Re-classify some log messages in commit path
From: Sean Paul
ATOMIC and DRIVER log categories do not typically contain per-frame log
messages. This patch re-classifies some messages in amd to chattier
categories to keep ATOMIC/DRIVER quiet.
Signed-off-by: Sean Paul
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +++--
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index fae5c1debfad..1fcbab2fd3c3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -113,8 +113,9 @@ static void amdgpu_display_flip_work_func(struct
work_struct *__work)
spin_unlock_irqrestore(>dev->event_lock, flags);
- DRM_DEBUG_DRIVER("crtc:%d[%p], pflip_stat:AMDGPU_FLIP_SUBMITTED, work:
%p,\n",
-amdgpu_crtc->crtc_id, amdgpu_crtc,
work);
+ drm_dbg_vbl(adev_to_drm(adev),
+ "crtc:%d[%p], pflip_stat:AMDGPU_FLIP_SUBMITTED, work:
%p,\n",
+ amdgpu_crtc->crtc_id, amdgpu_crtc, work);
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index b30656959fd8..45d130f86114 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9248,7 +9248,7 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
>flip_addrs[planes_count].address,
afb->tmz_surface, false);
- DRM_DEBUG_ATOMIC("plane: id=%d dcc_en=%d\n",
+ drm_dbg_state(state->dev, "plane: id=%d dcc_en=%d\n",
new_plane_state->plane->index,
bundle->plane_infos[planes_count].dcc.enable);
@@ -9282,7 +9282,7 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
dc_plane,
bundle->flip_addrs[planes_count].flip_timestamp_in_us);
- DRM_DEBUG_ATOMIC("%s Flipping to hi: 0x%x, low: 0x%x\n",
+ drm_dbg_state(state->dev, "%s Flipping to hi: 0x%x, low:
0x%x\n",
__func__,
bundle->flip_addrs[planes_count].address.grph.addr.high_part,
bundle->flip_addrs[planes_count].address.grph.addr.low_part);
@@ -9624,7 +9624,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
- DRM_DEBUG_ATOMIC(
+ drm_dbg_state(state->dev,
"amdgpu_crtc id:%d crtc_state_flags: enable:%d,
active:%d, "
"planes_changed:%d, mode_changed:%d,active_changed:%d,"
"connectors_changed:%d\n",
@@ -10328,7 +10328,7 @@ static int dm_update_crtc_state(struct
amdgpu_display_manager *dm,
if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
goto skip_modeset;
- DRM_DEBUG_ATOMIC(
+ drm_dbg_state(state->dev,
"amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, "
"planes_changed:%d, mode_changed:%d,active_changed:%d,"
"connectors_changed:%d\n",
--
Sean Paul, Software Engineer, Google / Chromium OS
[PATCH v2] drm/amdgpu: Add support for drm_privacy_screen
From: Sean Paul
This patch adds the necessary hooks to make amdgpu aware of privacy
screens. On devices with privacy screen drivers (such as thinkpad-acpi),
the amdgpu driver will defer probe until it's ready and then sync the sw
and hw state on each commit the connector is involved and enabled.
Changes in v2:
-Tweaked the drm_privacy_screen_get() error check to avoid logging
errors when privacy screen is absent (Hans)
Signed-off-by: Sean Paul
Link: https://patchwork.freedesktop.org/patch/477640/ #v1
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 +
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c| 3 +++
.../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 16 +++-
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2ab675123ae3..e2cfae56c020 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
#include "amdgpu_drv.h"
@@ -1988,6 +1989,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
{
struct drm_device *ddev;
struct amdgpu_device *adev;
+ struct drm_privacy_screen *privacy_screen;
unsigned long flags = ent->driver_data;
int ret, retry = 0, i;
bool supports_atomic = false;
@@ -2063,6 +2065,13 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
size = pci_resource_len(pdev, 0);
is_fw_fb = amdgpu_is_fw_framebuffer(base, size);
+ /* If the LCD panel has a privacy screen, defer probe until its ready */
+ privacy_screen = drm_privacy_screen_get(>dev, NULL);
+ if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ drm_privacy_screen_put(privacy_screen);
+
/* Get rid of things like offb */
ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
_kms_driver);
if (ret)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e1d3db3fe8de..9e2bb6523add 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9781,6 +9781,9 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
if (acrtc) {
new_crtc_state = drm_atomic_get_new_crtc_state(state,
>base);
old_crtc_state = drm_atomic_get_old_crtc_state(state,
>base);
+
+ /* Sync the privacy screen state between hw and sw */
+ drm_connector_update_privacy_screen(new_con_state);
}
/* Skip any modesets/resets */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 740435ae3997..594a8002975a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "dm_services.h"
#include "amdgpu.h"
#include "amdgpu_dm.h"
@@ -506,6 +507,7 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
struct amdgpu_dm_connector *aconnector,
int link_index)
{
+ struct drm_device *dev = dm->ddev;
struct dc_link_settings max_link_enc_cap = {0};
aconnector->dm_dp_aux.aux.name =
@@ -519,8 +521,20 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
drm_dp_cec_register_connector(>dm_dp_aux.aux,
>base);
- if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
+ if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
+ struct drm_privacy_screen *privacy_screen;
+
+ /* Reference given up in drm_connector_cleanup() */
+ privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
+ if (!IS_ERR(privacy_screen)) {
+
drm_connector_attach_privacy_screen_provider(>base,
+
privacy_screen);
+ } else if (PTR_ERR(privacy_screen) != -ENODEV) {
+ drm_err(dev, "Error getting privacy screen, ret=%d\n",
+ PTR_ERR(privacy_screen));
+ }
return;
+ }
dc_link_dp_get_max_link_enc_cap(aconnector->dc_link, _link_enc_cap);
aconnector->mst_mgr.cbs = _mst_cbs;
--
Sean Paul, Software Engineer, Google / Chromium OS
[PATCH] drm/amdgpu: Add support for drm_privacy_screen
From: Sean Paul
This patch adds the necessary hooks to make amdgpu aware of privacy
screens. On devices with privacy screen drivers (such as thinkpad-acpi),
the amdgpu driver will defer probe until it's ready and then sync the sw
and hw state on each commit the connector is involved and enabled.
Signed-off-by: Sean Paul
---
I tested this locally, but am not super confident everything is in the
correct place. Hopefully the intent of the patch is clear and we can
tweak positioning if needed.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 +
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c| 3 +++
.../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 16 +++-
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2ab675123ae3..e2cfae56c020 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
#include "amdgpu_drv.h"
@@ -1988,6 +1989,7 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
{
struct drm_device *ddev;
struct amdgpu_device *adev;
+ struct drm_privacy_screen *privacy_screen;
unsigned long flags = ent->driver_data;
int ret, retry = 0, i;
bool supports_atomic = false;
@@ -2063,6 +2065,13 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
size = pci_resource_len(pdev, 0);
is_fw_fb = amdgpu_is_fw_framebuffer(base, size);
+ /* If the LCD panel has a privacy screen, defer probe until its ready */
+ privacy_screen = drm_privacy_screen_get(>dev, NULL);
+ if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ drm_privacy_screen_put(privacy_screen);
+
/* Get rid of things like offb */
ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
_kms_driver);
if (ret)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index e1d3db3fe8de..9e2bb6523add 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9781,6 +9781,9 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
if (acrtc) {
new_crtc_state = drm_atomic_get_new_crtc_state(state,
>base);
old_crtc_state = drm_atomic_get_old_crtc_state(state,
>base);
+
+ /* Sync the privacy screen state between hw and sw */
+ drm_connector_update_privacy_screen(new_con_state);
}
/* Skip any modesets/resets */
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 740435ae3997..e369fc95585e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "dm_services.h"
#include "amdgpu.h"
#include "amdgpu_dm.h"
@@ -506,6 +507,7 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
struct amdgpu_dm_connector *aconnector,
int link_index)
{
+ struct drm_device *dev = dm->ddev;
struct dc_link_settings max_link_enc_cap = {0};
aconnector->dm_dp_aux.aux.name =
@@ -519,8 +521,20 @@ void amdgpu_dm_initialize_dp_connector(struct
amdgpu_display_manager *dm,
drm_dp_cec_register_connector(>dm_dp_aux.aux,
>base);
- if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP)
+ if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP) {
+ struct drm_privacy_screen *privacy_screen;
+
+ /* Reference given up in drm_connector_cleanup() */
+ privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
+ if (!IS_ERR(privacy_screen)) {
+
drm_connector_attach_privacy_screen_provider(>base,
+
privacy_screen);
+ } else {
+ drm_err(dev, "Error getting privacy screen, ret=%d\n",
+ PTR_ERR(privacy_screen));
+ }
return;
+ }
dc_link_dp_get_max_link_enc_cap(aconnector->dc_link, _link_enc_cap);
aconnector->mst_mgr.cbs = _mst_cbs;
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [Intel-gfx] [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
return -EINVAL;
>
> return 0;
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4045e2507e11c..a22d32a7a8719 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -38,6 +38,7 @@ struct drm_atomic_state;
> struct drm_private_obj;
> struct drm_private_state;
>
> +int drm_atomic_helper_check_crtcs(struct drm_atomic_state *state);
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 81c298488b0c8..cb1bf361ad3e3 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -94,12 +94,12 @@ int drm_plane_create_color_properties(struct drm_plane
> *plane,
> enum drm_color_range default_range);
>
> /**
> - * enum drm_color_lut_tests - hw-specific LUT tests to perform
> + * enum drm_color_lut_channels_tests - hw-specific LUT tests to perform
> *
> * The drm_color_lut_check() function takes a bitmask of the values here to
> * determine which tests to apply to a userspace-provided LUT.
> */
> -enum drm_color_lut_tests {
> +enum drm_color_lut_channels_tests {
> /**
>* @DRM_COLOR_LUT_EQUAL_CHANNELS:
>*
> @@ -119,5 +119,6 @@ enum drm_color_lut_tests {
> DRM_COLOR_LUT_NON_DECREASING = BIT(1),
> };
>
> -int drm_color_lut_check(const struct drm_property_blob *lut, u32 tests);
> +int drm_color_lut_channels_check(const struct drm_property_blob *lut,
> + u32 tests);
> #endif
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 2deb15d7e1610..cabd3ef1a6e32 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1072,6 +1072,17 @@ struct drm_crtc {
> /** @funcs: CRTC control functions */
> const struct drm_crtc_funcs *funcs;
>
> + /**
> + * @degamma_lut_size: Size of degamma LUT.
> + */
> + uint32_t degamma_lut_size;
> +
> + /**
> + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace
> such as
> + * X, which doesn't support large lut sizes.
> + */
> + uint32_t gamma_lut_size;
> +
> /**
>* @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
>* by calling drm_mode_crtc_set_gamma_size().
> --
> 2.33.0.882.g93a45727a2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
On Wed, Oct 13, 2021 at 2:12 PM Mark Yacoub wrote:
>
> From: Mark Yacoub
>
> [Why]
> drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT
> sizes. There is no need to check it within amdgpu_dm_atomic_check.
>
> [How]
> Remove the local call to verify LUT sizes and use DRM Core function
> instead.
>
> Tested on ChromeOS Zork.
>
> v1:
> Remove amdgpu_dm_verify_lut_sizes everywhere.
>
Reviewed-by: Sean Paul
> Signed-off-by: Mark Yacoub
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
> .../amd/display/amdgpu_dm/amdgpu_dm_color.c | 35 ---
> 3 files changed, 4 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index f74663b6b046e..47f8de1cfc3a5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10244,6 +10244,10 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
> }
> }
> #endif
> + ret = drm_atomic_helper_check_crtcs(state);
> + if (ret)
> + return ret;
> +
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
>
> @@ -10253,10 +10257,6 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
> dm_old_crtc_state->dsc_force_changed == false)
> continue;
>
> - ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
> - if (ret)
> - goto fail;
> -
> if (!new_crtc_state->enable)
> continue;
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index fcb9c4a629c32..22730e5542092 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -617,7 +617,6 @@ void amdgpu_dm_trigger_timing_sync(struct drm_device
> *dev);
> #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
>
> void amdgpu_dm_init_color_mod(void);
> -int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
> int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
> int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
> struct dc_plane_state *dc_plane_state);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index a022e5bb30a5c..319f8a8a89835 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -284,37 +284,6 @@ static int __set_input_tf(struct dc_transfer_func *func,
> return res ? 0 : -ENOMEM;
> }
>
> -/**
> - * Verifies that the Degamma and Gamma LUTs attached to the |crtc_state| are
> of
> - * the expected size.
> - * Returns 0 on success.
> - */
> -int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state)
> -{
> - const struct drm_color_lut *lut = NULL;
> - uint32_t size = 0;
> -
> - lut = __extract_blob_lut(crtc_state->degamma_lut, );
> - if (lut && size != MAX_COLOR_LUT_ENTRIES) {
> - DRM_DEBUG_DRIVER(
> - "Invalid Degamma LUT size. Should be %u but got
> %u.\n",
> - MAX_COLOR_LUT_ENTRIES, size);
> - return -EINVAL;
> - }
> -
> - lut = __extract_blob_lut(crtc_state->gamma_lut, );
> - if (lut && size != MAX_COLOR_LUT_ENTRIES &&
> - size != MAX_COLOR_LEGACY_LUT_ENTRIES) {
> - DRM_DEBUG_DRIVER(
> - "Invalid Gamma LUT size. Should be %u (or %u for
> legacy) but got %u.\n",
> - MAX_COLOR_LUT_ENTRIES, MAX_COLOR_LEGACY_LUT_ENTRIES,
> - size);
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> /**
> * amdgpu_dm_update_crtc_color_mgmt: Maps DRM color management to DC stream.
> * @crtc: amdgpu_dm crtc state
> @@ -348,10 +317,6 @@ int amdgpu_dm_update_crtc_color_mgmt(struct
> dm_crtc_state *crtc)
> bool is_legacy;
> int r;
>
> - r = amdgpu_dm_verify_lut_sizes(>base);
> - if (r)
> - return r;
> -
> degamma_lut = __extract_blob_lut(crtc->base.degamma_lut,
> _size);
> regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, _size);
>
> --
> 2.33.0.882.g93a45727a2-goog
>
Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
On Fri, Oct 01, 2021 at 04:34:34PM -0400, Sean Paul wrote:
> On Wed, Sep 29, 2021 at 03:39:25PM -0400, Mark Yacoub wrote:
> > From: Mark Yacoub
> >
> > [Why]
> > 1. drm_atomic_helper_check doesn't check for the LUT sizes of either Gamma
> > or Degamma props in the new CRTC state, allowing any invalid size to
> > be passed on.
> > 2. Each driver has its own LUT size, which could also be different for
> > legacy users.
> >
> > [How]
> > 1. Create |degamma_lut_size| and |gamma_lut_size| to save the LUT sizes
> > assigned by the driver when it's initializing its color and CTM
> > management.
> > 2. Create drm_atomic_helper_check_crtc which is called by
> > drm_atomic_helper_check to check the LUT sizes saved in drm_crtc that
> > they match the sizes in the new CRTC state.
> >
>
> Did you consider extending drm_color_lut_check() with the size checks?
>
> > Fixes: igt@kms_color@pipe-A-invalid-gamma-lut-sizes on MTK
> > Tested on Zork(amdgpu) and Jacuzzi(mediatek)
> >
> > Signed-off-by: Mark Yacoub
>
> nit: missing a space between name and email
>
>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 56 +
> > drivers/gpu/drm/drm_color_mgmt.c| 2 ++
> > include/drm/drm_atomic_helper.h | 1 +
> > include/drm/drm_crtc.h | 11 ++
> > 4 files changed, 70 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 2c0c6ec928200..265b9747250d1 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -930,6 +930,58 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_check_planes);
> >
> > +/**
> > + * drm_atomic_helper_check_planes - validate state object for CRTC changes
>
> Ctrl+c/Ctrl+v error here
>
> > + * @state: the driver state object
> > + *
> > + * Check the CRTC state object such as the Gamma/Degamma LUT sizes if the
> > new
>
> Are there missing words between "object" and "such"?
>
> > + * state holds them.
> > + *
> > + * RETURNS:
> > + * Zero for success or -errno
> > + */
> > +int drm_atomic_helper_check_crtc(struct drm_atomic_state *state)
>
> drm_atomic_helper_check_crtcs to be consistent with
> drm_atomic_helper_check_planes
>
> > +{
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *new_crtc_state;
> > + int i;
> > +
> > + for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
>
> no space before (
Ignore this, parsing error on my behalf.
>
> > + if (new_crtc_state->gamma_lut) {
>
> Perhaps gate these with a check of state->color_mgmt_changed first?
>
> > + uint64_t supported_lut_size = crtc->gamma_lut_size;
> > + uint32_t supported_legacy_lut_size = crtc->gamma_size;
> > + uint32_t new_state_lut_size =
> > + drm_color_lut_size(new_crtc_state->gamma_lut);
>
> nit: new_state_lut_size and supported_lut_size can be pulled out to top level
> scope
> to avoid re-instantiation on each iteration
>
> > +
> > + if (new_state_lut_size != supported_lut_size &&
> > + new_state_lut_size != supported_legacy_lut_size) {
>
> According to the docbook, "If drivers support multiple LUT sizes then they
> should publish the largest size, and sub-sample smaller sized LUTs". So
> should this check be > instead of != ?
>
> > + DRM_DEBUG_DRIVER(
>
> drm_dbg_state() is probably more appropriate
>
> > + "Invalid Gamma LUT size. Should be %u
> > (or %u for legacy) but got %u.\n",
> > + supported_lut_size,
> > + supported_legacy_lut_size,
> > + new_state_lut_size);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (new_crtc_state->degamma_lut) {
> > + uint32_t new_state_lut_size =
> > + drm_color_lut_size(new_crtc_state->degamma_lut);
> > + uint64_t supported_lut_size = crtc->degamma_lut_size;
> > +
> > + if (new_state_lut_size != supported_lut_size) {
> > +
Re: [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > Hi all,
> > >
> > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was
> > > to
> > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what
> > > this
> > > patch series is about.
> > >
> > > You will find two types of changes here:
> > >
> > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate)
> > > with
> > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it
> > > has
> > > already been done in previous commits such as b7ea04d2)
> > >
> > > - Replacing "drm_modeset_lock_all()" with
> > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > in the remaining places (as it has already been done in previous
> > > commits
> > > such as 57037094)
> > >
> > > Most of the changes are straight forward, except for a few cases in the
> > > "amd"
> > > and "i915" drivers where some extra dancing was needed to overcome the
> > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be
> > > used
> > > once inside the same function (the reason being that the macro expansion
> > > includes *labels*, and you can not have two labels named the same inside
> > > one
> > > function)
> > >
> > > Notice that, even after this patch series, some places remain where
> > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > present,
> > > all inside drm core (which makes sense), except for two (in "amd" and
> > > "i915")
> > > which cannot be replaced due to the way they are being used.
> > >
> > > Changes in v2:
> > >
> > > - Fix commit message typo
> > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > - Split drm/i915 patch into two simpler ones
> > > - Remove drm_modeset_(un)lock_all()
> > > - Fix build problems in non-x86 platforms
> > >
> > > Fernando Ramos (17):
> > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/vmwgfx: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/amd: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm: cleanup: remove drm_modeset_(un)lock_all()
> > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > >
> >
> > Thank you for revising, Fernando! I've pushed the set to drm-misc-next
> > (along
> > with the necessary drm-tip conflict resolutions).
>
> Ugh. Did anyone actually review the locking changes this does?
> I shot the previous i915 stuff down because the commit messages
> did not address any of it.
I reviewed the set on 9/17, I didn't see your feedback on that thread.
Sean
>
> --
> Ville Syrjälä
> Intel
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 1/2] drm: Add Gamma and Degamma LUT sizes props to drm_crtc to validate.
et)
> return ret;
>
> + ret = drm_atomic_helper_check_crtc(state);
> + if (ret)
> + return ret;
> +
> if (state->legacy_cursor_update)
> state->async_update = !drm_atomic_helper_async_check(dev,
> state);
>
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> b/drivers/gpu/drm/drm_color_mgmt.c
> index bb14f488c8f6c..72a1b628e7cdd 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -166,6 +166,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> struct drm_mode_config *config = >mode_config;
>
> if (degamma_lut_size) {
> + crtc->degamma_lut_size = degamma_lut_size;
> drm_object_attach_property(>base,
> config->degamma_lut_property, 0);
> drm_object_attach_property(>base,
> @@ -178,6 +179,7 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> config->ctm_property, 0);
>
> if (gamma_lut_size) {
> + crtc->gamma_lut_size = gamma_lut_size;
> drm_object_attach_property(>base,
> config->gamma_lut_property, 0);
> drm_object_attach_property(>base,
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4045e2507e11c..3eda13622ca1e 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -38,6 +38,7 @@ struct drm_atomic_state;
> struct drm_private_obj;
> struct drm_private_state;
>
> +int drm_atomic_helper_check_crtc(struct drm_atomic_state *state);
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 13eeba2a750af..c602be2cafca9 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1072,6 +1072,17 @@ struct drm_crtc {
> /** @funcs: CRTC control functions */
> const struct drm_crtc_funcs *funcs;
>
> + /**
> + * @degamma_lut_size: Size of degamma LUT.
> + */
> + uint32_t degamma_lut_size;
> +
> + /**
> + * @gamma_lut_size: Size of Gamma LUT. Not used by legacy userspace
> such as
> + * X, which doesn't support large lut sizes.
> + */
> + uint32_t gamma_lut_size;
> +
Above, you're checking
if (new_state_lut_size != gamma_size && new_state_lut_size != gamma_lut_size)
fail;
doesn't that imply that gamma_size and gamma_lut_size must always be equal? If
so, perhaps turf this new state and rename degamma_lut_size to degamma_size to
be consistent.
De-duping this and initializing crtc->gamma_size in the initialization would
mean the if (crtc->gamma_size) check in drm_crtc_supports_legacy_check() is no
longer useful (and possibly other similar checks), so some care will need to be
taken to avoid regression. I think the effort is worthwhile to avoid introducing
new state.
> /**
>* @gamma_size: Size of legacy gamma ramp reported to userspace. Set up
>* by calling drm_mode_crtc_set_gamma_size().
> --
> 2.33.0.685.g46640cef36-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
On Wed, Sep 29, 2021 at 03:39:26PM -0400, Mark Yacoub wrote:
> From: Mark Yacoub
>
> [Why]
> drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT
> sizes. There is no need to check it within amdgpu_dm_atomic_check.
>
> [How]
> Remove the local call to verify LUT sizes and use DRM Core function
> instead.
>
> Tested on ChromeOS Zork.
>
> Signed-off-by: Mark Yacoub
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 07adac1a8c42b..96a1d006b777e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10683,6 +10683,10 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
> }
> }
> #endif
> + ret = drm_atomic_helper_check_crtc(state);
> + if (ret)
> + return ret;
> +
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
>
> @@ -10692,10 +10696,6 @@ static int amdgpu_dm_atomic_check(struct drm_device
> *dev,
> dm_old_crtc_state->dsc_force_changed == false)
> continue;
>
> - ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
>From a quick glance, I think you can now delete this function. It's called from
amdgpu_dm_update_crtc_color_mgmt() which is part of the commit, so the lut sizes
should have already been checked.
If the call from amdgpu_dm_update_crtc_color_mgmt() is not possible to remove,
you could replace it with a call to the new helper function. And if _that_ is
not possible, please make amdgpu_dm_verify_lut_sizes() static :-)
Sean
> - if (ret)
> - goto fail;
> -
> if (!new_crtc_state->enable)
> continue;
>
> --
> 2.33.0.685.g46640cef36-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 +- > drivers/gpu/drm/tegra/dsi.c | 6 +- > drivers/gpu/drm/tegra/hdmi.c | 6 +- > drivers/gpu/drm/tegra/sor.c | 11 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++- > include/drm/drm_modeset_lock.h| 2 - > 30 files changed, 265 insertions(+), 292 deletions(-) > > > base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f > -- > 2.33.0 > -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 15/15] doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
On Thu, Sep 16, 2021 at 11:15:52PM +0200, Fernando Ramos wrote: > The previous commits do exactly what this entry in the TODO file asks > for, thus we can remove it now as it is no longer applicable. Thanks for doing this work! Can we remove drm_modeset_lock_all[_ctx] now? If so, let's queue that up as part of the set. Reviewed-by: Sean Paul > > Signed-off-by: Fernando Ramos > --- > Documentation/gpu/todo.rst| 17 - > Documentation/locking/ww-mutex-design.rst | 2 +- > 2 files changed, 1 insertion(+), 18 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 12e61869939e..6613543955e9 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -353,23 +353,6 @@ converted, except for struct drm_driver.gem_prime_mmap. > > Level: Intermediate > > -Use DRM_MODESET_LOCK_ALL_* helpers instead of boilerplate > -- > - > -For cases where drivers are attempting to grab the modeset locks with a local > -acquire context. Replace the boilerplate code surrounding > -drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN() and > -DRM_MODESET_LOCK_ALL_END() instead. > - > -This should also be done for all places where drm_modeset_lock_all() is still > -used. > - > -As a reference, take a look at the conversions already completed in drm core. > - > -Contact: Sean Paul, respective driver maintainers > - > -Level: Starter > - > Rename CMA helpers to DMA helpers > - > > diff --git a/Documentation/locking/ww-mutex-design.rst > b/Documentation/locking/ww-mutex-design.rst > index 6a4d7319f8f0..6a8f8beb9ec4 100644 > --- a/Documentation/locking/ww-mutex-design.rst > +++ b/Documentation/locking/ww-mutex-design.rst > @@ -60,7 +60,7 @@ Concepts > Compared to normal mutexes two additional concepts/objects show up in the > lock > interface for w/w mutexes: > > -Acquire context: To ensure eventual forward progress it is important the a > task > +Acquire context: To ensure eventual forward progress it is important that a > task > trying to acquire locks doesn't grab a new reservation id, but keeps the one > it > acquired when starting the lock acquisition. This ticket is stored in the > acquire context. Furthermore the acquire context keeps track of debugging > state > -- > 2.33.0 > -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 14/15] drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
drm_modeset_lock_all(dev);
> - dm_restore_drm_connector_state(dev, connector);
> - drm_modeset_unlock_all(dev);
> -
> - drm_kms_helper_hotplug_event(dev);
> } else if (param[0] == 0) {
> if (!aconnector->dc_link)
> goto unlock;
> @@ -1259,13 +1257,16 @@ static ssize_t trigger_hotplug(struct file *f, const
> char __user *buf,
>
> amdgpu_dm_update_connector_after_detect(aconnector);
>
> - drm_modeset_lock_all(dev);
> - dm_restore_drm_connector_state(dev, connector);
> - drm_modeset_unlock_all(dev);
> -
> - drm_kms_helper_hotplug_event(dev);
> + } else {
> + goto unlock;
> }
>
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> + dm_restore_drm_connector_state(dev, connector);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
Check ret here?
> +
> + drm_kms_helper_hotplug_event(dev);
> +
> unlock:
> mutex_unlock(>hpd_lock);
>
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 13/15] drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:50PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/gma500/psb_device.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_device.c
> b/drivers/gpu/drm/gma500/psb_device.c
> index 951725a0f7a3..4e27f65a1f16 100644
> --- a/drivers/gpu/drm/gma500/psb_device.c
> +++ b/drivers/gpu/drm/gma500/psb_device.c
> @@ -8,6 +8,7 @@
> #include
>
> #include
> +#include
>
> #include "gma_device.h"
> #include "intel_bios.h"
> @@ -169,8 +170,10 @@ static int psb_save_display_registers(struct drm_device
> *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> struct gma_connector *connector;
> struct psb_state *regs = _priv->regs.psb;
> + int ret;
>
> /* Display arbitration control + watermarks */
> regs->saveDSPARB = PSB_RVDC32(DSPARB);
> @@ -183,7 +186,7 @@ static int psb_save_display_registers(struct drm_device
> *dev)
> regs->saveCHICKENBIT = PSB_RVDC32(DSPCHICKENBIT);
>
> /* Save crtc and output state */
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> list_for_each_entry(crtc, >mode_config.crtc_list, head) {
> if (drm_helper_crtc_in_use(crtc))
> dev_priv->ops->save_crtc(crtc);
> @@ -193,7 +196,8 @@ static int psb_save_display_registers(struct drm_device
> *dev)
> if (connector->save)
> connector->save(>base);
>
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> +
> return 0;
Return ret here please
> }
>
> @@ -207,8 +211,10 @@ static int psb_restore_display_registers(struct
> drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> struct gma_connector *connector;
> struct psb_state *regs = _priv->regs.psb;
> + int ret;
>
> /* Display arbitration + watermarks */
> PSB_WVDC32(regs->saveDSPARB, DSPARB);
> @@ -223,7 +229,7 @@ static int psb_restore_display_registers(struct
> drm_device *dev)
> /*make sure VGA plane is off. it initializes to on after reset!*/
> PSB_WVDC32(0x8000, VGACNTRL);
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> list_for_each_entry(crtc, >mode_config.crtc_list, head)
> if (drm_helper_crtc_in_use(crtc))
> dev_priv->ops->restore_crtc(crtc);
> @@ -232,7 +238,7 @@ static int psb_restore_display_registers(struct
> drm_device *dev)
> if (connector->restore)
> connector->restore(>base);
>
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return 0;
Here too
> }
>
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 12/15] drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
+76,7 @@ static int i9xx_pipe_crc_auto_source(struct
> drm_i915_private *dev_priv,
>enum intel_pipe_crc_source *source)
> {
> struct drm_device *dev = _priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
> struct intel_encoder *encoder;
> struct intel_crtc *crtc;
> struct intel_digital_port *dig_port;
> @@ -83,7 +84,7 @@ static int i9xx_pipe_crc_auto_source(struct
> drm_i915_private *dev_priv,
>
> *source = INTEL_PIPE_CRC_SOURCE_PIPE;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> for_each_intel_encoder(dev, encoder) {
> if (!encoder->base.crtc)
> continue;
> @@ -120,7 +121,7 @@ static int i9xx_pipe_crc_auto_source(struct
> drm_i915_private *dev_priv,
> break;
> }
> }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 59fb4c710c8c..7a30e2ff2fed 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1009,31 +1009,35 @@ static void i915_driver_postclose(struct drm_device
> *dev, struct drm_file *file)
> static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> {
> struct drm_device *dev = _priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
> struct intel_encoder *encoder;
> + int ret;
>
> if (!HAS_DISPLAY(dev_priv))
> return;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> for_each_intel_encoder(dev, encoder)
> if (encoder->suspend)
> encoder->suspend(encoder);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> }
>
> static void intel_shutdown_encoders(struct drm_i915_private *dev_priv)
> {
> struct drm_device *dev = _priv->drm;
> + struct drm_modeset_acquire_ctx ctx;
> struct intel_encoder *encoder;
> + int ret;
>
> if (!HAS_DISPLAY(dev_priv))
> return;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> for_each_intel_encoder(dev, encoder)
> if (encoder->shutdown)
> encoder->shutdown(encoder);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> }
>
> void i915_driver_shutdown(struct drm_i915_private *i915)
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 11/15] drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:48PM +0200, Fernando Ramos wrote: > As requested in Documentation/gpu/todo.rst, replace driver calls to > drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and > DRM_MODESET_LOCK_ALL_END() > > Signed-off-by: Fernando Ramos > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 768012243b44..4cbc79eaee17 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1172,14 +1172,16 @@ static int _dpu_debugfs_status_show(struct seq_file > *s, void *data) > struct drm_display_mode *mode; > struct drm_framebuffer *fb; > struct drm_plane_state *state; > + struct drm_modeset_acquire_ctx ctx; > struct dpu_crtc_state *cstate; > > int i, out_width; > + int ret; Please put ret with i & out_width > > dpu_crtc = s->private; > crtc = _crtc->base; > > - drm_modeset_lock_all(crtc->dev); > + DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, ret); > cstate = to_dpu_crtc_state(crtc->state); > > mode = >state->adjusted_mode; > @@ -1263,7 +1265,7 @@ static int _dpu_debugfs_status_show(struct seq_file *s, > void *data) > dpu_crtc->vblank_cb_time = ktime_set(0, 0); > } > > - drm_modeset_unlock_all(crtc->dev); > + DRM_MODESET_LOCK_ALL_END(crtc->dev, ctx, ret); > > return 0; Return ret here > } > -- > 2.33.0 > -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 10/15] drm/nouveau: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:47PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index d7b9f7f8c9e3..eb613af4cdd5 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -667,15 +667,17 @@ nv50_audio_component_bind(struct device *kdev, struct
> device *hda_kdev,
> struct drm_device *drm_dev = dev_get_drvdata(kdev);
> struct nouveau_drm *drm = nouveau_drm(drm_dev);
> struct drm_audio_component *acomp = data;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
> return -ENOMEM;
>
> - drm_modeset_lock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret);
> acomp->ops = _audio_component_ops;
> acomp->dev = kdev;
> drm->audio.component = acomp;
> - drm_modeset_unlock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret);
> return 0;
Return ret here, with that fixed,
Reviewed-by: Sean Paul
> }
>
> @@ -686,12 +688,14 @@ nv50_audio_component_unbind(struct device *kdev, struct
> device *hda_kdev,
> struct drm_device *drm_dev = dev_get_drvdata(kdev);
> struct nouveau_drm *drm = nouveau_drm(drm_dev);
> struct drm_audio_component *acomp = data;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> - drm_modeset_lock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm_dev, ctx, 0, ret);
> drm->audio.component = NULL;
> acomp->ops = NULL;
> acomp->dev = NULL;
> - drm_modeset_unlock_all(drm_dev);
> + DRM_MODESET_LOCK_ALL_END(drm_dev, ctx, ret);
> }
>
> static const struct component_ops nv50_audio_component_bind_ops = {
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 09/15] drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:46PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/omapdrm/omap_fb.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 190afc564914..56013c3ef701 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -62,13 +62,15 @@ static int omap_framebuffer_dirty(struct drm_framebuffer
> *fb,
> unsigned num_clips)
> {
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> - drm_modeset_lock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(fb->dev, ctx, 0, ret);
>
> drm_for_each_crtc(crtc, fb->dev)
> omap_crtc_flush(crtc);
>
> - drm_modeset_unlock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret);
>
> return 0;
Return ret here, with that,
Reviewed-by: Sean Paul
> }
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 09/15] drm/omapdrm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:46PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/omapdrm/omap_fb.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 190afc564914..56013c3ef701 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -62,13 +62,15 @@ static int omap_framebuffer_dirty(struct drm_framebuffer
> *fb,
> unsigned num_clips)
> {
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> - drm_modeset_lock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(fb->dev, ctx, 0, ret);
>
> drm_for_each_crtc(crtc, fb->dev)
> omap_crtc_flush(crtc);
>
> - drm_modeset_unlock_all(fb->dev);
> + DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret);
>
> return 0;
Return ret here
> }
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 08/15] drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:45PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/radeon/radeon_device.c | 13 +
> drivers/gpu/drm/radeon/radeon_dp_mst.c | 7 +--
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c
> b/drivers/gpu/drm/radeon/radeon_device.c
> index 4f0fbf667431..3feb1fd44409 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -37,6 +37,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1559,7 +1560,9 @@ int radeon_suspend_kms(struct drm_device *dev, bool
> suspend,
> struct pci_dev *pdev;
> struct drm_crtc *crtc;
> struct drm_connector *connector;
> + struct drm_modeset_acquire_ctx ctx;
> int i, r;
> + int ret;
Could you please tuck this up with i & r?
>
> if (dev == NULL || dev->dev_private == NULL) {
> return -ENODEV;
> @@ -1573,12 +1576,12 @@ int radeon_suspend_kms(struct drm_device *dev, bool
> suspend,
>
> drm_kms_helper_poll_disable(dev);
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> /* turn off display hw */
> list_for_each_entry(connector, >mode_config.connector_list, head) {
> drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
You should check ret here
>
> /* unpin the front buffers and cursors */
> list_for_each_entry(crtc, >mode_config.crtc_list, head) {
> @@ -1663,7 +1666,9 @@ int radeon_resume_kms(struct drm_device *dev, bool
> resume, bool fbcon)
> struct radeon_device *rdev = dev->dev_private;
> struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> int r;
> + int ret;
Same suggestion here, move up with r
>
> if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> return 0;
> @@ -1741,11 +1746,11 @@ int radeon_resume_kms(struct drm_device *dev, bool
> resume, bool fbcon)
> if (fbcon) {
> drm_helper_resume_force_mode(dev);
> /* turn on display hw */
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> list_for_each_entry(connector,
> >mode_config.connector_list, head) {
> drm_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
> }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
Also check ret here
> }
>
> drm_kms_helper_poll_enable(dev);
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index ec867fa880a4..04fe7b0a6746 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -4,6 +4,7 @@
> #include
> #include
> #include
> +#include
>
> #include "atom.h"
> #include "ni_reg.h"
> @@ -737,11 +738,13 @@ static int radeon_debugfs_mst_info_show(struct seq_file
> *m, void *unused)
> struct radeon_device *rdev = (struct radeon_device *)m->private;
> struct drm_device *dev = rdev->ddev;
> struct drm_connector *connector;
> + struct drm_modeset_acquire_ctx ctx;
> struct radeon_connector *radeon_connector;
> struct radeon_connector_atom_dig *dig_connector;
> int i;
> + int ret;
Move up with i
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> list_for_each_entry(connector, >mode_config.connector_list, head) {
> if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort)
> continue;
> @@ -759,7 +762,7 @@ static int radeon_debugfs_mst_info_show(struct seq_file
> *m, void *unused)
> radeon_connector->cur_stream_attribs[i].fe,
>
> radeon_connector->cur_stream_attribs[i].slots);
> }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return 0;
> }
>
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 07/15] drm/shmobile: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:44PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> index 7db01904d18d..8ee215ab614e 100644
> --- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
> @@ -156,10 +156,12 @@ static int shmob_drm_pm_suspend(struct device *dev)
> static int shmob_drm_pm_resume(struct device *dev)
> {
> struct shmob_drm_device *sdev = dev_get_drvdata(dev);
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> - drm_modeset_lock_all(sdev->ddev);
> + DRM_MODESET_LOCK_ALL_BEGIN(sdev->ddev, ctx, 0, ret);
> shmob_drm_crtc_resume(>crtc);
> - drm_modeset_unlock_all(sdev->ddev);
> + DRM_MODESET_LOCK_ALL_END(sdev->ddev, ctx, ret);
>
> drm_kms_helper_poll_enable(sdev->ddev);
> return 0;
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 06/15] drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:43PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/tegra/dsi.c | 6 --
> drivers/gpu/drm/tegra/hdmi.c | 5 +++--
> drivers/gpu/drm/tegra/sor.c | 10 ++
> 3 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f46d377f0c30..77a496f6a2e9 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -202,10 +202,12 @@ static int tegra_dsi_show_regs(struct seq_file *s, void
> *data)
> struct tegra_dsi *dsi = node->info_ent->data;
> struct drm_crtc *crtc = dsi->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> unsigned int i;
> int err = 0;
> + int ret;
You can use err here instead. With that fixed,
Reviewed-by: Sean Paul
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, ret);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -220,7 +222,7 @@ static int tegra_dsi_show_regs(struct seq_file *s, void
> *data)
> }
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, ret);
> return err;
> }
>
> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index e5d2a4026028..669a2ebb55ae 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -1031,10 +1031,11 @@ static int tegra_hdmi_show_regs(struct seq_file *s,
> void *data)
> struct tegra_hdmi *hdmi = node->info_ent->data;
> struct drm_crtc *crtc = hdmi->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> unsigned int i;
> int err = 0;
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -1049,7 +1050,7 @@ static int tegra_hdmi_show_regs(struct seq_file *s,
> void *data)
> }
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
> return err;
> }
>
> diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> index 0ea320c1092b..323d95eb0cac 100644
> --- a/drivers/gpu/drm/tegra/sor.c
> +++ b/drivers/gpu/drm/tegra/sor.c
> @@ -1490,10 +1490,11 @@ static int tegra_sor_show_crc(struct seq_file *s,
> void *data)
> struct tegra_sor *sor = node->info_ent->data;
> struct drm_crtc *crtc = sor->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> int err = 0;
> u32 value;
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -1522,7 +1523,7 @@ static int tegra_sor_show_crc(struct seq_file *s, void
> *data)
> seq_printf(s, "%08x\n", value);
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
> return err;
> }
>
> @@ -1652,10 +1653,11 @@ static int tegra_sor_show_regs(struct seq_file *s,
> void *data)
> struct tegra_sor *sor = node->info_ent->data;
> struct drm_crtc *crtc = sor->output.encoder.crtc;
> struct drm_device *drm = node->minor->dev;
> + struct drm_modeset_acquire_ctx ctx;
> unsigned int i;
> int err = 0;
>
> - drm_modeset_lock_all(drm);
> + DRM_MODESET_LOCK_ALL_BEGIN(drm, ctx, 0, err);
>
> if (!crtc || !crtc->state->active) {
> err = -EBUSY;
> @@ -1670,7 +1672,7 @@ static int tegra_sor_show_regs(struct seq_file *s, void
> *data)
> }
>
> unlock:
> - drm_modeset_unlock_all(drm);
> + DRM_MODESET_LOCK_ALL_END(drm, ctx, err);
> return err;
> }
>
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 05/15] drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:42PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
Reviewed-by: Sean Paul
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 +++
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> index 28af34ab6ed6..7df35c6f1458 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c
> @@ -28,6 +28,7 @@
> #include "vmwgfx_drv.h"
> #include "vmwgfx_devcaps.h"
> #include
> +#include
> #include "vmwgfx_kms.h"
>
> int vmw_getparam_ioctl(struct drm_device *dev, void *data,
> @@ -172,6 +173,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
> struct drm_vmw_rect __user *clips_ptr;
> struct drm_vmw_rect *clips = NULL;
> struct drm_framebuffer *fb;
> + struct drm_modeset_acquire_ctx ctx;
> struct vmw_framebuffer *vfb;
> struct vmw_resource *res;
> uint32_t num_clips;
> @@ -203,7 +205,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
> goto out_no_copy;
> }
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
> if (!fb) {
> @@ -231,7 +233,7 @@ int vmw_present_ioctl(struct drm_device *dev, void *data,
> out_no_surface:
> drm_framebuffer_put(fb);
> out_no_fb:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> out_no_copy:
> kfree(clips);
> out_clips:
> @@ -250,6 +252,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev,
> void *data,
> struct drm_vmw_rect __user *clips_ptr;
> struct drm_vmw_rect *clips = NULL;
> struct drm_framebuffer *fb;
> + struct drm_modeset_acquire_ctx ctx;
> struct vmw_framebuffer *vfb;
> uint32_t num_clips;
> int ret;
> @@ -280,7 +283,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev,
> void *data,
> goto out_no_copy;
> }
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> fb = drm_framebuffer_lookup(dev, file_priv, arg->fb_id);
> if (!fb) {
> @@ -303,7 +306,7 @@ int vmw_present_readback_ioctl(struct drm_device *dev,
> void *data,
> out_no_ttm_lock:
> drm_framebuffer_put(fb);
> out_no_fb:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> out_no_copy:
> kfree(clips);
> out_clips:
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 74fa41909213..268095cb8c84 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
>
> #include "vmwgfx_kms.h"
>
> @@ -243,15 +244,17 @@ void vmw_kms_legacy_hotspot_clear(struct vmw_private
> *dev_priv)
> struct drm_device *dev = _priv->drm;
> struct vmw_display_unit *du;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> drm_for_each_crtc(crtc, dev) {
> du = vmw_crtc_to_du(crtc);
>
> du->hotspot_x = 0;
> du->hotspot_y = 0;
> }
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> }
>
> void vmw_kms_cursor_post_execbuf(struct vmw_private *dev_priv)
> @@ -1012,9 +1015,10 @@ static int vmw_framebuffer_bo_dirty(struct
> drm_framebuffer *framebuffer,
> struct vmw_framebuffer_bo *vfbd =
> vmw_framebuffer_to_vfbd(framebuffer);
> struct drm_clip_rect norect;
> + struct drm_modeset_acquire_ctx ctx;
> int ret, increment = 1;
>
> - drm_modeset_lock_all(_priv->drm);
> + DRM_MODESET_LOCK_ALL_BEGIN((_priv->drm), ctx, 0, ret);
>
> if (!num_clips) {
> num_clips = 1;
> @@ -1040,7 +1044,7 @@ static int vmw_framebuffer_bo_dirty(struct
> drm_framebuffer *framebuffer,
>
> vmw_cmd_flush(dev_priv, false);
>
> - drm_modeset_unlock_all(_priv->drm);
> + DRM_MODESET_LOCK_ALL_END((_priv->drm), ctx, ret);
>
> return ret;
> }
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 04/15] drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:41PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace driver calls to
> drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
> DRM_MODESET_LOCK_ALL_END()
>
> Signed-off-by: Fernando Ramos
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_client_modeset.c | 5 +++--
> drivers/gpu/drm/drm_crtc_helper.c| 18 --
> drivers/gpu/drm/drm_fb_helper.c | 10 ++
> drivers/gpu/drm/drm_framebuffer.c| 6 --
> 4 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c
> b/drivers/gpu/drm/drm_client_modeset.c
> index 5f5184f071ed..43f772543d2a 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -1062,9 +1062,10 @@ static int drm_client_modeset_commit_legacy(struct
> drm_client_dev *client)
> struct drm_device *dev = client->dev;
> struct drm_mode_set *mode_set;
> struct drm_plane *plane;
> + struct drm_modeset_acquire_ctx ctx;
> int ret = 0;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> drm_for_each_plane(plane, dev) {
> if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> drm_plane_force_disable(plane);
> @@ -1093,7 +1094,7 @@ static int drm_client_modeset_commit_legacy(struct
> drm_client_dev *client)
> goto out;
> }
> out:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index bff917531f33..f3ce073dff79 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -218,11 +218,14 @@ static void
> __drm_helper_disable_unused_functions(struct drm_device *dev)
> */
> void drm_helper_disable_unused_functions(struct drm_device *dev)
> {
> + struct drm_modeset_acquire_ctx ctx;
> + int ret;
> +
> WARN_ON(drm_drv_uses_atomic_modeset(dev));
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> __drm_helper_disable_unused_functions(dev);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> }
> EXPORT_SYMBOL(drm_helper_disable_unused_functions);
>
> @@ -942,12 +945,14 @@ void drm_helper_resume_force_mode(struct drm_device
> *dev)
> struct drm_crtc *crtc;
> struct drm_encoder *encoder;
> const struct drm_crtc_helper_funcs *crtc_funcs;
> + struct drm_modeset_acquire_ctx ctx;
> int encoder_dpms;
> bool ret;
> + int err;
>
> WARN_ON(drm_drv_uses_atomic_modeset(dev));
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err);
> drm_for_each_crtc(crtc, dev) {
>
> if (!crtc->enabled)
> @@ -982,7 +987,7 @@ void drm_helper_resume_force_mode(struct drm_device *dev)
>
> /* disable the unused connectors while restoring the modesetting */
> __drm_helper_disable_unused_functions(dev);
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, err);
> }
> EXPORT_SYMBOL(drm_helper_resume_force_mode);
>
> @@ -1002,9 +1007,10 @@ EXPORT_SYMBOL(drm_helper_resume_force_mode);
> int drm_helper_force_disable_all(struct drm_device *dev)
> {
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> int ret = 0;
>
> - drm_modeset_lock_all(dev);
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> drm_for_each_crtc(crtc, dev)
> if (crtc->enabled) {
> struct drm_mode_set set = {
> @@ -1016,7 +1022,7 @@ int drm_helper_force_disable_all(struct drm_device *dev)
> goto out;
> }
> out:
> - drm_modeset_unlock_all(dev);
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
> return ret;
> }
> EXPORT_SYMBOL(drm_helper_force_disable_all);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 3ab078321045..6860223f0068 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -940,10 +940,11 @@ static int setcmap_legacy(struct fb_cmap *cmap, struct
> fb_info *info)
> struct drm_fb_helper *fb_helper = info->par;
> struct drm_mode_set *modeset;
> struct drm_crtc *crtc;
> + struct drm_modeset_acquire_ctx ctx;
> u16 *r, *g, *b;
> int ret = 0;
>
> - drm_modeset_l
Re: [PATCH 02/15] dmr/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:39PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
>
With the subject fixed (s/dmr/drm),
Reviewed-by: Sean Paul
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 17 -
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 134a6acbd8fb..997a16e85c85 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13476,22 +13476,13 @@ void intel_display_resume(struct drm_device *dev)
> if (state)
> state->acquire_ctx =
>
> - drm_modeset_acquire_init(, 0);
> -
> - while (1) {
> - ret = drm_modeset_lock_all_ctx(dev, );
> - if (ret != -EDEADLK)
> - break;
> -
> - drm_modeset_backoff();
> - }
> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
>
> - if (!ret)
> - ret = __intel_display_resume(dev, state, );
> + ret = __intel_display_resume(dev, state, );
>
> intel_enable_ipc(dev_priv);
> - drm_modeset_drop_locks();
> - drm_modeset_acquire_fini();
> +
> + DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
>
> if (ret)
> drm_err(_priv->drm,
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 03/15] dmr/msm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:40PM +0200, Fernando Ramos wrote:
> As requested in Documentation/gpu/todo.rst, replace the boilerplate code
> surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN()
> and DRM_MODESET_LOCK_ALL_END()
>
With the subject fixed (s/dmr/drm/),
Reviewed-by: Sean Paul
> Signed-off-by: Fernando Ramos
> ---
> drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> index cabe15190ec1..c83db90b0e02 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
> @@ -99,20 +99,18 @@ static void msm_disp_capture_atomic_state(struct
> msm_disp_state *disp_state)
> {
> struct drm_device *ddev;
> struct drm_modeset_acquire_ctx ctx;
> + int ret;
>
> disp_state->timestamp = ktime_get();
>
> ddev = disp_state->drm_dev;
>
> - drm_modeset_acquire_init(, 0);
> -
> - while (drm_modeset_lock_all_ctx(ddev, ) != 0)
> - drm_modeset_backoff();
> + DRM_MODESET_LOCK_ALL_BEGIN(ddev, ctx, 0, ret);
>
> disp_state->atomic_state = drm_atomic_helper_duplicate_state(ddev,
> );
> - drm_modeset_drop_locks();
> - drm_modeset_acquire_fini();
> +
> + DRM_MODESET_LOCK_ALL_END(ddev, ctx, ret);
> }
>
> void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
> --
> 2.33.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH 01/15] dmr: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
On Thu, Sep 16, 2021 at 11:15:38PM +0200, Fernando Ramos wrote: > As requested in Documentation/gpu/todo.rst, replace the boilerplate code > surrounding drm_modeset_lock_all_ctx() with DRM_MODESET_LOCK_ALL_BEGIN() > and DRM_MODESET_LOCK_ALL_END() > Hi Fernando, Thank you for your patch. Could you please fix the subject, changing dmr to drm? > Signed-off-by: Fernando Ramos > --- > drivers/gpu/drm/drm_client_modeset.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index ced09c7c06f9..5f5184f071ed 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -574,6 +574,7 @@ static bool drm_client_firmware_config(struct > drm_client_dev *client, > int num_connectors_detected = 0; > int num_tiled_conns = 0; > struct drm_modeset_acquire_ctx ctx; > + int err; I think you can just reuse 'ret' instead of creating a new variable. That ensures if the lock fails we return the error from the macros. Sean > > if (!drm_drv_uses_atomic_modeset(dev)) > return false; > @@ -585,10 +586,7 @@ static bool drm_client_firmware_config(struct > drm_client_dev *client, > if (!save_enabled) > return false; > > - drm_modeset_acquire_init(, 0); > - > - while (drm_modeset_lock_all_ctx(dev, ) != 0) > - drm_modeset_backoff(); > + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, err); > > memcpy(save_enabled, enabled, count); > mask = GENMASK(count - 1, 0); > @@ -743,8 +741,7 @@ static bool drm_client_firmware_config(struct > drm_client_dev *client, > ret = false; > } > > - drm_modeset_drop_locks(); > - drm_modeset_acquire_fini(); > + DRM_MODESET_LOCK_ALL_END(dev, ctx, err); > > kfree(save_enabled); > return ret; > -- > 2.33.0 > -- Sean Paul, Software Engineer, Google / Chromium OS
[RESEND PATCH v6 06/14] drm/amd: Gate i2c transaction logs on drm_debug_syslog
From: Sean Paul
Since the logs protected by these checks specifically target syslog,
use the new drm_debug_syslog_enabled() call to avoid triggering
these prints when only trace is enabled.
Acked-by: Christian König
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20200608210505.48519-7-s...@poorly.run
#v5
Changes in v5:
-Added to the set
Changes in v6:
-None
---
drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
b/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
index 5c7d769aee3f..d9ceab332060 100644
--- a/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
+++ b/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
@@ -233,7 +233,7 @@ static uint32_t smu_v11_0_i2c_transmit(struct i2c_adapter
*control,
DRM_DEBUG_DRIVER("I2C_Transmit(), address = %x, bytes = %d , data: ",
(uint16_t)address, numbytes);
- if (drm_debug_enabled(DRM_UT_DRIVER)) {
+ if (drm_debug_syslog_enabled(DRM_UT_DRIVER)) {
print_hex_dump(KERN_INFO, "data: ", DUMP_PREFIX_NONE,
16, 1, data, numbytes, false);
}
@@ -389,7 +389,7 @@ static uint32_t smu_v11_0_i2c_receive(struct i2c_adapter
*control,
DRM_DEBUG_DRIVER("I2C_Receive(), address = %x, bytes = %d, data :",
(uint16_t)address, bytes_received);
- if (drm_debug_enabled(DRM_UT_DRIVER)) {
+ if (drm_debug_syslog_enabled(DRM_UT_DRIVER)) {
print_hex_dump(KERN_INFO, "data: ", DUMP_PREFIX_NONE,
16, 1, data, bytes_received, false);
}
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] Revert "drm/amd/display: Fix overlay validation by considering cursors"
On Wed, Jun 16, 2021 at 12:21 PM Rodrigo Siqueira
wrote:
>
> This reverts commit 04cc17a951f73f9a9092ca572b063e6292aeb085.
>
> The patch that we are reverting here was originally applied because it
> fixes multiple IGT issues and flickering in Android. However, after a
> discussion with Sean Paul and Mark, it looks like that this patch might
> cause problems on ChromeOS. For this reason, we decided to revert this
> patch.
Thanks for sending this, Siqueira!
To be clear for those unfamiliar, the issue extends beyond ChromeOS
(we're not just pushing our compositor problems on the rest of the
community).
Relying on cursor enable/disable for atomic creates non-deterministic
behavior which would be very hard for any compositor to reason out
without knowing the hardware-specific limitations. The case I'm
worried about is that the compositor has an overlay active without the
cursor and at some point the compositor enables the cursor which will
fail because of the overlay.
Reviewed-by: Sean Paul
>
> Cc: Nicholas Kazlauskas
> Cc: Harry Wentland
> Cc: Hersen Wu
> Cc: Sean Paul
> Cc: Mark Yacoub
> Cc: Greg Kroah-Hartman
> Signed-off-by: Rodrigo Siqueira
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 8358112b5822..3fd41e098c90 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10200,8 +10200,8 @@ static int validate_overlay(struct drm_atomic_state
> *state)
> {
> int i;
> struct drm_plane *plane;
> - struct drm_plane_state *new_plane_state;
> - struct drm_plane_state *primary_state, *cursor_state, *overlay_state
> = NULL;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
> + struct drm_plane_state *primary_state, *overlay_state = NULL;
>
> /* Check if primary plane is contained inside overlay */
> for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i)
> {
> @@ -10231,14 +10231,6 @@ static int validate_overlay(struct drm_atomic_state
> *state)
> if (!primary_state->crtc)
> return 0;
>
> - /* check if cursor plane is enabled */
> - cursor_state = drm_atomic_get_plane_state(state,
> overlay_state->crtc->cursor);
> - if (IS_ERR(cursor_state))
> - return PTR_ERR(cursor_state);
> -
> - if (drm_atomic_plane_disabling(plane->state, cursor_state))
> - return 0;
> -
> /* Perform the bounds check to ensure the overlay plane covers the
> primary */
> if (primary_state->crtc_x < overlay_state->crtc_x ||
> primary_state->crtc_y < overlay_state->crtc_y ||
> --
> 2.25.1
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Fix overlay validation by considering cursors
On Tue, Jun 8, 2021 at 3:07 PM Harry Wentland wrote: > > > > On 2021-06-08 3:47 a.m., Michel Dänzer wrote: > > On 2021-06-07 8:45 p.m., Sean Paul wrote: > >> > >> > >> On Mon, Jun 7, 2021 at 2:37 PM Harry Wentland >> <mailto:harry.wentl...@amd.com>> wrote: > >> > >> On 2021-06-07 2:19 p.m., Sean Paul wrote: > >> > On Tue, May 18, 2021 at 2:58 PM Rodrigo Siqueira > >> > mailto:rodrigo.sique...@amd.com>> wrote: > >> >> > > > > >> >> Hi Mark and Sean, > >> >> > >> >> I don't know if I fully comprehended the scenario which in my patch > >> >> might cause a ChromeOS crash, but this is what I understood: > >> >> > >> > > >> > Chrome compositor only does an atomic test when the layout geometry > >> > changes (ie: plane is added/removed/resized/moved). This does not > >> take > >> > into consideration the cursor. Once the atomic test has been > >> validated > >> > for a given layout geometry (set of planes/fbs along with their sizes > >> > and locations), Chrome assumes this will continue to be valid. > >> > > >> > So the situation I'm envisioning is if the cursor is hidden, an > >> > overlay-based strategy will pass in the kernel. If at any time the > >> > cursor becomes visible, the kernel will start failing commits which > >> > causes Chrome's GPU process to crash. > >> > > >> > In general I'm uneasy with using the cursor in the atomic check since > >> > it feels like it could be racy independent of the issue above. I > >> > haven't dove into the helper code enough to prove it, just a > >> > spidey-sense. > >> > > >> > >> Isn't the cursor supposed to be just another plane? If so, shouldn't > >> adding/removing the cursor trigger an atomic test? > >> > >> > >> Chrome is using legacy cursor. > >> > >> Yes it will trigger an atomic test in the kernel, and that atomic test > >> will fail. Unfortunately Chrome does not expect a failure so it will crash. > > > > The solution is probably indeed for atomic check to reject state which > > couldn't work if the cursor was enabled, even if the cursor is currently > > disabled. Otherwise one can hit various surprising errors via legacy APIs, > > as described in b836a274b797 "drm/amdgpu/dc: Require primary plane to be > > enabled whenever the CRTC is". > > > > Agreed. > > It's a bit unfortunate but the only way to deal with this better is if we had > some way for DRM master to tell us whether it will only use the atomic IOCTL > or use legacy IOCTLs (including a combination of atomic and legacy). > I think even with this information we wouldn't want to depend on cursor for atomic test. IMO kernel should not back the compositor into re-rendering the scene based on cursor visibility. Given that cursor is usually handled independently and asynchronously from composition, it makes things extremely complex and most compositors would probably just enable cursor all the time to work around this. Sean > Harry > > > > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v5 06/13] drm/amd: Gate i2c transaction logs on drm_debug_syslog
From: Sean Paul
Since the logs protected by these checks specifically target syslog,
use the new drm_debug_syslog_enabled() call to avoid triggering
these prints when only trace is enabled.
Signed-off-by: Sean Paul
Changes in v5:
-Added to the set
---
drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
b/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
index 9bffbab35041..9bc6baddd302 100644
--- a/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
+++ b/drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c
@@ -233,7 +233,7 @@ static uint32_t smu_v11_0_i2c_transmit(struct i2c_adapter
*control,
DRM_DEBUG_DRIVER("I2C_Transmit(), address = %x, bytes = %d , data: ",
(uint16_t)address, numbytes);
- if (drm_debug_enabled(DRM_UT_DRIVER)) {
+ if (drm_debug_syslog_enabled(DRM_UT_DRIVER)) {
print_hex_dump(KERN_INFO, "data: ", DUMP_PREFIX_NONE,
16, 1, data, numbytes, false);
}
@@ -387,7 +387,7 @@ static uint32_t smu_v11_0_i2c_receive(struct i2c_adapter
*control,
DRM_DEBUG_DRIVER("I2C_Receive(), address = %x, bytes = %d, data :",
(uint16_t)address, bytes_received);
- if (drm_debug_enabled(DRM_UT_DRIVER)) {
+ if (drm_debug_syslog_enabled(DRM_UT_DRIVER)) {
print_hex_dump(KERN_INFO, "data: ", DUMP_PREFIX_NONE,
16, 1, data, bytes_received, false);
}
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/dp_mst: Remove VCPI while disabling topology mgr
On Fri, Jan 17, 2020 at 3:27 PM Lyude Paul wrote: > > On Fri, 2020-01-17 at 11:19 -0500, Sean Paul wrote: > > On Mon, Dec 9, 2019 at 12:56 AM Lin, Wayne wrote: > > > > > > > > > > -Original Message- > > > > From: Lyude Paul > > > > Sent: Saturday, December 7, 2019 3:57 AM > > > > To: Lin, Wayne ; dri-de...@lists.freedesktop.org; > > > > amd-gfx@lists.freedesktop.org > > > > Cc: Kazlauskas, Nicholas ; Wentland, Harry > > > > ; Zuo, Jerry ; > > > > sta...@vger.kernel.org > > > > Subject: Re: [PATCH v2] drm/dp_mst: Remove VCPI while disabling topology > > > > mgr > > > > > > > > On Fri, 2019-12-06 at 14:24 -0500, Lyude Paul wrote: > > > > > Reviewed-by: Lyude Paul > > > > > > > > > > I'll go ahead and push this to drm-misc-next-fixes right now, thanks! > > > > > > > > Whoops-meant to say drm-misc-next here, anyway, pushed! > > > Thanks for your time! > > > > > > > I'm getting the following warning on unplug with this patch: > > \snip > > I think I've got a better fix for this that should avoid that problem, I'll > write up a patch and send it out in a bit Thanks Lyude! Should we revert this patch for the time being? > > > -- > Cheers, > Lyude Paul > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/dp_mst: Remove VCPI while disabling topology mgr
On Mon, Dec 9, 2019 at 12:56 AM Lin, Wayne wrote:
>
>
>
> > -Original Message-
> > From: Lyude Paul
> > Sent: Saturday, December 7, 2019 3:57 AM
> > To: Lin, Wayne ; dri-de...@lists.freedesktop.org;
> > amd-gfx@lists.freedesktop.org
> > Cc: Kazlauskas, Nicholas ; Wentland, Harry
> > ; Zuo, Jerry ;
> > sta...@vger.kernel.org
> > Subject: Re: [PATCH v2] drm/dp_mst: Remove VCPI while disabling topology
> > mgr
> >
> > On Fri, 2019-12-06 at 14:24 -0500, Lyude Paul wrote:
> > > Reviewed-by: Lyude Paul
> > >
> > > I'll go ahead and push this to drm-misc-next-fixes right now, thanks!
> >
> > Whoops-meant to say drm-misc-next here, anyway, pushed!
> Thanks for your time!
>
I'm getting the following warning on unplug with this patch:
[ 54.010099]
[ 54.011765] ==
[ 54.018670] WARNING: possible circular locking dependency detected
[ 54.025577] 5.5.0-rc6-02274-g77381c23ee63 #47 Not tainted
[ 54.031610] --
[ 54.038516] kworker/1:6/1040 is trying to acquire lock:
[ 54.044354] 888272af3228 (>payload_lock){+.+.}, at:
drm_dp_mst_topology_mgr_set_mst+0x218/0x2e4
[ 54.054957]
[ 54.054957] but task is already holding lock:
[ 54.061473] 888272af3060 (>lock){+.+.}, at:
drm_dp_mst_topology_mgr_set_mst+0x3c/0x2e4
[ 54.071193]
[ 54.071193] which lock already depends on the new lock.
[ 54.071193]
[ 54.080334]
[ 54.080334] the existing dependency chain (in reverse order) is:
[ 54.088697]
[ 54.088697] -> #1 (>lock){+.+.}:
[ 54.094440]__mutex_lock+0xc3/0x498
[ 54.099015]drm_dp_mst_topology_get_port_validated+0x25/0x80
[ 54.106018]drm_dp_update_payload_part1+0xa2/0x2e2
[ 54.112051]intel_mst_pre_enable_dp+0x144/0x18f
[ 54.117791]intel_encoders_pre_enable+0x63/0x70
[ 54.123532]hsw_crtc_enable+0xa1/0x722
[ 54.128396]intel_update_crtc+0x50/0x194
[ 54.133455]skl_commit_modeset_enables+0x40c/0x540
[ 54.139485]intel_atomic_commit_tail+0x5f7/0x130d
[ 54.145418]intel_atomic_commit+0x2c8/0x2d8
[ 54.150770]drm_atomic_helper_set_config+0x5a/0x70
[ 54.156801]drm_mode_setcrtc+0x2ab/0x833
[ 54.161862]drm_ioctl+0x2e5/0x424
[ 54.166242]vfs_ioctl+0x21/0x2f
[ 54.170426]do_vfs_ioctl+0x5fb/0x61e
[ 54.175096]ksys_ioctl+0x55/0x75
[ 54.179377]__x64_sys_ioctl+0x1a/0x1e
[ 54.184146]do_syscall_64+0x5c/0x6d
[ 54.188721]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 54.194946]
[ 54.194946] -> #0 (>payload_lock){+.+.}:
[ 54.201463]
[ 54.201463] other info that might help us debug this:
[ 54.201463]
[ 54.210410] Possible unsafe locking scenario:
[ 54.210410]
[ 54.217025]CPU0CPU1
[ 54.222082]
[ 54.227138] lock(>lock);
[ 54.230643]lock(>payload_lock);
[ 54.237742]lock(>lock);
[ 54.244062] lock(>payload_lock);
[ 54.248346]
[ 54.248346] *** DEADLOCK ***
[ 54.248346]
[ 54.254959] 7 locks held by kworker/1:6/1040:
[ 54.259822] #0: 888275c4f528 ((wq_completion)events){+.+.},
at: worker_thread+0x455/0x6e2
[ 54.269451] #1: c9000119beb0
((work_completion)(&(_priv->hotplug.hotplug_work)->work)){+.+.},
at: worker_thread+0x455/0x6e2
[ 54.282768] #2: 888272a403f0 (>mode_config.mutex){+.+.},
at: i915_hotplug_work_func+0x4b/0x2be
[ 54.293368] #3: 824fc6c0 (drm_connector_list_iter){.+.+},
at: i915_hotplug_work_func+0x17e/0x2be
[ 54.304061] #4: c9000119bc58 (crtc_ww_class_acquire){+.+.},
at: drm_helper_probe_detect_ctx+0x40/0xfd
[ 54.314855] #5: 888272a40470 (crtc_ww_class_mutex){+.+.}, at:
drm_modeset_lock+0x74/0xe2
[ 54.324385] #6: 888272af3060 (>lock){+.+.}, at:
drm_dp_mst_topology_mgr_set_mst+0x3c/0x2e4
[ 54.334597]
[ 54.334597] stack backtrace:
[ 54.339464] CPU: 1 PID: 1040 Comm: kworker/1:6 Not tainted
5.5.0-rc6-02274-g77381c23ee63 #47
[ 54.348893] Hardware name: Google Fizz/Fizz, BIOS
Google_Fizz.10139.39.0 01/04/2018
[ 54.357451] Workqueue: events i915_hotplug_work_func
[ 54.362995] Call Trace:
[ 54.365724] dump_stack+0x71/0x9c
[ 54.369427] check_noncircular+0x91/0xbc
[ 54.373809] ? __lock_acquire+0xc9e/0xf66
[ 54.378286] ? __lock_acquire+0xc9e/0xf66
[ 54.382763] ? lock_acquire+0x175/0x1ac
[ 54.387048] ? drm_dp_mst_topology_mgr_set_mst+0x218/0x2e4
[ 54.393177] ? __mutex_lock+0xc3/0x498
[ 54.397362] ? drm_dp_mst_topology_mgr_set_mst+0x218/0x2e4
[ 54.403492] ? drm_dp_mst_topology_mgr_set_mst+0x218/0x2e4
[ 54.409620] ? drm_dp_dpcd_access+0xd9/0x101
[ 54.414390] ? drm_dp_mst_topology_mgr_set_mst+0x218/0x2e4
[ 54.420517] ? drm_dp_mst_topology_mgr_set_mst+0x218/0x2e4
[ 54.426645] ? intel_digital_port_connected+0x34d/0x35c
[ 54.432482] ? intel_dp_detect+0x227/0x44e
[
Re: [PATCH v9 12/18] drm/dp_mst: Add branch bandwidth validation to MST atomic check
On Fri, Jan 17, 2020 at 10:26 AM Mikita Lipski wrote: > > > > On 1/17/20 10:09 AM, Sean Paul wrote: > > On Fri, Dec 13, 2019 at 3:09 PM wrote: > >> > >> From: Mikita Lipski > >> > > > > Hi Mikita, > > Unfortunately this patch causes a crash on my i915 device when I > > unplug my MST hub. The panic is below. > > Hi Sean, > > I thought this issue was fixed by Wayne Lin in > https://patchwork.freedesktop.org/patch/346736/?series=71388=1 > but now I checked it seems it never got pushed. I will resend Wayne's > patch once again. > No need to resend, I can push Wayne's patch. Thanks for the pointer, Sean > Thanks > Mikita > > > > [ 38.514014] BUG: kernel NULL pointer dereference, address: > > 0030 > > [ 38.521801] #PF: supervisor read access in kernel mode > > [ 38.527556] #PF: error_code(0x) - not-present page > > [ 38.533299] PGD 0 P4D 0 > > [ 38.536127] Oops: [#1] PREEMPT SMP PTI > > [ 38.540798] CPU: 1 PID: 1324 Comm: DrmThread Not tainted > > 5.5.0-rc6-02273-g9bb4096398e7 #36 > > [ 38.550040] Hardware name: Google Fizz/Fizz, BIOS > > Google_Fizz.10139.39.0 01/04/2018 > > [ 38.558606] RIP: 0010:drm_dp_mst_atomic_check_bw_limit+0x11/0x102 > > [ 38.565418] Code: 05 ff cb bf 19 48 f7 f6 c3 0f 1f 44 00 00 55 b8 > > 0b 80 ff 0f 48 89 e5 5d c3 55 48 89 e5 41 57 41 56 41 55 41 54 4c 8d > > 77 30 53 <48> 8b 47 30 49 89 fd 49 89 f7 45 31 e4 48 8d 58 e8 48 8d 53 > > 18 4c > > [ 38.586422] RSP: 0018:c9000139f9d8 EFLAGS: 00010282 > > [ 38.592264] RAX: RBX: 888272aeac88 RCX: > > 888236f529e0 > > [ 38.600242] RDX: 888272aeac88 RSI: 888236f529e0 RDI: > > > > [ 38.608220] RBP: c9000139fa00 R08: 0031 R09: > > 000e > > [ 38.616198] R10: 888236f529e8 R11: 8882621f3440 R12: > > > > [ 38.624176] R13: 888236f529d0 R14: 0030 R15: > > 888236f529e0 > > [ 38.632153] FS: 7cd9229ce700() GS:888276c8() > > knlGS: > > [ 38.641193] CS: 0010 DS: ES: CR0: 80050033 > > [ 38.647616] CR2: 0030 CR3: 0002618e8004 CR4: > > 003606e0 > > [ 38.655593] Call Trace: > > [ 38.658329] drm_dp_mst_atomic_check+0x152/0x16d > > [ 38.663484] intel_atomic_check+0xcfe/0x1e6f > > [ 38.668259] ? trace_hardirqs_on+0x28/0x3d > > [ 38.672835] ? intel_pipe_config_compare+0x1b38/0x1b38 > > [ 38.678580] drm_atomic_check_only+0x5ab/0x70f > > [ 38.683547] ? drm_atomic_set_crtc_for_connector+0xf5/0x102 > > [ 38.689778] ? drm_atomic_helper_shutdown+0xb6/0xb6 > > [ 38.695221] drm_atomic_commit+0x18/0x53 > > [ 38.699604] drm_atomic_helper_set_config+0x5a/0x70 > > [ 38.705057] drm_mode_setcrtc+0x2ab/0x833 > > [ 38.709537] ? rcu_read_unlock+0x57/0x57 > > [ 38.713920] ? drm_mode_getcrtc+0x173/0x173 > > [ 38.718594] drm_ioctl+0x2e5/0x424 > > [ 38.722392] ? drm_mode_getcrtc+0x173/0x173 > > [ 38.727069] vfs_ioctl+0x21/0x2f > > [ 38.730675] do_vfs_ioctl+0x5fb/0x61e > > [ 38.734766] ksys_ioctl+0x55/0x75 > > [ 38.738469] __x64_sys_ioctl+0x1a/0x1e > > [ 38.742659] do_syscall_64+0x5c/0x6d > > [ 38.746653] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > [ 38.752298] RIP: 0033:0x7cd92552d497 > > [ 38.756291] Code: 8a 66 90 48 8b 05 d1 d9 2b 00 64 c7 00 26 00 00 > > 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 > > 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a1 d9 2b 00 f7 d8 64 89 > > 01 48 > > [ 38.777296] RSP: 002b:7cd9229cd698 EFLAGS: 0246 ORIG_RAX: > > 0010 > > [ 38.785762] RAX: ffda RBX: 20323373da80 RCX: > > 7cd92552d497 > > [ 38.793740] RDX: 7cd9229cd6d0 RSI: c06864a2 RDI: > > 001c > > [ 38.801717] RBP: 7cd9229cd6c0 R08: R09: > > > > [ 38.809693] R10: R11: 0246 R12: > > 001c > > [ 38.817670] R13: R14: 7cd9229cd6d0 R15: > > c06864a2 > > [ 38.825642] Modules linked in: xt_nat cdc_ether r8152 bridge stp > > llc usbhid btusb btrtl btbcm btintel bluetooth asix usbnet > > ecdh_generic ecc mii snd_soc_hdac_hdmi snd_soc_dmic xhci_pci xhci_hcd > > snd_soc_skl snd_soc_sst_ipc snd_soc_sst_dsp snd_hda_ext_core > > snd_intel_dspcfg snd_hda_core usbcore usb_common acpi_als kfifo_buf
Re: [PATCH v9 12/18] drm/dp_mst: Add branch bandwidth validation to MST atomic check
On Fri, Dec 13, 2019 at 3:09 PM wrote: > > From: Mikita Lipski > Hi Mikita, Unfortunately this patch causes a crash on my i915 device when I unplug my MST hub. The panic is below. [ 38.514014] BUG: kernel NULL pointer dereference, address: 0030 [ 38.521801] #PF: supervisor read access in kernel mode [ 38.527556] #PF: error_code(0x) - not-present page [ 38.533299] PGD 0 P4D 0 [ 38.536127] Oops: [#1] PREEMPT SMP PTI [ 38.540798] CPU: 1 PID: 1324 Comm: DrmThread Not tainted 5.5.0-rc6-02273-g9bb4096398e7 #36 [ 38.550040] Hardware name: Google Fizz/Fizz, BIOS Google_Fizz.10139.39.0 01/04/2018 [ 38.558606] RIP: 0010:drm_dp_mst_atomic_check_bw_limit+0x11/0x102 [ 38.565418] Code: 05 ff cb bf 19 48 f7 f6 c3 0f 1f 44 00 00 55 b8 0b 80 ff 0f 48 89 e5 5d c3 55 48 89 e5 41 57 41 56 41 55 41 54 4c 8d 77 30 53 <48> 8b 47 30 49 89 fd 49 89 f7 45 31 e4 48 8d 58 e8 48 8d 53 18 4c [ 38.586422] RSP: 0018:c9000139f9d8 EFLAGS: 00010282 [ 38.592264] RAX: RBX: 888272aeac88 RCX: 888236f529e0 [ 38.600242] RDX: 888272aeac88 RSI: 888236f529e0 RDI: [ 38.608220] RBP: c9000139fa00 R08: 0031 R09: 000e [ 38.616198] R10: 888236f529e8 R11: 8882621f3440 R12: [ 38.624176] R13: 888236f529d0 R14: 0030 R15: 888236f529e0 [ 38.632153] FS: 7cd9229ce700() GS:888276c8() knlGS: [ 38.641193] CS: 0010 DS: ES: CR0: 80050033 [ 38.647616] CR2: 0030 CR3: 0002618e8004 CR4: 003606e0 [ 38.655593] Call Trace: [ 38.658329] drm_dp_mst_atomic_check+0x152/0x16d [ 38.663484] intel_atomic_check+0xcfe/0x1e6f [ 38.668259] ? trace_hardirqs_on+0x28/0x3d [ 38.672835] ? intel_pipe_config_compare+0x1b38/0x1b38 [ 38.678580] drm_atomic_check_only+0x5ab/0x70f [ 38.683547] ? drm_atomic_set_crtc_for_connector+0xf5/0x102 [ 38.689778] ? drm_atomic_helper_shutdown+0xb6/0xb6 [ 38.695221] drm_atomic_commit+0x18/0x53 [ 38.699604] drm_atomic_helper_set_config+0x5a/0x70 [ 38.705057] drm_mode_setcrtc+0x2ab/0x833 [ 38.709537] ? rcu_read_unlock+0x57/0x57 [ 38.713920] ? drm_mode_getcrtc+0x173/0x173 [ 38.718594] drm_ioctl+0x2e5/0x424 [ 38.722392] ? drm_mode_getcrtc+0x173/0x173 [ 38.727069] vfs_ioctl+0x21/0x2f [ 38.730675] do_vfs_ioctl+0x5fb/0x61e [ 38.734766] ksys_ioctl+0x55/0x75 [ 38.738469] __x64_sys_ioctl+0x1a/0x1e [ 38.742659] do_syscall_64+0x5c/0x6d [ 38.746653] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 38.752298] RIP: 0033:0x7cd92552d497 [ 38.756291] Code: 8a 66 90 48 8b 05 d1 d9 2b 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a1 d9 2b 00 f7 d8 64 89 01 48 [ 38.777296] RSP: 002b:7cd9229cd698 EFLAGS: 0246 ORIG_RAX: 0010 [ 38.785762] RAX: ffda RBX: 20323373da80 RCX: 7cd92552d497 [ 38.793740] RDX: 7cd9229cd6d0 RSI: c06864a2 RDI: 001c [ 38.801717] RBP: 7cd9229cd6c0 R08: R09: [ 38.809693] R10: R11: 0246 R12: 001c [ 38.817670] R13: R14: 7cd9229cd6d0 R15: c06864a2 [ 38.825642] Modules linked in: xt_nat cdc_ether r8152 bridge stp llc usbhid btusb btrtl btbcm btintel bluetooth asix usbnet ecdh_generic ecc mii snd_soc_hdac_hdmi snd_soc_dmic xhci_pci xhci_hcd snd_soc_skl snd_soc_sst_ipc snd_soc_sst_dsp snd_hda_ext_core snd_intel_dspcfg snd_hda_core usbcore usb_common acpi_als kfifo_buf industrialio xt_MASQUERADE iptable_nat nf_nat xt_mark fuse ip6table_filter iwlmvm mac80211 r8169 realtek iwlwifi lzo_rle lzo_compress zram cfg80211 [ 38.871839] CR2: 0030 [ 38.875542] ---[ end trace 6bb39ec52e30c7cb ]--- [ 38.886142] RIP: 0010:drm_dp_mst_atomic_check_bw_limit+0x11/0x102 [ 38.892957] Code: 05 ff cb bf 19 48 f7 f6 c3 0f 1f 44 00 00 55 b8 0b 80 ff 0f 48 89 e5 5d c3 55 48 89 e5 41 57 41 56 41 55 41 54 4c 8d 77 30 53 <48> 8b 47 30 49 89 fd 49 89 f7 45 31 e4 48 8d 58 e8 48 8d 53 18 4c [ 38.913964] RSP: 0018:c9000139f9d8 EFLAGS: 00010282 [ 38.919804] RAX: RBX: 888272aeac88 RCX: 888236f529e0 [ 38.927784] RDX: 888272aeac88 RSI: 888236f529e0 RDI: [ 38.935765] RBP: c9000139fa00 R08: 0031 R09: 000e [ 38.943733] R10: 888236f529e8 R11: 8882621f3440 R12: [ 38.951712] R13: 888236f529d0 R14: 0030 R15: 888236f529e0 [ 38.959692] FS: 7cd9229ce700() GS:888276c8() knlGS: [ 38.968730] CS: 0010 DS: ES: CR0: 80050033 [ 38.975144] CR2: 0030 CR3: 0002618e8004 CR4: 003606e0 [ 38.983121] Kernel panic - not syncing: Fatal exception [ 38.988967] Kernel Offset: disabled [
Re: [PATCH RESEND 07/14] drm/msm/hdmi: Provide ddc symlink in hdmi connector sysfs directory
On Mon, Aug 26, 2019 at 3:27 PM Andrzej Pietrasiewicz wrote: > > Use the ddc pointer provided by the generic connector. > > Signed-off-by: Andrzej Pietrasiewicz > Acked-by: Sam Ravnborg > Reviewed-by: Emil Velikov Acked-by: Sean Paul > --- > drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c > b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c > index 07b4cb877d82..1f03262b8a52 100644 > --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c > @@ -450,8 +450,10 @@ struct drm_connector *msm_hdmi_connector_init(struct > hdmi *hdmi) > > connector = _connector->base; > > - drm_connector_init(hdmi->dev, connector, _connector_funcs, > - DRM_MODE_CONNECTOR_HDMIA); > + drm_connector_init_with_ddc(hdmi->dev, connector, > + _connector_funcs, > + DRM_MODE_CONNECTOR_HDMIA, > + hdmi->i2c); > drm_connector_helper_add(connector, _hdmi_connector_helper_funcs); > > connector->polled = DRM_CONNECTOR_POLL_CONNECT | > -- > 2.17.1 > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 07/14] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()
On Mon, Oct 21, 2019 at 10:36:02PM -0400, Lyude Paul wrote:
> This probably hasn't caused any problems up until now since it's
> probably nearly impossible to encounter this in the wild, however if we
> were to receive a connection status notification from the MST hub after
> resume while we're in the middle of reprobing the link addresses for a
> topology then there's a much larger chance that a port could have
> changed from being an output port to input port (or vice versa). If we
> forget to update this bit of information, we'll potentially ignore a
> valid PDT change on a downstream port because we think it's an input
> port.
>
> So, make sure we read the input_port field in connection status
> notifications in drm_dp_mst_handle_conn_stat() to prevent this from
> happening once we've implemented suspend/resume reprobing.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 52 +++
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 7bf4db91ff90..c8e218b902ae 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2079,18 +2079,40 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> - int old_ddps;
> - bool dowork = false;
> + int old_ddps, ret;
> + u8 new_pdt;
> + bool dowork = false, create_connector = false;
>
> port = drm_dp_get_port(mstb, conn_stat->port_number);
> if (!port)
> return;
>
> - /* Locking is only needed if the port's exposed to userspace */
> - if (port->connector)
> + if (port->connector) {
> + if (!port->input && conn_stat->input_port) {
> + /*
> + * We can't remove a connector from an already exposed
> + * port, so just throw the port out and make sure we
> + * reprobe the link address of it's parent MSTB
> + */
> + drm_dp_mst_topology_unlink_port(mgr, port);
> + mstb->link_address_sent = false;
> + dowork = true;
> + goto out;
> + }
> +
> + /*
> + * Locking is only needed if the port's exposed to userspace
> + */
optional nit: this will still fit on one line
> drm_modeset_lock(>base.lock, NULL);
> + } else if (port->input && !conn_stat->input_port) {
> + create_connector = true;
> + /* Reprobe link address so we get num_sdp_streams */
> + mstb->link_address_sent = false;
> + dowork = true;
> + }
>
> old_ddps = port->ddps;
> + port->input = conn_stat->input_port;
> port->mcs = conn_stat->message_capability_status;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2103,21 +2125,23 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
> }
> }
>
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> - conn_stat->peer_device_type);
> - if (ret == 1) {
> - dowork = true;
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT for port %p: %d\n",
> - port, ret);
> - dowork = false;
> - }
> + new_pdt = port->input ? DP_PEER_DEVICE_NONE :
> conn_stat->peer_device_type;
> +
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> + port, ret);
> + dowork = false;
> }
>
> if (port->connector)
> drm_modeset_unlock(>base.lock);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
>
> +out:
> drm_dp_mst_topology_put_port(port);
> if (dowork)
> queue_work(system_long_wq, >mgr->work);
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 06/14] drm/dp_mst: Protect drm_dp_mst_port members with locking
s of drm_dp_mst_port other than port->connector, we
> simply grab >base.lock in drm_dp_mst_link_probe_work() for already
> registered ports, update said members and drop the lock before
> potentially registering a connector and probing the link address of it's
> children.
>
> Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock
> acquisition context in order to acquire >base.lock under
> _mutex and convert all it's users over to using the
> .detect_ctx probe hooks.
>
> With that, we finally have well defined locking.
>
> Changes since v4:
> * Get rid of port->mutex, stop using connection_mutex and just use our own
> modesetting lock - mgr->base.lock. Also, add a probe_lock that comes
> before this patch.
> * Just throw out ports that get changed from an output to an input, and
> replace them with new ports. This lets us ensure that modesetting
> contexts never see port->connector go from having a connector to being
> NULL.
> * Write an extremely detailed explanation of what problems this is
> trying to fix, since there's a _lot_ of context here and I honestly
> forgot some of it myself a couple times.
> * Don't grab mgr->lock when reading port->mstb in
> drm_dp_mst_handle_link_address_port(). It's not needed.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Cc: Sean Paul
> Signed-off-by: Lyude Paul
Overall makes sense to me. Thanks for the comprehensive commit message and
comments, they definitely help :)
Just one nit below,
Reviewed-by: Sean Paul
> ---
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 28 +--
> drivers/gpu/drm/drm_dp_mst_topology.c | 230 --
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 28 ++-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 32 +--
> drivers/gpu/drm/radeon/radeon_dp_mst.c| 24 +-
> include/drm/drm_dp_mst_helper.h | 38 ++-
> 6 files changed, 240 insertions(+), 140 deletions(-)
>
/snip
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 11d842f0bff5..7bf4db91ff90 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
/snip
> @@ -1912,35 +1984,40 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> - bool created = false;
> - int old_ddps = 0;
> + int old_ddps = 0, ret;
> + u8 new_pdt = DP_PEER_DEVICE_NONE;
> + bool created = false, send_link_addr = false;
>
> port = drm_dp_get_port(mstb, port_msg->port_number);
> if (!port) {
> - port = kzalloc(sizeof(*port), GFP_KERNEL);
> + port = drm_dp_mst_add_port(dev, mgr, mstb,
> +port_msg->port_number);
> if (!port)
> return;
> - kref_init(>topology_kref);
> - kref_init(>malloc_kref);
> - port->parent = mstb;
> - port->port_num = port_msg->port_number;
> - port->mgr = mgr;
> - port->aux.name = "DPMST";
> - port->aux.dev = dev->dev;
> - port->aux.is_remote = true;
> -
> - /*
> - * Make sure the memory allocation for our parent branch stays
> - * around until our own memory allocation is released
> + created = true;
> + } else if (port_msg->input_port && !port->input && port->connector) {
> + /* Destroying the connector is impossible in this context, so
> + * replace the port with a new one
>*/
> - drm_dp_mst_get_mstb_malloc(mstb);
> + drm_dp_mst_topology_unlink_port(mgr, port);
> + drm_dp_mst_topology_put_port(port);
>
> + port = drm_dp_mst_add_port(dev, mgr, mstb,
> +port_msg->port_number);
> + if (!port)
> + return;
> created = true;
> } else {
> + /* Locking is only needed when the port has a connector
> + * exposed to userspace
> + */
> + drm_modeset_lock(>base.lock, NULL);
Random musing: It's kind of unfortunate that we don't have a void varient of
drm_modeset_lock for when there's no acquire_ctx since we end up with a mix of
drm_modeset_lock calls with and without return checking.
/snip
> @@ -3441,22 +3516,31 @@ EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
> /**
Re: [PATCH v5 05/14] drm/dp_mst: Add probe_lock
On Mon, Oct 21, 2019 at 10:36:00PM -0400, Lyude Paul wrote:
> Currently, MST lacks locking in a lot of places that really should have
> some sort of locking. Hotplugging and link address code paths are some
> of the offenders here, as there is actually nothing preventing us from
> running a link address probe while at the same time handling a
> connection status update request - something that's likely always been
> possible but never seen in the wild because hotplugging has been broken
> for ages now (with the exception of amdgpu, for reasons I don't think
> are worth digging into very far).
>
> Note: I'm going to start using the term "in-memory topology layout" here
> to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
>
> Locking in these places is a little tougher then it looks though.
> Generally we protect anything having to do with the in-memory topology
> layout under >lock. But this becomes nearly impossible to do from
> the context of link address probes due to the fact that >lock is
> usually grabbed under random various modesetting locks, meaning that
> there's no way we can just invert the >lock order and keep it
> locked throughout the whole process of updating the topology.
>
> Luckily there are only two workers which can modify the in-memory
> topology layout: drm_dp_mst_up_req_work() and
> drm_dp_mst_link_probe_work(), meaning as long as we prevent these two
> workers from traveling the topology layout in parallel with the intent
> of updating it we don't need to worry about grabbing >lock in these
> workers for reads. We only need to grab >lock in these workers for
> writes, so that readers outside these two workers are still protected
> from the topology layout changing beneath them.
>
> So, add the new >probe_lock and use it in both
> drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally,
> add some more detailed explanations for how this locking is intended to
> work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
This seems like a good solution to me, thanks for working this through!
Any chance we could add a WARN_ON(!mutex_is_locked(>probe_lock)); somewhere
centrally called by all paths modifying the in-memory topology layout?
drm_dp_add_port perhaps? That way we have a fallback in case something else
starts mucking with the topology.
Other than that,
Reviewed-by: Sean Paul
>
> Signed-off-by: Lyude Paul
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Cc: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++-
> include/drm/drm_dp_mst_helper.h | 32 +++
> 2 files changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 08c316a727df..11d842f0bff5 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct
> drm_dp_mst_topology_mgr *m
> struct drm_dp_mst_branch *mstb)
> {
> struct drm_dp_mst_port *port;
> - struct drm_dp_mst_branch *mstb_child;
> +
> if (!mstb->link_address_sent)
> drm_dp_send_link_address(mgr, mstb);
>
> list_for_each_entry(port, >ports, next) {
> - if (port->input)
> - continue;
> + struct drm_dp_mst_branch *mstb_child = NULL;
>
> - if (!port->ddps)
> + if (port->input || !port->ddps)
> continue;
>
> if (!port->available_pbn)
> drm_dp_send_enum_path_resources(mgr, mstb, port);
>
> - if (port->mstb) {
> + if (port->mstb)
> mstb_child = drm_dp_mst_topology_get_mstb_validated(
> mgr, port->mstb);
> - if (mstb_child) {
> - drm_dp_check_and_send_link_address(mgr,
> mstb_child);
> - drm_dp_mst_topology_put_mstb(mstb_child);
> - }
> +
> + if (mstb_child) {
> + drm_dp_check_and_send_link_address(mgr, mstb_child);
> + drm_dp_mst_topology_put_mstb(mstb_child);
> }
> }
> }
>
> static void drm_dp_mst_link_probe_work(struct work_struct *work)
> {
> - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct
> drm_dp_mst_topology_mgr, work);
> + struct drm_dp_mst_topology_mgr *mgr =
> +
Re: [PATCH 5/6] drm/amdgpu/dm/mst: Report possible_crtcs incorrectly, for now
On Thu, Sep 26, 2019 at 06:51:07PM -0400, Lyude Paul wrote: > This commit is seperate from the previous one to make it easier to > revert in the future. Basically, there's multiple userspace applications > that interpret possible_crtcs very wrong: > > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277 > https://gitlab.gnome.org/GNOME/mutter/issues/759 > > While work is ongoing to fix these issues in userspace, we need to > report ->possible_crtcs incorrectly for now in order to avoid > introducing a regression in in userspace. Once these issues get fixed, > this commit should be reverted. > > Signed-off-by: Lyude Paul > Cc: Ville Syrjälä > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index b404f1ae6df7..fe8ac801d7a5 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -4807,6 +4807,17 @@ static int amdgpu_dm_crtc_init(struct > amdgpu_display_manager *dm, > if (!acrtc->mst_encoder) > goto fail; > > + /* > + * FIXME: This is a hack to workaround the following issues: > + * > + * https://gitlab.gnome.org/GNOME/mutter/issues/759 > + * https://gitlab.freedesktop.org/xorg/xserver/merge_requests/277 > + * > + * One these issues are closed, this should be removed Even when these issues are closed, we'll still be introducing a regression if we revert this change. Time for actually_possible_crtcs? :) You also might want to briefly explain the u/s bug in case the links go sour. > + */ > + acrtc->mst_encoder->base.possible_crtcs = > + amdgpu_dm_get_encoder_crtc_mask(dm->adev); Why don't we put this hack in amdgpu_dm_dp_create_fake_mst_encoder()? Sean > + > return 0; > > fail: > -- > 2.21.0 > -- Sean Paul, Software Engineer, Google / Chromium OS ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 27/27] drm/dp_mst: Add topology ref history tracking for debugging
On Tue, Sep 03, 2019 at 04:46:05PM -0400, Lyude Paul wrote:
> For very subtle mistakes with topology refs, it can be rather difficult
> to trace them down with the debugging info that we already have. I had
> one such issue recently while trying to implement suspend/resume
> reprobing for MST, and ended up coming up with this.
>
> Inspired by Chris Wilson's wakeref tracking for i915, this adds a very
> similar feature to the DP MST helpers, which allows for partial tracking
> of topology refs for both ports and branch devices. This is a lot less
> advanced then wakeref tracking: we merely keep a count of all of the
> spots where a topology ref has been grabbed or dropped, then dump out
> that history in chronological order when a port or branch device's
> topology refcount reaches 0. So far, I've found this incredibly useful
> for debugging topology refcount errors.
>
> Since this has the potential to be somewhat slow and loud, we add an
> expert kernel config option to enable or disable this feature,
> CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS.
>
Looks very useful indeed!
My only nit is that we could probably grow the list a little more aggressively
(or start it off at some size > 1) and avoid a bunch of reallocs. That said,
I'm not sure how often it's reallocated so it might not be an issue. Either
way,
Reviewed-by: Sean Paul
> Changes since v1:
> * Don't forget to destroy topology_ref_history_lock
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> ---
> drivers/gpu/drm/Kconfig | 14 ++
> drivers/gpu/drm/drm_dp_mst_topology.c | 233 +-
> include/drm/drm_dp_mst_helper.h | 45 +
> 3 files changed, 288 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e67c194c2aca..44fc2c2a6e2c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -93,6 +93,20 @@ config DRM_KMS_FB_HELPER
> help
> FBDEV helpers for KMS drivers.
>
> +config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
> +bool "Enable refcount backtrace history in the DP MST helpers"
> +select STACKDEPOT
> +depends on DRM_KMS_HELPER
> +depends on DEBUG_KERNEL
> +depends on EXPERT
> +help
> + Enables debug tracing for topology refs in DRM's DP MST helpers. A
> + history of each topology reference/dereference will be printed to
> the
> + kernel log once a port or branch device's topology refcount
> reaches 0.
> +
> + This has the potential to use a lot of memory and print some very
> + large kernel messages. If in doubt, say "N".
> +
> config DRM_FBDEV_EMULATION
> bool "Enable legacy fbdev support for your modesetting driver"
> depends on DRM
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5b5c0b3b3c0e..18f9a02927d9 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -28,6 +28,13 @@
> #include
> #include
>
> +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> +#include
> +#include
> +#include
> +#include
> +#endif
> +
> #include
> #include
> #include
> @@ -1405,12 +1412,189 @@ drm_dp_mst_put_port_malloc(struct drm_dp_mst_port
> *port)
> }
> EXPORT_SYMBOL(drm_dp_mst_put_port_malloc);
>
> +#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
> +
> +#define STACK_DEPTH 8
> +
> +static noinline void
> +__topology_ref_save(struct drm_dp_mst_topology_mgr *mgr,
> + struct drm_dp_mst_topology_ref_history *history,
> + enum drm_dp_mst_topology_ref_type type)
> +{
> + struct drm_dp_mst_topology_ref_entry *entry = NULL;
> + depot_stack_handle_t backtrace;
> + ulong stack_entries[STACK_DEPTH];
> + uint n;
> + int i;
> +
> + n = stack_trace_save(stack_entries, ARRAY_SIZE(stack_entries), 1);
> + backtrace = stack_depot_save(stack_entries, n, GFP_KERNEL);
> + if (!backtrace)
> + goto fail_alloc;
> +
> + /* Try to find an existing entry for this backtrace */
> + for (i = 0; i < history->len; i++) {
> + if (history->entries[i].backtrace == backtrace) {
> + entry = >entries[i];
> + break;
> + }
> + }
> +
> + /* Otherwise add one */
> + if (!entry) {
> + struct drm_dp_mst_topology_ref_entry *new;
> + int new_len = history->len + 1;
> +
> +
Re: [PATCH v2 26/27] drm/dp_mst: Also print unhashed pointers for malloc/topology references
gt; *port)
> int ret = kref_get_unless_zero(>topology_kref);
>
> if (ret)
> - DRM_DEBUG("port %p (%d)\n", port,
> - kref_read(>topology_kref));
> + DRM_DEBUG("port %p/%px (%d)\n",
> + port, port, kref_read(>topology_kref));
>
> return ret;
> }
> @@ -1569,7 +1574,8 @@ static void drm_dp_mst_topology_get_port(struct
> drm_dp_mst_port *port)
> {
> WARN_ON(kref_read(>topology_kref) == 0);
> kref_get(>topology_kref);
> - DRM_DEBUG("port %p (%d)\n", port, kref_read(>topology_kref));
> + DRM_DEBUG("port %p/%px (%d)\n",
> + port, port, kref_read(>topology_kref));
> }
>
> /**
> @@ -1585,8 +1591,8 @@ static void drm_dp_mst_topology_get_port(struct
> drm_dp_mst_port *port)
> */
> static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)
> {
> - DRM_DEBUG("port %p (%d)\n",
> - port, kref_read(>topology_kref) - 1);
> + DRM_DEBUG("port %p/%px (%d)\n",
> + port, port, kref_read(>topology_kref) - 1);
> kref_put(>topology_kref, drm_dp_destroy_port);
> }
>
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming
;dpcd,
> DP_RECEIVER_CAP_SIZE);
> - if (sret != DP_RECEIVER_CAP_SIZE) {
> - DRM_DEBUG_KMS("dpcd read failed - undocked during
> suspend?\n");
> - ret = -1;
> - goto out_unlock;
> - }
> + ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
> + DP_MST_EN |
> + DP_UP_REQ_EN |
> + DP_UPSTREAM_IS_SRC);
> + if (ret < 0) {
> + DRM_DEBUG_KMS("mst write failed - undocked during suspend?\n");
> + goto out_fail;
> + }
>
> - ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
> - DP_MST_EN | DP_UP_REQ_EN |
> DP_UPSTREAM_IS_SRC);
> - if (ret < 0) {
> - DRM_DEBUG_KMS("mst write failed - undocked during
> suspend?\n");
> - ret = -1;
> - goto out_unlock;
> - }
> + /* Some hubs forget their guids after they resume */
> + ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
> + if (ret != 16) {
> + DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n");
> + goto out_fail;
> + }
> + drm_dp_check_mstb_guid(mgr->mst_primary, guid);
>
> - /* Some hubs forget their guids after they resume */
> - sret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
> - if (sret != 16) {
> - DRM_DEBUG_KMS("dpcd read failed - undocked during
> suspend?\n");
> - ret = -1;
> - goto out_unlock;
> - }
> - drm_dp_check_mstb_guid(mgr->mst_primary, guid);
> + /* For the final step of resuming the topology, we need to bring the
> + * state of our in-memory topology back into sync with reality. So,
> + * restart the probing process as if we're probing a new hub
> + */
> + queue_work(system_long_wq, >work);
> + mutex_unlock(>lock);
>
> - ret = 0;
> - } else
> - ret = -1;
> + if (sync) {
> + DRM_DEBUG_KMS("Waiting for link probe work to finish re-syncing
> topology...\n");
> + flush_work(>work);
> + }
It took me longer than I'd like to admit to realize that most of the diff in
this function is just removing the indent. Would you mind splitting that out
into a separate patch so the reprobe change is more obvious?
With these nits fixed,
Reviewed-by: Sean Paul
>
> -out_unlock:
> + return 0;
> +
> +out_fail:
> mutex_unlock(>lock);
> - return ret;
> + return -1;
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5673ed75e428..b78364dcdef9 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7400,7 +7400,8 @@ void intel_dp_mst_resume(struct drm_i915_private
> *dev_priv)
> if (!intel_dp->can_mst)
> continue;
>
> - ret = drm_dp_mst_topology_mgr_resume(_dp->mst_mgr);
> + ret = drm_dp_mst_topology_mgr_resume(_dp->mst_mgr,
> + true);
> if (ret) {
> intel_dp->is_mst = false;
> drm_dp_mst_topology_mgr_set_mst(_dp->mst_mgr,
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 307584107d77..e459e2a79d78 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1309,14 +1309,14 @@ nv50_mstm_fini(struct nv50_mstm *mstm)
> }
>
> static void
> -nv50_mstm_init(struct nv50_mstm *mstm)
> +nv50_mstm_init(struct nv50_mstm *mstm, bool runtime)
> {
> int ret;
>
> if (!mstm || !mstm->mgr.mst_state)
> return;
>
> - ret = drm_dp_mst_topology_mgr_resume(>mgr);
> + ret = drm_dp_mst_topology_mgr_resume(>mgr, !runtime);
> if (ret == -1) {
> drm_dp_mst_topology_mgr_set_mst(>mgr, false);
> drm_kms_helper_hotplug_event(mstm->mgr.dev);
> @@ -2262,7 +2262,7 @@ nv50_display_init(struct drm_device *dev, bool resume,
> bool runtime)
> if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) {
> struct nouveau_encoder *nv_encoder =
> nouveau_encoder(encoder);
> - nv50_mstm_init(nv_encoder->dp.mstm);
> + nv50_mstm_init(nv_encoder->dp.mstm, runtime);
> }
> }
>
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 1efbb086f7ac..1bdee5ee6dcd 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -685,7 +685,8 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
>
> void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
> int __must_check
> -drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> +drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
> +bool sync);
>
> ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
>unsigned int offset, void *buffer, size_t size);
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously
On Wed, Sep 25, 2019 at 04:08:22PM -0400, Lyude Paul wrote:
> On Wed, 2019-09-25 at 14:16 -0400, Sean Paul wrote:
> > On Tue, Sep 03, 2019 at 04:45:41PM -0400, Lyude Paul wrote:
> > > When reprobing an MST topology during resume, we have to account for the
> > > fact that while we were suspended it's possible that mstbs may have been
> > > removed from any ports in the topology. Since iterating downwards in the
> > > topology requires that we hold >lock, destroying MSTBs from this
> > > context would result in attempting to lock >lock a second time and
> > > deadlocking.
> > >
> > > So, fix this by first moving destruction of MSTBs into
> > > destroy_connector_work, then rename destroy_connector_work and friends
> > > to reflect that they now destroy both ports and mstbs.
> > >
> > > Changes since v1:
> > > * s/destroy_connector_list/destroy_port_list/
> > > s/connector_destroy_lock/delayed_destroy_lock/
> > > s/connector_destroy_work/delayed_destroy_work/
> > > s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/
> > > s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
> > > - danvet
> > > * Use two loops in drm_dp_delayed_destroy_work() - danvet
> > > * Better explain why we need to do this - danvet
> > > * Use cancel_work_sync() instead of flush_work() - flush_work() doesn't
> > > account for work requeing
> > >
> > > Cc: Juston Li
> > > Cc: Imre Deak
> > > Cc: Ville Syrjälä
> > > Cc: Harry Wentland
> > > Cc: Daniel Vetter
> > > Signed-off-by: Lyude Paul
> >
> > Took me a while to grok this, and I'm still not 100% confident my mental
> > model
> > is correct, so please bear with me while I ask silly questions :)
> >
> > Now that the destroy is delayed, and the port remains in the topology, is it
> > possible we will underflow the topology kref by calling put_mstb multiple
> > times?
> > It looks like that would result in a WARN from refcount.c, and wouldn't call
> > the
> > destroy function multiple times, so that's nice :)
> >
> > Similarly, is there any defense against calling get_mstb() between destroy()
> > and
> > the delayed destroy worker running?
> >
> Good question! There's only one instance where we unconditionally grab an MSTB
> ref, drm_dp_mst_topology_mgr_set_mst(), and in that location we're guaranteed
> to be the only one with access to that mstb until we drop >lock.
> Everywhere else we use drm_dp_mst_topology_try_get_mstb(), which uses
> kref_get_unless_zero() to protect against that kind of situation (and forces
> the caller to check with __must_check)
Awesome, thanks for the breakdown!
Reviewed-by: Sean Paul
>
> > Sean
> >
> > > ---
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 162 +-
> > > include/drm/drm_dp_mst_helper.h | 26 +++--
> > > 2 files changed, 127 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 3054ec622506..738f260d4b15 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1113,34 +1113,17 @@ static void
> > > drm_dp_destroy_mst_branch_device(struct kref *kref)
> > > struct drm_dp_mst_branch *mstb =
> > > container_of(kref, struct drm_dp_mst_branch, topology_kref);
> > > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > > - struct drm_dp_mst_port *port, *tmp;
> > > - bool wake_tx = false;
> > >
> > > - mutex_lock(>lock);
> > > - list_for_each_entry_safe(port, tmp, >ports, next) {
> > > - list_del(>next);
> > > - drm_dp_mst_topology_put_port(port);
> > > - }
> > > - mutex_unlock(>lock);
> > > -
> > > - /* drop any tx slots msg */
> > > - mutex_lock(>mgr->qlock);
> > > - if (mstb->tx_slots[0]) {
> > > - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > > - mstb->tx_slots[0] = NULL;
> > > - wake_tx = true;
> > > - }
> > > - if (mstb->tx_slots[1]) {
> > > - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > > - mstb->tx_slots[1] = NULL;
> > > - wake_tx = true;
> > > - }
> > > - mutex_unlock(>mgr->qlock);
> > >
Re: [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
On Wed, Sep 25, 2019 at 05:00:00PM -0400, Lyude Paul wrote:
> On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> > On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > > Since we're going to be implementing suspend/resume reprobing very soon,
> > > we need to make sure we are extra careful to ensure that our locking
> > > actually protects the topology state where we expect it to. Turns out
> > > this isn't the case with drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt(), both of which change port->mstb without
> > > grabbing >lock.
> > >
> > > Additionally, since most callers of these functions are just using it to
> > > teardown the port's previous PDT and setup a new one we can simplify
> > > things a bit and combine drm_dp_port_setup_pdt() and
> > > drm_dp_port_teardown_pdt() into a single function:
> > > drm_dp_port_set_pdt(). This function also handles actually ensuring that
> > > we grab the correct locks when we need to modify port->mstb.
> > >
> > > Cc: Juston Li
> > > Cc: Imre Deak
> > > Cc: Ville Syrjälä
> > > Cc: Harry Wentland
> > > Cc: Daniel Vetter
> > > Signed-off-by: Lyude Paul
> > > ---
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> > > include/drm/drm_dp_mst_helper.h | 6 +-
> > > 2 files changed, 110 insertions(+), 77 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index d1610434a0cb..9944ef2ce885 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > > drm_dp_mst_branch *mstb)
> > > kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> > > }
> > >
> > > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> > > old_pdt)
> > > -{
> > > - struct drm_dp_mst_branch *mstb;
> > > -
> > > - switch (old_pdt) {
> > > - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > - case DP_PEER_DEVICE_SST_SINK:
> > > - /* remove i2c over sideband */
> > > - drm_dp_mst_unregister_i2c_bus(>aux);
> > > - break;
> > > - case DP_PEER_DEVICE_MST_BRANCHING:
> > > - mstb = port->mstb;
> > > - port->mstb = NULL;
> > > - drm_dp_mst_topology_put_mstb(mstb);
> > > - break;
> > > - }
> > > -}
> > > -
> > > static void drm_dp_destroy_port(struct kref *kref)
> > > {
> > > struct drm_dp_mst_port *port =
> > > @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct
> > > drm_dp_mst_port *port,
> > > return parent_lct + 1;
> > > }
> > >
> > > -/*
> > > - * return sends link address for new mstb
> > > - */
> > > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> > > {
> > > - int ret;
> > > - u8 rad[6], lct;
> > > - bool send_link = false;
> > > + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > > + struct drm_dp_mst_branch *mstb;
> > > + u8 rad[8], lct;
> > > + int ret = 0;
> > > +
> > > + if (port->pdt == new_pdt)
> >
> > Shouldn't we also ensure that access to port->pdt is also locked?
> >
>
> It's specifically port->mstb that needs to be protected under lock. We don't
> use port->pdt for traversing the topology at all, so keeping it under
> connection_mutex is sufficient.
>
I hadn't gotten to the connection_mutex patch yet when I made that comment :)
Reviewed-by: Sean Paul
> > Sean
> >
> > > + return 0;
> > > +
> > > + /* Teardown the old pdt, if there is one */
> > > + switch (port->pdt) {
> > > + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > > + case DP_PEER_DEVICE_SST_SINK:
> > > + /*
> > > + * If the new PDT would also have an i2c bus, don't bother
> > > + * with reregistering it
> > > + */
> > > + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > > + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > > + port->pdt = new_pdt;
> > > + return 0;
> > > +
Re: [PATCH v2 24/27] drm/amdgpu/dm: Resume short HPD IRQs before resuming MST topology
On Tue, Sep 03, 2019 at 04:46:02PM -0400, Lyude Paul wrote:
> Since we're going to be reprobing the entire topology state on resume
> now using sideband transactions, we need to ensure that we actually have
> short HPD irqs enabled before calling drm_dp_mst_topology_mgr_resume().
> So, do that.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 73630e2940d4..4d3c8bff77da 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1185,15 +1185,15 @@ static int dm_resume(void *handle)
> /* program HPD filter */
> dc_resume(dm->dc);
>
> - /* On resume we need to rewrite the MSTM control bits to enamble MST*/
> - s3_handle_mst(ddev, false);
> -
> /*
>* early enable HPD Rx IRQ, should be done before set mode as short
>* pulse interrupts are used for MST
>*/
> amdgpu_dm_irq_resume_early(adev);
>
> + /* On resume we need to rewrite the MSTM control bits to enamble MST*/
While we're here,
s/ / / && s/enamble/enable/ && s_*/_ */_
> + s3_handle_mst(ddev, false);
> +
> /* Do detection*/
> drm_connector_list_iter_begin(ddev, );
> drm_for_each_connector_iter(connector, ) {
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v2 22/27] drm/nouveau: Don't grab runtime PM refs for HPD IRQs
On Tue, Sep 03, 2019 at 04:46:00PM -0400, Lyude Paul wrote:
> In order for suspend/resume reprobing to work, we need to be able to
> perform sideband communications during suspend/resume, along with
> runtime PM suspend/resume. In order to do so, we also need to make sure
> that nouveau doesn't bother grabbing a runtime PM reference to do so,
> since otherwise we'll start deadlocking runtime PM again.
>
> Note that we weren't able to do this before, because of the DP MST
> helpers processing UP requests from topologies in the same context as
> drm_dp_mst_hpd_irq() which would have caused us to open ourselves up to
> receiving hotplug events and deadlocking with runtime suspend/resume.
> Now that those requests are handled asynchronously, this change should
> be completely safe.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Seems reasonable to me, but would feel better if a nouveau person confirmed
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/nouveau/nouveau_connector.c | 33 +++--
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 56871d34e3fb..f276918d3f3b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1131,6 +1131,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
> const char *name = connector->name;
> struct nouveau_encoder *nv_encoder;
> int ret;
> + bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
> +
> + if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
> + NV_DEBUG(drm, "service %s\n", name);
> + drm_dp_cec_irq(_connector->aux);
> + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
> + nv50_mstm_service(nv_encoder->dp.mstm);
> +
> + return NVIF_NOTIFY_KEEP;
> + }
>
> ret = pm_runtime_get(drm->dev->dev);
> if (ret == 0) {
> @@ -1151,25 +1161,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
> return NVIF_NOTIFY_DROP;
> }
>
> - if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
> - NV_DEBUG(drm, "service %s\n", name);
> - drm_dp_cec_irq(_connector->aux);
> - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
> - nv50_mstm_service(nv_encoder->dp.mstm);
> - } else {
> - bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
> -
> + if (!plugged)
> + drm_dp_cec_unset_edid(_connector->aux);
> + NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
> + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
> if (!plugged)
> - drm_dp_cec_unset_edid(_connector->aux);
> - NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
> - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
> - if (!plugged)
> - nv50_mstm_remove(nv_encoder->dp.mstm);
> - }
> -
> - drm_helper_hpd_irq_event(connector->dev);
> + nv50_mstm_remove(nv_encoder->dp.mstm);
> }
>
> + drm_helper_hpd_irq_event(connector->dev);
> +
> pm_runtime_mark_last_busy(drm->dev->dev);
> pm_runtime_put_autosuspend(drm->dev->dev);
> return NVIF_NOTIFY_KEEP;
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 21/27] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()
On Tue, Sep 03, 2019 at 04:45:59PM -0400, Lyude Paul wrote:
> This probably hasn't caused any problems up until now since it's
> probably nearly impossible to encounter this in the wild, however if we
> were to receive a connection status notification from the MST hub after
> resume while we're in the middle of reprobing the link addresses for a
> topology then there's a much larger chance that a port could have
> changed from being an output port to input port (or vice versa). If we
> forget to update this bit of information, we'll potentially ignore a
> valid PDT change on a downstream port because we think it's an input
> port.
>
> So, make sure we read the input_port field in connection status
> notifications in drm_dp_mst_handle_conn_stat() to prevent this from
> happening once we've implemented suspend/resume reprobing.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Nice catch! Same comment here re: port->mutex, but we can sort that out on the
other thread
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 259634c5d6dc..e407aba1fbd2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2078,18 +2078,23 @@ static void
> drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> struct drm_dp_connection_status_notify *conn_stat)
> {
> - struct drm_device *dev = mstb->mgr->dev;
> + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> + struct drm_device *dev = mgr->dev;
> struct drm_dp_mst_port *port;
> - int old_ddps;
> - bool dowork = false;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps, ret;
> + u8 new_pdt;
> + bool dowork = false, create_connector = false;
>
> port = drm_dp_get_port(mstb, conn_stat->port_number);
> if (!port)
> return;
>
> + mutex_lock(>lock);
> drm_modeset_lock(>mode_config.connection_mutex, NULL);
>
> old_ddps = port->ddps;
> + port->input = conn_stat->input_port;
> port->mcs = conn_stat->message_capability_status;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2102,23 +2107,41 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
> }
> }
>
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> - conn_stat->peer_device_type);
> - if (ret == 1) {
> - dowork = true;
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT for port %p: %d\n",
> - port, ret);
> - dowork = false;
> - }
> + new_pdt = port->input ? DP_PEER_DEVICE_NONE :
> conn_stat->peer_device_type;
> +
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> + port, ret);
> + dowork = false;
> + }
> +
> + /*
> + * We unset port->connector before dropping connection_mutex so that
> + * there's no chance any of the atomic MST helpers can accidentally
> + * associate a to-be-destroyed connector with a port.
> + */
> + if (port->connector && port->input) {
> + connector_to_destroy = port->connector;
> + port->connector = NULL;
> + } else if (!port->connector && !port->input) {
> + create_connector = true;
> }
>
> drm_modeset_unlock(>mode_config.connection_mutex);
> +
> + if (connector_to_destroy)
> + mgr->cbs->destroy_connector(mgr, connector_to_destroy);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
> +
> + mutex_unlock(>lock);
> +
> drm_dp_mst_topology_put_port(port);
> if (dowork)
> queue_work(system_long_wq, >mgr->work);
> -
> }
>
> static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct
> drm_dp_mst_topology_mgr *mgr,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v2 18/27] drm/dp_mst: Remove lies in {up,down}_rep_recv documentation
On Tue, Sep 03, 2019 at 04:45:56PM -0400, Lyude Paul wrote:
> These are most certainly accessed from far more than the mgr work. In
> fact, up_req_recv is -only- ever accessed from outside the mgr work.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> include/drm/drm_dp_mst_helper.h | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index f253ee43e9d9..8ba2a01324bb 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -489,15 +489,11 @@ struct drm_dp_mst_topology_mgr {
> int conn_base_id;
>
> /**
> - * @down_rep_recv: Message receiver state for down replies. This and
> - * @up_req_recv are only ever access from the work item, which is
> - * serialised.
> + * @down_rep_recv: Message receiver state for down replies.
>*/
> struct drm_dp_sideband_msg_rx down_rep_recv;
> /**
> - * @up_req_recv: Message receiver state for up requests. This and
> - * @down_rep_recv are only ever access from the work item, which is
> - * serialised.
> + * @up_req_recv: Message receiver state for up requests.
>*/
> struct drm_dp_sideband_msg_rx up_req_recv;
>
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 17/27] drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port
On Tue, Sep 03, 2019 at 04:45:55PM -0400, Lyude Paul wrote:
> The names for these functions are rather confusing. drm_dp_add_port()
> sounds like a function that would simply create a port and add it to a
> topology, and do nothing more. Similarly, drm_dp_update_port() would be
> assumed to be the function that should be used to update port
> information after initial creation.
>
> While those assumptions are currently correct in how these functions are
> used, a quick glance at drm_dp_add_port() reveals that drm_dp_add_port()
> can also update the information on a port, and seems explicitly designed
> to do so. This can be explained pretty simply by the fact that there's
> more situations that would involve updating the port information based
> on a link address response as opposed to a connection status
> notification than the driver's initial topology probe. Case in point:
> reprobing link addresses after suspend/resume.
>
> Since we're about to start using drm_dp_add_port() differently for
> suspend/resume reprobing, let's rename both functions to clarify what
> they actually do.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 9944ef2ce885..cfaf9eb7ace9 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1900,9 +1900,10 @@ void drm_dp_mst_connector_early_unregister(struct
> drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>
> -static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
> - struct drm_device *dev,
> - struct drm_dp_link_addr_reply_port *port_msg)
> +static void
> +drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> + struct drm_device *dev,
> + struct drm_dp_link_addr_reply_port
> *port_msg)
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> @@ -2011,8 +2012,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
> drm_dp_mst_topology_put_port(port);
> }
>
> -static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
> -struct drm_dp_connection_status_notify
> *conn_stat)
> +static void
> +drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_connection_status_notify *conn_stat)
> {
> struct drm_dp_mst_port *port;
> int old_ddps;
> @@ -2464,7 +2466,8 @@ static void drm_dp_send_link_address(struct
> drm_dp_mst_topology_mgr *mgr,
> drm_dp_check_mstb_guid(mstb, reply->guid);
>
> for (i = 0; i < reply->nports; i++)
> - drm_dp_add_port(mstb, mgr->dev, >ports[i]);
> + drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
> + >ports[i]);
>
> drm_kms_helper_hotplug_event(mgr->dev);
>
> @@ -3324,7 +3327,7 @@ static int drm_dp_mst_handle_up_req(struct
> drm_dp_mst_topology_mgr *mgr)
> }
>
> if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
> - drm_dp_update_port(mstb, _stat);
> + drm_dp_mst_handle_conn_stat(mstb, _stat);
>
> DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d
> pdt: %d\n",
> msg.u.conn_stat.port_number,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
gt;connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
> -port,
> -proppath);
> - if (!port->connector) {
> - /* remove it from the port list */
> - mutex_lock(>mgr->lock);
> - list_del(>next);
> - mutex_unlock(>mgr->lock);
> - /* drop port list reference */
> - drm_dp_mst_topology_put_port(port);
> - goto out;
> - }
> + port->connector = (*mgr->cbs->add_connector)(mgr, port,
> + proppath);
> + if (!port->connector)
> + goto fail;
> +
> if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
>port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> port->port_num >= DP_MST_LOGICAL_PORT_0) {
> @@ -1974,28 +1991,38 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>>aux.ddc);
> drm_connector_set_tile_property(port->connector);
> }
> - (*mstb->mgr->cbs->register_connector)(port->connector);
> +
> + (*mgr->cbs->register_connector)(port->connector);
> }
>
> -out:
> /* put reference to this port */
> drm_dp_mst_topology_put_port(port);
> + return;
> +
> +fail:
> + /* Remove it from the port list */
> + mutex_lock(>lock);
> + list_del(>next);
> + mutex_unlock(>lock);
> +
> + /* Drop the port list reference */
> + drm_dp_mst_topology_put_port(port);
> + /* And now drop our reference */
> + drm_dp_mst_topology_put_port(port);
> }
>
> static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
> struct drm_dp_connection_status_notify
> *conn_stat)
> {
> struct drm_dp_mst_port *port;
> - int old_pdt;
> int old_ddps;
> bool dowork = false;
> +
> port = drm_dp_get_port(mstb, conn_stat->port_number);
> if (!port)
> return;
>
> old_ddps = port->ddps;
> - old_pdt = port->pdt;
> - port->pdt = conn_stat->peer_device_type;
> port->mcs = conn_stat->message_capability_status;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2007,11 +2034,17 @@ static void drm_dp_update_port(struct
> drm_dp_mst_branch *mstb,
> port->available_pbn = 0;
> }
> }
> - if (old_pdt != port->pdt && !port->input) {
> - drm_dp_port_teardown_pdt(port, old_pdt);
>
> - if (drm_dp_port_setup_pdt(port))
> + if (!port->input) {
> + int ret = drm_dp_port_set_pdt(port,
> + conn_stat->peer_device_type);
> + if (ret == 1) {
> dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> + port, ret);
> + dowork = false;
> + }
> }
>
> drm_dp_mst_topology_put_port(port);
> @@ -4003,9 +4036,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port
> *port)
> if (port->connector)
> port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>
> - drm_dp_port_teardown_pdt(port, port->pdt);
> - port->pdt = DP_PEER_DEVICE_NONE;
> -
> + drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
> drm_dp_mst_put_port_malloc(port);
> }
>
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 5423a8adda78..f253ee43e9d9 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -55,8 +55,10 @@ struct drm_dp_vcpi {
> * @num_sdp_stream_sinks: Number of stream sinks
> * @available_pbn: Available bandwidth for this port.
> * @next: link to next port on this branch device
> - * @mstb: branch device attach below this port
> - * @aux: i2c aux transport to talk to device connected to this port.
> + * @mstb: branch device on this port, protected by
> + * _dp_mst_topology_mgr.lock
> + * @aux: i2c aux transport to talk to device connected to this port,
> protected
> + * by _dp_mst_topology_mgr.lock
> * @parent: branch device parent of this port
> * @vcpi: Virtual Channel Payload info for this port.
> * @connector: DRM connector this port is connected to.
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 14/27] drm/dp_mst: Destroy topology_mgr mutexes
On Tue, Sep 03, 2019 at 04:45:52PM -0400, Lyude Paul wrote: > Turns out we've been forgetting for a while now to actually destroy any > of the mutexes that we create in drm_dp_mst_topology_mgr. So, let's do > that. > > Cc: Juston Li > Cc: Imre Deak > Cc: Ville Syrjälä > Cc: Harry Wentland > Cc: Daniel Vetter > Signed-off-by: Lyude Paul Cleanup is overrated :) Reviewed-by: Sean Paul > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 74161f442584..2f88cc173500 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4339,6 +4339,11 @@ void drm_dp_mst_topology_mgr_destroy(struct > drm_dp_mst_topology_mgr *mgr) > mgr->aux = NULL; > drm_atomic_private_obj_fini(>base); > mgr->funcs = NULL; > + > + mutex_destroy(>delayed_destroy_lock); > + mutex_destroy(>payload_lock); > + mutex_destroy(>qlock); > + mutex_destroy(>lock); > } > EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); > > -- > 2.21.0 > -- Sean Paul, Software Engineer, Google / Chromium OS ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 08/27] drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor
On Tue, Sep 03, 2019 at 04:45:46PM -0400, Lyude Paul wrote:
> This will allow us to add some locking for port->* members, in
> particular the PDT and ->connector, which can't be done from
> drm_dp_destroy_port() since we don't know what locks the caller might be
> holding.
Might be nice to mention that this is already done in the delayed destroy worker
so readers don't need to go looking for it. Perhaps update this when you apply
the patch.
>
> Changes since v2:
> * Clarify commit message
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 40 +++
> 1 file changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index f5f1d8b50fb6..af3189df28aa 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1511,31 +1511,22 @@ static void drm_dp_destroy_port(struct kref *kref)
> container_of(kref, struct drm_dp_mst_port, topology_kref);
> struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>
> - if (!port->input) {
> - kfree(port->cached_edid);
> + /* There's nothing that needs locking to destroy an input port yet */
> + if (port->input) {
> + drm_dp_mst_put_port_malloc(port);
> + return;
> + }
>
> - /*
> - * The only time we don't have a connector
> - * on an output port is if the connector init
> - * fails.
> - */
> - if (port->connector) {
> - /* we can't destroy the connector here, as
> - * we might be holding the mode_config.mutex
> - * from an EDID retrieval */
> + kfree(port->cached_edid);
>
> - mutex_lock(>delayed_destroy_lock);
> - list_add(>next, >destroy_port_list);
> - mutex_unlock(>delayed_destroy_lock);
> - schedule_work(>delayed_destroy_work);
> - return;
> - }
> - /* no need to clean up vcpi
> - * as if we have no connector we never setup a vcpi */
> - drm_dp_port_teardown_pdt(port, port->pdt);
> - port->pdt = DP_PEER_DEVICE_NONE;
> - }
> - drm_dp_mst_put_port_malloc(port);
> + /*
> + * we can't destroy the connector here, as we might be holding the
> + * mode_config.mutex from an EDID retrieval
> + */
> + mutex_lock(>delayed_destroy_lock);
> + list_add(>next, >destroy_port_list);
> + mutex_unlock(>delayed_destroy_lock);
> + schedule_work(>delayed_destroy_work);
> }
>
> /**
> @@ -3998,7 +3989,8 @@ static void drm_dp_tx_work(struct work_struct *work)
> static inline void
> drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
> {
> - port->mgr->cbs->destroy_connector(port->mgr, port->connector);
> + if (port->connector)
> + port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>
> drm_dp_port_teardown_pdt(port, port->pdt);
> port->pdt = DP_PEER_DEVICE_NONE;
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v2 04/27] drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest
On Tue, Sep 03, 2019 at 04:45:42PM -0400, Lyude Paul wrote:
> Yes, apparently we've been testing this for every single driver load for
> quite a long time now. At least that means our PBN calculation is solid!
>
> Anyway, introduce self tests for MST and move this into there.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Reviewed-by: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 27 ---
> drivers/gpu/drm/selftests/Makefile| 2 +-
> .../gpu/drm/selftests/drm_modeset_selftests.h | 1 +
> .../drm/selftests/test-drm_dp_mst_helper.c| 34 +++
> .../drm/selftests/test-drm_modeset_common.h | 1 +
> 5 files changed, 37 insertions(+), 28 deletions(-)
> create mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 738f260d4b15..6f7f449ca12b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -47,7 +47,6 @@
> */
> static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
> char *buf);
> -static int test_calc_pbn_mode(void);
>
> static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
>
> @@ -3561,30 +3560,6 @@ int drm_dp_calc_pbn_mode(int clock, int bpp)
> }
> EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>
> -static int test_calc_pbn_mode(void)
> -{
> - int ret;
> - ret = drm_dp_calc_pbn_mode(154000, 30);
> - if (ret != 689) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> - 154000, 30, 689, ret);
> - return -EINVAL;
> - }
> - ret = drm_dp_calc_pbn_mode(234000, 30);
> - if (ret != 1047) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> - 234000, 30, 1047, ret);
> - return -EINVAL;
> - }
> - ret = drm_dp_calc_pbn_mode(297000, 24);
> - if (ret != 1063) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> - 297000, 24, 1063, ret);
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
> /* we want to kick the TX after we've ack the up/down IRQs. */
> static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
> {
> @@ -4033,8 +4008,6 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> if (!mgr->proposed_vcpis)
> return -ENOMEM;
> set_bit(0, >payload_mask);
> - if (test_calc_pbn_mode() < 0)
> - DRM_ERROR("MST PBN self-test failed\n");
>
> mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> if (mst_state == NULL)
> diff --git a/drivers/gpu/drm/selftests/Makefile
> b/drivers/gpu/drm/selftests/Makefile
> index aae88f8a016c..d2137342b371 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
>test-drm_format.o test-drm_framebuffer.o \
> - test-drm_damage_helper.o
> + test-drm_damage_helper.o test-drm_dp_mst_helper.o
>
> obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
> test-drm_cmdline_parser.o
> diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> index 464753746013..dec3ee3ec96f 100644
> --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> @@ -32,3 +32,4 @@ selftest(damage_iter_damage_one_intersect,
> igt_damage_iter_damage_one_intersect)
> selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside)
> selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved)
> selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible)
> +selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode)
> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> new file mode 100644
> index ..9baa5171988d
> --- /dev/null
> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> @@ -0,0 +1,34 @@
>
Re: [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously
; + if (port)
> + list_del(>next);
> + mutex_unlock(>delayed_destroy_lock);
> +
> + if (!port)
> + break;
> +
> + drm_dp_delayed_destroy_port(port);
> + send_hotplug = true;
> + go_again = true;
> + }
> + } while (go_again);
>
> - drm_dp_mst_put_port_malloc(port);
> - send_hotplug = true;
> - }
> if (send_hotplug)
> drm_kms_helper_hotplug_event(mgr->dev);
> }
> @@ -3957,12 +4010,13 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> mutex_init(>lock);
> mutex_init(>qlock);
> mutex_init(>payload_lock);
> - mutex_init(>destroy_connector_lock);
> + mutex_init(>delayed_destroy_lock);
> INIT_LIST_HEAD(>tx_msg_downq);
> - INIT_LIST_HEAD(>destroy_connector_list);
> + INIT_LIST_HEAD(>destroy_port_list);
> + INIT_LIST_HEAD(>destroy_branch_device_list);
> INIT_WORK(>work, drm_dp_mst_link_probe_work);
> INIT_WORK(>tx_work, drm_dp_tx_work);
> - INIT_WORK(>destroy_connector_work, drm_dp_destroy_connector_work);
> + INIT_WORK(>delayed_destroy_work, drm_dp_delayed_destroy_work);
> init_waitqueue_head(>tx_waitq);
> mgr->dev = dev;
> mgr->aux = aux;
> @@ -4005,7 +4059,7 @@ void drm_dp_mst_topology_mgr_destroy(struct
> drm_dp_mst_topology_mgr *mgr)
> {
> drm_dp_mst_topology_mgr_set_mst(mgr, false);
> flush_work(>work);
> - flush_work(>destroy_connector_work);
> + cancel_work_sync(>delayed_destroy_work);
> mutex_lock(>payload_lock);
> kfree(mgr->payloads);
> mgr->payloads = NULL;
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index fc349204a71b..4a4507fe928d 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -143,6 +143,12 @@ struct drm_dp_mst_branch {
>*/
> struct kref malloc_kref;
>
> + /**
> + * @destroy_next: linked-list entry used by
> + * drm_dp_delayed_destroy_work()
> + */
> + struct list_head destroy_next;
> +
> u8 rad[8];
> u8 lct;
> int num_ports;
> @@ -575,18 +581,24 @@ struct drm_dp_mst_topology_mgr {
> struct work_struct tx_work;
>
> /**
> - * @destroy_connector_list: List of to be destroyed connectors.
> + * @destroy_port_list: List of to be destroyed connectors.
> + */
> + struct list_head destroy_port_list;
> + /**
> + * @destroy_branch_device_list: List of to be destroyed branch
> + * devices.
>*/
> - struct list_head destroy_connector_list;
> + struct list_head destroy_branch_device_list;
> /**
> - * @destroy_connector_lock: Protects @connector_list.
> + * @delayed_destroy_lock: Protects @destroy_port_list and
> + * @destroy_branch_device_list.
>*/
> - struct mutex destroy_connector_lock;
> + struct mutex delayed_destroy_lock;
> /**
> - * @destroy_connector_work: Work item to destroy connectors. Needed to
> - * avoid locking inversion.
> + * @delayed_destroy_work: Work item to destroy MST port and branch
> + * devices, needed to avoid locking inversion.
>*/
> - struct work_struct destroy_connector_work;
> + struct work_struct delayed_destroy_work;
> };
>
> int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 02/27] drm/dp_mst: Get rid of list clear in destroy_connector_work
On Tue, Sep 03, 2019 at 04:45:40PM -0400, Lyude Paul wrote: > This seems to be some leftover detritus from before the port/mstb kref > cleanup and doesn't do anything anymore, so get rid of it. > > Cc: Juston Li > Cc: Imre Deak > Cc: Ville Syrjälä > Cc: Harry Wentland > Reviewed-by: Daniel Vetter > Signed-off-by: Lyude Paul Reviewed-by: Sean Paul > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 36db66a0ddb1..3054ec622506 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -3760,8 +3760,6 @@ static void drm_dp_destroy_connector_work(struct > work_struct *work) > list_del(>next); > mutex_unlock(>destroy_connector_lock); > > - INIT_LIST_HEAD(>next); > - > mgr->cbs->destroy_connector(mgr, port->connector); > > drm_dp_port_teardown_pdt(port, port->pdt); > -- > 2.21.0 > -- Sean Paul, Software Engineer, Google / Chromium OS ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 01/27] drm/dp_mst: Move link address dumping into a function
On Tue, Sep 03, 2019 at 04:45:39PM -0400, Lyude Paul wrote:
> Makes things easier to read.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Reviewed-by: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 35 ++-
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..36db66a0ddb1 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2103,6 +2103,28 @@ static void drm_dp_queue_down_tx(struct
> drm_dp_mst_topology_mgr *mgr,
> mutex_unlock(>qlock);
> }
>
> +static void
> +drm_dp_dump_link_address(struct drm_dp_link_address_ack_reply *reply)
> +{
> + struct drm_dp_link_addr_reply_port *port_reply;
> + int i;
> +
> + for (i = 0; i < reply->nports; i++) {
> + port_reply = >ports[i];
> + DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn: %d, dpcd_rev:
> %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n",
> + i,
> + port_reply->input_port,
> + port_reply->peer_device_type,
> + port_reply->port_number,
> + port_reply->dpcd_revision,
> + port_reply->mcs,
> + port_reply->ddps,
> + port_reply->legacy_device_plug_status,
> + port_reply->num_sdp_streams,
> + port_reply->num_sdp_stream_sinks);
> + }
> +}
> +
> static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>struct drm_dp_mst_branch *mstb)
> {
> @@ -2128,18 +2150,7 @@ static void drm_dp_send_link_address(struct
> drm_dp_mst_topology_mgr *mgr,
> DRM_DEBUG_KMS("link address nak received\n");
> } else {
> DRM_DEBUG_KMS("link address reply: %d\n",
> txmsg->reply.u.link_addr.nports);
> - for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) {
> - DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn:
> %d, dpcd_rev: %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n", i,
> -
> txmsg->reply.u.link_addr.ports[i].input_port,
> -
> txmsg->reply.u.link_addr.ports[i].peer_device_type,
> -
> txmsg->reply.u.link_addr.ports[i].port_number,
> -
> txmsg->reply.u.link_addr.ports[i].dpcd_revision,
> -txmsg->reply.u.link_addr.ports[i].mcs,
> -txmsg->reply.u.link_addr.ports[i].ddps,
> -
> txmsg->reply.u.link_addr.ports[i].legacy_device_plug_status,
> -
> txmsg->reply.u.link_addr.ports[i].num_sdp_streams,
> -
> txmsg->reply.u.link_addr.ports[i].num_sdp_stream_sinks);
> - }
> + drm_dp_dump_link_address(>reply.u.link_addr);
>
> drm_dp_check_mstb_guid(mstb,
> txmsg->reply.u.link_addr.guid);
>
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: Fix connector atomic_check compilation fail
From: Sean Paul
I missed amdgpu in my connnector_helper_funcs->atomic_check conversion,
which is understandably causing compilation failures.
Fixes: 6f3b62781bbd ("drm: Convert connector_helper_funcs->atomic_check to
accept drm_atomic_state")
Cc: Daniel Vetter
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Ben Skeggs
Cc: Laurent Pinchart
Cc: Kieran Bingham
Cc: Eric Anholt
Cc: Laurent Pinchart [for rcar lvds]
Cc: Sean Paul
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Sean Paul
Cc: David Airlie
Cc: Lyude Paul
Cc: Karol Herbst
Cc: Ilia Mirkin
Cc: dri-de...@lists.freedesktop.org
Cc: intel-...@lists.freedesktop.org
Cc: linux-renesas-...@vger.kernel.org
Reported-by: Chris Wilson
Signed-off-by: Sean Paul
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 3a723e553a193..3d5e828c3d284 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3953,9 +3953,10 @@ is_hdr_metadata_different(const struct
drm_connector_state *old_state,
static int
amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
-struct drm_connector_state *new_con_state)
+struct drm_atomic_state *state)
{
- struct drm_atomic_state *state = new_con_state->state;
+ struct drm_connector_state *new_con_state =
+ drm_atomic_get_new_connector_state(state, conn);
struct drm_connector_state *old_con_state =
drm_atomic_get_old_connector_state(state, conn);
struct drm_crtc *crtc = new_con_state->crtc;
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 5/5] drm: don't block fb changes for async plane updates
plane_state->fb;
> + struct drm_framebuffer *old_fb = plane->state->fb;
> +
> funcs = plane->helper_private;
> funcs->atomic_async_update(plane, plane_state);
>
> @@ -1665,11 +1661,17 @@ void drm_atomic_helper_async_commit(struct drm_device
> *dev,
>* plane->state in-place, make sure at least common
>* properties have been properly updated.
>*/
> - WARN_ON_ONCE(plane->state->fb != plane_state->fb);
> + WARN_ON_ONCE(plane->state->fb != new_fb);
> WARN_ON_ONCE(plane->state->crtc_x != plane_state->crtc_x);
> WARN_ON_ONCE(plane->state->crtc_y != plane_state->crtc_y);
> WARN_ON_ONCE(plane->state->src_x != plane_state->src_x);
> WARN_ON_ONCE(plane->state->src_y != plane_state->src_y);
> +
> + /*
> + * Make sure the FBs have been swapped so that cleanups in the
> + * new_state performs a cleanup in the old FB.
> + */
> + WARN_ON_ONCE(plane_state->fb != old_fb);
> }
> }
> EXPORT_SYMBOL(drm_atomic_helper_async_commit);
> diff --git a/include/drm/drm_modeset_helper_vtables.h
> b/include/drm/drm_modeset_helper_vtables.h
> index cfb7be40bed7..ce582e8e8f2f 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1174,6 +1174,11 @@ struct drm_plane_helper_funcs {
>* current one with the new plane configurations in the new
>* plane_state.
>*
> + * Drivers should also swap the framebuffers between plane state
Perhaps add "current" before plane state and then after add "(_plane.state)"
so it's more clear what you're referring to here?
> + * and new_state. This is required because prepare and cleanup calls
> + * are performed on the new_state object, then to cleanup the old
> + * framebuffer, it needs to be placed inside the new_state object.
I'd change this bit to:
* This is required since cleanup for async commits is performed on
* the new state, rather than old state like for traditional commits.
* Since we want to give up the reference on the current (old) fb instead
* of our brand new one, swap them in the driver during the async commit.
> + *
>* FIXME:
>* - It only works for single plane updates
>* - Async Pageflips are not supported yet
> --
> 2.20.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Thu, Feb 07, 2019 at 04:07:33PM -0500, Sean Paul wrote:
> On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> > The only thing now that makes drm_dev_unplug() special is that it sets
> > drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> > can remove drm_dev_unplug().
> >
> > Signed-off-by: Noralf Trønnes
> > ---
> >
> > Maybe s/unplugged/unregistered/ ?
> >
> > I looked at drm_device->registered, but using that would mean that
> > drm_dev_is_unplugged() would return before drm_device is registered.
> > And given that its current purpose is to prevent race against connector
> > registration, I stayed away from it.
>
> Makes sense. I think having unregistered along with registered might cause
> some
> confusion unless it's really clearly documented. Perhaps
> s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
> meaning...
>
> I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
> better off as a follow-on series where everyone can pile on opinions. So,
>
> Reviewed-by: Sean Paul
Looks like I was dropped from Cc, so I didn't see the replies after Oleksandr's
review. So go ahead and disregard this :-)
Sean
>
>
> >
> > Noralf.
> >
> >
> > drivers/gpu/drm/drm_drv.c | 27 +++
> > include/drm/drm_drv.h | 10 --
> > 2 files changed, 19 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 05bbc2b622fc..e0941200edc6 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> > */
> > void drm_dev_unplug(struct drm_device *dev)
> > {
> > - /*
> > -* After synchronizing any critical read section is guaranteed to see
> > -* the new value of ->unplugged, and any critical section which might
> > -* still have seen the old value of ->unplugged is guaranteed to have
> > -* finished.
> > -*/
> > - dev->unplugged = true;
> > - synchronize_srcu(_unplug_srcu);
> > -
> > drm_dev_unregister(dev);
> > drm_dev_put(dev);
> > }
> > @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> > * drm_dev_register() but does not deallocate the device. The caller must
> > call
> > * drm_dev_put() to drop their final reference.
> > *
> > - * A special form of unregistering for hotpluggable devices is
> > drm_dev_unplug(),
> > - * which can be called while there are still open users of @dev.
> > + * This function can be called while there are still open users of @dev as
> > long
> > + * as the driver protects its device resources using drm_dev_enter() and
> > + * drm_dev_exit().
> > *
> > * This should be called first in the device teardown code to make sure
> > - * userspace can't access the device instance any more.
> > + * userspace can't access the device instance any more. Drivers that
> > support
> > + * device unplug will probably want to call drm_atomic_helper_shutdown()
> > first
> > + * in order to disable the hardware on regular driver module unload.
> > */
> > void drm_dev_unregister(struct drm_device *dev)
> > {
> > @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> > if (drm_core_check_feature(dev, DRIVER_LEGACY))
> > drm_lastclose(dev);
> >
> > + /*
> > +* After synchronizing any critical read section is guaranteed to see
> > +* the new value of ->unplugged, and any critical section which might
> > +* still have seen the old value of ->unplugged is guaranteed to have
> > +* finished.
> > +*/
> > + dev->unplugged = true;
> > + synchronize_srcu(_unplug_srcu);
> > +
> > dev->registered = false;
> >
> > drm_client_dev_unregister(dev);
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index ca46a45a9cce..c50696c82a42 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> > * drm_dev_is_unplugged - is a DRM device unplugged
> > * @dev: DRM device
> > *
> > - * This function can be called to check whether a hotpluggable is
> > unplugged.
> > - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> > - * unplugged, these two functions guarantee that any store before calling
> >
Re: [PATCH 6/6] drm/drv: Remove drm_dev_unplug()
On Sun, Feb 03, 2019 at 04:42:00PM +0100, Noralf Trønnes wrote:
> There are no users left.
>
> Signed-off-by: Noralf Trønnes
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_drv.c | 17 -
> include/drm/drm_drv.h | 1 -
> 2 files changed, 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e0941200edc6..87210d4a9e53 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -354,23 +354,6 @@ void drm_dev_exit(int idx)
> }
> EXPORT_SYMBOL(drm_dev_exit);
>
> -/**
> - * drm_dev_unplug - unplug a DRM device
> - * @dev: DRM device
> - *
> - * This unplugs a hotpluggable DRM device, which makes it inaccessible to
> - * userspace operations. Entry-points can use drm_dev_enter() and
> - * drm_dev_exit() to protect device resources in a race free manner. This
> - * essentially unregisters the device like drm_dev_unregister(), but can be
> - * called while there are still open users of @dev.
> - */
> -void drm_dev_unplug(struct drm_device *dev)
> -{
> - drm_dev_unregister(dev);
> - drm_dev_put(dev);
> -}
> -EXPORT_SYMBOL(drm_dev_unplug);
> -
> /*
> * DRM internal mount
> * We want to be able to allocate our own "struct address_space" to control
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index c50696c82a42..b8765a6fc092 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -730,7 +730,6 @@ void drm_dev_put(struct drm_device *dev);
> void drm_put_dev(struct drm_device *dev);
> bool drm_dev_enter(struct drm_device *dev, int *idx);
> void drm_dev_exit(int idx);
> -void drm_dev_unplug(struct drm_device *dev);
>
> /**
> * drm_dev_is_unplugged - is a DRM device unplugged
> --
> 2.20.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/6] drm/udl: Use drm_dev_unregister()
On Sun, Feb 03, 2019 at 04:41:58PM +0100, Noralf Trønnes wrote: > drm_dev_unplug() has been stripped down and is going away. Open code its > 2 remaining function calls. > > Cc: Dave Airlie > Cc: Sean Paul Reviewed-by: Sean Paul > Signed-off-by: Noralf Trønnes > --- > drivers/gpu/drm/udl/udl_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > index 22cd2d13e272..063e8e1e2641 100644 > --- a/drivers/gpu/drm/udl/udl_drv.c > +++ b/drivers/gpu/drm/udl/udl_drv.c > @@ -106,7 +106,8 @@ static void udl_usb_disconnect(struct usb_interface > *interface) > drm_kms_helper_poll_disable(dev); > udl_fbdev_unplug(dev); > udl_drop_usb(dev); > - drm_dev_unplug(dev); > + drm_dev_unregister(dev); > + drm_dev_put(dev); > } > > /* > -- > 2.20.1 > -- Sean Paul, Software Engineer, Google / Chromium OS ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()
On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> The only thing now that makes drm_dev_unplug() special is that it sets
> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> can remove drm_dev_unplug().
>
> Signed-off-by: Noralf Trønnes
> ---
>
> Maybe s/unplugged/unregistered/ ?
>
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.
Makes sense. I think having unregistered along with registered might cause some
confusion unless it's really clearly documented. Perhaps
s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
meaning...
I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
better off as a follow-on series where everyone can pile on opinions. So,
Reviewed-by: Sean Paul
>
> Noralf.
>
>
> drivers/gpu/drm/drm_drv.c | 27 +++
> include/drm/drm_drv.h | 10 --
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 05bbc2b622fc..e0941200edc6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> */
> void drm_dev_unplug(struct drm_device *dev)
> {
> - /*
> - * After synchronizing any critical read section is guaranteed to see
> - * the new value of ->unplugged, and any critical section which might
> - * still have seen the old value of ->unplugged is guaranteed to have
> - * finished.
> - */
> - dev->unplugged = true;
> - synchronize_srcu(_unplug_srcu);
> -
> drm_dev_unregister(dev);
> drm_dev_put(dev);
> }
> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> * drm_dev_register() but does not deallocate the device. The caller must
> call
> * drm_dev_put() to drop their final reference.
> *
> - * A special form of unregistering for hotpluggable devices is
> drm_dev_unplug(),
> - * which can be called while there are still open users of @dev.
> + * This function can be called while there are still open users of @dev as
> long
> + * as the driver protects its device resources using drm_dev_enter() and
> + * drm_dev_exit().
> *
> * This should be called first in the device teardown code to make sure
> - * userspace can't access the device instance any more.
> + * userspace can't access the device instance any more. Drivers that support
> + * device unplug will probably want to call drm_atomic_helper_shutdown()
> first
> + * in order to disable the hardware on regular driver module unload.
> */
> void drm_dev_unregister(struct drm_device *dev)
> {
> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> if (drm_core_check_feature(dev, DRIVER_LEGACY))
> drm_lastclose(dev);
>
> + /*
> + * After synchronizing any critical read section is guaranteed to see
> + * the new value of ->unplugged, and any critical section which might
> + * still have seen the old value of ->unplugged is guaranteed to have
> + * finished.
> + */
> + dev->unplugged = true;
> + synchronize_srcu(_unplug_srcu);
> +
> dev->registered = false;
>
> drm_client_dev_unregister(dev);
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index ca46a45a9cce..c50696c82a42 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> * drm_dev_is_unplugged - is a DRM device unplugged
> * @dev: DRM device
> *
> - * This function can be called to check whether a hotpluggable is unplugged.
> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> - * unplugged, these two functions guarantee that any store before calling
> - * drm_dev_unplug() is visible to callers of this function after it completes
> + * This function can be called to check whether @dev is unregistered. This
> can
> + * be used to detect that the underlying parent device is gone.
> *
> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> - * recommended that drivers instead use the underlying drm_dev_enter() and
> + * WARNING: This function fundamentally races against drm_dev_unregister().
> It
> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> * drm_dev_exit() function pairs.
> */
> static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> --
> 2.20.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/6] drm: Fix drm_release() and device unplug
On Sun, Feb 03, 2019 at 04:41:55PM +0100, Noralf Trønnes wrote:
> If userspace has open fd(s) when drm_dev_unplug() is run, it will result
> in drm_dev_unregister() being called twice. First in drm_dev_unplug() and
> then later in drm_release() through the call to drm_put_dev().
>
> Since userspace already holds a ref on drm_device through the drm_minor,
> it's not necessary to add extra ref counting based on no open file
> handles. Instead just drm_dev_put() unconditionally in drm_dev_unplug().
>
> We now has this:
> - Userpace holds a ref on drm_device as long as there's open fd(s)
> - The driver holds a ref on drm_device as long as it's bound to the
> struct device
>
> When both sides are done with drm_device, it is released.
>
> Signed-off-by: Noralf Trønnes
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_drv.c | 6 +-
> drivers/gpu/drm/drm_file.c | 6 ++
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..05bbc2b622fc 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -376,11 +376,7 @@ void drm_dev_unplug(struct drm_device *dev)
> synchronize_srcu(_unplug_srcu);
>
> drm_dev_unregister(dev);
> -
> - mutex_lock(_global_mutex);
> - if (dev->open_count == 0)
> - drm_dev_put(dev);
> - mutex_unlock(_global_mutex);
> + drm_dev_put(dev);
> }
> EXPORT_SYMBOL(drm_dev_unplug);
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 46f48f245eb5..3f20f598cd7c 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -479,11 +479,9 @@ int drm_release(struct inode *inode, struct file *filp)
>
> drm_file_free(file_priv);
>
> - if (!--dev->open_count) {
> + if (!--dev->open_count)
> drm_lastclose(dev);
> - if (drm_dev_is_unplugged(dev))
> - drm_put_dev(dev);
> - }
> +
> mutex_unlock(_global_mutex);
>
> drm_minor_release(minor);
> --
> 2.20.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/7] drm: Move the legacy kms disable_all helper to crtc helpers
On Mon, Dec 10, 2018 at 10:58:20AM -0500, Alex Deucher wrote:
> On Mon, Dec 10, 2018 at 5:04 AM Daniel Vetter wrote:
> >
> > It's not a core function, and the matching atomic functions are also
> > not in the core. Plus the suspend/resume helper is also already there.
> >
> > Needs a tiny bit of open-coding, but less midlayer beats that I think.
> >
> > Cc: Sam Bobroff
> > Signed-off-by: Daniel Vetter
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Sean Paul
> > Cc: David Airlie
> > Cc: Ben Skeggs
> > Cc: Alex Deucher
> > Cc: "Christian König"
> > Cc: "David (ChunMing) Zhou"
> > Cc: Rex Zhu
> > Cc: Andrey Grodzovsky
> > Cc: Huang Rui
> > Cc: Shaoyun Liu
> > Cc: Monk Liu
> > Cc: nouv...@lists.freedesktop.org
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> > drivers/gpu/drm/drm_crtc.c | 31 ---
> > drivers/gpu/drm/drm_crtc_helper.c | 35 ++
> > drivers/gpu/drm/nouveau/nouveau_display.c | 2 +-
> > drivers/gpu/drm/radeon/radeon_display.c| 2 +-
> > include/drm/drm_crtc.h | 2 --
> > include/drm/drm_crtc_helper.h | 1 +
> > 7 files changed, 39 insertions(+), 36 deletions(-)
> >
/snip
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> > b/drivers/gpu/drm/drm_crtc_helper.c
> > index a3c81850e755..23159eb494f1 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -984,3 +984,38 @@ void drm_helper_resume_force_mode(struct drm_device
> > *dev)
> > drm_modeset_unlock_all(dev);
> > }
> > EXPORT_SYMBOL(drm_helper_resume_force_mode);
> > +
> > +/**
> > + * drm_helper_force_disable_all - Forcibly turn off all enabled CRTCs
> > + * @dev: DRM device whose CRTCs to turn off
> > + *
> > + * Drivers may want to call this on unload to ensure that all displays are
> > + * unlit and the GPU is in a consistent, low power state. Takes modeset
> > locks.
> > + *
> > + * Note: This should only be used by non-atomic legacy drivers. For an
> > atomic
> > + * version look at drm_atomic_helper_shutdown().
> > + *
> > + * Returns:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_helper_force_disable_all(struct drm_device *dev)
>
> Maybe put crtc somewhere in the function name so it's clear what we
> are disabling.
FWIW, I think it's more clear this way. set_config_internal will turn off
everything attached to the crtc, so _everything_ will be disabled in this case.
Either way,
Reviewed-by: Sean Paul
Sean
> With that fixed:
> Reviewed-by: Alex Deucher
>
> > +{
> > + struct drm_crtc *crtc;
> > + int ret = 0;
> > +
> > + drm_modeset_lock_all(dev);
> > + drm_for_each_crtc(crtc, dev)
> > + if (crtc->enabled) {
> > + struct drm_mode_set set = {
> > + .crtc = crtc,
> > + };
> > +
> > + ret = drm_mode_set_config_internal();
> > + if (ret)
> > + goto out;
> > + }
> > +out:
> > + drm_modeset_unlock_all(dev);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(drm_helper_force_disable_all);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
> > b/drivers/gpu/drm/nouveau/nouveau_display.c
> > index f326ffd86766..5d273a655479 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> > @@ -453,7 +453,7 @@ nouveau_display_fini(struct drm_device *dev, bool
> > suspend, bool runtime)
> > if (drm_drv_uses_atomic_modeset(dev))
> > drm_atomic_helper_shutdown(dev);
> > else
> > - drm_crtc_force_disable_all(dev);
> > + drm_helper_force_disable_all(dev);
> > }
> >
> > /* disable flip completion events */
> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c
> > b/drivers/gpu/drm/radeon/radeon_display.c
> > index e6912eb99b42..92332226e5cf 100644
> > --- a/drivers/gpu/drm/radeon/radeon_display.c
> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
> > @@ -1643,7 +1643,7 @@ void radeon_modeset_fini(struct radeon_device *rdev)
> > if (rdev->mode_info.m
Re: [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere
On Mon, Nov 12, 2018 at 04:01:14PM +0100, Maarten Lankhorst wrote:
> We already have __drm_atomic_helper_connector_reset() and
> __drm_atomic_helper_plane_reset(), extend this to crtc as well.
>
> Most drivers already have a gpu reset hook, correct it.
> Nouveau already implemented its own __drm_atomic_helper_crtc_reset(),
> convert it to the common one.
>
> Signed-off-by: Maarten Lankhorst
> Cc: Harry Wentland
> Cc: Leo Li
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "David (ChunMing) Zhou"
> Cc: David Airlie
> Cc: Liviu Dudau
> Cc: Brian Starkey
> Cc: Mali DP Maintainers
> Cc: Boris Brezillon
> Cc: Nicolas Ferre
> Cc: Alexandre Belloni
> Cc: Ludovic Desroches
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Sean Paul
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: Philipp Zabel
> Cc: CK Hu
> Cc: Matthias Brugger
> Cc: Rob Clark
> Cc: Ben Skeggs
> Cc: Tomi Valkeinen
> Cc: Laurent Pinchart
> Cc: Kieran Bingham
> Cc: Sandy Huang
> Cc: "Heiko Stübner"
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: Eric Anholt
> Cc: VMware Graphics
> Cc: Sinclair Yeh
> Cc: Thomas Hellstrom
> Cc: Tony Cheng
> Cc: Shirish S
> Cc: Mikita Lipski
> Cc: Bhawanpreet Lakha
> Cc: David Francis
> Cc: Anthony Koo
> Cc: Jeykumar Sankaran
> Cc: Jordan Crouse
> Cc: Bruce Wang
> Cc: Sravanthi Kollukuduru
> Cc: Archit Taneja
> Cc: Steve Kowalik
> Cc: Carsten Behling
> Cc: Haneen Mohammed
> Cc: Daniel Vetter
> Cc: Rodrigo Siqueira
> Cc: Mahesh Kumar
> Cc: amd-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--
> drivers/gpu/drm/arm/malidp_crtc.c | 5 +--
> .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 5 +--
> drivers/gpu/drm/drm_atomic_state_helper.c | 31 ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/imx/ipuv3-crtc.c | 5 +--
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 5 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 ++-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 6 +---
> drivers/gpu/drm/nouveau/dispnv50/head.c | 13 ++--
> drivers/gpu/drm/omapdrm/omap_crtc.c | 7 ++---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 4 +--
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 7 +++--
> drivers/gpu/drm/tegra/dc.c| 5 +--
> drivers/gpu/drm/vc4/vc4_crtc.c| 8 ++---
> drivers/gpu/drm/vkms/vkms_crtc.c | 7 +
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 9 +-
> include/drm/drm_atomic_state_helper.h | 2 ++
> 18 files changed, 56 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5064768642f3..770a71726cd1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2802,9 +2802,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc)
> if (WARN_ON(!state))
Can you give this the same treatment as the other allocation checks?
> return;
>
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> -
> + __drm_atomic_helper_crtc_reset(crtc, >base);
> }
>
> static struct drm_crtc_state *
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
> b/drivers/gpu/drm/arm/malidp_crtc.c
> index e1b72782848c..9a924ff27148 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -474,10 +474,7 @@ static void malidp_crtc_reset(struct drm_crtc *crtc)
>
> kfree(state);
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> - }
You're changing behavior slightly here. If the allocation fails in the old code,
you just continue on (and presumably use-after-free on the next crtc->state
access). Whereas now you're going to just deref NULL. Neither one are really
desireable :)
So you probably want to continue checking the allocation and clear crtc->s
Re: [PATCH] RFC: Make igts for cross-driver stuff mandatory?
On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
> Hi all,
>
> This is just to collect feedback on this idea, and see whether the
> overall dri-devel community stands on all this. I think the past few
> cross-vendor uapi extensions all came with igts attached, and
> personally I think there's lots of value in having them: A
> cross-vendor interface isn't useful if every driver implements it
> slightly differently.
>
> I think there's 2 questions here:
>
> - Do we want to make such testcases mandatory?
>
Yes, more testing == better code.
> - If yes, are we there yet, or is there something crucially missing
> still?
In my experience, no. Last week while trying to replicate an intel-gfx CI
failure, I tried compiling igt for one of my (intel) chromebooks. It seems like
cross-compilation (or, in my case, just specifying
prefix/ld_library_path/sbin_path) is broken on igt. If we want to impose
restrictions across the entire subsystem, we need to make sure that everyone can
build and deploy igt easily.
I managed to hack around everything and get it working, but I still haven't
tried switching out the toolchain. Once we have some GitLab CI to validate
cross-compilation, then we can consider making IGT mandatory.
It's possible that I'm just a meson n00b and didn't use the right incantation,
so maybe it already works, but then we need better documentation.
I've pasted my horrible hacks below, I also didn't have libunwind, so removed
its usage.
Sean
/snip
From ab8c7d274c32559295b38d6ceeaabded14b207d4 Mon Sep 17 00:00:00 2001
From: Sean Paul
Date: Thu, 25 Oct 2018 08:40:28 -0400
Subject: [PATCH] igt: Hacks to compile in CrOS chroot
Signed-off-by: Sean Paul
---
lib/igt_core.c| 78 ---
lib/meson.build | 1 -
meson.build | 4 +-
tests/gem_userptr_blits.c | 2 +
tools/meson.build | 7
5 files changed, 5 insertions(+), 87 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 23bb858f..ca65d7cc 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -71,8 +71,6 @@
#include "igt_sysrq.h"
#include "igt_rc.h"
-#define UNW_LOCAL_ONLY
-#include
#include
#ifdef HAVE_LIBGEN_H
@@ -1209,63 +1207,6 @@ static void write_stderr(const char *str)
static void print_backtrace(void)
{
- unw_cursor_t cursor;
- unw_context_t uc;
- int stack_num = 0;
-
- Dwfl_Callbacks cbs = {
- .find_elf = dwfl_linux_proc_find_elf,
- .find_debuginfo = dwfl_standard_find_debuginfo,
- };
-
- Dwfl *dwfl = dwfl_begin();
-
- if (dwfl_linux_proc_report(dwfl, getpid())) {
- dwfl_end(dwfl);
- dwfl = NULL;
- } else
- dwfl_report_end(dwfl, NULL, NULL);
-
- igt_info("Stack trace:\n");
-
- unw_getcontext();
- unw_init_local(, );
- while (unw_step() > 0) {
- char name[255];
- unw_word_t off, ip;
- Dwfl_Module *mod = NULL;
-
- unw_get_reg(, UNW_REG_IP, );
-
- if (dwfl)
- mod = dwfl_addrmodule(dwfl, ip);
-
- if (mod) {
- const char *src, *dwfl_name;
- Dwfl_Line *line;
- int lineno;
- GElf_Sym sym;
-
- line = dwfl_module_getsrc(mod, ip);
- dwfl_name = dwfl_module_addrsym(mod, ip, , NULL);
-
- if (line && dwfl_name) {
- src = dwfl_lineinfo(line, NULL, , NULL,
NULL, NULL);
- igt_info(" #%d %s:%d %s()\n", stack_num++,
src, lineno, dwfl_name);
- continue;
- }
- }
-
- if (unw_get_proc_name(, name, 255, ) < 0)
- igt_info(" #%d [+0x%x]\n", stack_num++,
-(unsigned int) ip);
- else
- igt_info(" #%d [%s+0x%x]\n", stack_num++, name,
-(unsigned int) off);
- }
-
- if (dwfl)
- dwfl_end(dwfl);
}
static const char hex[] = "0123456789abcdef";
@@ -1420,25 +1361,6 @@ xprintf(const char *fmt, ...)
static void print_backtrace_sig_safe(void)
{
- unw_cursor_t cursor;
- unw_context_t uc;
- int stack_num = 0;
-
- write_stderr("Stack trace: \n");
-
- unw_getcontext();
- unw_init_local(, );
- while (unw_step() > 0) {
- char name[255];
- unw_word_t off;
-
- if (unw_get_proc_name(, name, 255, ) < 0)
- xstrlcpy(name, "", 10);
-
- xprintf(" #%d [%s+0x%x]\n", stack_num++, name,
-
Re: [Intel-gfx] RFC: Migration to Gitlab
On Wed, Aug 22, 2018 at 01:44:56PM +0200, Daniel Vetter wrote: > Hi all, > > I think it's time to brainstorm a bit about the gitlab migration. Basic > reasons: > > - fd.o admins want to deprecate shell accounts and hand-rolled > infrastructure, because it's a pain to keep secure > > - gitlab will allow us to add committers on our own, greatly > simplifying that process (and offloading that task from fd.o admins). > > There's also some more benefits we might want to reap, like better CI > integration for basic build testing - no more "oops didn't build > drm-misc defconfigs" or "sry, forgot make check in maintainer-tools". > But that's all fully optional. > > For the full in-depth writeup of everything, see > > https://www.fooishbar.org/blog/gitlab-fdo-introduction/ > > I think now is also a good time, with mesa, xorg, wayland/weston and > others moved, to start thinking about how we'll move drm. There's a > few things to figure out though: > > - We probably want to split out maintainer-tools. That would address > the concern that there's 50+ committers to an auto-updating shell > script ... > /me laughs nervously > - We need to figure out how to handle the ACL trickery around drm-tip in > gitlab. > > - Probably good to stage the migration, with maintainer-tools, igt > leading. That will also make fd.o admins happy, who want to rework > their cloud infrastructure a bit before migrating the big kernel repos > over. > > - Figuring out the actual migration - we've been adding a pile of > committers since fd.o LDAP was converted to gitlab once back in > spring. We need to at least figure out how to move the new > accounts/committers. > > - Similar, maintainer-tools needs to move. We probably want to move > all the dim maintained kernel repos in one go, to avoid headaches with > double-accounts needed for committers. > > - CI, linux-next and everyone else should be fine, since the > cgit/non-ssh paths will keep working (they'll be read-only mirrors). > Need to double-check that with everyone. They can also pull the trees from git://gitlab.fd.o/blah as normal, just need to give them new pointers once we're stable. > > - Some organization structure would be good. > > https://cgit.freedesktop.org/drm > > libdrm won't be part of the gitlab drm group because that's already > moved under mesa (and you can't symlink/mulit-home anymore on gitlab): > > https://gitlab.freedesktop.org/mesa/drm > > But there's also drm_hwcomposer, which we might want to migrate into > drm too - gitlab requires a containing group, and > drm_hwcomposer/drm_hwcomposer is a bit silly. This seems fine to me. Our expansion plans likely aren't big enough to warrant a separate group. > > Note: Access rights can be done at any level in the hierarchy, the > organization is orthogonal to commit rights. > > - Anything else I've forgotten. > > A lot of this still needs to be figured out first. As a first step I'm > looking for volunteers who want to join the fun, besides comments and > thoughts on the overall topic of course. I'm pretty keen on getting this done, so I'll volunteer some cycles if there's something that needs doing. Sean > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Sean Paul, Software Engineer, Google / Chromium OS ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon: Fix oops upon driver load on PowerXpress laptops
On Mon, May 22, 2017 at 04:04:07PM +0200, Lukas Wunner wrote:
> On Sun, May 21, 2017 at 09:31:09AM +0200, Nicolai Stange wrote:
> > On Thu, May 18 2017, Lukas Wunner wrote:
> [snip]
> > > Reported-by: Nicolai Stange <nicsta...@gmail.com>
> > > Fixes: 7ffb0ce31cf9 ("drm/radeon: Don't register Thunderbolt eGPU with
> > > vga_switcheroo")
> > > Signed-off-by: Lukas Wunner <lu...@wunner.de>
> > > ---
> > >
> > > Awaiting a Tested-by: from Nicolai, but it's clear this is a bug and
> > > needs to be fixed, so sending out with a proper commit message now.
> > > The bug was only introduced to radeon, not amdgpu.
> >
> > Tested-by: Nicolai Stange <nicsta...@gmail.com>
> >
> > Thanks for the quick fix!
> >
> > > @Alex Deucher: I could push this to drm-misc-fixes but then it wouldn't
> > > land before -rc3 because Sean Paul has already sent out the -rc2 pull.
> > > I notice you haven't sent out a pull for -rc2 yet, so maybe you want to
> > > take it yourself? Whichever you prefer. Thanks & sorry for the breakage!
>
> I've learned this morning that Alex is on vacation. I've pushed
> the patch to drm-misc-fixes so that the issue is fixed in 4.12-rc3.
>
> @Sean Paul: I've fast-forwarded to 4.12-rc2 before pushing, please
> shout if I've done anything wrong. First time I'm doing this.
No shouting, but a heads-up on IRC is probably warranted for both pushing a
patch without R-b and fast-forwarding one of the branches.
Sean
>
> Thanks,
>
> Lukas
>
> > >
> > > drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c
> > > b/drivers/gpu/drm/radeon/radeon_kms.c
> > > index 6a68d440bc44..d0ad03674250 100644
> > > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > > @@ -116,7 +116,7 @@ int radeon_driver_load_kms(struct drm_device *dev,
> > > unsigned long flags)
> > > if ((radeon_runtime_pm != 0) &&
> > > radeon_has_atpx() &&
> > > ((flags & RADEON_IS_IGP) == 0) &&
> > > - !pci_is_thunderbolt_attached(rdev->pdev))
> > > + !pci_is_thunderbolt_attached(dev->pdev))
> > > flags |= RADEON_IS_PX;
> > >
> > > /* radeon_device_init should report only fatal error
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/5] drm/syncobj: add sync_file interaction.
On Fri, May 12, 2017 at 10:34:55AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airl...@redhat.com>
>
> This interface allows importing the fence from a sync_file into
> an existing drm sync object, or exporting the fence attached to
> an existing drm sync object into a new sync file object.
>
> This should only be used to interact with sync files where necessary.
>
> Signed-off-by: Dave Airlie <airl...@redhat.com>
With Daniel's comments taken into account,
Reviewed-by: Sean Paul <seanp...@chromium.org>
> ---
> drivers/gpu/drm/drm_syncobj.c | 56
> +++
> include/uapi/drm/drm.h| 6 +++--
> 2 files changed, 60 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 9a8c690..69ef20a 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -52,6 +52,7 @@
> #include
> #include
> #include
> +#include
>
> #include "drm_internal.h"
> #include
> @@ -290,6 +291,48 @@ static int drm_syncobj_fd_to_handle(struct drm_file
> *file_private,
> return 0;
> }
>
> +int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
> +int fd, int handle)
> +{
> + struct dma_fence *fence = sync_file_get_fence(fd);
> + if (!fence)
> + return -EINVAL;
> +
> + return drm_syncobj_replace_fence(file_private, handle, fence);
> +}
> +
> +int drm_syncobj_export_sync_file(struct drm_file *file_private,
> + int handle, int *p_fd)
> +{
> + int ret;
> + struct dma_fence *fence;
> + struct sync_file *sync_file;
> + int fd = get_unused_fd_flags(O_CLOEXEC);
> +
> + if (fd < 0)
> + return fd;
> +
> + ret = drm_syncobj_fence_get(file_private, handle, );
> + if (ret)
> + goto err_put_fd;
> +
> + sync_file = sync_file_create(fence);
> + if (!sync_file) {
> + ret = -EINVAL;
> + goto err_fence_put;
> + }
> +
> + fd_install(fd, sync_file->file);
> +
> + dma_fence_put(fence);
> + *p_fd = fd;
> + return 0;
> +err_fence_put:
> + dma_fence_put(fence);
> +err_put_fd:
> + put_unused_fd(fd);
> + return ret;
> +}
> /**
> * drm_syncobj_open - initalizes syncobj file-private structures at devnode
> open time
> * @dev: drm_device which is being opened by userspace
> @@ -372,6 +415,12 @@ drm_syncobj_handle_to_fd_ioctl(struct drm_device *dev,
> void *data,
> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> return -ENODEV;
>
> + if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_FENCE_SYNC_FILE)
> + return drm_syncobj_export_sync_file(file_private, args->handle,
> + >fd);
> + else if (args->flags)
> + return -EINVAL;
> +
> return drm_syncobj_handle_to_fd(file_private, args->handle,
> >fd);
> }
> @@ -385,6 +434,13 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev,
> void *data,
> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> return -ENODEV;
>
> + if (args->flags & DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE_FENCE)
> + return drm_syncobj_import_sync_file_fence(file_private,
> + args->fd,
> + args->handle);
> + else if (args->flags)
> + return -EINVAL;
> +
> return drm_syncobj_fd_to_handle(file_private, args->fd,
> >handle);
> }
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index db9e35e..d0e05f4 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -707,13 +707,15 @@ struct drm_syncobj_destroy {
> __u32 pad;
> };
>
> +#define DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE_FENCE (1 << 0)
> +#define DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_FENCE_SYNC_FILE (1 << 0)
> struct drm_syncobj_handle {
> __u32 handle;
> /** Flags.. only applicable for handle->fd */
> - __u32 flags;
> + __u32 fd_flags;
>
> __s32 fd;
> - __u32 pad;
> + __u32 flags;
> };
>
> /* timeout_ns is relative timeout in nanoseconds */
> --
> 2.9.3
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/5] drm/syncobj: add sync obj wait interface. (v2)
On Fri, May 12, 2017 at 10:34:54AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airl...@redhat.com>
>
> This interface will allow sync object to be used to back
> Vulkan fences. This API is pretty much the vulkan fence waiting
> API, and I've ported the code from amdgpu.
>
> v2: accept relative timeout, pass remaining time back
> to userspace.
>
> Signed-off-by: Dave Airlie <airl...@redhat.com>
Reviewed-by: Sean Paul <seanp...@chromium.org>
> ---
> drivers/gpu/drm/drm_internal.h | 2 +
> drivers/gpu/drm/drm_ioctl.c| 2 +
> drivers/gpu/drm/drm_syncobj.c | 139
> -
> include/uapi/drm/drm.h | 12
> 4 files changed, 154 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 44ef903..a508ad9 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -156,3 +156,5 @@ int drm_syncobj_handle_to_fd_ioctl(struct drm_device
> *dev, void *data,
> struct drm_file *file_private);
> int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private);
> +int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file_private);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 6da7adc..b142466 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -653,6 +653,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE,
> drm_syncobj_fd_to_handle_ioctl,
> DRM_UNLOCKED|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 835e987..9a8c690 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1,5 +1,7 @@
> /*
> * Copyright 2017 Red Hat
> + * Parts ported from amdgpu (fence wait code).
> + * Copyright 2016 Advanced Micro Devices, Inc.
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> * copy of this software and associated documentation files (the "Software"),
> @@ -31,10 +33,13 @@
> * that contain an optional fence. The fence can be updated with a new
> * fence, or be NULL.
> *
> + * syncobj's can be waited upon, where it will wait for the underlying
> + * fence.
> + *
> * syncobj's can be export to fd's and back, these fd's are opaque and
> * have no other use case, except passing the syncobj between processes.
> *
> - * TODO: sync_file interactions, waiting
> + * TODO: sync_file interactions.
> *
> * Their primary use-case is to implement Vulkan fences and semaphores.
> *
> @@ -383,3 +388,135 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev,
> void *data,
> return drm_syncobj_fd_to_handle(file_private, args->fd,
> >handle);
> }
> +
> +static int drm_syncobj_wait_all_fences(struct drm_device *dev,
> +struct drm_file *file_private,
> +struct drm_syncobj_wait *wait,
> +uint32_t *handles)
> +{
> + uint32_t i;
> + int ret = 0;
> + unsigned long timeout = nsecs_to_jiffies(wait->timeout_ns);
> +
> + for (i = 0; i < wait->count_handles; i++) {
> + struct dma_fence *fence;
> +
> + ret = drm_syncobj_fence_get(file_private, handles[i],
> + );
> + if (ret)
> + return ret;
> +
> + if (!fence)
> + continue;
> +
> + ret = dma_fence_wait_timeout(fence, true, timeout);
> +
> + dma_fence_put(fence);
> + if (ret < 0)
> + return ret;
> + if (ret == 0)
> + break;
> + timeout = ret;
> + }
> +
> + if (ret > 0)
> + wait->timeout_ns = jiffies_to_nsecs(ret);
> + wait->out_status = (ret > 0);
> + wait->first_signaled = 0;
> + return 0;
> +}
> +
> +static int drm_syncobj_wait_any_fence(struct drm_device *dev,
> + struct drm_file *file_private,
> +
Re: [PATCH 1/5] drm: introduce sync objects (v2)
On Fri, May 12, 2017 at 10:34:53AM +1000, Dave Airlie wrote: > From: Dave Airlie <airl...@redhat.com> > > Sync objects are new toplevel drm object, that contain a > pointer to a fence. This fence can be updated via command > submission ioctls via drivers. > > There is also a generic wait obj API modelled on the vulkan > wait API (with code modelled on some amdgpu code). > > These objects can be converted to an opaque fd that can be > passes between processes. > > v2: rename reference/unreference to put/get (Chris) > fix leaked reference (David Zhou) > drop mutex in favour of cmpxchg (Chris) > document drm_syncobj_fence_get > use ENOENT for syncobj lookup. > > Signed-off-by: Dave Airlie <airl...@redhat.com> With Daniel's comments addressed, Reviewed-by: Sean Paul <seanp...@chromium.org> > > fixup > --- > Documentation/gpu/drm-internals.rst | 3 + > Documentation/gpu/drm-mm.rst| 6 + > drivers/gpu/drm/Makefile| 2 +- > drivers/gpu/drm/drm_fops.c | 8 + > drivers/gpu/drm/drm_internal.h | 13 ++ > drivers/gpu/drm/drm_ioctl.c | 12 ++ > drivers/gpu/drm/drm_syncobj.c | 385 > > include/drm/drmP.h | 5 + > include/drm/drm_drv.h | 1 + > include/drm/drm_syncobj.h | 87 > include/uapi/drm/drm.h | 25 +++ > 11 files changed, 546 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/drm_syncobj.c > create mode 100644 include/drm/drm_syncobj.h > > diff --git a/Documentation/gpu/drm-internals.rst > b/Documentation/gpu/drm-internals.rst > index e35920d..2ea3bce 100644 > --- a/Documentation/gpu/drm-internals.rst > +++ b/Documentation/gpu/drm-internals.rst > @@ -98,6 +98,9 @@ DRIVER_ATOMIC > implement appropriate obj->atomic_get_property() vfuncs for any > modeset objects with driver specific properties. > > +DRIVER_SYNCOBJ > +Driver support drm sync objects. > + > Major, Minor and Patchlevel > ~~~ > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > index f5760b1..28aebe8 100644 > --- a/Documentation/gpu/drm-mm.rst > +++ b/Documentation/gpu/drm-mm.rst > @@ -483,3 +483,9 @@ DRM Cache Handling > > .. kernel-doc:: drivers/gpu/drm/drm_cache.c > :export: > + > +DRM Sync Objects > +=== > + > +.. kernel-doc:: drivers/gpu/drm/drm_syncobj.c > + :export: > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 3ee9579..b5e565c 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -16,7 +16,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \ > drm_framebuffer.o drm_connector.o drm_blend.o \ > drm_encoder.o drm_mode_object.o drm_property.o \ > drm_plane.o drm_color_mgmt.o drm_print.o \ > - drm_dumb_buffers.o drm_mode_config.o > + drm_dumb_buffers.o drm_mode_config.o drm_syncobj.o > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o > drm-$(CONFIG_DRM_VM) += drm_vm.o > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index afdf5b1..9a61df2 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -219,6 +219,9 @@ static int drm_open_helper(struct file *filp, struct > drm_minor *minor) > if (drm_core_check_feature(dev, DRIVER_GEM)) > drm_gem_open(dev, priv); > > + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > + drm_syncobj_open(priv); > + > if (drm_core_check_feature(dev, DRIVER_PRIME)) > drm_prime_init_file_private(>prime); > > @@ -266,6 +269,8 @@ static int drm_open_helper(struct file *filp, struct > drm_minor *minor) > out_prime_destroy: > if (drm_core_check_feature(dev, DRIVER_PRIME)) > drm_prime_destroy_file_private(>prime); > + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > + drm_syncobj_release(priv); > if (drm_core_check_feature(dev, DRIVER_GEM)) > drm_gem_release(dev, priv); > put_pid(priv->pid); > @@ -400,6 +405,9 @@ int drm_release(struct inode *inode, struct file *filp) > drm_property_destroy_user_blobs(dev, file_priv); > } > > + if (drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > + drm_syncobj_release(file_priv); > + > if (drm_core_check_feature(dev, DRIVER_GEM)) > drm_gem_release(dev, file_priv); > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index f3738
Re: [PATCH 1/5] drm: introduce sync objects
On Wed, Apr 26, 2017 at 01:28:29PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airl...@redhat.com>
>
> Sync objects are new toplevel drm object, that contain a
> pointer to a fence. This fence can be updated via command
> submission ioctls via drivers.
>
> There is also a generic wait obj API modelled on the vulkan
> wait API (with code modelled on some amdgpu code).
>
> These objects can be converted to an opaque fd that can be
> passes between processes.
>
> TODO: define sync_file interaction.
>
> Signed-off-by: Dave Airlie <airl...@redhat.com>
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> new file mode 100644
> index 000..3cc42b7
> --- /dev/null
> +++ b/include/drm/drm_syncobj.h
> +/**
> + * drm_gem_object_reference - acquire a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This acquires additional reference to @obj. It is illegal to call this
> + * without already holding a reference. No locks required.
> + */
> +static inline void
> +drm_syncobj_reference(struct drm_syncobj *obj)
> +{
> + kref_get(>refcount);
> +}
> +
> +/**
> + * __drm_gem_object_unreference - raw function to release a GEM BO reference
> + * @obj: GEM buffer object
> + *
> + * This function is meant to be used by drivers which are not encumbered with
> + * _device.struct_mutex legacy locking and which are using the
> + * gem_free_object_unlocked callback. It avoids all the locking checks and
> + * locking overhead of drm_gem_object_unreference() and
> + * drm_gem_object_unreference_unlocked().
> + *
> + * Drivers should never call this directly in their code. Instead they should
> + * wrap it up into a ``driver_gem_object_unreference(struct driver_gem_object
> + * *obj)`` wrapper function, and use that. Shared code should never call
> this, to
> + * avoid breaking drivers by accident which still depend upon
> + * _device.struct_mutex locking.
Lot's of gem_obj copypasta to clean up in the comment here and above
> + */
> +static inline void
> +drm_syncobj_unreference(struct drm_syncobj *obj)
> +{
> + kref_put(>refcount, drm_syncobj_free);
> +}
> +
> +int drm_syncobj_fence_get(struct drm_file *file_private,
> + u32 handle,
> + struct dma_fence **fence);
> +int drm_syncobj_replace_fence(struct drm_file *file_private,
> + u32 handle,
> + struct dma_fence *fence);
> +
> +#endif
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c5284..dd0b99c 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -647,6 +647,7 @@ struct drm_gem_open {
> #define DRM_CAP_CURSOR_HEIGHT0x9
> #define DRM_CAP_ADDFB2_MODIFIERS 0x10
> #define DRM_CAP_PAGE_FLIP_TARGET 0x11
> +#define DRM_CAP_SYNCOBJ 0x13
>
> /** DRM_IOCTL_GET_CAP ioctl argument type */
> struct drm_get_cap {
> @@ -696,6 +697,25 @@ struct drm_prime_handle {
> __s32 fd;
> };
>
> +struct drm_syncobj_create {
> + __u32 handle;
> + __u32 flags;
> +};
> +
> +struct drm_syncobj_destroy {
> + __u32 handle;
> + __u32 pad;
> +};
> +
> +struct drm_syncobj_handle {
> + __u32 handle;
> + /** Flags.. only applicable for handle->fd */
> + __u32 flags;
> +
> + __s32 fd;
> + __u32 pad;
> +};
> +
> #if defined(__cplusplus)
> }
> #endif
> @@ -814,6 +834,11 @@ extern "C" {
> #define DRM_IOCTL_MODE_CREATEPROPBLOBDRM_IOWR(0xBD, struct
> drm_mode_create_blob)
> #define DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, struct
> drm_mode_destroy_blob)
>
> +#define DRM_IOCTL_SYNCOBJ_CREATE DRM_IOWR(0xBF, struct
> drm_syncobj_create)
> +#define DRM_IOCTL_SYNCOBJ_DESTROYDRM_IOWR(0xC0, struct
> drm_syncobj_destroy)
> +#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct
> drm_syncobj_handle)
> +#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct
> drm_syncobj_handle)
> +
> /**
> * Device specific ioctls should only be in their respective headers
> * The device specific ioctl range is from 0x40 to 0x9f.
> --
> 2.9.3
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
