Re: [PATCHv7 2/6] drm/core: Add drm_afbc_framebuffer and a corresponding helper

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 30, 2020 at 7:44 PM Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Hi Daniel,
>
> W dniu 30.03.2020 o 19:01, Daniel Vetter pisze:
> > On Wed, Mar 11, 2020 at 3:55 PM Andrzej Pietrasiewicz
> > <andrzej.p@collabora.com> wrote:
> >>
> >> The new struct contains afbc-specific data.
>
> <snip>
>
> >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> >> index 439656f55c5d..37a3a023c114 100644
> >> --- a/Documentation/gpu/todo.rst
> >> +++ b/Documentation/gpu/todo.rst
> >> @@ -404,6 +404,21 @@ Contact: Laurent Pinchart, respective driver maintainers
> >>
> >>   Level: Intermediate
> >>
> >> +Encode cpp properly in malidp
> >> +-----------------------------
> >> +
> >> +cpp (chars per pixel) is not encoded properly in malidp, zero is
> >> +used instead. afbc implementation needs bpp or cpp, but if it is
> >> +zero it needs to be provided elsewhere, and so the bpp field exists
> >> +in struct drm_afbc_framebuffer.
> >> +
> >> +Properly encode cpp in malidp and remove the bpp field in struct
> >> +drm_afbc_framebuffer.
> >> +
> >> +Contact: malidp maintainers
> >> +
> >> +Level: Intermediate
> >
> > Just stumbled over this todo, which is really surprising. Also
> > definitely not something I wanted to ack, earlier versions at least
> > didn't have this.
> >
> > Why is this needed? drm_afbc_framebuffer contains a drm_framebuffer,
> > which has a pointer to drm_format_info, which we're always setting
> > (the core does that for all drivers, both for addfb and addfb2). Why
> > is that not good enough to get at cpp for everyone?
> >
> > Cheers, Daniel
> >
>
> Let me quote James https://patchwork.freedesktop.org/patch/345603/#comment_653081:
>
> "Seems we can remove this bpp or no need to define it as a pass in argument
> for size check, maybe the komeda/malidp get_afbc_bpp() function mislead
> you that afbc formats may have vendor specific bpp.
>
> But the story is:
>
> for afbc only formats like DRM_FORMAT_YUV420_8BIT/10BIT, we have set
> nothing in drm_format_info, neither cpp nor block_size, so both malidp
> or komeda introduce a get_bpp(), but actually the two funcs basically
> are same.
>
> So my suggestion is we can temporary use the get_afbc_bpp() in malidp
> or komeda. and eventually I think we'd better set the block size
> for these formats, then we can defines a common get_bpp() like pitch".

Hm iirc we had some good reasons not to set the block size for these,
but maybe with these afbc helpers that's changed. We could also
compute cpp/bpp in the helper, starting from the format_info/fourcc
code, for these special cases. Essentially move the exact computation
that komeda/malidp do right now to set afbc->bpp and move it into the
helper. Just kinda feels like unfinished work that we still leave this
to drivers, that's some very quirky api.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel