RE: [PATCH v2] drm: rcar-du: lvds: Get mode from state
Hi Laurent,
Thank you for your patch!
> From: linux-renesas-soc-ow...@vger.kernel.org
> On Behalf Of Laurent Pinchart
> Sent: 13 December 2019 18:28
> Subject: [PATCH v2] drm: rcar-du: lvds: Get mode from state
>
> The R-Car LVDS encoder driver implements the bridge .mode_set()
> operation for the sole purpose of storing the mode in the LVDS private
> data, to be used later when enabling the encoder.
>
> Switch to the bridge .atomic_enable() and .atomic_disable() operations
> in order to access the global atomic state, and get the mode from the
> state instead. Remove both the unneeded .mode_set() operation and the
> display_mode and mode fields storing state data from the rcar_lvds
> private structure.
>
> As a side effect we get the CRTC from the state, replace the CRTC
> pointer retrieved through the bridge's encoder that shouldn't be used by
> atomic drivers.
>
> Signed-off-by: Laurent Pinchart
> ---
> Changes since v1:
>
> - Call .atomic_enable() on the companion
> - Set companion->encoder in .attach()
>
> The patch has been tested on the Draak board with the HDMI output in
> LVDS dual-link mode, and on the Salvator-XS board with the HDMI, VGA and
> LVDS outputs in single-link mode.
>
> ---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 158 +++-
> 1 file changed, 85 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 8c6c172bbf2e..c550bfd59e71 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -65,9 +65,6 @@ struct rcar_lvds {
> struct clk *dotclkin[2];/* External DU clocks */
> } clocks;
>
> - struct drm_display_mode display_mode;
> - enum rcar_lvds_mode mode;
> -
> struct drm_bridge *companion;
> bool dual_link;
> };
> @@ -402,10 +399,51 @@ EXPORT_SYMBOL_GPL(rcar_lvds_clk_disable);
> * Bridge
> */
>
> -static void rcar_lvds_enable(struct drm_bridge *bridge)
> +static enum rcar_lvds_mode rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds,
> + const struct drm_connector *connector)
> +{
> + const struct drm_display_info *info;
> + enum rcar_lvds_mode mode;
> +
> + /*
> + * There is no API yet to retrieve LVDS mode from a bridge, only panels
> + * are supported.
> + */
> + if (!lvds->panel)
> + return RCAR_LVDS_MODE_JEIDA;
> +
> + info = >display_info;
> + if (!info->num_bus_formats || !info->bus_formats) {
> + dev_err(lvds->dev, "no LVDS bus format reported\n");
dev_warn perhaps?
Also, how about:
s/no LVDS bus format reported/no LVDS bus format reported, using JEIDA/
or something along those lines?
> + return RCAR_LVDS_MODE_JEIDA;
> + }
> +
> + switch (info->bus_formats[0]) {
> + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
Shall we take the below into account here?
https://lwn.net/Articles/794944/
> + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> + mode = RCAR_LVDS_MODE_JEIDA;
> + break;
> + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> + mode = RCAR_LVDS_MODE_VESA;
> + break;
> + default:
> + dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> + info->bus_formats[0]);
dev_warn perhaps?
Also, how about:
s/unsupported LVDS bus format 0x%04x/unsupported LVDS bus format 0x%04x, using
JEIDA/
or something along those lines?
> + return RCAR_LVDS_MODE_JEIDA;
> + }
> +
> + if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> + mode |= RCAR_LVDS_MODE_MIRROR;
> +
> + return mode;
> +}
> +
> +static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> + struct drm_atomic_state *state)
> {
> struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> - const struct drm_display_mode *mode = >display_mode;
> + struct drm_connector *connector;
> + struct drm_crtc *crtc;
> u32 lvdhcr;
> u32 lvdcr0;
> int ret;
> @@ -414,9 +452,14 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> if (ret < 0)
> return;
>
> + /* Retrieve the connector and CRTC through the atomic state. */
> + connector = drm_atomic_get_new_connector_for_encoder(state,
> + bridge->encoder);
> + crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> +
> /* Enable the companion LVDS encoder in dual-link m
Re: [PATCH v2] drm: rcar-du: lvds: Get mode from state
Hi Fabrizio,
On Mon, Dec 16, 2019 at 05:39:55PM +, Fabrizio Castro wrote:
> > From: linux-renesas-soc-ow...@vger.kernel.org
> > On Behalf Of Laurent Pinchart
> > Sent: 13 December 2019 18:28
> > Subject: [PATCH v2] drm: rcar-du: lvds: Get mode from state
> >
> > The R-Car LVDS encoder driver implements the bridge .mode_set()
> > operation for the sole purpose of storing the mode in the LVDS private
> > data, to be used later when enabling the encoder.
> >
> > Switch to the bridge .atomic_enable() and .atomic_disable() operations
> > in order to access the global atomic state, and get the mode from the
> > state instead. Remove both the unneeded .mode_set() operation and the
> > display_mode and mode fields storing state data from the rcar_lvds
> > private structure.
> >
> > As a side effect we get the CRTC from the state, replace the CRTC
> > pointer retrieved through the bridge's encoder that shouldn't be used by
> > atomic drivers.
> >
> > Signed-off-by: Laurent Pinchart
> > ---
> > Changes since v1:
> >
> > - Call .atomic_enable() on the companion
> > - Set companion->encoder in .attach()
> >
> > The patch has been tested on the Draak board with the HDMI output in
> > LVDS dual-link mode, and on the Salvator-XS board with the HDMI, VGA and
> > LVDS outputs in single-link mode.
> >
> > ---
> > drivers/gpu/drm/rcar-du/rcar_lvds.c | 158 +++-
> > 1 file changed, 85 insertions(+), 73 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 8c6c172bbf2e..c550bfd59e71 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -65,9 +65,6 @@ struct rcar_lvds {
> > struct clk *dotclkin[2];/* External DU clocks */
> > } clocks;
> >
> > - struct drm_display_mode display_mode;
> > - enum rcar_lvds_mode mode;
> > -
> > struct drm_bridge *companion;
> > bool dual_link;
> > };
> > @@ -402,10 +399,51 @@ EXPORT_SYMBOL_GPL(rcar_lvds_clk_disable);
> > * Bridge
> > */
> >
> > -static void rcar_lvds_enable(struct drm_bridge *bridge)
> > +static enum rcar_lvds_mode rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds,
> > + const struct drm_connector *connector)
> > +{
> > + const struct drm_display_info *info;
> > + enum rcar_lvds_mode mode;
> > +
> > + /*
> > +* There is no API yet to retrieve LVDS mode from a bridge, only panels
> > +* are supported.
> > +*/
> > + if (!lvds->panel)
> > + return RCAR_LVDS_MODE_JEIDA;
> > +
> > + info = >display_info;
> > + if (!info->num_bus_formats || !info->bus_formats) {
> > + dev_err(lvds->dev, "no LVDS bus format reported\n");
>
> dev_warn perhaps?
>
> Also, how about:
> s/no LVDS bus format reported/no LVDS bus format reported, using JEIDA/
> or something along those lines?
Ack.
> > + return RCAR_LVDS_MODE_JEIDA;
> > + }
> > +
> > + switch (info->bus_formats[0]) {
> > + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
>
> Shall we take the below into account here?
> https://lwn.net/Articles/794944/
Sure, but I think it's not required when multiple cases are grouped
together with no line in-between.
> > + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> > + mode = RCAR_LVDS_MODE_JEIDA;
> > + break;
> > + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> > + mode = RCAR_LVDS_MODE_VESA;
> > + break;
> > + default:
> > + dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
> > + info->bus_formats[0]);
>
> dev_warn perhaps?
>
> Also, how about:
> s/unsupported LVDS bus format 0x%04x/unsupported LVDS bus format 0x%04x,
> using JEIDA/
> or something along those lines?
Ack.
> > + return RCAR_LVDS_MODE_JEIDA;
> > + }
> > +
> > + if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
> > + mode |= RCAR_LVDS_MODE_MIRROR;
> > +
> > + return mode;
> > +}
> > +
> > +static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_atomic_state *state)
> > {
> > struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > - const struct drm_display_mode *mode = >display_mode;
> > + struct drm_con
[PATCH v2] drm: rcar-du: lvds: Get mode from state
The R-Car LVDS encoder driver implements the bridge .mode_set()
operation for the sole purpose of storing the mode in the LVDS private
data, to be used later when enabling the encoder.
Switch to the bridge .atomic_enable() and .atomic_disable() operations
in order to access the global atomic state, and get the mode from the
state instead. Remove both the unneeded .mode_set() operation and the
display_mode and mode fields storing state data from the rcar_lvds
private structure.
As a side effect we get the CRTC from the state, replace the CRTC
pointer retrieved through the bridge's encoder that shouldn't be used by
atomic drivers.
Signed-off-by: Laurent Pinchart
---
Changes since v1:
- Call .atomic_enable() on the companion
- Set companion->encoder in .attach()
The patch has been tested on the Draak board with the HDMI output in
LVDS dual-link mode, and on the Salvator-XS board with the HDMI, VGA and
LVDS outputs in single-link mode.
---
drivers/gpu/drm/rcar-du/rcar_lvds.c | 158 +++-
1 file changed, 85 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 8c6c172bbf2e..c550bfd59e71 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -65,9 +65,6 @@ struct rcar_lvds {
struct clk *dotclkin[2];/* External DU clocks */
} clocks;
- struct drm_display_mode display_mode;
- enum rcar_lvds_mode mode;
-
struct drm_bridge *companion;
bool dual_link;
};
@@ -402,10 +399,51 @@ EXPORT_SYMBOL_GPL(rcar_lvds_clk_disable);
* Bridge
*/
-static void rcar_lvds_enable(struct drm_bridge *bridge)
+static enum rcar_lvds_mode rcar_lvds_get_lvds_mode(struct rcar_lvds *lvds,
+ const struct drm_connector *connector)
+{
+ const struct drm_display_info *info;
+ enum rcar_lvds_mode mode;
+
+ /*
+* There is no API yet to retrieve LVDS mode from a bridge, only panels
+* are supported.
+*/
+ if (!lvds->panel)
+ return RCAR_LVDS_MODE_JEIDA;
+
+ info = >display_info;
+ if (!info->num_bus_formats || !info->bus_formats) {
+ dev_err(lvds->dev, "no LVDS bus format reported\n");
+ return RCAR_LVDS_MODE_JEIDA;
+ }
+
+ switch (info->bus_formats[0]) {
+ case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+ case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+ mode = RCAR_LVDS_MODE_JEIDA;
+ break;
+ case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+ mode = RCAR_LVDS_MODE_VESA;
+ break;
+ default:
+ dev_err(lvds->dev, "unsupported LVDS bus format 0x%04x\n",
+ info->bus_formats[0]);
+ return RCAR_LVDS_MODE_JEIDA;
+ }
+
+ if (info->bus_flags & DRM_BUS_FLAG_DATA_LSB_TO_MSB)
+ mode |= RCAR_LVDS_MODE_MIRROR;
+
+ return mode;
+}
+
+static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
+ struct drm_atomic_state *state)
{
struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
- const struct drm_display_mode *mode = >display_mode;
+ struct drm_connector *connector;
+ struct drm_crtc *crtc;
u32 lvdhcr;
u32 lvdcr0;
int ret;
@@ -414,9 +452,14 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
if (ret < 0)
return;
+ /* Retrieve the connector and CRTC through the atomic state. */
+ connector = drm_atomic_get_new_connector_for_encoder(state,
+bridge->encoder);
+ crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
+
/* Enable the companion LVDS encoder in dual-link mode. */
if (lvds->dual_link && lvds->companion)
- lvds->companion->funcs->enable(lvds->companion);
+ lvds->companion->funcs->atomic_enable(lvds->companion, state);
/*
* Hardcode the channels and control signals routing for now.
@@ -452,18 +495,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
* PLL clock configuration on all instances but the companion in
* dual-link mode.
*/
- if (!lvds->dual_link || lvds->companion)
+ if (!lvds->dual_link || lvds->companion) {
+ const struct drm_crtc_state *crtc_state =
+ drm_atomic_get_new_crtc_state(state, crtc);
+ const struct drm_display_mode *mode =
+ _state->adjusted_mode;
+
lvds->info->pll_setup(lvds, mode->clock * 1000);
+ }
/* Set the LVDS mode and select the input. */
- lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
+ lvdcr0 = rcar_lvds_get_lvds_mode(lvds, connector) << LVDCR0_LVMD_SHIFT;
if (lvds->bridge.encoder) {
- /*
