Hi Doug, On Wed, Oct 27, 2021 at 3:08 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Tue, Oct 26, 2021 at 2:56 PM Philip Chen <philipchen@xxxxxxxxxxxx> wrote: > > > > Fit ps8640 driver into runtime power management framework: > > > > First, break _poweron() to 3 parts: (1) turn on power and wait for > > ps8640's internal MCU to finish init (2) check panel HPD (which is > > proxied by GPIO9) (3) the other configs. As runtime_resume() can be > > called before panel is powered, we only add (1) to _resume() and leave > > (2)(3) to _pre_enable(). We also add (2) to _aux_transfer() as we want > > to ensure panel HPD is asserted before we start AUX CH transactions. > > > > Second, the original driver has a mysterious delay of 50 ms between (2) > > and (3). Since Parade's support can't explain what the delay is for, > > and we don't see removing the delay break any boards at hand, remove > > the delay to fit into this driver change. > > > > In addition, rename "powered" to "pre_enabled" and don't check for it > > in the pm_runtime calls. The pm_runtime calls are already refcounted > > so there's no reason to check there. The other user of "powered", > > _get_edid(), only cares if pre_enable() has already been called. > > > > Lastly, change some existing DRM_...() logging to dev_...() along the > > way, since DRM_...() seem to be deprecated in [1]. > > > > [1] https://patchwork.freedesktop.org/patch/454760/ > > > > Signed-off-by: Philip Chen <philipchen@xxxxxxxxxxxx> > > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > > Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > > --- > > In v3, I also added pm_suspend_ignore_children() in the ps8640_probe() > > but forgot to mention that in the v3 change log. > > > > Changes in v4: > > - Make ps8640_ensure_hpd() return int (This change was mis-added to > > another patch [2] in this patch series: > > [2] https://patchwork.kernel.org/project/dri-devel/patch/20211026121058.v3.2.I09899dea340f11feab97d719cb4b62bef3179e4b@changeid/) > > > > Changes in v3: > > - Fix typo/wording in the commit message. > > - Add ps8640_aux_transfer_msg() for AUX operation. In > > ps8640_aux_transfer(), wrap around ps8640_aux_transfer_msg() > > with PM operations and HPD check. > > - Document why autosuspend_delay is set to 500ms. > > > > drivers/gpu/drm/bridge/parade-ps8640.c | 186 +++++++++++++++---------- > > 1 file changed, 115 insertions(+), 71 deletions(-) > > Unfortunately, your patch no longer applies to drm-misc/drm-misc-next. > Commit 7abbc26fd667 ("drm/bridge: ps8640: Register and attach our DSI > device at probe") landed and that causes a merge conflict. Can you > rebase and resend? Yes, I will rebase and resend. > > This will also cause a conflict when Sam's change lands [1] so I guess > we can see whose lands first. Let me review that now and maybe you add > a Tested-by? If it lands that'll make it easier and you can just > rebase on both of them? As per your latest reply, I'll just rebase atop drm-misc-next for now. > > > > + pm_runtime_enable(dev); > > + /* > > + * In practice, ps8640_aux_transfer_msg() takes ~300ms to complete in > > + * the worst case. Set autosuspend_delay to 500ms. > > + */ > > + pm_runtime_set_autosuspend_delay(dev, 500); > > To be a little nitpicky, this makes it sound as if the 500 ms needs to > be greater than the 300 ms for correctness. That's not _really_ the > case. During the whole ps8640_aux_transfer_msg() we're holding a PM > Runtime reference (so it won't autosuspend no matter what) and at the > end of it we mark that we were busy so the timer won't start ticking > until then. Yeah....sorry, looking again, I agree the comment I added in v3 is pretty misleading. I think autosuspend_delay just needs to be consistently longer than the interval between each aux_transfer call during EDID read. I'll measure the interval and add the number to the comment. > > Really the 500 ms is because we're quite slow to power up (~300 ms) > and so we want to avoid powering down and powering up constantly. We > definitely need to avoid a power down / power up when reading the EDID > and an EDID read involves _several_ calls in a row to the AUX transfer > function. We also expect that after userspace reads the EDID it will > call us again "soon" to turn the power on and it's nice to not have to > wait the 300 ms again. The 500 ms here is really just a number that's > not too short but not too long. I suppose it's roughly related to the > 300 ms because that's the delay we're trying to avoid, but otherwise > they are unrelated. NOTE: the code will still be _correct_ if we miss > the 500 ms window, it'll just be a lot slower. > > [1] https://lore.kernel.org/r/20211020181901.2114645-2-sam@xxxxxxxxxxxx > > -Doug
