| Message ID | 20200117091627.1697-1-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/tgl: Add Wa_1606054188;tgl | expand |
On Fri, Jan 17, 2020 at 04:16:28AM -0500, Matt Atwood wrote: > On Tiger Lake we do not support source keying in the pixel formats P010, > P012, P016. > > Bspec: 52890 > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > --- > drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > index fca77ec1e0dd..67176524e60f 100644 > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, > unsigned int rotation = plane_state->hw.rotation; > struct drm_format_name_buf format_name; > > + /* Wa_1606054188;tgl ^ This should be a : > + * > + * TODO: Add format RGB64i when implemented > + * > + */ > + if (IS_GEN(dev_priv, 12) && > + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) > + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 > + | DRM_FORMAT_P016)) { > + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); > + return -EINVAL; > + } > + I think this whole WA and check should be added after the check for !fb else we might have a risk of dereferencing a NULL pointer. With the above fixs Reviewed-by: Manasi Navare <manasi.d.navare@intel.com> Manasi > if (!fb) > return 0; > > -- > 2.21.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jan 17, 2020 at 04:16:28AM -0500, Matt Atwood wrote: > On Tiger Lake we do not support source keying in the pixel formats P010, > P012, P016. > > Bspec: 52890 > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > --- > drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > index fca77ec1e0dd..67176524e60f 100644 > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, > unsigned int rotation = plane_state->hw.rotation; > struct drm_format_name_buf format_name; > > + /* Wa_1606054188;tgl > + * > + * TODO: Add format RGB64i when implemented > + * > + */ > + if (IS_GEN(dev_priv, 12) && > + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) > + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 > + | DRM_FORMAT_P016)) { if (a && b && c) Needless parens. Continuing | should go to the end. Also alignment is borked. > + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); Feels a bit overly verbose: "Source color keying not supported with P01x formats\n" > + return -EINVAL; > + } > + > if (!fb) > return 0; What Manasi said. In fact pls move the thing to the end of the function because I have more color key checks queued up in a branch and IIRC I put them to the very end of the function. > > -- > 2.21.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jan 28, 2020 at 07:05:38PM +0200, Ville Syrjälä wrote: > On Fri, Jan 17, 2020 at 04:16:28AM -0500, Matt Atwood wrote: > > On Tiger Lake we do not support source keying in the pixel formats P010, > > P012, P016. > > > > Bspec: 52890 > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > > index fca77ec1e0dd..67176524e60f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, > > unsigned int rotation = plane_state->hw.rotation; > > struct drm_format_name_buf format_name; > > > > + /* Wa_1606054188;tgl > > + * > > + * TODO: Add format RGB64i when implemented > > + * > > + */ > > + if (IS_GEN(dev_priv, 12) && > > + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) > > + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 > > + | DRM_FORMAT_P016)) { Oh, and that | stuff is actually just nonsense. That's not a bitfield or anything like that. I'd just add a small intel_format_is_p01x() function etc. > > if (a && b && c) > > Needless parens. > > Continuing | should go to the end. Also alignment is borked. > > > + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); > > Feels a bit overly verbose: > "Source color keying not supported with P01x formats\n" > > > + return -EINVAL; > > + } > > + > > if (!fb) > > return 0; > > What Manasi said. In fact pls move the thing to the end of the function > because I have more color key checks queued up in a branch and IIRC > I put them to the very end of the function. > > > > > -- > > 2.21.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Tue, Jan 28, 2020 at 07:30:59PM +0200, Ville Syrjälä wrote: > On Tue, Jan 28, 2020 at 07:05:38PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 17, 2020 at 04:16:28AM -0500, Matt Atwood wrote: > > > On Tiger Lake we do not support source keying in the pixel formats P010, > > > P012, P016. > > > > > > Bspec: 52890 > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > > > index fca77ec1e0dd..67176524e60f 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, > > > unsigned int rotation = plane_state->hw.rotation; > > > struct drm_format_name_buf format_name; > > > > > > + /* Wa_1606054188;tgl > > > + * > > > + * TODO: Add format RGB64i when implemented > > > + * > > > + */ > > > + if (IS_GEN(dev_priv, 12) && > > > + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) Yes I agree here no need to have paranthesis just if (IS_GEN(dev_priv, 12) && plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE) should suffice since bitwise & higher precedence than logical AND > > > + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 > > > + | DRM_FORMAT_P016)) { > > Oh, and that | stuff is actually just nonsense. That's not a > bitfield or anything like that. I'd just add a small > intel_format_is_p01x() function etc. Yes I agree with Ville. The DRM_FORMAT_ is a 4CC format identifie not a bitfield so you would need to define a static function something like: static bool intel_format_is_p01x() { if(format == DRM_FORMAT_P010 || format == DRM_FORMAT_P012 || format == DRM_FORMAT_P016) return true; return false; } Look at lookup_format_info() for reference. Hope this helps. Manasi > > > > > if (a && b && c) > > > > Needless parens. > > > > Continuing | should go to the end. Also alignment is borked. > > > > > + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); > > > > Feels a bit overly verbose: > > "Source color keying not supported with P01x formats\n" > > > > > + return -EINVAL; > > > + } > > > + > > > if (!fb) > > > return 0; > > > > What Manasi said. In fact pls move the thing to the end of the function > > because I have more color key checks queued up in a branch and IIRC > > I put them to the very end of the function. > > > > > > > > -- > > > 2.21.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jan 28, 2020 at 07:30:59PM +0200, Ville Syrjälä wrote: > On Tue, Jan 28, 2020 at 07:05:38PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 17, 2020 at 04:16:28AM -0500, Matt Atwood wrote: > > > On Tiger Lake we do not support source keying in the pixel formats P010, > > > P012, P016. > > > > > > Bspec: 52890 > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > > > index fca77ec1e0dd..67176524e60f 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, > > > unsigned int rotation = plane_state->hw.rotation; > > > struct drm_format_name_buf format_name; > > > > > > + /* Wa_1606054188;tgl > > > + * > > > + * TODO: Add format RGB64i when implemented > > > + * > > > + */ > > > + if (IS_GEN(dev_priv, 12) && > > > + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) Yes I agree, it should just be : if (IS_GEN(dev_priv, 12) && plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE) since bitwise & has higher precedence than logical AND > > > + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 > > > + | DRM_FORMAT_P016)) { > > Oh, and that | stuff is actually just nonsense. That's not a > bitfield or anything like that. I'd just add a small > intel_format_is_p01x() function etc. > Yes that is true here DRM_FORMAT_* is a 4CC format identifier so like Ville suggested the function to check format should look like: static bool intel_format_is_p01x() { if (format == DRM_FORMAT_P010 || format == DRM_FORMAT_P012 || format == DRM_FORMAT_P016) return true; return false; } Hope this helps, you can also look at the lookup_format_info() Manasi > > > > if (a && b && c) > > > > Needless parens. > > > > Continuing | should go to the end. Also alignment is borked. > > > > > + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); > > > > Feels a bit overly verbose: > > "Source color keying not supported with P01x formats\n" > > > > > + return -EINVAL; > > > + } > > > + > > > if (!fb) > > > return 0; > > > > What Manasi said. In fact pls move the thing to the end of the function > > because I have more color key checks queued up in a branch and IIRC > > I put them to the very end of the function. > > > > > > > > -- > > > 2.21.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jan 28, 2020 at 12:28:35PM -0800, Manasi Navare wrote: > On Tue, Jan 28, 2020 at 07:30:59PM +0200, Ville Syrjälä wrote: > > On Tue, Jan 28, 2020 at 07:05:38PM +0200, Ville Syrjälä wrote: > > > On Fri, Jan 17, 2020 at 04:16:28AM -0500, Matt Atwood wrote: > > > > On Tiger Lake we do not support source keying in the pixel formats P010, > > > > P012, P016. > > > > > > > > Bspec: 52890 > > > > Cc: Matt Roper <matthew.d.roper@intel.com> > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c > > > > index fca77ec1e0dd..67176524e60f 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c > > > > @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, > > > > unsigned int rotation = plane_state->hw.rotation; > > > > struct drm_format_name_buf format_name; > > > > > > > > + /* Wa_1606054188;tgl > > > > + * > > > > + * TODO: Add format RGB64i when implemented > > > > + * > > > > + */ > > > > + if (IS_GEN(dev_priv, 12) && > > > > + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) > > Yes I agree here no need to have paranthesis just > > if (IS_GEN(dev_priv, 12) && plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE) > should suffice since bitwise & higher precedence than logical AND > > > > > + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 > > > > + | DRM_FORMAT_P016)) { > > > > Oh, and that | stuff is actually just nonsense. That's not a > > bitfield or anything like that. I'd just add a small > > intel_format_is_p01x() function etc. > > Yes I agree with Ville. The DRM_FORMAT_ is a 4CC format identifie not a bitfield > so you would need to define a static function something like: > > static bool intel_format_is_p01x() > { > if(format == DRM_FORMAT_P010 || > format == DRM_FORMAT_P012 || > format == DRM_FORMAT_P016) > return true; switch() > > return false; > } > > Look at lookup_format_info() for reference. > Hope this helps. > > Manasi > > > > > > > > > if (a && b && c) > > > > > > Needless parens. > > > > > > Continuing | should go to the end. Also alignment is borked. > > > > > > > + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); > > > > > > Feels a bit overly verbose: > > > "Source color keying not supported with P01x formats\n" > > > > > > > + return -EINVAL; > > > > + } > > > > + > > > > if (!fb) > > > > return 0; > > > > > > What Manasi said. In fact pls move the thing to the end of the function > > > because I have more color key checks queued up in a branch and IIRC > > > I put them to the very end of the function. > > > > > > > > > > > -- > > > > 2.21.1 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c index fca77ec1e0dd..67176524e60f 100644 --- a/drivers/gpu/drm/i915/display/intel_sprite.c +++ b/drivers/gpu/drm/i915/display/intel_sprite.c @@ -2049,6 +2049,19 @@ static int skl_plane_check_fb(const struct intel_crtc_state *crtc_state, unsigned int rotation = plane_state->hw.rotation; struct drm_format_name_buf format_name; + /* Wa_1606054188;tgl + * + * TODO: Add format RGB64i when implemented + * + */ + if (IS_GEN(dev_priv, 12) && + (plane_state->ckey.flags & I915_SET_COLORKEY_SOURCE)) + if (fb->format->format & (DRM_FORMAT_P010 | DRM_FORMAT_P012 + | DRM_FORMAT_P016)) { + DRM_DEBUG_KMS("GEN12 does not support source color key planes in formats P01x\n"); + return -EINVAL; + } + if (!fb) return 0;
On Tiger Lake we do not support source keying in the pixel formats P010, P012, P016. Bspec: 52890 Cc: Matt Roper <matthew.d.roper@intel.com> Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> --- drivers/gpu/drm/i915/display/intel_sprite.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)