On Fri, May 14, 2021 at 09:18:00PM +0800, Kevin Tang wrote:
> Maxime Ripard <maxime@xxxxxxxxxx> 于2021年4月30日周五 下午5:22写道：
> > > + info = drm_format_info(fb->format->format);
> >
> > Here fb->format is the result of drm_format_info(fb->format->format)
>
> info->num_planes == 3? I will fix it on next version
It's not really what I meant. You don't need the call to drm_format_info
in the first place, the result is going to be fb->format.
So either you do info = fb->format and
> > > + if (fb->format->num_planes == 3) {
You use info here
> > > + /* UV pitch is 1/2 of Y pitch */
> > > + pitch = (fb->pitches[0] / info->cpp[0]) |
> > > + (fb->pitches[0] / info->cpp[0] << 15);
Or you can use fb->format->cpp here
Either way you should be consistent.
> > > +static struct sprd_plane *sprd_planes_init(struct drm_device *drm)
> > > +{
> > > + struct sprd_plane *plane, *primary;
> > > + enum drm_plane_type plane_type;
> > > + int i;
> > > +
> > > + for (i = 0; i < 6; i++) {
> > > + plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
> > > + DRM_PLANE_TYPE_OVERLAY;
> > > +
> > > + plane = drmm_universal_plane_alloc(drm, struct sprd_plane, base,
> > > + 1, &sprd_plane_funcs,
> > > + layer_fmts, ARRAY_SIZE(layer_fmts),
> > > + NULL, plane_type, NULL);
> > > + if (IS_ERR(plane)) {
> > > + drm_err(drm, "failed to init drm plane: %d\n", i);
> > > + return plane;
> > > + }
> > > +
> > > + drm_plane_helper_add(&plane->base, &sprd_plane_helper_funcs);
> > > +
> > > + sprd_plane_create_properties(plane, i);
> > > +
> > > + if (i == 0)
> > > + primary = plane;
> > > + }
> > > +
> > > + return primary;
> > > +}
> > > +
> > > +static void sprd_crtc_mode_set_nofb(struct drm_crtc *crtc)
> > > +{
> > > + struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > + struct drm_display_mode *mode = &crtc->state->adjusted_mode;
> > > +
> > > + if (mode->type & DRM_MODE_TYPE_PREFERRED)
> > > + drm_display_mode_to_videomode(mode, &dpu->ctx.vm);
> >
> > What happens if the mode isn't a preferred mode?
>
> Have already replied on the dsi driver side
>
> > > +}
> > > +
> > > +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc,
> > > + struct drm_atomic_state *state)
> > > +{
> > > + struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > +
> > > + sprd_dpu_init(dpu);
> >
> > I guess that call would fail here or program a bogus value. We already
> > discussed this in the previous version, but I'm really not sure why you
> > need this in the first place. Just use the crtc_state->adjusted_mode and
> > program that.
>
> Is also the enable_irq issue about this comment?
Not really? This is about the preferred mode stuff.
Maxime
Attachment:
signature.asc
Description: PGP signature
