[PATCH v6 6/6] drm/msm/dsi: add a comment to explain pkt_per_line encoding
From: Jonathan Marek Make it clear why the pkt_per_line value is being "divided by 2". Signed-off-by: Jonathan Marek Reviewed-by: Dmitry Baryshkov Signed-off-by: Jun Nie Tested-by: Neil Armstrong # on SM8550-QRD Tested-by: Neil Armstrong # on SM8650-QRD Tested-by: Neil Armstrong # on SM8650-HDK Reviewed-by: Jessica Zhang --- drivers/gpu/drm/msm/dsi/dsi_host.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 7252d36687e6..4768cff08381 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -885,7 +885,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod /* DSI_VIDEO_COMPRESSION_MODE & DSI_COMMAND_COMPRESSION_MODE * registers have similar offsets, so for below common code use * DSI_VIDEO_COMPRESSION_MODE_ for setting bits +* +* pkt_per_line is log2 encoded, >>1 works for supported values (1,2,4) */ + if (pkt_per_line > 4) + drm_warn_once(msm_host->dev, "pkt_per_line too big"); reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_PKT_PER_LINE(pkt_per_line >> 1); reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EOL_BYTE_NUM(eol_byte_num); reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EN; -- 2.34.1
[PATCH v6 5/6] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC
From: Jonathan Marek
Video mode DSC won't work if this field is not set correctly. Set it to fix
video mode DSC (for slice_per_pkt==1 cases at least).
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Jonathan Marek
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jun Nie
Tested-by: Neil Armstrong # on SM8550-QRD
Tested-by: Neil Armstrong # on SM8650-QRD
Tested-by: Neil Armstrong # on SM8650-HDK
Reviewed-by: Jessica Zhang
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 47f5858334f6..7252d36687e6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -857,6 +857,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
u32 slice_per_intf, total_bytes_per_intf;
u32 pkt_per_line;
u32 eol_byte_num;
+ u32 bytes_per_pkt;
/* first calculate dsc parameters and then program
* compress mode registers
@@ -864,6 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
+ bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
eol_byte_num = total_bytes_per_intf % 3;
@@ -901,6 +903,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL,
reg_ctrl);
dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2,
reg_ctrl2);
} else {
+ reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_WC(bytes_per_pkt);
dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
}
}
--
2.34.1
[PATCH v6 4/6] drm/msm/dsi: set video mode widebus enable bit when widebus is enabled
From: Jonathan Marek
The value returned by msm_dsi_wide_bus_enabled() doesn't match what the
driver is doing in video mode. Fix that by actually enabling widebus for
video mode.
Fixes: efcbd6f9cdeb ("drm/msm/dsi: Enable widebus for DSI")
Signed-off-by: Jonathan Marek
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
Reviewed-by: Jessica Zhang
Signed-off-by: Jun Nie
Tested-by: Neil Armstrong # on SM8550-QRD
Tested-by: Neil Armstrong # on SM8650-QRD
Tested-by: Neil Armstrong # on SM8650-HDK
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a50f4dda5941..47f5858334f6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -754,6 +754,8 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
data |= DSI_VID_CFG0_TRAFFIC_MODE(dsi_get_traffic_mode(flags));
data |= DSI_VID_CFG0_DST_FORMAT(dsi_get_vid_fmt(mipi_fmt));
data |= DSI_VID_CFG0_VIRT_CHANNEL(msm_host->channel);
+ if (msm_dsi_host_is_wide_bus_enabled(&msm_host->base))
+ data |= DSI_VID_CFG0_DATABUS_WIDEN;
dsi_write(msm_host, REG_DSI_VID_CFG0, data);
/* Do not swap RGB colors */
@@ -778,7 +780,6 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
if (cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3)
data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
- /* TODO: Allow for video-mode support once tested/fixed
*/
if (msm_dsi_host_is_wide_bus_enabled(&msm_host->base))
data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
--
2.34.1
[PATCH v6 3/6] drm/msm/dpu: enable compression bit in cfg2 for DSC
Enable compression bit in cfg2 register for DSC in the DSI case
per hardware version.
Signed-off-by: Jun Nie
Tested-by: Neil Armstrong # on SM8550-QRD
Tested-by: Neil Armstrong # on SM8650-QRD
Tested-by: Neil Armstrong # on SM8650-HDK
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 ++-
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 925ec6ada0e1..f2aab3e7c783 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -307,7 +307,8 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
phys_enc->hw_intf->ops.setup_timing_gen(phys_enc->hw_intf,
- &timing_params, fmt);
+ &timing_params, fmt,
+ phys_enc->dpu_kms->catalog->mdss_ver);
phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg);
/* setup which pp blk will connect to this intf */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index f97221423249..fa6debda0774 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -98,7 +98,8 @@
static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
const struct dpu_hw_intf_timing_params *p,
- const struct msm_format *fmt)
+ const struct msm_format *fmt,
+ const struct dpu_mdss_version *mdss_ver)
{
struct dpu_hw_blk_reg_map *c = &intf->hw;
u32 hsync_period, vsync_period;
@@ -177,6 +178,11 @@ static void dpu_hw_intf_setup_timing_engine(struct
dpu_hw_intf *intf,
if (p->wide_bus_en && !dp_intf)
data_width = p->width >> 1;
+ /* TODO: handle DSC+DP case, we only handle DSC+DSI case so far */
+ if (p->compression_en && !dp_intf &&
+ mdss_ver->core_major_ver >= 7)
+ intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+
hsync_data_start_x = hsync_start_x;
hsync_data_end_x = hsync_start_x + data_width - 1;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index f9015c67a574..ef947bf77693 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -81,7 +81,8 @@ struct dpu_hw_intf_cmd_mode_cfg {
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
const struct dpu_hw_intf_timing_params *p,
- const struct msm_format *fmt);
+ const struct msm_format *fmt,
+ const struct dpu_mdss_version *mdss_ver);
void (*setup_prg_fetch)(struct dpu_hw_intf *intf,
const struct dpu_hw_intf_prog_fetch *fetch);
--
2.34.1
[PATCH v6 2/6] drm/msm/dpu: adjust data width for widen bus case
data is valid for only half the active window if widebus is enabled Signed-off-by: Jun Nie Tested-by: Neil Armstrong # on SM8550-QRD Tested-by: Neil Armstrong # on SM8650-QRD Tested-by: Neil Armstrong # on SM8650-HDK Reviewed-by: Dmitry Baryshkov Reviewed-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c index 225c1c7768ff..f97221423249 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c @@ -168,6 +168,15 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf, data_width = p->width; + /* +* If widebus is enabled, data is valid for only half the active window +* since the data rate is doubled in this mode. But for the compression +* mode in DP case, the p->width is already adjusted in +* drm_mode_to_intf_timing_params() +*/ + if (p->wide_bus_en && !dp_intf) + data_width = p->width >> 1; + hsync_data_start_x = hsync_start_x; hsync_data_end_x = hsync_start_x + data_width - 1; -- 2.34.1
[PATCH v6 1/6] drm/msm/dpu: fix video mode DSC for DSI
From: Jonathan Marek
Add width change in DPU timing for DSC compression case to work with
DSI video mode.
Signed-off-by: Jonathan Marek
Signed-off-by: Jun Nie
Tested-by: Neil Armstrong # on SM8550-QRD
Tested-by: Neil Armstrong # on SM8650-QRD
Tested-by: Neil Armstrong # on SM8650-HDK
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Jessica Zhang
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 18 ++
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..48cef6e79c70 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -564,7 +564,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
return (num_dsc > 0) && (num_dsc > intf_count);
}
-static struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder
*drm_enc)
+struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
{
struct msm_drm_private *priv = drm_enc->dev->dev_private;
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 002e89cc1705..2167c46c1a45 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -334,6 +334,14 @@ static inline enum dpu_3d_blend_mode
dpu_encoder_helper_get_3d_blend_mode(
*/
unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc);
+/**
+ * dpu_encoder_get_dsc_config - get DSC config for the DPU encoder
+ * This helper function is used by physical encoder to get DSC config
+ * used for this encoder.
+ * @drm_enc: Pointer to encoder structure
+ */
+struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc);
+
/**
* dpu_encoder_get_drm_fmt - return DRM fourcc format
* @phys_enc: Pointer to physical encoder structure
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ef69c2f408c3..925ec6ada0e1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -11,6 +11,7 @@
#include "dpu_trace.h"
#include "disp/msm_disp_snapshot.h"
+#include
#include
#define DPU_DEBUG_VIDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \
@@ -115,6 +116,23 @@ static void drm_mode_to_intf_timing_params(
timing->h_front_porch = timing->h_front_porch >> 1;
timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
}
+
+ /*
+* for DSI, if compression is enabled, then divide the horizonal active
+* timing parameters by compression ratio. bits of 3 components(R/G/B)
+* is compressed into bits of 1 pixel.
+*/
+ if (phys_enc->hw_intf->cap->type != INTF_DP && timing->compression_en) {
+ struct drm_dsc_config *dsc =
+ dpu_encoder_get_dsc_config(phys_enc->parent);
+ /*
+* TODO: replace drm_dsc_get_bpp_int with logic to handle
+* fractional part if there is fraction
+*/
+ timing->width = timing->width * drm_dsc_get_bpp_int(dsc) /
+ (dsc->bits_per_component * 3);
+ timing->xres = timing->width;
+ }
}
static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params *timing)
--
2.34.1
[PATCH v6 0/6] Add DSC support to DSI video panel
This is follow up update to Jonathan's patch set. Changes vs V5: - Add hardware version check for compression bit change in cfg2 register Changes vs V4: - Polish width calculation with helper function - Split cfg2 compression bit into another patch Changes vs V3: - Rebase to latest msm-next-lumag branch. - Drop the slice_per_pkt change as it does impact basic DSC feature. - Remove change in generated dsi header - update DSC compressed width calculation with bpp and bpc - split wide bus impact on width into another patch - rename patch tile of VIDEO_COMPRESSION_MODE_CTRL_WC change - Polish warning usage - Add tags from reviewers Changes vs V2: - Drop the INTF_CFG2_DATA_HCTL_EN change as it is handled in latest mainline code. - Drop the bonded DSI patch as I do not have device to test it. - Address comments from version 2. Signed-off-by: Jun Nie --- Changes in v6: - Link to v5: https://lore.kernel.org/r/20240527-msm-drm-dsc-dsi-video-upstream-4-v5-0-f797ffba4...@linaro.org Changes in v5: - Link to v4: https://lore.kernel.org/r/20240524-msm-drm-dsc-dsi-video-upstream-4-v4-0-e61c05b40...@linaro.org --- Jonathan Marek (4): drm/msm/dpu: fix video mode DSC for DSI drm/msm/dsi: set video mode widebus enable bit when widebus is enabled drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC drm/msm/dsi: add a comment to explain pkt_per_line encoding Jun Nie (2): drm/msm/dpu: adjust data width for widen bus case drm/msm/dpu: enable compression bit in cfg2 for DSC drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h| 8 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c| 21 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 17 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 ++- drivers/gpu/drm/msm/dsi/dsi_host.c | 10 +- 6 files changed, 56 insertions(+), 5 deletions(-) --- base-commit: e6428bcb611f6c164856a41fc5a1ae8471a9b5a9 change-id: 20240524-msm-drm-dsc-dsi-video-upstream-4-22e2266fbe89 Best regards, -- Jun Nie
Re: [PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source
On 5/29/2024 5:02 PM, Dmitry Baryshkov wrote:
On Thu, 30 May 2024 at 00:57, Abhinav Kumar wrote:
On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
On Thu, 23 May 2024 at 02:57, Abhinav Kumar wrote:
On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
On Wed, 22 May 2024 at 21:38, Abhinav Kumar wrote:
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
Command mode panels provide TE signal back to the DSI host to signal
that the frame display has completed and update of the image will not
cause tearing. Usually it is connected to the first GPIO with the
mdp_vsync function, which is the default. In such case the property can
be skipped.
This is a good addition overall. Some comments below.
Signed-off-by: Dmitry Baryshkov
---
.../bindings/display/msm/dsi-controller-main.yaml| 16
1 file changed, 16 insertions(+)
diff --git
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 1fa28e976559..c1771c69b247 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -162,6 +162,21 @@ properties:
items:
enum: [ 0, 1, 2, 3 ]
+ qcom,te-source:
+$ref: /schemas/types.yaml#/definitions/string
+description:
+ Specifies the source of vsync signal from the panel used for
+ tearing elimination. The default is mdp_gpio0.
panel --> command mode panel?
+enum:
+ - mdp_gpio0
+ - mdp_gpio1
+ - mdp_gpio2
are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
sources?
No idea, unfortunately. They are gpioN or just mdp_vsync all over the
place. For the reference, in case of the SDM845 and Pixel3 the signal
is routed through SoC GPIO12.
GPIO12 on sdm845 is mdp_vsync_e.
Thats why I think its better we use mdp_vsync_p/s/e instead of mdp_gpio0/1/2
Sure. This matches pins description. Are you fine with changing
defines in DPU driver to VSYNC_P / _S / _E too ?
Sorry for the delay in responding.
As per the software docs, the registers still use GPIO0/1/2.
Only the pin descriptions use vsync_p/s/e.
Hence I think we can make DPU driver to use 0/1/2.
OK, what about the DT? I like the vsync_p/_s/_e idea.
Yes, vsync_p/_s/_e for DT is fine with me.
My comment was only for driver.
So driver would do:
vsync_p -> gpio0
vsync_s -> gpio1
vsync_e -> gpio2
In that case wouldnt it be better to name it like that?
+ - timer0
+ - timer1
+ - timer2
+ - timer3
+ - timer4
+
These are indicating watchdog timer sources right?
Yes.
ack.
required:
- port@0
- port@1
@@ -452,6 +467,7 @@ examples:
dsi0_out: endpoint {
remote-endpoint = <&sn65dsi86_in>;
data-lanes = <0 1 2 3>;
+ qcom,te-source = "mdp_gpio2";
I have a basic doubt on this. Should te-source should be in the input
port or the output one for the controller? Because TE is an input to the
DSI. And if the source is watchdog timer then it aligns even more as a
property of the input endpoint.
I don't really want to split this. Both data-lanes and te-source are
properties of the link between the DSI and panel. You can not really
say which side has which property.
TE is an input to the DSI from the panel. Between input and output port,
I think it belongs more to the input port.
Technically we don't have in/out ports. There are two ports which
define a link between two instances. For example, if the panel
supports getting information through DCS commands, then "panel input"
also becomes "panel output".
The ports are labeled dsi0_in and dsi0_out. Putting te source in
dsi0_out really looks very confusing to me.
dsi0_in is a port that connects DSI and DPU, so we should not be
putting panel-related data there.
Yes, true. But here we are using the "out" port which like you mentioned
is not logical either. Thats why I am not convinced or not sure if this
is the right way to model this.
I see two ports: mdss_dsi0_out and panel_in. Neither of them is
logical from this point of view. The TE source likewise isn't an input
to the panel, so we should not be using the panel_in port.
I didnt follow why this is a link property. Sorry , I didnt follow the
split part.
There is a link between the DSI host and the panel. I don't want to
end up in a situation when the properties of the link are split
between two different nodes.
It really depends on what the property denotes. I do not think this
should be the reason to do it this way.
It denot
Re: [PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source
On Thu, 30 May 2024 at 00:57, Abhinav Kumar wrote:
>
>
>
> On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
> > On Thu, 23 May 2024 at 02:57, Abhinav Kumar
> > wrote:
> >>
> >>
> >>
> >> On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 22 May 2024 at 21:38, Abhinav Kumar
> >>> wrote:
>
>
>
> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
> > Command mode panels provide TE signal back to the DSI host to signal
> > that the frame display has completed and update of the image will not
> > cause tearing. Usually it is connected to the first GPIO with the
> > mdp_vsync function, which is the default. In such case the property can
> > be skipped.
> >
>
> This is a good addition overall. Some comments below.
>
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > .../bindings/display/msm/dsi-controller-main.yaml| 16
> >
> > 1 file changed, 16 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> >
> > b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> > index 1fa28e976559..c1771c69b247 100644
> > ---
> > a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> > +++
> > b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
> > @@ -162,6 +162,21 @@ properties:
> > items:
> > enum: [ 0, 1, 2, 3 ]
> >
> > + qcom,te-source:
> > +$ref: /schemas/types.yaml#/definitions/string
> > +description:
> > + Specifies the source of vsync signal from the panel
> > used for
> > + tearing elimination. The default is mdp_gpio0.
>
> panel --> command mode panel?
>
> > +enum:
> > + - mdp_gpio0
> > + - mdp_gpio1
> > + - mdp_gpio2
>
> are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
> sources?
> >>>
> >>> No idea, unfortunately. They are gpioN or just mdp_vsync all over the
> >>> place. For the reference, in case of the SDM845 and Pixel3 the signal
> >>> is routed through SoC GPIO12.
> >>>
> >>
> >> GPIO12 on sdm845 is mdp_vsync_e.
> >>
> >> Thats why I think its better we use mdp_vsync_p/s/e instead of
> >> mdp_gpio0/1/2
> >
> > Sure. This matches pins description. Are you fine with changing
> > defines in DPU driver to VSYNC_P / _S / _E too ?
> >
>
> Sorry for the delay in responding.
>
> As per the software docs, the registers still use GPIO0/1/2.
>
> Only the pin descriptions use vsync_p/s/e.
>
> Hence I think we can make DPU driver to use 0/1/2.
OK, what about the DT? I like the vsync_p/_s/_e idea.
>
> >>
> In that case wouldnt it be better to name it like that?
>
> > + - timer0
> > + - timer1
> > + - timer2
> > + - timer3
> > + - timer4
> > +
>
> These are indicating watchdog timer sources right?
> >>>
> >>> Yes.
> >>>
>
> ack.
>
>
> > required:
> > - port@0
> > - port@1
> > @@ -452,6 +467,7 @@ examples:
> > dsi0_out: endpoint {
> >remote-endpoint =
> > <&sn65dsi86_in>;
> >data-lanes = <0 1 2 3>;
> > + qcom,te-source = "mdp_gpio2";
>
> I have a basic doubt on this. Should te-source should be in the input
> port or the output one for the controller? Because TE is an input to the
> DSI. And if the source is watchdog timer then it aligns even more as a
> property of the input endpoint.
> >>>
> >>> I don't really want to split this. Both data-lanes and te-source are
> >>> properties of the link between the DSI and panel. You can not really
> >>> say which side has which property.
> >>>
> >>
> >> TE is an input to the DSI from the panel. Between input and output port,
> >> I think it belongs more to the input port.
> >
> > Technically we don't have in/out ports. There are two ports which
> > define a link between two instances. For example, if the panel
> > supports getting information through DCS commands, then "panel input"
> > also becomes "panel output".
> >
>
> The ports are labeled dsi0_in and dsi0_out. Putting te source in
> dsi0_out really looks very confusing to me.
dsi0_in is a port that connects DSI and DPU, so we should not be
putting panel-related data there.
I see two ports: mdss_dsi0_out and panel_in. Neither of them is
logical from this point of view. The TE source likewise isn't an input
to the panel, so we should not be using the panel_in port.
>
> >>
> >> I didnt follow why thi
Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins
On Thu, 30 May 2024 at 01:11, Abhinav Kumar wrote: > > > > On 5/22/2024 12:59 PM, Dmitry Baryshkov wrote: > > On Wed, 22 May 2024 at 21:39, Abhinav Kumar > > wrote: > >> > >> > >> > >> On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: > >>> Command-mode DSI panels need to signal the display controlller when > >>> vsync happens, so that the device can start sending the next frame. Some > >>> devices (Google Pixel 3) use a non-default pin, so additional > >>> configuration is required. Add a way to specify this information in DT > >>> and handle it in the DSI and DPU drivers. > >>> > >> > >> Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or > >> gpio1? > > > > gpio2. If it was gpio0 then there were no issues at all. > > > > Got it. Instead of asking gpio1 or gpio2, I mistyped and asked gpio0 or > gpio1. > > While reviewing the code , I think the function > dpu_hw_setup_vsync_source is poorly named . It really doesnt configured > vsync_source. It actually configured watchdog timer. > > Can you pls include one more patch in this series to rename > dpu_hw_setup_vsync_source ---> dpu_hw_setup_wd_timer() Ack, sounds like a good idea. > > >> > >>> Signed-off-by: Dmitry Baryshkov > >>> --- > >>> Dmitry Baryshkov (7): > >>> dt-bindings: display/msm/dsi: allow specifying TE source > >>> drm/msm/dpu: convert vsync source defines to the enum > >>> drm/msm/dsi: drop unused GPIOs handling > >>> drm/msm/dpu: pull the is_cmd_mode out of > >>> _dpu_encoder_update_vsync_source() > >>> drm/msm/dpu: rework vsync_source handling > >>> drm/msm/dsi: parse vsync source from device tree > >>> drm/msm/dpu: support setting the TE source > >>> > >>>.../bindings/display/msm/dsi-controller-main.yaml | 16 > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 11 ++--- > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 5 +-- > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 2 +- > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 2 +- > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 26 ++-- > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 2 +- > >>>drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 44 > >>> > >>>drivers/gpu/drm/msm/dsi/dsi.h | 1 + > >>>drivers/gpu/drm/msm/dsi/dsi_host.c | 48 > >>> +- > >>>drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +++ > >>>drivers/gpu/drm/msm/msm_drv.h | 6 +++ > >>>12 files changed, 106 insertions(+), 62 deletions(-) > >>> --- > >>> base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd > >>> change-id: 20240514-dpu-handle-te-signal-82663c0211bd > >>> > >>> Best regards, > > > > > > -- With best wishes Dmitry
Re: [PATCH v2] Revert "drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set"
On 5/24/2024 1:22 PM, Dmitry Baryshkov wrote:
On Fri, May 24, 2024 at 12:58:53PM -0700, Abhinav Kumar wrote:
On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote:
In the DPU driver blank IRQ handling is called from a vblank worker and
can happen outside of the irq_enable / irq_disable pair. Using the
worker makes that completely asynchronous with the rest of the code.
Revert commit d13f638c9b88 ("drm/msm/dpu: drop
dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for
CMD DSI panels.
Call trace:
dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294
dpu_encoder_toggle_vblank_for_crtc+0x160/0x194
dpu_crtc_vblank+0xbc/0x228
dpu_kms_enable_vblank+0x18/0x24
vblank_ctrl_worker+0x34/0x6c
process_one_work+0x218/0x620
worker_thread+0x1ac/0x37c
kthread+0x114/0x118
ret_from_fork+0x10/0x20
Thanks for the stack.
Agreed that vblank can be controlled asynchronously through the worker.
And I am guessing that the worker thread ran and printed this error message
because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset had not
happened by then?
modeset happened, but the vblanks can be enabled and disabled
afterwards.
272 end:
273 if (ret) {
274 DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n",
275 DRMID(phys_enc->parent),
276 phys_enc->hw_pp->idx - PINGPONG_0, ret,
277 enable ? "true" : "false", refcount);
278 }
But how come this did not happen even with this revert.
IOW, I am still missing how moving the irq assignment back to modeset from
enable is fixing this?
It removes clearing of the IRQ fields in the irq_disable path, which
removes a requirement that vblank IRQ manipulations are called only
within the irq_enable/irq_disable brackets. I didn't have time to debug
this further on, so I think it's better to revert it now and return to
this cleanup later.
I see your point.
So before we moved the irq assignment to enable/disable, modeset was
setting it up but nothing was clearing it.
But after the change, disable was clearing it leading to the error.
Probably should have happened quite easily then?
Then if we kept only the assignment in enable and drop the parts in
disable it should have worked too.
If you would like to post that separately after some more investigation,
I am fine with this for now,
Reviewed-by: Abhinav Kumar
Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
Signed-off-by: Dmitry Baryshkov
---
Changes in v2:
- Expanded commit message to describe the reason for revert and added a
call trace (Abhinav)
- Link to v1:
https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6c...@linaro.org
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32
--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 11 +++-
5 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..a7d8ecf3f5be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct
drm_encoder *drm_enc,
phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
phys->cached_mode = crtc_state->adjusted_mode;
+ if (phys->ops.atomic_mode_set)
+ phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
}
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 002e89cc1705..30470cd15a48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -69,6 +69,8 @@ struct dpu_encoder_phys;
* @is_master: Whether this phys_enc is the current
master
* encoder. Can be switched at enable time. Based
* on split_role and current mode (CMD/VID).
+ * @atomic_mode_set: DRM Call. Set a DRM mode.
+ * This likely caches the mode, for use at enable.
* @enable: DRM Call. Enable a DRM mode.
* @disable: DRM Call. Disable mode.
* @control_vblank_irq Register/Deregister for VBLANK IRQ
@@ -93,6 +95,9 @@ struct dpu_encoder_phys;
struct dpu_encoder_phys_ops {
void (*prepare_commit)(struct dpu_encoder_phys *encoder);
bool (*is_master)(struct dpu_encoder_phys *encoder);
+ void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+ struct drm_crtc_state *crtc_state,
+ st
[PATCH v3 7/7] drm/msm/hdmi: also send the SPD and HDMI Vendor Specific InfoFrames
Extend the driver to send SPD and HDMI Vendor Specific InfoFrames.
While the HDMI block has special block to send HVS InfoFrame, use
GENERIC0 block instead. VENSPEC_INFO registers pack frame data in a way
that requires manual repacking in the driver, while GENERIC0 doesn't
have such format requirements. The msm-4.4 kernel uses GENERIC0 to send
HDR InfoFrame which we do not at this point anyway.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 93 ++
1 file changed, 93 insertions(+)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 3ac63edbf5bb..79ce3b6d1222 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -68,6 +68,8 @@ static void power_off(struct drm_bridge *bridge)
}
#define AVI_IFRAME_LINE_NUMBER 1
+#define SPD_IFRAME_LINE_NUMBER 1
+#define VENSPEC_IFRAME_LINE_NUMBER 3
static int msm_hdmi_config_avi_infoframe(struct hdmi *hdmi,
const u8 *buffer, size_t len)
@@ -141,6 +143,74 @@ static int msm_hdmi_config_audio_infoframe(struct hdmi
*hdmi,
return 0;
}
+static int msm_hdmi_config_spd_infoframe(struct hdmi *hdmi,
+const u8 *buffer, size_t len)
+{
+ u32 buf[7] = {};
+ u32 val;
+ int i;
+
+ if (len != HDMI_INFOFRAME_SIZE(SPD) || len - 3 > sizeof(buf)) {
+ DRM_DEV_ERROR(&hdmi->pdev->dev,
+ "failed to configure SPD infoframe\n");
+ return -EINVAL;
+ }
+
+ /* checksum gets written together with the body of the frame */
+ hdmi_write(hdmi, REG_HDMI_GENERIC1_HDR,
+ buffer[0] |
+ buffer[1] << 8 |
+ buffer[2] << 16);
+
+ memcpy(buf, &buffer[3], len - 3);
+
+ for (i = 0; i < ARRAY_SIZE(buf); i++)
+ hdmi_write(hdmi, REG_HDMI_GENERIC1(i), buf[i]);
+
+ val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+ val |= HDMI_GEN_PKT_CTRL_GENERIC1_SEND |
+HDMI_GEN_PKT_CTRL_GENERIC1_CONT |
+HDMI_GEN_PKT_CTRL_GENERIC1_LINE(SPD_IFRAME_LINE_NUMBER);
+ hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+ return 0;
+}
+
+static int msm_hdmi_config_hdmi_infoframe(struct hdmi *hdmi,
+ const u8 *buffer, size_t len)
+{
+ u32 buf[7] = {};
+ u32 val;
+ int i;
+
+ if (len < HDMI_INFOFRAME_HEADER_SIZE + HDMI_VENDOR_INFOFRAME_SIZE ||
+ len - 3 > sizeof(buf)) {
+ DRM_DEV_ERROR(&hdmi->pdev->dev,
+ "failed to configure HDMI infoframe\n");
+ return -EINVAL;
+ }
+
+ /* checksum gets written together with the body of the frame */
+ hdmi_write(hdmi, REG_HDMI_GENERIC0_HDR,
+ buffer[0] |
+ buffer[1] << 8 |
+ buffer[2] << 16);
+
+ memcpy(buf, &buffer[3], len - 3);
+
+ for (i = 0; i < ARRAY_SIZE(buf); i++)
+ hdmi_write(hdmi, REG_HDMI_GENERIC0(i), buf[i]);
+
+ val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+ val |= HDMI_GEN_PKT_CTRL_GENERIC0_SEND |
+HDMI_GEN_PKT_CTRL_GENERIC0_CONT |
+HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE |
+HDMI_GEN_PKT_CTRL_GENERIC0_LINE(VENSPEC_IFRAME_LINE_NUMBER);
+ hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+ return 0;
+}
+
static int msm_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
enum hdmi_infoframe_type type)
{
@@ -175,6 +245,25 @@ static int msm_hdmi_bridge_clear_infoframe(struct
drm_bridge *bridge,
break;
+ case HDMI_INFOFRAME_TYPE_SPD:
+ val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+ val &= ~(HDMI_GEN_PKT_CTRL_GENERIC1_SEND |
+HDMI_GEN_PKT_CTRL_GENERIC1_CONT |
+HDMI_GEN_PKT_CTRL_GENERIC1_LINE__MASK);
+ hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+ break;
+
+ case HDMI_INFOFRAME_TYPE_VENDOR:
+ val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+ val &= ~(HDMI_GEN_PKT_CTRL_GENERIC0_SEND |
+HDMI_GEN_PKT_CTRL_GENERIC0_CONT |
+HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE |
+HDMI_GEN_PKT_CTRL_GENERIC0_LINE__MASK);
+ hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+ break;
+
default:
drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe
type %x\n", type);
}
@@ -196,6 +285,10 @@ static int msm_hdmi_bridge_write_infoframe(struct
drm_bridge *bridge,
return msm_hdmi_config_avi_infoframe(hdmi, buffer, len);
case HDMI_INFOFRAME_TYPE_AUDIO:
return msm_hdmi_config_audio_infoframe(hdmi, buffer, len);
+
[PATCH v3 4/7] drm/msm/hdmi: switch to atomic bridge callbacks
Change MSM HDMI bridge to use atomic_* callbacks in preparation to
enablign the HDMI connector support.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 4a5b5112227f..d839c71091dc 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -126,7 +126,8 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
}
-static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
+static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -152,7 +153,8 @@ static void msm_hdmi_bridge_pre_enable(struct drm_bridge
*bridge)
msm_hdmi_hdcp_on(hdmi->hdcp_ctrl);
}
-static void msm_hdmi_bridge_post_disable(struct drm_bridge *bridge)
+static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -299,8 +301,11 @@ static enum drm_mode_status
msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
}
static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
- .pre_enable = msm_hdmi_bridge_pre_enable,
- .post_disable = msm_hdmi_bridge_post_disable,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
+ .atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
.mode_set = msm_hdmi_bridge_mode_set,
.mode_valid = msm_hdmi_bridge_mode_valid,
.edid_read = msm_hdmi_bridge_edid_read,
--
2.39.2
[PATCH v3 6/7] drm/msm/hdmi: update HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE definition
The GENERIC0_UPDATE field is a single bit. Redefine it as boolean to simplify its usage in the driver. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/registers/display/hdmi.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/registers/display/hdmi.xml b/drivers/gpu/drm/msm/registers/display/hdmi.xml index 6c81581016c7..fc711a842363 100644 --- a/drivers/gpu/drm/msm/registers/display/hdmi.xml +++ b/drivers/gpu/drm/msm/registers/display/hdmi.xml @@ -131,7 +131,7 @@ xsi:schemaLocation="https://gitlab.freedesktop.org/freedreno/ rules-fd.xsd"> --> - + -- 2.39.2
[PATCH v3 1/7] drm/connector: hdmi: accept NULL for Audio Infoframe
Allow passing NULL as audio infoframe as a way to disable Audio
Infoframe generation.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/display/drm_hdmi_state_helper.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index ce96837eea65..5356723d21f5 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -681,7 +681,7 @@
EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_update_infoframes);
/**
* drm_atomic_helper_connector_hdmi_update_audio_infoframe - Update the Audio
Infoframe
* @connector: A pointer to the HDMI connector
- * @frame: A pointer to the audio infoframe to write
+ * @frame: A pointer to the audio infoframe to write or NULL to disable
sending the frame
*
* This function is meant for HDMI connector drivers to update their
* audio infoframe. It will typically be used in one of the ALSA hooks
@@ -704,10 +704,16 @@
drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *co
mutex_lock(&connector->hdmi.infoframes.lock);
- memcpy(&infoframe->data, frame, sizeof(infoframe->data));
- infoframe->set = true;
+ if (frame) {
+ memcpy(&infoframe->data, frame, sizeof(infoframe->data));
+ infoframe->set = true;
+
+ ret = write_infoframe(connector, infoframe);
+ } else {
+ infoframe->set = false;
- ret = write_infoframe(connector, infoframe);
+ ret = clear_infoframe(connector, infoframe);
+ }
mutex_unlock(&connector->hdmi.infoframes.lock);
--
2.39.2
[PATCH v3 5/7] drm/msm/hdmi: make use of the drm_connector_hdmi framework
Setup the HDMI connector on the MSM HDMI outputs. Make use of
atomic_check hook and of the provided Infoframe infrastructure.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Kconfig| 2 +
drivers/gpu/drm/msm/hdmi/hdmi.c| 44 ++---
drivers/gpu/drm/msm/hdmi/hdmi.h| 16 +---
drivers/gpu/drm/msm/hdmi/hdmi_audio.c | 74 ---
drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 165 +
5 files changed, 160 insertions(+), 141 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 1931ecf73e32..b5c78c1dd744 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -164,6 +164,8 @@ config DRM_MSM_HDMI
bool "Enable HDMI support in MSM DRM driver"
depends on DRM_MSM
default y
+ select DRM_DISPLAY_HDMI_HELPER
+ select DRM_DISPLAY_HDMI_STATE_HELPER
help
Compile in support for the HDMI output MSM DRM driver. It can
be a primary or a secondary display on device. Note that this is used
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 24abcb7254cc..179da72f8f70 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include "hdmi.h"
@@ -165,8 +166,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
hdmi->dev = dev;
hdmi->encoder = encoder;
- hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
-
ret = msm_hdmi_bridge_init(hdmi);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: %d\n",
ret);
@@ -254,40 +253,12 @@ static int msm_hdmi_audio_hw_params(struct device *dev,
void *data,
struct hdmi_codec_params *params)
{
struct hdmi *hdmi = dev_get_drvdata(dev);
- unsigned int chan;
- unsigned int channel_allocation = 0;
unsigned int rate;
- unsigned int level_shift = 0; /* 0dB */
- bool down_mix = false;
+ int ret;
DRM_DEV_DEBUG(dev, "%u Hz, %d bit, %d channels\n", params->sample_rate,
params->sample_width, params->cea.channels);
- switch (params->cea.channels) {
- case 2:
- /* FR and FL speakers */
- channel_allocation = 0;
- chan = MSM_HDMI_AUDIO_CHANNEL_2;
- break;
- case 4:
- /* FC, LFE, FR and FL speakers */
- channel_allocation = 0x3;
- chan = MSM_HDMI_AUDIO_CHANNEL_4;
- break;
- case 6:
- /* RR, RL, FC, LFE, FR and FL speakers */
- channel_allocation = 0x0B;
- chan = MSM_HDMI_AUDIO_CHANNEL_6;
- break;
- case 8:
- /* FRC, FLC, RR, RL, FC, LFE, FR and FL speakers */
- channel_allocation = 0x1F;
- chan = MSM_HDMI_AUDIO_CHANNEL_8;
- break;
- default:
- return -EINVAL;
- }
-
switch (params->sample_rate) {
case 32000:
rate = HDMI_SAMPLE_RATE_32KHZ;
@@ -316,9 +287,12 @@ static int msm_hdmi_audio_hw_params(struct device *dev,
void *data,
return -EINVAL;
}
- msm_hdmi_audio_set_sample_rate(hdmi, rate);
- msm_hdmi_audio_info_setup(hdmi, 1, chan, channel_allocation,
- level_shift, down_mix);
+ ret =
drm_atomic_helper_connector_hdmi_update_audio_infoframe(hdmi->connector,
+
¶ms->cea);
+ if (ret)
+ return ret;
+
+ msm_hdmi_audio_info_setup(hdmi, rate, params->cea.channels);
return 0;
}
@@ -327,7 +301,7 @@ static void msm_hdmi_audio_shutdown(struct device *dev,
void *data)
{
struct hdmi *hdmi = dev_get_drvdata(dev);
- msm_hdmi_audio_info_setup(hdmi, 0, 0, 0, 0, 0);
+ msm_hdmi_audio_disable(hdmi);
}
static const struct hdmi_codec_ops msm_hdmi_audio_codec_ops = {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 4586baf36415..0ac034eaaf0f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -24,8 +24,8 @@ struct hdmi_platform_config;
struct hdmi_audio {
bool enabled;
- struct hdmi_audio_infoframe infoframe;
int rate;
+ int channels;
};
struct hdmi_hdcp_ctrl;
@@ -199,12 +199,6 @@ static inline int msm_hdmi_pll_8996_init(struct
platform_device *pdev)
/*
* audio:
*/
-/* Supported HDMI Audio channels and rates */
-#defineMSM_HDMI_AUDIO_CHANNEL_20
-#defineMSM_HDMI_AUDIO_CHANNEL_41
-#defineMSM_HDMI_AUDIO_CHANNEL_62
-#defineMSM_HDMI_AUDIO_CHANNEL_83
-
#defineHDMI_SAMPLE_RATE_32KHZ 0
#defineHDMI_SAMPLE_RATE_44_1KHZ1
#define
[PATCH v3 0/7] drm/msm: make use of the HDMI connector infrastructure
This patchset sits on top Maxime's HDMI connector patchset ([1]). Currently this is an RFC exploring the interface between HDMI bridges and HDMI connector code. This has been lightly verified on the Qualcomm DB820c, which has native HDMI output. If this approach is considered to be acceptable, I'll finish MSM HDMI bridge conversion (reworking the Audio Infoframe code). Other bridges can follow the same approach (we have lt9611 / lt9611uxc / adv7511 on Qualcomm hardware). [1] https://patchwork.freedesktop.org/series/122421/ To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Laurent Pinchart To: Jonas Karlman To: Jernej Skrabec To: Maarten Lankhorst To: Maxime Ripard To: Thomas Zimmermann To: David Airlie To: Daniel Vetter To: Rob Clark To: Abhinav Kumar To: Sean Paul To: Marijn Suijten Cc: dri-de...@lists.freedesktop.org Cc: linux-arm-...@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Dmitry Baryshkov Changes in v3: - Rebased on top of the merged HDMI connector patchset. - Changed drm_bridge_connector to use drmm_connector_init() (Maxime) - Added a check that write_infoframe callback is present if BRIDGE_OP_HDMI is set. - Moved drm_atomic_helper_connector_hdmi_check() call from drm_bridge_connector to the HDMI bridge driver to remove dependency from drm_kms_helpers on the optional HDMI state helpers. - Converted Audio InfoFrame handling code. - Added support for HDMI Vendor Specific and SPD InfoFrames. - Link to v2: https://lore.kernel.org/r/20240309-bridge-hdmi-connector-v2-0-1380bea3e...@linaro.org Changes in v2: - Dropped drm_connector_hdmi_setup(). Instead added drm_connector_hdmi_init() to be used by drm_bridge_connector. - Changed the drm_bridge_connector to act as a proxy for the HDMI connector infrastructure. This removes most of the logic from the bridge drivers. - Link to v1: https://lore.kernel.org/r/20240308-bridge-hdmi-connector-v1-0-90b693550...@linaro.org --- Dmitry Baryshkov (7): drm/connector: hdmi: accept NULL for Audio Infoframe drm/bridge-connector: switch to using drmm allocations drm/bridge-connector: implement glue code for HDMI connector drm/msm/hdmi: switch to atomic bridge callbacks drm/msm/hdmi: make use of the drm_connector_hdmi framework drm/msm/hdmi: update HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE definition drm/msm/hdmi: also send the SPD and HDMI Vendor Specific InfoFrames drivers/gpu/drm/display/drm_hdmi_state_helper.c | 14 +- drivers/gpu/drm/drm_bridge_connector.c | 114 -- drivers/gpu/drm/drm_debugfs.c | 2 + drivers/gpu/drm/msm/Kconfig | 2 + drivers/gpu/drm/msm/hdmi/hdmi.c | 44 +--- drivers/gpu/drm/msm/hdmi/hdmi.h | 16 +- drivers/gpu/drm/msm/hdmi/hdmi_audio.c | 74 ++- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 271 drivers/gpu/drm/msm/registers/display/hdmi.xml | 2 +- include/drm/drm_bridge.h| 82 +++ 10 files changed, 455 insertions(+), 166 deletions(-) --- base-commit: 03e98b48e2125d0cc99eeaace0f06290e20a1c55 change-id: 20240307-bridge-hdmi-connector-7e3754e661d0 Best regards, -- Dmitry Baryshkov
[PATCH v3 3/7] drm/bridge-connector: implement glue code for HDMI connector
In order to let bridge chains implement HDMI connector infrastructure,
add necessary glue code to the drm_bridge_connector. In case there is a
bridge that sets DRM_BRIDGE_OP_HDMI, drm_bridge_connector will register
itself as a HDMI connector and provide proxy drm_connector_hdmi_funcs
implementation.
Note, to simplify implementation, there can be only one bridge in a
chain that sets DRM_BRIDGE_OP_HDMI. Setting more than one is considered
an error. This limitation can be lifted later, if the need arises.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/drm_bridge_connector.c | 101 -
drivers/gpu/drm/drm_debugfs.c | 2 +
include/drm/drm_bridge.h | 82 ++
3 files changed, 182 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge_connector.c
b/drivers/gpu/drm/drm_bridge_connector.c
index e093fc8928dc..8dca3eda5381 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
/**
* DOC: overview
@@ -87,6 +88,13 @@ struct drm_bridge_connector {
* connector modes detection, if any (see &DRM_BRIDGE_OP_MODES).
*/
struct drm_bridge *bridge_modes;
+ /**
+* @bridge_hdmi:
+*
+* The bridge in the chain that implements necessary support for the
+* HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
+*/
+ struct drm_bridge *bridge_hdmi;
};
#define to_drm_bridge_connector(x) \
@@ -287,6 +295,61 @@ static const struct drm_connector_helper_funcs
drm_bridge_connector_helper_funcs
.disable_hpd = drm_bridge_connector_disable_hpd,
};
+static enum drm_mode_status
+drm_bridge_connector_tmds_char_rate_valid(const struct drm_connector
*connector,
+ const struct drm_display_mode *mode,
+ unsigned long long tmds_rate)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(connector);
+ struct drm_bridge *bridge;
+
+ bridge = bridge_connector->bridge_hdmi;
+ if (bridge)
+ return bridge->funcs->tmds_char_rate_valid ?
+ bridge->funcs->tmds_char_rate_valid(bridge, mode,
tmds_rate) :
+ MODE_OK;
+
+ return MODE_ERROR;
+}
+
+static int drm_bridge_connector_clear_infoframe(struct drm_connector
*connector,
+ enum hdmi_infoframe_type type)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(connector);
+ struct drm_bridge *bridge;
+
+ bridge = bridge_connector->bridge_hdmi;
+ if (bridge)
+ return bridge->funcs->clear_infoframe ?
+ bridge->funcs->clear_infoframe(bridge, type) :
+ 0;
+
+ return -EINVAL;
+}
+
+static int drm_bridge_connector_write_infoframe(struct drm_connector
*connector,
+ enum hdmi_infoframe_type type,
+ const u8 *buffer, size_t len)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(connector);
+ struct drm_bridge *bridge;
+
+ bridge = bridge_connector->bridge_hdmi;
+ if (bridge)
+ return bridge->funcs->write_infoframe(bridge, type, buffer,
len);
+
+ return -EINVAL;
+}
+
+static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs =
{
+ .tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
+ .clear_infoframe = drm_bridge_connector_clear_infoframe,
+ .write_infoframe = drm_bridge_connector_write_infoframe,
+};
+
/*
-
* Bridge Connector Initialisation
*/
@@ -312,6 +375,10 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
struct drm_connector *connector;
struct i2c_adapter *ddc = NULL;
struct drm_bridge *bridge, *panel_bridge = NULL;
+ const char *vendor = "Unknown";
+ const char *product = "Unknown";
+ unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
+ unsigned int max_bpc = 8;
int connector_type;
int ret;
@@ -348,6 +415,25 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
bridge_connector->bridge_detect = bridge;
if (bridge->ops & DRM_BRIDGE_OP_MODES)
bridge_connector->bridge_modes = bridge;
+ if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
+ if (bridge_connector->bridge_hdmi)
+ return ERR_PTR(-EBUSY);
+ if (!bridge->funcs->write_infoframe)
+
[PATCH v3 2/7] drm/bridge-connector: switch to using drmm allocations
Turn drm_bridge_connector to using drmm_kzalloc() and
drmm_connector_init() and drop the custom destroy function. The
drm_connector_unregister() and fwnode_handle_put() are already handled
by the drm_connector_cleanup() and so are safe to be dropped.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/drm_bridge_connector.c | 23 +--
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge_connector.c
b/drivers/gpu/drm/drm_bridge_connector.c
index 982552c9f92c..e093fc8928dc 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector
*connector, bool force)
return status;
}
-static void drm_bridge_connector_destroy(struct drm_connector *connector)
-{
- struct drm_bridge_connector *bridge_connector =
- to_drm_bridge_connector(connector);
-
- drm_connector_unregister(connector);
- drm_connector_cleanup(connector);
-
- fwnode_handle_put(connector->fwnode);
-
- kfree(bridge_connector);
-}
-
static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
struct dentry *root)
{
@@ -224,7 +212,6 @@ static const struct drm_connector_funcs
drm_bridge_connector_funcs = {
.reset = drm_atomic_helper_connector_reset,
.detect = drm_bridge_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = drm_bridge_connector_destroy,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
.debugfs_init = drm_bridge_connector_debugfs_init,
@@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
int connector_type;
int ret;
- bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
+ bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector),
GFP_KERNEL);
if (!bridge_connector)
return ERR_PTR(-ENOMEM);
@@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct
drm_device *drm,
return ERR_PTR(-EINVAL);
}
- ret = drm_connector_init_with_ddc(drm, connector,
- &drm_bridge_connector_funcs,
- connector_type, ddc);
+ ret = drmm_connector_init(drm, connector,
+ &drm_bridge_connector_funcs,
+ connector_type, ddc);
if (ret) {
kfree(bridge_connector);
return ERR_PTR(ret);
--
2.39.2
Re: [PATCH 0/7] drm/msm/dpu: handle non-default TE source pins
On 5/22/2024 12:59 PM, Dmitry Baryshkov wrote: On Wed, 22 May 2024 at 21:39, Abhinav Kumar wrote: On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: Command-mode DSI panels need to signal the display controlller when vsync happens, so that the device can start sending the next frame. Some devices (Google Pixel 3) use a non-default pin, so additional configuration is required. Add a way to specify this information in DT and handle it in the DSI and DPU drivers. Which pin is the pixel 3 using? Just wanted to know .. is it gpio0 or gpio1? gpio2. If it was gpio0 then there were no issues at all. Got it. Instead of asking gpio1 or gpio2, I mistyped and asked gpio0 or gpio1. While reviewing the code , I think the function dpu_hw_setup_vsync_source is poorly named . It really doesnt configured vsync_source. It actually configured watchdog timer. Can you pls include one more patch in this series to rename dpu_hw_setup_vsync_source ---> dpu_hw_setup_wd_timer() Signed-off-by: Dmitry Baryshkov --- Dmitry Baryshkov (7): dt-bindings: display/msm/dsi: allow specifying TE source drm/msm/dpu: convert vsync source defines to the enum drm/msm/dsi: drop unused GPIOs handling drm/msm/dpu: pull the is_cmd_mode out of _dpu_encoder_update_vsync_source() drm/msm/dpu: rework vsync_source handling drm/msm/dsi: parse vsync source from device tree drm/msm/dpu: support setting the TE source .../bindings/display/msm/dsi-controller-main.yaml | 16 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 11 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 5 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h| 26 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 44 drivers/gpu/drm/msm/dsi/dsi.h | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c | 48 +- drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 +++ drivers/gpu/drm/msm/msm_drv.h | 6 +++ 12 files changed, 106 insertions(+), 62 deletions(-) --- base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd change-id: 20240514-dpu-handle-te-signal-82663c0211bd Best regards,
Re: [PATCH 6/7] drm/msm/dsi: parse vsync source from device tree
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote: Allow board's device tree to specify the vsync source (aka TE source). If the property is omitted, the display controller driver will use the default setting. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi.h | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c| 11 +++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 5 + drivers/gpu/drm/msm/msm_drv.h | 6 ++ 4 files changed, 23 insertions(+) Reviewed-by: Abhinav Kumar
Re: [PATCH 1/7] dt-bindings: display/msm/dsi: allow specifying TE source
On 5/23/2024 2:58 AM, Dmitry Baryshkov wrote:
On Thu, 23 May 2024 at 02:57, Abhinav Kumar wrote:
On 5/22/2024 1:05 PM, Dmitry Baryshkov wrote:
On Wed, 22 May 2024 at 21:38, Abhinav Kumar wrote:
On 5/20/2024 5:12 AM, Dmitry Baryshkov wrote:
Command mode panels provide TE signal back to the DSI host to signal
that the frame display has completed and update of the image will not
cause tearing. Usually it is connected to the first GPIO with the
mdp_vsync function, which is the default. In such case the property can
be skipped.
This is a good addition overall. Some comments below.
Signed-off-by: Dmitry Baryshkov
---
.../bindings/display/msm/dsi-controller-main.yaml| 16
1 file changed, 16 insertions(+)
diff --git
a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
index 1fa28e976559..c1771c69b247 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml
@@ -162,6 +162,21 @@ properties:
items:
enum: [ 0, 1, 2, 3 ]
+ qcom,te-source:
+$ref: /schemas/types.yaml#/definitions/string
+description:
+ Specifies the source of vsync signal from the panel used for
+ tearing elimination. The default is mdp_gpio0.
panel --> command mode panel?
+enum:
+ - mdp_gpio0
+ - mdp_gpio1
+ - mdp_gpio2
are gpio0, gpio1 and gpio2 referring to the vsync_p, vsync_s and vsync_e
sources?
No idea, unfortunately. They are gpioN or just mdp_vsync all over the
place. For the reference, in case of the SDM845 and Pixel3 the signal
is routed through SoC GPIO12.
GPIO12 on sdm845 is mdp_vsync_e.
Thats why I think its better we use mdp_vsync_p/s/e instead of mdp_gpio0/1/2
Sure. This matches pins description. Are you fine with changing
defines in DPU driver to VSYNC_P / _S / _E too ?
Sorry for the delay in responding.
As per the software docs, the registers still use GPIO0/1/2.
Only the pin descriptions use vsync_p/s/e.
Hence I think we can make DPU driver to use 0/1/2.
In that case wouldnt it be better to name it like that?
+ - timer0
+ - timer1
+ - timer2
+ - timer3
+ - timer4
+
These are indicating watchdog timer sources right?
Yes.
ack.
required:
- port@0
- port@1
@@ -452,6 +467,7 @@ examples:
dsi0_out: endpoint {
remote-endpoint = <&sn65dsi86_in>;
data-lanes = <0 1 2 3>;
+ qcom,te-source = "mdp_gpio2";
I have a basic doubt on this. Should te-source should be in the input
port or the output one for the controller? Because TE is an input to the
DSI. And if the source is watchdog timer then it aligns even more as a
property of the input endpoint.
I don't really want to split this. Both data-lanes and te-source are
properties of the link between the DSI and panel. You can not really
say which side has which property.
TE is an input to the DSI from the panel. Between input and output port,
I think it belongs more to the input port.
Technically we don't have in/out ports. There are two ports which
define a link between two instances. For example, if the panel
supports getting information through DCS commands, then "panel input"
also becomes "panel output".
The ports are labeled dsi0_in and dsi0_out. Putting te source in
dsi0_out really looks very confusing to me.
I didnt follow why this is a link property. Sorry , I didnt follow the
split part.
There is a link between the DSI host and the panel. I don't want to
end up in a situation when the properties of the link are split
between two different nodes.
It really depends on what the property denotes. I do not think this
should be the reason to do it this way.
If we are unsure about input vs output port, do you think its better we
make it a property of the main dsi node instead?
No, it's not a property of the DSI node at all. If the vendor rewires
the panel GPIOs or (just for example regulators), it has nothing to do
with the DSI host.
Ack to this.
--
With best wishes
Dmitry
Re: [PATCH] drm/msm/adreno: Add A306A support
On 28.05.2024 9:43 PM, Barnabás Czémán wrote:
> From: Otto Pflüger
>
> Add support for Adreno 306A GPU what is found in MSM8917 SoC.
> This GPU marketing name is Adreno 308.
>
> Signed-off-by: Otto Pflüger
> [use internal name of the GPU, reword the commit message]
> Signed-off-by: Barnabás Czémán
> ---
[...]
>
> +static inline bool adreno_is_a306a(const struct adreno_gpu *gpu)
> +{
> + /* a306a marketing name is a308 */
> + return adreno_is_revn(gpu, 308);
> +}
The .c changes look good. Rob, do we still want .rev nowadays?
Re: (subset) [PATCH v4 0/3] drm/panel: two fixes for lg-sw43408
On Tue, 28 May 2024 22:39:17 +0300, Dmitry Baryshkov wrote: > Fix two issues with the panel-lg-sw43408 driver reported by the kernel > test robot. > > Applied to drm-misc-fixes, thanks! [1/3] drm/panel/lg-sw43408: select CONFIG_DRM_DISPLAY_DP_HELPER commit: 33defcacd207196a6b35857087e6335590adad62 [2/3] drm/panel/lg-sw43408: mark sw43408_backlight_ops as static commit: 8c318cb70c88aa02068db7518e852b909c9b400f Best regards, -- With best wishes Dmitry
Re: [PATCH v4 3/3] drm/display: split DSC helpers from DP helpers
On 2024-05-28 22:39:20, Dmitry Baryshkov wrote: > Currently the DRM DSC functions are selected by the > DRM_DISPLAY_DP_HELPER Kconfig symbol. This is not optimal, since the DSI > code (both panel and host drivers) end up selecting the seemingly > irrelevant DP helpers. Split the DSC code to be guarded by the separate > DRM_DISPLAY_DSC_HELPER Kconfig symbol. > > Reviewed-by: Jessica Zhang > Signed-off-by: Dmitry Baryshkov Reviewed-by: Marijn Suijten > --- > drivers/gpu/drm/amd/amdgpu/Kconfig | 1 + > drivers/gpu/drm/display/Kconfig| 6 ++ > drivers/gpu/drm/display/Makefile | 3 ++- > drivers/gpu/drm/i915/Kconfig | 1 + > drivers/gpu/drm/msm/Kconfig| 1 + > drivers/gpu/drm/panel/Kconfig | 6 +++--- > 6 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig > b/drivers/gpu/drm/amd/amdgpu/Kconfig > index 4232ab27f990..5933ca8c6b96 100644 > --- a/drivers/gpu/drm/amd/amdgpu/Kconfig > +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig > @@ -6,6 +6,7 @@ config DRM_AMDGPU > depends on !UML > select FW_LOADER > select DRM_DISPLAY_DP_HELPER > + select DRM_DISPLAY_DSC_HELPER > select DRM_DISPLAY_HDMI_HELPER > select DRM_DISPLAY_HDCP_HELPER > select DRM_DISPLAY_HELPER > diff --git a/drivers/gpu/drm/display/Kconfig b/drivers/gpu/drm/display/Kconfig > index 864a6488bfdf..f524cf95dec3 100644 > --- a/drivers/gpu/drm/display/Kconfig > +++ b/drivers/gpu/drm/display/Kconfig > @@ -59,6 +59,12 @@ config DRM_DISPLAY_DP_TUNNEL_STATE_DEBUG > > If in doubt, say "N". > > +config DRM_DISPLAY_DSC_HELPER > + bool > + depends on DRM_DISPLAY_HELPER > + help > + DRM display helpers for VESA DSC (used by DSI and DisplayPort). > + > config DRM_DISPLAY_HDCP_HELPER > bool > depends on DRM_DISPLAY_HELPER > diff --git a/drivers/gpu/drm/display/Makefile > b/drivers/gpu/drm/display/Makefile > index 17d2cc73ff56..2ec71e15c3cb 100644 > --- a/drivers/gpu/drm/display/Makefile > +++ b/drivers/gpu/drm/display/Makefile > @@ -6,7 +6,8 @@ drm_display_helper-y := drm_display_helper_mod.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_HELPER) += \ > drm_dp_dual_mode_helper.o \ > drm_dp_helper.o \ > - drm_dp_mst_topology.o \ > + drm_dp_mst_topology.o > +drm_display_helper-$(CONFIG_DRM_DISPLAY_DSC_HELPER) += \ > drm_dsc_helper.o > drm_display_helper-$(CONFIG_DRM_DISPLAY_DP_TUNNEL) += \ > drm_dp_tunnel.o > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig > index 5932024f8f95..117b84260b1c 100644 > --- a/drivers/gpu/drm/i915/Kconfig > +++ b/drivers/gpu/drm/i915/Kconfig > @@ -11,6 +11,7 @@ config DRM_I915 > select SHMEM > select TMPFS > select DRM_DISPLAY_DP_HELPER > + select DRM_DISPLAY_DSC_HELPER > select DRM_DISPLAY_HDCP_HELPER > select DRM_DISPLAY_HDMI_HELPER > select DRM_DISPLAY_HELPER > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig > index 1931ecf73e32..6dcd26180611 100644 > --- a/drivers/gpu/drm/msm/Kconfig > +++ b/drivers/gpu/drm/msm/Kconfig > @@ -111,6 +111,7 @@ config DRM_MSM_DSI > depends on DRM_MSM > select DRM_PANEL > select DRM_MIPI_DSI > + select DRM_DISPLAY_DSC_HELPER > default y > help > Choose this option if you have a need for MIPI DSI connector > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index 2ae0eb0638f3..3e3f63479544 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -340,7 +340,7 @@ config DRM_PANEL_LG_SW43408 > depends on OF > depends on DRM_MIPI_DSI > depends on BACKLIGHT_CLASS_DEVICE > - select DRM_DISPLAY_DP_HELPER > + select DRM_DISPLAY_DSC_HELPER > select DRM_DISPLAY_HELPER > help > Say Y here if you want to enable support for LG sw43408 panel. > @@ -549,7 +549,7 @@ config DRM_PANEL_RAYDIUM_RM692E5 > depends on OF > depends on DRM_MIPI_DSI > depends on BACKLIGHT_CLASS_DEVICE > - select DRM_DISPLAY_DP_HELPER > + select DRM_DISPLAY_DSC_HELPER > select DRM_DISPLAY_HELPER > help > Say Y here if you want to enable support for Raydium RM692E5-based > @@ -907,7 +907,7 @@ config DRM_PANEL_VISIONOX_R66451 > depends on OF > depends on DRM_MIPI_DSI > depends on BACKLIGHT_CLASS_DEVICE > - select DRM_DISPLAY_DP_HELPER > + select DRM_DISPLAY_DSC_HELPER > select DRM_DISPLAY_HELPER > help > Say Y here if you want to enable support for Visionox > > -- > 2.39.2 >
Re: [PATCH v4 2/3] drm/panel/lg-sw43408: mark sw43408_backlight_ops as static
On 2024-05-28 22:39:19, Dmitry Baryshkov wrote:
> Fix sparse warning regarding symbol 'sw43408_backlight_ops' not being
> declared.
>
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202404200739.hbwzvohr-...@intel.com/
> Reviewed-by: Neil Armstrong
> Fixes: 069a6c0e94f9 ("drm: panel: Add LG sw43408 panel driver")
> Signed-off-by: Dmitry Baryshkov
Reviewed-by: Marijn Suijten
> ---
> drivers/gpu/drm/panel/panel-lg-sw43408.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-lg-sw43408.c
> b/drivers/gpu/drm/panel/panel-lg-sw43408.c
> index 115f4702d59f..2b3a73696dce 100644
> --- a/drivers/gpu/drm/panel/panel-lg-sw43408.c
> +++ b/drivers/gpu/drm/panel/panel-lg-sw43408.c
> @@ -182,7 +182,7 @@ static int sw43408_backlight_update_status(struct
> backlight_device *bl)
> return mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
> }
>
> -const struct backlight_ops sw43408_backlight_ops = {
> +static const struct backlight_ops sw43408_backlight_ops = {
> .update_status = sw43408_backlight_update_status,
> };
>
>
> --
> 2.39.2
>
Re: [PATCH v4 1/3] drm/panel/lg-sw43408: select CONFIG_DRM_DISPLAY_DP_HELPER
On 2024-05-28 22:39:18, Dmitry Baryshkov wrote:
> This panel driver uses DSC PPS functions and as such depends on the
> DRM_DISPLAY_DP_HELPER. Select this symbol to make required functions
> available to the driver.
>
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202404200800.kysryyli-...@intel.com/
> Fixes: 069a6c0e94f9 ("drm: panel: Add LG sw43408 panel driver")
> Reviewed-by: Neil Armstrong
> Signed-off-by: Dmitry Baryshkov
Maybe good context to mention that the DSC<->DP discrepancy will be resolved in
the future, otherwise this patch is slightly unclear for anyone who isn't aware
of the current patch series and its context. Other than that, for the change
itself:
Reviewed-by: Marijn Suijten
> ---
> drivers/gpu/drm/panel/Kconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 982324ef5a41..2ae0eb0638f3 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -340,6 +340,8 @@ config DRM_PANEL_LG_SW43408
> depends on OF
> depends on DRM_MIPI_DSI
> depends on BACKLIGHT_CLASS_DEVICE
> + select DRM_DISPLAY_DP_HELPER
> + select DRM_DISPLAY_HELPER
> help
> Say Y here if you want to enable support for LG sw43408 panel.
> The panel has a 1080x2160@60Hz resolution and uses 24 bit RGB per
>
> --
> 2.39.2
>
