On Fri, Feb 09, 2024 at 01:17:27PM +0000, Kahola, Mika wrote: > > -----Original Message----- > > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > Sent: Friday, February 9, 2024 2:49 PM > > To: Kahola, Mika <mika.kah...@intel.com> > > Cc: Jani Nikula <jani.nik...@linux.intel.com>; > > intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for eDP > > > > On Fri, Feb 09, 2024 at 12:33:20PM +0000, Kahola, Mika wrote: > > > > -----Original Message----- > > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > > > Sent: Friday, February 9, 2024 2:19 PM > > > > To: Kahola, Mika <mika.kah...@intel.com> > > > > Cc: Jani Nikula <jani.nik...@linux.intel.com>; > > > > intel-gfx@lists.freedesktop.org > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset for > > > > eDP > > > > > > > > On Fri, Feb 09, 2024 at 12:13:02PM +0000, Kahola, Mika wrote: > > > > > > -----Original Message----- > > > > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > > > > > Sent: Friday, February 9, 2024 2:07 PM > > > > > > To: Kahola, Mika <mika.kah...@intel.com> > > > > > > Cc: Jani Nikula <jani.nik...@linux.intel.com>; > > > > > > intel-gfx@lists.freedesktop.org > > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full modeset > > > > > > for eDP > > > > > > > > > > > > On Fri, Feb 09, 2024 at 11:55:58AM +0000, Kahola, Mika wrote: > > > > > > > > -----Original Message----- > > > > > > > > From: Jani Nikula <jani.nik...@linux.intel.com> > > > > > > > > Sent: Friday, February 9, 2024 11:06 AM > > > > > > > > To: Kahola, Mika <mika.kah...@intel.com>; > > > > > > > > intel-gfx@lists.freedesktop.org > > > > > > > > Cc: Kahola, Mika <mika.kah...@intel.com> > > > > > > > > Subject: Re: [PATCH 2/2] drm/i915/display: Force full > > > > > > > > modeset for eDP > > > > > > > > > > > > > > > > On Tue, 06 Feb 2024, Mika Kahola <mika.kah...@intel.com> wrote: > > > > > > > > > Force full modeset for eDP when booting up. GOP programs > > > > > > > > > PLL parameters and hence, we would be able to use fastset for > > > > > > > > > eDP. > > > > > > > > > However, with fastset we are not setting PLL values from > > > > > > > > > the driver and rely that GOP and driver PLL values match. > > > > > > > > > We have discovered that with some of the panels this is > > > > > > > > > not true and hence we would need to program PLL values by > > > > > > > > > the driver. The patch suggests a workaround as enabling > > > > > > > > > full modeset when booting up. This way we force the driver to > > > > > > > > > write the PLL values to the hw. > > > > > > > > > > > > > > > > No. We want to avoid full modesets if possible, both in general > > > > > > > > and at probe. > > > > > > > > > > > > > > > > And when we do end up with modesets, the decision needs to > > > > > > > > be based on changes in the state that we can't write to the > > > > > > > > hardware without a modeset. > > > > > > > > > > > > > > > > We can't unconditionally force a modeset on eDP panels at probe. > > > > > > > > > > > > > > Thanks! Just wondering what the options here might be? With > > > > > > > fastest we end up having a mismatch with one PLL value with a > > > > > > few panels. > > > > > > > > > > > > You seem to be stuck in some infinite loop. If your PLL > > > > > > parameters are mismatching that should prevent the fastset, but > > > > > > then I guess you added some hack to allow the fastset despite > > > > > > the mismatch, and now you're trying to undo that hack by blindly > > > > > > forcing a > > > > full modeset? > > > > > > > > > > That's right, I found myself to be between a rock and a hard place. I > > > > > did discard the fastest but found out that we cannot do > > that. > > > > > > > > If you discarded it then why are you not already getting the full > > > > modeset you want? > > > > > > > Poor choice of words, I guess. What I meant that I discarded the state > > > verification in case of fastest. This way the mismatch is > > hidden under the carpet. > > > > Right. So just add the state verification back and it'll just work (tm). > > I wish it would be that simple. With fastest it seems that we are not > programming PLL values but instead rely the ones GOP programs. When reading > out these values from HW there is this one value that differs from the SW > state defined by our driver. I was thinking about that we should read the > state from HW and update our SW state based on that but then we would end up > comparing HW state to HW state, which to me is not what state verification is > all about.
The actual problem would appear to be that someone forgot to handle these snps/cx0 PLLs in pipe_config_compare(). Implementing these PLLs outside the dpll_mgr framework was another big mistake. And the ridonculous size of dpll_hw_state is another problem. We need to do some major refactoring to clean up this mess once and for all. Some kind of plan: 1. move all PLLs over to the dpll_mgr framework, starting with snps/cx0 2. move all PLL hw state verification into the dpll_mgr so it can be customized per-PLL type 3. Perhaps make dpll_hw_state opaque and split it up into per-PLL type variants. Need to think about memory allocation issues with this one... I think I'll see if I can quickly type up a start for 2... -- Ville Syrjälä Intel
