[FFmpeg-devel] [PATCH v08 2/3] fbtile tile/detile, hwcontext_drm detile NonLinear

[C Hanish Menon]

Hi Lynne,

On Sat, 11 Jul, 2020, 20:31 C Hanish Menon, <hanishkvc at gmail.com> wrote:

> Hi Lynne,
>
> On Sat, Jul 11, 2020 at 5:20 PM Lynne <dev at lynne.ee> wrote:
>
>> Jul 11, 2020, 08:52 by hanishkvc at gmail.com:
>>
>> > ** fbtile cpu based framebuffer tile/detile helpers
>> >
>> > Add helper routines which can be used to tile/detile framebuffer
>> > layouts between linear and specified tile layout, using the cpu.
>> >
>> > Currently it supports Legacy Intel Tile-X, Legacy Intel Tile-Y and
>> > Newer Intel Tile-Yf tiled layouts.
>> >
>> > Currently supported pixel format is 32bit RGB.
>> >
>> > It also contains fbtile_generic logic, which can be easily configured
>> > to support different kinds of tiling layouts, at the expense of some
>> > additional processing cycles, compared to developing custom (de)tiling
>> > logic. One requires to provide the tile walking parameters for the
>> > new tile layout to be supported. Once it is done, both tiling and
>> > detiling of the new tile layout can be handled automatically.
>> >
>> > This is non-public by default.
>> >
>>
>> I thought I made myself clear. No compile time public header options.
>> That means no SCOPEIN, no HWCTXDRM_SYNCRELATED_FORMATMODIFIER,
>> no DEBUG_FBTILE_FORMATMODIFIER_MAPPING, no FBTILE_SCOPE_PUBLIC,
>> no FBTILER_OPTI_UNROLL (just always unroll), no FBTILER_GENERIC_OPTI,
>> none of those please, at all. No, this is not up for discussion.
>>
>
> Reason HWCTXDRM_SYNCRELATED_FORMATMODIFIER is provided is because
> currently as the AVFrame doesn't have any generic tiling info related
> member, so the related HW AVFrame is currently used to notify any other
> users in future within hwcontext_drm that it has been detiled. But then
> again has there is no single right or wrong way here at one level, so I put
> it under a ifdef block and what I felt as a potentially sane default for
> now was set for the same.
>
> The DEBUG defs (DEBUG_FBTILE and DEBUG_...MAPPING) are provided so that in
> future, if someone wants to debug the flow for some reason like say a new
> tile layout being added or some other reason, then they don't have to hunt
> as to where to put debug prints to help understand the flow, instead they
> could use the already available compile time option to enable the debug
> logs and then get one level of understanding. And again currently a sane
> default of keeping it disabled (undef'd) is used.
>
> The reason UNROLL is put within ifdef is because if for some reason a
> tiling layout which involves less than 4x4 subtile block is required to be
> supported in future, then the unrolling will need to be avoided in this
> function or a new function without unrolling will have to be created. Again
> to help a future developer who may face such a situation, the code is
> properly identified and put into suitable ifdef blocks and suitably
> commented in the code. Again a sane default which is fine for the current
> situation is setup.
>

Rather to be more clear, once unrolled the logic can't be used for subtiles
and walks involving non multiples of 4 (along vertical direction) in this
case.

Will modify code to use unrolled opti version where this condition is
satisfied, which is the case for currently supported tile layouts. At same
time if this condition is not met by a new layout in future, the non
parallel simple version, which is also provided, can be used. The same is
noted as a comment in the code.


> Again the idea of FBTILE_SCOPE_PUBLIC, is so that a logically independent
> code, can be embedded into a some other code in a structured way, while at
> same time providing the flexibility to move it into a independent public
> api if required in future, if another use case for the logic comes up.
>

Had a look at the ffmpeg coding convention, will use the ff_ prefixed
convention for non static but internal to library helper functions. That
will keep the code simple and clean, while allowing it to be used by others
if required in future.


> I do agree that these compile time options won't be changed in a normal
> compile, but then again it is not for a normal compile situation, but for
> future flow changes if required in future for a developer (or a user who
> wants to experiment) who may have to touch the code.
>
> I am not sure your way of thinking of trying to structure everything to an
> immediate specific need, is the right way of approaching things in general,
> when the underlying logic is in reality independent and even when things
> are potentially (I do accept different people may implement the things in
> slightly different ways) structured in a sane way to provide future
> flexibility.
>
> At the same time I appreciate and am thankful to the volunteering you and
> other developers are doing here and will change the code to be more
> specific if you still feel that is what you, as an active ffmpeg developer,
> feel.
>
> I can't review this patch well at all with this.
>> You can depend on CONFIG_LIBDRM always being available, as we're never
>> ever
>> going to be using this code outside of hwcontext_drm.c.
>>
>
> As what I have mentioned above already, you seem to want to structure the
> code to be very specific to the current immediate use case, while I
> normally prefer structuring things to be flexible and adaptable for the
> unknown future, especially when the logic in question is in reality
> independent at one level.
>
> Also in this case I feel it is a sensible ifdef'd block for drm specific
> code (to map from format_modifier to the logic's id for the tiling layout)
> in an otherwise generic logic and we should retain it.
>
>
>> We align our switch cases with the switch indentation, so no extra
>> indent. Its not a block.
>>
>
> Understood the ffmpeg convention, will update.
>
>
>> Use av_log instead of av_log_once.
>>
>
> In a video flow, has the same unwanted setup can trigger more than once in
> a single run, like say an unknown tile layout, in such a situation I didnt
> want to flood the 'by-default' user visible log with message about the same
> more than once, so I used av_log_once and inturn changed the log level
> between the 1st and subsequent log entries. Isn't it what one is expected
> to do in such a situation, or am I missing something.
>
>
>> Also fix the changelog.
>>
>
> Sorry I didn't understand this fully. Can I assume that you don't want to
> mention about the fbtile helpers as an entry in the changelog, because its
> not a public api? From my perspective the reason why I added it to the
> changelog is so that if someone is looking at availability of some
> framebuffer tiling/detiling support in future, the changelog notes the same
> in a simple way. However if you feel it's not needed I will remove it. Or
> did you mean something else?
>
>
>> And please stop submitting the fbdetile filter patch, even for
>> informative reasons.
>> Just dump it in a git repository somewhere.
>>
>>
> As was being discussed in another thread in the mailing list, currently
> there is no central registry of available video filters out there, so dont
> you think mailing list is the next best location to have the source patch
> for the filter, for someone who may (or may not) look for things in future?
> Just asking and thinking out loud.
>
>
>>
>> > +// Common return values> +#define FBT_OK 0> +#define FBT_ERR 1
>>
>> Really? Use 0 for ok and AVERROR... for errors.
>>
>> Will change ;-)
>
>
>>
>> > +/*> + * Copy one AVFrame into the other, tiling or detiling as
>> required, if possible.> + * NOTE: Either the Source or the Destination
>> AVFrame (i.e one of them) should be linear.> + * NOTE: If the tiling layout
>> is not understood, it will do a simple copy.> + */
>>
>> If the contents aren't going to change you can avoid copying by just
>> using av_frame_ref.
>>
>>
> Thanks for suggesting the same (as previously mentioned, I haven't worked
> on ffmpeg before so am not aware of all the ffmpeg helper routines, and did
> this code more to solve a specific use case I had and parallely wanted to
> have some fun implementing the logic, as well as contribute something
> possibly useful back to ffmpeg as a thank you to all involved).
>
> However at the same time, when used for example like in hwcontext_drm, the
> underlying source is a specific hardware's mmap'd memory, so should one
> pass the source reference along to the destination (i am assuming that is
> what av_frame_ref is, have to check still), or shouldn't one move the data
> out of the hardware specific memory, as early as possible.
>
>
>>
>> Like I said, this is just a fraction of what I can review with all those
>> ifdefs around the code.
>>
>
> Thanks once again for providing the feedback, and sorry for all the
> ifdef's I have put in, I have specified the reason for some of those ifdefs
> above, based on your response, I will remove most of them, if that is what
> you feel is right.
>
>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
>
>
> --
> Keep ;-)
> HanishKVC
>