[Intel-gfx] [PATCH] Revert "drm/i915: re-order if/else ladder for hpd_irq_setup"

[Ville Syrjälä]

On Mon, Nov 30, 2020 at 09:46:18PM -0800, Lucas De Marchi wrote:
> On Mon, Nov 30, 2020 at 07:46:39PM +0200, Ville Syrjälä wrote:
> >On Mon, Nov 30, 2020 at 09:31:04AM -0800, Lucas De Marchi wrote:
> >> On Mon, Nov 30, 2020 at 04:19:54PM +0200, Ville Syrjälä wrote:
> >> >On Fri, Nov 27, 2020 at 08:52:29PM -0800, Lucas De Marchi wrote:
> >> >> On Fri, Nov 27, 2020 at 02:57:48PM +0000, Chris Wilson wrote:
> >> >> >We now use ilk_hpd_irq_setup for all GMCH platforms that do not have
> >> >> >hotplug. These are early gen3 and gen2 devices that now explode on boot
> >> >> >as they try to access non-existent registers.
> >> >>
> >> >> humn... true, my bad. But I don't think a revert is the right fix. It
> >> >> would be much better if we would not be setting up the hpd setup
> >> >> function at all for platforms that do not have hotplug. I think a
> >> >> separate early check for I915_HAS_HOTPLUG() would be deserved.
> >> >
> >> >I think it generally leads to much less convoluted logic when we keep
> >> >gmch vs. rest separate. So I'm confused as to what we're even trying
> >> >to achieve here?
> >>
> >> 1) Stop trying to setup hotplug in a platform that doesn't have hotplug
> >> was the main focus. Later it would be better to move some of these
> >> hotplug to display/  as they are clearly display related and account for
> >> a great portion of i915_irq.c.
> >>
> >> I left the I915_HAS_HOTPLUG() in the middle by
> >> mistake, it should had been an earlier call.
> >>
> >> 2) semi-related is the move of GMCH to the middle and I guess this is
> >> what you're complaining here. I find it's cumbersome to have it
> >> separate as we go and extend these checks for newer platforms. Almost
> >> everywhere we settled on having last platform first in the if/else
> >> ladders - this makes it much more clear on how/where to add a new
> >> platform.
> >
> >You never touch the gmch path for new platforms. What could be more
> >clear than that?
> 
> the second level branch mixing the code path for new and old platform
> instead of following the convention we settled on.

The convention I care about most is "clear code" which in many
cases is best achieved with gmch vs. not split.

> But I'm ok with
> moving it back as a HAS_* check in the middle of GEN_* check is proving
> controversial.
> 
> Lucas De Marchi
> 
> >
> >-- 
> >Ville Syrjälä
> >Intel

-- 
Ville Syrjälä
Intel