[Intel-gfx] [PATCH 10/10] drm/i915: Add privacy-screen support (v3)

[Rajat Jain]

Hello Hans,

I'm working on my platform's privacy-screen support based on your
patches, and had some (I know late) questions. Would be great if you
could please help answer. Please see inline.

On Tue, Oct 5, 2021 at 1:25 PM Hans de Goede <hdegoede at redhat.com> wrote:
>
> Add support for eDP panels with a built-in privacy screen using the
> new drm_privacy_screen class.
>
> Changes in v3:
> - Move drm_privacy_screen_get() call to intel_ddi_init_dp_connector()
>
> Changes in v2:
> - Call drm_connector_update_privacy_screen() from
>   intel_enable_ddi_dp() / intel_ddi_update_pipe_dp() instead of adding a
>   for_each_new_connector_in_state() loop to intel_atomic_commit_tail()
> - Move the probe-deferral check to the intel_modeset_probe_defer() helper
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/display/intel_ddi.c     | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
>  3 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index b4e7ac51aa31..a62550711e98 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -139,6 +139,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
>             new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
>             new_conn_state->base.content_type != old_conn_state->base.content_type ||
>             new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||
> +           new_conn_state->base.privacy_screen_sw_state != old_conn_state->base.privacy_screen_sw_state ||
>             !drm_connector_atomic_hdr_metadata_equal(old_state, new_state))
>                 crtc_state->mode_changed = true;
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 0d4cf7fa8720..272714e07cc6 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -25,6 +25,7 @@
>   *
>   */
>
> +#include <drm/drm_privacy_screen_consumer.h>
>  #include <drm/drm_scdc_helper.h>
>
>  #include "i915_drv.h"
> @@ -2946,6 +2947,7 @@ static void intel_enable_ddi_dp(struct intel_atomic_state *state,
>         if (port == PORT_A && DISPLAY_VER(dev_priv) < 9)
>                 intel_dp_stop_link_train(intel_dp, crtc_state);
>
> +       drm_connector_update_privacy_screen(conn_state);
>         intel_edp_backlight_on(crtc_state, conn_state);
>
>         if (!dig_port->lspcon.active || dig_port->dp.has_hdmi_sink)
> @@ -3161,6 +3163,7 @@ static void intel_ddi_update_pipe_dp(struct intel_atomic_state *state,
>         intel_drrs_update(intel_dp, crtc_state);
>
>         intel_backlight_update(state, encoder, crtc_state, conn_state);
> +       drm_connector_update_privacy_screen(conn_state);
>  }
>
>  void intel_ddi_update_pipe(struct intel_atomic_state *state,
> @@ -3979,6 +3982,19 @@ intel_ddi_init_dp_connector(struct intel_digital_port *dig_port)
>                 return NULL;
>         }
>
> +       if (dig_port->base.type == INTEL_OUTPUT_EDP) {
> +               struct drm_device *dev = dig_port->base.base.dev;
> +               struct drm_privacy_screen *privacy_screen;
> +
> +               privacy_screen = drm_privacy_screen_get(dev->dev, NULL);

Why pass NULL for con_id? Can we pass something more meaningful (e.g.
"eDP-1") so that the non-KMS platform components that provide the
privacy-screen can provide a more specific lookup? Or is that
information (connector name) not available at the time this call is
being made?

Thanks,

Rajat


> +               if (!IS_ERR(privacy_screen)) {
> +                       drm_connector_attach_privacy_screen_provider(&connector->base,
> +                                                                    privacy_screen);
> +               } else if (PTR_ERR(privacy_screen) != -ENODEV) {
> +                       drm_warn(dev, "Error getting privacy-screen\n");
> +               }
> +       }
> +
>         return connector;
>  }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 86dbe366a907..84715a779d9d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -42,6 +42,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_plane_helper.h>
> +#include <drm/drm_privacy_screen_consumer.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_rect.h>
>
> @@ -12769,6 +12770,8 @@ void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915)
>
>  bool intel_modeset_probe_defer(struct pci_dev *pdev)
>  {
> +       struct drm_privacy_screen *privacy_screen;
> +
>         /*
>          * apple-gmux is needed on dual GPU MacBook Pro
>          * to probe the panel if we're the inactive GPU.
> @@ -12776,6 +12779,13 @@ bool intel_modeset_probe_defer(struct pci_dev *pdev)
>         if (vga_switcheroo_client_probe_defer(pdev))
>                 return true;
>
> +       /* If the LCD panel has a privacy-screen, wait for it */
> +       privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL);
> +       if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER)
> +               return true;
> +
> +       drm_privacy_screen_put(privacy_screen);
> +
>         return false;
>  }
>
> --
> 2.31.1
>