Message-ID: <X9NxdLtIsGxbDmfL@aptenodytes>
Date:   Fri, 11 Dec 2020 14:17:40 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Daniel Vetter <daniel@...ll.ch>, dri-devel@...ts.freedesktop.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        David Airlie <airlied@...ux.ie>,
        Rob Herring <robh+dt@...nel.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v7 2/3] drm: Add support for the LogiCVC display
 controller

Hi,

On Mon 07 Dec 20, 11:42, Maxime Ripard wrote:
> 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 didn't really question myself about whether this could be done in helpers,
but it looks like the philosophy now is that the driver grabs the page flip
done event when it can and serves the event in the IRQ routine.

So nothing unusual about this driver in particular.

> 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?

As far as I understand, this could work just as well with a helper in my
case (and sun4i-drm's case as well). But in any case, what this patch implements
is the current philosophy and I guess that reworking it through helpers is
way out of the scope of this series ;)

> > > > +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

Thanks!

> > > > +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?

Right, I meant that it's not related to the CRTC in particular.

> > 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.

So I suppose this should be fixed by having the core enable the encoder at first
page flip and not before then, right?

In that case the change should indeed be separate from this series and my driver
should still enable the panel at encoder enable time.

In spite of that, I agree this implementation is not very appropriate so I'll
get rid of it in the next revision.

> > > > +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.

Oh right, I hadn't really noticed that the prototypes are 100% the same :)

Thanks for the review,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists