| Message ID | 20180614104709.2808-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 14, 2018 at 01:47:09PM +0300, Jani Nikula wrote: > Pass a local acpi_handle around instead of having a static dsm priv > structure. If we need it later, we can always move it to dev_priv, and > the change at hand will make that easier as well. > > Care is taken to preserve old behaviour, particularly using the last > non-NULL acpi handle, whether it makes sense or not. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_acpi.c | 27 +++++++++++---------------- > 1 file changed, 11 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c > index d1abf4bb7c81..6ba478e57b9b 100644 > --- a/drivers/gpu/drm/i915/intel_acpi.c > +++ b/drivers/gpu/drm/i915/intel_acpi.c > @@ -12,10 +12,6 @@ > #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */ > #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */ > > -static struct intel_dsm_priv { > - acpi_handle dhandle; > -} intel_dsm_priv; > - > static const guid_t intel_dsm_guid = > GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f, > 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c); > @@ -72,12 +68,12 @@ static char *intel_dsm_mux_type(u8 type) > } > } > > -static void intel_dsm_platform_mux_info(void) > +static void intel_dsm_platform_mux_info(acpi_handle dhandle) > { > int i; > union acpi_object *pkg, *connector_count; > > - pkg = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, &intel_dsm_guid, > + pkg = acpi_evaluate_dsm_typed(dhandle, &intel_dsm_guid, > INTEL_DSM_REVISION_ID, INTEL_DSM_FN_PLATFORM_MUX_INFO, > NULL, ACPI_TYPE_PACKAGE); > if (!pkg) { > @@ -107,41 +103,40 @@ static void intel_dsm_platform_mux_info(void) > ACPI_FREE(pkg); > } > > -static bool intel_dsm_pci_probe(struct pci_dev *pdev) > +static acpi_handle intel_dsm_pci_probe(struct pci_dev *pdev) > { > acpi_handle dhandle; > > dhandle = ACPI_HANDLE(&pdev->dev); > if (!dhandle) > - return false; > + return NULL; > > if (!acpi_check_dsm(dhandle, &intel_dsm_guid, INTEL_DSM_REVISION_ID, > 1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) { > DRM_DEBUG_KMS("no _DSM method for intel device\n"); > - return false; > + return NULL; > } > > - intel_dsm_priv.dhandle = dhandle; > - intel_dsm_platform_mux_info(); > + intel_dsm_platform_mux_info(dhandle); > > - return true; > + return dhandle; > } > > static bool intel_dsm_detect(void) > { > + acpi_handle dhandle = NULL; > char acpi_method_name[255] = { 0 }; > struct acpi_buffer buffer = {sizeof(acpi_method_name), acpi_method_name}; > struct pci_dev *pdev = NULL; > - bool has_dsm = false; > int vga_count = 0; > > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) { > vga_count++; > - has_dsm |= intel_dsm_pci_probe(pdev); > + dhandle = intel_dsm_pci_probe(pdev) ?: dhandle; I *think* gcc promises not to evaluate things twice with ?:, so should be safe even if intel_dsm_pci_probe() has some side effects. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > } > > - if (vga_count == 2 && has_dsm) { > - acpi_get_name(intel_dsm_priv.dhandle, ACPI_FULL_PATHNAME, &buffer); > + if (vga_count == 2 && dhandle) { > + acpi_get_name(dhandle, ACPI_FULL_PATHNAME, &buffer); > DRM_DEBUG_DRIVER("vga_switcheroo: detected DSM switching method %s handle\n", > acpi_method_name); > return true; > -- > 2.11.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 14 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jun 14, 2018 at 01:47:09PM +0300, Jani Nikula wrote: >> Pass a local acpi_handle around instead of having a static dsm priv >> structure. If we need it later, we can always move it to dev_priv, and >> the change at hand will make that easier as well. >> >> Care is taken to preserve old behaviour, particularly using the last >> non-NULL acpi handle, whether it makes sense or not. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_acpi.c | 27 +++++++++++---------------- >> 1 file changed, 11 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c >> index d1abf4bb7c81..6ba478e57b9b 100644 >> --- a/drivers/gpu/drm/i915/intel_acpi.c >> +++ b/drivers/gpu/drm/i915/intel_acpi.c >> @@ -12,10 +12,6 @@ >> #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */ >> #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */ >> >> -static struct intel_dsm_priv { >> - acpi_handle dhandle; >> -} intel_dsm_priv; >> - >> static const guid_t intel_dsm_guid = >> GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f, >> 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c); >> @@ -72,12 +68,12 @@ static char *intel_dsm_mux_type(u8 type) >> } >> } >> >> -static void intel_dsm_platform_mux_info(void) >> +static void intel_dsm_platform_mux_info(acpi_handle dhandle) >> { >> int i; >> union acpi_object *pkg, *connector_count; >> >> - pkg = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, &intel_dsm_guid, >> + pkg = acpi_evaluate_dsm_typed(dhandle, &intel_dsm_guid, >> INTEL_DSM_REVISION_ID, INTEL_DSM_FN_PLATFORM_MUX_INFO, >> NULL, ACPI_TYPE_PACKAGE); >> if (!pkg) { >> @@ -107,41 +103,40 @@ static void intel_dsm_platform_mux_info(void) >> ACPI_FREE(pkg); >> } >> >> -static bool intel_dsm_pci_probe(struct pci_dev *pdev) >> +static acpi_handle intel_dsm_pci_probe(struct pci_dev *pdev) >> { >> acpi_handle dhandle; >> >> dhandle = ACPI_HANDLE(&pdev->dev); >> if (!dhandle) >> - return false; >> + return NULL; >> >> if (!acpi_check_dsm(dhandle, &intel_dsm_guid, INTEL_DSM_REVISION_ID, >> 1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) { >> DRM_DEBUG_KMS("no _DSM method for intel device\n"); >> - return false; >> + return NULL; >> } >> >> - intel_dsm_priv.dhandle = dhandle; >> - intel_dsm_platform_mux_info(); >> + intel_dsm_platform_mux_info(dhandle); >> >> - return true; >> + return dhandle; >> } >> >> static bool intel_dsm_detect(void) >> { >> + acpi_handle dhandle = NULL; >> char acpi_method_name[255] = { 0 }; >> struct acpi_buffer buffer = {sizeof(acpi_method_name), acpi_method_name}; >> struct pci_dev *pdev = NULL; >> - bool has_dsm = false; >> int vga_count = 0; >> >> while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) { >> vga_count++; >> - has_dsm |= intel_dsm_pci_probe(pdev); >> + dhandle = intel_dsm_pci_probe(pdev) ?: dhandle; > > I *think* gcc promises not to evaluate things twice with ?:, so > should be safe even if intel_dsm_pci_probe() has some side effects. Yeah I was wondering if this was too clever, but then the alternative was pretty tedious with another temp variable and conditions etc. Or changing behaviour which I wanted to avoid. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks for the review, pushed to dinq. BR, Jani. > >> } >> >> - if (vga_count == 2 && has_dsm) { >> - acpi_get_name(intel_dsm_priv.dhandle, ACPI_FULL_PATHNAME, &buffer); >> + if (vga_count == 2 && dhandle) { >> + acpi_get_name(dhandle, ACPI_FULL_PATHNAME, &buffer); >> DRM_DEBUG_DRIVER("vga_switcheroo: detected DSM switching method %s handle\n", >> acpi_method_name); >> return true; >> -- >> 2.11.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 14 Jun 2018, Patchwork <patchwork@emeril.freedesktop.org> wrote: > == Series Details == > > Series: drm/i915/dsm: remove unnecessary dsm priv structure > URL : https://patchwork.freedesktop.org/series/44746/ > State : failure > > == Summary == > > = CI Bug Log - changes from CI_DRM_4313_full -> Patchwork_9298_full = > > == Summary - FAILURE == > > Serious unknown changes coming with Patchwork_9298_full absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_9298_full, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_9298_full: > > === IGT changes === > > ==== Possible regressions ==== > > igt@gem_eio@in-flight-internal-1us: > shard-kbl: PASS -> DMESG-FAIL > > igt@kms_draw_crc@draw-method-rgb565-blt-untiled: > shard-kbl: PASS -> FAIL > > > ==== Warnings ==== > > igt@gem_wait@write-busy-render: > shard-kbl: PASS -> SKIP +22 > > igt@perf_pmu@rc6: > shard-kbl: SKIP -> PASS +1 I seem to have jumped the gun with pushing the patch, but I don't see how any of these could have been caused by the patch at hand. BR, Jani. > > > == Known issues == > > Here are the changes found in Patchwork_9298_full that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@drv_selftest@live_gtt: > shard-kbl: PASS -> FAIL (fdo#105347) > > igt@drv_selftest@live_hangcheck: > shard-kbl: PASS -> DMESG-FAIL (fdo#106560) > > igt@drv_selftest@mock_scatterlist: > shard-glk: NOTRUN -> DMESG-WARN (fdo#103667) > > igt@drv_suspend@shrink: > shard-hsw: PASS -> INCOMPLETE (fdo#103540) > > igt@gem_exec_schedule@pi-ringfull-render: > shard-glk: NOTRUN -> FAIL (fdo#103158) > > igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic: > shard-glk: PASS -> FAIL (fdo#106509, fdo#105454) > > igt@kms_flip@flip-vs-expired-vblank: > shard-hsw: PASS -> FAIL (fdo#105363, fdo#102887) > > igt@kms_flip_tiling@flip-y-tiled: > shard-glk: PASS -> FAIL (fdo#103822, fdo#104724) > > igt@kms_frontbuffer_tracking@fbc-stridechange: > shard-kbl: PASS -> FAIL (fdo#106067) > > igt@kms_vblank@pipe-a-wait-forked-busy-hang: > shard-kbl: PASS -> FAIL (fdo#106066) > > igt@perf@buffer-fill: > shard-kbl: PASS -> DMESG-FAIL (fdo#106064) > > > ==== Possible fixes ==== > > igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing: > shard-glk: FAIL (fdo#105703) -> PASS > > igt@kms_flip@flip-vs-expired-vblank: > shard-apl: FAIL (fdo#105363, fdo#102887) -> PASS > > igt@kms_flip@modeset-vs-vblank-race: > shard-hsw: FAIL (fdo#103060) -> PASS > > igt@kms_flip@wf_vblank-ts-check-interruptible: > shard-glk: FAIL (fdo#100368) -> PASS > > igt@kms_flip_tiling@flip-x-tiled: > shard-glk: FAIL (fdo#103822, fdo#104724) -> PASS > > > fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 > fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887 > fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060 > fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158 > fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540 > fdo#103667 https://bugs.freedesktop.org/show_bug.cgi?id=103667 > fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822 > fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724 > fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347 > fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363 > fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454 > fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703 > fdo#106064 https://bugs.freedesktop.org/show_bug.cgi?id=106064 > fdo#106066 https://bugs.freedesktop.org/show_bug.cgi?id=106066 > fdo#106067 https://bugs.freedesktop.org/show_bug.cgi?id=106067 > fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509 > fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560 > > > == Participating hosts (5 -> 5) == > > No changes in participating hosts > > > == Build changes == > > * Linux: CI_DRM_4313 -> Patchwork_9298 > > CI_DRM_4313: 6e2266f47609a426ce704b1fef25906a8afc1155 @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4518: e4908004547b63131352fbc0ddcdb1d3d55480e0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_9298: cb53f4dfce8b7b947130d18383f5601842caea50 @ git://anongit.freedesktop.org/gfx-ci/linux > piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9298/shards.html
diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c index d1abf4bb7c81..6ba478e57b9b 100644 --- a/drivers/gpu/drm/i915/intel_acpi.c +++ b/drivers/gpu/drm/i915/intel_acpi.c @@ -12,10 +12,6 @@ #define INTEL_DSM_REVISION_ID 1 /* For Calpella anyway... */ #define INTEL_DSM_FN_PLATFORM_MUX_INFO 1 /* No args */ -static struct intel_dsm_priv { - acpi_handle dhandle; -} intel_dsm_priv; - static const guid_t intel_dsm_guid = GUID_INIT(0x7ed873d3, 0xc2d0, 0x4e4f, 0xa8, 0x54, 0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c); @@ -72,12 +68,12 @@ static char *intel_dsm_mux_type(u8 type) } } -static void intel_dsm_platform_mux_info(void) +static void intel_dsm_platform_mux_info(acpi_handle dhandle) { int i; union acpi_object *pkg, *connector_count; - pkg = acpi_evaluate_dsm_typed(intel_dsm_priv.dhandle, &intel_dsm_guid, + pkg = acpi_evaluate_dsm_typed(dhandle, &intel_dsm_guid, INTEL_DSM_REVISION_ID, INTEL_DSM_FN_PLATFORM_MUX_INFO, NULL, ACPI_TYPE_PACKAGE); if (!pkg) { @@ -107,41 +103,40 @@ static void intel_dsm_platform_mux_info(void) ACPI_FREE(pkg); } -static bool intel_dsm_pci_probe(struct pci_dev *pdev) +static acpi_handle intel_dsm_pci_probe(struct pci_dev *pdev) { acpi_handle dhandle; dhandle = ACPI_HANDLE(&pdev->dev); if (!dhandle) - return false; + return NULL; if (!acpi_check_dsm(dhandle, &intel_dsm_guid, INTEL_DSM_REVISION_ID, 1 << INTEL_DSM_FN_PLATFORM_MUX_INFO)) { DRM_DEBUG_KMS("no _DSM method for intel device\n"); - return false; + return NULL; } - intel_dsm_priv.dhandle = dhandle; - intel_dsm_platform_mux_info(); + intel_dsm_platform_mux_info(dhandle); - return true; + return dhandle; } static bool intel_dsm_detect(void) { + acpi_handle dhandle = NULL; char acpi_method_name[255] = { 0 }; struct acpi_buffer buffer = {sizeof(acpi_method_name), acpi_method_name}; struct pci_dev *pdev = NULL; - bool has_dsm = false; int vga_count = 0; while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) { vga_count++; - has_dsm |= intel_dsm_pci_probe(pdev); + dhandle = intel_dsm_pci_probe(pdev) ?: dhandle; } - if (vga_count == 2 && has_dsm) { - acpi_get_name(intel_dsm_priv.dhandle, ACPI_FULL_PATHNAME, &buffer); + if (vga_count == 2 && dhandle) { + acpi_get_name(dhandle, ACPI_FULL_PATHNAME, &buffer); DRM_DEBUG_DRIVER("vga_switcheroo: detected DSM switching method %s handle\n", acpi_method_name); return true;
Pass a local acpi_handle around instead of having a static dsm priv structure. If we need it later, we can always move it to dev_priv, and the change at hand will make that easier as well. Care is taken to preserve old behaviour, particularly using the last non-NULL acpi handle, whether it makes sense or not. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_acpi.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)