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.