Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, Nov 20, 2019 at 09:18:48PM +, Ville Syrjälä wrote:
> >
> > I'm not reenabling it - this is code is out there right now.
>
> > It's _dead_ code, and it's like that on purpose. You're now attempting
> > to re-enable it without fixing the underlying issues.
>
> Sounds like some sacred thing. Dead code which is not commented, neither
> removed, but which you can't touch.
>You can touch it by either removing it or fixing it. Anything else
>is a bit pointless.
Ok, then instead of pointless arguing, just need idea where to put it.
Like there is a big difference between "this code is wrong.point." and
and properly explained suggestion where/why it should be put.
Also yep to me any code which is not actually working properly or
should not be used, must be removed or commented out.
I don't really get this "dead" code concept. Sounds cool though.
>It's a global resource shared by the pipes. Hence it's quite similar
>to SAGV and cdclk.
>> So all this was simply about that I should put this function call into
>> skl_commit_modeset_disables?
>It should be in intel_atomic_commit_tail() most likely, just like cdclk.
Ok.
>I cooked up a small test that I believe should end up ping-ponging
>between one and two slices on icl, and so should end up exploding
>when the second slice gets shut down prematurely:
>git://github.com/vsyrjala/intel-gpu-tools.git dbuf_slice_test
Yep, because currently no tests explode on this.
>> Some one might come up that it is called "num_slices" or
>> "dbuf_num_slices". Seriously, may be "supported_num_dbuf_slices" to
>> keep everyones bloated egos happy??
Since we have ddb_size in device info already it should be next to that
actually. And we should probably unify the terminology a bit so we don't
mix dbuf and ddb in the same sentence.
May be still use ddb allocation struct to group things together, similar
how it is done for bw info in dev_priv. This will make it easier at least
in a sense that we won't have to constantly prefix this with dbuf/ddb.
Not sure if this needs to be done all in one as this feels more like
some cleanup/prep stuff, which is evergoing anyway.
I will anyway rename the var, as we wasted already way too much
time on this..
>It messes up the software state that we computed with something that
>we didn't compute. So going forward the state will be inconsistent.
>The core concept of atomic is that the state is precomputed and
>validated ahead of time. The commit phase is supposed to be dumb and
>isn't allowed to modify the state. This is also why we often pass
>the states as const during the commit phase, though unfortunately
>we can't do that always on account of the states being slightly
>overloaded with other metadata.
Sounds more philosophical question. You either have inconsistent
with itself software state or the software which is consistent with
itself but not with the hardware.
My idea was always to reflect hardware state as much as possible,
however I get your point here.
>> >Looking at something else unfortunately. Also it has just kept
>> >bitrotting since because no one has cared.
>>
>> That is what exactly surprises me. While we have issues
>> like commit serialization, sagv, mst, half of display buffer only
>> enabled, bw, and we are arguing about how supported_slices
>> should be named. Or that I shouldn't refer to DBuf slices as bits
>> but define as values and then decode those as bits. Priorities.
>Usually people complain about reviewers not pointing out
>all the issues in one sitting.
It was not about complaining but more about prioritization.
While trying to reach as perfect code/variable naming/placement is great,
this process is evergoing, while some things might be more important.
I don't believe DBUF_S1_BIT defined as BIT(0) or DBUF_S1 defined as 0
and then used anyway used as BIT(0) will bring any difference.
We are just arguing whether A + B or B + A is better here.
There are lots of assumptions, "dead" code and other stuff already.
And then there is a real important stuff like commit serialization problem,
the actual features implementation which have to wait, because of that.
>
> > > > >
> > > > > > for_each_new_intel_crtc_in_state(intel_state, crtc,
> > > > > > crtc_state,
> > > > > > i) {
> > > > > > const struct drm_display_mode *adjusted_mode =
> > > > > > _state->hw.adjusted_mode;
> > > > > > enum pipe pipe = crtc->pipe;
> > > > > > int hdisplay, vdisplay;
> > > > > > + u32 pipe_dbuf_slice_mask = \
> > > > > > + i915_get_allowed_dbuf_mask(dev_priv,
> > > > > > + pipe,
> > > > > > + active_pipes,
> > > > > > + crtc_state);
> > > > > >
> > > > > > if (!crtc_state->hw.enable)
> > > > > > continue;
> > > > > >
> > > > > > +
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, Nov 18, 2019 at 08:25:48PM +, Lisovskiy, Stanislav wrote:
> Hi,
>
> > > > > >
> > > > > > - /* If 2nd DBuf slice is no more required disable it */
> > > > > > - if (INTEL_GEN(dev_priv) >= 11 && required_slices <
> > > > > > hw_enabled_slices)
> > > > > > - icl_dbuf_slices_update(dev_priv,
> > > > > > required_slices);
> > > > > > + /*
> > > > > > + * If some other DBuf slice is not needed, disable it
> > > > > > here,
> > > > > > + * as here we only remove slices, which are not needed
> > > > > > anymore.
> > > > > > + */
> > > > > > + if (INTEL_GEN(dev_priv) >= 11 && required_slices !=
> > > > > > hw_enabled_slices)
> > > > > > + icl_dbuf_slices_update(dev_priv,
> > > > > > slices_intersection);
> > > > >
> > > > > This is still a totally wrong place for this. We need to be sure
> > > > > the
> > > > > planes have switched over to the new ddb layout before we power
> > > > > off
> > > > > anything.
> > > >
> > > > I have just modified already existing line here, not added by me.
> > > > The point of this patch is starting to use second DBuf slice but
> > > > not fixing all kinds of issues related to DBuf code at the same
> > > > time.
> > >
> > > You're re-enabling code I disabled on purpose. It has to be fixed
> > > properly or not touched at all. In hindsight I should have probably
> > > just ripped it all out to not give people bad ideas.
> >
> > - /* If 2nd DBuf slice is no more required disable it */
> > > > > > - if (INTEL_GEN(dev_priv) >= 11 && required_slices <
> > > > > > hw_enabled_slices)
> > > > > > - icl_dbuf_slices_update(dev_priv,
> > > > > > required_slices);
> >
> > I'm not reenabling it - this is code is out there right now.
>
> > It's _dead_ code, and it's like that on purpose. You're now attempting
> > to re-enable it without fixing the underlying issues.
>
> Sounds like some sacred thing. Dead code which is not commented, neither
> removed, but which you can't touch.
You can touch it by either removing it or fixing it. Anything else
is a bit pointless.
>
> > This is all again about changing all kinds of stuff at the same time.
> >
> > Why we can't just do things step by step? You have a point that
> > this is wrong place ok, but as we see current code causes no
> > regressions. Just tell me your idea where to put it and I will
> > do that, if you don't have it yet - let it stay there, if CI shows
> > no regressions currently.
>
> >The same global state tricks apply here as with the cdclk/sagv stuff.
> >1. enable new resources/bump up clocks
> >2. reconfigure planes/etc.
> >3. wait for vblanks
> >4. disable unused resources/drop the clocks
>
> It is different with SAGV, there we restrict whats needed first and relax
> some limitations afterwards.
It's a global resource shared by the pipes. Hence it's quite similar
to SAGV and cdclk.
> So all this was simply about that I should put this function call into
> skl_commit_modeset_disables?
It should be in intel_atomic_commit_tail() most likely, just like cdclk.
I cooked up a small test that I believe should end up ping-ponging
between one and two slices on icl, and so should end up exploding
when the second slice gets shut down prematurely:
git://github.com/vsyrjala/intel-gpu-tools.git dbuf_slice_test
> I could just have said it straight away to me, without mentioned "dead" code
> and other stuff..
I don't read minds either.
>
> > > > > > + return INTEL_INFO(dev_priv)->supported_slices;
> > > > > > +}
> > > > >
> > > > > Kinda pointless function now. 'supported_slices' is not a very
> > > > > good
> > > > > name. Should be 'num_dbuf_slices' or something along those lines.
> > > >
> > > > Supported slices really means how much slices we support in
> > > > hardware.
> > > > num_dbuf_slices will not reflect that, it will be more ambigious.
> > > > I don't like this name.
> > >
> > > supported_slices doesn't even say "dbuf" anywhere. So it's even more
> > > ambiguous. Also we already have num_pipes, num_sprites, etc.
> >
> ..and in current code we have... "enabled_slices", no? Ouch! :) IS this
> really that important and worth arguing at the very last moment?
> whether number of
> supported_slices_in_hardware, should be named "supported slices" or
> "num_dbuf_slices"? If yes, lets go on, miss some targets and continue
> discussing this "s important" thing.
>
> Some one might come up that it is called "num_slices" or
> "dbuf_num_slices". Seriously, may be "supported_num_dbuf_slices" to
> keep everyones bloated egos happy??
Since we have ddb_size in device info already it should be next to that
actually. And we should probably unify the terminology a bit so we don't
mix dbuf and ddb in the same sentence.
>
> > > > > > static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> > > > > > {
> > > > > > - I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) |
> > > > > > DBUF_POWER_REQUEST);
> > > > > > -
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, 2019-11-18 at 20:25 +, Lisovskiy, Stanislav wrote:
P.S: (In addition to my last yesterday letter):
>
> That is actually a violation of BSpec, we are not using two slices
> for same
> pipe while we could. We had already enough of bw issues, why should
> we make our life even harder?
>
> > > > > > +/*
> > > > > > + * Table taken from Bspec 49255
> > > > > > + * Pipes do have some preferred DBuf slice affinity,
> > > > > > + * plus there are some hardcoded requirements on how
> > > > > > + * those should be distributed for multipipe scenarios.
> > > > > > + * For more DBuf slices algorithm can get even more messy
> > > > > > + * and less readable, so decided to use a table almost
> > > > > > + * as is from BSpec itself - that way it is at least
> > > > > > easier
> > > > > > + * to compare, change and check.
> > > > > > + */
> > > > > > +struct dbuf_slice_conf_entry tgl_allowed_dbufs[] = {
> > > > > > +{ { 0, 0, 0, 0 }, 0 },
> > > > > > +ICL_PIPE_A_DBUF_SLICES(DBUF_S1_BIT | DBUF_S2_BIT),
> > > > > > +ICL_PIPE_B_DBUF_SLICES(DBUF_S1_BIT | DBUF_S2_BIT),
> > > > > > +ICL_PIPE_C_DBUF_SLICES(DBUF_S1_BIT | DBUF_S2_BIT),
> > > > > > +ICL_PIPE_D_DBUF_SLICES(DBUF_S1_BIT | DBUF_S2_BIT),
> > > > > > +ICL_PIPE_AB_DBUF_SLICES(DBUF_S2_BIT, DBUF_S1_BIT),
> > > > > > +ICL_PIPE_AC_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT),
> > > > > > +ICL_PIPE_BC_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT),
> > > > > > +ICL_PIPE_AD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT),
> > > > > > +ICL_PIPE_BD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT),
> > > > > > +ICL_PIPE_CD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT),
> > > > > > +ICL_PIPE_ABD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S1_BIT,
> > > > > > DBUF_S2_BIT),
> > > > > > +ICL_PIPE_ABC_DBUF_SLICES(DBUF_S1_BIT, DBUF_S1_BIT,
> > > > > > DBUF_S2_BIT),
> > > > > > +ICL_PIPE_ACD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT,
> > > > > > DBUF_S2_BIT),
> > > > > > +ICL_PIPE_BCD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT,
> > > > > > DBUF_S2_BIT),
> > > > > > +ICL_PIPE_ABCD_DBUF_SLICES(DBUF_S1_BIT, DBUF_S1_BIT,
> > > > > > DBUF_S2_BIT,
> > > > > > DBUF_S2_BIT),
> > > > > > +};
> > > > >
> > > > > My eyes!
> > > > >
> > > > > I have to think we should be able to reduce all that to a
> > > > > handful
> > > > > of lines of code.
> > > >
> > > > Yeah, then we are going to have a huge function with lots of
> > > > weird
> > > > definitions, unfortunately BSpec table has quite strange DBuf
> > > > assignments like
> > > > +ICL_PIPE_AB_DBUF_SLICES(DBUF_S2_BIT, DBUF_S1_BIT),
> > > > +ICL_PIPE_AC_DBUF_SLICES(DBUF_S1_BIT, DBUF_S2_BIT),
> > > >
> > > > i.e slices can get mixed in a quite various ways. There is no
> > > > rational pattern in those and could be even dangerous to try to
> > > > optimize it some way, as one day it might change again in some
> > > > unpredictable way.
> > > >
> > > > Would you prefer to have it like
> > > >
> > > > if (pipes are A and B)
> > > > program S2 to A and S1 to B
> > > > if (pipes are A and C)
> > > > program S1 to A and S2 to C
> > > > ...
> > > >
> > > > I would prefer at least to see it in a comparable way with the
> > > > table we have in BSpec, rather than having lots of weird
> > > > looking
> > > > if-else statements, GEN checks and so on.
> > > >
> > > > I knew this table was not going to look like it is done
> > > > typically
> > > > here - but really my opinion that it is way better than
> > > > hardcoding
> > > > it into some weird algorithm, which would be hard to compare to
> > > > initial table, which is already strange enough.
> > > >
> > > > If you don't like it - could you please explain why this is
> > > > exactly
> > > > worse than having long functions with hardcoded checks?
> > >
> > > I think we should be able to encode this simply using the
> > > "Pipe and DBUF ordering" stuff from the spec. Ie. just specify
> > > what
> > > is the distance of each pipe from each dbuf slice. Then all we
> > > have
> > > to do is minimize the total distance. The following tables I
> > > think
> > > should encode that reasonably concisely:
> > >
> > > /* pipeA - DBUF_S1 - pipeB - pipeC - DBUF_S2 */
> > > static const u8 icl_dbuf_distance[][] {
> > > [PIPE_A] = { [DBUF_S1] = 1, [DBUF_S2] = 4, },
> > > [PIPE_B] = { [DBUF_S1] = 1, [DBUF_S2] = 2, },
> > > [PIPE_C] = { [DBUF_S1] = 2, [DBUF_S2] = 1, },
> > > };
> > >
> > > /* pipeD - DBUF_S2 - pipeC - pipeA - DBUF_S1 - pipeB */
> > > static const u8 tgl_dbuf_distance[][] {
> > > [PIPE_A] = { [DBUF_S1] = 1, [DBUF_S2] = 2, },
> > > [PIPE_B] = { [DBUF_S1] = 1, [DBUF_S2] = 4, },
> > > [PIPE_C] = { [DBUF_S1] = 2, [DBUF_S2] = 1, },
> > > [PIPE_D] = { [DBUF_S1] = 4, [DBUF_S2] = 1, },
> > > };
> >
> > I think you are missing my point. I will explain why.
> > My initial idea was similar, use some kind of distance
> > metrics and encode everything in a smart way.
> > Current BSpec table has sometimes slices swapped for no reason.
> >
> > There are seem to be some hardware constraints we are not aware
> > of. For
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
Hi,
> > > > >
> > > > > - /* If 2nd DBuf slice is no more required disable it */
> > > > > - if (INTEL_GEN(dev_priv) >= 11 && required_slices <
> > > > > hw_enabled_slices)
> > > > > - icl_dbuf_slices_update(dev_priv,
> > > > > required_slices);
> > > > > + /*
> > > > > + * If some other DBuf slice is not needed, disable it
> > > > > here,
> > > > > + * as here we only remove slices, which are not needed
> > > > > anymore.
> > > > > + */
> > > > > + if (INTEL_GEN(dev_priv) >= 11 && required_slices !=
> > > > > hw_enabled_slices)
> > > > > + icl_dbuf_slices_update(dev_priv,
> > > > > slices_intersection);
> > > >
> > > > This is still a totally wrong place for this. We need to be sure
> > > > the
> > > > planes have switched over to the new ddb layout before we power
> > > > off
> > > > anything.
> > >
> > > I have just modified already existing line here, not added by me.
> > > The point of this patch is starting to use second DBuf slice but
> > > not fixing all kinds of issues related to DBuf code at the same
> > > time.
> >
> > You're re-enabling code I disabled on purpose. It has to be fixed
> > properly or not touched at all. In hindsight I should have probably
> > just ripped it all out to not give people bad ideas.
>
> - /* If 2nd DBuf slice is no more required disable it */
> > > > > - if (INTEL_GEN(dev_priv) >= 11 && required_slices <
> > > > > hw_enabled_slices)
> > > > > - icl_dbuf_slices_update(dev_priv,
> > > > > required_slices);
>
> I'm not reenabling it - this is code is out there right now.
> It's _dead_ code, and it's like that on purpose. You're now attempting
> to re-enable it without fixing the underlying issues.
Sounds like some sacred thing. Dead code which is not commented, neither
removed, but which you can't touch.
> This is all again about changing all kinds of stuff at the same time.
>
> Why we can't just do things step by step? You have a point that
> this is wrong place ok, but as we see current code causes no
> regressions. Just tell me your idea where to put it and I will
> do that, if you don't have it yet - let it stay there, if CI shows
> no regressions currently.
>The same global state tricks apply here as with the cdclk/sagv stuff.
>1. enable new resources/bump up clocks
>2. reconfigure planes/etc.
>3. wait for vblanks
>4. disable unused resources/drop the clocks
It is different with SAGV, there we restrict whats needed first and relax
some limitations afterwards.
So all this was simply about that I should put this function call into
skl_commit_modeset_disables?
I could just have said it straight away to me, without mentioned "dead" code
and other stuff..
> > > > > + return INTEL_INFO(dev_priv)->supported_slices;
> > > > > +}
> > > >
> > > > Kinda pointless function now. 'supported_slices' is not a very
> > > > good
> > > > name. Should be 'num_dbuf_slices' or something along those lines.
> > >
> > > Supported slices really means how much slices we support in
> > > hardware.
> > > num_dbuf_slices will not reflect that, it will be more ambigious.
> > > I don't like this name.
> >
> > supported_slices doesn't even say "dbuf" anywhere. So it's even more
> > ambiguous. Also we already have num_pipes, num_sprites, etc.
>
..and in current code we have... "enabled_slices", no? Ouch! :) IS this
really that important and worth arguing at the very last moment?
whether number of
supported_slices_in_hardware, should be named "supported slices" or
"num_dbuf_slices"? If yes, lets go on, miss some targets and continue
discussing this "s important" thing.
Some one might come up that it is called "num_slices" or
"dbuf_num_slices". Seriously, may be "supported_num_dbuf_slices" to
keep everyones bloated egos happy??
> > > > > static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> > > > > {
> > > > > - I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) |
> > > > > DBUF_POWER_REQUEST);
> > > > > - I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) |
> > > > > DBUF_POWER_REQUEST);
> > > > > - POSTING_READ(DBUF_CTL_S2);
> > > > > -
> > > > > - udelay(10);
> > > > > -
> > > > > - if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > > > > - !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > > > > - DRM_ERROR("DBuf power enable timeout\n");
> > > > > - else
> > > > > - /*
> > > > > - * FIXME: for now pretend that we only have 1
> > > > > slice,
> > > > > see
> > > > > - * intel_enabled_dbuf_slices_num().
> > > > > - */
> > > > > - dev_priv->wm.skl_hw.ddb.enabled_slices = 1;
> > > > > + /*
> > > > > + * Just power up 1 slice, we will
> > > > > + * figure out later which slices we have and what we
> > > > > need.
> > > > > + */
> > > > > + dev_priv->wm.skl_hw.ddb.enabled_slices = DBUF_S1_BIT;
> > > >
> > > > That stuff really shouldn't be modified by low level
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, Nov 18, 2019 at 05:17:51PM +, Lisovskiy, Stanislav wrote: > On Mon, 2019-11-18 at 17:27 +0200, Ville Syrjälä wrote: > > On Mon, Nov 18, 2019 at 09:19:18AM +, Lisovskiy, Stanislav wrote: > > > On Fri, 2019-11-15 at 22:19 +0200, Ville Syrjälä wrote: > > > > On Thu, Nov 14, 2019 at 02:24:49PM +0200, Stanislav Lisovskiy > > > > wrote: > > > > > Also implemented algorithm for choosing DBuf slice > > > > > configuration > > > > > based on active pipes, pipe ratio as stated in BSpec 12716. > > > > > > > > > > Now pipe allocation still stays proportional to pipe width as > > > > > before, > > > > > however within allowed DBuf slice for this particular > > > > > configuration. > > > > > > > > > > v2: Remove unneeded check from commit as ddb enabled slices > > > > > might > > > > > now differ from hw state. > > > > > > > > > > v3: - Added new field "supported_slices" to ddb to separate max > > > > > supported slices vs currently enabled, to avoid > > > > > confusion. > > > > > - Removed obsolete comments and code related to > > > > > DBuf(Matthew > > > > > Roper). > > > > > - Some code style and long line removal(Matthew Roper). > > > > > - Added WARN_ON to new ddb range offset calc > > > > > function(Matthew > > > > > Roper). > > > > > - Removed platform specific call to calc pipe ratio from > > > > > ddb > > > > > allocation function and fixed the return value(Matthew > > > > > Roper) > > > > > - Refactored DBUF slice allocation table to improve > > > > > readability > > > > > - Added DBUF slice allocation for TGL as well. > > > > > - ICL(however not TGL) seems to voluntarily enable second > > > > > DBuf > > > > > slice > > > > > after pm suspend/resume causing a mismatch failure, > > > > > because > > > > > we > > > > > update DBuf slices only if we do a modeset, however this > > > > > check > > > > > is done always. Fixed it to be done only when modeset for > > > > > ICL. > > > > > > > > > > v4: - Now enabled slices is not actually a number, but a > > > > > bitmask, > > > > > because we might need to enable second slice only and > > > > > number > > > > > of slices would still 1 and that current code doesn't > > > > > allow. > > > > > - Remove redundant duplicate code to have some unified way > > > > > of > > > > > enabling dbuf slices instead of hardcoding. > > > > > > > > > > v5: - Fix failing gen9_assert_dbuf_enabled as it was naively > > > > > thinking > > > > > that we have only one DBUF_CTL slice. Now another version > > > > > for > > > > > gen11 will check both slices as only second one can be > > > > > enabled, > > > > > to keep CI happy. > > > > > > > > > > v6: - Removed enabled dbuf assertion completely. Previous code > > > > > was keeping dbuf enabled, even(!) when _dbuf_disable is > > > > > called. > > > > > Currently we enable/disable dbuf slices based on > > > > > requrement > > > > > so no point in asserting this here. > > > > > - Removed unnecessary modeset check from > > > > > verify_wm_state(Matthew Roper) > > > > > - Slices intersection after union is same as final > > > > > result(Matthew Roper) > > > > > - Moved DBuf slices to intel_device_info(Matthew Roper) > > > > > - Some macros added(Matthew Roper) > > > > > - ddb range is now always less or equal to ddb size - no > > > > > need > > > > > for additional > > > > > checks(previously needed as we had some bandwidth checks > > > > > in > > > > > there which > > > > > could "suddenly" return ddb_size smaller than it is. > > > > > > > > > > v7: Minor costemic changes: > > > > > - Changed icl_dbuf_slices_restore name to > > > > > icl_program_dbuf_slices > > > > > as it more corresponds to the actual functionality. > > > > > - Some simplification with supported slices for BXT and GLK > > > > > - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as > > > > > this is not used for skl anymore. > > > > > - Changed DBuf slice assignment order for TGL case > > > > > > > > > > Reviewed-by: Matthew Roper > > > > > Signed-off-by: Stanislav Lisovskiy < > > > > > stanislav.lisovs...@intel.com> > > > > > Cc: Matthew Roper > > > > > Cc: Ville Syrjälä > > > > > Cc: James Ausmus > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 26 +- > > > > > .../drm/i915/display/intel_display_power.c| 98 ++--- > > > > > .../drm/i915/display/intel_display_power.h| 2 + > > > > > drivers/gpu/drm/i915/i915_drv.c | 5 + > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > > > drivers/gpu/drm/i915/i915_pci.c | 6 +- > > > > > drivers/gpu/drm/i915/i915_reg.h | 8 +- > > > > > drivers/gpu/drm/i915/intel_device_info.h | 1 + > > > > > drivers/gpu/drm/i915/intel_pm.c | 387 > > > > > -- > > > > > 9 files changed, 431 insertions(+),
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, 2019-11-18 at 17:27 +0200, Ville Syrjälä wrote: > On Mon, Nov 18, 2019 at 09:19:18AM +, Lisovskiy, Stanislav wrote: > > On Fri, 2019-11-15 at 22:19 +0200, Ville Syrjälä wrote: > > > On Thu, Nov 14, 2019 at 02:24:49PM +0200, Stanislav Lisovskiy > > > wrote: > > > > Also implemented algorithm for choosing DBuf slice > > > > configuration > > > > based on active pipes, pipe ratio as stated in BSpec 12716. > > > > > > > > Now pipe allocation still stays proportional to pipe width as > > > > before, > > > > however within allowed DBuf slice for this particular > > > > configuration. > > > > > > > > v2: Remove unneeded check from commit as ddb enabled slices > > > > might > > > > now differ from hw state. > > > > > > > > v3: - Added new field "supported_slices" to ddb to separate max > > > > supported slices vs currently enabled, to avoid > > > > confusion. > > > > - Removed obsolete comments and code related to > > > > DBuf(Matthew > > > > Roper). > > > > - Some code style and long line removal(Matthew Roper). > > > > - Added WARN_ON to new ddb range offset calc > > > > function(Matthew > > > > Roper). > > > > - Removed platform specific call to calc pipe ratio from > > > > ddb > > > > allocation function and fixed the return value(Matthew > > > > Roper) > > > > - Refactored DBUF slice allocation table to improve > > > > readability > > > > - Added DBUF slice allocation for TGL as well. > > > > - ICL(however not TGL) seems to voluntarily enable second > > > > DBuf > > > > slice > > > > after pm suspend/resume causing a mismatch failure, > > > > because > > > > we > > > > update DBuf slices only if we do a modeset, however this > > > > check > > > > is done always. Fixed it to be done only when modeset for > > > > ICL. > > > > > > > > v4: - Now enabled slices is not actually a number, but a > > > > bitmask, > > > > because we might need to enable second slice only and > > > > number > > > > of slices would still 1 and that current code doesn't > > > > allow. > > > > - Remove redundant duplicate code to have some unified way > > > > of > > > > enabling dbuf slices instead of hardcoding. > > > > > > > > v5: - Fix failing gen9_assert_dbuf_enabled as it was naively > > > > thinking > > > > that we have only one DBUF_CTL slice. Now another version > > > > for > > > > gen11 will check both slices as only second one can be > > > > enabled, > > > > to keep CI happy. > > > > > > > > v6: - Removed enabled dbuf assertion completely. Previous code > > > > was keeping dbuf enabled, even(!) when _dbuf_disable is > > > > called. > > > > Currently we enable/disable dbuf slices based on > > > > requrement > > > > so no point in asserting this here. > > > > - Removed unnecessary modeset check from > > > > verify_wm_state(Matthew Roper) > > > > - Slices intersection after union is same as final > > > > result(Matthew Roper) > > > > - Moved DBuf slices to intel_device_info(Matthew Roper) > > > > - Some macros added(Matthew Roper) > > > > - ddb range is now always less or equal to ddb size - no > > > > need > > > > for additional > > > > checks(previously needed as we had some bandwidth checks > > > > in > > > > there which > > > > could "suddenly" return ddb_size smaller than it is. > > > > > > > > v7: Minor costemic changes: > > > > - Changed icl_dbuf_slices_restore name to > > > > icl_program_dbuf_slices > > > > as it more corresponds to the actual functionality. > > > > - Some simplification with supported slices for BXT and GLK > > > > - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as > > > > this is not used for skl anymore. > > > > - Changed DBuf slice assignment order for TGL case > > > > > > > > Reviewed-by: Matthew Roper > > > > Signed-off-by: Stanislav Lisovskiy < > > > > stanislav.lisovs...@intel.com> > > > > Cc: Matthew Roper > > > > Cc: Ville Syrjälä > > > > Cc: James Ausmus > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 26 +- > > > > .../drm/i915/display/intel_display_power.c| 98 ++--- > > > > .../drm/i915/display/intel_display_power.h| 2 + > > > > drivers/gpu/drm/i915/i915_drv.c | 5 + > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > > drivers/gpu/drm/i915/i915_pci.c | 6 +- > > > > drivers/gpu/drm/i915/i915_reg.h | 8 +- > > > > drivers/gpu/drm/i915/intel_device_info.h | 1 + > > > > drivers/gpu/drm/i915/intel_pm.c | 387 > > > > -- > > > > 9 files changed, 431 insertions(+), 104 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > index 876fc25968bf..bd7aff675198 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, Nov 18, 2019 at 09:19:18AM +, Lisovskiy, Stanislav wrote:
> On Fri, 2019-11-15 at 22:19 +0200, Ville Syrjälä wrote:
> > On Thu, Nov 14, 2019 at 02:24:49PM +0200, Stanislav Lisovskiy wrote:
> > > Also implemented algorithm for choosing DBuf slice configuration
> > > based on active pipes, pipe ratio as stated in BSpec 12716.
> > >
> > > Now pipe allocation still stays proportional to pipe width as
> > > before,
> > > however within allowed DBuf slice for this particular
> > > configuration.
> > >
> > > v2: Remove unneeded check from commit as ddb enabled slices might
> > > now differ from hw state.
> > >
> > > v3: - Added new field "supported_slices" to ddb to separate max
> > > supported slices vs currently enabled, to avoid confusion.
> > > - Removed obsolete comments and code related to DBuf(Matthew
> > > Roper).
> > > - Some code style and long line removal(Matthew Roper).
> > > - Added WARN_ON to new ddb range offset calc function(Matthew
> > > Roper).
> > > - Removed platform specific call to calc pipe ratio from ddb
> > > allocation function and fixed the return value(Matthew Roper)
> > > - Refactored DBUF slice allocation table to improve readability
> > > - Added DBUF slice allocation for TGL as well.
> > > - ICL(however not TGL) seems to voluntarily enable second DBuf
> > > slice
> > > after pm suspend/resume causing a mismatch failure, because
> > > we
> > > update DBuf slices only if we do a modeset, however this
> > > check
> > > is done always. Fixed it to be done only when modeset for
> > > ICL.
> > >
> > > v4: - Now enabled slices is not actually a number, but a bitmask,
> > > because we might need to enable second slice only and number
> > > of slices would still 1 and that current code doesn't allow.
> > > - Remove redundant duplicate code to have some unified way of
> > > enabling dbuf slices instead of hardcoding.
> > >
> > > v5: - Fix failing gen9_assert_dbuf_enabled as it was naively
> > > thinking
> > > that we have only one DBUF_CTL slice. Now another version for
> > > gen11 will check both slices as only second one can be
> > > enabled,
> > > to keep CI happy.
> > >
> > > v6: - Removed enabled dbuf assertion completely. Previous code
> > > was keeping dbuf enabled, even(!) when _dbuf_disable is
> > > called.
> > > Currently we enable/disable dbuf slices based on requrement
> > > so no point in asserting this here.
> > > - Removed unnecessary modeset check from
> > > verify_wm_state(Matthew Roper)
> > > - Slices intersection after union is same as final
> > > result(Matthew Roper)
> > > - Moved DBuf slices to intel_device_info(Matthew Roper)
> > > - Some macros added(Matthew Roper)
> > > - ddb range is now always less or equal to ddb size - no need
> > > for additional
> > > checks(previously needed as we had some bandwidth checks in
> > > there which
> > > could "suddenly" return ddb_size smaller than it is.
> > >
> > > v7: Minor costemic changes:
> > > - Changed icl_dbuf_slices_restore name to
> > > icl_program_dbuf_slices
> > > as it more corresponds to the actual functionality.
> > > - Some simplification with supported slices for BXT and GLK
> > > - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as
> > > this is not used for skl anymore.
> > > - Changed DBuf slice assignment order for TGL case
> > >
> > > Reviewed-by: Matthew Roper
> > > Signed-off-by: Stanislav Lisovskiy
> > > Cc: Matthew Roper
> > > Cc: Ville Syrjälä
> > > Cc: James Ausmus
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 26 +-
> > > .../drm/i915/display/intel_display_power.c| 98 ++---
> > > .../drm/i915/display/intel_display_power.h| 2 +
> > > drivers/gpu/drm/i915/i915_drv.c | 5 +
> > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > drivers/gpu/drm/i915/i915_pci.c | 6 +-
> > > drivers/gpu/drm/i915/i915_reg.h | 8 +-
> > > drivers/gpu/drm/i915/intel_device_info.h | 1 +
> > > drivers/gpu/drm/i915/intel_pm.c | 387
> > > --
> > > 9 files changed, 431 insertions(+), 104 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 876fc25968bf..bd7aff675198 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13387,7 +13387,7 @@ static void verify_wm_state(struct
> > > intel_crtc *crtc,
> > >
> > > if (INTEL_GEN(dev_priv) >= 11 &&
> > > hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> > > - DRM_ERROR("mismatch in DBUF Slices (expected %u, got
> > > %u)\n",
> > > + DRM_ERROR("mismatch in DBUF Slices (expected %x, got
> > > %x)\n",
> > > sw_ddb->enabled_slices,
> >
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, 18 Nov 2019, "Lisovskiy, Stanislav" wrote: > I'm fine with fixing checkpatch and sparse failures(even though > most of those are about 80 line width limitation violation or line > continuations, which tbh are anyway quite common in our code). Please look through the warnings again. Only the COMMIT_LOG_LONG_LINE one is to be ignored in this case. There's nothing else about 80 character line width. Line continuation is about using backslash to continue to another line while that's only needed in macros. Please let's just try to leave the code in neater shape after our changes. We have these steps in CI so people wouldn't have to nitpick about them separately. > One thing I don't get is why this should cause any regression, as both > BAT and IGT are green here for two runs in a row. > > What is the point of having a CI then. Are you saying there's no point in having CI if it can't catch absolutely everything? Our CI is like every other CI everywhere; imperfect. Both in terms of hardware and software coverage. We would have no bugs reported by the community otherwise. BR, Jani. -- 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 v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Mon, 2019-11-18 at 16:22 +0200, Jani Nikula wrote: > On Thu, 14 Nov 2019, Stanislav Lisovskiy < > stanislav.lisovs...@intel.com> wrote: > > Also implemented algorithm for choosing DBuf slice configuration > > based on active pipes, pipe ratio as stated in BSpec 12716. > > > > Now pipe allocation still stays proportional to pipe width as > > before, > > however within allowed DBuf slice for this particular > > configuration. > > > > v2: Remove unneeded check from commit as ddb enabled slices might > > now differ from hw state. > > > > v3: - Added new field "supported_slices" to ddb to separate max > > supported slices vs currently enabled, to avoid confusion. > > - Removed obsolete comments and code related to DBuf(Matthew > > Roper). > > - Some code style and long line removal(Matthew Roper). > > - Added WARN_ON to new ddb range offset calc function(Matthew > > Roper). > > - Removed platform specific call to calc pipe ratio from ddb > > allocation function and fixed the return value(Matthew Roper) > > - Refactored DBUF slice allocation table to improve readability > > - Added DBUF slice allocation for TGL as well. > > - ICL(however not TGL) seems to voluntarily enable second DBuf > > slice > > after pm suspend/resume causing a mismatch failure, because > > we > > update DBuf slices only if we do a modeset, however this > > check > > is done always. Fixed it to be done only when modeset for > > ICL. > > > > v4: - Now enabled slices is not actually a number, but a bitmask, > > because we might need to enable second slice only and number > > of slices would still 1 and that current code doesn't allow. > > - Remove redundant duplicate code to have some unified way of > > enabling dbuf slices instead of hardcoding. > > > > v5: - Fix failing gen9_assert_dbuf_enabled as it was naively > > thinking > > that we have only one DBUF_CTL slice. Now another version for > > gen11 will check both slices as only second one can be > > enabled, > > to keep CI happy. > > > > v6: - Removed enabled dbuf assertion completely. Previous code > > was keeping dbuf enabled, even(!) when _dbuf_disable is > > called. > > Currently we enable/disable dbuf slices based on requrement > > so no point in asserting this here. > > - Removed unnecessary modeset check from > > verify_wm_state(Matthew Roper) > > - Slices intersection after union is same as final > > result(Matthew Roper) > > - Moved DBuf slices to intel_device_info(Matthew Roper) > > - Some macros added(Matthew Roper) > > - ddb range is now always less or equal to ddb size - no need > > for additional > > checks(previously needed as we had some bandwidth checks in > > there which > > could "suddenly" return ddb_size smaller than it is. > > > > v7: Minor costemic changes: > > - Changed icl_dbuf_slices_restore name to > > icl_program_dbuf_slices > > as it more corresponds to the actual functionality. > > - Some simplification with supported slices for BXT and GLK > > - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as > > this is not used for skl anymore. > > - Changed DBuf slice assignment order for TGL case > > > > Reviewed-by: Matthew Roper > > Signed-off-by: Stanislav Lisovskiy > > Cc: Matthew Roper > > Cc: Ville Syrjälä > > Cc: James Ausmus > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 26 +- > > .../drm/i915/display/intel_display_power.c| 98 ++--- > > .../drm/i915/display/intel_display_power.h| 2 + > > drivers/gpu/drm/i915/i915_drv.c | 5 + > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/i915_pci.c | 6 +- > > drivers/gpu/drm/i915/i915_reg.h | 8 +- > > drivers/gpu/drm/i915/intel_device_info.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 387 > > -- > > 9 files changed, 431 insertions(+), 104 deletions(-) > > This is a rather big patch, and that's one of the few things we can't > fix after committing. > > If there's a bug, by our rate of change this turns practically > un-revertible in a matter of weeks due to conflicts. If there's a > bisected regression, and this is the regressing commit, what are our > chances of identifying the bug based on the sha1 alone? Can't revert > and > can't immediately spot the bug/fix is a bad combination to have. > > Also we have this nice CI infrastructure reporting about valid > checkpatch and sparse issues for several versions of the patch. > Please > fix. > > > BR, > Jani. I'm fine with fixing checkpatch and sparse failures(even though most of those are about 80 line width limitation violation or line continuations, which tbh are anyway quite common in our code). One thing I don't get is why this should cause any regression, as both BAT and IGT are green here for two runs in a row.
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Thu, 14 Nov 2019, Stanislav Lisovskiy wrote:
> Also implemented algorithm for choosing DBuf slice configuration
> based on active pipes, pipe ratio as stated in BSpec 12716.
>
> Now pipe allocation still stays proportional to pipe width as before,
> however within allowed DBuf slice for this particular configuration.
>
> v2: Remove unneeded check from commit as ddb enabled slices might
> now differ from hw state.
>
> v3: - Added new field "supported_slices" to ddb to separate max
> supported slices vs currently enabled, to avoid confusion.
> - Removed obsolete comments and code related to DBuf(Matthew Roper).
> - Some code style and long line removal(Matthew Roper).
> - Added WARN_ON to new ddb range offset calc function(Matthew Roper).
> - Removed platform specific call to calc pipe ratio from ddb
> allocation function and fixed the return value(Matthew Roper)
> - Refactored DBUF slice allocation table to improve readability
> - Added DBUF slice allocation for TGL as well.
> - ICL(however not TGL) seems to voluntarily enable second DBuf slice
> after pm suspend/resume causing a mismatch failure, because we
> update DBuf slices only if we do a modeset, however this check
> is done always. Fixed it to be done only when modeset for ICL.
>
> v4: - Now enabled slices is not actually a number, but a bitmask,
> because we might need to enable second slice only and number
> of slices would still 1 and that current code doesn't allow.
> - Remove redundant duplicate code to have some unified way of
> enabling dbuf slices instead of hardcoding.
>
> v5: - Fix failing gen9_assert_dbuf_enabled as it was naively thinking
> that we have only one DBUF_CTL slice. Now another version for
> gen11 will check both slices as only second one can be enabled,
> to keep CI happy.
>
> v6: - Removed enabled dbuf assertion completely. Previous code
> was keeping dbuf enabled, even(!) when _dbuf_disable is called.
> Currently we enable/disable dbuf slices based on requrement
> so no point in asserting this here.
> - Removed unnecessary modeset check from verify_wm_state(Matthew Roper)
> - Slices intersection after union is same as final result(Matthew Roper)
> - Moved DBuf slices to intel_device_info(Matthew Roper)
> - Some macros added(Matthew Roper)
> - ddb range is now always less or equal to ddb size - no need for
> additional
> checks(previously needed as we had some bandwidth checks in there which
> could "suddenly" return ddb_size smaller than it is.
>
> v7: Minor costemic changes:
> - Changed icl_dbuf_slices_restore name to icl_program_dbuf_slices
> as it more corresponds to the actual functionality.
> - Some simplification with supported slices for BXT and GLK
> - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as
> this is not used for skl anymore.
> - Changed DBuf slice assignment order for TGL case
>
> Reviewed-by: Matthew Roper
> Signed-off-by: Stanislav Lisovskiy
> Cc: Matthew Roper
> Cc: Ville Syrjälä
> Cc: James Ausmus
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 26 +-
> .../drm/i915/display/intel_display_power.c| 98 ++---
> .../drm/i915/display/intel_display_power.h| 2 +
> drivers/gpu/drm/i915/i915_drv.c | 5 +
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_pci.c | 6 +-
> drivers/gpu/drm/i915/i915_reg.h | 8 +-
> drivers/gpu/drm/i915/intel_device_info.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 387 --
> 9 files changed, 431 insertions(+), 104 deletions(-)
This is a rather big patch, and that's one of the few things we can't
fix after committing.
If there's a bug, by our rate of change this turns practically
un-revertible in a matter of weeks due to conflicts. If there's a
bisected regression, and this is the regressing commit, what are our
chances of identifying the bug based on the sha1 alone? Can't revert and
can't immediately spot the bug/fix is a bad combination to have.
Also we have this nice CI infrastructure reporting about valid
checkpatch and sparse issues for several versions of the patch. Please
fix.
BR,
Jani.
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 876fc25968bf..bd7aff675198 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13387,7 +13387,7 @@ static void verify_wm_state(struct intel_crtc *crtc,
>
> if (INTEL_GEN(dev_priv) >= 11 &&
> hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> - DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> + DRM_ERROR("mismatch in DBUF Slices (expected %x, got %x)\n",
> sw_ddb->enabled_slices,
>
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Fri, 2019-11-15 at 22:19 +0200, Ville Syrjälä wrote:
> On Thu, Nov 14, 2019 at 02:24:49PM +0200, Stanislav Lisovskiy wrote:
> > Also implemented algorithm for choosing DBuf slice configuration
> > based on active pipes, pipe ratio as stated in BSpec 12716.
> >
> > Now pipe allocation still stays proportional to pipe width as
> > before,
> > however within allowed DBuf slice for this particular
> > configuration.
> >
> > v2: Remove unneeded check from commit as ddb enabled slices might
> > now differ from hw state.
> >
> > v3: - Added new field "supported_slices" to ddb to separate max
> > supported slices vs currently enabled, to avoid confusion.
> > - Removed obsolete comments and code related to DBuf(Matthew
> > Roper).
> > - Some code style and long line removal(Matthew Roper).
> > - Added WARN_ON to new ddb range offset calc function(Matthew
> > Roper).
> > - Removed platform specific call to calc pipe ratio from ddb
> > allocation function and fixed the return value(Matthew Roper)
> > - Refactored DBUF slice allocation table to improve readability
> > - Added DBUF slice allocation for TGL as well.
> > - ICL(however not TGL) seems to voluntarily enable second DBuf
> > slice
> > after pm suspend/resume causing a mismatch failure, because
> > we
> > update DBuf slices only if we do a modeset, however this
> > check
> > is done always. Fixed it to be done only when modeset for
> > ICL.
> >
> > v4: - Now enabled slices is not actually a number, but a bitmask,
> > because we might need to enable second slice only and number
> > of slices would still 1 and that current code doesn't allow.
> > - Remove redundant duplicate code to have some unified way of
> > enabling dbuf slices instead of hardcoding.
> >
> > v5: - Fix failing gen9_assert_dbuf_enabled as it was naively
> > thinking
> > that we have only one DBUF_CTL slice. Now another version for
> > gen11 will check both slices as only second one can be
> > enabled,
> > to keep CI happy.
> >
> > v6: - Removed enabled dbuf assertion completely. Previous code
> > was keeping dbuf enabled, even(!) when _dbuf_disable is
> > called.
> > Currently we enable/disable dbuf slices based on requrement
> > so no point in asserting this here.
> > - Removed unnecessary modeset check from
> > verify_wm_state(Matthew Roper)
> > - Slices intersection after union is same as final
> > result(Matthew Roper)
> > - Moved DBuf slices to intel_device_info(Matthew Roper)
> > - Some macros added(Matthew Roper)
> > - ddb range is now always less or equal to ddb size - no need
> > for additional
> > checks(previously needed as we had some bandwidth checks in
> > there which
> > could "suddenly" return ddb_size smaller than it is.
> >
> > v7: Minor costemic changes:
> > - Changed icl_dbuf_slices_restore name to
> > icl_program_dbuf_slices
> > as it more corresponds to the actual functionality.
> > - Some simplification with supported slices for BXT and GLK
> > - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as
> > this is not used for skl anymore.
> > - Changed DBuf slice assignment order for TGL case
> >
> > Reviewed-by: Matthew Roper
> > Signed-off-by: Stanislav Lisovskiy
> > Cc: Matthew Roper
> > Cc: Ville Syrjälä
> > Cc: James Ausmus
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 26 +-
> > .../drm/i915/display/intel_display_power.c| 98 ++---
> > .../drm/i915/display/intel_display_power.h| 2 +
> > drivers/gpu/drm/i915/i915_drv.c | 5 +
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/i915_pci.c | 6 +-
> > drivers/gpu/drm/i915/i915_reg.h | 8 +-
> > drivers/gpu/drm/i915/intel_device_info.h | 1 +
> > drivers/gpu/drm/i915/intel_pm.c | 387
> > --
> > 9 files changed, 431 insertions(+), 104 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 876fc25968bf..bd7aff675198 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13387,7 +13387,7 @@ static void verify_wm_state(struct
> > intel_crtc *crtc,
> >
> > if (INTEL_GEN(dev_priv) >= 11 &&
> > hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> > - DRM_ERROR("mismatch in DBUF Slices (expected %u, got
> > %u)\n",
> > + DRM_ERROR("mismatch in DBUF Slices (expected %x, got
> > %x)\n",
> > sw_ddb->enabled_slices,
> > hw->ddb.enabled_slices);
> >
> > @@ -14614,15 +14614,24 @@ static void
> > skl_commit_modeset_enables(struct intel_atomic_state *state)
> > u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> > u8 required_slices =
Re: [Intel-gfx] [PATCH v7] drm/i915: Enable second dbuf slice for ICL and TGL
On Thu, Nov 14, 2019 at 02:24:49PM +0200, Stanislav Lisovskiy wrote:
> Also implemented algorithm for choosing DBuf slice configuration
> based on active pipes, pipe ratio as stated in BSpec 12716.
>
> Now pipe allocation still stays proportional to pipe width as before,
> however within allowed DBuf slice for this particular configuration.
>
> v2: Remove unneeded check from commit as ddb enabled slices might
> now differ from hw state.
>
> v3: - Added new field "supported_slices" to ddb to separate max
> supported slices vs currently enabled, to avoid confusion.
> - Removed obsolete comments and code related to DBuf(Matthew Roper).
> - Some code style and long line removal(Matthew Roper).
> - Added WARN_ON to new ddb range offset calc function(Matthew Roper).
> - Removed platform specific call to calc pipe ratio from ddb
> allocation function and fixed the return value(Matthew Roper)
> - Refactored DBUF slice allocation table to improve readability
> - Added DBUF slice allocation for TGL as well.
> - ICL(however not TGL) seems to voluntarily enable second DBuf slice
> after pm suspend/resume causing a mismatch failure, because we
> update DBuf slices only if we do a modeset, however this check
> is done always. Fixed it to be done only when modeset for ICL.
>
> v4: - Now enabled slices is not actually a number, but a bitmask,
> because we might need to enable second slice only and number
> of slices would still 1 and that current code doesn't allow.
> - Remove redundant duplicate code to have some unified way of
> enabling dbuf slices instead of hardcoding.
>
> v5: - Fix failing gen9_assert_dbuf_enabled as it was naively thinking
> that we have only one DBUF_CTL slice. Now another version for
> gen11 will check both slices as only second one can be enabled,
> to keep CI happy.
>
> v6: - Removed enabled dbuf assertion completely. Previous code
> was keeping dbuf enabled, even(!) when _dbuf_disable is called.
> Currently we enable/disable dbuf slices based on requrement
> so no point in asserting this here.
> - Removed unnecessary modeset check from verify_wm_state(Matthew Roper)
> - Slices intersection after union is same as final result(Matthew Roper)
> - Moved DBuf slices to intel_device_info(Matthew Roper)
> - Some macros added(Matthew Roper)
> - ddb range is now always less or equal to ddb size - no need for
> additional
> checks(previously needed as we had some bandwidth checks in there which
> could "suddenly" return ddb_size smaller than it is.
>
> v7: Minor costemic changes:
> - Changed icl_dbuf_slices_restore name to icl_program_dbuf_slices
> as it more corresponds to the actual functionality.
> - Some simplification with supported slices for BXT and GLK
> - skl_pipe_downscale_amount -> icl_pipe_downscale_amount as
> this is not used for skl anymore.
> - Changed DBuf slice assignment order for TGL case
>
> Reviewed-by: Matthew Roper
> Signed-off-by: Stanislav Lisovskiy
> Cc: Matthew Roper
> Cc: Ville Syrjälä
> Cc: James Ausmus
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 26 +-
> .../drm/i915/display/intel_display_power.c| 98 ++---
> .../drm/i915/display/intel_display_power.h| 2 +
> drivers/gpu/drm/i915/i915_drv.c | 5 +
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_pci.c | 6 +-
> drivers/gpu/drm/i915/i915_reg.h | 8 +-
> drivers/gpu/drm/i915/intel_device_info.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 387 --
> 9 files changed, 431 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 876fc25968bf..bd7aff675198 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13387,7 +13387,7 @@ static void verify_wm_state(struct intel_crtc *crtc,
>
> if (INTEL_GEN(dev_priv) >= 11 &&
> hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> - DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> + DRM_ERROR("mismatch in DBUF Slices (expected %x, got %x)\n",
> sw_ddb->enabled_slices,
> hw->ddb.enabled_slices);
>
> @@ -14614,15 +14614,24 @@ static void skl_commit_modeset_enables(struct
> intel_atomic_state *state)
> u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> u8 required_slices = state->wm_results.ddb.enabled_slices;
> struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> + u8 slices_union = hw_enabled_slices | required_slices;
> + u8 slices_intersection = required_slices;
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>
