On Tue, Feb 25, 2020 at 3:38 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 23.02.20 um 05:26 schrieb tang pengchuan:
On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <sam@ravnborg.org mailto:sam@ravnborg.org> wrote:
Hi Kevin/tang. Thanks for the quick and detailed feedback. Your questions are addressed below. Sam > > > +static int sprd_drm_bind(struct device *dev) > > > +{ > > > + struct drm_device *drm; > > > + struct sprd_drm *sprd; > > > + int err; > > > + > > > + drm = drm_dev_alloc(&sprd_drm_drv, dev); > > > + if (IS_ERR(drm)) > > > + return PTR_ERR(drm); > > You should embed drm_device in struct sprd_drm. > > See example code in drm/drm_drv.c > > > > This is what modern drm drivers do. > > > > I *think* you can drop the component framework if you do this. > > > I have read it(drm/drm_drv.c) carefully, if drop the component framework, > the whole our drm driver maybe need to redesign, so i still want to keep > current design. OK, fine. > > > + sprd_drm_mode_config_init(drm); > > > + > > > + /* bind and init sub drivers */ > > > + err = component_bind_all(drm->dev, drm); > > > + if (err) { > > > + DRM_ERROR("failed to bind all component.\n"); > > > + goto err_dc_cleanup; > > > + } > > When you have a drm_device - which you do here. > > Then please use drm_err() and friends for error messages. > > Please verify all uses of DRM_XXX > > > modern drm drivers need drm_xxx to replace DRM_XXX? Yes, use of DRM_XXX is deprecated - when you have a drm_device. > > > > > + /* with irq_enabled = true, we can use the vblank feature. */ > > > + drm->irq_enabled = true; > > I cannot see any irq being installed. This looks wrong. > > > Our display controller isr is been register on crtc driver(sprd_dpu.c), not > here. I think you just need to move this to next patch and then it is fine.Here is the advice given by Thomas Zimmermann, similar to the advice you gave. I have given thomas feedback about my questions, maybe thomas didn't see my email, so there is no reply.
I have been busy last week. Sorry for not getting back to you.
But I've always been confused, because irq is initialized in drm driver for other guys, why not for me?
Do you have an example?
Hi Thomas, The the irq is initialized in the sub-device code, but the device state is set on kms driver. E.g Here is the device state set on kms driver:
206 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#206 *static* *int* mtk_drm_kms_init http://10.0.1.79:8081/s?refs=mtk_drm_kms_init&project=sprdroidr_trunk(*struct* drm_device http://10.0.1.79:8081/s?defs=drm_device&project=sprdroidr_trunk *drm http://10.0.1.79:8081/s?refs=drm&project=sprdroidr_trunk) ......
298 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#298 /*299 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#299 * We don't use the drm_irq_install() helpers provided by the DRM300 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#300 * core, so we need to set this manually in order to allow the301 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#301 * DRM_IOCTL_WAIT_VBLANK to operate correctly.302 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#302 */303 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#303 drm http://10.0.1.79:8081/s?defs=drm&project=sprdroidr_trunk->irq_enabled http://10.0.1.79:8081/s?defs=irq_enabled&project=sprdroidr_trunk = *true*;
Here is irq install on subdev: 265 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#265 *static* *int* mtk_disp_rdma_probe http://10.0.1.79:8081/s?refs=mtk_disp_rdma_probe&project=sprdroidr_trunk(*struct* platform_device http://10.0.1.79:8081/s?defs=platform_device&project=sprdroidr_trunk *pdev http://10.0.1.79:8081/s?refs=pdev&project=sprdroidr_trunk) ...... 298 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#298 ret http://10.0.1.79:8081/s?defs=ret&project=sprdroidr_trunk = devm_request_irq http://10.0.1.79:8081/s?defs=devm_request_irq&project=sprdroidr_trunk(dev http://10.0.1.79:8081/s?defs=dev&project=sprdroidr_trunk, irq http://10.0.1.79:8081/s?defs=irq&project=sprdroidr_trunk, mtk_disp_rdma_irq_handler http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#mtk_disp_rdma_irq_handler,299 http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#299 IRQF_TRIGGER_NONE http://10.0.1.79:8081/s?defs=IRQF_TRIGGER_NONE&project=sprdroidr_trunk, dev_name http://10.0.1.79:8081/s?defs=dev_name&project=sprdroidr_trunk(dev http://10.0.1.79:8081/s?defs=dev&project=sprdroidr_trunk), priv http://10.0.1.79:8081/s?defs=priv&project=sprdroidr_trunk);
I'm not sure if my understanding is wrong...
Best regards Thomas
Can you help to tell the reason in detail, looking forward to your
answers.
Thomas's suggestion：
This line indicates the problem's design. The irq is initialized in the sub-device code, but the device state is set here. Instead both should be set in the same place.
/* reset all the states of crtc/plane/encoder/connector */drm_mode_config_reset(drm);/* init kms poll for handling hpd */drm_kms_helper_poll_init(drm);Most of this function's code should be moved into the sub-device bind function.
Here, maybe do:
- allocate device structures
- call component_bind_all()
- on success, register device
The sub-device function should then do
- init modesetting, crtc, planes, etc.
- do drm_mode_config_reset()
- init vblanking
- init the irq
- do drm_kms_helper_poll_init()
roughtly in that order. It makes sense to call drm_vblank_init() after drm_mode_config_reset(), as vblanking uses some of the mode-config
fields.
Sam
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer