date:20221109


On 10/11/2022 02:47, Kuogee Hsieh wrote:


On 11/2/2022 11:04 AM, Dmitry Baryshkov wrote:

On 02/11/2022 20:28, Doug Anderson wrote:

Hi,

On Wed, Nov 2, 2022 at 10:23 AM Dmitry Baryshkov
 wrote:



1. Someone figures out how to model this with the bridge chain and
then we only allow HBR3 if we detect we've got a TCPC that supports
it. This seems like the cleanest / best but feels like a long pole.
Not only have we been trying to get the TCPC-modeled-as-a-bridge stuff
landed for a long time but even when we do it we still don't have a
solution for how to communicate the number of lanes and other stuff
between the TCPC and the DP controller so we have to enrich the bridge
interface.


I think we'd need some OOB interface. For example for DSI interfaces we
have mipi_dsi_device struct to communicate such OOB data.

Also take a note regarding data-lanes from my previous email.


Right, we can somehow communicate the max link rate through the bridge
chain to the DP controller in an OOB manner that would work.


I'd note that our dp_panel has some notion of such OOB data. So do AUX 
drivers including the panel-edp. My suggestion would be to consider 
both of them while modelling the OOB data.






2. We add in a DT property to the display controller node that says
the max link rate for use on this board. This feels like a hack, but
maybe it's not too bad. Certainly it would be incredibly simple to
implement. Actually... ...one could argue that even if we later model
the TCPC as a bridge that this property would still be valid / useful!
You could certainly imagine that the SoC supports HBR3 and the TCPC
supports HBR3 but that the board routing between the SoC and the TCPC
is bad and only supports HBR2. In this case the only way out is
essentially a "board constraint" AKA a DT property in the DP
controller.


We have been discussing similar topics with Abhinav. Krzysztof 
suggested

using link-frequencies property to provide max and min values.


questions,

1)is Krzysztof suggested had been implemented?


I can not parse this question, please excuse me.

Yes, Krzysztof suggested this being implemented as a link property, see 
media/video-interfaces.txt.


Moreover your implementation goes against both the existing definition 
(array with the list of frequencies) and Krzysztof's suggested extension 
(min and max). Listing just a single frequency goes against both these 
suggestions. In case of DP we have a fixed set of frequencies. Thus I'd 
suggest listing all supported frequencies instead.



2) where is link property i can add link-frequencies?


link node. Create outbound graph node, add link-frequencies there. Also 
as you are touching this part, please move the data-lanes property too.







This sounds good to me and seems worth doing even if we eventually do 
#1.


And the bonus point is that it can be done easily.



--
With best wishes
Dmitry

Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane

2022-11-09 Thread Jessica Zhang

On 11/9/2022 5:59 AM, Daniel Vetter wrote:

On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote:

On 09/11/2022 16:52, Daniel Vetter wrote:

On Tue, Nov 08, 2022 at 06:25:29PM +, Simon Ser wrote:

On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov
wrote:

On 29/10/2022 01:59, Jessica Zhang wrote:

Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
drm_plane. In addition, add support for setting and getting the values
of these properties.

COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
represents the format of the color fill. Userspace can set enable solid
fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
framebuffer to NULL.

I suppose that COLOR_FILL should override framebuffer rather than
requiring that FB is set to NULL. In other words, if color_filL_format
is non-zero, it would make sense to ignore the FB. Then one can use the
color_fill_format property to quickly switch between filled plane and
FB-backed one.

That would be inconsistent with the rest of the KMS uAPI. For instance,
the kernel will error out if CRTC has active=0 but a connector is still
linked to the CRTC. IOW, the current uAPI errors out if the KMS state
is inconsistent.

So if the use-case here really is to solid-fill a plane (and not just
provide a background color for the crtc overall), then I guess we could
also extend addfb to make that happen. We've talked in the past about
propertery-fying framebuffer objects, and that would sort out this uapi
wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.

But if the use-cases are all background color then just doing the crtc
background color would be tons simpler (and likely also easier to support
for more hardware).

No. The hardware supports multiple color-filled planes, which do not have to
cover the whole CRTC.

The use case here means the userspace use-case. What the hw can do on any
given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not
meant to reflect 100% exactly what a specific chip can do, but instead:
- provide features userspace actually needs. If you want per-plane fill,
you need userspace that makes use of per-plane fill, and if all you have
is crtc background, then that's it.

Hey Daniel,

The userspace use case we're trying to support is the Android HWC
SOLID_FILL hint here [1], which is specifying per-plane fill.

Thanks,

Jessica Zhang

[1]
https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52

- we should create uapi with an eye towards what's actually possible on a
reasonable set of drivers and hw. Sometimes that means a slightly more
restricted set so that it's possible to implement in more places,
especially if that restricted feature set still gets the job done for
userspace.

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

Re: [Freedreno] [PATCH v3 2/2] drm/msm: Hangcheck progress detection

2022-11-09 Thread Chia-I Wu

On Fri, Nov 4, 2022 at 8:08 AM Rob Clark  wrote:
>
> From: Rob Clark 
>
> If the hangcheck timer expires, check if the fw's position in the
> cmdstream has advanced (changed) since last timer expiration, and
> allow it up to three additional "extensions" to it's alotted time.
> The intention is to continue to catch "shader stuck in a loop" type
> hangs quickly, but allow more time for things that are actually
> making forward progress.
>
> Because we need to sample the CP state twice to detect if there has
> not been progress, this also cuts the the timer's duration in half.
>
> v2: Fix typo (REG_A6XX_CP_CSQ_IB2_STAT), add comment
> v3: Only halve hangcheck timer duration for generations which
> support progress detection (hdanton); removed unused a5xx
> progress (without knowing how to adjust for data buffered
> in ROQ it is too likely to report a false negative)
>
> Signed-off-by: Rob Clark 
> Reviewed-by: Akhil P Oommen 
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 34 +++
>  drivers/gpu/drm/msm/msm_drv.c |  1 -
>  drivers/gpu/drm/msm/msm_drv.h |  8 ++-
>  drivers/gpu/drm/msm/msm_gpu.c | 31 +++-
>  drivers/gpu/drm/msm/msm_gpu.h |  3 +++
>  drivers/gpu/drm/msm/msm_ringbuffer.h  | 24 +++
>  6 files changed, 98 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 1ff605c18ee6..7fe60c65a1eb 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1843,6 +1843,39 @@ static uint32_t a6xx_get_rptr(struct msm_gpu *gpu, 
> struct msm_ringbuffer *ring)
> return ring->memptrs->rptr = gpu_read(gpu, REG_A6XX_CP_RB_RPTR);
>  }
>
> +static bool a6xx_progress(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
> +{
> +   struct msm_cp_state cp_state = {
> +   .ib1_base = gpu_read64(gpu, REG_A6XX_CP_IB1_BASE),
> +   .ib2_base = gpu_read64(gpu, REG_A6XX_CP_IB2_BASE),
> +   .ib1_rem  = gpu_read(gpu, REG_A6XX_CP_IB1_REM_SIZE),
> +   .ib2_rem  = gpu_read(gpu, REG_A6XX_CP_IB2_REM_SIZE),
> +   };
> +   bool progress;
> +
> +   /*
> +* Adjust the remaining data to account for what has already been
> +* fetched from memory, but not yet consumed by the SQE.
> +*
> +* This is not *technically* correct, the amount buffered could
> +* exceed the IB size due to hw prefetching ahead, but:
> +*
> +* (1) We aren't trying to find the exact position, just whether
> +* progress has been made
> +* (2) The CP_REG_TO_MEM at the end of a submit should be enough
> +* to prevent prefetching into an unrelated submit.  (And
> +* either way, at some point the ROQ will be full.)
> +*/
> +   cp_state.ib1_rem += gpu_read(gpu, REG_A6XX_CP_CSQ_IB1_STAT) >> 16;
> +   cp_state.ib2_rem += gpu_read(gpu, REG_A6XX_CP_CSQ_IB2_STAT) >> 16;
> +
> +   progress = !!memcmp(_state, >last_cp_state, 
> sizeof(cp_state));
> +
> +   ring->last_cp_state = cp_state;
> +
> +   return progress;
> +}
> +
>  static u32 a618_get_speed_bin(u32 fuse)
>  {
> if (fuse == 0)
> @@ -1961,6 +1994,7 @@ static const struct adreno_gpu_funcs funcs = {
> .create_address_space = a6xx_create_address_space,
> .create_private_address_space = 
> a6xx_create_private_address_space,
> .get_rptr = a6xx_get_rptr,
> +   .progress = a6xx_progress,
> },
> .get_timestamp = a6xx_get_timestamp,
>  };
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 670651cdfa79..c3b77b44b2aa 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -419,7 +419,6 @@ static int msm_drm_init(struct device *dev, const struct 
> drm_driver *drv)
> priv->dev = ddev;
>
> priv->wq = alloc_ordered_workqueue("msm", 0);
> -   priv->hangcheck_period = DRM_MSM_HANGCHECK_DEFAULT_PERIOD;
>
> INIT_LIST_HEAD(>objects);
> mutex_init(>obj_lock);
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 0609daf4fa4c..876d8d5eec2f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -225,7 +225,13 @@ struct msm_drm_private {
>
> struct drm_atomic_state *pm_state;
>
> -   /* For hang detection, in ms */
> +   /**
> +* hangcheck_period: For hang detection, in ms
> +*
> +* Note that in practice, a submit/job will get at least two hangcheck
> +* periods, due to checking for progress being implemented as simply
> +* "have the CP position registers changed since last time?"
> +*/
> unsigned int hangcheck_period;
>
> /**
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c

Re: [Freedreno] [PATCH] drm/msm/dp: remove limitation of link rate at 5.4G to support HBR3




On 11/2/2022 11:04 AM, Dmitry Baryshkov wrote:

On 02/11/2022 20:28, Doug Anderson wrote:

Hi,

On Wed, Nov 2, 2022 at 10:23 AM Dmitry Baryshkov
 wrote:



1. Someone figures out how to model this with the bridge chain and
then we only allow HBR3 if we detect we've got a TCPC that supports
it. This seems like the cleanest / best but feels like a long pole.
Not only have we been trying to get the TCPC-modeled-as-a-bridge stuff
landed for a long time but even when we do it we still don't have a
solution for how to communicate the number of lanes and other stuff
between the TCPC and the DP controller so we have to enrich the bridge
interface.


I think we'd need some OOB interface. For example for DSI interfaces we
have mipi_dsi_device struct to communicate such OOB data.

Also take a note regarding data-lanes from my previous email.


Right, we can somehow communicate the max link rate through the bridge
chain to the DP controller in an OOB manner that would work.


I'd note that our dp_panel has some notion of such OOB data. So do AUX 
drivers including the panel-edp. My suggestion would be to consider 
both of them while modelling the OOB data.






2. We add in a DT property to the display controller node that says
the max link rate for use on this board. This feels like a hack, but
maybe it's not too bad. Certainly it would be incredibly simple to
implement. Actually... ...one could argue that even if we later model
the TCPC as a bridge that this property would still be valid / useful!
You could certainly imagine that the SoC supports HBR3 and the TCPC
supports HBR3 but that the board routing between the SoC and the TCPC
is bad and only supports HBR2. In this case the only way out is
essentially a "board constraint" AKA a DT property in the DP
controller.


We have been discussing similar topics with Abhinav. Krzysztof 
suggested

using link-frequencies property to provide max and min values.


questions,

1)is Krzysztof suggested had been implemented?

2) where is link property i can add link-frequencies?




This sounds good to me and seems worth doing even if we eventually do 
#1.


And the bonus point is that it can be done easily.

Re: [Freedreno] [PATCH v2 1/2] arm64: dts: qcom: Add link-frequencies property to specify the max link rate allowed


On 09/11/2022 21:34, Kuogee Hsieh wrote:

This patch add link-frequencies property to allow each platform to specify its
DP max link rate.

Signed-off-by: Kuogee Hsieh 
---
  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
  1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 93e39fc..7e5d755 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -441,6 +441,7 @@ ap_i2c_tpm:  {
pinctrl-names = "default";
pinctrl-0 = <_hot_plug_det>;
data-lanes = <0 1>;
+link-frequencies = <81>;


The link frequency is a property of the link - in other words a graph 
connection. Please don't put random propreties into DT nodes where they 
are not to be used.




  };
  
  _mdp {


--
With best wishes
Dmitry

[Freedreno] [PATCH v2 2/2] drm/msm/dp: add support of max dp link rate

Since it is not every platform supports HBR3 link rate, this patch
limit the DP link rate at max link rate if it is specified at DTS file.
Otherwise, the max dp link rate will be limited at HBR2 as before.

Changes in v2:
-- add max link rate from dtsi

Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 1 +
 drivers/gpu/drm/msm/dp/dp_panel.c   | 5 ++---
 drivers/gpu/drm/msm/dp/dp_panel.h   | 1 +
 drivers/gpu/drm/msm/dp/dp_parser.c  | 8 
 drivers/gpu/drm/msm/dp/dp_parser.h  | 1 +
 5 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 29c9845..0e1a9b3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -390,6 +390,7 @@ static int dp_display_process_hpd_high(struct 
dp_display_private *dp)
struct edid *edid;
 
dp->panel->max_dp_lanes = dp->parser->max_dp_lanes;
+   dp->panel->max_dp_link_rate = dp->parser->max_dp_link_rate;
 
rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector);
if (rc)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 5149ceb..87c27ca 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -78,9 +78,8 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
if (link_info->num_lanes > dp_panel->max_dp_lanes)
link_info->num_lanes = dp_panel->max_dp_lanes;
 
-   /* Limit support upto HBR2 until HBR3 support is added */
-   if (link_info->rate >= (drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4)))
-   link_info->rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_5_4);
+   if (link_info->rate > dp_panel->max_dp_link_rate)
+   link_info->rate = dp_panel->max_dp_link_rate;
 
drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h 
b/drivers/gpu/drm/msm/dp/dp_panel.h
index d861197a..f04d021 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -50,6 +50,7 @@ struct dp_panel {
 
u32 vic;
u32 max_dp_lanes;
+   u32 max_dp_link_rate;
 
u32 max_bw_code;
 };
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c 
b/drivers/gpu/drm/msm/dp/dp_parser.c
index dd73221..d2e31c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -95,6 +95,7 @@ static int dp_parser_misc(struct dp_parser *parser)
 {
struct device_node *of_node = parser->pdev->dev.of_node;
int len;
+   u32 rate;
 
len = drm_of_get_data_lanes_count(of_node, 1, DP_MAX_NUM_DP_LANES);
if (len < 0) {
@@ -104,6 +105,13 @@ static int dp_parser_misc(struct dp_parser *parser)
}
 
parser->max_dp_lanes = len;
+
+   len = of_property_read_s32(of_node, "link-frequencies", );
+   if (len >= 0)
+   parser->max_dp_link_rate = rate;
+   else
+   parser->max_dp_link_rate = 54;  /* default HBR2 */
+
return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h 
b/drivers/gpu/drm/msm/dp/dp_parser.h
index 866c1a8..ba63375 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -119,6 +119,7 @@ struct dp_parser {
struct dp_io io;
struct dp_display_data disp_data;
u32 max_dp_lanes;
+   u32 max_dp_link_rate;
struct drm_bridge *next_bridge;
 
int (*parse)(struct dp_parser *parser);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[Freedreno] [PATCH v2 1/2] arm64: dts: qcom: Add link-frequencies property to specify the max link rate allowed

This patch add link-frequencies property to allow each platform to specify its
DP max link rate.

Signed-off-by: Kuogee Hsieh 
---
 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi 
b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 93e39fc..7e5d755 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -441,6 +441,7 @@ ap_i2c_tpm:  {
pinctrl-names = "default";
pinctrl-0 = <_hot_plug_det>;
data-lanes = <0 1>;
+link-frequencies = <81>;
 };
 
 _mdp {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[Freedreno] [PATCH v2 0/2] Add DP max link rate support

dd DP link-frequencies property to DTS file and support function to DP driver.

Kuogee Hsieh (2):
  arm64: dts: qcom: Add link-frequencies property to specify the max
link rate allowed
  drm/msm/dp: add support of max dp link rate

 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
 drivers/gpu/drm/msm/dp/dp_display.c| 1 +
 drivers/gpu/drm/msm/dp/dp_panel.c  | 5 ++---
 drivers/gpu/drm/msm/dp/dp_panel.h  | 1 +
 drivers/gpu/drm/msm/dp/dp_parser.c | 8 
 drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
 6 files changed, 14 insertions(+), 3 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc



>-Original Message-
>From: Kalyan Thota 
>Sent: Wednesday, November 9, 2022 6:53 PM
>To: dmitry.barysh...@linaro.org; Kalyan Thota (QUIC)
>; dri-de...@lists.freedesktop.org; linux-arm-
>m...@vger.kernel.org; freedreno@lists.freedesktop.org;
>devicet...@vger.kernel.org
>Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
>diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
>; Abhinav Kumar (QUIC)
>
>Subject: RE: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>
>
>>-Original Message-
>>From: Dmitry Baryshkov 
>>Sent: Wednesday, November 9, 2022 6:02 PM
>>To: Kalyan Thota (QUIC) ; dri-
>>de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
>>freedreno@lists.freedesktop.org; devicet...@vger.kernel.org
>>Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
>>diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
>>; Abhinav Kumar (QUIC)
>>
>>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management
>>support for the crtc
>>
>>WARNING: This email originated from outside of Qualcomm. Please be wary
>>of any links or attachments, and do not enable macros.
>>
>>On 09/11/2022 15:16, Kalyan Thota wrote:
>>> Add color management support for the crtc provided there are enough
>>> dspps that can be allocated from the catalogue
>>>
>>> Signed-off-by: Kalyan Thota 
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  6 
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53
>>+
>>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 4170fbe..6bd3a64 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -18,9 +18,11 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>> +#include 
>>>   #include 
>>>   #include 
>>>   #include 
>>> +#include "../../../drm_crtc_internal.h"
>>
>>If it's internal, it is not supposed to be used by other parties,
>>including the msm drm. At least a comment why you are including this file 
>>would
>be helpful.
>>
>>>
>>>   #include "dpu_kms.h"
>>>   #include "dpu_hw_lm.h"
>>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct
>>> drm_crtc
>>*crtc)
>>>   spin_unlock_irqrestore(>event_lock, flags);
>>>   }
>>>
>>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>>> + u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>>> + u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>>> + u32 degamma_id =
>>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>>> +
>>> + return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
>>> +drm_mode_obj_find_prop_id(>base, gamma_id) ||
>>> +drm_mode_obj_find_prop_id(>base, degamma_id));
>>> +}
>>> +
>>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>>   {
>>>   struct drm_encoder *encoder;
>>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct
>>> drm_device *dev, struct drm_plane *plane,
>>>
>>>   drm_crtc_helper_add(crtc, _crtc_helper_funcs);
>>>
>>> - drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>>> -
>>>   /* save user friendly CRTC name for later */
>>>   snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>>> crtc->base.id);
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> index 539b68b..8bac395 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>>dpu_crtc_get_client_type(
>>>   return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>>   }
>>>
>>> +/**
>>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>>> +management enabled
>>> + * @crtc: Pointer to drm crtc object  */ bool
>>> +dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>>> +
>>>   #endif /* _DPU_CRTC_H_ */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 4c56a16..ebc3f25 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct
>drm_encoder
>>*drm_enc)
>>>   static struct msm_display_topology dpu_encoder_get_topology(
>>>   struct dpu_encoder_virt *dpu_enc,
>>>   struct dpu_kms *dpu_kms,
>>> - struct drm_display_mode *mode)
>>> + struct drm_display_mode *mode,
>>> + struct drm_crtc *crtc)
>>>   {
>>>   struct msm_display_topology topology = {0};
>>>   int i, intf_count = 0;
>>> @@ -573,11 +574,9 @@ static struct

Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane

2022-11-09 Thread Pekka Paalanen

On Tue, 8 Nov 2022 23:01:47 +0100
Sebastian Wick  wrote:

> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser  wrote:
> >
> > cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.

Hi all,

thanks! Comments below.

> >
> > On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov 
> >  wrote:
> >  
> > > On 29/10/2022 01:59, Jessica Zhang wrote:  
> > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > > drm_plane. In addition, add support for setting and getting the values
> > > > of these properties.
> > > >
> > > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > > represents the format of the color fill. Userspace can set enable solid
> > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > > framebuffer to NULL.
> > > >
> > > > Signed-off-by: Jessica Zhang   
> > >
> > > Planes report supported formats using the drm_mode_getplane(). You'd
> > > also need to tell userspace, which formats are supported for color fill.
> > > I don't think one supports e.g. YV12.
> > >
> > > A bit of generic comment for the discussion (this is an RFC anyway).
> > > Using color_fill/color_fill_format properties sounds simple, but this
> > > might be not generic enough. Limiting color_fill to 32 bits would
> > > prevent anybody from using floating point formats (e.g.
> > > DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> > > e.g. using 64-bit for the color_fill value, but then this doesn't sound
> > > extensible too much.
> > >
> > > So, a question for other hardware maintainers. Do we have hardware that
> > > supports such 'color filled' planes? Do we want to support format
> > > modifiers for filling color/data? Because what I have in mind is closer
> > > to the blob structure, which can then be used for filling the plane:
> > >
> > > struct color_fill_blob {
> > >  u32 pixel_format;
> > >  u64 modifiers4];
> > >  u32 pixel_data_size; // fixme: is this necessary?
> > >  u8 pixel_data[];
> > > };
> > >
> > > And then... This sounds a lot like a custom framebuffer.
> > >
> > > So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> > > flag to the framebuffers, which would e.g. mean that the FB gets stamped
> > > all over the plane. This would also save us from changing if (!fb)
> > > checks all over the drm core.
> > >
> > > Another approach might be using a format modifier instead of the FB flag.
> > >
> > > What do you think?  
> >
> > First off, we only need to represent the value of a single pixel here. So 
> > I'm
> > not quite following why we need format modifiers. Format modifiers describe 
> > how
> > pixels are laid out in memory. Since there's a single pixel described, this
> > is non-sensical to me, the format modifier is always LINEAR.

Agreed.

> >
> > Then, I can understand why putting the pixel_format in there is tempting to
> > guarantee future extensibility, but it also adds complexity. For instance, 
> > how
> > does user-space figure out which formats can be used for COLOR_FILL? Can
> > user-space use any format supported by the plane? What does it mean for
> > multi-planar formats? Do we really want the kernel to have conversion logic 
> > for
> > all existing formats? Do we need to also add a new read-only blob prop to
> > indicate supported COLOR_FILL formats?

Right. This does not seem to require pixel formats at all.

The point of pixel formats is to be able to feed large amounts of data
as-is into hardware and avoid the CPU ever touching it. You do that
with DRM FBs pointing to suitably allocated hardware buffers. But here
we have exactly one pixel, which I imagine will always be read by the
CPU so the driver will convert it into a hardware-specific format and
program it; probably the driver will not create an internal DRM FB for
it.

The above might also be a reason to not model this as a special-case
DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
DRM FB to avoid the need to allocate e.g. a dumb buffer or a
GPU-specific buffer.

What one does need is what Sebastian brought up: does it support alpha
or not?

Userspace would also be interested in the supported precision of the
values, but the hardware pixel component order is irrelevant because the
driver will always convert the one pixel with CPU anyway.

YUV vs. RGB is a another question. The KMS color pipeline is defined in
terms of RGBA as far as I know, and alpha-blending YUV values makes no
sense. So will there ever be any need to set an YUV fill? I have a hard
time imagining it.

If you do set an YUV fill, the KMS color pipeline most likely needs to
convert it to RGBA for blending, and then you need the plane properties
COLOR_ENCODING and COLOR_RANGE.

But why bother when userspace can convert that one pixel to RGBA itself
anyway?

> > We've recently-ish standardized a new Wayland protocol [1]

Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane

2022-11-09 Thread Daniel Vetter

On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote:
> On 09/11/2022 16:52, Daniel Vetter wrote:
> > On Tue, Nov 08, 2022 at 06:25:29PM +, Simon Ser wrote:
> > > On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov 
> > >  wrote:
> > > 
> > > > On 29/10/2022 01:59, Jessica Zhang wrote:
> > > > 
> > > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > > > drm_plane. In addition, add support for setting and getting the values
> > > > > of these properties.
> > > > > 
> > > > > COLOR_FILL represents the color fill of a plane while 
> > > > > COLOR_FILL_FORMAT
> > > > > represents the format of the color fill. Userspace can set enable 
> > > > > solid
> > > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > > > framebuffer to NULL.
> > > > 
> > > > I suppose that COLOR_FILL should override framebuffer rather than
> > > > requiring that FB is set to NULL. In other words, if color_filL_format
> > > > is non-zero, it would make sense to ignore the FB. Then one can use the
> > > > color_fill_format property to quickly switch between filled plane and
> > > > FB-backed one.
> > > 
> > > That would be inconsistent with the rest of the KMS uAPI. For instance,
> > > the kernel will error out if CRTC has active=0 but a connector is still
> > > linked to the CRTC. IOW, the current uAPI errors out if the KMS state
> > > is inconsistent.
> > 
> > So if the use-case here really is to solid-fill a plane (and not just
> > provide a background color for the crtc overall), then I guess we could
> > also extend addfb to make that happen. We've talked in the past about
> > propertery-fying framebuffer objects, and that would sort out this uapi
> > wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.
> > 
> > But if the use-cases are all background color then just doing the crtc
> > background color would be tons simpler (and likely also easier to support
> > for more hardware).
> 
> No. The hardware supports multiple color-filled planes, which do not have to
> cover the whole CRTC.

The use case here means the userspace use-case. What the hw can do on any
given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not
meant to reflect 100% exactly what a specific chip can do, but instead:
- provide features userspace actually needs. If you want per-plane fill,
  you need userspace that makes use of per-plane fill, and if all you have
  is crtc background, then that's it.
- we should create uapi with an eye towards what's actually possible on a
  reasonable set of drivers and hw. Sometimes that means a slightly more
  restricted set so that it's possible to implement in more places,
  especially if that restricted feature set still gets the job done for
  userspace.

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

Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane


On 09/11/2022 16:52, Daniel Vetter wrote:

On Tue, Nov 08, 2022 at 06:25:29PM +, Simon Ser wrote:

On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov 
 wrote:


On 29/10/2022 01:59, Jessica Zhang wrote:


Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
drm_plane. In addition, add support for setting and getting the values
of these properties.

COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
represents the format of the color fill. Userspace can set enable solid
fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
framebuffer to NULL.


I suppose that COLOR_FILL should override framebuffer rather than
requiring that FB is set to NULL. In other words, if color_filL_format
is non-zero, it would make sense to ignore the FB. Then one can use the
color_fill_format property to quickly switch between filled plane and
FB-backed one.


That would be inconsistent with the rest of the KMS uAPI. For instance,
the kernel will error out if CRTC has active=0 but a connector is still
linked to the CRTC. IOW, the current uAPI errors out if the KMS state
is inconsistent.


So if the use-case here really is to solid-fill a plane (and not just
provide a background color for the crtc overall), then I guess we could
also extend addfb to make that happen. We've talked in the past about
propertery-fying framebuffer objects, and that would sort out this uapi
wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.

But if the use-cases are all background color then just doing the crtc
background color would be tons simpler (and likely also easier to support
for more hardware).


No. The hardware supports multiple color-filled planes, which do not 
have to cover the whole CRTC.


--
With best wishes
Dmitry

Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane

2022-11-09 Thread Daniel Vetter

On Tue, Nov 08, 2022 at 06:25:29PM +, Simon Ser wrote:
> On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov 
>  wrote:
> 
> > On 29/10/2022 01:59, Jessica Zhang wrote:
> > 
> > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> > > drm_plane. In addition, add support for setting and getting the values
> > > of these properties.
> > > 
> > > COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> > > represents the format of the color fill. Userspace can set enable solid
> > > fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> > > framebuffer to NULL.
> > 
> > I suppose that COLOR_FILL should override framebuffer rather than
> > requiring that FB is set to NULL. In other words, if color_filL_format
> > is non-zero, it would make sense to ignore the FB. Then one can use the
> > color_fill_format property to quickly switch between filled plane and
> > FB-backed one.
> 
> That would be inconsistent with the rest of the KMS uAPI. For instance,
> the kernel will error out if CRTC has active=0 but a connector is still
> linked to the CRTC. IOW, the current uAPI errors out if the KMS state
> is inconsistent.

So if the use-case here really is to solid-fill a plane (and not just
provide a background color for the crtc overall), then I guess we could
also extend addfb to make that happen. We've talked in the past about
propertery-fying framebuffer objects, and that would sort out this uapi
wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at least.

But if the use-cases are all background color then just doing the crtc
background color would be tons simpler (and likely also easier to support
for more hardware).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc



>-Original Message-
>From: Dmitry Baryshkov 
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) ; dri-
>de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicet...@vger.kernel.org
>Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
>diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
>; Abhinav Kumar (QUIC)
>
>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 09/11/2022 15:16, Kalyan Thota wrote:
>> Add color management support for the crtc provided there are enough
>> dspps that can be allocated from the catalogue
>>
>> Signed-off-by: Kalyan Thota 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  6 
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53
>+
>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4170fbe..6bd3a64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -18,9 +18,11 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>   #include 
>>   #include 
>>   #include 
>> +#include "../../../drm_crtc_internal.h"
>
>If it's internal, it is not supposed to be used by other parties, including 
>the msm
>drm. At least a comment why you are including this file would be helpful.
>
>>
>>   #include "dpu_kms.h"
>>   #include "dpu_hw_lm.h"
>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>*crtc)
>>   spin_unlock_irqrestore(>event_lock, flags);
>>   }
>>
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>> + u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>> + u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>> + u32 degamma_id =
>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>> +
>> + return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
>> +drm_mode_obj_find_prop_id(>base, gamma_id) ||
>> +drm_mode_obj_find_prop_id(>base, degamma_id));
>> +}
>> +
>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>   {
>>   struct drm_encoder *encoder;
>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>>   drm_crtc_helper_add(crtc, _crtc_helper_funcs);
>>
>> - drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> -
>>   /* save user friendly CRTC name for later */
>>   snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>> crtc->base.id);
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 539b68b..8bac395 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>dpu_crtc_get_client_type(
>>   return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>   }
>>
>> +/**
>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>> +management enabled
>> + * @crtc: Pointer to drm crtc object
>> + */
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>> +
>>   #endif /* _DPU_CRTC_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c56a16..ebc3f25 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder
>*drm_enc)
>>   static struct msm_display_topology dpu_encoder_get_topology(
>>   struct dpu_encoder_virt *dpu_enc,
>>   struct dpu_kms *dpu_kms,
>> - struct drm_display_mode *mode)
>> + struct drm_display_mode *mode,
>> + struct drm_crtc *crtc)
>>   {
>>   struct msm_display_topology topology = {0};
>>   int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ static struct msm_display_topology
>dpu_encoder_get_topology(
>>   else
>>   topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT)
>> ? 2 : 1;
>>
>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>> - if (dpu_kms->catalog->dspp &&
>> - (dpu_kms->catalog->dspp_count >= topology.num_lm))
>> + if (dpu_crtc_has_color_enabled(crtc) &&
>> + (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
>See the comment to the previous patch. It still applies here.
>
>>   topology.num_dspp = topology.num_lm;
>> - }
>>
>>   topology.num_enc = 0;
>>

Re: [Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc


On 09/11/2022 15:39, Kalyan Thota wrote:




-Original Message-
From: Dmitry Baryshkov 
Sent: Wednesday, November 9, 2022 6:02 PM
To: Kalyan Thota (QUIC) ; dri-
de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
freedreno@lists.freedesktop.org; devicet...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
; Abhinav Kumar (QUIC)

Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
for the crtc

WARNING: This email originated from outside of Qualcomm. Please be wary of
any links or attachments, and do not enable macros.

On 09/11/2022 15:16, Kalyan Thota wrote:

Add color management support for the crtc provided there are enough
dspps that can be allocated from the catalogue

Signed-off-by: Kalyan Thota 
---
   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  6 
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53

+

   4 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..6bd3a64 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -18,9 +18,11 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
+#include "../../../drm_crtc_internal.h"


If it's internal, it is not supposed to be used by other parties, including the 
msm
drm. At least a comment why you are including this file would be helpful.


This header file was included to make use of " drm_mode_obj_find_prop_id" 
function from DRM framework.
Should I add a comment near function definition ?


No, it would have been better to add a comment near the #include 
directive. However if this is the only user, you don't need it at all. 
You see, you know whether the CRTC has color management propreties at 
the time of creation. And this can not change. So, you just add a 
boolean to dpu_crtc (or dpu_crtc_state, whichever suits better).




   #include "dpu_kms.h"
   #include "dpu_hw_lm.h"
@@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc

*crtc)

   spin_unlock_irqrestore(>event_lock, flags);
   }

+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
+ u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
+ u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
+ u32 degamma_id =
+crtc->dev->mode_config.degamma_lut_property->base.id;
+
+ return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
+drm_mode_obj_find_prop_id(>base, gamma_id) ||
+drm_mode_obj_find_prop_id(>base, degamma_id));
+}
+
   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
   {
   struct drm_encoder *encoder;
@@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
*dev, struct drm_plane *plane,

   drm_crtc_helper_add(crtc, _crtc_helper_funcs);

- drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
-
   /* save user friendly CRTC name for later */
   snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
crtc->base.id);

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..8bac395 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type

dpu_crtc_get_client_type(

   return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
   }

+/**
+ * dpu_crtc_has_color_enabled - check if the crtc has color
+management enabled
+ * @crtc: Pointer to drm crtc object
+ */
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
+
   #endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4c56a16..ebc3f25 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder

*drm_enc)

   static struct msm_display_topology dpu_encoder_get_topology(
   struct dpu_encoder_virt *dpu_enc,
   struct dpu_kms *dpu_kms,
- struct drm_display_mode *mode)
+ struct drm_display_mode *mode,
+ struct drm_crtc *crtc)
   {
   struct msm_display_topology topology = {0};
   int i, intf_count = 0;
@@ -573,11 +574,9 @@ static struct msm_display_topology

dpu_encoder_get_topology(

   else
   topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT)
? 2 : 1;

- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
- if (dpu_kms->catalog->dspp &&
- (dpu_kms->catalog->dspp_count >= topology.num_lm))
+ if

Re: [Freedreno] [v8] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280


On 09/11/2022 15:14, Kalyan Thota wrote:

Flush mechanism for DSPP blocks has changed in sc7280 family, it
allows individual sub blocks to be flushed in coordination with
master flush control.

Representation: master_flush && (PCC_flush | IGC_flush .. etc )

This change adds necessary support for the above design.

Changes in v1:
- Few nits (Doug, Dmitry)
- Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)

Changes in v2:
- Move the address offset to flush macro (Dmitry)
- Seperate ops for the sub block flush (Dmitry)

Changes in v3:
- Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)

Changes in v4:
- Use shorter version for unsigned int (Stephen)

Changes in v5:
- Spurious patch please ignore.

Changes in v6:
- Add SOB tag (Doug, Dmitry)

Changes in v7:
- Cache flush mask per dspp (Dmitry)
- Few nits (Marijn)

Changes in v8:
- Few nits (Marijn)

Signed-off-by: Kalyan Thota 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c   |  2 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c |  5 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +++--
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 66 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h |  7 ++-
  5 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 601d687..4170fbe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -766,7 +766,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
  
  		/* stage config flush mask */

ctl->ops.update_pending_flush_dspp(ctl,
-   mixer[i].hw_dspp->idx);
+   mixer[i].hw_dspp->idx, DPU_DSPP_PCC);
}
  }
  
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c

index 27f029f..0eecb2f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -65,7 +65,10 @@
(PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
  
  #define CTL_SC7280_MASK \

-   (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) | 
BIT(DPU_CTL_VM_CFG))
+   (BIT(DPU_CTL_ACTIVE_CFG) | \
+BIT(DPU_CTL_FETCH_ACTIVE) | \
+BIT(DPU_CTL_VM_CFG) | \
+BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
  
  #define MERGE_3D_SM8150_MASK (0)
  
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h

index 38aa38a..35f4810 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -161,10 +161,12 @@ enum {
   * DSPP sub-blocks
   * @DPU_DSPP_PCC Panel color correction block
   * @DPU_DSPP_GC  Gamma correction block
+ * @DPU_DSPP_IGC Inverse gamma correction block
   */
  enum {
DPU_DSPP_PCC = 0x1,
DPU_DSPP_GC,
+   DPU_DSPP_IGC,
DPU_DSPP_MAX
  };
  
@@ -188,16 +190,18 @@ enum {
  
  /**

   * CTL sub-blocks
- * @DPU_CTL_SPLIT_DISPLAY: CTL supports video mode split display
- * @DPU_CTL_FETCH_ACTIVE:  Active CTL for fetch HW (SSPPs)
- * @DPU_CTL_VM_CFG:CTL config to support multiple VMs
- * @DPU_CTL_MAX
+ * @DPU_CTL_SPLIT_DISPLAY CTL supports video mode split display
+ * @DPU_CTL_FETCH_ACTIVE  Active CTL for fetch HW (SSPPs)
+ * @DPU_CTL_VM_CFGCTL config to support multiple VMs
+ * @DPU_CTL_DSPP_BLOCK_FLUSH  CTL config to support dspp sub-block flush
+ * @DPU_CTL_MAX   Maximum value


No unnecessary whitespace changes please.


   */
  enum {
DPU_CTL_SPLIT_DISPLAY = 0x1,
DPU_CTL_ACTIVE_CFG,
DPU_CTL_FETCH_ACTIVE,
DPU_CTL_VM_CFG,
+   DPU_CTL_DSPP_SUB_BLOCK_FLUSH,
DPU_CTL_MAX
  };
  
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c

index a35ecb6..29821ea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -28,22 +28,23 @@
  #define   CTL_INTF_ACTIVE   0x0F4
  #define   CTL_MERGE_3D_FLUSH0x100
  #define   CTL_DSC_ACTIVE0x0E8
-#define   CTL_DSC_FLUSH0x104
+#define   CTL_DSC_FLUSH 0x104
  #define   CTL_WB_FLUSH  0x108
  #define   CTL_INTF_FLUSH0x110
  #define   CTL_INTF_MASTER   0x134
  #define   CTL_FETCH_PIPE_ACTIVE 0x0FC
+#define   CTL_DSPP_n_FLUSH(n)   ((0x13C) + ((n) * 4))
  
-#define CTL_MIXER_BORDER_OUTBIT(24)

-#define CTL_FLUSH_MASK_CTL  BIT(17)
+#define   CTL_MIXER_BORDER_OUT  BIT(24)
+#define   CTL_FLUSH_MASK_CTLBIT(17)
  
-#define DPU_REG_RESET_TIMEOUT_US2000

-#define  MERGE_3D_IDX   23
-#define  DSC_IDX22
-#define  INTF_IDX   31
-#define WB_IDX  16
-#define CTL_INVALID_BIT

Re: [Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc



>-Original Message-
>From: Dmitry Baryshkov 
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) ; dri-
>de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicet...@vger.kernel.org
>Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
>diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
>; Abhinav Kumar (QUIC)
>
>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 09/11/2022 15:16, Kalyan Thota wrote:
>> Add color management support for the crtc provided there are enough
>> dspps that can be allocated from the catalogue
>>
>> Signed-off-by: Kalyan Thota 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  6 
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53
>+
>>   4 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4170fbe..6bd3a64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -18,9 +18,11 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>   #include 
>>   #include 
>>   #include 
>> +#include "../../../drm_crtc_internal.h"
>
>If it's internal, it is not supposed to be used by other parties, including 
>the msm
>drm. At least a comment why you are including this file would be helpful.
>
This header file was included to make use of " drm_mode_obj_find_prop_id" 
function from DRM framework.
Should I add a comment near function definition ?
>>
>>   #include "dpu_kms.h"
>>   #include "dpu_hw_lm.h"
>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>*crtc)
>>   spin_unlock_irqrestore(>event_lock, flags);
>>   }
>>
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>> + u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>> + u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>> + u32 degamma_id =
>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>> +
>> + return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
>> +drm_mode_obj_find_prop_id(>base, gamma_id) ||
>> +drm_mode_obj_find_prop_id(>base, degamma_id));
>> +}
>> +
>>   enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>>   {
>>   struct drm_encoder *encoder;
>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>>   drm_crtc_helper_add(crtc, _crtc_helper_funcs);
>>
>> - drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> -
>>   /* save user friendly CRTC name for later */
>>   snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>> crtc->base.id);
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 539b68b..8bac395 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>dpu_crtc_get_client_type(
>>   return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>>   }
>>
>> +/**
>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>> +management enabled
>> + * @crtc: Pointer to drm crtc object
>> + */
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>> +
>>   #endif /* _DPU_CRTC_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c56a16..ebc3f25 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder
>*drm_enc)
>>   static struct msm_display_topology dpu_encoder_get_topology(
>>   struct dpu_encoder_virt *dpu_enc,
>>   struct dpu_kms *dpu_kms,
>> - struct drm_display_mode *mode)
>> + struct drm_display_mode *mode,
>> + struct drm_crtc *crtc)
>>   {
>>   struct msm_display_topology topology = {0};
>>   int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ static struct msm_display_topology
>dpu_encoder_get_topology(
>>   else
>>   topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT)
>> ? 2 : 1;
>>
>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>> - if (dpu_kms->catalog->dspp &&
>> - (dpu_kms->catalog->dspp_count >= topology.num_lm))
>> + if (dpu_crtc_has_color_enabled(crtc) &&
>> + (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
>See the comment to the

Re: [Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc


On 09/11/2022 15:16, Kalyan Thota wrote:

Add color management support for the crtc provided there are
enough dspps that can be allocated from the catalogue

Signed-off-by: Kalyan Thota 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  6 
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53 +
  4 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..6bd3a64 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -18,9 +18,11 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
+#include "../../../drm_crtc_internal.h"


If it's internal, it is not supposed to be used by other parties, 
including the msm drm. At least a comment why you are including this 
file would be helpful.


  
  #include "dpu_kms.h"

  #include "dpu_hw_lm.h"
@@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc *crtc)
spin_unlock_irqrestore(>event_lock, flags);
  }
  
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc)

+{
+   u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
+   u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
+   u32 degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id;
+
+   return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
+  drm_mode_obj_find_prop_id(>base, gamma_id) ||
+  drm_mode_obj_find_prop_id(>base, degamma_id));
+}
+
  enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
  {
struct drm_encoder *encoder;
@@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, 
struct drm_plane *plane,
  
  	drm_crtc_helper_add(crtc, _crtc_helper_funcs);
  
-	drm_crtc_enable_color_mgmt(crtc, 0, true, 0);

-
/* save user friendly CRTC name for later */
snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id);
  
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h

index 539b68b..8bac395 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type 
dpu_crtc_get_client_type(
return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
  }
  
+/**

+ * dpu_crtc_has_color_enabled - check if the crtc has color management enabled
+ * @crtc: Pointer to drm crtc object
+ */
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
+
  #endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4c56a16..ebc3f25 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
  static struct msm_display_topology dpu_encoder_get_topology(
struct dpu_encoder_virt *dpu_enc,
struct dpu_kms *dpu_kms,
-   struct drm_display_mode *mode)
+   struct drm_display_mode *mode,
+   struct drm_crtc *crtc)
  {
struct msm_display_topology topology = {0};
int i, intf_count = 0;
@@ -573,11 +574,9 @@ static struct msm_display_topology 
dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
  
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {

-   if (dpu_kms->catalog->dspp &&
-   (dpu_kms->catalog->dspp_count >= topology.num_lm))
+   if (dpu_crtc_has_color_enabled(crtc) &&
+   (dpu_kms->catalog->dspp_count >= topology.num_lm))


See the comment to the previous patch. It still applies here.


topology.num_dspp = topology.num_lm;
-   }
  
  	topology.num_enc = 0;

topology.num_intf = intf_count;
@@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
}
}
  
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);

+   topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, 
crtc_state->crtc);
  
  	/* Reserve dynamic resources now. */

if (!ret) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 552a89c..47a73fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -13,6 +13,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 

  #include 
@@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, 
unsigned crtc_mask)
dpu_kms_wait_for_commit_done(kms, crtc);
  }
  
+/**

+ * _dpu_kms_possible_dspps - Evaluate how

Re: [Freedreno] [PATCH 3/4] drm/msm/disp/dpu1: helper function to determine if encoder is virtual


On 09/11/2022 15:16, Kalyan Thota wrote:

Add a helper function to determine if an encoder is of type
virtual.


Please use commit messages to describe why you are doing something, not 
what you are doing. In this case I can predict, why do you need this API 
without going to patch 4.




Signed-off-by: Kalyan Thota 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++
  2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d6ad1f..4c56a16 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2469,6 +2469,17 @@ bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
return dpu_enc->disp_info.is_external;
  }
  
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc)

+{
+   struct dpu_encoder_virt *dpu_enc;
+
+   if (!drm_enc)
+   return false;
+
+   dpu_enc = to_dpu_encoder_virt(drm_enc);
+   return (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_VIRTUAL);
+}
+
  struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
int drm_enc_mode)
  {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 43f0d8b..6ae3c62 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -136,6 +136,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder 
*encoder);
  bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
  
  /**

+ * dpu_encoder_is_virtual - find if the encoder is of type virtual.
+ * @drm_enc:Pointer to previously created drm encoder structure
+ */
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc);
+
+/**
   * dpu_encoder_init - initialize virtual encoder object
   * @dev:Pointer to drm device structure
   * @disp_info:  Pointer to display information structure


--
With best wishes
Dmitry

Re: [Freedreno] [PATCH 2/4] drm/msm/disp/dpu1: populate disp_info if an interface is external


On 09/11/2022 15:16, Kalyan Thota wrote:

DRM encoder type is same for eDP and DP (DRM_MODE_ENCODER_TMDS)
populate is_external information in the disp_info so as to
differentiate between eDP and DP on the DPU encoder side.

Signed-off-by: Kalyan Thota 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 +---
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +++---
  drivers/gpu/drm/msm/dp/dp_display.c |  5 +
  drivers/gpu/drm/msm/msm_drv.h   |  7 ++-
  4 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..5d6ad1f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2412,7 +2412,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
drm_encoder *enc,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
-   int ret = 0;
+   int ret = 0, intf_i;
  
  	dpu_enc = to_dpu_encoder_virt(enc);
  
@@ -2424,13 +2424,16 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,

timer_setup(_enc->frame_done_timer,
dpu_encoder_frame_done_timeout, 0);
  
+	intf_i = disp_info->h_tile_instance[0];

if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
-   else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+   else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-   priv->dp[disp_info->h_tile_instance[0]]);
+   priv->dp[intf_i]);
+   disp_info->is_external = msm_dp_is_external(priv->dp[intf_i]);
+   }


I will quite myself: "And DSI can be pluggable too. Please enumerate 
connector types here rather than doing that in DP driver."

Your s/pluggable/external/ doesn't fix the issue.

  
  	INIT_DELAYED_WORK(_enc->delayed_off_work,

dpu_encoder_off_work);
@@ -2455,6 +2458,17 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
drm_encoder *enc,
  
  }
  
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc)

+{
+   struct dpu_encoder_virt *dpu_enc;
+
+   if (!drm_enc)
+   return false;
+
+   dpu_enc = to_dpu_encoder_virt(drm_enc);
+   return dpu_enc->disp_info.is_external;
+}
+
  struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
int drm_enc_mode)
  {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..43f0d8b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -25,16 +25,18 @@
   * @num_of_h_tiles: Number of horizontal tiles in case of split interface
   * @h_tile_instance:Controller instance used per tile. Number of elements 
is
   *  based on num_of_h_tiles
- * @is_cmd_modeBoolean to indicate if the CMD mode is requested
+ * @is_cmd_mode:Boolean to indicate if the CMD mode is requested
+ * @is_external:Boolean to indicate if the intf is external
   * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *  used instead of panel TE in cmd mode panels
- * @dsc:   DSC configuration data for DSC-enabled displays
+ *  used instead of panel TE in cmd mode panels
+ * @dsc:DSC configuration data for DSC-enabled displays
   */
  struct msm_display_info {
int intf_type;
uint32_t num_of_h_tiles;
uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
bool is_cmd_mode;
+   bool is_external;
bool is_te_using_watchdog_timer;
struct drm_dsc_config *dsc;
  };
@@ -128,6 +130,12 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct 
drm_encoder *encoder);
  void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
  
  /**

+ * dpu_encoder_is_external - find if the encoder is of type external
+ * @drm_enc:Pointer to previously created drm encoder structure
+ */
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
+
+/**
   * dpu_encoder_init - initialize virtual encoder object
   * @dev:Pointer to drm device structure
   * @disp_info:  Pointer to display information structure
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index bfd0aef..0bbdcca5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1509,6 +1509,11 @@ bool msm_dp_wide_bus_available(const struct msm_dp 
*dp_display)
return dp->wide_bus_en;
  }
  
+bool

Re: [Freedreno] [PATCH 1/4] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder


On 09/11/2022 15:16, Kalyan Thota wrote:

Pin each crtc with one encoder. This arrangement will
disallow crtc switching between encoders and also will
facilitate to advertise certain features on crtc based
on encoder type.

Signed-off-by: Kalyan Thota 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..552a89c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,19 +798,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
max_crtc_count = min(max_crtc_count, primary_planes_idx);
  
  	/* Create one CRTC per encoder */

+   encoder = list_first_entry(&(dev)->mode_config.encoder_list,
+   struct drm_encoder, head);


Please use drm_for_each_encoder() here.


for (i = 0; i < max_crtc_count; i++) {
crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
-   if (IS_ERR(crtc)) {
+   if (IS_ERR(crtc) || IS_ERR_OR_NULL(encoder)) {


Why? Not to mention that the OR_NULL part is quite frequently a mistake.


ret = PTR_ERR(crtc);
return ret;
}
priv->crtcs[priv->num_crtcs++] = crtc;
+   encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
+   encoder = list_next_entry(encoder, head);
}
  
-	/* All CRTCs are compatible with all encoders */

-   drm_for_each_encoder(encoder, dev)
-   encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
-
return 0;
  }
  


--
With best wishes
Dmitry

[Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc

Add color management support for the crtc provided there are
enough dspps that can be allocated from the catalogue

Signed-off-by: Kalyan Thota 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h|  6 
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53 +
 4 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..6bd3a64 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -18,9 +18,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include "../../../drm_crtc_internal.h"
 
 #include "dpu_kms.h"
 #include "dpu_hw_lm.h"
@@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc *crtc)
spin_unlock_irqrestore(>event_lock, flags);
 }
 
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc)
+{
+   u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
+   u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
+   u32 degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id;
+
+   return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
+  drm_mode_obj_find_prop_id(>base, gamma_id) ||
+  drm_mode_obj_find_prop_id(>base, degamma_id));
+}
+
 enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
 {
struct drm_encoder *encoder;
@@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, 
struct drm_plane *plane,
 
drm_crtc_helper_add(crtc, _crtc_helper_funcs);
 
-   drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
-
/* save user friendly CRTC name for later */
snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..8bac395 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type 
dpu_crtc_get_client_type(
return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
 }
 
+/**
+ * dpu_crtc_has_color_enabled - check if the crtc has color management enabled
+ * @crtc: Pointer to drm crtc object
+ */
+bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
+
 #endif /* _DPU_CRTC_H_ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4c56a16..ebc3f25 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
 static struct msm_display_topology dpu_encoder_get_topology(
struct dpu_encoder_virt *dpu_enc,
struct dpu_kms *dpu_kms,
-   struct drm_display_mode *mode)
+   struct drm_display_mode *mode,
+   struct drm_crtc *crtc)
 {
struct msm_display_topology topology = {0};
int i, intf_count = 0;
@@ -573,11 +574,9 @@ static struct msm_display_topology 
dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
 
-   if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
-   if (dpu_kms->catalog->dspp &&
-   (dpu_kms->catalog->dspp_count >= topology.num_lm))
+   if (dpu_crtc_has_color_enabled(crtc) &&
+   (dpu_kms->catalog->dspp_count >= topology.num_lm))
topology.num_dspp = topology.num_lm;
-   }
 
topology.num_enc = 0;
topology.num_intf = intf_count;
@@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
}
}
 
-   topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
+   topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, 
crtc_state->crtc);
 
/* Reserve dynamic resources now. */
if (!ret) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 552a89c..47a73fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -537,6 +538,44 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, 
unsigned crtc_mask)
dpu_kms_wait_for_commit_done(kms, crtc);
 }
 
+/**
+ * _dpu_kms_possible_dspps - Evaluate how many dspps pairs can be facilitated
+ to enable color features for crtcs.
+ * @dpu_kms:Pointer to dpu kms structure
+ * Returns: count of dspp pairs
+ *
+ * Baring single entry, if atleast 2 dspps are available in the catalogue,
+ * then color can be

[Freedreno] [PATCH 3/4] drm/msm/disp/dpu1: helper function to determine if encoder is virtual

Add a helper function to determine if an encoder is of type
virtual.

Signed-off-by: Kalyan Thota 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d6ad1f..4c56a16 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2469,6 +2469,17 @@ bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
return dpu_enc->disp_info.is_external;
 }
 
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc)
+{
+   struct dpu_encoder_virt *dpu_enc;
+
+   if (!drm_enc)
+   return false;
+
+   dpu_enc = to_dpu_encoder_virt(drm_enc);
+   return (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_VIRTUAL);
+}
+
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
int drm_enc_mode)
 {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 43f0d8b..6ae3c62 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -136,6 +136,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder 
*encoder);
 bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
 
 /**
+ * dpu_encoder_is_virtual - find if the encoder is of type virtual.
+ * @drm_enc:Pointer to previously created drm encoder structure
+ */
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc);
+
+/**
  * dpu_encoder_init - initialize virtual encoder object
  * @dev:Pointer to drm device structure
  * @disp_info:  Pointer to display information structure
-- 
2.7.4

[Freedreno] [PATCH 2/4] drm/msm/disp/dpu1: populate disp_info if an interface is external

DRM encoder type is same for eDP and DP (DRM_MODE_ENCODER_TMDS)
populate is_external information in the disp_info so as to
differentiate between eDP and DP on the DPU encoder side.

Signed-off-by: Kalyan Thota 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +++---
 drivers/gpu/drm/msm/dp/dp_display.c |  5 +
 drivers/gpu/drm/msm/msm_drv.h   |  7 ++-
 4 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..5d6ad1f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2412,7 +2412,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
drm_encoder *enc,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
-   int ret = 0;
+   int ret = 0, intf_i;
 
dpu_enc = to_dpu_encoder_virt(enc);
 
@@ -2424,13 +2424,16 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
drm_encoder *enc,
timer_setup(_enc->frame_done_timer,
dpu_encoder_frame_done_timeout, 0);
 
+   intf_i = disp_info->h_tile_instance[0];
if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
-   else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+   else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
-   priv->dp[disp_info->h_tile_instance[0]]);
+   priv->dp[intf_i]);
+   disp_info->is_external = msm_dp_is_external(priv->dp[intf_i]);
+   }
 
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
@@ -2455,6 +2458,17 @@ int dpu_encoder_setup(struct drm_device *dev, struct 
drm_encoder *enc,
 
 }
 
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
+{
+   struct dpu_encoder_virt *dpu_enc;
+
+   if (!drm_enc)
+   return false;
+
+   dpu_enc = to_dpu_encoder_virt(drm_enc);
+   return dpu_enc->disp_info.is_external;
+}
+
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
int drm_enc_mode)
 {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..43f0d8b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -25,16 +25,18 @@
  * @num_of_h_tiles: Number of horizontal tiles in case of split interface
  * @h_tile_instance:Controller instance used per tile. Number of elements 
is
  *  based on num_of_h_tiles
- * @is_cmd_modeBoolean to indicate if the CMD mode is requested
+ * @is_cmd_mode:Boolean to indicate if the CMD mode is requested
+ * @is_external:Boolean to indicate if the intf is external
  * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *  used instead of panel TE in cmd mode panels
- * @dsc:   DSC configuration data for DSC-enabled displays
+ *  used instead of panel TE in cmd mode panels
+ * @dsc:DSC configuration data for DSC-enabled displays
  */
 struct msm_display_info {
int intf_type;
uint32_t num_of_h_tiles;
uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
bool is_cmd_mode;
+   bool is_external;
bool is_te_using_watchdog_timer;
struct drm_dsc_config *dsc;
 };
@@ -128,6 +130,12 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct 
drm_encoder *encoder);
 void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
 
 /**
+ * dpu_encoder_is_external - find if the encoder is of type external
+ * @drm_enc:Pointer to previously created drm encoder structure
+ */
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc);
+
+/**
  * dpu_encoder_init - initialize virtual encoder object
  * @dev:Pointer to drm device structure
  * @disp_info:  Pointer to display information structure
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index bfd0aef..0bbdcca5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1509,6 +1509,11 @@ bool msm_dp_wide_bus_available(const struct msm_dp 
*dp_display)
return dp->wide_bus_en;
 }
 
+bool msm_dp_is_external(const struct msm_dp *dp_display)
+{
+   return (dp_display->connector_type == DRM_MODE_CONNECTOR_DisplayPort);
+}
+
 void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 {
struct dp_display_private *dp;
diff

[Freedreno] [PATCH 1/4] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

Pin each crtc with one encoder. This arrangement will
disallow crtc switching between encoders and also will
facilitate to advertise certain features on crtc based
on encoder type.

Signed-off-by: Kalyan Thota 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..552a89c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,19 +798,19 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
max_crtc_count = min(max_crtc_count, primary_planes_idx);
 
/* Create one CRTC per encoder */
+   encoder = list_first_entry(&(dev)->mode_config.encoder_list,
+   struct drm_encoder, head);
for (i = 0; i < max_crtc_count; i++) {
crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
-   if (IS_ERR(crtc)) {
+   if (IS_ERR(crtc) || IS_ERR_OR_NULL(encoder)) {
ret = PTR_ERR(crtc);
return ret;
}
priv->crtcs[priv->num_crtcs++] = crtc;
+   encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
+   encoder = list_next_entry(encoder, head);
}
 
-   /* All CRTCs are compatible with all encoders */
-   drm_for_each_encoder(encoder, dev)
-   encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
-
return 0;
 }
 
-- 
2.7.4

[Freedreno] [v8] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280