cdclk_compute_crawl_and_squash_midpoint() was still assuming
that cd2x divider == 1 (ie. full divider == 2). Remove that
assumption by computing the dividers properly.
We'll also toss in a WARN in case the divider somehow ends
up different between the old and new cdclk configs. That should
never happen given we have div==2 in all the cdclk table entries
for the affected platforms.
If in the future we need a config where the divider also needs
to be changed then we likely need to add an extra step into the
cdclk programming sequence to make sure things stay within
legal limits throughout the process.
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240207013334.29606-4-ville.syrjala@linux.intel.com
Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
Prevent accessing the HW from the get_modes hooks of connectors deriving
the mode list from the display's EDID. drm_edid_connector_add_modes()
will return the mode list based on the EDID which was cached during a
previous detection/get_modes call.
This also fixes the NULL deref problem (10085) which was
introduced/revealed by
commit bab87ef4db ("drm/i915: Disable hotplug detection handlers during driver init/shutdown")
After the above change MST connectors will not change state during
driver init/shutdown; thus some of these connectors with no I2C/DDC
adapter registered for them (since the given MST port has no sink
connected) may stay then in the 'unknown' connector status. The
get_modes() hook should not try to use the I2C/DDC adapter in this state
(which would lead to the above NULL deref) which this patch ensures.
v2:
- Remove the redundant check from intel_crt_ddc_get_modes().
- Rebase on latest drm-tip.
- Add Fixes: line / related commit notes.
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10085
Fixes: bab87ef4db ("drm/i915: Disable hotplug detection handlers during driver init/shutdown")
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Jouni Högander <jouni.hogander@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240212175237.2625812-2-imre.deak@intel.com
Prevent accessing the HW from the SDVO/TV get_modes connector hook.
Returning 0 from the hook will make the caller -
drm_helper_probe_single_connector_modes() - return a default/EDID
override mode list to users. This matches the case where
intel_sdvo_get_tv_modes() fails to retrieve the current mode list due to
a HW access failure.
v2: Clarify the commit message wrt. which modes get_modes() returns. (Jouni)
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Jouni Högander <jouni.hogander@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240212175237.2625812-1-imre.deak@intel.com
For validation purposes, it might be useful to be able to
force Bigjoiner mode, even if current dotclock/resolution
do not require that.
Lets add such to option to debugfs.
v2: - Apparently intel_dp_need_bigjoiner can't be used, when
debugfs entry is created so lets just check manually
the DISPLAY_VER.
v3: - Switch to intel_connector from drm_connector(Jani Nikula)
- Remove redundant modeset lock(Jani Nikula)
- Use kstrtobool_from_user for boolean value(Jani Nikula)
v4: - Apply the changes to proper function(Jani Nikula)
v5: - Removed unnecessary check from i915_bigjoiner_enable_show
(Ville Syrjälä)
- Added eDP connector check to intel_connector_debugfs_add
(Ville Syrjälä)
- Removed debug message in order to prevent dmesg flooding
(Ville Syrjälä)
v6: - Assume now always that m->private is intel_connector
- Fixed other similar conflicts
v7: - Move bigjoiner force option to intel_connector(Ville Syrjälä)
- Use DEFINE_SHOW_STORE_ATTRIBUTE instead of defining fops
manually.(Ville Syrjälä)
v8: - Pass intel_connector to debugfs_create_file, instead of drm_connector.
(Jani Nikula)
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240212125011.66174-1-uma.shankar@intel.com
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: 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-16-ville.syrjala@linux.intel.com
Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
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: Andrzej Hajda <andrzej.hajda@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-15-ville.syrjala@linux.intel.com
Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
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: Andrzej Hajda <andrzej.hajda@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-14-ville.syrjala@linux.intel.com
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: 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-9-ville.syrjala@linux.intel.com
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: Andrzej Hajda <andrzej.hajda@intel.com>
Acked-by: Nirmoy Das <nirmoy.das@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-8-ville.syrjala@linux.intel.com
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: Nirmoy Das <nirmoy.das@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-6-ville.syrjala@linux.intel.com
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: Andrzej Hajda <andrzej.hajda@intel.com>
Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Reviewed-by: Nirmoy Das <nirmoy.das@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-5-ville.syrjala@linux.intel.com
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: Andrzej Hajda <andrzej.hajda@intel.com>
Acked-by: Nirmoy Das <nirmoy.das@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-2-ville.syrjala@linux.intel.com
