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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 18 May 2021 16:23:00 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Kevin Tang <kevin3.tang@...il.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Orson Zhai <orsonzhai@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller
 driver

On Fri, May 14, 2021 at 09:18:00PM +0800, Kevin Tang wrote:
> Maxime Ripard <maxime@...no.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

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