On Tue, Oct 27, 2020 at 08:23:18PM +0100, Daniel Vetter wrote:
> On Tue, Oct 27, 2020 at 06:14:59PM +0100, Thierry Reding wrote:
> > On Tue, Oct 27, 2020 at 09:45:54AM -0700, Douglas Anderson wrote:
> > > The simple panel code currently allows panels to define fixed delays
> > > at certain stages of initialization. These work OK, but they don't
> > > really map all that clearly to the requirements presented in many
> > > panel datasheets. Instead of defining a fixed delay, those datasheets
> > > provide a timing diagram and specify a minimum amount of time that
> > > needs to pass from event A to event B.
> > >
> > > Because of the way things are currently defined, most panels end up
> > > over-delaying. One prime example here is that a number of panels I've
> > > looked at define the amount of time that must pass between turning a
> > > panel off and turning it back on again. Since there is no way to
> > > specify this, many developers have listed this as the "unprepare"
> > > delay. However, if nobody ever tried to turn the panel on again in
> > > the next 500 ms (or whatever the delay was) then this delay was
> > > pointless. It's better to do the delay only in the case that someone
> > > tried to turn the panel on too quickly.
> > >
> > > Let's support specifying delays as constraints. We'll start with the
> > > one above and also a second one: the minimum time between prepare
> > > being done and doing the enable. On the panel I'm looking at, there's
> > > an 80 ms minimum time between HPD being asserted by the panel and
> > > setting the backlight enable GPIO. By specifying as a constraint we
> > > can enforce this without over-delaying. Specifically the link
> > > training is allowed to happen in parallel with this delay so adding a
> > > fixed 80 ms delay isn't ideal.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > > ---
> > >
> > > drivers/gpu/drm/panel/panel-simple.c | 51 ++++++++++++++++++++++++----
> > > 1 file changed, 44 insertions(+), 7 deletions(-)
> >
> > This has always been bugging me a bit about the current setup, so I very
> > much like this idea.
> >
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> > > index 2be358fb46f7..cbbe71a2a940 100644
> > > --- a/drivers/gpu/drm/panel/panel-simple.c
> > > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > @@ -92,6 +92,19 @@ struct panel_desc {
> > > unsigned int unprepare;
> > > } delay;
> > >
> > > + /**
> > > + * @prepare_to_enable_ms: If this many milliseconds hasn't passed after
> > > + * prepare finished, add a delay to the start
> > > + * of enable.
> > > + * @unprepare_to_prepare_ms: If this many milliseconds hasn't passed
> > > + * unprepare finished, add a delay to the
> > > + * start of prepare.
> >
> > I find this very difficult to understand and it's also not clear from
> > this what exactly the delay is. Perhaps this can be somewhat clarified
> > Something like the below perhaps?
> >
> > @prepare_to_enable_ms: The minimum time, in milliseconds, that
> > needs to have passed between when prepare finished and enable
> > may begin. If at enable time less time has passed since
> > prepare finished, the driver waits for the remaining time.
>
> Also maybe split the kerneldoc into the sub-structure (this should work I
> think), so that you can go really wild on formatting :-)
I have a patch somewhere where I inlined all the comments and polished
them a bit. Will try to dig it up in the weekend.
It was motivated by a small W=1 detour.
Sam
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
