Re: [PATCH 9/9] drm/i915: Add privacy-screen support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
> Add support for eDP panels with a built-in privacy screen using the
> new drm_privacy_screen class.
> 
> One thing which stands out here is the addition of these 2 lines to
> intel_atomic_commit_tail:
> 
>         for_each_new_connector_in_state(&state->base, connector, ...
>                 drm_connector_update_privacy_screen(connector, state);
> 
> It may seem more logical to instead take care of updating the
> privacy-screen state by marking the crtc as needing a modeset and then
> do this in both the encoder update_pipe (for fast-sets) and enable
> (for full modesets) callbacks. But ATM these callbacks only get passed
> the new connector_state and these callbacks are all called after
> drm_atomic_helper_swap_state() at which point there is no way to get
> the old state from the new state.

I was going to suggest that you workaround this simply by adding a variable
that corresponds to the most recently committed privacy screen state somewhere
in a driver private structure. But, then I realized that's basically the same
as what you're doing now except that your current solution stores said state
in a shared struct. So, I think you probably do have the right idea here as
long as we don't get any non-ACPI providers in the future. This also seems
like something that wouldn't be difficult to fixup down the line if that ends
up changing.

> 
> Without access to the old state, we do not know if the sw_state of
> the privacy-screen has changes so we would need to call
> drm_privacy_screen_set_sw_state() unconditionally. This is undesirable
> since all current known privacy-screen providers use ACPI calls which
> are somewhat expensive to make.
> 
> Also, as all providers use ACPI calls, rather then poking GPU registers,
> there is no need to order this together with other encoder operations.
> Since no GPU poking is involved having this as a separate step of the
> commit process actually is the logical thing to do.
> 
> Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  5 +++++
>  drivers/gpu/drm/i915/display/intel_dp.c      | 10 ++++++++++
>  drivers/gpu/drm/i915/i915_pci.c              | 12 ++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 5560d2f4c352..7285873d329a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -10140,6 +10140,8 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
>         struct drm_device *dev = state->base.dev;
>         struct drm_i915_private *dev_priv = to_i915(dev);
>         struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> +       struct drm_connector_state *new_connector_state;
> +       struct drm_connector *connector;
>         struct intel_crtc *crtc;
>         u64 put_domains[I915_MAX_PIPES] = {};
>         intel_wakeref_t wakeref = 0;
> @@ -10237,6 +10239,9 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
>                         intel_color_load_luts(new_crtc_state);
>         }
>  
> +       for_each_new_connector_in_state(&state->base, connector,
> new_connector_state, i)
> +               drm_connector_update_privacy_screen(connector, &state-
> >base);
> +
>         /*
>          * Now that the vblank has passed, we can go ahead and program the
>          * optimal watermarks on platforms that need two-step watermark
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7f8e8865048f..3aa2072cccf6 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -37,6 +37,7 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_privacy_screen_consumer.h>
>  #include <drm/drm_probe_helper.h>
>  
>  #include "g4x_dp.h"
> @@ -5217,6 +5218,7 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
>         struct drm_connector *connector = &intel_connector->base;
>         struct drm_display_mode *fixed_mode = NULL;
>         struct drm_display_mode *downclock_mode = NULL;
> +       struct drm_privacy_screen *privacy_screen;
>         bool has_dpcd;
>         enum pipe pipe = INVALID_PIPE;
>         struct edid *edid;
> @@ -5308,6 +5310,14 @@ static bool intel_edp_init_connector(struct intel_dp
> *intel_dp,
>                                 fixed_mode->hdisplay, fixed_mode->vdisplay);
>         }
>  
> +       privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
> +       if (!IS_ERR(privacy_screen)) {
> +               drm_connector_attach_privacy_screen_provider(connector,
> +                                                           
> privacy_screen);
> +       } else if (PTR_ERR(privacy_screen) != -ENODEV) {
> +               drm_warn(&dev_priv->drm, "Error getting privacy-screen\n");
> +       }
> +
>         return true;
>  
>  out_vdd_off:
> diff --git a/drivers/gpu/drm/i915/i915_pci.c
> b/drivers/gpu/drm/i915/i915_pci.c
> index 146f7e39182a..d6913f567a1c 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -25,6 +25,7 @@
>  #include <linux/vga_switcheroo.h>
>  
>  #include <drm/drm_drv.h>
> +#include <drm/drm_privacy_screen_consumer.h>
>  #include <drm/i915_pciids.h>
>  
>  #include "i915_drv.h"
> @@ -1167,6 +1168,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>  {
>         struct intel_device_info *intel_info =
>                 (struct intel_device_info *) ent->driver_data;
> +       struct drm_privacy_screen *privacy_screen;
>         int err;
>  
>         if (intel_info->require_force_probe &&
> @@ -1195,7 +1197,17 @@ static int i915_pci_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>         if (vga_switcheroo_client_probe_defer(pdev))
>                 return -EPROBE_DEFER;
>  
> +       /*
> +        * We do not handle -EPROBE_DEFER further into the probe process, so
> +        * check if we have a laptop-panel privacy-screen for which the
> driver
> +        * has not loaded yet here.
> +        */
> +       privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL);
> +       if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -
> EPROBE_DEFER)
> +               return -EPROBE_DEFER;
> +
>         err = i915_driver_probe(pdev, ent);
> +       drm_privacy_screen_put(privacy_screen);
>         if (err)
>                 return err;
>  

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux