Re: [PATCH 2/2] drm/i915: only assign dev_priv->pch_id on successful pch detection

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 26 Jan 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting David Weinehall (2018-01-26 15:53:53)
>> On Fri, Jan 26, 2018 at 04:48:05PM +0200, Jani Nikula wrote:
>> > Currently pch_id gets assigned also when there's no pch. It doesn't look
>> > like it makes a difference, but do the right thing anyway.
>> 
>> Makes sense.
>> 
>> Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>
>
> I suspect this is the one that upset gvt.

Oh, right. I failed to notice intel_virt_detect_pch() also sets pch_id
so this patch clobbers the information.

What a crap interface for a function. Either you return stuff to be set
by the caller, or you set stuff within, but you don't split the
responsibility between layers. And you certainly don't do that only in
some cases. Ugh.

BR,
Jani.

>  
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index 3e8664de025d..0332e315650c 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -183,8 +183,6 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>> >  
>> >               id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
>> >  
>> > -             dev_priv->pch_id = id;
>> > -
>> >               if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
>> >                       dev_priv->pch_type = PCH_IBX;
>> >                       DRM_DEBUG_KMS("Found Ibex Peak PCH\n");
>> > @@ -272,6 +270,8 @@ static void intel_detect_pch(struct drm_i915_private *dev_priv)
>> >                       continue;
>> >               }
>> >  
>> > +             dev_priv->pch_id = id;
>> > +
>> >               break;
>> >       }
>> >       if (!pch)
>> > -- 
>> > 2.11.0
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx