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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 34C36C54E6A for ; Mon, 18 Mar 2024 16:11:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=zNVAgntzA3PjvxteNdyMiYt34S7cLBpNB4rBkU/nbqk=; b=FsLAS8zvwsGAGs k3FMegIfsfp+wDCTs5s5OKZFjZCW/wHJOdZcnlHh8rNkzKbBnM8XdXida6kCVB/f69qyvjgDrYPp7 rvGvaGVKegfAMjxNYvqyz7l4ZbKEncloK5BToA3bHMcdn1GtXWDfecAPPcvy/6QSWHHNG6nQsQMtE 11q8EfAF94XZO3GJjgiz69YiS/XCDL3TqFSO4XCZZMvc4rfDsaDsMF7QMpJqbFPh9Df7QSc17Y7Sr xb5oyGP0S0u7TJkA5VpKW6lrgjepYWVKBo1Hl9XJjsEcyb2URG1r+a5eQ7QRlQnUUHMX/yeE6c/ey Sm7D3DnHqkiqJtp3vfng==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rmFaP-00000009AOR-3mhV; Mon, 18 Mar 2024 16:11:41 +0000 Received: from mgamail.intel.com ([198.175.65.21]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rmFaM-00000009ANB-1uM5; Mon, 18 Mar 2024 16:11:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1710778298; x=1742314298; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=84xfgGQdhPYLNJtOqDpwXG2/hc97FrIjHbfrfTzN6qs=; b=kaX0kFNnWUc1eAyNhyCJWDvMd8hydTtjYl0xxJp+AU7aS5LMUOR1SHLa 8/UDyuj8xGdtnU+nkQTuHnpLX9l4YquZFTSPsXWURmVIX1rS33DA7+TWU 6lAc6kJIsisIYbrTnJ/ANORaW+5y2WBpTd0hSOPofUQ+sLhpy+arxI0BQ MrdUT7RVJEg2XiQQ6UGsx3jzez9q6bDrJUCfmWn9r1Qfy8TMsxjITViUs pvyUEOhyDDweByUltMA71RPbb6sBRqVIc6dcgtWFiH6CINd7IGWOBQCBV gJE5cKl+DfOdyT/id+tejEDH8j96xYkbHXMMqdpOtvd8gU8j4L9MjQU/Q g==; X-IronPort-AV: E=McAfee;i="6600,9927,11017"; a="5536455" X-IronPort-AV: E=Sophos;i="6.07,134,1708416000"; d="scan'208";a="5536455" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orvoesa113.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Mar 2024 09:11:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,11017"; a="827781855" X-IronPort-AV: E=Sophos;i="6.07,134,1708416000"; d="scan'208";a="827781855" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga001.jf.intel.com with SMTP; 18 Mar 2024 09:11:31 -0700 Received: by stinkbox (sSMTP sendmail emulation); Mon, 18 Mar 2024 18:11:30 +0200 Date: Mon, 18 Mar 2024 18:11:30 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Cc: Maarten Lankhorst , Thomas Zimmermann , David Airlie , Daniel Vetter , Jonathan Corbet , Sandy Huang , Heiko =?iso-8859-1?Q?St=FCbner?= , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Hans Verkuil , Sebastian Wick , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-sunxi@lists.linux.dev Subject: Re: [PATCH v9 20/27] drm/connector: hdmi: Add Infoframes generation Message-ID: References: <20240311-kms-hdmi-connector-state-v9-0-d45890323344@kernel.org> <20240311-kms-hdmi-connector-state-v9-20-d45890323344@kernel.org> <20240318-abstract-myna-of-exercise-adfcde@houat> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240318-abstract-myna-of-exercise-adfcde@houat> X-Patchwork-Hint: comment X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240318_091139_173708_B30B749D X-CRM114-Status: GOOD ( 42.20 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Mar 18, 2024 at 02:49:47PM +0100, Maxime Ripard wrote: > Hi, > = > On Fri, Mar 15, 2024 at 10:22:05AM +0200, Ville Syrj=E4l=E4 wrote: > > On Mon, Mar 11, 2024 at 03:49:48PM +0100, Maxime Ripard wrote: > > > Infoframes in KMS is usually handled by a bunch of low-level helpers > > > that require quite some boilerplate for drivers. This leads to > > > discrepancies with how drivers generate them, and which are actually > > > sent. > > > = > > > Now that we have everything needed to generate them in the HDMI > > > connector state, we can generate them in our common logic so that > > > drivers can simply reuse what we precomputed. > > > = > > > Signed-off-by: Maxime Ripard > > > --- > > > drivers/gpu/drm/Kconfig | 1 + > > > drivers/gpu/drm/drm_atomic_state_helper.c | 323 +++++++++++= ++++++++++ > > > drivers/gpu/drm/drm_connector.c | 14 + > > > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 1 + > > > drivers/gpu/drm/tests/drm_connector_test.c | 12 + > > > include/drm/drm_atomic_state_helper.h | 8 + > > > include/drm/drm_connector.h | 133 +++++++++ > > > 7 files changed, 492 insertions(+) > > > = > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > > index 872edb47bb53..ad9c467e20ce 100644 > > > --- a/drivers/gpu/drm/Kconfig > > > +++ b/drivers/gpu/drm/Kconfig > > > @@ -97,10 +97,11 @@ config DRM_KUNIT_TEST > > > If in doubt, say "N". > > > = > > > config DRM_KMS_HELPER > > > tristate > > > depends on DRM > > > + select DRM_DISPLAY_HDMI_HELPER > > > help > > > CRTC helpers for KMS drivers. > > > = > > > config DRM_DEBUG_DP_MST_TOPOLOGY_REFS > > > bool "Enable refcount backtrace history in the DP MST helper= s" > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/= drm/drm_atomic_state_helper.c > > > index e66272c0d006..2bf53666fc9d 100644 > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > @@ -36,10 +36,12 @@ > > > #include > > > #include > > > #include > > > #include > > > = > > > +#include > > > + > > > #include > > > #include > > > = > > > /** > > > * DOC: atomic state reset and initialization > > > @@ -912,10 +914,143 @@ hdmi_compute_config(const struct drm_connector= *connector, > > > } > > > = > > > return -EINVAL; > > > } > > > = > > > +static int hdmi_generate_avi_infoframe(const struct drm_connector *c= onnector, > > > + struct drm_connector_state *state) > > > +{ > > > + const struct drm_display_mode *mode =3D > > > + connector_state_get_mode(state); > > > + struct drm_connector_hdmi_infoframe *infoframe =3D > > > + &state->hdmi.infoframes.avi; > > > + struct hdmi_avi_infoframe *frame =3D > > > + &infoframe->data.avi; > > > + bool is_full_range =3D state->hdmi.is_full_range; > > > + enum hdmi_quantization_range rgb_quant_range =3D > > > + is_full_range ? HDMI_QUANTIZATION_RANGE_FULL : HDMI_QUANTIZATION_R= ANGE_LIMITED; > > > + int ret; > > > + > > > + ret =3D drm_hdmi_avi_infoframe_from_display_mode(frame, connector, = mode); > > > + if (ret) > > > + return ret; > > > + > > > + frame->colorspace =3D state->hdmi.output_format; > > > + > > > + drm_hdmi_avi_infoframe_quant_range(frame, connector, mode, rgb_quan= t_range); > > = > > drm_hdmi_avi_infoframe_quant_range() doesn't handle YCbCr currently. > = > I guess it's not really a problem anymore if we drop YUV422 selection, > but I'll add a comment. > = > > > + drm_hdmi_avi_infoframe_colorimetry(frame, state); > > > + drm_hdmi_avi_infoframe_bars(frame, state); > > > + > > > + infoframe->set =3D true; > > > + > > > + return 0; > > > +} > > > + > > > > > + > > > +#define UPDATE_INFOFRAME(c, os, ns, i) \ > > > + write_or_clear_infoframe(c, \ > > > + &(c)->hdmi.infoframes.i, \ > > > + &(os)->hdmi.infoframes.i, \ > > > + &(ns)->hdmi.infoframes.i) > > = > > This macro feels like pointless obfuscation to me. > = > I'll remove it then. > = > > > > > @@ -1984,20 +2063,73 @@ struct drm_connector { > > > = > > > /** > > > * @hdmi: HDMI-related variable and properties. > > > */ > > > struct { > > > +#define DRM_CONNECTOR_HDMI_VENDOR_LEN 8 > > > + /** > > > + * @vendor: HDMI Controller Vendor Name > > > + */ > > > + unsigned char vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] __nonstring; > > > + > > > +#define DRM_CONNECTOR_HDMI_PRODUCT_LEN 16 > > > + /** > > > + * @product: HDMI Controller Product Name > > > + */ > > > + unsigned char product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] __nonstring; > > > + > > > /** > > > * @supported_formats: Bitmask of @hdmi_colorspace > > > * supported by the controller. > > > */ > > > unsigned long supported_formats; > > > = > > > /** > > > * @funcs: HDMI connector Control Functions > > > */ > > > const struct drm_connector_hdmi_funcs *funcs; > > > + > > > + /** > > > + * @infoframes: Current Infoframes output by the connector > > > + */ > > > + struct { > > > + /** > > > + * @lock: Mutex protecting against concurrent access to > > > + * the infoframes, most notably between KMS and ALSA. > > > + */ > > > + struct mutex lock; > > > + > > > + /** > > > + * @audio: Current Audio Infoframes structure. Protected > > > + * by @lock. > > > + */ > > > + struct drm_connector_hdmi_infoframe audio; > > > + > > > + /** > > > + * @avi: Current AVI Infoframes structure. Protected by > > > + * @lock. > > > + */ > > > + struct drm_connector_hdmi_infoframe avi; > > > + > > > + /** > > > + * @hdr_drm: Current DRM (Dynamic Range and Mastering) > > > + * Infoframes structure. Protected by @lock. > > > + */ > > > + struct drm_connector_hdmi_infoframe hdr_drm; > > > + > > > + /** > > > + * @spd: Current SPD Infoframes structure. Protected by > > > + * @lock. > > > + */ > > > + struct drm_connector_hdmi_infoframe spd; > > > + > > > + /** > > > + * @vendor: Current HDMI Vendor Infoframes structure. > > > + * Protected by @lock. > > > + */ > > > + struct drm_connector_hdmi_infoframe hdmi; > > > + } infoframes; > > > } hdmi; > > = > > What's the deal with this bloat? These are already tracked in the > > connector's state so this looks entirely redundant. > = > The next patch in this series is about adding debugfs entries to read > the infoframes, and thus we need to care about concurrency between > debugfs files accesses and commits. Copying the things we care about > from the state to the entity is the typical solution for that, but I > guess we could also take the proper locks and access the current > connector state. Yeah, just lock and dump the latest state. That is the only thing that should of interest to anyone in userspace. Also are you actually adding some kind of ad-hoc state dump things just for these? Why not do whatever is needed to include them in the normal .atomic_state_print() stuff? -- = Ville Syrj=E4l=E4 Intel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel