[Intel-xe] [PATCH v2 1/2] drm/xe: add skip_pcode flag

[Koby Elbaz]

On 24/10/2023 13:40, Jani Nikula wrote:
> On Sun, 22 Oct 2023, Koby Elbaz <kelbaz at habana.ai> wrote:
>> Per device, set this flag to enable access to the PCODE uC or to skip it.
>>
>> Signed-off-by: Koby Elbaz <kelbaz at habana.ai>
>> ---
>> Changes in v2:
>> - Added the flag 'skip_pcode' instead of 'has_pcode'
> I don't know why change was made from positive to negative flag.
>
> Generally it's better to use positive flags, because they just read
> better. Negative flags often lead to double negatives in source, such as
> !disable.
>
> Please consider using a positive flag, even if it's not "has_something".
>
> BR,
> Jani.


I agree that it's generally better to use positive flags for the reasons you
mentioned above.
However, I had a discussion with Lucas about the naming convention the
other day, and it was agreed that although using the "has_" prefix
might align with other flags we have, we will still have to manually
enable this flag (has_pcode = true) for all existing devices, while
potentially forgetting to enable this flag for devices not yet supported
by Xe (but will be supported soon), and therefore regress upon rebase.
Having said that, I decided to add the skip_pcode flag, and respectively
rename the previously merged "bypass_mtcfg" to "skip_mtcfg".


Thanks,
Koby.


>>   drivers/gpu/drm/xe/xe_device_types.h | 2 ++
>>   drivers/gpu/drm/xe/xe_pci.c          | 2 ++
>>   drivers/gpu/drm/xe/xe_pcode.c        | 9 +++++++++
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 44d622d4cc3a..29199671d9e8 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -257,6 +257,8 @@ struct xe_device {
>>   		u8 enable_display:1;
>>   		/** @bypass_mtcfg: Bypass Multi-Tile configuration from MTCFG register */
>>   		u8 bypass_mtcfg:1;
>> +		/** @skip_pcode: skip access to PCODE uC */
>> +		u8 skip_pcode:1;
>>   		/** @supports_mmio_ext: supports MMIO extension/s */
>>   		u8 supports_mmio_ext:1;
>>   		/** @has_heci_gscfi: device has heci gscfi */
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 2fae45b9d88e..05b0aa9f2ab7 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -61,6 +61,7 @@ struct xe_device_desc {
>>   
>>   	u8 has_llc:1;
>>   	u8 bypass_mtcfg:1;
>> +	u8 skip_pcode:1;
>>   	u8 supports_mmio_ext:1;
>>   };
>>   
>> @@ -578,6 +579,7 @@ static int xe_info_init(struct xe_device *xe,
>>   	xe->info.media_name = media_desc ? media_desc->name : "none";
>>   	xe->info.has_llc = desc->has_llc;
>>   	xe->info.bypass_mtcfg = desc->bypass_mtcfg;
>> +	xe->info.skip_pcode = desc->skip_pcode;
>>   	xe->info.supports_mmio_ext = desc->supports_mmio_ext;
>>   	xe->info.tile_mmio_ext_size = graphics_desc->tile_mmio_ext_size;
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
>> index 4a240acf7625..b324dc2a5deb 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode.c
>> +++ b/drivers/gpu/drm/xe/xe_pcode.c
>> @@ -61,6 +61,9 @@ static int pcode_mailbox_rw(struct xe_gt *gt, u32 mbox, u32 *data0, u32 *data1,
>>   {
>>   	int err;
>>   
>> +	if (gt_to_xe(gt)->info.skip_pcode)
>> +		return 0;
>> +
>>   	lockdep_assert_held(&gt->pcode.lock);
>>   
>>   	if ((xe_mmio_read32(gt, PCODE_MAILBOX) & PCODE_READY) != 0)
>> @@ -249,6 +252,9 @@ int xe_pcode_init(struct xe_gt *gt)
>>   	int timeout_us = 180000000; /* 3 min */
>>   	int ret;
>>   
>> +	if (gt_to_xe(gt)->info.skip_pcode)
>> +		return 0;
>> +
>>   	if (!IS_DGFX(gt_to_xe(gt)))
>>   		return 0;
>>   
>> @@ -280,6 +286,9 @@ int xe_pcode_probe(struct xe_gt *gt)
>>   {
>>   	drmm_mutex_init(&gt_to_xe(gt)->drm, &gt->pcode.lock);
>>   
>> +	if (gt_to_xe(gt)->info.skip_pcode)
>> +		return 0;
>> +
>>   	if (!IS_DGFX(gt_to_xe(gt)))
>>   		return 0;