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 C0BC5C48BEB for ; Wed, 21 Feb 2024 21:24:58 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 45FE610E62E; Wed, 21 Feb 2024 21:24:58 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="Cf/fiNIH"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.17]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7F32210E62E for ; Wed, 21 Feb 2024 21:24:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1708550698; x=1740086698; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=TvYdEILWeI1u4tiGgp6PPY60D5QPsIzGkeFU+wB7TMg=; b=Cf/fiNIHmwfdmGwZjC4vKLo2Entt6x0e+d9RCxl5tfNhHmw5sTtyCdeS B8aWvNWzmRBVGQperZY/kcZDnEKN6HUGBDj6t/B7AwoAVIgpSG1uzBPXd DSO6v4sUiNFhKAU8VfTk+DKBmwzTCKg5G8KAbBgtzZQC7780xTXN5tVuV KcL29NxXN+mQYqUJDjwWAesXjduyVck4SVl8pRIXIYQ/GQCG/AA33rb1s PI2M6agl1g3vAAYVsS72rc2OSCgktfjJaMzHPl38hYqMsS3TGTC+Xzigy WWehzMOFV3wOF2DKcHztXmdADSUmVQT9o0P7jnDaUYO1Vt4HvjRw+mduI Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10991"; a="2865058" X-IronPort-AV: E=Sophos;i="6.06,176,1705392000"; d="scan'208";a="2865058" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orvoesa109.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Feb 2024 13:24:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10991"; a="827411671" X-IronPort-AV: E=Sophos;i="6.06,176,1705392000"; d="scan'208";a="827411671" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga001.jf.intel.com with SMTP; 21 Feb 2024 13:24:52 -0800 Received: by stinkbox (sSMTP sendmail emulation); Wed, 21 Feb 2024 23:24:52 +0200 Date: Wed, 21 Feb 2024 23:24:52 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rodrigo Vivi Cc: "Manna, Animesh" , "intel-gfx@lists.freedesktop.org" , "Hogander, Jouni" , "Murthy, Arun R" Subject: Re: [PATCH v3] drm/i915/panelreplay: Panel replay workaround with VRR Message-ID: References: <20240220141919.3502674-1-animesh.manna@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Wed, Feb 21, 2024 at 11:08:18PM +0200, Ville Syrjälä wrote: > On Wed, Feb 21, 2024 at 03:58:48PM -0500, Rodrigo Vivi wrote: > > On Wed, Feb 21, 2024 at 08:19:35PM +0000, Manna, Animesh wrote: > > > > > > > > > > -----Original Message----- > > > > From: Vivi, Rodrigo > > > > Sent: Tuesday, February 20, 2024 11:12 PM > > > > To: Manna, Animesh > > > > Cc: intel-gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Hogander, > > > > Jouni ; Murthy, Arun R > > > > > > > > Subject: Re: [PATCH v3] drm/i915/panelreplay: Panel replay workaround with > > > > VRR > > > > > > > > On Tue, Feb 20, 2024 at 07:49:19PM +0530, Animesh Manna wrote: > > > > > Panel Replay VSC SDP not getting sent when VRR is enabled and W1 and > > > > > W2 are 0. So Program Set Context Latency in > > > > TRANS_SET_CONTEXT_LATENCY > > > > > register to at least a value of 1. > > > > > > > > > > HSD: 14015406119 > > > > > > > > Unnecessary mark since the wa_name already is a pointer to the HSD. > > > > > > > > > > > > > > v1: Initial version. > > > > > v2: Update timings stored in adjusted_mode struct. [Ville] > > > > > v3: Add WA in compute_config(). [Ville] > > > > > > > > > > Signed-off-by: Animesh Manna > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_dp.c | 12 ++++++++++++ > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > index 217196196e50..eb0fa513cd0f 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > > > > @@ -2948,6 +2948,18 @@ intel_dp_compute_config(struct intel_encoder > > > > *encoder, > > > > > intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state); > > > > > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, > > > > pipe_config, > > > > > conn_state); > > > > > > > > > > + /* > > > > > + * WA: HSD-14015406119 > > > > > > > > this is not the right one. You should use the lineage one and then mark the > > > > platforms. > > > > > > > > /* wa_14015401596: xe_lpd, xe_hpd */ > > > > > > > > or perhaps > > > > > > > > /* wa_14015401596: display versions: 13, 14 */ > > > > > > > > and also add a check for the display version with it. > > > > > > Sure. > > > > > > > > > > > > + * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY > > > > register > > > > > + * to at least a value of 1 when Panel Replay is enabled with VRR. > > > > > + * Value for TRANS_SET_CONTEXT_LATENCY is calculated by > > > > substracting > > > > > + * crtc_vdisplay from crtc_vblank_start, so incrementing > > > > crtc_vblank_start > > > > > + * by 1 if both are equal. > > > > > + */ > > > > > + if (pipe_config->vrr.enable && pipe_config->has_panel_replay && > > > > > + adjusted_mode->crtc_vblank_start == adjusted_mode- > > > > >crtc_vdisplay) > > > > > + adjusted_mode->crtc_vblank_start += 1; > > > > > > > > why to mess with the vblank start instead of going to > > > > intel_set_transcoder_timings() and change directly what is getting written to > > > > the register when the register gets written? > > > > > > I have done in previous version of this patch. But as per review feedback, added now here. > > > https://patchwork.freedesktop.org/series/129632/#rev1 > > > https://patchwork.freedesktop.org/series/129632/#rev2 > > > > > > > > > > > In case the answer is becasue by then we don't have the vrr.enable or > > > > something like that, then we should consider move around when we set that > > > > register? > > > > > > This was not acceptable in earlier versions. As per feedback instead of atomic-commit need to add in compute-config phase. > > > > > > > > > > > or perhaps create a specific flag? one extra variable, 3 less comment lines... > > > > > > The above comment is not clear to me, can you please elaborate more here. > > > > something like: > > > > @intel_dp_compute_config() > > > > +if (pipe_config->vrr.enable && pipe_config->has_panel_replay && > > + adjusted_mode->crtc_vblank_start == adjusted_mode-crtc_vdisplay) > > + pipe_config->mode_flags = I915_MODE_FLAG_MIN_TRANS_CONTEXT_LATENCY_1 > > > > then > > @intel_set_transcoder_timings() > > +u32 context_latency; > > > > +if (crtc_state->mode_flags & I915_MODE_FLAG_MIN_TRANS_CONTEXT_LATENCY_1) > > + context_latency = 1; > > +else > > + crtc_vblank_start - crtc_vdisplay; > > > > -crtc_vblank_start - crtc_vdisplay); > > +context_latency); > > > > Ville, thoughts? > > I think what we need is a intel_crtc_vblank_delay() or somesuch thing > that accounts for all the things (eg. this w/a, dsb execution latency > when we start to use dsb for double buffered registers, etc). > And it should probably be called from some central place so that > it works for all output types. intel_crtc_compute_config() comes to > mind, but I guess we need to first ponder whether there are bits > of code being executed prior to that that would need to know the > actual vblank_start... PSR2 itself might have a chicken vs. egg issue here. Wehether we can enable it not depends on the length of the vblank. Whether that means the full vblank or the pipe's shrunken vblank I don't know. Someone needs to figure that out. -- Ville Syrjälä Intel