| Message ID | 1447333880-16555-1-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);