Re: [PATCH] drm: Add support for integrated privacy screens
On Fri, Oct 25, 2019 at 4:36 AM Thierry Reding wrote:
>
> On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote:
> > Hi,
> >
> > Thanks for your review and comments. Please see inline below.
> >
> > On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding
> > wrote:
> > >
> > > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> > > > Certain laptops now come with panels that have integrated privacy
> > > > screens on them. This patch adds support for such panels by adding
> > > > a privacy-screen property to the drm_connector for the panel, that
> > > > the userspace can then use to control and check the status. The idea
> > > > was discussed here:
> > > >
> > > > https://lkml.org/lkml/2019/10/1/786
> > > >
> > > > ACPI methods are used to identify, query and control privacy screen:
> > > >
> > > > * Identifying an ACPI object corresponding to the panel: The patch
> > > > follows ACPI Spec 6.3 (available at
> > > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > > > Pages 1119 - 1123 describe what I believe, is a standard way of
> > > > identifying / addressing "display panels" in the ACPI tables, thus
> > > > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > > > to identify and attach ACPI nodes to drm connectors may be useful for
> > > > reasons other privacy-screens, in future.
> > > >
> > > > * Identifying the presence of privacy screen, and controlling it, is
> > > > done
> > > > via ACPI _DSM methods.
> > > >
> > > > Currently, this is done only for the Intel display ports. But in future,
> > > > this can be done for any other ports if the hardware becomes available
> > > > (e.g. external monitors supporting integrated privacy screens?).
> > > >
> > > > Also, this code can be extended in future to support non-ACPI methods
> > > > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > > > privacy-screen).
> > > >
> > > > Signed-off-by: Rajat Jain
> > > > ---
> > > > drivers/gpu/drm/Makefile| 1 +
> > > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> > > > drivers/gpu/drm/drm_connector.c | 38 +
> > > > drivers/gpu/drm/drm_privacy_screen.c| 176
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> > > > include/drm/drm_connector.h | 18 +++
> > > > include/drm/drm_mode_config.h | 7 +
> > > > include/drm/drm_privacy_screen.h| 33 +
> > > > 8 files changed, 281 insertions(+)
> > > > create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > > > create mode 100644 include/drm/drm_privacy_screen.h
> > >
> > > I like this much better than the prior proposal to use sysfs. However
> > > the support currently looks a bit tangled. I realize that we only have a
> > > single implementation for this in hardware right now, so there's no use
> > > in over-engineering things, but I think we can do a better job from the
> > > start without getting into too many abstractions. See below.
> > >
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 82ff826b33cc..e1fc33d69bb7 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> > > > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > > drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > >
> > > > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > > > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> > > > drm_dma.o drm_scatter.o drm_lock.o
> > > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > > drm-$(CONFIG_DRM_VM) += drm_vm.o
> > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > index 7a26bfb5329c..44131165e4ea 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > @@ -30,6 +30,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> > > > drm_connector *connector,
> > > > fence_ptr);
> > > > } else if (property == connector->max_bpc_property) {
> > > > state->max_requested_bpc = val;
> > > > + } else if (property == config->privacy_screen_property) {
> > > > + drm_privacy_screen_set_val(connector, val);
> > >
> > > This doesn't look right. Shouldn't you store the value in the connector
> > > state and then leave it up to the connector driver to set it
> > > appropriately? I think that also has the advantage of untangling this
> > > support a little.
> >
> > Hopefully this gets answered in my explanations below.
> >
> > >
> > > > } else if (connector->funcs->atomic_set_property) {
> > > > return connector->f
Re: [PATCH] drm: Add support for integrated privacy screens
On Fri, 25 Oct 2019 21:03:12 +0200
Daniel Vetter wrote:
> On Fri, Oct 25, 2019 at 1:36 PM Thierry Reding
> wrote:
> >
> > On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote:
> > > Hi,
> > >
> > > Thanks for your review and comments. Please see inline below.
> > >
> > > On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding
> > > wrote:
> > > >
> > > > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> > > > > Certain laptops now come with panels that have integrated privacy
> > > > > screens on them. This patch adds support for such panels by adding
> > > > > a privacy-screen property to the drm_connector for the panel, that
> > > > > the userspace can then use to control and check the status. The idea
> > > > > was discussed here:
> > > > >
> > > > > https://lkml.org/lkml/2019/10/1/786
> > > > >
> > > > > ACPI methods are used to identify, query and control privacy screen:
> > > > >
> > > > > * Identifying an ACPI object corresponding to the panel: The patch
> > > > > follows ACPI Spec 6.3 (available at
> > > > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > > > > Pages 1119 - 1123 describe what I believe, is a standard way of
> > > > > identifying / addressing "display panels" in the ACPI tables, thus
> > > > > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > > > > to identify and attach ACPI nodes to drm connectors may be useful for
> > > > > reasons other privacy-screens, in future.
> > > > >
> > > > > * Identifying the presence of privacy screen, and controlling it, is
> > > > > done
> > > > > via ACPI _DSM methods.
> > > > >
> > > > > Currently, this is done only for the Intel display ports. But in
> > > > > future,
> > > > > this can be done for any other ports if the hardware becomes available
> > > > > (e.g. external monitors supporting integrated privacy screens?).
> > > > >
> > > > > Also, this code can be extended in future to support non-ACPI methods
> > > > > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > > > > privacy-screen).
> > > > >
> > > > > Signed-off-by: Rajat Jain
> > > > > ---
> > > > > drivers/gpu/drm/Makefile| 1 +
> > > > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> > > > > drivers/gpu/drm/drm_connector.c | 38 +
> > > > > drivers/gpu/drm/drm_privacy_screen.c| 176
> > > > >
> > > > > drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> > > > > include/drm/drm_connector.h | 18 +++
> > > > > include/drm/drm_mode_config.h | 7 +
> > > > > include/drm/drm_privacy_screen.h| 33 +
> > > > > 8 files changed, 281 insertions(+)
> > > > > create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > > > > create mode 100644 include/drm/drm_privacy_screen.h
> > > >
> > > > I like this much better than the prior proposal to use sysfs. However
> > > > the support currently looks a bit tangled. I realize that we only have a
> > > > single implementation for this in hardware right now, so there's no use
> > > > in over-engineering things, but I think we can do a better job from the
> > > > start without getting into too many abstractions. See below.
> > > >
> > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > > index 82ff826b33cc..e1fc33d69bb7 100644
> > > > > --- a/drivers/gpu/drm/Makefile
> > > > > +++ b/drivers/gpu/drm/Makefile
> > > > > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> > > > > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > > > drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > > >
> > > > > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > > > > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o
> > > > > drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> > > > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > > > drm-$(CONFIG_DRM_VM) += drm_vm.o
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > index 7a26bfb5329c..44131165e4ea 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > > @@ -30,6 +30,7 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > >
> > > > > @@ -766,6 +767,8 @@ static int
> > > > > drm_atomic_connector_set_property(struct drm_connector *connector,
> > > > > fence_ptr);
> > > > > } else if (property == connector->max_bpc_property) {
> > > > > state->max_requested_bpc = val;
> > > > > + } else if (property == config->privacy_screen_property) {
> > > > > + drm_privacy_screen_set_val(connector, val);
> > > >
> > > > This doesn't look right. Shouldn't you store the value in the connector
> > > > state and then leave it up to the connector driver
Re: [PATCH] drm: Add support for integrated privacy screens
On Fri, Oct 25, 2019 at 1:36 PM Thierry Reding wrote:
>
> On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote:
> > Hi,
> >
> > Thanks for your review and comments. Please see inline below.
> >
> > On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding
> > wrote:
> > >
> > > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> > > > Certain laptops now come with panels that have integrated privacy
> > > > screens on them. This patch adds support for such panels by adding
> > > > a privacy-screen property to the drm_connector for the panel, that
> > > > the userspace can then use to control and check the status. The idea
> > > > was discussed here:
> > > >
> > > > https://lkml.org/lkml/2019/10/1/786
> > > >
> > > > ACPI methods are used to identify, query and control privacy screen:
> > > >
> > > > * Identifying an ACPI object corresponding to the panel: The patch
> > > > follows ACPI Spec 6.3 (available at
> > > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > > > Pages 1119 - 1123 describe what I believe, is a standard way of
> > > > identifying / addressing "display panels" in the ACPI tables, thus
> > > > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > > > to identify and attach ACPI nodes to drm connectors may be useful for
> > > > reasons other privacy-screens, in future.
> > > >
> > > > * Identifying the presence of privacy screen, and controlling it, is
> > > > done
> > > > via ACPI _DSM methods.
> > > >
> > > > Currently, this is done only for the Intel display ports. But in future,
> > > > this can be done for any other ports if the hardware becomes available
> > > > (e.g. external monitors supporting integrated privacy screens?).
> > > >
> > > > Also, this code can be extended in future to support non-ACPI methods
> > > > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > > > privacy-screen).
> > > >
> > > > Signed-off-by: Rajat Jain
> > > > ---
> > > > drivers/gpu/drm/Makefile| 1 +
> > > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> > > > drivers/gpu/drm/drm_connector.c | 38 +
> > > > drivers/gpu/drm/drm_privacy_screen.c| 176
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> > > > include/drm/drm_connector.h | 18 +++
> > > > include/drm/drm_mode_config.h | 7 +
> > > > include/drm/drm_privacy_screen.h| 33 +
> > > > 8 files changed, 281 insertions(+)
> > > > create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > > > create mode 100644 include/drm/drm_privacy_screen.h
> > >
> > > I like this much better than the prior proposal to use sysfs. However
> > > the support currently looks a bit tangled. I realize that we only have a
> > > single implementation for this in hardware right now, so there's no use
> > > in over-engineering things, but I think we can do a better job from the
> > > start without getting into too many abstractions. See below.
> > >
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 82ff826b33cc..e1fc33d69bb7 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> > > > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > > drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > > >
> > > > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > > > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> > > > drm_dma.o drm_scatter.o drm_lock.o
> > > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > > drm-$(CONFIG_DRM_VM) += drm_vm.o
> > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > index 7a26bfb5329c..44131165e4ea 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > @@ -30,6 +30,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> > > > drm_connector *connector,
> > > > fence_ptr);
> > > > } else if (property == connector->max_bpc_property) {
> > > > state->max_requested_bpc = val;
> > > > + } else if (property == config->privacy_screen_property) {
> > > > + drm_privacy_screen_set_val(connector, val);
> > >
> > > This doesn't look right. Shouldn't you store the value in the connector
> > > state and then leave it up to the connector driver to set it
> > > appropriately? I think that also has the advantage of untangling this
> > > support a little.
> >
> > Hopefully this gets answered in my explanations below.
> >
> > >
> > > > } else if (connector->funcs->atomic_set_property) {
> > > > return connector->f
Re: [PATCH] drm: Add support for integrated privacy screens
On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote:
> Hi,
>
> Thanks for your review and comments. Please see inline below.
>
> On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding
> wrote:
> >
> > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> > > Certain laptops now come with panels that have integrated privacy
> > > screens on them. This patch adds support for such panels by adding
> > > a privacy-screen property to the drm_connector for the panel, that
> > > the userspace can then use to control and check the status. The idea
> > > was discussed here:
> > >
> > > https://lkml.org/lkml/2019/10/1/786
> > >
> > > ACPI methods are used to identify, query and control privacy screen:
> > >
> > > * Identifying an ACPI object corresponding to the panel: The patch
> > > follows ACPI Spec 6.3 (available at
> > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > > Pages 1119 - 1123 describe what I believe, is a standard way of
> > > identifying / addressing "display panels" in the ACPI tables, thus
> > > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > > to identify and attach ACPI nodes to drm connectors may be useful for
> > > reasons other privacy-screens, in future.
> > >
> > > * Identifying the presence of privacy screen, and controlling it, is done
> > > via ACPI _DSM methods.
> > >
> > > Currently, this is done only for the Intel display ports. But in future,
> > > this can be done for any other ports if the hardware becomes available
> > > (e.g. external monitors supporting integrated privacy screens?).
> > >
> > > Also, this code can be extended in future to support non-ACPI methods
> > > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > > privacy-screen).
> > >
> > > Signed-off-by: Rajat Jain
> > > ---
> > > drivers/gpu/drm/Makefile| 1 +
> > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> > > drivers/gpu/drm/drm_connector.c | 38 +
> > > drivers/gpu/drm/drm_privacy_screen.c| 176
> > > drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> > > include/drm/drm_connector.h | 18 +++
> > > include/drm/drm_mode_config.h | 7 +
> > > include/drm/drm_privacy_screen.h| 33 +
> > > 8 files changed, 281 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > > create mode 100644 include/drm/drm_privacy_screen.h
> >
> > I like this much better than the prior proposal to use sysfs. However
> > the support currently looks a bit tangled. I realize that we only have a
> > single implementation for this in hardware right now, so there's no use
> > in over-engineering things, but I think we can do a better job from the
> > start without getting into too many abstractions. See below.
> >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 82ff826b33cc..e1fc33d69bb7 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> > > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > >
> > > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> > > drm_dma.o drm_scatter.o drm_lock.o
> > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > drm-$(CONFIG_DRM_VM) += drm_vm.o
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 7a26bfb5329c..44131165e4ea 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -30,6 +30,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> > > drm_connector *connector,
> > > fence_ptr);
> > > } else if (property == connector->max_bpc_property) {
> > > state->max_requested_bpc = val;
> > > + } else if (property == config->privacy_screen_property) {
> > > + drm_privacy_screen_set_val(connector, val);
> >
> > This doesn't look right. Shouldn't you store the value in the connector
> > state and then leave it up to the connector driver to set it
> > appropriately? I think that also has the advantage of untangling this
> > support a little.
>
> Hopefully this gets answered in my explanations below.
>
> >
> > > } else if (connector->funcs->atomic_set_property) {
> > > return connector->funcs->atomic_set_property(connector,
> > > state, property, val);
> > > @@ -842,6 +845,8 @@ drm_atomic_connector_get_property(struct
> > > drm_connector *connector,
> > > *val = 0;
> > > } else if (property == connect
Re: [PATCH] drm: Add support for integrated privacy screens
On Tue 2019-10-22 17:12:06, Rajat Jain wrote: > Certain laptops now come with panels that have integrated privacy > screens on them. This patch adds support for such panels by adding > a privacy-screen property to the drm_connector for the panel, that > the userspace can then use to control and check the status. The idea > was discussed here: Much better than separate /sys interface, thanks! Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany signature.asc Description: Digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Add support for integrated privacy screens
Hi,
Thanks for your review and comments. Please see inline below.
On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding wrote:
>
> On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> > Certain laptops now come with panels that have integrated privacy
> > screens on them. This patch adds support for such panels by adding
> > a privacy-screen property to the drm_connector for the panel, that
> > the userspace can then use to control and check the status. The idea
> > was discussed here:
> >
> > https://lkml.org/lkml/2019/10/1/786
> >
> > ACPI methods are used to identify, query and control privacy screen:
> >
> > * Identifying an ACPI object corresponding to the panel: The patch
> > follows ACPI Spec 6.3 (available at
> > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > Pages 1119 - 1123 describe what I believe, is a standard way of
> > identifying / addressing "display panels" in the ACPI tables, thus
> > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > to identify and attach ACPI nodes to drm connectors may be useful for
> > reasons other privacy-screens, in future.
> >
> > * Identifying the presence of privacy screen, and controlling it, is done
> > via ACPI _DSM methods.
> >
> > Currently, this is done only for the Intel display ports. But in future,
> > this can be done for any other ports if the hardware becomes available
> > (e.g. external monitors supporting integrated privacy screens?).
> >
> > Also, this code can be extended in future to support non-ACPI methods
> > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > privacy-screen).
> >
> > Signed-off-by: Rajat Jain
> > ---
> > drivers/gpu/drm/Makefile| 1 +
> > drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> > drivers/gpu/drm/drm_connector.c | 38 +
> > drivers/gpu/drm/drm_privacy_screen.c| 176
> > drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> > include/drm/drm_connector.h | 18 +++
> > include/drm/drm_mode_config.h | 7 +
> > include/drm/drm_privacy_screen.h| 33 +
> > 8 files changed, 281 insertions(+)
> > create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > create mode 100644 include/drm/drm_privacy_screen.h
>
> I like this much better than the prior proposal to use sysfs. However
> the support currently looks a bit tangled. I realize that we only have a
> single implementation for this in hardware right now, so there's no use
> in over-engineering things, but I think we can do a better job from the
> start without getting into too many abstractions. See below.
>
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 82ff826b33cc..e1fc33d69bb7 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> >
> > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> > drm_dma.o drm_scatter.o drm_lock.o
> > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > drm-$(CONFIG_DRM_VM) += drm_vm.o
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 7a26bfb5329c..44131165e4ea 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -30,6 +30,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> > drm_connector *connector,
> > fence_ptr);
> > } else if (property == connector->max_bpc_property) {
> > state->max_requested_bpc = val;
> > + } else if (property == config->privacy_screen_property) {
> > + drm_privacy_screen_set_val(connector, val);
>
> This doesn't look right. Shouldn't you store the value in the connector
> state and then leave it up to the connector driver to set it
> appropriately? I think that also has the advantage of untangling this
> support a little.
Hopefully this gets answered in my explanations below.
>
> > } else if (connector->funcs->atomic_set_property) {
> > return connector->funcs->atomic_set_property(connector,
> > state, property, val);
> > @@ -842,6 +845,8 @@ drm_atomic_connector_get_property(struct drm_connector
> > *connector,
> > *val = 0;
> > } else if (property == connector->max_bpc_property) {
> > *val = state->max_requested_bpc;
> > + } else if (property == config->privacy_screen_property) {
> > + *val = drm_privacy_screen_get_val(connector);
>
> Similarly, I think this can just return the atomic state's value
Re: [PATCH] drm: Add support for integrated privacy screens
Hi Rajat, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [cannot apply to v5.4-rc4 next-20191024] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Rajat-Jain/drm-Add-support-for-integrated-privacy-screens/20191025-020550 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f116b96685a046a89c25d4a6ba2da489145c reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry' include/linux/skbuff.h:888: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'list' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'head_frag' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'encapsulation' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_valid' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'vlan_present' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_level' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff' include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common' include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock' include/net/sock.h:515: warning: Function parameter or me
Re: [PATCH] drm: Add support for integrated privacy screens
Hi Rajat,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.4-rc4 next-20191024]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Rajat-Jain/drm-Add-support-for-integrated-privacy-screens/20191025-020550
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
f116b96685a046a89c25d4a6ba2da489145c
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/kobject.h:19,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from drivers/gpu//drm/drm_privacy_screen.c:8:
drivers/gpu//drm/drm_privacy_screen.c: In function
'drm_privacy_screen_present':
>> include/linux/kern_levels.h:5:18: warning: format '%s' expects argument of
>> type 'char *', but argument 2 has type 'struct device *' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
#define KERN_WARNING KERN_SOH "4" /* warning conditions */
^~~~
>> include/drm/drm_print.h:290:15: note: in expansion of macro 'KERN_WARNING'
printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
^
include/drm/drm_print.h:297:2: note: in expansion of macro '_DRM_PRINTK'
_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
^~~
>> drivers/gpu//drm/drm_privacy_screen.c:170:3: note: in expansion of macro
>> 'DRM_WARN'
DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
^~~~
drivers/gpu//drm/drm_privacy_screen.c:170:14: note: format string is defined
here
DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
~^
--
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/kobject.h:19,
from include/linux/of.h:17,
from include/linux/irqdomain.h:35,
from include/linux/acpi.h:13,
from drivers/gpu/drm/drm_privacy_screen.c:8:
drivers/gpu/drm/drm_privacy_screen.c: In function
'drm_privacy_screen_present':
>> include/linux/kern_levels.h:5:18: warning: format '%s' expects argument of
>> type 'char *', but argument 2 has type 'struct device *' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^
include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
#define KERN_WARNING KERN_SOH "4" /* warning conditions */
^~~~
>> include/drm/drm_print.h:290:15: note: in expansion of macro 'KERN_WARNING'
printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
^
include/drm/drm_print.h:297:2: note: in expansion of macro '_DRM_PRINTK'
_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
^~~
drivers/gpu/drm/drm_privacy_screen.c:170:3: note: in expansion of macro
'DRM_WARN'
DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
^~~~
drivers/gpu/drm/drm_privacy_screen.c:170:14: note: format string is defined
here
DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",
~^
vim +/KERN_WARNING +290 include/drm/drm_print.h
02c9656b2f0d69 Haneen Mohammed 2017-10-17 288
02c9656b2f0d69 Haneen Mohammed 2017-10-17 289 #define _DRM_PRINTK(once,
level, fmt, ...) \
db87086492581c Joe Perches 2018-03-16 @290
printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
02c9656b2f0d69 Haneen Mohammed 2017-10-17 291
:: The code at line 290 was first introduced by commit
:: db87086492581c87f768b7d17d01308153ecffc1 drm: Reduce object size of
DRM_DEV_ uses
:: TO: Joe Perches
:: CC: Daniel Vetter
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.fre
Re: [PATCH] drm: Add support for integrated privacy screens
On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> Certain laptops now come with panels that have integrated privacy
> screens on them. This patch adds support for such panels by adding
> a privacy-screen property to the drm_connector for the panel, that
> the userspace can then use to control and check the status. The idea
> was discussed here:
>
> https://lkml.org/lkml/2019/10/1/786
>
> ACPI methods are used to identify, query and control privacy screen:
>
> * Identifying an ACPI object corresponding to the panel: The patch
> follows ACPI Spec 6.3 (available at
> https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> Pages 1119 - 1123 describe what I believe, is a standard way of
> identifying / addressing "display panels" in the ACPI tables, thus
> allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> to identify and attach ACPI nodes to drm connectors may be useful for
> reasons other privacy-screens, in future.
>
> * Identifying the presence of privacy screen, and controlling it, is done
> via ACPI _DSM methods.
>
> Currently, this is done only for the Intel display ports. But in future,
> this can be done for any other ports if the hardware becomes available
> (e.g. external monitors supporting integrated privacy screens?).
>
> Also, this code can be extended in future to support non-ACPI methods
> (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> privacy-screen).
>
> Signed-off-by: Rajat Jain
> ---
> drivers/gpu/drm/Makefile| 1 +
> drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> drivers/gpu/drm/drm_connector.c | 38 +
> drivers/gpu/drm/drm_privacy_screen.c| 176
> drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> include/drm/drm_connector.h | 18 +++
> include/drm/drm_mode_config.h | 7 +
> include/drm/drm_privacy_screen.h| 33 +
> 8 files changed, 281 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> create mode 100644 include/drm/drm_privacy_screen.h
I like this much better than the prior proposal to use sysfs. However
the support currently looks a bit tangled. I realize that we only have a
single implementation for this in hardware right now, so there's no use
in over-engineering things, but I think we can do a better job from the
start without getting into too many abstractions. See below.
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 82ff826b33cc..e1fc33d69bb7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
>
> +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> drm_dma.o drm_scatter.o drm_lock.o
> drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7a26bfb5329c..44131165e4ea 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -30,6 +30,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
> fence_ptr);
> } else if (property == connector->max_bpc_property) {
> state->max_requested_bpc = val;
> + } else if (property == config->privacy_screen_property) {
> + drm_privacy_screen_set_val(connector, val);
This doesn't look right. Shouldn't you store the value in the connector
state and then leave it up to the connector driver to set it
appropriately? I think that also has the advantage of untangling this
support a little.
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -842,6 +845,8 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
> *val = 0;
> } else if (property == connector->max_bpc_property) {
> *val = state->max_requested_bpc;
> + } else if (property == config->privacy_screen_property) {
> + *val = drm_privacy_screen_get_val(connector);
Similarly, I think this can just return the atomic state's value for
this.
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4c766624b20d..a31e0382132b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/
