Re: [PATCH v4 4/8] drm/i915/gen9: WM memory bandwidth related workaround

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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