On Fri, Oct 25, 2019 at 4:36 AM Thierry Reding <thierry.red...@gmail.com> 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 <thierry.red...@gmail.com>
> > 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 <raja...@google.com>
> > > > ---
> > > > 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 <drm/drm_atomic.h>
> > > > #include <drm/drm_print.h>
> > > > #include <drm/drm_drv.h>
> > > > +#include <drm/drm_privacy_screen.h>
> > > > #include <drm/drm_writeback.h>
> > > > #include <drm/drm_vblank.h>
> > > >
> > > > @@ -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 for
> > > this.
> >
> > I did think about having a state variable in software to get and set
> > this. However, I think it is not very far fetched that some platforms
> > may have "hardware kill" switches that allow hardware to switch
> > privacy-screen on and off directly, in addition to the software
> > control that we are implementing. Privacy is a touchy subject in
> > enterprise, and anything that reduces the possibility of having any
> > inconsistency between software state and hardware state is desirable.
> > So in this case, I chose to not have a state in software about this -
> > we just report the hardware state everytime we are asked for it.
>
> So this doesn't really work with atomic KMS, then. The main idea behind
> atomic KMS is that you apply a configuration either completely or not at
> all. So at least for setting this property you'd have to go through the
> state object.
>
> Now, for reading out the property you might be able to get away with the
> above. I'm not sure if that's enough to keep the state up-to-date,
> though. Is there some way for a kill switch to trigger an interrupt or
> other event of some sort so that the state could be kept up-to-date?
I was basically imagining a hardware that I don't have at the moment.
So I do not know the answer, but I think the answer may be yes.
>
> Daniel (or anyone else), do you know of any precedent for state that
> might get modified behind the atomic helpers' back? Seems to me like we
> need to find some point where we can actually read back the current
> "hardware value" of this privacy screen property and store that back
> into the state.
>
I'll wait for suggestions.
What I did not quite understand about the use of a local state in
software is what does this local software state buy us? Even with
local state, in the "set_property" flow, we'll update it at the same
time when we flush it out to hardware. Is it about avoiding an ACPI
call during the "get_property" flow? Also, is it worth it if i brings
with it complexity of interrupt handling?
> >
> > >
> > > > } 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/drivers/gpu/drm/drm_connector.c
> > > > @@ -821,6 +821,11 @@ static const struct drm_prop_enum_list
> > > > drm_panel_orientation_enum_list[] = {
> > > > { DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, "Right Side Up" },
> > > > };
> > > >
> > > > +static const struct drm_prop_enum_list drm_privacy_screen_enum_list[]
> > > > = {
> > > > + { DRM_PRIVACY_SCREEN_DISABLED, "Disabled" },
> > > > + { DRM_PRIVACY_SCREEN_ENABLED, "Enabled" },
> > > > +};
> > > > +
> > > > static const struct drm_prop_enum_list drm_dvi_i_select_enum_list[] = {
> > > > { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and
> > > > TV-out */
> > > > { DRM_MODE_SUBCONNECTOR_DVID, "DVI-D" }, /* DVI-I */
> > > > @@ -2253,6 +2258,39 @@ static void drm_tile_group_free(struct kref
> > > > *kref)
> > > > kfree(tg);
> > > > }
> > > >
> > > > +/**
> > > > + * drm_connector_init_privacy_screen_property -
> > > > + * create and attach the connecter's privacy-screen property.
> > > > + * @connector: connector for which to init the privacy-screen property.
> > > > + *
> > > > + * This function creates and attaches the "privacy-screen" property to
> > > > the
> > > > + * connector. Initial state of privacy-screen is set to disabled.
> > > > + *
> > > > + * Returns:
> > > > + * Zero on success, negative errno on failure.
> > > > + */
> > > > +int drm_connector_init_privacy_screen_property(struct drm_connector
> > > > *connector)
> > > > +{
> > > > + struct drm_device *dev = connector->dev;
> > > > + struct drm_property *prop;
> > > > +
> > > > + prop = dev->mode_config.privacy_screen_property;
> > > > + if (!prop) {
> > > > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> > > > + "privacy-screen",
> > > > drm_privacy_screen_enum_list,
> > >
> > > Seems to me like the -screen suffix here is somewhat redundant. Yes, the
> > > thing that we enable/disable may be called a "privacy screen", but the
> > > property that we enable/disable on the connector is the "privacy" of the
> > > user. I'd reflect that in all the related variable names and so on as
> > > well.
> >
> > IMHO a property called "privacy" may be a little generic for the users
> > to understand what it is. For e.g. when I started looking at code, I
> > found the "Content Protection" property and I got confused thinking
> > may be it provides something similar to what I'm trying to do. I think
> > "privacy-screen" conveys the complete context without being long, so
> > there is no confusion or ambiguity. But I don't mind changing it if a
> > property "privacy" is what people think is better to convey what it
> > is, as long as it is clear to user.
>
> This being a property of a display connector it doesn't seem very
> ambiguous to me what this is. How this ends up being presented to the
> users is mostly orthogonal anyway. We've got a bunch of properties whose
> purpose may not be clear to the average user. The properties, while they
> are UABI, don't typically face the user directly. They're still part of
> an API, so as long as they are properly documented there shouldn't be
> any ambiguities.
>
OK, I do not mind changing to whatever is acceptable. Jani / Daniel,
do you have any recommendation here?
> > >
> > > > + ARRAY_SIZE(drm_privacy_screen_enum_list));
> > > > + if (!prop)
> > > > + return -ENOMEM;
> > > > +
> > > > + dev->mode_config.privacy_screen_property = prop;
> > > > + }
> > > > +
> > > > + drm_object_attach_property(&connector->base, prop,
> > > > + DRM_PRIVACY_SCREEN_DISABLED);
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_init_privacy_screen_property);
> > > > +
> > > > /**
> > > > * drm_mode_put_tile_group - drop a reference to a tile group.
> > > > * @dev: DRM device
> > > > diff --git a/drivers/gpu/drm/drm_privacy_screen.c
> > > > b/drivers/gpu/drm/drm_privacy_screen.c
> > > > new file mode 100644
> > > > index 000000000000..1d68e8aa6c5f
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/drm_privacy_screen.c
> > > > @@ -0,0 +1,176 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * DRM privacy Screen code
> > > > + *
> > > > + * Copyright © 2019 Google Inc.
> > > > + */
> > > > +
> > > > +#include <linux/acpi.h>
> > > > +#include <linux/pci.h>
> > > > +
> > > > +#include <drm/drm_connector.h>
> > > > +#include <drm/drm_device.h>
> > > > +#include <drm/drm_print.h>
> > > > +
> > > > +#define DRM_CONN_DSM_REVID 1
> > > > +
> > > > +#define DRM_CONN_DSM_FN_PRIVACY_GET_STATUS 1
> > > > +#define DRM_CONN_DSM_FN_PRIVACY_ENABLE 2
> > > > +#define DRM_CONN_DSM_FN_PRIVACY_DISABLE 3
> > > > +
> > > > +static const guid_t drm_conn_dsm_guid =
> > > > + GUID_INIT(0xC7033113, 0x8720, 0x4CEB,
> > > > + 0x90, 0x90, 0x9D, 0x52, 0xB3, 0xE5, 0x2D, 0x73);
> > > > +
> > > > +/*
> > > > + * Makes _DSM call to set privacy screen status or get privacy screen.
> > > > Return
> > > > + * value matters only for PRIVACY_GET_STATUS case. Returns 0 if
> > > > disabled, 1 if
> > > > + * enabled.
> > > > + */
> > > > +static int acpi_privacy_screen_call_dsm(acpi_handle conn_handle, u64
> > > > func)
> > > > +{
> > > > + union acpi_object *obj;
> > > > + int ret = 0;
> > > > +
> > > > + obj = acpi_evaluate_dsm(conn_handle, &drm_conn_dsm_guid,
> > > > + DRM_CONN_DSM_REVID, func, NULL);
> > > > + if (!obj) {
> > > > + DRM_DEBUG_DRIVER("failed to evaluate _DSM for fn %llx\n",
> > > > func);
> > > > + /* Can't do much. For get_val, assume privacy_screen
> > > > disabled */
> > > > + goto done;
> > > > + }
> > > > +
> > > > + if (func == DRM_CONN_DSM_FN_PRIVACY_GET_STATUS &&
> > > > + obj->type == ACPI_TYPE_INTEGER)
> > > > + ret = !!obj->integer.value;
> > > > +done:
> > > > + ACPI_FREE(obj);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +void drm_privacy_screen_set_val(struct drm_connector *connector,
> > > > + enum drm_privacy_screen val)
> > > > +{
> > > > + acpi_handle acpi_handle = connector->privacy_screen_handle;
> > > > +
> > > > + if (!acpi_handle)
> > > > + return;
> > > > +
> > > > + if (val == DRM_PRIVACY_SCREEN_DISABLED)
> > > > + acpi_privacy_screen_call_dsm(acpi_handle,
> > > > +
> > > > DRM_CONN_DSM_FN_PRIVACY_DISABLE);
> > > > + else if (val == DRM_PRIVACY_SCREEN_ENABLED)
> > > > + acpi_privacy_screen_call_dsm(acpi_handle,
> > > > +
> > > > DRM_CONN_DSM_FN_PRIVACY_ENABLE);
> > > > +}
> > > > +
> > > > +enum drm_privacy_screen drm_privacy_screen_get_val(struct drm_connector
> > > > + *connector)
> > > > +{
> > > > + acpi_handle acpi_handle = connector->privacy_screen_handle;
> > > > +
> > > > + if (acpi_handle &&
> > > > + acpi_privacy_screen_call_dsm(acpi_handle,
> > > > +
> > > > DRM_CONN_DSM_FN_PRIVACY_GET_STATUS))
> > > > + return DRM_PRIVACY_SCREEN_ENABLED;
> > > > +
> > > > + return DRM_PRIVACY_SCREEN_DISABLED;
> > > > +}
> > > > +
> > > > +/*
> > > > + * See ACPI Spec v6.3, Table B-2, "Display Type" for details.
> > > > + * In short, these macros define the base _ADR values for ACPI nodes
> > > > + */
> > > > +#define ACPI_BASE_ADR_FOR_OTHERS (0ULL << 8)
> > > > +#define ACPI_BASE_ADR_FOR_VGA (1ULL << 8)
> > > > +#define ACPI_BASE_ADR_FOR_TV (2ULL << 8)
> > > > +#define ACPI_BASE_ADR_FOR_EXT_MON (3ULL << 8)
> > > > +#define ACPI_BASE_ADR_FOR_INTEGRATED (4ULL << 8)
> > > > +
> > > > +#define ACPI_DEVICE_ID_SCHEME (1ULL << 31)
> > > > +#define ACPI_FIRMWARE_CAN_DETECT (1ULL << 16)
> > > > +
> > > > +/*
> > > > + * Ref: ACPI Spec 6.3
> > > > + *
> > > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
> > > > + * Pages 1119 - 1123 describe, what I believe, a standard way of
> > > > + * identifying / addressing "display panels" in the ACPI. Thus it
> > > > provides
> > > > + * a way for the ACPI to define devices for the display panels attached
> > > > + * to the system. It thus provides a way for the BIOS to export any
> > > > panel
> > > > + * specific properties to the system via ACPI (like device trees).
> > > > + *
> > > > + * The following function looks up the ACPI node for a connector and
> > > > links
> > > > + * to it. Technically it is independent from the privacy_screen code,
> > > > and
> > > > + * ideally may be called for all connectors. It is generally a good
> > > > idea to
> > > > + * be able to attach an ACPI node to describe anything if needed.
> > > > (This can
> > > > + * help in future for other panel specific features maybe). However, it
> > > > + * needs a "port index" which I believe is the index within a
> > > > particular
> > > > + * type of port (Ref to the pages of spec mentioned above). This port
> > > > index
> > > > + * unfortunately is not available in DRM code, so currently its call is
> > > > + * originated from i915 driver.
> > > > + */
> > > > +static int drm_connector_attach_acpi_node(struct drm_connector
> > > > *connector,
> > > > + u8 port_index)
> > > > +{
> > > > + struct device *dev = &connector->dev->pdev->dev;
> > > > + struct acpi_device *conn_dev;
> > > > + u64 conn_addr;
> > > > +
> > > > + /*
> > > > + * Determine what _ADR to look for, depending on device type and
> > > > + * port number. Potentially we only care about the
> > > > + * eDP / integrated displays?
> > > > + */
> > > > + switch (connector->connector_type) {
2) > > > > + case DRM_MODE_CONNECTOR_eDP:
> > > > + conn_addr = ACPI_BASE_ADR_FOR_INTEGRATED + port_index;
> > > > + break;
> > > > + case DRM_MODE_CONNECTOR_VGA:
> > > > + conn_addr = ACPI_BASE_ADR_FOR_VGA + port_index;
> > > > + break;
> > > > + case DRM_MODE_CONNECTOR_TV:
> > > > + conn_addr = ACPI_BASE_ADR_FOR_TV + port_index;
> > > > + break;
> > > > + case DRM_MODE_CONNECTOR_DisplayPort:
> > > > + conn_addr = ACPI_BASE_ADR_FOR_EXT_MON + port_index;
> > > > + break;
> > > > + default:
> > > > + return -ENOTSUPP;
> > > > + }
> > > > +
> > > > + conn_addr |= ACPI_DEVICE_ID_SCHEME;
> > > > + conn_addr |= ACPI_FIRMWARE_CAN_DETECT;
> > > > +
> > > > + DRM_DEV_DEBUG(dev, "%s: Finding drm_connector ACPI node at
> > > > _ADR=%llX\n",
> > > > + __func__, conn_addr);
> > > > +
> > > > + /* Look up the connector device, under the PCI device */
> > > > + conn_dev = acpi_find_child_device(ACPI_COMPANION(dev),
> > > > + conn_addr, false);
> > > > + if (!conn_dev)
> > > > + return -ENODEV;
> > > > +
> > > > + connector->privacy_screen_handle = conn_dev->handle;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +bool drm_privacy_screen_present(struct drm_connector *connector, u8
> > > > port_index)
> > >
> > > This is the main part that I think is a little tangled. This is a very
> > > specific implementation that hides in a generic API.
> >
> > I agree that this is an ACPI specific implementation, but other than
> > that, I think it does not have any driver specific details. More
> > detailed thoughts on this below.
>
> Well, the port_index kind of ties this to i915 because that uses this
> concept. Other drivers may not.
>
> Also, I'm wondering if you couldn't somehow derive the port_index from
> the connector. If all this does is to find an ACPI node corresponding to
> a connector, shouldn't the connector really be all that you need?
>
Yes, I agree that finding an ACPI node corresponding to a connector,
should require only the info that the connector already has.
> > > I we store the privacy setting in the atomic state, there isn't really a
> > > reason to store the privacy handle in the connector. Instead it could be
> > > simply stored in the driver that supports this.
> > >
> > > Ideally I think we'd have a very small drm_privacy_screen object type
> > > that would just wrap this, but perhaps we don't need that right away,
> > > given that we only have a single implementation so far.
> >
> > Yes, agreed.
> >
> > >
> > > However, I think if we just pushed this specific implementation into the
> > > drivers that'd help pave the way for something more generic later on
> > > without a lot of extra work up front.
> > >
> > > For example you could turn the drm_connector_attach_acpi_node() into a
> > > helper that simply returns the ACPI handle, something like this perhaps:
> > >
> > > struct acpi_handle *drm_acpi_find_privacy_screen(struct
> > > drm_connector *connector,
> > > unsigned int
> > > port)
> > > {
> > > ...
> > > }
> >
> > Yes, I like that idea of making it a helper function. In fact, finding
> > an ACPI node for the connector doesn't have to do anything with
> > privacy screen (so it can be used for other purposes also, in future).
>
> Looks like I misunderstood the purpose of that function. You store the
> ACPI handle as connector->privacy_screen_handle, so I was assuming that
> it was getting an ACPI handle for some privacy screen subdevice.
>
> > > That the i915 driver would then call and store the returned value
> > > internally. When it commits the atomic state for the connector it can
> > > then call the drm_acpi_set_privacy() (I think that'd be a better name
> > > for your drm_privacy_screen_set_val()) by passing that handle and the
> > > value from the atomic state.
> > >
> > > The above has the advantage that we don't clutter the generic core with
> > > something that's not at all generic. If eventually we see that these
> > > types of privacy screens are implemented in more device we can always
> > > refactor this into something really generic and maybe even decide to put
> > > it into the drm_connector directly.
> >
> > This is where I think I'm in slight disagreement. I think the
> > functionality we're adding is still "generic", just that the only
> > hardware *I have today* to test is using Intel eDP ports. But I don't
> > see why AMD CPU laptops can't have it (For E.g. HP's Elitebook 745 G5
> > seems to use AMD and has integrated privacy screen feature
> > http://www8.hp.com/h20195/V2/GetPDF.aspx/4aa7-2802eee) .
> > My worry is that if we don't make it generic today, we might see
> > duplicate / similar-but-different / different ways of this in other
> > places (e.g.
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=110ea1d833ad)
> > because unless it is generic to start with, it is difficult for some
> > one to change later for the fear of breaking hardware that is already
> > in market given that
> > * hardware may not be available to new developer to test for
> > regressions (also there is very little motivation to check any
> > hardware other than your own).
> > * specially for a code that relies on firmware ACPI (firmware
> > upgrades in field are always costly).
> >
> > My understanding is that we're adding 2 functionalities here:
> >
> > 1) Identify and Attach ACPI node to DRM connector. Since this is
> > following ACPI spec, I think this is generic enough.
>
> It's probably worth making this a separate patch in that case. If the
> ACPI handle really represents the connector itself, then it seems fine
> to store it in the connector. But it shouldn't be called privacy_screen
> in that case.
Yes, I agree.
>
> > 2) Use ACPI _DSM mthods to identify screen, set and get values. This
> > is where I think we're setting (generic) expectations for the ACPI
> > methods in how they should behave if ACPI is to be used to control
> > privacy screen. If we put this in generic code today, future
> > developers can look at this to understand how their ACPI for new
> > platforms is to behave if they want to use this generic code. If we
> > put it in i915 specific code, this will be seen as driver specific
> > behavior and developers may choose some other behavior in their
> > driver.
>
> I think it's fine to put this functionality into generic code. What I
> don't think is good to do is to have this code called from generic code.
> It's opt-in functionality that drivers should call if they know that the
> connector has an associated ACPI handle that can be used for privacy
> screen control.
Ah, OK, I see where the disconnect was. Sure, I think we are converging now.
>
> After reading the patch again and realizing that you're not actually
> dealing with an ACPI handle to the privacy screen directly but one for
> the connector, I think this is okay. You do in fact call into this from
> the i915 only. I still don't think the naming is great, and it'd be nice
> to see ACPI as part of the function name to make that explicit. We could
> always address that at a later point, but may as well do it right from
> the start.
Sure, I agree and will change the naming.
>
> > I agree that the functionality we're adding is ACPI specific (today -
> > but can be extended to gpio in future for non x86 platforms), but not
> > necessarily driver specific. Actually the only reason, I had to call
> > the drm_privacy_screen_present() (and the
> > drm_init_privacy_screen_property()) function from i915 code is because
> > we need a port_index to lookup ACPI node. If we had that available in
> > drm code, we wouldn't need to call anything from i915 at all.
>
> You're kind of proving my point about this API being driver-specific, or
> at least ACPI specific. Non-ACPI devices (maybe even non-i915 devices?)
> may not have a concept of a port index. Encoding this in the API makes
> the API non-generic.
>
> As I mentioned above, if we could derive the port index from the
> connector, that'd be much better. Could you perhaps use drm_connector's
> index field?
That's a good idea. I'll check it out to see if it matches my needs.
If it does, I think we might have found a way to make it totally
separate from i915. I don't have a vast range of test hardware and
monitors etc to test though. So I'll test with what I have available.
I'll keep you updated.
>
> Unless there's a way to reliably detect this type of functionality from
> generic code, I think it should always be called from the driver.
>
> > So, for the reasons stated above, IMHO it is better to retain this
> > functionality in drm code instead of i915 driver. But I'm new to the
> > drm / i915 code, and would be happy to change my code if people have
> > strong opinions about this. Let me know.
>
> Maybe I was being unclear. I wasn't arguing that all the code should be
> moved into the i915 driver. All I was saying that instead of storing the
> ACPI handle inside struct drm_connector, we should maybe store it inside
> the i915 driver's connector structure. struct drm_connector is a very
> generic concept and each and every connector object on every platform
> will get that ACPI handle pointer if you add it there. I don't think an
> ACPI handle belongs there. For example, on ARM SoCs it's common to have
> connectors be backed by a struct device (or struct platform_device more
> specifically). But we don't put that information into drm_connector
> because PCI graphics adapters don't have a struct device that represents
> the connector.
OK, I understand what you are saying. The ACPI handle essentially
denotes the firmware node for that drm_connector device, analogous to
the device tree nodes. Ideally, I wanted to have it stored in the
drm_connector->kdev->fwnode which I think was the best place as it
would also make the ACPI node visible in the sysfs like it is visible
for PCI devices today. Unfortunately I found that the
drm_connector->kdev is not created at the time when I need to lookup
and register privacy screen (kdev is created much later), so that is
not an option. Note that the drm_privacy_screen_set_val() and
drm_privacy_screen_get_val() will need to access this ACPI handle to
do their jobs. So unless I move that code or the property representing
privacy screen also into i915, I can't really put that handle in i915.
Thanks,
Rajat
>
> Thierry
>
> >
> > Thanks & Best Regards,
> >
> > Rajat
> >
> > >
> > > > +{
> > > > + acpi_handle handle;
> > > > +
> > > > + if (drm_connector_attach_acpi_node(connector, port_index))
> > > > + return false;
> > > > +
> > > > + handle = connector->privacy_screen_handle;
> > > > + if (!acpi_check_dsm(handle, &drm_conn_dsm_guid,
> > > > + DRM_CONN_DSM_REVID,
> > > > + 1 << DRM_CONN_DSM_FN_PRIVACY_GET_STATUS |
> > > > + 1 << DRM_CONN_DSM_FN_PRIVACY_ENABLE |
> > > > + 1 << DRM_CONN_DSM_FN_PRIVACY_DISABLE)) {
> > > > + DRM_WARN("%s: Odd, connector ACPI node but no privacy
> > > > scrn?\n",
> > > > + connector->dev->dev);
> > > > + return false;
> > > > + }
> > > > + DRM_DEV_INFO(connector->dev->dev, "supports privacy screen\n");
> > > > + return true;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 57e9f0ba331b..3ff3962d27db 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -39,6 +39,7 @@
> > > > #include <drm/drm_dp_helper.h>
> > > > #include <drm/drm_edid.h>
> > > > #include <drm/drm_hdcp.h>
> > > > +#include <drm/drm_privacy_screen.h>
> > > > #include <drm/drm_probe_helper.h>
> > > > #include <drm/i915_drm.h>
> > > >
> > > > @@ -6354,6 +6355,8 @@ intel_dp_add_properties(struct intel_dp
> > > > *intel_dp, struct drm_connector *connect
> > > >
> > > > connector->state->scaling_mode = DRM_MODE_SCALE_ASPECT;
> > > >
> > > > + if (drm_privacy_screen_present(connector, port - PORT_A))
> > > > +
> > > > drm_connector_init_privacy_screen_property(connector);
> > > > }
> > > > }
> > > >
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index 681cb590f952..63b8318bd68c 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -225,6 +225,20 @@ enum drm_link_status {
> > > > DRM_LINK_STATUS_BAD = DRM_MODE_LINK_STATUS_BAD,
> > > > };
> > > >
> > > > +/**
> > > > + * enum drm_privacy_screen - privacy_screen status
> > > > + *
> > > > + * This enum is used to track and control the state of the privacy
> > > > screen.
> > > > + * There are no separate #defines for the uapi!
> > > > + *
> > > > + * @DRM_PRIVACY_SCREEN_DISABLED: The privacy-screen on the panel is
> > > > disabled
> > > > + * @DRM_PRIVACY_SCREEN_ENABLED: The privacy-screen on the panel is
> > > > enabled
> > > > + */
> > > > +enum drm_privacy_screen {
> > > > + DRM_PRIVACY_SCREEN_DISABLED = 0,
> > > > + DRM_PRIVACY_SCREEN_ENABLED = 1,
> > > > +};
> > > > +
> > >
> > > Shouldn't this go into include/uapi/drm/drm_mode.h? That would have the
> > > advantage of giving userspace symbolic names to use when setting the
> > > property.
> > >
> > > Maybe also rename these to something like:
> > >
> > > #define DRM_MODE_PRIVACY_DISABLED 0
> > > #define DRM_MODE_PRIVACY_ENABLED 1
> > >
> > > for consistency with other properties.
> > >
> > > Thierry
> > >
> > > > /**
> > > > * enum drm_panel_orientation - panel_orientation info for
> > > > &drm_display_info
> > > > *
> > > > @@ -1410,6 +1424,9 @@ struct drm_connector {
> > > >
> > > > /** @hdr_sink_metadata: HDR Metadata Information read from sink */
> > > > struct hdr_sink_metadata hdr_sink_metadata;
> > > > +
> > > > + /* Handle used by privacy screen code */
> > > > + void *privacy_screen_handle;
> > > > };
> > > >
> > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> > > > @@ -1543,6 +1560,7 @@ int drm_connector_init_panel_orientation_property(
> > > > struct drm_connector *connector, int width, int height);
> > > > int drm_connector_attach_max_bpc_property(struct drm_connector
> > > > *connector,
> > > > int min, int max);
> > > > +int drm_connector_init_privacy_screen_property(struct drm_connector
> > > > *connector);
> > > >
> > > > /**
> > > > * struct drm_tile_group - Tile group metadata
> > > > diff --git a/include/drm/drm_mode_config.h
> > > > b/include/drm/drm_mode_config.h
> > > > index 3bcbe30339f0..6d5d23da90d4 100644
> > > > --- a/include/drm/drm_mode_config.h
> > > > +++ b/include/drm/drm_mode_config.h
> > > > @@ -813,6 +813,13 @@ struct drm_mode_config {
> > > > */
> > > > struct drm_property *panel_orientation_property;
> > > >
> > > > + /**
> > > > + * @privacy_screen_property: Optional connector property to
> > > > indicate
> > > > + * and control the state (enabled / disabled) of privacy-screen
> > > > on the
> > > > + * panel, if present.
> > > > + */
> > > > + struct drm_property *privacy_screen_property;
> > > > +
> > > > /**
> > > > * @writeback_fb_id_property: Property for writeback connectors,
> > > > storing
> > > > * the ID of the output framebuffer.
> > > > diff --git a/include/drm/drm_privacy_screen.h
> > > > b/include/drm/drm_privacy_screen.h
> > > > new file mode 100644
> > > > index 000000000000..c589bbc47656
> > > > --- /dev/null
> > > > +++ b/include/drm/drm_privacy_screen.h
> > > > @@ -0,0 +1,33 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +/*
> > > > + * Copyright © 2019 Google Inc.
> > > > + */
> > > > +
> > > > +#ifndef __DRM_PRIVACY_SCREEN_H__
> > > > +#define __DRM_PRIVACY_SCREEN_H__
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > +bool drm_privacy_screen_present(struct drm_connector *connector, u8
> > > > port);
> > > > +void drm_privacy_screen_set_val(struct drm_connector *connector,
> > > > + enum drm_privacy_screen val);
> > > > +enum drm_privacy_screen drm_privacy_screen_get_val(struct drm_connector
> > > > + *connector);
> > > > +#else
> > > > +static inline bool drm_privacy_screen_present(struct drm_connector
> > > > *connector,
> > > > + u8 port)
> > > > +{
> > > > + return false;
> > > > +}
> > > > +
> > > > +void drm_privacy_screen_set_val(struct drm_connector *connector,
> > > > + enum drm_privacy_screen val)
> > > > +{ }
> > > > +
> > > > +enum drm_privacy_screen drm_privacy_screen_get_val(
> > > > + struct drm_connector *connector)
> > > > +{
> > > > + return DRM_PRIVACY_SCREEN_DISABLED;
> > > > +}
> > > > +#endif /* CONFIG_ACPI */
> > > > +
> > > > +#endif /* __DRM_PRIVACY_SCREEN_H__ */
> > > > --
> > > > 2.23.0.866.gb869b98d4c-goog
> > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
