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

Reply via email to