From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9B48EECAAA1 for ; Fri, 9 Sep 2022 14:50:05 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0710C10EDEF; Fri, 9 Sep 2022 14:50:04 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0375D10EDEF for ; Fri, 9 Sep 2022 14:49:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1662734999; x=1694270999; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=pivWd/RwR9gJTu2sG5/ppSoUV5oC5p9Z0YLn/ZoXKNY=; b=XKCmEoDIGzn9BXk+odcj8uEWVZ2/5bPrAmSs+PPgBhesasZQfNfqPnRb zkb7zxyAjQe6S6Lk2Ww7ZsdJJyOZaAp/IoWArTKqvCBiOEOXegQY7BVJf WNeQEkotaeAmAZ+UIw870VdxgnRqZo7w5hrflRjjUixSuCeAUw5aTBn5h snWn9Zu204ajhnwTkaknPcZvKCzjlpGL40w/02+BIoto3cUrcFmp+pcsP AitUQqiRUfMZFJCo9VTJy+kbgecw62ANpZ6cWvJ9qsM0psfD/43DiJ7LC sUpuCXxXv0VgnXSJqPD7Nf4ohZ052ykjIdVuNfmzbXrDXOFoUmxkNN/qI g==; X-IronPort-AV: E=McAfee;i="6500,9779,10465"; a="298828456" X-IronPort-AV: E=Sophos;i="5.93,303,1654585200"; d="scan'208";a="298828456" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Sep 2022 07:49:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.93,303,1654585200"; d="scan'208";a="592629723" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.191]) by orsmga006.jf.intel.com with SMTP; 09 Sep 2022 07:49:54 -0700 Received: by stinkbox (sSMTP sendmail emulation); Fri, 09 Sep 2022 17:49:53 +0300 Date: Fri, 9 Sep 2022 17:49:53 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Subject: Re: [PATCH v4 4/8] drm/vc4: hdmi: Simplify the hotplug handling Message-ID: References: <20220829134731.213478-1-maxime@cerno.tech> <20220829134731.213478-5-maxime@cerno.tech> <20220909143644.izsfwanwn7xq5hvi@houat> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220909143644.izsfwanwn7xq5hvi@houat> X-Patchwork-Hint: comment X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Dom Cobley , Tim Gover , Dave Stevenson , David Airlie , dri-devel@lists.freedesktop.org, Thomas Zimmermann , Daniel Vetter , Phil Elwell Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 > > > --- > > > 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ä to the lot if you don't think a resend is needed otherwise. -- Ville Syrjälä Intel