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 08074CD6E6B for ; Thu, 4 Jun 2026 06:51:55 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B791710E3C6; Thu, 4 Jun 2026 06:51:54 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="cxixIQaa"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.16]) by gabe.freedesktop.org (Postfix) with ESMTPS id 19D4210E3C3; Thu, 4 Jun 2026 06:51:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1780555913; x=1812091913; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=pNJTO+/N4E17hWTL5NAwNJ2+OxzFkMqNRUuYWcWt4PA=; b=cxixIQaarG6vtX6Ib9+xF8YAhedGCFacHWiyJpGOofWT8TvYoh9SYtL6 jgIE9yz6eiZ6f85N2bdCwtJQXtx8CRYADEgjTygipy9e57r2Covnl5BN8 wySHG2yuVpO0WL1ZQowypibriZeZQiSf0+/rD/DUMGVDxmDqcyiYBiMiO +r93KJiCIh3UA480Dj36I/32VcC/xDcRvMj3LL52LMgFWFFMC/vRQouc6 82zqmc2qXdJuV7+4p+2dIsUnWdtwozwmRD/0Ns6Fa+0jgWgL/vxhFoNsB 37rTS7qz5JLXYW7loQ5/Bw8l25yEbh4QVjWkHTmBj+6HtKNormK5OoBbz g==; X-CSE-ConnectionGUID: CB+sorZpQPyI7ZLkXvpSnA== X-CSE-MsgGUID: ifw7UVE6SumWLTMNRoNlnA== X-IronPort-AV: E=McAfee;i="6800,10657,11806"; a="81557421" X-IronPort-AV: E=Sophos;i="6.24,186,1774335600"; d="scan'208";a="81557421" Received: from orviesa005.jf.intel.com ([10.64.159.145]) by orvoesa108.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Jun 2026 23:51:53 -0700 X-CSE-ConnectionGUID: DH0acMbPTTa35yc/dksYmQ== X-CSE-MsgGUID: TbPBt3j+SRaDobaOpBqq3Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.24,186,1774335600"; d="scan'208";a="249385724" Received: from vpanait-mobl.ger.corp.intel.com (HELO localhost) ([10.245.245.33]) by orviesa005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Jun 2026 23:51:51 -0700 From: Jani Nikula To: =?utf-8?Q?Micha=C5=82?= Grzelak , intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org Cc: Suraj Kandpal , =?utf-8?Q?Micha=C5=82?= Grzelak Subject: Re: [PATCH v6 5/8] drm/i915: override LT's VS/PE when requested In-Reply-To: <20260603230544.1993439-6-michal.grzelak@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs Bertel Jungin Aukio 5, 02600 Espoo, Finland References: <20260603230544.1993439-1-michal.grzelak@intel.com> <20260603230544.1993439-6-michal.grzelak@intel.com> Date: Thu, 04 Jun 2026 09:51:48 +0300 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Thu, 04 Jun 2026, Micha=C5=82 Grzelak wrote: > Add accessor function for LT to read requested table from VBT #57. > Parse the requested table and transform data into port's buffer. > > Add helper for checking if devdata is safe for dereference. Proceed with > default values if not. > > Add helper to check if VS/PE-O buffer has been allocated during > allocate_vswing_preemph_override(). Proceed with default values if not. > > LT's VS/PE-O tables have less columns than xe3plpd_lt_phy_buf_trans > contains fields. Thus copy txswing and txswing_level from default VS/PE > values onto VS/PE-O tables. > > Use 6th table if encoder supports DP 2.0 or higher. Otherwise use 5th > table for DP. > > There are no changes to intel_ddi_dp_level() since selection of correct > row of intel_ddi_buf_trans_entry is same as when no override request has > been done. > > Tables 1-4 are not used at all and are most likely to be zeroed. 5th > table is used for any mode below DP 2.0 (exclusive). 6th table is used > for any mode above DP 2.0 (inclusive). > > Indices for other tables have not yet been observed to be used as of > now. > > v5->v6 > - remove drm_WARN_ONCE (Suraj) > - pass default VS/PE tables to LT's VBT accessor (Suraj) > - set txswing & _level from default VS/PE tables (Suraj) > - add helper checking if VS/PE-O has been allocated (Suraj) > - check if devdata is not NULL > > v4->v5 > - add if-ladder instead of function pointer > - blend index computation with table parsing > - remove WARN and debug messages > - remove enums entirely > - add spaces around operators (Suraj) > - remove spaces after type casting (Suraj) > - remove INTEL_DISPLAY_STATE_WARN (Suraj) > > v3->v4 > - stick to solely changing VBT data into current structures (Jani) > - move iterator declaration to declaration block (Suraj) > > v2->v3 > - remove unnecessary braces from if block (Suraj) > - return -EINVAL instead of -1 (Suraj) > > Signed-off-by: Micha=C5=82 Grzelak > --- > drivers/gpu/drm/i915/display/intel_bios.c | 40 +++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_bios.h | 6 +++ > .../drm/i915/display/intel_ddi_buf_trans.c | 37 ++++++++++++++++- > 3 files changed, 82 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/= i915/display/intel_bios.c > index bc48ed9a7cbf5..302a9465a637b 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -3846,6 +3846,11 @@ int intel_bios_hdmi_ddc_pin(const struct intel_bio= s_encoder_data *devdata) > return map_ddc_pin(devdata->display, devdata->child.ddc_pin); > } >=20=20 > +bool intel_bios_encoder_allocated_vspeo(const struct intel_bios_encoder_= data *devdata) > +{ > + return !!devdata->vspeo; > +} Please don't add this function. Just return NULL from get call below and handle it in the caller. > + > bool intel_bios_encoder_requests_vspeo(const struct intel_bios_encoder_d= ata *devdata) > { > return devdata->display->vbt.version >=3D 218 && devdata->child.use_vbt= _vswing; > @@ -3861,6 +3866,41 @@ bool intel_bios_encoder_supports_tbt(const struct = intel_bios_encoder_data *devda > return devdata->display->vbt.version >=3D 209 && devdata->child.tbt; > } >=20=20 > +const struct intel_ddi_buf_trans * > +intel_bios_get_lt_vspeo(const struct intel_bios_encoder_data *devdata, > + const struct intel_ddi_buf_trans *buf_trans, > + int idx) > +{ > + struct intel_display *display =3D devdata->display; > + struct intel_ddi_buf_trans *vspeo =3D (void *)devdata->vspeo; > + union intel_ddi_buf_trans_entry *entries =3D (void *)vspeo->entries; What's with the casts? > + const u32 *tables =3D display->vbt.vspeo.tables; > + int num_columns =3D display->vbt.vspeo.num_columns; > + int num_rows =3D display->vbt.vspeo.num_rows; > + size_t offset =3D 0; > + int level; > + > + offset +=3D idx * num_rows * num_columns; I think the division of responsibilities is not great if the caller passes in an index that's tied to the VBT data format. > + > + for (level =3D 0; level < num_rows; level++) { > + u8 txswing =3D buf_trans->entries[level].lt.txswing; > + u8 txswing_level =3D buf_trans->entries[level].lt.txswing_level; > + u32 main_cursor =3D tables[offset]; > + u32 pre_cursor =3D tables[offset + 1]; > + u32 post_cursor =3D tables[offset + 2]; > + > + entries[level].lt.txswing =3D txswing; > + entries[level].lt.txswing_level =3D txswing_level; > + entries[level].lt.main_cursor =3D main_cursor; > + entries[level].lt.pre_cursor =3D pre_cursor; > + entries[level].lt.post_cursor =3D post_cursor; > + > + offset +=3D num_columns; > + } > + > + return vspeo; > +} > + > bool intel_bios_encoder_is_dedicated_external(const struct intel_bios_en= coder_data *devdata) > { > return devdata->display->vbt.version >=3D 264 && > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h b/drivers/gpu/drm/= i915/display/intel_bios.h > index 7a50a272cd27d..1a9b27d8e5789 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.h > +++ b/drivers/gpu/drm/i915/display/intel_bios.h > @@ -73,6 +73,12 @@ bool intel_bios_get_dsc_params(struct intel_encoder *e= ncoder, > const struct intel_bios_encoder_data * > intel_bios_encoder_data_lookup(struct intel_display *display, enum port = port); >=20=20 > +const struct intel_ddi_buf_trans * > +intel_bios_get_lt_vspeo(const struct intel_bios_encoder_data *devdata, > + const struct intel_ddi_buf_trans *buf_trans, > + int idx); > + > +bool intel_bios_encoder_allocated_vspeo(const struct intel_bios_encoder_= data *devdata); > bool intel_bios_encoder_requests_vspeo(const struct intel_bios_encoder_d= ata *devdata); > bool intel_bios_encoder_supports_dvi(const struct intel_bios_encoder_dat= a *devdata); > bool intel_bios_encoder_supports_hdmi(const struct intel_bios_encoder_da= ta *devdata); > diff --git a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c b/drivers= /gpu/drm/i915/display/intel_ddi_buf_trans.c > index 4cd1e4d76c7af..f936868d6113a 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c > @@ -1784,6 +1784,24 @@ xe3plpd_get_lt_buf_trans(struct intel_encoder *enc= oder, > return intel_get_buf_trans(&xe3plpd_lt_trans_dp14, n_entries); > } >=20=20 > +static const struct intel_ddi_buf_trans * > +xe3plpd_get_lt_vspeo_buf_trans(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + int *n_entries) > +{ > + const struct intel_ddi_buf_trans *buf_trans; > + > + buf_trans =3D encoder->get_buf_trans(encoder, crtc_state, n_entries); > + if (intel_crtc_has_dp_encoder(crtc_state)) { > + if (intel_dp_is_uhbr(crtc_state)) > + return intel_bios_get_lt_vspeo(encoder->devdata, buf_trans, 5); > + else > + return intel_bios_get_lt_vspeo(encoder->devdata, buf_trans, 4); > + } > + > + return buf_trans; > +} I need to think about this. Basically this approach is duplicating the platform if-else ladders *and* the CRTC type and port clock etc. checks already existing in this file. It's not great for maintainability, and it's a lot of code for the feature. Plus there's all the added code in intel_bios.c too. > + > void intel_ddi_buf_trans_init(struct intel_encoder *encoder) > { > struct intel_display *display =3D to_intel_display(encoder); > @@ -1857,5 +1875,22 @@ const struct intel_ddi_buf_trans *intel_ddi_buf_tr= ans_get(struct intel_encoder * > const struct intel_crtc_state *crtc_state, > int *n_entries) > { > - return encoder->get_buf_trans(encoder, crtc_state, n_entries); > + struct intel_display *display =3D to_intel_display(encoder); > + const struct intel_ddi_buf_trans *buf_trans; > + > + if (!encoder->devdata) The intel_bios_* functions need to check that, not here. > + return encoder->get_buf_trans(encoder, crtc_state, n_entries); > + > + if (!intel_bios_encoder_requests_vspeo(encoder->devdata)) > + return encoder->get_buf_trans(encoder, crtc_state, n_entries); > + > + if (!intel_bios_encoder_allocated_vspeo(encoder->devdata)) > + return encoder->get_buf_trans(encoder, crtc_state, n_entries); Ditto for the allocation, don't check here. > + > + if (HAS_LT_PHY(display)) > + buf_trans =3D xe3plpd_get_lt_vspeo_buf_trans(encoder, crtc_state, n_en= tries); > + else > + buf_trans =3D encoder->get_buf_trans(encoder, crtc_state, n_entries); > + > + return intel_get_buf_trans(buf_trans, n_entries); This function has four paths to call encoder->get_buf_trans(). There *must* be only one. > } --=20 Jani Nikula, Intel