diff mbox series

[10/10] drm/i915: Add privacy-screen support (v2)

Message ID 20211002163618.99175-11-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series drm: Add privacy-screen class and connector properties | expand

Commit Message

Hans de Goede Oct. 2, 2021, 4:36 p.m. UTC
Add support for eDP panels with a built-in privacy screen using the
new drm_privacy_screen class.

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@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_atomic.c  |  1 +
 drivers/gpu/drm/i915/display/intel_ddi.c     |  3 +++
 drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      | 10 ++++++++++
 4 files changed, 24 insertions(+)

Comments

Ville Syrjälä Oct. 4, 2021, 3:37 p.m. UTC | #1
On Sat, Oct 02, 2021 at 06:36:18PM +0200, Hans de Goede wrote:
> Add support for eDP panels with a built-in privacy screen using the
> new drm_privacy_screen class.
> 
> 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@redhat.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/display/intel_ddi.c     |  3 +++
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      | 10 ++++++++++
>  4 files changed, 24 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 51cd0420e00e..e4496c830a35 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"
> @@ -3022,6 +3023,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)
> @@ -3247,6 +3249,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,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index e67f3207ba54..9a5dbe51458d 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>
>  #include <drm/drm_drv.h>
> @@ -12693,6 +12694,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.
> @@ -12700,6 +12703,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;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 74a657ae131a..91207310dc0d 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"
> @@ -4808,6 +4809,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;
> @@ -4902,6 +4904,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");
> +	}

I'm thinking this should go into intel_ddi_init_dp_connector()
on account of only the ddi codepaths having the
drm_connector_update_privacy_screen() calls.

Otherwise seems ok.

> +
>  	return true;
>  
>  out_vdd_off:
> -- 
> 2.31.1
Hans de Goede Oct. 4, 2021, 4:02 p.m. UTC | #2
Hi,

On 10/4/21 5:37 PM, Ville Syrjälä wrote:
> On Sat, Oct 02, 2021 at 06:36:18PM +0200, Hans de Goede wrote:
>> Add support for eDP panels with a built-in privacy screen using the
>> new drm_privacy_screen class.
>>
>> 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@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_atomic.c  |  1 +
>>  drivers/gpu/drm/i915/display/intel_ddi.c     |  3 +++
>>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
>>  drivers/gpu/drm/i915/display/intel_dp.c      | 10 ++++++++++
>>  4 files changed, 24 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 51cd0420e00e..e4496c830a35 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"
>> @@ -3022,6 +3023,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)
>> @@ -3247,6 +3249,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,
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index e67f3207ba54..9a5dbe51458d 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>
>>  #include <drm/drm_drv.h>
>> @@ -12693,6 +12694,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.
>> @@ -12700,6 +12703,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;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 74a657ae131a..91207310dc0d 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"
>> @@ -4808,6 +4809,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;
>> @@ -4902,6 +4904,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");
>> +	}
> 
> I'm thinking this should go into intel_ddi_init_dp_connector()
> on account of only the ddi codepaths having the
> drm_connector_update_privacy_screen() calls.

This should only be done in case of intel_dp_is_edp(intel_dp) returning
true. We don't have a mapping which connector has the privacy screen
(hence the NULL argument as second parameter to drm_privacy_screen_get()),
so if there are multiple DP connectors we want this to only happen on
the eDP one.

Yes this assumes that devices with a builtin privacy-screen have only
one eDP connector. For now this holds true, if this becomes not true in
the future then that is something to figure out at that point in time
(with a lot of luck the privacy-screen will be controlled through DP
by then and not through some random ACPI interface).

So I traced the call-graph and I see that we can also end up in
intel_edp_init_connector() from g4x_dp_init(), which I assume is
your main reason for requesting this change. But unless someone
retro-fits an electronic privacy-screen to the old hw supported
by g4x_dp_init() then drm_privacy_screen_get() will always
return -ENODEV, so in that case this is a no-op.

> Otherwise seems ok.

Thanks, note if you still want me to move this please let me know,
this does mean adding a intel_dp_is_edp() check to intel_ddi.c
which so far is free of these kinda checks.

Regards,

Hans
Ville Syrjälä Oct. 4, 2021, 6:48 p.m. UTC | #3
On Mon, Oct 04, 2021 at 06:02:21PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/4/21 5:37 PM, Ville Syrjälä wrote:
> > On Sat, Oct 02, 2021 at 06:36:18PM +0200, Hans de Goede wrote:
> >> Add support for eDP panels with a built-in privacy screen using the
> >> new drm_privacy_screen class.
> >>
> >> 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@redhat.com>
> >> ---
> >>  drivers/gpu/drm/i915/display/intel_atomic.c  |  1 +
> >>  drivers/gpu/drm/i915/display/intel_ddi.c     |  3 +++
> >>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
> >>  drivers/gpu/drm/i915/display/intel_dp.c      | 10 ++++++++++
> >>  4 files changed, 24 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 51cd0420e00e..e4496c830a35 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"
> >> @@ -3022,6 +3023,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)
> >> @@ -3247,6 +3249,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,
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index e67f3207ba54..9a5dbe51458d 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>
> >>  #include <drm/drm_drv.h>
> >> @@ -12693,6 +12694,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.
> >> @@ -12700,6 +12703,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;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> >> index 74a657ae131a..91207310dc0d 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"
> >> @@ -4808,6 +4809,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;
> >> @@ -4902,6 +4904,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");
> >> +	}
> > 
> > I'm thinking this should go into intel_ddi_init_dp_connector()
> > on account of only the ddi codepaths having the
> > drm_connector_update_privacy_screen() calls.
> 
> This should only be done in case of intel_dp_is_edp(intel_dp) returning
> true. We don't have a mapping which connector has the privacy screen
> (hence the NULL argument as second parameter to drm_privacy_screen_get()),
> so if there are multiple DP connectors we want this to only happen on
> the eDP one.
> 
> Yes this assumes that devices with a builtin privacy-screen have only
> one eDP connector. For now this holds true, if this becomes not true in
> the future then that is something to figure out at that point in time
> (with a lot of luck the privacy-screen will be controlled through DP
> by then and not through some random ACPI interface).
> 
> So I traced the call-graph and I see that we can also end up in
> intel_edp_init_connector() from g4x_dp_init(), which I assume is
> your main reason for requesting this change. But unless someone
> retro-fits an electronic privacy-screen to the old hw supported
> by g4x_dp_init() then drm_privacy_screen_get() will always
> return -ENODEV, so in that case this is a no-op.
> 
> > Otherwise seems ok.
> 
> Thanks, note if you still want me to move this please let me know,
> this does mean adding a intel_dp_is_edp() check to intel_ddi.c
> which so far is free of these kinda checks.

Yeah I think less confusing there with a check. And I'd probably
check the connector type rather than the encoder type. Otherwise
I suspect a year from now I'll end up wondering what happened to
the rest of the related code when I see the thing in the common 
init function.
diff mbox series

Patch

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 51cd0420e00e..e4496c830a35 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"
@@ -3022,6 +3023,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)
@@ -3247,6 +3249,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,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index e67f3207ba54..9a5dbe51458d 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>
 #include <drm/drm_drv.h>
@@ -12693,6 +12694,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.
@@ -12700,6 +12703,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;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 74a657ae131a..91207310dc0d 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"
@@ -4808,6 +4809,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;
@@ -4902,6 +4904,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: