Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Tue, Aug 29, 2023 at 09:04:28AM -0700, Matt Roper wrote: > On Tue, Aug 29, 2023 at 06:50:02AM -0700, Govindapillai, Vinod wrote: > > On Mon, 2023-08-28 at 17:16 -0700, Matt Roper wrote: > > > On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote: > > > > In LNL onwards, FBC can be associated to the first three planes. > > > > > > The title of this patch shouldn't say "any plane" when the reality is > > > that only the first three support FBC (the upper two do not). > > > > > > > The FBC will be enabled for first FBC capable visible plane > > > > until the userspace can select one of these FBC capable plane > > > > explicitly > > > > > > Even if we add new Intel-specific uapi to select this explicitly, is any > > > userspace actually going to use it? Would it make more sense to try to > > > come up with a heuristic to guess which plane would benefit the most and > > > switch to that automatically without userspace needing to be involved in > > > the decision? For that matter, do we have a real-world use case lined > > > up where we can see that switching FBC to a higher plane is beneficial? > > > Even if the hardware suddenly has this capability, it isn't necessarily > > > worth adding the extra complexity to the driver if we don't expect to > > > get real-world benefit from it. > > > > > > BTW, I'm not super familiar with all the FBC-specific details, but it > > > feels like if we do go forward with this, the decision to select a > > > specific plane for FBC usage should be handled more deliberately during > > > the atomic check phase. Right now it doesn't seem like we're really > > > making a firm decision on which plane to use, and we're probably not > > > handling all the cases where the register needs to be reprogrammed > > > if/when the FBC moves from one plane to another (potentially on a > > > per-frame basis). > > > > Hi Matt, > > > > Agree with the comments about the subject line and also about your > > suggestions about some code > > changes, will update accordingly. > > > > Yes.. the logic for how to choose a plane for FBC is still open. > > > > While discussing with Uma, it was decided that we split this task into two > > where we first have this > > possibility to associate FBC to first three planes and then decide on this > > logic to select the plane > > for fbc after discussing with Ville. ( Jira VLK-19372 / VLK-21508) > > > > In this patch, "fbc" pointer is associated with first three plane implying > > those three are FBC > > capable. And because there is no selection logic there, I choose the first > > available fbc capable > > plane for FBC and the rest two to have a no_fbc_reason as "fbc enabled on > > another plane" which will > > prevent them from being configured for FBC. > > > > Ville has done refactoring to FBC code earlier and I think the current code > > flow should handle > > disable/enable/register handling etc. Ville had some comments about the > > approach I explained above, > > I have asked for some clarifications > > > > BR > > Vinod > > I also just noticed that there's a workaround (Wa_14019417088) that > requires us to always bind FBC to plane 3. > > "Plane - FBC binding is fixed to Plane 3." > > And it's marked as a permanent workaround for Xe2_LPD. So it sounds > like even though the hardware was designed to support FBC on any of the > three planes, the reality is that we can only use it on one specific > plane for this platform; moving it around will have to wait for the next > platform down the road. That w/a is a mess. Trawling the HSD confirmed my hunch that it's "plane 3" because of Windows compositor behaviour. So for us that is just nonsense. And looks like we can avoid the whole mess by simply programming FBC_CTL twice; first without the enable bit set so the plane binding gets updated, and then again with the with enable bit set. That is in fact documented as the "alternate w/a". -- Ville Syrjälä Intel
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Tue, Aug 29, 2023 at 01:34:56PM +, Govindapillai, Vinod wrote:
> On Tue, 2023-08-29 at 10:50 +0300, Ville Syrjälä wrote:
> > On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> > > In LNL onwards, FBC can be associated to the first three planes.
> > > The FBC will be enabled for first FBC capable visible plane
> > > until the userspace can select one of these FBC capable plane
> > > explicitly
> > >
> > > Bspec: 69560
> > > Signed-off-by: Vinod Govindapillai
> > > ---
> > > drivers/gpu/drm/i915/display/intel_fbc.c | 29 +++
> > > .../drm/i915/display/skl_universal_plane.c | 5 +++-
> > > drivers/gpu/drm/i915/i915_reg.h | 4 +++
> > > 3 files changed, 37 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 45e205a0f740..62f59630d410 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct
> > > intel_fbc *fbc)
> > > CHICKEN_FBC_STRIDE_MASK, val);
> > > }
> > >
> > > +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> > > +{
> > > + switch (fbc->state.plane->id) {
> > > + default:
> > > + MISSING_CASE(fbc->state.plane->id);
> > > + fallthrough;
> > > + case 0:
> > > + return DPFC_CTL_PLANE_BINDING_1;
> > > + case 1:
> > > + return DPFC_CTL_PLANE_BINDING_2;
> > > + case 2:
> > > + return DPFC_CTL_PLANE_BINDING_3;
> > > + }
> > > +}
> > > +
> > > static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > > {
> > > const struct intel_fbc_state *fbc_state = >state;
> > > @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > > if (IS_IVYBRIDGE(i915))
> > > dpfc_ctl |=
> > > DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
> > >
> > > + if (DISPLAY_VER(i915) >= 20)
> > > + dpfc_ctl |= lnl_plane_binding(fbc);
> > > +
> > > if (fbc_state->fence_id >= 0)
> > > dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
> > >
> > > @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct
> > > intel_atomic_state *state,
> > > }
> > > }
> > >
> > > + /*
> > > + * From LNL, FBC can be assigned on any plane. Until a provision
> > > is
> > > + * provided for the userspace to select a plane for FBC, lets
> > > select
> > > + * the first visible plane that is FBC capable.
> > > + */
> > > + if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&
> > > + fbc->state.plane != plane) {
> > > + plane_state->no_fbc_reason = "fbc enabled on another
> > > plane";
> > > + return 0;
> > > + }
> >
> > This isn't right. fbc->state.plane is an internal state thing
> > that chanes willy nilly, so we don't want to check it here.
> >
> > But we shouldn't need this kind of check anyway. The code
> > should already work just fine if you just do the tweak to
> > skl_plane_has_fbc((). Which plane actually gets FBC will be
> > a bit random, but I think that's fine for the moment.
>
> Currently, intel_crtc_init() creates the primary plane and other planes. Then
> the "fbc" is
> associated with the primary plane based on skl_plane_has_fbc()
>
> In Lnl, the first 3 planes are FBC capable. So my thinking has been that
> those three planes can have
> the fbc pointers.
>
> And because at the moment we don't have a logic to select which frame should
> get this FBC,
It's there (in intel_fbc_update()/__intel_fbc_enable() more or
less). It's just "first come first serve".
> I used
> the above snippet to avoid FBC being assoicated with multiple planes in a
> pipe. So the first visible
> fbc capable plane will had no_fbc_reason as NULL and the other 2 planes will
> have "fbc on another
> plane".
>
> BR
> Vinod
>
> >
> > > +
> > > plane_state->no_fbc_reason = NULL;
> > >
> > > return 0;
> > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > index 4d01c7ae4485..1291351c9941 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > > @@ -1962,7 +1962,10 @@ static bool skl_plane_has_fbc(struct
> > > drm_i915_private *dev_priv,
> > > if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
> > > return false;
> > >
> > > - return plane_id == PLANE_PRIMARY;
> > > + if (DISPLAY_VER(dev_priv) >= 20)
> > > + return plane_id <= PLANE_SPRITE1;
> > > + else
> > > + return plane_id == PLANE_PRIMARY;
> > > }
> > >
> > > static struct intel_fbc *skl_plane_fbc(struct drm_i915_private
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Tue, Aug 29, 2023 at 06:50:02AM -0700, Govindapillai, Vinod wrote:
> On Mon, 2023-08-28 at 17:16 -0700, Matt Roper wrote:
> > On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> > > In LNL onwards, FBC can be associated to the first three planes.
> >
> > The title of this patch shouldn't say "any plane" when the reality is
> > that only the first three support FBC (the upper two do not).
> >
> > > The FBC will be enabled for first FBC capable visible plane
> > > until the userspace can select one of these FBC capable plane
> > > explicitly
> >
> > Even if we add new Intel-specific uapi to select this explicitly, is any
> > userspace actually going to use it? Would it make more sense to try to
> > come up with a heuristic to guess which plane would benefit the most and
> > switch to that automatically without userspace needing to be involved in
> > the decision? For that matter, do we have a real-world use case lined
> > up where we can see that switching FBC to a higher plane is beneficial?
> > Even if the hardware suddenly has this capability, it isn't necessarily
> > worth adding the extra complexity to the driver if we don't expect to
> > get real-world benefit from it.
> >
> > BTW, I'm not super familiar with all the FBC-specific details, but it
> > feels like if we do go forward with this, the decision to select a
> > specific plane for FBC usage should be handled more deliberately during
> > the atomic check phase. Right now it doesn't seem like we're really
> > making a firm decision on which plane to use, and we're probably not
> > handling all the cases where the register needs to be reprogrammed
> > if/when the FBC moves from one plane to another (potentially on a
> > per-frame basis).
>
> Hi Matt,
>
> Agree with the comments about the subject line and also about your
> suggestions about some code
> changes, will update accordingly.
>
> Yes.. the logic for how to choose a plane for FBC is still open.
>
> While discussing with Uma, it was decided that we split this task into two
> where we first have this
> possibility to associate FBC to first three planes and then decide on this
> logic to select the plane
> for fbc after discussing with Ville. ( Jira VLK-19372 / VLK-21508)
>
> In this patch, "fbc" pointer is associated with first three plane implying
> those three are FBC
> capable. And because there is no selection logic there, I choose the first
> available fbc capable
> plane for FBC and the rest two to have a no_fbc_reason as "fbc enabled on
> another plane" which will
> prevent them from being configured for FBC.
>
> Ville has done refactoring to FBC code earlier and I think the current code
> flow should handle
> disable/enable/register handling etc. Ville had some comments about the
> approach I explained above,
> I have asked for some clarifications
>
> BR
> Vinod
I also just noticed that there's a workaround (Wa_14019417088) that
requires us to always bind FBC to plane 3.
"Plane - FBC binding is fixed to Plane 3."
And it's marked as a permanent workaround for Xe2_LPD. So it sounds
like even though the hardware was designed to support FBC on any of the
three planes, the reality is that we can only use it on one specific
plane for this platform; moving it around will have to wait for the next
platform down the road.
Matt
>
>
>
> >
> > >
> > > Bspec: 69560
> > > Signed-off-by: Vinod Govindapillai
> > > ---
> > > drivers/gpu/drm/i915/display/intel_fbc.c | 29 +++
> > > .../drm/i915/display/skl_universal_plane.c | 5 +++-
> > > drivers/gpu/drm/i915/i915_reg.h | 4 +++
> > > 3 files changed, 37 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 45e205a0f740..62f59630d410 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct
> > > intel_fbc *fbc)
> > > CHICKEN_FBC_STRIDE_MASK, val);
> > > }
> > >
> > > +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> > > +{
> > > + switch (fbc->state.plane->id) {
> > > + default:
> > > + MISSING_CASE(fbc->state.plane->id);
> > > + fallthrough;
> > > + case 0:
> > > + return DPFC_CTL_PLANE_BINDING_1;
> > > + case 1:
> > > + return DPFC_CTL_PLANE_BINDING_2;
> > > + case 2:
> > > + return DPFC_CTL_PLANE_BINDING_3;
> > > + }
> > > +}
> > > +
> > > static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > > {
> > > const struct intel_fbc_state *fbc_state = >state;
> > > @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > > if (IS_IVYBRIDGE(i915))
> > > dpfc_ctl |=
> > > DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
> > >
> > > + if
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Mon, 2023-08-28 at 17:16 -0700, Matt Roper wrote:
> On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> > In LNL onwards, FBC can be associated to the first three planes.
>
> The title of this patch shouldn't say "any plane" when the reality is
> that only the first three support FBC (the upper two do not).
>
> > The FBC will be enabled for first FBC capable visible plane
> > until the userspace can select one of these FBC capable plane
> > explicitly
>
> Even if we add new Intel-specific uapi to select this explicitly, is any
> userspace actually going to use it? Would it make more sense to try to
> come up with a heuristic to guess which plane would benefit the most and
> switch to that automatically without userspace needing to be involved in
> the decision? For that matter, do we have a real-world use case lined
> up where we can see that switching FBC to a higher plane is beneficial?
> Even if the hardware suddenly has this capability, it isn't necessarily
> worth adding the extra complexity to the driver if we don't expect to
> get real-world benefit from it.
>
> BTW, I'm not super familiar with all the FBC-specific details, but it
> feels like if we do go forward with this, the decision to select a
> specific plane for FBC usage should be handled more deliberately during
> the atomic check phase. Right now it doesn't seem like we're really
> making a firm decision on which plane to use, and we're probably not
> handling all the cases where the register needs to be reprogrammed
> if/when the FBC moves from one plane to another (potentially on a
> per-frame basis).
Hi Matt,
Agree with the comments about the subject line and also about your suggestions
about some code
changes, will update accordingly.
Yes.. the logic for how to choose a plane for FBC is still open.
While discussing with Uma, it was decided that we split this task into two
where we first have this
possibility to associate FBC to first three planes and then decide on this
logic to select the plane
for fbc after discussing with Ville. ( Jira VLK-19372 / VLK-21508)
In this patch, "fbc" pointer is associated with first three plane implying
those three are FBC
capable. And because there is no selection logic there, I choose the first
available fbc capable
plane for FBC and the rest two to have a no_fbc_reason as "fbc enabled on
another plane" which will
prevent them from being configured for FBC.
Ville has done refactoring to FBC code earlier and I think the current code
flow should handle
disable/enable/register handling etc. Ville had some comments about the
approach I explained above,
I have asked for some clarifications
BR
Vinod
>
> >
> > Bspec: 69560
> > Signed-off-by: Vinod Govindapillai
> > ---
> > drivers/gpu/drm/i915/display/intel_fbc.c | 29 +++
> > .../drm/i915/display/skl_universal_plane.c | 5 +++-
> > drivers/gpu/drm/i915/i915_reg.h | 4 +++
> > 3 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 45e205a0f740..62f59630d410 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct
> > intel_fbc *fbc)
> > CHICKEN_FBC_STRIDE_MASK, val);
> > }
> >
> > +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> > +{
> > + switch (fbc->state.plane->id) {
> > + default:
> > + MISSING_CASE(fbc->state.plane->id);
> > + fallthrough;
> > + case 0:
> > + return DPFC_CTL_PLANE_BINDING_1;
> > + case 1:
> > + return DPFC_CTL_PLANE_BINDING_2;
> > + case 2:
> > + return DPFC_CTL_PLANE_BINDING_3;
> > + }
> > +}
> > +
> > static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > {
> > const struct intel_fbc_state *fbc_state = >state;
> > @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > if (IS_IVYBRIDGE(i915))
> > dpfc_ctl |=
> > DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
> >
> > + if (DISPLAY_VER(i915) >= 20)
> > + dpfc_ctl |= lnl_plane_binding(fbc);
> > +
> > if (fbc_state->fence_id >= 0)
> > dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
> >
> > @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct
> > intel_atomic_state *state,
> > }
> > }
> >
> > + /*
> > + * From LNL, FBC can be assigned on any plane. Until a provision is
> > + * provided for the userspace to select a plane for FBC, lets select
> > + * the first visible plane that is FBC capable.
> > + */
> > + if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&
>
> Isn't the fbc->state.plane here redundant with the plane check on the
> next line (since a NULL plane wouldn't
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Tue, 2023-08-29 at 10:50 +0300, Ville Syrjälä wrote:
> On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> > In LNL onwards, FBC can be associated to the first three planes.
> > The FBC will be enabled for first FBC capable visible plane
> > until the userspace can select one of these FBC capable plane
> > explicitly
> >
> > Bspec: 69560
> > Signed-off-by: Vinod Govindapillai
> > ---
> > drivers/gpu/drm/i915/display/intel_fbc.c | 29 +++
> > .../drm/i915/display/skl_universal_plane.c | 5 +++-
> > drivers/gpu/drm/i915/i915_reg.h | 4 +++
> > 3 files changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 45e205a0f740..62f59630d410 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct
> > intel_fbc *fbc)
> > CHICKEN_FBC_STRIDE_MASK, val);
> > }
> >
> > +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> > +{
> > + switch (fbc->state.plane->id) {
> > + default:
> > + MISSING_CASE(fbc->state.plane->id);
> > + fallthrough;
> > + case 0:
> > + return DPFC_CTL_PLANE_BINDING_1;
> > + case 1:
> > + return DPFC_CTL_PLANE_BINDING_2;
> > + case 2:
> > + return DPFC_CTL_PLANE_BINDING_3;
> > + }
> > +}
> > +
> > static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > {
> > const struct intel_fbc_state *fbc_state = >state;
> > @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> > if (IS_IVYBRIDGE(i915))
> > dpfc_ctl |=
> > DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
> >
> > + if (DISPLAY_VER(i915) >= 20)
> > + dpfc_ctl |= lnl_plane_binding(fbc);
> > +
> > if (fbc_state->fence_id >= 0)
> > dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
> >
> > @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct
> > intel_atomic_state *state,
> > }
> > }
> >
> > + /*
> > + * From LNL, FBC can be assigned on any plane. Until a provision is
> > + * provided for the userspace to select a plane for FBC, lets select
> > + * the first visible plane that is FBC capable.
> > + */
> > + if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&
> > + fbc->state.plane != plane) {
> > + plane_state->no_fbc_reason = "fbc enabled on another plane";
> > + return 0;
> > + }
>
> This isn't right. fbc->state.plane is an internal state thing
> that chanes willy nilly, so we don't want to check it here.
>
> But we shouldn't need this kind of check anyway. The code
> should already work just fine if you just do the tweak to
> skl_plane_has_fbc((). Which plane actually gets FBC will be
> a bit random, but I think that's fine for the moment.
Currently, intel_crtc_init() creates the primary plane and other planes. Then
the "fbc" is
associated with the primary plane based on skl_plane_has_fbc()
In Lnl, the first 3 planes are FBC capable. So my thinking has been that those
three planes can have
the fbc pointers.
And because at the moment we don't have a logic to select which frame should
get this FBC, I used
the above snippet to avoid FBC being assoicated with multiple planes in a pipe.
So the first visible
fbc capable plane will had no_fbc_reason as NULL and the other 2 planes will
have "fbc on another
plane".
BR
Vinod
>
> > +
> > plane_state->no_fbc_reason = NULL;
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 4d01c7ae4485..1291351c9941 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1962,7 +1962,10 @@ static bool skl_plane_has_fbc(struct
> > drm_i915_private *dev_priv,
> > if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
> > return false;
> >
> > - return plane_id == PLANE_PRIMARY;
> > + if (DISPLAY_VER(dev_priv) >= 20)
> > + return plane_id <= PLANE_SPRITE1;
> > + else
> > + return plane_id == PLANE_PRIMARY;
> > }
> >
> > static struct intel_fbc *skl_plane_fbc(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index aefad14ab27a..b207774f3c33 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1327,6 +1327,10 @@
> > #define DPFC_CTL_PLANE_IVB(i9xx_plane)
> > REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB,
> > (i9xx_plane))
> > #define DPFC_CTL_FENCE_EN_IVBREG_BIT(28) /*
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> In LNL onwards, FBC can be associated to the first three planes.
> The FBC will be enabled for first FBC capable visible plane
> until the userspace can select one of these FBC capable plane
> explicitly
>
> Bspec: 69560
> Signed-off-by: Vinod Govindapillai
> ---
> drivers/gpu/drm/i915/display/intel_fbc.c | 29 +++
> .../drm/i915/display/skl_universal_plane.c| 5 +++-
> drivers/gpu/drm/i915/i915_reg.h | 4 +++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 45e205a0f740..62f59630d410 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct intel_fbc
> *fbc)
>CHICKEN_FBC_STRIDE_MASK, val);
> }
>
> +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> +{
> + switch (fbc->state.plane->id) {
> + default:
> + MISSING_CASE(fbc->state.plane->id);
> + fallthrough;
> + case 0:
> + return DPFC_CTL_PLANE_BINDING_1;
> + case 1:
> + return DPFC_CTL_PLANE_BINDING_2;
> + case 2:
> + return DPFC_CTL_PLANE_BINDING_3;
> + }
> +}
> +
> static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> {
> const struct intel_fbc_state *fbc_state = >state;
> @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> if (IS_IVYBRIDGE(i915))
> dpfc_ctl |= DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
>
> + if (DISPLAY_VER(i915) >= 20)
> + dpfc_ctl |= lnl_plane_binding(fbc);
> +
> if (fbc_state->fence_id >= 0)
> dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
>
> @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct
> intel_atomic_state *state,
> }
> }
>
> + /*
> + * From LNL, FBC can be assigned on any plane. Until a provision is
> + * provided for the userspace to select a plane for FBC, lets select
> + * the first visible plane that is FBC capable.
> + */
> + if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&
> + fbc->state.plane != plane) {
> + plane_state->no_fbc_reason = "fbc enabled on another plane";
> + return 0;
> + }
This isn't right. fbc->state.plane is an internal state thing
that chanes willy nilly, so we don't want to check it here.
But we shouldn't need this kind of check anyway. The code
should already work just fine if you just do the tweak to
skl_plane_has_fbc((). Which plane actually gets FBC will be
a bit random, but I think that's fine for the moment.
> +
> plane_state->no_fbc_reason = NULL;
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4d01c7ae4485..1291351c9941 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1962,7 +1962,10 @@ static bool skl_plane_has_fbc(struct drm_i915_private
> *dev_priv,
> if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
> return false;
>
> - return plane_id == PLANE_PRIMARY;
> + if (DISPLAY_VER(dev_priv) >= 20)
> + return plane_id <= PLANE_SPRITE1;
> + else
> + return plane_id == PLANE_PRIMARY;
> }
>
> static struct intel_fbc *skl_plane_fbc(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index aefad14ab27a..b207774f3c33 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1327,6 +1327,10 @@
> #define DPFC_CTL_PLANE_IVB(i9xx_plane)
> REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB, (i9xx_plane))
> #define DPFC_CTL_FENCE_EN_IVB REG_BIT(28) /* ivb+ */
> #define DPFC_CTL_PERSISTENT_MODE REG_BIT(25) /* g4x-snb */
> +#define DPFC_CTL_PLANE_BINDING_MASKREG_GENMASK(12, 11) /*
> XE */
> +#define DPFC_CTL_PLANE_BINDING_1
> REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 0) /* XE */
> +#define DPFC_CTL_PLANE_BINDING_2
> REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 1) /* XE */
> +#define DPFC_CTL_PLANE_BINDING_3
> REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 2) /* XE */
> #define DPFC_CTL_FALSE_COLOR REG_BIT(10) /* ivb+ */
> #define DPFC_CTL_SR_EN REG_BIT(10) /* g4x only */
> #define DPFC_CTL_SR_EXIT_DIS REG_BIT(9) /* g4x only
> */
> --
> 2.34.1
--
Ville Syrjälä
Intel
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Mon, Aug 28, 2023 at 09:20:34AM +0300, Vinod Govindapillai wrote:
> In LNL onwards, FBC can be associated to the first three planes.
The title of this patch shouldn't say "any plane" when the reality is
that only the first three support FBC (the upper two do not).
> The FBC will be enabled for first FBC capable visible plane
> until the userspace can select one of these FBC capable plane
> explicitly
Even if we add new Intel-specific uapi to select this explicitly, is any
userspace actually going to use it? Would it make more sense to try to
come up with a heuristic to guess which plane would benefit the most and
switch to that automatically without userspace needing to be involved in
the decision? For that matter, do we have a real-world use case lined
up where we can see that switching FBC to a higher plane is beneficial?
Even if the hardware suddenly has this capability, it isn't necessarily
worth adding the extra complexity to the driver if we don't expect to
get real-world benefit from it.
BTW, I'm not super familiar with all the FBC-specific details, but it
feels like if we do go forward with this, the decision to select a
specific plane for FBC usage should be handled more deliberately during
the atomic check phase. Right now it doesn't seem like we're really
making a firm decision on which plane to use, and we're probably not
handling all the cases where the register needs to be reprogrammed
if/when the FBC moves from one plane to another (potentially on a
per-frame basis).
>
> Bspec: 69560
> Signed-off-by: Vinod Govindapillai
> ---
> drivers/gpu/drm/i915/display/intel_fbc.c | 29 +++
> .../drm/i915/display/skl_universal_plane.c| 5 +++-
> drivers/gpu/drm/i915/i915_reg.h | 4 +++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 45e205a0f740..62f59630d410 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -649,6 +649,21 @@ static void skl_fbc_program_cfb_stride(struct intel_fbc
> *fbc)
>CHICKEN_FBC_STRIDE_MASK, val);
> }
>
> +static u32 lnl_plane_binding(struct intel_fbc *fbc)
> +{
> + switch (fbc->state.plane->id) {
> + default:
> + MISSING_CASE(fbc->state.plane->id);
> + fallthrough;
> + case 0:
> + return DPFC_CTL_PLANE_BINDING_1;
> + case 1:
> + return DPFC_CTL_PLANE_BINDING_2;
> + case 2:
> + return DPFC_CTL_PLANE_BINDING_3;
> + }
> +}
> +
> static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> {
> const struct intel_fbc_state *fbc_state = >state;
> @@ -660,6 +675,9 @@ static u32 ivb_dpfc_ctl(struct intel_fbc *fbc)
> if (IS_IVYBRIDGE(i915))
> dpfc_ctl |= DPFC_CTL_PLANE_IVB(fbc_state->plane->i9xx_plane);
>
> + if (DISPLAY_VER(i915) >= 20)
> + dpfc_ctl |= lnl_plane_binding(fbc);
> +
> if (fbc_state->fence_id >= 0)
> dpfc_ctl |= DPFC_CTL_FENCE_EN_IVB;
>
> @@ -1250,6 +1268,17 @@ static int intel_fbc_check_plane(struct
> intel_atomic_state *state,
> }
> }
>
> + /*
> + * From LNL, FBC can be assigned on any plane. Until a provision is
> + * provided for the userspace to select a plane for FBC, lets select
> + * the first visible plane that is FBC capable.
> + */
> + if (DISPLAY_VER(i915) >= 20 && fbc->state.plane &&
Isn't the fbc->state.plane here redundant with the plane check on the
next line (since a NULL plane wouldn't match there either)?
> + fbc->state.plane != plane) {
> + plane_state->no_fbc_reason = "fbc enabled on another plane";
If you set this here...
> + return 0;
> + }
> +
> plane_state->no_fbc_reason = NULL;
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4d01c7ae4485..1291351c9941 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1962,7 +1962,10 @@ static bool skl_plane_has_fbc(struct drm_i915_private
> *dev_priv,
> if ((DISPLAY_RUNTIME_INFO(dev_priv)->fbc_mask & BIT(fbc_id)) == 0)
> return false;
>
> - return plane_id == PLANE_PRIMARY;
> + if (DISPLAY_VER(dev_priv) >= 20)
> + return plane_id <= PLANE_SPRITE1;
> + else
> + return plane_id == PLANE_PRIMARY;
...and also point all three FBC-capable planes at the pipe's FBC
structure, then won't intel_fbc_update() see the non-NULL reason and try
to turn off the pipe's FBC (even though it's being used on a different
plane)?
Matt
> }
>
> static struct intel_fbc *skl_plane_fbc(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Mon, 2023-08-28 at 12:00 +0300, Jani Nikula wrote: > On Mon, 28 Aug 2023, Vinod Govindapillai > wrote: > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index aefad14ab27a..b207774f3c33 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1327,6 +1327,10 @@ > > #define DPFC_CTL_PLANE_IVB(i9xx_plane) > > REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB, > > (i9xx_plane)) > > #define DPFC_CTL_FENCE_EN_IVBREG_BIT(28) /* ivb+ > > */ > > #define DPFC_CTL_PERSISTENT_MODE REG_BIT(25) /* g4x-snb */ > > +#define DPFC_CTL_PLANE_BINDING_MASK REG_GENMASK(12, 11) /* XE */ > > +#define DPFC_CTL_PLANE_BINDING_1 > > REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 0) > > /* XE */ > > +#define DPFC_CTL_PLANE_BINDING_2 > > REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 1) > > /* XE */ > > +#define DPFC_CTL_PLANE_BINDING_3 > > REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 2) > > /* XE */ > > What's with the /* XE */ comments? Forgot to update that! I will add "lnl+" similar to lines above. BR Vinod > > BR, > Jani. > >
Re: [Intel-gfx] [Intel-xe] [PATCH 3/4] drm/i915/lnl: support FBC on any plane
On Mon, 28 Aug 2023, Vinod Govindapillai wrote: > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index aefad14ab27a..b207774f3c33 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1327,6 +1327,10 @@ > #define DPFC_CTL_PLANE_IVB(i9xx_plane) > REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_IVB, (i9xx_plane)) > #define DPFC_CTL_FENCE_EN_IVB REG_BIT(28) /* ivb+ */ > #define DPFC_CTL_PERSISTENT_MODE REG_BIT(25) /* g4x-snb */ > +#define DPFC_CTL_PLANE_BINDING_MASKREG_GENMASK(12, 11) /* > XE */ > +#define DPFC_CTL_PLANE_BINDING_1 > REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 0) /* XE */ > +#define DPFC_CTL_PLANE_BINDING_2 > REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 1) /* XE */ > +#define DPFC_CTL_PLANE_BINDING_3 > REG_FIELD_PREP(DPFC_CTL_PLANE_BINDING_MASK, 2) /* XE */ What's with the /* XE */ comments? BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
