Re: [Intel-gfx] [PATCH 0/4] drm/i915: Clean up crtc state flag checks

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 20 Oct 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 20, 2022 at 04:45:55PM +0300, Jani Nikula wrote:
>> On Thu, 20 Oct 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Some cleanups for checking whether the crtc was flagged for
>> > modesets/fastsets/color update.
>> 
>> I wish we could avoid piling more static inlines in
>> intel_display_types.h, but the clarity added here is great.
>
> I mainly put them there since the first one was already there.
> Dunno if the function call overhead would really be measurable,
> even though we do use these a lot. Should measure it on some
> real slouch of a machine I guess.

Well, I think some things can be static inlines just fine. In
particular, static inlines that don't require pulling in *extra* headers
are largely just fine. But as soon as you need to look at e.g. struct
drm_i915_private, you're toast. The ones added here only need the
definition of struct intel_crtc_state which is right there.

In this case, I'm more worried about the general bloating of
intel_display_types.h. And it's not even all that bad with these
patches, just piling more stuff little by little.

So I wonder if we could have intel_crtc_state.h defining just struct
intel_crtc_state and maybe some things only contained within it.

I've found one of the biggest obstacles to splitting more stuff is
actually the enums. A lot of places need the last _MAX and _NUM
enumerators for defining member array sizes. Ditto with
intel_crtc_state.

I've toyed with having intel_display_limits.h with just the limits as
#defines and separate build checks to match the enum max. I've also
toyed with having intel_display_enums.h with just the enums, so we keep
single point of truth but avoid including a lot of unnecessary stuff
from intel_display.h and intel_display_types.h. Got some patches, but
didn't get too far.

A lot of random rambling here, but does not impact the patches at hand.


BR,
Jani.




>
>> 
>> On the series,
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> Thanks.
>
>> 
>> >
>> > Ville Syrjälä (4):
>> >   drm/i915: Introduce intel_crtc_needs_fastset()
>> >   drm/i915: Remove some local 'mode_changed' bools
>> >   drm/i915: Don't flag both full modeset and fastset at the same time
>> >   drm/i915: Introduce intel_crtc_needs_color_update()
>> >
>> >  drivers/gpu/drm/i915/display/hsw_ips.c        |  8 ++--
>> >  drivers/gpu/drm/i915/display/intel_crtc.c     |  3 +-
>> >  drivers/gpu/drm/i915/display/intel_cursor.c   |  6 ++-
>> >  drivers/gpu/drm/i915/display/intel_display.c  | 46 +++++++++----------
>> >  .../drm/i915/display/intel_display_types.h    | 14 ++++++
>> >  .../drm/i915/display/intel_modeset_verify.c   |  3 +-
>> >  6 files changed, 46 insertions(+), 34 deletions(-)
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center