[PATCH v4 4/8] drm/vc4: hdmi: Simplify the hotplug handling

[Maxime Ripard]

Hi Ville

Thanks for your review

On Mon, Sep 05, 2022 at 08:38:11PM +0300, Ville Syrjälä wrote:
> On Mon, Aug 29, 2022 at 03:47:27PM +0200, Maxime Ripard wrote:
> > Our detect callback has a bunch of operations to perform depending on
> > the current and last status of the connector, such a setting the CEC
> > physical address or enabling the scrambling again.
> > 
> > This is currently dealt with a bunch of if / else statetements that make
> > it fairly difficult to read and extend.
> > 
> > Let's move all that logic to a function of its own.
> > 
> > Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 66 ++++++++++++++++++++++------------
> >  1 file changed, 44 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index b786645eaeb7..9fad57ebdd11 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -273,17 +273,53 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
> >  
> >  static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder);
> >  
> > +static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
> > +				    enum drm_connector_status status)
> > +{
> > +	struct drm_connector *connector = &vc4_hdmi->connector;
> > +	struct edid *edid;
> > +
> > +	/*
> > +	 * NOTE: This function should really be called with
> > +	 * vc4_hdmi->mutex held, but doing so results in reentrancy
> > +	 * issues since cec_s_phys_addr_from_edid might call
> > +	 * .adap_enable, which leads to that funtion being called with
> > +	 * our mutex held.
> > +	 *
> > +	 * Concurrency isn't an issue at the moment since we don't share
> > +	 * any state with any of the other frameworks so we can ignore
> > +	 * the lock for now.
> > +	 */
> > +
> > +	if (status == connector->status)
> > +		return;
> 
> This looks to have a change in behaviour to not call
> vc4_hdmi_enable_scrambling() unless a change in the
> connector status was detected. The previous code called
> it regarless.

Yeah, good point

> When I was doing the i915 stuff I did have a sink that
> left hpd asserted while turned off, and when powering
> back up it instead generated a pulse on the hpd line.
> Not sure if such a pulse is always slow enough that
> you can reasonably guarantee a detection of both edges...
> 
> Apart from that (and the cec locking mess that I dared
> not even look at) the rest of the series looks OK to me.

Can I add your R-B and remove the check you mentioned above while
applying, or would you prefer that I send a new version?

Thanks!
Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220909/38e9d36d/attachment-0001.sig>