[Intel-gfx] [RESEND] drm/i915: stop conflating HAS_DISPLAY() and disabled display

[Jani Nikula]

On Mon, 16 Sep 2019, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Mon, Sep 16, 2019 at 05:05:10PM +0300, Jani Nikula wrote:
>> On Mon, 16 Sep 2019, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > Quoting Jani Nikula (2019-09-16 10:29:01)
>> >> Stop setting ->pipe_mask to zero when display is disabled, allowing us
>> >> to have different code paths for not actually having display hardware,
>> >> and having display hardware disabled. This lets us develop those two
>> >> avenues independently.
>> >> 
>> >> There are no functional changes for when there is no display. However,
>> >> all uses of for_each_pipe() and for_each_pipe_masked() will start
>> >> running for the disabled display case. Put one of the more significant
>> >> ones behind checks for INTEL_DISPLAY_ENABLED(), otherwise the cases
>> >> should not be hit with disabled display, or they seem benign. Fingers
>> >> crossed.
>> >> 
>> >> All in all, this might not be the ideal solution. In fact we may have
>> >> had something along the lines of this in the past, but we ended up
>> >> conflating the two cases. Possibly even by recommendation by yours
>> >> truly; I did not dare dig up that part of the history. But the perfect
>> >> is the enemy of the good, this is a straightforward change, and lets us
>> >> get actual work done in both fronts without interfering with each other.
>> >> 
>> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> >> Cc: José Roberto de Souza <jose.souza at intel.com>
>> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_display.c | 12 +++++++-----
>> >>  drivers/gpu/drm/i915/intel_device_info.c     |  8 ++------
>> >>  2 files changed, 9 insertions(+), 11 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> >> index e75945a53e06..ac24f96582ca 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> >> @@ -16281,11 +16281,13 @@ int intel_modeset_init(struct drm_device *dev)
>> >>                       INTEL_NUM_PIPES(dev_priv),
>> >>                       INTEL_NUM_PIPES(dev_priv) > 1 ? "s" : "");
>> >>  
>> >> -       for_each_pipe(dev_priv, pipe) {
>> >> -               ret = intel_crtc_init(dev_priv, pipe);
>> >> -               if (ret) {
>> >> -                       drm_mode_config_cleanup(dev);
>> >> -                       return ret;
>> >> +       if (HAS_DISPLAY(dev_priv) && INTEL_DISPLAY_ENABLED(dev_priv)) {
>> >> +               for_each_pipe(dev_priv, pipe) {
>> >> +                       ret = intel_crtc_init(dev_priv, pipe);
>> >
>> > What direction are you planning to take, avoid enabling anything related
>> > to display? My worry is that in
>> >
>> > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14418/fi-bsw-kefka/igt@i915_selftest@live_gt_timelines.html
>> >
>> > we still see weird events like
>> >
>> > <7> [444.313823] [drm:i915_redisable_vga_power_on [i915]] Something enabled VGA plane, disabling it
>> >
>> > and I'm not sure how you intend to curtail that. (Or if that's even
>> > possible.)
>> 
>> The main goal here (in this specific patch) is to decouple disabled but
>> existing display from non-existing display. That lets us develop the two
>> cases independently, and I acknowledge I may have been simple minded
>> enough at some point to believe they could be put in the same bucket.
>
> What's the actual use case for the "disabled but existing display"?
>
> So far I've thought that the only use case is regression testing
> of the "hw has no display" case on hw which in fact has a display.
> If we have separate codepaths we can't do that effectively. At
> which point we might as well get rid of the "disable display"
> capability entirely.

The problem seems to be that we simply can't have the same code paths,
with e.g. bios enabling display hw behind our backs. And patching either
code path with just one knob causes problems to the other. So I want to
decouple the two to make our lives easier for the immediate future.

If we can think of better ways to do this, and better utilize shared
code paths, the decoupling doesn't really prevent us from doing that
either. One idea was probing, then disabling everything, and either
using -EPROBE_DEFER or reprobing and then pretending there is no
display.

But right now this is getting in the way. Need to unblock work. Later we
can re-evaluate whether we need display disable or not and how to best
support it.

Pushed the patch, thanks for review. Does not have to mean end of
discussion though.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center