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

[Daniel Kurtz]

Hi Philipp,

On Wed, Dec 16, 2015 at 5:52 PM, Philipp Zabel <p.zabel at 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
>