Re: [PATCH 06/12] drm/i915/bdw: Add 4 level switching infrastructure

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 03, 2015 at 06:31:03PM +0530, akash goel wrote:
> On Fri, Feb 20, 2015 at 11:16 PM, Michel Thierry
> <michel.thierry@intel.com> wrote:
> > +static void gen8_map_page_directory(struct i915_page_directory_pointer_entry *pdp,
> > +                                   struct i915_page_directory_entry *pd,
> > +                                   int index,
> > +                                   struct drm_device *dev)
> > +{
> > +       gen8_ppgtt_pdpe_t *page_directorypo;
> > +       gen8_ppgtt_pdpe_t pdpe;
> > +
> > +       /* We do not need to clflush because no platform requiring flush
> > +        * supports 64b pagetables. */
> 
> Would be more appropriate to place this comment, either after the ‘if’
> condition or
> at the end of the function (where clflush would have been placed, had
> LLC not been there for platforms supporting 64 bit).
> And same comment can be probably added, at the end of
> gen8_map_page_directory_pointer function also.
> 
> > +       if (!USES_FULL_48BIT_PPGTT(dev))
> > +               return;

Ok this on a lot of levels:
- a function calle map_something, which doesn't actually return a map.
  Must be renamed asap to something that makes sense, in the kernel
  everything call map_foo actually maps foo somewhere and returns where.
- The comment is fairly useless since it doesn't mention which platforms
  flushing is required on. Either we need to split functions up more into
  4G and 48bit variants if this difference is due to the pagetable layout.
  Or we need to replace it with appropriate platform checks.

Or maybe this if is just dead code and should be remove entirely?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx