Re: [PATCH v2 2/2] drm/privacy_screen_x86: Add entry for ChromeOS privacy-screen
On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote:
> Add a static entry in the x86 table, to detect and wait for
> privacy-screen on some ChromeOS platforms.
>
> Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is
> enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe
> shall return EPROBE_DEFER until a platform driver actually registers the
> privacy-screen: https://hansdegoede.livejournal.com/25948.html
>
> Signed-off-by: Rajat Jain
> ---
> v2: * Use #if instead of #elif
> * Reorder the patches in the series.
> * Rebased on drm-tip
>
> drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c
> b/drivers/gpu/drm/drm_privacy_screen_x86.c
> index a2cafb294ca6..0c5699ad70a3 100644
> --- a/drivers/gpu/drm/drm_privacy_screen_x86.c
> +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c
> @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void)
> }
> #endif
>
> +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> +static bool __init detect_chromeos_privacy_screen(void)
Does marking this __init work in case there is a deferral? Can it happen
that privacy screen is a module and so will get loaded only after we
discarded __init sections.
> +{
> + if (!acpi_dev_present("GOOG0010", NULL, -1))
> + return false;
> +
> + pr_info("%s: Need to wait for ChromeOS privacy-screen drvr", __func__);
I still do not see how this message is helpful. If it is really desired,
I'd put something into the code that calls into lookups.
> + return true;
> +
> +}
> +#endif
> +
> static const struct arch_init_data arch_init_data[] __initconst = {
> #if IS_ENABLED(CONFIG_THINKPAD_ACPI)
> {
> @@ -58,6 +70,16 @@ static const struct arch_init_data arch_init_data[]
> __initconst = {
> .detect = detect_thinkpad_privacy_screen,
> },
> #endif
> +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN)
> + {
> + .lookup = {
> + .dev_id = NULL,
> + .con_id = NULL,
> + .provider = "privacy_screen-GOOG0010:00",
> + },
> + .detect = detect_chromeos_privacy_screen,
> + },
> +#endif
> };
>
> void __init drm_privacy_screen_lookup_init(void)
> --
> 2.34.1.307.g9b7440fafd-goog
>
Thanks.
--
Dmitry
Re: [PATCH v2 1/2] platform/chrome: Add driver for ChromeOS privacy-screen
Hi Rajat,
On Fri, Dec 17, 2021 at 12:28:49PM -0800, Rajat Jain wrote:
> This adds the ACPI driver for the ChromeOS privacy screen that is
> present on some chromeos devices.
>
> Note that ideally, we'd want this privacy screen driver to be probed
> BEFORE the drm probe in order to avoid a drm probe deferral:
> https://hansdegoede.livejournal.com/25948.html
>
> In practise, I found that ACPI drivers are bound to their devices AFTER
> the drm probe on chromebooks. So on chromebooks with privacy-screen,
> this patch along with the next one in this series results in a probe
> deferral of about 250ms for i915 driver. However, it did not result in
> any user noticeable delay of splash screen in my personal experience.
>
> In future if this probe deferral turns out to be an issue, we can
> consider turning this ACPI driver into something that is probed
> earlier than the drm drivers.
>
> Signed-off-by: Rajat Jain
> ---
> v2: * Reword the commit log
> * Make the Kconfig into a tristate
> * Reorder the patches in the series.
>
> drivers/platform/chrome/Kconfig | 9 ++
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/chromeos_priv_scrn.c | 132 +++
> 3 files changed, 142 insertions(+)
> create mode 100644 drivers/platform/chrome/chromeos_priv_scrn.c
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index ccc23d8686e8..d1c209a45a62 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -243,6 +243,15 @@ config CROS_USBPD_NOTIFY
> To compile this driver as a module, choose M here: the
> module will be called cros_usbpd_notify.
>
> +config CHROMEOS_PRIVACY_SCREEN
> + tristate "ChromeOS Privacy Screen support"
> + depends on ACPI
> + depends on DRM
> + select DRM_PRIVACY_SCREEN
> + help
> + This driver provides the support needed for the in-built electronic
> + privacy screen that is present on some ChromeOS devices.
> +
> source "drivers/platform/chrome/wilco_ec/Kconfig"
>
> endif # CHROMEOS_PLATFORMS
> diff --git a/drivers/platform/chrome/Makefile
> b/drivers/platform/chrome/Makefile
> index f901d2e43166..cfa0bb4e9e34 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -4,6 +4,7 @@
> CFLAGS_cros_ec_trace.o:= -I$(src)
>
> obj-$(CONFIG_CHROMEOS_LAPTOP)+= chromeos_laptop.o
> +obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN)+= chromeos_priv_scrn.o
> obj-$(CONFIG_CHROMEOS_PSTORE)+= chromeos_pstore.o
> obj-$(CONFIG_CHROMEOS_TBMC) += chromeos_tbmc.o
> obj-$(CONFIG_CROS_EC)+= cros_ec.o
> diff --git a/drivers/platform/chrome/chromeos_priv_scrn.c
> b/drivers/platform/chrome/chromeos_priv_scrn.c
> new file mode 100644
> index ..a4cbf5c79c2a
> --- /dev/null
> +++ b/drivers/platform/chrome/chromeos_priv_scrn.c
I think we can spare a few more characters :) chromeos_privacy_screen.c
maybe?
And also see if maybe variables in the code are not that unseemly long
even if not abbreviated?
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * chromeos_priv_scrn.c - ChromeOS Privacy Screen support
I'd avoid mentioning file name as those tend to change.
> + *
> + * Copyright (C) 2022 The Chromium OS Authors
This is not correct copyright for kernel contributions. It should be
attributed to "Google LLC". Note that it is different from CrOS
userspace.
> + *
> + */
> +
> +#include
> +#include
> +
> +/*
> + * The DSM (Define Specific Method) constants below are the agreed API with
> + * the firmware team, on how to control privacy screen using ACPI methods.
> + */
> +#define PRIV_SCRN_DSM_REVID 1 /* DSM version */
> +#define PRIV_SCRN_DSM_FN_GET_STATUS 1 /* Get privacy screen status */
> +#define PRIV_SCRN_DSM_FN_ENABLE 2 /* Enable privacy
> screen */
> +#define PRIV_SCRN_DSM_FN_DISABLE 3 /* Disable privacy screen */
> +
> +static const guid_t chromeos_priv_scrn_dsm_guid =
> + GUID_INIT(0xc7033113, 0x8720, 0x4ceb,
> + 0x90, 0x90, 0x9d, 0x52, 0xb3, 0xe5, 0x2d, 0x73);
> +
> +static void
> +chromeos_priv_scrn_get_hw_state(struct drm_privacy_screen *drm_priv_scrn)
> +{
> + union acpi_object *obj;
> + acpi_handle handle;
> + struct device *priv_scrn = drm_priv_scrn->dev.parent;
This is really bad that we need to poke into internals of
drm_privacy_screen to get to "our" device. I think there is only one
consume of the privacy screen API at the moment, the thinkpad driver, so
maybe it is not too late to change drm_privacy_screen_register() to
either accept instance of struct drm_privacy_screen (which then could be
embedded into something) or accept a void pointer to attach arbitrary
data to it, and then add drm_privacy_screen_get_drvdata() to get to that
pointer.
> +
> + if
Re: [PATCH v2 2/2] drm/privacy_screen_x86: Add entry for ChromeOS privacy-screen
On Mon, Dec 20, 2021 at 12:29:18PM -0800, Rajat Jain wrote: > Hello, > > On Mon, Dec 20, 2021 at 11:50 AM Dmitry Torokhov > wrote: > > > > On Fri, Dec 17, 2021 at 12:28:50PM -0800, Rajat Jain wrote: > > > Add a static entry in the x86 table, to detect and wait for > > > privacy-screen on some ChromeOS platforms. > > > > > > Please note that this means that if CONFIG_CHROMEOS_PRIVACY_SCREEN is > > > enabled, and if "GOOG0010" device is found in ACPI, then the i915 probe > > > shall return EPROBE_DEFER until a platform driver actually registers the > > > privacy-screen: https://hansdegoede.livejournal.com/25948.html > > > > > > Signed-off-by: Rajat Jain > > > --- > > > v2: * Use #if instead of #elif > > > * Reorder the patches in the series. > > > * Rebased on drm-tip > > > > > > drivers/gpu/drm/drm_privacy_screen_x86.c | 22 ++ > > > 1 file changed, 22 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_privacy_screen_x86.c > > > b/drivers/gpu/drm/drm_privacy_screen_x86.c > > > index a2cafb294ca6..0c5699ad70a3 100644 > > > --- a/drivers/gpu/drm/drm_privacy_screen_x86.c > > > +++ b/drivers/gpu/drm/drm_privacy_screen_x86.c > > > @@ -47,6 +47,18 @@ static bool __init detect_thinkpad_privacy_screen(void) > > > } > > > #endif > > > > > > +#if IS_ENABLED(CONFIG_CHROMEOS_PRIVACY_SCREEN) > > > +static bool __init detect_chromeos_privacy_screen(void) > > > > Does marking this __init work in case there is a deferral? > > Yes, I have verified that for Chromeos case, it is a deferral. > > > Can it happen > > that privacy screen is a module and so will get loaded only after we > > discarded __init sections. > > Perhaps. But I do not think that is a problem. All the functions and > data in this file are in __init sections, and this entry is here to > ensure that the drm probe will wait for the privacy screen driver > (whenever it is loaded). Ah, OK, we are not leaking detect() pointers outside this module. > That is the reason, ideally we would want to > somehow restrict the privacy screen to be built into the kernel so as > to minimize the delay if any. I understand, but we can not code to the config we expect to use on Chrome OS, we need to make sure we cover all possibilities. Thanks. -- Dmitry
Re: [PATCH] video: fbdev: mb862xx: remove redundant assignment to pointer ptr
On Tue, Dec 21, 2021 at 3:01 AM Colin Ian King wrote:
> The pointer ptr is being assigned a value that is never read. The
> pointer is being re-assigned later in a loop. The assignment is
> redundant and can be removed.
>
> Signed-off-by: Colin Ian King
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFC 4/6] drm/amdgpu: Serialize non TDR gpu recovery with TDRs
Am 20.12.21 um 23:17 schrieb Andrey Grodzovsky:
On 2021-12-20 2:20 a.m., Christian König wrote:
Am 17.12.21 um 23:27 schrieb Andrey Grodzovsky:
Use reset domain wq also for non TDR gpu recovery trigers
such as sysfs and RAS. We must serialize all possible
GPU recoveries to gurantee no concurrency there.
For TDR call the original recovery function directly since
it's already executed from within the wq. For others just
use a wrapper to qeueue work and wait on it to finish.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33
+-
drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 +-
3 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index b5ff76aae7e0..8e96b9a14452 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1296,6 +1296,8 @@ bool amdgpu_device_has_job_running(struct
amdgpu_device *adev);
bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
struct amdgpu_job* job);
+int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev,
+ struct amdgpu_job *job);
void amdgpu_device_pci_config_reset(struct amdgpu_device *adev);
int amdgpu_device_pci_reset(struct amdgpu_device *adev);
bool amdgpu_device_need_post(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b595e6d699b5..55cd67b9ede2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4979,7 +4979,7 @@ static void amdgpu_device_recheck_guilty_jobs(
* Returns 0 for success or an error on failure.
*/
-int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
+int amdgpu_device_gpu_recover_imp(struct amdgpu_device *adev,
struct amdgpu_job *job)
{
struct list_head device_list, *device_list_handle = NULL;
@@ -5236,6 +5236,37 @@ int amdgpu_device_gpu_recover(struct
amdgpu_device *adev,
return r;
}
+struct recover_work_struct {
Please add an amdgpu_ prefix to the name.
+ struct work_struct base;
+ struct amdgpu_device *adev;
+ struct amdgpu_job *job;
+ int ret;
+};
+
+static void amdgpu_device_queue_gpu_recover_work(struct work_struct
*work)
+{
+ struct recover_work_struct *recover_work = container_of(work,
struct recover_work_struct, base);
+
+ recover_work->ret =
amdgpu_device_gpu_recover_imp(recover_work->adev, recover_work->job);
+}
+/*
+ * Serialize gpu recover into reset domain single threaded wq
+ */
+int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
+ struct amdgpu_job *job)
+{
+ struct recover_work_struct work = {.adev = adev, .job = job};
+
+ INIT_WORK(&work.base, amdgpu_device_queue_gpu_recover_work);
+
+ if (!queue_work(adev->reset_domain.wq, &work.base))
+ return -EAGAIN;
+
+ flush_work(&work.base);
+
+ return work.ret;
+}
Maybe that should be part of the scheduler code? Not really sure,
just an idea.
Seems to me that since the reset domain is almost always above a
single scheduler granularity then it wouldn't feet very well there.
Yeah, but what if we introduce an drm_sched_recover_queue and
drm_sched_recover_work object?
It's probably ok to go forward with that for now, but this handling
makes quite some sense to have independent of which driver is using it.
So as soon as we see a second similar implementation we should move it
into common code.
Regards,
Christian.
Andrey
Apart from that looks good to me,
Christian.
+
/**
* amdgpu_device_get_pcie_info - fence pcie info about the PCIE slot
*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index bfc47bea23db..38c9fd7b7ad4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -63,7 +63,7 @@ static enum drm_gpu_sched_stat
amdgpu_job_timedout(struct drm_sched_job *s_job)
ti.process_name, ti.tgid, ti.task_name, ti.pid);
if (amdgpu_device_should_recover_gpu(ring->adev)) {
- amdgpu_device_gpu_recover(ring->adev, job);
+ amdgpu_device_gpu_recover_imp(ring->adev, job);
} else {
drm_sched_suspend_timeout(&ring->sched);
if (amdgpu_sriov_vf(adev))
