* [PATCH] drm/exynos: only run atomic_check() if crtc is active @ 2015-11-12 13:11 Gustavo Padovan 2015-11-12 13:49 ` Thierry Reding 0 siblings, 1 reply; 6+ messages in thread From: Gustavo Padovan @ 2015-11-12 13:11 UTC (permalink / raw) To: linux-samsung-soc; +Cc: tjakobi, Gustavo Padovan, dri-devel From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace direct cross-driver call with drm mode). The whole atomic update was failing if the hdmi display is not present/active. Add a test to only run atomic_check() if the CRTC is active. Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> --- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index b3ba27f..1d3ca0a 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc, { struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); + if (!state->active) + return 0; + if (exynos_crtc->ops->atomic_check) return exynos_crtc->ops->atomic_check(exynos_crtc, state); -- 2.1.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active 2015-11-12 13:11 [PATCH] drm/exynos: only run atomic_check() if crtc is active Gustavo Padovan @ 2015-11-12 13:49 ` Thierry Reding 2015-11-17 10:06 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Thierry Reding @ 2015-11-12 13:49 UTC (permalink / raw) To: Gustavo Padovan; +Cc: tjakobi, linux-samsung-soc, Gustavo Padovan, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 1175 bytes --] On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace > direct cross-driver call with drm mode). The whole atomic update > was failing if the hdmi display is not present/active. Add a test > to only run atomic_check() if the CRTC is active. > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > --- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > index b3ba27f..1d3ca0a 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc, > { > struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > > + if (!state->active) > + return 0; > + > if (exynos_crtc->ops->atomic_check) > return exynos_crtc->ops->atomic_check(exynos_crtc, state); > This looks like something that the core should be doing. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active 2015-11-12 13:49 ` Thierry Reding @ 2015-11-17 10:06 ` Daniel Vetter 2015-11-17 14:19 ` Andrzej Hajda 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2015-11-17 10:06 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-samsung-soc, Gustavo Padovan, dri-devel, tjakobi On Thu, Nov 12, 2015 at 02:49:29PM +0100, Thierry Reding wrote: > On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace > > direct cross-driver call with drm mode). The whole atomic update > > was failing if the hdmi display is not present/active. Add a test > > to only run atomic_check() if the CRTC is active. > > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > --- > > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > > index b3ba27f..1d3ca0a 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > > @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc, > > { > > struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > > > > + if (!state->active) > > + return 0; > > + > > if (exynos_crtc->ops->atomic_check) > > return exynos_crtc->ops->atomic_check(exynos_crtc, state); > > > > This looks like something that the core should be doing. Nah, this is a bug in exynos atomic_check code. Drivers _must_ check state irrespective of state->active - if they forgoe any checking when active = false then legacy DPMS On might spuriuosly fail, which will upset userspace. There shouldn't be any exceptions to this rule. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active 2015-11-17 10:06 ` Daniel Vetter @ 2015-11-17 14:19 ` Andrzej Hajda 2015-11-17 16:12 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Andrzej Hajda @ 2015-11-17 14:19 UTC (permalink / raw) To: Daniel Vetter, Thierry Reding Cc: tjakobi, linux-samsung-soc, Gustavo Padovan, dri-devel, Szyprowski, Marek Hi Daniel, On 11/17/2015 11:06 AM, Daniel Vetter wrote: > On Thu, Nov 12, 2015 at 02:49:29PM +0100, Thierry Reding wrote: >> On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote: >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> >>> Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace >>> direct cross-driver call with drm mode). The whole atomic update >>> was failing if the hdmi display is not present/active. Add a test >>> to only run atomic_check() if the CRTC is active. >>> >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>> index b3ba27f..1d3ca0a 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c >>> @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc, >>> { >>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); >>> >>> + if (!state->active) >>> + return 0; >>> + >>> if (exynos_crtc->ops->atomic_check) >>> return exynos_crtc->ops->atomic_check(exynos_crtc, state); >>> >> >> This looks like something that the core should be doing. > > Nah, this is a bug in exynos atomic_check code. Drivers _must_ check state > irrespective of state->active - if they forgoe any checking when active = > false then legacy DPMS On might spuriuosly fail, which will upset > userspace. There shouldn't be any exceptions to this rule. > > Cheers, Daniel > What about the situation when we have two display pipelines with separate crtcs: - one for panel, fimd->dsi->panel, - one for hdmi, mixer->hdmi->TV. Since TV is not connected mixer state have mode initially filled with zeros and active field set to 0. How should we handle situation if userspace tries to enable dpms on HDMI connector? How should we handle situation userspace tries to start panel pipeline? In this case atomic_check for mixer is called also, but since it will not be used and its state will not be changed it should not return error, am I right? Regards Andrzej _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active 2015-11-17 14:19 ` Andrzej Hajda @ 2015-11-17 16:12 ` Daniel Vetter 2015-11-20 9:15 ` Andrzej Hajda 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2015-11-17 16:12 UTC (permalink / raw) To: Andrzej Hajda Cc: Daniel Vetter, Thierry Reding, linux-samsung-soc, Gustavo Padovan, dri-devel, tjakobi, Szyprowski, Marek On Tue, Nov 17, 2015 at 03:19:35PM +0100, Andrzej Hajda wrote: > Hi Daniel, > > On 11/17/2015 11:06 AM, Daniel Vetter wrote: > > On Thu, Nov 12, 2015 at 02:49:29PM +0100, Thierry Reding wrote: > >> On Thu, Nov 12, 2015 at 11:11:20AM -0200, Gustavo Padovan wrote: > >>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> > >>> Fixes an regression added by 3ae2436 (drm/exynos/mixer: replace > >>> direct cross-driver call with drm mode). The whole atomic update > >>> was failing if the hdmi display is not present/active. Add a test > >>> to only run atomic_check() if the CRTC is active. > >>> > >>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > >>> --- > >>> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > >>> index b3ba27f..1d3ca0a 100644 > >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c > >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c > >>> @@ -55,6 +55,9 @@ static int exynos_crtc_atomic_check(struct drm_crtc *crtc, > >>> { > >>> struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc); > >>> > >>> + if (!state->active) > >>> + return 0; > >>> + > >>> if (exynos_crtc->ops->atomic_check) > >>> return exynos_crtc->ops->atomic_check(exynos_crtc, state); > >>> > >> > >> This looks like something that the core should be doing. > > > > Nah, this is a bug in exynos atomic_check code. Drivers _must_ check state > > irrespective of state->active - if they forgoe any checking when active = > > false then legacy DPMS On might spuriuosly fail, which will upset > > userspace. There shouldn't be any exceptions to this rule. > > > > Cheers, Daniel > > > > What about the situation when we have two display pipelines > with separate crtcs: > - one for panel, fimd->dsi->panel, > - one for hdmi, mixer->hdmi->TV. > > Since TV is not connected mixer state have mode initially filled > with zeros and active field set to 0. How should we handle situation > if userspace tries to enable dpms on HDMI connector? > How should we handle situation userspace tries to start panel pipeline? > In this case atomic_check for mixer is called also, but since > it will not be used and its state will not be changed it > should not return error, am I right? Check crtc_state->enable, skip if false. That's the "is this pipeline configured" knob. For plane/connector it's state->crtc, but with the same role. I guess we could check that in the helpers, but we need to be careful to still call ->atomic_check for the disabling transition, in case userspace is asking for a vblank-synced flip that disables a plane but somehow that's not possible. I somewhat prefer to handle that all in drivers though. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/exynos: only run atomic_check() if crtc is active 2015-11-17 16:12 ` Daniel Vetter @ 2015-11-20 9:15 ` Andrzej Hajda 0 siblings, 0 replies; 6+ messages in thread From: Andrzej Hajda @ 2015-11-20 9:15 UTC (permalink / raw) To: Daniel Vetter Cc: linux-samsung-soc, dri-devel, tjakobi, Gustavo Padovan, Szyprowski, Marek On 11/17/2015 05:12 PM, Daniel Vetter wrote: (...) > > Check crtc_state->enable, skip if false. That's the "is this pipeline > configured" knob. For plane/connector it's state->crtc, but with the same > role. > > I guess we could check that in the helpers, but we need to be careful to > still call ->atomic_check for the disabling transition, in case userspace > is asking for a vblank-synced flip that disables a plane but somehow > that's not possible. I somewhat prefer to handle that all in drivers > though. > -Daniel > Thanks for the explanation, Gustavo will you update your patch? Regards Andrzej _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-20 9:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-12 13:11 [PATCH] drm/exynos: only run atomic_check() if crtc is active Gustavo Padovan 2015-11-12 13:49 ` Thierry Reding 2015-11-17 10:06 ` Daniel Vetter 2015-11-17 14:19 ` Andrzej Hajda 2015-11-17 16:12 ` Daniel Vetter 2015-11-20 9:15 ` Andrzej Hajda
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.