Em Qui, 2016-11-10 às 11:24 +0530, Mahesh Kumar escreveu:
> Hi,
>
>
>
(removed a bunch of stuff here)
> >
> > >
> >
> >
> > >
> > > + bool y_tile_enabled = false;
> > > +
> > if (!platforms_that_require_the_wa) {
> > wa = WATERMARK_WA_NONE;
> > return;
> > }
> this function is not called by any GEN other than GEN9, will it be ok
> to
> add "if (!GEN9)" check?
Well, there's Gemnilake support floating around the mailing list now...
> >
> > >
> > > + if (!memdev_info->valid)
> > > + goto exit;
> > Our default behavior should be to apply the WA then in doubt, not
> > to avoid it, so the return value here and in the other error cases
> > should be WATERAMARK_WA_Y_TILED.
> >
> > Also, you can avoid the gotos by just setting mem_wa at the
> > beginning
> > of the function, then you can just "return" later instead of goto.
> >
> >
> > >
> > > +
> > > + system_bw = memdev_info->mem_speed_khz * memdev_info-
> > > >
> > > > num_channels *
> > > + memdev_info-
> > > >channel_width_bytes;
> > > +
> > > + if (!system_bw)
> > > + goto exit;
> > This shouldn't be possible. Otherwise, the previous patch needs to
> > be
> > fixed in a way to not allow system_bw to end up as zero. Please
> > either
> > remove the check or change to "if (WARN_ON(!system_bw))".
> yes, ideally this should not be possible, but what if because of any
> reason BIOS is not writing the register OR we don't have BIOS
> altogether?
Then the previous patch should be able to deal with it, and leave us
with memdev_info->valid=false.
>
> If we add WARN_ON here, it'll flood the logs, we can add
> WARN_ON_ONCE,
> but we are anyway printing the warning in previous patch, if
> frequency
> or any other variable calculated is ZERO, IMO will add more prints
> in
> previous patch instead of adding here.
Usually we add WARNs when we expect them to never ever be triggered. So
instead of making it WARN_ON_ONCE, we should make sure the condition is
never met: if the memory info is bad, set memdev_info->valid to false.
Thanks,
Paulo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
