Re: [PATCH v10 2/2] drm: add kms driver for loongson display controller

[Sui Jingfeng]

Hi,

On 2023/4/4 22:10, Emil Velikov wrote:
> > +static void lsdc_crtc_reset(struct drm_crtc *crtc)
> > +{
> > +       struct lsdc_display_pipe *dispipe = crtc_to_display_pipe(crtc);
> > +       struct drm_device *ddev = crtc->dev;
> > +       struct lsdc_device *ldev = to_lsdc(ddev);
> > +       struct lsdc_crtc_state *priv_crtc_state;
> > +       unsigned int index = dispipe->index;
> > +       u32 val;
> > +
> > +       val = LSDC_PF_XRGB8888 | CFG_RESET_N;
> > +       if (ldev->descp->chip == CHIP_LS7A2000)
> > +               val |= LSDC_DMA_STEP_64_BYTES;
> > +
> > +       lsdc_crtc_wreg32(ldev, LSDC_CRTC0_CFG_REG, index, val);
> > +
> > +       if (ldev->descp->chip == CHIP_LS7A2000) {
> > +               val = PHY_CLOCK_EN | PHY_DATA_EN;
> > +               lsdc_crtc_wreg32(ldev, LSDC_CRTC0_PANEL_CONF_REG, index, val);
> > +       }
> > +
> AFAICT no other driver touches the HW in their reset callback. Should
> the above be moved to another callback?
You may correct in the 95% of all cases.

After reading the comments of the reset callback of struct 
drm_crtc_funcs in drm_crtc.h,

It seems that it does not prohibit us to touches the hardware.

I copy that comments and paste into here for easier to read,as below:


    /*
     * @reset:
     *
     * Reset CRTC hardware and software state to off. This function isn't
     * called by the core directly, only through drm_mode_config_reset().
     * It's not a helper hook only for historical reasons.
     *
     * Atomic drivers can use drm_atomic_helper_crtc_reset() to reset
     * atomic state using this hook.
     */


It seem allowable to reset CRTC hardware in this callback, did it cue us?

What we know is that this reset callback (and others, such as encoder's 
reset)

is called by drm_mode_config_reset(). It is the first set of functions 
get called

before other hardware related callbacks.


I don't not see how other drivers implement this callback, after you 
mention this

I skim over a few, I found tilcdc also writing the hardware in their 
tilcdc_crtc_reset()

function. See it in drm/tildc/tilclc_crtc.c


In addition, Loongson platform support efifb,  in order to light up the 
monitor in

firmware stage and the booting stage, the firmware touch the display 
hardware

register directly. After efifb get kick out, when drm/loongson driver 
taken over the

hardware, the register setting state still remain in the hardware 
register. Those

register setting may no longer correct for the subsequent operationd. 
What we

doing here is to giving the hardware a basic healthy state prepare to be 
update

further. As the reset callback is call very early, we found that it's 
the best fit.

The reset will also get called when resume(S3).


The problem is that we don't find a good to place to move those setting 
currently.