Re: [PATCH v7 02/14] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.

From: Daniel Kurtz <djkurtz@chromium.org>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>,
	"open list:OPEN FIRMWARE AND..." <devicetree@vger.kernel.org>,
	Paul Bolle <pebolle@tiscali.nl>,
	Jitao Shi <jitao.shi@mediatek.com>,
	Pawel Moll <pawel.moll@arm.com>, Jie Qiu <jie.qiu@mediatek.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Cawa Cheng <cawa.cheng@mediatek.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Rob Herring <robh+dt@kernel.org>,
	linux-mediatek@lists.infradead.org,
	Kumar Gala <galak@codeaurora.org>, YT Shen <yt.shen@mediatek.com>,
	Sasha Hauer <kernel@pengutronix.de>,
	Matthias Brugger <matthias.bgg@gmail.com>
Subject: Re: [PATCH v7 02/14] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.
Date: Thu, 17 Dec 2015 00:10:04 +0800	[thread overview]
Message-ID: <CAGS+omCFYc2B+8bWy8kT-j2LzieYnPErP+xp7qyB81aGObHUGw@mail.gmail.com> (raw)
In-Reply-To: <1450259579.3421.17.camel@pengutronix.de>

Hi Philipp,

On Wed, Dec 16, 2015 at 5:52 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Daniel,
>
> Am Dienstag, den 15.12.2015, 02:57 +0800 schrieb Daniel Kurtz:
>> HI Philipp,
>>
>> This driver is looking really good.
>>
>> But, still some things to think about (mostly small) inline below...
>
> Most of my answers below seem to be "ok" or some form thereof, but I
> have one or two questions regarding the layer_config and crtc_reset
> suggestions.

Answers to your questions below...

>
> [...]
>> > +static void mtk_ovl_layer_config(void __iomem *ovl_base, unsigned int idx,
>> > +               struct mtk_plane_state *state)
>> > +{
>> > +       struct mtk_plane_pending_state *pending = &state->pending;
>> > +       unsigned int addr = pending->addr;
>> > +       unsigned int pitch = pending->pitch & 0xffff;
>> > +       unsigned int fmt = pending->format;
>> > +       unsigned int offset = (pending->y << 16) | pending->x;
>> > +       unsigned int src_size = (pending->height << 16) | pending->width;
>> > +       unsigned int con;
>> > +
>> > +       con = has_rb_swapped(fmt) << 24 | ovl_fmt_convert(fmt) << 12;
>>
>> Call these conversion routines earlier (during atomic_check) and just add the
>> resulting "con" value to pending.
>
> You mean to add a .layer_atomic_check callback to the mtk_ddp_comp ops?

I didn't have any particular implementation in mind.
But, yeah... maybe a new "check" callback to pre-compute and formally
check the provided state.
This might be overkill though if it ends up adding a lot of overhead
for these values which can never really fail anyway.

> [...]

> How about this:
>
> static void mtk_drm_crtc_reset(struct drm_crtc *crtc)
> {
>         struct mtk_crtc_state *state;
>
>         if (crtc->state) {
>                 if (crtc->state->mode_blob)
>                         drm_property_unreference_blob(crtc->state->mode_blob);
>
>                 state = to_mtk_crtc_state(crtc->state);
>                 memset(state, 0, sizeof(*state));
>         } else {
>                 state = kzalloc(sizeof(*state), GFP_KERNEL);
>                 if (!state)
>                         return;
>                 crtc->state = &state->base;
>         }
>
>         state->base.crtc = crtc;
> }

lgtm

> [...]

> Thanks for the review!

Thanks for the patches!!

>
> regards
> Philipp
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel