Hi,

On Mon, Dec 14, 2020 at 08:55:36PM +0200, Tomi Valkeinen wrote:
> On 14/12/2020 19:39, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Dec 08, 2020 at 02:28:53PM +0200, Tomi Valkeinen wrote:
> >> ULPS doesn't work, and I have been unable to get it to work. As ULPS
> >> is a minor power-saving feature which requires substantial amount of
> >> non-trivial code, and we have trouble just getting and
> >> keeping DSI working at all, remove ULPS support.
> >>
> >> When the DSI driver works reliably for command and video mode displays,
> >> someone interested can add it back.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> > 
> > Is it really 'minor power-saving'? If I search for DSI and ULPS among
> > the first results is a TI datasheet for SN65DSI84, which claims device
> > active current in the more than 100mA range and ULPS current in the
> > less than 10mA range.
> 
> I don't have any numbers, just my guesses. For videomode displays
> or command mode displays in active use, I don't think ULPS matters
> much. The link is mostly not in ULPS. And if the display is
> blanked, things are off, so again not in ULPS.
> 
> It's only for command mode displays, when updated rarely, where I
> think ULPS matters. Which, of course, is probably not unusual use
> case if you have a cmdmode display. But whether OMAP DSI power
> savings matches SN65DSI84, I have no clue.

Right. FWIW I don't expect savings to be as big as this. The
comparision is not "active current", but "idle current" since
we do disable the clocks among other things. Considering the
amount of power-saving is pure guess-work I suggest to rephrase
the commit message to something like this:

ULPS is a niche power-saving optimization feature only
affecting enabled command mode panels showing a static
picture. It never worked with the omapdrm driver and I have
been unable to get it working. Keeping DSI command mode panels
working is hard enough without this, so remove ULPS support.

FWIW I'm fine with this being removed:

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> > Considering all known omapdrm DSI users are battery powered devices
> > caring for saving as much power as possible, it might be good to just
> > keep this until it is being fixed considering this is very close to
> > the end of the series anyways?
> 
> I don't like to leave known to be broken code around, unless
> someone has plans to work on it. I wouldn't be surprised to see
> ULPS still broken two years from now =). It should be trivial to
> add the relevant bits back later.

Ack.

> But I can leave it here if you think it's better, presuming it
> doesn't have bigger conflicts with the 29/29 or break anything.
> However, I have only a few days left in TI, which is why I'm
> rushing here a bit (*). If I hit problems, I either have to drop
> the whole series, or push it in its current form.
> 
>  Tomi
> 
> (*) But I will fix possible issues caused by my push, of course.

Best of luck on whatever you do next!

-- Sebastian