Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Pandiyan, Dhinakaran]

On Wed, 2018-01-24 at 17:16 +0200, Ville Syrjälä wrote:
> On Wed, Jan 24, 2018 at 04:16:19AM +, Pandiyan, Dhinakaran wrote:
> > On Tue, 2018-01-23 at 20:33 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä 
> > > 
> > > Replace the ad-hoc plane indexing scheme used by the frontbuffer
> > > tracking with enum plane_id.
> > > 
> > > The old video overlay not being part of the plane_id namespace
> > > will just be given the high bit.
> > 
> > I'm curious why the bit corresponding to PLANE_SPRITE0 is not re-used
> > for the overlay. From a cursory read seems like platforms with overlays
> > and sprites are mutually exclusive.
> 
> Currently yes, but maybe not in the future. I still have dreams of
> exposing all the planes on these platforms, which would mean having
> at least one sprite plane that's not the video overlay. I haven't
> really figured out how to make it all work out nicely with enum
> plane_id though as that's a per-pipe thing and the planes on the
> old hw are not necessarily tied to a single pipe. Perhaps one
> option would be to assign the plane ids globally, ie. PRIMARY==A,
> PLANE1==B, PLANE2==C, etc. Not sure if that would actually work out
> though.
> 
> > 
> > Related question, how different is this overlay from the sprite planes?
> 
> Very different. I would like to expose it as a drm_plane eventually,
> but there's a bit of actual work involved to convert the overlay
> register updates to use mmio, and to handle the render cache
> reconfiguration (which still involves stuffing MI commands to the
> ring) in a decent way.
> 
> I do have a basic mmio conversion sitting on some branch somewhere,

I'm interested in seeing how this is done, is this in your public repo?
And thanks for the explanation.

-DK

> but I don't think I got it to look as nice as I'd like. Plus it's
> probably bitrotted enough by now that it needs some rework anyway.
> 
> > 
> > 
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  | 11 +++
> > >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> > >  drivers/gpu/drm/i915/intel_fbc.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
> > >  4 files changed, 9 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8333692dac5a..bd545b1c9546 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
> > >   *
> > >   * We have one bit per pipe and per scanout plane type.
> > >   */
> > > -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
> > >  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> > > -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> > > - (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > > -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> > > - (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > > -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> > > - (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > > +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> > > + (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> > > - (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + 
> > > (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > > + (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + 
> > > INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> > >   (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index d585ce4c8732..3cc35add362f 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13168,7 +13168,7 @@ intel_primary_plane_create(struct 
> > > drm_i915_private *dev_priv, enum pipe pipe)
> > >   else
> > >   primary->i9xx_plane = (enum i9xx_plane_id) pipe;
> > >   primary->id = PLANE_PRIMARY;
> > > - primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > + primary->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, primary->id);
> > >   primary->check_plane = intel_check_primary_plane;
> > >  
> > >   if (INTEL_GEN(dev_priv) >= 9) {
> > > @@ -13289,7 +13289,7 @@ intel_cursor_plane_create(struct drm_i915_private 
> > > *dev_priv,
> > >   cursor->pipe = pipe;
> > >   cursor->i9xx_plane = (enum i9xx_plane_id) pipe;
> > >   cursor->id = PLANE_CURSOR;
> > > - cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> > > + cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id);
> > >  
> > >   if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
> > >   cursor->update_plane = i845_update_cursor;
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c 
> > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 9dc2b8b5f2db..a8a8a80497a8 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c

Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Ville Syrjälä]

On Wed, Jan 24, 2018 at 07:36:01PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 23, 2018 at 09:11:23PM +, Chris Wilson wrote:
> > Quoting Ville Syrjala (2018-01-23 18:33:43)
> > > From: Ville Syrjälä 
> > > 
> > > Replace the ad-hoc plane indexing scheme used by the frontbuffer
> > > tracking with enum plane_id.
> > > 
> > > The old video overlay not being part of the plane_id namespace
> > > will just be given the high bit.
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  | 11 +++
> > >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> > >  drivers/gpu/drm/i915/intel_fbc.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
> > >  4 files changed, 9 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8333692dac5a..bd545b1c9546 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
> > >   *
> > >   * We have one bit per pipe and per scanout plane type.
> > >   */
> > > -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
> > >  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> > 
> > I would feel safer with a BUILD_BUG_ON to catch plane_id overflowing
> > INTEL_FRONTBUFFER_BITS_PER_PIPE (-1 for the overlay reservation).
> 
> Actaully we'll be going past that BITS_PER_PIPE-1 limit real soon
> now, but the BITS_PER_PIPE will be sufficient for some time I think.
> So I think I'd just rather ignore the overlap between the plane_id
> and the overlay because that's not going happen on actual hardware.
> 
> But I think asserting that BITS_PER_PIPE is sufficient is a good idea.
> Just need to figure out where to put it. There's no init function
> or anything like that in the frontbuffer tracking code, so that's
> not going to work. I guess one option would be to include the
> assert in the INTEL_FRONTBUFFER() macro itself.

Pushed this as is to dinq. Thanks for the review.

I'll send that BUILD_BUG_ON() as a followup.

> 
> > 
> > > -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> > > -   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > > -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> > > -   (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > > -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> > > -   (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > > +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> > > +   (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> > > -   (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + 
> > > (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > > +   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + 
> > > INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> > > (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > 
> > The conversion looks straightforward nevertheless, and indeed no reason
> > to have move than one indexing id for a plane.
> > Reviewed-by: Chris Wilson 
> > -Chris
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Ville Syrjälä]

On Tue, Jan 23, 2018 at 09:11:23PM +, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-01-23 18:33:43)
> > From: Ville Syrjälä 
> > 
> > Replace the ad-hoc plane indexing scheme used by the frontbuffer
> > tracking with enum plane_id.
> > 
> > The old video overlay not being part of the plane_id namespace
> > will just be given the high bit.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 11 +++
> >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> >  drivers/gpu/drm/i915/intel_fbc.c |  2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
> >  4 files changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 8333692dac5a..bd545b1c9546 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
> >   *
> >   * We have one bit per pipe and per scanout plane type.
> >   */
> > -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
> >  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> 
> I would feel safer with a BUILD_BUG_ON to catch plane_id overflowing
> INTEL_FRONTBUFFER_BITS_PER_PIPE (-1 for the overlay reservation).

Actaully we'll be going past that BITS_PER_PIPE-1 limit real soon
now, but the BITS_PER_PIPE will be sufficient for some time I think.
So I think I'd just rather ignore the overlap between the plane_id
and the overlay because that's not going happen on actual hardware.

But I think asserting that BITS_PER_PIPE is sufficient is a good idea.
Just need to figure out where to put it. There's no init function
or anything like that in the frontbuffer tracking code, so that's
not going to work. I guess one option would be to include the
assert in the INTEL_FRONTBUFFER() macro itself.

> 
> > -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> > -   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> > -   (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> > -   (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> > +   (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> >  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> > -   (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + 
> > (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > +   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + 
> > INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> > (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> 
> The conversion looks straightforward nevertheless, and indeed no reason
> to have move than one indexing id for a plane.
> Reviewed-by: Chris Wilson 
> -Chris

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Ville Syrjälä]

On Wed, Jan 24, 2018 at 04:16:19AM +, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-01-23 at 20:33 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Replace the ad-hoc plane indexing scheme used by the frontbuffer
> > tracking with enum plane_id.
> > 
> > The old video overlay not being part of the plane_id namespace
> > will just be given the high bit.
> 
> I'm curious why the bit corresponding to PLANE_SPRITE0 is not re-used
> for the overlay. From a cursory read seems like platforms with overlays
> and sprites are mutually exclusive.

Currently yes, but maybe not in the future. I still have dreams of
exposing all the planes on these platforms, which would mean having
at least one sprite plane that's not the video overlay. I haven't
really figured out how to make it all work out nicely with enum
plane_id though as that's a per-pipe thing and the planes on the
old hw are not necessarily tied to a single pipe. Perhaps one
option would be to assign the plane ids globally, ie. PRIMARY==A,
PLANE1==B, PLANE2==C, etc. Not sure if that would actually work out
though.

> 
> Related question, how different is this overlay from the sprite planes?

Very different. I would like to expose it as a drm_plane eventually,
but there's a bit of actual work involved to convert the overlay
register updates to use mmio, and to handle the render cache
reconfiguration (which still involves stuffing MI commands to the
ring) in a decent way.

I do have a basic mmio conversion sitting on some branch somewhere,
but I don't think I got it to look as nice as I'd like. Plus it's
probably bitrotted enough by now that it needs some rework anyway.

> 
> 
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  | 11 +++
> >  drivers/gpu/drm/i915/intel_display.c |  4 ++--
> >  drivers/gpu/drm/i915/intel_fbc.c |  2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
> >  4 files changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 8333692dac5a..bd545b1c9546 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
> >   *
> >   * We have one bit per pipe and per scanout plane type.
> >   */
> > -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
> >  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> > -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> > -   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> > -   (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> > -   (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> > +   (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> >  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> > -   (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + 
> > (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> > +   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + 
> > INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> > (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index d585ce4c8732..3cc35add362f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13168,7 +13168,7 @@ intel_primary_plane_create(struct drm_i915_private 
> > *dev_priv, enum pipe pipe)
> > else
> > primary->i9xx_plane = (enum i9xx_plane_id) pipe;
> > primary->id = PLANE_PRIMARY;
> > -   primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > +   primary->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, primary->id);
> > primary->check_plane = intel_check_primary_plane;
> >  
> > if (INTEL_GEN(dev_priv) >= 9) {
> > @@ -13289,7 +13289,7 @@ intel_cursor_plane_create(struct drm_i915_private 
> > *dev_priv,
> > cursor->pipe = pipe;
> > cursor->i9xx_plane = (enum i9xx_plane_id) pipe;
> > cursor->id = PLANE_CURSOR;
> > -   cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> > +   cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id);
> >  
> > if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
> > cursor->update_plane = i845_update_cursor;
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c 
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index 9dc2b8b5f2db..a8a8a80497a8 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1373,7 +1373,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> >  
> > for_each_pipe(dev_priv, pipe) {
> > fbc->possible_framebuffer_bits |=
> > -   INTEL_FRONTBUFFER_PRIMARY(pipe);
> > +   INTEL_FRONTBUFFER(pipe, 

Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Pandiyan, Dhinakaran]

On Tue, 2018-01-23 at 20:33 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Replace the ad-hoc plane indexing scheme used by the frontbuffer
> tracking with enum plane_id.
> 
> The old video overlay not being part of the plane_id namespace
> will just be given the high bit.

I'm curious why the bit corresponding to PLANE_SPRITE0 is not re-used
for the overlay. From a cursory read seems like platforms with overlays
and sprites are mutually exclusive.

Related question, how different is this overlay from the sprite planes?


> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 11 +++
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  drivers/gpu/drm/i915/intel_fbc.c |  2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
>  4 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8333692dac5a..bd545b1c9546 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
>   *
>   * We have one bit per pipe and per scanout plane type.
>   */
> -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
>  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> - (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> - (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> - (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> + (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> - (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + 
> (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> + (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + 
> INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
>   (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index d585ce4c8732..3cc35add362f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13168,7 +13168,7 @@ intel_primary_plane_create(struct drm_i915_private 
> *dev_priv, enum pipe pipe)
>   else
>   primary->i9xx_plane = (enum i9xx_plane_id) pipe;
>   primary->id = PLANE_PRIMARY;
> - primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> + primary->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, primary->id);
>   primary->check_plane = intel_check_primary_plane;
>  
>   if (INTEL_GEN(dev_priv) >= 9) {
> @@ -13289,7 +13289,7 @@ intel_cursor_plane_create(struct drm_i915_private 
> *dev_priv,
>   cursor->pipe = pipe;
>   cursor->i9xx_plane = (enum i9xx_plane_id) pipe;
>   cursor->id = PLANE_CURSOR;
> - cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> + cursor->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, cursor->id);
>  
>   if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
>   cursor->update_plane = i845_update_cursor;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c 
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 9dc2b8b5f2db..a8a8a80497a8 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1373,7 +1373,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  
>   for_each_pipe(dev_priv, pipe) {
>   fbc->possible_framebuffer_bits |=
> - INTEL_FRONTBUFFER_PRIMARY(pipe);
> + INTEL_FRONTBUFFER(pipe, PLANE_PRIMARY);
>  
>   if (fbc_on_pipe_a_only(dev_priv))
>   break;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index a92c748ca8ab..18ef0392362e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -345,6 +345,8 @@ skl_plane_get_hw_state(struct intel_plane *plane)
>   return ret;
>  }
>  
> +
> +
>  static void
>  chv_update_csc(struct intel_plane *plane, uint32_t format)
>  {
> @@ -1427,7 +1429,7 @@ intel_sprite_plane_create(struct drm_i915_private 
> *dev_priv,
>   intel_plane->pipe = pipe;
>   intel_plane->i9xx_plane = plane;
>   intel_plane->id = PLANE_SPRITE0 + plane;
> - intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
> + intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER(pipe, intel_plane->id);
>   intel_plane->check_plane = intel_check_sprite_plane;
>  
>   possible_crtcs = (1 << pipe);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Chris Wilson]

Quoting Ville Syrjala (2018-01-23 18:33:43)
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index a92c748ca8ab..18ef0392362e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -345,6 +345,8 @@ skl_plane_get_hw_state(struct intel_plane *plane)
> return ret;
>  }
>  
> +
> +
>  static void
>  chv_update_csc(struct intel_plane *plane, uint32_t format)
>  {

And just in case this unwanted chunk managed to escape notice...
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Use enum plane_id for frontbuffer tracking

[Chris Wilson]

Quoting Ville Syrjala (2018-01-23 18:33:43)
> From: Ville Syrjälä 
> 
> Replace the ad-hoc plane indexing scheme used by the frontbuffer
> tracking with enum plane_id.
> 
> The old video overlay not being part of the plane_id namespace
> will just be given the high bit.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 11 +++
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  drivers/gpu/drm/i915/intel_fbc.c |  2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  4 +++-
>  4 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8333692dac5a..bd545b1c9546 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2404,16 +2404,11 @@ enum hdmi_force_audio {
>   *
>   * We have one bit per pipe and per scanout plane type.
>   */
> -#define INTEL_MAX_SPRITE_BITS_PER_PIPE 5
>  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8

I would feel safer with a BUILD_BUG_ON to catch plane_id overflowing
INTEL_FRONTBUFFER_BITS_PER_PIPE (-1 for the overlay reservation).

> -#define INTEL_FRONTBUFFER_PRIMARY(pipe) \
> -   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> -#define INTEL_FRONTBUFFER_CURSOR(pipe) \
> -   (1 << (1 + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> -#define INTEL_FRONTBUFFER_SPRITE(pipe, plane) \
> -   (1 << (2 + plane + (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> +#define INTEL_FRONTBUFFER(pipe, plane_id) \
> +   (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> -   (1 << (2 + INTEL_MAX_SPRITE_BITS_PER_PIPE + 
> (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe
> +   (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + 
> INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))

The conversion looks straightforward nevertheless, and indeed no reason
to have move than one indexing id for a plane.
Reviewed-by: Chris Wilson 
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx