| Message ID | 20180325015240.75464-5-stschake@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stefan, On 25 March 2018 at 02:52, Stefan Schake <stschake@gmail.com> wrote: > +static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4) > +{ > + return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS), > + SCALER_OLEDOFFS_DISPFIFO); > +} This needs to be managed as a global resource through atomic state objects, rather than checking the current hardware state. Cheers, Daniel
Hey Daniel, On Sun, Mar 25, 2018 at 10:01 AM, Daniel Stone <daniel@fooishbar.org> wrote: > Hi Stefan, > > On 25 March 2018 at 02:52, Stefan Schake <stschake@gmail.com> wrote: >> +static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4) >> +{ >> + return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS), >> + SCALER_OLEDOFFS_DISPFIFO); >> +} > > This needs to be managed as a global resource through atomic state > objects, rather than checking the current hardware state. Do you mean as a property or some such that is accessible to userland or merely that this could be raced? I haven't had much luck finding examples for resources shared between CRTCs in the current tree. My understanding here was that if userland commits on CRTC B after a check-only on A, we are no longer bound by the earlier result for the check-only. Otherwise, I would have to already commit my CTM block to one CRTC at check (possibly check only) time. Thanks, Stefan
On Sun, Mar 25, 2018 at 08:14:35PM +0200, Stefan Schake wrote: > Hey Daniel, > > On Sun, Mar 25, 2018 at 10:01 AM, Daniel Stone <daniel@fooishbar.org> wrote: > > Hi Stefan, > > > > On 25 March 2018 at 02:52, Stefan Schake <stschake@gmail.com> wrote: > >> +static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4) > >> +{ > >> + return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS), > >> + SCALER_OLEDOFFS_DISPFIFO); > >> +} > > > > This needs to be managed as a global resource through atomic state > > objects, rather than checking the current hardware state. > > Do you mean as a property or some such that is accessible to userland > or merely that this could be raced? > > I haven't had much luck finding examples for resources shared between CRTCs > in the current tree. My understanding here was that if userland commits > on CRTC B after a check-only on A, we are no longer bound by the earlier > result for the check-only. Otherwise, I would have to already commit my > CTM block to one CRTC at check (possibly check only) time. https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-private-state since you only have one CTM it's a shared resource which internally needs to be tracked as a driver private thing. Cheers, Daniel
On 26 March 2018 at 09:29, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sun, Mar 25, 2018 at 08:14:35PM +0200, Stefan Schake wrote: >> On Sun, Mar 25, 2018 at 10:01 AM, Daniel Stone <daniel@fooishbar.org> wrote: >> > On 25 March 2018 at 02:52, Stefan Schake <stschake@gmail.com> wrote: >> >> +static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4) >> >> +{ >> >> + return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS), >> >> + SCALER_OLEDOFFS_DISPFIFO); >> >> +} >> > >> > This needs to be managed as a global resource through atomic state >> > objects, rather than checking the current hardware state. >> >> Do you mean as a property or some such that is accessible to userland >> or merely that this could be raced? >> >> I haven't had much luck finding examples for resources shared between CRTCs >> in the current tree. My understanding here was that if userland commits >> on CRTC B after a check-only on A, we are no longer bound by the earlier >> result for the check-only. Otherwise, I would have to already commit my >> CTM block to one CRTC at check (possibly check only) time. > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#handling-driver-private-state > > since you only have one CTM it's a shared resource which internally needs > to be tracked as a driver private thing. Indeed, the above is exactly what I meant. Checking based on the hardware status will falsely succeed if you go from having zero CRTCs using CTM, to multiple CRTCs using CTM, in a single atomic commit, as the hardware status won't have changed in time. Cheers, Daniel
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index bafb0102fe1d..180b93ec447e 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -676,10 +676,17 @@ static enum drm_mode_status vc4_crtc_mode_valid(struct drm_crtc *crtc, return MODE_OK; } +static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4) +{ + return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS), + SCALER_OLEDOFFS_DISPFIFO); +} + static int vc4_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state); + struct drm_crtc_state *old_state = crtc->state; struct drm_device *dev = crtc->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct drm_plane *plane; @@ -701,6 +708,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc, */ if (vc4_crtc_atomic_check_ctm(state)) return -EINVAL; + + /* We can only enable CTM for one fifo or CRTC at a time */ + if (!old_state->ctm && vc4_crtc_get_ctm_fifo(vc4)) + return -EINVAL; } drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state)
We only have one hardware block to do the CTM and need to reject attempts to enable it for multiple CRTCs simultaneously. Signed-off-by: Stefan Schake <stschake@gmail.com> --- v2: No change drivers/gpu/drm/vc4/vc4_crtc.c | 11 +++++++++++ 1 file changed, 11 insertions(+)