On Mon, 08 Dec 2025, Ville Syrjala <[email protected]> wrote: > From: Ville Syrjälä <[email protected]> > > Remove the MSR VGA register access from the power well hook, and > just do it once in intel_vga_disable(). > > Turns out that the hardware has two levels of MDA vs. CGA decode > logic: GPU level and display engine level. When we write the MSR > register MDA/CGA mode selection bit both decode logics are updated > accordingly, so that whichever register accessed the GPU claims > will also be claimed by the display engine on the RMbus. If the > two get out of sync the GPU will claim the register accesses but > the display engine will not, leading to RMbus NoClaim errors. > > The way to get the two decode logics out of sync is by resetting > the power well housing the VGA stuff, while we are in CGA mode. > At that point only the display engine level decode logic will be > updated with the new MSR register reset value (which selects MDA > mode), and the GPU level decode logic will retain its previous > state (GGA mode). To avoid the mismatch we just have to switch > to MDA mode with an explicit MSR register write. > > Currently this is being done in a somewhat dodgy manner whenever > the power well gets enabled. But doing if from the power well > hook is not actually necessary since the GPU level decode logic > will retain its state even when the power well is disabled. Ie. > doing it just the one time is sufficient, and that can be done > when we're anyway writing other VGA registers while disabling > the VGA plane. > > See commit f9dcb0dfee98 ("drm/i915: touch VGA MSR after we > enable the power well") for the original details. > > Signed-off-by: Ville Syrjälä <[email protected]>
I'll take your word for it. Looks plausible. Acked-by: Jani Nikula <[email protected]> > --- > .../i915/display/intel_display_power_well.c | 3 -- > drivers/gpu/drm/i915/display/intel_vga.c | 40 +++++++++---------- > 2 files changed, 20 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c > b/drivers/gpu/drm/i915/display/intel_display_power_well.c > index db185a859133..52b20118ace6 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c > @@ -204,9 +204,6 @@ int intel_power_well_refcount(struct i915_power_well > *power_well) > static void hsw_power_well_post_enable(struct intel_display *display, > u8 irq_pipe_mask, bool has_vga) > { > - if (has_vga) > - intel_vga_reset_io_mem(display); > - > if (irq_pipe_mask) > gen8_irq_power_well_post_enable(display, irq_pipe_mask); > } > diff --git a/drivers/gpu/drm/i915/display/intel_vga.c > b/drivers/gpu/drm/i915/display/intel_vga.c > index f13734cfd904..39c68aec647b 100644 > --- a/drivers/gpu/drm/i915/display/intel_vga.c > +++ b/drivers/gpu/drm/i915/display/intel_vga.c > @@ -47,8 +47,8 @@ void intel_vga_disable(struct intel_display *display) > struct pci_dev *pdev = to_pci_dev(display->drm->dev); > i915_reg_t vga_reg = intel_vga_cntrl_reg(display); > enum pipe pipe; > + u8 msr, sr1; > u32 tmp; > - u8 sr1; > > tmp = intel_de_read(display, vga_reg); > if (tmp & VGA_DISP_DISABLE) > @@ -66,35 +66,35 @@ void intel_vga_disable(struct intel_display *display) > > /* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */ > vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO); > + > outb(0x01, VGA_SEQ_I); > sr1 = inb(VGA_SEQ_D); > outb(sr1 | VGA_SR01_SCREEN_OFF, VGA_SEQ_D); > + > + msr = inb(VGA_MIS_R); > + /* > + * VGA_MIS_COLOR controls both GPU level and display engine level > + * MDA vs. CGA decode logic. But when the register gets reset > + * (reset value has VGA_MIS_COLOR=0) by the power well, only the > + * display engine level decode logic gets notified. > + * > + * Switch to MDA mode to make sure the GPU level decode logic will > + * be in sync with the display engine level decode logic after the > + * power well has been reset. Otherwise the GPU will claim CGA > + * register accesses but the display engine will not, causing > + * RMbus NoClaim errors. > + */ > + msr &= ~VGA_MIS_COLOR; > + outb(msr, VGA_MIS_W); > + > vga_put(pdev, VGA_RSRC_LEGACY_IO); > + > udelay(300); > > intel_de_write(display, vga_reg, VGA_DISP_DISABLE); > intel_de_posting_read(display, vga_reg); > } > > -void intel_vga_reset_io_mem(struct intel_display *display) > -{ > - struct pci_dev *pdev = to_pci_dev(display->drm->dev); > - > - /* > - * After we re-enable the power well, if we touch VGA register 0x3d5 > - * we'll get unclaimed register interrupts. This stops after we write > - * anything to the VGA MSR register. The vgacon module uses this > - * register all the time, so if we unbind our driver and, as a > - * consequence, bind vgacon, we'll get stuck in an infinite loop at > - * console_unlock(). So make here we touch the VGA MSR register, making > - * sure vgacon can keep working normally without triggering interrupts > - * and error messages. > - */ > - vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO); > - outb(inb(VGA_MIS_R), VGA_MIS_W); > - vga_put(pdev, VGA_RSRC_LEGACY_IO); > -} > - > static int intel_gmch_vga_set_state(struct intel_display *display, bool > enable_decode) > { > struct pci_dev *pdev = to_pci_dev(display->drm->dev); -- Jani Nikula, Intel
