date:20230726





On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:

As the DPU driver has switched to fetching data from MDSS driver, we can
now drop the UBWC and highest_bank_bit parts of the DPU hw catalog.

Signed-off-by: Dmitry Baryshkov 
---
  .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h   |  6 --
  .../drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h|  6 --
  .../drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h|  6 --
  .../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h   |  6 --
  .../drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h|  7 ---
  .../drm/msm/disp/dpu1/catalog/dpu_6_2_sc7180.h|  6 --
  .../drm/msm/disp/dpu1/catalog/dpu_6_3_sm6115.h|  7 ---
  .../drm/msm/disp/dpu1/catalog/dpu_6_5_qcm2290.h   |  5 -
  .../drm/msm/disp/dpu1/catalog/dpu_7_0_sm8350.h|  6 --
  .../drm/msm/disp/dpu1/catalog/dpu_7_2_sc7280.h|  7 ---
  .../drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h  |  7 ---
  .../drm/msm/disp/dpu1/catalog/dpu_8_1_sm8450.h|  7 ---
  .../drm/msm/disp/dpu1/catalog/dpu_9_0_sm8550.h|  6 --
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 15 ---
  14 files changed, 97 deletions(-)



Reviewed-by: Abhinav Kumar

Re: [Freedreno] [PATCH 5/6] drm/msm/dpu: use MDSS data for programming SSPP





On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:

Switch to using data from MDSS driver to program the SSPP fetch and UBWC
configuration.

Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  7 +--
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++-
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |  1 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c  |  3 ++-
  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h  |  2 ++
  6 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index cf70a9bd1034..bfd82c2921af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
  #include "dpu_hw_sspp.h"
  #include "dpu_kms.h"
  
+#include "msm_mdss.h"

+
  #include 
  
  #define DPU_FETCH_CONFIG_RESET_VALUE   0x0087

@@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct dpu_sw_pipe 
*pipe,
DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
DPU_FETCH_CONFIG_RESET_VALUE |
ctx->ubwc->highest_bank_bit << 18);


Does this needs to be protected with if ctx->ubwc check?


-   switch (ctx->ubwc->ubwc_version) {
-   case DPU_HW_UBWC_VER_10:
+   switch (ctx->ubwc->ubwc_enc_version) {
+   case UBWC_1_0:


The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the 
catalog for the encoder version in the first place? Because looking at 
the registers UBWC_x_x is the correct value.


If we cannot find the reason why, it should be noted in the commit text 
that the values we are using did change.



fast_clear = fmt->alpha_enable ? BIT(31) : 0;
DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
fast_clear | (ctx->ubwc->ubwc_swizzle & 
0x1) |
BIT(8) |
(ctx->ubwc->highest_bank_bit << 4));
break;
-   case DPU_HW_UBWC_VER_20:
+   case UBWC_2_0:
fast_clear = fmt->alpha_enable ? BIT(31) : 0;
DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
fast_clear | (ctx->ubwc->ubwc_swizzle) |
(ctx->ubwc->highest_bank_bit << 4));
break;
-   case DPU_HW_UBWC_VER_30:
+   case UBWC_3_0:
DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
BIT(30) | (ctx->ubwc->ubwc_swizzle) |
(ctx->ubwc->highest_bank_bit << 4));
break;
-   case DPU_HW_UBWC_VER_40:
+   case UBWC_4_0:
DPU_REG_WRITE(c, SSPP_UBWC_STATIC_CTRL,
DPU_FORMAT_IS_YUV(fmt) ? 0 : BIT(30));
break;
@@ -793,7 +795,9 @@ static const struct dpu_sspp_cfg *_sspp_offset(enum 
dpu_sspp sspp,
  }
  
  struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,

-   void __iomem *addr, const struct dpu_mdss_cfg *catalog)
+void __iomem *addr,
+const struct dpu_mdss_cfg *catalog,
+const struct msm_mdss_data *mdss_data)
  {
struct dpu_hw_sspp *hw_pipe;
const struct dpu_sspp_cfg *cfg;
@@ -813,7 +817,7 @@ struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
  
  	/* Assign ops */

hw_pipe->catalog = catalog;
-   hw_pipe->ubwc = catalog->ubwc;
+   hw_pipe->ubwc = mdss_data;
hw_pipe->idx = idx;
hw_pipe->cap = cfg;
_setup_layer_ops(hw_pipe, hw_pipe->cap->features);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index 74b98b6b3bc3..8d4ae9d24674 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -351,7 +351,7 @@ struct dpu_hw_sspp {
struct dpu_hw_blk base;
struct dpu_hw_blk_reg_map hw;
const struct dpu_mdss_cfg *catalog;
-   const struct dpu_ubwc_cfg *ubwc;
+   const struct msm_mdss_data *ubwc;
  
  	/* Pipe */

enum dpu_sspp idx;
@@ -368,9 +368,12 @@ struct dpu_kms;
   * @idx:  Pipe index for which driver object is required
   * @addr: Mapped register io address of MDP
   * @catalog : Pointer to mdss catalog data
+ * @mdss_data: UBWC / MDSS configuration data
   */
  struct dpu_hw_sspp *dpu_hw_sspp_init(enum dpu_sspp idx,
-   void __iomem *addr, const struct dpu_mdss_cfg *catalog);
+void __iomem

Re: [Freedreno] [PATCH 4/6] drm/msm/mdss: populate missing data





On 7/26/2023 3:58 PM, Dmitry Baryshkov wrote:

On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar  wrote:




On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:

As we are going to use MDSS data for DPU programming, populate missing
MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
programming, so skip them.



Can you pls point me to the downstream references you used for msm8998?


msm-3.18, drivers/video/msm/mdss/mdss_mdp.c

See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP
1.07 (msm8996), 1.14 (msm8937) / 1.16  (msm8953) and 3.0 (msm8998).



It sets has_ubwc but I still cannot locate where it says version is 1.0.
Because the 0x58 register reads 0 and not 1 for msm8998.


Was that just taken from catalog? If so I would ask for the reference
for the catalog too.

As per the register the decoder version is 0x0 and not 1.

Even encoder version is 0 from what i see and not 1. Thats why a
dec_version of UBWC_1_0 is not doing anything i assume.

Some additional questions:

1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc
programming need to be protected by if (ctx->ubwc) now ?


It's hard to discuss the question which is irrelevant for this patch.
Nevertheless, yes, it needs to be protected because e.g. qcm2290
doesn't have UBWC support.



Sorry but your commit text made me look into this function and wonder 
now what happens to that code. But I will continue this in this next 
patch so that you can see the code and the question together.



2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
catalog for the encoder version in the first place? Because looking at
the registers UBWC_x_x is the correct value.


Huh. This definitely should be asked next to the code that you wish to
discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU
revision.



Sure, so again the next patch.





Signed-off-by: Dmitry Baryshkov 
---
   drivers/gpu/drm/msm/msm_mdss.c | 21 +++--
   1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index ed836c659688..9bb7be4b9ebb 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
* UBWC_n and the rest of params comes from hw data.
*/
   switch (msm_mdss->mdss_data->ubwc_dec_version) {
+ case 0: /* no UBWC */
+ case UBWC_1_0:
+ /* do nothing */
+ break;
   case UBWC_2_0:
   msm_mdss_setup_ubwc_dec_20(msm_mdss);
   break;
@@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
   return 0;
   }

+static const struct msm_mdss_data msm8998_data = {
+ .ubwc_enc_version = UBWC_1_0,
+ .ubwc_dec_version = UBWC_1_0,
+ .highest_bank_bit = 1,
+};
+
+static const struct msm_mdss_data qcm2290_data = {
+ /* no UBWC */
+ .highest_bank_bit = 0x2,
+};
+
   static const struct msm_mdss_data sc7180_data = {
   .ubwc_enc_version = UBWC_2_0,
   .ubwc_dec_version = UBWC_2_0,
   .ubwc_static = 0x1e,
+ .highest_bank_bit = 0x3,
   };

   static const struct msm_mdss_data sc7280_data = {
@@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
   .ubwc_dec_version = UBWC_2_0,
   .ubwc_swizzle = 7,
   .ubwc_static = 0x11f,
+ .highest_bank_bit = 0x1,
   };

   static const struct msm_mdss_data sm8250_data = {
@@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {

   static const struct of_device_id mdss_dt_match[] = {
   { .compatible = "qcom,mdss" },
- { .compatible = "qcom,msm8998-mdss" },
- { .compatible = "qcom,qcm2290-mdss" },
+ { .compatible = "qcom,msm8998-mdss", .data = _data },
+ { .compatible = "qcom,qcm2290-mdss", .data = _data },
   { .compatible = "qcom,sdm845-mdss", .data = _data },
   { .compatible = "qcom,sc7180-mdss", .data = _data },
   { .compatible = "qcom,sc7280-mdss", .data = _data },

Re: [Freedreno] [PATCH 4/6] drm/msm/mdss: populate missing data

On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar  wrote:
>
>
>
> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> > As we are going to use MDSS data for DPU programming, populate missing
> > MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
> > programming, so skip them.
> >
>
> Can you pls point me to the downstream references you used for msm8998?

msm-3.18, drivers/video/msm/mdss/mdss_mdp.c

See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP
1.07 (msm8996), 1.14 (msm8937) / 1.16  (msm8953) and 3.0 (msm8998).

> Was that just taken from catalog? If so I would ask for the reference
> for the catalog too.
>
> As per the register the decoder version is 0x0 and not 1.
>
> Even encoder version is 0 from what i see and not 1. Thats why a
> dec_version of UBWC_1_0 is not doing anything i assume.
>
> Some additional questions:
>
> 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc
> programming need to be protected by if (ctx->ubwc) now ?

It's hard to discuss the question which is irrelevant for this patch.
Nevertheless, yes, it needs to be protected because e.g. qcm2290
doesn't have UBWC support.

> 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
> What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
> catalog for the encoder version in the first place? Because looking at
> the registers UBWC_x_x is the correct value.

Huh. This definitely should be asked next to the code that you wish to
discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU
revision.

>
>
> > Signed-off-by: Dmitry Baryshkov 
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 21 +++--
> >   1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index ed836c659688..9bb7be4b9ebb 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >* UBWC_n and the rest of params comes from hw data.
> >*/
> >   switch (msm_mdss->mdss_data->ubwc_dec_version) {
> > + case 0: /* no UBWC */
> > + case UBWC_1_0:
> > + /* do nothing */
> > + break;
> >   case UBWC_2_0:
> >   msm_mdss_setup_ubwc_dec_20(msm_mdss);
> >   break;
> > @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
> >   return 0;
> >   }
> >
> > +static const struct msm_mdss_data msm8998_data = {
> > + .ubwc_enc_version = UBWC_1_0,
> > + .ubwc_dec_version = UBWC_1_0,
> > + .highest_bank_bit = 1,
> > +};
> > +
> > +static const struct msm_mdss_data qcm2290_data = {
> > + /* no UBWC */
> > + .highest_bank_bit = 0x2,
> > +};
> > +
> >   static const struct msm_mdss_data sc7180_data = {
> >   .ubwc_enc_version = UBWC_2_0,
> >   .ubwc_dec_version = UBWC_2_0,
> >   .ubwc_static = 0x1e,
> > + .highest_bank_bit = 0x3,
> >   };
> >
> >   static const struct msm_mdss_data sc7280_data = {
> > @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
> >   .ubwc_dec_version = UBWC_2_0,
> >   .ubwc_swizzle = 7,
> >   .ubwc_static = 0x11f,
> > + .highest_bank_bit = 0x1,
> >   };
> >
> >   static const struct msm_mdss_data sm8250_data = {
> > @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
> >
> >   static const struct of_device_id mdss_dt_match[] = {
> >   { .compatible = "qcom,mdss" },
> > - { .compatible = "qcom,msm8998-mdss" },
> > - { .compatible = "qcom,qcm2290-mdss" },
> > + { .compatible = "qcom,msm8998-mdss", .data = _data },
> > + { .compatible = "qcom,qcm2290-mdss", .data = _data },
> >   { .compatible = "qcom,sdm845-mdss", .data = _data },
> >   { .compatible = "qcom,sc7180-mdss", .data = _data },
> >   { .compatible = "qcom,sc7280-mdss", .data = _data },



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

On Wed, Jul 26, 2023 at 3:33 PM Dmitry Baryshkov
 wrote:
>
> On Thu, 27 Jul 2023 at 01:04, Rob Clark  wrote:
> >
> > On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
> >  wrote:
> > >
> > > On 26/07/2023 23:11, Rob Clark wrote:
> > > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > > >  wrote:
> > > >>
> > > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark  wrote:
> > > >>>
> > > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen 
> > > >>>  wrote:
> > > 
> > >  On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > >
> > > > On 07/07/2023 00:10, Rob Clark wrote:
> > > >> From: Rob Clark 
> > > >>
> > > >> There are cases where there are differences due to SoC integration.
> > > >> Such as cache-coherency support, and (in the next patch) e-fuse to
> > > >> speedbin mappings.
> > > >
> > > > I have the feeling that we are trying to circumvent the way DT 
> > > > works. I'd
> > > > suggest adding explicit SoC-compatible strings to Adreno bindings 
> > > > and then
> > > > using of_device_id::data and then of_device_get_match_data().
> > > >
> > >  Just thinking, then how about a unique compatible string which we 
> > >  match
> > >  to identify gpu->info and drop chip-id check completely here?
> > > >>>
> > > >>> Ok, I think we could do this, so something like:
> > > >>>
> > > >>>compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", 
> > > >>> "qcom,adreno"
> > > >>>
> > > >>> ?
> > > >>>
> > > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > > >>> or sm6375, so I suppose we could get away with this change
> > > >>
> > > >> I think we can even skip the 619.0 part in the SoC compat string.
> > > >> So it will be:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > > >>
> > > >> In future we can drop the chipid part completely and handle that as a
> > > >> part of SoC data:
> > > >>
> > > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > > >>
> > > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > > >>
> > > >
> > > > I don't think we can do that, there are cases where the same SoC had
> > > > multiple revisions of adreno.
> > >
> > > Is that the case for the production versions of the SoC? In other
> > > subsystems what we usually do is that we add support only for the latest
> > > SoC revision (which would probably mean the latest GPU patch revision).
> > > Previous GPU revisions can be added in the following way (pure example):
> > >
> > > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > > sample
> > > qcom,sm4350-v1-adreno -> 6,1,9,0
> > >
> >
> > My recollection was that nexus4 shipped with an early version of 8064
> > which needed userspace workarounds that later 8064 did not.  Not sure
> > if that is the only such example, but it is one that userspace needed
> > to be aware of.
>
> Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.
>
> And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

I no longer have a n4 that boots.. but if I did both it and the later
ones should work properly if they expose the appropriate chip id

I do still prefer parsing the chip-id out of the compatible.  It
avoids needing separate table entries just to have a different
chip-id.  Maybe the scheme that is used elsewhere makes sense when it
is only the kernel that needs to be aware of the device-id.  And maybe
we could just done matching based on compat-id in userspace as well,
but (a) msm and freedreno pre-date dt, and (b) that ship has already
sailed.

BR,
-R

> >
> > Anyways, future things, it sounds like we'll be able to read the id
> > from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> > so I don't want to change any of the existing compat strings.
>
> I think so too. Current compat strings should stay.
>
> >
> > BR,
> > -R
> >
> > > >  We could possibly do that with future
> > > > things where we can read the chip-id from fw.
> > > >
> > > > What we _could_ do at the expense of making the compatible parsing a
> > > > tiny bit more complex is something like:
> > > >
> > > > compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > > >
> > > > BR,
> > > > -R
> > > >
> > > >>>
> > > >>> BR,
> > > >>> -R
> > > >>>
> > >  -Akhil
> > > 
> > > >>
> > > >> Signed-off-by: Rob Clark 
> > > >> ---
> > > >>drivers/gpu/drm/msm/adreno/adreno_device.c | 34 
> > > >> +++---
> > > >>drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
> > > >>2 files changed, 31 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > > >> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >> index 3c531da417b9..e62bc895a31f 100644
> > > >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > >> +++

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

On Thu, 27 Jul 2023 at 01:04, Rob Clark  wrote:
>
> On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
>  wrote:
> >
> > On 26/07/2023 23:11, Rob Clark wrote:
> > > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> > >  wrote:
> > >>
> > >> On Wed, 26 Jul 2023 at 21:28, Rob Clark  wrote:
> > >>>
> > >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen 
> > >>>  wrote:
> > 
> >  On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > >
> > > On 07/07/2023 00:10, Rob Clark wrote:
> > >> From: Rob Clark 
> > >>
> > >> There are cases where there are differences due to SoC integration.
> > >> Such as cache-coherency support, and (in the next patch) e-fuse to
> > >> speedbin mappings.
> > >
> > > I have the feeling that we are trying to circumvent the way DT works. 
> > > I'd
> > > suggest adding explicit SoC-compatible strings to Adreno bindings and 
> > > then
> > > using of_device_id::data and then of_device_get_match_data().
> > >
> >  Just thinking, then how about a unique compatible string which we match
> >  to identify gpu->info and drop chip-id check completely here?
> > >>>
> > >>> Ok, I think we could do this, so something like:
> > >>>
> > >>>compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", 
> > >>> "qcom,adreno"
> > >>>
> > >>> ?
> > >>>
> > >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> > >>> or sm6375, so I suppose we could get away with this change
> > >>
> > >> I think we can even skip the 619.0 part in the SoC compat string.
> > >> So it will be:
> > >>
> > >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> > >>
> > >> In future we can drop the chipid part completely and handle that as a
> > >> part of SoC data:
> > >>
> > >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> > >>
> > >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> > >>
> > >
> > > I don't think we can do that, there are cases where the same SoC had
> > > multiple revisions of adreno.
> >
> > Is that the case for the production versions of the SoC? In other
> > subsystems what we usually do is that we add support only for the latest
> > SoC revision (which would probably mean the latest GPU patch revision).
> > Previous GPU revisions can be added in the following way (pure example):
> >
> > qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> > sample
> > qcom,sm4350-v1-adreno -> 6,1,9,0
> >
>
> My recollection was that nexus4 shipped with an early version of 8064
> which needed userspace workarounds that later 8064 did not.  Not sure
> if that is the only such example, but it is one that userspace needed
> to be aware of.

Good question. I don't have nexus4, and both nexus7 and ifc6410 work fine.

And this is a perfect use case for "qcom,apq8064-v1.1-adreno" compat string.

>
> Anyways, future things, it sounds like we'll be able to read the id
> from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
> so I don't want to change any of the existing compat strings.

I think so too. Current compat strings should stay.

>
> BR,
> -R
>
> > >  We could possibly do that with future
> > > things where we can read the chip-id from fw.
> > >
> > > What we _could_ do at the expense of making the compatible parsing a
> > > tiny bit more complex is something like:
> > >
> > > compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> > >
> > > BR,
> > > -R
> > >
> > >>>
> > >>> BR,
> > >>> -R
> > >>>
> >  -Akhil
> > 
> > >>
> > >> Signed-off-by: Rob Clark 
> > >> ---
> > >>drivers/gpu/drm/msm/adreno/adreno_device.c | 34 
> > >> +++---
> > >>drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
> > >>2 files changed, 31 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > >> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > >> index 3c531da417b9..e62bc895a31f 100644
> > >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > >> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > >>  .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >>  .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > >>  .init = a6xx_gpu_init,
> > >> +   }, {
> > >> +   .machine = "qcom,sm4350",
> > >> +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > >> +   .revn = 619,
> > >> +   .fw = {
> > >> +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > >> +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > >> +   },
> > >> +   .gmem = SZ_512K,
> > >> +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > >> +   .init = a6xx_gpu_init,
> > >> +   .zapfw = "a615_zap.mdt",
> > >> +

Re: [Freedreno] [PATCH 4/6] drm/msm/mdss: populate missing data





On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:

As we are going to use MDSS data for DPU programming, populate missing
MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
programming, so skip them.



Can you pls point me to the downstream references you used for msm8998?

Was that just taken from catalog? If so I would ask for the reference 
for the catalog too.


As per the register the decoder version is 0x0 and not 1.

Even encoder version is 0 from what i see and not 1. Thats why a 
dec_version of UBWC_1_0 is not doing anything i assume.


Some additional questions:

1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc 
programming need to be protected by if (ctx->ubwc) now ?


2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the 
catalog for the encoder version in the first place? Because looking at 
the registers UBWC_x_x is the correct value.




Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/msm_mdss.c | 21 +++--
  1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index ed836c659688..9bb7be4b9ebb 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 * UBWC_n and the rest of params comes from hw data.
 */
switch (msm_mdss->mdss_data->ubwc_dec_version) {
+   case 0: /* no UBWC */
+   case UBWC_1_0:
+   /* do nothing */
+   break;
case UBWC_2_0:
msm_mdss_setup_ubwc_dec_20(msm_mdss);
break;
@@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
return 0;
  }
  
+static const struct msm_mdss_data msm8998_data = {

+   .ubwc_enc_version = UBWC_1_0,
+   .ubwc_dec_version = UBWC_1_0,
+   .highest_bank_bit = 1,
+};
+
+static const struct msm_mdss_data qcm2290_data = {
+   /* no UBWC */
+   .highest_bank_bit = 0x2,
+};
+
  static const struct msm_mdss_data sc7180_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_static = 0x1e,
+   .highest_bank_bit = 0x3,
  };
  
  static const struct msm_mdss_data sc7280_data = {

@@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = 7,
.ubwc_static = 0x11f,
+   .highest_bank_bit = 0x1,
  };
  
  static const struct msm_mdss_data sm8250_data = {

@@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
  
  static const struct of_device_id mdss_dt_match[] = {

{ .compatible = "qcom,mdss" },
-   { .compatible = "qcom,msm8998-mdss" },
-   { .compatible = "qcom,qcm2290-mdss" },
+   { .compatible = "qcom,msm8998-mdss", .data = _data },
+   { .compatible = "qcom,qcm2290-mdss", .data = _data },
{ .compatible = "qcom,sdm845-mdss", .data = _data },
{ .compatible = "qcom,sc7180-mdss", .data = _data },
{ .compatible = "qcom,sc7280-mdss", .data = _data },

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

On Wed, Jul 26, 2023 at 2:43 PM Dmitry Baryshkov
 wrote:
>
> On 26/07/2023 23:11, Rob Clark wrote:
> > On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
> >  wrote:
> >>
> >> On Wed, 26 Jul 2023 at 21:28, Rob Clark  wrote:
> >>>
> >>> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen  
> >>> wrote:
> 
>  On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> >
> > On 07/07/2023 00:10, Rob Clark wrote:
> >> From: Rob Clark 
> >>
> >> There are cases where there are differences due to SoC integration.
> >> Such as cache-coherency support, and (in the next patch) e-fuse to
> >> speedbin mappings.
> >
> > I have the feeling that we are trying to circumvent the way DT works. 
> > I'd
> > suggest adding explicit SoC-compatible strings to Adreno bindings and 
> > then
> > using of_device_id::data and then of_device_get_match_data().
> >
>  Just thinking, then how about a unique compatible string which we match
>  to identify gpu->info and drop chip-id check completely here?
> >>>
> >>> Ok, I think we could do this, so something like:
> >>>
> >>>compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", 
> >>> "qcom,adreno"
> >>>
> >>> ?
> >>>
> >>> It looks like we don't have gpu dt bits upstream yet for either sm4350
> >>> or sm6375, so I suppose we could get away with this change
> >>
> >> I think we can even skip the 619.0 part in the SoC compat string.
> >> So it will be:
> >>
> >> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
> >>
> >> In future we can drop the chipid part completely and handle that as a
> >> part of SoC data:
> >>
> >> compatible = "qcom,sm4350-adreno", "qcom,adreno";
> >>
> >> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
> >>
> >
> > I don't think we can do that, there are cases where the same SoC had
> > multiple revisions of adreno.
>
> Is that the case for the production versions of the SoC? In other
> subsystems what we usually do is that we add support only for the latest
> SoC revision (which would probably mean the latest GPU patch revision).
> Previous GPU revisions can be added in the following way (pure example):
>
> qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial
> sample
> qcom,sm4350-v1-adreno -> 6,1,9,0
>

My recollection was that nexus4 shipped with an early version of 8064
which needed userspace workarounds that later 8064 did not.  Not sure
if that is the only such example, but it is one that userspace needed
to be aware of.

Anyways, future things, it sounds like we'll be able to read the id
from the hw/fw.  I'm not really a fan of breaking dtb fwd/bk compat,
so I don't want to change any of the existing compat strings.

BR,
-R

> >  We could possibly do that with future
> > things where we can read the chip-id from fw.
> >
> > What we _could_ do at the expense of making the compatible parsing a
> > tiny bit more complex is something like:
> >
> > compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"
> >
> > BR,
> > -R
> >
> >>>
> >>> BR,
> >>> -R
> >>>
>  -Akhil
> 
> >>
> >> Signed-off-by: Rob Clark 
> >> ---
> >>drivers/gpu/drm/msm/adreno/adreno_device.c | 34 
> >> +++---
> >>drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
> >>2 files changed, 31 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> >> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> index 3c531da417b9..e62bc895a31f 100644
> >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> >>  .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >>  .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> >>  .init = a6xx_gpu_init,
> >> +   }, {
> >> +   .machine = "qcom,sm4350",
> >> +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >> +   .revn = 619,
> >> +   .fw = {
> >> +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> >> +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> >> +   },
> >> +   .gmem = SZ_512K,
> >> +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >> +   .init = a6xx_gpu_init,
> >> +   .zapfw = "a615_zap.mdt",
> >> +   .hwcg = a615_hwcg,
> >> +   }, {
> >> +   .machine = "qcom,sm6375",
> >> +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> >> +   .revn = 619,
> >> +   .fw = {
> >> +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> >> +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> >> +   },
> >> +   .gmem = SZ_512K,
> >> +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> >> +   .init = a6xx_gpu_init,
>

Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU

On Sat, Jul 15, 2023 at 6:38 AM Konrad Dybcio  wrote:
>
> On 7.07.2023 18:08, Rob Clark wrote:
> > On Thu, Jul 6, 2023 at 5:25 PM Konrad Dybcio  
> > wrote:
> >>
> >> Apart from all these comments, I don't really see the point of this patch,
> >> other than trying to tie together Qualcomm's almost-meaningless chipids on
> >> a7xx into the picture..
> >>
> >> Since they can't even be read back from the hardware, I don't think trying
> >> to force them into the upstream kernel makes any sense.
> >
> > Sure, we _could_ pick our own arbitrary identifiers, we don't have to
> > align with kgsl.  But that would be a super huge PITA for mesa, which
> > has support for both kernels.
> Perhaps I'm biased towards keeping this kind of stuff out of the kernel,
> but I'd say that Linux should decide on one logical path.
>
> In between us starting this discussion, Qualcomm managed to introduce
> yet another way of deciding what GPU is present on the system in their
> downstream driver[1]. We're at like 5 now. Do we wanna keep up each time?
> New ID rework for A8xx when that comes out one day?

[snip]

> [1] they now read parts of socinfo from smem and decide the CHIPID and
> speedbin based on that, but it's not available on most existing SoCs,
> that was thrown in with SOCID v17

This is actually exactly what we want.. something that we can read
from hw/fw and that blob userspace also uses (so we don't have to
worry about qc forgetting to change the id, etc)

BR,
-R

Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU


On 27/07/2023 00:44, Rob Clark wrote:

On Wed, Jul 26, 2023 at 2:38 PM Dmitry Baryshkov
 wrote:


On 27/07/2023 00:37, Rob Clark wrote:

On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
 wrote:


On 07/07/2023 00:10, Rob Clark wrote:

From: Rob Clark 

/* Helper for formating the chip_id in the way that userspace tools like
 * crashdec expect.
 */
#define ADRENO_CHIPID_FMT "u.%u.%u.%u"
-#define ADRENO_CHIPID_ARGS(_r) (_r).core, (_r).major, (_r).minor, (_r).patchid
+#define ADRENO_CHIPID_ARGS(_c) \
+ (((_c) >> 24) & 0xff), \
+ (((_c) >> 16) & 0xff), \
+ (((_c) >> 8)  & 0xff), \
+ ((_c) & 0xff)


So, we still have some meaning for chipid?


Only enough to do the inverse of what userspace does when parsing
devcoredump to construct chip-id.  Basically it is just a different
way to represent a 32b #


What about passing it in the direct form? The macro adds assumptions.


It is uabi

I wouldn't call it adding assumptions as much as just a funny way to
format a number.  In some cases it might work out to something that
vaguely resembles a marketing name (6.3.0.2), in other cases it won't
(12.34.56.78).. it's just formatting a 32b # to match the way existing
userspace parses it


I see. Sounds fine then.

--
With best wishes
Dmitry

Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU

On Wed, Jul 26, 2023 at 2:38 PM Dmitry Baryshkov
 wrote:
>
> On 27/07/2023 00:37, Rob Clark wrote:
> > On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
> >  wrote:
> >>
> >> On 07/07/2023 00:10, Rob Clark wrote:
> >>> From: Rob Clark 
> >>>
> >>>/* Helper for formating the chip_id in the way that userspace tools 
> >>> like
> >>> * crashdec expect.
> >>> */
> >>>#define ADRENO_CHIPID_FMT "u.%u.%u.%u"
> >>> -#define ADRENO_CHIPID_ARGS(_r) (_r).core, (_r).major, (_r).minor, 
> >>> (_r).patchid
> >>> +#define ADRENO_CHIPID_ARGS(_c) \
> >>> + (((_c) >> 24) & 0xff), \
> >>> + (((_c) >> 16) & 0xff), \
> >>> + (((_c) >> 8)  & 0xff), \
> >>> + ((_c) & 0xff)
> >>
> >> So, we still have some meaning for chipid?
> >
> > Only enough to do the inverse of what userspace does when parsing
> > devcoredump to construct chip-id.  Basically it is just a different
> > way to represent a 32b #
>
> What about passing it in the direct form? The macro adds assumptions.

It is uabi

I wouldn't call it adding assumptions as much as just a funny way to
format a number.  In some cases it might work out to something that
vaguely resembles a marketing name (6.3.0.2), in other cases it won't
(12.34.56.78).. it's just formatting a 32b # to match the way existing
userspace parses it

BR,
-R

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries


On 26/07/2023 23:11, Rob Clark wrote:

On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
 wrote:


On Wed, 26 Jul 2023 at 21:28, Rob Clark  wrote:


On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen  wrote:


On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:


On 07/07/2023 00:10, Rob Clark wrote:

From: Rob Clark 

There are cases where there are differences due to SoC integration.
Such as cache-coherency support, and (in the next patch) e-fuse to
speedbin mappings.


I have the feeling that we are trying to circumvent the way DT works. I'd
suggest adding explicit SoC-compatible strings to Adreno bindings and then
using of_device_id::data and then of_device_get_match_data().


Just thinking, then how about a unique compatible string which we match
to identify gpu->info and drop chip-id check completely here?


Ok, I think we could do this, so something like:

   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350
or sm6375, so I suppose we could get away with this change


I think we can even skip the 619.0 part in the SoC compat string.
So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a
part of SoC data:

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)



I don't think we can do that, there are cases where the same SoC had
multiple revisions of adreno.


Is that the case for the production versions of the SoC? In other 
subsystems what we usually do is that we add support only for the latest 
SoC revision (which would probably mean the latest GPU patch revision). 
Previous GPU revisions can be added in the following way (pure example):


qcom,sm4350-adreno -> 6,1,9,1 // assuming v2.0 or v1.1 is the commercial 
sample

qcom,sm4350-v1-adreno -> 6,1,9,0


 We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a
tiny bit more complex is something like:

compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R



BR,
-R


-Akhil



Signed-off-by: Rob Clark 
---
   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
   2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 3c531da417b9..e62bc895a31f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
 .inactive_period = DRM_MSM_INACTIVE_PERIOD,
 .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
 .init = a6xx_gpu_init,
+   }, {
+   .machine = "qcom,sm4350",
+   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+   .revn = 619,
+   .fw = {
+   [ADRENO_FW_SQE] = "a630_sqe.fw",
+   [ADRENO_FW_GMU] = "a619_gmu.bin",
+   },
+   .gmem = SZ_512K,
+   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+   .init = a6xx_gpu_init,
+   .zapfw = "a615_zap.mdt",
+   .hwcg = a615_hwcg,
+   }, {
+   .machine = "qcom,sm6375",
+   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
+   .revn = 619,
+   .fw = {
+   [ADRENO_FW_SQE] = "a630_sqe.fw",
+   [ADRENO_FW_GMU] = "a619_gmu.bin",
+   },
+   .gmem = SZ_512K,
+   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
+   .init = a6xx_gpu_init,
+   .zapfw = "a615_zap.mdt",
+   .hwcg = a615_hwcg,
 }, {
 .rev = ADRENO_REV(6, 1, 9, ANY_ID),
 .revn = 619,
@@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct adreno_rev rev)
 /* identify gpu: */
 for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
 const struct adreno_info *info = [i];
+   if (info->machine && !of_machine_is_compatible(info->machine))
+   continue;
 if (adreno_cmp_rev(info->rev, rev))
 return info;
 }
@@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct device 
*master, void *data)
 config.rev.minor, config.rev.patchid);
 priv->is_a2xx = config.rev.core == 2;
+   priv->has_cached_coherent =
+   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
 gpu = info->init(drm);
 if (IS_ERR(gpu)) {
@@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct device 
*master, void *data)
 if (ret)
 return ret;
-   priv->has_cached_coherent =
-   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
-   !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
-
 return 0;
   }
diff --git

Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU


On 27/07/2023 00:37, Rob Clark wrote:

On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
 wrote:


On 07/07/2023 00:10, Rob Clark wrote:

From: Rob Clark 

Since the revision becomes an opaque identifier with future GPUs, move
away from treating different ranges of bits as having a given meaning.
This means that we need to explicitly list different patch revisions in
the device table.

Signed-off-by: Rob Clark 
---
   drivers/gpu/drm/msm/adreno/a4xx_gpu.c  |   2 +-
   drivers/gpu/drm/msm/adreno/a5xx_gpu.c  |  11 +-
   drivers/gpu/drm/msm/adreno/a5xx_power.c|   2 +-
   drivers/gpu/drm/msm/adreno/a6xx_gmu.c  |  13 ++-
   drivers/gpu/drm/msm/adreno/a6xx_gpu.c  |   9 +-
   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++---
   drivers/gpu/drm/msm/adreno/adreno_gpu.c|  16 +--
   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  51 
   8 files changed, 122 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index 715436cb3996..8b4cdf95f445 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
   gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x0022);
   /* Early A430's have a timing issue with SP/TP power collapse;
  disabling HW clock gating prevents it. */
- if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
+ if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
   gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
   else
   gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0x);
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index f0803e94ebe5..70d2b5342cd9 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
   struct msm_drm_private *priv = dev->dev_private;
   struct platform_device *pdev = priv->gpu_pdev;
   struct adreno_platform_config *config = pdev->dev.platform_data;
+ const struct adreno_info *info;
   struct a5xx_gpu *a5xx_gpu = NULL;
   struct adreno_gpu *adreno_gpu;
   struct msm_gpu *gpu;
@@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)

   nr_rings = 4;

- if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
+ /*
+  * Note that we wouldn't have been able to get this far if there is not
+  * a device table entry for this chip_id
+  */
+ info = adreno_find_info(config->chip_id);
+ if (WARN_ON(!info))
+ return ERR_PTR(-EINVAL);
+
+ if (info->revn == 510)
   nr_rings = 1;

   ret = adreno_gpu_init(dev, pdev, adreno_gpu, , nr_rings);
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c 
b/drivers/gpu/drm/msm/adreno/a5xx_power.c
index 0e63a1429189..7705f8010484 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
@@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)

   /* The battery current limiter isn't enabled for A540 */
   config = AGC_LM_CONFIG_BCL_DISABLED;
- config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
+ config |= adreno_patchid(adreno_gpu) << AGC_LM_CONFIG_GPU_VERSION_SHIFT;

   /* For now disable GPMU side throttling */
   config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index f1bb20574018..a9ba547a120c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, 
unsigned int state)
   gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
   (1 << 31) | (0xa << 18) | (0xa0));

- chipid = adreno_gpu->rev.core << 24;
- chipid |= adreno_gpu->rev.major << 16;
- chipid |= adreno_gpu->rev.minor << 12;
- chipid |= adreno_gpu->rev.patchid << 8;
+ /* Note that the GMU has a slightly different layout for
+  * chip_id, for whatever reason, so a bit of massaging
+  * is needed.  The upper 16b are the same, but minor and
+  * patchid are packed in four bits each with the lower
+  * 8b unused:
+  */
+ chipid  = adreno_gpu->chip_id & 0x;
+ chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
+ chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */


I'd beg for explicit FIELD_GET and FIELD_PREP here.



   gmu_write(gmu, REG_A6XX_GMU_HFI_SFR_ADDR, chipid);

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 77b23c004b94..ed075729ca09 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2344,10 +2344,13 @@ struct msm_gpu *a6xx_gpu_init(struct

Re: [Freedreno] [PATCH 12/12] drm/msm/adreno: Switch to chip-id for identifying GPU

On Thu, Jul 6, 2023 at 8:45 PM Dmitry Baryshkov
 wrote:
>
> On 07/07/2023 00:10, Rob Clark wrote:
> > From: Rob Clark 
> >
> > Since the revision becomes an opaque identifier with future GPUs, move
> > away from treating different ranges of bits as having a given meaning.
> > This means that we need to explicitly list different patch revisions in
> > the device table.
> >
> > Signed-off-by: Rob Clark 
> > ---
> >   drivers/gpu/drm/msm/adreno/a4xx_gpu.c  |   2 +-
> >   drivers/gpu/drm/msm/adreno/a5xx_gpu.c  |  11 +-
> >   drivers/gpu/drm/msm/adreno/a5xx_power.c|   2 +-
> >   drivers/gpu/drm/msm/adreno/a6xx_gmu.c  |  13 ++-
> >   drivers/gpu/drm/msm/adreno/a6xx_gpu.c  |   9 +-
> >   drivers/gpu/drm/msm/adreno/adreno_device.c | 128 ++---
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.c|  16 +--
> >   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  51 
> >   8 files changed, 122 insertions(+), 110 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c 
> > b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > index 715436cb3996..8b4cdf95f445 100644
> > --- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
> > @@ -145,7 +145,7 @@ static void a4xx_enable_hwcg(struct msm_gpu *gpu)
> >   gpu_write(gpu, REG_A4XX_RBBM_CLOCK_DELAY_HLSQ, 0x0022);
> >   /* Early A430's have a timing issue with SP/TP power collapse;
> >  disabling HW clock gating prevents it. */
> > - if (adreno_is_a430(adreno_gpu) && adreno_gpu->rev.patchid < 2)
> > + if (adreno_is_a430(adreno_gpu) && adreno_patchid(adreno_gpu) < 2)
> >   gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0);
> >   else
> >   gpu_write(gpu, REG_A4XX_RBBM_CLOCK_CTL, 0x);
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
> > b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > index f0803e94ebe5..70d2b5342cd9 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> > @@ -1744,6 +1744,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> >   struct msm_drm_private *priv = dev->dev_private;
> >   struct platform_device *pdev = priv->gpu_pdev;
> >   struct adreno_platform_config *config = pdev->dev.platform_data;
> > + const struct adreno_info *info;
> >   struct a5xx_gpu *a5xx_gpu = NULL;
> >   struct adreno_gpu *adreno_gpu;
> >   struct msm_gpu *gpu;
> > @@ -1770,7 +1771,15 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> >
> >   nr_rings = 4;
> >
> > - if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
> > + /*
> > +  * Note that we wouldn't have been able to get this far if there is 
> > not
> > +  * a device table entry for this chip_id
> > +  */
> > + info = adreno_find_info(config->chip_id);
> > + if (WARN_ON(!info))
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (info->revn == 510)
> >   nr_rings = 1;
> >
> >   ret = adreno_gpu_init(dev, pdev, adreno_gpu, , nr_rings);
> > diff --git a/drivers/gpu/drm/msm/adreno/a5xx_power.c 
> > b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > index 0e63a1429189..7705f8010484 100644
> > --- a/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > +++ b/drivers/gpu/drm/msm/adreno/a5xx_power.c
> > @@ -179,7 +179,7 @@ static void a540_lm_setup(struct msm_gpu *gpu)
> >
> >   /* The battery current limiter isn't enabled for A540 */
> >   config = AGC_LM_CONFIG_BCL_DISABLED;
> > - config |= adreno_gpu->rev.patchid << AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> > + config |= adreno_patchid(adreno_gpu) << 
> > AGC_LM_CONFIG_GPU_VERSION_SHIFT;
> >
> >   /* For now disable GPMU side throttling */
> >   config |= AGC_LM_CONFIG_THROTTLE_DISABLE;
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c 
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index f1bb20574018..a9ba547a120c 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -790,10 +790,15 @@ static int a6xx_gmu_fw_start(struct a6xx_gmu *gmu, 
> > unsigned int state)
> >   gmu_write(gmu, REG_A6XX_GMU_AHB_FENCE_RANGE_0,
> >   (1 << 31) | (0xa << 18) | (0xa0));
> >
> > - chipid = adreno_gpu->rev.core << 24;
> > - chipid |= adreno_gpu->rev.major << 16;
> > - chipid |= adreno_gpu->rev.minor << 12;
> > - chipid |= adreno_gpu->rev.patchid << 8;
> > + /* Note that the GMU has a slightly different layout for
> > +  * chip_id, for whatever reason, so a bit of massaging
> > +  * is needed.  The upper 16b are the same, but minor and
> > +  * patchid are packed in four bits each with the lower
> > +  * 8b unused:
> > +  */
> > + chipid  = adreno_gpu->chip_id & 0x;
> > + chipid |= (adreno_gpu->chip_id << 4) & 0xf000; /* minor */
> > + chipid |= (adreno_gpu->chip_id << 8) & 0x0f00; /* patchid */
>
> I'd beg for explicit FIELD_GET and FIELD_PREP here.
>
> >
> >

Re: [Freedreno] [PATCH 3/6] drm/msm/mdss: export UBWC data

On Thu, 27 Jul 2023 at 00:21, Abhinav Kumar  wrote:
>
>
>
> On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> > DPU programming requires knowledge of some of UBWC parameters. This
> > results in duplication of UBWC data between MDSS and DPU drivers. Export
> > the required data from MDSS driver.
> >
> > Signed-off-by: Dmitry Baryshkov 
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 30 +-
> >   drivers/gpu/drm/msm/msm_mdss.h | 27 +++
> >   2 files changed, 40 insertions(+), 17 deletions(-)
> >   create mode 100644 drivers/gpu/drm/msm/msm_mdss.h
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index d1e57099b517..ed836c659688 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -13,7 +13,7 @@
> >   #include 
> >   #include 
> >
> > -#include "msm_drv.h"
> > +#include "msm_mdss.h"
> >   #include "msm_kms.h"
> >
> >   #define HW_REV  0x0
> > @@ -26,16 +26,6 @@
> >
> >   #define MIN_IB_BW   4UL /* Min ib vote 400MB */
> >
> > -struct msm_mdss_data {
> > - u32 ubwc_enc_version;
> > - /* can be read from register 0x58 */
> > - u32 ubwc_dec_version;
> > - u32 ubwc_swizzle;
> > - u32 ubwc_static;
> > - u32 highest_bank_bit;
> > - u32 macrotile_mode;
> > -};
> > -
> >   struct msm_mdss {
> >   struct device *dev;
> >
> > @@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss 
> > *msm_mdss)
> >   return 0;
> >   }
> >
> > -#define UBWC_1_0 0x1000
> > -#define UBWC_2_0 0x2000
> > -#define UBWC_3_0 0x3000
> > -#define UBWC_4_0 0x4000
> > -#define UBWC_4_3 0x4003
> > -
> >   static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
> >   {
> >   const struct msm_mdss_data *data = msm_mdss->mdss_data;
> > @@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss 
> > *msm_mdss)
> >   }
> >   }
> >
> > +const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> > +{
> > + struct msm_mdss *mdss;
> > +
> > + if (!dev)
> > + return ERR_PTR(-EINVAL);
> > +
> > + mdss = dev_get_drvdata(dev);
> > +
> > + return mdss->mdss_data;
> > +}
> > +
> >   static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> >   {
> >   int ret;
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h
> > new file mode 100644
> > index ..02bbab42adbc
> > --- /dev/null
> > +++ b/drivers/gpu/drm/msm/msm_mdss.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2018, The Linux Foundation
> > + */
>
> Fix the copyright year .

No, the copyright year is correct. The data was moved from the c file
(which has this copyright) and there are no copyrightable additions.

>
> Apart from that,
>
> Reviewed-by: Abhinav Kumar 



-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 3/6] drm/msm/mdss: export UBWC data





On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:

DPU programming requires knowledge of some of UBWC parameters. This
results in duplication of UBWC data between MDSS and DPU drivers. Export
the required data from MDSS driver.

Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/msm_mdss.c | 30 +-
  drivers/gpu/drm/msm/msm_mdss.h | 27 +++
  2 files changed, 40 insertions(+), 17 deletions(-)
  create mode 100644 drivers/gpu/drm/msm/msm_mdss.h

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index d1e57099b517..ed836c659688 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -13,7 +13,7 @@
  #include 
  #include 
  
-#include "msm_drv.h"

+#include "msm_mdss.h"
  #include "msm_kms.h"
  
  #define HW_REV0x0

@@ -26,16 +26,6 @@
  
  #define MIN_IB_BW	4UL /* Min ib vote 400MB */
  
-struct msm_mdss_data {

-   u32 ubwc_enc_version;
-   /* can be read from register 0x58 */
-   u32 ubwc_dec_version;
-   u32 ubwc_swizzle;
-   u32 ubwc_static;
-   u32 highest_bank_bit;
-   u32 macrotile_mode;
-};
-
  struct msm_mdss {
struct device *dev;
  
@@ -185,12 +175,6 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)

return 0;
  }
  
-#define UBWC_1_0 0x1000

-#define UBWC_2_0 0x2000
-#define UBWC_3_0 0x3000
-#define UBWC_4_0 0x4000
-#define UBWC_4_3 0x4003
-
  static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
  {
const struct msm_mdss_data *data = msm_mdss->mdss_data;
@@ -236,6 +220,18 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss 
*msm_mdss)
}
  }
  
+const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)

+{
+   struct msm_mdss *mdss;
+
+   if (!dev)
+   return ERR_PTR(-EINVAL);
+
+   mdss = dev_get_drvdata(dev);
+
+   return mdss->mdss_data;
+}
+
  static int msm_mdss_enable(struct msm_mdss *msm_mdss)
  {
int ret;
diff --git a/drivers/gpu/drm/msm/msm_mdss.h b/drivers/gpu/drm/msm/msm_mdss.h
new file mode 100644
index ..02bbab42adbc
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_mdss.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2018, The Linux Foundation
+ */


Fix the copyright year .

Apart from that,

Reviewed-by: Abhinav Kumar

Re: [Freedreno] [PATCH 2/6] drm/msm/mdss: rename ubwc_version to ubwc_enc_version





On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:

Rename the ubwc_version field to ubwc_enc_version, it denotes the
version of the UBWC encoder, not the "UBWC version".

Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/msm_mdss.c | 26 +-
  1 file changed, 13 insertions(+), 13 deletions(-)



Yes, this is the encoder version

Reviewed-by: Abhinav Kumar

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

On Wed, Jul 26, 2023 at 1:00 PM Dmitry Baryshkov
 wrote:
>
> On Wed, 26 Jul 2023 at 21:28, Rob Clark  wrote:
> >
> > On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen  
> > wrote:
> > >
> > > On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > > >
> > > > On 07/07/2023 00:10, Rob Clark wrote:
> > > > > From: Rob Clark 
> > > > >
> > > > > There are cases where there are differences due to SoC integration.
> > > > > Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > > speedbin mappings.
> > > >
> > > > I have the feeling that we are trying to circumvent the way DT works. 
> > > > I'd
> > > > suggest adding explicit SoC-compatible strings to Adreno bindings and 
> > > > then
> > > > using of_device_id::data and then of_device_get_match_data().
> > > >
> > > Just thinking, then how about a unique compatible string which we match
> > > to identify gpu->info and drop chip-id check completely here?
> >
> > Ok, I think we could do this, so something like:
> >
> >   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
> >
> > ?
> >
> > It looks like we don't have gpu dt bits upstream yet for either sm4350
> > or sm6375, so I suppose we could get away with this change
>
> I think we can even skip the 619.0 part in the SoC compat string.
> So it will be:
>
> compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";
>
> In future we can drop the chipid part completely and handle that as a
> part of SoC data:
>
> compatible = "qcom,sm4350-adreno", "qcom,adreno";
>
> With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)
>

I don't think we can do that, there are cases where the same SoC had
multiple revisions of adreno.  We could possibly do that with future
things where we can read the chip-id from fw.

What we _could_ do at the expense of making the compatible parsing a
tiny bit more complex is something like:

   compatible = "qcom,sm4350-adreno-619.0", "qcom,adreno"

BR,
-R

> >
> > BR,
> > -R
> >
> > > -Akhil
> > >
> > > > >
> > > > > Signed-off-by: Rob Clark 
> > > > > ---
> > > > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 
> > > > > +++---
> > > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
> > > > >   2 files changed, 31 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > > > > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > index 3c531da417b9..e62bc895a31f 100644
> > > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > > > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > > > .init = a6xx_gpu_init,
> > > > > +   }, {
> > > > > +   .machine = "qcom,sm4350",
> > > > > +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > +   .revn = 619,
> > > > > +   .fw = {
> > > > > +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > +   },
> > > > > +   .gmem = SZ_512K,
> > > > > +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > +   .init = a6xx_gpu_init,
> > > > > +   .zapfw = "a615_zap.mdt",
> > > > > +   .hwcg = a615_hwcg,
> > > > > +   }, {
> > > > > +   .machine = "qcom,sm6375",
> > > > > +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > +   .revn = 619,
> > > > > +   .fw = {
> > > > > +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > > +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > > +   },
> > > > > +   .gmem = SZ_512K,
> > > > > +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > > +   .init = a6xx_gpu_init,
> > > > > +   .zapfw = "a615_zap.mdt",
> > > > > +   .hwcg = a615_hwcg,
> > > > > }, {
> > > > > .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > > .revn = 619,
> > > > > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct 
> > > > > adreno_rev rev)
> > > > > /* identify gpu: */
> > > > > for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > > > const struct adreno_info *info = [i];
> > > > > +   if (info->machine && 
> > > > > !of_machine_is_compatible(info->machine))
> > > > > +   continue;
> > > > > if (adreno_cmp_rev(info->rev, rev))
> > > > > return info;
> > > > > }
> > > > > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct 
> > > > > device *master, void *data)
> > > > > config.rev.minor, config.rev.patchid);
> > > > > priv->is_a2xx = config.rev.core == 2;
> > > > > +   priv->has_cached_coherent =
> > > > > +   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > > >

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

On Wed, 26 Jul 2023 at 21:28, Rob Clark  wrote:
>
> On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen  
> wrote:
> >
> > On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> > >
> > > On 07/07/2023 00:10, Rob Clark wrote:
> > > > From: Rob Clark 
> > > >
> > > > There are cases where there are differences due to SoC integration.
> > > > Such as cache-coherency support, and (in the next patch) e-fuse to
> > > > speedbin mappings.
> > >
> > > I have the feeling that we are trying to circumvent the way DT works. I'd
> > > suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > > using of_device_id::data and then of_device_get_match_data().
> > >
> > Just thinking, then how about a unique compatible string which we match
> > to identify gpu->info and drop chip-id check completely here?
>
> Ok, I think we could do this, so something like:
>
>   compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"
>
> ?
>
> It looks like we don't have gpu dt bits upstream yet for either sm4350
> or sm6375, so I suppose we could get away with this change

I think we can even skip the 619.0 part in the SoC compat string.
So it will be:

compatible = "qcom,sm4350-adreno", qcom,adreno-619.0", "qcom,adreno";

In future we can drop the chipid part completely and handle that as a
part of SoC data:

compatible = "qcom,sm4350-adreno", "qcom,adreno";

With the driver knowing that sm4350-adreno means ADRENO_ID(6,1,9,0)

>
> BR,
> -R
>
> > -Akhil
> >
> > > >
> > > > Signed-off-by: Rob Clark 
> > > > ---
> > > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++---
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
> > > >   2 files changed, 31 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > > > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > index 3c531da417b9..e62bc895a31f 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > > .init = a6xx_gpu_init,
> > > > +   }, {
> > > > +   .machine = "qcom,sm4350",
> > > > +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > +   .revn = 619,
> > > > +   .fw = {
> > > > +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > +   },
> > > > +   .gmem = SZ_512K,
> > > > +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > +   .init = a6xx_gpu_init,
> > > > +   .zapfw = "a615_zap.mdt",
> > > > +   .hwcg = a615_hwcg,
> > > > +   }, {
> > > > +   .machine = "qcom,sm6375",
> > > > +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > +   .revn = 619,
> > > > +   .fw = {
> > > > +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > > +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > > +   },
> > > > +   .gmem = SZ_512K,
> > > > +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > > +   .init = a6xx_gpu_init,
> > > > +   .zapfw = "a615_zap.mdt",
> > > > +   .hwcg = a615_hwcg,
> > > > }, {
> > > > .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > > .revn = 619,
> > > > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct 
> > > > adreno_rev rev)
> > > > /* identify gpu: */
> > > > for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > > const struct adreno_info *info = [i];
> > > > +   if (info->machine && 
> > > > !of_machine_is_compatible(info->machine))
> > > > +   continue;
> > > > if (adreno_cmp_rev(info->rev, rev))
> > > > return info;
> > > > }
> > > > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct 
> > > > device *master, void *data)
> > > > config.rev.minor, config.rev.patchid);
> > > > priv->is_a2xx = config.rev.core == 2;
> > > > +   priv->has_cached_coherent =
> > > > +   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > > gpu = info->init(drm);
> > > > if (IS_ERR(gpu)) {
> > > > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct 
> > > > device *master, void *data)
> > > > if (ret)
> > > > return ret;
> > > > -   priv->has_cached_coherent =
> > > > -   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > > -   !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > > -
> > > > return 0;
> > > >   }
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> > > > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > > index e08d41337169..d5335b99c64c 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> >

Re: [Freedreno] [PATCH 06/12] drm/msm/adreno: Allow SoC specific gpu device table entries

On Thu, Jul 13, 2023 at 1:26 PM Akhil P Oommen  wrote:
>
> On Fri, Jul 07, 2023 at 05:34:04AM +0300, Dmitry Baryshkov wrote:
> >
> > On 07/07/2023 00:10, Rob Clark wrote:
> > > From: Rob Clark 
> > >
> > > There are cases where there are differences due to SoC integration.
> > > Such as cache-coherency support, and (in the next patch) e-fuse to
> > > speedbin mappings.
> >
> > I have the feeling that we are trying to circumvent the way DT works. I'd
> > suggest adding explicit SoC-compatible strings to Adreno bindings and then
> > using of_device_id::data and then of_device_get_match_data().
> >
> Just thinking, then how about a unique compatible string which we match
> to identify gpu->info and drop chip-id check completely here?

Ok, I think we could do this, so something like:

  compatible = "qcom,sm4350-adreno-619.0", qcom,adreno-619.0", "qcom,adreno"

?

It looks like we don't have gpu dt bits upstream yet for either sm4350
or sm6375, so I suppose we could get away with this change

BR,
-R

> -Akhil
>
> > >
> > > Signed-off-by: Rob Clark 
> > > ---
> > >   drivers/gpu/drm/msm/adreno/adreno_device.c | 34 +++---
> > >   drivers/gpu/drm/msm/adreno/adreno_gpu.h|  1 +
> > >   2 files changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > index 3c531da417b9..e62bc895a31f 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > @@ -258,6 +258,32 @@ static const struct adreno_info gpulist[] = {
> > > .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > .quirks = ADRENO_QUIRK_HAS_CACHED_COHERENT,
> > > .init = a6xx_gpu_init,
> > > +   }, {
> > > +   .machine = "qcom,sm4350",
> > > +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > +   .revn = 619,
> > > +   .fw = {
> > > +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > +   },
> > > +   .gmem = SZ_512K,
> > > +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > +   .init = a6xx_gpu_init,
> > > +   .zapfw = "a615_zap.mdt",
> > > +   .hwcg = a615_hwcg,
> > > +   }, {
> > > +   .machine = "qcom,sm6375",
> > > +   .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > +   .revn = 619,
> > > +   .fw = {
> > > +   [ADRENO_FW_SQE] = "a630_sqe.fw",
> > > +   [ADRENO_FW_GMU] = "a619_gmu.bin",
> > > +   },
> > > +   .gmem = SZ_512K,
> > > +   .inactive_period = DRM_MSM_INACTIVE_PERIOD,
> > > +   .init = a6xx_gpu_init,
> > > +   .zapfw = "a615_zap.mdt",
> > > +   .hwcg = a615_hwcg,
> > > }, {
> > > .rev = ADRENO_REV(6, 1, 9, ANY_ID),
> > > .revn = 619,
> > > @@ -409,6 +435,8 @@ const struct adreno_info *adreno_info(struct 
> > > adreno_rev rev)
> > > /* identify gpu: */
> > > for (i = 0; i < ARRAY_SIZE(gpulist); i++) {
> > > const struct adreno_info *info = [i];
> > > +   if (info->machine && !of_machine_is_compatible(info->machine))
> > > +   continue;
> > > if (adreno_cmp_rev(info->rev, rev))
> > > return info;
> > > }
> > > @@ -563,6 +591,8 @@ static int adreno_bind(struct device *dev, struct 
> > > device *master, void *data)
> > > config.rev.minor, config.rev.patchid);
> > > priv->is_a2xx = config.rev.core == 2;
> > > +   priv->has_cached_coherent =
> > > +   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT);
> > > gpu = info->init(drm);
> > > if (IS_ERR(gpu)) {
> > > @@ -574,10 +604,6 @@ static int adreno_bind(struct device *dev, struct 
> > > device *master, void *data)
> > > if (ret)
> > > return ret;
> > > -   priv->has_cached_coherent =
> > > -   !!(info->quirks & ADRENO_QUIRK_HAS_CACHED_COHERENT) &&
> > > -   !adreno_has_gmu_wrapper(to_adreno_gpu(gpu));
> > > -
> > > return 0;
> > >   }
> > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> > > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > index e08d41337169..d5335b99c64c 100644
> > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > > @@ -61,6 +61,7 @@ extern const struct adreno_reglist a612_hwcg[], 
> > > a615_hwcg[], a630_hwcg[], a640_h
> > >   extern const struct adreno_reglist a660_hwcg[], a690_hwcg[];
> > >   struct adreno_info {
> > > +   const char *machine;
> > > struct adreno_rev rev;
> > > uint32_t revn;
> > > const char *fw[ADRENO_FW_MAX];
> >
> > --
> > With best wishes
> > Dmitry
> >

Re: [Freedreno] [PATCH v4 04/17] dt-bindings: display/msm: Remove DSI1 ports from SM6350/SM6375 example

2023-07-26 Thread Rob Herring



On Sun, 23 Jul 2023 18:08:42 +0200, Marijn Suijten wrote:
> Both SM6350 and SM6375 support only a single DSI link, and don't have a
> corresponding dsi1 node in DTS.  Their examples should not suggest an
> output interface port on the display-controller node to this inexistant
> DSI host, with a dsi1_in label reference that doesn't exist in the
> example either.
> 
> Fixes: 3b7502b0c205 ("dt-bindings: display/msm: Add SM6350 MDSS")
> Fixes: 2a5c1021bc77 ("dt-bindings: display/msm: Add SM6375 MDSS")
> Signed-off-by: Marijn Suijten 
> ---
>  .../devicetree/bindings/display/msm/qcom,sm6350-mdss.yaml  | 7 
> ---
>  .../devicetree/bindings/display/msm/qcom,sm6375-mdss.yaml  | 7 
> ---
>  2 files changed, 14 deletions(-)
> 

Acked-by: Rob Herring

Re: [Freedreno] [PATCH v6 2/2] arm64: dts: qcom: sdm845-db845c: Mark cont splash memory region as reserved

On 26/07/2023 15:27, Amit Pundir wrote:
> Adding a reserved memory region for the framebuffer memory
> (the splash memory region set up by the bootloader).
> 
> It fixes a kernel panic (arm-smmu: Unhandled context fault
> at this particular memory region) reported on DB845c running
> v5.10.y.
> 
> Cc: sta...@vger.kernel.org # v5.10+
> Reviewed-by: Caleb Connolly 
> Signed-off-by: Amit Pundir 
> ---


Acked-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

[Freedreno] [PATCH v6 2/2] arm64: dts: qcom: sdm845-db845c: Mark cont splash memory region as reserved

2023-07-26 Thread Amit Pundir

Adding a reserved memory region for the framebuffer memory
(the splash memory region set up by the bootloader).

It fixes a kernel panic (arm-smmu: Unhandled context fault
at this particular memory region) reported on DB845c running
v5.10.y.

Cc: sta...@vger.kernel.org # v5.10+
Reviewed-by: Caleb Connolly 
Signed-off-by: Amit Pundir 
---
v6: Collected review tag, updated commit message for the
context and marked for stable kernel versions.

v5: Re-sending with updated dt-bindings patch in mdss-common
schema.

v4: Re-sending this along with a new dt-bindings patch to
document memory-region property in qcom,sdm845-mdss
schema and keep dtbs_check happy.

v3: Point this reserved region to MDSS.

v2: Updated commit message.

There was some dicussion on v1 but it didn't go anywhere,
https://lore.kernel.org/linux-kernel/20230124182857.1524912-1-amit.pun...@linaro.org/T/#u.
The general consensus is that this memory should be freed and be
made resuable but that (releasing this piece of memory) has been
tried before and it is not trivial to return the reserved memory
node to the system RAM pool in this case.

 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts 
b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index d6b464cb61d6..f546f6f57c1e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -101,6 +101,14 @@ hdmi_con: endpoint {
};
};
 
+   reserved-memory {
+   /* Cont splash region set up by the bootloader */
+   cont_splash_mem: framebuffer@9d40 {
+   reg = <0x0 0x9d40 0x0 0x240>;
+   no-map;
+   };
+   };
+
lt9611_1v8: lt9611-vdd18-regulator {
compatible = "regulator-fixed";
regulator-name = "LT9611_1V8";
@@ -506,6 +514,7 @@  {
 };
 
  {
+   memory-region = <_splash_mem>;
status = "okay";
 };
 
-- 
2.25.1

[Freedreno] [PATCH v6 1/2] dt-bindings: display/msm: mdss-common: add memory-region property

2023-07-26 Thread Amit Pundir

Add and document the reserved memory region property in the
mdss-common schema.

For now (sdm845-db845c), it points to a framebuffer memory
region reserved by the bootloader for splash screen.

Acked-by: Krzysztof Kozlowski 
Reviewed-by: Rob Herring 
Signed-off-by: Amit Pundir 
---
v6: Re-sending with review and ack tags. Ideally this
dt-binding patch should be marked for stable as well,
like the follow-up sdm845-db845c.dts patch in the series
but it can't be cherry picked cleanly on older LTS
versions. I can do it separately if it is required.

v5: Moving the dt-binding to mdss-common schema with
updated commit message and property description.

v4: Adding this new dt-binding patch, in qcom,sdm845-mdss
schema, in the v4 of the follow-up patch for
sdm845-db845c.
https://lore.kernel.org/lkml/20230712130215.666924-2-amit.pun...@linaro.org/

 .../devicetree/bindings/display/msm/mdss-common.yaml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml 
b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
index ccd7d6417523..84ed2757ded5 100644
--- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
+++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
@@ -77,6 +77,12 @@ properties:
 items:
   - description: MDSS_CORE reset
 
+  memory-region:
+maxItems: 1
+description:
+  Phandle to a node describing a reserved framebuffer memory region.
+  For example, the splash memory region set up by the bootloader.
+
 required:
   - reg
   - reg-names
-- 
2.25.1

Re: [Freedreno] [PATCH v5 2/2] arm64: dts: qcom: sdm845-db845c: Mark cont splash memory region as reserved

On 13/07/2023 18:52, Amit Pundir wrote:
> Adding a reserved memory region for the framebuffer memory
> (the splash memory region set up by the bootloader).
> 
> Signed-off-by: Amit Pundir 
> ---

I think your commit msg misses describing the actual problem, impact to
users and finally cc-stable.

Best regards,
Krzysztof

Re: [Freedreno] [PATCH v5 1/2] dt-bindings: display/msm: mdss-common: add memory-region property

2023-07-26 Thread Linux regression tracking (Thorsten Leemhuis)

On 13/07/2023 18:52, Amit Pundir wrote:
> Add and document the reserved memory region property in the
> mdss-common schema.
> 
> For now (sdm845-db845c), it points to a framebuffer memory
> region reserved by the bootloader for splash screen.
> 
> Signed-off-by: Amit Pundir 
> ---

Acked-by: Krzysztof Kozlowski 

Best regards,
Krzysztof

Re: [Freedreno] [PATCH 2/2] drm/bridge: lt9611: Do not generate HFP/HBP/HSA and EOT packet

Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
for once, to make this easily accessible to everyone.

What's the status wrt to this regression (caused by 8ddce13ae69 from
Marek)? It looks like things are stalled and the regression still is
unresolved, but I ask because I might be missing something.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

On 14.07.23 08:11, Amit Pundir wrote:
> On Thu, 13 Jul 2023 at 23:58, Marek Vasut  wrote:
>>
>> On 7/13/23 20:09, Abhinav Kumar wrote:
>>>
>>>
>>> On 7/12/2023 10:41 AM, Marek Vasut wrote:
 On 7/9/23 03:03, Abhinav Kumar wrote:
>
>
> On 7/7/2023 1:47 AM, Neil Armstrong wrote:
>> On 07/07/2023 09:18, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On 06/07/2023 11:20, Amit Pundir wrote:
 On Wed, 5 Jul 2023 at 11:09, Dmitry Baryshkov
  wrote:
>
> [Adding freedreno@ to cc list]
>
> On Wed, 5 Jul 2023 at 08:31, Jagan Teki
>  wrote:
>>
>> Hi Amit,
>>
>> On Wed, Jul 5, 2023 at 10:15 AM Amit Pundir
>>  wrote:
>>>
>>> Hi Marek,
>>>
>>> On Wed, 5 Jul 2023 at 01:48, Marek Vasut  wrote:

 Do not generate the HS front and back porch gaps, the HSA gap and
 EOT packet, as these packets are not required. This makes the
 bridge
 work with Samsung DSIM on i.MX8MM and i.MX8MP.
>>>
>>> This patch broke display on Dragonboard 845c (SDM845) devboard
>>> running
>>> AOSP. This is what I see
>>> https://people.linaro.org/~amit.pundir/db845c-userdebug/v6.5-broken-display/PXL_20230704_150156326.jpg.
>>> Reverting this patch fixes this regression for me.
>>
>> Might be msm dsi host require proper handling on these updated
>> mode_flags? did they?
>
> The msm DSI host supports those flags. Also, I'd like to point out
> that the patch didn't change the rest of the driver code. So even if
> drm/msm ignored some of the flags, it should not have caused the
> issue. Most likely the issue is on the lt9611 side. I's suspect that
> additional programming is required to make it work with these flags.

 I spent some time today on smoke testing these flags (individually
 and
 in limited combination) on DB845c, to narrow down this breakage to
 one
 or more flag(s) triggering it. Here are my observations in limited
 testing done so far.

 There is no regression with MIPI_DSI_MODE_NO_EOT_PACKET when enabled
 alone and system boots to UI as usual.

 MIPI_DSI_MODE_VIDEO_NO_HFP always trigger the broken display as in
 the
 screenshot[1] shared earlier as well.

 Adding either of MIPI_DSI_MODE_VIDEO_NO_HSA and
 MIPI_DSI_MODE_VIDEO_NO_HBP always result in no display, unless paired
 with MIPI_DSI_MODE_VIDEO_NO_HFP and in that case we get the broken
 display as reported.

 In short other than MIPI_DSI_MODE_NO_EOT_PACKET flag, all other flags
 added in this commit break the display on DB845c one way or another.
>>>
>>> I think the investigation would be to understand why samsung-dsim
>>> requires
>>> such flags and/or what are the difference in behavior between MSM
>>> DSI and samsung DSIM
>>> for those flags ?
>>>
>>> If someone has access to the lt9611 datasheet, so it requires
>>> HSA/HFP/HBP to be
>>> skipped ? and does MSM DSI and samsung DSIM skip them in the same
>>> way ?
>>
>> I think there's a mismatch, where on one side this flags sets the
>> link in LP-11 while
>> in HSA/HFP/HPB while on the other it completely removes those
>> blanking packets.
>>
>> The name MIPI_DSI_MODE_VIDEO_NO_HBP suggests removal of HPB, not
>> LP-11 while HPB.
>> the registers used in both controllers are different:
>> - samsung-dsim: DSIM_HBP_DISABLE_MODE
>> - msm dsi: DSI_VID_CFG0_HBP_POWER_STOP
>>
>> The first one suggest removing the packet, while the second one
>> suggests powering
>> off the line while in the blanking packet period.
>>
>> @Abhinav, can you comment on that ?
>>
>
> I dont get what it means by completely removes blanking packets in DSIM.

 MIPI_DSI_MODE_VIDEO_NO_HFP means the HBP period is just skipped by DSIM.

 Maybe there is a need for new set of flags which differentiate between
 HBP skipped (i.e. NO HBP) and HBP LP11 ?

>>>
>>> No, the section of the MIPI DSI spec I posted below clearly states there
>>> are

Re: [Freedreno] [PATCH] dt-bindings: display: msm: sm6125-mdss: drop unneeded status from examples

2023-07-26 Thread Marijn Suijten

On 2023-07-26 10:42:24, Dmitry Baryshkov wrote:
> On 26/07/2023 10:31, Krzysztof Kozlowski wrote:
> > On 26/07/2023 09:27, Krzysztof Kozlowski wrote:
> >> On 25/07/2023 13:46, Marijn Suijten wrote:
> >>> On 2023-07-25 12:16:10, Krzysztof Kozlowski wrote:
>  Example DTS should not have 'status' property.
> 
>  Signed-off-by: Krzysztof Kozlowski 
>  ---
>    .../devicetree/bindings/display/msm/qcom,sm6125-mdss.yaml   | 6 --
> >>>
> >>> This is not needed: it has already been corrected in v3 and v4 of the
> >>> respective series (among other changes) and the patches were only picked
> >>> to a preliminary (draft) pull to get an overview of the outstanding work
> >>> for this subsystem.  That branch happens to be included in regular -next
> >>> releases though.
> >>>
> >>> 6.6 drm/msm display pull: 
> >>> https://gitlab.freedesktop.org/drm/msm/-/merge_requests/69
> >>> v3: 
> >>> https://lore.kernel.org/linux-arm-msm/20230718-sm6125-dpu-v3-0-6c5a56e99...@somainline.org/
> >>> v4: 
> >>> https://lore.kernel.org/linux-arm-msm/20230723-sm6125-dpu-v4-0-a3f287dd6...@somainline.org/
> >>
> >> What do you mean? The old code (one I am fixing) is in current next...
> >>
> >> If this was fixed, why next gets some outdated branches of drm next?
> >> Each maintainers next tree is supposed to be fed into the next, without
> >> delays.
> >>
> > 
> > Ah, I think I understood - some work in progress was applied to
> > work-in-progress branch of drm/msm and this somehow got pushed to
> > linux-next? How anyone is supposed to work on next branches if they are
> > outdated or have stuff known to be incomplete?
> 
> The drm/msm & bindings parts were considered final, but then I failed to 
> send 'applied' series for some reason. And then it was natural for 
> Marijn to send an updated revision.

There were comments on some of the patches that would have an effect on
the binding parts (including the examples).

- Marijn

Re: [Freedreno] [PATCH] dt-bindings: display: msm: sm6125-mdss: drop unneeded status from examples

On 26/07/2023 10:31, Krzysztof Kozlowski wrote:

On 26/07/2023 09:27, Krzysztof Kozlowski wrote:

On 25/07/2023 13:46, Marijn Suijten wrote:

On 2023-07-25 12:16:10, Krzysztof Kozlowski wrote:

Example DTS should not have 'status' property.

Signed-off-by: Krzysztof Kozlowski
---
.../devicetree/bindings/display/msm/qcom,sm6125-mdss.yaml | 6 --

This is not needed: it has already been corrected in v3 and v4 of the
respective series (among other changes) and the patches were only picked
to a preliminary (draft) pull to get an overview of the outstanding work
for this subsystem. That branch happens to be included in regular -next
releases though.

6.6 drm/msm display pull:
https://gitlab.freedesktop.org/drm/msm/-/merge_requests/69
v3:
https://lore.kernel.org/linux-arm-msm/20230718-sm6125-dpu-v3-0-6c5a56e99...@somainline.org/
v4:
https://lore.kernel.org/linux-arm-msm/20230723-sm6125-dpu-v4-0-a3f287dd6...@somainline.org/

What do you mean? The old code (one I am fixing) is in current next...

If this was fixed, why next gets some outdated branches of drm next?
Each maintainers next tree is supposed to be fed into the next, without
delays.

Ah, I think I understood - some work in progress was applied to
work-in-progress branch of drm/msm and this somehow got pushed to
linux-next? How anyone is supposed to work on next branches if they are
outdated or have stuff known to be incomplete?

The drm/msm & bindings parts were considered final, but then I failed to
send 'applied' series for some reason. And then it was natural for
Marijn to send an updated revision.

--
With best wishes
Dmitry

Re: [Freedreno] [PATCH] dt-bindings: display: msm: sm6125-mdss: drop unneeded status from examples

On 26/07/2023 09:27, Krzysztof Kozlowski wrote:
> On 25/07/2023 13:46, Marijn Suijten wrote:
>> On 2023-07-25 12:16:10, Krzysztof Kozlowski wrote:
>>> Example DTS should not have 'status' property.
>>>
>>> Signed-off-by: Krzysztof Kozlowski 
>>> ---
>>>  .../devicetree/bindings/display/msm/qcom,sm6125-mdss.yaml   | 6 --
>>
>> This is not needed: it has already been corrected in v3 and v4 of the
>> respective series (among other changes) and the patches were only picked
>> to a preliminary (draft) pull to get an overview of the outstanding work
>> for this subsystem.  That branch happens to be included in regular -next
>> releases though.
>>
>> 6.6 drm/msm display pull: 
>> https://gitlab.freedesktop.org/drm/msm/-/merge_requests/69
>> v3: 
>> https://lore.kernel.org/linux-arm-msm/20230718-sm6125-dpu-v3-0-6c5a56e99...@somainline.org/
>> v4: 
>> https://lore.kernel.org/linux-arm-msm/20230723-sm6125-dpu-v4-0-a3f287dd6...@somainline.org/
> 
> What do you mean? The old code (one I am fixing) is in current next...
> 
> If this was fixed, why next gets some outdated branches of drm next?
> Each maintainers next tree is supposed to be fed into the next, without
> delays.
> 

Ah, I think I understood - some work in progress was applied to
work-in-progress branch of drm/msm and this somehow got pushed to
linux-next? How anyone is supposed to work on next branches if they are
outdated or have stuff known to be incomplete?

Best regards,
Krzysztof

Re: [Freedreno] [PATCH] dt-bindings: display: msm: sm6125-mdss: drop unneeded status from examples