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?
Thanks!
Maxime
Attachment:
signature.asc
Description: PGP signature
