Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
On Tue, 01 Oct 2019, Ville Syrjälä wrote: > On Tue, Oct 01, 2019 at 05:28:51PM +0300, Jani Nikula wrote: >> On Tue, 01 Oct 2019, Ville Syrjälä wrote: >> > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote: >> >> Quoting Jani Nikula (2019-10-01 14:43:53) >> >> > Split out code related to vga client and vga switcheroo >> >> > register/unregister and state handling from i915_drv.c and >> >> > intel_display.c. >> >> > >> >> > It's a bit difficult to draw the line how much to move to the new file >> >> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo() >> >> > and i915_resume_switcheroo() in place was cleanest. >> >> > >> >> > No functional changes. >> >> > >> >> > Cc: Ville Syrjälä >> >> > Cc: Chris Wilson >> >> > Signed-off-by: Jani Nikula >> >> > >> >> > --- >> >> > >> >> > It's also a bit fuzzy if this is a sensible split anyway. Could also >> >> > name it intel_vga and move these from intel_display.c there? >> >> >> >> My initial thought that the switcheroo interface would remain in core, >> > >> > Yeah the switcheroo stuff should perhaps stays with the rest of the pm >> > hooks. >> >> Okay, so keep all of switcheroo in i915_drv.c, and move all the vgaarb >> stuff (incl the ones I mentioned from intel_display.c) to >> intel_vga.[ch]? > > I can get behind that plan. Patch in your inbox, I'll look into switcheroo later. BR, Jani. > >> >> > >> >> that it is more of a global power state that we currently just use for >> >> the legacy vga switching. >> >> >> >> The patch looks fine, on a pure mechanical pov, >> >> Reviewed-by: Chris Wilson >> >> >> >> For the sake of argument, could you float the split in the other >> >> direction? >> >> Please elaborate. Move switcheroo higher in the call chain? >> >> BR, >> Jani. >> >> >> >> >> >> And maybe Ville has a good opinion on how it is meant to work :) >> >> -Chris >> >> -- >> Jani Nikula, Intel Open Source Graphics Center -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
On Tue, Oct 01, 2019 at 05:28:51PM +0300, Jani Nikula wrote: > On Tue, 01 Oct 2019, Ville Syrjälä wrote: > > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote: > >> Quoting Jani Nikula (2019-10-01 14:43:53) > >> > Split out code related to vga client and vga switcheroo > >> > register/unregister and state handling from i915_drv.c and > >> > intel_display.c. > >> > > >> > It's a bit difficult to draw the line how much to move to the new file > >> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo() > >> > and i915_resume_switcheroo() in place was cleanest. > >> > > >> > No functional changes. > >> > > >> > Cc: Ville Syrjälä > >> > Cc: Chris Wilson > >> > Signed-off-by: Jani Nikula > >> > > >> > --- > >> > > >> > It's also a bit fuzzy if this is a sensible split anyway. Could also > >> > name it intel_vga and move these from intel_display.c there? > >> > >> My initial thought that the switcheroo interface would remain in core, > > > > Yeah the switcheroo stuff should perhaps stays with the rest of the pm > > hooks. > > Okay, so keep all of switcheroo in i915_drv.c, and move all the vgaarb > stuff (incl the ones I mentioned from intel_display.c) to > intel_vga.[ch]? I can get behind that plan. > > > > >> that it is more of a global power state that we currently just use for > >> the legacy vga switching. > >> > >> The patch looks fine, on a pure mechanical pov, > >> Reviewed-by: Chris Wilson > >> > >> For the sake of argument, could you float the split in the other > >> direction? > > Please elaborate. Move switcheroo higher in the call chain? > > BR, > Jani. > > > >> > >> And maybe Ville has a good opinion on how it is meant to work :) > >> -Chris > > -- > Jani Nikula, Intel Open Source Graphics Center -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
Quoting Jani Nikula (2019-10-01 15:28:51) > On Tue, 01 Oct 2019, Ville Syrjälä wrote: > > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote: > >> For the sake of argument, could you float the split in the other > >> direction? > > Please elaborate. Move switcheroo higher in the call chain? I was thinking along the lines of pulling switcheroo into i915/i915_vga_client.c and seeing where that lead. I leave the details to the poor soul pulling at the stands of spaghetti. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
On Tue, 01 Oct 2019, Ville Syrjälä wrote: > On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote: >> Quoting Jani Nikula (2019-10-01 14:43:53) >> > Split out code related to vga client and vga switcheroo >> > register/unregister and state handling from i915_drv.c and >> > intel_display.c. >> > >> > It's a bit difficult to draw the line how much to move to the new file >> > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo() >> > and i915_resume_switcheroo() in place was cleanest. >> > >> > No functional changes. >> > >> > Cc: Ville Syrjälä >> > Cc: Chris Wilson >> > Signed-off-by: Jani Nikula >> > >> > --- >> > >> > It's also a bit fuzzy if this is a sensible split anyway. Could also >> > name it intel_vga and move these from intel_display.c there? >> >> My initial thought that the switcheroo interface would remain in core, > > Yeah the switcheroo stuff should perhaps stays with the rest of the pm hooks. Okay, so keep all of switcheroo in i915_drv.c, and move all the vgaarb stuff (incl the ones I mentioned from intel_display.c) to intel_vga.[ch]? > >> that it is more of a global power state that we currently just use for >> the legacy vga switching. >> >> The patch looks fine, on a pure mechanical pov, >> Reviewed-by: Chris Wilson >> >> For the sake of argument, could you float the split in the other >> direction? Please elaborate. Move switcheroo higher in the call chain? BR, Jani. >> >> And maybe Ville has a good opinion on how it is meant to work :) >> -Chris -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
On Tue, Oct 01, 2019 at 03:12:49PM +0100, Chris Wilson wrote: > Quoting Jani Nikula (2019-10-01 14:43:53) > > Split out code related to vga client and vga switcheroo > > register/unregister and state handling from i915_drv.c and > > intel_display.c. > > > > It's a bit difficult to draw the line how much to move to the new file > > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo() > > and i915_resume_switcheroo() in place was cleanest. > > > > No functional changes. > > > > Cc: Ville Syrjälä > > Cc: Chris Wilson > > Signed-off-by: Jani Nikula > > > > --- > > > > It's also a bit fuzzy if this is a sensible split anyway. Could also > > name it intel_vga and move these from intel_display.c there? > > My initial thought that the switcheroo interface would remain in core, Yeah the switcheroo stuff should perhaps stays with the rest of the pm hooks. > that it is more of a global power state that we currently just use for > the legacy vga switching. > > The patch looks fine, on a pure mechanical pov, > Reviewed-by: Chris Wilson > > For the sake of argument, could you float the split in the other > direction? > > And maybe Ville has a good opinion on how it is meant to work :) > -Chris -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
Quoting Jani Nikula (2019-10-01 14:43:53) > Split out code related to vga client and vga switcheroo > register/unregister and state handling from i915_drv.c and > intel_display.c. > > It's a bit difficult to draw the line how much to move to the new file > from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo() > and i915_resume_switcheroo() in place was cleanest. > > No functional changes. > > Cc: Ville Syrjälä > Cc: Chris Wilson > Signed-off-by: Jani Nikula > > --- > > It's also a bit fuzzy if this is a sensible split anyway. Could also > name it intel_vga and move these from intel_display.c there? My initial thought that the switcheroo interface would remain in core, that it is more of a global power state that we currently just use for the legacy vga switching. The patch looks fine, on a pure mechanical pov, Reviewed-by: Chris Wilson For the sake of argument, could you float the split in the other direction? And maybe Ville has a good opinion on how it is meant to work :) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/display: split out intel_vga_client.[ch]
On Tue, Oct 01, 2019 at 04:43:53PM +0300, Jani Nikula wrote:
> Split out code related to vga client and vga switcheroo
> register/unregister and state handling from i915_drv.c and
> intel_display.c.
The two things don't really have anything in common except both have
"vga" in the name, so not sure it makes sense to put them in the same
place. OTOH they are pretty small so probably not worth it to have two
files.
Also the vgaarb stuff is still broken but I guess no one really cares.
>
> It's a bit difficult to draw the line how much to move to the new file
> from i915_drv.c, but it seemed to me keeping i915_suspend_switcheroo()
> and i915_resume_switcheroo() in place was cleanest.
>
> No functional changes.
>
> Cc: Ville Syrjälä
> Cc: Chris Wilson
> Signed-off-by: Jani Nikula
>
> ---
>
> It's also a bit fuzzy if this is a sensible split anyway. Could also
> name it intel_vga and move these from intel_display.c there?
>
> i915_vgacntrl_reg
> i915_disable_vga
> i915_redisable_vga
> i915_redisable_vga_power_on
Considering that's the only reason we register with vgaarb it probably
makes sense to move them as well.
> ---
> drivers/gpu/drm/i915/Makefile | 3 +-
> drivers/gpu/drm/i915/display/intel_display.c | 29
> drivers/gpu/drm/i915/display/intel_display.h | 1 -
> .../gpu/drm/i915/display/intel_vga_client.c | 140 ++
> .../gpu/drm/i915/display/intel_vga_client.h | 14 ++
> drivers/gpu/drm/i915/i915_drv.c | 94 +---
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> 7 files changed, 166 insertions(+), 118 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/display/intel_vga_client.c
> create mode 100644 drivers/gpu/drm/i915/display/intel_vga_client.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e04463d85401..ca770463e01f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -184,7 +184,8 @@ i915-y += \
> display/intel_psr.o \
> display/intel_quirks.o \
> display/intel_sprite.o \
> - display/intel_tc.o
> + display/intel_tc.o \
> + display/intel_vga_client.o
> i915-$(CONFIG_ACPI) += \
> display/intel_acpi.o \
> display/intel_opregion.o
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index f1328c08f4ad..6278d62bc87d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -17188,35 +17188,6 @@ void intel_modeset_driver_remove(struct
> drm_i915_private *i915)
> intel_fbc_cleanup_cfb(i915);
> }
>
> -/*
> - * set vga decode state - true == enable VGA decode
> - */
> -int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv, bool
> state)
> -{
> - unsigned reg = INTEL_GEN(dev_priv) >= 6 ? SNB_GMCH_CTRL :
> INTEL_GMCH_CTRL;
> - u16 gmch_ctrl;
> -
> - if (pci_read_config_word(dev_priv->bridge_dev, reg, _ctrl)) {
> - DRM_ERROR("failed to read control word\n");
> - return -EIO;
> - }
> -
> - if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !state)
> - return 0;
> -
> - if (state)
> - gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
> - else
> - gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
> -
> - if (pci_write_config_word(dev_priv->bridge_dev, reg, gmch_ctrl)) {
> - DRM_ERROR("failed to write control word\n");
> - return -EIO;
> - }
> -
> - return 0;
> -}
> -
> #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>
> struct intel_display_error_state {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index 4b9e18e5a263..a7b0d11a3316 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -579,7 +579,6 @@ void intel_display_print_error_state(struct
> drm_i915_error_state_buf *e,
> void intel_modeset_init_hw(struct drm_i915_private *i915);
> int intel_modeset_init(struct drm_i915_private *i915);
> void intel_modeset_driver_remove(struct drm_i915_private *i915);
> -int intel_modeset_vga_set_state(struct drm_i915_private *dev_priv, bool
> state);
> void intel_display_resume(struct drm_device *dev);
> void i915_redisable_vga(struct drm_i915_private *dev_priv);
> void i915_redisable_vga_power_on(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/display/intel_vga_client.c
> b/drivers/gpu/drm/i915/display/intel_vga_client.c
> new file mode 100644
> index ..04ef1443f40e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/display/intel_vga_client.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include "i915_drv.h"
> +#include "intel_acpi.h"
> +#include "intel_vga_client.h"
> +
> +static
