Re: [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller driver

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3189 bytes --]

On Fri, May 14, 2021 at 09:18:00PM +0800, Kevin Tang wrote:
> Maxime Ripard <maxime@cerno.tech> 于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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]