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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 09, 2022 at 04:36:44PM +0200, Maxime Ripard wrote:
> 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@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?

Nah. Feel free slap on
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
to the lot if you don't think a resend is needed otherwise.

-- 
Ville Syrjälä
Intel