| Message ID | 20220418150936.5499-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix DISP_POS_Y and DISP_HEIGHT defines | expand |
On Mon, 2022-04-18 at 17:09 +0200, Hans de Goede wrote: > Commit 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > introduced DISP_POS_Y and DISP_HEIGHT defines but accidentally set these > their masks to REG_GENMASK(31, 0) instead of REG_GENMASK(31, 16). > > This breaks the primary display pane on at least pineview machines, fix > the mask to fix the primary display pane only showing black. > > Tested on an Acer One AO532h with an Intel N450 SoC. > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > Fixes: 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note this fixes a regression in 5.18-rc# and I'm not entirely sure what > the procedure is here. Once I get a Reviewed-by or Acked-by and I push > this to drm-intel-next (where it also is necessary), should I then also > push it to drm-intel-fixes or will the current drm-intel-fixes > maintainer pick it up? > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 51f46fe45c72..5f1f38684d65 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4352,12 +4352,12 @@ > #define _DSPAADDR 0x70184 > #define _DSPASTRIDE 0x70188 > #define _DSPAPOS 0x7018C /* reserved */ > -#define DISP_POS_Y_MASK REG_GENMASK(31, 0) > +#define DISP_POS_Y_MASK REG_GENMASK(31, 16) > #define DISP_POS_Y(y) REG_FIELD_PREP(DISP_POS_Y_MASK, (y)) > #define DISP_POS_X_MASK REG_GENMASK(15, 0) > #define DISP_POS_X(x) REG_FIELD_PREP(DISP_POS_X_MASK, (x)) > #define _DSPASIZE 0x70190 > -#define DISP_HEIGHT_MASK REG_GENMASK(31, 0) > +#define DISP_HEIGHT_MASK REG_GENMASK(31, 16) > #define DISP_HEIGHT(h) REG_FIELD_PREP(DISP_HEIGHT_MASK, (h)) > #define DISP_WIDTH_MASK REG_GENMASK(15, 0) > #define DISP_WIDTH(w) REG_FIELD_PREP(DISP_WIDTH_MASK, (w))
On Mon, Apr 18, 2022 at 05:09:36PM +0200, Hans de Goede wrote: > Commit 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > introduced DISP_POS_Y and DISP_HEIGHT defines but accidentally set these > their masks to REG_GENMASK(31, 0) instead of REG_GENMASK(31, 16). > > This breaks the primary display pane on at least pineview machines, fix > the mask to fix the primary display pane only showing black. > > Tested on an Acer One AO532h with an Intel N450 SoC. > > Fixes: 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Note this fixes a regression in 5.18-rc# and I'm not entirely sure what > the procedure is here. Once I get a Reviewed-by or Acked-by and I push > this to drm-intel-next (where it also is necessary), should I then also > push it to drm-intel-fixes or will the current drm-intel-fixes > maintainer pick it up? > --- > drivers/gpu/drm/i915/i915_reg.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 51f46fe45c72..5f1f38684d65 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4352,12 +4352,12 @@ > #define _DSPAADDR 0x70184 > #define _DSPASTRIDE 0x70188 > #define _DSPAPOS 0x7018C /* reserved */ > -#define DISP_POS_Y_MASK REG_GENMASK(31, 0) > +#define DISP_POS_Y_MASK REG_GENMASK(31, 16) Doh. I guess I only tested it on plane A where the plane gets its size from PIPESRC instead. And looks like the failure mode is such that the likes of kms_plane/pixel-formats still gets consistent looking CRCs even with the misconfigured plane size :/ Thanks for the fix. Pushed to drm-intel-next. > #define DISP_POS_Y(y) REG_FIELD_PREP(DISP_POS_Y_MASK, (y)) > #define DISP_POS_X_MASK REG_GENMASK(15, 0) > #define DISP_POS_X(x) REG_FIELD_PREP(DISP_POS_X_MASK, (x)) > #define _DSPASIZE 0x70190 > -#define DISP_HEIGHT_MASK REG_GENMASK(31, 0) > +#define DISP_HEIGHT_MASK REG_GENMASK(31, 16) > #define DISP_HEIGHT(h) REG_FIELD_PREP(DISP_HEIGHT_MASK, (h)) > #define DISP_WIDTH_MASK REG_GENMASK(15, 0) > #define DISP_WIDTH(w) REG_FIELD_PREP(DISP_WIDTH_MASK, (w)) > -- > 2.35.1
Hi Ville, On 4/20/22 16:03, Ville Syrjälä wrote: > On Mon, Apr 18, 2022 at 05:09:36PM +0200, Hans de Goede wrote: >> Commit 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") >> introduced DISP_POS_Y and DISP_HEIGHT defines but accidentally set these >> their masks to REG_GENMASK(31, 0) instead of REG_GENMASK(31, 16). >> >> This breaks the primary display pane on at least pineview machines, fix >> the mask to fix the primary display pane only showing black. >> >> Tested on an Acer One AO532h with an Intel N450 SoC. >> >> Fixes: 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") >> Cc: José Roberto de Souza <jose.souza@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Note this fixes a regression in 5.18-rc# and I'm not entirely sure what >> the procedure is here. Once I get a Reviewed-by or Acked-by and I push >> this to drm-intel-next (where it also is necessary), should I then also >> push it to drm-intel-fixes or will the current drm-intel-fixes >> maintainer pick it up? >> --- >> drivers/gpu/drm/i915/i915_reg.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 51f46fe45c72..5f1f38684d65 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -4352,12 +4352,12 @@ >> #define _DSPAADDR 0x70184 >> #define _DSPASTRIDE 0x70188 >> #define _DSPAPOS 0x7018C /* reserved */ >> -#define DISP_POS_Y_MASK REG_GENMASK(31, 0) >> +#define DISP_POS_Y_MASK REG_GENMASK(31, 16) > > Doh. I guess I only tested it on plane A where the plane gets its size > from PIPESRC instead. And looks like the failure mode is such that > the likes of kms_plane/pixel-formats still gets consistent looking CRCs > even with the misconfigured plane size :/ > > Thanks for the fix. Pushed to drm-intel-next. Thank you pushing this out, will you (or someone else from Intel) also take care of getting this on its way to 5.18-rc# ? Regards, Hans > >> #define DISP_POS_Y(y) REG_FIELD_PREP(DISP_POS_Y_MASK, (y)) >> #define DISP_POS_X_MASK REG_GENMASK(15, 0) >> #define DISP_POS_X(x) REG_FIELD_PREP(DISP_POS_X_MASK, (x)) >> #define _DSPASIZE 0x70190 >> -#define DISP_HEIGHT_MASK REG_GENMASK(31, 0) >> +#define DISP_HEIGHT_MASK REG_GENMASK(31, 16) >> #define DISP_HEIGHT(h) REG_FIELD_PREP(DISP_HEIGHT_MASK, (h)) >> #define DISP_WIDTH_MASK REG_GENMASK(15, 0) >> #define DISP_WIDTH(w) REG_FIELD_PREP(DISP_WIDTH_MASK, (w)) >> -- >> 2.35.1 >
On Wed, Apr 20, 2022 at 05:32:43PM +0200, Hans de Goede wrote: > Hi Ville, > > On 4/20/22 16:03, Ville Syrjälä wrote: > > On Mon, Apr 18, 2022 at 05:09:36PM +0200, Hans de Goede wrote: > >> Commit 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > >> introduced DISP_POS_Y and DISP_HEIGHT defines but accidentally set these > >> their masks to REG_GENMASK(31, 0) instead of REG_GENMASK(31, 16). > >> > >> This breaks the primary display pane on at least pineview machines, fix > >> the mask to fix the primary display pane only showing black. > >> > >> Tested on an Acer One AO532h with an Intel N450 SoC. > >> > >> Fixes: 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > >> Cc: José Roberto de Souza <jose.souza@intel.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> Note this fixes a regression in 5.18-rc# and I'm not entirely sure what > >> the procedure is here. Once I get a Reviewed-by or Acked-by and I push > >> this to drm-intel-next (where it also is necessary), should I then also > >> push it to drm-intel-fixes or will the current drm-intel-fixes > >> maintainer pick it up? > >> --- > >> drivers/gpu/drm/i915/i915_reg.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> index 51f46fe45c72..5f1f38684d65 100644 > >> --- a/drivers/gpu/drm/i915/i915_reg.h > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > >> @@ -4352,12 +4352,12 @@ > >> #define _DSPAADDR 0x70184 > >> #define _DSPASTRIDE 0x70188 > >> #define _DSPAPOS 0x7018C /* reserved */ > >> -#define DISP_POS_Y_MASK REG_GENMASK(31, 0) > >> +#define DISP_POS_Y_MASK REG_GENMASK(31, 16) > > > > Doh. I guess I only tested it on plane A where the plane gets its size > > from PIPESRC instead. And looks like the failure mode is such that > > the likes of kms_plane/pixel-formats still gets consistent looking CRCs > > even with the misconfigured plane size :/ > > > > Thanks for the fix. Pushed to drm-intel-next. > > Thank you pushing this out, will you (or someone else from Intel) > also take care of getting this on its way to 5.18-rc# ? It has a fixes tag so it should get cherry-picked for fixes.
Quoting Ville Syrjälä (2022-04-20 19:23:57) > On Wed, Apr 20, 2022 at 05:32:43PM +0200, Hans de Goede wrote: > > Hi Ville, > > > > On 4/20/22 16:03, Ville Syrjälä wrote: > > > On Mon, Apr 18, 2022 at 05:09:36PM +0200, Hans de Goede wrote: > > >> Commit 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > > >> introduced DISP_POS_Y and DISP_HEIGHT defines but accidentally set these > > >> their masks to REG_GENMASK(31, 0) instead of REG_GENMASK(31, 16). > > >> > > >> This breaks the primary display pane on at least pineview machines, fix > > >> the mask to fix the primary display pane only showing black. > > >> > > >> Tested on an Acer One AO532h with an Intel N450 SoC. > > >> > > >> Fixes: 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") > > >> Cc: José Roberto de Souza <jose.souza@intel.com> > > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > >> --- > > >> Note this fixes a regression in 5.18-rc# and I'm not entirely sure what > > >> the procedure is here. Once I get a Reviewed-by or Acked-by and I push > > >> this to drm-intel-next (where it also is necessary), should I then also > > >> push it to drm-intel-fixes or will the current drm-intel-fixes > > >> maintainer pick it up? > > >> --- > > >> drivers/gpu/drm/i915/i915_reg.h | 4 ++-- > > >> 1 file changed, 2 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > >> index 51f46fe45c72..5f1f38684d65 100644 > > >> --- a/drivers/gpu/drm/i915/i915_reg.h > > >> +++ b/drivers/gpu/drm/i915/i915_reg.h > > >> @@ -4352,12 +4352,12 @@ > > >> #define _DSPAADDR 0x70184 > > >> #define _DSPASTRIDE 0x70188 > > >> #define _DSPAPOS 0x7018C /* reserved */ > > >> -#define DISP_POS_Y_MASK REG_GENMASK(31, 0) > > >> +#define DISP_POS_Y_MASK REG_GENMASK(31, 16) > > > > > > Doh. I guess I only tested it on plane A where the plane gets its size > > > from PIPESRC instead. And looks like the failure mode is such that > > > the likes of kms_plane/pixel-formats still gets consistent looking CRCs > > > even with the misconfigured plane size :/ > > > > > > Thanks for the fix. Pushed to drm-intel-next. > > > > Thank you pushing this out, will you (or someone else from Intel) > > also take care of getting this on its way to 5.18-rc# ? > > It has a fixes tag so it should get cherry-picked for fixes. Yeah, it sould get picked up for next week's drm-intel-fixes PR. For both drm-intel-next and drm-intel-gt-next, committers only push to the -next branches and the rest is handled by tooling and maintainers as long as the Fixes: tags are correct. If a Fixes: tag has been missed when committing, only then you need to manually let maintainers know to pick the patch up. Regards, Joonas > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 51f46fe45c72..5f1f38684d65 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4352,12 +4352,12 @@ #define _DSPAADDR 0x70184 #define _DSPASTRIDE 0x70188 #define _DSPAPOS 0x7018C /* reserved */ -#define DISP_POS_Y_MASK REG_GENMASK(31, 0) +#define DISP_POS_Y_MASK REG_GENMASK(31, 16) #define DISP_POS_Y(y) REG_FIELD_PREP(DISP_POS_Y_MASK, (y)) #define DISP_POS_X_MASK REG_GENMASK(15, 0) #define DISP_POS_X(x) REG_FIELD_PREP(DISP_POS_X_MASK, (x)) #define _DSPASIZE 0x70190 -#define DISP_HEIGHT_MASK REG_GENMASK(31, 0) +#define DISP_HEIGHT_MASK REG_GENMASK(31, 16) #define DISP_HEIGHT(h) REG_FIELD_PREP(DISP_HEIGHT_MASK, (h)) #define DISP_WIDTH_MASK REG_GENMASK(15, 0) #define DISP_WIDTH(w) REG_FIELD_PREP(DISP_WIDTH_MASK, (w))
Commit 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") introduced DISP_POS_Y and DISP_HEIGHT defines but accidentally set these their masks to REG_GENMASK(31, 0) instead of REG_GENMASK(31, 16). This breaks the primary display pane on at least pineview machines, fix the mask to fix the primary display pane only showing black. Tested on an Acer One AO532h with an Intel N450 SoC. Fixes: 428cb15d5b00 ("drm/i915: Clean up pre-skl primary plane registers") Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Note this fixes a regression in 5.18-rc# and I'm not entirely sure what the procedure is here. Once I get a Reviewed-by or Acked-by and I push this to drm-intel-next (where it also is necessary), should I then also push it to drm-intel-fixes or will the current drm-intel-fixes maintainer pick it up? --- drivers/gpu/drm/i915/i915_reg.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)