[PATCH v7 2/3] drm: Add support for the LogiCVC display controller

[Maxime Ripard]

On Wed, Dec 02, 2020 at 05:06:40PM +0100, Paul Kocialkowski wrote:
> > > +static void logicvc_crtc_atomic_begin(struct drm_crtc *drm_crtc,
> > > +				      struct drm_atomic_state *state)
> > > +{
> > > +	struct logicvc_crtc *crtc = logicvc_crtc(drm_crtc);
> > > +	struct drm_crtc_state *crtc_state =
> > > +		drm_atomic_get_old_crtc_state(state, drm_crtc);
> > > +	struct drm_device *drm_dev = drm_crtc->dev;
> > > +	unsigned long flags;
> > > +
> > > +	/* Register pending event, only if vblank is already on. */
> > > +	if (drm_crtc->state->event && crtc_state->active) {
> > > +		spin_lock_irqsave(&drm_dev->event_lock, flags);
> > > +		WARN_ON(drm_crtc_vblank_get(drm_crtc) != 0);
> > > +
> > > +		crtc->event = drm_crtc->state->event;
> > > +		drm_crtc->state->event = NULL;
> > > +
> > > +		spin_unlock_irqrestore(&drm_dev->event_lock, flags);
> > > +	}
> > > +}
> > 
> > That's unusual to do it in atomic_begin, why do you need it?
> 
> This is to cover the case where we need to send a page flip event but the
> crtc is already on. In that case, neither atomic_enable nor atomic_disable
> will be called so we need to rely on atomic_begin to grab that event.
> This happens for example when a single plane is updated.
> 
> The same thing is done in e.g. sun4i-drm.

Yeah, but I'm not sure why that's needed in the first place on sun4i-drm
either. This looks to me as either something that should be handled by
the helpers, or isn't needed at all. Just like the other times you
fiddle with the vblank in your driver.

I looked around and the only drivers that have that logic seem to be ARM
HDLCD, Atmel HCLDC, Meson, Tegra. This looks like it might be some cargo
cult.

Daniel, do you know why that would be needed?

> > > +static void logicvc_version_print(struct logicvc_drm *logicvc)
> > > +{
> > > +	u32 version;
> > > +
> > > +	regmap_read(logicvc->regmap, LOGICVC_IP_VERSION_REG, &version);
> > > +
> > > +	DRM_INFO("LogiCVC version %d.%02d.%c\n",
> > > +		 (int)LOGICVC_FIELD_GET(LOGICVC_IP_VERSION_MAJOR, version),
> > > +		 (int)LOGICVC_FIELD_GET(LOGICVC_IP_VERSION_MINOR, version),
> > > +		 (char)LOGICVC_FIELD_GET(LOGICVC_IP_VERSION_LEVEL, version) +
> > > +		 'a');
> > 
> > DRM_DEV_INFO?
> 
> Okay but now according to Sam, "DRM_DEV_ERROR() and friends are deprecated"
> so I wonder which is the right one to use at this point.

AFAIU, it's drm_info / drm_err

> > > +static void logicvc_encoder_enable(struct drm_encoder *drm_encoder)
> > > +{
> > > +	struct logicvc_drm *logicvc = logicvc_drm(drm_encoder->dev);
> > > +	struct logicvc_interface *interface =
> > > +		logicvc_interface_from_drm_encoder(drm_encoder);
> > > +
> > > +	regmap_update_bits(logicvc->regmap, LOGICVC_POWER_CTRL_REG,
> > > +			   LOGICVC_POWER_CTRL_VIDEO_ENABLE,
> > > +			   LOGICVC_POWER_CTRL_VIDEO_ENABLE);
> > > +
> > > +	if (interface->drm_panel) {
> > > +		drm_panel_prepare(interface->drm_panel);
> > > +
> > > +		/* Encoder enable is too early to enable the panel and a white
> > > +		 * screen will be seen if the panel gets enabled before the
> > > +		 * first page flip is done (and no other framebuffer
> > > +		 * configuration remains from the boot software). */
> > > +		interface->drm_panel_enabled = false;
> > > +	}
> > > +}
> > 
> > That's fishy (and the similar stuff in commit_tail). Is it because you
> > need to have the CRTC powered before the encoder?
> > 
> > If so, you should try the commit_tail_rpm variant, it makes sure the
> > CRTC is powered on before making a commit.
> 
> No, this is unrelated to CRTC vs encoder enable order. Instead, it's about
> panel enable order: I don't want to enable the panel before a buffer was
> flipped on the CRTC otherwise a blank/white/garbage screen will be shown.

Well, since the encoder will enable the panel, it's kind of related
though?

> This is why this drm_panel_enabled variable is used to make sure we don't
> enable the panel before.
> 
> This is nothing specific to my hardware, but a general concern that probably
> exists in every DRM driver. Nobody really seems to care about it but I've
> decided that I would in this driver. Now if you think this is too exotic,
> I don't mind removing it.

If this is a concern of yours and affects multiple drivers, then it
should be fixed in the core, not in one particular driver.

> > > +static void logicvc_connector_destroy(struct drm_connector *drm_connector)
> > > +{
> > > +	drm_connector_cleanup(drm_connector);
> > > +}
> > 
> > I guess you don't need that intermediate function?
> 
> I would need to check if that call is necessary or if some implied mechanism
> calls it for me already.

What I meant is that you don't need logicvc_connector_destroy, you can
directly set the atomic_destroy_state to drm_connector_cleanup.

Maximey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201207/ba8c6829/attachment-0001.sig>