| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAA8EJppkZTRApQbuEUACw=r2kBH+321_f1ZqRf_-veEuwB56zg@mail.gmail.com>
Date: Fri, 11 Feb 2022 00:12:32 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
Rob Clark <robdclark@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>, Sean Paul <sean@...rly.run>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Stephen Boyd <swboyd@...omium.org>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
intel-gfx@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/2] drm: Add HPD state to drm_connector_oob_hotplug_event()
On Thu, 10 Feb 2022 at 23:54, Bjorn Andersson
<bjorn.andersson@...aro.org> wrote:
>
> On Tue 08 Feb 02:39 PST 2022, Greg Kroah-Hartman wrote:
>
> > On Mon, Feb 07, 2022 at 08:43:27PM -0800, Bjorn Andersson wrote:
> > > In some implementations, such as the Qualcomm platforms, the display
> > > driver has no way to query the current HPD state and as such it's
> > > impossible to distinguish between disconnect and attention events.
> > >
> > > Add a parameter to drm_connector_oob_hotplug_event() to pass the HPD
> > > state.
> > >
> > > Also push the test for unchanged state in the displayport altmode driver
> > > into the i915 driver, to allow other drivers to act upon each update.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > > ---
> > >
> > > Note that the Intel driver has only been compile tested with this patch.
> > >
> > > drivers/gpu/drm/drm_connector.c | 6 ++++--
> > > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++++++++++---
> > > drivers/gpu/drm/i915/i915_drv.h | 3 +++
> > > drivers/usb/typec/altmodes/displayport.c | 9 ++-------
> > > include/drm/drm_connector.h | 5 +++--
> > > 5 files changed, 23 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index a50c82bc2b2f..ad7295597c0f 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -2825,6 +2825,7 @@ struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > > /**
> > > * drm_connector_oob_hotplug_event - Report out-of-band hotplug event to connector
> > > * @connector_fwnode: fwnode_handle to report the event on
> > > + * @hpd_state: number of data lanes available
> >
> > "number"?
> >
> > > *
> > > * On some hardware a hotplug event notification may come from outside the display
> > > * driver / device. An example of this is some USB Type-C setups where the hardware
> > > @@ -2834,7 +2835,8 @@ struct drm_connector *drm_connector_find_by_fwnode(struct fwnode_handle *fwnode)
> > > * This function can be used to report these out-of-band events after obtaining
> > > * a drm_connector reference through calling drm_connector_find_by_fwnode().
> > > */
> > > -void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
> > > +void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode,
> > > + bool hpd_state)
> >
> > This is a boolean, how can it be a number?
> >
>
> The kerneldoc wasn't appropriately updated as this went from being
> "number of data lanes" to "the hot plug detect (hpd) state".
>
> > And having a "flag" like this is a pain, how do you know what the
> > parameter really means?
> >
>
> You're right, "state" isn't a boolean property, let's rename it
> "hpd_high" to clarify it.
"connected" ?
>
> > > {
> > > struct drm_connector *connector;
> > >
> > > @@ -2843,7 +2845,7 @@ void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode)
> > > return;
> > >
> > > if (connector->funcs->oob_hotplug_event)
> > > - connector->funcs->oob_hotplug_event(connector);
> > > + connector->funcs->oob_hotplug_event(connector, hpd_state);
> > >
> > > drm_connector_put(connector);
> > > }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 146b83916005..00520867d37b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -4816,15 +4816,23 @@ static int intel_dp_connector_atomic_check(struct drm_connector *conn,
> > > return intel_modeset_synced_crtcs(state, conn);
> > > }
> > >
> > > -static void intel_dp_oob_hotplug_event(struct drm_connector *connector)
> > > +static void intel_dp_oob_hotplug_event(struct drm_connector *connector, bool hpd_state)
> > > {
> > > struct intel_encoder *encoder = intel_attached_encoder(to_intel_connector(connector));
> > > struct drm_i915_private *i915 = to_i915(connector->dev);
> > > + bool need_work = false;
> > >
> > > spin_lock_irq(&i915->irq_lock);
> > > - i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> > > + if (hpd_state != i915->hotplug.oob_hotplug_state) {
> > > + i915->hotplug.event_bits |= BIT(encoder->hpd_pin);
> > > +
> > > + i915->hotplug.oob_hotplug_state = hpd_state;
> > > + need_work = true;
> > > + }
> > > spin_unlock_irq(&i915->irq_lock);
> > > - queue_delayed_work(system_wq, &i915->hotplug.hotplug_work, 0);
> > > +
> > > + if (need_work)
> > > + queue_delayed_work(system_wq, &i915->hotplug.hotplug_work, 0);
> > > }
> > >
> > > static const struct drm_connector_funcs intel_dp_connector_funcs = {
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8c1706fd81f9..543ebf1cfcf4 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -149,6 +149,9 @@ struct i915_hotplug {
> > > /* Whether or not to count short HPD IRQs in HPD storms */
> > > u8 hpd_short_storm_enabled;
> > >
> > > + /* Last state reported by oob_hotplug_event */
> > > + bool oob_hotplug_state;
> > > +
> > > /*
> > > * if we get a HPD irq from DP and a HPD irq from non-DP
> > > * the non-DP HPD could block the workqueue on a mode config
> > > diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> > > index c1d8c23baa39..a4596be4d34a 100644
> > > --- a/drivers/usb/typec/altmodes/displayport.c
> > > +++ b/drivers/usb/typec/altmodes/displayport.c
> > > @@ -59,7 +59,6 @@ struct dp_altmode {
> > > struct typec_displayport_data data;
> > >
> > > enum dp_state state;
> > > - bool hpd;
> > >
> > > struct mutex lock; /* device lock */
> > > struct work_struct work;
> > > @@ -143,10 +142,7 @@ static int dp_altmode_status_update(struct dp_altmode *dp)
> > > if (!ret)
> > > dp->state = DP_STATE_CONFIGURE;
> > > } else {
> > > - if (dp->hpd != hpd) {
> > > - drm_connector_oob_hotplug_event(dp->connector_fwnode);
> > > - dp->hpd = hpd;
> > > - }
> > > + drm_connector_oob_hotplug_event(dp->connector_fwnode, hpd);
> > > }
> > >
> > > return ret;
> > > @@ -573,8 +569,7 @@ void dp_altmode_remove(struct typec_altmode *alt)
> > > cancel_work_sync(&dp->work);
> > >
> > > if (dp->connector_fwnode) {
> > > - if (dp->hpd)
> > > - drm_connector_oob_hotplug_event(dp->connector_fwnode);
> > > + drm_connector_oob_hotplug_event(dp->connector_fwnode, false);
> >
> > See, what does "false" here mean?
> >
> > Name the function for what it does, do not have random flags as
> > parameters, that makes it impossible to understand what the code is
> > doing when you are reading it, without having to jump around and figure
> > out what the flags are saying.
> >
> > And here they just don't even seem to be right :(
> >
>
> Both the old and new code will signal to the DRM driver that the cable
> was removed, the change is that we're carrying the level in the call
> rather than just indicating that the state has changed.
>
> We could introduce some HPD_HIGH/HPD_LOW defines to make it easier to
> read. But the various places I'm looking at just represented the hpd
> state as a bool.
>
> Regards,
> Bjorn
--
With best wishes
Dmitry
Powered by blists - more mailing lists