| Message ID | 1477355301-7035-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> On Mon, 2016-10-24 at 17:28 -0700, Anusha Srivatsa wrote: > Currently, for display there is only one DMC image for KBL. > Remove the stepping_info table for KBL and use the no_stepping_info > array for loading the firmware. > > v2: Removed the block of code as pointed out by Rodrigo to make the > loads as generic as possible. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 1ea0e1f..d7a04bc 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -168,12 +168,6 @@ struct stepping_info { > char substepping; > }; > > -static const struct stepping_info kbl_stepping_info[] = { > - {'A', '0'}, {'B', '0'}, {'C', '0'}, > - {'D', '0'}, {'E', '0'}, {'F', '0'}, > - {'G', '0'}, {'H', '0'}, {'I', '0'}, > -}; > - > static const struct stepping_info skl_stepping_info[] = { > {'A', '0'}, {'B', '0'}, {'C', '0'}, > {'D', '0'}, {'E', '0'}, {'F', '0'}, > @@ -194,10 +188,7 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) > const struct stepping_info *si; > unsigned int size; > > - if (IS_KABYLAKE(dev_priv)) { > - size = ARRAY_SIZE(kbl_stepping_info); > - si = kbl_stepping_info; > - } else if (IS_SKYLAKE(dev_priv)) { > + if (IS_SKYLAKE(dev_priv)) { > size = ARRAY_SIZE(skl_stepping_info); > si = skl_stepping_info; > } else if (IS_BROXTON(dev_priv)) {
On ti, 2016-10-25 at 18:00 +0000, Vivi, Rodrigo wrote: > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > On Mon, 2016-10-24 at 17:28 -0700, Anusha Srivatsa wrote: > > Currently, for display there is only one DMC image for KBL. > > Remove the stepping_info table for KBL and use the no_stepping_info > > array for loading the firmware. > > > > v2: Removed the block of code as pointed out by Rodrigo to make the > > loads as generic as possible. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> I assume the goal is to simplify the code. kbl_dmc_ver1_01.bin has indeed only a single *,* entry and I assume it was confirmed that there is no plan to release KBL firmware images in the future with multiple stepping entries. With that this looks ok: Acked-by: Imre Deak <imre.deak@intel.com> I think it would be good to also emit a warn from the driver if we use no_stepping_info and there are stepping entries in the firmware image other than *,* since then we may end up loading the wrong firmware version. > > --- > > drivers/gpu/drm/i915/intel_csr.c | 11 +---------- > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > b/drivers/gpu/drm/i915/intel_csr.c > > index 1ea0e1f..d7a04bc 100644 > > --- a/drivers/gpu/drm/i915/intel_csr.c > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > @@ -168,12 +168,6 @@ struct stepping_info { > > char substepping; > > }; > > > > -static const struct stepping_info kbl_stepping_info[] = { > > - {'A', '0'}, {'B', '0'}, {'C', '0'}, > > - {'D', '0'}, {'E', '0'}, {'F', '0'}, > > - {'G', '0'}, {'H', '0'}, {'I', '0'}, > > -}; > > - > > static const struct stepping_info skl_stepping_info[] = { > > {'A', '0'}, {'B', '0'}, {'C', '0'}, > > {'D', '0'}, {'E', '0'}, {'F', '0'}, > > @@ -194,10 +188,7 @@ intel_get_stepping_info(struct > > drm_i915_private *dev_priv) > > const struct stepping_info *si; > > unsigned int size; > > > > - if (IS_KABYLAKE(dev_priv)) { > > - size = ARRAY_SIZE(kbl_stepping_info); > > - si = kbl_stepping_info; > > - } else if (IS_SKYLAKE(dev_priv)) { > > + if (IS_SKYLAKE(dev_priv)) { > > size = ARRAY_SIZE(skl_stepping_info); > > si = skl_stepping_info; > > } else if (IS_BROXTON(dev_priv)) { > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2016-10-26 at 15:32 +0300, Imre Deak wrote: > On ti, 2016-10-25 at 18:00 +0000, Vivi, Rodrigo wrote: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > On Mon, 2016-10-24 at 17:28 -0700, Anusha Srivatsa wrote: > > > Currently, for display there is only one DMC image for KBL. > > > Remove the stepping_info table for KBL and use the no_stepping_info > > > array for loading the firmware. > > > > > > v2: Removed the block of code as pointed out by Rodrigo to make the > > > loads as generic as possible. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > I assume the goal is to simplify the code. kbl_dmc_ver1_01.bin has > indeed only a single *,* entry and I assume it was confirmed that there > is no plan to release KBL firmware images in the future with multiple > stepping entries. With that this looks ok: That is the case. > Acked-by: Imre Deak <imre.deak@intel.com> Thanks. > > I think it would be good to also emit a warn from the driver if we use > no_stepping_info and there are stepping entries in the firmware image > other than *,* since then we may end up loading the wrong firmware > version. yeap, this seems a good idea. a conservative approach. although, along with the confirmation we got the promise that this kind of information would be now on in the official release notes we receive along with the binary images. > > > > --- > > > drivers/gpu/drm/i915/intel_csr.c | 11 +---------- > > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > > b/drivers/gpu/drm/i915/intel_csr.c > > > index 1ea0e1f..d7a04bc 100644 > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > @@ -168,12 +168,6 @@ struct stepping_info { > > > char substepping; > > > }; > > > > > > -static const struct stepping_info kbl_stepping_info[] = { > > > - {'A', '0'}, {'B', '0'}, {'C', '0'}, > > > - {'D', '0'}, {'E', '0'}, {'F', '0'}, > > > - {'G', '0'}, {'H', '0'}, {'I', '0'}, > > > -}; > > > - > > > static const struct stepping_info skl_stepping_info[] = { > > > {'A', '0'}, {'B', '0'}, {'C', '0'}, > > > {'D', '0'}, {'E', '0'}, {'F', '0'}, > > > @@ -194,10 +188,7 @@ intel_get_stepping_info(struct > > > drm_i915_private *dev_priv) > > > const struct stepping_info *si; > > > unsigned int size; > > > > > > - if (IS_KABYLAKE(dev_priv)) { > > > - size = ARRAY_SIZE(kbl_stepping_info); > > > - si = kbl_stepping_info; > > > - } else if (IS_SKYLAKE(dev_priv)) { > > > + if (IS_SKYLAKE(dev_priv)) { > > > size = ARRAY_SIZE(skl_stepping_info); > > > si = skl_stepping_info; > > > } else if (IS_BROXTON(dev_priv)) { > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Patch merged to dinq. Thanks for the patch and thanks for the ack. On Wed, 2016-10-26 at 09:12 -0700, Rodrigo Vivi wrote: > On Wed, 2016-10-26 at 15:32 +0300, Imre Deak wrote: > > On ti, 2016-10-25 at 18:00 +0000, Vivi, Rodrigo wrote: > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > On Mon, 2016-10-24 at 17:28 -0700, Anusha Srivatsa wrote: > > > > Currently, for display there is only one DMC image for KBL. > > > > Remove the stepping_info table for KBL and use the no_stepping_info > > > > array for loading the firmware. > > > > > > > > v2: Removed the block of code as pointed out by Rodrigo to make the > > > > loads as generic as possible. > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > > I assume the goal is to simplify the code. kbl_dmc_ver1_01.bin has > > indeed only a single *,* entry and I assume it was confirmed that there > > is no plan to release KBL firmware images in the future with multiple > > stepping entries. With that this looks ok: > > That is the case. > > > Acked-by: Imre Deak <imre.deak@intel.com> > > Thanks. > > > > > I think it would be good to also emit a warn from the driver if we use > > no_stepping_info and there are stepping entries in the firmware image > > other than *,* since then we may end up loading the wrong firmware > > version. > > yeap, this seems a good idea. a conservative approach. > although, along with the confirmation we got the promise > that this kind of information would be now on in the official > release notes we receive along with the binary images. > > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_csr.c | 11 +---------- > > > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > > > b/drivers/gpu/drm/i915/intel_csr.c > > > > index 1ea0e1f..d7a04bc 100644 > > > > --- a/drivers/gpu/drm/i915/intel_csr.c > > > > +++ b/drivers/gpu/drm/i915/intel_csr.c > > > > @@ -168,12 +168,6 @@ struct stepping_info { > > > > char substepping; > > > > }; > > > > > > > > -static const struct stepping_info kbl_stepping_info[] = { > > > > - {'A', '0'}, {'B', '0'}, {'C', '0'}, > > > > - {'D', '0'}, {'E', '0'}, {'F', '0'}, > > > > - {'G', '0'}, {'H', '0'}, {'I', '0'}, > > > > -}; > > > > - > > > > static const struct stepping_info skl_stepping_info[] = { > > > > {'A', '0'}, {'B', '0'}, {'C', '0'}, > > > > {'D', '0'}, {'E', '0'}, {'F', '0'}, > > > > @@ -194,10 +188,7 @@ intel_get_stepping_info(struct > > > > drm_i915_private *dev_priv) > > > > const struct stepping_info *si; > > > > unsigned int size; > > > > > > > > - if (IS_KABYLAKE(dev_priv)) { > > > > - size = ARRAY_SIZE(kbl_stepping_info); > > > > - si = kbl_stepping_info; > > > > - } else if (IS_SKYLAKE(dev_priv)) { > > > > + if (IS_SKYLAKE(dev_priv)) { > > > > size = ARRAY_SIZE(skl_stepping_info); > > > > si = skl_stepping_info; > > > > } else if (IS_BROXTON(dev_priv)) { > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 1ea0e1f..d7a04bc 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -168,12 +168,6 @@ struct stepping_info { char substepping; }; -static const struct stepping_info kbl_stepping_info[] = { - {'A', '0'}, {'B', '0'}, {'C', '0'}, - {'D', '0'}, {'E', '0'}, {'F', '0'}, - {'G', '0'}, {'H', '0'}, {'I', '0'}, -}; - static const struct stepping_info skl_stepping_info[] = { {'A', '0'}, {'B', '0'}, {'C', '0'}, {'D', '0'}, {'E', '0'}, {'F', '0'}, @@ -194,10 +188,7 @@ intel_get_stepping_info(struct drm_i915_private *dev_priv) const struct stepping_info *si; unsigned int size; - if (IS_KABYLAKE(dev_priv)) { - size = ARRAY_SIZE(kbl_stepping_info); - si = kbl_stepping_info; - } else if (IS_SKYLAKE(dev_priv)) { + if (IS_SKYLAKE(dev_priv)) { size = ARRAY_SIZE(skl_stepping_info); si = skl_stepping_info; } else if (IS_BROXTON(dev_priv)) {
Currently, for display there is only one DMC image for KBL. Remove the stepping_info table for KBL and use the no_stepping_info array for loading the firmware. v2: Removed the block of code as pointed out by Rodrigo to make the loads as generic as possible. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)