[PATCH RFC v3 2/6] drm/sprd: add Unisoc's drm kms master
tang pengchuan
kevin3.tang at gmail.com
Tue Feb 25 09:45:51 UTC 2020
On Tue, Feb 25, 2020 at 3:38 PM Thomas Zimmermann <tzimmermann at 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 at ravnborg.org
> > <mailto:sam at 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 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200225/6c73fcac/attachment-0001.htm>
More information about the dri-devel
mailing list