[RFC PATCHv2 4/9] drm/tidss: add new driver for TI Keystone platforms

[Daniel Vetter]

On Wed, Nov 7, 2018 at 3:10 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Wed, Nov 7, 2018 at 2:40 PM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > Hi Jyri,
> >
> > (CC'ing Daniel Vetter)
> >
> > On Wednesday, 31 October 2018 18:24:28 EET Jyri Sarha wrote:
> > > Hi Laurent,
> > > Tomi is busy with other things so I have taken over applying these
> > > comments. The rest is more or less clear, or commented by Tomi, but this
> > > is something have not addressed:
> > >
> > > On 30/07/18 17:12, Laurent Pinchart wrote:
> > > >> +static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
> > > >> +                              struct drm_crtc_state *old_crtc_state)
> > > >> +{
> > > >> +  struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> > > >> +  struct drm_device *ddev = crtc->dev;
> > > >> +  struct tidss_device *tidss = ddev->dev_private;
> > > >> +
> > > >> +  dev_dbg(ddev->dev, "%s, crtc enabled %d, event %p\n",
> > > >> +          __func__, tcrtc->enabled, crtc->state->event);
> > > >> +
> > > >> +  /* Only flush the CRTC if it is currently enabled. */
> > > >> +  if (!tcrtc->enabled)
> > > >
> > > > In atomic drivers state should be stored in state structures. You could
> > > > check old_crtc_state for this and remove the enabled field in struct
> > > > tidss_crtc.
> > >
> > > The variable is need for tracking the HW state trough the state
> > > transition. I do not know which state variable I should use to keep that
> > > state information stored trough the process where one state changes into
> > > another.
> > >
> > > The drm_crtc_state already contains couple of variables describing
> > > whether crtc is enabled or not, or if the mode is going to change in the
> > > state transition (giving a hint that crtc is going go through
> > > disable-enable cycle). I tried to use all of those, and the old state
> > > variable, to accomplish the same behaviour as the current code has, but
> > > I could not.
> > >
> > > One of the problematic cases was a new drm client making an atomic
> > > commit, the old one being bf-console, with the same mode as the one was
> > > using. In that situation the crtc goes trough disable-enable cycle, but
> > > I could not find any way to detect the situation from the old and new
> > > crtc state. Enable-disable cycle means that we should not flip the
> > > go-bit, but just configure everything and enable the crtc, e.g skip the
> > > atomic flush and wait for enable instead.
> >
> > Thanks for the report. If we can't detect this from the drm_crtc_state, I
> > think it's a shortcoming of the KMS core, and should be fixed. Daniel, what's
> > your opinion ?
>
> drm_atomic_crtc_needs_modeset() not good enough? Also please nota that
> using atomic helpers when they don't fit your hw's need is very much
> discouraged. They do take care of some of the boring book-keeping for
> common cases. But eventually all big drivers end up writing partially
> their own frameworks, only picking the pieces from the shared atomic
> helpers that do fully fit. If you want to know how, check out how the
> helpers decide when to call the ->enable and ->disable hooks.

Another option: Overwrite the top-level atomic_check function, compute
when exactly you need to set the go bit and when not (you have both
old and new state, this fully describes the transition). And then use
that special derived state to decided at runtime whether to flush the
go bit or not.
-Daniel

> In any case, fully agreed with Laurent, don't do this.
> -Daniel
>
> > > In any case this is for HW state, not for DRM state tracking. I could
> > > add a call back to dispc ops for asking if the video port is enabled and
> > > use that instead if you think that is more formally correct.
> >
> > I don't think a callback is worth it. The idea behind drm_crtc_state, if I
> > understand it correctly, is to track all state, software and hardware. One
> > option could be to extent drm_crtc_state (as in subclassing the object) with a
> > custom hardware enable field, but I thought this would be covered by the
> > standard fields.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch