GuC issue
Hi, I'm currently implementing GuC/HuC firmware support in one Safety Critical OS. I'm following i915 code and I implemented all paths (I don't want GuC submission or SLPC features). I need GuC to authenticate HuC firmware blob. I mirrored GuC implementation in my code. After GuC DMA transfer succeeds, I'm reading GUC_STATUS register. HW returns INTEL_BOOTROM_STATUS_JUMP_PASSED as bootrom status and INTEL_GUC_LOAD_STATUS_LAPIC_DONE as GuC load status. Unfortunately, after one second of waiting, the status didn't get changed to INTEL_GUC_LOAD_STATUS_READY at all. What is a potential issue here? Could you please help me? In addition to this, could you please point out some documentation about GuC's ADS struct? Thanks, Maksym
Re: [PATCH v3 1/4] drm/i915/alpm: Add ALPM register definitions
On Tue, 2024-02-06 at 15:25 +, Murthy, Arun R wrote: > > > -Original Message- > > From: Hogander, Jouni > > Sent: Tuesday, January 30, 2024 4:41 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Murthy, Arun R ; Hogander, Jouni > > ; Nikula, Jani > > Subject: [PATCH v3 1/4] drm/i915/alpm: Add ALPM register > > definitions > > > > Add ALPM register definitions for Lunar Lake. > > > > v3: > > - Fix ALPM_CTL2_A address > > - Remove duplicate defines > > v2: > > - Use REG_BIT instead of BIT > > - Add commit message > > > > Cc: Jani Nikula > > > > Signed-off-by: Jouni Högander > > --- > Reviewed-by: Arun R Murthy Thank you Arun for your review. These are now pushed to drm-intel-next. BR, Jouni Högander > > Thanks and Regards, > Arun R Murthy > > > drivers/gpu/drm/i915/display/intel_psr_regs.h | 57 > > +++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h > > b/drivers/gpu/drm/i915/display/intel_psr_regs.h > > index bc252f38239e..8427a736f639 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr_regs.h > > +++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h > > @@ -296,4 +296,61 @@ > > > > _SEL_FETCH_PLANE_OFFSET_1_A - \ > > > > _SEL_FETCH_PLANE_BASE_1_A) > > > > +#define _ALPM_CTL_A0x60950 > > +#define ALPM_CTL(tran) _MMIO_TRANS2(tran, _ALPM_CTL_A) > > +#define ALPM_CTL_ALPM_ENABLE REG_BIT(31) > > +#define ALPM_CTL_ALPM_AUX_LESS_ENABLE REG_BIT(30) > > +#define ALPM_CTL_LOBF_ENABLE REG_BIT(29) > > +#define ALPM_CTL_EXTENDED_FAST_WAKE_ENABLEREG_BIT(28) > > +#define ALPM_CTL_KEEP_FEC_ENABLE_FOR_AUX_WAKE_SLEEP REG_BIT(27) > > +#define ALPM_CTL_RESTORE_OCCURED REG_BIT(26) > > +#define ALPM_CTL_RESTORE_TO_SLEEP REG_BIT(25) > > +#define > > ALPM_CTL_RESTORE_TO_DEEP_SLEEPREG_BIT(24) > > +#define ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_MASK > > REG_GENMASK(23, 21) > > +#define ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_50_SYMBOLS > > REG_FIELD_PREP(ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_MASK, 0) > > +#define ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_128_SYMBOLS > > REG_FIELD_PREP(ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_MASK, 1) > > +#define ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_256_SYMBOLS > > REG_FIELD_PREP(ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_MASK, 2) > > +#define ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_512_SYMBOLS > > REG_FIELD_PREP(ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_MASK, 3) > > +#define ALPM_CTL_AUX_WAKE_SLEEP_HOLD_ENABLE REG_BIT(20) > > +#define ALPM_CTL_ALPM_ENTRY_CHECK_MASK > > REG_GENMASK(19, 16) > > +#define ALPM_CTL_ALPM_ENTRY_CHECK(val) > > REG_FIELD_PREP(ALPM_CTL_ALPM_ENTRY_CHECK_MASK, val) > > +#define ALPM_CTL_EXTENDED_FAST_WAKE_TIME_MASK > > REG_GENMASK(13, 8) > > +#define ALPM_CTL_EXTENDED_FAST_WAKE_MIN_LINES 5 > > +#define ALPM_CTL_EXTENDED_FAST_WAKE_TIME(lines) > > REG_FIELD_PREP(ALPM_CTL_EXTENDED_FAST_WAKE_TIME_MASK, > > (lines) - ALPM_CTL_EXTENDED_FAST_WAKE_MIN_LINES) > > +#define ALPM_CTL_AUX_LESS_WAKE_TIME_MASK > > REG_GENMASK(5, 0) > > +#define ALPM_CTL_AUX_LESS_WAKE_TIME(val) > > REG_FIELD_PREP(ALPM_CTL_AUX_LESS_WAKE_TIME_MASK, val) > > + > > +#define _ALPM_CTL2_A 0x60954 > > +#define ALPM_CTL2(tran)_MMIO_TRANS2(tran, _ALPM_CTL2_A) > > +#define ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY_MASK > > REG_GENMASK(28, 24) > > +#define ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY(val) > > REG_FIELD_PREP(ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY_MASK, > > val) > > +#define ALPM_CTL2_AUX_LESS_WAKE_TIME_EXTENSION_MASK > > REG_GENMASK(19, 16) > > +#define ALPM_CTL2_AUX_LESS_WAKE_TIME_EXTENSION(val) > > REG_FIELD_PREP(ALPM_CTL2_AUX_LESS_WAKE_TIME_EXTENSION_MA > > SK, val) > > +#define ALPM_CTL2_NUMBER_OF_LTTPR_MASK > > REG_GENMASK(15, 12) > > +#define ALPM_CTL2_NUMBER_OF_LTTPR(val) > > REG_FIELD_PREP(ALPM_CTL2_NUMBER_OF_LTTPR_MASK, val) > > +#define ALPM_CTL2_LTTPR_AUX_LESS_SLEEP_HOLD_TIME_MASK > > REG_GENMASK(10, 8) > > +#define ALPM_CTL2_LTTPR_AUX_LESS_SLEEP_HOLD_TIME(val) > > REG_FIELD_PREP(ALPM_CTL2_LTTPR_AUX_LESS_SLEEP_HOLD_TIME_M > > ASK, val) > > +#define ALPM_CTL2_FEC_DECODE_EN_POSITION_AFTER_WAKE_SR > > REG_BIT(4) > > +#define > > ALPM_CTL2_NUMBER_AUX_LESS_ML_PHY_SLEEP_SEQUENCES_MASK > > REG_GENMASK(2, 0) > > +#define ALPM_CTL2_NUMBER_AUX_LESS_ML_PHY_SLEEP_SEQUENCES(val) > > REG_FIELD_PREP(ALPM_CTL2_NUMBER_AUX_LESS_ML_PHY_SLEEP_SE > > QUENCES_MASK, val) > > + > > +#define _PORT_ALPM_CTL_A 0x16fa2c > > +#define PORT_ALPM_CTL(tran)_MMIO_TRANS2(tran, > > _PORT_ALPM_CTL_A) > > +#define PORT_ALPM_CTL_ALPM_AUX_LESS_ENABLEREG_BIT(31) > > +#define PORT_ALPM_CTL_MAX_PHY_SWING_SETUP_MASK > > REG_GENMASK(23, 20) >
Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
On 06/02/2024 20:51, Souza, Jose wrote:
On Tue, 2024-02-06 at 12:42 -0800, John Harrison wrote:
On 2/6/2024 08:33, Tvrtko Ursulin wrote:
On 01/02/2024 18:25, Souza, Jose wrote:
On Wed, 2024-01-24 at 08:55 +, Tvrtko Ursulin wrote:
On 24/01/2024 08:19, Joonas Lahtinen wrote:
Add reporting of the GuC submissio/VF interface version via GETPARAM
properties. Mesa intends to use this information to check for old
firmware versions with known bugs before enabling features like async
compute.
There was
https://patchwork.freedesktop.org/patch/560704/?series=124592&rev=1
which does everything in one go so would be my preference.
IMO Joonas version brings less burden to be maintained(no new struct).
But both versions works, please just get into some agreement so we
can move this forward.
So I would really prefer the query. Simplified version would do like
the compile tested only:
Vivaik's patch is definitely preferred. It is much cleaner to make one
single call than having to make four separate calls. It is also
extensible to other firmwares if required. The only blockage against it
was whether it was a good thing to report at all. If that blockage is no
longer valid then we should just merge the patch that has already been
discussed, polished, fixed, etc. rather than starting the whole process
from scratch.
Agreed.
Vivaik can you please rebase and send it again?
Note there was review feedback not addressed so do that too please.
AFAIR incorrect usage of copy item, pad/rsvd/mbz checking and questions
about padding in general. Last is why I proposed a simplified version
which is not future extensible and avoids the need for padding.
Regards,
Tvrtko
And note that it is four calls not three. The code below is missing the
branch version number.
John.
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..999687f6a3d4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct
drm_i915_private *i915,
return hwconfig->size;
}
+static int
+query_guc_submission_version(struct drm_i915_private *i915,
+ struct drm_i915_query_item *query)
+{
+ struct drm_i915_query_guc_submission_version __user *query_ptr =
+ u64_to_user_ptr(query->data_ptr);
+ struct drm_i915_query_guc_submission_version ver;
+ struct intel_guc *guc = &to_gt(i915)->uc.guc;
+ const size_t size = sizeof(ver);
+ int ret;
+
+ if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
+ return -ENODEV;
+
+ ret = copy_query_item(&ver, size, size, query);
+ if (ret != 0)
+ return ret;
+
+ if (ver.major || ver.minor || ver.patch)
+ return -EINVAL;
+
+ ver.major = guc->submission_version.major;
+ ver.minor = guc->submission_version.minor;
+ ver.patch = guc->submission_version.patch;
+
+ if (copy_to_user(query_ptr, &ver, size))
+ return -EFAULT;
+
+ return 0;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private
*dev_priv,
struct drm_i915_query_item
*query_item) = {
query_topology_info,
@@ -559,6 +590,7 @@ static int (* const i915_query_funcs[])(struct
drm_i915_private *dev_priv,
query_memregion_info,
query_hwconfig_blob,
query_geometry_subslices,
+ query_guc_submission_version,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct
drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 550c496ce76d..d80d9b5e1eda 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3038,6 +3038,7 @@ struct drm_i915_query_item {
* - %DRM_I915_QUERY_MEMORY_REGIONS (see struct
drm_i915_query_memory_regions)
* - %DRM_I915_QUERY_HWCONFIG_BLOB (see `GuC HWCONFIG blob
uAPI`)
* - %DRM_I915_QUERY_GEOMETRY_SUBSLICES (see struct
drm_i915_query_topology_info)
+ * - %DRM_I915_QUERY_GUC_SUBMISSION_VERSION (see struct
drm_i915_query_guc_submission_version)
*/
__u64 query_id;
#define DRM_I915_QUERY_TOPOLOGY_INFO 1
@@ -3046,6 +3047,7 @@ struct drm_i915_query_item {
#define DRM_I915_QUERY_MEMORY_REGIONS 4
#define DRM_I915_QUERY_HWCONFIG_BLOB 5
#define DRM_I915_QUERY_GEOMETRY_SUBSLICES 6
+#define DRM_I915_QUERY_GUC_SUBMISSION_VERSION 7
/* Must be kept compact -- no holes and well documented */
/**
@@ -3591,6 +3593,15 @@ struct drm_i915_query_memory_regions {
struct drm_i915_memory_region_info regions[];
};
+/**
+* struct drm_i915_query_guc_submission_version - query GuC submission
interface version
+*/
+struct drm_i915_query_guc_submission_version {
+ __u64 major;
+ __u64 minor;
+ __u64 patch;
+};
+
/**
* DOC: GuC HWCONFIG blob uAPI
*
It
Re: [PATCH] drm/i915/gt: Prevent possible NULL dereference in __caps_show()
Hi,
On 06/02/2024 16:45, Nikita Zhandarovich wrote:
After falling through the switch statement to default case 'repr' is
initialized with NULL, which will lead to incorrect dereference of
'!repr[n]' in the following loop.
Fix it with the help of an additional check for NULL.
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: 4ec76dbeb62b ("drm/i915/gt: Expose engine properties via sysfs")
Signed-off-by: Nikita Zhandarovich
---
P.S. The NULL-deref problem might be dealt with this way but I am
not certain that the rest of the __caps_show() behaviour remains
correct if we end up in default case. For instance, as far as I
can tell, buf might turn out to be w/o '\0'. I could use some
direction if this has to be addressed as well.
drivers/gpu/drm/i915/gt/sysfs_engines.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c
b/drivers/gpu/drm/i915/gt/sysfs_engines.c
index 021f51d9b456..6b130b732867 100644
--- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
+++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
@@ -105,7 +105,7 @@ __caps_show(struct intel_engine_cs *engine,
len = 0;
for_each_set_bit(n, &caps, show_unknown ? BITS_PER_LONG : count) {
- if (n >= count || !repr[n]) {
+ if (n >= count || !repr || !repr[n]) {
There are two input combinations to this function when repr is NULL.
First is show_unknown=true and caps=0, which means the for_each_set_bit
will not execute its body. (No bits set.)
Second is show_unknown=false and caps=~0, which means count is zero so
for_each_set_bit will again not run. (Bitfield size input param is zero.)
So unless I am missing something I do not see the null pointer dereference.
What could theoretically happen is that a third input combination
appears, where caps is not zero in the show_unknown=true case, either
via a fully un-handled engine->class (switch), or a new capability bit
not added to the static array a bit above.
That would assert during driver development here:
if (GEM_WARN_ON(show_unknown))
Granted that could be after the dereference in "if (n >= count ||
!repr[n])", but would be caught in debug builds (CI) and therefore not
be able to "ship" (get merge to the repo).
Your second question is about empty buffer returned i.e. len=0 at the
end of the function? (Which is when the buffer will not be null
terminated - or you see another option?)
That I think is safe too since it just results in a zero length read in
sysfs.
Regards,
Tvrtko
if (GEM_WARN_ON(show_unknown))
len += sysfs_emit_at(buf, len, "[%x] ", n);
} else {
Re: Re: linux-next: build failure after merge of the drm-misc tree
On Wed, 07 Feb 2024, Thomas Hellström wrote: > Indeed. Not even drm-misc itself compiles with xe enabled. I'll ping > drm-misc maintainers. We'll need CONFIG_DRM_XE=m enabled in drm-rerere/drm-misc-*_defconfig, and get people to use that. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 2/3] drm/xe/hdcp: Enable HDCP for XE
On Mon, 05 Feb 2024, Daniele Ceraolo Spurio
wrote:
> On 2/2/2024 12:37 AM, Suraj Kandpal wrote:
>> Enable HDCP for Xe by defining functions which take care of
>> interaction of HDCP as a client with the GSC CS interface.
>>
>> Signed-off-by: Suraj Kandpal
>> ---
>> drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 188 ++-
>> 1 file changed, 184 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> index 0f11a39333e2..eca941d7b281 100644
>> --- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> +++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
>> @@ -3,8 +3,24 @@
>>* Copyright 2023, Intel Corporation.
>>*/
>>
>> +#include "abi/gsc_command_header_abi.h"
>
> My original idea was for the users to not include this header and rely
> on the size provided by the emit functions. I see you use the check the
> input size, which I didn't do on the proxy side because the buffer is
> sized to be big enough for all possible commands. Overall not a blocker,
> just consider the option.
>
>> #include "i915_drv.h"
>
> Do you actually need i915_drv.h? You shouldn't be using any structure
> from i915 here. If you just need it for the pointers to struct
> drm_i915_private, just add a forward declaration for the structure.
Xe side it really must be struct xe_device, not drm_i915_private.
See xe Makefile.
BR,
Jani.
>
>> #include "intel_hdcp_gsc.h"
>> +#include "xe_bo.h"
>> +#include "xe_map.h"
>> +#include "xe_gsc_submit.h"
>> +
>> +#define HECI_MEADDRESS_HDCP 18
>> +
>> +struct intel_hdcp_gsc_message {
>> +struct xe_bo *hdcp_bo;
>> +u64 hdcp_cmd_in;
>> +u64 hdcp_cmd_out;
>> +};
>> +
>> +#define HOST_SESSION_CLIENT_MASK GENMASK_ULL(63, 56)
>> +#define HDCP_GSC_MESSAGE_SIZE sizeof(struct intel_hdcp_gsc_message)
>
> this define is unused. Also, intel_hdcp_gsc_message is not the actual
> message, but just contains a pointer to the object that holds the message.
>
>> +#define HDCP_GSC_HEADER_SIZE sizeof(struct intel_gsc_mtl_header)
>>
>> bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
>> {
>> @@ -13,22 +29,186 @@ bool intel_hdcp_gsc_cs_required(struct drm_i915_private
>> *i915)
>>
>> bool intel_hdcp_gsc_check_status(struct drm_i915_private *i915)
>> {
>> -return false;
>> +return true;
>
> Shouldn't you actually do a check in here?
>
>> +}
>> +
>> +/*This function helps allocate memory for the command that we will send to
>> gsc cs */
>> +static int intel_hdcp_gsc_initialize_message(struct drm_i915_private *i915,
>
> Having a drm_i915_private here that is actually an xe_device is really
> weird. I understand that the aim is to re-use most of the display code
> from i915, but if you need to different back-ends maybe just have the
> function accept a void pointer and then internally cast it to
> drm_i915_private or xe_device based on the driver, or use struct
> intel_display and cast it back to i915 or Xe with a container_of. I'll
> leave the final comment on this to someone that has more understanding
> than me of what's going on on the display side of things.
>
>> + struct intel_hdcp_gsc_message
>> *hdcp_message)
>> +{
>> +struct xe_bo *bo = NULL;
>> +u64 cmd_in, cmd_out;
>> +int err, ret = 0;
>> +
>> +/* allocate object of two page for HDCP command memory and store it */
>> +
>> +xe_device_mem_access_get(i915);
>> +bo = xe_bo_create_pin_map(i915, xe_device_get_root_tile(i915), NULL,
>> PAGE_SIZE * 2,
>> + ttm_bo_type_kernel,
>> + XE_BO_CREATE_SYSTEM_BIT |
>> + XE_BO_CREATE_GGTT_BIT);
>> +
>> +if (IS_ERR(bo)) {
>> +drm_err(&i915->drm, "Failed to allocate bo for HDCP streaming
>> command!\n");
>> +ret = err;
>> +goto out;
>> +}
>> +
>> +cmd_in = xe_bo_ggtt_addr(bo);
>> +
>> +if (iosys_map_is_null(&bo->vmap)) {
>
> this can't happen, if the bo fails to map then xe_bo_create_pin_map will
> return an error.
>
>> +drm_err(&i915->drm, "Failed to map gsc message page!\n");
>> +ret = PTR_ERR(&bo->vmap);
>> +goto out;
>> +}
>> +
>> +cmd_out = cmd_in + PAGE_SIZE;
>> +
>> +xe_map_memset(i915, &bo->vmap, 0, 0, bo->size);
>> +
>> +hdcp_message->hdcp_bo = bo;
>> +hdcp_message->hdcp_cmd_in = cmd_in;
>> +hdcp_message->hdcp_cmd_out = cmd_out;
>> +out:
>> +xe_device_mem_access_put(i915);
>> +return ret;
>> +}
>> +
>> +static int intel_hdcp_gsc_hdcp2_init(struct drm_i915_private *i915)
>> +{
>> +struct intel_hdcp_gsc_message *hdcp_message;
>> +int ret;
>> +
>> +hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL);
>> +
>> +if (!hdcp_message)
>> +return -ENOMEM;
>> +
>> +/*
>> + * NOTE: No need to lock the comp mutex here as it is already
>> + * going to be tak
Re: [PATCH 1/2] drm/i915/lnl: Add pkgc related register
On Thu, 2024-02-01 at 14:21 +0530, Suraj Kandpal wrote: > Add the register that needs to read and written onto for > deep pkgc programming. > > Signed-off-by: Suraj Kandpal > --- > drivers/gpu/drm/i915/display/skl_watermark_regs.h | 4 > 1 file changed, 4 insertions(+) Reviewed-by: Vinod Govindapillai > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark_regs.h > b/drivers/gpu/drm/i915/display/skl_watermark_regs.h > index 628c5920ad49..20b30c9a6613 100644 > --- a/drivers/gpu/drm/i915/display/skl_watermark_regs.h > +++ b/drivers/gpu/drm/i915/display/skl_watermark_regs.h > @@ -157,4 +157,8 @@ > #define MTL_LATENCY_SAGV _MMIO(0x4578c) > #define MTL_LATENCY_QCLK_SAGVREG_GENMASK(12, 0) > > +#define LNL_PKG_C_LATENCY _MMIO(0x46460) > +#define LNL_ADDED_WAKE_TIME_MASK REG_GENMASK(28, 16) > +#define LNL_PKG_C_LATENCY_MASK REG_GENMASK(12, 0) > + > #endif /* __SKL_WATERMARK_REGS_H__ */
Re: [PATCH 2/2] drm/i915/lnl: Program PKGC_LATENCY register
Hi Suraj,
On Mon, 2024-02-05 at 13:31 +0530, Suraj Kandpal wrote:
> Program the PKGC_LATENCY register with the highest latency from
> level 1 and above LP registers else program with all 1's.
> This is used to improve package C residency by sending the highest
> latency tolerance requirement (LTR) when the planes are done with the
> frame until the next frame programming window (set context latency,
> window 2) starts.
> Bspec: 68986
>
> --v2
> -Fix indentation [Chaitanya]
>
> Signed-off-by: Suraj Kandpal
> Reviewed-by: Chaitanya Kumar Borah
> ---
> drivers/gpu/drm/i915/display/skl_watermark.c | 31
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 051a02ac01a4..1ce4b33a407a 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -3394,6 +3394,34 @@ static void skl_read_wm_latency(struct
> drm_i915_private *i915, u16 wm[])
> adjust_wm_latency(i915, wm, num_levels, read_latency);
> }
>
> +/*
> + * Program PKG_C_LATENCY Pkg C with highest valid latency from
> + * watermark level1 and up and above. If watermark level 1 is
> + * invalid program it with all 1's.
> + * Program PKG_C_LATENCY Added Wake Time = 0.
> + */
Could you please confirm if the added wake time = 0 always? The Bspec says the
otherway!
> +static void intel_program_pkgc_latency(struct drm_i915_private *i915,
> + u16 wm_latency[])
> +{
> + u16 max_value = 0;
> + u32 clear = 0, val = 0;
> + int max_level = i915->display.wm.num_levels, i;
> +
> + for (i = 1; i <= max_level; i++) {
> + if (wm_latency[i] == 0)
> + break;
> + else if (wm_latency[i] > max_value)
> + max_value = wm_latency[i];
> + }
May be efficient to iterate max to 1 (skl_max_wm_level_for_vblank())
Also better to call skl_wm_lateny() instead of accessing the lantency values
directly as there are
some latency adjustments been done per platforms.
> +
> + if (max_value == 0)
> + max_value = ~0 & LNL_PKG_C_LATENCY_MASK;
> +
> + clear |= LNL_ADDED_WAKE_TIME_MASK | LNL_PKG_C_LATENCY_MASK;
As mentioned above the waketime is cleared here. Please double check if it is
as expected.
> + val |= max_value;
> + intel_uncore_rmw(&i915->uncore, LNL_PKG_C_LATENCY, clear, val);
> +}
> +
> static void skl_setup_wm_latency(struct drm_i915_private *i915)
> {
> if (HAS_HW_SAGV_WM(i915))
> @@ -3407,6 +3435,9 @@ static void skl_setup_wm_latency(struct
> drm_i915_private *i915)
> skl_read_wm_latency(i915, i915->display.wm.skl_latency);
>
> intel_print_wm_latency(i915, "Gen9 Plane",
> i915->display.wm.skl_latency);
> +
> + if (DISPLAY_VER(i915) >= 20)
> + intel_program_pkgc_latency(i915,
> i915->display.wm.skl_latency);
skl_setup_wm_latency() gets called only at the init time. Though latency values
dont change, but as
per bspec you need to disable this for VRR. I guess you would need to have
provision to update this
based on that. So should this check be moved to intel_atomic_check()?
BR
Vinod
> }
>
> static const struct intel_wm_funcs skl_wm_funcs = {
[PULL] drm-intel-next
Hi Dave & Sima - drm-intel-next-2024-02-07: drm/i915 feature pull for v6.9: Features and functionality: - Early transport for panel replay and PSR (Jouni) - New ARL PCI IDs (Matt) - DP TPS4 PHY test pattern support (Khaled) Refactoring and cleanups: - Unify and improve VSC SDP for PSR and non-PSR cases (Jouni) - Refactor memory regions and improve debug logging (Ville) - Rework global state serialization (Ville) - Remove unused CDCLK divider fields (Gustavo) - Unify HDCP connector logging format (Jani) - Use display instead of graphics version in display code (Jani) - Move VBT and opregion debugfs next to the implementation (Jani) - Abstract opregion interface, use opaque type (Jani) Fixes: - Fix MTL stolen memory access (Ville) - Fix initial display plane readout for MTL (Ville) - Fix HPD handling during driver init/shutdown (Imre) - Cursor vblank evasion fixes (Ville) - Various VSC SDP fixes (Jouni) - Allow PSR mode changes without full modeset (Jouni) - Fix CDCLK sanitization on module load for Xe2_LPD (Gustavo) - Fix the max DSC bpc supported by the source (Ankit) - Add missing LNL ALPM AUX wake configuration (Jouni) - Cx0 PHY state readout and verify fixes (Mika) - Fix PSR (panel replay) debugfs for MST connectors (Imre) - Fail HDCP repeater authentication if Type1 device not present (Suraj) - Ratelimit debug logging in vm_fault_ttm (Nirmoy) - Use a fake PCH for MTL because south display is not on the PCH (Haridhar) - Disable DSB for Xe driver for now (José) - Fix some LNL display register changes (Lucas) - Fix build on ChromeOS (Paz Zcharya) - Preserve current shared DPLL for fastsets on Type-C ports (Ville) - Fix state checker warnings for MG/TC/TBT PLLs (Ville) - Fix HDCP repeater ctl register value on errors (Jani) - Allow FBC with CCS modifiers on SKL+ (Ville) - Fix HDCP GGTT pinning (Ville) DRM core changes: - Add ratelimited drm dbg print (Nirmoy) - DPCD PSR early transport macro (Jouni) Merges: - Backmerge drm-next to bring Xe driver to drm-intel-next (Jani) BR, Jani. The following changes since commit 205e18c13545ab43cc4fe4930732b4feef551198: nouveau/gsp: handle engines in runl without nonstall interrupts. (2024-01-15 16:04:48 +1000) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-next-2024-02-07 for you to fetch changes up to 449c2d5948ba8c784dcbc5c67df1d8c54748caa4: drm/i915/alpm: Alpm aux wake configuration for lnl (2024-02-07 09:58:04 +0200) drm/i915 feature pull for v6.9: Features and functionality: - Early transport for panel replay and PSR (Jouni) - New ARL PCI IDs (Matt) - DP TPS4 PHY test pattern support (Khaled) Refactoring and cleanups: - Unify and improve VSC SDP for PSR and non-PSR cases (Jouni) - Refactor memory regions and improve debug logging (Ville) - Rework global state serialization (Ville) - Remove unused CDCLK divider fields (Gustavo) - Unify HDCP connector logging format (Jani) - Use display instead of graphics version in display code (Jani) - Move VBT and opregion debugfs next to the implementation (Jani) - Abstract opregion interface, use opaque type (Jani) Fixes: - Fix MTL stolen memory access (Ville) - Fix initial display plane readout for MTL (Ville) - Fix HPD handling during driver init/shutdown (Imre) - Cursor vblank evasion fixes (Ville) - Various VSC SDP fixes (Jouni) - Allow PSR mode changes without full modeset (Jouni) - Fix CDCLK sanitization on module load for Xe2_LPD (Gustavo) - Fix the max DSC bpc supported by the source (Ankit) - Add missing LNL ALPM AUX wake configuration (Jouni) - Cx0 PHY state readout and verify fixes (Mika) - Fix PSR (panel replay) debugfs for MST connectors (Imre) - Fail HDCP repeater authentication if Type1 device not present (Suraj) - Ratelimit debug logging in vm_fault_ttm (Nirmoy) - Use a fake PCH for MTL because south display is not on the PCH (Haridhar) - Disable DSB for Xe driver for now (José) - Fix some LNL display register changes (Lucas) - Fix build on ChromeOS (Paz Zcharya) - Preserve current shared DPLL for fastsets on Type-C ports (Ville) - Fix state checker warnings for MG/TC/TBT PLLs (Ville) - Fix HDCP repeater ctl register value on errors (Jani) - Allow FBC with CCS modifiers on SKL+ (Ville) - Fix HDCP GGTT pinning (Ville) DRM core changes: - Add ratelimited drm dbg print (Nirmoy) - DPCD PSR early transport macro (Jouni) Merges: - Backmerge drm-next to bring Xe driver to drm-intel-next (Jani) Ankit Nautiyal (1): drm/i915/dp: Fix the max DSC bpc supported by source Gustavo Sousa (5): drm/i915/cdclk: Remove divider field from tables drm/i915/xe2lpd: Update bxt_sanitize_cdclk() drm/i915/cdclk: Extract bxt_cdclk_ctl() drm/i915/cdclk: Reorder bxt_sanitize_cdclk() drm/i915/cdclk: Re-use bxt_cdclk_ctl() when sanitizing Haridhar Kalvala (1): drm/i915/mtl: Add fake PCH for Me
Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
Quoting Tvrtko Ursulin (2024-02-07 10:44:01)
>
> On 06/02/2024 20:51, Souza, Jose wrote:
> > On Tue, 2024-02-06 at 12:42 -0800, John Harrison wrote:
> >> On 2/6/2024 08:33, Tvrtko Ursulin wrote:
> >>> On 01/02/2024 18:25, Souza, Jose wrote:
> On Wed, 2024-01-24 at 08:55 +, Tvrtko Ursulin wrote:
> > On 24/01/2024 08:19, Joonas Lahtinen wrote:
> >> Add reporting of the GuC submissio/VF interface version via GETPARAM
> >> properties. Mesa intends to use this information to check for old
> >> firmware versions with known bugs before enabling features like async
> >> compute.
> >
> > There was
> > https://patchwork.freedesktop.org/patch/560704/?series=124592&rev=1
> > which does everything in one go so would be my preference.
>
> IMO Joonas version brings less burden to be maintained(no new struct).
> But both versions works, please just get into some agreement so we
> can move this forward.
> >>>
> >>> So I would really prefer the query. Simplified version would do like
> >>> the compile tested only:
> >> Vivaik's patch is definitely preferred. It is much cleaner to make one
> >> single call than having to make four separate calls. It is also
> >> extensible to other firmwares if required. The only blockage against it
> >> was whether it was a good thing to report at all. If that blockage is no
> >> longer valid then we should just merge the patch that has already been
> >> discussed, polished, fixed, etc. rather than starting the whole process
> >> from scratch.
> >
> > Agreed.
> >
> > Vivaik can you please rebase and send it again?
>
> Note there was review feedback not addressed so do that too please.
> AFAIR incorrect usage of copy item, pad/rsvd/mbz checking and questions
> about padding in general. Last is why I proposed a simplified version
> which is not future extensible and avoids the need for padding.
Yeah, I don't think there is point an adding an extensible interface as
we're not going to add further FW version queries. This only the
submission interface version we're going to expose:
* Note that the spec for the CSS header defines this version number
* as 'vf_version' as it was originally intended for virtualisation.
* However, it is applicable to native submission as well.
If somebody wants to work on the simplified version like Tvrtko
suggested below, I have no objection. We can also remove the reference
to the VF version even if that's used by the header definition.
But if there are just suggestions but no actual patches floated, then we
should be merging the GETPARAM version with the "VF" word removed.
We've already discussed on the topic for some months so doing the
minimal changes to fulfill Mesa requirements should be considered a
priority to avoid further delays.
>
> Regards,
>
> Tvrtko
>
> >
> >
> >>
> >> And note that it is four calls not three. The code below is missing the
> >> branch version number.
Not even kernel uses the 'build' version anywhere. I don't see how there
could be 'build' version for the VF interface version? It's not supposed
to version a concrete firmware build but the API contract implemented by
the build where patch version should already be incremented for each
fix.
So adding the build does not seem appropriate as there is no plan to
extend this API any further.
Regards, Joonas
> >>
> >> John.
> >>
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_query.c
> >>> b/drivers/gpu/drm/i915/i915_query.c
> >>> index 00871ef99792..999687f6a3d4 100644
> >>> --- a/drivers/gpu/drm/i915/i915_query.c
> >>> +++ b/drivers/gpu/drm/i915/i915_query.c
> >>> @@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct
> >>> drm_i915_private *i915,
> >>> return hwconfig->size;
> >>> }
> >>>
> >>> +static int
> >>> +query_guc_submission_version(struct drm_i915_private *i915,
> >>> + struct drm_i915_query_item *query)
> >>> +{
> >>> + struct drm_i915_query_guc_submission_version __user *query_ptr =
> >>> + u64_to_user_ptr(query->data_ptr);
> >>> + struct drm_i915_query_guc_submission_version ver;
> >>> + struct intel_guc *guc = &to_gt(i915)->uc.guc;
> >>> + const size_t size = sizeof(ver);
> >>> + int ret;
> >>> +
> >>> + if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
> >>> + return -ENODEV;
> >>> +
> >>> + ret = copy_query_item(&ver, size, size, query);
> >>> + if (ret != 0)
> >>> + return ret;
> >>> +
> >>> + if (ver.major || ver.minor || ver.patch)
> >>> + return -EINVAL;
> >>> +
> >>> + ver.major = guc->submission_version.major;
> >>> + ver.minor = guc->submission_version.minor;
> >>> + ver.patch = guc->submission_version.patch;
> >>> +
> >>> + if (copy_to_user(query_ptr, &ver, size))
> >>> + return -EFAULT;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int (* const i915_query_f
[PATCH 0/4] XE HDCP Enablement
This patch series enables HDCP on XE. Mainly includes rewriting functions that were responsible for sending hdcp messages via gsc cs. Signed-off-by: Suraj Kandpal Suraj Kandpal (4): drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file drm/xe: Use gsc_proxy_init_done to check proxy status drm/xe/hdcp: Enable HDCP for XE drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 10 +- drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 7 +- drivers/gpu/drm/xe/Makefile | 1 + .../gpu/drm/xe/abi/gsc_command_header_abi.h | 2 + drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 227 +- drivers/gpu/drm/xe/xe_gsc_proxy.c | 2 +- drivers/gpu/drm/xe/xe_gsc_proxy.h | 1 + drivers/gpu/drm/xe/xe_gsc_submit.c| 19 ++ drivers/gpu/drm/xe/xe_gsc_submit.h| 1 + 9 files changed, 253 insertions(+), 17 deletions(-) -- 2.25.1
[PATCH 1/4] drm/i915/hdcp: Move intel_hdcp_gsc_message def away from header file
Move intel_hdcp_gsc_message definition into intel_hdcp_gsc_message.c
so that intel_hdcp_gsc_message can be redefined for xe as needed.
Signed-off-by: Suraj Kandpal
---
drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 6 ++
drivers/gpu/drm/i915/display/intel_hdcp_gsc.h | 7 +--
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
index 18117b789b16..e44f60f00e8b 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
@@ -13,6 +13,12 @@
#include "intel_hdcp_gsc.h"
#include "intel_hdcp_gsc_message.h"
+struct intel_hdcp_gsc_message {
+ struct i915_vma *vma;
+ void *hdcp_cmd_in;
+ void *hdcp_cmd_out;
+};
+
bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
{
return DISPLAY_VER(i915) >= 14;
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
index eba2057c5a9e..5f610df61cc9 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
+++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.h
@@ -10,12 +10,7 @@
#include
struct drm_i915_private;
-
-struct intel_hdcp_gsc_message {
- struct i915_vma *vma;
- void *hdcp_cmd_in;
- void *hdcp_cmd_out;
-};
+struct intel_hdcp_gsc_message;
bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915);
ssize_t intel_hdcp_gsc_msg_send(struct drm_i915_private *i915, u8 *msg_in,
--
2.25.1
[PATCH 2/4] drm/xe: Use gsc_proxy_init_done to check proxy status
Expose gsc_proxy_init_done so that we can check if gsc proxy has
been initialized or not.
Signed-off-by: Suraj Kandpal
---
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 25 ++--
drivers/gpu/drm/xe/xe_gsc_proxy.c| 2 +-
drivers/gpu/drm/xe/xe_gsc_proxy.h| 1 +
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
index 0f11a39333e2..ca17dfbc3fe9 100644
--- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
+++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
@@ -5,15 +5,36 @@
#include "i915_drv.h"
#include "intel_hdcp_gsc.h"
+#include "xe_gt.h"
+#include "xe_gsc_proxy.h"
+#include "xe_pm.h"
bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
{
return true;
}
-bool intel_hdcp_gsc_check_status(struct drm_i915_private *i915)
+bool intel_hdcp_gsc_check_status(struct xe_device *xe)
{
- return false;
+ struct xe_tile *tile = xe_device_get_root_tile(xe);
+ struct xe_gt *gt = tile->media_gt;
+ int ret = true;
+
+ xe_pm_runtime_get(xe);
+ ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GSC);
+ if (ret) {
+ drm_dbg_kms(&xe->drm, "failed to get forcewake to disable GSC
interrupts\n");
+ return false;
+ }
+
+ if (!gsc_proxy_init_done(>->uc.gsc))
+ ret = false;
+
+ if (!ret)
+ xe_force_wake_put(gt_to_fw(gt), XE_FW_GSC);
+ xe_pm_runtime_get(xe);
+
+ return ret;
}
int intel_hdcp_gsc_init(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c
b/drivers/gpu/drm/xe/xe_gsc_proxy.c
index 309ef80e3b95..f37f18a36209 100644
--- a/drivers/gpu/drm/xe/xe_gsc_proxy.c
+++ b/drivers/gpu/drm/xe/xe_gsc_proxy.c
@@ -66,7 +66,7 @@ static inline struct xe_device *kdev_to_xe(struct device
*kdev)
return dev_get_drvdata(kdev);
}
-static bool gsc_proxy_init_done(struct xe_gsc *gsc)
+bool gsc_proxy_init_done(struct xe_gsc *gsc)
{
struct xe_gt *gt = gsc_to_gt(gsc);
u32 fwsts1 = xe_mmio_read32(gt, HECI_FWSTS1(MTL_GSC_HECI1_BASE));
diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.h
b/drivers/gpu/drm/xe/xe_gsc_proxy.h
index 908f9441f093..10de5359fbb8 100644
--- a/drivers/gpu/drm/xe/xe_gsc_proxy.h
+++ b/drivers/gpu/drm/xe/xe_gsc_proxy.h
@@ -11,6 +11,7 @@
struct xe_gsc;
int xe_gsc_proxy_init(struct xe_gsc *gsc);
+bool gsc_proxy_init_done(struct xe_gsc *gsc);
void xe_gsc_proxy_remove(struct xe_gsc *gsc);
int xe_gsc_proxy_start(struct xe_gsc *gsc);
--
2.25.1
[PATCH 3/4] drm/xe/hdcp: Enable HDCP for XE
Enable HDCP for Xe by defining functions which take care of
interaction of HDCP as a client with the GSC CS interface.
--v2
-add kfree at appropriate place [Daniele]
-forward declare drm_i915_private [Daniele]
-remove useless define [Daniele]
-move host session logic to xe_gsc_submit.c [Daniele]
-call xe_gsc_check_and_update_pending directly in an if condition
[Daniele]
-use xe_device instead of drm_i915_private [Daniele]
Signed-off-by: Suraj Kandpal
---
drivers/gpu/drm/i915/display/intel_hdcp_gsc.c | 4 +-
.../gpu/drm/xe/abi/gsc_command_header_abi.h | 2 +
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 184 +-
drivers/gpu/drm/xe/xe_gsc_submit.c| 19 ++
drivers/gpu/drm/xe/xe_gsc_submit.h| 1 +
5 files changed, 202 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
index e44f60f00e8b..9e895f714f90 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp_gsc.c
@@ -123,8 +123,10 @@ static int intel_hdcp_gsc_hdcp2_init(struct
drm_i915_private *i915)
i915->display.hdcp.hdcp_message = hdcp_message;
ret = intel_hdcp_gsc_initialize_message(i915, hdcp_message);
- if (ret)
+ if (ret) {
drm_err(&i915->drm, "Could not initialize hdcp_message\n");
+ kfree(hdcp_message);
+ }
return ret;
}
diff --git a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
index a4c2646803b5..d2fbf71439be 100644
--- a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
+++ b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
@@ -21,6 +21,8 @@ struct intel_gsc_mtl_header {
/* FW allows host to decide host_session handle as it sees fit. */
u64 host_session_handle;
+#define HECI_MEADDRESS_PXP 17
+#define HECI_MEADDRESS_HDCP 18
/* handle generated by FW for messages that need to be re-submitted */
u64 gsc_message_handle;
diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
index ca17dfbc3fe9..d95c1b3b2d9c 100644
--- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
+++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
@@ -3,11 +3,26 @@
* Copyright 2023, Intel Corporation.
*/
-#include "i915_drv.h"
+#include
+
+#include "abi/gsc_command_header_abi.h"
#include "intel_hdcp_gsc.h"
#include "xe_gt.h"
#include "xe_gsc_proxy.h"
#include "xe_pm.h"
+#include "xe_bo.h"
+#include "xe_map.h"
+#include "xe_gsc_submit.h"
+
+#define HECI_MEADDRESS_HDCP 18
+
+struct intel_hdcp_gsc_message {
+ struct xe_bo *hdcp_bo;
+ u64 hdcp_cmd_in;
+ u64 hdcp_cmd_out;
+};
+
+#define HDCP_GSC_HEADER_SIZE sizeof(struct intel_gsc_mtl_header)
bool intel_hdcp_gsc_cs_required(struct drm_i915_private *i915)
{
@@ -37,19 +52,174 @@ bool intel_hdcp_gsc_check_status(struct xe_device *xe)
return ret;
}
-int intel_hdcp_gsc_init(struct drm_i915_private *i915)
+/*This function helps allocate memory for the command that we will send to gsc
cs */
+static int intel_hdcp_gsc_initialize_message(struct xe_device *xe,
+struct intel_hdcp_gsc_message
*hdcp_message)
+{
+ struct xe_bo *bo = NULL;
+ u64 cmd_in, cmd_out;
+ int err, ret = 0;
+
+ /* allocate object of two page for HDCP command memory and store it */
+ xe_device_mem_access_get(xe);
+ bo = xe_bo_create_pin_map(xe, xe_device_get_root_tile(xe), NULL,
PAGE_SIZE * 2,
+ ttm_bo_type_kernel,
+ XE_BO_CREATE_SYSTEM_BIT |
+ XE_BO_CREATE_GGTT_BIT);
+
+ if (IS_ERR(bo)) {
+ drm_err(&xe->drm, "Failed to allocate bo for HDCP streaming
command!\n");
+ ret = err;
+ goto out;
+ }
+
+ cmd_in = xe_bo_ggtt_addr(bo);
+ cmd_out = cmd_in + PAGE_SIZE;
+ xe_map_memset(xe, &bo->vmap, 0, 0, bo->size);
+
+ hdcp_message->hdcp_bo = bo;
+ hdcp_message->hdcp_cmd_in = cmd_in;
+ hdcp_message->hdcp_cmd_out = cmd_out;
+out:
+ xe_device_mem_access_put(xe);
+ return ret;
+}
+
+static int intel_hdcp_gsc_hdcp2_init(struct xe_device *xe)
+{
+ struct intel_hdcp_gsc_message *hdcp_message;
+ int ret;
+
+ hdcp_message = kzalloc(sizeof(*hdcp_message), GFP_KERNEL);
+
+ if (!hdcp_message)
+ return -ENOMEM;
+
+ /*
+* NOTE: No need to lock the comp mutex here as it is already
+* going to be taken before this function called
+*/
+ xe->display.hdcp.hdcp_message = hdcp_message;
+ ret = intel_hdcp_gsc_initialize_message(xe, hdcp_message);
+
+ if (ret)
+ drm_err(&xe->drm, "Could not initialize hdcp_message\n");
+
+ return ret;
+}
+
+int intel_hdcp_gsc_init(struct xe_device *xe)
+{
+ struct i915_hd
[PATCH 4/4] drm/xe/hdcp: Add intel_hdcp_gsc_message to Makefile
Add intel_hdcp_gsc_message to Makefile and add corresponding
changes to xe_hdcp_gsc.c to make it build.
Signed-off-by: Suraj Kandpal
---
drivers/gpu/drm/xe/Makefile | 1 +
drivers/gpu/drm/xe/display/xe_hdcp_gsc.c | 18 ++
2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index c531210695db..2b654c908ff3 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -254,6 +254,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
i915-display/intel_global_state.o \
i915-display/intel_gmbus.o \
i915-display/intel_hdcp.o \
+ i915-display/intel_hdcp_gsc_message.o \
i915-display/intel_hdmi.o \
i915-display/intel_hotplug.o \
i915-display/intel_hotplug_irq.o \
diff --git a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
index d95c1b3b2d9c..52a22e6d72ab 100644
--- a/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
+++ b/drivers/gpu/drm/xe/display/xe_hdcp_gsc.c
@@ -4,9 +4,11 @@
*/
#include
+#include
#include "abi/gsc_command_header_abi.h"
#include "intel_hdcp_gsc.h"
+#include "intel_hdcp_gsc_message.h"
#include "xe_gt.h"
#include "xe_gsc_proxy.h"
#include "xe_pm.h"
@@ -108,6 +110,22 @@ static int intel_hdcp_gsc_hdcp2_init(struct xe_device *xe)
return ret;
}
+static const struct i915_hdcp_ops gsc_hdcp_ops = {
+ .initiate_hdcp2_session = intel_hdcp_gsc_initiate_session,
+ .verify_receiver_cert_prepare_km =
+ intel_hdcp_gsc_verify_receiver_cert_prepare_km,
+ .verify_hprime = intel_hdcp_gsc_verify_hprime,
+ .store_pairing_info = intel_hdcp_gsc_store_pairing_info,
+ .initiate_locality_check = intel_hdcp_gsc_initiate_locality_check,
+ .verify_lprime = intel_hdcp_gsc_verify_lprime,
+ .get_session_key = intel_hdcp_gsc_get_session_key,
+ .repeater_check_flow_prepare_ack =
+ intel_hdcp_gsc_repeater_check_flow_prepare_ack,
+ .verify_mprime = intel_hdcp_gsc_verify_mprime,
+ .enable_hdcp_authentication = intel_hdcp_gsc_enable_authentication,
+ .close_hdcp_session = intel_hdcp_gsc_close_session,
+};
+
int intel_hdcp_gsc_init(struct xe_device *xe)
{
struct i915_hdcp_arbiter *data;
--
2.25.1
[RFC] drm/i915: Add GuC submission interface version query
From: Tvrtko Ursulin
Add a new query to the GuC submission interface version.
Mesa intends to use this information to check for old firmware versions
with a known bug where using the render and compute command streamers
simultaneously can cause GPU hangs due issues in firmware scheduling.
Based on patches from Vivaik and Joonas.
There is a little bit of an open around the width required for versions.
While the GuC FW iface tells they are u8, i915 GuC code uses u32:
#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
...
struct intel_uc_fw_ver {
u32 major;
u32 minor;
u32 patch;
u32 build;
};
So we could make the query u8, and refactor the struct intel_uc_fw_ver
to use u8, or not. To avoid any doubts on why are we assigning u32 to
u8 I simply opted to use u64. Which avoids the need to add any padding
too.
Compile tested only.
Signed-off-by: Tvrtko Ursulin
Cc: Kenneth Graunke
Cc: Jose Souza
Cc: Sagar Ghuge
Cc: Paulo Zanoni
Cc: John Harrison
Cc: Rodrigo Vivi
Cc: Jani Nikula
Cc: Tvrtko Ursulin
Cc: Vivaik Balasubrawmanian
---
drivers/gpu/drm/i915/i915_query.c | 32 +++
include/uapi/drm/i915_drm.h | 11 +++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..999687f6a3d4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct drm_i915_private
*i915,
return hwconfig->size;
}
+static int
+query_guc_submission_version(struct drm_i915_private *i915,
+struct drm_i915_query_item *query)
+{
+ struct drm_i915_query_guc_submission_version __user *query_ptr =
+ u64_to_user_ptr(query->data_ptr);
+ struct drm_i915_query_guc_submission_version ver;
+ struct intel_guc *guc = &to_gt(i915)->uc.guc;
+ const size_t size = sizeof(ver);
+ int ret;
+
+ if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
+ return -ENODEV;
+
+ ret = copy_query_item(&ver, size, size, query);
+ if (ret != 0)
+ return ret;
+
+ if (ver.major || ver.minor || ver.patch)
+ return -EINVAL;
+
+ ver.major = guc->submission_version.major;
+ ver.minor = guc->submission_version.minor;
+ ver.patch = guc->submission_version.patch;
+
+ if (copy_to_user(query_ptr, &ver, size))
+ return -EFAULT;
+
+ return 0;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item)
= {
query_topology_info,
@@ -559,6 +590,7 @@ static int (* const i915_query_funcs[])(struct
drm_i915_private *dev_priv,
query_memregion_info,
query_hwconfig_blob,
query_geometry_subslices,
+ query_guc_submission_version,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 550c496ce76d..d80d9b5e1eda 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3038,6 +3038,7 @@ struct drm_i915_query_item {
* - %DRM_I915_QUERY_MEMORY_REGIONS (see struct
drm_i915_query_memory_regions)
* - %DRM_I915_QUERY_HWCONFIG_BLOB (see `GuC HWCONFIG blob uAPI`)
* - %DRM_I915_QUERY_GEOMETRY_SUBSLICES (see struct
drm_i915_query_topology_info)
+* - %DRM_I915_QUERY_GUC_SUBMISSION_VERSION (see struct
drm_i915_query_guc_submission_version)
*/
__u64 query_id;
#define DRM_I915_QUERY_TOPOLOGY_INFO 1
@@ -3046,6 +3047,7 @@ struct drm_i915_query_item {
#define DRM_I915_QUERY_MEMORY_REGIONS 4
#define DRM_I915_QUERY_HWCONFIG_BLOB 5
#define DRM_I915_QUERY_GEOMETRY_SUBSLICES 6
+#define DRM_I915_QUERY_GUC_SUBMISSION_VERSION 7
/* Must be kept compact -- no holes and well documented */
/**
@@ -3591,6 +3593,15 @@ struct drm_i915_query_memory_regions {
struct drm_i915_memory_region_info regions[];
};
+/**
+* struct drm_i915_query_guc_submission_version - query GuC submission
interface version
+*/
+struct drm_i915_query_guc_submission_version {
+ __u64 major;
+ __u64 minor;
+ __u64 patch;
+};
+
/**
* DOC: GuC HWCONFIG blob uAPI
*
--
2.40.1
Re: [PATCH 11/19] drm/i915/dp: Add support for DP tunnel BW allocation
On Wed, Feb 07, 2024 at 01:08:43AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:42PM +0200, Imre Deak wrote:
> > Add support to enable the DP tunnel BW allocation mode. Follow-up
> > patches will call the required helpers added here to prepare for a
> > modeset on a link with DP tunnels, the last change in the patchset
> > actually enabling BWA.
> >
> > With BWA enabled, the driver will expose the full mode list a display
> > supports, regardless of any BW limitation on a shared (Thunderbolt)
> > link. Such BW limits will be checked against only during a modeset, when
> > the driver has the full knowledge of each display's BW requirement.
> >
> > If the link BW changes in a way that a connector's modelist may also
> > change, userspace will get a hotplug notification for all the connectors
> > sharing the same link (so it can adjust the mode used for a display).
> >
> > The BW limitation can change at any point, asynchronously to modesets
> > on a given connector, so a modeset can fail even though the atomic check
> > for it passed. In such scenarios userspace will get a bad link
> > notification and in response is supposed to retry the modeset.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/Kconfig | 13 +
> > drivers/gpu/drm/i915/Kconfig.debug| 1 +
> > drivers/gpu/drm/i915/Makefile | 3 +
> > drivers/gpu/drm/i915/display/intel_atomic.c | 2 +
> > .../gpu/drm/i915/display/intel_display_core.h | 1 +
> > .../drm/i915/display/intel_display_types.h| 9 +
> > .../gpu/drm/i915/display/intel_dp_tunnel.c| 642 ++
> > .../gpu/drm/i915/display/intel_dp_tunnel.h| 131
> > 8 files changed, 802 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.c
> > create mode 100644 drivers/gpu/drm/i915/display/intel_dp_tunnel.h
> >
> > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> > index b5d6e3352071f..4636913c17868 100644
> > --- a/drivers/gpu/drm/i915/Kconfig
> > +++ b/drivers/gpu/drm/i915/Kconfig
> > @@ -155,6 +155,19 @@ config DRM_I915_PXP
> > protected session and manage the status of the alive software session,
> > as well as its life cycle.
> >
> > +config DRM_I915_DP_TUNNEL
> > + bool "Enable DP tunnel support"
> > + depends on DRM_I915
> > + select DRM_DISPLAY_DP_TUNNEL
> > + default y
> > + help
> > + Choose this option to detect DP tunnels and enable the Bandwidth
> > + Allocation mode for such tunnels. This allows using the maximum
> > + resolution allowed by the link BW on all displays sharing the
> > + link BW, for instance on a Thunderbolt link.
> > +
> > + If in doubt, say "Y".
> > +
> > menu "drm/i915 Debugging"
> > depends on DRM_I915
> > depends on EXPERT
> > diff --git a/drivers/gpu/drm/i915/Kconfig.debug
> > b/drivers/gpu/drm/i915/Kconfig.debug
> > index 5b7162076850c..bc18e2d9ea05d 100644
> > --- a/drivers/gpu/drm/i915/Kconfig.debug
> > +++ b/drivers/gpu/drm/i915/Kconfig.debug
> > @@ -28,6 +28,7 @@ config DRM_I915_DEBUG
> > select STACKDEPOT
> > select STACKTRACE
> > select DRM_DP_AUX_CHARDEV
> > + select DRM_DISPLAY_DEBUG_DP_TUNNEL_STATE if DRM_I915_DP_TUNNEL
> > select X86_MSR # used by igt/pm_rpm
> > select DRM_VGEM # used by igt/prime_vgem (dmabuf interop checks)
> > select DRM_DEBUG_MM if DRM=y
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index c13f14edb5088..3ef6ed41e62b4 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -369,6 +369,9 @@ i915-y += \
> > display/vlv_dsi.o \
> > display/vlv_dsi_pll.o
> >
> > +i915-$(CONFIG_DRM_I915_DP_TUNNEL) += \
> > + display/intel_dp_tunnel.o
> > +
> > i915-y += \
> > i915_perf.o
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > index ec0d5168b5035..96ab37e158995 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > @@ -29,6 +29,7 @@
> > * See intel_atomic_plane.c for the plane-specific atomic functionality.
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -38,6 +39,7 @@
> > #include "intel_atomic.h"
> > #include "intel_cdclk.h"
> > #include "intel_display_types.h"
> > +#include "intel_dp_tunnel.h"
> > #include "intel_global_state.h"
> > #include "intel_hdcp.h"
> > #include "intel_psr.h"
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h
> > b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index a90f1aa201be8..0993d25a0a686 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -522,6 +522,7 @@ struct intel_display {
> > } wq;
> >
> > /* Grouping using named structs. Keep sorted. */
> > + struct drm_dp_tunnel_mgr *dp_t
Re: [PATCH 1/2] drm/i915: Prevent HW access during init from SDVO TV get_modes hook
On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> 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() - keep the previously
> detected
> mode list of the connector.
I don't see where this is done? Not sure if looking at wrong place, but
I see it tries using some override edid and in case that fails as well
uses drm_add_modes_noedid?
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 2571ef5a1b211..ccea0efbd136f 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2287,6 +2287,7 @@ static const struct drm_display_mode
> sdvo_tv_modes[] = {
> static int intel_sdvo_get_tv_modes(struct drm_connector *connector)
I see intel_sdvo_get_tv_modes is called from intel_sdvo_get_modes. Why
there is no need to do the same in intel_sdvo_get_lvds_modes and
intel_sdvo_get_ddc_modes as well?
BR,
Jouni Högander
> {
> struct intel_sdvo *intel_sdvo =
> intel_attached_sdvo(to_intel_connector(connector));
> + struct drm_i915_private *i915 = to_i915(intel_sdvo-
> >base.base.dev);
> struct intel_sdvo_connector *intel_sdvo_connector =
> to_intel_sdvo_connector(connector);
> const struct drm_connector_state *conn_state = connector-
> >state;
> @@ -2298,6 +2299,9 @@ static int intel_sdvo_get_tv_modes(struct
> drm_connector *connector)
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> connector->base.id, connector->name);
>
> + if (!intel_display_driver_check_access(i915))
> + return 0;
> +
> /*
> * Read the list of supported input resolutions for the
> selected TV
> * format.
Re: [PATCH] kunit: device: Unregister the kunit_bus on shutdown
On Fri, 02 Feb 2024, Rae Moar wrote:
> On Thu, Feb 1, 2024 at 1:06 AM David Gow wrote:
>>
>> If KUnit is built as a module, and it's unloaded, the kunit_bus is not
>> unregistered. This causes an error if it's then re-loaded later, as we
>> try to re-register the bus.
>>
>> Unregister the bus and root_device on shutdown, if it looks valid.
>>
>> In addition, be more specific about the value of kunit_bus_device. It
>> is:
>> - a valid struct device* if the kunit_bus initialised correctly.
>> - an ERR_PTR if it failed to initialise.
>> - NULL before initialisation and after shutdown.
>>
>> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
>> Signed-off-by: David Gow
>
> Hello,
>
> I have tested this with modules and it looks good to me!
>
> Thanks!
> -Rae
>
> Reviewed-by: Rae Moar
Thanks for the patch and review!
Is this on its way to some v6.8-rc's? The regression in -rc1 is hurting
our CI.
Thanks,
Jani.
--
Jani Nikula, Intel
Re: [PATCH 1/2] drm/i915: Prevent HW access during init from SDVO TV get_modes hook
On Wed, 2024-02-07 at 15:26 +0200, Hogander, Jouni wrote:
> On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> > 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() - keep the previously
> > detected
> > mode list of the connector.
>
> I don't see where this is done? Not sure if looking at wrong place,
> but
> I see it tries using some override edid and in case that fails as
> well
> uses drm_add_modes_noedid?
>
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > index 2571ef5a1b211..ccea0efbd136f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > @@ -2287,6 +2287,7 @@ static const struct drm_display_mode
> > sdvo_tv_modes[] = {
> > static int intel_sdvo_get_tv_modes(struct drm_connector
> > *connector)
>
> I see intel_sdvo_get_tv_modes is called from intel_sdvo_get_modes.
> Why
> there is no need to do the same in intel_sdvo_get_lvds_modes and
> intel_sdvo_get_ddc_modes as well?
It seems you are taking care of intel_svdo_get_ddc_modes in next patch.
Intel_svdo_get_lvds_modes doesn't touch the hw -> nothing to do there.
>
> BR,
>
> Jouni Högander
>
> > {
> > struct intel_sdvo *intel_sdvo =
> > intel_attached_sdvo(to_intel_connector(connector));
> > + struct drm_i915_private *i915 = to_i915(intel_sdvo-
> > > base.base.dev);
> > struct intel_sdvo_connector *intel_sdvo_connector =
> > to_intel_sdvo_connector(connector);
> > const struct drm_connector_state *conn_state = connector-
> > > state;
> > @@ -2298,6 +2299,9 @@ static int intel_sdvo_get_tv_modes(struct
> > drm_connector *connector)
> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > connector->base.id, connector->name);
> >
> > + if (!intel_display_driver_check_access(i915))
> > + return 0;
> > +
> > /*
> > * Read the list of supported input resolutions for the
> > selected TV
> > * format.
>
Re: [PATCH 1/2] drm/i915: Prevent HW access during init from SDVO TV get_modes hook
On Wed, Feb 07, 2024 at 03:45:15PM +0200, Hogander, Jouni wrote:
> On Wed, 2024-02-07 at 15:26 +0200, Hogander, Jouni wrote:
> > On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> > > 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() - keep the previously
> > > detected
> > > mode list of the connector.
> >
>
> > I don't see where this is done? Not sure if looking at wrong place,
> > but I see it tries using some override edid and in case that fails
> > as well uses drm_add_modes_noedid?
Some default and EDID override modes are also added to the connector
mode list, similarly to when the HW access in intel_sdvo_get_tv_modes()
fails (along with any modes specified via the kernel command line, which
happens unconditionally, see drm_helper_probe_add_cmdline_mode()).
All the modes detected by the encoder detect and the connector get_modes
hooks gets added to the drm_connector::probed_modes list. From this
__drm_helper_update_and_validate() will copy any new modes to the
drm_connector::modes list (which will be returned to user space/kernel
client). Thus, returning 0 from the above TV connector get_modes hook
will preserve the list of modes on the drm_connector::modes list from an
earlier encoder detect/connector get_modes call.
> >
> > >
> > > Signed-off-by: Imre Deak
> > > ---
> > > drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > index 2571ef5a1b211..ccea0efbd136f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > @@ -2287,6 +2287,7 @@ static const struct drm_display_mode
> > > sdvo_tv_modes[] = {
> > > static int intel_sdvo_get_tv_modes(struct drm_connector
> > > *connector)
> >
> > I see intel_sdvo_get_tv_modes is called from intel_sdvo_get_modes.
> > Why
> > there is no need to do the same in intel_sdvo_get_lvds_modes and
> > intel_sdvo_get_ddc_modes as well?
>
> It seems you are taking care of intel_svdo_get_ddc_modes in next patch.
> Intel_svdo_get_lvds_modes doesn't touch the hw -> nothing to do there.
>
> >
> > BR,
> >
> > Jouni Högander
> >
> > > {
> > > struct intel_sdvo *intel_sdvo =
> > > intel_attached_sdvo(to_intel_connector(connector));
> > > + struct drm_i915_private *i915 = to_i915(intel_sdvo-
> > > > base.base.dev);
> > > struct intel_sdvo_connector *intel_sdvo_connector =
> > > to_intel_sdvo_connector(connector);
> > > const struct drm_connector_state *conn_state = connector-
> > > > state;
> > > @@ -2298,6 +2299,9 @@ static int intel_sdvo_get_tv_modes(struct
> > > drm_connector *connector)
> > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > connector->base.id, connector->name);
> > >
> > > + if (!intel_display_driver_check_access(i915))
> > > + return 0;
> > > +
> > > /*
> > > * Read the list of supported input resolutions for the
> > > selected TV
> > > * format.
> >
>
Re: [PATCH 1/2] drm/i915: Prevent HW access during init from SDVO TV get_modes hook
On Wed, Feb 07, 2024 at 03:45:15PM +0200, Hogander, Jouni wrote:
> On Wed, 2024-02-07 at 15:26 +0200, Hogander, Jouni wrote:
> > On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> > > 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() - keep the previously
> > > detected
> > > mode list of the connector.
> >
> > I don't see where this is done? Not sure if looking at wrong place,
> > but
> > I see it tries using some override edid and in case that fails as
> > well
> > uses drm_add_modes_noedid?
> >
> > >
> > > Signed-off-by: Imre Deak
> > > ---
> > > drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > index 2571ef5a1b211..ccea0efbd136f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > @@ -2287,6 +2287,7 @@ static const struct drm_display_mode
> > > sdvo_tv_modes[] = {
> > > static int intel_sdvo_get_tv_modes(struct drm_connector
> > > *connector)
> >
> > I see intel_sdvo_get_tv_modes is called from intel_sdvo_get_modes.
> > Why there is no need to do the same in intel_sdvo_get_lvds_modes and
> > intel_sdvo_get_ddc_modes as well?
>
> It seems you are taking care of intel_svdo_get_ddc_modes in next patch.
> Intel_svdo_get_lvds_modes doesn't touch the hw -> nothing to do there.
Yes, all the connectors that read out an EDID - vs. the TV connector
which doesn't - is handled by the next patch. And yes, the connectors
which don't access the HW in the get_modes hook doesn't need this check.
Btw, I think all the connectors should work - eventually - in this way:
access the HW only in the encoder detect hook and from the get_modes
hook only return the detected mode list w/o accessing the HW. DP-SST and
HDMI do already the correct thing wrt. this.
>
> >
> > BR,
> >
> > Jouni Högander
> >
> > > {
> > > struct intel_sdvo *intel_sdvo =
> > > intel_attached_sdvo(to_intel_connector(connector));
> > > + struct drm_i915_private *i915 = to_i915(intel_sdvo-
> > > > base.base.dev);
> > > struct intel_sdvo_connector *intel_sdvo_connector =
> > > to_intel_sdvo_connector(connector);
> > > const struct drm_connector_state *conn_state = connector-
> > > > state;
> > > @@ -2298,6 +2299,9 @@ static int intel_sdvo_get_tv_modes(struct
> > > drm_connector *connector)
> > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > connector->base.id, connector->name);
> > >
> > > + if (!intel_display_driver_check_access(i915))
> > > + return 0;
> > > +
> > > /*
> > > * Read the list of supported input resolutions for the
> > > selected TV
> > > * format.
> >
>
Re: [PATCH 14/19] drm/i915/dp: Compute DP tunel BW during encoder state computation
On Wed, Feb 07, 2024 at 01:25:19AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:45PM +0200, Imre Deak wrote:
> > Compute the BW required through a DP tunnel on links with such tunnels
> > detected and add the corresponding atomic state during a modeset.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 16 +---
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 13 +
> > 2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 78dfe8be6031d..6968fdb7ffcdf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2880,6 +2880,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > struct drm_connector_state *conn_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + struct intel_atomic_state *state =
> > to_intel_atomic_state(conn_state->state);
> > struct drm_display_mode *adjusted_mode = &pipe_config->hw.adjusted_mode;
> > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > const struct drm_display_mode *fixed_mode;
> > @@ -2980,6 +2981,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state);
> > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp, pipe_config,
> > conn_state);
> >
> > + intel_dp_tunnel_atomic_compute_stream_bw(state, intel_dp, connector,
> > +pipe_config);
>
> Error handling seems awol?
Yes, along with checking the return from
drm_dp_tunnel_atomic_set_stream_bw(), thanks for spotting this.
>
> > +
> > return 0;
> > }
> >
> > @@ -6087,6 +6091,15 @@ static int intel_dp_connector_atomic_check(struct
> > drm_connector *conn,
> > return ret;
> > }
> >
> > + if (!intel_connector_needs_modeset(state, conn))
> > + return 0;
> > +
> > + ret = intel_dp_tunnel_atomic_check_state(state,
> > +intel_dp,
> > +intel_conn);
> > + if (ret)
> > + return ret;
> > +
> > /*
> > * We don't enable port sync on BDW due to missing w/as and
> > * due to not having adjusted the modeset sequence appropriately.
> > @@ -6094,9 +6107,6 @@ static int intel_dp_connector_atomic_check(struct
> > drm_connector *conn,
> > if (DISPLAY_VER(dev_priv) < 9)
> > return 0;
> >
> > - if (!intel_connector_needs_modeset(state, conn))
> > - return 0;
> > -
> > if (conn->has_tile) {
> > ret = intel_modeset_tile_group(state, conn->tile_group->id);
> > if (ret)
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 520393dc8b453..cbfab3173b9ef 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -42,6 +42,7 @@
> > #include "intel_dp.h"
> > #include "intel_dp_hdcp.h"
> > #include "intel_dp_mst.h"
> > +#include "intel_dp_tunnel.h"
> > #include "intel_dpio_phy.h"
> > #include "intel_hdcp.h"
> > #include "intel_hotplug.h"
> > @@ -523,6 +524,7 @@ static int intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >struct drm_connector_state *conn_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + struct intel_atomic_state *state =
> > to_intel_atomic_state(conn_state->state);
> > struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> > struct intel_dp *intel_dp = &intel_mst->primary->dp;
> > const struct intel_connector *connector =
> > @@ -619,6 +621,9 @@ static int intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >
> > intel_psr_compute_config(intel_dp, pipe_config, conn_state);
> >
> > + intel_dp_tunnel_atomic_compute_stream_bw(state, intel_dp, connector,
> > +pipe_config);
> > +
> > return 0;
> > }
> >
> > @@ -876,6 +881,14 @@ intel_dp_mst_atomic_check(struct drm_connector
> > *connector,
> > if (ret)
> > return ret;
> >
> > + if (intel_connector_needs_modeset(state, connector)) {
> > + ret = intel_dp_tunnel_atomic_check_state(state,
> > +
> > intel_connector->mst_port,
> > +intel_connector);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > return drm_dp_atomic_release_time_slots(&state->base,
> >
> > &intel_connector->mst_port->mst_mgr,
> > intel_connector->port);
> > --
> > 2.39.2
>
Re: [PATCH v2 2/4] eventfd: simplify eventfd_signal()
For vfio_ap_ops.c
Reviewed-by: Anthony Krowiak
On 2/6/24 2:44 PM, Stefan Hajnoczi wrote:
vhost and VIRTIO-related parts:
Reviewed-by: Stefan Hajnoczi
On Wed, 22 Nov 2023 at 07:50, Christian Brauner wrote:
Ever since the evenfd type was introduced back in 2007 in commit
e1ad7468c77d ("signal/timer/event: eventfd core") the eventfd_signal()
function only ever passed 1 as a value for @n. There's no point in
keeping that additional argument.
Signed-off-by: Christian Brauner
---
arch/x86/kvm/hyperv.c | 2 +-
arch/x86/kvm/xen.c| 2 +-
drivers/accel/habanalabs/common/device.c | 2 +-
drivers/fpga/dfl.c| 2 +-
drivers/gpu/drm/drm_syncobj.c | 6 +++---
drivers/gpu/drm/i915/gvt/interrupt.c | 2 +-
drivers/infiniband/hw/mlx5/devx.c | 2 +-
drivers/misc/ocxl/file.c | 2 +-
drivers/s390/cio/vfio_ccw_chp.c | 2 +-
drivers/s390/cio/vfio_ccw_drv.c | 4 ++--
drivers/s390/cio/vfio_ccw_ops.c | 6 +++---
drivers/s390/crypto/vfio_ap_ops.c | 2 +-
drivers/usb/gadget/function/f_fs.c| 4 ++--
drivers/vdpa/vdpa_user/vduse_dev.c| 6 +++---
drivers/vfio/fsl-mc/vfio_fsl_mc_intr.c| 2 +-
drivers/vfio/pci/vfio_pci_core.c | 6 +++---
drivers/vfio/pci/vfio_pci_intrs.c | 12 ++--
drivers/vfio/platform/vfio_platform_irq.c | 4 ++--
drivers/vhost/vdpa.c | 4 ++--
drivers/vhost/vhost.c | 10 +-
drivers/vhost/vhost.h | 2 +-
drivers/virt/acrn/ioeventfd.c | 2 +-
drivers/xen/privcmd.c | 2 +-
fs/aio.c | 2 +-
fs/eventfd.c | 9 +++--
include/linux/eventfd.h | 4 ++--
mm/memcontrol.c | 10 +-
mm/vmpressure.c | 2 +-
samples/vfio-mdev/mtty.c | 4 ++--
virt/kvm/eventfd.c| 4 ++--
30 files changed, 60 insertions(+), 63 deletions(-)
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 238afd7335e4..4943f6b2bbee 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -2388,7 +2388,7 @@ static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu,
struct kvm_hv_hcall *h
if (!eventfd)
return HV_STATUS_INVALID_PORT_ID;
- eventfd_signal(eventfd, 1);
+ eventfd_signal(eventfd);
return HV_STATUS_SUCCESS;
}
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index e53fad915a62..523bb6df5ac9 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -2088,7 +2088,7 @@ static bool kvm_xen_hcall_evtchn_send(struct kvm_vcpu
*vcpu, u64 param, u64 *r)
if (ret < 0 && ret != -ENOTCONN)
return false;
} else {
- eventfd_signal(evtchnfd->deliver.eventfd.ctx, 1);
+ eventfd_signal(evtchnfd->deliver.eventfd.ctx);
}
*r = 0;
diff --git a/drivers/accel/habanalabs/common/device.c
b/drivers/accel/habanalabs/common/device.c
index 9711e8fc979d..3a89644f087c 100644
--- a/drivers/accel/habanalabs/common/device.c
+++ b/drivers/accel/habanalabs/common/device.c
@@ -2044,7 +2044,7 @@ static void hl_notifier_event_send(struct
hl_notifier_event *notifier_event, u64
notifier_event->events_mask |= event_mask;
if (notifier_event->eventfd)
- eventfd_signal(notifier_event->eventfd, 1);
+ eventfd_signal(notifier_event->eventfd);
mutex_unlock(¬ifier_event->lock);
}
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index dd7a783d53b5..e73f88050f08 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -1872,7 +1872,7 @@ static irqreturn_t dfl_irq_handler(int irq, void *arg)
{
struct eventfd_ctx *trigger = arg;
- eventfd_signal(trigger, 1);
+ eventfd_signal(trigger);
return IRQ_HANDLED;
}
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 01da6789d044..b9cc62982196 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1365,7 +1365,7 @@ static void syncobj_eventfd_entry_fence_func(struct
dma_fence *fence,
struct syncobj_eventfd_entry *entry =
container_of(cb, struct syncobj_eventfd_entry, fence_cb);
- eventfd_signal(entry->ev_fd_ctx, 1);
+ eventfd_signal(entry->ev_fd_ctx);
syncobj_eventfd_entry_free(entry);
}
@@ -1388,13 +1388,13 @@ syncobj_eventfd_entry_func(struct drm_syncobj *syncobj,
entry->fence = fence;
if (entry->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE) {
- eventfd_signal(entry->ev_fd_ctx, 1);
+ eventfd_signal(entry->ev_fd_ctx);
syncobj_eventfd_entry_free(entry);
} else {
re
Re: [PATCH v2 2/4] eventfd: simplify eventfd_signal()
On Wed, Nov 22, 2023 at 1:49 PM Christian Brauner wrote:
>
> Ever since the evenfd type was introduced back in 2007 in commit
> e1ad7468c77d ("signal/timer/event: eventfd core") the eventfd_signal()
> function only ever passed 1 as a value for @n. There's no point in
> keeping that additional argument.
>
> Signed-off-by: Christian Brauner
> ---
> arch/x86/kvm/hyperv.c | 2 +-
> arch/x86/kvm/xen.c| 2 +-
> virt/kvm/eventfd.c| 4 ++--
> 30 files changed, 60 insertions(+), 63 deletions(-)
For KVM:
Acked-by: Paolo Bonzini
[PATCH] drm/i915/panelreplay: Panel replay workaround with VRR
Panel Replay VSC SDP not getting sent when VRR is enabled
and W1 and W2 are 0. So Program Set Context Latency in
TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
Signed-off-by: Animesh Manna
---
drivers/gpu/drm/i915/display/intel_display.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 1b844cac4c58..c1ec78b5b6c5 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2618,8 +2618,16 @@ static void intel_set_transcoder_timings(const struct
intel_crtc_state *crtc_sta
* TRANS_SET_CONTEXT_LATENCY to configure the pipe vblank start.
*/
if (DISPLAY_VER(dev_priv) >= 13) {
- intel_de_write(dev_priv,
TRANS_SET_CONTEXT_LATENCY(cpu_transcoder),
- crtc_vblank_start - crtc_vdisplay);
+ /*
+* WA: Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
register
+* to at least a value of 1 when Panel Replay is enabled with
VRR.
+*/
+ if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
+ (crtc_vblank_start == crtc_vdisplay))
+ intel_de_write(dev_priv,
TRANS_SET_CONTEXT_LATENCY(cpu_transcoder), 1);
+ else
+ intel_de_write(dev_priv,
TRANS_SET_CONTEXT_LATENCY(cpu_transcoder),
+ crtc_vblank_start - crtc_vdisplay);
/*
* VBLANK_START not used by hw, just clear it
--
2.29.0
Re: [RFC] drm/i915: Add GuC submission interface version query
Quoting Tvrtko Ursulin (2024-02-07 13:56:12)
> From: Tvrtko Ursulin
>
> Add a new query to the GuC submission interface version.
>
> Mesa intends to use this information to check for old firmware versions
> with a known bug where using the render and compute command streamers
> simultaneously can cause GPU hangs due issues in firmware scheduling.
>
> Based on patches from Vivaik and Joonas.
>
> There is a little bit of an open around the width required for versions.
> While the GuC FW iface tells they are u8, i915 GuC code uses u32:
>
> #define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
> #define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
> #define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
> ...
> struct intel_uc_fw_ver {
> u32 major;
> u32 minor;
> u32 patch;
> u32 build;
> };
>
> So we could make the query u8, and refactor the struct intel_uc_fw_ver
> to use u8, or not. To avoid any doubts on why are we assigning u32 to
> u8 I simply opted to use u64. Which avoids the need to add any padding
> too.
This a single-shot init time query so I guess u64 is fine too, to keep
the code straightforward.
> Compile tested only.
If Mesa folks confirm this is working for them and after you add link to
the Mesa PR, then you can add my:
Reviewed-by: Joonas Lahtinen
Regards, Joonas
> Signed-off-by: Tvrtko Ursulin
> Cc: Kenneth Graunke
> Cc: Jose Souza
> Cc: Sagar Ghuge
> Cc: Paulo Zanoni
> Cc: John Harrison
> Cc: Rodrigo Vivi
> Cc: Jani Nikula
> Cc: Tvrtko Ursulin
> Cc: Vivaik Balasubrawmanian
> ---
> drivers/gpu/drm/i915/i915_query.c | 32 +++
> include/uapi/drm/i915_drm.h | 11 +++
> 2 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c
> b/drivers/gpu/drm/i915/i915_query.c
> index 00871ef99792..999687f6a3d4 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct drm_i915_private
> *i915,
> return hwconfig->size;
> }
>
> +static int
> +query_guc_submission_version(struct drm_i915_private *i915,
> +struct drm_i915_query_item *query)
> +{
> + struct drm_i915_query_guc_submission_version __user *query_ptr =
> + u64_to_user_ptr(query->data_ptr);
> + struct drm_i915_query_guc_submission_version ver;
> + struct intel_guc *guc = &to_gt(i915)->uc.guc;
> + const size_t size = sizeof(ver);
> + int ret;
> +
> + if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
> + return -ENODEV;
> +
> + ret = copy_query_item(&ver, size, size, query);
> + if (ret != 0)
> + return ret;
> +
> + if (ver.major || ver.minor || ver.patch)
> + return -EINVAL;
> +
> + ver.major = guc->submission_version.major;
> + ver.minor = guc->submission_version.minor;
> + ver.patch = guc->submission_version.patch;
> +
> + if (copy_to_user(query_ptr, &ver, size))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
> struct drm_i915_query_item
> *query_item) = {
> query_topology_info,
> @@ -559,6 +590,7 @@ static int (* const i915_query_funcs[])(struct
> drm_i915_private *dev_priv,
> query_memregion_info,
> query_hwconfig_blob,
> query_geometry_subslices,
> + query_guc_submission_version,
> };
>
> int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file
> *file)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 550c496ce76d..d80d9b5e1eda 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3038,6 +3038,7 @@ struct drm_i915_query_item {
> * - %DRM_I915_QUERY_MEMORY_REGIONS (see struct
> drm_i915_query_memory_regions)
> * - %DRM_I915_QUERY_HWCONFIG_BLOB (see `GuC HWCONFIG blob uAPI`)
> * - %DRM_I915_QUERY_GEOMETRY_SUBSLICES (see struct
> drm_i915_query_topology_info)
> +* - %DRM_I915_QUERY_GUC_SUBMISSION_VERSION (see struct
> drm_i915_query_guc_submission_version)
> */
> __u64 query_id;
> #define DRM_I915_QUERY_TOPOLOGY_INFO 1
> @@ -3046,6 +3047,7 @@ struct drm_i915_query_item {
> #define DRM_I915_QUERY_MEMORY_REGIONS 4
> #define DRM_I915_QUERY_HWCONFIG_BLOB 5
> #define DRM_I915_QUERY_GEOMETRY_SUBSLICES 6
> +#define DRM_I915_QUERY_GUC_SUBMISSION_VERSION 7
> /* Must be kept compact -- no holes and well documented */
>
> /**
> @@ -3591,6 +3593,15 @@ struct drm_i915_query_memory_regions {
> struct drm_i915_memory_region_info regions[];
> };
>
> +/**
> +* struct drm_i915_query_guc_submission_version - query GuC submission
> interface version
> +*/
✗ Fi.CI.SPARSE: warning for XE HDCP Enablement (rev3)
== Series Details == Series: XE HDCP Enablement (rev3) URL : https://patchwork.freedesktop.org/series/129456/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately. - +./arch/x86/include/asm/bitops.h:116:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:116:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:147:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:147:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:149:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:149:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:153:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:153:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:155:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:155:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:155:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:155:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:173:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:173:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:175:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:175:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:179:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:179:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:181:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:181:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:181:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:181:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:185:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:185:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:187:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:187:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:191:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:191:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:194:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:194:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:194:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:194:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:236:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:236:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:238:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:238:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' +./include/asm-generic/bito
Re: [RFC 6/8] cgroup/drm: Introduce weight based drm cgroup control
Hello.
(I hope I'm replying to the latest iteration and it has some validitiy
still. Sorry for my late reply. Few points caught my attention.)
On Tue, Oct 24, 2023 at 05:07:25PM +0100, Tvrtko Ursulin
wrote:
> @@ -15,10 +17,28 @@ struct drm_cgroup_state {
> struct cgroup_subsys_state css;
>
> struct list_head clients;
> +
> + unsigned int weight;
> +
> + unsigned int sum_children_weights;
> +
> + bool over;
> + bool over_budget;
> +
> + u64 per_s_budget_us;
Nit: sounds reversed (cf USEC_PER_SEC).
> +static int drmcg_period_ms = 2000;
> +module_param(drmcg_period_ms, int, 0644);
> +
cgroup subsystems as loadable modules were abandoded long time ago.
I'm not sure if this works as expected then.
The common way is __seutp(), see for instance __setup() in mm/memcontrol.c
> +static bool __start_scanning(unsigned int period_us)
...
> + css_for_each_descendant_post(node, &root->css) {
> + struct drm_cgroup_state *drmcs = css_to_drmcs(node);
> + struct drm_cgroup_state *parent;
> + u64 active;
> +
> + if (!css_tryget_online(node))
> + goto out;
> + if (!node->parent) {
> + css_put(node);
> + continue;
> + }
I think the parent check can go first witout put'ting (RCU is enough for
node).
> + if (!css_tryget_online(node->parent)) {
> + css_put(node);
> + goto out;
> + }
Online parent is implied onlined node. So this can be removed.
...
> +
> + css_put(node);
> + }
I wonder if the first passes could be implemented with rstat flushing
and then only invoke signalling based on the data. (As that's
best-effort).
> +
> + /*
> + * 4th pass - send out over/under budget notifications.
> + */
> + css_for_each_descendant_post(node, &root->css) {
> + struct drm_cgroup_state *drmcs = css_to_drmcs(node);
> +
> + if (!css_tryget_online(node))
> + goto out_retry;
> +
> + if (drmcs->over || drmcs->over_budget)
> + drmcs_signal_budget(drmcs,
> + drmcs->active_us,
> + drmcs->per_s_budget_us);
> + drmcs->over_budget = drmcs->over;
> +
> + css_put(node);
> + }
> static struct cgroup_subsys_state *
> @@ -82,6 +365,7 @@ drmcs_alloc(struct cgroup_subsys_state *parent_css)
>
> if (!parent_css) {
> drmcs = &root_drmcs.drmcs;
> + INIT_DELAYED_WORK(&root_drmcs.scan_work, scan_worker);
This reminds me discussion
https://lore.kernel.org/lkml/rlm36iypckvxol2edyr25jyo4imvlidtepbcjdaa2ouvwh3wjq@pqyuk3v2jesb/
I.e. worker needn't be initialized if controller is "invisible".
> +static void drmcs_attach(struct cgroup_taskset *tset)
> +{
> + struct drm_cgroup_state *old = old_drmcs;
> + struct cgroup_subsys_state *css;
> + struct drm_file *fpriv, *next;
> + struct drm_cgroup_state *new;
> + struct task_struct *task;
> + bool migrated = false;
> +
> + if (!old)
> + return;
> +
> + task = cgroup_taskset_first(tset, &css);
> + new = css_to_drmcs(task_css(task, drm_cgrp_id));
> + if (new == old)
> + return;
> +
> + mutex_lock(&drmcg_mutex);
> +
> + list_for_each_entry_safe(fpriv, next, &old->clients, clink) {
> + cgroup_taskset_for_each(task, css, tset) {
> + struct cgroup_subsys_state *old_css;
> +
> + if (task->flags & PF_KTHREAD)
> + continue;
I'm curious, is this because of particular kthreads? Or would it fail
somehow below? (Like people should not migrate kthreads normally, so
their expectation cannot be high.)
Michal
signature.asc
Description: PGP signature
Re: [RFC 1/8] cgroup: Add the DRM cgroup controller
Hello.
On Tue, Oct 24, 2023 at 05:07:20PM +0100, Tvrtko Ursulin
wrote:
> +struct drm_cgroup_state {
> + struct cgroup_subsys_state css;
> +};
> +
> +struct drm_root_cgroup_state {
> + struct drm_cgroup_state drmcs;
> +};
> +
> +static struct drm_root_cgroup_state root_drmcs;
Special struct type for root state is uncommon.
Have
struct drm_cgroup_state root_drmcs;
and possible future members can be global state variables.
> +static void drmcs_free(struct cgroup_subsys_state *css)
> +{
> + struct drm_cgroup_state *drmcs = css_to_drmcs(css);
> +
> + if (drmcs != &root_drmcs.drmcs)
> + kfree(drmcs);
> +}
I think it can be relied on root cgroup not being ever free'd by cgroup
core.
Michal
signature.asc
Description: PGP signature
Re: [PATCH] drm/i915/panelreplay: Panel replay workaround with VRR
On Wed, 2024-02-07 at 20:05 +0530, Animesh Manna wrote:
> Panel Replay VSC SDP not getting sent when VRR is enabled
> and W1 and W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
Do you have Bspec number. You could add it here.
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 1b844cac4c58..c1ec78b5b6c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2618,8 +2618,16 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
> * TRANS_SET_CONTEXT_LATENCY to configure the pipe vblank
> start.
> */
> if (DISPLAY_VER(dev_priv) >= 13) {
> - intel_de_write(dev_priv,
> TRANS_SET_CONTEXT_LATENCY(cpu_transcoder),
> - crtc_vblank_start - crtc_vdisplay);
> + /*
> + * WA: Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register
Do you have HSD number to add here?
BR,
Jouni Högander
> + * to at least a value of 1 when Panel Replay is
> enabled with VRR.
> + */
> + if (crtc_state->vrr.enable && crtc_state-
> >has_panel_replay &&
> + (crtc_vblank_start == crtc_vdisplay))
> + intel_de_write(dev_priv,
> TRANS_SET_CONTEXT_LATENCY(cpu_transcoder), 1);
> + else
> + intel_de_write(dev_priv,
> TRANS_SET_CONTEXT_LATENCY(cpu_transcoder),
> + crtc_vblank_start -
> crtc_vdisplay);
>
> /*
> * VBLANK_START not used by hw, just clear it
Re: [PATCH 1/2] drm/i915: Prevent HW access during init from SDVO TV get_modes hook
On Wed, 2024-02-07 at 16:16 +0200, Imre Deak wrote:
> On Wed, Feb 07, 2024 at 03:45:15PM +0200, Hogander, Jouni wrote:
> > On Wed, 2024-02-07 at 15:26 +0200, Hogander, Jouni wrote:
> > > On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> > > > 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() - keep the previously
> > > > detected
> > > > mode list of the connector.
> > >
> > > I don't see where this is done? Not sure if looking at wrong
> > > place,
> > > but
> > > I see it tries using some override edid and in case that fails as
> > > well
> > > uses drm_add_modes_noedid?
> > >
> > > >
> > > > Signed-off-by: Imre Deak
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > index 2571ef5a1b211..ccea0efbd136f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > @@ -2287,6 +2287,7 @@ static const struct drm_display_mode
> > > > sdvo_tv_modes[] = {
> > > > static int intel_sdvo_get_tv_modes(struct drm_connector
> > > > *connector)
> > >
> > > I see intel_sdvo_get_tv_modes is called from
> > > intel_sdvo_get_modes.
> > > Why there is no need to do the same in intel_sdvo_get_lvds_modes
> > > and
> > > intel_sdvo_get_ddc_modes as well?
> >
> > It seems you are taking care of intel_svdo_get_ddc_modes in next
> > patch.
> > Intel_svdo_get_lvds_modes doesn't touch the hw -> nothing to do
> > there.
>
> Yes, all the connectors that read out an EDID - vs. the TV connector
> which doesn't - is handled by the next patch. And yes, the connectors
> which don't access the HW in the get_modes hook doesn't need this
> check.
> Btw, I think all the connectors should work - eventually - in this
> way:
> access the HW only in the encoder detect hook and from the get_modes
> hook only return the detected mode list w/o accessing the HW. DP-SST
> and
> HDMI do already the correct thing wrt. this.
>
Thank you for the clarification.
Reviewed-by: Jouni Högander
> >
> > >
> > > BR,
> > >
> > > Jouni Högander
> > >
> > > > {
> > > > struct intel_sdvo *intel_sdvo =
> > > > intel_attached_sdvo(to_intel_connector(connector));
> > > > + struct drm_i915_private *i915 = to_i915(intel_sdvo-
> > > > > base.base.dev);
> > > > struct intel_sdvo_connector *intel_sdvo_connector =
> > > > to_intel_sdvo_connector(connector);
> > > > const struct drm_connector_state *conn_state =
> > > > connector-
> > > > > state;
> > > > @@ -2298,6 +2299,9 @@ static int intel_sdvo_get_tv_modes(struct
> > > > drm_connector *connector)
> > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > > connector->base.id, connector->name);
> > > >
> > > > + if (!intel_display_driver_check_access(i915))
> > > > + return 0;
> > > > +
> > > > /*
> > > > * Read the list of supported input resolutions for the
> > > > selected TV
> > > > * format.
> > >
> >
Re: [PATCH 2/2] drm/i915: Prevent HW access during init from connector get_modes hooks
On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> 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.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10085
> Signed-off-by: Imre Deak
Reviewed-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_crt.c | 7 +++
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 4
> drivers/gpu/drm/i915/display/intel_dvo.c | 5 +
> drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> 4 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c
> b/drivers/gpu/drm/i915/display/intel_crt.c
> index b9733a73e21d4..27c5595e5d6bc 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -646,9 +646,13 @@ static const struct drm_edid
> *intel_crt_get_edid(struct drm_connector *connector
> static int intel_crt_ddc_get_modes(struct drm_connector *connector,
> struct i2c_adapter *ddc)
> {
> + struct drm_i915_private *i915 = to_i915(connector->dev);
> const struct drm_edid *drm_edid;
> int ret;
>
> + if (!intel_display_driver_check_access(i915))
> + return drm_edid_connector_add_modes(connector);
> +
> drm_edid = intel_crt_get_edid(connector, ddc);
> if (!drm_edid)
> return 0;
> @@ -933,6 +937,9 @@ static int intel_crt_get_modes(struct
> drm_connector *connector)
> struct i2c_adapter *ddc;
> int ret;
>
> + if (!intel_display_driver_check_access(dev_priv))
> + return drm_edid_connector_add_modes(connector);
> +
> wakeref = intel_display_power_get(dev_priv,
> intel_encoder-
> >power_domain);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 5fa25a5a36b55..5307ddd4edcf5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -1197,6 +1197,7 @@ static bool
> intel_dp_mst_initial_fastset_check(struct intel_encoder *encoder,
> static int intel_dp_mst_get_ddc_modes(struct drm_connector
> *connector)
> {
> struct intel_connector *intel_connector =
> to_intel_connector(connector);
> + struct drm_i915_private *i915 = to_i915(intel_connector-
> >base.dev);
> struct intel_dp *intel_dp = intel_connector->mst_port;
> const struct drm_edid *drm_edid;
> int ret;
> @@ -1204,6 +1205,9 @@ static int intel_dp_mst_get_ddc_modes(struct
> drm_connector *connector)
> if (drm_connector_is_unregistered(connector))
> return intel_connector_update_modes(connector, NULL);
>
> + if (!intel_display_driver_check_access(i915))
> + return drm_edid_connector_add_modes(connector);
> +
> drm_edid = drm_dp_mst_edid_read(connector, &intel_dp-
> >mst_mgr, intel_connector->port);
>
> ret = intel_connector_update_modes(connector, drm_edid);
> diff --git a/drivers/gpu/drm/i915/display/intel_dvo.c
> b/drivers/gpu/drm/i915/display/intel_dvo.c
> index 8ca9ae4798a89..c076da75b066e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_dvo.c
> @@ -30,6 +30,7 @@
>
> #include
> #include
> +#include
>
> #include "i915_drv.h"
> #include "i915_reg.h"
> @@ -338,8 +339,12 @@ intel_dvo_detect(struct drm_connector
> *_connector, bool force)
> static int intel_dvo_get_modes(struct drm_connector *_connector)
> {
> struct intel_connector *connector =
> to_intel_connector(_connector);
> + struct drm_i915_private *i915 = to_i915(connector->base.dev);
> int num_modes;
>
> + if (!intel_display_driver_check_access(i915))
> + return drm_edid_connector_add_modes(&connector-
> >base);
> +
> /*
> * We should probably have an i2c driver get_modes function
> for those
> * devices which will have a fixed set of modes determined by
> the chip
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index ccea0efbd136f..dae5abf56bbf5 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2189,12 +2189,16 @@ intel_sdvo_detect(struct drm_connector
> *connector, bool force)
>
> static int intel_sdvo_get_ddc_modes(struct drm_connector *connector)
> {
> + struct drm_i915_private *i915 = to_i915(connector->dev);
> int num_modes = 0;
> const struct drm_edid *drm_edid;
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> connector->base.id, connector->name);
>
> + if (!intel_display_driver_check_access(i915))
> +
Re: [PATCH] drm/i915/panelreplay: Panel replay workaround with VRR
On Wed, Feb 07, 2024 at 08:05:09PM +0530, Animesh Manna wrote:
> Panel Replay VSC SDP not getting sent when VRR is enabled
> and W1 and W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
>
> Signed-off-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 1b844cac4c58..c1ec78b5b6c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2618,8 +2618,16 @@ static void intel_set_transcoder_timings(const struct
> intel_crtc_state *crtc_sta
>* TRANS_SET_CONTEXT_LATENCY to configure the pipe vblank start.
>*/
> if (DISPLAY_VER(dev_priv) >= 13) {
> - intel_de_write(dev_priv,
> TRANS_SET_CONTEXT_LATENCY(cpu_transcoder),
> -crtc_vblank_start - crtc_vdisplay);
> + /*
> + * WA: Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> register
> + * to at least a value of 1 when Panel Replay is enabled with
> VRR.
> + */
> + if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
> + (crtc_vblank_start == crtc_vdisplay))
> + intel_de_write(dev_priv,
> TRANS_SET_CONTEXT_LATENCY(cpu_transcoder), 1);
Nak. This needs to match the actual timings we have stored in the mode.
> + else
> + intel_de_write(dev_priv,
> TRANS_SET_CONTEXT_LATENCY(cpu_transcoder),
> +crtc_vblank_start - crtc_vdisplay);
>
> /*
>* VBLANK_START not used by hw, just clear it
> --
> 2.29.0
--
Ville Syrjälä
Intel
✗ Fi.CI.BAT: failure for XE HDCP Enablement (rev3)
== Series Details ==
Series: XE HDCP Enablement (rev3)
URL : https://patchwork.freedesktop.org/series/129456/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_14239 -> Patchwork_129456v3
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_129456v3 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_129456v3, please notify your bug team
(i915-ci-in...@lists.freedesktop.org) to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/index.html
Participating hosts (37 -> 37)
--
Additional (2): fi-bsw-n3050 bat-arls-3
Missing(2): bat-mtlp-8 fi-snb-2520m
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_129456v3:
### IGT changes ###
Possible regressions
* igt@i915_selftest@live@hangcheck:
- bat-adln-1: [PASS][1] -> [ABORT][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-adln-1/igt@i915_selftest@l...@hangcheck.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/bat-adln-1/igt@i915_selftest@l...@hangcheck.html
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_selftest@live@ring_submission:
- {bat-arls-2}: NOTRUN -> [DMESG-FAIL][3] +21 other tests dmesg-fail
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/bat-arls-2/igt@i915_selftest@live@ring_submission.html
- {bat-arls-3}: NOTRUN -> [INCOMPLETE][4]
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/bat-arls-3/igt@i915_selftest@live@ring_submission.html
Known issues
Here are the changes found in Patchwork_129456v3 that come from known issues:
### CI changes ###
Issues hit
* boot:
- fi-bsw-n3050: NOTRUN -> [FAIL][5] ([i915#8293])
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/fi-bsw-n3050/boot.html
Possible fixes
* boot:
- fi-apl-guc: [FAIL][6] ([i915#8293]) -> [PASS][7]
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-apl-guc/boot.html
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/fi-apl-guc/boot.html
### IGT changes ###
Issues hit
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#4613]) +3
other tests skip
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/fi-apl-guc/igt@gem_lmem_swapp...@basic.html
* igt@kms_hdmi_inject@inject-audio:
- fi-apl-guc: NOTRUN -> [SKIP][9] ([fdo#109271]) +13 other tests
skip
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/fi-apl-guc/igt@kms_hdmi_inj...@inject-audio.html
Warnings
* igt@i915_module_load@reload:
- fi-kbl-7567u: [DMESG-WARN][10] ([i915#180] / [i915#8585]) ->
[DMESG-WARN][11] ([i915#180] / [i915#1982] / [i915#8585])
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@i915_module_l...@reload.html
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129456v3/fi-kbl-7567u/igt@i915_module_l...@reload.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#10194]: https://gitlab.freedesktop.org/drm/intel/issues/10194
[i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
[i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
[i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
[i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
[i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
[i915#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
[i915#10208]: https://gitlab.freedesktop.org/drm/intel/issues/10208
[i915#10209]: https://gitlab.freedesktop.org/drm/intel/issues/10209
[i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
[i915#10212]: https://gitlab.freedesktop.org/drm/intel/issues/10212
[i915#10213]: https://gitlab.freedesktop.org/drm/intel/issues/10213
[i915#10214]: https://gitlab.freedesktop.org/drm/intel/issues/10214
[i915#10215]: https://gitlab.freedesktop.org/drm/intel/issues/10215
[i915#10216]: https://gitlab.freedesktop.org/drm/intel/issues/10216
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4077]: https:/
Re: [PATCH 1/2] drm/i915: Prevent HW access during init from SDVO TV get_modes hook
On Wed, Feb 07, 2024 at 04:10:28PM +0200, Imre Deak wrote:
> On Wed, Feb 07, 2024 at 03:45:15PM +0200, Hogander, Jouni wrote:
> > On Wed, 2024-02-07 at 15:26 +0200, Hogander, Jouni wrote:
> > > On Tue, 2024-02-06 at 17:39 +0200, Imre Deak wrote:
> > > > 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() - keep the previously
> > > > detected
> > > > mode list of the connector.
> > >
> >
> > > I don't see where this is done? Not sure if looking at wrong place,
> > > but I see it tries using some override edid and in case that fails
> > > as well uses drm_add_modes_noedid?
>
> Some default and EDID override modes are also added to the connector
> mode list, similarly to when the HW access in intel_sdvo_get_tv_modes()
> fails (along with any modes specified via the kernel command line, which
> happens unconditionally, see drm_helper_probe_add_cmdline_mode()).
>
> All the modes detected by the encoder detect and the connector get_modes
> hooks gets added to the drm_connector::probed_modes list. From this
> __drm_helper_update_and_validate() will copy any new modes to the
> drm_connector::modes list (which will be returned to user space/kernel
> client). Thus, returning 0 from the above TV connector get_modes hook
> will preserve the list of modes on the drm_connector::modes list from an
> earlier encoder detect/connector get_modes call.
Drat, I realized the above is not quite correct. All the modes on
drm_connector:modes will be flagged as MODE_STALE and removed by
drm_mode_prune_invalid(). This means if get_modes() returns 0 only the
default/override modes will be returned, not the ones detected in an
earlier detect/get_modes call. I think this is ok, matching the case
where HW access fails, but needs at least a clarficiation in the
comment. Thanks for questioning it..
>
> > >
> > > >
> > > > Signed-off-by: Imre Deak
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_sdvo.c | 4
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > index 2571ef5a1b211..ccea0efbd136f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> > > > @@ -2287,6 +2287,7 @@ static const struct drm_display_mode
> > > > sdvo_tv_modes[] = {
> > > > static int intel_sdvo_get_tv_modes(struct drm_connector
> > > > *connector)
> > >
> > > I see intel_sdvo_get_tv_modes is called from intel_sdvo_get_modes.
> > > Why
> > > there is no need to do the same in intel_sdvo_get_lvds_modes and
> > > intel_sdvo_get_ddc_modes as well?
> >
> > It seems you are taking care of intel_svdo_get_ddc_modes in next patch.
> > Intel_svdo_get_lvds_modes doesn't touch the hw -> nothing to do there.
> >
> > >
> > > BR,
> > >
> > > Jouni Högander
> > >
> > > > {
> > > > struct intel_sdvo *intel_sdvo =
> > > > intel_attached_sdvo(to_intel_connector(connector));
> > > > + struct drm_i915_private *i915 = to_i915(intel_sdvo-
> > > > > base.base.dev);
> > > > struct intel_sdvo_connector *intel_sdvo_connector =
> > > > to_intel_sdvo_connector(connector);
> > > > const struct drm_connector_state *conn_state = connector-
> > > > > state;
> > > > @@ -2298,6 +2299,9 @@ static int intel_sdvo_get_tv_modes(struct
> > > > drm_connector *connector)
> > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > > connector->base.id, connector->name);
> > > >
> > > > + if (!intel_display_driver_check_access(i915))
> > > > + return 0;
> > > > +
> > > > /*
> > > > * Read the list of supported input resolutions for the
> > > > selected TV
> > > > * format.
> > >
> >
✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add GuC submission interface version query
== Series Details == Series: drm/i915: Add GuC submission interface version query URL : https://patchwork.freedesktop.org/series/129627/ State : warning == Summary == Error: dim checkpatch failed b1fb7e21feba drm/i915: Add GuC submission interface version query -:105: WARNING:LONG_LINE_COMMENT: line length of 103 exceeds 100 columns #105: FILE: include/uapi/drm/i915_drm.h:3016: +* - %DRM_I915_QUERY_GUC_SUBMISSION_VERSION (see struct drm_i915_query_guc_submission_version) -:122: WARNING:BLOCK_COMMENT_STYLE: Block comments should align the * on each line #122: FILE: include/uapi/drm/i915_drm.h:3572: +/** +* struct drm_i915_query_guc_submission_version - query GuC submission interface version total: 0 errors, 2 warnings, 0 checks, 73 lines checked
✗ Fi.CI.SPARSE: warning for drm/i915: Add GuC submission interface version query
== Series Details == Series: drm/i915: Add GuC submission interface version query URL : https://patchwork.freedesktop.org/series/129627/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately.
✓ Fi.CI.BAT: success for drm/i915: Add GuC submission interface version query
== Series Details ==
Series: drm/i915: Add GuC submission interface version query
URL : https://patchwork.freedesktop.org/series/129627/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_14239 -> Patchwork_129627v1
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/index.html
Participating hosts (37 -> 37)
--
Additional (2): fi-bsw-n3050 fi-pnv-d510
Missing(2): bat-mtlp-8 fi-snb-2520m
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_129627v1:
### IGT changes ###
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@gem_exec_fence@basic-busy@vcs1:
- {bat-arls-1}: [PASS][1] -> [DMESG-WARN][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-arls-1/igt@gem_exec_fence@basic-b...@vcs1.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/bat-arls-1/igt@gem_exec_fence@basic-b...@vcs1.html
Known issues
Here are the changes found in Patchwork_129627v1 that come from known issues:
### CI changes ###
Issues hit
* boot:
- fi-cfl-8109u: [PASS][3] -> [FAIL][4] ([i915#8293])
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-cfl-8109u/boot.html
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/fi-cfl-8109u/boot.html
### IGT changes ###
Issues hit
* igt@gem_lmem_swapping@basic:
- fi-pnv-d510:NOTRUN -> [SKIP][5] ([fdo#109271]) +28 other tests
skip
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/fi-pnv-d510/igt@gem_lmem_swapp...@basic.html
* igt@gem_lmem_swapping@random-engines:
- fi-bsw-n3050: NOTRUN -> [SKIP][6] ([fdo#109271]) +15 other tests
skip
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/fi-bsw-n3050/igt@gem_lmem_swapp...@random-engines.html
Possible fixes
* igt@gem_exec_create@basic@smem:
- {bat-arls-1}: [DMESG-WARN][7] ([i915#10194]) -> [PASS][8]
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-arls-1/igt@gem_exec_create@ba...@smem.html
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/bat-arls-1/igt@gem_exec_create@ba...@smem.html
* igt@i915_selftest@live@hangcheck:
- {bat-adls-6}: [DMESG-WARN][9] ([i915#5591]) -> [PASS][10]
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-adls-6/igt@i915_selftest@l...@hangcheck.html
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/bat-adls-6/igt@i915_selftest@l...@hangcheck.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#10194]: https://gitlab.freedesktop.org/drm/intel/issues/10194
[i915#10215]: https://gitlab.freedesktop.org/drm/intel/issues/10215
[i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591
[i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
Build changes
-
* Linux: CI_DRM_14239 -> Patchwork_129627v1
CI-20190529: 20190529
CI_DRM_14239: 473fff9e18e4e77aa4c1f1ae5484a6fb809a89e6 @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_7705: 45aef708b65772e54ee9a68b1f3885fa25140fdf @
https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_129627v1: 473fff9e18e4e77aa4c1f1ae5484a6fb809a89e6 @
git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
f552657aee88 drm/i915: Add GuC submission interface version query
== Logs ==
For more details see:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129627v1/index.html
Re: [PATCH 3/3] Start separating pipe vs transcoder set logic for bigjoiner during modeset
On Fri, Feb 02, 2024 at 12:02:30PM +0200, Stanislav Lisovskiy wrote:
> Handle only bigjoiner masters in skl_commit_modeset_enables/disables,
> slave crtcs should be handled by master hooks. Same for encoders.
> That way we can also remove a bunch of checks like
> intel_crtc_is_bigjoiner_slave.
>
> v2: Get rid of master vs slave checks and separation in crtc enable/disable
> hooks.
> Use unified iteration cycle for all of those, while enabling/disabling
> transcoder only for those pipes where its needed(Ville Syrjälä)
>
> Signed-off-by: Stanislav Lisovskiy
> ---
> drivers/gpu/drm/i915/display/intel_ddi.c | 3 +-
> drivers/gpu/drm/i915/display/intel_display.c | 116 +--
> drivers/gpu/drm/i915/display/intel_display.h | 6 +
> 3 files changed, 86 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 922194b957be2..6c690aefec9d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3340,8 +3340,7 @@ static void intel_enable_ddi(struct intel_atomic_state
> *state,
> {
> drm_WARN_ON(state->base.dev, crtc_state->has_pch_encoder);
>
> - if (!intel_crtc_is_bigjoiner_slave(crtc_state))
> - intel_ddi_enable_transcoder_func(encoder, crtc_state);
> + intel_ddi_enable_transcoder_func(encoder, crtc_state);
>
> /* Enable/Disable DP2.0 SDP split config before transcoder */
> intel_audio_sdp_split_update(crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 616a890b2658f..8fef59d1d119f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1631,18 +1631,12 @@ static void hsw_configure_cpu_transcoder(const struct
> intel_crtc_state *crtc_sta
> hsw_set_transconf(crtc_state);
> }
>
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> - struct intel_crtc *crtc)
> +static void hsw_crtc_enable_pre_transcoder(struct intel_atomic_state *state,
> +struct intel_crtc *crtc)
> {
> const struct intel_crtc_state *new_crtc_state =
> intel_atomic_get_new_crtc_state(state, crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> - bool psl_clkgate_wa;
> -
> - if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> - return;
>
> intel_dmc_enable_pipe(dev_priv, crtc->pipe);
>
> @@ -1665,10 +1659,16 @@ static void hsw_crtc_enable(struct intel_atomic_state
> *state,
> intel_set_pipe_src_size(new_crtc_state);
> if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> bdw_set_pipe_misc(new_crtc_state);
> +}
>
> - if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> - !transcoder_is_dsi(cpu_transcoder))
> - hsw_configure_cpu_transcoder(new_crtc_state);
> +static void hsw_crtc_enable_post_transcoder(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + const struct intel_crtc_state *new_crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> + bool psl_clkgate_wa;
>
> crtc->active = true;
>
> @@ -1701,8 +1701,7 @@ static void hsw_crtc_enable(struct intel_atomic_state
> *state,
>
> intel_initial_watermarks(state, crtc);
>
> - if (intel_crtc_is_bigjoiner_slave(new_crtc_state))
> - intel_crtc_vblank_on(new_crtc_state);
> + intel_crtc_vblank_on(new_crtc_state);
>
> intel_encoders_enable(state, crtc);
All the encoder stuff belongs in the transcoder-only path.
>
> @@ -1724,6 +1723,40 @@ static void hsw_crtc_enable(struct intel_atomic_state
> *state,
> }
> }
>
> +static void hsw_crtc_enable(struct intel_atomic_state *state,
> + struct intel_crtc *crtc)
> +{
> + const struct intel_crtc_state *new_crtc_state =
> + intel_atomic_get_new_crtc_state(state, crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> + struct intel_crtc *_crtc;
> + int slave_pipe_mask = intel_crtc_bigjoiner_slave_pipes(new_crtc_state);
> + int pipe_mask = slave_pipe_mask | crtc->pipe;
> +
> + if (drm_WARN_ON(&dev_priv->drm, crtc->active))
> + return;
> +
> + /*
> + * Use reverse iterator to go through slave pipes first.
> + * TODO: We might need smarter iterator here
> + */
> + for_each_intel_crtc_in_pipe_mask_reverse(&dev_priv->dr
✓ Fi.CI.BAT: success for drm/i915/cdclk: More hardcoded cd2x divider nukage (rev2)
== Series Details ==
Series: drm/i915/cdclk: More hardcoded cd2x divider nukage (rev2)
URL : https://patchwork.freedesktop.org/series/129611/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_14239 -> Patchwork_129611v2
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/index.html
Participating hosts (37 -> 37)
--
Additional (2): fi-bsw-n3050 fi-pnv-d510
Missing(2): bat-mtlp-8 fi-snb-2520m
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_129611v2:
### IGT changes ###
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_module_load@load:
- {bat-arls-2}: [PASS][1] -> [ABORT][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-arls-2/igt@i915_module_l...@load.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/bat-arls-2/igt@i915_module_l...@load.html
* igt@i915_selftest@live@dmabuf:
- {bat-arls-1}: [DMESG-WARN][3] ([i915#10194]) -> [DMESG-FAIL][4]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-arls-1/igt@i915_selftest@l...@dmabuf.html
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/bat-arls-1/igt@i915_selftest@l...@dmabuf.html
Known issues
Here are the changes found in Patchwork_129611v2 that come from known issues:
### CI changes ###
Possible fixes
* boot:
- fi-apl-guc: [FAIL][5] ([i915#8293]) -> [PASS][6]
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-apl-guc/boot.html
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-apl-guc/boot.html
### IGT changes ###
Issues hit
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#4613]) +3
other tests skip
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-apl-guc/igt@gem_lmem_swapp...@basic.html
- fi-pnv-d510:NOTRUN -> [SKIP][8] ([fdo#109271]) +28 other tests
skip
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-pnv-d510/igt@gem_lmem_swapp...@basic.html
* igt@gem_lmem_swapping@random-engines:
- fi-bsw-n3050: NOTRUN -> [SKIP][9] ([fdo#109271]) +15 other tests
skip
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-bsw-n3050/igt@gem_lmem_swapp...@random-engines.html
* igt@i915_selftest@live@hangcheck:
- fi-skl-guc: [PASS][10] -> [DMESG-FAIL][11] ([i915#10112])
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-skl-guc/igt@i915_selftest@l...@hangcheck.html
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-skl-guc/igt@i915_selftest@l...@hangcheck.html
* igt@kms_hdmi_inject@inject-audio:
- fi-apl-guc: NOTRUN -> [SKIP][12] ([fdo#109271]) +13 other tests
skip
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-apl-guc/igt@kms_hdmi_inj...@inject-audio.html
Warnings
* igt@i915_module_load@reload:
- fi-kbl-7567u: [DMESG-WARN][13] ([i915#180] / [i915#8585]) ->
[DMESG-WARN][14] ([i915#180] / [i915#1982] / [i915#8585])
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@i915_module_l...@reload.html
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129611v2/fi-kbl-7567u/igt@i915_module_l...@reload.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#10112]: https://gitlab.freedesktop.org/drm/intel/issues/10112
[i915#10194]: https://gitlab.freedesktop.org/drm/intel/issues/10194
[i915#10215]: https://gitlab.freedesktop.org/drm/intel/issues/10215
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
[i915#8585]: https://gitlab.freedesktop.org/drm/intel/issues/8585
Build changes
-
* Linux: CI_DRM_14239 -> Patchwork_129611v2
CI-20190529: 20190529
CI_DRM_14239: 473fff9e18e4e77aa4c1f1ae5484a6fb809a89e6 @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_7705: 45aef708b65772e54ee9a68b1f3885fa25140fdf @
https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_129611v2: 473fff9e18e4e77aa4c1f1ae5484a6fb809a89e6 @
git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
cc5d4cf08c23 drm/i915/cdclk: Document CDCLK update methods
531a06bbaa70 drm/i915/cdclk: Remove the hardcoded divider from
cdclk_comput
✗ Fi.CI.CHECKPATCH: warning for drm/i915/panelreplay: Panel replay workaround with VRR
== Series Details == Series: drm/i915/panelreplay: Panel replay workaround with VRR URL : https://patchwork.freedesktop.org/series/129632/ State : warning == Summary == Error: dim checkpatch failed 2d9bc540b38f drm/i915/panelreplay: Panel replay workaround with VRR -:26: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'crtc_vblank_start == crtc_vdisplay' #26: FILE: drivers/gpu/drm/i915/display/intel_display.c:2625: + if (crtc_state->vrr.enable && crtc_state->has_panel_replay && + (crtc_vblank_start == crtc_vdisplay)) total: 0 errors, 0 warnings, 1 checks, 18 lines checked
✗ Fi.CI.BAT: failure for drm/i915/panelreplay: Panel replay workaround with VRR
== Series Details ==
Series: drm/i915/panelreplay: Panel replay workaround with VRR
URL : https://patchwork.freedesktop.org/series/129632/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_14239 -> Patchwork_129632v1
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_129632v1 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_129632v1, please notify your bug team
(i915-ci-in...@lists.freedesktop.org) to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/index.html
Participating hosts (37 -> 39)
--
Additional (3): bat-arls-3 fi-bsw-n3050 fi-pnv-d510
Missing(1): fi-snb-2520m
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_129632v1:
### IGT changes ###
Possible regressions
* igt@i915_selftest@live@client:
- bat-adlm-1: [PASS][1] -> [INCOMPLETE][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/bat-adlm-1/igt@i915_selftest@l...@client.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/bat-adlm-1/igt@i915_selftest@l...@client.html
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@kms_addfb_basic@basic-x-tiled-legacy:
- {bat-arls-3}: NOTRUN -> [ABORT][3]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/bat-arls-3/igt@kms_addfb_ba...@basic-x-tiled-legacy.html
Known issues
Here are the changes found in Patchwork_129632v1 that come from known issues:
### CI changes ###
Possible fixes
* boot:
- fi-apl-guc: [FAIL][4] ([i915#8293]) -> [PASS][5]
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-apl-guc/boot.html
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/fi-apl-guc/boot.html
### IGT changes ###
Issues hit
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#4613]) +3
other tests skip
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/fi-apl-guc/igt@gem_lmem_swapp...@basic.html
- fi-pnv-d510:NOTRUN -> [SKIP][7] ([fdo#109271]) +28 other tests
skip
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/fi-pnv-d510/igt@gem_lmem_swapp...@basic.html
* igt@gem_lmem_swapping@random-engines:
- fi-bsw-n3050: NOTRUN -> [SKIP][8] ([fdo#109271]) +15 other tests
skip
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/fi-bsw-n3050/igt@gem_lmem_swapp...@random-engines.html
* igt@kms_hdmi_inject@inject-audio:
- fi-apl-guc: NOTRUN -> [SKIP][9] ([fdo#109271]) +13 other tests
skip
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/fi-apl-guc/igt@kms_hdmi_inj...@inject-audio.html
Warnings
* igt@i915_module_load@reload:
- fi-kbl-7567u: [DMESG-WARN][10] ([i915#180] / [i915#8585]) ->
[DMESG-WARN][11] ([i915#180] / [i915#1982] / [i915#8585])
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@i915_module_l...@reload.html
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129632v1/fi-kbl-7567u/igt@i915_module_l...@reload.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#10194]: https://gitlab.freedesktop.org/drm/intel/issues/10194
[i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
[i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
[i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
[i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
[i915#10211]: https://gitlab.freedesktop.org/drm/intel/issues/10211
[i915#10215]: https://gitlab.freedesktop.org/drm/intel/issues/10215
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
[i915#8585]: https://gitlab.freedesktop.org/drm/intel/issues/8585
[i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
Build changes
-
* Linux: CI_DRM_14239 -> Patchwork_129632v1
CI-20190529: 20190529
CI_DRM_14239: 473fff9e18e4e77aa4c1f1ae5484a6fb809a89e6 @
[PATCH] drm/buddy: Fix alloc_range() error handling code
Few users have observed display corruption when they boot
the machine to KDE Plasma or playing games. We have root
caused the problem that whenever alloc_range() couldn't
find the required memory blocks the function was returning
SUCCESS in some of the corner cases.
The right approach would be if the total allocated size
is less than the required size, the function should
return -ENOSPC.
Gitlab ticket link - https://gitlab.freedesktop.org/drm/amd/-/issues/3097
Fixes: 0a1844bf0b53 ("drm/buddy: Improve contiguous memory allocation")
Signed-off-by: Arunpravin Paneer Selvam
Tested-by: Mario Limonciello
---
drivers/gpu/drm/drm_buddy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index f57e6d74fb0e..c1a99bf4dffd 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -539,6 +539,12 @@ static int __alloc_range(struct drm_buddy *mm,
} while (1);
list_splice_tail(&allocated, blocks);
+
+ if (total_allocated < size) {
+ err = -ENOSPC;
+ goto err_free;
+ }
+
return 0;
err_undo:
--
2.25.1
Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
On 2/7/2024 03:36, Joonas Lahtinen wrote:
Quoting Tvrtko Ursulin (2024-02-07 10:44:01)
On 06/02/2024 20:51, Souza, Jose wrote:
On Tue, 2024-02-06 at 12:42 -0800, John Harrison wrote:
On 2/6/2024 08:33, Tvrtko Ursulin wrote:
On 01/02/2024 18:25, Souza, Jose wrote:
On Wed, 2024-01-24 at 08:55 +, Tvrtko Ursulin wrote:
On 24/01/2024 08:19, Joonas Lahtinen wrote:
Add reporting of the GuC submissio/VF interface version via GETPARAM
properties. Mesa intends to use this information to check for old
firmware versions with known bugs before enabling features like async
compute.
There was
https://patchwork.freedesktop.org/patch/560704/?series=124592&rev=1
which does everything in one go so would be my preference.
IMO Joonas version brings less burden to be maintained(no new struct).
But both versions works, please just get into some agreement so we
can move this forward.
So I would really prefer the query. Simplified version would do like
the compile tested only:
Vivaik's patch is definitely preferred. It is much cleaner to make one
single call than having to make four separate calls. It is also
extensible to other firmwares if required. The only blockage against it
was whether it was a good thing to report at all. If that blockage is no
longer valid then we should just merge the patch that has already been
discussed, polished, fixed, etc. rather than starting the whole process
from scratch.
Agreed.
Vivaik can you please rebase and send it again?
Note there was review feedback not addressed so do that too please.
AFAIR incorrect usage of copy item, pad/rsvd/mbz checking and questions
about padding in general. Last is why I proposed a simplified version
which is not future extensible and avoids the need for padding.
Yeah, I don't think there is point an adding an extensible interface as
we're not going to add further FW version queries. This only the
submission interface version we're going to expose:
The media team have flip flopped multiple times about whether they need
a HuC version query.
* Note that the spec for the CSS header defines this version number
* as 'vf_version' as it was originally intended for virtualisation.
* However, it is applicable to native submission as well.
If somebody wants to work on the simplified version like Tvrtko
suggested below, I have no objection. We can also remove the reference
to the VF version even if that's used by the header definition.
But if there are just suggestions but no actual patches floated, then we
should be merging the GETPARAM version with the "VF" word removed.
The original patch was posted to the mailing list many months ago. Why
do you say 'just suggestions but no patches floated'?
We've already discussed on the topic for some months so doing the
minimal changes to fulfill Mesa requirements should be considered a
priority to avoid further delays.
Regards,
Tvrtko
And note that it is four calls not three. The code below is missing the
branch version number.
Not even kernel uses the 'build' version anywhere. I don't see how there
could be 'build' version for the VF interface version? It's not supposed
to version a concrete firmware build but the API contract implemented by
the build where patch version should already be incremented for each
fix.
So adding the build does not seem appropriate as there is no plan to
extend this API any further.
I did not say "build" version. There is no build version. I said
"branch" version. And the branch version absolute becomes important if
we ever have to release a bug fix to an LTS branch. So it needs to be
part of the interface from the beginning and the UMDs need to be using
it from the beginning.
John.
Regards, Joonas
John.
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..999687f6a3d4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct
drm_i915_private *i915,
return hwconfig->size;
}
+static int
+query_guc_submission_version(struct drm_i915_private *i915,
+ struct drm_i915_query_item *query)
+{
+ struct drm_i915_query_guc_submission_version __user *query_ptr =
+ u64_to_user_ptr(query->data_ptr);
+ struct drm_i915_query_guc_submission_version ver;
+ struct intel_guc *guc = &to_gt(i915)->uc.guc;
+ const size_t size = sizeof(ver);
+ int ret;
+
+ if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
+ return -ENODEV;
+
+ ret = copy_query_item(&ver, size, size, query);
+ if (ret != 0)
+ return ret;
+
+ if (ver.major || ver.minor || ver.patch)
+ return -EINVAL;
+
+ ver.major = guc->submission_version.major;
+ ver.minor = guc->submission_version.minor;
+ ver.patch = guc->submission_version.patch;
+
+ if (copy_to_user(query_ptr, &ver, size))
+
[PATCH] dma-buf: try to catch swiotlb bounce buffers
They rather fundamentally break the entire concept of zero copy, so if
an exporter manages to hand these out things will break all over.
Luckily there's not too many case that use
swiotlb_sync_single_for_device/cpu():
- The generic iommu dma-api code in drivers/iommu/dma-iommu.c. We can
catch that with sg_dma_is_swiotlb() reliably.
- The generic direct dma code in kernel/dma/direct.c. We can mostly
catch that with looking for a NULL dma_ops, except for some powerpc
special cases.
- Xen, which I don't bother to catch here.
Implement these checks in dma_buf_map_attachment when
CONFIG_DMA_API_DEBUG is enabled.
Signed-off-by: Daniel Vetter
Cc: Sumit Semwal
Cc: "Christian König"
Cc: linux-me...@vger.kernel.org
Cc: linaro-mm-...@lists.linaro.org
Cc: Paul Cercueil
---
Entirely untested, but since I sent the mail with the idea I figured I
might as well type it up after I realized there's a lot fewer cases to
check. That is, if I haven't completely misread the dma-api and swiotlb
code.
-Sima
---
drivers/dma-buf/dma-buf.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d1e7f823fbdb..d6f95523f995 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -28,6 +28,12 @@
#include
#include
+#ifdef CONFIG_DMA_API_DEBUG
+#include
+#include
+#include
+#endif
+
#include
#include
@@ -1149,10 +1155,13 @@ struct sg_table *dma_buf_map_attachment(struct
dma_buf_attachment *attach,
#ifdef CONFIG_DMA_API_DEBUG
if (!IS_ERR(sg_table)) {
struct scatterlist *sg;
+ struct device *dev = attach->dev;
u64 addr;
int len;
int i;
+ bool is_direct_dma = !get_dma_ops(dev);
+
for_each_sgtable_dma_sg(sg_table, sg, i) {
addr = sg_dma_address(sg);
len = sg_dma_len(sg);
@@ -1160,7 +1169,15 @@ struct sg_table *dma_buf_map_attachment(struct
dma_buf_attachment *attach,
pr_debug("%s: addr %llx or len %x is not page
aligned!\n",
__func__, addr, len);
}
+
+ if (is_direct_dma) {
+ phys_addr_t paddr = dma_to_phys(dev, addr);
+
+ WARN_ON_ONCE(is_swiotlb_buffer(dev, paddr));
+ }
}
+
+ WARN_ON_ONCE(sg_dma_is_swiotlb(sg));
}
#endif /* CONFIG_DMA_API_DEBUG */
return sg_table;
--
2.43.0
Re: [RFC PATCH] drm/i915: Add GETPARAM for GuC submission version
On Wed, 2024-02-07 at 13:36 +0200, Joonas Lahtinen wrote:
> Quoting Tvrtko Ursulin (2024-02-07 10:44:01)
> >
> > On 06/02/2024 20:51, Souza, Jose wrote:
> > > On Tue, 2024-02-06 at 12:42 -0800, John Harrison wrote:
> > > > On 2/6/2024 08:33, Tvrtko Ursulin wrote:
> > > > > On 01/02/2024 18:25, Souza, Jose wrote:
> > > > > > On Wed, 2024-01-24 at 08:55 +, Tvrtko Ursulin wrote:
> > > > > > > On 24/01/2024 08:19, Joonas Lahtinen wrote:
> > > > > > > > Add reporting of the GuC submissio/VF interface version via
> > > > > > > > GETPARAM
> > > > > > > > properties. Mesa intends to use this information to check for
> > > > > > > > old
> > > > > > > > firmware versions with known bugs before enabling features like
> > > > > > > > async
> > > > > > > > compute.
> > > > > > >
> > > > > > > There was
> > > > > > > https://patchwork.freedesktop.org/patch/560704/?series=124592&rev=1
> > > > > > > which does everything in one go so would be my preference.
> > > > > >
> > > > > > IMO Joonas version brings less burden to be maintained(no new
> > > > > > struct).
> > > > > > But both versions works, please just get into some agreement so we
> > > > > > can move this forward.
> > > > >
> > > > > So I would really prefer the query. Simplified version would do like
> > > > > the compile tested only:
> > > > Vivaik's patch is definitely preferred. It is much cleaner to make one
> > > > single call than having to make four separate calls. It is also
> > > > extensible to other firmwares if required. The only blockage against it
> > > > was whether it was a good thing to report at all. If that blockage is no
> > > > longer valid then we should just merge the patch that has already been
> > > > discussed, polished, fixed, etc. rather than starting the whole process
> > > > from scratch.
> > >
> > > Agreed.
> > >
> > > Vivaik can you please rebase and send it again?
> >
> > Note there was review feedback not addressed so do that too please.
> > AFAIR incorrect usage of copy item, pad/rsvd/mbz checking and questions
> > about padding in general. Last is why I proposed a simplified version
> > which is not future extensible and avoids the need for padding.
>
> Yeah, I don't think there is point an adding an extensible interface as
> we're not going to add further FW version queries. This only the
> submission interface version we're going to expose:
>
> * Note that the spec for the CSS header defines this version number
> * as 'vf_version' as it was originally intended for virtualisation.
> * However, it is applicable to native submission as well.
>
> If somebody wants to work on the simplified version like Tvrtko
> suggested below, I have no objection. We can also remove the reference
> to the VF version even if that's used by the header definition.
>
> But if there are just suggestions but no actual patches floated, then we
> should be merging the GETPARAM version with the "VF" word removed.
>
> We've already discussed on the topic for some months so doing the
> minimal changes to fulfill Mesa requirements should be considered a
> priority to avoid further delays.
This is
Reviewed-by: José Roberto de Souza
Tested-by: José Roberto de Souza
Here the user-space usage:
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25233
>
> >
> > Regards,
> >
> > Tvrtko
> >
> > >
> > >
> > > >
> > > > And note that it is four calls not three. The code below is missing the
> > > > branch version number.
>
> Not even kernel uses the 'build' version anywhere. I don't see how there
> could be 'build' version for the VF interface version? It's not supposed
> to version a concrete firmware build but the API contract implemented by
> the build where patch version should already be incremented for each
> fix.
>
> So adding the build does not seem appropriate as there is no plan to
> extend this API any further.
>
> Regards, Joonas
>
> > > >
> > > > John.
> > > >
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_query.c
> > > > > b/drivers/gpu/drm/i915/i915_query.c
> > > > > index 00871ef99792..999687f6a3d4 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_query.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_query.c
> > > > > @@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct
> > > > > drm_i915_private *i915,
> > > > > return hwconfig->size;
> > > > > }
> > > > >
> > > > > +static int
> > > > > +query_guc_submission_version(struct drm_i915_private *i915,
> > > > > + struct drm_i915_query_item *query)
> > > > > +{
> > > > > + struct drm_i915_query_guc_submission_version __user
> > > > > *query_ptr =
> > > > > + u64_to_user_ptr(query->data_ptr);
> > > > > + struct drm_i915_query_guc_submission_version ver;
> > > > > + struct intel_guc *guc = &to_gt(i915)->uc.guc;
> > > > > + const size_t size = sizeof(ver);
> > > > > + int ret;
> > > > > +
> > > > > + if (!intel_uc_uses_guc_submission(&to_gt(
Re: [RFC] drm/i915: Add GuC submission interface version query
On 2/7/2024 03:56, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Add a new query to the GuC submission interface version.
Mesa intends to use this information to check for old firmware versions
with a known bug where using the render and compute command streamers
simultaneously can cause GPU hangs due issues in firmware scheduling.
Based on patches from Vivaik and Joonas.
There is a little bit of an open around the width required for versions.
While the GuC FW iface tells they are u8, i915 GuC code uses u32:
#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
...
struct intel_uc_fw_ver {
u32 major;
u32 minor;
u32 patch;
u32 build;
};
This is copied from generic code which supports firmwares other than
GuC. Only GuC promises to use 8-bit version components. Other firmwares
very definitely do not. There is no open.
So we could make the query u8, and refactor the struct intel_uc_fw_ver
to use u8, or not. To avoid any doubts on why are we assigning u32 to
u8 I simply opted to use u64. Which avoids the need to add any padding
too.
I don't follow how potential 8 vs 32 confusion means jump to 64?!
Compile tested only.
Signed-off-by: Tvrtko Ursulin
Cc: Kenneth Graunke
Cc: Jose Souza
Cc: Sagar Ghuge
Cc: Paulo Zanoni
Cc: John Harrison
Cc: Rodrigo Vivi
Cc: Jani Nikula
Cc: Tvrtko Ursulin
Cc: Vivaik Balasubrawmanian
---
drivers/gpu/drm/i915/i915_query.c | 32 +++
include/uapi/drm/i915_drm.h | 11 +++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..999687f6a3d4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct drm_i915_private
*i915,
return hwconfig->size;
}
+static int
+query_guc_submission_version(struct drm_i915_private *i915,
+struct drm_i915_query_item *query)
+{
+ struct drm_i915_query_guc_submission_version __user *query_ptr =
+ u64_to_user_ptr(query->data_ptr);
+ struct drm_i915_query_guc_submission_version ver;
+ struct intel_guc *guc = &to_gt(i915)->uc.guc;
+ const size_t size = sizeof(ver);
+ int ret;
+
+ if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
+ return -ENODEV;
+
+ ret = copy_query_item(&ver, size, size, query);
+ if (ret != 0)
+ return ret;
+
+ if (ver.major || ver.minor || ver.patch)
+ return -EINVAL;
+
+ ver.major = guc->submission_version.major;
+ ver.minor = guc->submission_version.minor;
+ ver.patch = guc->submission_version.patch;
This needs to include the branch version (currently set to zero) in the
definition. And the UMD needs to barf if branch comes back as non-zero.
I.e. there is no guarantee that a branched version will have the w/a +
fix that they are wanting.
John.
+
+ if (copy_to_user(query_ptr, &ver, size))
+ return -EFAULT;
+
+ return 0;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item)
= {
query_topology_info,
@@ -559,6 +590,7 @@ static int (* const i915_query_funcs[])(struct
drm_i915_private *dev_priv,
query_memregion_info,
query_hwconfig_blob,
query_geometry_subslices,
+ query_guc_submission_version,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 550c496ce76d..d80d9b5e1eda 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3038,6 +3038,7 @@ struct drm_i915_query_item {
* - %DRM_I915_QUERY_MEMORY_REGIONS (see struct
drm_i915_query_memory_regions)
* - %DRM_I915_QUERY_HWCONFIG_BLOB (see `GuC HWCONFIG blob uAPI`)
* - %DRM_I915_QUERY_GEOMETRY_SUBSLICES (see struct
drm_i915_query_topology_info)
+* - %DRM_I915_QUERY_GUC_SUBMISSION_VERSION (see struct
drm_i915_query_guc_submission_version)
*/
__u64 query_id;
#define DRM_I915_QUERY_TOPOLOGY_INFO 1
@@ -3046,6 +3047,7 @@ struct drm_i915_query_item {
#define DRM_I915_QUERY_MEMORY_REGIONS 4
#define DRM_I915_QUERY_HWCONFIG_BLOB 5
#define DRM_I915_QUERY_GEOMETRY_SUBSLICES 6
+#define DRM_I915_QUERY_GUC_SUBMISSION_VERSION 7
/* Must be kept compact -- no holes and well documented */
/**
@@ -3591,6 +3593,15 @@ struct drm_i915_query_memory_regions {
struct drm_i915_memory_region_info regions[];
};
+/**
+* struct drm_i915_query_guc_submission_version - query GuC submission
inte
✓ Fi.CI.BAT: success for drm/buddy: Fix alloc_range() error handling code
== Series Details ==
Series: drm/buddy: Fix alloc_range() error handling code
URL : https://patchwork.freedesktop.org/series/129637/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_14239 -> Patchwork_129637v1
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/index.html
Participating hosts (37 -> 36)
--
Additional (2): fi-bsw-n3050 bat-arls-3
Missing(3): bat-arls-2 fi-snb-2520m bat-adls-6
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_129637v1:
### IGT changes ###
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_selftest@live@ring_submission:
- {bat-arls-3}: NOTRUN -> [INCOMPLETE][1]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/bat-arls-3/igt@i915_selftest@live@ring_submission.html
Known issues
Here are the changes found in Patchwork_129637v1 that come from known issues:
### CI changes ###
Possible fixes
* boot:
- fi-apl-guc: [FAIL][2] ([i915#8293]) -> [PASS][3]
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-apl-guc/boot.html
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-apl-guc/boot.html
### IGT changes ###
Issues hit
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#4613]) +3
other tests skip
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-apl-guc/igt@gem_lmem_swapp...@basic.html
* igt@gem_lmem_swapping@random-engines:
- fi-bsw-n3050: NOTRUN -> [SKIP][5] ([fdo#109271]) +15 other tests
skip
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-bsw-n3050/igt@gem_lmem_swapp...@random-engines.html
* igt@kms_hdmi_inject@inject-audio:
- fi-apl-guc: NOTRUN -> [SKIP][6] ([fdo#109271]) +13 other tests
skip
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-apl-guc/igt@kms_hdmi_inj...@inject-audio.html
Possible fixes
* igt@i915_selftest@live@sanitycheck:
- fi-kbl-7567u: [DMESG-WARN][7] ([i915#9730]) -> [PASS][8] +36 other
tests pass
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@i915_selftest@l...@sanitycheck.html
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-kbl-7567u/igt@i915_selftest@l...@sanitycheck.html
* igt@kms_addfb_basic@invalid-set-prop:
- fi-kbl-7567u: [DMESG-WARN][9] ([i915#8585] / [i915#9730]) ->
[PASS][10] +39 other tests pass
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@kms_addfb_ba...@invalid-set-prop.html
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-kbl-7567u/igt@kms_addfb_ba...@invalid-set-prop.html
* igt@kms_force_connector_basic@prune-stale-modes:
- fi-kbl-7567u: [DMESG-WARN][11] ([i915#8585]) -> [PASS][12] +1 other
test pass
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@kms_force_connector_ba...@prune-stale-modes.html
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-kbl-7567u/igt@kms_force_connector_ba...@prune-stale-modes.html
* igt@kms_frontbuffer_tracking@basic:
- fi-kbl-7567u: [DMESG-WARN][13] ([i915#180] / [i915#8585] /
[i915#9730]) -> [PASS][14]
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@kms_frontbuffer_track...@basic.html
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-kbl-7567u/igt@kms_frontbuffer_track...@basic.html
* igt@kms_pm_rpm@basic-pci-d3-state:
- fi-kbl-7567u: [DMESG-WARN][15] ([i915#180] / [i915#8585]) ->
[PASS][16] +44 other tests pass
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14239/fi-kbl-7567u/igt@kms_pm_...@basic-pci-d3-state.html
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129637v1/fi-kbl-7567u/igt@kms_pm_...@basic-pci-d3-state.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#10194]: https://gitlab.freedesktop.org/drm/intel/issues/10194
[i915#10196]: https://gitlab.freedesktop.org/drm/intel/issues/10196
[i915#10197]: https://gitlab.freedesktop.org/drm/intel/issues/10197
[i915#10200]: https://gitlab.freedesktop.org/drm/intel/issues/10200
[i915#10202]: https://gitlab.freedesktop.org/drm/intel/issues/10202
[i915#10206]: https://gitlab.freedesktop.org/drm/intel/issues/10206
[i915#10207]: https://gitlab.freedesktop.org/drm/intel/issues/10207
[i915#10208]: https://gitlab.freedeskt
Re: [RFC] drm/i915: Add GuC submission interface version query
On 07/02/2024 18:12, John Harrison wrote:
On 2/7/2024 03:56, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Add a new query to the GuC submission interface version.
Mesa intends to use this information to check for old firmware versions
with a known bug where using the render and compute command streamers
simultaneously can cause GPU hangs due issues in firmware scheduling.
Based on patches from Vivaik and Joonas.
There is a little bit of an open around the width required for versions.
While the GuC FW iface tells they are u8, i915 GuC code uses u32:
#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
...
struct intel_uc_fw_ver {
u32 major;
u32 minor;
u32 patch;
u32 build;
};
This is copied from generic code which supports firmwares other than
GuC. Only GuC promises to use 8-bit version components. Other firmwares
very definitely do not. There is no open.
Ack.
So we could make the query u8, and refactor the struct intel_uc_fw_ver
to use u8, or not. To avoid any doubts on why are we assigning u32 to
u8 I simply opted to use u64. Which avoids the need to add any padding
too.
I don't follow how potential 8 vs 32 confusion means jump to 64?!
Suggestion was to use u8 in the uapi in order to align with GuC FW ABI (or
however it's called), in which case there would be:
ver.major = guc->submission_version.major;
which would be:
(u8) = (u32)
And I was anticipating someone not liking that either. Using too wide u64
simply avoids the need to add a padding element to the uapi struct.
If you are positive we need to include a branch number, even though it does not
seem to be implemented in the code even(*) then I can make uapi 4x u32 and
achieve the same.
(*)
static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32 css_value)
{
/* Get version numbers from the CSS header */
ver->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR, css_value);
ver->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css_value);
ver->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, css_value);
}
No branch field in the CSS header?
And Why is UMD supposed to reject a non-zero branch? Like how would 1.1.3.0 be
fine and 1.1.3.1 be bad? I don't get it. But anyway, I can respin if you
definitely confirm.
Regards,
Tvrtko
Compile tested only.
Signed-off-by: Tvrtko Ursulin
Cc: Kenneth Graunke
Cc: Jose Souza
Cc: Sagar Ghuge
Cc: Paulo Zanoni
Cc: John Harrison
Cc: Rodrigo Vivi
Cc: Jani Nikula
Cc: Tvrtko Ursulin
Cc: Vivaik Balasubrawmanian
---
drivers/gpu/drm/i915/i915_query.c | 32 +++
include/uapi/drm/i915_drm.h | 11 +++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..999687f6a3d4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct
drm_i915_private *i915,
return hwconfig->size;
}
+static int
+query_guc_submission_version(struct drm_i915_private *i915,
+ struct drm_i915_query_item *query)
+{
+ struct drm_i915_query_guc_submission_version __user *query_ptr =
+ u64_to_user_ptr(query->data_ptr);
+ struct drm_i915_query_guc_submission_version ver;
+ struct intel_guc *guc = &to_gt(i915)->uc.guc;
+ const size_t size = sizeof(ver);
+ int ret;
+
+ if (!intel_uc_uses_guc_submission(&to_gt(i915)->uc))
+ return -ENODEV;
+
+ ret = copy_query_item(&ver, size, size, query);
+ if (ret != 0)
+ return ret;
+
+ if (ver.major || ver.minor || ver.patch)
+ return -EINVAL;
+
+ ver.major = guc->submission_version.major;
+ ver.minor = guc->submission_version.minor;
+ ver.patch = guc->submission_version.patch;
This needs to include the branch version (currently set to zero) in the
definition. And the UMD needs to barf if branch comes back as non-zero.
I.e. there is no guarantee that a branched version will have the w/a +
fix that they are wanting.
John.
+
+ if (copy_to_user(query_ptr, &ver, size))
+ return -EFAULT;
+
+ return 0;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private
*dev_priv,
struct drm_i915_query_item *query_item) = {
query_topology_info,
@@ -559,6 +590,7 @@ static int (* const i915_query_funcs[])(struct
drm_i915_private *dev_priv,
query_memregion_info,
query_hwconfig_blob,
query_geometry_subslices,
+ query_guc_submission_version,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct
drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 550c496ce76d..d80d9b5e1eda 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -
Re: [RFC] drm/i915: Add GuC submission interface version query
On 2/7/2024 10:49, Tvrtko Ursulin wrote:
On 07/02/2024 18:12, John Harrison wrote:
On 2/7/2024 03:56, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Add a new query to the GuC submission interface version.
Mesa intends to use this information to check for old firmware versions
with a known bug where using the render and compute command streamers
simultaneously can cause GPU hangs due issues in firmware scheduling.
Based on patches from Vivaik and Joonas.
There is a little bit of an open around the width required for
versions.
While the GuC FW iface tells they are u8, i915 GuC code uses u32:
#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
...
struct intel_uc_fw_ver {
u32 major;
u32 minor;
u32 patch;
u32 build;
};
This is copied from generic code which supports firmwares other than
GuC. Only GuC promises to use 8-bit version components. Other
firmwares very definitely do not. There is no open.
Ack.
So we could make the query u8, and refactor the struct intel_uc_fw_ver
to use u8, or not. To avoid any doubts on why are we assigning u32 to
u8 I simply opted to use u64. Which avoids the need to add any padding
too.
I don't follow how potential 8 vs 32 confusion means jump to 64?!
Suggestion was to use u8 in the uapi in order to align with GuC FW ABI
(or however it's called), in which case there would be:
ver.major = guc->submission_version.major;
which would be:
(u8) = (u32)
And I was anticipating someone not liking that either. Using too wide
u64 simply avoids the need to add a padding element to the uapi struct.
If you are positive we need to include a branch number, even though it
does not seem to be implemented in the code even(*) then I can make
uapi 4x u32 and achieve the same.
It's not implemented in the code because we've never had to, and it is
yet another train wreck waiting to happen. There are a bunch of issues
at different levels that need to be resolved. But that is all in the
kernel and/or firmware and so can be added by a later kernel update when
necessary. However, if the UMDs are not already taking it into account
or its not even in the UAPI, then we can't back fill in the kernel
later, we are just broken.
(*)
static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32
css_value)
{
/* Get version numbers from the CSS header */
ver->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR, css_value);
ver->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css_value);
ver->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, css_value);
}
No branch field in the CSS header?
I think there is, it's just not officially implemented yet.
And Why is UMD supposed to reject a non-zero branch? Like how would
1.1.3.0 be fine and 1.1.3.1 be bad? I don't get it. But anyway, I can
respin if you definitely confirm.
Because that is backwards. The branch number goes at the front.
So, for example (using made up numbers, I don't recall offhand what
versions we have where) say we currently have 0.1.3.0 in tip and 0.1.1.0
in the last LTS. We then need to ship a critical security fix and back
port it to the LTS. Tip becomes 0.1.3.1 but the LTS can't become 0.1.1.1
because that version already exists in the history of tip and does not
contain the fix. So the LTS gets branched to 1.1.0.0. We then have both
branches potentially moving forwards with completely independent versioning.
Exactly the same as 5.8.x, 5.9,y, 6.0.z, etc in the Linux kernel
versioning. You cannot make any assumptions about what might be in
1.4.5.6 compared to 0.1.2.3. 1.4.5.6 could actually 0.1.0.3 with a stack
of security fixes but none of the features, workarounds or bug fixes
that are in 0.1.2.3.
Hence, if the branch number changes then all bets are off. You have to
start over and reject anything you do not explicitly know about.
This is why we were saying that exposing version numbers to UMDs breaks
down horribly as soon as we have to start branching. There is no clean
or simple way to do this.
John.
Regards,
Tvrtko
Compile tested only.
Signed-off-by: Tvrtko Ursulin
Cc: Kenneth Graunke
Cc: Jose Souza
Cc: Sagar Ghuge
Cc: Paulo Zanoni
Cc: John Harrison
Cc: Rodrigo Vivi
Cc: Jani Nikula
Cc: Tvrtko Ursulin
Cc: Vivaik Balasubrawmanian
---
drivers/gpu/drm/i915/i915_query.c | 32
+++
include/uapi/drm/i915_drm.h | 11 +++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..999687f6a3d4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -551,6 +551,37 @@ static int query_hwconfig_blob(struct
drm_i915_private *i915,
return hwconfig->size;
}
+static int
+query_guc_submission_version(struct drm_i915_private *i915,
+
Re: [RFC] drm/i915: Add GuC submission interface version query
On Wed, 2024-02-07 at 11:34 -0800, John Harrison wrote:
> On 2/7/2024 10:49, Tvrtko Ursulin wrote:
> > On 07/02/2024 18:12, John Harrison wrote:
> > > On 2/7/2024 03:56, Tvrtko Ursulin wrote:
> > > > From: Tvrtko Ursulin
> > > >
> > > > Add a new query to the GuC submission interface version.
> > > >
> > > > Mesa intends to use this information to check for old firmware versions
> > > > with a known bug where using the render and compute command streamers
> > > > simultaneously can cause GPU hangs due issues in firmware scheduling.
> > > >
> > > > Based on patches from Vivaik and Joonas.
> > > >
> > > > There is a little bit of an open around the width required for
> > > > versions.
> > > > While the GuC FW iface tells they are u8, i915 GuC code uses u32:
> > > >
> > > > #define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
> > > > #define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
> > > > #define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
> > > > ...
> > > > struct intel_uc_fw_ver {
> > > > u32 major;
> > > > u32 minor;
> > > > u32 patch;
> > > > u32 build;
> > > > };
> > > This is copied from generic code which supports firmwares other than
> > > GuC. Only GuC promises to use 8-bit version components. Other
> > > firmwares very definitely do not. There is no open.
> >
> > Ack.
> >
> > > >
> > > > So we could make the query u8, and refactor the struct intel_uc_fw_ver
> > > > to use u8, or not. To avoid any doubts on why are we assigning u32 to
> > > > u8 I simply opted to use u64. Which avoids the need to add any padding
> > > > too.
> > > I don't follow how potential 8 vs 32 confusion means jump to 64?!
> >
> > Suggestion was to use u8 in the uapi in order to align with GuC FW ABI
> > (or however it's called), in which case there would be:
> >
> > ver.major = guc->submission_version.major;
> >
> > which would be:
> >
> > (u8) = (u32)
> >
> > And I was anticipating someone not liking that either. Using too wide
> > u64 simply avoids the need to add a padding element to the uapi struct.
> >
> > If you are positive we need to include a branch number, even though it
> > does not seem to be implemented in the code even(*) then I can make
> > uapi 4x u32 and achieve the same.
> It's not implemented in the code because we've never had to, and it is
> yet another train wreck waiting to happen. There are a bunch of issues
> at different levels that need to be resolved. But that is all in the
> kernel and/or firmware and so can be added by a later kernel update when
> necessary. However, if the UMDs are not already taking it into account
> or its not even in the UAPI, then we can't back fill in the kernel
> later, we are just broken.
This sounds to me like a firmware version for internal testing or for
pre-production HW, would any branched firmware be released to customers?
>
> >
> > (*)
> > static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32
> > css_value)
> > {
> > /* Get version numbers from the CSS header */
> > ver->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR, css_value);
> > ver->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css_value);
> > ver->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, css_value);
> > }
> >
> > No branch field in the CSS header?
> I think there is, it's just not officially implemented yet.
>
> >
> > And Why is UMD supposed to reject a non-zero branch? Like how would
> > 1.1.3.0 be fine and 1.1.3.1 be bad? I don't get it. But anyway, I can
> > respin if you definitely confirm.
> Because that is backwards. The branch number goes at the front.
>
> So, for example (using made up numbers, I don't recall offhand what
> versions we have where) say we currently have 0.1.3.0 in tip and 0.1.1.0
> in the last LTS. We then need to ship a critical security fix and back
> port it to the LTS. Tip becomes 0.1.3.1 but the LTS can't become 0.1.1.1
> because that version already exists in the history of tip and does not
> contain the fix. So the LTS gets branched to 1.1.0.0. We then have both
> branches potentially moving forwards with completely independent versioning.
>
> Exactly the same as 5.8.x, 5.9,y, 6.0.z, etc in the Linux kernel
> versioning. You cannot make any assumptions about what might be in
> 1.4.5.6 compared to 0.1.2.3. 1.4.5.6 could actually 0.1.0.3 with a stack
> of security fixes but none of the features, workarounds or bug fixes
> that are in 0.1.2.3.
>
> Hence, if the branch number changes then all bets are off. You have to
> start over and reject anything you do not explicitly know about.
>
> This is why we were saying that exposing version numbers to UMDs breaks
> down horribly as soon as we have to start branching. There is no clean
> or simple way to do this.
>
> John.
>
>
> >
> > Regards,
> >
> > Tvrtko
> >
> > > >
> > > > Compile tested only.
> > > >
> > > > Signed-off-by: Tvrtko Ursulin
> > > > Cc: Kenneth Graunke
>
Re: [RFC] drm/i915: Add GuC submission interface version query
On 2/7/2024 11:43, Souza, Jose wrote:
On Wed, 2024-02-07 at 11:34 -0800, John Harrison wrote:
On 2/7/2024 10:49, Tvrtko Ursulin wrote:
On 07/02/2024 18:12, John Harrison wrote:
On 2/7/2024 03:56, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Add a new query to the GuC submission interface version.
Mesa intends to use this information to check for old firmware versions
with a known bug where using the render and compute command streamers
simultaneously can cause GPU hangs due issues in firmware scheduling.
Based on patches from Vivaik and Joonas.
There is a little bit of an open around the width required for
versions.
While the GuC FW iface tells they are u8, i915 GuC code uses u32:
#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
...
struct intel_uc_fw_ver {
u32 major;
u32 minor;
u32 patch;
u32 build;
};
This is copied from generic code which supports firmwares other than
GuC. Only GuC promises to use 8-bit version components. Other
firmwares very definitely do not. There is no open.
Ack.
So we could make the query u8, and refactor the struct intel_uc_fw_ver
to use u8, or not. To avoid any doubts on why are we assigning u32 to
u8 I simply opted to use u64. Which avoids the need to add any padding
too.
I don't follow how potential 8 vs 32 confusion means jump to 64?!
Suggestion was to use u8 in the uapi in order to align with GuC FW ABI
(or however it's called), in which case there would be:
ver.major = guc->submission_version.major;
which would be:
(u8) = (u32)
And I was anticipating someone not liking that either. Using too wide
u64 simply avoids the need to add a padding element to the uapi struct.
If you are positive we need to include a branch number, even though it
does not seem to be implemented in the code even(*) then I can make
uapi 4x u32 and achieve the same.
It's not implemented in the code because we've never had to, and it is
yet another train wreck waiting to happen. There are a bunch of issues
at different levels that need to be resolved. But that is all in the
kernel and/or firmware and so can be added by a later kernel update when
necessary. However, if the UMDs are not already taking it into account
or its not even in the UAPI, then we can't back fill in the kernel
later, we are just broken.
This sounds to me like a firmware version for internal testing or for
pre-production HW, would any branched firmware be released to customers?
See comments below. Branching is about back porting critical fixes to
older releases. I.e. supporting LTS releases. There is absolutely
nothing internal only or testing related about branching.
Just because we haven't had to do so yet doesn't mean we won't need to
do so tomorrow.
John.
(*)
static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32
css_value)
{
/* Get version numbers from the CSS header */
ver->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR, css_value);
ver->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css_value);
ver->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, css_value);
}
No branch field in the CSS header?
I think there is, it's just not officially implemented yet.
And Why is UMD supposed to reject a non-zero branch? Like how would
1.1.3.0 be fine and 1.1.3.1 be bad? I don't get it. But anyway, I can
respin if you definitely confirm.
Because that is backwards. The branch number goes at the front.
So, for example (using made up numbers, I don't recall offhand what
versions we have where) say we currently have 0.1.3.0 in tip and 0.1.1.0
in the last LTS. We then need to ship a critical security fix and back
port it to the LTS. Tip becomes 0.1.3.1 but the LTS can't become 0.1.1.1
because that version already exists in the history of tip and does not
contain the fix. So the LTS gets branched to 1.1.0.0. We then have both
branches potentially moving forwards with completely independent versioning.
Exactly the same as 5.8.x, 5.9,y, 6.0.z, etc in the Linux kernel
versioning. You cannot make any assumptions about what might be in
1.4.5.6 compared to 0.1.2.3. 1.4.5.6 could actually 0.1.0.3 with a stack
of security fixes but none of the features, workarounds or bug fixes
that are in 0.1.2.3.
Hence, if the branch number changes then all bets are off. You have to
start over and reject anything you do not explicitly know about.
This is why we were saying that exposing version numbers to UMDs breaks
down horribly as soon as we have to start branching. There is no clean
or simple way to do this.
John.
Regards,
Tvrtko
Compile tested only.
Signed-off-by: Tvrtko Ursulin
Cc: Kenneth Graunke
Cc: Jose Souza
Cc: Sagar Ghuge
Cc: Paulo Zanoni
Cc: John Harrison
Cc: Rodrigo Vivi
Cc: Jani Nikula
Cc: Tvrtko Ursulin
Cc: Vivaik Balasubrawmanian
---
drivers/gpu/drm/i915/i915_query.c | 32
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Tue, Jan 23, 2024 at 12:28:33PM +0200, Imre Deak wrote:
> +static char yes_no_chr(int val)
> +{
> + return val ? 'Y' : 'N';
> +}
We have str_yes_no() already.
v> +
> +#define SKIP_DPRX_CAPS_CHECK BIT(0)
> +#define ALLOW_ALLOCATED_BW_CHANGEBIT(1)
> +
> +static bool tunnel_regs_are_valid(struct drm_dp_tunnel_mgr *mgr,
> + const struct drm_dp_tunnel_regs *regs,
> + unsigned int flags)
> +{
> + int drv_group_id = tunnel_reg_drv_group_id(regs);
> + bool check_dprx = !(flags & SKIP_DPRX_CAPS_CHECK);
> + bool ret = true;
> +
> + if (!tunnel_reg_bw_alloc_supported(regs)) {
> + if (tunnel_group_id(drv_group_id)) {
> + drm_dbg_kms(mgr->dev,
> + "DPTUN: A non-zero group ID is only allowed
> with BWA support\n");
> + ret = false;
> + }
> +
> + if (tunnel_reg(regs, DP_ALLOCATED_BW)) {
> + drm_dbg_kms(mgr->dev,
> + "DPTUN: BW is allocated without BWA
> support\n");
> + ret = false;
> + }
> +
> + return ret;
> + }
> +
> + if (!tunnel_group_id(drv_group_id)) {
> + drm_dbg_kms(mgr->dev,
> + "DPTUN: BWA support requires a non-zero group
> ID\n");
> + ret = false;
> + }
> +
> + if (check_dprx && hweight8(tunnel_reg_max_dprx_lane_count(regs)) != 1) {
> + drm_dbg_kms(mgr->dev,
> + "DPTUN: Invalid DPRX lane count: %d\n",
> + tunnel_reg_max_dprx_lane_count(regs));
> +
> + ret = false;
> + }
> +
> + if (check_dprx && !tunnel_reg_max_dprx_rate(regs)) {
> + drm_dbg_kms(mgr->dev,
> + "DPTUN: DPRX rate is 0\n");
> +
> + ret = false;
> + }
> +
> + if (tunnel_reg(regs, DP_ALLOCATED_BW) > tunnel_reg(regs,
> DP_ESTIMATED_BW)) {
> + drm_dbg_kms(mgr->dev,
> + "DPTUN: Allocated BW %d > estimated BW %d Mb/s\n",
> + DPTUN_BW_ARG(tunnel_reg(regs, DP_ALLOCATED_BW) *
> + tunnel_reg_bw_granularity(regs)),
> + DPTUN_BW_ARG(tunnel_reg(regs, DP_ESTIMATED_BW) *
> + tunnel_reg_bw_granularity(regs)));
> +
> + ret = false;
> + }
> +
> + return ret;
> +}
> +
> +static bool tunnel_info_changes_are_valid(struct drm_dp_tunnel *tunnel,
> + const struct drm_dp_tunnel_regs *regs,
> + unsigned int flags)
> +{
> + int new_drv_group_id = tunnel_reg_drv_group_id(regs);
> + bool ret = true;
> +
> + if (tunnel->bw_alloc_supported != tunnel_reg_bw_alloc_supported(regs)) {
> + tun_dbg(tunnel,
> + "BW alloc support has changed %c -> %c\n",
> + yes_no_chr(tunnel->bw_alloc_supported),
> + yes_no_chr(tunnel_reg_bw_alloc_supported(regs)));
> +
> + ret = false;
> + }
> +
> + if (tunnel->group->drv_group_id != new_drv_group_id) {
> + tun_dbg(tunnel,
> + "Driver/group ID has changed %d:%d:* -> %d:%d:*\n",
> + tunnel_group_drv_id(tunnel->group->drv_group_id),
> + tunnel_group_id(tunnel->group->drv_group_id),
> + tunnel_group_drv_id(new_drv_group_id),
> + tunnel_group_id(new_drv_group_id));
> +
> + ret = false;
> + }
> +
> + if (!tunnel->bw_alloc_supported)
> + return ret;
> +
> + if (tunnel->bw_granularity != tunnel_reg_bw_granularity(regs)) {
> + tun_dbg(tunnel,
> + "BW granularity has changed: %d -> %d Mb/s\n",
> + DPTUN_BW_ARG(tunnel->bw_granularity),
> + DPTUN_BW_ARG(tunnel_reg_bw_granularity(regs)));
> +
> + ret = false;
> + }
> +
> + /*
> + * On some devices at least the BW alloc mode enabled status is always
> + * reported as 0, so skip checking that here.
> + */
So it's reported as supported and we enable it, but it's never
reported back as being enabled?
> +
> + if (!(flags & ALLOW_ALLOCATED_BW_CHANGE) &&
> + tunnel->allocated_bw !=
> + tunnel_reg(regs, DP_ALLOCATED_BW) * tunnel->bw_granularity) {
> + tun_dbg(tunnel,
> + "Allocated BW has changed: %d -> %d Mb/s\n",
> + DPTUN_BW_ARG(tunnel->allocated_bw),
> + DPTUN_BW_ARG(tunnel_reg(regs, DP_ALLOCATED_BW) *
> tunnel->bw_granularity));
> +
> + ret = false;
> + }
> +
> + return ret;
> +}
> +
> +static int
> +read_and_verify_tunnel_regs(struct drm_dp_tunnel *tunnel,
> + struct drm_dp_tunne
✗ Fi.CI.CHECKPATCH: warning for dma-buf: try to catch swiotlb bounce buffers
== Series Details == Series: dma-buf: try to catch swiotlb bounce buffers URL : https://patchwork.freedesktop.org/series/129638/ State : warning == Summary == Error: dim checkpatch failed 04a7282ab816 dma-buf: try to catch swiotlb bounce buffers -:80: WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Daniel Vetter ' != 'Signed-off-by: Daniel Vetter ' total: 0 errors, 1 warnings, 0 checks, 40 lines checked
✓ Fi.CI.BAT: success for dma-buf: try to catch swiotlb bounce buffers
== Series Details == Series: dma-buf: try to catch swiotlb bounce buffers URL : https://patchwork.freedesktop.org/series/129638/ State : success == Summary == CI Bug Log - changes from CI_DRM_14240 -> Patchwork_129638v1 Summary --- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/index.html Participating hosts (36 -> 37) -- Additional (2): bat-mtlp-8 bat-arls-2 Missing(1): fi-snb-2520m Known issues Here are the changes found in Patchwork_129638v1 that come from known issues: ### CI changes ### Issues hit * boot: - fi-bsw-n3050: [PASS][1] -> [FAIL][2] ([i915#8293]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14240/fi-bsw-n3050/boot.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/fi-bsw-n3050/boot.html - fi-apl-guc: [PASS][3] -> [FAIL][4] ([i915#8293]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14240/fi-apl-guc/boot.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/fi-apl-guc/boot.html - fi-cfl-8109u: [PASS][5] -> [FAIL][6] ([i915#8293]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14240/fi-cfl-8109u/boot.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/fi-cfl-8109u/boot.html ### IGT changes ### Issues hit * igt@debugfs_test@basic-hwmon: - bat-mtlp-8: NOTRUN -> [SKIP][7] ([i915#9318]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@debugfs_t...@basic-hwmon.html * igt@gem_lmem_swapping@verify-random: - bat-mtlp-8: NOTRUN -> [SKIP][8] ([i915#4613]) +3 other tests skip [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@gem_lmem_swapp...@verify-random.html * igt@gem_mmap@basic: - bat-mtlp-8: NOTRUN -> [SKIP][9] ([i915#4083]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@gem_m...@basic.html * igt@gem_mmap_gtt@basic: - bat-mtlp-8: NOTRUN -> [SKIP][10] ([i915#4077]) +2 other tests skip [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@gem_mmap_...@basic.html * igt@gem_render_tiled_blits@basic: - bat-mtlp-8: NOTRUN -> [SKIP][11] ([i915#4079]) +1 other test skip [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@gem_render_tiled_bl...@basic.html * igt@i915_pm_rps@basic-api: - bat-mtlp-8: NOTRUN -> [SKIP][12] ([i915#6621]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@i915_pm_...@basic-api.html * igt@i915_selftest@live@workarounds: - bat-dg2-8: [PASS][13] -> [DMESG-FAIL][14] ([i915#9500]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14240/bat-dg2-8/igt@i915_selftest@l...@workarounds.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-dg2-8/igt@i915_selftest@l...@workarounds.html * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy: - bat-mtlp-8: NOTRUN -> [SKIP][15] ([i915#5190]) [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_addfb_ba...@addfb25-y-tiled-small-legacy.html * igt@kms_addfb_basic@basic-y-tiled-legacy: - bat-mtlp-8: NOTRUN -> [SKIP][16] ([i915#4212]) +8 other tests skip [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_addfb_ba...@basic-y-tiled-legacy.html * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy: - bat-mtlp-8: NOTRUN -> [SKIP][17] ([i915#4213]) +1 other test skip [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-legacy.html * igt@kms_dsc@dsc-basic: - bat-mtlp-8: NOTRUN -> [SKIP][18] ([i915#3555] / [i915#3840] / [i915#9159]) [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_...@dsc-basic.html * igt@kms_force_connector_basic@force-load-detect: - bat-mtlp-8: NOTRUN -> [SKIP][19] ([fdo#109285]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_force_connector_ba...@force-load-detect.html * igt@kms_force_connector_basic@prune-stale-modes: - bat-mtlp-8: NOTRUN -> [SKIP][20] ([i915#5274]) [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_force_connector_ba...@prune-stale-modes.html * igt@kms_setmode@basic-clone-single-crtc: - bat-mtlp-8: NOTRUN -> [SKIP][21] ([i915#3555] / [i915#8809]) [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_129638v1/bat-mtlp-8/igt@kms_setm...@basic-clone-single-crtc.html * igt@prime_vgem@basic-fence-mmap: - bat-mtlp-8: NOTRUN -> [SKIP][22] ([i915#37
Re: [RFC] drm/i915: Add GuC submission interface version query
On Wed, 2024-02-07 at 11:52 -0800, John Harrison wrote:
> On 2/7/2024 11:43, Souza, Jose wrote:
> > On Wed, 2024-02-07 at 11:34 -0800, John Harrison wrote:
> > > On 2/7/2024 10:49, Tvrtko Ursulin wrote:
> > > > On 07/02/2024 18:12, John Harrison wrote:
> > > > > On 2/7/2024 03:56, Tvrtko Ursulin wrote:
> > > > > > From: Tvrtko Ursulin
> > > > > >
> > > > > > Add a new query to the GuC submission interface version.
> > > > > >
> > > > > > Mesa intends to use this information to check for old firmware
> > > > > > versions
> > > > > > with a known bug where using the render and compute command
> > > > > > streamers
> > > > > > simultaneously can cause GPU hangs due issues in firmware
> > > > > > scheduling.
> > > > > >
> > > > > > Based on patches from Vivaik and Joonas.
> > > > > >
> > > > > > There is a little bit of an open around the width required for
> > > > > > versions.
> > > > > > While the GuC FW iface tells they are u8, i915 GuC code uses u32:
> > > > > >
> > > > > > #define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
> > > > > > #define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
> > > > > > #define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
> > > > > > ...
> > > > > > struct intel_uc_fw_ver {
> > > > > > u32 major;
> > > > > > u32 minor;
> > > > > > u32 patch;
> > > > > > u32 build;
> > > > > > };
> > > > > This is copied from generic code which supports firmwares other than
> > > > > GuC. Only GuC promises to use 8-bit version components. Other
> > > > > firmwares very definitely do not. There is no open.
> > > > Ack.
> > > >
> > > > > > So we could make the query u8, and refactor the struct
> > > > > > intel_uc_fw_ver
> > > > > > to use u8, or not. To avoid any doubts on why are we assigning u32
> > > > > > to
> > > > > > u8 I simply opted to use u64. Which avoids the need to add any
> > > > > > padding
> > > > > > too.
> > > > > I don't follow how potential 8 vs 32 confusion means jump to 64?!
> > > > Suggestion was to use u8 in the uapi in order to align with GuC FW ABI
> > > > (or however it's called), in which case there would be:
> > > >
> > > > ver.major = guc->submission_version.major;
> > > >
> > > > which would be:
> > > >
> > > > (u8) = (u32)
> > > >
> > > > And I was anticipating someone not liking that either. Using too wide
> > > > u64 simply avoids the need to add a padding element to the uapi struct.
> > > >
> > > > If you are positive we need to include a branch number, even though it
> > > > does not seem to be implemented in the code even(*) then I can make
> > > > uapi 4x u32 and achieve the same.
> > > It's not implemented in the code because we've never had to, and it is
> > > yet another train wreck waiting to happen. There are a bunch of issues
> > > at different levels that need to be resolved. But that is all in the
> > > kernel and/or firmware and so can be added by a later kernel update when
> > > necessary. However, if the UMDs are not already taking it into account
> > > or its not even in the UAPI, then we can't back fill in the kernel
> > > later, we are just broken.
> > This sounds to me like a firmware version for internal testing or for
> > pre-production HW, would any branched firmware be released to customers?
> See comments below. Branching is about back porting critical fixes to
> older releases. I.e. supporting LTS releases. There is absolutely
> nothing internal only or testing related about branching.
>
> Just because we haven't had to do so yet doesn't mean we won't need to
> do so tomorrow.
>
> John.
>
> >
> > > > (*)
> > > > static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32
> > > > css_value)
> > > > {
> > > > /* Get version numbers from the CSS header */
> > > > ver->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR, css_value);
> > > > ver->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css_value);
> > > > ver->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, css_value);
> > > > }
> > > >
> > > > No branch field in the CSS header?
> > > I think there is, it's just not officially implemented yet.
> > >
> > > > And Why is UMD supposed to reject a non-zero branch? Like how would
> > > > 1.1.3.0 be fine and 1.1.3.1 be bad? I don't get it. But anyway, I can
> > > > respin if you definitely confirm.
> > > Because that is backwards. The branch number goes at the front.
> > >
> > > So, for example (using made up numbers, I don't recall offhand what
> > > versions we have where) say we currently have 0.1.3.0 in tip and 0.1.1.0
> > > in the last LTS. We then need to ship a critical security fix and back
> > > port it to the LTS. Tip becomes 0.1.3.1 but the LTS can't become 0.1.1.1
> > > because that version already exists in the history of tip and does not
> > > contain the fix. So the LTS gets branched to 1.1.0.0. We then have both
> > > branches potentially moving forwards with completely independent
> > > versioning.
> > >
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Wed, Feb 07, 2024 at 10:02:18PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2024 at 12:28:33PM +0200, Imre Deak wrote:
> > +static char yes_no_chr(int val)
> > +{
> > + return val ? 'Y' : 'N';
> > +}
>
> We have str_yes_no() already.
Ok, will use this.
> v> +
> > +#define SKIP_DPRX_CAPS_CHECK BIT(0)
> > +#define ALLOW_ALLOCATED_BW_CHANGE BIT(1)
> > +
> > +static bool tunnel_regs_are_valid(struct drm_dp_tunnel_mgr *mgr,
> > + const struct drm_dp_tunnel_regs *regs,
> > + unsigned int flags)
> > +{
> > + int drv_group_id = tunnel_reg_drv_group_id(regs);
> > + bool check_dprx = !(flags & SKIP_DPRX_CAPS_CHECK);
> > + bool ret = true;
> > +
> > + if (!tunnel_reg_bw_alloc_supported(regs)) {
> > + if (tunnel_group_id(drv_group_id)) {
> > + drm_dbg_kms(mgr->dev,
> > + "DPTUN: A non-zero group ID is only allowed
> > with BWA support\n");
> > + ret = false;
> > + }
> > +
> > + if (tunnel_reg(regs, DP_ALLOCATED_BW)) {
> > + drm_dbg_kms(mgr->dev,
> > + "DPTUN: BW is allocated without BWA
> > support\n");
> > + ret = false;
> > + }
> > +
> > + return ret;
> > + }
> > +
> > + if (!tunnel_group_id(drv_group_id)) {
> > + drm_dbg_kms(mgr->dev,
> > + "DPTUN: BWA support requires a non-zero group
> > ID\n");
> > + ret = false;
> > + }
> > +
> > + if (check_dprx && hweight8(tunnel_reg_max_dprx_lane_count(regs)) != 1) {
> > + drm_dbg_kms(mgr->dev,
> > + "DPTUN: Invalid DPRX lane count: %d\n",
> > + tunnel_reg_max_dprx_lane_count(regs));
> > +
> > + ret = false;
> > + }
> > +
> > + if (check_dprx && !tunnel_reg_max_dprx_rate(regs)) {
> > + drm_dbg_kms(mgr->dev,
> > + "DPTUN: DPRX rate is 0\n");
> > +
> > + ret = false;
> > + }
> > +
> > + if (tunnel_reg(regs, DP_ALLOCATED_BW) > tunnel_reg(regs,
> > DP_ESTIMATED_BW)) {
> > + drm_dbg_kms(mgr->dev,
> > + "DPTUN: Allocated BW %d > estimated BW %d Mb/s\n",
> > + DPTUN_BW_ARG(tunnel_reg(regs, DP_ALLOCATED_BW) *
> > +tunnel_reg_bw_granularity(regs)),
> > + DPTUN_BW_ARG(tunnel_reg(regs, DP_ESTIMATED_BW) *
> > +tunnel_reg_bw_granularity(regs)));
> > +
> > + ret = false;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static bool tunnel_info_changes_are_valid(struct drm_dp_tunnel *tunnel,
> > + const struct drm_dp_tunnel_regs *regs,
> > + unsigned int flags)
> > +{
> > + int new_drv_group_id = tunnel_reg_drv_group_id(regs);
> > + bool ret = true;
> > +
> > + if (tunnel->bw_alloc_supported != tunnel_reg_bw_alloc_supported(regs)) {
> > + tun_dbg(tunnel,
> > + "BW alloc support has changed %c -> %c\n",
> > + yes_no_chr(tunnel->bw_alloc_supported),
> > + yes_no_chr(tunnel_reg_bw_alloc_supported(regs)));
> > +
> > + ret = false;
> > + }
> > +
> > + if (tunnel->group->drv_group_id != new_drv_group_id) {
> > + tun_dbg(tunnel,
> > + "Driver/group ID has changed %d:%d:* -> %d:%d:*\n",
> > + tunnel_group_drv_id(tunnel->group->drv_group_id),
> > + tunnel_group_id(tunnel->group->drv_group_id),
> > + tunnel_group_drv_id(new_drv_group_id),
> > + tunnel_group_id(new_drv_group_id));
> > +
> > + ret = false;
> > + }
> > +
> > + if (!tunnel->bw_alloc_supported)
> > + return ret;
> > +
> > + if (tunnel->bw_granularity != tunnel_reg_bw_granularity(regs)) {
> > + tun_dbg(tunnel,
> > + "BW granularity has changed: %d -> %d Mb/s\n",
> > + DPTUN_BW_ARG(tunnel->bw_granularity),
> > + DPTUN_BW_ARG(tunnel_reg_bw_granularity(regs)));
> > +
> > + ret = false;
> > + }
> > +
> > + /*
> > +* On some devices at least the BW alloc mode enabled status is always
> > +* reported as 0, so skip checking that here.
> > +*/
>
> So it's reported as supported and we enable it, but it's never
> reported back as being enabled?
Yes, at least using an engineering TBT (DP adapter) FW. I'll check if
this is fixed already on released platforms/FWs.
> > +
> > + if (!(flags & ALLOW_ALLOCATED_BW_CHANGE) &&
> > + tunnel->allocated_bw !=
> > + tunnel_reg(regs, DP_ALLOCATED_BW) * tunnel->bw_granularity) {
> > + tun_dbg(tunnel,
> > + "Allocated BW has changed: %d -> %d Mb/s\n",
> > + DPTUN_BW_ARG(tunnel->allocated_bw),
> > +
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Wed, Feb 07, 2024 at 10:48:53PM +0200, Imre Deak wrote:
> On Wed, Feb 07, 2024 at 10:02:18PM +0200, Ville Syrjälä wrote:
> > > [...]
> > > +static int
> > > +drm_dp_tunnel_atomic_check_group_bw(struct drm_dp_tunnel_group_state
> > > *new_group_state,
> > > + u32 *failed_stream_mask)
> > > +{
> > > + struct drm_dp_tunnel_group *group = to_group(new_group_state->base.obj);
> > > + struct drm_dp_tunnel_state *new_tunnel_state;
> > > + u32 group_stream_mask = 0;
> > > + int group_bw = 0;
> > > +
> > > + for_each_tunnel_state(new_group_state, new_tunnel_state) {
> > > + struct drm_dp_tunnel *tunnel =
> > > new_tunnel_state->tunnel_ref.tunnel;
> > > + int max_dprx_bw = get_max_dprx_bw(tunnel);
> > > + int tunnel_bw =
> > > drm_dp_tunnel_atomic_get_tunnel_bw(new_tunnel_state);
> > > +
> > > + tun_dbg(tunnel,
> > > + "%sRequired %d/%d Mb/s total for tunnel.\n",
> > > + tunnel_bw > max_dprx_bw ? "Not enough BW: " : "",
> > > + DPTUN_BW_ARG(tunnel_bw),
> > > + DPTUN_BW_ARG(max_dprx_bw));
> > > +
> > > + if (tunnel_bw > max_dprx_bw) {
> >
> > I'm a bit confused why we're checking this here. Aren't we already
> > checking this somewhere else?
>
> Ah, yes this should be checked already by the encoder compute config +
> the MST link BW check. It can be removed, thanks.
Though neither of that is guaranteed for drivers in general, so
shouldn't it be here still?
> > > + *failed_stream_mask = new_tunnel_state->stream_mask;
> > > + return -ENOSPC;
> > > + }
> > > +
> > > + group_bw += min(roundup(tunnel_bw, tunnel->bw_granularity),
> > > + max_dprx_bw);
> > > + group_stream_mask |= new_tunnel_state->stream_mask;
> > > + }
> > > +
> > > + tun_grp_dbg(group,
> > > + "%sRequired %d/%d Mb/s total for tunnel group.\n",
> > > + group_bw > group->available_bw ? "Not enough BW: " : "",
> > > + DPTUN_BW_ARG(group_bw),
> > > + DPTUN_BW_ARG(group->available_bw));
> > > +
> > > + if (group_bw > group->available_bw) {
> > > + *failed_stream_mask = group_stream_mask;
> > > + return -ENOSPC;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
Re: [PATCH] kunit: device: Unregister the kunit_bus on shutdown
On Wed, Feb 7, 2024 at 8:36 AM Jani Nikula wrote:
>
> On Fri, 02 Feb 2024, Rae Moar wrote:
> > On Thu, Feb 1, 2024 at 1:06 AM David Gow wrote:
> >>
> >> If KUnit is built as a module, and it's unloaded, the kunit_bus is not
> >> unregistered. This causes an error if it's then re-loaded later, as we
> >> try to re-register the bus.
> >>
> >> Unregister the bus and root_device on shutdown, if it looks valid.
> >>
> >> In addition, be more specific about the value of kunit_bus_device. It
> >> is:
> >> - a valid struct device* if the kunit_bus initialised correctly.
> >> - an ERR_PTR if it failed to initialise.
> >> - NULL before initialisation and after shutdown.
> >>
> >> Fixes: d03c720e03bd ("kunit: Add APIs for managing devices")
> >> Signed-off-by: David Gow
> >
> > Hello,
> >
> > I have tested this with modules and it looks good to me!
> >
> > Thanks!
> > -Rae
> >
> > Reviewed-by: Rae Moar
>
> Thanks for the patch and review!
>
> Is this on its way to some v6.8-rc's? The regression in -rc1 is hurting
> our CI.
Hello!
This patch has been accepted on the kselftest/kunit-fixes branch
(https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit-fixes&id=829388b725f8d266ccec32a2f446717d8693eaba)
and is heading towards a future v6.8-rc.
Thanks!
-Rae
>
>
> Thanks,
> Jani.
>
>
> --
> Jani Nikula, Intel
Re: [RFC] drm/i915: Add GuC submission interface version query
On 2/7/2024 12:47, Souza, Jose wrote:
On Wed, 2024-02-07 at 11:52 -0800, John Harrison wrote:
On 2/7/2024 11:43, Souza, Jose wrote:
On Wed, 2024-02-07 at 11:34 -0800, John Harrison wrote:
On 2/7/2024 10:49, Tvrtko Ursulin wrote:
On 07/02/2024 18:12, John Harrison wrote:
On 2/7/2024 03:56, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Add a new query to the GuC submission interface version.
Mesa intends to use this information to check for old firmware versions
with a known bug where using the render and compute command streamers
simultaneously can cause GPU hangs due issues in firmware scheduling.
Based on patches from Vivaik and Joonas.
There is a little bit of an open around the width required for
versions.
While the GuC FW iface tells they are u8, i915 GuC code uses u32:
#define CSS_SW_VERSION_UC_MAJOR (0xFF << 16)
#define CSS_SW_VERSION_UC_MINOR (0xFF << 8)
#define CSS_SW_VERSION_UC_PATCH (0xFF << 0)
...
struct intel_uc_fw_ver {
u32 major;
u32 minor;
u32 patch;
u32 build;
};
This is copied from generic code which supports firmwares other than
GuC. Only GuC promises to use 8-bit version components. Other
firmwares very definitely do not. There is no open.
Ack.
So we could make the query u8, and refactor the struct intel_uc_fw_ver
to use u8, or not. To avoid any doubts on why are we assigning u32 to
u8 I simply opted to use u64. Which avoids the need to add any padding
too.
I don't follow how potential 8 vs 32 confusion means jump to 64?!
Suggestion was to use u8 in the uapi in order to align with GuC FW ABI
(or however it's called), in which case there would be:
ver.major = guc->submission_version.major;
which would be:
(u8) = (u32)
And I was anticipating someone not liking that either. Using too wide
u64 simply avoids the need to add a padding element to the uapi struct.
If you are positive we need to include a branch number, even though it
does not seem to be implemented in the code even(*) then I can make
uapi 4x u32 and achieve the same.
It's not implemented in the code because we've never had to, and it is
yet another train wreck waiting to happen. There are a bunch of issues
at different levels that need to be resolved. But that is all in the
kernel and/or firmware and so can be added by a later kernel update when
necessary. However, if the UMDs are not already taking it into account
or its not even in the UAPI, then we can't back fill in the kernel
later, we are just broken.
This sounds to me like a firmware version for internal testing or for
pre-production HW, would any branched firmware be released to customers?
See comments below. Branching is about back porting critical fixes to
older releases. I.e. supporting LTS releases. There is absolutely
nothing internal only or testing related about branching.
Just because we haven't had to do so yet doesn't mean we won't need to
do so tomorrow.
John.
(*)
static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32
css_value)
{
/* Get version numbers from the CSS header */
ver->major = FIELD_GET(CSS_SW_VERSION_UC_MAJOR, css_value);
ver->minor = FIELD_GET(CSS_SW_VERSION_UC_MINOR, css_value);
ver->patch = FIELD_GET(CSS_SW_VERSION_UC_PATCH, css_value);
}
No branch field in the CSS header?
I think there is, it's just not officially implemented yet.
And Why is UMD supposed to reject a non-zero branch? Like how would
1.1.3.0 be fine and 1.1.3.1 be bad? I don't get it. But anyway, I can
respin if you definitely confirm.
Because that is backwards. The branch number goes at the front.
So, for example (using made up numbers, I don't recall offhand what
versions we have where) say we currently have 0.1.3.0 in tip and 0.1.1.0
in the last LTS. We then need to ship a critical security fix and back
port it to the LTS. Tip becomes 0.1.3.1 but the LTS can't become 0.1.1.1
because that version already exists in the history of tip and does not
contain the fix. So the LTS gets branched to 1.1.0.0. We then have both
branches potentially moving forwards with completely independent versioning.
Exactly the same as 5.8.x, 5.9,y, 6.0.z, etc in the Linux kernel
versioning. You cannot make any assumptions about what might be in
1.4.5.6 compared to 0.1.2.3. 1.4.5.6 could actually 0.1.0.3 with a stack
of security fixes but none of the features, workarounds or bug fixes
that are in 0.1.2.3.
Hence, if the branch number changes then all bets are off. You have to
start over and reject anything you do not explicitly know about.
This is why we were saying that exposing version numbers to UMDs breaks
down horribly as soon as we have to start branching. There is no clean
or simple way to do this.
Odd versioning, would expect that fixes on LTS would increase patch version.
You can't. That would create multiple firmware entities with the same
version number.
E.g. everything is on 0.1.2.3. Tip moves on
Re: [PATCH 02/19] drm/dp: Add support for DP tunneling
On Wed, Feb 07, 2024 at 10:48:43PM +0200, Imre Deak wrote:
> On Wed, Feb 07, 2024 at 10:02:18PM +0200, Ville Syrjälä wrote:
> > On Tue, Jan 23, 2024 at 12:28:33PM +0200, Imre Deak wrote:
> > > + [...]
> > > +static int group_allocated_bw(struct drm_dp_tunnel_group *group)
> > > +{
> > > + struct drm_dp_tunnel *tunnel;
> > > + int group_allocated_bw = 0;
> > > +
> > > + for_each_tunnel_in_group(group, tunnel) {
> > > + if (check_tunnel(tunnel) == 0 &&
> > > + tunnel->bw_alloc_enabled)
> > > + group_allocated_bw += tunnel->allocated_bw;
> > > + }
> > > +
> > > + return group_allocated_bw;
> > > +}
> > > +
> > > +static int calc_group_available_bw(const struct drm_dp_tunnel *tunnel)
> > > +{
> > > + return group_allocated_bw(tunnel->group) -
> > > +tunnel->allocated_bw +
> > > +tunnel->estimated_bw;
> >
> > Hmm. So the estimated_bw=actually_free_bw + tunnel->allocated_bw?
>
> Yes.
>
> > Ie. how much bw might be available for this tunnel right now?
>
> Correct.
>
> > And here we're trying to deduce the total bandwidth available by
> > adding in the allocated_bw of all the other tunnels in the group?
>
> Yes.
>
> > Rather weird that we can't just get that number directly...
>
> It is. Imo this could be simply communicated via a DPCD register
> dedicated for this. Perhaps adding this should be requested from TBT
> architects.
One reason for this design can be that a host/driver may not see all the
tunnels in the group. In that case the tunnel's current usable BW will
be only its estimated_bw (that is it can't use the BW already allocated
by other tunnels in the group, until those are released by the other
host/driver).
> I assume this could also use a code comment.
>
> > > +}
> > > +
> > > +static int update_group_available_bw(struct drm_dp_tunnel *tunnel,
> > > + const struct drm_dp_tunnel_regs *regs)
> > > +{
> > > + struct drm_dp_tunnel *tunnel_iter;
> > > + int group_available_bw;
> > > + bool changed;
> > > +
> > > + tunnel->estimated_bw = tunnel_reg(regs, DP_ESTIMATED_BW) *
> > > tunnel->bw_granularity;
> > > +
> > > + if (calc_group_available_bw(tunnel) == tunnel->group->available_bw)
> > > + return 0;
> > > +
> > > + for_each_tunnel_in_group(tunnel->group, tunnel_iter) {
> > > + int err;
> > > +
> > > + if (tunnel_iter == tunnel)
> > > + continue;
> > > +
> > > + if (check_tunnel(tunnel_iter) != 0 ||
> > > + !tunnel_iter->bw_alloc_enabled)
> > > + continue;
> > > +
> > > + err = drm_dp_dpcd_probe(tunnel_iter->aux, DP_DPCD_REV);
> > > + if (err) {
> > > + tun_dbg(tunnel_iter,
> > > + "Probe failed, assume disconnected (err %pe)\n",
> > > + ERR_PTR(err));
> > > + drm_dp_tunnel_set_io_error(tunnel_iter);
> > > + }
> > > + }
> > > +
> > > + group_available_bw = calc_group_available_bw(tunnel);
> > > +
> > > + tun_dbg(tunnel, "Updated group available BW: %d->%d\n",
> > > + DPTUN_BW_ARG(tunnel->group->available_bw),
> > > + DPTUN_BW_ARG(group_available_bw));
> > > +
> > > + changed = tunnel->group->available_bw != group_available_bw;
> > > +
> > > + tunnel->group->available_bw = group_available_bw;
> > > +
> > > + return changed ? 1 : 0;
> > > +}
> > > +
> > > +static int set_bw_alloc_mode(struct drm_dp_tunnel *tunnel, bool enable)
> > > +{
> > > + u8 mask = DP_DISPLAY_DRIVER_BW_ALLOCATION_MODE_ENABLE |
> > > DP_UNMASK_BW_ALLOCATION_IRQ;
> > > + u8 val;
> > > +
> > > + if (drm_dp_dpcd_readb(tunnel->aux, DP_DPTX_BW_ALLOCATION_MODE_CONTROL,
> > > &val) < 0)
> > > + goto out_err;
> > > +
> > > + if (enable)
> > > + val |= mask;
> > > + else
> > > + val &= ~mask;
> > > +
> > > + if (drm_dp_dpcd_writeb(tunnel->aux, DP_DPTX_BW_ALLOCATION_MODE_CONTROL,
> > > val) < 0)
> > > + goto out_err;
> > > +
> > > + tunnel->bw_alloc_enabled = enable;
> > > +
> > > + return 0;
> > > +
> > > +out_err:
> > > + drm_dp_tunnel_set_io_error(tunnel);
> > > +
> > > + return -EIO;
> > > +}
> > > +
> > > +/**
> > > + * drm_dp_tunnel_enable_bw_alloc: Enable DP tunnel BW allocation mode
> > > + * @tunnel: Tunnel object
> > > + *
> > > + * Enable the DP tunnel BW allocation mode on @tunnel if it supports it.
> > > + *
> > > + * Returns 0 in case of success, negative error code otherwise.
> > > + */
> > > +int drm_dp_tunnel_enable_bw_alloc(struct drm_dp_tunnel *tunnel)
> > > +{
> > > + struct drm_dp_tunnel_regs regs;
> > > + int err = check_tunnel(tunnel);
> > > +
> > > + if (err)
> > > + return err;
> > > +
> > > + if (!tunnel->bw_alloc_supported)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (!tunnel_group_id(tunnel->group->drv_group_id))
> > > + return -EINVAL;
> > > +
> > > + err = set_bw_alloc_mode(tunnel, true);
> > > + if (err)
> > > + goto out;
> > > +
> > > + err = read_and_verify_tu
Re: [PATCH 2/2] xfs: disable large folio support in xfile_create
On Thu, 11 Jan 2024 18:22:50 -0800 "Darrick J. Wong" wrote: > On Thu, Jan 11, 2024 at 10:45:53PM +, Matthew Wilcox wrote: > > On Thu, Jan 11, 2024 at 02:00:53PM -0800, Andrew Morton wrote: > > > On Wed, 10 Jan 2024 12:04:51 -0800 "Darrick J. Wong" > > > wrote: > > > > > > > > > Fixing this will require a bit of an API change, and prefeably > > > > > > sorting out > > > > > > the hwpoison story for pages vs folio and where it is placed in the > > > > > > shmem > > > > > > API. For now use this one liner to disable large folios. > > > > > > > > > > > > Reported-by: Darrick J. Wong > > > > > > Signed-off-by: Christoph Hellwig > > > > > > > > > > Can someone who knows more about shmem.c than I do please review > > > > > https://lore.kernel.org/linux-xfs/20240103084126.513354-4-...@lst.de/ > > > > > so that I can feel slightly more confident as hch and I sort through > > > > > the > > > > > xfile.c issues? > > > > > > > > > > For this patch, > > > > > Reviewed-by: Darrick J. Wong > > > > > > > > ...except that I'm still getting 2M THPs even with this enabled, so I > > > > guess either we get to fix it now, or create our own private tmpfs mount > > > > so that we can pass in huge=never, similar to what i915 does. :( > > > > > > What is "this"? Are you saying that $Subject doesn't work, or that the > > > above-linked please-review patch doesn't work? > > > > shmem pays no attention to the mapping_large_folio_support() flag, > > so the proposed fix doesn't work. It ought to, but it has its own way > > of doing it that predates mapping_large_folio_support existing. > > Yep. It turned out to be easier to fix xfile.c to deal with large > folios than I thought it would be. Or so I think. We'll see what > happens on fstestscloud overnight. Where do we stand with this? Should I merge these two patches into 6.8-rcX, cc:stable?
Re: [PATCH] drm/buddy: Fix alloc_range() error handling code
Am 07.02.24 um 18:44 schrieb Arunpravin Paneer Selvam:
Few users have observed display corruption when they boot
the machine to KDE Plasma or playing games. We have root
caused the problem that whenever alloc_range() couldn't
find the required memory blocks the function was returning
SUCCESS in some of the corner cases.
The right approach would be if the total allocated size
is less than the required size, the function should
return -ENOSPC.
Gitlab ticket link - https://gitlab.freedesktop.org/drm/amd/-/issues/3097
Fixes: 0a1844bf0b53 ("drm/buddy: Improve contiguous memory allocation")
Signed-off-by: Arunpravin Paneer Selvam
Tested-by: Mario Limonciello
Acked-by: Christian König
CC: stable.. ?
---
drivers/gpu/drm/drm_buddy.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index f57e6d74fb0e..c1a99bf4dffd 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -539,6 +539,12 @@ static int __alloc_range(struct drm_buddy *mm,
} while (1);
list_splice_tail(&allocated, blocks);
+
+ if (total_allocated < size) {
+ err = -ENOSPC;
+ goto err_free;
+ }
+
return 0;
err_undo:
[PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
Now that include/linux/bits.h implements fixed-width GENMASK_*, use them to implement the i915/xe specific macros. Converting each driver to use the generic macros are left for later, when/if other driver-specific macros are also generalized. Signed-off-by: Lucas De Marchi Acked-by: Jani Nikula --- drivers/gpu/drm/i915/i915_reg_defs.h | 108 +++ 1 file changed, 11 insertions(+), 97 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h index a685db1e815d..52f99eb96f86 100644 --- a/drivers/gpu/drm/i915/i915_reg_defs.h +++ b/drivers/gpu/drm/i915/i915_reg_defs.h @@ -9,76 +9,19 @@ #include #include -/** - * REG_BIT() - Prepare a u32 bit value - * @__n: 0-based bit number - * - * Local wrapper for BIT() to force u32, with compile time checks. - * - * @return: Value with bit @__n set. - */ -#define REG_BIT(__n) \ - ((u32)(BIT(__n) + \ - BUILD_BUG_ON_ZERO(__is_constexpr(__n) && \ -((__n) < 0 || (__n) > 31 - -/** - * REG_BIT8() - Prepare a u8 bit value - * @__n: 0-based bit number - * - * Local wrapper for BIT() to force u8, with compile time checks. - * - * @return: Value with bit @__n set. - */ -#define REG_BIT8(__n) \ - ((u8)(BIT(__n) +\ - BUILD_BUG_ON_ZERO(__is_constexpr(__n) && \ -((__n) < 0 || (__n) > 7 - -/** - * REG_GENMASK() - Prepare a continuous u32 bitmask - * @__high: 0-based high bit - * @__low: 0-based low bit - * - * Local wrapper for GENMASK() to force u32, with compile time checks. - * - * @return: Continuous bitmask from @__high to @__low, inclusive. - */ -#define REG_GENMASK(__high, __low) \ - ((u32)(GENMASK(__high, __low) + \ - BUILD_BUG_ON_ZERO(__is_constexpr(__high) && \ -__is_constexpr(__low) && \ -((__low) < 0 || (__high) > 31 || (__low) > (__high) - -/** - * REG_GENMASK64() - Prepare a continuous u64 bitmask - * @__high: 0-based high bit - * @__low: 0-based low bit - * - * Local wrapper for GENMASK_ULL() to force u64, with compile time checks. - * - * @return: Continuous bitmask from @__high to @__low, inclusive. +/* + * Wrappers over the generic BIT_* and GENMASK_* implementations, + * for compatibility reasons with previous implementation */ -#define REG_GENMASK64(__high, __low) \ - ((u64)(GENMASK_ULL(__high, __low) + \ - BUILD_BUG_ON_ZERO(__is_constexpr(__high) && \ -__is_constexpr(__low) && \ -((__low) < 0 || (__high) > 63 || (__low) > (__high) +#define REG_GENMASK(__high, __low) GENMASK_U32(__high, __low) +#define REG_GENMASK64(__high, __low) GENMASK_U64(__high, __low) +#define REG_GENMASK16(__high, __low) GENMASK_U16(__high, __low) +#define REG_GENMASK8(__high, __low)GENMASK_U8(__high, __low) -/** - * REG_GENMASK8() - Prepare a continuous u8 bitmask - * @__high: 0-based high bit - * @__low: 0-based low bit - * - * Local wrapper for GENMASK() to force u8, with compile time checks. - * - * @return: Continuous bitmask from @__high to @__low, inclusive. - */ -#define REG_GENMASK8(__high, __low) \ - ((u8)(GENMASK(__high, __low) + \ - BUILD_BUG_ON_ZERO(__is_constexpr(__high) && \ -__is_constexpr(__low) && \ -((__low) < 0 || (__high) > 7 || (__low) > (__high) +#define REG_BIT(__n) BIT_U32(__n) +#define REG_BIT64(__n) BIT_U64(__n) +#define REG_BIT16(__n) BIT_U16(__n) +#define REG_BIT8(__n) BIT_U8(__n) /* * Local integer constant expression version of is_power_of_2(). @@ -143,35 +86,6 @@ */ #define REG_FIELD_GET64(__mask, __val) ((u64)FIELD_GET(__mask, __val)) -/** - * REG_BIT16() - Prepare a u16 bit value - * @__n: 0-based bit number - * - * Local wrapper for BIT() to force u16, with compile time - * checks. - * - * @return: Value with bit @__n set. - */ -#define REG_BIT16(__n) \ - ((u16)(BIT(__n) +\ - BUILD_BUG_ON_ZERO(__is_constexpr(__n) && \ -((__n) < 0 || (__n) > 15 - -/** - * REG_GENMASK16() - Prepare a continuous u8 bitmask - * @__high: 0-based high bit - * @__low: 0-based low bit - * - * Local wrapper for GENMASK() to for
[PATCH v3 1/3] bits: introduce fixed-type genmasks
From: Yury Norov Generalize __GENMASK() to support different types, and implement fixed-types versions of GENMASK() based on it. The fixed-type version allows more strict checks to the min/max values accepted, which is useful for defining registers like implemented by i915 and xe drivers with their REG_GENMASK*() macros. The strict checks rely on shift-count-overflow compiler check to fail the build if a number outside of the range allowed is passed. Example: #define FOO_MASK GENMASK_U32(33, 4) will generate a warning like: ../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow] 41 | (((t)~0ULL - ((t)(1) << (l)) + 1) & \ | ^~ Signed-off-by: Yury Norov Signed-off-by: Lucas De Marchi Acked-by: Jani Nikula --- include/linux/bitops.h | 1 - include/linux/bits.h | 32 ++-- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/include/linux/bitops.h b/include/linux/bitops.h index 2ba557e067fe..1db50c69cfdb 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -15,7 +15,6 @@ # define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n))) #endif -#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE) #define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) #define BITS_TO_U64(nr)__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) #define BITS_TO_U32(nr)__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) diff --git a/include/linux/bits.h b/include/linux/bits.h index 7c0cf5031abe..bd56f32de44e 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -6,6 +6,8 @@ #include #include +#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE) + #define BIT_MASK(nr) (UL(1) << ((nr) % BITS_PER_LONG)) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) #define BIT_ULL_MASK(nr) (ULL(1) << ((nr) % BITS_PER_LONG_LONG)) @@ -30,16 +32,26 @@ #define GENMASK_INPUT_CHECK(h, l) 0 #endif -#define __GENMASK(h, l) \ - (((~UL(0)) - (UL(1) << (l)) + 1) & \ -(~UL(0) >> (BITS_PER_LONG - 1 - (h -#define GENMASK(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) +/* + * Generate a mask for the specified type @t. Additional checks are made to + * guarantee the value returned fits in that type, relying on + * shift-count-overflow compiler check to detect incompatible arguments. + * For example, all these create build errors or warnings: + * + * - GENMASK(15, 20): wrong argument order + * - GENMASK(72, 15): doesn't fit unsigned long + * - GENMASK_U32(33, 15): doesn't fit in a u32 + */ +#define __GENMASK(t, h, l) \ + (GENMASK_INPUT_CHECK(h, l) + \ +(((t)~0ULL - ((t)(1) << (l)) + 1) & \ +((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h) -#define __GENMASK_ULL(h, l) \ - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ -(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h -#define GENMASK_ULL(h, l) \ - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) +#define GENMASK(h, l) __GENMASK(unsigned long, h, l) +#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l) +#define GENMASK_U8(h, l) __GENMASK(u8, h, l) +#define GENMASK_U16(h, l) __GENMASK(u16, h, l) +#define GENMASK_U32(h, l) __GENMASK(u32, h, l) +#define GENMASK_U64(h, l) __GENMASK(u64, h, l) #endif /* __LINUX_BITS_H */ -- 2.43.0
[PATCH v3 0/3] Fixed-type GENMASK/BIT
ove the implementation of REG_GENMASK/REG_BIT to a more appropriate place to be shared by i915, xe and possibly other parts of the kernel. For now this re-defines the old macros. In future we may start using the new macros directly, but that's a more intrusive search-and-replace. Changes from v2: - Document both in commit message and code about the strict type checking and give examples how it´d break with invalid params. v1: https://lore.kernel.org/intel-xe/20230509051403.2748545-1-lucas.demar...@intel.com/ v2: https://lore.kernel.org/intel-xe/20240124050205.3646390-1-lucas.demar...@intel.com Lucas De Marchi (2): bits: Introduce fixed-type BIT drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_* Yury Norov (1): bits: introduce fixed-type genmasks drivers/gpu/drm/i915/i915_reg_defs.h | 108 +++ include/linux/bitops.h | 1 - include/linux/bits.h | 51 ++--- 3 files changed, 51 insertions(+), 109 deletions(-) -- 2.43.0
[PATCH v3 2/3] bits: Introduce fixed-type BIT
Implement fixed-type BIT() to help drivers add stricter checks, like was done for GENMASK. Signed-off-by: Lucas De Marchi Acked-by: Jani Nikula --- include/linux/bits.h | 17 + 1 file changed, 17 insertions(+) diff --git a/include/linux/bits.h b/include/linux/bits.h index bd56f32de44e..811846ce110e 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -24,12 +24,16 @@ #define GENMASK_INPUT_CHECK(h, l) \ (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ __is_constexpr((l) > (h)), (l) > (h), 0))) +#define BIT_INPUT_CHECK(type, b) \ + ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ + __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0 #else /* * BUILD_BUG_ON_ZERO is not available in h files included from asm files, * disable the input check if that is the case. */ #define GENMASK_INPUT_CHECK(h, l) 0 +#define BIT_INPUT_CHECK(type, b) 0 #endif /* @@ -54,4 +58,17 @@ #define GENMASK_U32(h, l) __GENMASK(u32, h, l) #define GENMASK_U64(h, l) __GENMASK(u64, h, l) +/* + * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The + * following examples generate compiler warnings due to shift-count-overflow: + * + * - BIT_U8(8) + * - BIT_U32(-1) + * - BIT_U32(40) + */ +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b))) +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b))) +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b))) +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b))) + #endif /* __LINUX_BITS_H */ -- 2.43.0
