[PATCHv5,00/34] Add AFBC support for Rockchip
mbox series

Message ID 20191217145020.14645-1-andrzej.p@collabora.com
Headers show
Series
  • Add AFBC support for Rockchip
Related show

Message

Andrzej Pietrasiewicz Dec. 17, 2019, 2:49 p.m. UTC
This series adds AFBC support for Rockchip. It is inspired by:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c

This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
rework has been done, the main goal of which was to move all afbc-related
checks to helpers, so that core does not deal with it.

A new struct drm_afbc_framebuffer is added, which stores afbc-related
driver-specific data. Because of that, in drivers that wish to
use this feature, the struct must be allocated directly in the driver
code rather than inside helpers, so the first portion of the patchset
does the necessary refactoring.

Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
the latter being the ultimate purpose of this work and the 3 subsequent
portions of the patchset move komeda and malidp to generic helpers and add
afbc support to rockchip.

The idea is to make all afbc users follow a similar pattern. In fb_create()
they allocate struct drm_afbc_framebuffer, do their specific checks which
can be done before object lookups, do object lookups and a special version
of a size check, which understands struct drm_afbc_framebuffer, followed
by any other driver-specific checks and initializing the gem object.
The helpers for the common parts are factored out so that drivers
can use them.

The komeda driver has been the farthest away from such a pattern, so it
required most changes. However, due to the fact that I don't have any
komeda hardware I did the changes to komeda in an incremental fashion with
a series of (usually) very small, easy to understand steps. malidp was
pretty straightforward, and rockchip's afbc checks follow the pattern.

I kindly ask for reviewing the series. I need to mention that my ultimate
goal is merging afbc for rockchip and I don't have other hardware, so some
help from malidp and komeda developers/maintainers would be appreciated.

@Liviu, @James, @Mihail, @Brian: a kind request to you to have a look and
test the patchset, as I don't have appropriate hardware.

Rebased onto drm-misc-next.

v4..v5:
- used proper way of subclassing drm_framebuffer (Daniel Vetter)
- added documentation to exported functions (Liviu Dudau)
- reordered new functions in drm_gem_framebuffer_helper.c to make a saner
diff (Liviu Dudau)
- used "2" suffix instead of "_special" for the special version of size
checks (Liviu Dudau)
- dropped unnecessarily added condition in drm_get_format_info() (Liviu
Dudau)
- dropped "block_size = 0;" trick in framebuffer_check() (Daniel Vetter)
- relaxed sticking to 80 characters per line rule in some cases

v3..v4:

- addressed (some) comments from Daniel Stone, Ezequiel Garcia, Daniel
Vetter and James Qian Wang - thank you for input
- refactored helpers to ease accommodating drivers with afbc needs
- moved afbc checks to helpers
- converted komeda, malidp and (the newly added) rockchip to use the afbc
helpers
- eliminated a separate, dedicated source code file

v2..v3:

- addressed (some) comments from Daniel Stone, Liviu Dudau, Daniel Vetter
and Brian Starkey - thank you all

In this iteration some rework has been done. The checking logic is now moved
to framebuffer_check() so it is common to all drivers. But the common part
is not good for komeda, so this series is not good for merging yet.
I kindly ask for feedback whether the changes are in the right direction.
I also kindly ask for input on how to accommodate komeda.

The CONFIG_DRM_AFBC option has been eliminated in favour of adding
drm_afbc.c to drm_kms_helper.

v1..v2:

- addressed comments from Daniel Stone, Ayan Halder, Mihail Atanassov
- coding style fixes** BLURB HERE ***


Andrzej Pietrasiewicz (34):
  drm/core: Add afbc helper functions
  drm/gem-fb-helper: Allow drivers to allocate struct drm_framebuffer on
    their own
  drm/gem-fb-helper: Add special version of drm_gem_fb_size_check
  drm/gem-fb-helper: Add generic afbc size checks
  drm/komeda: Use afbc helper
  drm/komeda: Move checking src coordinates to komeda_fb_create
  drm/komeda: Use the already available local variable
  drm/komeda: Retrieve drm_format_info once
  drm/komeda: Explicitly require 1 plane for AFBC
  drm/komeda: Move pitches comparison to komeda_fb_create
  drm/komeda: Provide and use komeda_fb_get_pixel_addr variant not
    requiring a fb
  drm/komeda: Factor out object lookups for non-afbc case
  drm/komeda: Make komeda_fb_none_size_check independent from
    framebuffer
  drm/komeda: Factor out object lookups for afbc case
  drm/komeda: Free komeda_fb_afbc_size_check from framebuffer dependency
  drm/komeda: Simplify error handling
  drm/komeda: Move object lookup before size checks
  drm/komeda: Move object assignments to framebuffer to after size
    checks
  drm/komeda: Make the size checks independent from framebuffer
    structure
  drm/komeda: Move helper invocation to after size checks
  drm/komeda: Use helper for common tasks
  drm/komeda: Use return value of drm_gem_fb_lookup
  drm/komeda: Use special helper for non-afbc size checks
  drm/komeda: Factor in the invocation of special helper
  drm/komeda: Use special helper for afbc case size check
  drm/komeda: Factor in the invocation of special helper, afbc case
  drm/komeda: Move special helper invocation outside if-else
  drm/komeda: Move to helper checking afbc buffer size
  drm/arm/malidp: Make verify funcitons invocations independent
  drm/arm/malidp: Integrate verify functions
  drm/arm/malidp: Factor in afbc framebuffer verification
  drm/arm/malidp: Use generic helpers for afbc checks
  drm/rockchip: Use helper for common task
  drm/rockchip: Add support for afbc

 .../arm/display/komeda/d71/d71_component.c    |   6 +-
 .../arm/display/komeda/komeda_framebuffer.c   | 273 ++++++++---------
 .../arm/display/komeda/komeda_framebuffer.h   |  21 +-
 .../display/komeda/komeda_pipeline_state.c    |  14 +-
 drivers/gpu/drm/arm/malidp_drv.c              | 155 ++++------
 drivers/gpu/drm/drm_fourcc.c                  |  53 ++++
 drivers/gpu/drm/drm_gem_framebuffer_helper.c  | 287 ++++++++++++++----
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    | 111 ++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 147 ++++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |  12 +
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c   |  83 ++++-
 include/drm/drm_fourcc.h                      |   4 +
 include/drm/drm_framebuffer.h                 |  50 +++
 include/drm/drm_gem_framebuffer_helper.h      |  34 +++
 14 files changed, 907 insertions(+), 343 deletions(-)

Comments

Andrzej Pietrasiewicz Jan. 30, 2020, 9:08 a.m. UTC | #1
Hi All,

A gentle reminder.

Please also see inline:

W dniu 17.12.2019 o 15:49, Andrzej Pietrasiewicz pisze:
> This series adds AFBC support for Rockchip. It is inspired by:
> 
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> 
> This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
> rework has been done, the main goal of which was to move all afbc-related
> checks to helpers, so that core does not deal with it.
> 
> A new struct drm_afbc_framebuffer is added, which stores afbc-related
> driver-specific data. Because of that, in drivers that wish to
> use this feature, the struct must be allocated directly in the driver
> code rather than inside helpers, so the first portion of the patchset
> does the necessary refactoring.
> 
> Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
> the latter being the ultimate purpose of this work and the 3 subsequent
> portions of the patchset move komeda and malidp to generic helpers and add
> afbc support to rockchip.

If changes in komeda and malidp is too much to digest at a time I can
focus on rockchip only. This would amount to patches 1-4 and 33-34.
After all the ultimate purpose of this work and time spent on it
is landing afbc support for rockchip.

Regards,

Andrzej
Liviu Dudau Jan. 30, 2020, 11:44 a.m. UTC | #2
Hi Andrzej,

Sorry for the delay in reviewing the patches. I am hoping to get through the review
early next week if that is OK with you.

Best regards,
Liviu

On Thu, Jan 30, 2020 at 10:08:15AM +0100, Andrzej Pietrasiewicz wrote:
> Hi All,
> 
> A gentle reminder.
> 
> Please also see inline:
> 
> W dniu 17.12.2019 o 15:49, Andrzej Pietrasiewicz pisze:
> > This series adds AFBC support for Rockchip. It is inspired by:
> > 
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > 
> > This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
> > rework has been done, the main goal of which was to move all afbc-related
> > checks to helpers, so that core does not deal with it.
> > 
> > A new struct drm_afbc_framebuffer is added, which stores afbc-related
> > driver-specific data. Because of that, in drivers that wish to
> > use this feature, the struct must be allocated directly in the driver
> > code rather than inside helpers, so the first portion of the patchset
> > does the necessary refactoring.
> > 
> > Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
> > the latter being the ultimate purpose of this work and the 3 subsequent
> > portions of the patchset move komeda and malidp to generic helpers and add
> > afbc support to rockchip.
> 
> If changes in komeda and malidp is too much to digest at a time I can
> focus on rockchip only. This would amount to patches 1-4 and 33-34.
> After all the ultimate purpose of this work and time spent on it
> is landing afbc support for rockchip.
> 
> Regards,
> 
> Andrzej
Andrzej Pietrasiewicz Jan. 30, 2020, 11:57 a.m. UTC | #3
Hi Liviu,

W dniu 30.01.2020 o 12:44, Liviu Dudau pisze:
> Hi Andrzej,
> 
> Sorry for the delay in reviewing the patches. I am hoping to get through the review
> early next week if that is OK with you.

Thanks, that would be great.

Andrzej

> 
> Best regards,
> Liviu
> 
> On Thu, Jan 30, 2020 at 10:08:15AM +0100, Andrzej Pietrasiewicz wrote:
>> Hi All,
>>
>> A gentle reminder.
>>
>> Please also see inline:
>>
>> W dniu 17.12.2019 o 15:49, Andrzej Pietrasiewicz pisze:
>>> This series adds AFBC support for Rockchip. It is inspired by:
>>>
>>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>
>>> This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
>>> rework has been done, the main goal of which was to move all afbc-related
>>> checks to helpers, so that core does not deal with it.
>>>
>>> A new struct drm_afbc_framebuffer is added, which stores afbc-related
>>> driver-specific data. Because of that, in drivers that wish to
>>> use this feature, the struct must be allocated directly in the driver
>>> code rather than inside helpers, so the first portion of the patchset
>>> does the necessary refactoring.
>>>
>>> Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
>>> the latter being the ultimate purpose of this work and the 3 subsequent
>>> portions of the patchset move komeda and malidp to generic helpers and add
>>> afbc support to rockchip.
>>
>> If changes in komeda and malidp is too much to digest at a time I can
>> focus on rockchip only. This would amount to patches 1-4 and 33-34.
>> After all the ultimate purpose of this work and time spent on it
>> is landing afbc support for rockchip.
>>
>> Regards,
>>
>> Andrzej
>
Andrzej Pietrasiewicz Feb. 7, 2020, 11:44 a.m. UTC | #4
Hi All again,

@malidp and komeda folks: I kindly ask you to have your say. If there is
no interest from you (which is ok with me) I will resend the series
dropping the komeda and malidp part.

Andrzej

W dniu 30.01.2020 o 10:08, Andrzej Pietrasiewicz pisze:
> Hi All,
> 
> A gentle reminder.
> 
> Please also see inline:
> 
> W dniu 17.12.2019 o 15:49, Andrzej Pietrasiewicz pisze:
>> This series adds AFBC support for Rockchip. It is inspired by:
>>
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
>>
>>
>> This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
>> rework has been done, the main goal of which was to move all afbc-related
>> checks to helpers, so that core does not deal with it.
>>
>> A new struct drm_afbc_framebuffer is added, which stores afbc-related
>> driver-specific data. Because of that, in drivers that wish to
>> use this feature, the struct must be allocated directly in the driver
>> code rather than inside helpers, so the first portion of the patchset
>> does the necessary refactoring.
>>
>> Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
>> the latter being the ultimate purpose of this work and the 3 subsequent
>> portions of the patchset move komeda and malidp to generic helpers and add
>> afbc support to rockchip.
> 
> If changes in komeda and malidp is too much to digest at a time I can
> focus on rockchip only. This would amount to patches 1-4 and 33-34.
> After all the ultimate purpose of this work and time spent on it
> is landing afbc support for rockchip.
> 
> Regards,
> 
> Andrzej
Liviu Dudau Feb. 7, 2020, 5:10 p.m. UTC | #5
On Fri, Feb 07, 2020 at 12:44:28PM +0100, Andrzej Pietrasiewicz wrote:
> Hi All again,

Hi Andrzej,

> 
> @malidp and komeda folks: I kindly ask you to have your say. If there is
> no interest from you (which is ok with me) I will resend the series
> dropping the komeda and malidp part.

I *am* trying to test your patches, unfortunately my colleagues in China have been
kept out of the office for longer than I thought they will, so it is a bit difficult
to coordinate.

As a backup, if I don't manage to give you feedback by end of Tuesday, please re-send
the patches with malidp and rockchip and skip the komeda ones.

Best regards,
Liviu

> 
> Andrzej
> 
> W dniu 30.01.2020 o 10:08, Andrzej Pietrasiewicz pisze:
> > Hi All,
> > 
> > A gentle reminder.
> > 
> > Please also see inline:
> > 
> > W dniu 17.12.2019 o 15:49, Andrzej Pietrasiewicz pisze:
> > > This series adds AFBC support for Rockchip. It is inspired by:
> > > 
> > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > > 
> > > 
> > > This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
> > > rework has been done, the main goal of which was to move all afbc-related
> > > checks to helpers, so that core does not deal with it.
> > > 
> > > A new struct drm_afbc_framebuffer is added, which stores afbc-related
> > > driver-specific data. Because of that, in drivers that wish to
> > > use this feature, the struct must be allocated directly in the driver
> > > code rather than inside helpers, so the first portion of the patchset
> > > does the necessary refactoring.
> > > 
> > > Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
> > > the latter being the ultimate purpose of this work and the 3 subsequent
> > > portions of the patchset move komeda and malidp to generic helpers and add
> > > afbc support to rockchip.
> > 
> > If changes in komeda and malidp is too much to digest at a time I can
> > focus on rockchip only. This would amount to patches 1-4 and 33-34.
> > After all the ultimate purpose of this work and time spent on it
> > is landing afbc support for rockchip.
> > 
> > Regards,
> > 
> > Andrzej
>
Daniel Vetter Feb. 20, 2020, 4:54 p.m. UTC | #6
On Tue, Dec 17, 2019 at 03:49:46PM +0100, Andrzej Pietrasiewicz wrote:
> This series adds AFBC support for Rockchip. It is inspired by:
> 
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> 
> This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
> rework has been done, the main goal of which was to move all afbc-related
> checks to helpers, so that core does not deal with it.
> 
> A new struct drm_afbc_framebuffer is added, which stores afbc-related
> driver-specific data. Because of that, in drivers that wish to
> use this feature, the struct must be allocated directly in the driver
> code rather than inside helpers, so the first portion of the patchset
> does the necessary refactoring.
> 
> Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
> the latter being the ultimate purpose of this work and the 3 subsequent
> portions of the patchset move komeda and malidp to generic helpers and add
> afbc support to rockchip.
> 
> The idea is to make all afbc users follow a similar pattern. In fb_create()
> they allocate struct drm_afbc_framebuffer, do their specific checks which
> can be done before object lookups, do object lookups and a special version
> of a size check, which understands struct drm_afbc_framebuffer, followed
> by any other driver-specific checks and initializing the gem object.
> The helpers for the common parts are factored out so that drivers
> can use them.
> 
> The komeda driver has been the farthest away from such a pattern, so it
> required most changes. However, due to the fact that I don't have any
> komeda hardware I did the changes to komeda in an incremental fashion with
> a series of (usually) very small, easy to understand steps. malidp was
> pretty straightforward, and rockchip's afbc checks follow the pattern.
> 
> I kindly ask for reviewing the series. I need to mention that my ultimate
> goal is merging afbc for rockchip and I don't have other hardware, so some
> help from malidp and komeda developers/maintainers would be appreciated.
> 
> @Liviu, @James, @Mihail, @Brian: a kind request to you to have a look and
> test the patchset, as I don't have appropriate hardware.
> 
> Rebased onto drm-misc-next.
> 
> v4..v5:
> - used proper way of subclassing drm_framebuffer (Daniel Vetter)
> - added documentation to exported functions (Liviu Dudau)
> - reordered new functions in drm_gem_framebuffer_helper.c to make a saner
> diff (Liviu Dudau)
> - used "2" suffix instead of "_special" for the special version of size
> checks (Liviu Dudau)
> - dropped unnecessarily added condition in drm_get_format_info() (Liviu
> Dudau)
> - dropped "block_size = 0;" trick in framebuffer_check() (Daniel Vetter)
> - relaxed sticking to 80 characters per line rule in some cases
> 
> v3..v4:
> 
> - addressed (some) comments from Daniel Stone, Ezequiel Garcia, Daniel
> Vetter and James Qian Wang - thank you for input
> - refactored helpers to ease accommodating drivers with afbc needs
> - moved afbc checks to helpers
> - converted komeda, malidp and (the newly added) rockchip to use the afbc
> helpers
> - eliminated a separate, dedicated source code file
> 
> v2..v3:
> 
> - addressed (some) comments from Daniel Stone, Liviu Dudau, Daniel Vetter
> and Brian Starkey - thank you all
> 
> In this iteration some rework has been done. The checking logic is now moved
> to framebuffer_check() so it is common to all drivers. But the common part
> is not good for komeda, so this series is not good for merging yet.
> I kindly ask for feedback whether the changes are in the right direction.
> I also kindly ask for input on how to accommodate komeda.
> 
> The CONFIG_DRM_AFBC option has been eliminated in favour of adding
> drm_afbc.c to drm_kms_helper.
> 
> v1..v2:
> 
> - addressed comments from Daniel Stone, Ayan Halder, Mihail Atanassov
> - coding style fixes** BLURB HERE ***
> 
> 
> Andrzej Pietrasiewicz (34):
>   drm/core: Add afbc helper functions
>   drm/gem-fb-helper: Allow drivers to allocate struct drm_framebuffer on
>     their own
>   drm/gem-fb-helper: Add special version of drm_gem_fb_size_check
>   drm/gem-fb-helper: Add generic afbc size checks
>   drm/komeda: Use afbc helper
>   drm/komeda: Move checking src coordinates to komeda_fb_create
>   drm/komeda: Use the already available local variable
>   drm/komeda: Retrieve drm_format_info once
>   drm/komeda: Explicitly require 1 plane for AFBC
>   drm/komeda: Move pitches comparison to komeda_fb_create
>   drm/komeda: Provide and use komeda_fb_get_pixel_addr variant not
>     requiring a fb
>   drm/komeda: Factor out object lookups for non-afbc case
>   drm/komeda: Make komeda_fb_none_size_check independent from
>     framebuffer
>   drm/komeda: Factor out object lookups for afbc case
>   drm/komeda: Free komeda_fb_afbc_size_check from framebuffer dependency
>   drm/komeda: Simplify error handling
>   drm/komeda: Move object lookup before size checks
>   drm/komeda: Move object assignments to framebuffer to after size
>     checks
>   drm/komeda: Make the size checks independent from framebuffer
>     structure
>   drm/komeda: Move helper invocation to after size checks
>   drm/komeda: Use helper for common tasks
>   drm/komeda: Use return value of drm_gem_fb_lookup
>   drm/komeda: Use special helper for non-afbc size checks
>   drm/komeda: Factor in the invocation of special helper
>   drm/komeda: Use special helper for afbc case size check
>   drm/komeda: Factor in the invocation of special helper, afbc case
>   drm/komeda: Move special helper invocation outside if-else
>   drm/komeda: Move to helper checking afbc buffer size
>   drm/arm/malidp: Make verify funcitons invocations independent
>   drm/arm/malidp: Integrate verify functions
>   drm/arm/malidp: Factor in afbc framebuffer verification
>   drm/arm/malidp: Use generic helpers for afbc checks
>   drm/rockchip: Use helper for common task
>   drm/rockchip: Add support for afbc
> 
>  .../arm/display/komeda/d71/d71_component.c    |   6 +-
>  .../arm/display/komeda/komeda_framebuffer.c   | 273 ++++++++---------
>  .../arm/display/komeda/komeda_framebuffer.h   |  21 +-
>  .../display/komeda/komeda_pipeline_state.c    |  14 +-
>  drivers/gpu/drm/arm/malidp_drv.c              | 155 ++++------
>  drivers/gpu/drm/drm_fourcc.c                  |  53 ++++
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c  | 287 ++++++++++++++----
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c    | 111 ++++++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 147 ++++++++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |  12 +
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c   |  83 ++++-
>  include/drm/drm_fourcc.h                      |   4 +
>  include/drm/drm_framebuffer.h                 |  50 +++
>  include/drm/drm_gem_framebuffer_helper.h      |  34 +++
>  14 files changed, 907 insertions(+), 343 deletions(-)

I think this isn't achieving it's goal. Even if we take out the rockchip
enabling patch at the ent it's still like 200 lines more for something
that's supposed to unify and clean code up.

Plus it looks enormously complicated, something that I missed in my
previous quick glance. Hence proposal for all the things you're going to
add to drm core/helpers, and not a bit more :-)

int
drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb,
			   struct drm_device *dev, struct drm_file *file,
			   const struct drm_mode_fb_cmd2 *mode_cmd,
			   const struct drm_framebuffer_funcs *funcs);

This is going to do _exactly_ what drm_gem_fb_create_with_funcs already
does, except it doesn't do the kzalloc (so that would need to be moved out
so we can share code). No other additional sub-parts exposed, I think
that's just not worth it in this case. So none of this size check stuff.

2nd piece, your drm_afbc_framebuffer as in patch 4, with the subclassing.

3rd piece, again in drm_gem_framebuffer_helper.c:

int drm_gem_afbc_init(struct drm_afbc_framebuffer *afbc_fb);

Drivers are supposed to call this after they've a) allocated their fb
structure, containing the drm_afbc_framebuffer somewhere and b) called
drm_gem_fb_init_with_funcs(). This function is going to fill out all the
additional fields, and this function is also going to do all the size
validation and everything else.

Nothing else, so no finer split up of helper check functions, or of afbc
computation functions, or of anything else. That mix of split-out stuff
and mix of computed values in drm_afbc_framebuffer but also functions that
compute afbc values from modifiers and fb sizes seems to just lead to a
huge confusion and not actually to a code reduction. So
- none of the functions exported in patch 1, just stuff them into
  drm_gem_framebuffer_helper.c.
- none of the helper subfunctions you export in patch 2, or adapt in patch
  3
- Also not this size check structure with the data pointer you add in
  patch 4.

The bikeshed I'm still seeing here is whether drm_afbc_framebuffer and the
drm_gem_afbc_init() function should be considered core stuff or not, I
guess you can make an argument for either.

This should also make conversion easier since as a first steps you'd do:

- Put the new drm_afbc_framebuffer in place and adjust all tha
  fb_to_komeda functions and upcasting (this should be mechanically)

- Add the call to drm_gem_afbc_init().

- Starting using the new values in drm_afbc_framebuffer and slowly delete
  code

- Optional, but would be nice to do: Convert driver over to
  drm_gem_fb_init_with_funcs().

Thoughts?

Cheers, Daniel
Daniel Vetter Feb. 21, 2020, 7:54 p.m. UTC | #7
On Thu, Feb 20, 2020 at 5:54 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Dec 17, 2019 at 03:49:46PM +0100, Andrzej Pietrasiewicz wrote:
> > This series adds AFBC support for Rockchip. It is inspired by:
> >
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/factory-gru-9017.B-chromeos-4.4/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> >
> > This is the fifth iteration of the afbc series. Between v3 and v4 a lot of
> > rework has been done, the main goal of which was to move all afbc-related
> > checks to helpers, so that core does not deal with it.
> >
> > A new struct drm_afbc_framebuffer is added, which stores afbc-related
> > driver-specific data. Because of that, in drivers that wish to
> > use this feature, the struct must be allocated directly in the driver
> > code rather than inside helpers, so the first portion of the patchset
> > does the necessary refactoring.
> >
> > Then, there are 3 users of afbc: komeda, malidp and, finally, rockchip,
> > the latter being the ultimate purpose of this work and the 3 subsequent
> > portions of the patchset move komeda and malidp to generic helpers and add
> > afbc support to rockchip.
> >
> > The idea is to make all afbc users follow a similar pattern. In fb_create()
> > they allocate struct drm_afbc_framebuffer, do their specific checks which
> > can be done before object lookups, do object lookups and a special version
> > of a size check, which understands struct drm_afbc_framebuffer, followed
> > by any other driver-specific checks and initializing the gem object.
> > The helpers for the common parts are factored out so that drivers
> > can use them.
> >
> > The komeda driver has been the farthest away from such a pattern, so it
> > required most changes. However, due to the fact that I don't have any
> > komeda hardware I did the changes to komeda in an incremental fashion with
> > a series of (usually) very small, easy to understand steps. malidp was
> > pretty straightforward, and rockchip's afbc checks follow the pattern.
> >
> > I kindly ask for reviewing the series. I need to mention that my ultimate
> > goal is merging afbc for rockchip and I don't have other hardware, so some
> > help from malidp and komeda developers/maintainers would be appreciated.
> >
> > @Liviu, @James, @Mihail, @Brian: a kind request to you to have a look and
> > test the patchset, as I don't have appropriate hardware.
> >
> > Rebased onto drm-misc-next.
> >
> > v4..v5:
> > - used proper way of subclassing drm_framebuffer (Daniel Vetter)
> > - added documentation to exported functions (Liviu Dudau)
> > - reordered new functions in drm_gem_framebuffer_helper.c to make a saner
> > diff (Liviu Dudau)
> > - used "2" suffix instead of "_special" for the special version of size
> > checks (Liviu Dudau)
> > - dropped unnecessarily added condition in drm_get_format_info() (Liviu
> > Dudau)
> > - dropped "block_size = 0;" trick in framebuffer_check() (Daniel Vetter)
> > - relaxed sticking to 80 characters per line rule in some cases
> >
> > v3..v4:
> >
> > - addressed (some) comments from Daniel Stone, Ezequiel Garcia, Daniel
> > Vetter and James Qian Wang - thank you for input
> > - refactored helpers to ease accommodating drivers with afbc needs
> > - moved afbc checks to helpers
> > - converted komeda, malidp and (the newly added) rockchip to use the afbc
> > helpers
> > - eliminated a separate, dedicated source code file
> >
> > v2..v3:
> >
> > - addressed (some) comments from Daniel Stone, Liviu Dudau, Daniel Vetter
> > and Brian Starkey - thank you all
> >
> > In this iteration some rework has been done. The checking logic is now moved
> > to framebuffer_check() so it is common to all drivers. But the common part
> > is not good for komeda, so this series is not good for merging yet.
> > I kindly ask for feedback whether the changes are in the right direction.
> > I also kindly ask for input on how to accommodate komeda.
> >
> > The CONFIG_DRM_AFBC option has been eliminated in favour of adding
> > drm_afbc.c to drm_kms_helper.
> >
> > v1..v2:
> >
> > - addressed comments from Daniel Stone, Ayan Halder, Mihail Atanassov
> > - coding style fixes** BLURB HERE ***
> >
> >
> > Andrzej Pietrasiewicz (34):
> >   drm/core: Add afbc helper functions
> >   drm/gem-fb-helper: Allow drivers to allocate struct drm_framebuffer on
> >     their own
> >   drm/gem-fb-helper: Add special version of drm_gem_fb_size_check
> >   drm/gem-fb-helper: Add generic afbc size checks
> >   drm/komeda: Use afbc helper
> >   drm/komeda: Move checking src coordinates to komeda_fb_create
> >   drm/komeda: Use the already available local variable
> >   drm/komeda: Retrieve drm_format_info once
> >   drm/komeda: Explicitly require 1 plane for AFBC
> >   drm/komeda: Move pitches comparison to komeda_fb_create
> >   drm/komeda: Provide and use komeda_fb_get_pixel_addr variant not
> >     requiring a fb
> >   drm/komeda: Factor out object lookups for non-afbc case
> >   drm/komeda: Make komeda_fb_none_size_check independent from
> >     framebuffer
> >   drm/komeda: Factor out object lookups for afbc case
> >   drm/komeda: Free komeda_fb_afbc_size_check from framebuffer dependency
> >   drm/komeda: Simplify error handling
> >   drm/komeda: Move object lookup before size checks
> >   drm/komeda: Move object assignments to framebuffer to after size
> >     checks
> >   drm/komeda: Make the size checks independent from framebuffer
> >     structure
> >   drm/komeda: Move helper invocation to after size checks
> >   drm/komeda: Use helper for common tasks
> >   drm/komeda: Use return value of drm_gem_fb_lookup
> >   drm/komeda: Use special helper for non-afbc size checks
> >   drm/komeda: Factor in the invocation of special helper
> >   drm/komeda: Use special helper for afbc case size check
> >   drm/komeda: Factor in the invocation of special helper, afbc case
> >   drm/komeda: Move special helper invocation outside if-else
> >   drm/komeda: Move to helper checking afbc buffer size
> >   drm/arm/malidp: Make verify funcitons invocations independent
> >   drm/arm/malidp: Integrate verify functions
> >   drm/arm/malidp: Factor in afbc framebuffer verification
> >   drm/arm/malidp: Use generic helpers for afbc checks
> >   drm/rockchip: Use helper for common task
> >   drm/rockchip: Add support for afbc
> >
> >  .../arm/display/komeda/d71/d71_component.c    |   6 +-
> >  .../arm/display/komeda/komeda_framebuffer.c   | 273 ++++++++---------
> >  .../arm/display/komeda/komeda_framebuffer.h   |  21 +-
> >  .../display/komeda/komeda_pipeline_state.c    |  14 +-
> >  drivers/gpu/drm/arm/malidp_drv.c              | 155 ++++------
> >  drivers/gpu/drm/drm_fourcc.c                  |  53 ++++
> >  drivers/gpu/drm/drm_gem_framebuffer_helper.c  | 287 ++++++++++++++----
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c    | 111 ++++++-
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 147 ++++++++-
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h   |  12 +
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c   |  83 ++++-
> >  include/drm/drm_fourcc.h                      |   4 +
> >  include/drm/drm_framebuffer.h                 |  50 +++
> >  include/drm/drm_gem_framebuffer_helper.h      |  34 +++
> >  14 files changed, 907 insertions(+), 343 deletions(-)
>
> I think this isn't achieving it's goal. Even if we take out the rockchip
> enabling patch at the ent it's still like 200 lines more for something
> that's supposed to unify and clean code up.
>
> Plus it looks enormously complicated, something that I missed in my
> previous quick glance. Hence proposal for all the things you're going to
> add to drm core/helpers, and not a bit more :-)
>
> int
> drm_gem_fb_init_with_funcs(struct drm_framebuffer *fb,
>                            struct drm_device *dev, struct drm_file *file,
>                            const struct drm_mode_fb_cmd2 *mode_cmd,
>                            const struct drm_framebuffer_funcs *funcs);
>
> This is going to do _exactly_ what drm_gem_fb_create_with_funcs already
> does, except it doesn't do the kzalloc (so that would need to be moved out
> so we can share code). No other additional sub-parts exposed, I think
> that's just not worth it in this case. So none of this size check stuff.
>
> 2nd piece, your drm_afbc_framebuffer as in patch 4, with the subclassing.
>
> 3rd piece, again in drm_gem_framebuffer_helper.c:
>
> int drm_gem_afbc_init(struct drm_afbc_framebuffer *afbc_fb);
>
> Drivers are supposed to call this after they've a) allocated their fb
> structure, containing the drm_afbc_framebuffer somewhere and b) called
> drm_gem_fb_init_with_funcs(). This function is going to fill out all the
> additional fields, and this function is also going to do all the size
> validation and everything else.
>
> Nothing else, so no finer split up of helper check functions, or of afbc
> computation functions, or of anything else. That mix of split-out stuff
> and mix of computed values in drm_afbc_framebuffer but also functions that
> compute afbc values from modifiers and fb sizes seems to just lead to a
> huge confusion and not actually to a code reduction. So
> - none of the functions exported in patch 1, just stuff them into
>   drm_gem_framebuffer_helper.c.
> - none of the helper subfunctions you export in patch 2, or adapt in patch
>   3
> - Also not this size check structure with the data pointer you add in
>   patch 4.
>
> The bikeshed I'm still seeing here is whether drm_afbc_framebuffer and the
> drm_gem_afbc_init() function should be considered core stuff or not, I
> guess you can make an argument for either.
>
> This should also make conversion easier since as a first steps you'd do:
>
> - Put the new drm_afbc_framebuffer in place and adjust all tha
>   fb_to_komeda functions and upcasting (this should be mechanically)
>
> - Add the call to drm_gem_afbc_init().
>
> - Starting using the new values in drm_afbc_framebuffer and slowly delete
>   code
>
> - Optional, but would be nice to do: Convert driver over to
>   drm_gem_fb_init_with_funcs().
>
> Thoughts?

I forgot to add that maybe we'll need a ->get_format_info
implementation which supplies a suitable structure (with block_size ==
0 to disable the checks). But I'm kinda assume that the buffer will be
bigger than a compressed one, since you also have the compression
metadata on top. So just keeping the size checks for uncompressed
buffer, and then doing the additional validation on top, might work
out.

But yeah if that fails we'll need a drm_afbc_gem_get_format_info()
hook that drivers can set up.
-Daniel

>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch