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 ?
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
