On 8/22/2025 5:01 PM, Jani Nikula wrote:
On Wed, 20 Aug 2025, Ankit Nautiyal <ankit.k.nauti...@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.
