[RFC,00/33] drm/i915/display: mass conversion to intel_de_*() register accessors
mbox series

Message ID cover.1579871655.git.jani.nikula@intel.com
Headers show
Series
  • drm/i915/display: mass conversion to intel_de_*() register accessors
Related show

Message

Jani Nikula Jan. 24, 2020, 1:25 p.m. UTC
Hey all,

So I sent [1] to convert some forcewake register accessors... but what if we
just ripped off the bandage once and for all? It's going to hurt, a lot, but
we'd get it done.

This completely rids us of the "dev_priv" dependency in display/.

All the patches here are per-file and independent of each other. We could also
pick and apply the ones that are least likely to conflict.

Opinions?


BR,
Jani.


PS. I didn't bother looking at the checkpatch warnings this may generate at this
point. I just used the --linux-spacing option for spatch, and closed my eyes. I
completely scripted the generation of the series, apart from just a couple of
build fixes.


Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

[1] https://patchwork.freedesktop.org/series/72476/


Jani Nikula (33):
  drm/i915/icl_dsi: use intel_de_*() functions for register access
  drm/i915/audio: use intel_de_*() functions for register access
  drm/i915/cdclk: use intel_de_*() functions for register access
  drm/i915/color: use intel_de_*() functions for register access
  drm/i915/combo_phy: use intel_de_*() functions for register access
  drm/i915/crt: use intel_de_*() functions for register access
  drm/i915/ddi: use intel_de_*() functions for register access
  drm/i915/display: use intel_de_*() functions for register access
  drm/i915/display_power: use intel_de_*() functions for register access
  drm/i915/dp: use intel_de_*() functions for register access
  drm/i915/dpio_phy: use intel_de_*() functions for register access
  drm/i915/dpll_mgr: use intel_de_*() functions for register access
  drm/i915/dp_mst: use intel_de_*() functions for register access
  drm/i915/dsb: use intel_de_*() functions for register access
  drm/i915/dvo: use intel_de_*() functions for register access
  drm/i915/fbc: use intel_de_*() functions for register access
  drm/i915/fifo_underrun: use intel_de_*() functions for register access
  drm/i915/gmbus: use intel_de_*() functions for register access
  drm/i915/hdcp: use intel_de_*() functions for register access
  drm/i915/hdmi: use intel_de_*() functions for register access
  drm/i915/lpe_audio: use intel_de_*() functions for register access
  drm/i915/lvds: use intel_de_*() functions for register access
  drm/i915/overlay: use intel_de_*() functions for register access
  drm/i915/panel: use intel_de_*() functions for register access
  drm/i915/pipe_crc: use intel_de_*() functions for register access
  drm/i915/psr: use intel_de_*() functions for register access
  drm/i915/sdvo: use intel_de_*() functions for register access
  drm/i915/sprite: use intel_de_*() functions for register access
  drm/i915/tv: use intel_de_*() functions for register access
  drm/i915/vdsc: use intel_de_*() functions for register access
  drm/i915/vga: use intel_de_*() functions for register access
  drm/i915/vlv_dsi: use intel_de_*() functions for register access
  drm/i915/vlv_dsi_pll: use intel_de_*() functions for register access

 drivers/gpu/drm/i915/display/icl_dsi.c        |  271 ++--
 drivers/gpu/drm/i915/display/intel_audio.c    |  112 +-
 drivers/gpu/drm/i915/display/intel_cdclk.c    |  133 +-
 drivers/gpu/drm/i915/display/intel_color.c    |  204 +--
 .../gpu/drm/i915/display/intel_combo_phy.c    |   66 +-
 drivers/gpu/drm/i915/display/intel_crt.c      |   51 +-
 drivers/gpu/drm/i915/display/intel_ddi.c      |  474 +++----
 drivers/gpu/drm/i915/display/intel_display.c  | 1171 +++++++++--------
 .../drm/i915/display/intel_display_power.c    |  294 +++--
 drivers/gpu/drm/i915/display/intel_dp.c       |  234 ++--
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |   11 +-
 drivers/gpu/drm/i915/display/intel_dpio_phy.c |   77 +-
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  388 +++---
 drivers/gpu/drm/i915/display/intel_dsb.c      |   24 +-
 drivers/gpu/drm/i915/display/intel_dvo.c      |   34 +-
 drivers/gpu/drm/i915/display/intel_fbc.c      |  106 +-
 .../drm/i915/display/intel_fifo_underrun.c    |   37 +-
 drivers/gpu/drm/i915/display/intel_gmbus.c    |   74 +-
 drivers/gpu/drm/i915/display/intel_hdcp.c     |  142 +-
 drivers/gpu/drm/i915/display/intel_hdmi.c     |  258 ++--
 .../gpu/drm/i915/display/intel_lpe_audio.c    |   14 +-
 drivers/gpu/drm/i915/display/intel_lvds.c     |   57 +-
 drivers/gpu/drm/i915/display/intel_overlay.c  |   45 +-
 drivers/gpu/drm/i915/display/intel_panel.c    |  255 ++--
 drivers/gpu/drm/i915/display/intel_pipe_crc.c |   20 +-
 drivers/gpu/drm/i915/display/intel_psr.c      |   70 +-
 drivers/gpu/drm/i915/display/intel_sdvo.c     |   30 +-
 drivers/gpu/drm/i915/display/intel_sprite.c   |  320 +++--
 drivers/gpu/drm/i915/display/intel_tv.c       |  138 +-
 drivers/gpu/drm/i915/display/intel_vdsc.c     |  413 +++---
 drivers/gpu/drm/i915/display/intel_vga.c      |    7 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c        |  345 ++---
 drivers/gpu/drm/i915/display/vlv_dsi_pll.c    |   49 +-
 33 files changed, 3158 insertions(+), 2766 deletions(-)

Comments

Chris Wilson Jan. 24, 2020, 1:54 p.m. UTC | #1
Quoting Jani Nikula (2020-01-24 13:25:21)
> Hey all,
> 
> So I sent [1] to convert some forcewake register accessors... but what if we
> just ripped off the bandage once and for all? It's going to hurt, a lot, but
> we'd get it done.
> 
> This completely rids us of the "dev_priv" dependency in display/.
> 
> All the patches here are per-file and independent of each other. We could also
> pick and apply the ones that are least likely to conflict.
> 
> Opinions?
> 
> 
> BR,
> Jani.
> 
> 
> PS. I didn't bother looking at the checkpatch warnings this may generate at this
> point. I just used the --linux-spacing option for spatch, and closed my eyes. I
> completely scripted the generation of the series, apart from just a couple of
> build fixes.

Yup. Suck it all in, clean up with the usual code refreshes.
Schadenfreude-by: Chris Wilson <chris@chris-wilson.co.uk>

I've looked at a couple of patches to confirm that it does appear purely
mechanical,
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Matt Roper Jan. 24, 2020, 9:35 p.m. UTC | #2
On Fri, Jan 24, 2020 at 03:25:21PM +0200, Jani Nikula wrote:
> Hey all,
> 
> So I sent [1] to convert some forcewake register accessors... but what if we
> just ripped off the bandage once and for all? It's going to hurt, a lot, but
> we'd get it done.
> 
> This completely rids us of the "dev_priv" dependency in display/.
> 
> All the patches here are per-file and independent of each other. We could also
> pick and apply the ones that are least likely to conflict.
> 
> Opinions?
> 
> 
> BR,
> Jani.
> 
> 
> PS. I didn't bother looking at the checkpatch warnings this may generate at this
> point. I just used the --linux-spacing option for spatch, and closed my eyes. I
> completely scripted the generation of the series, apart from just a couple of
> build fixes.
> 
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> [1] https://patchwork.freedesktop.org/series/72476/
> 
> 
> Jani Nikula (33):
>   drm/i915/icl_dsi: use intel_de_*() functions for register access
>   drm/i915/audio: use intel_de_*() functions for register access
>   drm/i915/cdclk: use intel_de_*() functions for register access
>   drm/i915/color: use intel_de_*() functions for register access
>   drm/i915/combo_phy: use intel_de_*() functions for register access
>   drm/i915/crt: use intel_de_*() functions for register access
>   drm/i915/ddi: use intel_de_*() functions for register access
>   drm/i915/display: use intel_de_*() functions for register access
>   drm/i915/display_power: use intel_de_*() functions for register access
>   drm/i915/dp: use intel_de_*() functions for register access
>   drm/i915/dpio_phy: use intel_de_*() functions for register access
>   drm/i915/dpll_mgr: use intel_de_*() functions for register access
>   drm/i915/dp_mst: use intel_de_*() functions for register access
>   drm/i915/dsb: use intel_de_*() functions for register access
>   drm/i915/dvo: use intel_de_*() functions for register access
>   drm/i915/fbc: use intel_de_*() functions for register access
>   drm/i915/fifo_underrun: use intel_de_*() functions for register access
>   drm/i915/gmbus: use intel_de_*() functions for register access
>   drm/i915/hdcp: use intel_de_*() functions for register access
>   drm/i915/hdmi: use intel_de_*() functions for register access
>   drm/i915/lpe_audio: use intel_de_*() functions for register access
>   drm/i915/lvds: use intel_de_*() functions for register access
>   drm/i915/overlay: use intel_de_*() functions for register access
>   drm/i915/panel: use intel_de_*() functions for register access
>   drm/i915/pipe_crc: use intel_de_*() functions for register access
>   drm/i915/psr: use intel_de_*() functions for register access
>   drm/i915/sdvo: use intel_de_*() functions for register access
>   drm/i915/sprite: use intel_de_*() functions for register access
>   drm/i915/tv: use intel_de_*() functions for register access
>   drm/i915/vdsc: use intel_de_*() functions for register access
>   drm/i915/vga: use intel_de_*() functions for register access
>   drm/i915/vlv_dsi: use intel_de_*() functions for register access
>   drm/i915/vlv_dsi_pll: use intel_de_*() functions for register access
> 
>  drivers/gpu/drm/i915/display/icl_dsi.c        |  271 ++--
>  drivers/gpu/drm/i915/display/intel_audio.c    |  112 +-
>  drivers/gpu/drm/i915/display/intel_cdclk.c    |  133 +-
>  drivers/gpu/drm/i915/display/intel_color.c    |  204 +--
>  .../gpu/drm/i915/display/intel_combo_phy.c    |   66 +-
>  drivers/gpu/drm/i915/display/intel_crt.c      |   51 +-
>  drivers/gpu/drm/i915/display/intel_ddi.c      |  474 +++----
>  drivers/gpu/drm/i915/display/intel_display.c  | 1171 +++++++++--------
>  .../drm/i915/display/intel_display_power.c    |  294 +++--
>  drivers/gpu/drm/i915/display/intel_dp.c       |  234 ++--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c   |   11 +-
>  drivers/gpu/drm/i915/display/intel_dpio_phy.c |   77 +-
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c |  388 +++---
>  drivers/gpu/drm/i915/display/intel_dsb.c      |   24 +-
>  drivers/gpu/drm/i915/display/intel_dvo.c      |   34 +-
>  drivers/gpu/drm/i915/display/intel_fbc.c      |  106 +-
>  .../drm/i915/display/intel_fifo_underrun.c    |   37 +-
>  drivers/gpu/drm/i915/display/intel_gmbus.c    |   74 +-
>  drivers/gpu/drm/i915/display/intel_hdcp.c     |  142 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c     |  258 ++--
>  .../gpu/drm/i915/display/intel_lpe_audio.c    |   14 +-
>  drivers/gpu/drm/i915/display/intel_lvds.c     |   57 +-
>  drivers/gpu/drm/i915/display/intel_overlay.c  |   45 +-
>  drivers/gpu/drm/i915/display/intel_panel.c    |  255 ++--
>  drivers/gpu/drm/i915/display/intel_pipe_crc.c |   20 +-
>  drivers/gpu/drm/i915/display/intel_psr.c      |   70 +-
>  drivers/gpu/drm/i915/display/intel_sdvo.c     |   30 +-
>  drivers/gpu/drm/i915/display/intel_sprite.c   |  320 +++--
>  drivers/gpu/drm/i915/display/intel_tv.c       |  138 +-
>  drivers/gpu/drm/i915/display/intel_vdsc.c     |  413 +++---
>  drivers/gpu/drm/i915/display/intel_vga.c      |    7 +-
>  drivers/gpu/drm/i915/display/vlv_dsi.c        |  345 ++---
>  drivers/gpu/drm/i915/display/vlv_dsi_pll.c    |   49 +-
>  33 files changed, 3158 insertions(+), 2766 deletions(-)

There's a lot of display (watermark) code in intel_pm.c as well, even
though it doesn't live in the display/ directory.  We should probably
pull the watermark stuff out into a separate display/intel_wm.c or
something soon, but in the meantime we'll probably want to switch a
bunch of that code over to using these new functions.  But I guess you
can't do that with coccinelle though since there are parts of the file
that aren't display-related and shouldn't use the same display helpers.


Matt

> 
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi Jan. 24, 2020, 10:30 p.m. UTC | #3
On Fri, Jan 24, 2020 at 01:54:58PM +0000, Chris Wilson wrote:
> Quoting Jani Nikula (2020-01-24 13:25:21)
> > Hey all,
> > 
> > So I sent [1] to convert some forcewake register accessors... but what if we
> > just ripped off the bandage once and for all? It's going to hurt, a lot, but
> > we'd get it done.
> > 
> > This completely rids us of the "dev_priv" dependency in display/.
> > 
> > All the patches here are per-file and independent of each other. We could also
> > pick and apply the ones that are least likely to conflict.
> > 
> > Opinions?
> > 
> > 
> > BR,
> > Jani.
> > 
> > 
> > PS. I didn't bother looking at the checkpatch warnings this may generate at this
> > point. I just used the --linux-spacing option for spatch, and closed my eyes. I
> > completely scripted the generation of the series, apart from just a couple of
> > build fixes.
> 
> Yup. Suck it all in, clean up with the usual code refreshes.
> Schadenfreude-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I've looked at a couple of patches to confirm that it does appear purely
> mechanical,
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>

Since it is purely mechanical with coccinelle, why not to make in only one patch?

Anyway:
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Jan. 25, 2020, 2:55 p.m. UTC | #4
On Fri, 24 Jan 2020, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> On Fri, Jan 24, 2020 at 01:54:58PM +0000, Chris Wilson wrote:
>> Quoting Jani Nikula (2020-01-24 13:25:21)
>> > Hey all,
>> > 
>> > So I sent [1] to convert some forcewake register accessors... but what if we
>> > just ripped off the bandage once and for all? It's going to hurt, a lot, but
>> > we'd get it done.
>> > 
>> > This completely rids us of the "dev_priv" dependency in display/.
>> > 
>> > All the patches here are per-file and independent of each other. We could also
>> > pick and apply the ones that are least likely to conflict.
>> > 
>> > Opinions?
>> > 
>> > 
>> > BR,
>> > Jani.
>> > 
>> > 
>> > PS. I didn't bother looking at the checkpatch warnings this may generate at this
>> > point. I just used the --linux-spacing option for spatch, and closed my eyes. I
>> > completely scripted the generation of the series, apart from just a couple of
>> > build fixes.
>> 
>> Yup. Suck it all in, clean up with the usual code refreshes.
>> Schadenfreude-by: Chris Wilson <chris@chris-wilson.co.uk>
>> 
>> I've looked at a couple of patches to confirm that it does appear purely
>> mechanical,
>> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Since it is purely mechanical with coccinelle, why not to make in only one patch?

Because such a mega patch would conflict before being able to
merge. You'd have to block everything else. I don't think I'd be able to
merge these in one go either even if we wanted to.

> Anyway:
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Thanks,
Jani.

>
>> -Chris
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Joonas Lahtinen Jan. 27, 2020, 1:48 p.m. UTC | #5
Quoting Jani Nikula (2020-01-24 15:25:21)
> Hey all,
> 
> So I sent [1] to convert some forcewake register accessors... but what if we
> just ripped off the bandage once and for all? It's going to hurt, a lot, but
> we'd get it done.
> 
> This completely rids us of the "dev_priv" dependency in display/.
> 
> All the patches here are per-file and independent of each other. We could also
> pick and apply the ones that are least likely to conflict.
> 
> Opinions?

Acked-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
Jani Nikula Jan. 27, 2020, 6:10 p.m. UTC | #6
On Sat, 25 Jan 2020, Jani Nikula <jani.nikula@intel.com> wrote:
> On Fri, 24 Jan 2020, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> On Fri, Jan 24, 2020 at 01:54:58PM +0000, Chris Wilson wrote:
>>> Quoting Jani Nikula (2020-01-24 13:25:21)
>>> > Hey all,
>>> > 
>>> > So I sent [1] to convert some forcewake register accessors... but what if we
>>> > just ripped off the bandage once and for all? It's going to hurt, a lot, but
>>> > we'd get it done.
>>> > 
>>> > This completely rids us of the "dev_priv" dependency in display/.
>>> > 
>>> > All the patches here are per-file and independent of each other. We could also
>>> > pick and apply the ones that are least likely to conflict.
>>> > 
>>> > Opinions?
>>> > 
>>> > 
>>> > BR,
>>> > Jani.
>>> > 
>>> > 
>>> > PS. I didn't bother looking at the checkpatch warnings this may generate at this
>>> > point. I just used the --linux-spacing option for spatch, and closed my eyes. I
>>> > completely scripted the generation of the series, apart from just a couple of
>>> > build fixes.
>>> 
>>> Yup. Suck it all in, clean up with the usual code refreshes.
>>> Schadenfreude-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> 
>>> I've looked at a couple of patches to confirm that it does appear purely
>>> mechanical,
>>> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> Since it is purely mechanical with coccinelle, why not to make in only one patch?
>
> Because such a mega patch would conflict before being able to
> merge. You'd have to block everything else. I don't think I'd be able to
> merge these in one go either even if we wanted to.

Indeed, I started pushing the patches, pushed all that applied, and
eight will need a rebase.

Thanks for the acks.

BR,
Jani.



>
>> Anyway:
>> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Thanks,
> Jani.
>
>>
>>> -Chris
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Jan. 27, 2020, 6:29 p.m. UTC | #7
On Fri, 24 Jan 2020, Matt Roper <matthew.d.roper@intel.com> wrote:
> There's a lot of display (watermark) code in intel_pm.c as well, even
> though it doesn't live in the display/ directory.  We should probably
> pull the watermark stuff out into a separate display/intel_wm.c or
> something soon, but in the meantime we'll probably want to switch a
> bunch of that code over to using these new functions.  But I guess you
> can't do that with coccinelle though since there are parts of the file
> that aren't display-related and shouldn't use the same display helpers.

Yeah, display/ was a clear-cut line. I may have already pushed some
patches using intel_de_*_fw from the top level code, but I think it
would be better to move all large chunks of code that do display uncore
stuff under display/.

BR,
Jani.