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

[Jyri Sarha]

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.

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.

Best regards,
Jyri




-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki