| Message ID | 7b6ffa43307522833103fe29ec6a084b7d621a16.1687423204.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: renesas: shmobile: Atomic conversion + DT support | expand |
Hi Geert, Thank you for the patch. On Thu, Jun 22, 2023 at 11:21:40AM +0200, Geert Uytterhoeven wrote: > Replace the call to the legacy drm_handle_vblank() function with a call > to the new drm_crtc_handle_vblank() helper. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index c98e2bdd888c3274..6eaf2c5a104f451a 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -86,7 +86,7 @@ static irqreturn_t shmob_drm_irq(int irq, void *arg) > spin_unlock_irqrestore(&sdev->irq_lock, flags); > > if (status & LDINTR_VES) { > - drm_handle_vblank(dev, 0); > + drm_crtc_handle_vblank(&sdev->crtc.base); > shmob_drm_crtc_finish_page_flip(&sdev->crtc); > } >
Hi, I'm fine with this patch but I I don't see the benefit. This reply is more about my personal question. On 2023/6/22 17:21, Geert Uytterhoeven wrote: > Replace the call to the legacy drm_handle_vblank() function with a call > to the new drm_crtc_handle_vblank() helper. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index c98e2bdd888c3274..6eaf2c5a104f451a 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -86,7 +86,7 @@ static irqreturn_t shmob_drm_irq(int irq, void *arg) > spin_unlock_irqrestore(&sdev->irq_lock, flags); > > if (status & LDINTR_VES) { > - drm_handle_vblank(dev, 0); > + drm_crtc_handle_vblank(&sdev->crtc.base); After switching to drm_crtc_handle_vblank(), your driver need another deference to the pointer of 'struct drm_crtc' to get the pointer of 'struct drm_device'; Plus another call to get the index(display pipe) of the CRTC by calling drm_crtc_index(crtc). Consider that shmob-drm support only one display pipe, is it that the switching is less straight forward than the original implement ? ``` /** * drm_crtc_handle_vblank - handle a vblank event * @crtc: where this event occurred * * Drivers should call this routine in their vblank interrupt handlers to * update the vblank counter and send any signals that may be pending. * * This is the native KMS version of drm_handle_vblank(). * * Note that for a given vblank counter value drm_crtc_handle_vblank() * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time() * provide a barrier: Any writes done before calling * drm_crtc_handle_vblank() will be visible to callers of the later * functions, if the vblank count is the same or a later one. * * See also &drm_vblank_crtc.count. * * Returns: * True if the event was successfully handled, false on failure. */ bool drm_crtc_handle_vblank(struct drm_crtc *crtc) { return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc)); } ``` Is it that drm_crtc_handle_vblank() function is preferred over drm_handle_vblank() in the future? I'm fine with this question answered. > shmob_drm_crtc_finish_page_flip(&sdev->crtc); > } >
Hi Sui, On Sat, Jun 24, 2023 at 11:33 AM Sui Jingfeng <suijingfeng@loongson.cn> wrote: > I'm fine with this patch but I I don't see the benefit. > > This reply is more about my personal question. > > On 2023/6/22 17:21, Geert Uytterhoeven wrote: > > Replace the call to the legacy drm_handle_vblank() function with a call > > to the new drm_crtc_handle_vblank() helper. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn> > > --- > > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > index c98e2bdd888c3274..6eaf2c5a104f451a 100644 > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > @@ -86,7 +86,7 @@ static irqreturn_t shmob_drm_irq(int irq, void *arg) > > spin_unlock_irqrestore(&sdev->irq_lock, flags); > > > > if (status & LDINTR_VES) { > > - drm_handle_vblank(dev, 0); > > + drm_crtc_handle_vblank(&sdev->crtc.base); > > > After switching to drm_crtc_handle_vblank(), > > your driver need another deference to the pointer of 'struct drm_crtc' > to get the pointer of 'struct drm_device'; > > Plus another call to get the index(display pipe) of the CRTC by calling > drm_crtc_index(crtc). That is correct. > Consider that shmob-drm support only one display pipe, > > is it that the switching is less straight forward than the original > implement ? > > > ``` > > /** > * drm_crtc_handle_vblank - handle a vblank event > * @crtc: where this event occurred > * > * Drivers should call this routine in their vblank interrupt handlers to > * update the vblank counter and send any signals that may be pending. > * > * This is the native KMS version of drm_handle_vblank(). > * > * Note that for a given vblank counter value drm_crtc_handle_vblank() > * and drm_crtc_vblank_count() or drm_crtc_vblank_count_and_time() > * provide a barrier: Any writes done before calling > * drm_crtc_handle_vblank() will be visible to callers of the later > * functions, if the vblank count is the same or a later one. > * > * See also &drm_vblank_crtc.count. > * > * Returns: > * True if the event was successfully handled, false on failure. > */ > bool drm_crtc_handle_vblank(struct drm_crtc *crtc) > { > return drm_handle_vblank(crtc->dev, drm_crtc_index(crtc)); > } > > ``` > > Is it that drm_crtc_handle_vblank() function is preferred over > drm_handle_vblank() in the future? > > I'm fine with this question answered. I think the native KMS version is preferred over the legacy version, cfr. /** * drm_handle_vblank - handle a vblank event [...] * This is the legacy version of drm_crtc_handle_vblank(). */ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) Gr{oetje,eeting}s, Geert
diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c index c98e2bdd888c3274..6eaf2c5a104f451a 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c @@ -86,7 +86,7 @@ static irqreturn_t shmob_drm_irq(int irq, void *arg) spin_unlock_irqrestore(&sdev->irq_lock, flags); if (status & LDINTR_VES) { - drm_handle_vblank(dev, 0); + drm_crtc_handle_vblank(&sdev->crtc.base); shmob_drm_crtc_finish_page_flip(&sdev->crtc); }
Replace the call to the legacy drm_handle_vblank() function with a call to the new drm_crtc_handle_vblank() helper. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)