[PATCH 08/12] drm/i915/display: Add guardband check for feature latencies

[Nautiyal, Ankit K]

On 8/22/2025 5:01 PM, Jani Nikula wrote:
> On Wed, 20 Aug 2025, Ankit Nautiyal <ankit.k.nautiyal at intel.com> wrote:
>> +	if (HAS_VRR(display) && intel_vrr_possible(crtc_state)) {
> Nitpick, and a tangential to designing stuff:
>
> intel_vrr_possible() never returns true for !HAS_VRR(). The HAS_VRR()
> check is redundant. Adding redundant checks adds uncertainty about what
> intel_vrr_possible() can return. "Whoa, can it return true even for
> !HAS_VRR()? Why?" And then it reinforces the mentality that everything
> needs redundancy and double checking.

Hi Jani,

Thanks for the review! You're right : since flipline is only set when 
HAS_VRR() is true, the extra check was unnecessary.

I'll drop the HAS_VRR() check and update the patch.

>
> This is not about just that one check and one line. The idea is that for
> most "has feature" checks that enable something in the crtc state, you
> do that check in very few places, and the fields in crtc state dictate
> the rest. You're not supposed to have to second guess what crtc state
> has.

That makes sense - checking for HAS_* at some places and then trusting 
the crtc state keeps things cleaner.

I noticed a couple of places, where HAS_VRR() and intel_vrr_possible are 
used together. I will go through those and send a follow-up patch to 
simplify them.

I'll also keep this principle in mind while writing and reviewing code 
going forward.

Thanks for the insight!

Regards,

Ankit

>
> Food for though.
>
>
> BR,
> Jani.
>
>