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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxx>
to the lot if you don't think a resend is needed otherwise.
--
Ville Syrjälä
Intel
