Re: [Freedreno] [DPU PATCH] drm/msm: Remove dpu_edid_parser

[Rob Clark]

On Fri, Feb 23, 2018 at 5:31 PM,   wrote:
> The bitmaps in drm_hdmi_info dont seem to be exposed to userspace.
>
> Our mode selection logic is in userspace at the moment which means its
> better userspace knows which modes support what.
>
> If we decide to move mode validation entirely to the driver, we can use
> these bitmaps to validate.
>
> But this is again a fundamental question then.
>
> Is there no use-case so far where userspace has had to know which modes
> support which formats?
>

I think historically a lot more of the focus upstream so far has been
"desktop style" use cases, where RGB to the sink makes more sense.
That doesn't mean there isn't a use-case for YUV.  I'm not sure, and
haven't really given much thought into, how to expose this to
userspace.  But I guess the same thing comes up w/ HDR and higher
bit-depth RGB modes (like 10_10_10 formats, etc), and people are
giving thought to those things these days.  I haven't been following
the HDR work *that* closely, but I guess if you want to use HDR, you
want to know what modes support it at the sink.

Either way, I think this is something which should be supported in drm
core with a standardized UAPI, rather than by driver rolling it's own
edid parsing and custom UAPI.

(I guess extending the modeinfo struct might be difficult, but perhaps
a blob property w/ supplemental info might make sense??  Or maybe
someone already has some other ideas?)

BR,
-R

> Thanks
>
> Abhinav
>
>
> On 2018-02-23 13:51, Sean Paul wrote:
>>
>> On Fri, Feb 23, 2018 at 01:29:03PM -0800, abhin...@codeaurora.org wrote:
>>>
>>> I am OK with removing some parts of the parser but not entirely.
>>>
>>> Fundamentally, we went ahead with the parser for a couple of reasons:
>>>
>>> -> We would like to be able to associate the color format with the mode.
>>> This helps us decide that if the same mode supports both RGB/YUV then
>>> which
>>> one we can pick.
>>>
>>> -> The upstream parser currently just adds it to the supported formats of
>>> the sink info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; However the
>>> link
>>> between the mode and the format itself is unclear
>>>
>>> I would like to know if any of the above have been addressed so far
>>> before
>>> reverting this.
>>
>>
>> The information of which modes support which formats is stored in bitmaps
>> within
>> drm_hdmi_info. The documentation is here:
>>
>>
>> https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html?highlight=drm_hdmi_info#c.drm_hdmi_info
>>
>> Is this sufficient?
>>
>> Sean
>>
>>>
>>> Thanks
>>>
>>> Abhinav
>>>
>>>
>>> On 2018-02-23 11:15, Sean Paul wrote:
>>> > Use drm_edid to parse the edid instead of the hand rolled dpu version.
>>> >
>>> > Signed-off-by: Sean Paul 
>>> > ---
>>> >  drivers/gpu/drm/msm/Makefile  |   1 -
>>> >  drivers/gpu/drm/msm/dp/dp_audio.c |  22 +-
>>> >  drivers/gpu/drm/msm/dp/dp_display.c   |   7 -
>>> >  drivers/gpu/drm/msm/dp/dp_panel.c |  77 ++-
>>> >  drivers/gpu/drm/msm/dp/dp_panel.h |   7 +-
>>> >  drivers/gpu/drm/msm/dpu_edid_parser.c | 657 --
>>> >  drivers/gpu/drm/msm/dpu_edid_parser.h | 166 ---
>>> >  7 files changed, 55 insertions(+), 882 deletions(-)
>>> >  delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.c
>>> >  delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.h
>>> >
>>> > diff --git a/drivers/gpu/drm/msm/Makefile
>>> > b/drivers/gpu/drm/msm/Makefile
>>> > index fcd85ae28d66..2fb9ba11df19 100644
>>> > --- a/drivers/gpu/drm/msm/Makefile
>>> > +++ b/drivers/gpu/drm/msm/Makefile
>>> > @@ -86,7 +86,6 @@ msm-y := \
>>> >   disp/dpu1/dpu_rm.o \
>>> >   disp/dpu1/dpu_vbif.o \
>>> >   dpu_dbg.o \
>>> > - dpu_edid_parser.o \
>>> >   dpu_io_util.o \
>>> >   dpu_dbg_evtlog.o \
>>> >   dpu_power_handle.o \
>>> > diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c
>>> > b/drivers/gpu/drm/msm/dp/dp_audio.c
>>> > index 4e0cdcd15c8c..ce00b8b52631 100644
>>> > --- a/drivers/gpu/drm/msm/dp/dp_audio.c
>>> > +++ b/drivers/gpu/drm/msm/dp/dp_audio.c
>>> > @@ -18,6 +18,7 @@
>>> >  #include 
>>> >
>>> >  #include 
>>> > +#include 
>>> >
>>> >  #include "dp_catalog.h"
>>> >  #include "dp_audio.h"
>>> > @@ -480,7 +481,6 @@ static int dp_audio_get_edid_blk(struct
>>> > platform_device *pdev,
>>> >  {
>>> >   int rc = 0;
>>> >   struct dp_audio_private *audio;
>>> > - struct dpu_edid_ctrl *edid;
>>> >
>>> >   audio = get_audio_get_data(pdev);
>>> >   if (IS_ERR(audio)) {
>>> > @@ -488,19 +488,29 @@ static int dp_audio_get_edid_blk(struct
>>> > platform_device *pdev,
>>> >   goto end;
>>> >   }
>>> >
>>> > - if (!audio->panel || !audio->panel->edid_ctrl) {
>>> > + if (!audio->panel || !audio->panel->edid) {
>>> >   pr_err("invalid panel data\n");
>>> >   rc = -EINVAL;
>>> >   goto end;
>>> >   }
>>> >
>>> > - edid = audio->panel->edid_ctrl;
>>> > -
>>> > +/*
>>> > + 

Re: [Freedreno] [DPU PATCH] drm/mipi: Remove Qualcomm-specific dsi packet header format

[abhinavk]

Alright, found it

https://cgit.freedesktop.org/~seanpaul/dpu-staging/commit/?h=mtp-testing=34906195473f9e04601c49a45e3fedce0132eb7e

Thanks

Abhinav

On 2018-02-23 07:06, Sean Paul wrote:
On Thu, Feb 22, 2018 at 9:28 PM, Abhinav Kumar  
wrote:
Looks good. Can you point us to the fix done in the dsi-staging 
driver.




All of the downstream changes are in the mtp-testing branch of the
dpu-staging tree. The on-list patches are in the for-next-staging, and
the patches which have been reviewed on list will go to the for-next
branch.

Sean



Thanks

Abhinav

-Original Message-
From: Rob Clark [mailto:robdcl...@gmail.com]
Sent: Thursday, February 22, 2018 11:49 AM
To: Sean Paul 
Cc: freedreno ; linux-arm-msm 
; Kristian H. Kristensen 
; Jeykumar Sankaran ; 
Abhinav Kumar 
Subject: Re: [DPU PATCH] drm/mipi: Remove Qualcomm-specific dsi packet 
header format


On Thu, Feb 22, 2018 at 12:37 PM, Sean Paul  
wrote:

msm/dsi already formats the packet header correctly, so this breaks
every driver except for the downstream dsi-staging driver (which I've
submitted a patch for).

Signed-off-by: Sean Paul 


Reviewed-by: Rob Clark 


---
 drivers/gpu/drm/drm_mipi_dsi.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
b/drivers/gpu/drm/drm_mipi_dsi.c index 688c8a82ba37..4b47226b90d4
100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -454,7 +454,7 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet 
*packet,

return -EINVAL;

memset(packet, 0, sizeof(*packet));
-   packet->header[2] = ((msg->channel & 0x3) << 6) | (msg->type 
& 0x3f);
+   packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type 
&

+ 0x3f);

/* TODO: compute ECC if hardware support is not available */

@@ -466,16 +466,16 @@ int mipi_dsi_create_packet(struct 
mipi_dsi_packet *packet,

 * and 2.
 */
if (mipi_dsi_packet_format_is_long(msg->type)) {
-   packet->header[0] = (msg->tx_len >> 0) & 0xff;
-   packet->header[1] = (msg->tx_len >> 8) & 0xff;
+   packet->header[1] = (msg->tx_len >> 0) & 0xff;
+   packet->header[2] = (msg->tx_len >> 8) & 0xff;

packet->payload_length = msg->tx_len;
packet->payload = msg->tx_buf;
} else {
const u8 *tx = msg->tx_buf;

-   packet->header[0] = (msg->tx_len > 0) ? tx[0] : 0;
-   packet->header[1] = (msg->tx_len > 1) ? tx[1] : 0;
+   packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
+   packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
}

packet->size = sizeof(packet->header) +
packet->payload_length;
--
2.16.1.291.g4437f3f132-goog


--
To unsubscribe from this list: send the line "unsubscribe 
linux-arm-msm" in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH] drm/msm: Remove dpu_edid_parser

[abhinavk]

I am OK with removing some parts of the parser but not entirely.

Fundamentally, we went ahead with the parser for a couple of reasons:

-> We would like to be able to associate the color format with the mode. 
This helps us decide that if the same mode supports both RGB/YUV then 
which one we can pick.


-> The upstream parser currently just adds it to the supported formats 
of the sink info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; However 
the link between the mode and the format itself is unclear


I would like to know if any of the above have been addressed so far 
before reverting this.


Thanks

Abhinav


On 2018-02-23 11:15, Sean Paul wrote:

Use drm_edid to parse the edid instead of the hand rolled dpu version.

Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/msm/Makefile  |   1 -
 drivers/gpu/drm/msm/dp/dp_audio.c |  22 +-
 drivers/gpu/drm/msm/dp/dp_display.c   |   7 -
 drivers/gpu/drm/msm/dp/dp_panel.c |  77 ++-
 drivers/gpu/drm/msm/dp/dp_panel.h |   7 +-
 drivers/gpu/drm/msm/dpu_edid_parser.c | 657 --
 drivers/gpu/drm/msm/dpu_edid_parser.h | 166 ---
 7 files changed, 55 insertions(+), 882 deletions(-)
 delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.c
 delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.h

diff --git a/drivers/gpu/drm/msm/Makefile 
b/drivers/gpu/drm/msm/Makefile

index fcd85ae28d66..2fb9ba11df19 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -86,7 +86,6 @@ msm-y := \
disp/dpu1/dpu_rm.o \
disp/dpu1/dpu_vbif.o \
dpu_dbg.o \
-   dpu_edid_parser.o \
dpu_io_util.o \
dpu_dbg_evtlog.o \
dpu_power_handle.o \
diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c
b/drivers/gpu/drm/msm/dp/dp_audio.c
index 4e0cdcd15c8c..ce00b8b52631 100644
--- a/drivers/gpu/drm/msm/dp/dp_audio.c
+++ b/drivers/gpu/drm/msm/dp/dp_audio.c
@@ -18,6 +18,7 @@
 #include 

 #include 
+#include 

 #include "dp_catalog.h"
 #include "dp_audio.h"
@@ -480,7 +481,6 @@ static int dp_audio_get_edid_blk(struct
platform_device *pdev,
 {
int rc = 0;
struct dp_audio_private *audio;
-   struct dpu_edid_ctrl *edid;

audio = get_audio_get_data(pdev);
if (IS_ERR(audio)) {
@@ -488,19 +488,29 @@ static int dp_audio_get_edid_blk(struct
platform_device *pdev,
goto end;
}

-   if (!audio->panel || !audio->panel->edid_ctrl) {
+   if (!audio->panel || !audio->panel->edid) {
pr_err("invalid panel data\n");
rc = -EINVAL;
goto end;
}

-   edid = audio->panel->edid_ctrl;
-
+/*
+ * TODO:
+ * audio_data_blk should be changed to be a cea_sad array
+ * audio_data_blk_size should be changed to contain a count of sads
+ *
+ * Once this is done, use drm_edid_to_sad() to extract this from
+ * audio->panel->edid. I'd change this, but I don't have that code :/
+ */
+#if 0
blk->audio_data_blk = edid->audio_data_block;
blk->audio_data_blk_size = edid->adb_size;
+#endif
+
+   blk->spk_alloc_data_blk_size = drm_edid_to_speaker_allocation(
+   audio->panel->edid,
+   blk->spkr_alloc_data_block);

-   blk->spk_alloc_data_blk = edid->spkr_alloc_data_block;
-   blk->spk_alloc_data_blk_size = edid->sadb_size;
 end:
return rc;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 6f2c06809bf5..846ef33aecbd 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -373,12 +373,6 @@ static int dp_display_bind(struct device *dev,
struct device *master,
goto end;
}

-   rc = dp->panel->dpu_edid_register(dp->panel);
-   if (rc) {
-   pr_err("DRM DP EDID register failed\n");
-   goto end;
-   }
-
rc = dp->power->power_client_init(dp->power, >phandle);
if (rc) {
pr_err("Power client create failed\n");
@@ -412,7 +406,6 @@ static void dp_display_unbind(struct device *dev,
struct device *master,
}

(void)dp->power->power_client_deinit(dp->power);
-   (void)dp->panel->dpu_edid_deregister(dp->panel);
(void)dp->aux->drm_aux_deregister(dp->aux);
dp_display_deinitialize_hdcp(dp);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 11c3338e23b1..63f9bf407508 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -16,6 +16,9 @@

 #include "dp_panel.h"

+#include 
+#include 
+
 #define DP_PANEL_DEFAULT_BPP 24
 #define DP_MAX_DS_PORT_COUNT 1

@@ -151,9 +154,10 @@ static int dp_panel_read_edid(struct dp_panel 
*dp_panel,

panel = container_of(dp_panel, struct dp_panel_private, dp_panel);

do {
-   dpu_get_edid(connector, >aux->drm_aux->ddc,
-  

Re: [Freedreno] [DPU PATCH] drm/msm: Remove dpu_edid_parser

[abhinavk]

The bitmaps in drm_hdmi_info dont seem to be exposed to userspace.

Our mode selection logic is in userspace at the moment which means its 
better userspace knows which modes support what.


If we decide to move mode validation entirely to the driver, we can use 
these bitmaps to validate.


But this is again a fundamental question then.

Is there no use-case so far where userspace has had to know which modes 
support which formats?


Thanks

Abhinav

On 2018-02-23 13:51, Sean Paul wrote:
On Fri, Feb 23, 2018 at 01:29:03PM -0800, abhin...@codeaurora.org 
wrote:

I am OK with removing some parts of the parser but not entirely.

Fundamentally, we went ahead with the parser for a couple of reasons:

-> We would like to be able to associate the color format with the 
mode.
This helps us decide that if the same mode supports both RGB/YUV then 
which

one we can pick.

-> The upstream parser currently just adds it to the supported formats 
of
the sink info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; However the 
link

between the mode and the format itself is unclear

I would like to know if any of the above have been addressed so far 
before

reverting this.


The information of which modes support which formats is stored in 
bitmaps within

drm_hdmi_info. The documentation is here:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html?highlight=drm_hdmi_info#c.drm_hdmi_info

Is this sufficient?

Sean



Thanks

Abhinav


On 2018-02-23 11:15, Sean Paul wrote:
> Use drm_edid to parse the edid instead of the hand rolled dpu version.
>
> Signed-off-by: Sean Paul 
> ---
>  drivers/gpu/drm/msm/Makefile  |   1 -
>  drivers/gpu/drm/msm/dp/dp_audio.c |  22 +-
>  drivers/gpu/drm/msm/dp/dp_display.c   |   7 -
>  drivers/gpu/drm/msm/dp/dp_panel.c |  77 ++-
>  drivers/gpu/drm/msm/dp/dp_panel.h |   7 +-
>  drivers/gpu/drm/msm/dpu_edid_parser.c | 657 --
>  drivers/gpu/drm/msm/dpu_edid_parser.h | 166 ---
>  7 files changed, 55 insertions(+), 882 deletions(-)
>  delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.c
>  delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.h
>
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index fcd85ae28d66..2fb9ba11df19 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -86,7 +86,6 @@ msm-y := \
>disp/dpu1/dpu_rm.o \
>disp/dpu1/dpu_vbif.o \
>dpu_dbg.o \
> -  dpu_edid_parser.o \
>dpu_io_util.o \
>dpu_dbg_evtlog.o \
>dpu_power_handle.o \
> diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c
> b/drivers/gpu/drm/msm/dp/dp_audio.c
> index 4e0cdcd15c8c..ce00b8b52631 100644
> --- a/drivers/gpu/drm/msm/dp/dp_audio.c
> +++ b/drivers/gpu/drm/msm/dp/dp_audio.c
> @@ -18,6 +18,7 @@
>  #include 
>
>  #include 
> +#include 
>
>  #include "dp_catalog.h"
>  #include "dp_audio.h"
> @@ -480,7 +481,6 @@ static int dp_audio_get_edid_blk(struct
> platform_device *pdev,
>  {
>int rc = 0;
>struct dp_audio_private *audio;
> -  struct dpu_edid_ctrl *edid;
>
>audio = get_audio_get_data(pdev);
>if (IS_ERR(audio)) {
> @@ -488,19 +488,29 @@ static int dp_audio_get_edid_blk(struct
> platform_device *pdev,
>goto end;
>}
>
> -  if (!audio->panel || !audio->panel->edid_ctrl) {
> +  if (!audio->panel || !audio->panel->edid) {
>pr_err("invalid panel data\n");
>rc = -EINVAL;
>goto end;
>}
>
> -  edid = audio->panel->edid_ctrl;
> -
> +/*
> + * TODO:
> + *audio_data_blk should be changed to be a cea_sad array
> + *audio_data_blk_size should be changed to contain a count of sads
> + *
> + *Once this is done, use drm_edid_to_sad() to extract this from
> + *audio->panel->edid. I'd change this, but I don't have that code :/
> + */
> +#if 0
>blk->audio_data_blk = edid->audio_data_block;
>blk->audio_data_blk_size = edid->adb_size;
> +#endif
> +
> +  blk->spk_alloc_data_blk_size = drm_edid_to_speaker_allocation(
> +  audio->panel->edid,
> +  blk->spkr_alloc_data_block);
>
> -  blk->spk_alloc_data_blk = edid->spkr_alloc_data_block;
> -  blk->spk_alloc_data_blk_size = edid->sadb_size;
>  end:
>return rc;
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6f2c06809bf5..846ef33aecbd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -373,12 +373,6 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
>goto end;
>}
>
> -  rc = dp->panel->dpu_edid_register(dp->panel);
> -  if (rc) {
> -  pr_err("DRM DP EDID register failed\n");
> -  goto end;
> -  }
> -
>rc = dp->power->power_client_init(dp->power, >phandle);
>if (rc) {
>pr_err("Power client create failed\n");
> @@ -412,7 +406,6 @@ static void 

Re: [Freedreno] [DPU PATCH] drm/msm: Remove dpu_edid_parser

[Sean Paul]

On Fri, Feb 23, 2018 at 01:29:03PM -0800, abhin...@codeaurora.org wrote:
> I am OK with removing some parts of the parser but not entirely.
> 
> Fundamentally, we went ahead with the parser for a couple of reasons:
> 
> -> We would like to be able to associate the color format with the mode.
> This helps us decide that if the same mode supports both RGB/YUV then which
> one we can pick.
> 
> -> The upstream parser currently just adds it to the supported formats of
> the sink info->color_formats |= DRM_COLOR_FORMAT_YCRCB420; However the link
> between the mode and the format itself is unclear
> 
> I would like to know if any of the above have been addressed so far before
> reverting this.

The information of which modes support which formats is stored in bitmaps within
drm_hdmi_info. The documentation is here:

https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html?highlight=drm_hdmi_info#c.drm_hdmi_info

Is this sufficient?

Sean

> 
> Thanks
> 
> Abhinav
> 
> 
> On 2018-02-23 11:15, Sean Paul wrote:
> > Use drm_edid to parse the edid instead of the hand rolled dpu version.
> > 
> > Signed-off-by: Sean Paul 
> > ---
> >  drivers/gpu/drm/msm/Makefile  |   1 -
> >  drivers/gpu/drm/msm/dp/dp_audio.c |  22 +-
> >  drivers/gpu/drm/msm/dp/dp_display.c   |   7 -
> >  drivers/gpu/drm/msm/dp/dp_panel.c |  77 ++-
> >  drivers/gpu/drm/msm/dp/dp_panel.h |   7 +-
> >  drivers/gpu/drm/msm/dpu_edid_parser.c | 657 --
> >  drivers/gpu/drm/msm/dpu_edid_parser.h | 166 ---
> >  7 files changed, 55 insertions(+), 882 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.c
> >  delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.h
> > 
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index fcd85ae28d66..2fb9ba11df19 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -86,7 +86,6 @@ msm-y := \
> > disp/dpu1/dpu_rm.o \
> > disp/dpu1/dpu_vbif.o \
> > dpu_dbg.o \
> > -   dpu_edid_parser.o \
> > dpu_io_util.o \
> > dpu_dbg_evtlog.o \
> > dpu_power_handle.o \
> > diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c
> > b/drivers/gpu/drm/msm/dp/dp_audio.c
> > index 4e0cdcd15c8c..ce00b8b52631 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_audio.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_audio.c
> > @@ -18,6 +18,7 @@
> >  #include 
> > 
> >  #include 
> > +#include 
> > 
> >  #include "dp_catalog.h"
> >  #include "dp_audio.h"
> > @@ -480,7 +481,6 @@ static int dp_audio_get_edid_blk(struct
> > platform_device *pdev,
> >  {
> > int rc = 0;
> > struct dp_audio_private *audio;
> > -   struct dpu_edid_ctrl *edid;
> > 
> > audio = get_audio_get_data(pdev);
> > if (IS_ERR(audio)) {
> > @@ -488,19 +488,29 @@ static int dp_audio_get_edid_blk(struct
> > platform_device *pdev,
> > goto end;
> > }
> > 
> > -   if (!audio->panel || !audio->panel->edid_ctrl) {
> > +   if (!audio->panel || !audio->panel->edid) {
> > pr_err("invalid panel data\n");
> > rc = -EINVAL;
> > goto end;
> > }
> > 
> > -   edid = audio->panel->edid_ctrl;
> > -
> > +/*
> > + * TODO:
> > + * audio_data_blk should be changed to be a cea_sad array
> > + * audio_data_blk_size should be changed to contain a count of sads
> > + *
> > + * Once this is done, use drm_edid_to_sad() to extract this from
> > + * audio->panel->edid. I'd change this, but I don't have that code :/
> > + */
> > +#if 0
> > blk->audio_data_blk = edid->audio_data_block;
> > blk->audio_data_blk_size = edid->adb_size;
> > +#endif
> > +
> > +   blk->spk_alloc_data_blk_size = drm_edid_to_speaker_allocation(
> > +   audio->panel->edid,
> > +   blk->spkr_alloc_data_block);
> > 
> > -   blk->spk_alloc_data_blk = edid->spkr_alloc_data_block;
> > -   blk->spk_alloc_data_blk_size = edid->sadb_size;
> >  end:
> > return rc;
> >  }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 6f2c06809bf5..846ef33aecbd 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -373,12 +373,6 @@ static int dp_display_bind(struct device *dev,
> > struct device *master,
> > goto end;
> > }
> > 
> > -   rc = dp->panel->dpu_edid_register(dp->panel);
> > -   if (rc) {
> > -   pr_err("DRM DP EDID register failed\n");
> > -   goto end;
> > -   }
> > -
> > rc = dp->power->power_client_init(dp->power, >phandle);
> > if (rc) {
> > pr_err("Power client create failed\n");
> > @@ -412,7 +406,6 @@ static void dp_display_unbind(struct device *dev,
> > struct device *master,
> > }
> > 
> > (void)dp->power->power_client_deinit(dp->power);
> > -   (void)dp->panel->dpu_edid_deregister(dp->panel);
> > (void)dp->aux->drm_aux_deregister(dp->aux);
> > 

[Freedreno] [DPU PATCH] drm/msm: Remove dpu_edid_parser

[Sean Paul]

Use drm_edid to parse the edid instead of the hand rolled dpu version.

Signed-off-by: Sean Paul 
---
 drivers/gpu/drm/msm/Makefile  |   1 -
 drivers/gpu/drm/msm/dp/dp_audio.c |  22 +-
 drivers/gpu/drm/msm/dp/dp_display.c   |   7 -
 drivers/gpu/drm/msm/dp/dp_panel.c |  77 ++-
 drivers/gpu/drm/msm/dp/dp_panel.h |   7 +-
 drivers/gpu/drm/msm/dpu_edid_parser.c | 657 --
 drivers/gpu/drm/msm/dpu_edid_parser.h | 166 ---
 7 files changed, 55 insertions(+), 882 deletions(-)
 delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.c
 delete mode 100644 drivers/gpu/drm/msm/dpu_edid_parser.h

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index fcd85ae28d66..2fb9ba11df19 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -86,7 +86,6 @@ msm-y := \
disp/dpu1/dpu_rm.o \
disp/dpu1/dpu_vbif.o \
dpu_dbg.o \
-   dpu_edid_parser.o \
dpu_io_util.o \
dpu_dbg_evtlog.o \
dpu_power_handle.o \
diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c 
b/drivers/gpu/drm/msm/dp/dp_audio.c
index 4e0cdcd15c8c..ce00b8b52631 100644
--- a/drivers/gpu/drm/msm/dp/dp_audio.c
+++ b/drivers/gpu/drm/msm/dp/dp_audio.c
@@ -18,6 +18,7 @@
 #include 
 
 #include 
+#include 
 
 #include "dp_catalog.h"
 #include "dp_audio.h"
@@ -480,7 +481,6 @@ static int dp_audio_get_edid_blk(struct platform_device 
*pdev,
 {
int rc = 0;
struct dp_audio_private *audio;
-   struct dpu_edid_ctrl *edid;
 
audio = get_audio_get_data(pdev);
if (IS_ERR(audio)) {
@@ -488,19 +488,29 @@ static int dp_audio_get_edid_blk(struct platform_device 
*pdev,
goto end;
}
 
-   if (!audio->panel || !audio->panel->edid_ctrl) {
+   if (!audio->panel || !audio->panel->edid) {
pr_err("invalid panel data\n");
rc = -EINVAL;
goto end;
}
 
-   edid = audio->panel->edid_ctrl;
-
+/*
+ * TODO:
+ * audio_data_blk should be changed to be a cea_sad array
+ * audio_data_blk_size should be changed to contain a count of sads
+ *
+ * Once this is done, use drm_edid_to_sad() to extract this from
+ * audio->panel->edid. I'd change this, but I don't have that code :/
+ */
+#if 0
blk->audio_data_blk = edid->audio_data_block;
blk->audio_data_blk_size = edid->adb_size;
+#endif
+
+   blk->spk_alloc_data_blk_size = drm_edid_to_speaker_allocation(
+   audio->panel->edid,
+   blk->spkr_alloc_data_block);
 
-   blk->spk_alloc_data_blk = edid->spkr_alloc_data_block;
-   blk->spk_alloc_data_blk_size = edid->sadb_size;
 end:
return rc;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 6f2c06809bf5..846ef33aecbd 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -373,12 +373,6 @@ static int dp_display_bind(struct device *dev, struct 
device *master,
goto end;
}
 
-   rc = dp->panel->dpu_edid_register(dp->panel);
-   if (rc) {
-   pr_err("DRM DP EDID register failed\n");
-   goto end;
-   }
-
rc = dp->power->power_client_init(dp->power, >phandle);
if (rc) {
pr_err("Power client create failed\n");
@@ -412,7 +406,6 @@ static void dp_display_unbind(struct device *dev, struct 
device *master,
}
 
(void)dp->power->power_client_deinit(dp->power);
-   (void)dp->panel->dpu_edid_deregister(dp->panel);
(void)dp->aux->drm_aux_deregister(dp->aux);
dp_display_deinitialize_hdcp(dp);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 11c3338e23b1..63f9bf407508 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -16,6 +16,9 @@
 
 #include "dp_panel.h"
 
+#include 
+#include 
+
 #define DP_PANEL_DEFAULT_BPP 24
 #define DP_MAX_DS_PORT_COUNT 1
 
@@ -151,9 +154,10 @@ static int dp_panel_read_edid(struct dp_panel *dp_panel,
panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
 
do {
-   dpu_get_edid(connector, >aux->drm_aux->ddc,
-   (void **)_panel->edid_ctrl);
-   if (!dp_panel->edid_ctrl->edid) {
+   kfree(dp_panel->edid);
+   dp_panel->edid = drm_get_edid(connector,
+ >aux->drm_aux->ddc);
+   if (!dp_panel->edid) {
pr_err("EDID read failed\n");
retry_cnt++;
panel->aux->reconfig(panel->aux);
@@ -163,7 +167,7 @@ static int dp_panel_read_edid(struct dp_panel *dp_panel,
}
} while (retry_cnt < max_retry);
 
-   return -EINVAL;
+   return drm_add_edid_modes(connector, 

Re: [Freedreno] [RFC 4/4] drm/msm/mdp5: writeback support

[Rob Clark]

On Fri, Feb 23, 2018 at 11:30 AM, Sean Paul  wrote:
> On Fri, Feb 23, 2018 at 08:17:54AM -0500, Rob Clark wrote:
>> In a way, based on the original writeback patch from Jilai Wang, but a
>> lot has shifted around since then.
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  drivers/gpu/drm/msm/Makefile  |   1 +
>>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  23 +-
>>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  |  38 +++-
>>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h  |   7 +-
>>  drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c   | 367 
>> ++
>>  drivers/gpu/drm/msm/dsi/dsi_host.c|   4 +-
>>  6 files changed, 431 insertions(+), 9 deletions(-)
>>  create mode 100644 drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index cd40c050b2d7..c9f50adef2db 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -45,6 +45,7 @@ msm-y := \
>>   disp/mdp5/mdp5_mixer.o \
>>   disp/mdp5/mdp5_plane.o \
>>   disp/mdp5/mdp5_smp.o \
>> + disp/mdp5/mdp5_wb.o \
>>   msm_atomic.o \
>>   msm_debugfs.o \
>>   msm_drv.o \
>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
>> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
>> index 9893e43ba6c5..b00ca88b741d 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
>> @@ -484,7 +484,11 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc 
>> *crtc,
>>   }
>>
>>   /* Restore vblank irq handling after power is enabled */
>> - drm_crtc_vblank_on(crtc);
>> +// TODO we can't ->get_scanout_pos() for wb (since virtual intf)..
>> +// perhaps drm core should be clever enough not to 
>> drm_reset_vblank_timestamp()
>> +// for virtual encoders / writeback?
>> + if (mdp5_cstate->pipeline.intf->type != INTF_WB)
>> + drm_crtc_vblank_on(crtc);
>>
>>   mdp5_crtc_mode_set_nofb(crtc);
>>
>> @@ -518,7 +522,11 @@ int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
>>   u32 caps;
>>   int ret;
>>
>> - caps = MDP_LM_CAP_DISPLAY;
>> + if (pipeline->intf->type == INTF_WB)
>> + caps = MDP_LM_CAP_WB;
>> + else
>> + caps = MDP_LM_CAP_DISPLAY;
>> +
>>   if (need_right_mixer)
>>   caps |= MDP_LM_CAP_PAIR;
>>
>> @@ -545,6 +553,7 @@ int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
>>   mdp5_cstate->err_irqmask = intf2err(intf->num);
>>   mdp5_cstate->vblank_irqmask = intf2vblank(pipeline->mixer, intf);
>>
>> +// XXX should we be treating WB as cmd_mode??
>>   if ((intf->type == INTF_DSI) &&
>>   (intf->mode == MDP5_INTF_DSI_MODE_COMMAND)) {
>>   mdp5_cstate->pp_done_irqmask = lm2ppdone(pipeline->mixer);
>> @@ -639,8 +648,12 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
>>   }
>>
>>   /* bail out early if there aren't any planes */
>> - if (!cnt)
>> - return 0;
>> + if (!cnt) {
>> + if (!state->active)
>> + return 0;
>> + dev_err(dev->dev, "%s has no planes!\n", crtc->name);
>> + return -EINVAL;
>> + }
>
> This seems unrelated?
>

hmm, yeah, kinda.  It was a case that I hit before working out all the
bugs in my kmscube writeback mode, but I guess isn't directly related
to wb.

>>
>>   hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
>>
>> @@ -1160,7 +1173,7 @@ void mdp5_crtc_wait_for_commit_done(struct drm_crtc 
>> *crtc)
>>
>>   if (mdp5_cstate->cmd_mode)
>>   mdp5_crtc_wait_for_pp_done(crtc);
>> - else
>> + else if (mdp5_cstate->pipeline.intf->type != INTF_WB)
>>   mdp5_crtc_wait_for_flush_done(crtc);
>>  }
>>
>> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
>> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>> index 1f44d8f15ce1..239010905637 100644
>> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
>> @@ -427,7 +427,8 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>>* the MDP5 interfaces) than the number of layer mixers present in HW,
>>* but let's be safe here anyway
>>*/
>> - num_crtcs = min(priv->num_encoders, mdp5_kms->num_hwmixers);
>> + num_crtcs = min(priv->num_encoders + hw_cfg->wb.count,
>> + mdp5_kms->num_hwmixers);
>>
>>   /*
>>* Construct planes equaling the number of hw pipes, and CRTCs for the
>> @@ -482,6 +483,33 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>>   encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
>>   }
>>
>> + /*
>> +  * Lastly, construct writeback connectors.
>> +  */
>> + for (i = 0; i < hw_cfg->wb.count; i++) {
>> + struct drm_writeback_connector *wb_conn;
>> + struct mdp5_ctl *ctl;
>> +
>> +  

Re: [Freedreno] [PATCH v7 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

[Vivek Gautam]

On Fri, Feb 23, 2018 at 9:10 PM, Jordan Crouse  wrote:
> On Fri, Feb 23, 2018 at 04:06:39PM +0530, Vivek Gautam wrote:
>> On Fri, Feb 23, 2018 at 5:22 AM, Jordan Crouse  
>> wrote:
>> > On Wed, Feb 07, 2018 at 04:01:19PM +0530, Vivek Gautam wrote:
>> >> From: Sricharan R 
>> >>
>> >> The smmu device probe/remove and add/remove master device callbacks
>> >> gets called when the smmu is not linked to its master, that is without
>> >> the context of the master device. So calling runtime apis in those places
>> >> separately.
>> >>
>> >> Signed-off-by: Sricharan R 
>> >> [vivek: Cleanup pm runtime calls]
>> >> Signed-off-by: Vivek Gautam 
>> >> ---
>> >>  drivers/iommu/arm-smmu.c | 42 ++
>> >>  1 file changed, 38 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> >> index 9e2f917e16c2..c024f69c1682 100644
>> >> --- a/drivers/iommu/arm-smmu.c
>> >> +++ b/drivers/iommu/arm-smmu.c
>> >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct 
>> >> iommu_domain *domain)
>> >>   struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> >>   struct arm_smmu_device *smmu = smmu_domain->smmu;
>> >>   struct arm_smmu_cfg *cfg = _domain->cfg;
>> >> - int irq;
>> >> + int ret, irq;
>> >>
>> >>   if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
>> >>   return;
>> >>
>> >> + ret = pm_runtime_get_sync(smmu->dev);
>> >> + if (ret)
>> >> + return;
>> >> +
>> >>   /*
>> >>* Disable the context bank and free the page tables before freeing
>> >>* it.
>> >> @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct 
>> >> iommu_domain *domain)
>> >>
>> >>   free_io_pgtable_ops(smmu_domain->pgtbl_ops);
>> >>   __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
>> >> +
>> >> + pm_runtime_put_sync(smmu->dev);
>> >>  }
>> >>
>> >>  static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>> >> @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev)
>> >>   while (i--)
>> >>   cfg->smendx[i] = INVALID_SMENDX;
>> >>
>> >> - ret = arm_smmu_master_alloc_smes(dev);
>> >> + ret = pm_runtime_get_sync(smmu->dev);
>> >>   if (ret)
>> >>   goto out_cfg_free;
>> >
>> > Hey Vivek, I just hit a problem with this on sdm845. It turns out that
>> > pm_runtime_get_sync() returns a positive 1 if the device is already active.
>> >
>> > I hit this in the GPU code. The a6xx has two platform devices that each 
>> > use a
>> > different sid on the iommu. The GPU is probed normally from a platform 
>> > driver
>> > and it in turn initializes the GMU device by way of a phandle.
>> >
>> > Because the GMU isn't probed with a platform driver we need to call
>> > of_dma_configure() on the device to set up the IOMMU for the device which 
>> > ends
>> > up calling through this path and we discover that the smmu->dev is already
>> > powered (pm_runtime_get_sync returns 1).
>> >
>> > I'm not immediately sure if this is a bug on sdm845 or not because a 
>> > cursory
>> > inspection says that the SMMU device shouldn't be powered at this time but 
>> > there
>> > might be a connection that I'm not seeing. Obviously if the SMMU was left
>> > powered thats a bad thing. But putting that aside it is obvious that this
>> > code should be accommodating of the possibility that the device is already
>> > powered, and so this should be
>> >
>> > if (ret < 0)
>> > goto out_cfg_free;
>>
>> Right, as Tomasz also pointed, we should surely check the negative value of
>> pm_runtime_get_sync().
>
> Sorry, I didn't notice that Tomasz had pointed it out as well. I wanted to
> quickly get it on the mailing list so you could catch it in your time zone.
>
>> From your description, it may be that the GPU has turned on the smmu, and
>> then once if goes and probes the GMU, the GMU device also wants to turn-on
>> the same smmu device. But that's already active. So pm_runtime_get_sync()
>> returns 1.
>> Am i making sense?
>
> My concern is that this is happening during the probe and we shouldn't be
> energizing the GPU at this point. But it is entirely possible that the
> bus is on for other reasons. I'll do a bit of digging today and see exactly
> which device is at fault.

I will try to check it myself too.

regards
Vivek
>
>
> Jordan
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted 

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Sean Paul]

On Fri, Feb 23, 2018 at 04:48:58PM +, Liviu Dudau wrote:
> On Fri, Feb 23, 2018 at 11:43:29AM -0500, Sean Paul wrote:
> > On Fri, Feb 23, 2018 at 11:25:11AM -0500, Rob Clark wrote:
> > > On Fri, Feb 23, 2018 at 10:59 AM, Sean Paul  wrote:
> > > >
> > > > Have we considered hiding writeback behind a client cap instead?
> > > 
> > > It is kinda *almost* unneeded, since the connector reports itself as
> > > disconnected.
> > > 
> > > I'm not sure what the reason was to drop the cap, but I think it would
> > > be better to have a cap so WB connectors don't show up in, for ex,
> > > xrandr
> > 
> > Yeah, the disconnected hack is kind of gross, IMO. I hate to introduce 
> > churn in
> > the patch series given that it was initially introduced with the client cap.
> 
> Haha, that's the reverse of Daniel's position:
> 
> https://lists.freedesktop.org/archives/dri-devel/2016-October/120519.html

Yeah, it happens :(. I don't think it's a dealbreaker either way, it just seems
awkward to expose a connector which is "disconnected", but available for use. I
don't think we have any other connectors which are supposed to be used in this
state.

> 
> > 
> > There are also cases where we might want to make writeback unavailable, 
> > such as
> > when content protection is enabled. In those cases, it's conceivable that we
> > might want to use disconnected as a signal to u/s. I suppose we could also 
> > just
> > fail the check, so most of this is just academic.
> 
> Not sure what other hardware out there does, but on Mali DP's case you
> would be outputing the protected content by putting the display
> processor in secure mode, which automatically disables writeback for us.
> Or to put in another way, you don't need a writeback framebuffer if you
> are in non-secure mode as you can get access to the framebuffer used for
> the plane anyway.

Yeah, I was mostly thinking about the case where you might have HDCP enabled on
the HDMI connector and be able to slurp up the content via a writeback. However
if the buffer is not secure in the first place, then it's already accessible in
userspace so I don't think that writeback presents a new security hole.

/me needs to get HDCP out of his head.

Sean


> 
> Best regards,
> Liviu
> 
> > 
> > Sean
> > 
> > 
> > > 
> > > BR,
> > > -R
> > 
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> 
> -- 
> 
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---
> ¯\_(ツ)_/¯

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Liviu Dudau]

On Fri, Feb 23, 2018 at 11:39:06AM -0500, Sean Paul wrote:
> On Fri, Feb 23, 2018 at 04:21:05PM +, Liviu Dudau wrote:
> > On Fri, Feb 23, 2018 at 10:59:35AM -0500, Sean Paul wrote:
> > > On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
> > > > From: Brian Starkey 
> > > > 
> > > > Writeback connectors represent writeback engines which can write the
> > > > CRTC output to a memory framebuffer. Add a writeback connector type and
> > > > related support functions.
> > > > 
> > > > Drivers should initialize a writeback connector with
> > > > drm_writeback_connector_init() which takes care of setting up all the
> > > > writeback-specific details on top of the normal functionality of
> > > > drm_connector_init().
> > > > 
> > > > Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> > > > output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose 
> > > > the
> > > > supported writeback formats to userspace.
> > > > 
> > > > When a framebuffer is attached to a writeback connector with the
> > > > WRITEBACK_FB_ID property, it is used only once (for the commit in which
> > > > it was included), and userspace can never read back the value of
> > > > WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
> > > > attached to a CRTC.
> > > > 
> > > > Changes since v1:
> > > >  - Added drm_writeback.c + documentation
> > > >  - Added helper to initialize writeback connector in one go
> > > >  - Added core checks
> > > >  - Squashed into a single commit
> > > >  - Dropped the client cap
> > > >  - Writeback framebuffers are no longer persistent
> > > > 
> > > > Changes since v2:
> > > >  Daniel Vetter:
> > > >  - Subclass drm_connector to drm_writeback_connector
> > > >  - Relax check to allow CRTC to be set without an FB
> > > >  - Add some writeback_ prefixes
> > > >  - Drop PIXEL_FORMATS_SIZE property, as it was unnecessary
> > > >  Gustavo Padovan:
> > > >  - Add drm_writeback_job to handle writeback signalling centrally
> > > > 
> > > > Changes since v3:
> > > >  - Rebased
> > > >  - Rename PIXEL_FORMATS -> WRITEBACK_PIXEL_FORMATS
> > > > 
> > > > Changes since v4:
> > > >  - Added atomic_commit() vfunc to connector helper funcs, so that
> > > >writeback jobs are committed from atomic helpers
> > > > 
> > > > Signed-off-by: Brian Starkey 
> > > > [rebased and fixed conflicts]
> > > > Signed-off-by: Mihail Atanassov 
> > > > Signed-off-by: Liviu Dudau 
> > > > [rebased and added atomic_commit() vfunc for writeback jobs]
> > > > Signed-off-by: Rob Clark 
> > > > ---
> > > >  Documentation/gpu/drm-kms.rst|   9 ++
> > > >  drivers/gpu/drm/Makefile |   2 +-
> > > >  drivers/gpu/drm/drm_atomic.c | 130 
> > > >  drivers/gpu/drm/drm_atomic_helper.c  |  30 
> > > >  drivers/gpu/drm/drm_connector.c  |   4 +-
> > > >  drivers/gpu/drm/drm_writeback.c  | 257 
> > > > +++
> > > >  include/drm/drm_atomic.h |   3 +
> > > >  include/drm/drm_connector.h  |  13 ++
> > > >  include/drm/drm_mode_config.h|  14 ++
> > > >  include/drm/drm_modeset_helper_vtables.h |  11 ++
> > > >  include/drm/drm_writeback.h  |  89 +++
> > > >  include/uapi/drm/drm_mode.h  |   1 +
> > > >  12 files changed, 561 insertions(+), 2 deletions(-)
> > > >  create mode 100644 drivers/gpu/drm/drm_writeback.c
> > > >  create mode 100644 include/drm/drm_writeback.h
> > > > 
> > > > diff --git a/Documentation/gpu/drm-kms.rst 
> > > > b/Documentation/gpu/drm-kms.rst
> > > > index 2dcf5b42015d..e7590dd2f71e 100644
> > > > --- a/Documentation/gpu/drm-kms.rst
> > > > +++ b/Documentation/gpu/drm-kms.rst
> > > > @@ -370,6 +370,15 @@ Connector Functions Reference
> > > >  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > > :export:
> > > >  
> > > > +Writeback Connectors
> > > > +
> > > > +
> > > > +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> > > > +  :doc: overview
> > > > +
> > > > +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> > > > +  :export:
> > > > +
> > > >  Encoder Abstraction
> > > >  ===
> > > >  
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 50093ff4479b..3d708959b224 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -18,7 +18,7 @@ drm-y   :=drm_auth.o drm_bufs.o 
> > > > drm_cache.o \
> > > > drm_encoder.o drm_mode_object.o drm_property.o \
> > > > drm_plane.o drm_color_mgmt.o drm_print.o \
> > > > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > > > -   drm_syncobj.o drm_lease.o
> > > > +   drm_syncobj.o drm_lease.o drm_writeback.o
> > > >  
> > > >  drm-$(CONFIG_DRM_LIB_RANDOM) += 

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Liviu Dudau]

On Fri, Feb 23, 2018 at 11:43:29AM -0500, Sean Paul wrote:
> On Fri, Feb 23, 2018 at 11:25:11AM -0500, Rob Clark wrote:
> > On Fri, Feb 23, 2018 at 10:59 AM, Sean Paul  wrote:
> > >
> > > Have we considered hiding writeback behind a client cap instead?
> > 
> > It is kinda *almost* unneeded, since the connector reports itself as
> > disconnected.
> > 
> > I'm not sure what the reason was to drop the cap, but I think it would
> > be better to have a cap so WB connectors don't show up in, for ex,
> > xrandr
> 
> Yeah, the disconnected hack is kind of gross, IMO. I hate to introduce churn 
> in
> the patch series given that it was initially introduced with the client cap.

Haha, that's the reverse of Daniel's position:

https://lists.freedesktop.org/archives/dri-devel/2016-October/120519.html

> 
> There are also cases where we might want to make writeback unavailable, such 
> as
> when content protection is enabled. In those cases, it's conceivable that we
> might want to use disconnected as a signal to u/s. I suppose we could also 
> just
> fail the check, so most of this is just academic.

Not sure what other hardware out there does, but on Mali DP's case you
would be outputing the protected content by putting the display
processor in secure mode, which automatically disables writeback for us.
Or to put in another way, you don't need a writeback framebuffer if you
are in non-secure mode as you can get access to the framebuffer used for
the plane anyway.

Best regards,
Liviu

> 
> Sean
> 
> 
> > 
> > BR,
> > -R
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Sean Paul]

On Fri, Feb 23, 2018 at 11:25:11AM -0500, Rob Clark wrote:
> On Fri, Feb 23, 2018 at 10:59 AM, Sean Paul  wrote:
> >
> > Have we considered hiding writeback behind a client cap instead?
> 
> It is kinda *almost* unneeded, since the connector reports itself as
> disconnected.
> 
> I'm not sure what the reason was to drop the cap, but I think it would
> be better to have a cap so WB connectors don't show up in, for ex,
> xrandr

Yeah, the disconnected hack is kind of gross, IMO. I hate to introduce churn in
the patch series given that it was initially introduced with the client cap.

There are also cases where we might want to make writeback unavailable, such as
when content protection is enabled. In those cases, it's conceivable that we
might want to use disconnected as a signal to u/s. I suppose we could also just
fail the check, so most of this is just academic.

Sean


> 
> BR,
> -R

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 4/4] drm/msm/mdp5: writeback support

[Sean Paul]

On Fri, Feb 23, 2018 at 08:17:54AM -0500, Rob Clark wrote:
> In a way, based on the original writeback patch from Jilai Wang, but a
> lot has shifted around since then.
> 
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/Makefile  |   1 +
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  23 +-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  |  38 +++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h  |   7 +-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c   | 367 
> ++
>  drivers/gpu/drm/msm/dsi/dsi_host.c|   4 +-
>  6 files changed, 431 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index cd40c050b2d7..c9f50adef2db 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -45,6 +45,7 @@ msm-y := \
>   disp/mdp5/mdp5_mixer.o \
>   disp/mdp5/mdp5_plane.o \
>   disp/mdp5/mdp5_smp.o \
> + disp/mdp5/mdp5_wb.o \
>   msm_atomic.o \
>   msm_debugfs.o \
>   msm_drv.o \
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> index 9893e43ba6c5..b00ca88b741d 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> @@ -484,7 +484,11 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc 
> *crtc,
>   }
>  
>   /* Restore vblank irq handling after power is enabled */
> - drm_crtc_vblank_on(crtc);
> +// TODO we can't ->get_scanout_pos() for wb (since virtual intf)..
> +// perhaps drm core should be clever enough not to 
> drm_reset_vblank_timestamp()
> +// for virtual encoders / writeback?
> + if (mdp5_cstate->pipeline.intf->type != INTF_WB)
> + drm_crtc_vblank_on(crtc);
>  
>   mdp5_crtc_mode_set_nofb(crtc);
>  
> @@ -518,7 +522,11 @@ int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
>   u32 caps;
>   int ret;
>  
> - caps = MDP_LM_CAP_DISPLAY;
> + if (pipeline->intf->type == INTF_WB)
> + caps = MDP_LM_CAP_WB;
> + else
> + caps = MDP_LM_CAP_DISPLAY;
> +
>   if (need_right_mixer)
>   caps |= MDP_LM_CAP_PAIR;
>  
> @@ -545,6 +553,7 @@ int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
>   mdp5_cstate->err_irqmask = intf2err(intf->num);
>   mdp5_cstate->vblank_irqmask = intf2vblank(pipeline->mixer, intf);
>  
> +// XXX should we be treating WB as cmd_mode??
>   if ((intf->type == INTF_DSI) &&
>   (intf->mode == MDP5_INTF_DSI_MODE_COMMAND)) {
>   mdp5_cstate->pp_done_irqmask = lm2ppdone(pipeline->mixer);
> @@ -639,8 +648,12 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
>   }
>  
>   /* bail out early if there aren't any planes */
> - if (!cnt)
> - return 0;
> + if (!cnt) {
> + if (!state->active)
> + return 0;
> + dev_err(dev->dev, "%s has no planes!\n", crtc->name);
> + return -EINVAL;
> + }

This seems unrelated?

>  
>   hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
>  
> @@ -1160,7 +1173,7 @@ void mdp5_crtc_wait_for_commit_done(struct drm_crtc 
> *crtc)
>  
>   if (mdp5_cstate->cmd_mode)
>   mdp5_crtc_wait_for_pp_done(crtc);
> - else
> + else if (mdp5_cstate->pipeline.intf->type != INTF_WB)
>   mdp5_crtc_wait_for_flush_done(crtc);
>  }
>  
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 1f44d8f15ce1..239010905637 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -427,7 +427,8 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>* the MDP5 interfaces) than the number of layer mixers present in HW,
>* but let's be safe here anyway
>*/
> - num_crtcs = min(priv->num_encoders, mdp5_kms->num_hwmixers);
> + num_crtcs = min(priv->num_encoders + hw_cfg->wb.count,
> + mdp5_kms->num_hwmixers);
>  
>   /*
>* Construct planes equaling the number of hw pipes, and CRTCs for the
> @@ -482,6 +483,33 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>   encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
>   }
>  
> + /*
> +  * Lastly, construct writeback connectors.
> +  */
> + for (i = 0; i < hw_cfg->wb.count; i++) {
> + struct drm_writeback_connector *wb_conn;
> + struct mdp5_ctl *ctl;
> +
> + ctl = mdp5_ctlm_request(mdp5_kms->ctlm, -1);
> + if (!ctl) {
> + dev_err(dev->dev,
> + "failed to allocate ctl for writeback %d\n", i);
> + continue;
> + }
> +
> + wb_conn = mdp5_wb_connector_init(dev, ctl,

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Rob Clark]

On Fri, Feb 23, 2018 at 10:59 AM, Sean Paul  wrote:
>
> Have we considered hiding writeback behind a client cap instead?

It is kinda *almost* unneeded, since the connector reports itself as
disconnected.

I'm not sure what the reason was to drop the cap, but I think it would
be better to have a cap so WB connectors don't show up in, for ex,
xrandr

BR,
-R
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Liviu Dudau]

On Fri, Feb 23, 2018 at 10:59:35AM -0500, Sean Paul wrote:
> On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
> > From: Brian Starkey 
> > 
> > Writeback connectors represent writeback engines which can write the
> > CRTC output to a memory framebuffer. Add a writeback connector type and
> > related support functions.
> > 
> > Drivers should initialize a writeback connector with
> > drm_writeback_connector_init() which takes care of setting up all the
> > writeback-specific details on top of the normal functionality of
> > drm_connector_init().
> > 
> > Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> > output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose the
> > supported writeback formats to userspace.
> > 
> > When a framebuffer is attached to a writeback connector with the
> > WRITEBACK_FB_ID property, it is used only once (for the commit in which
> > it was included), and userspace can never read back the value of
> > WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
> > attached to a CRTC.
> > 
> > Changes since v1:
> >  - Added drm_writeback.c + documentation
> >  - Added helper to initialize writeback connector in one go
> >  - Added core checks
> >  - Squashed into a single commit
> >  - Dropped the client cap
> >  - Writeback framebuffers are no longer persistent
> > 
> > Changes since v2:
> >  Daniel Vetter:
> >  - Subclass drm_connector to drm_writeback_connector
> >  - Relax check to allow CRTC to be set without an FB
> >  - Add some writeback_ prefixes
> >  - Drop PIXEL_FORMATS_SIZE property, as it was unnecessary
> >  Gustavo Padovan:
> >  - Add drm_writeback_job to handle writeback signalling centrally
> > 
> > Changes since v3:
> >  - Rebased
> >  - Rename PIXEL_FORMATS -> WRITEBACK_PIXEL_FORMATS
> > 
> > Changes since v4:
> >  - Added atomic_commit() vfunc to connector helper funcs, so that
> >writeback jobs are committed from atomic helpers
> > 
> > Signed-off-by: Brian Starkey 
> > [rebased and fixed conflicts]
> > Signed-off-by: Mihail Atanassov 
> > Signed-off-by: Liviu Dudau 
> > [rebased and added atomic_commit() vfunc for writeback jobs]
> > Signed-off-by: Rob Clark 
> > ---
> >  Documentation/gpu/drm-kms.rst|   9 ++
> >  drivers/gpu/drm/Makefile |   2 +-
> >  drivers/gpu/drm/drm_atomic.c | 130 
> >  drivers/gpu/drm/drm_atomic_helper.c  |  30 
> >  drivers/gpu/drm/drm_connector.c  |   4 +-
> >  drivers/gpu/drm/drm_writeback.c  | 257 
> > +++
> >  include/drm/drm_atomic.h |   3 +
> >  include/drm/drm_connector.h  |  13 ++
> >  include/drm/drm_mode_config.h|  14 ++
> >  include/drm/drm_modeset_helper_vtables.h |  11 ++
> >  include/drm/drm_writeback.h  |  89 +++
> >  include/uapi/drm/drm_mode.h  |   1 +
> >  12 files changed, 561 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/gpu/drm/drm_writeback.c
> >  create mode 100644 include/drm/drm_writeback.h
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 2dcf5b42015d..e7590dd2f71e 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -370,6 +370,15 @@ Connector Functions Reference
> >  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > :export:
> >  
> > +Writeback Connectors
> > +
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> > +  :doc: overview
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> > +  :export:
> > +
> >  Encoder Abstraction
> >  ===
> >  
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 50093ff4479b..3d708959b224 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -18,7 +18,7 @@ drm-y   :=drm_auth.o drm_bufs.o drm_cache.o \
> > drm_encoder.o drm_mode_object.o drm_property.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > -   drm_syncobj.o drm_lease.o
> > +   drm_syncobj.o drm_lease.o drm_writeback.o
> >  
> >  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> >  drm-$(CONFIG_DRM_VM) += drm_vm.o
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 46733d534587..019f131fe8be 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -30,6 +30,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -638,6 +639,46 @@ static void drm_atomic_crtc_print_state(struct 
> > drm_printer *p,
> > crtc->funcs->atomic_print_state(p, state);
> >  }
> >  
> > +/**
> > + * 

Re: [Freedreno] [RFC 2/4] drm: writeback: Add out-fences for writeback connectors

[Sean Paul]

On Fri, Feb 23, 2018 at 08:17:52AM -0500, Rob Clark wrote:
> From: Brian Starkey 
> 
> Add the WRITEBACK_OUT_FENCE_PTR property to writeback connectors, to
> enable userspace to get a fence which will signal once the writeback is
> complete. It is not allowed to request an out-fence without a
> framebuffer attached to the connector.
> 
> A timeline is added to drm_writeback_connector for use by the writeback
> out-fences.
> 
> In the case of a commit failure or DRM_MODE_ATOMIC_TEST_ONLY, the fence
> is set to -1.
> 
> Changes from v2:
>  - Rebase onto Gustavo Padovan's v9 explicit sync series
>  - Change out_fence_ptr type to s32 __user *
>  - Set *out_fence_ptr to -1 in drm_atomic_connector_set_property
>  - Store fence in drm_writeback_job
>  Gustavo Padovan:
>  - Move out_fence_ptr out of connector_state
>  - Signal fence from drm_writeback_signal_completion instead of
>in driver directly
> 
> Changes from v3:
>  - Rebase onto 7e9081c5aac7 drm/fence: fix memory overwrite when setting 
> out_fence fd
>(change out_fence_ptr to s32 __user *, for real this time.)
>  - Update documentation around WRITEBACK_OUT_FENCE_PTR
> 
> Signed-off-by: Brian Starkey 
> [rebased and fixed conflicts]
> Signed-off-by: Mihail Atanassov 
> Signed-off-by: Liviu Dudau 
> Signed-off-by: Rob Clark 

Reviewed-by: Sean Paul 

> ---
>  drivers/gpu/drm/drm_atomic.c|  99 
>  drivers/gpu/drm/drm_writeback.c | 109 
> +++-
>  include/drm/drm_atomic.h|   8 +++
>  include/drm/drm_connector.h |   8 +--
>  include/drm/drm_mode_config.h   |   8 +++
>  include/drm/drm_writeback.h |  41 ++-
>  6 files changed, 257 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 019f131fe8be..fc8c4da409ff 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -318,6 +318,35 @@ static s32 __user *get_out_fence_for_crtc(struct 
> drm_atomic_state *state,
>   return fence_ptr;
>  }
>  
> +static int set_out_fence_for_connector(struct drm_atomic_state *state,
> + struct drm_connector *connector,
> + s32 __user *fence_ptr)
> +{
> + unsigned int index = drm_connector_index(connector);
> +
> + if (!fence_ptr)
> + return 0;
> +
> + if (put_user(-1, fence_ptr))
> + return -EFAULT;
> +
> + state->connectors[index].out_fence_ptr = fence_ptr;
> +
> + return 0;
> +}
> +
> +static s32 __user *get_out_fence_for_connector(struct drm_atomic_state 
> *state,
> +struct drm_connector *connector)
> +{
> + unsigned int index = drm_connector_index(connector);
> + s32 __user *fence_ptr;
> +
> + fence_ptr = state->connectors[index].out_fence_ptr;
> + state->connectors[index].out_fence_ptr = NULL;
> +
> + return fence_ptr;
> +}
> +
>  /**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
>   * @state: the CRTC whose incoming state to update
> @@ -676,6 +705,12 @@ static int drm_atomic_connector_check(struct 
> drm_connector *connector,
>   return -EINVAL;
>   }
>  
> + if (writeback_job->out_fence && !writeback_job->fb) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence 
> without framebuffer\n",
> +  connector->base.id, connector->name);
> + return -EINVAL;
> + }
> +
>   return 0;
>  }
>  
> @@ -1277,6 +1312,11 @@ static int drm_atomic_connector_set_property(struct 
> drm_connector *connector,
>   if (fb)
>   drm_framebuffer_unreference(fb);
>   return ret;
> + } else if (property == config->writeback_out_fence_ptr_property) {
> + s32 __user *fence_ptr = u64_to_user_ptr(val);
> +
> + return set_out_fence_for_connector(state->state, connector,
> +fence_ptr);
>   } else if (connector->funcs->atomic_set_property) {
>   return connector->funcs->atomic_set_property(connector,
>   state, property, val);
> @@ -1361,6 +1401,8 @@ drm_atomic_connector_get_property(struct drm_connector 
> *connector,
>   } else if (property == config->writeback_fb_id_property) {
>   /* Writeback framebuffer is one-shot, write and forget */
>   *val = 0;
> + } else if (property == config->writeback_out_fence_ptr_property) {
> + *val = 0;
>   } else if (connector->funcs->atomic_get_property) {
>   return connector->funcs->atomic_get_property(connector,
>   state, property, val);
> @@ -2221,7 +2263,7 @@ static int setup_out_fence(struct 

Re: [Freedreno] [PATCH v7 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

[Jordan Crouse]

On Fri, Feb 23, 2018 at 04:06:39PM +0530, Vivek Gautam wrote:
> On Fri, Feb 23, 2018 at 5:22 AM, Jordan Crouse  wrote:
> > On Wed, Feb 07, 2018 at 04:01:19PM +0530, Vivek Gautam wrote:
> >> From: Sricharan R 
> >>
> >> The smmu device probe/remove and add/remove master device callbacks
> >> gets called when the smmu is not linked to its master, that is without
> >> the context of the master device. So calling runtime apis in those places
> >> separately.
> >>
> >> Signed-off-by: Sricharan R 
> >> [vivek: Cleanup pm runtime calls]
> >> Signed-off-by: Vivek Gautam 
> >> ---
> >>  drivers/iommu/arm-smmu.c | 42 ++
> >>  1 file changed, 38 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> >> index 9e2f917e16c2..c024f69c1682 100644
> >> --- a/drivers/iommu/arm-smmu.c
> >> +++ b/drivers/iommu/arm-smmu.c
> >> @@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct 
> >> iommu_domain *domain)
> >>   struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >>   struct arm_smmu_device *smmu = smmu_domain->smmu;
> >>   struct arm_smmu_cfg *cfg = _domain->cfg;
> >> - int irq;
> >> + int ret, irq;
> >>
> >>   if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> >>   return;
> >>
> >> + ret = pm_runtime_get_sync(smmu->dev);
> >> + if (ret)
> >> + return;
> >> +
> >>   /*
> >>* Disable the context bank and free the page tables before freeing
> >>* it.
> >> @@ -932,6 +936,8 @@ static void arm_smmu_destroy_domain_context(struct 
> >> iommu_domain *domain)
> >>
> >>   free_io_pgtable_ops(smmu_domain->pgtbl_ops);
> >>   __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
> >> +
> >> + pm_runtime_put_sync(smmu->dev);
> >>  }
> >>
> >>  static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> >> @@ -1407,14 +1413,22 @@ static int arm_smmu_add_device(struct device *dev)
> >>   while (i--)
> >>   cfg->smendx[i] = INVALID_SMENDX;
> >>
> >> - ret = arm_smmu_master_alloc_smes(dev);
> >> + ret = pm_runtime_get_sync(smmu->dev);
> >>   if (ret)
> >>   goto out_cfg_free;
> >
> > Hey Vivek, I just hit a problem with this on sdm845. It turns out that
> > pm_runtime_get_sync() returns a positive 1 if the device is already active.
> >
> > I hit this in the GPU code. The a6xx has two platform devices that each use 
> > a
> > different sid on the iommu. The GPU is probed normally from a platform 
> > driver
> > and it in turn initializes the GMU device by way of a phandle.
> >
> > Because the GMU isn't probed with a platform driver we need to call
> > of_dma_configure() on the device to set up the IOMMU for the device which 
> > ends
> > up calling through this path and we discover that the smmu->dev is already
> > powered (pm_runtime_get_sync returns 1).
> >
> > I'm not immediately sure if this is a bug on sdm845 or not because a cursory
> > inspection says that the SMMU device shouldn't be powered at this time but 
> > there
> > might be a connection that I'm not seeing. Obviously if the SMMU was left
> > powered thats a bad thing. But putting that aside it is obvious that this
> > code should be accommodating of the possibility that the device is already
> > powered, and so this should be
> >
> > if (ret < 0)
> > goto out_cfg_free;
> 
> Right, as Tomasz also pointed, we should surely check the negative value of
> pm_runtime_get_sync().

Sorry, I didn't notice that Tomasz had pointed it out as well. I wanted to
quickly get it on the mailing list so you could catch it in your time zone.

> From your description, it may be that the GPU has turned on the smmu, and
> then once if goes and probes the GMU, the GMU device also wants to turn-on
> the same smmu device. But that's already active. So pm_runtime_get_sync()
> returns 1.
> Am i making sense?

My concern is that this is happening during the probe and we shouldn't be
energizing the GPU at this point. But it is entirely possible that the
bus is on for other reasons. I'll do a bit of digging today and see exactly
which device is at fault.


Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH] drm/mipi: Remove Qualcomm-specific dsi packet header format

[Sean Paul]

On Thu, Feb 22, 2018 at 9:28 PM, Abhinav Kumar  wrote:
> Looks good. Can you point us to the fix done in the dsi-staging driver.
>

All of the downstream changes are in the mtp-testing branch of the
dpu-staging tree. The on-list patches are in the for-next-staging, and
the patches which have been reviewed on list will go to the for-next
branch.

Sean


> Thanks
>
> Abhinav
>
> -Original Message-
> From: Rob Clark [mailto:robdcl...@gmail.com]
> Sent: Thursday, February 22, 2018 11:49 AM
> To: Sean Paul 
> Cc: freedreno ; linux-arm-msm 
> ; Kristian H. Kristensen 
> ; Jeykumar Sankaran ; Abhinav 
> Kumar 
> Subject: Re: [DPU PATCH] drm/mipi: Remove Qualcomm-specific dsi packet header 
> format
>
> On Thu, Feb 22, 2018 at 12:37 PM, Sean Paul  wrote:
>> msm/dsi already formats the packet header correctly, so this breaks
>> every driver except for the downstream dsi-staging driver (which I've
>> submitted a patch for).
>>
>> Signed-off-by: Sean Paul 
>
> Reviewed-by: Rob Clark 
>
>> ---
>>  drivers/gpu/drm/drm_mipi_dsi.c | 10 +-
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c
>> b/drivers/gpu/drm/drm_mipi_dsi.c index 688c8a82ba37..4b47226b90d4
>> 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -454,7 +454,7 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet 
>> *packet,
>> return -EINVAL;
>>
>> memset(packet, 0, sizeof(*packet));
>> -   packet->header[2] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
>> +   packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type &
>> + 0x3f);
>>
>> /* TODO: compute ECC if hardware support is not available */
>>
>> @@ -466,16 +466,16 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet 
>> *packet,
>>  * and 2.
>>  */
>> if (mipi_dsi_packet_format_is_long(msg->type)) {
>> -   packet->header[0] = (msg->tx_len >> 0) & 0xff;
>> -   packet->header[1] = (msg->tx_len >> 8) & 0xff;
>> +   packet->header[1] = (msg->tx_len >> 0) & 0xff;
>> +   packet->header[2] = (msg->tx_len >> 8) & 0xff;
>>
>> packet->payload_length = msg->tx_len;
>> packet->payload = msg->tx_buf;
>> } else {
>> const u8 *tx = msg->tx_buf;
>>
>> -   packet->header[0] = (msg->tx_len > 0) ? tx[0] : 0;
>> -   packet->header[1] = (msg->tx_len > 1) ? tx[1] : 0;
>> +   packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
>> +   packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
>> }
>>
>> packet->size = sizeof(packet->header) +
>> packet->payload_length;
>> --
>> 2.16.1.291.g4437f3f132-goog
>>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Liviu Dudau]

On Fri, Feb 23, 2018 at 09:24:10AM -0500, Rob Clark wrote:
> On Fri, Feb 23, 2018 at 9:00 AM, Liviu Dudau  wrote:
> > Hi Rob,
> >
> > On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
> >> From: Brian Starkey 
> >>
> >> Writeback connectors represent writeback engines which can write the
> >> CRTC output to a memory framebuffer. Add a writeback connector type and
> >> related support functions.
> >>
> >> Drivers should initialize a writeback connector with
> >> drm_writeback_connector_init() which takes care of setting up all the
> >> writeback-specific details on top of the normal functionality of
> >> drm_connector_init().
> >>
> >> Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> >> output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose the
> >> supported writeback formats to userspace.
> >>
> >> When a framebuffer is attached to a writeback connector with the
> >> WRITEBACK_FB_ID property, it is used only once (for the commit in which
> >> it was included), and userspace can never read back the value of
> >> WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
> >> attached to a CRTC.
> >
> > [snip]
> >
> >> +static bool create_writeback_properties(struct drm_device *dev)
> >> +{
> >> + struct drm_property *prop;
> >> +
> >> + if (!dev->mode_config.writeback_fb_id_property) {
> >> + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> >> +   "WRITEBACK_FB_ID",
> >> +   DRM_MODE_OBJECT_FB);
> >> + if (!prop)
> >> + return false;
> >> + dev->mode_config.writeback_fb_id_property = prop;
> >> + }
> >> +
> >> + if (!dev->mode_config.writeback_pixel_formats_property) {
> >> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | 
> >> DRM_MODE_PROP_IMMUTABLE,
> >> +"WRITEBACK_PIXEL_FORMATS", 0);
> >> + if (!prop)
> >> + return false;
> >> + dev->mode_config.writeback_pixel_formats_property = prop;
> >> + }
> >> +
> >> + return true;
> >> +}
> >
> > based on a buildbot warning and the fact that the next patch starts
> > returning -ENOMEM inside the above function, I have this variant in my
> > internal tree that I was preparing to send out once I've got my i-g-t
> > tests in better shape:
> >
> > +static int create_writeback_properties(struct drm_device *dev)
> > +{
> > +   struct drm_property *prop;
> > +
> > +   if (!dev->mode_config.writeback_fb_id_property) {
> > +   prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> > + "WRITEBACK_FB_ID",
> > + DRM_MODE_OBJECT_FB);
> > +   if (!prop)
> > +   return -ENOMEM;
> > +   dev->mode_config.writeback_fb_id_property = prop;
> > +   }
> > +
> > +   if (!dev->mode_config.writeback_pixel_formats_property) {
> > +   prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | 
> > DRM_MODE_PROP_IMMUTABLE,
> > +  "WRITEBACK_PIXEL_FORMATS", 0);
> > +   if (!prop)
> > +   return -ENOMEM;
> > +   dev->mode_config.writeback_pixel_formats_property = prop;
> > +   }
> > +
> > +   return 0;
> > +}
> >
> >
> > Feel free to use this version in the next update if you're going to send
> > one, otherwise I guess we could be patching it later.
> >
> 
> hmm, I meant to keep my addition of funcs->atomic_commit() as a
> separate fixup patch so it would be easier for you to fold back into
> your patchset, but accidentally squashed it at some point and was too
> lazy to split it out again.  Sorry about that.

I'm not too fussed about who pushes Brian's framework patches into
drm-next so I don't mind at all you merging your addition. Just wanted
to make sure we're on the same page (didn't know you're going to send
your series this week).

I also feel I need to appologise for my delay in getting the i-g-t
patches into shape, I've been splitting myself between various other
tasks, not all kernel related.

Best regards,
Liviu

> 
> BR,
> -R
> 
> > Best regards,
> > Liviu
> >
> >
> >> +
> >> +static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
> >> + .destroy = drm_encoder_cleanup,
> >> +};
> >> +
> >> +/**
> >> + * drm_writeback_connector_init - Initialize a writeback connector and 
> >> its properties
> >> + * @dev: DRM device
> >> + * @wb_connector: Writeback connector to initialize
> >> + * @con_funcs: Connector funcs vtable
> >> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the 
> >> internal encoder
> >> + * @formats: Array of supported pixel formats for the writeback engine
> >> + * @n_formats: 

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

[Rob Clark]

On Fri, Feb 23, 2018 at 9:00 AM, Liviu Dudau  wrote:
> Hi Rob,
>
> On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
>> From: Brian Starkey 
>>
>> Writeback connectors represent writeback engines which can write the
>> CRTC output to a memory framebuffer. Add a writeback connector type and
>> related support functions.
>>
>> Drivers should initialize a writeback connector with
>> drm_writeback_connector_init() which takes care of setting up all the
>> writeback-specific details on top of the normal functionality of
>> drm_connector_init().
>>
>> Writeback connectors have a WRITEBACK_FB_ID property, used to set the
>> output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose the
>> supported writeback formats to userspace.
>>
>> When a framebuffer is attached to a writeback connector with the
>> WRITEBACK_FB_ID property, it is used only once (for the commit in which
>> it was included), and userspace can never read back the value of
>> WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
>> attached to a CRTC.
>
> [snip]
>
>> +static bool create_writeback_properties(struct drm_device *dev)
>> +{
>> + struct drm_property *prop;
>> +
>> + if (!dev->mode_config.writeback_fb_id_property) {
>> + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
>> +   "WRITEBACK_FB_ID",
>> +   DRM_MODE_OBJECT_FB);
>> + if (!prop)
>> + return false;
>> + dev->mode_config.writeback_fb_id_property = prop;
>> + }
>> +
>> + if (!dev->mode_config.writeback_pixel_formats_property) {
>> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | 
>> DRM_MODE_PROP_IMMUTABLE,
>> +"WRITEBACK_PIXEL_FORMATS", 0);
>> + if (!prop)
>> + return false;
>> + dev->mode_config.writeback_pixel_formats_property = prop;
>> + }
>> +
>> + return true;
>> +}
>
> based on a buildbot warning and the fact that the next patch starts
> returning -ENOMEM inside the above function, I have this variant in my
> internal tree that I was preparing to send out once I've got my i-g-t
> tests in better shape:
>
> +static int create_writeback_properties(struct drm_device *dev)
> +{
> +   struct drm_property *prop;
> +
> +   if (!dev->mode_config.writeback_fb_id_property) {
> +   prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> + "WRITEBACK_FB_ID",
> + DRM_MODE_OBJECT_FB);
> +   if (!prop)
> +   return -ENOMEM;
> +   dev->mode_config.writeback_fb_id_property = prop;
> +   }
> +
> +   if (!dev->mode_config.writeback_pixel_formats_property) {
> +   prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | 
> DRM_MODE_PROP_IMMUTABLE,
> +  "WRITEBACK_PIXEL_FORMATS", 0);
> +   if (!prop)
> +   return -ENOMEM;
> +   dev->mode_config.writeback_pixel_formats_property = prop;
> +   }
> +
> +   return 0;
> +}
>
>
> Feel free to use this version in the next update if you're going to send
> one, otherwise I guess we could be patching it later.
>

hmm, I meant to keep my addition of funcs->atomic_commit() as a
separate fixup patch so it would be easier for you to fold back into
your patchset, but accidentally squashed it at some point and was too
lazy to split it out again.  Sorry about that.

BR,
-R

> Best regards,
> Liviu
>
>
>> +
>> +static const struct drm_encoder_funcs drm_writeback_encoder_funcs = {
>> + .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +/**
>> + * drm_writeback_connector_init - Initialize a writeback connector and its 
>> properties
>> + * @dev: DRM device
>> + * @wb_connector: Writeback connector to initialize
>> + * @con_funcs: Connector funcs vtable
>> + * @enc_helper_funcs: Encoder helper funcs vtable to be used by the 
>> internal encoder
>> + * @formats: Array of supported pixel formats for the writeback engine
>> + * @n_formats: Length of the formats array
>> + *
>> + * This function creates the writeback-connector-specific properties if they
>> + * have not been already created, initializes the connector as
>> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
>> + * values. It will also create an internal encoder associated with the
>> + * drm_writeback_connector and set it to use the @enc_helper_funcs vtable 
>> for
>> + * the encoder helper.
>> + *
>> + * Drivers should always use this function instead of drm_connector_init() 
>> to
>> + * set up writeback connectors.
>> + *
>> + * Returns: 0 on success, or a negative error code
>> + */
>> +int drm_writeback_connector_init(struct drm_device *dev,
>> 

[Freedreno] [RFC 4/4] drm/msm/mdp5: writeback support

[Rob Clark]

In a way, based on the original writeback patch from Jilai Wang, but a
lot has shifted around since then.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/Makefile  |   1 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  23 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  |  38 +++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h  |   7 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c   | 367 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c|   4 +-
 6 files changed, 431 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index cd40c050b2d7..c9f50adef2db 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -45,6 +45,7 @@ msm-y := \
disp/mdp5/mdp5_mixer.o \
disp/mdp5/mdp5_plane.o \
disp/mdp5/mdp5_smp.o \
+   disp/mdp5/mdp5_wb.o \
msm_atomic.o \
msm_debugfs.o \
msm_drv.o \
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
index 9893e43ba6c5..b00ca88b741d 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
@@ -484,7 +484,11 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
}
 
/* Restore vblank irq handling after power is enabled */
-   drm_crtc_vblank_on(crtc);
+// TODO we can't ->get_scanout_pos() for wb (since virtual intf)..
+// perhaps drm core should be clever enough not to drm_reset_vblank_timestamp()
+// for virtual encoders / writeback?
+   if (mdp5_cstate->pipeline.intf->type != INTF_WB)
+   drm_crtc_vblank_on(crtc);
 
mdp5_crtc_mode_set_nofb(crtc);
 
@@ -518,7 +522,11 @@ int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
u32 caps;
int ret;
 
-   caps = MDP_LM_CAP_DISPLAY;
+   if (pipeline->intf->type == INTF_WB)
+   caps = MDP_LM_CAP_WB;
+   else
+   caps = MDP_LM_CAP_DISPLAY;
+
if (need_right_mixer)
caps |= MDP_LM_CAP_PAIR;
 
@@ -545,6 +553,7 @@ int mdp5_crtc_setup_pipeline(struct drm_crtc *crtc,
mdp5_cstate->err_irqmask = intf2err(intf->num);
mdp5_cstate->vblank_irqmask = intf2vblank(pipeline->mixer, intf);
 
+// XXX should we be treating WB as cmd_mode??
if ((intf->type == INTF_DSI) &&
(intf->mode == MDP5_INTF_DSI_MODE_COMMAND)) {
mdp5_cstate->pp_done_irqmask = lm2ppdone(pipeline->mixer);
@@ -639,8 +648,12 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
}
 
/* bail out early if there aren't any planes */
-   if (!cnt)
-   return 0;
+   if (!cnt) {
+   if (!state->active)
+   return 0;
+   dev_err(dev->dev, "%s has no planes!\n", crtc->name);
+   return -EINVAL;
+   }
 
hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
 
@@ -1160,7 +1173,7 @@ void mdp5_crtc_wait_for_commit_done(struct drm_crtc *crtc)
 
if (mdp5_cstate->cmd_mode)
mdp5_crtc_wait_for_pp_done(crtc);
-   else
+   else if (mdp5_cstate->pipeline.intf->type != INTF_WB)
mdp5_crtc_wait_for_flush_done(crtc);
 }
 
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 1f44d8f15ce1..239010905637 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -427,7 +427,8 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
 * the MDP5 interfaces) than the number of layer mixers present in HW,
 * but let's be safe here anyway
 */
-   num_crtcs = min(priv->num_encoders, mdp5_kms->num_hwmixers);
+   num_crtcs = min(priv->num_encoders + hw_cfg->wb.count,
+   mdp5_kms->num_hwmixers);
 
/*
 * Construct planes equaling the number of hw pipes, and CRTCs for the
@@ -482,6 +483,33 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
}
 
+   /*
+* Lastly, construct writeback connectors.
+*/
+   for (i = 0; i < hw_cfg->wb.count; i++) {
+   struct drm_writeback_connector *wb_conn;
+   struct mdp5_ctl *ctl;
+
+   ctl = mdp5_ctlm_request(mdp5_kms->ctlm, -1);
+   if (!ctl) {
+   dev_err(dev->dev,
+   "failed to allocate ctl for writeback %d\n", i);
+   continue;
+   }
+
+   wb_conn = mdp5_wb_connector_init(dev, ctl,
+   hw_cfg->wb.instances[i].id);
+   if (IS_ERR(wb_conn)) {
+   ret = PTR_ERR(wb_conn);
+   dev_err(dev->dev,
+   

[Freedreno] [RFC 0/4] drm/msm/mdp5: writeback connector support

[Rob Clark]

(Sorry, meant to send this earlier, but got distracted on other things)

The first two patches are Brian Starkey's earlier writeback-connector
patches, with very minor rebasing to drm-next/v4.16-rc1, plus one small
addition to add atomic_commit() vfunc to the connector helpers, so that
writeback jobs could be committed directly from the atomic helpers.

I've tested this on dragonboard 410c (apq8016), with a hacked up
kmscube[1] to use the output of writeback as src texture for the next
frame.  (Yes, I know this isn't a "real" userspace, but I needed some
test code.)

So far, I've only managed to get WB2, which is connected to LM3, to
work.  I think there should be a WB0 attached to LM0 (which is also
attached to the DSI INTF).  It isn't clear to me if this can be used at
the same time as DSI video mode output.  If that is possible, then we
could support attaching both DSI and WB encoder+connector to the same
CRTC and re-use all the same planes being used for scanout to flatten
the composited image to a single buffer.  This would be an obvious and
simple use-case for weston and drm-hwc, to reduce power/bandwidth when
the screen is not updating by flattening all layers into a single layer
for the next frame.  But my attempts at making this work just made the
hardware grumpy.

These patches apply on top of msm-next[2] (which contains a couple other
required fixes, in particular improved CTL START signal handling).  The
patches can also be found on the msm-next-writeback[3] branch.

[1] https://github.com/robclark/kmscube/commits/writeback
[2] https://cgit.freedesktop.org/~robclark/linux/log/?h=msm-next
[3] https://cgit.freedesktop.org/~robclark/linux/log/?h=msm-next-writeback

Brian Starkey (2):
  drm: Add writeback connector type
  drm: writeback: Add out-fences for writeback connectors

Rob Clark (2):
  drm/msm/mdp5: add config for writeback pipes
  drm/msm/mdp5: writeback support

 Documentation/gpu/drm-kms.rst |   9 +
 drivers/gpu/drm/Makefile  |   2 +-
 drivers/gpu/drm/drm_atomic.c  | 229 ++-
 drivers/gpu/drm/drm_atomic_helper.c   |  30 +++
 drivers/gpu/drm/drm_connector.c   |   4 +-
 drivers/gpu/drm/drm_writeback.c   | 362 +
 drivers/gpu/drm/msm/Makefile  |   1 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5.xml.h  |   2 -
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c  |  17 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.h  |  11 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  23 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  |  39 +++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h  |   7 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c   | 367 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c|   4 +-
 include/drm/drm_atomic.h  |  11 +
 include/drm/drm_connector.h   |  13 ++
 include/drm/drm_mode_config.h |  22 ++
 include/drm/drm_modeset_helper_vtables.h  |  11 +
 include/drm/drm_writeback.h   | 128 +++
 include/uapi/drm/drm_mode.h   |   1 +
 21 files changed, 1265 insertions(+), 28 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_writeback.c
 create mode 100644 drivers/gpu/drm/msm/disp/mdp5/mdp5_wb.c
 create mode 100644 include/drm/drm_writeback.h

-- 
2.14.3

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [DPU PATCH] drm/mipi: Remove Qualcomm-specific dsi packet header format

[Abhinav Kumar]

Looks good. Can you point us to the fix done in the dsi-staging driver.

Thanks

Abhinav

-Original Message-
From: Rob Clark [mailto:robdcl...@gmail.com] 
Sent: Thursday, February 22, 2018 11:49 AM
To: Sean Paul 
Cc: freedreno ; linux-arm-msm 
; Kristian H. Kristensen 
; Jeykumar Sankaran ; Abhinav 
Kumar 
Subject: Re: [DPU PATCH] drm/mipi: Remove Qualcomm-specific dsi packet header 
format

On Thu, Feb 22, 2018 at 12:37 PM, Sean Paul  wrote:
> msm/dsi already formats the packet header correctly, so this breaks 
> every driver except for the downstream dsi-staging driver (which I've 
> submitted a patch for).
>
> Signed-off-by: Sean Paul 

Reviewed-by: Rob Clark 

> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c 
> b/drivers/gpu/drm/drm_mipi_dsi.c index 688c8a82ba37..4b47226b90d4 
> 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -454,7 +454,7 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet *packet,
> return -EINVAL;
>
> memset(packet, 0, sizeof(*packet));
> -   packet->header[2] = ((msg->channel & 0x3) << 6) | (msg->type & 0x3f);
> +   packet->header[0] = ((msg->channel & 0x3) << 6) | (msg->type & 
> + 0x3f);
>
> /* TODO: compute ECC if hardware support is not available */
>
> @@ -466,16 +466,16 @@ int mipi_dsi_create_packet(struct mipi_dsi_packet 
> *packet,
>  * and 2.
>  */
> if (mipi_dsi_packet_format_is_long(msg->type)) {
> -   packet->header[0] = (msg->tx_len >> 0) & 0xff;
> -   packet->header[1] = (msg->tx_len >> 8) & 0xff;
> +   packet->header[1] = (msg->tx_len >> 0) & 0xff;
> +   packet->header[2] = (msg->tx_len >> 8) & 0xff;
>
> packet->payload_length = msg->tx_len;
> packet->payload = msg->tx_buf;
> } else {
> const u8 *tx = msg->tx_buf;
>
> -   packet->header[0] = (msg->tx_len > 0) ? tx[0] : 0;
> -   packet->header[1] = (msg->tx_len > 1) ? tx[1] : 0;
> +   packet->header[1] = (msg->tx_len > 0) ? tx[0] : 0;
> +   packet->header[2] = (msg->tx_len > 1) ? tx[1] : 0;
> }
>
> packet->size = sizeof(packet->header) + 
> packet->payload_length;
> --
> 2.16.1.291.g4437f3f132-goog
>
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno