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

[Laurent Pinchart]

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