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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 Apr 2023 13:39:07 +0800
From:   Sui Jingfeng <15330273260@....cn>
To:     Emil Velikov <emil.l.velikov@...il.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian Koenig <christian.koenig@....com>,
        linaro-mm-sig@...ts.linaro.org, Li Yi <liyi@...ngson.cn>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        nathan@...nel.org, linux-media@...r.kernel.org,
        loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH v10 2/2] drm: add kms driver for loongson display
 controller

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.