| Message ID | 20190217024852.23328-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | VSP1: Display writeback support | expand |
Hi Laurent, On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > Hello, > > This patch series implements display writeback support for the R-Car > Gen3 platforms in the VSP1 driver. > > DRM/KMS provides a writeback API through a special type of writeback > connectors. This series takes a different approach by exposing writeback > as a V4L2 device. While there is nothing fundamentally wrong with > writeback connectors, display for R-Car Gen3 platforms relies on the > VSP1 driver behind the scene, which already implements V4L2 support. > Enabling writeback through V4L2 is thus significantly easier in this > case. How does this look to an application? (I'm entirely ignorant about R-Car). They are interacting with the DRM device, and then need to open and configure a v4l2 device to get the writeback? Can the process which isn't controlling the DRM device independently capture the screen output? I didn't see any major complication to implementing this as a writeback connector. If you want/need to use the vb2_queue, couldn't you just do that entirely from within the kernel? Honestly (predictably?), to me it seems like a bad idea to introduce a second, non-compatible interface for display writeback. Any application interested in display writeback (e.g. compositors) will need to implement both in order to support all HW. drm_hwcomposer already supports writeback via DRM writeback connectors. While I can see the advantages of having writeback exposed via v4l2 for streaming use-cases, I think it would be better to have it done in such a way that it works for all writeback connectors, rather than being VSP1-specific. That would allow any application to choose whichever method is most appropriate for their use-case, without limiting themselves to a subset of hardware. > > The writeback pixel format is restricted to RGB, due to the VSP1 > outputting RGB to the display and lacking a separate colour space > conversion unit for writeback. The resolution can be freely picked by > will result in cropping or composing, not scaling. > > Writeback requests are queued to the hardware on page flip (atomic > flush), and complete at the next vblank. This means that a queued > writeback buffer will not be processed until the next page flip, but > once it starts being written to by the VSP, it will complete at the next > vblank regardless of whether another page flip occurs at that time. > This sounds the same as mali-dp, and so fits directly with the semantics of writeback connectors. Thanks, -Brian > The code is based on a merge of the media master branch, the drm-next > branch and the R-Car DT next branch. For convenience patches can be > found at > > git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > > Kieran Bingham (2): > Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > media: vsp1: Provide a writeback video device > > Laurent Pinchart (5): > media: vsp1: wpf: Fix partition configuration for display pipelines > media: vsp1: Replace leftover occurrence of fragment with body > media: vsp1: Fix addresses of display-related registers for VSP-DL > media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > media: vsp1: Replace the display list internal flag with a flags field > > drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > drivers/media/platform/vsp1/vsp1_video.h | 6 + > drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > 11 files changed, 378 insertions(+), 75 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Brian, On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > > Hello, > > > > This patch series implements display writeback support for the R-Car > > Gen3 platforms in the VSP1 driver. > > > > DRM/KMS provides a writeback API through a special type of writeback > > connectors. This series takes a different approach by exposing writeback > > as a V4L2 device. While there is nothing fundamentally wrong with > > writeback connectors, display for R-Car Gen3 platforms relies on the > > VSP1 driver behind the scene, which already implements V4L2 support. > > Enabling writeback through V4L2 is thus significantly easier in this > > case. > > How does this look to an application? (I'm entirely ignorant about > R-Car). They are interacting with the DRM device, and then need to > open and configure a v4l2 device to get the writeback? Can the process > which isn't controlling the DRM device independently capture the > screen output? It can. The V4L2 capture device is independently controlled, and doesn't affect the DRM/KMS device. The only dependency is that the V4L2 device will only provide frames on atomic commit, a blocking VIDIOC_DQBUF or a select()/poll() call will block until the atomic commit. > I didn't see any major complication to implementing this as a > writeback connector. If you want/need to use the vb2_queue, couldn't > you just do that entirely from within the kernel? In theory yes, in practice possibly, but with a higher complexity. I didn't go for V4L2 to avoid writeback connectors, but because the infrastructure was there already in the VSP driver. > Honestly (predictably?), to me it seems like a bad idea to introduce a > second, non-compatible interface for display writeback. Any > application interested in display writeback (e.g. compositors) will > need to implement both in order to support all HW. drm_hwcomposer > already supports writeback via DRM writeback connectors. > > While I can see the advantages of having writeback exposed via v4l2 > for streaming use-cases, I think it would be better to have it done in > such a way that it works for all writeback connectors, rather than > being VSP1-specific. That would allow any application to choose > whichever method is most appropriate for their use-case, without > limiting themselves to a subset of hardware. I get your point. There's no much use arguing, as I believe you're right :-) I'll give this another shot using writeback connectors. > > The writeback pixel format is restricted to RGB, due to the VSP1 > > outputting RGB to the display and lacking a separate colour space > > conversion unit for writeback. The resolution can be freely picked by > > will result in cropping or composing, not scaling. > > > > Writeback requests are queued to the hardware on page flip (atomic > > flush), and complete at the next vblank. This means that a queued > > writeback buffer will not be processed until the next page flip, but > > once it starts being written to by the VSP, it will complete at the next > > vblank regardless of whether another page flip occurs at that time. > > This sounds the same as mali-dp, and so fits directly with the > semantics of writeback connectors. > > > The code is based on a merge of the media master branch, the drm-next > > branch and the R-Car DT next branch. For convenience patches can be > > found at > > > > git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > > > > Kieran Bingham (2): > > Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > > media: vsp1: Provide a writeback video device > > > > Laurent Pinchart (5): > > media: vsp1: wpf: Fix partition configuration for display pipelines > > media: vsp1: Replace leftover occurrence of fragment with body > > media: vsp1: Fix addresses of display-related registers for VSP-DL > > media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > > media: vsp1: Replace the display list internal flag with a flags field > > > > drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > > drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > > drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > > drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > > drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > > drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > > drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > > drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > > drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > > drivers/media/platform/vsp1/vsp1_video.h | 6 + > > drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > > 11 files changed, 378 insertions(+), 75 deletions(-)
Hi Brian, On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > > Hello, > > > > This patch series implements display writeback support for the R-Car > > Gen3 platforms in the VSP1 driver. > > > > DRM/KMS provides a writeback API through a special type of writeback > > connectors. This series takes a different approach by exposing writeback > > as a V4L2 device. While there is nothing fundamentally wrong with > > writeback connectors, display for R-Car Gen3 platforms relies on the > > VSP1 driver behind the scene, which already implements V4L2 support. > > Enabling writeback through V4L2 is thus significantly easier in this > > case. > > How does this look to an application? (I'm entirely ignorant about > R-Car). They are interacting with the DRM device, and then need to > open and configure a v4l2 device to get the writeback? Can the process > which isn't controlling the DRM device independently capture the > screen output? > > I didn't see any major complication to implementing this as a > writeback connector. If you want/need to use the vb2_queue, couldn't > you just do that entirely from within the kernel? > > Honestly (predictably?), to me it seems like a bad idea to introduce a > second, non-compatible interface for display writeback. Any > application interested in display writeback (e.g. compositors) will > need to implement both in order to support all HW. drm_hwcomposer > already supports writeback via DRM writeback connectors. > > While I can see the advantages of having writeback exposed via v4l2 > for streaming use-cases, I think it would be better to have it done in > such a way that it works for all writeback connectors, rather than > being VSP1-specific. That would allow any application to choose > whichever method is most appropriate for their use-case, without > limiting themselves to a subset of hardware. So I gave writeback connectors a go, and it wasn't very pretty. There writeback support in the DRM core leaks jobs, and is missing support for the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job driver-specific data. I'm working on these issues and will submit patches. In the meantime, I need to test my implementation, so I need a command line application that will let me exercise the API. I assume you've tested your code, haven't you ? :-) Could you tell me how I can test writeback ? > > The writeback pixel format is restricted to RGB, due to the VSP1 > > outputting RGB to the display and lacking a separate colour space > > conversion unit for writeback. The resolution can be freely picked by > > will result in cropping or composing, not scaling. > > > > Writeback requests are queued to the hardware on page flip (atomic > > flush), and complete at the next vblank. This means that a queued > > writeback buffer will not be processed until the next page flip, but > > once it starts being written to by the VSP, it will complete at the next > > vblank regardless of whether another page flip occurs at that time. > > This sounds the same as mali-dp, and so fits directly with the > semantics of writeback connectors. > > > The code is based on a merge of the media master branch, the drm-next > > branch and the R-Car DT next branch. For convenience patches can be > > found at > > > > git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > > > > Kieran Bingham (2): > > Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > > media: vsp1: Provide a writeback video device > > > > Laurent Pinchart (5): > > media: vsp1: wpf: Fix partition configuration for display pipelines > > media: vsp1: Replace leftover occurrence of fragment with body > > media: vsp1: Fix addresses of display-related registers for VSP-DL > > media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > > media: vsp1: Replace the display list internal flag with a flags field > > > > drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > > drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > > drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > > drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > > drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > > drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > > drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > > drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > > drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > > drivers/media/platform/vsp1/vsp1_video.h | 6 + > > drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > > 11 files changed, 378 insertions(+), 75 deletions(-)
Hi Laurent, On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote: > Hi Brian, > > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > > On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > > > Hello, > > > > > > This patch series implements display writeback support for the R-Car > > > Gen3 platforms in the VSP1 driver. > > > > > > DRM/KMS provides a writeback API through a special type of writeback > > > connectors. This series takes a different approach by exposing writeback > > > as a V4L2 device. While there is nothing fundamentally wrong with > > > writeback connectors, display for R-Car Gen3 platforms relies on the > > > VSP1 driver behind the scene, which already implements V4L2 support. > > > Enabling writeback through V4L2 is thus significantly easier in this > > > case. > > > > How does this look to an application? (I'm entirely ignorant about > > R-Car). They are interacting with the DRM device, and then need to > > open and configure a v4l2 device to get the writeback? Can the process > > which isn't controlling the DRM device independently capture the > > screen output? > > > > I didn't see any major complication to implementing this as a > > writeback connector. If you want/need to use the vb2_queue, couldn't > > you just do that entirely from within the kernel? > > > > Honestly (predictably?), to me it seems like a bad idea to introduce a > > second, non-compatible interface for display writeback. Any > > application interested in display writeback (e.g. compositors) will > > need to implement both in order to support all HW. drm_hwcomposer > > already supports writeback via DRM writeback connectors. > > > > While I can see the advantages of having writeback exposed via v4l2 > > for streaming use-cases, I think it would be better to have it done in > > such a way that it works for all writeback connectors, rather than > > being VSP1-specific. That would allow any application to choose > > whichever method is most appropriate for their use-case, without > > limiting themselves to a subset of hardware. > > So I gave writeback connectors a go, and it wasn't very pretty. Sorry you didn't have a good time :-( > There writeback support in the DRM core leaks jobs, Is this the cleanup on check fail, or something else? One possible pitfall is that you must set the job in the connector state to NULL after you call drm_writeback_queue_job(). The API there could easily be changed to pass in the connector_state and clear it in drm_writeback_queue_job() instead of relying on drivers to do it. > and is missing support for > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job > driver-specific data. I'm working on these issues and will submit > patches. > Hm, yes that didn't occur to me; we don't have a prepare_fb callback. > In the meantime, I need to test my implementation, so I need a command > line application that will let me exercise the API. I assume you've > tested your code, haven't you ? :-) Could you tell me how I can test > writeback ? Indeed, there's igts on the list which I wrote and tested: https://patchwork.kernel.org/patch/10764975/ And there's support in drm_hwcomposer (though I must admit I haven't personally run the drm_hwc code): https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 I'm afraid I haven't really touched any of the writeback code for a couple of years - Liviu picked that up. He's on holiday until Monday, but he should be able to help with the status of the igts. Hope that helps, -Brian > > > > The writeback pixel format is restricted to RGB, due to the VSP1 > > > outputting RGB to the display and lacking a separate colour space > > > conversion unit for writeback. The resolution can be freely picked by > > > will result in cropping or composing, not scaling. > > > > > > Writeback requests are queued to the hardware on page flip (atomic > > > flush), and complete at the next vblank. This means that a queued > > > writeback buffer will not be processed until the next page flip, but > > > once it starts being written to by the VSP, it will complete at the next > > > vblank regardless of whether another page flip occurs at that time. > > > > This sounds the same as mali-dp, and so fits directly with the > > semantics of writeback connectors. > > > > > The code is based on a merge of the media master branch, the drm-next > > > branch and the R-Car DT next branch. For convenience patches can be > > > found at > > > > > > git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > > > > > > Kieran Bingham (2): > > > Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > > > media: vsp1: Provide a writeback video device > > > > > > Laurent Pinchart (5): > > > media: vsp1: wpf: Fix partition configuration for display pipelines > > > media: vsp1: Replace leftover occurrence of fragment with body > > > media: vsp1: Fix addresses of display-related registers for VSP-DL > > > media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > > > media: vsp1: Replace the display list internal flag with a flags field > > > > > > drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > > > drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > > > drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > > > drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > > > drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > > > drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > > > drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > > > drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > > > drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > > > drivers/media/platform/vsp1/vsp1_video.h | 6 + > > > drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > > > 11 files changed, 378 insertions(+), 75 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Brian, On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote: > On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote: > > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > >> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > >>> Hello, > >>> > >>> This patch series implements display writeback support for the R-Car > >>> Gen3 platforms in the VSP1 driver. > >>> > >>> DRM/KMS provides a writeback API through a special type of writeback > >>> connectors. This series takes a different approach by exposing writeback > >>> as a V4L2 device. While there is nothing fundamentally wrong with > >>> writeback connectors, display for R-Car Gen3 platforms relies on the > >>> VSP1 driver behind the scene, which already implements V4L2 support. > >>> Enabling writeback through V4L2 is thus significantly easier in this > >>> case. > >> > >> How does this look to an application? (I'm entirely ignorant about > >> R-Car). They are interacting with the DRM device, and then need to > >> open and configure a v4l2 device to get the writeback? Can the process > >> which isn't controlling the DRM device independently capture the > >> screen output? > >> > >> I didn't see any major complication to implementing this as a > >> writeback connector. If you want/need to use the vb2_queue, couldn't > >> you just do that entirely from within the kernel? > >> > >> Honestly (predictably?), to me it seems like a bad idea to introduce a > >> second, non-compatible interface for display writeback. Any > >> application interested in display writeback (e.g. compositors) will > >> need to implement both in order to support all HW. drm_hwcomposer > >> already supports writeback via DRM writeback connectors. > >> > >> While I can see the advantages of having writeback exposed via v4l2 > >> for streaming use-cases, I think it would be better to have it done in > >> such a way that it works for all writeback connectors, rather than > >> being VSP1-specific. That would allow any application to choose > >> whichever method is most appropriate for their use-case, without > >> limiting themselves to a subset of hardware. > > > > So I gave writeback connectors a go, and it wasn't very pretty. > > Sorry you didn't have a good time :-( No worries. That was to be expected with such young code :-) > > There writeback support in the DRM core leaks jobs, > > Is this the cleanup on check fail, or something else? Yes, that's the problem. I have patches for it that I will post soon. > One possible pitfall is that you must set the job in the connector > state to NULL after you call drm_writeback_queue_job(). The API there > could easily be changed to pass in the connector_state and clear it in > drm_writeback_queue_job() instead of relying on drivers to do it. I also have a patch for that :-) > > and is missing support for > > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job > > driver-specific data. I'm working on these issues and will submit > > patches. > > Hm, yes that didn't occur to me; we don't have a prepare_fb callback. > > > In the meantime, I need to test my implementation, so I need a command > > line application that will let me exercise the API. I assume you've > > tested your code, haven't you ? :-) Could you tell me how I can test > > writeback ? > > Indeed, there's igts on the list which I wrote and tested: > > https://patchwork.kernel.org/patch/10764975/ Will you get these merged ? Pushing everybody to use the writeback connector API without any test is mainline isn't nice, it almost makes me want to go back to V4L2. igt test cases are nice to have, but what I need now is a tool to execise the API manually, similar to modetest, with command line parameters to configure the device, and the ability to capture frames to disk using writeback. How did you perform such tests when you developed writeback support ? > And there's support in drm_hwcomposer (though I must admit I haven't > personally run the drm_hwc code): > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 That won't help me much as I don't have an android port for the R-Car boards. > I'm afraid I haven't really touched any of the writeback code for a > couple of years - Liviu picked that up. He's on holiday until Monday, > but he should be able to help with the status of the igts. > > Hope that helps, > > >>> The writeback pixel format is restricted to RGB, due to the VSP1 > >>> outputting RGB to the display and lacking a separate colour space > >>> conversion unit for writeback. The resolution can be freely picked by > >>> will result in cropping or composing, not scaling. > >>> > >>> Writeback requests are queued to the hardware on page flip (atomic > >>> flush), and complete at the next vblank. This means that a queued > >>> writeback buffer will not be processed until the next page flip, but > >>> once it starts being written to by the VSP, it will complete at the next > >>> vblank regardless of whether another page flip occurs at that time. > >> > >> This sounds the same as mali-dp, and so fits directly with the > >> semantics of writeback connectors. > >> > >>> The code is based on a merge of the media master branch, the drm-next > >>> branch and the R-Car DT next branch. For convenience patches can be > >>> found at > >>> > >>> git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > >>> > >>> Kieran Bingham (2): > >>> Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > >>> media: vsp1: Provide a writeback video device > >>> > >>> Laurent Pinchart (5): > >>> media: vsp1: wpf: Fix partition configuration for display pipelines > >>> media: vsp1: Replace leftover occurrence of fragment with body > >>> media: vsp1: Fix addresses of display-related registers for VSP-DL > >>> media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > >>> media: vsp1: Replace the display list internal flag with a flags field > >>> > >>> drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > >>> drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > >>> drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > >>> drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > >>> drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > >>> drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > >>> drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > >>> drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > >>> drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > >>> drivers/media/platform/vsp1/vsp1_video.h | 6 + > >>> drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > >>> 11 files changed, 378 insertions(+), 75 deletions(-)
Hi Laurent, On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote: > Hi Brian, > > On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote: > > On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote: > > > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > > >> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > > >>> Hello, > > >>> > > >>> This patch series implements display writeback support for the R-Car > > >>> Gen3 platforms in the VSP1 driver. > > >>> > > >>> DRM/KMS provides a writeback API through a special type of writeback > > >>> connectors. This series takes a different approach by exposing writeback > > >>> as a V4L2 device. While there is nothing fundamentally wrong with > > >>> writeback connectors, display for R-Car Gen3 platforms relies on the > > >>> VSP1 driver behind the scene, which already implements V4L2 support. > > >>> Enabling writeback through V4L2 is thus significantly easier in this > > >>> case. > > >> > > >> How does this look to an application? (I'm entirely ignorant about > > >> R-Car). They are interacting with the DRM device, and then need to > > >> open and configure a v4l2 device to get the writeback? Can the process > > >> which isn't controlling the DRM device independently capture the > > >> screen output? > > >> > > >> I didn't see any major complication to implementing this as a > > >> writeback connector. If you want/need to use the vb2_queue, couldn't > > >> you just do that entirely from within the kernel? > > >> > > >> Honestly (predictably?), to me it seems like a bad idea to introduce a > > >> second, non-compatible interface for display writeback. Any > > >> application interested in display writeback (e.g. compositors) will > > >> need to implement both in order to support all HW. drm_hwcomposer > > >> already supports writeback via DRM writeback connectors. > > >> > > >> While I can see the advantages of having writeback exposed via v4l2 > > >> for streaming use-cases, I think it would be better to have it done in > > >> such a way that it works for all writeback connectors, rather than > > >> being VSP1-specific. That would allow any application to choose > > >> whichever method is most appropriate for their use-case, without > > >> limiting themselves to a subset of hardware. > > > > > > So I gave writeback connectors a go, and it wasn't very pretty. > > > > Sorry you didn't have a good time :-( > > No worries. That was to be expected with such young code :-) > > > > There writeback support in the DRM core leaks jobs, > > > > Is this the cleanup on check fail, or something else? > > Yes, that's the problem. I have patches for it that I will post soon. > > > One possible pitfall is that you must set the job in the connector > > state to NULL after you call drm_writeback_queue_job(). The API there > > could easily be changed to pass in the connector_state and clear it in > > drm_writeback_queue_job() instead of relying on drivers to do it. > > I also have a patch for that :-) > > > > and is missing support for > > > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job > > > driver-specific data. I'm working on these issues and will submit > > > patches. > > > > Hm, yes that didn't occur to me; we don't have a prepare_fb callback. > > > > > In the meantime, I need to test my implementation, so I need a command > > > line application that will let me exercise the API. I assume you've > > > tested your code, haven't you ? :-) Could you tell me how I can test > > > writeback ? > > > > Indeed, there's igts on the list which I wrote and tested: > > > > https://patchwork.kernel.org/patch/10764975/ > > Will you get these merged ? Pushing everybody to use the writeback > connector API without any test is mainline isn't nice, it almost makes > me want to go back to V4L2. I wasn't trying to be pushy - I only shared my opinion that I didn't think it was a good idea to introduce a second display writeback API, when we already have one. You're entirely entitled to ignore my opinion. The tests have been available since the very early versions of the writeback series. I don't know what's blocking them from merging, I haven't been tracking it very closely. If you'd be happy to provide your review and test on them, that may help the process along? > > igt test cases are nice to have, but what I need now is a tool to > execise the API manually, similar to modetest, with command line > parameters to configure the device, and the ability to capture frames to > disk using writeback. How did you perform such tests when you developed > writeback support ? > I used a pre-existing internal tool which does exactly that. I appreciate that we don't have upstream tooling for writeback. As you say, it's a young API (well, not by date, but certainly by usage). I also do appreciate you taking the time to consider it, identifying issues which we did not, and for fixing them. The only way it stops being a young API, with bugs and no tooling, is if people adopt it. Thanks, -Brian > > And there's support in drm_hwcomposer (though I must admit I haven't > > personally run the drm_hwc code): > > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 > > That won't help me much as I don't have an android port for the R-Car > boards. > > > I'm afraid I haven't really touched any of the writeback code for a > > couple of years - Liviu picked that up. He's on holiday until Monday, > > but he should be able to help with the status of the igts. > > > > Hope that helps, > > > > >>> The writeback pixel format is restricted to RGB, due to the VSP1 > > >>> outputting RGB to the display and lacking a separate colour space > > >>> conversion unit for writeback. The resolution can be freely picked by > > >>> will result in cropping or composing, not scaling. > > >>> > > >>> Writeback requests are queued to the hardware on page flip (atomic > > >>> flush), and complete at the next vblank. This means that a queued > > >>> writeback buffer will not be processed until the next page flip, but > > >>> once it starts being written to by the VSP, it will complete at the next > > >>> vblank regardless of whether another page flip occurs at that time. > > >> > > >> This sounds the same as mali-dp, and so fits directly with the > > >> semantics of writeback connectors. > > >> > > >>> The code is based on a merge of the media master branch, the drm-next > > >>> branch and the R-Car DT next branch. For convenience patches can be > > >>> found at > > >>> > > >>> git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > > >>> > > >>> Kieran Bingham (2): > > >>> Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > > >>> media: vsp1: Provide a writeback video device > > >>> > > >>> Laurent Pinchart (5): > > >>> media: vsp1: wpf: Fix partition configuration for display pipelines > > >>> media: vsp1: Replace leftover occurrence of fragment with body > > >>> media: vsp1: Fix addresses of display-related registers for VSP-DL > > >>> media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > > >>> media: vsp1: Replace the display list internal flag with a flags field > > >>> > > >>> drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > > >>> drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > > >>> drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > > >>> drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > > >>> drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > > >>> drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > > >>> drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > > >>> drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > > >>> drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > > >>> drivers/media/platform/vsp1/vsp1_video.h | 6 + > > >>> drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > > >>> 11 files changed, 378 insertions(+), 75 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Brian, On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote: > On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote: > > On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote: > >> On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote: > >>> On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > >>>> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > >>>>> Hello, > >>>>> > >>>>> This patch series implements display writeback support for the R-Car > >>>>> Gen3 platforms in the VSP1 driver. > >>>>> > >>>>> DRM/KMS provides a writeback API through a special type of writeback > >>>>> connectors. This series takes a different approach by exposing writeback > >>>>> as a V4L2 device. While there is nothing fundamentally wrong with > >>>>> writeback connectors, display for R-Car Gen3 platforms relies on the > >>>>> VSP1 driver behind the scene, which already implements V4L2 support. > >>>>> Enabling writeback through V4L2 is thus significantly easier in this > >>>>> case. > >>>> > >>>> How does this look to an application? (I'm entirely ignorant about > >>>> R-Car). They are interacting with the DRM device, and then need to > >>>> open and configure a v4l2 device to get the writeback? Can the process > >>>> which isn't controlling the DRM device independently capture the > >>>> screen output? > >>>> > >>>> I didn't see any major complication to implementing this as a > >>>> writeback connector. If you want/need to use the vb2_queue, couldn't > >>>> you just do that entirely from within the kernel? > >>>> > >>>> Honestly (predictably?), to me it seems like a bad idea to introduce a > >>>> second, non-compatible interface for display writeback. Any > >>>> application interested in display writeback (e.g. compositors) will > >>>> need to implement both in order to support all HW. drm_hwcomposer > >>>> already supports writeback via DRM writeback connectors. > >>>> > >>>> While I can see the advantages of having writeback exposed via v4l2 > >>>> for streaming use-cases, I think it would be better to have it done in > >>>> such a way that it works for all writeback connectors, rather than > >>>> being VSP1-specific. That would allow any application to choose > >>>> whichever method is most appropriate for their use-case, without > >>>> limiting themselves to a subset of hardware. > >>> > >>> So I gave writeback connectors a go, and it wasn't very pretty. > >> > >> Sorry you didn't have a good time :-( > > > > No worries. That was to be expected with such young code :-) > > > >>> There writeback support in the DRM core leaks jobs, > >> > >> Is this the cleanup on check fail, or something else? > > > > Yes, that's the problem. I have patches for it that I will post soon. > > > >> One possible pitfall is that you must set the job in the connector > >> state to NULL after you call drm_writeback_queue_job(). The API there > >> could easily be changed to pass in the connector_state and clear it in > >> drm_writeback_queue_job() instead of relying on drivers to do it. > > > > I also have a patch for that :-) > > > >>> and is missing support for > >>> the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job > >>> driver-specific data. I'm working on these issues and will submit > >>> patches. > >> > >> Hm, yes that didn't occur to me; we don't have a prepare_fb callback. > >> > >>> In the meantime, I need to test my implementation, so I need a command > >>> line application that will let me exercise the API. I assume you've > >>> tested your code, haven't you ? :-) Could you tell me how I can test > >>> writeback ? > >> > >> Indeed, there's igts on the list which I wrote and tested: > >> > >> https://patchwork.kernel.org/patch/10764975/ > > > > Will you get these merged ? Pushing everybody to use the writeback > > connector API without any test is mainline isn't nice, it almost makes > > me want to go back to V4L2. > > I wasn't trying to be pushy - I only shared my opinion that I didn't > think it was a good idea to introduce a second display writeback API, > when we already have one. You're entirely entitled to ignore my > opinion. I agree we should aim for a single API. My preference would have been for V4L2 universally, but now that we have writeback connectors upstream, the choice has been made, so we should stick to it. > The tests have been available since the very early versions of the > writeback series. I don't know what's blocking them from merging, I > haven't been tracking it very closely. > > If you'd be happy to provide your review and test on them, that may > help the process along? Now that I've sent an entirely untested patch series out, this is next on my list :-) > > igt test cases are nice to have, but what I need now is a tool to > > execise the API manually, similar to modetest, with command line > > parameters to configure the device, and the ability to capture frames to > > disk using writeback. How did you perform such tests when you developed > > writeback support ? > > I used a pre-existing internal tool which does exactly that. Any hope of sharing the sources ? > I appreciate that we don't have upstream tooling for writeback. As you > say, it's a young API (well, not by date, but certainly by usage). > > I also do appreciate you taking the time to consider it, identifying > issues which we did not, and for fixing them. The only way it stops > being a young API, with bugs and no tooling, is if people adopt it. If the developers who initially pushed the API upstream without an open-source test tool could spend a bit of time on this issue, I'm sure it would help too :-) > >> And there's support in drm_hwcomposer (though I must admit I haven't > >> personally run the drm_hwc code): > >> > >> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 > > > > That won't help me much as I don't have an android port for the R-Car > > boards. > > > >> I'm afraid I haven't really touched any of the writeback code for a > >> couple of years - Liviu picked that up. He's on holiday until Monday, > >> but he should be able to help with the status of the igts. > >> > >> Hope that helps, > >> > >>>>> The writeback pixel format is restricted to RGB, due to the VSP1 > >>>>> outputting RGB to the display and lacking a separate colour space > >>>>> conversion unit for writeback. The resolution can be freely picked by > >>>>> will result in cropping or composing, not scaling. > >>>>> > >>>>> Writeback requests are queued to the hardware on page flip (atomic > >>>>> flush), and complete at the next vblank. This means that a queued > >>>>> writeback buffer will not be processed until the next page flip, but > >>>>> once it starts being written to by the VSP, it will complete at the next > >>>>> vblank regardless of whether another page flip occurs at that time. > >>>> > >>>> This sounds the same as mali-dp, and so fits directly with the > >>>> semantics of writeback connectors. > >>>> > >>>>> The code is based on a merge of the media master branch, the drm-next > >>>>> branch and the R-Car DT next branch. For convenience patches can be > >>>>> found at > >>>>> > >>>>> git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > >>>>> > >>>>> Kieran Bingham (2): > >>>>> Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > >>>>> media: vsp1: Provide a writeback video device > >>>>> > >>>>> Laurent Pinchart (5): > >>>>> media: vsp1: wpf: Fix partition configuration for display pipelines > >>>>> media: vsp1: Replace leftover occurrence of fragment with body > >>>>> media: vsp1: Fix addresses of display-related registers for VSP-DL > >>>>> media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > >>>>> media: vsp1: Replace the display list internal flag with a flags field > >>>>> > >>>>> drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > >>>>> drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > >>>>> drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > >>>>> drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > >>>>> drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > >>>>> drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > >>>>> drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > >>>>> drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > >>>>> drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > >>>>> drivers/media/platform/vsp1/vsp1_video.h | 6 + > >>>>> drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > >>>>> 11 files changed, 378 insertions(+), 75 deletions(-)
On Thu, Feb 21, 2019 at 02:23:10PM +0200, Laurent Pinchart wrote: > On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote: [snip] > > > > I used a pre-existing internal tool which does exactly that. > > Any hope of sharing the sources ? > Not in a timescale or form which would be useful to you. I'm convinced people only ask questions like this to make us look like Bad Guys. Opening everything up is a process, and it's going to take us time. Sure we could be doing better, but I also think there's a lot of people who do worse. > > I appreciate that we don't have upstream tooling for writeback. As you > > say, it's a young API (well, not by date, but certainly by usage). > > > > I also do appreciate you taking the time to consider it, identifying > > issues which we did not, and for fixing them. The only way it stops > > being a young API, with bugs and no tooling, is if people adopt it. > > If the developers who initially pushed the API upstream without an > open-source test tool could spend a bit of time on this issue, I'm sure > it would help too :-) > No one suggested a test test tool before. In fact, the DRM subsystem explicitly requires that features land with something that isn't only a test tool, hence why we did drm_hwcomposer. That said, yes, we should be trying harder to get the igts landed. I personally think igts are far more useful than a random example C file, but I guess opinions differ. Thanks, -Brian
On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote: > Hi Laurent, > > On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote: > > Hi Brian, > > > > On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote: > > > On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote: > > > > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote: > > > >> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote: > > > >>> Hello, > > > >>> > > > >>> This patch series implements display writeback support for the R-Car > > > >>> Gen3 platforms in the VSP1 driver. > > > >>> > > > >>> DRM/KMS provides a writeback API through a special type of writeback > > > >>> connectors. This series takes a different approach by exposing writeback > > > >>> as a V4L2 device. While there is nothing fundamentally wrong with > > > >>> writeback connectors, display for R-Car Gen3 platforms relies on the > > > >>> VSP1 driver behind the scene, which already implements V4L2 support. > > > >>> Enabling writeback through V4L2 is thus significantly easier in this > > > >>> case. > > > >> > > > >> How does this look to an application? (I'm entirely ignorant about > > > >> R-Car). They are interacting with the DRM device, and then need to > > > >> open and configure a v4l2 device to get the writeback? Can the process > > > >> which isn't controlling the DRM device independently capture the > > > >> screen output? > > > >> > > > >> I didn't see any major complication to implementing this as a > > > >> writeback connector. If you want/need to use the vb2_queue, couldn't > > > >> you just do that entirely from within the kernel? > > > >> > > > >> Honestly (predictably?), to me it seems like a bad idea to introduce a > > > >> second, non-compatible interface for display writeback. Any > > > >> application interested in display writeback (e.g. compositors) will > > > >> need to implement both in order to support all HW. drm_hwcomposer > > > >> already supports writeback via DRM writeback connectors. > > > >> > > > >> While I can see the advantages of having writeback exposed via v4l2 > > > >> for streaming use-cases, I think it would be better to have it done in > > > >> such a way that it works for all writeback connectors, rather than > > > >> being VSP1-specific. That would allow any application to choose > > > >> whichever method is most appropriate for their use-case, without > > > >> limiting themselves to a subset of hardware. > > > > > > > > So I gave writeback connectors a go, and it wasn't very pretty. > > > > > > Sorry you didn't have a good time :-( > > > > No worries. That was to be expected with such young code :-) > > > > > > There writeback support in the DRM core leaks jobs, > > > > > > Is this the cleanup on check fail, or something else? > > > > Yes, that's the problem. I have patches for it that I will post soon. > > > > > One possible pitfall is that you must set the job in the connector > > > state to NULL after you call drm_writeback_queue_job(). The API there > > > could easily be changed to pass in the connector_state and clear it in > > > drm_writeback_queue_job() instead of relying on drivers to do it. > > > > I also have a patch for that :-) > > > > > > and is missing support for > > > > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job > > > > driver-specific data. I'm working on these issues and will submit > > > > patches. > > > > > > Hm, yes that didn't occur to me; we don't have a prepare_fb callback. > > > > > > > In the meantime, I need to test my implementation, so I need a command > > > > line application that will let me exercise the API. I assume you've > > > > tested your code, haven't you ? :-) Could you tell me how I can test > > > > writeback ? > > > > > > Indeed, there's igts on the list which I wrote and tested: > > > > > > https://patchwork.kernel.org/patch/10764975/ > > > > Will you get these merged ? Pushing everybody to use the writeback > > connector API without any test is mainline isn't nice, it almost makes > > me want to go back to V4L2. > > I wasn't trying to be pushy - I only shared my opinion that I didn't > think it was a good idea to introduce a second display writeback API, > when we already have one. You're entirely entitled to ignore my > opinion. > > The tests have been available since the very early versions of the > writeback series. I don't know what's blocking them from merging, I > haven't been tracking it very closely. > > If you'd be happy to provide your review and test on them, that may > help the process along? > > > > > igt test cases are nice to have, but what I need now is a tool to > > execise the API manually, similar to modetest, with command line > > parameters to configure the device, and the ability to capture frames to > > disk using writeback. How did you perform such tests when you developed > > writeback support ? > > > > I used a pre-existing internal tool which does exactly that. > > I appreciate that we don't have upstream tooling for writeback. As you > say, it's a young API (well, not by date, but certainly by usage). > > I also do appreciate you taking the time to consider it, identifying > issues which we did not, and for fixing them. The only way it stops > being a young API, with bugs and no tooling, is if people adopt it. Mentioned on irc already to Laurent, but here for completeness: igt has pretty decent support for combining that into one utility. We support piles of standard stuff like --interactive-debug already, plus you can add whatever additional things you need and feel like. There's quite a few such igt tests/tools combinations already. -Daniel > > Thanks, > -Brian > > > > And there's support in drm_hwcomposer (though I must admit I haven't > > > personally run the drm_hwc code): > > > > > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 > > > > That won't help me much as I don't have an android port for the R-Car > > boards. > > > > > I'm afraid I haven't really touched any of the writeback code for a > > > couple of years - Liviu picked that up. He's on holiday until Monday, > > > but he should be able to help with the status of the igts. > > > > > > Hope that helps, > > > > > > >>> The writeback pixel format is restricted to RGB, due to the VSP1 > > > >>> outputting RGB to the display and lacking a separate colour space > > > >>> conversion unit for writeback. The resolution can be freely picked by > > > >>> will result in cropping or composing, not scaling. > > > >>> > > > >>> Writeback requests are queued to the hardware on page flip (atomic > > > >>> flush), and complete at the next vblank. This means that a queued > > > >>> writeback buffer will not be processed until the next page flip, but > > > >>> once it starts being written to by the VSP, it will complete at the next > > > >>> vblank regardless of whether another page flip occurs at that time. > > > >> > > > >> This sounds the same as mali-dp, and so fits directly with the > > > >> semantics of writeback connectors. > > > >> > > > >>> The code is based on a merge of the media master branch, the drm-next > > > >>> branch and the R-Car DT next branch. For convenience patches can be > > > >>> found at > > > >>> > > > >>> git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback > > > >>> > > > >>> Kieran Bingham (2): > > > >>> Revert "[media] v4l: vsp1: Supply frames to the DU continuously" > > > >>> media: vsp1: Provide a writeback video device > > > >>> > > > >>> Laurent Pinchart (5): > > > >>> media: vsp1: wpf: Fix partition configuration for display pipelines > > > >>> media: vsp1: Replace leftover occurrence of fragment with body > > > >>> media: vsp1: Fix addresses of display-related registers for VSP-DL > > > >>> media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse > > > >>> media: vsp1: Replace the display list internal flag with a flags field > > > >>> > > > >>> drivers/media/platform/vsp1/vsp1_dl.c | 118 ++++++++++++-- > > > >>> drivers/media/platform/vsp1/vsp1_dl.h | 6 +- > > > >>> drivers/media/platform/vsp1/vsp1_drm.c | 24 ++- > > > >>> drivers/media/platform/vsp1/vsp1_drv.c | 17 +- > > > >>> drivers/media/platform/vsp1/vsp1_pipe.c | 5 + > > > >>> drivers/media/platform/vsp1/vsp1_pipe.h | 6 + > > > >>> drivers/media/platform/vsp1/vsp1_regs.h | 6 +- > > > >>> drivers/media/platform/vsp1/vsp1_rwpf.h | 2 + > > > >>> drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++---- > > > >>> drivers/media/platform/vsp1/vsp1_video.h | 6 + > > > >>> drivers/media/platform/vsp1/vsp1_wpf.c | 65 ++++++-- > > > >>> 11 files changed, 378 insertions(+), 75 deletions(-) > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Brian, On Thu, Feb 21, 2019 at 01:44:56PM +0000, Brian Starkey wrote: > On Thu, Feb 21, 2019 at 02:23:10PM +0200, Laurent Pinchart wrote: > > On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote: > > [snip] > > >> I used a pre-existing internal tool which does exactly that. > > > > Any hope of sharing the sources ? > > Not in a timescale or form which would be useful to you. I'm convinced > people only ask questions like this to make us look like Bad Guys. I didn't have big hopes, but it was a honest question. > Opening everything up is a process, and it's going to take us time. > Sure we could be doing better, but I also think there's a lot of > people who do worse. No question that ARM is neither the worst nor the best. I also value your and the other graphics developer's efforts more than I value ARM's open-source policy, as you are fighting against company policies to drive open-source. > >> I appreciate that we don't have upstream tooling for writeback. As you > >> say, it's a young API (well, not by date, but certainly by usage). > >> > >> I also do appreciate you taking the time to consider it, identifying > >> issues which we did not, and for fixing them. The only way it stops > >> being a young API, with bugs and no tooling, is if people adopt it. > > > > If the developers who initially pushed the API upstream without an > > open-source test tool could spend a bit of time on this issue, I'm sure > > it would help too :-) > > No one suggested a test test tool before. In fact, the DRM subsystem > explicitly requires that features land with something that isn't only > a test tool, hence why we did drm_hwcomposer. > > That said, yes, we should be trying harder to get the igts landed. I > personally think igts are far more useful than a random example C > file, but I guess opinions differ. I think both are useful, for different purposes. Automated test cases are very valuable to test for compliance and catch regressions, while a manualy tool is very valuable during development.