| Message ID | 20180925001913.29460-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: DRM_FORMAT_C8 is not possible with Yf tiling | expand |
On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote: > Function intel_framebuffer_init() checks for the possibilities during > framebuffer creation (addfb ioctl time). It is missing the fact that > the indexed format is not supported with Yf tiling. > > It is worth noticing that skl_plane_format_mod_supported() correctly > handles for the C8/Yf combination, but this function runs during > modeset time, so we only reject the combination later. > > Ville recently proposed a new IGT test that only uses addfb to assert > supported formats, so that IGT was failing. Add the check so we get > green squares right from the start after Ville merges his test. I have two of three (possibly) nicer ways to solve this: https://patchwork.freedesktop.org/series/39700/ https://patchwork.freedesktop.org/series/39383/ https://patchwork.freedesktop.org/series/39813/ Would be nice if someone could figure out a solution (one of those or perhaps some other solution I didn't think of) that enough people are willing to accept. > > Also drive-by fix the missing /* fall through */ in the chunk we > modified by just turning it into a "break;" since IMHO breaks are > easier to read than fall-throughs. > > BSpec: 18565 > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet) > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index eb25037d7b38..fdff1779f778 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, > goto err; > } > /* fall through */ > - case I915_FORMAT_MOD_Y_TILED: > case I915_FORMAT_MOD_Yf_TILED: > + if (mode_cmd->pixel_format == DRM_FORMAT_C8) { > + DRM_DEBUG_KMS("Indexed format does not support Yf tiling\n"); > + goto err; > + } > + /* fall through */ > + case I915_FORMAT_MOD_Y_TILED: > if (INTEL_GEN(dev_priv) < 9) { > DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n", > mode_cmd->modifier[0]); > goto err; > } > + break; > case DRM_FORMAT_MOD_LINEAR: > case I915_FORMAT_MOD_X_TILED: > break; > -- > 2.14.4
Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu: > On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote: > > Function intel_framebuffer_init() checks for the possibilities > > during > > framebuffer creation (addfb ioctl time). It is missing the fact > > that > > the indexed format is not supported with Yf tiling. > > > > It is worth noticing that skl_plane_format_mod_supported() > > correctly > > handles for the C8/Yf combination, but this function runs during > > modeset time, so we only reject the combination later. > > > > Ville recently proposed a new IGT test that only uses addfb to > > assert > > supported formats, so that IGT was failing. Add the check so we get > > green squares right from the start after Ville merges his test. > > I have two of three (possibly) nicer ways to solve this: > https://patchwork.freedesktop.org/series/39700/ I thought about implementing this one when looking at the code. I agree the duplicated checks are horrible. I thought maybe this model wouldn't be acceptable due to the inefficiency of always looping over everything vs the current linear solution. I see no review comments on this series besides the vc4 patch. Did you get anything that's not appearing on patchwork? > https://patchwork.freedesktop.org/series/39383/ You have blocked your own patch with your own review here. > https://patchwork.freedesktop.org/series/39813/ Looks like there's some potential controversy to be untangled here if we wish to follow this route. > solution 4 I guess it would be to simply not have the checks at all. But this would be an interface change instead of just refactoring code duplication. > > Would be nice if someone could figure out a solution (one of those or > perhaps some other solution I didn't think of) that enough people are > willing to accept. I could see solution 1 moving forward more easily, and even could volunteer myself to review a rebased version. In the meantime, we could actually review/commit this immediate fix and have a correct-but-not-yet-reworked codebase instead of waiting for a patch that has been abandoned since March. I don't think one series should block the other. > > > > > Also drive-by fix the missing /* fall through */ in the chunk we > > modified by just turning it into a "break;" since IMHO breaks are > > easier to read than fall-throughs. > > > > BSpec: 18565 > > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index eb25037d7b38..fdff1779f778 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct > > intel_framebuffer *intel_fb, > > goto err; > > } > > /* fall through */ > > - case I915_FORMAT_MOD_Y_TILED: > > case I915_FORMAT_MOD_Yf_TILED: > > + if (mode_cmd->pixel_format == DRM_FORMAT_C8) { > > + DRM_DEBUG_KMS("Indexed format does not > > support Yf tiling\n"); > > + goto err; > > + } > > + /* fall through */ > > + case I915_FORMAT_MOD_Y_TILED: > > if (INTEL_GEN(dev_priv) < 9) { > > DRM_DEBUG_KMS("Unsupported tiling > > 0x%llx!\n", > > mode_cmd->modifier[0]); > > goto err; > > } > > + break; > > case DRM_FORMAT_MOD_LINEAR: > > case I915_FORMAT_MOD_X_TILED: > > break; > > -- > > 2.14.4 > >
On Tue, Sep 25, 2018 at 03:02:21PM -0700, Paulo Zanoni wrote: > Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu: > > On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote: > > > Function intel_framebuffer_init() checks for the possibilities > > > during > > > framebuffer creation (addfb ioctl time). It is missing the fact > > > that > > > the indexed format is not supported with Yf tiling. > > > > > > It is worth noticing that skl_plane_format_mod_supported() > > > correctly > > > handles for the C8/Yf combination, but this function runs during > > > modeset time, so we only reject the combination later. > > > > > > Ville recently proposed a new IGT test that only uses addfb to > > > assert > > > supported formats, so that IGT was failing. Add the check so we get > > > green squares right from the start after Ville merges his test. > > > > I have two of three (possibly) nicer ways to solve this: > > https://patchwork.freedesktop.org/series/39700/ > > I thought about implementing this one when looking at the code. I agree > the duplicated checks are horrible. I thought maybe this model wouldn't > be acceptable due to the inefficiency of always looping over everything > vs the current linear solution. > > I see no review comments on this series besides the vc4 patch. Did you > get anything that's not appearing on patchwork? > > > https://patchwork.freedesktop.org/series/39383/ > > You have blocked your own patch with your own review here. > > > https://patchwork.freedesktop.org/series/39813/ > > Looks like there's some potential controversy to be untangled here if > we wish to follow this route. > > > solution 4 > > I guess it would be to simply not have the checks at all. But this > would be an interface change instead of just refactoring code > duplication. > > > > > > Would be nice if someone could figure out a solution (one of those or > > perhaps some other solution I didn't think of) that enough people are > > willing to accept. > > I could see solution 1 moving forward more easily, and even could > volunteer myself to review a rebased version. > > In the meantime, we could actually review/commit this immediate fix and > have a correct-but-not-yet-reworked codebase instead of waiting for a > patch that has been abandoned since March. I don't think one series > should block the other. Sure. Was just trying to trick someone into finishing what I started ;) Patch is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Also drive-by fix the missing /* fall through */ in the chunk we > > > modified by just turning it into a "break;" since IMHO breaks are > > > easier to read than fall-throughs. > > > > > > BSpec: 18565 > > > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet) > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index eb25037d7b38..fdff1779f778 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct > > > intel_framebuffer *intel_fb, > > > goto err; > > > } > > > /* fall through */ > > > - case I915_FORMAT_MOD_Y_TILED: > > > case I915_FORMAT_MOD_Yf_TILED: > > > + if (mode_cmd->pixel_format == DRM_FORMAT_C8) { > > > + DRM_DEBUG_KMS("Indexed format does not > > > support Yf tiling\n"); > > > + goto err; > > > + } > > > + /* fall through */ > > > + case I915_FORMAT_MOD_Y_TILED: > > > if (INTEL_GEN(dev_priv) < 9) { > > > DRM_DEBUG_KMS("Unsupported tiling > > > 0x%llx!\n", > > > mode_cmd->modifier[0]); > > > goto err; > > > } > > > + break; > > > case DRM_FORMAT_MOD_LINEAR: > > > case I915_FORMAT_MOD_X_TILED: > > > break; > > > -- > > > 2.14.4 > > > >
Em Qui, 2018-09-27 às 17:16 +0300, Ville Syrjälä escreveu: > On Tue, Sep 25, 2018 at 03:02:21PM -0700, Paulo Zanoni wrote: > > Em Ter, 2018-09-25 às 15:02 +0300, Ville Syrjälä escreveu: > > > On Mon, Sep 24, 2018 at 05:19:11PM -0700, Paulo Zanoni wrote: > > > > Function intel_framebuffer_init() checks for the possibilities > > > > during > > > > framebuffer creation (addfb ioctl time). It is missing the fact > > > > that > > > > the indexed format is not supported with Yf tiling. > > > > > > > > It is worth noticing that skl_plane_format_mod_supported() > > > > correctly > > > > handles for the C8/Yf combination, but this function runs > > > > during > > > > modeset time, so we only reject the combination later. > > > > > > > > Ville recently proposed a new IGT test that only uses addfb to > > > > assert > > > > supported formats, so that IGT was failing. Add the check so we > > > > get > > > > green squares right from the start after Ville merges his test. > > > > > > I have two of three (possibly) nicer ways to solve this: > > > https://patchwork.freedesktop.org/series/39700/ > > > > I thought about implementing this one when looking at the code. I > > agree > > the duplicated checks are horrible. I thought maybe this model > > wouldn't > > be acceptable due to the inefficiency of always looping over > > everything > > vs the current linear solution. > > > > I see no review comments on this series besides the vc4 patch. Did > > you > > get anything that's not appearing on patchwork? > > > > > https://patchwork.freedesktop.org/series/39383/ > > > > You have blocked your own patch with your own review here. > > > > > https://patchwork.freedesktop.org/series/39813/ > > > > Looks like there's some potential controversy to be untangled here > > if > > we wish to follow this route. > > > > > solution 4 > > > > I guess it would be to simply not have the checks at all. But this > > would be an interface change instead of just refactoring code > > duplication. > > > > > > > > > > Would be nice if someone could figure out a solution (one of > > > those or > > > perhaps some other solution I didn't think of) that enough people > > > are > > > willing to accept. > > > > I could see solution 1 moving forward more easily, and even could > > volunteer myself to review a rebased version. > > > > In the meantime, we could actually review/commit this immediate fix > > and > > have a correct-but-not-yet-reworked codebase instead of waiting for > > a > > patch that has been abandoned since March. I don't think one series > > should block the other. > > Sure. Was just trying to trick someone into finishing what I started > ;) > > Patch is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks a lot! Feel free to CC me on the series that kills the whole thing, I agree it's a good thing to do. > > > > > > > > > > > > > > > > Also drive-by fix the missing /* fall through */ in the chunk > > > > we > > > > modified by just turning it into a "break;" since IMHO breaks > > > > are > > > > easier to read than fall-throughs. > > > > > > > > BSpec: 18565 > > > > Testcase: igt/kms_addfb_basic/expected-formats (not merged yet) > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index eb25037d7b38..fdff1779f778 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -14473,13 +14473,19 @@ static int > > > > intel_framebuffer_init(struct > > > > intel_framebuffer *intel_fb, > > > > goto err; > > > > } > > > > /* fall through */ > > > > - case I915_FORMAT_MOD_Y_TILED: > > > > case I915_FORMAT_MOD_Yf_TILED: > > > > + if (mode_cmd->pixel_format == DRM_FORMAT_C8) { > > > > + DRM_DEBUG_KMS("Indexed format does not > > > > support Yf tiling\n"); > > > > + goto err; > > > > + } > > > > + /* fall through */ > > > > + case I915_FORMAT_MOD_Y_TILED: > > > > if (INTEL_GEN(dev_priv) < 9) { > > > > DRM_DEBUG_KMS("Unsupported tiling > > > > 0x%llx!\n", > > > > mode_cmd->modifier[0]); > > > > goto err; > > > > } > > > > + break; > > > > case DRM_FORMAT_MOD_LINEAR: > > > > case I915_FORMAT_MOD_X_TILED: > > > > break; > > > > -- > > > > 2.14.4 > > > > > > > >
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index eb25037d7b38..fdff1779f778 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14473,13 +14473,19 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb, goto err; } /* fall through */ - case I915_FORMAT_MOD_Y_TILED: case I915_FORMAT_MOD_Yf_TILED: + if (mode_cmd->pixel_format == DRM_FORMAT_C8) { + DRM_DEBUG_KMS("Indexed format does not support Yf tiling\n"); + goto err; + } + /* fall through */ + case I915_FORMAT_MOD_Y_TILED: if (INTEL_GEN(dev_priv) < 9) { DRM_DEBUG_KMS("Unsupported tiling 0x%llx!\n", mode_cmd->modifier[0]); goto err; } + break; case DRM_FORMAT_MOD_LINEAR: case I915_FORMAT_MOD_X_TILED: break;
Function intel_framebuffer_init() checks for the possibilities during framebuffer creation (addfb ioctl time). It is missing the fact that the indexed format is not supported with Yf tiling. It is worth noticing that skl_plane_format_mod_supported() correctly handles for the C8/Yf combination, but this function runs during modeset time, so we only reject the combination later. Ville recently proposed a new IGT test that only uses addfb to assert supported formats, so that IGT was failing. Add the check so we get green squares right from the start after Ville merges his test. Also drive-by fix the missing /* fall through */ in the chunk we modified by just turning it into a "break;" since IMHO breaks are easier to read than fall-throughs. BSpec: 18565 Testcase: igt/kms_addfb_basic/expected-formats (not merged yet) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)