From: Maxime Ripard <maxime@cerno.tech> To: Kevin Tang <kevin3.tang@gmail.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Orson Zhai <orsonzhai@gmail.com>, Chunyan Zhang <zhang.lyra@gmail.com>, "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>, ML dri-devel <dri-devel@lists.freedesktop.org>, devicetree@vger.kernel.org Subject: Re: [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller driver Date: Tue, 18 May 2021 16:23:00 +0200 [thread overview] Message-ID: <20210518142300.stvrnyfxnojdidug@gilmour> (raw) In-Reply-To: <CAFPSGXYim34tVydpB3ukD8XOc9Y2xSzm3RHyWuUx-mRGPLXwiQ@mail.gmail.com> [-- 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 --]
WARNING: multiple messages have this Message-ID (diff)
From: Maxime Ripard <maxime@cerno.tech> To: Kevin Tang <kevin3.tang@gmail.com> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, David Airlie <airlied@linux.ie>, Chunyan Zhang <zhang.lyra@gmail.com>, "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>, ML dri-devel <dri-devel@lists.freedesktop.org>, Rob Herring <robh+dt@kernel.org>, Orson Zhai <orsonzhai@gmail.com>, Sean Paul <sean@poorly.run> Subject: Re: [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller driver Date: Tue, 18 May 2021 16:23:00 +0200 [thread overview] Message-ID: <20210518142300.stvrnyfxnojdidug@gilmour> (raw) In-Reply-To: <CAFPSGXYim34tVydpB3ukD8XOc9Y2xSzm3RHyWuUx-mRGPLXwiQ@mail.gmail.com> [-- 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 --]
next prev parent reply other threads:[~2021-05-18 14:23 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-25 12:36 [PATCH v5 0/6] Add Unisoc's drm kms module Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-25 12:36 ` [PATCH v5 1/6] dt-bindings: display: add Unisoc's drm master bindings Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-30 9:12 ` Maxime Ripard 2021-04-30 9:12 ` Maxime Ripard 2021-04-25 12:36 ` [PATCH v5 2/6] drm/sprd: add Unisoc's drm kms master Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-25 12:36 ` [PATCH v5 3/6] dt-bindings: display: add Unisoc's dpu bindings Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-25 12:36 ` [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller driver Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-30 9:22 ` Maxime Ripard 2021-04-30 9:22 ` Maxime Ripard 2021-04-30 12:20 ` Kevin Tang 2021-04-30 12:20 ` Kevin Tang 2021-05-17 9:27 ` Joerg Roedel 2021-05-17 9:27 ` Joerg Roedel 2021-05-17 16:35 ` Robin Murphy 2021-05-17 16:35 ` Robin Murphy 2021-05-26 7:59 ` Chunyan Zhang 2021-05-26 8:07 ` Chunyan Zhang 2021-05-26 8:07 ` Chunyan Zhang 2021-05-14 13:18 ` Kevin Tang 2021-05-14 13:18 ` Kevin Tang 2021-05-18 14:23 ` Maxime Ripard [this message] 2021-05-18 14:23 ` Maxime Ripard 2021-05-12 6:25 ` Chunyan Zhang 2021-05-12 6:25 ` Chunyan Zhang 2021-04-25 12:36 ` [PATCH v5 5/6] dt-bindings: display: add Unisoc's mipi dsi controller bindings Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-25 12:36 ` [PATCH v5 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver Kevin Tang 2021-04-25 12:36 ` Kevin Tang 2021-04-25 16:27 ` kernel test robot 2021-04-30 9:35 ` Maxime Ripard 2021-05-12 13:53 ` Kevin Tang 2021-05-18 14:20 ` Maxime Ripard 2021-05-18 14:20 ` Maxime Ripard 2021-04-26 6:17 [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller driver kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210518142300.stvrnyfxnojdidug@gilmour \ --to=maxime@cerno.tech \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=kevin3.tang@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mark.rutland@arm.com \ --cc=orsonzhai@gmail.com \ --cc=robh+dt@kernel.org \ --cc=sean@poorly.run \ --cc=zhang.lyra@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.