Re: [igt-dev] [PATCH i-g-t 4/6] tools/intel_watermark: Show whether each plane is enabled

Ville Syrjälä
2019-01-29T13:30:31Z

On Mon, Jan 28, 2019 at 10:53:33PM +0000, Souza, Jose wrote:
> On Mon, 2019-01-28 at 22:01 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > To make it easier to spot errors with watermarks vs. plane
> > being enabled/disabled indicate which planes are actually
> > enabled and which are not.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > tools/intel_watermark.c | 40 ++++++++++++++++++++++++++++++++++++++-
> > -
> > 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/intel_watermark.c b/tools/intel_watermark.c
> > index 97b769250927..8405d6489628 100644
> > --- a/tools/intel_watermark.c
> > +++ b/tools/intel_watermark.c
> > @@ -180,6 +180,20 @@ static const char *skl_wm_linetime_reg_name(int
> > pipe)
> > return reg_name;
> > }
> >
> > +static const char *skl_plane_ctl_reg_name(int pipe, int plane)
> > +{
> > + static char reg_name[32];
> > +
> > + if (plane == 0)
> > + snprintf(reg_name, sizeof(reg_name), "CUR_CTL_%c",
> > + pipe_name(pipe));
> > + else
> > + snprintf(reg_name, sizeof(reg_name),
> > "PLANE_CTL_%1d_%c",
> > + plane, pipe_name(pipe));
> > +
> > + return reg_name;
> > +}
> > +
> > static const char *skl_wm_reg_name(int pipe, int plane, int level)
> > {
> > static char reg_name[32];
> > @@ -231,6 +245,7 @@ static void skl_wm_dump(void)
> > uint32_t wm[num_levels][num_pipes][max_planes];
> > uint32_t wm_trans[num_pipes][max_planes];
> > uint32_t buf_cfg[num_pipes][max_planes];
> > + uint32_t plane_ctl[num_pipes][max_planes];
> > uint32_t wm_linetime[num_pipes];
> >
> > intel_register_access_init(intel_get_pci_device(), 0, -1);
> > @@ -243,6 +258,7 @@ static void skl_wm_dump(void)
> > for (plane = 0; plane < num_planes; plane++) {
> > addr = base_addr + pipe * 0x1000 + plane *
> > 0x100;
> >
> > + plane_ctl[pipe][plane] = read_reg(addr + 0x80);
> > wm_trans[pipe][plane] = read_reg(addr +
> > 0x00168);
> > buf_cfg[pipe][plane] = read_reg(addr +
> > 0x0017C);
> > for (level = 0; level < num_levels; level++) {
> > @@ -259,6 +275,19 @@ static void skl_wm_dump(void)
> > }
> > printf("\n\n");
> >
> > + for (plane = 0; plane < max_planes; plane++) {
> > + for (pipe = 0; pipe < num_pipes; pipe++) {
> > + if (plane >= skl_num_planes(devid, pipe))
> > + break;
> > +
> > + printf("%18s 0x%08x\t" ,
> > + skl_plane_ctl_reg_name(pipe, plane),
> > + plane_ctl[pipe][plane]);
> > + }
> > + printf("\n");
> > + }
> > + printf("\n");
> > +
> > for (plane = 0; plane < max_planes; plane++) {
> > for (level = 0; level < num_levels; level++) {
> > for (pipe = 0; pipe < num_pipes; pipe++) {
> > @@ -312,8 +341,15 @@ static void skl_wm_dump(void)
> > printf("LINETIME: %d (%.3f usec)\n", linetime,
> > linetime* 0.125f);
> >
> > printf("LEVEL");
> > - for (plane = 0; plane < num_planes; plane++)
> > - printf("%10s", skl_plane_name(pipe, plane));
> > + for (plane = 0; plane < num_planes; plane++) {
> > + if (plane == 0)
> > + enable =
> > REG_DECODE1(plane_ctl[pipe][plane], 0, 3) ||
> > + REG_DECODE1(plane_ctl[pipe][pla
> > ne], 5, 1);
>
> I guess we only enabled ARGB and AND/XOR cursors?! But why not keep it
> simple and check for REG_DECODE1(plane_ctl[pipe][plane], 0, 5)?

Because bits 3 and 4 aren't part of the cursor format. Well, apparently
they are now even though they are always 0. I wonder when that was
respecified... apparently for HSW. OK, been like that for a while then
:)

>
> Other than that LGTM:
>
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>
>
> > + else
> > + enable =
> > REG_DECODE1(plane_ctl[pipe][plane], 31, 1);
> > + printf("%9s%c", skl_plane_name(pipe, plane),
> > + endis_ast(enable));
> > + }
> > printf("\n");
> >
> > for (level = 0; level < num_levels; level++) {

--
Ville Syrjälä
Intel