- 07 Feb, 2024 5 commits
-
-
Ville Syrjälä authored
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. Reviewed-by:
Uma Shankar <uma.shankar@intel.com> Tested-by:
Paz Zcharya <pazz@chromium.org> Signed-off-by:
Ville Syrjälä <ville.syrjala@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20240202224340.30647-17-ville.syrjala@linux.intel.com
-
Ville Syrjälä authored
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) v3: "fix" xe Reviewed-by:Andrzej Hajda <andrzej.hajda@intel.com> Tested-by:
Lucas De Marchi <lucas.demarchi@intel.com>
-
Ville Syrjälä authored
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... v2: "fix" xe Reviewed-by:
-
Ville Syrjälä authored
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... Reviewed-by:
-
Ville Syrjälä authored
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. v2: "fix" xe Reviewed-by:
-
- 06 Feb, 2024 11 commits
-
-
Ville Syrjälä authored
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 Reviewed-by: -
Ville Syrjälä authored
The address we read from the PTE is a dma address, not a physical address. Rename the variable to say so. Reviewed-by:
-
Ville Syrjälä authored
MTL stolen memory looks more like local memory, so use the (now fixed) lmem path when doing the initial plane readout. Reviewed-by:
-
Ville Syrjälä authored
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. Reviewed-by:
-
Ville Syrjälä authored
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 Reviewed-by:
Nirmoy Das <nirmoy.das@intel.com> Tested-by:
-
Ville Syrjälä authored
0x108100 and 0x1080c0 have been around since snb. Rename the defines appropriately. v2: Rebase Reviewed-by:
-
Ville Syrjälä authored
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() Reviewed-by:
-
Ville Syrjälä authored
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: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Reviewed-by:Radhakrishna Sripada <radhakrishna.sripada@intel.com> Reviewed-by:
-
Ville Syrjälä authored
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. Reviewed-by:
-
Ville Syrjälä authored
Dump the details about every memory region into dmesg at probe time. Avoids having to dig those out from random places when debugging stuff. Reviewed-by:
-
Ville Syrjälä authored
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(&M->io) @@ expression M; @@ - M.io_size + resource_size(&M.io) Reviewed-by:
-
- 05 Feb, 2024 2 commits
-
-
Ville Syrjälä authored
AFAICS there is no hardware restriction on where in ggtt the hdcp gsc message object needs to be bound. And as it's a regular shmem object we don't need it be in the mappabe range either. So pin it high to make avoid needlessly wasting the precious mappable range for it. Cc: Suraj Kandpal <suraj.kandpal@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> Cc: Uma Shankar <uma.shankar@intel.com> Signed-off-by:
-
Ville Syrjälä authored
intel_hdcp_component_init()->...->intel_hdcp_gsc_initialize_message() will allocate ggtt address space for some hdcp gsc message thing. That is currently being done way too early as we haven't even taken over the BIOS fb yet. So this has the potential of corrupting ggtt PTEs that need to be preserved until the BIOS fb takover is done. Only call intel_hdcp_component_init() once all the BIOS fb takeover, and full ggtt init (which currently also needs to reserve very specific ranges of ggtt, thus assuming that no one else has stolen them yet) is done. Cc: Suraj Kandpal <suraj.kandpal@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> Cc: Uma Shankar <uma.shankar@intel.com> Signed-off-by:
-
- 02 Feb, 2024 5 commits
-
-
Ville Syrjälä authored
Only display workarounds 0391 and 0475 call for disabling FBC with render compression, and those are listed only for pre-prod SKL steppings. So it should be safe to enable FB+CCS on production hardware. AFAIK CCS is limited to 50% bandwidth reduction (perhaps clear color can do better?). FBC can exceed that number by quite a bit, given the right kind of framebuffer contents. So piling on both kinds of compressions could still make sense. Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10125Signed-off-by:
Rodrigo Vivi <rodrigo.vivi@intel.com>
-
Ville Syrjälä authored
Pull all the state swap stuff into its own function to declutter intel_atomic_commit() a bit. Note that currently the state swap is spread across both sides of the unprepare branch in intel_atomic_commit(), but we can pull all of it ahead a bit since we bail on the first error, and thus there is no change in behaviour from the reordering. Signed-off-by:
Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
-
Ville Syrjälä authored
Instead of injecting extra crtc commits to serialize the global state let's hand roll a bit of commit machinery to take care of the hardware synchronization. Rather than basing everything on the crtc commits we track these as their own thing. I think this makes more sense as the hardware blocks we are working with are not in any way tied to the pipes, so the completion should not be tied in with the vblank machinery either. The difference to the old behaviour is that: - we no longer pull extra crtcs into the commit which should make drm_atomic_check_only() happier - since those crtcs don't get pulled in we also don't end up reprogamming them and thus don't need to wait their vblanks to pass/etc. So this should be tad faster as well. TODO: perhaps have each global object complete its own commit once the post-plane update phase is done? Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6728Signed-off-by:
-
Ville Syrjälä authored
drm_atomic_check_only() gets upset if we try to add extra crtcs to any commit that isn't flagged with DRM_MODE_ATOMIC_ALLOW_MODESET. This conflicts with how SAGV watermarks work on pre-ADL as we need to manually switch over the SAGV watermarks before we can safely enable SAGV. So in order to make SAGV usage possible we need to compute each pipe's use of SAGV watermarks as if there aren't any other active pipes. Ie. if the current pipe isn't the one blocking SAGV then we make it use the SAGV watermarks, even if some other pipe prevents SAGV from actually being used. Otherwise we could end up with a pipes using the normal watermarks (but not blocking SAGV), and some other pipe in parallel enabling SAGV, which would likely cause underruns. The alternative approach of preventing SAGV usage until all pipes simultanously end up using SAGV watermarks would only really work if userspace always adds all pipes to every commits, which isn't the case typically. The downside of this is that we will end up using the less optimal SAGV watermarks even if some other pipe prevents SAGV from actually being enabled. In which case the system won't achieve the minimum possible power consumption. Signed-off-by:
-
Paz Zcharya authored
Commit 8015bee0 ("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 <linux/debugfs.h>. The missing inclusion of this header file is breaking the ChromeOS build -- add an explicit include to fix that. Signed-off-by:
Jani Nikula <jani.nikula@intel.com> Signed-off-by:
-
- 30 Jan, 2024 2 commits
-
-
Lucas De Marchi authored
Some registers for DDI A/B moved to PICA and now follow the same format as the ones for the PORT_TC ports. The wrapper here deals with 2 issues: - Share the implementation between xe2lpd and previous platforms: there are minor layout changes, it's mostly the register location that changed - Handle offsets after TC ports v2: - Explain better the trick to use just the second range (Matt Roper) - Add missing conversions after rebase (Matt Roper) - Use macro instead of inline function, avoiding includes in the header (Jani) - Prefix old macros with underscore so they don't get used by mistake, and name the new ones using the previous names v3: Use the same logic for the recently-introduced XELPDP_PORT_MSGBUS_TIMER (Gustavo) Signed-off-by:Matt Roper <matthew.d.roper@intel.com> Reviewed-by:
Gustavo Sousa <gustavo.sousa@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20240126224638.4132016-3-lucas.demarchi@intel.com
-
Lucas De Marchi authored
Bits to enable/disable and check state for D2D moved from XELPDP_PORT_BUF_CTL1 to DDI_BUF_CTL (now named DDI_CTL_DE in the spec). Make the functions mtl_ddi_disable_d2d() and mtl_ddi_enable_d2d generic to work with multiple reg location and bitfield layout. v2: Set/Clear XE2LPD_DDI_BUF_D2D_LINK_ENABLE in saved_port_bits when enabling/disabling D2D so DDI_BUF_CTL is correctly programmed in other places without overriding these bits (Clint) v3: Leave saved_port_bits alone as those bits are not meant to be modified outside of the port initialization. Rather propagate the additional bit in DDI_BUF_CTL to be set when that register is written again after D2D is enabled. Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by:
-
- 26 Jan, 2024 3 commits
-
-
Ville Syrjälä authored
No real reason why the PLL flags need to be a bitmask. Switch to booleans to make the code simpler. Signed-off-by:
-
Ville Syrjälä authored
TC ports have both the MG/TC and TBT PLLs selected simultanously (so that we can switch from MG/TC to TBT as a fallback). This doesn't play well with the state checker that assumes that the old PLL shouldn't have the pipe in its pipe_mask anymore. Suppress that check for these PLLs to avoid spurious WARNs when you disconnect a TC port and a non-disabling modeset happens before actually disabling the port. v2: Only suppress when one of the PLLs is the TBT PLL and the other one is not Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/9816Signed-off-by: -
Ville Syrjälä authored
Make the log easier to parse by including the name of the PLL in the debug prints regarding said PLL. Signed-off-by:
-
- 23 Jan, 2024 1 commit
-
-
Ville Syrjälä authored
Currently icl_compute_tc_phy_dplls() assumes that the active PLL will be the TC PLL (as opposed to the TBT PLL). The actual PLL will be selected during the modeset enable sequence, but we need to put *something* into the crtc_state->shared_dpll already during compute_config(). The downside of assuming one PLL or the other is that we'll fail to fastset if the assumption doesn't match what was in use previously. So let's instead keep the same PLL that was in use previously (assuming there was one). This should allow fastset to work again when using TBT PLL, at least in the steady state. Now, assuming we want keep the same PLL may not be entirely correct either. But we should be covered by the type-c link reset handling which will force a full modeset by flagging connectors_changed=true which means the resulting modeset can't be converted into a fastset even if the full crtc state looks identical. Cc: Imre Deak <imre.deak@intel.com> Cc: Suraj Kandpal <suraj.kandpal@intel.com> Signed-off-by:
Suraj Kandpal <suraj.kandpal@intel.com>
-
- 22 Jan, 2024 9 commits
-
-
Ville Syrjälä authored
This reverts commit cfeff354. A core design consideration with legacy cursor updates is that the cursor must not touch any other plane, even if we were to force it to take the slow path. That is the real reason why the cursor uses a fixed ddb allocation, not because bspec says so. Treating cursors as any other plane during ddb allocation violates that, which means we can now pull other planes into fully unsynced legacy cursor mailbox commits. That is definitely not something we've ever considered when designing the rest of the code. The noarm+arm register write split in particular makes that dangerous as previous updates can get disarmed pretty much at any random time, and not necessarily in an order that is actually safe (eg. against ddb overlaps). So if we were to do this then: - someone needs to expend the appropriate amount of brain cells thinking through all the tricky details - we should do it for all skl+ platforms since all of those have double buffered wm/ddb registers. The current arbitrary mtl+ cutoff doesn't really make sense For the moment just go back to the original behaviour where the cursor's ddb alloation does not change outside of modeset/fastset. As of now anything else isn't safe. Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by:
-
Ville Syrjälä authored
Our legacy cursor updates are actually mailbox updates. Ie. the hardware latches things once per frame on start of vblank, but we issue an number of updates per frame, withough any attempt to synchronize against the vblank in software. So in theory only the last update issued during the frame will latch, and the previous ones are discarded. However this can lead to problems with maintaining the ggtt/iommu mappings as we have no idea which updates will actually latch. The problem is exacerbated by the hardware's annoying disarming behaviour; any non-arming register write will disarm an already armed update, only to be rearmed later by the arming register (CURBASE in case of cursors). If a disarming write happens just before the start of vblank, and the arming write happens after start of vblank we have effectively prevented the hardware from latching anything. And if we manage to straddle multiple sequential vblank starts in this manner we effectively prevent the hardware from latching any new registers for an arbitrary amount of time. This provides more time for the (potentially still in use by the hardware) gtt/iommu mappings to be torn down. A partial solution, of course, is to use vblank evasion to avoid the register writes from spreading on both sides of the start of vblank. I've previously highlighted this problem as a general issue affecting mailbox updates. I even added some notes to the {i9xx,skl}_crtc_planes_update_arm() to remind us that the noarm and arm phases both need to pulled into the vblank evasion critical section if we actually decided to implement mailbox updates in general. But as I never impelemented the noarm+arm split for cursors we don't have to worry about that for the moment. We've been lucky enough so far that this hasn't really caused problems. One thing that does help is that Xorg generally sticks to the same cursor BO. But igt seems pretty good at hitting this on MTL now, so apparently we have to start thinking about this. v2: Wait for PSR exit to avoid the vblank evasion timeout (1ms) tripping due to PSR exit latency (~5ms typically) Reviewed-by:Jouni Högander <jouni.hogander@intel.com> Reviewed-by:
-
Ville Syrjälä authored
intel_vblank.c seems like the appropriate place for the core vblank evasion code. Move it there. Signed-off-by:
-
Ville Syrjälä authored
There isn't really any reason to make the caller suffer through checking the vblank evasion min/max scanlines. If we somehow ended up with bogus values (which really shouldn't happen) then just skip the actual vblank evasion loop but otherwise plow ahead as normal. The only "real" change is that we now get+put a vblank reference even if the min/max values are bogus, previously we skipped directly to the end. Signed-off-by:
-
Ville Syrjälä authored
Pull the core vblank evasion loop into its own function, so that we can reuse it elsewhere later. Signed-off-by:
-
Ville Syrjälä authored
Pull the need_vlv_dsi_wa details into intel_vblank_evade_init() so that caller doesn't have to care about it. Signed-off-by:
-
Ville Syrjälä authored
Collect the information needed for vblank evasions into a structure that we can pass around more easily. And let's rename intel_crtc_vblank_evade_scanlines() to just intel_vblank_evade_init() so that better describes the intended usage of initializing the context. Signed-off-by:
-
Ville Syrjälä authored
Drop the vblank reference only after we've done the hideous need_vlv_dsi_wa stuff. This will make it easier to reuse the the vblank evasion machinery elsewhere. Keeping the vblank reference for a bit longer is not a problem. In fact we might want to not drop it at all until intel_pipe_update_end(), but we'll leave that idea for later. Signed-off-by:
-
Ville Syrjälä authored
We'll be needing to do vblank evasion around legacy cursor updates, which don't have the intel_atomic_state around. So let's remove this dependency on a full commit and pass the crtc state in by hand. Signed-off-by:
-
- 19 Jan, 2024 2 commits
-
-
Ville Syrjälä authored
Declaring a struct and immediately zeroing it with memset() seems a bit silly to me. Just zero initialize the struct when declaring it. Signed-off-by:
Mika Kahola <mika.kahola@intel.com>
-
Ville Syrjälä authored
On HSW non-ULT (or at least on Dell Latitude E6540) external displays start to flicker when we enable PSR on the eDP. We observe a much higher SR and PC6 residency than should be possible with an external display, and indeen much higher than what we observe with eDP disabled and only the external display enabled. Looks like the hardware is somehow ignoring the fact that the external display is active during PSR. I wasn't able to redproduce this on my HSW ULT machine, or BDW. So either there's something specific about this particular laptop (eg. some unknown firmware thing) or the issue is limited to just non-ULT HSW systems. All known registers that could affect this look perfectly reasonable on the affected machine. As a workaround let's unmask the LPSP event to prevent PSR entry except while in LPSP mode (only pipe A + eDP active). This will prevent PSR entry entirely when multiple pipes are active. The one slight downside is that we now also prevent PSR entry when driving eDP with pipe B or C, but I think that's a reasonable tradeoff to avoid having to implement a more complex workaround. Cc: stable@vger.kernel.org Fixes: 783d8b80 ("drm/i915/psr: Re-enable PSR1 on hsw/bdw") Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10092Signed-off-by:
-
