Re: [Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector
Daniel Vetter writes: Hm, I think to address Chris' concern we should split this into two pieces: connector_break_all links which only breaks connector-encoder stuff, used in both places as is. And a new encoder_break_all links which we'll use in sanitize_crtc in a new encoder loop. I've got a different solution, although it requires a bit more code: Instead of having a single break_all_links() function I move the link breaking code into the two functions where it is used (ie sanitize_encoder() and sanitize_crtc()). This gives one a bit more flexibility in implementing what is needed and makes the code a bit clearer. I will send later - once I had the opportunity to test. Cheers, Egbert. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector
On Mon, Apr 14, 2014 at 07:26:08PM +0200, Egbert Eich wrote: Depending on the SDVO output_flags SDVO may have multiple connectors linking to the same encoder (in intel_connector-encoder-base). Only one of those connectors should be active (ie link to the encoder thru drm_connector-encoder. If intel_connector_break_all_links() is called from intel_sanitize_crtc() we may brake the crtc connection of an encoder thru an inactive connector in which case intel_connector_break_all_links() will not be called again for the active connector due to if (connector-encoder-base.crtc != crtc-base) continue; in intel_sanitize_crtc(). This will however leave the drm_connector-encoder linkage for this active connector in place. Subsequently this will cause multiple warnings in intel_connector_check_state() to trigger and the driver will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL). To avoid this break the encoder links only if the connector is active (ie. has drm_connector-encoder set). Signed-off-by: Egbert Eich e...@suse.de This just leaves me with a nagging doubt that not all links will then be broken. Do we have sufficient sanity checks to detect the obverse? Would it be worth adding a second pass over the connector_list to assert that all links are then broken? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector
Chris Wilson writes: On Mon, Apr 14, 2014 at 07:26:08PM +0200, Egbert Eich wrote: Depending on the SDVO output_flags SDVO may have multiple connectors linking to the same encoder (in intel_connector-encoder-base). Only one of those connectors should be active (ie link to the encoder thru drm_connector-encoder. If intel_connector_break_all_links() is called from intel_sanitize_crtc() we may brake the crtc connection of an encoder thru an inactive connector in which case intel_connector_break_all_links() will not be called again for the active connector due to if (connector-encoder-base.crtc != crtc-base) continue; in intel_sanitize_crtc(). This will however leave the drm_connector-encoder linkage for this active connector in place. Subsequently this will cause multiple warnings in intel_connector_check_state() to trigger and the driver will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL). To avoid this break the encoder links only if the connector is active (ie. has drm_connector-encoder set). Signed-off-by: Egbert Eich e...@suse.de This just leaves me with a nagging doubt that not all links will then be broken. Do we have sufficient sanity checks to detect the obverse? Would it be worth adding a second pass over the connector_list to assert that all links are then broken? Possibly. Maybe we should rework intel_sanitize_encoder() a bit: loop over all drm_connectors, see if a drm_connector-encoder points to the encoder in question and make sure that the intel_encoder state (ie connectors_active and base.crtc) is in sync with the results. I'll think about it some more ... Cheers, Egbert. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector
On Mon, Apr 14, 2014 at 07:26:08PM +0200, Egbert Eich wrote:
Depending on the SDVO output_flags SDVO may have multiple connectors
linking to the same encoder (in intel_connector-encoder-base).
Only one of those connectors should be active (ie link to the encoder
thru drm_connector-encoder.
If intel_connector_break_all_links() is called from intel_sanitize_crtc()
we may brake the crtc connection of an encoder thru an inactive connector
in which case intel_connector_break_all_links() will not be called again
for the active connector due to
if (connector-encoder-base.crtc != crtc-base)
continue;
in intel_sanitize_crtc().
This will however leave the drm_connector-encoder linkage for this
active connector in place. Subsequently this will cause multiple
warnings in intel_connector_check_state() to trigger and the driver
will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).
To avoid this break the encoder links only if the connector is active
(ie. has drm_connector-encoder set).
Signed-off-by: Egbert Eich e...@suse.de
---
drivers/gpu/drm/i915/intel_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..041f847 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11390,6 +11390,8 @@ static void
intel_connector_break_all_links(struct intel_connector *connector)
{
connector-base.dpms = DRM_MODE_DPMS_OFF;
+ if (!connector-base.encoder)
+ return;
Hm, I think to address Chris' concern we should split this into two
pieces: connector_break_all links which only breaks connector-encoder
stuff, used in both places as is. And a new encoder_break_all links which
we'll use in sanitize_crtc in a new encoder loop.
Really nice catch though!
-Daniel
connector-base.encoder = NULL;
connector-encoder-connectors_active = false;
connector-encoder-base.crtc = NULL;
--
1.8.4.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
[Intel-gfx] [PATCH 1/2] drm/i915: Only break encoder linked when linked to connector
Depending on the SDVO output_flags SDVO may have multiple connectors
linking to the same encoder (in intel_connector-encoder-base).
Only one of those connectors should be active (ie link to the encoder
thru drm_connector-encoder.
If intel_connector_break_all_links() is called from intel_sanitize_crtc()
we may brake the crtc connection of an encoder thru an inactive connector
in which case intel_connector_break_all_links() will not be called again
for the active connector due to
if (connector-encoder-base.crtc != crtc-base)
continue;
in intel_sanitize_crtc().
This will however leave the drm_connector-encoder linkage for this
active connector in place. Subsequently this will cause multiple
warnings in intel_connector_check_state() to trigger and the driver
will eventually die in drm_encoder_crtc_ok() (because of crtc == NULL).
To avoid this break the encoder links only if the connector is active
(ie. has drm_connector-encoder set).
Signed-off-by: Egbert Eich e...@suse.de
---
drivers/gpu/drm/i915/intel_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..041f847 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11390,6 +11390,8 @@ static void
intel_connector_break_all_links(struct intel_connector *connector)
{
connector-base.dpms = DRM_MODE_DPMS_OFF;
+ if (!connector-base.encoder)
+ return;
connector-base.encoder = NULL;
connector-encoder-connectors_active = false;
connector-encoder-base.crtc = NULL;
--
1.8.4.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
