Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes
On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote: > Currently we can't change MBUS join status without doing a modeset, > because we are lacking mechanism to synchronize those with vblank. > However then this means that we can't do a fastset, if there is a need > to change MBUS join state. Fix that by implementing such change. > We already call correspondent check and update at pre_plane dbuf update, > so the only thing left is to have a non-modeset version of that. > If active pipes stay the same then fastset is possible and only MBUS > join state/ddb allocation updates would be committed. > > v2: Implement additional changes according to BSpec. > Vblank wait is needed after MBus/Dbuf programming in case if > no modeset is done and we are switching from single to multiple > displays, i.e mbus join state switches from "joined" to "non-joined" > state. Otherwise vblank wait is not needed according to spec. > > v3: Split mbus and dbox programming into to pre/post plane update parts, > how it should be done according to BSpec. > > v4: - Place "single display to multiple displays scenario" MBUS/DBOX > programming > after DDB reallocation, but before crtc enabling(that is where is has > to be according to spec). > - Check if crtc is still active, not only the old state. > - Do a vblank wait if MBUX DBOX register was modified. > - And of course do vblank wait only if crtc was active. > - Do vblank wait only if we are not doing a modeset, if we are doing > something before *commit_modeset_enables, because all crtcs might be > disabled at this moment, so we will get WARN if try waiting for vblank > then. > - Still getting FIFO underrun so try waiting for vblank in pre_plane > update > as well. > - Write also pipe that we need to sync with to MBUS_CTL register. > > v5: - Do vblank wait only for the first pipe, if mbus is joined > - Check also if new/old_dbuf_state is not NULL, before getting single pipe > and active pipes. > > Signed-off-by: Stanislav Lisovskiy Thank you for this patch, Stanislav! We tested it on a MTL-U based Chromebook (Screebo), using different configurations (eDP, eDP + HDMI, HDMI, etc.), and it worked well -- joined the mbus + no visual issues or i915 errors. Tested-by: Paz Zcharya Just a small note, checkpatch.pl is complaining about a few things. I assume you probably saw it, but flagging just in case.
Re: [PATCH 1/2] drm/i915: Update mbus in intel_dbuf_mbus_update and do it properly
On Wed, Feb 28, 2024 at 10:02:12AM +0200, Stanislav Lisovskiy wrote: > According to BSpec we need to do correspondent MBUS updates before > or after DBUF reallocation, depending on whether we are reducing > or increasing amount of pipes(typical scenario is swithing between > multiple and single displays). > > As of BSpec 49213 if we are swithing from multiple to single display > MBUS registers should be updated with correspondent values _before_ > Dbuf reallocation happens, however if we are switching from single > display to multiple then it should happen _after_ DDB reallocation(i.e > plane programming). > > Signed-off-by: Stanislav Lisovskiy Thank you for this patch, Stanislav! We tested it on a MTL-U based Chromebook (Screebo), using different configurations (eDP, eDP + HDMI, HDMI, etc.), and it worked well -- joined the mbus + no visual issues or i915 errors. Tested-by: Paz Zcharya
Re: [PATCH] drm/i915/display: Include debugfs.h in intel_display_debugfs_params.c
On Thu, Feb 01, 2024 at 11:22:16AM +0200, Jani Nikula wrote:
> On Wed, 31 Jan 2024, Paz Zcharya wrote:
> > Commit 8015bee0bfec ("drm/i915/display: Add framework to add parameters
> > specific to display") added the file intel_display_debugfs_params.c,
> > which calls the functions "debugfs_create_{bool, ulong, str}" -- all of
> > which are defined in . The missing inclusion of this
> > header file is breaking the ChromeOS build -- add an explicit include
> > to fix that.
> >
>
> Thanks for the patch, apparently in our configs some paths lead to
> debugfs.h. Just goes on to show how interdependent the kernel headers
> are.
>
> Out of curiousity, what value do you have for CONFIG_DEBUG_FS kconfig?
>
> Fixes: 8015bee0bfec ("drm/i915/display: Add framework to add parameters
> specific to display")
> Reviewed-by: Jani Nikula
>
> BR,
> Jani.
>
Thank you so much for the super prompt reply!
In ChromeOS the value is CONFIG_DEBUG_FS=y.
Best,
Paz
>
> > Signed-off-by: Paz Zcharya
> > ---
> > drivers/gpu/drm/i915/display/intel_display_debugfs_params.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> > index b7e68eb62452..f35718748555 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
> > @@ -3,6 +3,7 @@
> > * Copyright © 2023 Intel Corporation
> > */
> >
> > +#include
> > #include
> >
> > #include
>
> --
> Jani Nikula, Intel
[PATCH] drm/i915/display: Include debugfs.h in intel_display_debugfs_params.c
Commit 8015bee0bfec ("drm/i915/display: Add framework to add parameters
specific to display") added the file intel_display_debugfs_params.c,
which calls the functions "debugfs_create_{bool, ulong, str}" -- all of
which are defined in . The missing inclusion of this
header file is breaking the ChromeOS build -- add an explicit include
to fix that.
Signed-off-by: Paz Zcharya
---
drivers/gpu/drm/i915/display/intel_display_debugfs_params.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
index b7e68eb62452..f35718748555 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs_params.c
@@ -3,6 +3,7 @@
* Copyright © 2023 Intel Corporation
*/
+#include
#include
#include
--
2.43.0.594.gd9cf4e227d-goog
Re: [PATCH v3 16/16] drm/i915: Annotate more of the BIOS fb takeover failure paths
On Mon, Jan 22, 2024 at 03:12:01PM +, Shankar, Uma wrote: > > > > -Original Message- > > From: Intel-gfx On Behalf Of Ville > > Syrjala > > Sent: Tuesday, January 16, 2024 1:27 PM > > To: intel-gfx@lists.freedesktop.org > > Subject: [PATCH v3 16/16] drm/i915: Annotate more of the BIOS fb takeover > > failure paths > > > > From: Ville Syrjälä > > > > Annotate a few more of the failure paths on the initial BIOS fb takeover to > > avoid > > having to guess why things aren't working the way we expect. > > Looks Good to me. > Reviewed-by: Uma Shankar > > > Signed-off-by: Ville Syrjälä Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
Re: [PATCH v3 15/16] drm/i915: Try to relocate the BIOS fb to the start of ggtt
On Tue, Jan 16, 2024 at 09:56:35AM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > On MTL the GOP (for whatever reason) likes to bind its framebuffer > high up in the ggtt address space. This can conflict with whatever > ggtt_reserve_guc_top() is trying to do, and the result is that > ggtt_reserve_guc_top() fails and then we proceed to explode when > trying to tear down the driver. Thus far I haven't analyzed what > causes the actual fireworks, but it's not super important as even > if it didn't explode we'd still fail the driver load and the user > would be left with an unusable GPU. > > To remedy this (without having to figure out exactly what > ggtt_reserve_guc_top() is trying to achieve) we can attempt to > relocate the BIOS framebuffer to a lower ggtt address. We can do > this at this early point in driver init because nothing else is > supposed to be clobbering the ggtt yet. So we simply change where > in the ggtt we pin the vma, the original PTEs will be left as is, > and the new PTEs will get written with the same dma addresses. > The plane will keep on scanning out from the original PTEs until > we are done with the whole process, and at that point we rewrite > the plane's surface address register to point at the new ggtt > address. > > Since we don't need a specific ggtt address for the plane > (apart from needing it to land in the mappable region for > normal stolen objects) we'll just try to pin it without a fixed > offset first. It should end up at the lowest available address > (which really should be 0 at this point in the driver init). > If that fails we'll fall back to just pinning it exactly to the > origianal address. > > To make sure we don't accidentlally pin it partially over the > original ggtt range (as that would corrupt the original PTEs) > we reserve the original range temporarily during this process. > > v2: Try to pin explicitly to ggtt offset 0 as otherwise DG2 puts it > even higher (atm we have no PIN_LOW flag to force it low) > > Cc: Paz Zcharya > Reviewed-by: Andrzej Hajda > Signed-off-by: Ville Syrjälä Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
Re: [PATCH v3 14/16] drm/i915: Tweak BIOS fb reuse check
On Tue, Jan 16, 2024 at 09:56:34AM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Currently we assume that we bind the BIOS fb exactly into the same > ggtt address where the BIOS left it. That is about to change, and > in order to keep intel_reuse_initial_plane_obj() working as intended > we need to compare the original ggtt offset (called 'base' here) > as opposed to the actual vma ggtt offset we selected. Otherwise > the first plane could change the ggtt offset, and then subsequent > planes would no longer notice that they are in fact using the same > ggtt offset that the first plane was already using. Thus the reuse > check will fail and we proceed to turn off these subsequent planes. > > TODO: would probably make more sense to do the pure readout first > for all the planes, then check for fb reuse, and only then proceed > to pin the object into the final location in the ggtt... > > Cc: Paz Zcharya > Reviewed-by: Andrzej Hajda > Signed-off-by: Ville Syrjälä Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
Re: [PATCH v3 13/16] drm/i915/fbdev: Fix smem_start for LMEMBAR stolen objects
On Tue, Jan 16, 2024 at 09:56:33AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> The "io" address of an object is its dma address minus the
> region.start. Subtract the latter to make smem_start correct.
> The current code happens to work for genuine LMEM objects
> as LMEM region.start==0, but for LMEMBAR stolen objects
> region.start!=0.
>
> TODO: perhaps just set smem_start=0 always as our .fb_mmap()
> implementation no longer depends on it? Need to double check
> it's not needed for anything else...
>
> Cc: Paz Zcharya
> Reviewed-by: Andrzej Hajda
> Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> ---
> drivers/gpu/drm/i915/display/intel_fbdev_fb.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> index 1ac05d90b2e8..0665f943f65f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> @@ -79,7 +79,8 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915,
> struct fb_info *info
> /* Use fbdev's framebuffer from lmem for discrete */
> info->fix.smem_start =
> (unsigned long)(mem->io.start +
> - i915_gem_object_get_dma_address(obj,
> 0));
> + i915_gem_object_get_dma_address(obj, 0)
> -
> + mem->region.start);
> info->fix.smem_len = obj->base.size;
> } else {
> struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> --
> 2.41.0
>
Re: [PATCH v3 12/16] drm/i915: Simplify intel_initial_plane_config() calling convention
On Tue, Jan 16, 2024 at 09:56:32AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> There's no reason the caller of intel_initial_plane_config() should
> have to loop over the CRTCs. Pull the loop into the function to
> make life simpler for the caller.
>
> Cc: Paz Zcharya
> Reviewed-by: Andrzej Hajda
> Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> ---
> .../drm/i915/display/intel_display_driver.c | 7 +---
> .../drm/i915/display/intel_plane_initial.c| 40 +++
> .../drm/i915/display/intel_plane_initial.h| 4 +-
> 3 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c
> b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index ecf9cb74734b..f3fe1743243b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -415,7 +415,6 @@ int intel_display_driver_probe_nogem(struct
> drm_i915_private *i915)
> {
> struct drm_device *dev = >drm;
> enum pipe pipe;
> - struct intel_crtc *crtc;
> int ret;
>
> if (!HAS_DISPLAY(i915))
> @@ -467,11 +466,7 @@ int intel_display_driver_probe_nogem(struct
> drm_i915_private *i915)
> intel_acpi_assign_connector_fwnodes(i915);
> drm_modeset_unlock_all(dev);
>
> - for_each_intel_crtc(dev, crtc) {
> - if (!to_intel_crtc_state(crtc->base.state)->uapi.active)
> - continue;
> - intel_crtc_initial_plane_config(crtc);
> - }
> + intel_initial_plane_config(i915);
>
> /*
>* Make sure hardware watermarks really match the state we read out.
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index 78bff6181ceb..b7e12b60d68b 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -357,25 +357,31 @@ static void plane_config_fini(struct
> intel_initial_plane_config *plane_config)
> i915_vma_put(plane_config->vma);
> }
>
> -void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
> +void intel_initial_plane_config(struct drm_i915_private *i915)
> {
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - struct intel_initial_plane_config plane_config = {};
> + struct intel_crtc *crtc;
>
> - /*
> - * Note that reserving the BIOS fb up front prevents us
> - * from stuffing other stolen allocations like the ring
> - * on top. This prevents some ugliness at boot time, and
> - * can even allow for smooth boot transitions if the BIOS
> - * fb is large enough for the active pipe configuration.
> - */
> - dev_priv->display.funcs.display->get_initial_plane_config(crtc,
> _config);
> + for_each_intel_crtc(>drm, crtc) {
> + struct intel_initial_plane_config plane_config = {};
>
> - /*
> - * If the fb is shared between multiple heads, we'll
> - * just get the first one.
> - */
> - intel_find_initial_plane_obj(crtc, _config);
> + if (!to_intel_crtc_state(crtc->base.state)->uapi.active)
> + continue;
>
> - plane_config_fini(_config);
> + /*
> + * Note that reserving the BIOS fb up front prevents us
> + * from stuffing other stolen allocations like the ring
> + * on top. This prevents some ugliness at boot time, and
> + * can even allow for smooth boot transitions if the BIOS
> + * fb is large enough for the active pipe configuration.
> + */
> + i915->display.funcs.display->get_initial_plane_config(crtc,
> _config);
> +
> + /*
> + * If the fb is shared between multiple heads, we'll
> + * just get the first one.
> + */
> + intel_find_initial_plane_obj(crtc, _config);
> +
> + plane_config_fini(_config);
> + }
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.h
> b/drivers/gpu/drm/i915/display/intel_plane_initial.h
> index c7e35ab3182b..64ab95239cd4 100
Re: [PATCH v3 11/16] drm/i915: Split the smem and lmem plane readout apart
On Tue, Jan 16, 2024 at 09:56:31AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Declutter initial_plane_vma() a bit by pulling the lmem and smem
> readout paths into their own functions.
>
> TODO: the smem path should still be fixed to get and validate
> the dma address from the pte as well
>
> Cc: Paz Zcharya
> Reviewed-by: Andrzej Hajda
> Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> ---
> .../drm/i915/display/intel_display_types.h| 2 +
> .../drm/i915/display/intel_plane_initial.c| 145 +++---
> 2 files changed, 95 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index ae2e8cff9d69..319ba7aed4fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -780,6 +780,8 @@ struct intel_plane_state {
>
> struct intel_initial_plane_config {
> struct intel_framebuffer *fb;
> + struct intel_memory_region *mem;
> + resource_size_t phys_base;
> struct i915_vma *vma;
> unsigned int tiling;
> int size;
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index 48b74319f45c..78bff6181ceb 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -44,6 +44,93 @@ intel_reuse_initial_plane_obj(struct drm_i915_private
> *i915,
> return false;
> }
>
> +static bool
> +initial_plane_phys_lmem(struct drm_i915_private *i915,
> + struct intel_initial_plane_config *plane_config)
> +{
> + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> + struct intel_memory_region *mem;
> + dma_addr_t dma_addr;
> + gen8_pte_t pte;
> + u32 base;
> +
> + base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
> +
> + gte += base / I915_GTT_PAGE_SIZE;
> +
> + pte = ioread64(gte);
> + if (!(pte & GEN12_GGTT_PTE_LM)) {
> + drm_err(>drm,
> + "Initial plane programming missing PTE_LM bit\n");
> + return false;
> + }
> +
> + dma_addr = pte & GEN12_GGTT_PTE_ADDR_MASK;
> +
> + if (IS_DGFX(i915))
> + mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> + else
> + mem = i915->mm.stolen_region;
> + if (!mem) {
> + drm_dbg_kms(>drm,
> + "Initial plane memory region not initialized\n");
> + return false;
> + }
> +
> + /*
> + * On lmem we don't currently expect this to
> + * ever be placed in the stolen portion.
> + */
> + if (dma_addr < mem->region.start || dma_addr > mem->region.end) {
> + drm_err(>drm,
> + "Initial plane programming using invalid range,
> dma_addr=%pa (%s [%pa-%pa])\n",
> + _addr, mem->region.name, >region.start,
> >region.end);
> + return false;
> + }
> +
> + drm_dbg(>drm,
> + "Using dma_addr=%pa, based on initial plane programming\n",
> + _addr);
> +
> + plane_config->phys_base = dma_addr - mem->region.start;
> + plane_config->mem = mem;
> +
> + return true;
> +}
> +
> +static bool
> +initial_plane_phys_smem(struct drm_i915_private *i915,
> + struct intel_initial_plane_config *plane_config)
> +{
> + struct intel_memory_region *mem;
> + u32 base;
> +
> + base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
> +
> + mem = i915->mm.stolen_region;
> + if (!mem) {
> + drm_dbg_kms(>drm,
> + "Initial plane memory region not initialized\n");
> + return false;
> + }
> +
> + /* FIXME get and validate the dma_addr from the PTE */
> + plane_config->phys_base = base;
> + plane_config->mem = mem;
> +
> + return true;
> +}
> +
> +static bool
> +initial_plane_phys(struct drm_i915_private *i915,
> +
Re: [PATCH v3 10/16] drm/i915: s/phys_base/dma_addr/
On Tue, Jan 16, 2024 at 09:56:30AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> The address we read from the PTE is a dma address, not a physical
> address. Rename the variable to say so.
>
> Cc: Paz Zcharya
> Reviewed-by: Andrzej Hajda
> Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> ---
> .../gpu/drm/i915/display/intel_plane_initial.c| 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index c72d4cacf631..48b74319f45c 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -61,6 +61,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
> if (IS_DGFX(i915) || HAS_LMEMBAR_SMEM_STOLEN(i915)) {
> gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> + dma_addr_t dma_addr;
> gen8_pte_t pte;
>
> gte += base / I915_GTT_PAGE_SIZE;
> @@ -72,7 +73,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> return NULL;
> }
>
> - phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
> + dma_addr = pte & GEN12_GGTT_PTE_ADDR_MASK;
>
> if (IS_DGFX(i915))
> mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> @@ -88,18 +89,18 @@ initial_plane_vma(struct drm_i915_private *i915,
>* On lmem we don't currently expect this to
>* ever be placed in the stolen portion.
>*/
> - if (phys_base < mem->region.start || phys_base >
> mem->region.end) {
> + if (dma_addr < mem->region.start || dma_addr > mem->region.end)
> {
> drm_err(>drm,
> - "Initial plane programming using invalid range,
> phys_base=%pa (%s [%pa-%pa])\n",
> - _base, mem->region.name,
> >region.start, >region.end);
> + "Initial plane programming using invalid range,
> dma_addr=%pa (%s [%pa-%pa])\n",
> + _addr, mem->region.name,
> >region.start, >region.end);
> return NULL;
> }
>
> drm_dbg(>drm,
> - "Using phys_base=%pa, based on initial plane
> programming\n",
> - _base);
> + "Using dma_addr=%pa, based on initial plane
> programming\n",
> + _addr);
>
> - phys_base -= mem->region.start;
> + phys_base = dma_addr - mem->region.start;
> } else {
> phys_base = base;
> mem = i915->mm.stolen_region;
> --
> 2.41.0
>
Re: [PATCH v3 09/16] drm/i915: Fix MTL initial plane readout
On Mon, Jan 22, 2024 at 03:09:23PM +, Shankar, Uma wrote:
>
>
> > -Original Message-
> > From: Intel-gfx On Behalf Of Ville
> > Syrjala
> > Sent: Tuesday, January 16, 2024 1:26 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [PATCH v3 09/16] drm/i915: Fix MTL initial plane readout
> >
> > From: Ville Syrjälä
> >
> > MTL stolen memory looks more like local memory, so use the (now fixed) lmem
> > path when doing the initial plane readout.
>
> Looks Good to me.
> Reviewed-by: Uma Shankar
>
> > Cc: Paz Zcharya
> > Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> > ---
> > .../drm/i915/display/intel_plane_initial.c| 25 +--
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index db594ccf0323..c72d4cacf631 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -59,7 +59,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> > return NULL;
> >
> > base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
> > - if (IS_DGFX(i915)) {
> > + if (IS_DGFX(i915) || HAS_LMEMBAR_SMEM_STOLEN(i915)) {
> > gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > gen8_pte_t pte;
> >
> > @@ -73,11 +73,20 @@ initial_plane_vma(struct drm_i915_private *i915,
> > }
> >
> > phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
> > - mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> > +
> > + if (IS_DGFX(i915))
> > + mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> > + else
> > + mem = i915->mm.stolen_region;
> > + if (!mem) {
> > + drm_dbg_kms(>drm,
> > + "Initial plane memory region not
> > initialized\n");
> > + return NULL;
> > + }
> >
> > /*
> > -* We don't currently expect this to ever be placed in the
> > -* stolen portion.
> > +* On lmem we don't currently expect this to
> > +* ever be placed in the stolen portion.
> > */
> > if (phys_base < mem->region.start || phys_base > mem-
> > >region.end) {
> > drm_err(>drm,
> > @@ -94,11 +103,13 @@ initial_plane_vma(struct drm_i915_private *i915,
> > } else {
> > phys_base = base;
> > mem = i915->mm.stolen_region;
> > + if (!mem) {
> > + drm_dbg_kms(>drm,
> > + "Initial plane memory region not
> > initialized\n");
> > + return NULL;
> > + }
> > }
> >
> > - if (!mem)
> > - return NULL;
> > -
> > size = round_up(plane_config->base + plane_config->size,
> > mem->min_page_size);
> > size -= base;
> > --
> > 2.41.0
>
Re: [PATCH v3 08/16] drm/i915: Fix region start during initial plane readout
On Mon, Jan 22, 2024 at 03:07:52PM +, Shankar, Uma wrote:
>
>
> > -Original Message-
> > From: Intel-gfx On Behalf Of Ville
> > Syrjala
> > Sent: Tuesday, January 16, 2024 1:26 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [PATCH v3 08/16] drm/i915: Fix region start during initial plane
> > readout
> >
> > From: Ville Syrjälä
> >
> > On MTL the stolen region starts at offset 8MiB from the start of LMEMBAR.
> > The
> > dma addresses are thus also offset by 8MiB. However the mm_node/etc. is zero
> > based, and i915_pages_create_for_stolen() will add the appropriate
> > region.start
> > into the sg dma address. So when we do the readout we need to convert the
> > dma
> > address read from the PTE to be zero based as well.
> >
> > Note that currently we don't take this path on MTL, but we should and thus
> > this
> > needs to be fixed. For lmem this works correctly already as the lmem
> > region.start==0.
> >
> > While at it let's also make sure the address points to somewhere within the
> > memory region. We don't need to check the size as
> > i915_gem_object_create_region_at() should later fail if the object size
> > exceeds
> > the region size.
>
> Looks Good to me.
> Reviewed-by: Uma Shankar
>
> > Cc: Paz Zcharya
> > Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> > ---
> > drivers/gpu/drm/i915/display/intel_plane_initial.c | 8 +---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index ffc92b18fcf5..db594ccf0323 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -79,16 +79,18 @@ initial_plane_vma(struct drm_i915_private *i915,
> > * We don't currently expect this to ever be placed in the
> > * stolen portion.
> > */
> > - if (phys_base >= resource_size(>region)) {
> > + if (phys_base < mem->region.start || phys_base > mem-
> > >region.end) {
> > drm_err(>drm,
> > - "Initial plane programming using invalid range,
> > phys_base=%pa\n",
> > - _base);
> > + "Initial plane programming using invalid range,
> > phys_base=%pa (%s [%pa-%pa])\n",
> > + _base, mem->region.name,
> > >region.start,
> > +>region.end);
> > return NULL;
> > }
> >
> > drm_dbg(>drm,
> > "Using phys_base=%pa, based on initial plane
> > programming\n",
> > _base);
> > +
> > + phys_base -= mem->region.start;
> > } else {
> > phys_base = base;
> > mem = i915->mm.stolen_region;
> > --
> > 2.41.0
>
Re: [PATCH v3 07/16] drm/i915: Fix PTE decode during initial plane readout
On Tue, Jan 16, 2024 at 11:46:13AM +0100, Nirmoy Das wrote: > > On 1/16/2024 8:56 AM, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > When multiple pipes are enabled by the BIOS we try to read out each > > in turn. But we do the readout for the second only after the inherited > > vma for the first has been rebound into its original place (and thus > > the PTEs have been rewritten). Unlike the BIOS we set some high caching > > bits in the PTE on MTL which confuses the readout for the second plane. > > Filter out the non-address bits from the PTE value appropriately to > > fix this. > > > > I suppose it might also be possible that the BIOS would already set > > some caching bits as well, in which case we'd run into this same > > issue already for the first plane. > > > > TODO: > > - should abstract the PTE decoding to avoid details leaking all over > > - should probably do the readout for all the planes before > > we touch anything (including the PTEs) so that we truly read > >out the BIOS state > > > > Cc: Paz Zcharya > > Reviewed-by: Andrzej Hajda > > Signed-off-by: Ville Syrjälä > Acked-by: Nirmoy Das Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya > > --- > > drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c > > b/drivers/gpu/drm/i915/display/intel_plane_initial.c > > index a55c09cbd0e4..ffc92b18fcf5 100644 > > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c > > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c > > @@ -72,7 +72,7 @@ initial_plane_vma(struct drm_i915_private *i915, > > return NULL; > > } > > - phys_base = pte & I915_GTT_PAGE_MASK; > > + phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK; > > mem = i915->mm.regions[INTEL_REGION_LMEM_0]; > > /*
Re: [PATCH v4 06/16] drm/i915: Rename the DSM/GSM registers
On Thu, Jan 25, 2024 at 12:28:04PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 0x108100 and 0x1080c0 have been around since snb. Rename the
> defines appropriately.
>
> v2: Rebase
>
> Cc: Paz Zcharya
> Reviewed-by: Andrzej Hajda
> Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> ---
> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 4 ++--
> drivers/gpu/drm/i915/gt/intel_ggtt.c| 2 +-
> drivers/gpu/drm/i915/gt/intel_region_lmem.c | 2 +-
> drivers/gpu/drm/i915/i915_reg.h | 7 ---
> 4 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index 9ddcae9b3997..ad6dd7f3259b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -935,7 +935,7 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915,
> u16 type,
> GEM_BUG_ON((dsm_base + dsm_size) > lmem_size);
> } else {
> /* Use DSM base address instead for stolen memory */
> - dsm_base = intel_uncore_read64(uncore, GEN12_DSMBASE) &
> GEN12_BDSM_MASK;
> + dsm_base = intel_uncore_read64(uncore, GEN6_DSMBASE) &
> GEN11_BDSM_MASK;
> if (WARN_ON(lmem_size < dsm_base))
> return ERR_PTR(-ENODEV);
> dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> @@ -943,7 +943,7 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915,
> u16 type,
>
> if (i915_direct_stolen_access(i915)) {
> drm_dbg(>drm, "Using direct DSM access\n");
> - io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) &
> GEN12_BDSM_MASK;
> + io_start = intel_uncore_read64(uncore, GEN6_DSMBASE) &
> GEN11_BDSM_MASK;
> io_size = dsm_size;
> } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> io_start = 0;
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index bce5d8025340..ec1cbe229f0e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -1163,7 +1163,7 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt,
> u64 size)
>
> if (i915_direct_stolen_access(i915)) {
> drm_dbg(>drm, "Using direct GSM access\n");
> - phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) &
> GEN12_BDSM_MASK;
> + phys_addr = intel_uncore_read64(uncore, GEN6_GSMBASE) &
> GEN11_BDSM_MASK;
> } else {
> phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> gen6_gttadr_offset(i915);
> }
> diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> index af357089da6e..51bb27e10a4f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> @@ -240,7 +240,7 @@ static struct intel_memory_region *setup_lmem(struct
> intel_gt *gt)
> lmem_size -= tile_stolen;
> } else {
> /* Stolen starts from GSMBASE without CCS */
> - lmem_size = intel_uncore_read64(>uncore, GEN12_GSMBASE);
> + lmem_size = intel_uncore_read64(>uncore, GEN6_GSMBASE);
> }
>
> i915_resize_lmem_bar(i915, lmem_size);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b5f5e0bc6608..1ad55aafe679 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6323,9 +6323,10 @@ enum skl_power_gate {
> #define GMS_MASK REG_GENMASK(15, 8)
> #define GGMS_MASK REG_GENMASK(7, 6)
>
> -#define GEN12_GSMBASE_MMIO(0x108100)
> -#define GEN12_DSMBASE_MMIO(0x1080C0)
> -#define GEN12_BDSM_MASKREG_GENMASK64(63, 20)
> +#define GEN6_GSMBASE _MMIO(0x108100)
> +#define GEN6_DSMBASE _MMIO(0x1080C0)
> +#define GEN6_BDSM_MASK REG_GENMASK64(31, 20)
> +#define GEN11_BDSM_MASKREG_GENMASK64(63, 20)
>
> #define XEHP_CLOCK_GATE_DIS _MMIO(0x101014)
> #define SGSI_SIDECLK_DIS REG_BIT(17)
> --
> 2.43.0
>
Re: [PATCH v4 05/16] drm/i915: Disable the "binder"
On Thu, Jan 25, 2024 at 12:27:36PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Now that the GGTT PTE updates go straight to GSMBASE (bypassing
> GTTMMADR) there should be no more risk of system hangs? So the
> "binder" (ie. update the PTEs via MI_UPDATE_GTT) is no longer
> necessary, disable it.
>
> My main worry with the MI_UPDATE_GTT are:
> - only used on this one platform so very limited testing coverage
> - async so more opprtunities to screw things up
> - what happens if the engine hangs while we're waiting for MI_UPDATE_GTT
> to finish?
> - requires working command submission, so even getting a working
> display now depends on a lot more extra components working correctly
>
> TODO: MI_UPDATE_GTT might be interesting as an optimization
> though, so perhaps someone should look into always using it
> (assuming the GPU is alive and well)?
>
> v2: Keep using MI_UPDATE_GTT on VM guests
> v3: use i915_direct_stolen_access()
>
> Cc: Paz Zcharya
> Cc: Nirmoy Das
> Signed-off-by: Ville Syrjälä
Hi Ville,
Thank you so much for this incredible series.
It solves the issue regarding MTL initial plane readout
that Andrzej Hajda and I worked on in
https://patchwork.freedesktop.org/patch/570811/?series=127130=2
In addition, it solved the issue with the new GOP.
I tested it on two different devices with Meteor Lake and it worked perfectly:
no i915 errors, no flickers or observable issues.
Tested-by: Paz Zcharya
> ---
> drivers/gpu/drm/i915/gt/intel_gtt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> b/drivers/gpu/drm/i915/gt/intel_gtt.c
> index 86f73fe558ca..7811a8c9da06 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> @@ -24,7 +24,8 @@
> bool i915_ggtt_require_binder(struct drm_i915_private *i915)
> {
> /* Wa_13010847436 & Wa_14019519902 */
> - return MEDIA_VER_FULL(i915) == IP_VER(13, 0);
> + return !i915_direct_stolen_access(i915) &&
> + MEDIA_VER_FULL(i915) == IP_VER(13, 0);
> }
>
> static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
> --
> 2.43.0
>
Re: [PATCH v4 04/16] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
On Thu, Jan 25, 2024 at 12:27:14PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > On MTL accessing stolen memory via the BARs is somehow borked, > and it can hang the machine. As a workaround let's bypass the > BARs and just go straight to DSMBASE/GSMBASE instead. > > Note that on every other platform this itself would hang the > machine, but on MTL the system firmware is expected to relax > the access permission guarding stolen memory to enable this > workaround, and thus direct CPU accesses should be fine. > > The raw stolen memory areas won't be passed to VMs so we'll > need to risk using the BAR there for the initial setup. Once > command submission is up we should switch to MI_UPDATE_GTT > which at least shouldn't hang the whole machine. > > v2: Don't use direct GSM/DSM access on guests > Add w/a number > v3: Check register 0x138914 to see if pcode did its job > Add some debug prints > > Cc: Paz Zcharya > Cc: Nirmoy Das > Cc: Joonas Lahtinen > Reviewed-by: Andrzej Hajda > Reviewed-by: Radhakrishna Sripada > Signed-off-by: Ville Syrjälä Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
Re: [PATCH v3 03/16] drm/i915: Remove ad-hoc lmem/stolen debugs
On Tue, Jan 16, 2024 at 11:23:37AM +0100, Nirmoy Das wrote: > > On 1/16/2024 8:56 AM, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Now that intel_memory_regions_hw_probe() prints out each and every > > memory region there's no reason to have ad-hoc debugs to do similar > > things elsewhere. > > > > Cc: Paz Zcharya > > Reviewed-by: Andrzej Hajda > > Signed-off-by: Ville Syrjälä > Reviewed-by: Nirmoy Das Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
Re: [PATCH v3 02/16] drm/i915: Print memory region info during probe
On Tue, Jan 16, 2024 at 11:20:37AM +0100, Nirmoy Das wrote: > > On 1/16/2024 8:56 AM, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Dump the details about every memory region into dmesg at probe time. > > Avoids having to dig those out from random places when debugging stuff. > > > > Cc: Paz Zcharya > > Reviewed-by: Andrzej Hajda > > Signed-off-by: Ville Syrjälä > Reviewed-by: Nirmoy Das Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
Re: [PATCH v3 01/16] drm/i915: Use struct resource for memory region IO as well
On Tue, Jan 16, 2024 at 11:23:00AM +0100, Nirmoy Das wrote: > > On 1/16/2024 8:56 AM, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > mem->region is a struct resource, but mem->io_start and > > mem->io_size are not for whatever reason. Let's unify this > > and convert the io stuff into a struct resource as well. > > Should make life a little less annoying when you don't have > > juggle between two different approaches all the time. > > > > Mostly done using cocci (with manual tweaks at all the > > places where we mutate io_size by hand): > > @@ > > struct intel_memory_region *M; > > expression START, SIZE; > > @@ > > - M->io_start = START; > > - M->io_size = SIZE; > > + M->io = DEFINE_RES_MEM(START, SIZE); > > > > @@ > > struct intel_memory_region *M; > > @@ > > - M->io_start > > + M->io.start > > > > @@ > > struct intel_memory_region M; > > @@ > > - M.io_start > > + M.io.start > > > > @@ > > expression M; > > @@ > > - M->io_size > > + resource_size(>io) > > > > @@ > > expression M; > > @@ > > - M.io_size > > + resource_size() > > > > Cc: Paz Zcharya > > Reviewed-by: Andrzej Hajda > > Signed-off-by: Ville Syrjälä > Acked-by: Nirmoy Das Hi Ville, Thank you so much for this incredible series. It solves the issue regarding MTL initial plane readout that Andrzej Hajda and I worked on in https://patchwork.freedesktop.org/patch/570811/?series=127130=2 In addition, it solved the issue with the new GOP. I tested it on two different devices with Meteor Lake and it worked perfectly: no i915 errors, no flickers or observable issues. Tested-by: Paz Zcharya
[Intel-gfx] [PATCH] [v2] drm/i915/display: Check GGTT to determine phys_base
There was an assumption that for iGPU there should be a 1:1 mapping
of GGTT to physical address pointing to the framebuffer.
This assumption is not strictly true effective generation 8 or newer.
Fix that by checking GGTT to determine the phys address on gen8+.
The following algorithm for phys_base should be valid for all platforms:
1. Find pte
2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
- On older platforms, stolen_region points to system memory, starting at 0
- on DG2, it uses lmem region which starts at 0 as well
- on MTL, stolen_region points to stolen-local which starts at 0x80
Changes from v1:
- Add an if statement for gen7-, where there is a 1:1 mapping
Signed-off-by: Paz Zcharya
---
.../drm/i915/display/intel_plane_initial.c| 64 +++
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index a55c09cbd0e4..7d9bb631b93b 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -59,44 +59,58 @@ initial_plane_vma(struct drm_i915_private *i915,
return NULL;
base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
- if (IS_DGFX(i915)) {
+
+ if (GRAPHICS_VER(i915) < 8) {
+ /*
+* In gen7-, there is a 1:1 mapping
+* between GSM and physical address.
+*/
+ phys_base = base;
+ mem = i915->mm.stolen_region;
+ } else {
+ /*
+* In gen8+, there is no 1:1 mapping between
+* GSM and physical address, so we need to
+* check GGTT to determine the physical address.
+*/
gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
gen8_pte_t pte;
gte += base / I915_GTT_PAGE_SIZE;
-
pte = ioread64(gte);
- if (!(pte & GEN12_GGTT_PTE_LM)) {
- drm_err(>drm,
- "Initial plane programming missing PTE_LM
bit\n");
- return NULL;
- }
-
- phys_base = pte & I915_GTT_PAGE_MASK;
- mem = i915->mm.regions[INTEL_REGION_LMEM_0];
- /*
-* We don't currently expect this to ever be placed in the
-* stolen portion.
-*/
- if (phys_base >= resource_size(>region)) {
- drm_err(>drm,
- "Initial plane programming using invalid range,
phys_base=%pa\n",
- _base);
- return NULL;
+ if (IS_DGFX(i915)) {
+ if (!(pte & GEN12_GGTT_PTE_LM)) {
+ drm_err(>drm,
+ "Initial plane programming missing
PTE_LM bit\n");
+ return NULL;
+ }
+ mem = i915->mm.regions[INTEL_REGION_LMEM_0];
+ } else {
+ mem = i915->mm.stolen_region;
}
- drm_dbg(>drm,
- "Using phys_base=%pa, based on initial plane
programming\n",
- _base);
- } else {
- phys_base = base;
- mem = i915->mm.stolen_region;
+ phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
}
if (!mem)
return NULL;
+ /*
+* We don't currently expect this to ever be placed in the
+* stolen portion.
+*/
+ if (phys_base >= resource_size(>region)) {
+ drm_err(>drm,
+ "Initial plane programming using invalid range,
phys_base=%pa\n",
+ _base);
+ return NULL;
+ }
+
+ drm_dbg(>drm,
+ "Using phys_base=%pa, based on initial plane programming\n",
+ _base);
+
size = round_up(plane_config->base + plane_config->size,
mem->min_page_size);
size -= base;
--
2.43.0.472.g3155946c3a-goog
Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
On Tue, Nov 28, 2023 at 12:12:08PM +0100, Andrzej Hajda wrote:
> On 28.11.2023 04:47, Paz Zcharya wrote:
> >
> > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya wrote:
> > >
> > > On 21.11.2023 13:06, Andrzej Hajda wrote:
> > >
> > > > The simplest approach would be then do the same as in case of DGFX:
> > > > gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > > > gen8_pte_t pte;
> > > >
> > > > gte += base / I915_GTT_PAGE_SIZE;
> > > >
> > > > pte = ioread64(gte);
> > > > phys_base = pte & I915_GTT_PAGE_MASK;
> > > >
> > > > Regards
> > > > Andrzej
> >
> > Hey Andrzej,
> >
> > On a second thought, what do you think about something like
> >
> > + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > + gen8_pte_t pte;
> > + gte += base / I915_GTT_PAGE_SIZE;
> > + pte = ioread64(gte);
> > + pte = pte & I915_GTT_PAGE_MASK;
> > + phys_base = pte - i915->mm.stolen_region->region.start;
> >
> > The only difference is the last line.
>
> Bingo :) It seems to be generic algorithm to get phys_base for all
> platforms:
> - on older platforms stolen_region points to system memory which starts at
> 0,
> - on DG2 it uses lmem region which starts at 0 as well,
> - on MTL stolen_region points to stolen-local which starts at 0x80.
>
> So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced
> with sth generic.
> 1. Find pte.
> 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
> i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
> 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
>
> Regards
> Andrzej
>
>
Hey Andrzej,
I uploaded https://patchwork.freedesktop.org/series/127130/ based on
algorithm. Please take a look and let me know if you'd like me to change
anything.
Really appreciate all of your help!
Best,
Paz
[Intel-gfx] [PATCH] drm/i915/display: Check GGTT to determine phys_base
There was an assumption that for iGPU there should be a 1:1 mapping
of GGTT to physical address pointing to actual framebuffer.
This assumption is not valid anymore for MTL.
Fix that by checking GGTT to determine the phys address.
The following algorithm for phys_base should be valid for all platforms:
1. Find pte
2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
- On older platforms, stolen_region points to system memory, starting at 0
- on DG2, it uses lmem region which starts at 0 as well
- on MTL, stolen_region points to stolen-local which starts at 0x80
Signed-off-by: Paz Zcharya
---
.../drm/i915/display/intel_plane_initial.c| 45 +--
1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index a55c09cbd0e4..c4bf02ea966c 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -51,6 +51,8 @@ initial_plane_vma(struct drm_i915_private *i915,
struct intel_memory_region *mem;
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
+ gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
+ gen8_pte_t pte;
resource_size_t phys_base;
u32 base, size;
u64 pinctl;
@@ -59,44 +61,41 @@ initial_plane_vma(struct drm_i915_private *i915,
return NULL;
base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
- if (IS_DGFX(i915)) {
- gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
- gen8_pte_t pte;
+ gte += base / I915_GTT_PAGE_SIZE;
- gte += base / I915_GTT_PAGE_SIZE;
+ pte = ioread64(gte);
- pte = ioread64(gte);
+ if (IS_DGFX(i915)) {
if (!(pte & GEN12_GGTT_PTE_LM)) {
drm_err(>drm,
"Initial plane programming missing PTE_LM
bit\n");
return NULL;
}
-
- phys_base = pte & I915_GTT_PAGE_MASK;
mem = i915->mm.regions[INTEL_REGION_LMEM_0];
-
- /*
-* We don't currently expect this to ever be placed in the
-* stolen portion.
-*/
- if (phys_base >= resource_size(>region)) {
- drm_err(>drm,
- "Initial plane programming using invalid range,
phys_base=%pa\n",
- _base);
- return NULL;
- }
-
- drm_dbg(>drm,
- "Using phys_base=%pa, based on initial plane
programming\n",
- _base);
} else {
- phys_base = base;
mem = i915->mm.stolen_region;
}
+ phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
+
if (!mem)
return NULL;
+ /*
+* We don't currently expect this to ever be placed in the
+* stolen portion.
+*/
+ if (phys_base >= resource_size(>region)) {
+ drm_err(>drm,
+ "Initial plane programming using invalid range,
phys_base=%pa\n",
+ _base);
+ return NULL;
+ }
+
+ drm_dbg(>drm,
+ "Using phys_base=%pa, based on initial plane programming\n",
+ _base);
+
size = round_up(plane_config->base + plane_config->size,
mem->min_page_size);
size -= base;
--
2.43.0.rc1.413.gea7ed67945-goog
Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
On Tue, Nov 28, 2023 at 12:12:08PM +0100, Andrzej Hajda wrote:
> On 28.11.2023 04:47, Paz Zcharya wrote:
> >
> > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya wrote:
> >
> > Hey Andrzej,
> >
> > On a second thought, what do you think about something like
> >
> > + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> > + gen8_pte_t pte;
> > + gte += base / I915_GTT_PAGE_SIZE;
> > + pte = ioread64(gte);
> > + pte = pte & I915_GTT_PAGE_MASK;
> > + phys_base = pte - i915->mm.stolen_region->region.start;
> >
> > The only difference is the last line.
>
> Bingo :) It seems to be generic algorithm to get phys_base for all
> platforms:
> - on older platforms stolen_region points to system memory which starts at
> 0,
> - on DG2 it uses lmem region which starts at 0 as well,
> - on MTL stolen_region points to stolen-local which starts at 0x80.
>
> So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced
> with sth generic.
> 1. Find pte.
> 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
> i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
> 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
>
> Regards
> Andrzej
>
>
Good stuff!! I'll work on this revision and resubmit.
Thank you so much Andrzej!
Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya wrote: > > On 21.11.2023 13:06, Andrzej Hajda wrote: > > On 18.11.2023 00:01, Paz Zcharya wrote: > > > On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote: > > > > On Sun, Nov 05, 2023 at 05:27:03PM +, Paz Zcharya wrote: > > > > > > Hi Rodrigo, thanks for the great comments. > > > > > > Apologies for using a wrong/confusing terminology. I think 'phys_base' > > > is supposed to be the offset in the GEM BO, where base (or > > > "Surface Base Address") is supposed to be the GTT offset. > > > > Since base is taken from PLANE_SURF register it should be resolvable via > > GGTT to physical address pointing to actual framebuffer. > > I couldn't find anything in the specs. > > It was quite cryptic. I meant I have not found anything about assumption > from commit history that for iGPU there should be 1:1 mapping, this is why > there was an assignment "phys_base = base". Possibly the assumption is not > valid anymore for MTL(?). > Without the assumption we need to check GGTT to determine phys address. > > > The simplest approach would be then do the same as in case of DGFX: > > gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; > > gen8_pte_t pte; > > > > gte += base / I915_GTT_PAGE_SIZE; > > > > pte = ioread64(gte); > > phys_base = pte & I915_GTT_PAGE_MASK; > > > > Regards > > Andrzej Hey Andrzej, On a second thought, what do you think about something like + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; + gen8_pte_t pte; + gte += base / I915_GTT_PAGE_SIZE; + pte = ioread64(gte); + pte = pte & I915_GTT_PAGE_MASK; + phys_base = pte - i915->mm.stolen_region->region.start; The only difference is the last line. Based on what I wrote before, I think `phys_base` is named incorrectly and that it does not reflect the physical address, but the start offset of i915->mm.stolen_region. So if we offset the start value of the stolen region, this code looks correct to me (and it also works on my MeteorLake device). What do you think? Many thanks, Paz
Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
On Wed, Nov 22, 2023 at 02:26:55PM +0100, Andrzej Hajda wrote: > > > On 21.11.2023 13:06, Andrzej Hajda wrote: > > On 18.11.2023 00:01, Paz Zcharya wrote: > > > On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote: > > > > On Sun, Nov 05, 2023 at 05:27:03PM +, Paz Zcharya wrote: > > > > > > Hi Rodrigo, thanks for the great comments. > > > > > > Apologies for using a wrong/confusing terminology. I think 'phys_base' > > > is supposed to be the offset in the GEM BO, where base (or > > > "Surface Base Address") is supposed to be the GTT offset. > > > > Since base is taken from PLANE_SURF register it should be resolvable via > > GGTT to physical address pointing to actual framebuffer. > > I couldn't find anything in the specs. > > It was quite cryptic. I meant I have not found anything about assumption > from commit history that for iGPU there should be 1:1 mapping, this is why > there was an assignment "phys_base = base". Possibly the assumption is not > valid anymore for MTL(?). > Without the assumption we need to check GGTT to determine phys address. > > > The simplest approach would be then do the same as in case of DGFX: > > gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; > > gen8_pte_t pte; > > > > gte += base / I915_GTT_PAGE_SIZE; > > > > pte = ioread64(gte); > > phys_base = pte & I915_GTT_PAGE_MASK; > > > > Regards > > Andrzej Hey Andrzej, Sorry for the late response. I was OOO :) I tried using the code you mentioned. It translates (in the very specific case of MTL + GOP driver) to phys_base == 0080_h. Unfortunately, it results in a corrupted screen -- the framebuffer is filled with zeros. It seems like `i915_vma_pin_ww` already reserves and binds the GEM BO to the correct address space independently of the value of `phys_base`. The only thing `phys_base` affects is the value of `stolen->start` https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/gem/i915_gem_stolen.c#L747 So it seems to me that the maybe `phys_base` is named incorrectly and that it does not reflect the physical address, but the start offset of i915->mm.stolen_region. I'm happy to run more tests / debug further. Do you have more ideas of things to try? Many thanks, Paz
Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
> > Fix the value of variable `phys_base` to be the relative offset in
> > stolen memory, and not the absolute offset of the GSM.
>
> to me it looks like the other way around. phys_base is the physical
> base address for the frame_buffer. Setting it to zero doesn't seem
> to make that relative. And also doesn't look right.
>
> >
> > Currently, the value of `phys_base` is set to "Surface Base Address,"
> > which in the case of Meter Lake is 0xfc00_.
>
> I don't believe this is a fixed value. IIRC this comes from the register
> set by video bios, where the idea is to reuse the fb that was used so
> far.
>
> With this in mind I don't understand how that could overflow. Maybe
> the size of the stolen is not right? maybe the size? maybe different
> memory region?
>
Hi Rodrigo, thanks for the great comments.
Apologies for using a wrong/confusing terminology. I think 'phys_base'
is supposed to be the offset in the GEM BO, where base (or
"Surface Base Address") is supposed to be the GTT offset.
Other than what I wrote before, I noticed that the function 'i915_vma_pin'
which calls to 'i915_gem_gtt_reserve' is the one that binds the right
address space in the GTT for that stolen region.
I see that in the function 'i915_vma_insert' (full call stack below),
where if (flags & PIN_OFFSET_FIXED), then when calling 'i915_gem_gtt_reserve'
we add an offset.
Specifically in MeteorLake, and specifically when using GOP driver, this
offset is equal to 0xfc00_. But as you mentioned, this is not strict.
The if statement always renders true because in the function
'initial_plane_vma' we always set
pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
where pinctl == flags (see file 'intel_plane_initial.c' line 145).
Call stack:
drm_mm_reserve_node
i915_gem_gtt_reserve
i915_vma_insert
i915_vma_pin_ww
i915_vma_pin
initial_plane_vma
intel_alloc_initial_plane_obj
intel_find_initial_plane_obj
Therefore, I believe the variable 'phys_base' in the
function 'initial_plane_vma,' should be the the offset in the GEM BO
and not the GTT offset, and because the base is added later on
in the function 'i915_gem_gtt_reserve', this value should not be
equal to base and be 0.
Hope it makes more sense.
> > This causes the
> > function `i915_gem_object_create_region_at` to fail in line 128, when
> > it attempts to verify that the range does not overflow:
> >
> > if (range_overflows(offset, size, resource_size(>region)))
> > return ERR_PTR(-EINVAL);
> >
> > where:
> > offset = 0xfc00
> > size = 0x8ca000
> > mem->region.end + 1 = 0x440
> > mem->region.start = 0x80
> > resource_size(>region) = 0x3c0
> >
> > call stack:
> > i915_gem_object_create_region_at
> > initial_plane_vma
> > intel_alloc_initial_plane_obj
> > intel_find_initial_plane_obj
> > intel_crtc_initial_plane_config
> >
> > Looking at the flow coming next, we see that `phys_base` is only used
> > once, in function `_i915_gem_object_stolen_init`, in the context of
> > the offset *in* the stolen memory. Combining that with an
> > examinination of the history of the file seems to indicate the
> > current value set is invalid.
> >
> > call stack (functions using `phys_base`)
> > _i915_gem_object_stolen_init
> > __i915_gem_object_create_region
> > i915_gem_object_create_region_at
> > initial_plane_vma
> > intel_alloc_initial_plane_obj
> > intel_find_initial_plane_obj
> > intel_crtc_initial_plane_config
> >
> > [drm:_i915_gem_object_stolen_init] creating preallocated stolen
> > object: stolen_offset=0x, size=0x008ca000
> >
> > Signed-off-by: Paz Zcharya
> > ---
> >
> > drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index a55c09cbd0e4..e696cb13756a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> > "Using phys_base=%pa, based on initial plane
> > programming\n",
> > _base);
> > } else {
> > - phys_base = base;
> > + phys_base = 0;
> > mem = i915->mm.stolen_region;
> > }
> >
> > --
> > 2.42.0.869.gea05f2083d-goog
> >
[Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute
Fix the value of variable `phys_base` to be the relative offset in
stolen memory, and not the absolute offset of the GSM.
Currently, the value of `phys_base` is set to "Surface Base Address,"
which in the case of Meter Lake is 0xfc00_. This causes the
function `i915_gem_object_create_region_at` to fail in line 128, when
it attempts to verify that the range does not overflow:
if (range_overflows(offset, size, resource_size(>region)))
return ERR_PTR(-EINVAL);
where:
offset = 0xfc00
size = 0x8ca000
mem->region.end + 1 = 0x440
mem->region.start = 0x80
resource_size(>region) = 0x3c0
call stack:
i915_gem_object_create_region_at
initial_plane_vma
intel_alloc_initial_plane_obj
intel_find_initial_plane_obj
intel_crtc_initial_plane_config
Looking at the flow coming next, we see that `phys_base` is only used
once, in function `_i915_gem_object_stolen_init`, in the context of
the offset *in* the stolen memory. Combining that with an
examinination of the history of the file seems to indicate the
current value set is invalid.
call stack (functions using `phys_base`)
_i915_gem_object_stolen_init
__i915_gem_object_create_region
i915_gem_object_create_region_at
initial_plane_vma
intel_alloc_initial_plane_obj
intel_find_initial_plane_obj
intel_crtc_initial_plane_config
[drm:_i915_gem_object_stolen_init] creating preallocated stolen
object: stolen_offset=0x, size=0x008ca000
Signed-off-by: Paz Zcharya
---
drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c
b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index a55c09cbd0e4..e696cb13756a 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
"Using phys_base=%pa, based on initial plane
programming\n",
_base);
} else {
- phys_base = base;
+ phys_base = 0;
mem = i915->mm.stolen_region;
}
--
2.42.0.869.gea05f2083d-goog
Re: [Intel-gfx] [PATCH] drm/i915/display: Only fail fastset on PSR2
On Wed, Nov 01, 2023 at 06:26:47AM +, Hogander, Jouni wrote:
> On Tue, 2023-10-31 at 23:21 +0000, Paz Zcharya wrote:
> > Currently, i915 fails fastset if both the sink and the source support
> > any version of PSR and regardless of the configuration setting of the
> > driver (i.e., i915.enable_psr kernel argument). However, the
> > implementation of PSR1 enable sequence is already seamless
> > and works smoothly with fastset. Accordingly, do not fail fastset
> > if PSR2 is not enabled.
>
> Thank you for the patch. Check similar patch I sent some time ago to
> trybot:
>
> https://patchwork.freedesktop.org/series/125392/
>
I missed this patch. I apologize!
This is great work and exactly what we (Google ChromeOS) need.
I think your patch is better than mine, so let's abort my patch
and continue the discussion at series/125392.
By the way, we have verified your patch on two Meteor Lake devices
running ChromeOS and it works smoothly (no flickering or modesets).
I'll comment on the other patch as well.
> If we want to temporarily do this only for psr1 I think you could check
> what I've done in drivers/gpu/drm/i915/display/intel_display.c in my
> patch and modify your patch accordingly. Otherwise e.g. our IGT
> testcases which are toggling PSR enable/disable/psr1/psr2 are to my
> understanding performing full modeset and possible issues are not
> revealed.
>
> After modifying drivers/gpu/drm/i915/display/intel_display.c you can
> also verify it is really seamless and smooth by toggling different PSR
> states via /sys/kernel/debug/dri/0/i915_edp_psr_debug. That interface
> is performing atomic commit when PSR mode is changed.
>
> BR,
>
> Jouni Högander
> >
> > Signed-off-by: Paz Zcharya
> > ---
> >
> > drivers/gpu/drm/i915/display/intel_dp.c | 4 ++--
> > drivers/gpu/drm/i915/display/intel_psr.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_psr.h | 1 +
> > 3 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index e0e4cb529284..a1af96e31518 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2584,8 +2584,8 @@ bool intel_dp_initial_fastset_check(struct
> > intel_encoder *encoder,
> > fastset = false;
> > }
> >
> > - if (CAN_PSR(intel_dp)) {
> > - drm_dbg_kms(>drm, "[ENCODER:%d:%s] Forcing full
> > modeset to compute PSR state\n",
> > + if (CAN_PSR(intel_dp) && psr2_global_enabled(intel_dp)) {
> > + drm_dbg_kms(>drm, "[ENCODER:%d:%s] Forcing full
> > modeset due to PSR2\n",
> > encoder->base.base.id, encoder-
> > >base.name);
> > crtc_state->uapi.mode_changed = true;
> > fastset = false;
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 97d5eef10130..388bc3246db9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -187,7 +187,7 @@ static bool psr_global_enabled(struct intel_dp
> > *intel_dp)
> > }
> > }
> >
> > -static bool psr2_global_enabled(struct intel_dp *intel_dp)
> > +bool psr2_global_enabled(struct intel_dp *intel_dp)
> > {
> > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > b/drivers/gpu/drm/i915/display/intel_psr.h
> > index 0b95e8aa615f..6f3c36389cd3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -21,6 +21,7 @@ struct intel_encoder;
> > struct intel_plane;
> > struct intel_plane_state;
> >
> > +bool psr2_global_enabled(struct intel_dp *intel_dp);
> > void intel_psr_init_dpcd(struct intel_dp *intel_dp);
> > void intel_psr_pre_plane_update(struct intel_atomic_state *state,
> > struct intel_crtc *crtc);
>
.
[Intel-gfx] [PATCH] drm/i915/display: Only fail fastset on PSR2
Currently, i915 fails fastset if both the sink and the source support
any version of PSR and regardless of the configuration setting of the
driver (i.e., i915.enable_psr kernel argument). However, the
implementation of PSR1 enable sequence is already seamless
and works smoothly with fastset. Accordingly, do not fail fastset
if PSR2 is not enabled.
Signed-off-by: Paz Zcharya
---
drivers/gpu/drm/i915/display/intel_dp.c | 4 ++--
drivers/gpu/drm/i915/display/intel_psr.c | 2 +-
drivers/gpu/drm/i915/display/intel_psr.h | 1 +
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index e0e4cb529284..a1af96e31518 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2584,8 +2584,8 @@ bool intel_dp_initial_fastset_check(struct intel_encoder
*encoder,
fastset = false;
}
- if (CAN_PSR(intel_dp)) {
- drm_dbg_kms(>drm, "[ENCODER:%d:%s] Forcing full modeset
to compute PSR state\n",
+ if (CAN_PSR(intel_dp) && psr2_global_enabled(intel_dp)) {
+ drm_dbg_kms(>drm, "[ENCODER:%d:%s] Forcing full modeset
due to PSR2\n",
encoder->base.base.id, encoder->base.name);
crtc_state->uapi.mode_changed = true;
fastset = false;
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
b/drivers/gpu/drm/i915/display/intel_psr.c
index 97d5eef10130..388bc3246db9 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -187,7 +187,7 @@ static bool psr_global_enabled(struct intel_dp *intel_dp)
}
}
-static bool psr2_global_enabled(struct intel_dp *intel_dp)
+bool psr2_global_enabled(struct intel_dp *intel_dp)
{
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
b/drivers/gpu/drm/i915/display/intel_psr.h
index 0b95e8aa615f..6f3c36389cd3 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -21,6 +21,7 @@ struct intel_encoder;
struct intel_plane;
struct intel_plane_state;
+bool psr2_global_enabled(struct intel_dp *intel_dp);
void intel_psr_init_dpcd(struct intel_dp *intel_dp);
void intel_psr_pre_plane_update(struct intel_atomic_state *state,
struct intel_crtc *crtc);
--
2.42.0.820.g83a721a137-goog
