Re: [PATCH 1/2] drm/i915/display_wa: Add helpers to check wa

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 6/30/2025 12:53 PM, Jani Nikula wrote:
> On Mon, 30 Jun 2025, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
>> Introduce a generic helper to check display workarounds using an enum.
>>
>> Convert Wa_16023588340 to use the new interface, simplifying WA checks
>> and making future additions easier.
>>
>> Suggested-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display_wa.c | 13 +++++++++++++
>>   drivers/gpu/drm/i915/display/intel_display_wa.h |  9 +++++++++
>>   drivers/gpu/drm/i915/display/intel_fbc.c        |  2 +-
>>   3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.c b/drivers/gpu/drm/i915/display/intel_display_wa.c
>> index f57280e9d041..70ba66fc7e26 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
>> @@ -39,3 +39,16 @@ void intel_display_wa_apply(struct intel_display *display)
>>   	else if (DISPLAY_VER(display) == 11)
>>   		gen11_display_wa_apply(display);
>>   }
>> +
>> +bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa)
>> +{
>> +	switch (wa) {
>> +	case INTEL_DISPLAY_WA_16023588340:
>> +		return intel_display_needs_wa_16023588340(display);
>> +	default:
>> +		MISSING_CASE(wa);
> MISSING_CASE() is a bit of a problem for i915 and display
> separation. Please let's not add more. Use drm_WARN() or something.

Sure. Will get rid of the MISSING_CASE().


>
>> +		break;
>> +	}
>> +
>> +	return false;
>> +}
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h
>> index babd9d16603d..853939ebf1ac 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h
>> @@ -7,6 +7,7 @@
>>   #define __INTEL_DISPLAY_WA_H__
>>   
>>   #include <linux/types.h>
>> +#include <i915_utils.h>
> Please don't put that in the header. And shouldn't be needed anyway if
> you drop the MISSING_CASE().

Alright will remove this header file.


>
>>   
>>   struct intel_display;
>>   
>> @@ -21,4 +22,12 @@ static inline bool intel_display_needs_wa_16023588340(struct intel_display *disp
>>   bool intel_display_needs_wa_16023588340(struct intel_display *display);
>>   #endif
>>   
>> +enum intel_display_wa {
>> +	INTEL_DISPLAY_WA_16023588340,
>> +};
>> +
>> +bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa);
>> +
>> +#define _intel_display_wa_expand(__wa) INTEL_DISPLAY_WA_##__wa
> Can't we just do this inline instead of adding another macro?

Yes sure will use :

#define intel_display_wa(__display, __wa) \
         __intel_display_wa((__display), INTEL_DISPLAY_WA_##__wa)


Thanks & Regards,

Ankit

>
>> +#define intel_display_wa(__display, __wa) __intel_display_wa((__display), _intel_display_wa_expand(__wa))
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
>> index ec1ef8694c35..f4b7ff549fd4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>> @@ -1464,7 +1464,7 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
>>   		return 0;
>>   	}
>>   
>> -	if (intel_display_needs_wa_16023588340(display)) {
>> +	if (intel_display_wa(display, 16023588340)) {
>>   		plane_state->no_fbc_reason = "Wa_16023588340";
>>   		return 0;
>>   	}