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

[Hans de Goede <hdegoede@redhat.com>]

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