| Message ID | 20170726180802.23817-6-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ben, On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: > + } else if (INTEL_GEN(dev_priv) >= 9) { > intel_primary_formats = skl_primary_formats; > num_formats = ARRAY_SIZE(skl_primary_formats); > - modifiers = skl_format_modifiers; > + if (pipe >= PIPE_C) > + modifiers = skl_format_modifiers_ccs; > + else > + modifiers = skl_format_modifiers_noccs; Shouldn't that be (pipe < PIPE_C)? Cheers, Daniel
Hi, On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: > +static const uint64_t skl_plane_format_modifiers_noccs[] = { > + I915_FORMAT_MOD_Yf_TILED, > + I915_FORMAT_MOD_Y_TILED, > + I915_FORMAT_MOD_X_TILED, > + DRM_FORMAT_MOD_LINEAR, > + DRM_FORMAT_MOD_INVALID > +}; > + > static const uint64_t skl_plane_format_modifiers[] = { > + I915_FORMAT_MOD_Yf_TILED_CCS, > + I915_FORMAT_MOD_Y_TILED_CCS, > I915_FORMAT_MOD_Yf_TILED, > I915_FORMAT_MOD_Y_TILED, > I915_FORMAT_MOD_X_TILED, This is also missing the relevant hunk in format_mod_supported. > @@ -1234,6 +1244,19 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, > plane_formats = skl_plane_formats; > num_plane_formats = ARRAY_SIZE(skl_plane_formats); > modifiers = skl_plane_format_modifiers; > + } else if (INTEL_GEN(dev_priv) >= 9) { > + intel_plane->can_scale = true; > + state->scaler_id = -1; > + > + intel_plane->update_plane = skl_update_plane; > + intel_plane->disable_plane = skl_disable_plane; > + > + plane_formats = skl_plane_formats; > + num_plane_formats = ARRAY_SIZE(skl_plane_formats); > + if (pipe >= PIPE_C) > + modifiers = skl_plane_format_modifiers_noccs; > + else > + modifiers = skl_plane_format_modifiers; This explains the inconsistency. Cheers, Daniel
On 17-07-29 13:53:10, Daniel Stone wrote: >Hi Ben, > >On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: >> + } else if (INTEL_GEN(dev_priv) >= 9) { >> intel_primary_formats = skl_primary_formats; >> num_formats = ARRAY_SIZE(skl_primary_formats); >> - modifiers = skl_format_modifiers; >> + if (pipe >= PIPE_C) >> + modifiers = skl_format_modifiers_ccs; >> + else >> + modifiers = skl_format_modifiers_noccs; > >Shouldn't that be (pipe < PIPE_C)? > >Cheers, >Daniel Yep. It was wrong in v7 too :/. I have it fixed locally.
On Sat, Jul 29, 2017 at 09:25:50AM -0700, Ben Widawsky wrote: > On 17-07-29 13:53:10, Daniel Stone wrote: > > Hi Ben, > > > > On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: > > > + } else if (INTEL_GEN(dev_priv) >= 9) { > > > intel_primary_formats = skl_primary_formats; > > > num_formats = ARRAY_SIZE(skl_primary_formats); > > > - modifiers = skl_format_modifiers; > > > + if (pipe >= PIPE_C) > > > + modifiers = skl_format_modifiers_ccs; > > > + else > > > + modifiers = skl_format_modifiers_noccs; > > > > Shouldn't that be (pipe < PIPE_C)? > > > > Cheers, > > Daniel > > Yep. It was wrong in v7 too :/. I have it fixed locally. Shouldn't the igt catch this, or does it not try to exercise all the plane/crtc combos there are? -Daniel
On 17-07-31 10:29:55, Daniel Vetter wrote: >On Sat, Jul 29, 2017 at 09:25:50AM -0700, Ben Widawsky wrote: >> On 17-07-29 13:53:10, Daniel Stone wrote: >> > Hi Ben, >> > >> > On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: >> > > + } else if (INTEL_GEN(dev_priv) >= 9) { >> > > intel_primary_formats = skl_primary_formats; >> > > num_formats = ARRAY_SIZE(skl_primary_formats); >> > > - modifiers = skl_format_modifiers; >> > > + if (pipe >= PIPE_C) >> > > + modifiers = skl_format_modifiers_ccs; >> > > + else >> > > + modifiers = skl_format_modifiers_noccs; >> > >> > Shouldn't that be (pipe < PIPE_C)? >> > >> > Cheers, >> > Daniel >> >> Yep. It was wrong in v7 too :/. I have it fixed locally. > >Shouldn't the igt catch this, or does it not try to exercise all the >plane/crtc combos there are? >-Daniel I don't know whether or not IGT should have caught this, but it wouldn't have because I had been sending these without Ville's patches, so my patches alone don't even build (since his patches defined the modifiers). >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
On Tue, Aug 01, 2017 at 09:14:50AM -0700, Ben Widawsky wrote: > On 17-07-31 10:29:55, Daniel Vetter wrote: > > On Sat, Jul 29, 2017 at 09:25:50AM -0700, Ben Widawsky wrote: > > > On 17-07-29 13:53:10, Daniel Stone wrote: > > > > Hi Ben, > > > > > > > > On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: > > > > > + } else if (INTEL_GEN(dev_priv) >= 9) { > > > > > intel_primary_formats = skl_primary_formats; > > > > > num_formats = ARRAY_SIZE(skl_primary_formats); > > > > > - modifiers = skl_format_modifiers; > > > > > + if (pipe >= PIPE_C) > > > > > + modifiers = skl_format_modifiers_ccs; > > > > > + else > > > > > + modifiers = skl_format_modifiers_noccs; > > > > > > > > Shouldn't that be (pipe < PIPE_C)? > > > > > > > > Cheers, > > > > Daniel > > > > > > Yep. It was wrong in v7 too :/. I have it fixed locally. > > > > Shouldn't the igt catch this, or does it not try to exercise all the > > plane/crtc combos there are? > > -Daniel > > I don't know whether or not IGT should have caught this, but it wouldn't have > because I had been sending these without Ville's patches, so my patches alone > don't even build (since his patches defined the modifiers). You can run igt testcases locally. I expect you to do that, at least for the stuff you're touching. Does this mean there's no igts for this at all? -Daniel
On 17-08-02 12:14:15, Daniel Vetter wrote: >On Tue, Aug 01, 2017 at 09:14:50AM -0700, Ben Widawsky wrote: >> On 17-07-31 10:29:55, Daniel Vetter wrote: >> > On Sat, Jul 29, 2017 at 09:25:50AM -0700, Ben Widawsky wrote: >> > > On 17-07-29 13:53:10, Daniel Stone wrote: >> > > > Hi Ben, >> > > > >> > > > On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: >> > > > > + } else if (INTEL_GEN(dev_priv) >= 9) { >> > > > > intel_primary_formats = skl_primary_formats; >> > > > > num_formats = ARRAY_SIZE(skl_primary_formats); >> > > > > - modifiers = skl_format_modifiers; >> > > > > + if (pipe >= PIPE_C) >> > > > > + modifiers = skl_format_modifiers_ccs; >> > > > > + else >> > > > > + modifiers = skl_format_modifiers_noccs; >> > > > >> > > > Shouldn't that be (pipe < PIPE_C)? >> > > > >> > > > Cheers, >> > > > Daniel >> > > >> > > Yep. It was wrong in v7 too :/. I have it fixed locally. >> > >> > Shouldn't the igt catch this, or does it not try to exercise all the >> > plane/crtc combos there are? >> > -Daniel >> >> I don't know whether or not IGT should have caught this, but it wouldn't have >> because I had been sending these without Ville's patches, so my patches alone >> don't even build (since his patches defined the modifiers). > >You can run igt testcases locally. I expect you to do that, at least for >the stuff you're touching. > >Does this mean there's no igts for this at all? >-Daniel I haven't been running IGT locally, so I don't know if they touch this path. We've done all testing so far with kmscube, X, and weston; then measure the overall bandwidth. In this case, I'd stop testing these since Jason/Daniel took over my other patches. I don't even know how to locally make sure I get a display on pipe C. You could easily make an IGT make sure that you get back the right list of modifiers for a given pipe on a given GEN. Probably should do one. >-- >Daniel Vetter >Software Engineer, Intel Corporation >http://blog.ffwll.ch
On Wed, Aug 2, 2017 at 5:43 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > On 17-08-02 12:14:15, Daniel Vetter wrote: >> >> On Tue, Aug 01, 2017 at 09:14:50AM -0700, Ben Widawsky wrote: >>> >>> On 17-07-31 10:29:55, Daniel Vetter wrote: >>> > On Sat, Jul 29, 2017 at 09:25:50AM -0700, Ben Widawsky wrote: >>> > > On 17-07-29 13:53:10, Daniel Stone wrote: >>> > > > Hi Ben, >>> > > > >>> > > > On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: >>> > > > > + } else if (INTEL_GEN(dev_priv) >= 9) { >>> > > > > intel_primary_formats = skl_primary_formats; >>> > > > > num_formats = ARRAY_SIZE(skl_primary_formats); >>> > > > > - modifiers = skl_format_modifiers; >>> > > > > + if (pipe >= PIPE_C) >>> > > > > + modifiers = skl_format_modifiers_ccs; >>> > > > > + else >>> > > > > + modifiers = skl_format_modifiers_noccs; >>> > > > >>> > > > Shouldn't that be (pipe < PIPE_C)? >>> > > > >>> > > > Cheers, >>> > > > Daniel >>> > > >>> > > Yep. It was wrong in v7 too :/. I have it fixed locally. >>> > >>> > Shouldn't the igt catch this, or does it not try to exercise all the >>> > plane/crtc combos there are? >>> > -Daniel >>> >>> I don't know whether or not IGT should have caught this, but it wouldn't >>> have >>> because I had been sending these without Ville's patches, so my patches >>> alone >>> don't even build (since his patches defined the modifiers). >> >> >> You can run igt testcases locally. I expect you to do that, at least for >> the stuff you're touching. >> >> Does this mean there's no igts for this at all? >> -Daniel > > > I haven't been running IGT locally, so I don't know if they touch this path. > We've done all testing so far with kmscube, X, and weston; then measure the > overall bandwidth. In this case, I'd stop testing these since Jason/Daniel > took > over my other patches. I don't even know how to locally make sure I get a > display on pipe C. > > You could easily make an IGT make sure that you get back the right list of > modifiers for a given pipe on a given GEN. Probably should do one. That's not a terribly interesting testcase (we have none of this kind I think). But I thought there was a CCS igt that created some simple compressed buffers and tried to display them, and then checked it all looked good using crcs, i.e. an actual functional testcase, and that should have caught the mislisten on pipe C (if it bothered to test all planes that expose the CCS modifier). Iirc Ville had that. We do need that for merging the kernel side. Thanks, Daniel
On 17-08-03 10:08:51, Daniel Vetter wrote: >On Wed, Aug 2, 2017 at 5:43 PM, Ben Widawsky <ben@bwidawsk.net> wrote: >> On 17-08-02 12:14:15, Daniel Vetter wrote: >>> >>> On Tue, Aug 01, 2017 at 09:14:50AM -0700, Ben Widawsky wrote: >>>> >>>> On 17-07-31 10:29:55, Daniel Vetter wrote: >>>> > On Sat, Jul 29, 2017 at 09:25:50AM -0700, Ben Widawsky wrote: >>>> > > On 17-07-29 13:53:10, Daniel Stone wrote: >>>> > > > Hi Ben, >>>> > > > >>>> > > > On 26 July 2017 at 19:08, Ben Widawsky <ben@bwidawsk.net> wrote: >>>> > > > > + } else if (INTEL_GEN(dev_priv) >= 9) { >>>> > > > > intel_primary_formats = skl_primary_formats; >>>> > > > > num_formats = ARRAY_SIZE(skl_primary_formats); >>>> > > > > - modifiers = skl_format_modifiers; >>>> > > > > + if (pipe >= PIPE_C) >>>> > > > > + modifiers = skl_format_modifiers_ccs; >>>> > > > > + else >>>> > > > > + modifiers = skl_format_modifiers_noccs; >>>> > > > >>>> > > > Shouldn't that be (pipe < PIPE_C)? >>>> > > > >>>> > > > Cheers, >>>> > > > Daniel >>>> > > >>>> > > Yep. It was wrong in v7 too :/. I have it fixed locally. >>>> > >>>> > Shouldn't the igt catch this, or does it not try to exercise all the >>>> > plane/crtc combos there are? >>>> > -Daniel >>>> >>>> I don't know whether or not IGT should have caught this, but it wouldn't >>>> have >>>> because I had been sending these without Ville's patches, so my patches >>>> alone >>>> don't even build (since his patches defined the modifiers). >>> >>> >>> You can run igt testcases locally. I expect you to do that, at least for >>> the stuff you're touching. >>> >>> Does this mean there's no igts for this at all? >>> -Daniel >> >> >> I haven't been running IGT locally, so I don't know if they touch this path. >> We've done all testing so far with kmscube, X, and weston; then measure the >> overall bandwidth. In this case, I'd stop testing these since Jason/Daniel >> took >> over my other patches. I don't even know how to locally make sure I get a >> display on pipe C. >> >> You could easily make an IGT make sure that you get back the right list of >> modifiers for a given pipe on a given GEN. Probably should do one. > >That's not a terribly interesting testcase (we have none of this kind >I think). But I thought there was a CCS igt that created some simple >compressed buffers and tried to display them, and then checked it all >looked good using crcs, i.e. an actual functional testcase, and that >should have caught the mislisten on pipe C (if it bothered to test all >planes that expose the CCS modifier). Iirc Ville had that. We do need >that for merging the kernel side. > >Thanks, Daniel Yeah, we did have that already - just nothing for blobifiers specifically. It sounds like igt_ccs is broken at the moment. >-- >Daniel Vetter >Software Engineer, Intel Corporation >+41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 40cb02c0ac2b..fc990189c97d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -88,7 +88,17 @@ static const uint32_t skl_primary_formats[] = { DRM_FORMAT_VYUY, }; -static const uint64_t skl_format_modifiers[] = { +static const uint64_t skl_format_modifiers_noccs[] = { + I915_FORMAT_MOD_Yf_TILED, + I915_FORMAT_MOD_Y_TILED, + I915_FORMAT_MOD_X_TILED, + DRM_FORMAT_MOD_LINEAR, + DRM_FORMAT_MOD_INVALID +}; + +static const uint64_t skl_format_modifiers_ccs[] = { + I915_FORMAT_MOD_Yf_TILED_CCS, + I915_FORMAT_MOD_Y_TILED_CCS, I915_FORMAT_MOD_Yf_TILED, I915_FORMAT_MOD_Y_TILED, I915_FORMAT_MOD_X_TILED, @@ -12899,6 +12909,10 @@ static bool skl_mod_supported(uint32_t format, uint64_t modifier) case DRM_FORMAT_XBGR8888: case DRM_FORMAT_ARGB8888: case DRM_FORMAT_ABGR8888: + if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS || + modifier == I915_FORMAT_MOD_Y_TILED_CCS) + return true; + /* fall through */ case DRM_FORMAT_RGB565: case DRM_FORMAT_XRGB2101010: case DRM_FORMAT_XBGR2101010: @@ -13144,10 +13158,20 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); primary->check_plane = intel_check_primary_plane; - if (INTEL_GEN(dev_priv) >= 9) { + if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) { + intel_primary_formats = skl_primary_formats; + num_formats = ARRAY_SIZE(skl_primary_formats); + modifiers = skl_format_modifiers_ccs; + + primary->update_plane = skylake_update_primary_plane; + primary->disable_plane = skylake_disable_primary_plane; + } else if (INTEL_GEN(dev_priv) >= 9) { intel_primary_formats = skl_primary_formats; num_formats = ARRAY_SIZE(skl_primary_formats); - modifiers = skl_format_modifiers; + if (pipe >= PIPE_C) + modifiers = skl_format_modifiers_ccs; + else + modifiers = skl_format_modifiers_noccs; primary->update_plane = skylake_update_primary_plane; primary->disable_plane = skylake_disable_primary_plane; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 05a15063ee97..97d29cc061ad 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1079,7 +1079,17 @@ static uint32_t skl_plane_formats[] = { DRM_FORMAT_VYUY, }; +static const uint64_t skl_plane_format_modifiers_noccs[] = { + I915_FORMAT_MOD_Yf_TILED, + I915_FORMAT_MOD_Y_TILED, + I915_FORMAT_MOD_X_TILED, + DRM_FORMAT_MOD_LINEAR, + DRM_FORMAT_MOD_INVALID +}; + static const uint64_t skl_plane_format_modifiers[] = { + I915_FORMAT_MOD_Yf_TILED_CCS, + I915_FORMAT_MOD_Y_TILED_CCS, I915_FORMAT_MOD_Yf_TILED, I915_FORMAT_MOD_Y_TILED, I915_FORMAT_MOD_X_TILED, @@ -1224,7 +1234,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, } intel_plane->base.state = &state->base; - if (INTEL_GEN(dev_priv) >= 9) { + if (INTEL_GEN(dev_priv) >= 10) { intel_plane->can_scale = true; state->scaler_id = -1; @@ -1234,6 +1244,19 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, plane_formats = skl_plane_formats; num_plane_formats = ARRAY_SIZE(skl_plane_formats); modifiers = skl_plane_format_modifiers; + } else if (INTEL_GEN(dev_priv) >= 9) { + intel_plane->can_scale = true; + state->scaler_id = -1; + + intel_plane->update_plane = skl_update_plane; + intel_plane->disable_plane = skl_disable_plane; + + plane_formats = skl_plane_formats; + num_plane_formats = ARRAY_SIZE(skl_plane_formats); + if (pipe >= PIPE_C) + modifiers = skl_plane_format_modifiers_noccs; + else + modifiers = skl_plane_format_modifiers; } else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { intel_plane->can_scale = false; intel_plane->max_downscale = 1;
v2: Support sprite plane. Support pipe C/D limitation on GEN9. v3: Rename structure (Ville) Handle GLK (Ville) This requires rebase on the correct Ville patches Cc: Daniel Stone <daniels@collabora.com> Cc: Kristian Høgsberg <krh@bitplanet.net> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_display.c | 30 +++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_sprite.c | 25 ++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 4 deletions(-)