| Message ID | 20211117064158.27451-4-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mialbox: move cmdq suspend, resume and remove after cmdq_mbox_flush | expand |
On Wed, Nov 17, 2021 at 02:41:58PM +0800, jason-jh.lin wrote: > @@ -158,6 +159,7 @@ static void mtk_drm_crtc_destroy(struct drm_crtc *crtc) > mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle); > > if (mtk_crtc->cmdq_client.chan) { > + device_link_remove(mtk_crtc->drm_dev, mtk_crtc->cmdq_client.chan->mbox->dev); > mbox_free_channel(mtk_crtc->cmdq_client.chan); > mtk_crtc->cmdq_client.chan = NULL; > } [...] > @@ -956,6 +959,16 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, > } > > if (mtk_crtc->cmdq_client.chan) { > + struct device_link *link; > + > + /* add devlink to cmdq dev to make sure suspend/resume order is correct */ > + link = device_link_add(dev, mtk_crtc->cmdq_client.chan->mbox->dev, > + DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS); > + if (!link) { > + dev_err(dev, "Unable to link dev=%s\n", > + dev_name(mtk_crtc->cmdq_client.chan->mbox->dev)); > + } > + If device_link_add() failed, doesn't mtk_drm_crtc_create() need to return an error and exit? OTOH, if device_link_add() failed, won't it bring any side effects to call device_link_remove()?
Hi Tzung-Bi, Thanks for the reviews. > From: Linux-mediatek <linux-mediatek-bounces@lists.infradead.org> On > Behalf Of Tzung-Bi Shih > Sent: Thursday, November 18, 2021 1:55 PM > To: Jason-JH Lin (林睿祥) <Jason-JH.Lin@mediatek.com> > Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>; Philipp Zabel < > p.zabel@pengutronix.de>; Matthias Brugger <matthias.bgg@gmail.com>; > Jassi Brar <jassisinghbrar@gmail.com>; David Airlie <airlied@linux.ie > linux-mediatek@lists.infradead.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > hsinyi@chromium.org; fshao@chromium.org; Nancy Lin (林欣螢) < > Nancy.Lin@mediatek.com>; Singo Chang (張興國)Singo.Chang@mediatek.com> > Subject: Re: [PATCH 3/3] drm/mediatek: add devlink to cmdq dev > > On Wed, Nov 17, 2021 at 02:41:58PM +0800, jason-jh.lin wrote: > > @@ -158,6 +159,7 @@ static void mtk_drm_crtc_destroy(struct > > drm_crtc *crtc) > > mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle); > > > > if (mtk_crtc->cmdq_client.chan) { > > + device_link_remove(mtk_crtc->drm_dev, mtk_crtc-> > > cmdq_client.chan->mbox->dev); > > > > mbox_free_channel(mtk_crtc->cmdq_client.chan); > > mtk_crtc->cmdq_client.chan = NULL; > > } > > [...] > > @@ -956,6 +959,16 @@ int mtk_drm_crtc_create(struct drm_device > > *drm_dev, > > } > > > > if (mtk_crtc->cmdq_client.chan) { > > + struct device_link *link; > > + > > + /* add devlink to cmdq dev to make sure suspend/resume > > order is correct */ > > + link = device_link_add(dev, mtk_crtc->cmdq_client.chan- > > > mbox->dev, > > > > + DL_FLAG_PM_RUNTIME | > > DL_FLAG_STATELESS); > > + if (!link) { > > + dev_err(dev, "Unable to link dev=%s\n", > > + dev_name(mtk_crtc->cmdq_client.chan- > > > mbox->dev)); > > > > + } > > + > > If device_link_add() failed, doesn't mtk_drm_crtc_create() need to > return an error and exit? OK, I'll add the return error at the next verion. > > OTOH, if device_link_add() failed, won't it bring any side effects to > call device_link_remove()? > Because device_link_remove() will find the device_link of supplier and consumer, then delete the device_link between them. If device_link_add() failed, supplier and consumer won't create the device_link. So calling device_link_remove() won't do anything without deive_link and won't bring any side effects. Regards, Jason-JH.Lin > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > > > > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!125dmOZ7xCLkwbIrzZf1VqVLQBbdT4RFHoMjR38U1GeCxaTM6xvvB-mSr7sTc6BuY5IH$ > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!125dmOZ7xCLkwbIrzZf1VqVLQBbdT4RFHoMjR38U1GeCxaTM6xvvB-mSr7sTc6BuY5IH$ > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!125dmOZ7xCLkwbIrzZf1VqVLQBbdT4RFHoMjR38U1GeCxaTM6xvvB-mSr7sTc6BuY5IH$ >
Hi Tzung-Bi, Thanks for the reviews. > From: Linux-mediatek <linux-mediatek-bounces@lists.infradead.org> On > Behalf Of Tzung-Bi Shih > Sent: Thursday, November 18, 2021 1:55 PM > To: Jason-JH Lin (林睿祥) <Jason-JH.Lin@mediatek.com> > Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>; Philipp Zabel < > p.zabel@pengutronix.de>; Matthias Brugger <matthias.bgg@gmail.com>; > Jassi Brar <jassisinghbrar@gmail.com>; David Airlie <airlied@linux.ie > >; Daniel Vetter <daniel@ffwll.ch>; dri-devel@lists.freedesktop.org; > linux-mediatek@lists.infradead.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > hsinyi@chromium.org; fshao@chromium.org; Nancy Lin (林欣螢) < > Nancy.Lin@mediatek.com>; Singo Chang (張興國) <Singo.Chang@mediatek.com> > Subject: Re: [PATCH 3/3] drm/mediatek: add devlink to cmdq dev > > On Wed, Nov 17, 2021 at 02:41:58PM +0800, jason-jh.lin wrote: > > @@ -158,6 +159,7 @@ static void mtk_drm_crtc_destroy(struct > > drm_crtc *crtc) > > mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle); > > > > if (mtk_crtc->cmdq_client.chan) { > > + device_link_remove(mtk_crtc->drm_dev, mtk_crtc- > > >cmdq_client.chan->mbox->dev); > > mbox_free_channel(mtk_crtc->cmdq_client.chan); > > mtk_crtc->cmdq_client.chan = NULL; > > } > > [...] > > @@ -956,6 +959,16 @@ int mtk_drm_crtc_create(struct drm_device > > *drm_dev, > > } > > > > if (mtk_crtc->cmdq_client.chan) { > > + struct device_link *link; > > + > > + /* add devlink to cmdq dev to make sure suspend/resume > > order is correct */ > > + link = device_link_add(dev, mtk_crtc->cmdq_client.chan- > > >mbox->dev, > > + DL_FLAG_PM_RUNTIME | > > DL_FLAG_STATELESS); > > + if (!link) { > > + dev_err(dev, "Unable to link dev=%s\n", > > + dev_name(mtk_crtc->cmdq_client.chan- > > >mbox->dev)); > > + } > > + > > If device_link_add() failed, doesn't mtk_drm_crtc_create() need to > return an error and exit? OK, I'll add the return error at the next verion. > > OTOH, if device_link_add() failed, won't it bring any side effects to > call device_link_remove()? > Because device_link_remove() will find the device_link of supplier and consumer, then delete the device_link between them. If device_link_add() failed, supplier and consumer won't create the device_link. So calling device_link_remove() won't do anything without deive_link and won't bring any side effects. Regards, Jason-JH.Lin > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!125dmOZ7xCLkwbIrzZf1VqVLQBbdT4RFHoMjR38U1GeCxaTM6xvvB-mSr7sTc6BuY5IH$ >
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index 24d9bde4d6e2..0a472719709d 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -59,6 +59,7 @@ struct mtk_drm_crtc { #endif struct device *mmsys_dev; + struct device *drm_dev; struct mtk_mutex *mutex; unsigned int ddp_comp_nr; struct mtk_ddp_comp **ddp_comp; @@ -158,6 +159,7 @@ static void mtk_drm_crtc_destroy(struct drm_crtc *crtc) mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle); if (mtk_crtc->cmdq_client.chan) { + device_link_remove(mtk_crtc->drm_dev, mtk_crtc->cmdq_client.chan->mbox->dev); mbox_free_channel(mtk_crtc->cmdq_client.chan); mtk_crtc->cmdq_client.chan = NULL; } @@ -888,6 +890,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, return -ENOMEM; mtk_crtc->mmsys_dev = priv->mmsys_dev; + mtk_crtc->drm_dev = dev; mtk_crtc->ddp_comp_nr = path_len; mtk_crtc->ddp_comp = devm_kmalloc_array(dev, mtk_crtc->ddp_comp_nr, sizeof(*mtk_crtc->ddp_comp), @@ -956,6 +959,16 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, } if (mtk_crtc->cmdq_client.chan) { + struct device_link *link; + + /* add devlink to cmdq dev to make sure suspend/resume order is correct */ + link = device_link_add(dev, mtk_crtc->cmdq_client.chan->mbox->dev, + DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS); + if (!link) { + dev_err(dev, "Unable to link dev=%s\n", + dev_name(mtk_crtc->cmdq_client.chan->mbox->dev)); + } + ret = of_property_read_u32_index(priv->mutex_node, "mediatek,gce-events", drm_crtc_index(&mtk_crtc->base),
Add devlink to cmdq to make sure the order of suspend and resume is correct. Signed-off-by: jason-jh.lin <jason-jh.lin@mediatek.com> --- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)