Re: [PATCH v6 1/3] drm/lsdc: add drm driver for loongson display controller

[suijingfeng]

On 2022/2/9 16:36, Maxime Ripard wrote:
> On Fri, Feb 04, 2022 at 12:41:37AM +0800, Sui Jingfeng wrote:
> > > > +static int lsdc_primary_plane_atomic_check(struct drm_plane *plane,
> > > > +					   struct drm_atomic_state *state)
> > > > +{
> > > > +	struct drm_device *ddev = plane->dev;
> > > > +	struct lsdc_device *ldev = to_lsdc(ddev);
> > > > +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> > > > +	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> > > > +	struct drm_framebuffer *new_fb = new_plane_state->fb;
> > > > +	struct drm_framebuffer *old_fb = old_plane_state->fb;
> > > > +	struct drm_crtc *crtc = new_plane_state->crtc;
> > > > +	u32 new_format = new_fb->format->format;
> > > > +	struct drm_crtc_state *new_crtc_state;
> > > > +	struct lsdc_crtc_state *priv_crtc_state;
> > > > +	int ret;
> > > > +
> > > > +	if (!crtc)
> > > > +		return 0;
> > > > +
> > > > +	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > > +	if (WARN_ON(!new_crtc_state))
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv_crtc_state = to_lsdc_crtc_state(new_crtc_state);
> > > > +
> > > > +	ret = drm_atomic_helper_check_plane_state(new_plane_state,
> > > > +						  new_crtc_state,
> > > > +						  DRM_PLANE_HELPER_NO_SCALING,
> > > > +						  DRM_PLANE_HELPER_NO_SCALING,
> > > > +						  false,
> > > > +						  true);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/*
> > > > +	 * Require full modeset if enabling or disabling a plane,
> > > > +	 * or changing its position, size, depth or format.
> > > > +	 */
> > > > +	if ((!new_fb || !old_fb ||
> > > > +	     old_plane_state->crtc_x != new_plane_state->crtc_x ||
> > > > +	     old_plane_state->crtc_y != new_plane_state->crtc_y ||
> > > > +	     old_plane_state->crtc_w != new_plane_state->crtc_w ||
> > > > +	     old_plane_state->crtc_h != new_plane_state->crtc_h ||
> > > > +	     old_fb->format->format != new_format))
> > > > +		new_crtc_state->mode_changed = true;
> > > > +
> > > > +
> > > > +	priv_crtc_state->pix_fmt = lsdc_primary_get_default_format(crtc);
> > > Storing the pixel format in the CRTC state is weird? What would happen
> > > if you have a primary plane and a cursor in different formats?
> > >
> > > Also, reading the default format from a register doesn't look right.
> > > atomic_check can occur at any time, including before a previous commit,
> > > or while the hardware is disabled. You should rely on either a constant
> > > or the previous state here.
> > Currently, private CRTC state(priv_crtc_state) is not get used by the cursor's
> > atomic_check() and atomic_update(). I means this is only for the primary plane.
> > And both and the primary and the cursor using  XRGB8888 format, what I really
> > want here is let the atomic_update to update the framebuffer's format, because
> > the firmware (pmon) of some board set the framebuffer's format as RGB565.
> atomic_update will be called each time the plane state is changed, so it
> won't be an issue: when the first state will be committed, your
> atomic_update function will be called and thus you'll overwrite what was
> left of the firmware setup.

I am agree with you that  the format is a property of the plane, the 
code using wrong abstraction.

this is because i am confused by our DC hardware at that time. 
atomic_update is failed to update

the FB format because our CRTC hardware has bug. It can only success 
update fb format with CRTC

reset bit set in drm_crtc_funcs.reset(). Otherwise the CRTC still using 
RGB565 format to parse the

RGBX8888 frame buffer.  We finally found the hardware bug via debugfs, 
just dump the register value.

we found that the fb format didn't get update. The hardware bug leading 
us using the wrong abstraction.

we do not understand the DRM core fully at that time. We have revised 
our patch, thank you point out that.

> > If the hardware's format is same with the plane state, then there is no need to
> > update the FB's format register, save a function call, right?
> My point was more about the fact that you're using the wrong abstraction
> there. The format is a property of the plane, not from the CRTC. In KMS
> (and in most drivers), you can have multiple planes with different
> formats all attached to the same CRTC just fine.
>
> Maxime

Our CRTC have cursor plane and primary plane, composite is doing by the 
CRTC hardware automatically.

there no depth property, the cursor is always on the top of the CRTC.  
If the CRTC format is not same format

I think we should convert the format of cursor to the format of primary 
plane at atomic_update, by introduce

a shadow plane. Thanks for your guiding.