<div dir="ltr"><div dir="ltr"><br></div><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Feb 23, 2020 at 12:26 PM tang pengchuan <<a href="mailto:kevin3.tang@gmail.com" target="_blank">kevin3.tang@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <<a href="mailto:sam@ravnborg.org" target="_blank">sam@ravnborg.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kevin/tang.<br>
<br>
Thanks for the quick and detailed feedback.<br>
Your questions are addressed below.<br>
<br>
        Sam<br>
<br>
<br>
> > > +static int sprd_drm_bind(struct device *dev)<br>
> > > +{<br>
> > > +     struct drm_device *drm;<br>
> > > +     struct sprd_drm *sprd;<br>
> > > +     int err;<br>
> > > +<br>
> > > +     drm = drm_dev_alloc(&sprd_drm_drv, dev);<br>
> > > +     if (IS_ERR(drm))<br>
> > > +             return PTR_ERR(drm);<br>
> > You should embed drm_device in struct sprd_drm.<br>
> > See example code in drm/drm_drv.c<br>
> ><br>
> > This is what modern drm drivers do.<br>
> ><br>
> > I *think* you can drop the component framework if you do this.<br>
> ><br>
> I have read it(drm/drm_drv.c) carefully, if drop the component framework,<br>
> the whole our drm driver maybe need to redesign, so i still want to keep<br>
> current design.<br>
OK, fine.<br>
<br>
> > > +     sprd_drm_mode_config_init(drm);<br>
> > > +<br>
> > > +     /* bind and init sub drivers */<br>
> > > +     err = component_bind_all(drm->dev, drm);<br>
> > > +     if (err) {<br>
> > > +             DRM_ERROR("failed to bind all component.\n");<br>
> > > +             goto err_dc_cleanup;<br>
> > > +     }<br>
> > When you have a drm_device - which you do here.<br>
> > Then please use drm_err() and friends for error messages.<br>
> > Please verify all uses of DRM_XXX<br>
> ><br>
>   modern drm drivers need drm_xxx to replace DRM_XXX?<br>
Yes, use of DRM_XXX is deprecated - when you have a drm_device.<br>
<br>
> ><br>
> > > +     /* with irq_enabled = true, we can use the vblank feature. */<br>
> > > +     drm->irq_enabled = true;<br>
> > I cannot see any irq being installed. This looks wrong.<br>
> ><br>
> Our display controller isr is been register on crtc driver(sprd_dpu.c), not<br>
> here.<br>
<br>
I think you just need to move this to next patch and then it is fine.<br></blockquote><div>Here is the advice given by Thomas Zimmermann, similar to the advice you gave.</div><div>I have given thomas feedback about my questions, maybe thomas didn't see my email, so there is no reply.<br></div><div><br></div><div>But I've always been confused, because irq is initialized

 in drm driver for other guys, why not for me?<br></div><div>Can you help to tell the reason in detail, looking forward to your answers.<br></div></div></div></blockquote><div>But I've always been confused, because the irq is initialized in the<br></div><div>sub-device code, but the device state is set by drm_drv.</div><div>Everyone else seems to do like this. why can't for me?</div><div><br></div><div>Can you help to tell the reason in detail, looking forward to your answers. </div><div><br></div><div>BR</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><div><br></div><div>Thomas's suggestion：<br></div><div>-------------------------------------------------------------------------------------------</div><table cellpadding="0" style="border-collapse:collapse;margin-top:0px;width:auto;font-family:Roboto,RobotoDraft,Helvetica,Arial,sans-serif;font-size:0.875rem;letter-spacing:0.2px;display:block"></table>This line indicates the problem's design. The irq is initialized in the<br>sub-device code, but the device state is set here. Instead both should<br>be set in the same place.<span style="color:rgb(80,0,80)"><br><br>> +<br>> +     /* reset all the states of crtc/plane/encoder/connector */<br>> +     drm_mode_config_reset(drm);<br>> +<br>> +     /* init kms poll for handling hpd */<br>> +     drm_kms_helper_poll_init(drm);<br><br></span>Most of this function's code should be moved into the sub-device bind<br>function.<br><br>Here, maybe do:<br><br> * allocate device structures<br> * call component_bind_all()<br> * on success, register device<br><br>The sub-device function should then do<br><br> * init modesetting, crtc, planes, etc.<br> * do drm_mode_config_reset()<br> * init vblanking<br> * init the irq<br> * do drm_kms_helper_poll_init()<br><br>roughtly in that order. It makes sense to call drm_vblank_init() after<br><div>drm_mode_config_reset(), as vblanking uses some of the mode-config fields. </div><div>------------------------------------------------------------------------------------------------------</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
        Sam<br>
<br>
</blockquote></div></div>
</blockquote></div></div>
</div>