On Fri, Oct 25, 2019 at 4:36 AM Thierry Reding <thierry.reding@xxxxxxxxx> 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.reding@xxxxxxxxx> 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 <rajatja@xxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
