| Message ID | 20180717125320.6046-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ville Syrjala (2018-07-17 13:53:19) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Most plattforms don't have a fixed 1MiB WOPCM so stop saying that they > do. > > Also toss in a FIXME about actually using the WOPCM size we probed from > the hardware instead of assuming the fixed 1MiB size. > > Cc: Jackie Li <yaodong.li@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Michał wrote a near identical patch > --- > drivers/gpu/drm/i915/intel_wopcm.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > index 74bf76f3fddc..75c7a2b0c869 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -71,6 +71,12 @@ > */ > void intel_wopcm_init_early(struct intel_wopcm *wopcm) > { > + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > + > + if (!HAS_GUC(i915)) > + return; > + > + /* FIXME use the size we actually probed from the hardware */ > wopcm->size = GEN9_WOPCM_SIZE; > > DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); > @@ -163,7 +169,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > u32 guc_wopcm_rsvd; > int err; > > - GEM_BUG_ON(!wopcm->size); > + if (!wopcm->size) > + return 0; ...except he chose to keep the GEM_BUG_ON. My personal preference would be to use the driver value (wopcm->size) here, so Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> but if Michał et al feel strongly that they would rather keep the !size sanity check, they need to speak up now :) -Chris
On Tue, 17 Jul 2018 14:53:19 +0200, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Most plattforms don't have a fixed 1MiB WOPCM so stop saying that they > do. > > Also toss in a FIXME about actually using the WOPCM size we probed from > the hardware instead of assuming the fixed 1MiB size. > > Cc: Jackie Li <yaodong.li@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_wopcm.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c > b/drivers/gpu/drm/i915/intel_wopcm.c > index 74bf76f3fddc..75c7a2b0c869 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -71,6 +71,12 @@ > */ > void intel_wopcm_init_early(struct intel_wopcm *wopcm) > { > + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > + > + if (!HAS_GUC(i915)) > + return; > + > + /* FIXME use the size we actually probed from the hardware */ > wopcm->size = GEN9_WOPCM_SIZE; > DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); > @@ -163,7 +169,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > u32 guc_wopcm_rsvd; > int err; > - GEM_BUG_ON(!wopcm->size); > + if (!wopcm->size) > + return 0; Maybe better option would be to use: if (!HAS_GUC(i915)) return 0; which will match conditions used in init_early and init_hw and then we will also allow to run remaining detailed checks ... > if (guc_fw_size >= wopcm->size) { > DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.", Thanks, Michal
On Tue, Jul 17, 2018 at 03:53:19PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Most plattforms don't have a fixed 1MiB WOPCM so stop saying that they > do. > > Also toss in a FIXME about actually using the WOPCM size we probed from > the hardware instead of assuming the fixed 1MiB size. > > Cc: Jackie Li <yaodong.li@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_wopcm.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > index 74bf76f3fddc..75c7a2b0c869 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -71,6 +71,12 @@ > */ > void intel_wopcm_init_early(struct intel_wopcm *wopcm) > { > + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > + > + if (!HAS_GUC(i915)) > + return; Single use dev_priv, drop the local? > + > + /* FIXME use the size we actually probed from the hardware */ > wopcm->size = GEN9_WOPCM_SIZE; I don't think that's exposed to us in any way. I'd drop the FIXME - with that: Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> -Michał > > DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); > @@ -163,7 +169,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > u32 guc_wopcm_rsvd; > int err; > > - GEM_BUG_ON(!wopcm->size); > + if (!wopcm->size) > + return 0; > > if (guc_fw_size >= wopcm->size) { > DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.", > -- > 2.16.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 17, 2018 at 03:52:41PM +0200, Michał Winiarski wrote: > On Tue, Jul 17, 2018 at 03:53:19PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Most plattforms don't have a fixed 1MiB WOPCM so stop saying that they > > do. > > > > Also toss in a FIXME about actually using the WOPCM size we probed from > > the hardware instead of assuming the fixed 1MiB size. > > > > Cc: Jackie Li <yaodong.li@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_wopcm.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > > index 74bf76f3fddc..75c7a2b0c869 100644 > > --- a/drivers/gpu/drm/i915/intel_wopcm.c > > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > > @@ -71,6 +71,12 @@ > > */ > > void intel_wopcm_init_early(struct intel_wopcm *wopcm) > > { > > + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > > + > > + if (!HAS_GUC(i915)) > > + return; > > Single use dev_priv, drop the local? > > > + > > + /* FIXME use the size we actually probed from the hardware */ > > wopcm->size = GEN9_WOPCM_SIZE; > > I don't think that's exposed to us in any way. > I'd drop the FIXME - with that: Ok - I was wrong, keep the fixme. > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > -Michał > > > > > DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); > > @@ -163,7 +169,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > > u32 guc_wopcm_rsvd; > > int err; > > > > - GEM_BUG_ON(!wopcm->size); > > + if (!wopcm->size) > > + return 0; But I'd also go with HAS_GUC and keep the BUG_ON as Michał suggested. -Michał > > > > if (guc_fw_size >= wopcm->size) { > > DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.", > > -- > > 2.16.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 07/17/2018 05:53 AM, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Most plattforms don't have a fixed 1MiB WOPCM so stop saying that they > do. > > Also toss in a FIXME about actually using the WOPCM size we probed from > the hardware instead of assuming the fixed 1MiB size. > > Cc: Jackie Li <yaodong.li@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_wopcm.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c > index 74bf76f3fddc..75c7a2b0c869 100644 > --- a/drivers/gpu/drm/i915/intel_wopcm.c > +++ b/drivers/gpu/drm/i915/intel_wopcm.c > @@ -71,6 +71,12 @@ > */ > void intel_wopcm_init_early(struct intel_wopcm *wopcm) > { > + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); > + > + if (!HAS_GUC(i915)) > + return; > + > + /* FIXME use the size we actually probed from the hardware */ > wopcm->size = GEN9_WOPCM_SIZE; > > DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); > @@ -163,7 +169,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) > u32 guc_wopcm_rsvd; > int err; > > - GEM_BUG_ON(!wopcm->size); > + if (!wopcm->size) > + return 0; > > if (guc_fw_size >= wopcm->size) { > DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.", The patch looks fine to me. Reviewed-by: Jackie Li <yaodong.li@intel.com> Regards, -Jackie
diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c index 74bf76f3fddc..75c7a2b0c869 100644 --- a/drivers/gpu/drm/i915/intel_wopcm.c +++ b/drivers/gpu/drm/i915/intel_wopcm.c @@ -71,6 +71,12 @@ */ void intel_wopcm_init_early(struct intel_wopcm *wopcm) { + struct drm_i915_private *i915 = wopcm_to_i915(wopcm); + + if (!HAS_GUC(i915)) + return; + + /* FIXME use the size we actually probed from the hardware */ wopcm->size = GEN9_WOPCM_SIZE; DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024); @@ -163,7 +169,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm) u32 guc_wopcm_rsvd; int err; - GEM_BUG_ON(!wopcm->size); + if (!wopcm->size) + return 0; if (guc_fw_size >= wopcm->size) { DRM_ERROR("GuC FW (%uKiB) is too big to fit in WOPCM.",