Hello,
On Wednesday, 19 December 2018 09:26:08 EET Andrzej Hajda wrote:
> On 30.11.2018 14:42, Neil Armstrong wrote:
> > From: Zheng Yang <zhengy...@rock-chips.com>
> >
> > To get input/output bus_format/enc_format dynamically, this patch
> >
> > introduce following funstion in plat_data:
> > - get_input_bus_format
> > - get_output_bus_format
> > - get_enc_in_encoding
> > - get_enc_out_encoding
>
> It seems fishy. On one side description says about dynamic resolution of
> formats and encodings.
>
> On the other side these functions as only argument takes platform_data
> which should be rather static.
>
> Where is this "dynamic" thing? The only usage of these callbacks I have
> found in next patches is also not dynamic, the functions just return
> some static value.
>
> Moreover function takes void* argument, which is again something
> suspicious, why cannot you pass know structure?
>
> And finally encoding usually should depend on display mode, it should
> not depend only static data.
>
>
> What kind of problems do you want to solve here?
I would add that this doesn't seem to be specific to dw-hdmi in any way. I'd
prefer an API at the drm_bridge level to handle this.
> > Signed-off-by: Zheng Yang <zhengy...@rock-chips.com>
> > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> > ---
> >
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 28 +++++++++++++++++------
> > include/drm/bridge/dw_hdmi.h | 5 ++++
> > 2 files changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > 4a9a24e854db..bd564ffdf18b 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -1810,6 +1810,7 @@ static void hdmi_disable_overflow_interrupts(struct
> > dw_hdmi *hdmi)>
> > static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode
> > *mode) {
> >
> > int ret;
> >
> > + void *data = hdmi->plat_data->phy_data;
> >
> > hdmi_disable_overflow_interrupts(hdmi);
> >
> > @@ -1821,10 +1822,13 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
> > struct drm_display_mode *mode)>
> > dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
> >
> > }
> >
> > - if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> > - (hdmi->vic == 21) || (hdmi->vic == 22) ||
> > - (hdmi->vic == 2) || (hdmi->vic == 3) ||
> > - (hdmi->vic == 17) || (hdmi->vic == 18))
> > + if (hdmi->plat_data->get_enc_out_encoding)
> > + hdmi->hdmi_data.enc_out_encoding =
> > + hdmi->plat_data->get_enc_out_encoding(data);
> > + else if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
> > + (hdmi->vic == 21) || (hdmi->vic == 22) ||
> > + (hdmi->vic == 2) || (hdmi->vic == 3) ||
> > + (hdmi->vic == 17) || (hdmi->vic == 18))
> >
> > hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_601;
> >
> > else
> >
> > hdmi->hdmi_data.enc_out_encoding = V4L2_YCBCR_ENC_709;
> >
> > @@ -1833,21 +1837,31 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
> > struct drm_display_mode *mode)>
> > hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0;
> >
> > /* TOFIX: Get input format from plat data or fallback to RGB888 */
> >
> > - if (hdmi->plat_data->input_bus_format)
> > + if (hdmi->plat_data->get_input_bus_format)
> > + hdmi->hdmi_data.enc_in_bus_format =
> > + hdmi->plat_data->get_input_bus_format(data);
> > + else if (hdmi->plat_data->input_bus_format)
> >
> > hdmi->hdmi_data.enc_in_bus_format =
> >
> > hdmi->plat_data->input_bus_format;
> >
> > else
> >
> > hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >
> > /* TOFIX: Get input encoding from plat data or fallback to none */
> >
> > - if (hdmi->plat_data->input_bus_encoding)
> > + if (hdmi->plat_data->get_enc_in_encoding)
> > + hdmi->hdmi_data.enc_in_encoding =
> > + hdmi->plat_data->get_enc_in_encoding(data);
> > + else if (hdmi->plat_data->input_bus_encoding)
> >
> > hdmi->hdmi_data.enc_in_encoding =
> >
> > hdmi->plat_data->input_bus_encoding;
> >
> > else
> >
> > hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT;
> >
> > /* TOFIX: Default to RGB888 output format */
> >
> > - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > + if (hdmi->plat_data->get_output_bus_format)
> > + hdmi->hdmi_data.enc_out_bus_format =
> > + hdmi->plat_data->get_output_bus_format(data);
> > + else
> > + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> >
> > hdmi->hdmi_data.pix_repet_factor = 0;
> > hdmi->hdmi_data.hdcp_enable = 0;
> >
> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index 7a02744ce0bc..2e797f782c51 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -142,6 +142,11 @@ struct dw_hdmi_plat_data {
> >
> > int (*configure_phy)(struct dw_hdmi *hdmi,
> >
> > const struct dw_hdmi_plat_data *pdata,
> > unsigned long mpixelclock);
> >
> > +
> > + unsigned long (*get_input_bus_format)(void *data);
> > + unsigned long (*get_output_bus_format)(void *data);
> > + unsigned long (*get_enc_in_encoding)(void *data);
> > + unsigned long (*get_enc_out_encoding)(void *data);
> >
> > };
> >
> > struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
