diff mbox series

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

Message ID 20211005202322.700909-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. 5, 2021, 8:23 p.m. UTC
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@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(+)

Comments

Ville Syrjala Oct. 13, 2021, 9:59 p.m. UTC | #1
On Tue, Oct 05, 2021 at 10:23:22PM +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 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@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) {

Connector type check would be a bit more consistent with what this is
about I think. But there's is 1:1 correspondence with the encoder type
for eDP so not a particularly important point.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		struct drm_device *dev = dig_port->base.base.dev;
> +		struct drm_privacy_screen *privacy_screen;
> +
> +		privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
> +		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
Rajat Jain Nov. 3, 2021, 11:16 p.m. UTC | #2
Hello Hans,

Thanks a lot for working on this diligently and getting almost all of
it finally merged!

On Wed, Oct 13, 2021 at 2:59 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Oct 05, 2021 at 10:23:22PM +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 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@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) {
>
> Connector type check would be a bit more consistent with what this is
> about I think. But there's is 1:1 correspondence with the encoder type
> for eDP so not a particularly important point.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I see only 8 out of 10 patches in this series were applied to drm-tip.
I'm curious if there is any reason for which the last 2 patches were
not applied:

[Patch 9/10]: drm/i915: Add intel_modeset_probe_defer() helper
[Patch 10/10]: drm/i915: Add privacy-screen support (v3)

I look forward to getting them merged so that I can use them.

Thanks & Best regards,

Rajat

>
> > +             struct drm_device *dev = dig_port->base.base.dev;
> > +             struct drm_privacy_screen *privacy_screen;
> > +
> > +             privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
> > +             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
>
> --
> Ville Syrjälä
> Intel
Hans de Goede Nov. 4, 2021, 8:56 a.m. UTC | #3
Hi,

On 11/4/21 00:16, Rajat Jain wrote:
> Hello Hans,
> 
> Thanks a lot for working on this diligently and getting almost all of
> it finally merged!
> 
> On Wed, Oct 13, 2021 at 2:59 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>>
>> On Tue, Oct 05, 2021 at 10:23:22PM +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 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@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) {
>>
>> Connector type check would be a bit more consistent with what this is
>> about I think. But there's is 1:1 correspondence with the encoder type
>> for eDP so not a particularly important point.
>>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I see only 8 out of 10 patches in this series were applied to drm-tip.
> I'm curious if there is any reason for which the last 2 patches were
> not applied:
> 
> [Patch 9/10]: drm/i915: Add intel_modeset_probe_defer() helper
> [Patch 10/10]: drm/i915: Add privacy-screen support (v3)
> 
> I look forward to getting them merged so that I can use them.

The main-parts of the patch-set were merged through drm-misc-next,
the 2 i915 patches had a conflict there since the series was
based on drm-tip and some of the surrounding i915 code had some
small changes in drm-intel-next which was not in drm-misc-next yet.

Once drm-intel-next merges in recent drm-misc-next changes 
(after the merge window closes) I will push the remaining 2 patches
through drm-intel-next and then everything will be in drm-tip and
on its way to 5.17 .

Regards,

Hans



>>> +             struct drm_device *dev = dig_port->base.base.dev;
>>> +             struct drm_privacy_screen *privacy_screen;
>>> +
>>> +             privacy_screen = drm_privacy_screen_get(dev->dev, NULL);
>>> +             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
>>
>> --
>> Ville Syrjälä
>> Intel
>
Rajat Jain Nov. 17, 2021, 1:59 p.m. UTC | #4
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@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@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
>
Hans de Goede Nov. 17, 2021, 4:18 p.m. UTC | #5
Hi Rajat,

On 11/17/21 14:59, Rajat Jain wrote:
> 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@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@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?

For the x86 ACPI case it does not matter because the static lookups
added by drivers/gpu/drm/drm_privacy_screen_x86.c do not set
a con_id in the lookup and if the lookup lack a con_id then
the value passed to drm_privacy_screen_get() is a no-op.

So, if it helps you to pass a connector-name then go for it.

As for the connecter_name already being set at this point,
I don't know, you need to check.

But also see below.

>> +               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;
>>  }

So this is also going to be an interesting challenge for your device-tree (ish)
case. We cannot delay returning the -EPROBE_DEFER until the code in
intel_ddi_init_dp_connector() gets called because much of the i915 code is not
ready to deal with tearing down the house again once we are at that point (AFAIK).

For now I guess you/we could just hardcode "eDP-1" here. That is likely going
to be correct in all relevant cases (for now).

Regards,

Hans
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 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);
+		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;
 }