Re: [Freedreno] [PATCH v2 06/11] drm/msm/disp/dpu1: Don't use DSC with mode_3d
On 14-10-21, 16:50, Dmitry Baryshkov wrote:
> On 14/10/2021 16:41, Dmitry Baryshkov wrote:
> > On 07/10/2021 10:08, Vinod Koul wrote:
> > > We cannot enable mode_3d when we are using the DSC. So pass
> > > configuration to detect DSC is enabled and not enable mode_3d
> > > when we are using DSC
> > >
> > > We add a helper dpu_encoder_helper_get_dsc_mode() to detect dsc
> > > enabled and pass this to .setup_intf_cfg()
> > >
> > > Signed-off-by: Vinod Koul
> > > ---
> > > Changes since
> > > v1:
> > > - Move this patch from 7 to 6
> > > - Update the changelog
> > > - Make dsc as int and store the DSC indices
> > >
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 +++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 ++
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 5 +++--
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++
> > > 4 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > 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 e7270eb6b84b..fca07ed03317 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > @@ -332,6 +332,17 @@ static inline enum dpu_3d_blend_mode
> > > dpu_encoder_helper_get_3d_blend_mode(
> > > return BLEND_3D_NONE;
> > > }
> > > +static inline bool dpu_encoder_helper_get_dsc_mode(struct
> > > dpu_encoder_phys *phys_enc)
> > > +{
> > > + struct drm_encoder *drm_enc = phys_enc->parent;
> > > + struct msm_drm_private *priv = drm_enc->dev->dev_private;
> > > +
> > > + if (priv->dsc)
> > > + return BIT(0) | BIT(1); /* Hardcoding for 2 DSC topology */
> >
> > Please use defined values here rater than just BIT().
>
> Ah, it's a list of DSC blocks used. So the function name is misleading (as
> it's not a mode). I think we'd better pass DSC_n names here. What about
> using an array for cfg->dsc?
Yeah I can do better names.
>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /**
> > > * dpu_encoder_helper_split_config - split display configuration
> > > helper function
> > > * This helper function may be used by physical encoders to
> > > configure
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > > index aa01698d6b25..8e5c0911734c 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > > @@ -70,6 +70,8 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> > > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
> > > intf_cfg.stream_sel = cmd_enc->stream_sel;
> > > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> > > + intf_cfg.dsc = dpu_encoder_helper_get_dsc_mode(phys_enc);
> > > +
> > > ctl->ops.setup_intf_cfg(ctl, &intf_cfg);
> > > }
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > index 64740ddb983e..3c79bd9c2fe5 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > > @@ -118,7 +118,7 @@ static u32 dpu_hw_ctl_get_pending_flush(struct
> > > dpu_hw_ctl *ctx)
> > > return ctx->pending_flush_mask;
> > > }
> > > -static inline void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
> > > +static void dpu_hw_ctl_trigger_flush_v1(struct dpu_hw_ctl *ctx)
> > > {
> > > if (ctx->pending_flush_mask & BIT(MERGE_3D_IDX))
> > > @@ -519,7 +519,8 @@ static void dpu_hw_ctl_intf_cfg(struct
> > > dpu_hw_ctl *ctx,
> > > intf_cfg |= (cfg->intf & 0xF) << 4;
> > > - if (cfg->mode_3d) {
> > > + /* In DSC we can't set merge, so check for dsc too */
> > > + if (cfg->mode_3d && !cfg->dsc) {
> >
> > The more I think about this hunk, the more I'm unsure about it.
> > Downstream has the following topoligies defined:
> > * @SDE_RM_TOPOLOGY_DUALPIPE_3DMERGE_DSC: 2 LM, 2 PP, 3DMux, 1 DSC, 1
> > INTF/WB
> > * @SDE_RM_TOPOLOGY_QUADPIPE_3DMERGE_DSC 4 LM, 4 PP, 3DMux, 3 DSC, 2 INTF
> >
> > While the latter is not supported on sdm845, the former one should be
> > (by the hardware). So in the driver I think we should make sure that
> > mode_3d does not get set rather than disallowing it here.
> >
> > > intf_cfg |= BIT(19);
> > > intf_cfg |= (cfg->mode_3d - 0x1) << 20;
> > > }
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > index 806c171e5df2..5dfac5994bd4 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > > @@ -39,6 +39,7 @@ struct dpu_hw_stage_cfg {
> > > * @mode_3d: 3d mux configuration
> > > * @merge_3d: 3d merge block used
> > > * @intf_mo
Re: [Freedreno] [PATCH v2] drm/msm: use compatible lists to find mdp node
Quoting Krishna Manikandan (2021-10-18 22:30:28)
> In the current implementation, substring comparison
> using device node name is used to find mdp node
> during driver probe. Use compatible string list instead
> of node name to get mdp node from the parent mdss node.
>
> Signed-off-by: Krishna Manikandan
>
> Changes in v2:
>- Use compatible lists instead of duplicate string
> check (Stephen Boyd)
> ---
> drivers/gpu/drm/msm/msm_drv.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 2e6fc18..451d667 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1241,9 +1241,13 @@ static int add_components_mdp(struct device *mdp_dev,
> return 0;
> }
>
> -static int compare_name_mdp(struct device *dev, void *data)
> +static int find_mdp_node(struct device *dev, void *data)
> {
> - return (strstr(dev_name(dev), "mdp") != NULL);
> + if (!dev->driver)
I don't think we want to wait for the device to have an attached driver.
That could be far later than when we're trying to add all the
components here. Can you reference the match tables directly?
> + return 0;
> +
> + return (of_match_node(dev->driver->of_match_table,
> + dev->of_node) != NULL);
Drop useless parenthesis.
> }
>
> static int add_display_components(struct platform_device *pdev,
> @@ -1268,7 +1272,7 @@ static int add_display_components(struct
> platform_device *pdev,
> return ret;
> }
>
> - mdp_dev = device_find_child(dev, NULL, compare_name_mdp);
> + mdp_dev = device_find_child(dev, NULL, find_mdp_node);
> if (!mdp_dev) {
> DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n");
> of_platform_depopulate(dev);
Re: [Freedreno] [PATCH 2/2] drm/msm/dsi: stop setting clock parents manually
On 2021-10-06 13:48, Dmitry Baryshkov wrote:
There is no reason to set clock parents manually, use device tree to
assign DSI/display clock parents to DSI PHY clocks. Dropping this
manual
setup allows us to drop repeating code and to move registration of hw
clock providers to generic place.
Signed-off-by: Dmitry Baryshkov
I believe this was reviewed previously on
https://patchwork.freedesktop.org/patch/443470/
Hence,
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dsi/dsi.h | 2 -
drivers/gpu/drm/msm/dsi/dsi_host.c| 53 ---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 11 +-
drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 11 --
4 files changed, 2 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h
b/drivers/gpu/drm/msm/dsi/dsi.h
index 7dfb6d198ca9..c03a8d09c764 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -173,8 +173,6 @@ int msm_dsi_phy_enable(struct msm_dsi_phy *phy,
void msm_dsi_phy_disable(struct msm_dsi_phy *phy);
void msm_dsi_phy_set_usecase(struct msm_dsi_phy *phy,
enum msm_dsi_phy_usecase uc);
-int msm_dsi_phy_get_clk_provider(struct msm_dsi_phy *phy,
- struct clk **byte_clk_provider, struct clk **pixel_clk_provider);
void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy);
int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy);
void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct
msm_dsi_phy *phy);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1ffcd0577e99..9600b4fa27eb 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2232,59 +2232,6 @@ void msm_dsi_host_set_phy_mode(struct
mipi_dsi_host *host,
msm_host->cphy_mode = src_phy->cphy_mode;
}
-int msm_dsi_host_set_src_pll(struct mipi_dsi_host *host,
- struct msm_dsi_phy *src_phy)
-{
- struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
- struct clk *byte_clk_provider, *pixel_clk_provider;
- int ret;
-
- msm_host->cphy_mode = src_phy->cphy_mode;
-
- ret = msm_dsi_phy_get_clk_provider(src_phy,
- &byte_clk_provider, &pixel_clk_provider);
- if (ret) {
- pr_info("%s: can't get provider from pll, don't set parent\n",
- __func__);
- return 0;
- }
-
- ret = clk_set_parent(msm_host->byte_clk_src, byte_clk_provider);
- if (ret) {
- pr_err("%s: can't set parent to byte_clk_src. ret=%d\n",
- __func__, ret);
- goto exit;
- }
-
- ret = clk_set_parent(msm_host->pixel_clk_src, pixel_clk_provider);
- if (ret) {
- pr_err("%s: can't set parent to pixel_clk_src. ret=%d\n",
- __func__, ret);
- goto exit;
- }
-
- if (msm_host->dsi_clk_src) {
- ret = clk_set_parent(msm_host->dsi_clk_src, pixel_clk_provider);
- if (ret) {
- pr_err("%s: can't set parent to dsi_clk_src. ret=%d\n",
- __func__, ret);
- goto exit;
- }
- }
-
- if (msm_host->esc_clk_src) {
- ret = clk_set_parent(msm_host->esc_clk_src, byte_clk_provider);
- if (ret) {
- pr_err("%s: can't set parent to esc_clk_src. ret=%d\n",
- __func__, ret);
- goto exit;
- }
- }
-
-exit:
- return ret;
-}
-
void msm_dsi_host_reset_phy(struct mipi_dsi_host *host)
{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 49a0a0841487..9342a822ad20 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -78,10 +78,7 @@ static int dsi_mgr_setup_components(int id)
msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
- ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
- } else if (!other_dsi) {
- ret = 0;
- } else {
+ } else if (other_dsi) {
struct msm_dsi *master_link_dsi = IS_MASTER_DSI_LINK(id) ?
msm_dsi : other_dsi;
struct msm_dsi *slave_link_dsi = IS_MASTER_DSI_LINK(id) ?
@@ -107,13 +104,9 @@ static int dsi_mgr_setup_components(int id)
MSM_DSI_PHY_SLAVE);
msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
- ret = msm_dsi_host_set_src_pll(msm_dsi->host,
clk_master_dsi->phy);
- if (ret)
-
Re: [Freedreno] [PATCH 1/2] drm/msm/dsi: untangle cphy setting from the src pll setting
On 2021-10-06 13:48, Dmitry Baryshkov wrote:
Move DPHY/CPHY setting from msm_dsi_host_set_src_pll() to new function
msm_dsi_host_set_phy_mode().
Signed-off-by: Dmitry Baryshkov
Just a minor comment, can you also include the part which removes
msm_host->cphy_mode = src_phy->cphy_mode; from msm_dsi_host_set_src_pll
in this change itself so that its clear that you are removing from there
and moving it into a new API?
You can still keep my
Reviewed-by: Abhinav Kumar
once you address this.
---
drivers/gpu/drm/msm/dsi/dsi.h | 2 ++
drivers/gpu/drm/msm/dsi/dsi_host.c| 8
drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +++
3 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h
b/drivers/gpu/drm/msm/dsi/dsi.h
index b50db91cb8a7..7dfb6d198ca9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -118,6 +118,8 @@ unsigned long msm_dsi_host_get_mode_flags(struct
mipi_dsi_host *host);
struct drm_bridge *msm_dsi_host_get_bridge(struct mipi_dsi_host
*host);
int msm_dsi_host_register(struct mipi_dsi_host *host, bool
check_defer);
void msm_dsi_host_unregister(struct mipi_dsi_host *host);
+void msm_dsi_host_set_phy_mode(struct mipi_dsi_host *host,
+ struct msm_dsi_phy *src_phy);
int msm_dsi_host_set_src_pll(struct mipi_dsi_host *host,
struct msm_dsi_phy *src_phy);
void msm_dsi_host_reset_phy(struct mipi_dsi_host *host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..1ffcd0577e99 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2224,6 +2224,14 @@ void msm_dsi_host_cmd_xfer_commit(struct
mipi_dsi_host *host, u32 dma_base,
wmb();
}
+void msm_dsi_host_set_phy_mode(struct mipi_dsi_host *host,
+ struct msm_dsi_phy *src_phy)
+{
+ struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+ msm_host->cphy_mode = src_phy->cphy_mode;
+}
+
int msm_dsi_host_set_src_pll(struct mipi_dsi_host *host,
struct msm_dsi_phy *src_phy)
{
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index c41d39f5b7cf..49a0a0841487 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -77,6 +77,7 @@ static int dsi_mgr_setup_components(int id)
return ret;
msm_dsi_phy_set_usecase(msm_dsi->phy, MSM_DSI_PHY_STANDALONE);
+ msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
ret = msm_dsi_host_set_src_pll(msm_dsi->host, msm_dsi->phy);
} else if (!other_dsi) {
ret = 0;
@@ -104,6 +105,8 @@ static int dsi_mgr_setup_components(int id)
MSM_DSI_PHY_MASTER);
msm_dsi_phy_set_usecase(clk_slave_dsi->phy,
MSM_DSI_PHY_SLAVE);
+ msm_dsi_host_set_phy_mode(msm_dsi->host, msm_dsi->phy);
+ msm_dsi_host_set_phy_mode(other_dsi->host, other_dsi->phy);
ret = msm_dsi_host_set_src_pll(msm_dsi->host,
clk_master_dsi->phy);
if (ret)
return ret;
Re: [Freedreno] [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
drm_get_edid() internally calls to drm_connector_update_edid_property()
and then drm_add_display_info(), which parses the EDID.
This happens in the function intel_hdmi_set_edid() and
intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()).
Once EDID is parsed, the monitor HDMI support information is available
through drm_display_info.is_hdmi. Retriving the same information with
drm_detect_hdmi_monitor() is less efficient. Change to
drm_display_info.is_hdmi
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez
---
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index b04685bb6439..008e5b0ba408 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
to_intel_connector(connector)->detect_edid = edid;
if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
- intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
+ intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
connected = true;
}
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 6cb27599ea03..b4065e4df644 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
*connector)
if (edid->input & DRM_EDID_INPUT_DIGITAL) {
status = connector_status_connected;
if (intel_sdvo_connector->is_hdmi) {
- intel_sdvo->has_hdmi_monitor =
drm_detect_hdmi_monitor(edid);
intel_sdvo->has_hdmi_audio =
drm_detect_monitor_audio(edid);
+ intel_sdvo->has_hdmi_monitor =
+
connector->display_info.is_hdmi;
}
} else
status = connector_status_disconnected;
--
2.33.0
Re: [Freedreno] [PATCH v3 01/13] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
According to the documentation, drm_add_edid_modes
"... Also fills out the &drm_display_info structure and ELD in @connector
with any information which can be derived from the edid."
drm_add_edid_modes accepts a struct edid *edid parameter which may have a
value or may be null. When it is not null, connector->display_info and
connector->eld are updated according to the edid. When edid=NULL, only
connector->eld is reset. Reset connector->display_info to be consistent
and accurate.
Since drm_edid_is_valid() considers NULL as an invalid EDID, simplify the
code to avoid duplicating code in the case of NULL/error.
Signed-off-by: Claudio Suarez
---
drivers/gpu/drm/drm_edid.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6325877c5fd6..a019a26ede7a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5356,14 +5356,14 @@ int drm_add_edid_modes(struct drm_connector *connector,
struct edid *edid)
int num_modes = 0;
u32 quirks;
- if (edid == NULL) {
- clear_eld(connector);
- return 0;
- }
if (!drm_edid_is_valid(edid)) {
+ /* edid == NULL or invalid here */
clear_eld(connector);
- drm_warn(connector->dev, "%s: EDID invalid.\n",
-connector->name);
+ drm_reset_display_info(connector);
+ if (edid)
+ drm_warn(connector->dev,
+"[CONNECTOR:%d:%s] EDID invalid.\n",
+connector->base.id, connector->name);
return 0;
}
--
2.33.0
[Freedreno] [PATCH v2] drm/msm/dpu: Add CRC support for DPU
Add CRC support to DPU, which is currently not supported by
this driver. Only supports CRC for CRTC for now, but will extend support
to other blocks later on.
Changes in v2:
- Added kfree() calls for return paths in dpu_crtc_get_crc()
- Propogated error code for dpu_crtc_get_crc()
- Renamed skip_count
- Removed dpu_crtc_is_valid_crc_source()
- Removed wait for commit in dpu_crtc_set_crc_source()
- Moved crc_source from struct dpu_crtc to struct dpu_crtc_state
- Moved CRC register constants from dpu_hw_util.h to dpu_hw_lm.c
Validated with IGT kms_pipe_crc_basic, and kms_cursor_crc
Test: kms_pipe_crc_basic
Subtests Passed:
- bad-source
- read-crc-pipe-A
- read-crc-pipe-A-frame-sequence
- nonblocking-crc-pipe-A
- nonblocking-crc-pipe-A-frame-sequence
- disable-crc-after-crtc-pipe-A[1]
- compare-crc-sanitycheck-pipe-A[1]
Rest skipped
Test: kms_cursor_crc
Subtests Passed:
- pipe-A-cursor-size-change
- pipe-A-cursor-alpha-opaque
- pipe-A-cursor-alpha-transparent
Subtests Failed:
- pipe-A-cursor-dpms
- pipe-A-cursor-*-onscreen
- pipe-A-cursor-*-offscreen
Rest skipped
Tested on Qualcomm RB3 (debian, sdm845), Qualcomm RB5 (debian, qrb5165)
Reported-by: kernel test robot
Signed-off-by: Jessica Zhang
[1] Skipped on RB5 due to issue related to DPMS. Planning to upload a
fix for this in the future.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 153 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 19 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 56 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h | 13 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 +-
5 files changed, 239 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index eecfa88517e0..e91568d4f09a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2014-2018 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
* Copyright (C) 2013 Red Hat
* Author: Rob Clark
*/
@@ -70,6 +70,102 @@ static struct drm_encoder *get_encoder_from_crtc(struct
drm_crtc *crtc)
return NULL;
}
+static enum dpu_crtc_crc_source dpu_crtc_parse_crc_source(const char *src_name)
+{
+ if (!src_name ||
+ !strcmp(src_name, "none"))
+ return DPU_CRTC_CRC_SOURCE_NONE;
+ if (!strcmp(src_name, "auto") ||
+ !strcmp(src_name, "lm"))
+ return DPU_CRTC_CRC_SOURCE_LAYER_MIXER;
+
+ return DPU_CRTC_CRC_SOURCE_INVALID;
+}
+
+static int dpu_crtc_verify_crc_source(struct drm_crtc *crtc,
+ const char *src_name, size_t *values_cnt)
+{
+ enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
+ struct dpu_crtc_state *crtc_state = to_dpu_crtc_state(crtc->state);
+
+ if (source < 0) {
+ DRM_DEBUG_DRIVER("Invalid source %s for CRTC%d\n", src_name,
crtc->index);
+ return -EINVAL;
+ }
+
+ if (source == DPU_CRTC_CRC_SOURCE_LAYER_MIXER)
+ *values_cnt = crtc_state->num_mixers;
+
+ return 0;
+}
+
+static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
+{
+ enum dpu_crtc_crc_source source = dpu_crtc_parse_crc_source(src_name);
+ enum dpu_crtc_crc_source current_source;
+ struct drm_crtc_commit *commit;
+ struct dpu_crtc_state *crtc_state;
+ struct drm_device *drm_dev = crtc->dev;
+ struct dpu_crtc_mixer *m;
+
+ bool was_enabled;
+ bool enable = false;
+ int i, ret = 0;
+
+ if (source < 0) {
+ DRM_DEBUG_DRIVER("Invalid CRC source %s for CRTC%d\n",
src_name, crtc->index);
+ return -EINVAL;
+ }
+
+ ret = drm_modeset_lock(&crtc->mutex, NULL);
+
+ if (ret)
+ return ret;
+
+ enable = (source != DPU_CRTC_CRC_SOURCE_NONE);
+ crtc_state = to_dpu_crtc_state(crtc->state);
+
+ spin_lock_irq(&drm_dev->event_lock);
+ current_source = crtc_state->crc_source;
+ spin_unlock_irq(&drm_dev->event_lock);
+
+ was_enabled = (current_source != DPU_CRTC_CRC_SOURCE_NONE);
+
+ if (!was_enabled && enable) {
+ ret = drm_crtc_vblank_get(crtc);
+
+ if (ret)
+ goto cleanup;
+
+ } else if (was_enabled && !enable) {
+ drm_crtc_vblank_put(crtc);
+ }
+
+ spin_lock_irq(&drm_dev->event_lock);
+ crtc_state->crc_source = source;
+ spin_unlock_irq(&drm_dev->event_lock);
+
+ crtc_state->crc_frame_skip_count = 0;
+
+ for (i = 0; i < crtc_state->num_mixers; ++i) {
+ m = &crtc_state->mixers[i];
+
+ if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
+ continue;
+
+ /* Calculate MISR over 1 frame */
+ m->hw_lm->
Re: [Freedreno] [PATCH v2 01/13] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Tue, Oct 19, 2021 at 09:35:08PM +0300, Ville Syrjälä wrote:
> On Sat, Oct 16, 2021 at 08:42:14PM +0200, Claudio Suarez wrote:
> > According to the documentation, drm_add_edid_modes
> > "... Also fills out the &drm_display_info structure and ELD in @connector
> > with any information which can be derived from the edid."
> >
> > drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> > value or may be null. When it is not null, connector->display_info and
> > connector->eld are updated according to the edid. When edid=NULL, only
> > connector->eld is reset. Reset connector->display_info to be consistent
> > and accurate.
> >
> > Signed-off-by: Claudio Suarez
> > ---
> > drivers/gpu/drm/drm_edid.c | 11 +--
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 6325877c5fd6..c643db17782c 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -5356,14 +5356,13 @@ int drm_add_edid_modes(struct drm_connector
> > *connector, struct edid *edid)
> > int num_modes = 0;
> > u32 quirks;
> >
> > - if (edid == NULL) {
> > - clear_eld(connector);
> > - return 0;
> > - }
> > if (!drm_edid_is_valid(edid)) {
>
> OK, so drm_edid_is_valid() will happily accept NULL and considers
> it invalid. You may want to mention that explicitly in the commit
> message.
Thank you for your comments, I appreciate :)
I'm sending new mails with the new commit messages.
> > + /* edid == NULL or invalid here */
> > clear_eld(connector);
> > - drm_warn(connector->dev, "%s: EDID invalid.\n",
> > -connector->name);
> > + drm_reset_display_info(connector);
> > + if (edid)
> > + drm_warn(connector->dev, "%s: EDID invalid.\n",
> > +connector->name);
>
> Could you respin this to use the standard [CONNECTOR:%d:%s] form
> while at it? Or I guess a patch to mass convert the whole drm_edid.c
> might be another option.
Good point.
I like the idea of a new patch. I'll start working on it. I can change
this drm_warn here to avoid merge conflicts.
> Patch looks good.
> Reviewed-by: Ville Syrjälä
Thanks!
BR
Claudio Suarez.
Re: [Freedreno] [PATCH v2 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Sat, Oct 16, 2021 at 08:42:26PM +0200, Claudio Suarez wrote:
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi where possible.
We still need proof in the commit message that display_info
is actually populated by the time this gets called.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..008e5b0ba408 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..b4065e4df644 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> connector->display_info.is_hdmi;
> }
> } else
> status = connector_status_disconnected;
> --
> 2.33.0
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v2 01/13] gpu/drm: make drm_add_edid_modes() consistent when updating connector->display_info
On Sat, Oct 16, 2021 at 08:42:14PM +0200, Claudio Suarez wrote:
> According to the documentation, drm_add_edid_modes
> "... Also fills out the &drm_display_info structure and ELD in @connector
> with any information which can be derived from the edid."
>
> drm_add_edid_modes accepts a struct edid *edid parameter which may have a
> value or may be null. When it is not null, connector->display_info and
> connector->eld are updated according to the edid. When edid=NULL, only
> connector->eld is reset. Reset connector->display_info to be consistent
> and accurate.
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_edid.c | 11 +--
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6325877c5fd6..c643db17782c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5356,14 +5356,13 @@ int drm_add_edid_modes(struct drm_connector
> *connector, struct edid *edid)
> int num_modes = 0;
> u32 quirks;
>
> - if (edid == NULL) {
> - clear_eld(connector);
> - return 0;
> - }
> if (!drm_edid_is_valid(edid)) {
OK, so drm_edid_is_valid() will happily accept NULL and considers
it invalid. You may want to mention that explicitly in the commit
message.
> + /* edid == NULL or invalid here */
> clear_eld(connector);
> - drm_warn(connector->dev, "%s: EDID invalid.\n",
> - connector->name);
> + drm_reset_display_info(connector);
> + if (edid)
> + drm_warn(connector->dev, "%s: EDID invalid.\n",
> + connector->name);
Could you respin this to use the standard [CONNECTOR:%d:%s] form
while at it? Or I guess a patch to mass convert the whole drm_edid.c
might be another option.
Patch looks good.
Reviewed-by: Ville Syrjälä
> return 0;
> }
>
> --
> 2.33.0
>
>
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v2 04/11] drm/msm/disp/dpu1: Add DSC support in RM
On 19-10-21, 18:52, Dmitry Baryshkov wrote:
> On Tue, 19 Oct 2021 at 18:30, Vinod Koul wrote:
> >
> > On 14-10-21, 17:11, Dmitry Baryshkov wrote:
> > > On 07/10/2021 10:08, Vinod Koul wrote:
> >
> > > > +static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> > > > + struct dpu_global_state *global_state,
> > > > + struct drm_encoder *enc)
> > > > +{
> > > > + struct msm_drm_private *priv;
> > > > +
> > > > + priv = enc->dev->dev_private;
> > > > +
> > > > + if (!priv)
> > > > + return -EIO;
> > > > +
> > > > + /* check if DSC is supported */
> > > > + if (!priv->dsc)
> > > > + return 0;
> > > > +
> > > > + /* check if DSC 0 & 1 and allocated or not */
> > > > + if (global_state->dsc_to_enc_id[0] ||
> > > > global_state->dsc_to_enc_id[1]) {
> > > > + DPU_ERROR("DSC 0|1 is already allocated\n");
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > + global_state->dsc_to_enc_id[0] = enc->base.id;
> > > > + global_state->dsc_to_enc_id[1] = enc->base.id;
> > >
> > > Still hardcoding DSC_0 and DSC_1.
> >
> > Yes!
> >
> > > Could you please add num_dsc to the topology and allocate the requested
> > > amount of DSC blocks? Otherwise this would break for the DSI + DP case.
> >
> > It wont as we check for dsc and dont proceed, so it cant make an impact
> > in non dsc case.
> >
> > Nevertheless I agree with you, so I am making it based on dsc defined in
> > topology. Do we need additional field for num_dsc in topology, num_enc
> > should be it, right?
>
> I'd vote for the separate num_dsc.
Okay will update... will move up topology patch up in the order for that
as well
--
~Vinod
Re: [Freedreno] [PATCH v2 04/11] drm/msm/disp/dpu1: Add DSC support in RM
On Tue, 19 Oct 2021 at 18:30, Vinod Koul wrote:
>
> On 14-10-21, 17:11, Dmitry Baryshkov wrote:
> > On 07/10/2021 10:08, Vinod Koul wrote:
>
> > > +static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> > > + struct dpu_global_state *global_state,
> > > + struct drm_encoder *enc)
> > > +{
> > > + struct msm_drm_private *priv;
> > > +
> > > + priv = enc->dev->dev_private;
> > > +
> > > + if (!priv)
> > > + return -EIO;
> > > +
> > > + /* check if DSC is supported */
> > > + if (!priv->dsc)
> > > + return 0;
> > > +
> > > + /* check if DSC 0 & 1 and allocated or not */
> > > + if (global_state->dsc_to_enc_id[0] || global_state->dsc_to_enc_id[1])
> > > {
> > > + DPU_ERROR("DSC 0|1 is already allocated\n");
> > > + return -EIO;
> > > + }
> > > +
> > > + global_state->dsc_to_enc_id[0] = enc->base.id;
> > > + global_state->dsc_to_enc_id[1] = enc->base.id;
> >
> > Still hardcoding DSC_0 and DSC_1.
>
> Yes!
>
> > Could you please add num_dsc to the topology and allocate the requested
> > amount of DSC blocks? Otherwise this would break for the DSI + DP case.
>
> It wont as we check for dsc and dont proceed, so it cant make an impact
> in non dsc case.
>
> Nevertheless I agree with you, so I am making it based on dsc defined in
> topology. Do we need additional field for num_dsc in topology, num_enc
> should be it, right?
I'd vote for the separate num_dsc.
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 04/11] drm/msm/disp/dpu1: Add DSC support in RM
On 14-10-21, 17:11, Dmitry Baryshkov wrote:
> On 07/10/2021 10:08, Vinod Koul wrote:
> > +static int _dpu_rm_reserve_dsc(struct dpu_rm *rm,
> > + struct dpu_global_state *global_state,
> > + struct drm_encoder *enc)
> > +{
> > + struct msm_drm_private *priv;
> > +
> > + priv = enc->dev->dev_private;
> > +
> > + if (!priv)
> > + return -EIO;
> > +
> > + /* check if DSC is supported */
> > + if (!priv->dsc)
> > + return 0;
> > +
> > + /* check if DSC 0 & 1 and allocated or not */
> > + if (global_state->dsc_to_enc_id[0] || global_state->dsc_to_enc_id[1]) {
> > + DPU_ERROR("DSC 0|1 is already allocated\n");
> > + return -EIO;
> > + }
> > +
> > + global_state->dsc_to_enc_id[0] = enc->base.id;
> > + global_state->dsc_to_enc_id[1] = enc->base.id;
>
> Still hardcoding DSC_0 and DSC_1.
Yes!
> Could you please add num_dsc to the topology and allocate the requested
> amount of DSC blocks? Otherwise this would break for the DSI + DP case.
It wont as we check for dsc and dont proceed, so it cant make an impact
in non dsc case.
Nevertheless I agree with you, so I am making it based on dsc defined in
topology. Do we need additional field for num_dsc in topology, num_enc
should be it, right?
--
~Vinod
Re: [Freedreno] [PATCH v2 02/11] drm/msm/disp/dpu1: Add support for DSC
On 14-10-21, 17:40, Dmitry Baryshkov wrote:
> On 07/10/2021 10:08, Vinod Koul wrote:
> > +static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
> > + struct msm_display_dsc_config *dsc, u32 mode)
> > +{
> > + struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> > + u32 data, lsb, bpp;
> > + u32 initial_lines = dsc->initial_lines;
> > + bool is_cmd_mode = !(mode & BIT(2));
>
> DSC_MODE_VIDEO
Updated
> > +static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc,
> > +struct msm_display_dsc_config *dsc)
>
> I thought that it might make sense to pass just drm_dsc_rc_range_parameters
> here, but it's a matter of personal preference. I won't insist on doing
> that.
This is called from encoder, so prefer not to have encoder invoke
dsc->drm->rc_range_params
So will keep this.
--
~Vinod
