[PATCH RFC v3 2/6] drm/sprd: add Unisoc's drm kms master

[tang pengchuan]

On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <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.

But I've always been confused, because irq is initialized in drm driver for
other guys, why not for me?
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200223/1d505124/attachment-0001.htm>