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