On Mon, 16 Sep 2019, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> 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.
This patch tries to do the decoupling with the least amount of
functional changes to the disabled display case (no matter how broken
the status quo is), but also without sprinkling INTEL_DISPLAY_ENABLED()
checks at every for_each_display{,_masked}. Some arbitrarily chosen
balance.
After this, I'm hoping we can develop the cases independently, without
each of them stomping on the other's feet. Who knows, maybe the next
patch should revert the hunk you quote above. I do know I'm not signing
up for fixing everything about the disabled display path, at least right
away, but the point is to remove obstacles.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
