Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On Fri, Apr 12, 2024 at 02:10:30AM +0200, Konrad Dybcio wrote: > > > On 4/12/24 01:49, Elliot Berman wrote: > > On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote: > > > > > > > > > On 4/11/24 22:09, Elliot Berman wrote: > > > > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: > > > > > > > > > > > > > > > On 4/11/24 20:55, Elliot Berman wrote: > > > > > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > > > > > > > In preparation for parsing the chip "feature code" (FC) and > > > > > > > "product > > > > > > > code" (PC) (essentially the parameters that let us conclusively > > > > > > > characterize the sillicon we're running on, including various > > > > > > > speed > > > > > > > bins), move the socinfo version defines to the public header and > > > > > > > include some more FC/PC defines. > > > > > > > > > > > > > > Signed-off-by: Konrad Dybcio > > > > > > > --- > > > > > > [...] > > > > > > > > > > > 0xf is the last one. > > > > > > One more question, are the "internal/external feature codes" referring to > > > internality/externality of the chips (i.e. "are they QC-lab-only > > > engineering > > > samples), or what else does that represent? > > > > Yes, QC-lab-only engineering samples is the right interpretation of > > these feature codes. > > Do you think it would be beneficial to keep the logic for these ESes in > the upstream GPU driver? Otherwise, I can yank out half of the added lines. > Should be fine to yank, IMO.
Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On 4/12/24 01:49, Elliot Berman wrote: On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote: On 4/11/24 22:09, Elliot Berman wrote: On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: On 4/11/24 20:55, Elliot Berman wrote: On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: In preparation for parsing the chip "feature code" (FC) and "product code" (PC) (essentially the parameters that let us conclusively characterize the sillicon we're running on, including various speed bins), move the socinfo version defines to the public header and include some more FC/PC defines. Signed-off-by: Konrad Dybcio --- [...] 0xf is the last one. One more question, are the "internal/external feature codes" referring to internality/externality of the chips (i.e. "are they QC-lab-only engineering samples), or what else does that represent? Yes, QC-lab-only engineering samples is the right interpretation of these feature codes. Do you think it would be beneficial to keep the logic for these ESes in the upstream GPU driver? Otherwise, I can yank out half of the added lines. Konrad
Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On Thu, Apr 11, 2024 at 10:24:08PM +0200, Konrad Dybcio wrote: > > > On 4/11/24 22:09, Elliot Berman wrote: > > On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: > > > > > > > > > On 4/11/24 20:55, Elliot Berman wrote: > > > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > > > > > In preparation for parsing the chip "feature code" (FC) and "product > > > > > code" (PC) (essentially the parameters that let us conclusively > > > > > characterize the sillicon we're running on, including various speed > > > > > bins), move the socinfo version defines to the public header and > > > > > include some more FC/PC defines. > > > > > > > > > > Signed-off-by: Konrad Dybcio > > > > > --- > > [...] > > > > > 0xf is the last one. > > One more question, are the "internal/external feature codes" referring to > internality/externality of the chips (i.e. "are they QC-lab-only engineering > samples), or what else does that represent? Yes, QC-lab-only engineering samples is the right interpretation of these feature codes. Thanks, Elliot
Re: [PATCH 4/6] drm/msm/adreno: Implement SMEM-based speed bin
On 4/11/24 23:46, Dmitry Baryshkov wrote:
On Fri, 12 Apr 2024 at 00:35, Konrad Dybcio wrote:
On 4/10/24 21:26, Dmitry Baryshkov wrote:
On Wed, Apr 10, 2024 at 01:42:33PM +0200, Konrad Dybcio wrote:
On 4/6/24 05:23, Dmitry Baryshkov wrote:
On Fri, Apr 05, 2024 at 10:41:32AM +0200, Konrad Dybcio wrote:
On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
abstracted through SMEM, instead of being directly available in a fuse.
Add support for SMEM-based speed binning, which includes getting
"feature code" and "product code" from said source and parsing them
to form something that lets us match OPPs against.
Signed-off-by: Konrad Dybcio
---
[...]
+ }
+
+ ret = qcom_smem_get_product_code();
+ if (ret) {
+ dev_err(dev, "Couldn't get product code from SMEM!\n");
+ return ret;
+ }
+
+ /* Don't consider fcode for external feature codes */
+ if (fcode <= SOCINFO_FC_EXT_RESERVE)
+ fcode = SOCINFO_FC_UNKNOWN;
+
+ *speedbin = FIELD_PREP(ADRENO_SKU_ID_PCODE, pcode) |
+ FIELD_PREP(ADRENO_SKU_ID_FCODE, fcode);
What about just asking the qcom_smem for the 'gpu_bin' and hiding gory
details there? It almost feels that handling raw PCODE / FCODE here is
too low-level and a subject to change depending on the socinfo format.
No, the FCODE & PCODE can be interpreted differently across consumers.
That's why I wrote about asking for 'gpu_bin'.
I'd rather keep the magic GPU LUTs inside the adreno driver, especially
since not all Snapdragons feature Adreno, but all Adrenos are on
Snapdragons (modulo a2xx but I refuse to make design decisions treating
these equally to e.g. a6xx)
LUTs - yes. I wanted to push (FC << a) | (PC << b) and all the RESERVE
/ UNKNOWN magic there.
Ohh this specifically.. yeah I considered pushing that there as well,
but I realized this is specific to the GPU. The socinfo APIs should
only return a valid/unknown code for both P and F and let the consumer
figure out how to interpret these.
+
+ return ret;
}
int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
@@ -1098,9 +1129,9 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
devm_pm_opp_set_clkname(dev, "core");
}
- if (adreno_read_speedbin(dev, ) || !speedbin)
+ if (adreno_read_speedbin(adreno_gpu, dev, ) || !speedbin)
speedbin = 0x;
- adreno_gpu->speedbin = (uint16_t) (0x & speedbin);
the &= 0x should probably go to the adreno_read_speedbin / nvmem
case. WDYT?
Ok, I can keep it, though realistically if this ever does anything
useful, it likely means the dt is wrong
Yes, but if DT is wrong, we should probably fail in a sensible way. I
just wanted to point out that previously we had this &0x, while your
patch silently removes it.
Right, but I don't believe it actually matters.. If that AND ever did
anything, this was a silent failure with garbage data passed in anyway.
If you really insist, I can remove it separately.
I'd say, up to Rob or up to your consideration.
Konrad
Re: [PATCH 4/6] drm/msm/adreno: Implement SMEM-based speed bin
On Fri, 12 Apr 2024 at 00:35, Konrad Dybcio wrote:
>
>
>
> On 4/10/24 21:26, Dmitry Baryshkov wrote:
> > On Wed, Apr 10, 2024 at 01:42:33PM +0200, Konrad Dybcio wrote:
> >>
> >>
> >> On 4/6/24 05:23, Dmitry Baryshkov wrote:
> >>> On Fri, Apr 05, 2024 at 10:41:32AM +0200, Konrad Dybcio wrote:
> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
> abstracted through SMEM, instead of being directly available in a fuse.
>
> Add support for SMEM-based speed binning, which includes getting
> "feature code" and "product code" from said source and parsing them
> to form something that lets us match OPPs against.
>
> Signed-off-by: Konrad Dybcio
> ---
> >>
> >> [...]
> >>
> >>>
> + }
> +
> + ret = qcom_smem_get_product_code();
> + if (ret) {
> + dev_err(dev, "Couldn't get product code from SMEM!\n");
> + return ret;
> + }
> +
> + /* Don't consider fcode for external feature codes */
> + if (fcode <= SOCINFO_FC_EXT_RESERVE)
> + fcode = SOCINFO_FC_UNKNOWN;
> +
> + *speedbin = FIELD_PREP(ADRENO_SKU_ID_PCODE, pcode) |
> + FIELD_PREP(ADRENO_SKU_ID_FCODE, fcode);
> >>>
> >>> What about just asking the qcom_smem for the 'gpu_bin' and hiding gory
> >>> details there? It almost feels that handling raw PCODE / FCODE here is
> >>> too low-level and a subject to change depending on the socinfo format.
> >>
> >> No, the FCODE & PCODE can be interpreted differently across consumers.
> >
> > That's why I wrote about asking for 'gpu_bin'.
>
> I'd rather keep the magic GPU LUTs inside the adreno driver, especially
> since not all Snapdragons feature Adreno, but all Adrenos are on
> Snapdragons (modulo a2xx but I refuse to make design decisions treating
> these equally to e.g. a6xx)
LUTs - yes. I wanted to push (FC << a) | (PC << b) and all the RESERVE
/ UNKNOWN magic there.
>
> >
> >>
> >>>
> +
> + return ret;
> }
> int adreno_gpu_init(struct drm_device *drm, struct platform_device
> *pdev,
> @@ -1098,9 +1129,9 @@ int adreno_gpu_init(struct drm_device *drm, struct
> platform_device *pdev,
> devm_pm_opp_set_clkname(dev, "core");
> }
> - if (adreno_read_speedbin(dev, ) || !speedbin)
> + if (adreno_read_speedbin(adreno_gpu, dev, ) || !speedbin)
> speedbin = 0x;
> - adreno_gpu->speedbin = (uint16_t) (0x & speedbin);
> >>>
> >>> the &= 0x should probably go to the adreno_read_speedbin / nvmem
> >>> case. WDYT?
> >>
> >> Ok, I can keep it, though realistically if this ever does anything
> >> useful, it likely means the dt is wrong
> >
> > Yes, but if DT is wrong, we should probably fail in a sensible way. I
> > just wanted to point out that previously we had this &0x, while your
> > patch silently removes it.
>
> Right, but I don't believe it actually matters.. If that AND ever did
> anything, this was a silent failure with garbage data passed in anyway.
>
> If you really insist, I can remove it separately.
I'd say, up to Rob or up to your consideration.
--
With best wishes
Dmitry
Re: [PATCH 10/12] drm/msm: convert msm_format::alpha_enable to the flag
On Fri, 12 Apr 2024 at 00:20, Abhinav Kumar wrote:
>
>
>
> On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
> > Instead of having a bool field alpha_enable, convert it to the
> > flag, this save space in the tables and allows us to handle all booleans
> > in the same way.
> >
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 12 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 24 ++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 7 +++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 3 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 ++--
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 2 +-
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 3 ++-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 3 ++-
> > drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
> > drivers/gpu/drm/msm/msm_drv.h | 4 ++--
> > 11 files changed, 40 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 9041b0d71b25..201010038660 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -342,7 +342,7 @@ static void _dpu_crtc_setup_blend_cfg(struct
> > dpu_crtc_mixer *mixer,
> >
> > /* default to opaque blending */
> > if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
> > - !format->alpha_enable) {
> > + !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)) {
> > blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
> > DPU_BLEND_BG_ALPHA_BG_CONST;
> > } else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) {
> > @@ -373,8 +373,8 @@ static void _dpu_crtc_setup_blend_cfg(struct
> > dpu_crtc_mixer *mixer,
> > lm->ops.setup_blend_config(lm, pstate->stage,
> > fg_alpha, bg_alpha, blend_op);
> >
> > - DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
> > - >pixel_format, format->alpha_enable, blend_op);
> > + DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%lu blend_op:0x%x\n",
> > + >pixel_format, format->flags &
> > MSM_FORMAT_FLAG_ALPHA_ENABLE, blend_op);
> > }
> >
> > static void _dpu_crtc_program_lm_output_roi(struct drm_crtc *crtc)
> > @@ -472,7 +472,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> > *crtc,
> >
> > format = msm_framebuffer_format(pstate->base.fb);
> >
> > - if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
> > + if (pstate->stage == DPU_STAGE_BASE &&
> > + format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)
> > bg_alpha_enable = true;
> >
> > set_bit(pstate->pipe.sspp->idx, fetch_active);
> > @@ -495,7 +496,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> > *crtc,
> > for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> > _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate,
> > format);
> >
> > - if (bg_alpha_enable && !format->alpha_enable)
> > + if (bg_alpha_enable &&
> > + !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE))
> > mixer[lm_idx].mixer_op_mode = 0;
> > else
> > mixer[lm_idx].mixer_op_mode |=
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > index baf0fd67bf42..de9e93cb42c4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > @@ -36,7 +36,6 @@ bp, flg, fm, np)
> > \
> > {
> > \
> > .pixel_format = DRM_FORMAT_ ## fmt, \
> > .fetch_type = MDP_PLANE_INTERLEAVED, \
> > - .alpha_enable = alpha,\
> > .element = { (e0), (e1), (e2), (e3) },\
> > .bpc_g_y = g, \
> > .bpc_b_cb = b,\
> > @@ -46,7 +45,9 @@ bp, flg, fm, np)
> > \
> > .unpack_count = uc, \
> > .bpp = bp,\
> > .fetch_mode = fm, \
> > - .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | flg, \
> > + .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | \
> > +
Re: [PATCH 4/6] drm/msm/adreno: Implement SMEM-based speed bin
On 4/10/24 21:26, Dmitry Baryshkov wrote:
On Wed, Apr 10, 2024 at 01:42:33PM +0200, Konrad Dybcio wrote:
On 4/6/24 05:23, Dmitry Baryshkov wrote:
On Fri, Apr 05, 2024 at 10:41:32AM +0200, Konrad Dybcio wrote:
On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
abstracted through SMEM, instead of being directly available in a fuse.
Add support for SMEM-based speed binning, which includes getting
"feature code" and "product code" from said source and parsing them
to form something that lets us match OPPs against.
Signed-off-by: Konrad Dybcio
---
[...]
+ }
+
+ ret = qcom_smem_get_product_code();
+ if (ret) {
+ dev_err(dev, "Couldn't get product code from SMEM!\n");
+ return ret;
+ }
+
+ /* Don't consider fcode for external feature codes */
+ if (fcode <= SOCINFO_FC_EXT_RESERVE)
+ fcode = SOCINFO_FC_UNKNOWN;
+
+ *speedbin = FIELD_PREP(ADRENO_SKU_ID_PCODE, pcode) |
+ FIELD_PREP(ADRENO_SKU_ID_FCODE, fcode);
What about just asking the qcom_smem for the 'gpu_bin' and hiding gory
details there? It almost feels that handling raw PCODE / FCODE here is
too low-level and a subject to change depending on the socinfo format.
No, the FCODE & PCODE can be interpreted differently across consumers.
That's why I wrote about asking for 'gpu_bin'.
I'd rather keep the magic GPU LUTs inside the adreno driver, especially
since not all Snapdragons feature Adreno, but all Adrenos are on
Snapdragons (modulo a2xx but I refuse to make design decisions treating
these equally to e.g. a6xx)
+
+ return ret;
}
int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
@@ -1098,9 +1129,9 @@ int adreno_gpu_init(struct drm_device *drm, struct
platform_device *pdev,
devm_pm_opp_set_clkname(dev, "core");
}
- if (adreno_read_speedbin(dev, ) || !speedbin)
+ if (adreno_read_speedbin(adreno_gpu, dev, ) || !speedbin)
speedbin = 0x;
- adreno_gpu->speedbin = (uint16_t) (0x & speedbin);
the &= 0x should probably go to the adreno_read_speedbin / nvmem
case. WDYT?
Ok, I can keep it, though realistically if this ever does anything
useful, it likely means the dt is wrong
Yes, but if DT is wrong, we should probably fail in a sensible way. I
just wanted to point out that previously we had this &0x, while your
patch silently removes it.
Right, but I don't believe it actually matters.. If that AND ever did
anything, this was a silent failure with garbage data passed in anyway.
If you really insist, I can remove it separately.
Konrad
Re: [PATCH 10/12] drm/msm: convert msm_format::alpha_enable to the flag
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
Instead of having a bool field alpha_enable, convert it to the
flag, this save space in the tables and allows us to handle all booleans
in the same way.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 12 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 24 ++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 7 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 4 ++--
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 2 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 3 ++-
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 9
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 3 ++-
drivers/gpu/drm/msm/disp/mdp_format.c | 2 +-
drivers/gpu/drm/msm/msm_drv.h | 4 ++--
11 files changed, 40 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 9041b0d71b25..201010038660 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -342,7 +342,7 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer
*mixer,
/* default to opaque blending */
if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
- !format->alpha_enable) {
+ !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)) {
blend_op = DPU_BLEND_FG_ALPHA_FG_CONST |
DPU_BLEND_BG_ALPHA_BG_CONST;
} else if (pstate->base.pixel_blend_mode == DRM_MODE_BLEND_PREMULTI) {
@@ -373,8 +373,8 @@ static void _dpu_crtc_setup_blend_cfg(struct dpu_crtc_mixer
*mixer,
lm->ops.setup_blend_config(lm, pstate->stage,
fg_alpha, bg_alpha, blend_op);
- DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%u blend_op:0x%x\n",
- >pixel_format, format->alpha_enable, blend_op);
+ DRM_DEBUG_ATOMIC("format:%p4cc, alpha_en:%lu blend_op:0x%x\n",
+ >pixel_format, format->flags &
MSM_FORMAT_FLAG_ALPHA_ENABLE, blend_op);
}
static void _dpu_crtc_program_lm_output_roi(struct drm_crtc *crtc)
@@ -472,7 +472,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
format = msm_framebuffer_format(pstate->base.fb);
- if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
+ if (pstate->stage == DPU_STAGE_BASE &&
+ format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE)
bg_alpha_enable = true;
set_bit(pstate->pipe.sspp->idx, fetch_active);
@@ -495,7 +496,8 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate,
format);
- if (bg_alpha_enable && !format->alpha_enable)
+ if (bg_alpha_enable &&
+ !(format->flags & MSM_FORMAT_FLAG_ALPHA_ENABLE))
mixer[lm_idx].mixer_op_mode = 0;
else
mixer[lm_idx].mixer_op_mode |=
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index baf0fd67bf42..de9e93cb42c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -36,7 +36,6 @@ bp, flg, fm, np)
\
{ \
.pixel_format = DRM_FORMAT_ ## fmt, \
.fetch_type = MDP_PLANE_INTERLEAVED, \
- .alpha_enable = alpha,\
.element = { (e0), (e1), (e2), (e3) },\
.bpc_g_y = g, \
.bpc_b_cb = b,\
@@ -46,7 +45,9 @@ bp, flg, fm, np)
\
.unpack_count = uc, \
.bpp = bp,\
.fetch_mode = fm, \
- .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | flg, \
+ .flags = MSM_FORMAT_FLAG_UNPACK_TIGHT | \
+ (alpha ? MSM_FORMAT_FLAG_ALPHA_ENABLE : 0) | \
+ flg, \
In the previous two patches where the same thing was done for
unpack_tight and unpack_align_msb, it was different in the sense that
just on the basis of which macro we were choosing we knew the value of
those flags so you could just
Re: [PATCH 09/12] drm/msm: convert msm_format::unpack_align_msb to the flag
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having a u8 or bool field unpack_align_msb, convert it to the flag, this save space in the tables and allows us to handle all booleans in the same way. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 12 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 2 +- drivers/gpu/drm/msm/msm_drv.h | 4 ++-- 4 files changed, 6 insertions(+), 14 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH 08/12] drm/msm: convert msm_format::unpack_tight to the flag
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: Instead of having a u8 or bool field unpack_tight, convert it to the flag, this save space in the tables and allows us to handle all booleans in the same way. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 22 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 3 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 3 +- drivers/gpu/drm/msm/disp/mdp_format.c | 52 ++--- drivers/gpu/drm/msm/msm_drv.h | 4 +- 7 files changed, 41 insertions(+), 47 deletions(-) Reviewed-by: Abhinav Kumar
[pull] drm/msm: drm-msm-next-2024-04-11 for v6.9-rc4
Hi Dave, Fixes for v6.9, description below The following changes since commit 4be445f5b6b6810baf397b2d159bd07c3573fd75: drm/msm/dpu: capture snapshot on the first commit_done timeout (2024-03-04 11:44:03 +0200) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git tags/drm-msm-next-2024-04-11 for you to fetch changes up to 9dc23cba0927d09cb481da064c8413eb9df42e2b: drm/msm/adreno: Set highest_bank_bit for A619 (2024-04-05 11:24:53 -0700) Fixes for v6.9 Display: - Fixes for PM refcount leak when DP goes to disconnected state and also when link training fails. This is also one of the issues found with the pm runtime series - Add missing newlines to prints in msm_fb and msm_kms - Change permissions of some dpu debugfs entries which write to const data from catalog to read-only to avoid protection faults - Fix the interface table for the catalog of X1E80100. This is an important fix to bringup DP for X1E80100. - Logging fix to print the callback symbol in the invalid IRQ message case rather than printing when its known to be NULL. - Bindings fix to add DP node as child of mdss for mdss node - Minor typo fix in DP driver API which handles port status change GPU: - fix CHRASHDUMP_READ() - fix HHB (highest bank bit) for a619 to fix UBWC corruption Abhinav Kumar (1): drm/msm/dp: fix typo in dp_display_handle_port_status_changed() Dmitry Baryshkov (3): drm/msm/dpu: don't allow overriding data from catalog drm/msm/dpu: make error messages at dpu_core_irq_register_callback() more sensible dt-bindings: display/msm: sm8150-mdss: add DP node Johan Hovold (2): drm/msm/dp: fix runtime PM leak on disconnect drm/msm/dp: fix runtime PM leak on connect failure Kuogee Hsieh (1): drm/msm/dp: assign correct DP controller ID to x1e80100 interface table Luca Weiss (1): drm/msm/adreno: Set highest_bank_bit for A619 Miguel Ojeda (1): drm/msm: fix the `CRASHDUMP_READ` target of `a6xx_get_shader_block()` Stephen Boyd (1): drm/msm: Add newlines to some debug prints .../bindings/display/msm/qcom,sm8150-mdss.yaml | 9 ++ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 +++ drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c| 2 +- .../drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h | 34 -- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 10 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 8 ++--- drivers/gpu/drm/msm/dp/dp_display.c| 6 ++-- drivers/gpu/drm/msm/msm_fb.c | 6 ++-- drivers/gpu/drm/msm/msm_kms.c | 4 +-- 9 files changed, 63 insertions(+), 20 deletions(-)
Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On 4/11/24 22:09, Elliot Berman wrote: On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: On 4/11/24 20:55, Elliot Berman wrote: On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: In preparation for parsing the chip "feature code" (FC) and "product code" (PC) (essentially the parameters that let us conclusively characterize the sillicon we're running on, including various speed bins), move the socinfo version defines to the public header and include some more FC/PC defines. Signed-off-by: Konrad Dybcio --- [...] 0xf is the last one. One more question, are the "internal/external feature codes" referring to internality/externality of the chips (i.e. "are they QC-lab-only engineering samples), or what else does that represent? Konrad
Re: [PATCH] drm/msm: Drop msm_read/writel
On Thu, Apr 11, 2024 at 08:27:22AM -0700, Bjorn Andersson wrote:
> On Thu, Apr 11, 2024 at 04:31:41AM +0300, Dmitry Baryshkov wrote:
> > On Wed, Apr 10, 2024 at 11:52:52PM +0200, Konrad Dybcio wrote:
> [..]
> > > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > index e4275d3ad581..5a5dc3faa971 100644
> > > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > > @@ -12,10 +12,10 @@
> > >
> > > #include "dsi.h"
> > >
> > > -#define dsi_phy_read(offset) msm_readl((offset))
> > > -#define dsi_phy_write(offset, data) msm_writel((data), (offset))
> > > -#define dsi_phy_write_udelay(offset, data, delay_us) {
> > > msm_writel((data), (offset)); udelay(delay_us); }
> > > -#define dsi_phy_write_ndelay(offset, data, delay_ns) {
> > > msm_writel((data), (offset)); ndelay(delay_ns); }
> > > +#define dsi_phy_read(offset) readl((offset))
> > > +#define dsi_phy_write(offset, data) writel((data), (offset))
> > > +#define dsi_phy_write_udelay(offset, data, delay_us) { writel((data),
> > > (offset)); udelay(delay_us); }
> > > +#define dsi_phy_write_ndelay(offset, data, delay_ns) { writel((data),
> > > (offset)); ndelay(delay_ns); }
> >
> > What about also inlining these wrappers?
> >
>
> But that should be done in a separate commit, no?
Yesm of course.
>
> PS. Too much scrolling to find your comments, please trim your replies.
Ack. I'm probably too used to GMail and Thunderbird extension which
collapses quotes.
>
> Thanks,
> Bjorn
--
With best wishes
Dmitry
Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On Thu, Apr 11, 2024 at 10:05:30PM +0200, Konrad Dybcio wrote: > > > On 4/11/24 20:55, Elliot Berman wrote: > > On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: > > > In preparation for parsing the chip "feature code" (FC) and "product > > > code" (PC) (essentially the parameters that let us conclusively > > > characterize the sillicon we're running on, including various speed > > > bins), move the socinfo version defines to the public header and > > > include some more FC/PC defines. > > > > > > Signed-off-by: Konrad Dybcio > > > --- > > [...] > > > > + SOCINFO_FC_EXT_RESERVE, > > > +}; > > > > SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped > > SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8 > > feature codes so far. > > > > We should remove the EXT_RESERVE and test for the Y0-YF (internal > > feature code) values instead. > > OK > > > > > > + > > > +/* Internal feature codes */ > > > +/* Valid values: 0 <= n <= 0xf */ > > > +#define SOCINFO_FC_Yn(n) (0xf1 + n) > > > +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10) > > > > We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies > > it's reserved for some future use, but it's really the max value it > > could be. > > So, should SOCINFO_FC_Yn(0x10) also be considered valid, or is (0xf) > the last one? > 0xf is the last one. Thanks, Elliot > > > > > + > > > +/* Product codes */ > > > +#define SOCINFO_PC_UNKNOWN 0 > > > +/* Valid values: 0 <= n <= 8, the rest is reserved */ > > > +#define SOCINFO_PCn(n) (n + 1) > > > +#define SOCINFO_PC_RESERVE (BIT(31) - 1) > > > > Similar comments here as the SOCINFO_FC_EXT_*. It's more like known > > values are [0,8], but more values could come in future chipsets. > > Ok, sounds good, I'll remove the comment then > > Konrad
Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On 4/11/24 20:55, Elliot Berman wrote: On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote: In preparation for parsing the chip "feature code" (FC) and "product code" (PC) (essentially the parameters that let us conclusively characterize the sillicon we're running on, including various speed bins), move the socinfo version defines to the public header and include some more FC/PC defines. Signed-off-by: Konrad Dybcio --- [...] + SOCINFO_FC_EXT_RESERVE, +}; SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8 feature codes so far. We should remove the EXT_RESERVE and test for the Y0-YF (internal feature code) values instead. OK + +/* Internal feature codes */ +/* Valid values: 0 <= n <= 0xf */ +#define SOCINFO_FC_Yn(n) (0xf1 + n) +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10) We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies it's reserved for some future use, but it's really the max value it could be. So, should SOCINFO_FC_Yn(0x10) also be considered valid, or is (0xf) the last one? + +/* Product codes */ +#define SOCINFO_PC_UNKNOWN 0 +/* Valid values: 0 <= n <= 8, the rest is reserved */ +#define SOCINFO_PCn(n) (n + 1) +#define SOCINFO_PC_RESERVE (BIT(31) - 1) Similar comments here as the SOCINFO_FC_EXT_*. It's more like known values are [0,8], but more values could come in future chipsets. Ok, sounds good, I'll remove the comment then Konrad
Re: [PATCH 07/12] drm/msm: merge dpu_format and mdp_format in struct msm_format
On Thu, 11 Apr 2024 at 22:15, Abhinav Kumar wrote:
>
>
>
> On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
> > Structures dpu_format and mdp_format are largely the same structures.
> > In order to remove duplication between format databases, merge these two
> > stucture definitions into the global struct msm_format.
> >
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +-
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 184 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 2 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 10 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 41 +---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 +--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 14 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 16 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 2 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 74 +++
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 4 +-
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 26 +--
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 7 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 54 ++---
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 4 +-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 2 +-
> > drivers/gpu/drm/msm/disp/mdp_format.c | 28 ++-
> > drivers/gpu/drm/msm/disp/mdp_kms.h| 13 --
> > drivers/gpu/drm/msm/msm_drv.h | 28 +++
> > 24 files changed, 279 insertions(+), 288 deletions(-)
> >
>
>
>
> > int mdp5_smp_assign(struct mdp5_smp *smp, struct mdp5_smp_state *state,
> > diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c
> > b/drivers/gpu/drm/msm/disp/mdp_format.c
> > index 30919641c813..5fc55f41e74f 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp_format.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp_format.c
> > @@ -63,26 +63,24 @@ static struct csc_cfg csc_convert[CSC_MAX] = {
> > };
> >
> > #define FMT(name, a, r, g, b, e0, e1, e2, e3, alpha, tight, c, cnt, fp,
> > cs, yuv) { \
> > - .base = {\
> > - .pixel_format = DRM_FORMAT_ ## name, \
> > - .flags = yuv ? MSM_FORMAT_FLAG_YUV : 0, \
> > - }, \
> > + .pixel_format = DRM_FORMAT_ ## name, \
> > .bpc_a = BPC ## a ## A, \
> > - .bpc_r = BPC ## r, \
> > - .bpc_g = BPC ## g, \
> > - .bpc_b = BPC ## b, \
> > - .unpack = { e0, e1, e2, e3 },\
> > + .bpc_r_cr = BPC ## r,\
> > + .bpc_g_y = BPC ## g, \
> > + .bpc_b_cb = BPC ## b,\
> > + .element = { e0, e1, e2, e3 }, \
> > + .fetch_type = fp,\
> > + .chroma_sample = cs, \
> > .alpha_enable = alpha, \
> > .unpack_tight = tight, \
> > - .cpp = c,\
> > .unpack_count = cnt, \
> > - .fetch_type = fp,\
> > - .chroma_sample = cs, \
>
> Minor nit:
>
> These two lines are only moving the locations of assignment so
> unnecessary change?
Sure, let's drop that. I think it was just C of some kind.
>
> Rest LGTM,
>
> Reviewed-by: Abhinav Kumar
>
> For validation, are you relying mostly on the CI here OR also other
> internal farms? Even though mostly its just making code common, basic
> display coming up on one target each of MDP4/MDP5/DPU will be great to
> be safe.
It was a visual inspection, but not for each and every platform.
--
With best wishes
Dmitry
Re: [PATCH 07/12] drm/msm: merge dpu_format and mdp_format in struct msm_format
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote:
Structures dpu_format and mdp_format are largely the same structures.
In order to remove duplication between format databases, merge these two
stucture definitions into the global struct msm_format.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 184 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 10 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 41 +---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 14 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 16 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 74 +++
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 4 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 26 +--
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 7 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 54 ++---
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.c | 4 +-
drivers/gpu/drm/msm/disp/mdp5/mdp5_smp.h | 2 +-
drivers/gpu/drm/msm/disp/mdp_format.c | 28 ++-
drivers/gpu/drm/msm/disp/mdp_kms.h| 13 --
drivers/gpu/drm/msm/msm_drv.h | 28 +++
24 files changed, 279 insertions(+), 288 deletions(-)
int mdp5_smp_assign(struct mdp5_smp *smp, struct mdp5_smp_state *state,
diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c
b/drivers/gpu/drm/msm/disp/mdp_format.c
index 30919641c813..5fc55f41e74f 100644
--- a/drivers/gpu/drm/msm/disp/mdp_format.c
+++ b/drivers/gpu/drm/msm/disp/mdp_format.c
@@ -63,26 +63,24 @@ static struct csc_cfg csc_convert[CSC_MAX] = {
};
#define FMT(name, a, r, g, b, e0, e1, e2, e3, alpha, tight, c, cnt, fp, cs, yuv) { \
- .base = {\
- .pixel_format = DRM_FORMAT_ ## name, \
- .flags = yuv ? MSM_FORMAT_FLAG_YUV : 0, \
- }, \
+ .pixel_format = DRM_FORMAT_ ## name, \
.bpc_a = BPC ## a ## A, \
- .bpc_r = BPC ## r, \
- .bpc_g = BPC ## g, \
- .bpc_b = BPC ## b, \
- .unpack = { e0, e1, e2, e3 },\
+ .bpc_r_cr = BPC ## r,\
+ .bpc_g_y = BPC ## g, \
+ .bpc_b_cb = BPC ## b,\
+ .element = { e0, e1, e2, e3 }, \
+ .fetch_type = fp,\
+ .chroma_sample = cs, \
.alpha_enable = alpha, \
.unpack_tight = tight, \
- .cpp = c,\
.unpack_count = cnt, \
- .fetch_type = fp,\
- .chroma_sample = cs, \
Minor nit:
These two lines are only moving the locations of assignment so
unnecessary change?
Rest LGTM,
Reviewed-by: Abhinav Kumar
For validation, are you relying mostly on the CI here OR also other
internal farms? Even though mostly its just making code common, basic
display coming up on one target each of MDP4/MDP5/DPU will be great to
be safe.
Re: [PATCH 2/6] soc: qcom: smem: Add pcode/fcode getters
On Fri, Apr 05, 2024 at 10:41:30AM +0200, Konrad Dybcio wrote:
> Introduce getters for SoC product and feature codes and export them.
>
> Signed-off-by: Konrad Dybcio
> ---
> drivers/soc/qcom/smem.c | 66
> +++
> include/linux/soc/qcom/smem.h | 2 ++
> 2 files changed, 68 insertions(+)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7191fa0c087f..e89b4d26877a 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -795,6 +795,72 @@ int qcom_smem_get_soc_id(u32 *id)
> }
> EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
>
> +/**
> + * qcom_smem_get_feature_code() - return the feature code
> + * @id: On success, we return the feature code here.
^^ code
> + *
> + * Look up the feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_feature_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EINVAL;
> +
> + raw_code = __le32_to_cpu(info->feature_code);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_RESERVE)
> + raw_code = SOCINFO_FC_UNKNOWN;
> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
> +
> +/**
> + * qcom_smem_get_product_code() - return the product code
> + * @id: On success, we return the product code here.
^^ code
> + *
> + * Look up feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_product_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EINVAL;
> +
> + raw_code = __le32_to_cpu(info->pcode);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_RESERVE)
> + raw_code = SOCINFO_FC_UNKNOWN;
> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_product_code);
> +
> static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
> {
> struct smem_header *header;
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index a36a3b9d4929..aef8c9fc6c08 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -13,5 +13,7 @@ int qcom_smem_get_free_space(unsigned host);
> phys_addr_t qcom_smem_virt_to_phys(void *p);
>
> int qcom_smem_get_soc_id(u32 *id);
> +int qcom_smem_get_feature_code(u32 *code);
> +int qcom_smem_get_product_code(u32 *code);
>
> #endif
>
> --
> 2.40.1
>
>
Re: [PATCH 1/6] soc: qcom: Move some socinfo defines to the header, expand them
On Fri, Apr 05, 2024 at 10:41:29AM +0200, Konrad Dybcio wrote:
> In preparation for parsing the chip "feature code" (FC) and "product
> code" (PC) (essentially the parameters that let us conclusively
> characterize the sillicon we're running on, including various speed
> bins), move the socinfo version defines to the public header and
> include some more FC/PC defines.
>
> Signed-off-by: Konrad Dybcio
> ---
> drivers/soc/qcom/socinfo.c | 8
> include/linux/soc/qcom/socinfo.h | 36
> 2 files changed, 36 insertions(+), 8 deletions(-)
>
...
> diff --git a/include/linux/soc/qcom/socinfo.h
> b/include/linux/soc/qcom/socinfo.h
...
> @@ -74,4 +84,30 @@ struct socinfo {
> __le32 boot_core;
> };
>
> +/* Internal feature codes */
> +enum feature_code {
> + /* External feature codes */
> + SOCINFO_FC_UNKNOWN = 0x0,
> + SOCINFO_FC_AA,
> + SOCINFO_FC_AB,
> + SOCINFO_FC_AC,
> + SOCINFO_FC_AD,
> + SOCINFO_FC_AE,
> + SOCINFO_FC_AF,
> + SOCINFO_FC_AG,
> + SOCINFO_FC_AH,
> + SOCINFO_FC_EXT_RESERVE,
> +};
SOCINFO_FC_EXT_RESERVE was a convenient limit since we mapped
SOCINFO_FC_AA -> string "AA" via an array, and we've only needed the 8
feature codes so far.
We should remove the EXT_RESERVE and test for the Y0-YF (internal
feature code) values instead.
> +
> +/* Internal feature codes */
> +/* Valid values: 0 <= n <= 0xf */
> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
> +#define SOCINFO_FC_INT_RESERVE SOCINFO_FC_Yn(0x10)
We probably should've named this SOCINFO_FC_INT_MAX. Reserve implies
it's reserved for some future use, but it's really the max value it
could be.
> +
> +/* Product codes */
> +#define SOCINFO_PC_UNKNOWN 0
> +/* Valid values: 0 <= n <= 8, the rest is reserved */
> +#define SOCINFO_PCn(n) (n + 1)
> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
Similar comments here as the SOCINFO_FC_EXT_*. It's more like known
values are [0,8], but more values could come in future chipsets.
> +
> #endif
>
Re: [PATCH 06/12] drm/msm/dpu: pull format flag definitions to msm_drv.h
On 4/11/2024 11:41 AM, Abhinav Kumar wrote: On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: In preparation to merger of formats databases, pull format flag definitions to msm_drv.h header, so that they are visibile to both dpu and mdp drivers. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 98 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 28 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 8 +- drivers/gpu/drm/msm/disp/mdp_format.c | 6 +- drivers/gpu/drm/msm/disp/mdp_kms.h | 3 +- drivers/gpu/drm/msm/msm_drv.h | 24 + 8 files changed, 91 insertions(+), 84 deletions(-) +#define DPU_FORMAT_IS_YUV(X) MSM_FORMAT_IS_YUV(&(X)->base) +#define DPU_FORMAT_IS_DX(X) MSM_FORMAT_IS_DX(&(X)->base) +#define DPU_FORMAT_IS_LINEAR(X) MSM_FORMAT_IS_LINEAR(&(X)->base) +#define DPU_FORMAT_IS_TILE(X) MSM_FORMAT_IS_TILE(&(X)->base) +#define DPU_FORMAT_IS_UBWC(X) MSM_FORMAT_IS_UBWC(&(X)->base) Do we need another wrapper macro on top of MSM_FORMAT_*** macros? Why cant we use them directly? Same comment for MDP_FORMAT_IS_YUV macro as well. Rest LGTM. never mind, the next change does exactly this. Hence this one LGTM Reviewed-by: Abhinav Kumar
Re: [PATCH 06/12] drm/msm/dpu: pull format flag definitions to msm_drv.h
On 12/2/2023 1:40 PM, Dmitry Baryshkov wrote: In preparation to merger of formats databases, pull format flag definitions to msm_drv.h header, so that they are visibile to both dpu and mdp drivers. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 98 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 28 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 8 +- drivers/gpu/drm/msm/disp/mdp_format.c | 6 +- drivers/gpu/drm/msm/disp/mdp_kms.h | 3 +- drivers/gpu/drm/msm/msm_drv.h | 24 + 8 files changed, 91 insertions(+), 84 deletions(-) +#define DPU_FORMAT_IS_YUV(X) MSM_FORMAT_IS_YUV(&(X)->base) +#define DPU_FORMAT_IS_DX(X)MSM_FORMAT_IS_DX(&(X)->base) +#define DPU_FORMAT_IS_LINEAR(X)MSM_FORMAT_IS_LINEAR(&(X)->base) +#define DPU_FORMAT_IS_TILE(X) MSM_FORMAT_IS_TILE(&(X)->base) +#define DPU_FORMAT_IS_UBWC(X) MSM_FORMAT_IS_UBWC(&(X)->base) Do we need another wrapper macro on top of MSM_FORMAT_*** macros? Why cant we use them directly? Same comment for MDP_FORMAT_IS_YUV macro as well. Rest LGTM.
Re: [PATCH] drm/msm: Drop msm_read/writel
On Thu, Apr 11, 2024 at 04:31:41AM +0300, Dmitry Baryshkov wrote:
> On Wed, Apr 10, 2024 at 11:52:52PM +0200, Konrad Dybcio wrote:
[..]
> > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > index e4275d3ad581..5a5dc3faa971 100644
> > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > @@ -12,10 +12,10 @@
> >
> > #include "dsi.h"
> >
> > -#define dsi_phy_read(offset) msm_readl((offset))
> > -#define dsi_phy_write(offset, data) msm_writel((data), (offset))
> > -#define dsi_phy_write_udelay(offset, data, delay_us) { msm_writel((data),
> > (offset)); udelay(delay_us); }
> > -#define dsi_phy_write_ndelay(offset, data, delay_ns) { msm_writel((data),
> > (offset)); ndelay(delay_ns); }
> > +#define dsi_phy_read(offset) readl((offset))
> > +#define dsi_phy_write(offset, data) writel((data), (offset))
> > +#define dsi_phy_write_udelay(offset, data, delay_us) { writel((data),
> > (offset)); udelay(delay_us); }
> > +#define dsi_phy_write_ndelay(offset, data, delay_ns) { writel((data),
> > (offset)); ndelay(delay_ns); }
>
> What about also inlining these wrappers?
>
But that should be done in a separate commit, no?
PS. Too much scrolling to find your comments, please trim your replies.
Thanks,
Bjorn
