mbox series

[0/4] drm/i915: Clean up crtc state flag checks

Message ID 20221020120457.19528-1-ville.syrjala@linux.intel.com (mailing list archive)
Headers show
Series drm/i915: Clean up crtc state flag checks | expand

Message

Ville Syrjälä Oct. 20, 2022, 12:04 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Some cleanups for checking whether the crtc was flagged for
modesets/fastsets/color update.

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(-)

Comments

Jani Nikula Oct. 20, 2022, 1:45 p.m. UTC | #1
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.

On the series,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> 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(-)
Ville Syrjälä Oct. 20, 2022, 2:17 p.m. UTC | #2
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.

> 
> 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 Oct. 20, 2022, 3:02 p.m. UTC | #3
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