Re: [PATCH 1/3] drm: of: Mark empty drm_of_get_data_lanes_count and drm_of_get_data_lanes_ep static
On Sun, Jun 12, 2022 at 03:21:50PM +0200, Marek Vasut wrote:
> Mark empty implementations of drm_of_get_data_lanes_count and
> drm_of_get_data_lanes_ep as static inline, just like the rest
> of empty implementations of various functions in drm_of.h .
> Add missing comma to drm_of_get_data_lanes_count_ep() .
>
> Fixes: fc801750b197 ("drm: of: Add drm_of_get_data_lanes_count and
> drm_of_get_data_lanes_ep")
> Reported-by: kernel test robot
> Signed-off-by: Marek Vasut
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Maxime Ripard
> Cc: Robert Foss
> Cc: Sam Ravnborg
> To: dri-devel@lists.freedesktop.org
Acked-by: Sam Ravnborg
Re: [PATCH v3] drm/bridge: ti-sn65dsi83: Convert to drm_of_get_data_lanes_count
Hi Marek,
On Sun, Jun 12, 2022 at 12:29:47PM +0200, Marek Vasut wrote:
> Convert driver to use this new helper to standardize
> OF "data-lanes" parsing.
>
> Reviewed-by: Andrzej Hajda
> Signed-off-by: Marek Vasut
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Maxime Ripard
> Cc: Robert Foss
> Cc: Sam Ravnborg
> To: dri-devel@lists.freedesktop.org
Looks fine.
I wondered why the _ep variant could not be used, but endpoint is
required later and there is no need to look it up twice.
Acked-by: Sam Ravnborg
Sam
> ---
> V2: - Rename drm_of_get_data_lanes{,_ep} to drm_of_get_data_lanes_count{,_ep}
> - Add RB from Andrzej
> V3: - Rebase on latest next
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 8bf99b32776e2..b27c0d7c451ad 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -620,7 +620,7 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
> int dsi_lanes, ret;
>
> endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> - dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> + dsi_lanes = drm_of_get_data_lanes_count(endpoint, 1, 4);
> host_node = of_graph_get_remote_port_parent(endpoint);
> host = of_find_mipi_dsi_host_by_node(host_node);
> of_node_put(host_node);
> --
> 2.35.1
Re: [PATCH v2] drm/bridge: ti-sn65dsi83: Do not cache dsi_lanes and host twice
Hi Marek, On Sun, Jun 12, 2022 at 12:29:18PM +0200, Marek Vasut wrote: > The DSI lane count can be accessed via the dsi device pointer, make use > of that. The DSI host pointer is only used in sn65dsi83_host_attach(), > move the code around so that the host does not have to be cached in the > driver private data. This simplifies the code further. No functional > change. > > This has the added bonus that lt9211, tc358767, sn65dsi83 now use very > similar *_mipi_dsi_host_attach() which is ripe for deduplication. > > Signed-off-by: Marek Vasut > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Maxime Ripard > Cc: Robert Foss > Cc: Sam Ravnborg Looks OK, Acked-by: Sam Ravnborg
Re: [PATCH] drm: Fix htmldocs indentation warning w/ DP AUX power requirements
On Sat, Jun 11, 2022 at 09:55:04AM -0700, Douglas Anderson wrote:
> Two blank lines are needed to make the rst valid.
>
> Fixes: 69ef4a192bba ("drm: Document the power requirements for DP AUX
> transfers")
> Reported-by: Stephen Rothwell
> Signed-off-by: Douglas Anderson
Acked-by: Sam Ravnborg
> ---
>
> include/drm/display/drm_dp_helper.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/display/drm_dp_helper.h
> b/include/drm/display/drm_dp_helper.h
> index dc3c02225fcf..c5f8f45511ed 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -372,8 +372,10 @@ struct drm_dp_aux {
>* Also note that this callback can be called no matter the
>* state @dev is in and also no matter what state the panel is
>* in. It's expected:
> + *
>* - If the @dev providing the AUX bus is currently unpowered then
>* it will power itself up for the transfer.
> + *
>* - If we're on eDP (using a drm_panel) and the panel is not in a
>* state where it can respond (it's not powered or it's in a
>* low power state) then this function may return an error, but
> --
> 2.36.1.476.g0c4daa206d-goog
Re: [PATCH] dt-bindings: display: panel-simple: Add Arm virtual platforms display
Hi Rob,
On Fri, Jun 10, 2022 at 02:38:18PM -0600, Rob Herring wrote:
> 'arm,rtsm-display' is a panel for Arm, Ltd. virtual platforms (e.g. FVP).
> The binding has been in use for a long time, but was never documented.
>
> Some users and an example have a 'panel-dpi' compatible, but that's not
> needed without a 'panel-timing' node which none of the users have since
> commit 928faf5e3e8d ("arm64: dts: fvp: Remove panel timings"). The
> example does have a 'panel-timing' node, but it should not for the
> same reasons the node was removed in the dts files. So update the
> example in arm,pl11x.yaml to match the schema.
>
> Cc: Linus Walleij
> Cc: Robin Murphy
> Cc: Andre Przywara
> Signed-off-by: Rob Herring
> ---
> .../bindings/display/arm,pl11x.yaml | 15 +-
> .../bindings/display/panel/panel-simple.yaml | 20 +--
> 2 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/arm,pl11x.yaml
> b/Documentation/devicetree/bindings/display/arm,pl11x.yaml
> index b545c6d20325..6cc9045e5c68 100644
> --- a/Documentation/devicetree/bindings/display/arm,pl11x.yaml
> +++ b/Documentation/devicetree/bindings/display/arm,pl11x.yaml
> @@ -159,25 +159,12 @@ examples:
> };
>
> panel {
> -compatible = "arm,rtsm-display", "panel-dpi";
> -power-supply = <_supply>;
> +compatible = "arm,rtsm-display";
>
> port {
> clcd_panel: endpoint {
> remote-endpoint = <_pads>;
> };
> };
> -
> -panel-timing {
> -clock-frequency = <25175000>;
> -hactive = <640>;
> -hback-porch = <40>;
> -hfront-porch = <24>;
> -hsync-len = <96>;
> -vactive = <480>;
> -vback-porch = <32>;
> -vfront-porch = <11>;
> -vsync-len = <2>;
> -};
> };
> ...
> diff --git
> a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> index 21ba90c9fe33..97afd276c54a 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> @@ -19,9 +19,6 @@ description: |
>
>If the panel is more advanced a dedicated binding file is required.
>
> -allOf:
> - - $ref: panel-common.yaml#
> -
> properties:
>
>compatible:
> @@ -35,6 +32,8 @@ properties:
>- ampire,am-480272h3tmqw-t01h
> # Ampire AM-800480R3TMQW-A1H 7.0" WVGA TFT LCD panel
>- ampire,am800480r3tmqwa1h
> +# Arm, Ltd. Virtual Platforms Display
> + - arm,rtsm-display
> # AU Optronics Corporation 10.1" WSVGA TFT LCD panel
>- auo,b101aw03
> # AU Optronics Corporation 10.1" WSVGA TFT LCD panel
> @@ -340,9 +339,18 @@ properties:
>
> additionalProperties: false
>
> -required:
> - - compatible
> - - power-supply
> +allOf:
> + - $ref: panel-common.yaml#
> + - if:
> + # Most 'simple' panels must have a single supply, but a virtual
> display does not
> + not:
> +properties:
> + compatible:
> +contains:
> + const: arm,rtsm-display
> +then:
> + required:
> +- power-supply
Sorry, but I do not like this change. The beauty of panel-simple is that
this is a collection of simple display with identical bindings because
the HW is more or less the same (in general - not in details like size
etc).
Any panels that requires more are pushed out to their own binding and
for arm,rtsm-display that would be better.
It is not this single exceptions that bothers me, it is the many
exceptions we will have in a few years from now.
Sam
Re: [PATCH] staging: ftbft: Use backlight helper
On Fri, Jun 10, 2022 at 10:43:06PM +0200, Stephen Kitt wrote: > Hi Sam, > > On Fri, 10 Jun 2022 22:22:59 +0200, Sam Ravnborg wrote: > > On Tue, Jun 07, 2022 at 08:55:16PM +0200, Stephen Kitt wrote: > > > backlight_properties.fb_blank is deprecated. The states it represents > > > are handled by other properties; but instead of accessing those > > > properties directly, drivers should use the helpers provided by > > > backlight.h. > > > > > > Instead of manually checking the power state in struct > > > backlight_properties, use backlight_is_blank(). > > > > > > Signed-off-by: Stephen Kitt > > > Cc: Greg Kroah-Hartman > > > Cc: "Noralf Trønnes" > > > Cc: Thomas Zimmermann > > > Cc: Andy Shevchenko > > > Cc: Javier Martinez Canillas > > > Cc: Len Baker > > > Cc: dri-devel@lists.freedesktop.org > > > Cc: linux-fb...@vger.kernel.org > > > Cc: linux-stag...@lists.linux.dev > > > --- > > > drivers/staging/fbtft/fb_ssd1351.c | 3 +-- > > > drivers/staging/fbtft/fbtft-core.c | 3 +-- > > > 2 files changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/staging/fbtft/fb_ssd1351.c > > > b/drivers/staging/fbtft/fb_ssd1351.c index 6fd549a424d5..b8d55aa8c5c7 > > > 100644 --- a/drivers/staging/fbtft/fb_ssd1351.c > > > +++ b/drivers/staging/fbtft/fb_ssd1351.c > > > @@ -196,8 +196,7 @@ static int update_onboard_backlight(struct > > > backlight_device *bd) "%s: power=%d, fb_blank=%d\n", > > > __func__, bd->props.power, bd->props.fb_blank); > > Could you try to kill this use of props.fb_blank too? ^^ > > A local variable should do the trick. > > I have a pending patch changing this to show props.state instead, that way > the debug info shows all the backlight-relevant information from props. How > does that sound? In the end we want to have only _one_ state for backlight, so the simpler solution that just use one of the helpers would be better. For this driver there is no reason to do much anyway. Sam
Re: [PATCH 1/7] fbdev: aty128fb: Use backlight helper
Hi Stepehen, On Tue, Jun 07, 2022 at 09:23:29PM +0200, Stephen Kitt wrote: > Instead of retrieving the backlight brightness in struct > backlight_properties manually, and then checking whether the backlight > should be on at all, use backlight_get_brightness() which does all > this and insulates this from future changes. > > Signed-off-by: Stephen Kitt > Cc: Paul Mackerras > Cc: Helge Deller > Cc: linux-fb...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org This and the other 6 patches in this series are all: Reviewed-by: Sam Ravnborg I did not receive the cover letter, which is why I reply to this mail. Sam
Re: [PATCH] staging: ftbft: Use backlight helper
Hi Stephen, On Tue, Jun 07, 2022 at 08:55:16PM +0200, Stephen Kitt wrote: > backlight_properties.fb_blank is deprecated. The states it represents > are handled by other properties; but instead of accessing those > properties directly, drivers should use the helpers provided by > backlight.h. > > Instead of manually checking the power state in struct > backlight_properties, use backlight_is_blank(). > > Signed-off-by: Stephen Kitt > Cc: Greg Kroah-Hartman > Cc: "Noralf Trønnes" > Cc: Thomas Zimmermann > Cc: Andy Shevchenko > Cc: Javier Martinez Canillas > Cc: Len Baker > Cc: dri-devel@lists.freedesktop.org > Cc: linux-fb...@vger.kernel.org > Cc: linux-stag...@lists.linux.dev > --- > drivers/staging/fbtft/fb_ssd1351.c | 3 +-- > drivers/staging/fbtft/fbtft-core.c | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/fbtft/fb_ssd1351.c > b/drivers/staging/fbtft/fb_ssd1351.c > index 6fd549a424d5..b8d55aa8c5c7 100644 > --- a/drivers/staging/fbtft/fb_ssd1351.c > +++ b/drivers/staging/fbtft/fb_ssd1351.c > @@ -196,8 +196,7 @@ static int update_onboard_backlight(struct > backlight_device *bd) > "%s: power=%d, fb_blank=%d\n", > __func__, bd->props.power, bd->props.fb_blank); Could you try to kill this use of props.fb_blank too? ^^ A local variable should do the trick. Sam > > - on = (bd->props.power == FB_BLANK_UNBLANK) && > - (bd->props.fb_blank == FB_BLANK_UNBLANK); > + on = !backlight_is_blank(bd); > /* Onboard backlight connected to GPIO0 on SSD1351, GPIO1 unused */ > write_reg(par, 0xB5, on ? 0x03 : 0x02); > > diff --git a/drivers/staging/fbtft/fbtft-core.c > b/drivers/staging/fbtft/fbtft-core.c > index 60b2278d8b16..9b3eaed80cdd 100644 > --- a/drivers/staging/fbtft/fbtft-core.c > +++ b/drivers/staging/fbtft/fbtft-core.c > @@ -137,8 +137,7 @@ static int fbtft_backlight_update_status(struct > backlight_device *bd) > "%s: polarity=%d, power=%d, fb_blank=%d\n", > __func__, polarity, bd->props.power, bd->props.fb_blank); > > - if ((bd->props.power == FB_BLANK_UNBLANK) && > - (bd->props.fb_blank == FB_BLANK_UNBLANK)) > + if (!backlight_is_blank(bd)) > gpiod_set_value(par->gpio.led[0], polarity); > else > gpiod_set_value(par->gpio.led[0], !polarity); > > base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56 > -- > 2.30.2
Re: [PATCH 2/3] drm/panel: panel-dsi-cm: Use backlight helpers
Hi Stephen, > > > > > > > > Thanks, I’ll wait a little more to see if there are any other reviews of > > > > the patches and then push a v2 with that fix. > > > It would be very nice if you could kill all uses of FB_BLANK in the > > > drivers/gpu/drm/panel/* drivers, and post them as one series. > > > This is long overdue to introduce the backlight helpers. > > > > > > The three you posted is already a nice step forward, and there may be > > > more panel drivers I have missed. > > > > With this series on top of 5.19-rc1, the only remaining .fb_blank reference > > is in acx565akm_backlight_init() in panel-sony-acx565akm.c; I was planning > > on nuking that along with the other .fb_blank initialisers in a series > > removing .fb_blank entirely from backlight_properties. I’ll add it as a > > fourth patch for drm/panel if that makes things easier! > > That’s in drivers/gpu/drm/panel of course, there are a few others elsewhere > (I’ve got patches in flight for most of them, I’ve got the rest ready for > submission). > > > There will still be references to FB_BLANK constants since they’re used for > > backlight_properties.power values. Would it make sense to rename those? > > Just to make sure — I’m cleaning up backlight_properties.fb_blank, not > fb_ops.fb_blank. I wasn’t planning on touching the latter... Nuking props.fb_blank - that a fine goal - so keep focus there. We can then later revisit the other clean-up possibilities. Since you do this tree-wide do not do the mistake and try to cover too much at the same time, because then you never finish. So forget my comment for now and keep up the good work on removing props.fb_blank. Sam
Re: [PATCH 2/3] drm/panel: panel-dsi-cm: Use backlight helpers
Hi Stephen.
On Fri, Jun 10, 2022 at 07:47:20PM +0200, Stephen Kitt wrote:
> Hi Sebastian,
>
> On Thu, 9 Jun 2022 23:52:36 +0200, Sebastian Reichel
> wrote:
> > On Tue, Jun 07, 2022 at 08:20:25PM +0200, Stephen Kitt wrote:
> > > diff --git a/drivers/gpu/drm/panel/panel-dsi-cm.c
> > > b/drivers/gpu/drm/panel/panel-dsi-cm.c index b58cb064975f..aa36dc6cedd3
> > > 100644 --- a/drivers/gpu/drm/panel/panel-dsi-cm.c
> > > +++ b/drivers/gpu/drm/panel/panel-dsi-cm.c
> > > @@ -86,16 +86,10 @@ static void dsicm_bl_power(struct panel_drv_data
> > > *ddata, bool enable) return;
> > >
> > > if (enable) {
> > > - backlight->props.fb_blank = FB_BLANK_UNBLANK;
> > > - backlight->props.state = ~(BL_CORE_FBBLANK |
> > > BL_CORE_SUSPENDED);
> > > - backlight->props.power = FB_BLANK_UNBLANK;
> > > + backlight_enable(backlight);
> > > } else {
> > > - backlight->props.fb_blank = FB_BLANK_NORMAL;
> > > - backlight->props.power = FB_BLANK_POWERDOWN;
> > > - backlight->props.state |= BL_CORE_FBBLANK |
> > > BL_CORE_SUSPENDED;
> > > + backlight_disable(backlight);
> > > }
> >
> > The brackets can be removed now. Otherwise:
>
> >
> > Reviewed-by: Sebastian Reichel
>
> Thanks, I’ll wait a little more to see if there are any other reviews of the
> patches and then push a v2 with that fix.
It would be very nice if you could kill all uses of FB_BLANK in the
drivers/gpu/drm/panel/* drivers, and post them as one series.
This is long overdue to introduce the backlight helpers.
The three you posted is already a nice step forward, and there may be
more panel drivers I have missed.
Sam
Re: [PATCH v2] fbdev: atmel_lcdfb: Rework backlight status updates
Hi Stephen, thanks! On Thu, Jun 09, 2022 at 08:04:40PM +0200, Stephen Kitt wrote: > Instead of checking the state of various backlight_properties fields > against the memorised state in atmel_lcdfb_info.bl_power, > atmel_bl_update_status() should retrieve the desired state using > backlight_get_brightness (which takes into account the power state, > blanking etc.). This means the explicit checks using props.fb_blank > and props.power can be dropped. > > The backlight framework ensures that backlight is never negative, so > the test before reading the brightness from the hardware always ends > up false and the whole block can be removed. The framework retrieves > the brightness from the hardware through atmel_bl_get_brightness() > when necessary. > > As a result, bl_power in struct atmel_lcdfb_info is no longer > necessary, so remove that while we're at it. Since we only ever care > about reading the current state in backlight_properties, drop the > updates at the end of the function. > > Signed-off-by: Stephen Kitt > Cc: Nicolas Ferre > Cc: Helge Deller > Cc: Alexandre Belloni > Cc: Claudiu Beznea > Cc: linux-fb...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-arm-ker...@lists.infradead.org Acked-by: Sam Ravnborg Sam
Re: [PATCH] fbdev: atmel_lcdfb: Rework backlight status updates
Hi Stephen, thanks for taking care of all these backlight simplifications - this really helps to make the code simpler and more readable. On Thu, Jun 09, 2022 at 10:54:12AM +0100, Daniel Thompson wrote: > On Wed, Jun 08, 2022 at 10:56:23PM +0200, Stephen Kitt wrote: > > Instead of checking the state of various backlight_properties fields > > against the memorised state in atmel_lcdfb_info.bl_power, > > atmel_bl_update_status() should retrieve the desired state using > > backlight_get_brightness (which takes into account the power state, > > blanking etc.). This means the explicit checks using props.fb_blank > > and props.power can be dropped. > > > > Then brightness can only be negative if the backlight is on but > > props.brightness is negative, so the test before reading the > > brightness value from the hardware can be simplified to > > (brightness < 0). > > props.brightness should always be in the interval 0..max_brightness. > > This is enforced by the main backlight code (and APIs to set the > brightness use unsigned values). Thus props.brightness could only be > negative is the driver explicitly sets a negative value as some kind of > placeholder (which this driver does not do). > > I don't think there is any need to keep this logic. Daniel is right - please drop the "if (brightness < 0)" logic. I have looked a bit on the datasheet in my attempt to do a drm version of this driver - something that I am yet to succeed and the backlight core avoid any negative values. Sam
Mark olpc_dcon BROKEN [Was: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c]
Hi Javier.
On Thu, Jun 09, 2022 at 03:09:21PM +0200, Javier Martinez Canillas wrote:
> Hello Thomas,
>
> On 6/9/22 13:49, Thomas Zimmermann wrote:
> > Hi Javier
> >
> > Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
> >> From: Daniel Vetter
> >>
> >> Well except when the olpc dcon fbdev driver is enabled, that thing
> >> digs around in there in rather unfixable ways.
> >
> > There is fb_client_register() to set up a 'client' on top of an fbdev.
> > The client would then get messages about modesetting, blanks, removals,
> > etc. But you'd probably need an OLPC to convert dcon, and the mechanism
> > itself is somewhat unloved these days.
> >
> > Your patch complicates the fbdev code AFAICT. So I'd either drop it or,
> > even better, build a nicer interface for dcon.
> >
> > The dcon driver appears to look only at the first entry. Maybe add
> > fb_info_get_by_index() and fb_info_put() and export those. They would be
> > trivial wrappers somewhere in fbmem.c:
> >
> > #if IS_ENABLED(CONFIG_FB_OLPC_DCON)
> > struct fb_info *fb_info_get_by_index(unsigned int index)
> > {
> > return get_fb_info(index);
> > }
> > EXPORT_SYMBOL()
> > void fb_info_put(struct fb_info *fb_info)
> > {
> > put_fb_info(fb_info);
> > }
> > EXPORT_SYMBOL()
> > #endif
> >
> > In dcon itself, using the new interfaces will actually acquire a
> > reference to keep the display alive. The code at [1] could be replaced.
> > And a call to fb_info_put() needs to go into dcon_remove(). [2]
> >
>
> Thanks for your suggestions, that makes sense to me. I'll drop this
> patch from the set and post as a follow-up a different approach as
> you suggested.
To repeat myself from irc.
olpc_dcon is a staging driver and we should avoid inventing anything in
core code for to make staging drivers works.
Geert suggested EXPORT_SYMPBOL_NS_GPL() that could work and narrow it
down to olpc_dcon.
The better approach is to mark said driver BROKEN and then someone can
fix it it there is anyone who cares.
Last commit to olpc_dcon was in 2019: e40219d5e4b2177bfd4d885e7b64e3b236af40ac
and maybe Jerry Lin cares enough to fix it.
Added Jerry and Greg to the mail.
Sam
Re: [PATCH v4 1/8] drm/panel: Add an API drm_panel_get_orientation() to return panel orientation
Hi Hans, > > Please move this up so it is together with the other get_* methods, in > > alphabetic order. That is, right after get_modes(), and then this also > > matches the order in the .c file with is extra bonus. > > The downside of moving this up is that it will break drivers which don't > use c99 style named-struct-field initializers for there drm_panel_funcs. > > I admit that no drivers should be using the old style struct init, but > are we sure that that is the case? There is no in-tree driver that uses old style struct init for drm_panel_funcs - so we are safe here. I just did a quick git grep -A 4 drm_panel_funcs to verify it, browsing through the output did not reveal any old style users. Sam
Re: [PATCH v4 8/8] drm/mediatek: Config orientation property if panel provides it
Hi Hsin-Yi,
On Mon, Jun 06, 2022 at 11:24:31PM +0800, Hsin-Yi Wang wrote:
> Panel orientation property should be set before drm_dev_register().
> Mediatek drm driver calls drm_dev_register() in .bind(). However, most
> panels sets orientation property relatively late, mostly in .get_modes()
> callback, since this is when they are able to get the connector and
> binds the orientation property to it, though the value should be known
> when the panel is probed.
>
> Let the drm driver check if the remote end point is a panel and if it
> contains the orientation property. If it does, set it before
> drm_dev_register() is called.
I do not know the best way to do what you need to do here.
But it seems wrong to introduce a deprecated function
(drm_of_find_panel_or_bridge), to set the rotation property early.
I think you need to find a way to do this utilising the panel_bridge or
something.
Sam
>
> Signed-off-by: Hsin-Yi Wang
> Reviewed-by: Hans de Goede
> Reviewed-by: AngeloGioacchino Del Regno
>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index d9f10a33e6fa..c56282412bfa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -185,6 +185,7 @@ struct mtk_dsi {
> struct drm_encoder encoder;
> struct drm_bridge bridge;
> struct drm_bridge *next_bridge;
> + struct drm_panel *panel;
> struct drm_connector *connector;
> struct phy *phy;
>
> @@ -822,6 +823,12 @@ static int mtk_dsi_encoder_init(struct drm_device *drm,
> struct mtk_dsi *dsi)
> ret = PTR_ERR(dsi->connector);
> goto err_cleanup_encoder;
> }
> +
> + /* Read panel orientation */
> + if (dsi->panel)
> + drm_connector_set_panel_orientation(dsi->connector,
> + drm_panel_get_orientation(dsi->panel));
> +
> drm_connector_attach_encoder(dsi->connector, >encoder);
>
> return 0;
> @@ -837,6 +844,9 @@ static int mtk_dsi_bind(struct device *dev, struct device
> *master, void *data)
> struct drm_device *drm = data;
> struct mtk_dsi *dsi = dev_get_drvdata(dev);
>
> + /* Get panel if existed */
> + drm_of_find_panel_or_bridge(dev->of_node, 0, 0, >panel, NULL);
> +
> ret = mtk_dsi_encoder_init(drm, dsi);
> if (ret)
> return ret;
> --
> 2.36.1.255.ge46751e96f-goog
Re: [PATCH v4 0/8] Add a panel API to return panel orientation
Hi Hsin-Yi, thanks for this nice series. On Mon, Jun 06, 2022 at 11:24:23PM +0800, Hsin-Yi Wang wrote: > Panels usually call drm_connector_set_panel_orientation(), which is > later than drm/kms driver calling drm_dev_register(). This leads to a > WARN()[1]. > > The orientation property is known earlier. For example, some panels > parse the property through device tree during probe. > > The series add a panel API drm_panel_get_orientation() for drm/kms > drivers. The drivers can use the API to get panel's orientation, so they > can call drm_connector_set_panel_orientation() before drm_dev_register(). > > Panel needs to implement .get_orientation callback to return the property. The following comment appears in every panel driver: + /* +* drm drivers are expected to call drm_panel_get_orientation() to get +* panel's orientation then drm_connector_set_panel_orientation() to +* set the property before drm_dev_register(). Otherwise there will be +* a WARN_ON if orientation is set after drm is registered. +*/ Please move it to the drm_panel c or h file, it is noise to add it to all panel drivers. Sam
Re: [PATCH v4 5/8] drm/panel: panel-simple: Implement .get_orientation callback
Hi Hsin-Yi,
On Mon, Jun 06, 2022 at 11:24:28PM +0800, Hsin-Yi Wang wrote:
> To return the orientation property to drm/kms driver.
>
> Signed-off-by: Hsin-Yi Wang
> Reviewed-by: Hans de Goede
> Reviewed-by: Douglas Anderson
> ---
> drivers/gpu/drm/panel/panel-simple.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 4a2e580a2f7b..f232b8cf4075 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -411,7 +411,12 @@ static int panel_simple_get_modes(struct drm_panel
> *panel,
> /* add hard-coded panel modes */
> num += panel_simple_get_non_edid_modes(p, connector);
>
> - /* set up connector's "panel orientation" property */
> + /*
> + * drm drivers are expected to call drm_panel_get_orientation() to get
> + * panel's orientation then drm_connector_set_panel_orientation() to
> + * set the property before drm_dev_register(). Otherwise there will be
> + * a WARN_ON if orientation is set after drm is registered.
> + */
This comment is not really relevant here. If we need to explain this
then put it in drm_panel.c/h - as this applies for all panels and not
just the panel_simple.
Keep in mind, this is the source new panels often use a inspiration and
no need to have this copied around.
> drm_connector_set_panel_orientation(connector, p->orientation);
>
> return num;
> @@ -434,6 +439,14 @@ static int panel_simple_get_timings(struct drm_panel
> *panel,
> return p->desc->num_timings;
> }
>
> +static enum drm_panel_orientation panel_simple_get_orientation(struct
> drm_panel *panel)
> +{
> + struct panel_simple *p = to_panel_simple(panel);
> +
> + return p->orientation;
> +}
> +
> +
> static const struct drm_panel_funcs panel_simple_funcs = {
> .disable = panel_simple_disable,
> .unprepare = panel_simple_unprepare,
> @@ -441,6 +454,7 @@ static const struct drm_panel_funcs panel_simple_funcs = {
> .enable = panel_simple_enable,
> .get_modes = panel_simple_get_modes,
> .get_timings = panel_simple_get_timings,
> + .get_orientation = panel_simple_get_orientation,
I like the order in this list to match the order in the .h file.
So my OCD would like you to move it up right after get_modes,
but feel free to ignore this.
With the suggested changes:
Reviewed-by: Sam Ravnborg
> };
>
> static struct panel_desc panel_dpi;
> --
> 2.36.1.255.ge46751e96f-goog
Re: [PATCH v4 1/8] drm/panel: Add an API drm_panel_get_orientation() to return panel orientation
Hi Hsin-Yi,
On Mon, Jun 06, 2022 at 11:24:24PM +0800, Hsin-Yi Wang wrote:
> Panels usually call drm_connector_set_panel_orientation(), which is
> later than drm/kms driver calling drm_dev_register(). This leads to a
> WARN().
>
> The orientation property is known earlier. For example, some panels
> parse the property through device tree during probe.
>
> Add an API to return the property from panel to drm/kms driver, so the
> drivers are able to call drm_connector_set_panel_orientation() before
> drm_dev_register().
>
> Suggested-by: Hans de Goede
> Signed-off-by: Hsin-Yi Wang
> Reviewed-by: Hans de Goede
> Reviewed-by: Douglas Anderson
> ---
> v3->v4: Add a blank line.
> ---
> drivers/gpu/drm/drm_panel.c | 9 +
> include/drm/drm_panel.h | 10 ++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f634371c717a..e12056cfeca8 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -223,6 +223,15 @@ int drm_panel_get_modes(struct drm_panel *panel,
> }
> EXPORT_SYMBOL(drm_panel_get_modes);
>
> +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel *panel)
const as mentioned by Stephen.
> +{
> + if (panel && panel->funcs && panel->funcs->get_orientation)
> + return panel->funcs->get_orientation(panel);
> +
> + return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +}
> +EXPORT_SYMBOL(drm_panel_get_orientation);
> +
> #ifdef CONFIG_OF
> /**
> * of_drm_find_panel - look up a panel using a device tree node
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index d279ee455f01..5dadbf3b0370 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -133,6 +133,15 @@ struct drm_panel_funcs {
>* Allows panels to create panels-specific debugfs files.
>*/
> void (*debugfs_init)(struct drm_panel *panel, struct dentry *root);
> +
> + /**
> + * @get_orientation:
> + *
> + * Return the panel orientation set by device tree or EDID.
> + *
> + * This function is optional.
> + */
> + enum drm_panel_orientation (*get_orientation)(struct drm_panel *panel);
Please move this up so it is together with the other get_* methods, in
alphabetic order. That is, right after get_modes(), and then this also
matches the order in the .c file with is extra bonus.
With the two fixes:
Reviewed-by: Sam Ravnborg
> };
>
> /**
> @@ -202,6 +211,7 @@ int drm_panel_enable(struct drm_panel *panel);
> int drm_panel_disable(struct drm_panel *panel);
>
> int drm_panel_get_modes(struct drm_panel *panel, struct drm_connector
> *connector);
> +enum drm_panel_orientation drm_panel_get_orientation(struct drm_panel
> *panel);
>
> #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL)
> struct drm_panel *of_drm_find_panel(const struct device_node *np);
> --
> 2.36.1.255.ge46751e96f-goog
Re: [PATCH] dt-bindings: Fix properties without any type
On Thu, May 19, 2022 at 04:14:11PM -0500, Rob Herring wrote: > Now that the schema tools can extract type information for all > properties (in order to decode dtb files), finding properties missing > any type definition is fairly trivial though not yet automated. > > Fix the various property schemas which are missing a type. Most of these > tend to be device specific properties which don't have a vendor prefix. > A vendor prefix is how we normally ensure a type is defined. > > Signed-off-by: Rob Herring Acked-by: Sam Ravnborg # for everything in .../bindings/display/
Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem
Hi Venkateshwar, On Thu, May 12, 2022 at 07:23:13PM +0530, Venkateshwar Rao Gannavarapu wrote: > The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video > data from AXI-4 stream interface. > > It supports upto 4 lanes, optional register interface for the DPHY > and multiple RGB color formats. > This is a MIPI-DSI host driver and provides DSI bus for panels. > This driver also helps to communicate with its panel using panel > framework. Thanks for submitting this driver. I have added a few comments in the following that I hope you will find useful to improve the driver. Sam > > Signed-off-by: Venkateshwar Rao Gannavarapu > > --- > drivers/gpu/drm/xlnx/Kconfig| 14 ++ > drivers/gpu/drm/xlnx/Makefile | 1 + > drivers/gpu/drm/xlnx/xlnx_dsi.c | 456 > > 3 files changed, 471 insertions(+) > create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c > > diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig > index c3d0826..caa632b 100644 > --- a/drivers/gpu/drm/xlnx/Kconfig > +++ b/drivers/gpu/drm/xlnx/Kconfig > @@ -14,3 +14,17 @@ config DRM_ZYNQMP_DPSUB > This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose > this option if you have a Xilinx ZynqMP SoC with DisplayPort > subsystem. It would be nice to have it in some sort of alphabetic order, both in the Kconfig file and also the Makefile. > + > +config DRM_XLNX_DSI > + tristate "Xilinx DRM DSI Subsystem Driver" > + depends on DRM && OF > + select DRM_KMS_HELPER > + select DRM_MIPI_DSI > + select DRM_PANEL > + select BACKLIGHT_LCD_SUPPORT The BACKLIGHT_LCD_SUPPORT symbol is not relevant and can be dropped. > + select BACKLIGHT_CLASS_DEVICE > + select DRM_PANEL_SIMPLE The symbol DRM_PANEL_SIMPLE is used to enable the panel_simple driver and should not be selected here. > + help > + DRM bridge driver for Xilinx programmable DSI subsystem controller. > + choose this option if you hava a Xilinx MIPI-DSI Tx subsytem in > + video pipeline. > diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile > index 51c24b7..1e97fbe 100644 > --- a/drivers/gpu/drm/xlnx/Makefile > +++ b/drivers/gpu/drm/xlnx/Makefile > @@ -1,2 +1,3 @@ > zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o > obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o > +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c > new file mode 100644 > index 000..a5291f3 > --- /dev/null > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c > @@ -0,0 +1,456 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * > + * Xilinx FPGA MIPI DSI Tx Controller driver. > + * > + * Copyright (C) 2022 Xilinx, Inc. > + * > + * Author: Venkateshwar Rao G I noticed this is not the mail used in the s-o-b line. > + * > + */ > + > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* DSI Tx IP registers */ > +#define XDSI_CCR 0x00 > +#define XDSI_CCR_COREENB BIT(0) > +#define XDSI_CCR_SOFTRST BIT(1) > +#define XDSI_CCR_CRREADY BIT(2) > +#define XDSI_CCR_CMDMODE BIT(3) > +#define XDSI_CCR_DFIFORSTBIT(4) > +#define XDSI_CCR_CMDFIFORST BIT(5) > +#define XDSI_PCR 0x04 > +#define XDSI_PCR_LANES_MASK 3 > +#define XDSI_PCR_VIDEOMODE(x)(((x) & 0x3) << 3) > +#define XDSI_PCR_VIDEOMODE_MASK (0x3 << 3) GENMASK()? Same for other XXX_MASK definitions below > +#define XDSI_PCR_VIDEOMODE_SHIFT 3 > +#define XDSI_PCR_BLLPTYPE(x) ((x) << 5) > +#define XDSI_PCR_BLLPMODE(x) ((x) << 6) > +#define XDSI_PCR_PIXELFORMAT_MASK(0x3 << 11) > +#define XDSI_PCR_PIXELFORMAT_SHIFT 11 > +#define XDSI_PCR_EOTPENABLE(x) ((x) << 13) > +#define XDSI_GIER0x20 > +#define XDSI_ISR 0x24 > +#define XDSI_IER 0x28 > +#define XDSI_STR 0x2C > +#define XDSI_STR_RDY_SHPKT BIT(6) > +#define XDSI_STR_RDY_LNGPKT BIT(7) > +#define XDSI_STR_DFIFO_FULL BIT(8) > +#define XDSI_STR_DFIFO_EMPTY BIT(9) > +#define XDSI_STR_WAITFR_DATA BIT(10) > +#define XDSI_STR_CMD_EXE_PGS BIT(11) > +#define XDSI_STR_CCMD_PROC BIT(12) > +#define XDSI_STR_LPKT_MASK (0x5 << 7) > +#define XDSI_CMD 0x30 > +#define XDSI_CMD_QUEUE_PACKET(x) ((x) & GENMASK(23, 0)) > +#define XDSI_DFR 0x34 > +#define XDSI_TIME1 0x50 > +#define XDSI_TIME1_BLLP_BURST(x) ((x) & GENMASK(15, 0)) > +#define XDSI_TIME1_HSA(x)(((x) & GENMASK(15, 0)) << 16) > +#define XDSI_TIME2 0x54 > +#define XDSI_TIME2_VACT(x) ((x) & GENMASK(15, 0)) > +#define
Re: [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
On Wed, May 11, 2022 at 07:34:38PM +0200, Javier Martinez Canillas wrote: > Hello Guenter, > > On 5/11/22 19:17, Guenter Roeck wrote: > > On 5/11/22 10:00, Sam Ravnborg wrote: > > [snip] > > >>> struct fb_info *registered_fb[FB_MAX] __read_mostly; > >>> -EXPORT_SYMBOL(registered_fb); > >>> - > >>> int num_registered_fb __read_mostly; > >>> +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) > >>> +EXPORT_SYMBOL(registered_fb); > >>> EXPORT_SYMBOL(num_registered_fb); > >>> +#endif > >> > >> It is stuff like this I refer to as "ugly" in the comment above. > >> > > > > My "solution" for that kind of thing is to use a namespace, > > such as > > > > EXPORT_SYMBOL_NS(registered_fb, FB_OLPC_DCON); > > EXPORT_SYMBOL_NS(num_registered_fb, FB_OLPC_DCON); > > > > Using a namespace in this case is indeed a great idea I think. > > I've used in the past to limit the export of a symbol for within a driver > that could be scattered across different compilations units, but it never > occurred to me using it to limit symbols exported by core code. > > > and import it from the offending code. That avoids ifdefs > > while at the same time limiting the use of the symbols > > to the expected scope. Of course that could be abused but > > that abuse would be obvious. > > > > Agreed. For the next revision, besides using an namespaced export symbol > as you suggested, I'll include a comment to make clear that it shouldn't > by any other driver and FB_OLPC_DCON fixed instead. A very nice compromise, thanks Guenter and Javier. Sam
Re: [PATCH v2 3/3] drm/panel: introduce ebbg,ft8719 panel
Hi Joel, On Wed, May 11, 2022 at 10:58:11AM +0530, Joel Selvaraj wrote: > Add DRM panel driver for EBBG FT8719 6.18" 2246x1080 DSI video mode > panel, which can be found on some Xiaomi Poco F1 phones. The panel's > backlight is managed through QCOM WLED driver. > > Signed-off-by: Joel Selvaraj Driver looks good: Reviewed-by: Sam Ravnborg I expect someone else to pick it up and apply. Sam
Re: [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
Hi Javier. On Wed, May 11, 2022 at 01:32:30PM +0200, Javier Martinez Canillas wrote: > From: Daniel Vetter > > Well except when the olpc dcon fbdev driver is enabled, that thing > digs around in there in rather unfixable ways. > > Cc oldc_dcon maintainers as fyi. Another way to fix this is to mark FB_OLPC_DCON and add a TODO entry to fix this. We are really not supposed to carry such special code around to keep staging working. I know this may not be a popular viewpoint, but just look at the ugly workarounds required here. Sam > > v2: I typoed the config name (0day) > > Cc: kernel test robot > Cc: Jens Frederich > Cc: Jon Nettleton > Cc: Greg Kroah-Hartman > Cc: linux-stag...@lists.linux.dev > Signed-off-by: Daniel Vetter > Signed-off-by: Daniel Vetter > Reviewed-by: Javier Martinez Canillas > Cc: Daniel Vetter > Cc: Helge Deller > Cc: Matthew Wilcox > Cc: Sam Ravnborg > Cc: Tetsuo Handa > Cc: Zhen Lei > Cc: Alex Deucher > Cc: Xiyu Yang > Cc: linux-fb...@vger.kernel.org > Cc: Zheyu Ma > Cc: Guenter Roeck > Signed-off-by: Javier Martinez Canillas > --- > > (no changes since v1) > > drivers/video/fbdev/core/fbmem.c | 8 ++-- > include/linux/fb.h | 7 +++ > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index 265efa189bcc..6cab5f4c1fb3 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -50,10 +50,14 @@ > static DEFINE_MUTEX(registration_lock); > > struct fb_info *registered_fb[FB_MAX] __read_mostly; > -EXPORT_SYMBOL(registered_fb); > - > int num_registered_fb __read_mostly; > +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) > +EXPORT_SYMBOL(registered_fb); > EXPORT_SYMBOL(num_registered_fb); > +#endif It is stuff like this I refer to as "ugly" in the comment above. Sam
Re: [PATCH v2] fbdev: Use helper to get fb_info in all file operations
On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote: > A reference to the framebuffer device struct fb_info is stored in the file > private data, but this reference could no longer be valid and must not be > accessed directly. Instead, the file_fb_info() accessor function must be > used since it does sanity checking to make sure that the fb_info is valid. > > This can happen for example if the registered framebuffer device is for a > driver that just uses a framebuffer provided by the system firmware. In > that case, the fbdev core would unregister the framebuffer device when a > real video driver is probed and ask to remove conflicting framebuffers. > > Most fbdev file operations already use the helper to get the fb_info but > get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two. > > Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be > exported. Rename it and add a fb_ prefix to denote that is public now. > > Reported-by: Junxiao Chang > Signed-off-by: Javier Martinez Canillas Acked-by: Sam Ravnborg
Re: [PATCH] fbdev: Use helper to get fb_info in all file operations
Hi Javier,
On Tue, May 03, 2022 at 06:46:16PM +0200, Javier Martinez Canillas wrote:
> A reference to the framebuffer device struct fb_info is stored in the file
> private data, but this reference could no longer be valid and must not be
> accessed directly. Instead, the file_fb_info() accessor function must be
> used since it does sanity checking to make sure that the fb_info is valid.
>
> This can happen for example if the registered framebuffer device is for a
> driver that just uses a framebuffer provided by the system firmware. In
> that case, the fbdev core would unregister the framebuffer device when a
> real video driver is probed and ask to remove conflicting framebuffers.
>
> Most fbdev file operations already use the helper to get the fb_info but
> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
>
> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
> exported. Rename it and add a fb_ prefix to denote that is public now.
>
> Reported-by: Junxiao Chang
> Signed-off-by: Javier Martinez Canillas
> ---
>
> drivers/video/fbdev/core/fb_defio.c | 5 -
> drivers/video/fbdev/core/fbmem.c| 24 +++-
> include/linux/fb.h | 1 +
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c
> b/drivers/video/fbdev/core/fb_defio.c
> index 842c66b3e33d..ac1c88b3fbb5 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault
> *vmf)
>
> int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int
> datasync)
> {
> - struct fb_info *info = file->private_data;
> + struct fb_info *info = fb_file_fb_info(file->private_data);
This looks wrong. I assume you wanted to write:
> + struct fb_info *info = fb_file_fb_info(file);
Sam
Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct
Hi Thomas,
> > > +
> > > /* this is to find and return the vmalloc-ed fb pages */
> > > static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> > > {
> > > @@ -59,7 +113,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault
> > > *vmf)
> > > printk(KERN_ERR "no mapping available\n");
> > > BUG_ON(!page->mapping);
> > > - page->index = vmf->pgoff;
> > > + page->index = vmf->pgoff; /* for page_mkclean() */
> > > vmf->page = page;
> > > return 0;
> > > @@ -95,7 +149,11 @@ static vm_fault_t fb_deferred_io_mkwrite(struct
> > > vm_fault *vmf)
> > > struct page *page = vmf->page;
> > > struct fb_info *info = vmf->vma->vm_private_data;
> > > struct fb_deferred_io *fbdefio = info->fbdefio;
> > > - struct list_head *pos = >pagelist;
> > > + struct fb_deferred_io_pageref *pageref;
> > > + unsigned long offset;
> > > + vm_fault_t ret;
> > > +
> > > + offset = (vmf->address - vmf->vma->vm_start);
> > > /* this is a callback we get when userspace first tries to
> > > write to the page. we schedule a workqueue. that workqueue
> > > @@ -112,6 +170,12 @@ static vm_fault_t fb_deferred_io_mkwrite(struct
> > > vm_fault *vmf)
> > > if (fbdefio->first_io && list_empty(>pagelist))
> > > fbdefio->first_io(info);
> > > + pageref = fb_deferred_io_pageref_get(info, offset, page);
> > Compared to the old code we now do all the sorting and stuff without
> > the page locked, which seem like a big change.
>
> We never touch any of the page's fields in fb_deferred_io_pageref_get().
> It's only used to initialize the pageref's page pointer. The pagerefs are
> all protected by fbdev-internal locking. Is there a reason why we should
> further hold the page lock?
I only commented because it was a change in scope of the lock, I did not
see anything wrong in the locking, but then I do not understand locking
so that does not say much.
>
> All sorting is done by the pageref addresses, which implicitly correspond to
> 'offset'. After looking at the new function again, I'll change it to sort
> directly by offset. It's clearer in its intend.
Looks forward for the re-spin.
Sam
Re: [PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite
Hi Thomas.
On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
> Refactor the page-write handler for deferred I/O. Drivers use the
> function to let fbdev track written pages of mmap'ed framebuffer
> memory.
I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).
But if you consider it better keep it. Again just my thoughts when
reading the code.
Sam
>
> v2:
> * don't export the helper until we have an external caller
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Javier Martinez Canillas
> ---
> drivers/video/fbdev/core/fb_defio.c | 68 -
> 1 file changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c
> b/drivers/video/fbdev/core/fb_defio.c
> index a03b9c64fc61..214581ce5840 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t
> start, loff_t end, int datasy
> }
> EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>
> -/* vm_ops->page_mkwrite handler */
> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +/*
> + * Adds a page to the dirty list. Requires caller to hold
> + * struct fb_deferred_io.lock. Call this from struct
> + * vm_operations_struct.page_mkwrite.
> + */
> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned
> long offset,
> + struct page *page)
> {
> - struct page *page = vmf->page;
> - struct fb_info *info = vmf->vma->vm_private_data;
> struct fb_deferred_io *fbdefio = info->fbdefio;
> struct fb_deferred_io_pageref *pageref;
> - unsigned long offset;
> vm_fault_t ret;
>
> - offset = (vmf->address - vmf->vma->vm_start);
> -
> - /* this is a callback we get when userspace first tries to
> - write to the page. we schedule a workqueue. that workqueue
> - will eventually mkclean the touched pages and execute the
> - deferred framebuffer IO. then if userspace touches a page
> - again, we repeat the same scheme */
> -
> - file_update_time(vmf->vma->vm_file);
> -
> - /* protect against the workqueue changing the page list */
> - mutex_lock(>lock);
> -
> /* first write in this cycle, notify the driver */
> if (fbdefio->first_io && list_empty(>pagelist))
> fbdefio->first_io(info);
> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
> *vmf)
>*/
> lock_page(pageref->page);
>
> - mutex_unlock(>lock);
> -
> /* come back after delay to process the deferred IO */
> schedule_delayed_work(>deferred_work, fbdefio->delay);
> return VM_FAULT_LOCKED;
> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault
> *vmf)
> return ret;
> }
>
> +/*
> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
> + * @fb_info: The fbdev info structure
> + * @vmf: The VM fault
> + *
> + * This is a callback we get when userspace first tries to
> + * write to the page. We schedule a workqueue. That workqueue
> + * will eventually mkclean the touched pages and execute the
> + * deferred framebuffer IO. Then if userspace touches a page
> + * again, we repeat the same scheme.
> + *
> + * Returns:
> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
> + */
> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct
> vm_fault *vmf)
> +{
> + struct page *page = vmf->page;
> + struct fb_deferred_io *fbdefio = info->fbdefio;
> + unsigned long offset;
> + vm_fault_t ret;
> +
> + offset = (vmf->address - vmf->vma->vm_start);
> +
> + file_update_time(vmf->vma->vm_file);
> +
> + /* protect against the workqueue changing the page list */
> + mutex_lock(>lock);
> + ret = __fb_deferred_io_track_page(info, offset, page);
> + mutex_unlock(>lock);
> +
> + return ret;
> +}
> +
> +/* vm_ops->page_mkwrite handler */
> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +{
> + struct fb_info *info = vmf->vma->vm_private_data;
> +
> + return fb_deferred_io_page_mkwrite(info, vmf);
> +}
> +
> static const struct vm_operations_struct fb_deferred_io_vm_ops = {
> .fault = fb_deferred_io_fault,
> .page_mkwrite = fb_deferred_io_mkwrite,
> --
> 2.36.0
Re: [PATCH v2 2/3] fbdev: Track deferred-I/O pages in pageref struct
Hi Thomas,
a little ramblings below. Just my thoughts while trying to understand
the code - especially since I looked at it before.
Sam
On Mon, Apr 25, 2022 at 01:27:50PM +0200, Thomas Zimmermann wrote:
> Store the per-page state for fbdev's deferred I/O in struct
> fb_deferred_io_pageref. Maintain a list of pagerefs for the pages
> that have to be written back to video memory. Update all affected
> drivers.
>
> As with pages before, fbdev acquires a pageref when an mmaped page
> of the framebuffer is being written to. It holds the pageref in a
> list of all currently written pagerefs until it flushes the written
> pages to video memory. Writeback occurs periodically. After writeback
> fbdev releases all pagerefs and builds up a new dirty list until the
> next writeback occurs.
>
> Using pagerefs has a number of benefits.
>
> For pages of the framebuffer, the deferred I/O code used struct
> page.lru as an entry into the list of dirty pages. The lru field is
> owned by the page cache, which makes deferred I/O incompatible with
> some memory pages (e.g., most notably DRM's GEM SHMEM allocator).
> struct fb_deferred_io_pageref now provides an entry into a list of
> dirty framebuffer pages, freeing lru for use with the page cache.
>
> Drivers also assumed that struct page.index is the page offset into
> the framebuffer. This is not true for DRM buffers, which are located
> at various offset within a mapped area. struct fb_deferred_io_pageref
> explicitly stores an offset into the framebuffer. struct page.index
> is now only the page offset into the mapped area.
>
> These changes will allow DRM to use fbdev deferred I/O without an
> intermediate shadow buffer.
>
> v2:
> * minor fixes in commit message
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Javier Martinez Canillas
> ---
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> index 02a4a4fa3da3..db02e17e12b6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
> @@ -322,12 +322,13 @@ static void vmw_deferred_io(struct fb_info *info,
> struct vmw_fb_par *par = info->par;
> unsigned long start, end, min, max;
> unsigned long flags;
> - struct page *page;
> + struct fb_deferred_io_pageref *pageref;
> int y1, y2;
>
> min = ULONG_MAX;
> max = 0;
> - list_for_each_entry(page, pagelist, lru) {
> + list_for_each_entry(pageref, pagelist, list) {
> + struct page *page = pageref->page;
> start = page->index << PAGE_SHIFT;
> end = start + PAGE_SIZE - 1;
> min = min(min, start);
This is the same change in all deferred_io drivers like above.
We now traverse pageref->list where pagelist points to the head.
It took me a while to understand that pagelist points to a list of
fb_deferred_io_pageref and not a list of pages.
I had been helped, had this been renamed to s/pagelist/pagereflist/.
If you see no point in this then just ignore my comment, I have not
stared at kernel code for a while and is thus easy to confuse.
> diff --git a/drivers/video/fbdev/core/fb_defio.c
> b/drivers/video/fbdev/core/fb_defio.c
> index 6924d489a289..a03b9c64fc61 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -36,6 +36,60 @@ static struct page *fb_deferred_io_page(struct fb_info
> *info, unsigned long offs
> return page;
> }
>
> +static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct
> fb_info *info,
> + unsigned long
> offset,
> + struct page
> *page)
> +{
> + struct fb_deferred_io *fbdefio = info->fbdefio;
> + struct list_head *pos = >pagelist;
> + unsigned long pgoff = offset >> PAGE_SHIFT;
> + struct fb_deferred_io_pageref *pageref, *cur;
> +
> + if (WARN_ON_ONCE(pgoff >= info->npagerefs))
> + return NULL; /* incorrect allocation size */
> +
> + /* 1:1 mapping between pageref and page offset */
> + pageref = >pagerefs[pgoff];
> +
> + /*
> + * This check is to catch the case where a new process could start
> + * writing to the same page through a new PTE. This new access
> + * can cause a call to .page_mkwrite even if the original process'
> + * PTE is marked writable.
> + */
> + if (!list_empty(>list))
> + goto pageref_already_added;
> +
> + pageref->page = page;
> + pageref->offset = pgoff << PAGE_SHIFT;
> +
> + if (unlikely(fbdefio->sort_pagelist)) {
> + /*
> + * We loop through the list of pagerefs before adding, in
> + * order to keep the pagerefs sorted. This has significant
> + * overhead of O(n^2) with n being the number of written
> + * pages. If possible, drivers should try to work with
> +
Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers
Hi Thomas,
On Mon, Apr 25, 2022 at 07:26:32PM +0200, Sam Ravnborg wrote:
> Hi Thomas,
>
> > diff --git a/drivers/video/fbdev/core/fb_defio.c
> > b/drivers/video/fbdev/core/fb_defio.c
> > index 6aaf6d0abf39..6924d489a289 100644
> > --- a/drivers/video/fbdev/core/fb_defio.c
> > +++ b/drivers/video/fbdev/core/fb_defio.c
> > @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct
> > vm_area_struct *vma)
> > vma->vm_private_data = info;
> > return 0;
> > }
> > +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
> >
> > /* workqueue callback */
> > static void fb_deferred_io_work(struct work_struct *work)
> > diff --git a/drivers/video/fbdev/core/fbmem.c
> > b/drivers/video/fbdev/core/fbmem.c
> > index 84427470367b..52440e3f8f69 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1334,7 +1334,6 @@ static int
> > fb_mmap(struct file *file, struct vm_area_struct * vma)
> > {
> > struct fb_info *info = file_fb_info(file);
> > - int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> > unsigned long mmio_pgoff;
> > unsigned long start;
> > u32 len;
> > @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct *
> > vma)
> > return -ENODEV;
> > mutex_lock(>mm_lock);
> >
> > - fb_mmap_fn = info->fbops->fb_mmap;
> > -
> > -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> > - if (info->fbdefio)
> > - fb_mmap_fn = fb_deferred_io_mmap;
> > -#endif
> > -
> > - if (fb_mmap_fn) {
> > + if (info->fbops->fb_mmap) {
> > int res;
> >
> > /*
> > @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct *
> > vma)
> > * SME protection is removed ahead of the call
> > */
> > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > - res = fb_mmap_fn(info, vma);
> > + res = info->fbops->fb_mmap(info, vma);
> > mutex_unlock(>mm_lock);
> > return res;
> > }
> >
> > + /*
> > +* FB deferred I/O wants you to handle mmap in your drivers. At a
> > +* minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> > +*/
> > + if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for
> > defered I/O.\n"))
> > + return -ENODEV;
> > +
>
> If not configured - then why not just call fb_deferred_io_mmap(), as
> this seems to be the default implementation anyway.
> Then drivers that needs it can override - and the rest fallback to the
> default.
Just to be clear - I already read:
"
Leave the mmap handling to drivers and expect them to call the
helper for deferred I/O by thmeselves.
"
But this does not help me understand why we need to explicit do what
could be a simple default implementation.
Chances are that I am stupid and it is obvious..
Sam
Re: [PATCH v2 1/3] fbdev: Put mmap for deferred I/O into drivers
Hi Thomas,
> diff --git a/drivers/video/fbdev/core/fb_defio.c
> b/drivers/video/fbdev/core/fb_defio.c
> index 6aaf6d0abf39..6924d489a289 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -181,6 +181,7 @@ int fb_deferred_io_mmap(struct fb_info *info, struct
> vm_area_struct *vma)
> vma->vm_private_data = info;
> return 0;
> }
> +EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
>
> /* workqueue callback */
> static void fb_deferred_io_work(struct work_struct *work)
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 84427470367b..52440e3f8f69 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1334,7 +1334,6 @@ static int
> fb_mmap(struct file *file, struct vm_area_struct * vma)
> {
> struct fb_info *info = file_fb_info(file);
> - int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma);
> unsigned long mmio_pgoff;
> unsigned long start;
> u32 len;
> @@ -1343,14 +1342,7 @@ fb_mmap(struct file *file, struct vm_area_struct * vma)
> return -ENODEV;
> mutex_lock(>mm_lock);
>
> - fb_mmap_fn = info->fbops->fb_mmap;
> -
> -#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
> - if (info->fbdefio)
> - fb_mmap_fn = fb_deferred_io_mmap;
> -#endif
> -
> - if (fb_mmap_fn) {
> + if (info->fbops->fb_mmap) {
> int res;
>
> /*
> @@ -1358,11 +1350,18 @@ fb_mmap(struct file *file, struct vm_area_struct *
> vma)
>* SME protection is removed ahead of the call
>*/
> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> - res = fb_mmap_fn(info, vma);
> + res = info->fbops->fb_mmap(info, vma);
> mutex_unlock(>mm_lock);
> return res;
> }
>
> + /*
> + * FB deferred I/O wants you to handle mmap in your drivers. At a
> + * minimum, point struct fb_ops.fb_mmap to fb_deferred_io_mmap().
> + */
> + if (dev_WARN_ONCE(info->dev, info->fbdefio, "fbdev mmap not set up for
> defered I/O.\n"))
> + return -ENODEV;
> +
If not configured - then why not just call fb_deferred_io_mmap(), as
this seems to be the default implementation anyway.
Then drivers that needs it can override - and the rest fallback to the
default.
Sam
Re: [PATCH] drm/panel: simple: Add missing bus flags for Innolux G070Y2-L01
On Wed, Apr 06, 2022 at 11:36:27AM +0200, Marek Vasut wrote:
> The DE signal is active high on this display, fill in the missing bus_flags.
> This aligns panel_desc with its display_timing .
>
> Fixes: a5d2ade627dca ("drm/panel: simple: Add support for Innolux G070Y2-L01")
> Signed-off-by: Marek Vasut
> Cc: Christoph Fritz
> Cc: Laurent Pinchart
> Cc: Maxime Ripard
> Cc: Sam Ravnborg
> Cc: Thomas Zimmermann
Acked-by: Sam Ravnborg
> ---
> drivers/gpu/drm/panel/panel-simple.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 65c2a3000e471..18fccb0d47382 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -2149,6 +2149,7 @@ static const struct panel_desc innolux_g070y2_l01 = {
> .unprepare = 800,
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> .connector_type = DRM_MODE_CONNECTOR_LVDS,
> };
>
> --
> 2.35.1
Re: [PATCH] fbcon: change fbcon_*registered_fb variables to static
On Sat, Apr 23, 2022 at 08:56:18AM -0400, Tom Rix wrote:
> Sparse reports these issues
> fbcon.c:106:16: warning: symbol 'fbcon_registered_fb' was not declared.
> Should it be static?
> fbcon.c:107:5: warning: symbol 'fbcon_num_registered_fb' was not declared.
> Should it be static?
>
> These variables are only used in fbcon.c. Single file use variables should
> be static, so change their storage-class specifiers to static.
>
> Signed-off-by: Tom Rix
Acked-by: Sam Ravnborg
> ---
> drivers/video/fbdev/core/fbcon.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c
> b/drivers/video/fbdev/core/fbcon.c
> index c4e91715ef00..225ac0fe0143 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -103,8 +103,8 @@ enum {
>
> static struct fbcon_display fb_display[MAX_NR_CONSOLES];
>
> -struct fb_info *fbcon_registered_fb[FB_MAX];
> -int fbcon_num_registered_fb;
> +static struct fb_info *fbcon_registered_fb[FB_MAX];
> +static int fbcon_num_registered_fb;
>
> #define fbcon_for_each_registered_fb(i) \
> for (i = 0; WARN_CONSOLE_UNLOCKED(), i < FB_MAX; i++) \
> --
> 2.27.0
Re: [PATCH v3 1/4] drm: mxsfb: Wrap FIFO reset and comments into mxsfb_reset_block()
On Sun, Apr 17, 2022 at 04:07:57AM +0200, Marek Vasut wrote: > Wrap FIFO reset and comments into mxsfb_reset_block(), this is a clean up. > No functional change. > > Reviewed-by: Lucas Stach > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner Acked-by: Sam Ravnborg
Re: [PATCH v2 2/2] drm/panel: simple: Add Startek KD070WVFPA043-C069A panel support
On Fri, Apr 22, 2022 at 03:36:14PM -0300, Fabio Estevam wrote: > From: Heiko Schocher > > Add Startek KD070WVFPA043-C069A 7" TFT LCD panel support. > > Signed-off-by: Heiko Schocher > Signed-off-by: Fabio Estevam Acked-by: Sam Ravnborg
Re: [PATCH v3 4/4] drm: mxsfb: Reorder mxsfb_crtc_mode_set_nofb()
On Sun, Apr 17, 2022 at 04:08:00AM +0200, Marek Vasut wrote: > Reorder mxsfb_crtc_mode_set_nofb() such that all functions which perform > register IO are called from one single location in this function. This is > a clean up. No functional change. > > Reviewed-by: Lucas Stach > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner Acked-by: Sam Ravnborg
Re: [PATCH v3 3/4] drm: mxsfb: Factor out mxsfb_set_mode()
On Sun, Apr 17, 2022 at 04:07:59AM +0200, Marek Vasut wrote: > Pull mode registers programming from mxsfb_enable_controller() into > dedicated function mxsfb_set_mode(). This is a clean up. No functional > change. > > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner Acked-by: Sam Ravnborg
Re: [PATCH v3 2/4] drm: mxsfb: Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr()
On Sun, Apr 17, 2022 at 04:07:58AM +0200, Marek Vasut wrote: > Replace mxsfb_get_fb_paddr() with drm_fb_cma_get_gem_addr() to correctly > handle > FB offset. > > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner Acked-by: Sam Ravnborg
Re: [PATCH v2 2/2] drm: bridge: ldb: Implement simple NXP i.MX8M LDB bridge
Hi Marek, On Mon, Apr 18, 2022 at 04:51:05PM +0200, Marek Vasut wrote: > The i.MX8MP contains two syscon registers which are responsible > for configuring the on-SoC DPI-to-LVDS serializer. Implement a > simple bridge driver for this serializer. > > Signed-off-by: Marek Vasut > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Maxime Ripard > Cc: Peng Fan > Cc: Robby Cai > Cc: Robert Foss > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > To: dri-devel@lists.freedesktop.org Good to see a bridge driver that stays away from the deprecated stuff, and it looks nice a clean. I like how disable is the exact opposite of enable. A few comments in the following - with the Kconfig text updated: Reviewed-by: Sam Ravnborg Sam > -- > V2: - Rename syscon to fsl,syscon > --- > drivers/gpu/drm/bridge/Kconfig | 8 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/nxp-ldb.c | 343 +++ > 3 files changed, 352 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/nxp-ldb.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 20f9bc7f4be54..7fe7088a2bef5 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -185,6 +185,14 @@ config DRM_NWL_MIPI_DSI > This enables the Northwest Logic MIPI DSI Host controller as > for example found on NXP's i.MX8 Processors. > > +config DRM_NXP_LDB > + tristate "NXP i.MX8M LDB bridge" > + depends on OF > + select DRM_KMS_HELPER > + select DRM_PANEL_BRIDGE > + help > + Support for i.MX8M DPI-to-LVDS on-SoC encoder. As commented in the bindings patch - is this i.MX8M or i.MX8MP? > + > config DRM_NXP_PTN3460 > tristate "NXP PTN3460 DP/LVDS bridge" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index bdffad2a7ed3a..f800b2331d9e0 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o > obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o > obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += > megachips-stdp-ge-b850v3-fw.o > +obj-$(CONFIG_DRM_NXP_LDB) += nxp-ldb.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o > diff --git a/drivers/gpu/drm/bridge/nxp-ldb.c > b/drivers/gpu/drm/bridge/nxp-ldb.c > new file mode 100644 > index 0..7b8de235876ea > --- /dev/null > +++ b/drivers/gpu/drm/bridge/nxp-ldb.c > @@ -0,0 +1,343 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2022 Marek Vasut > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#define LDB_CTRL 0x5c > +#define LDB_CTRL_CH0_ENABLE BIT(0) > +#define LDB_CTRL_CH0_DI_SELECT BIT(1) > +#define LDB_CTRL_CH1_ENABLE BIT(2) > +#define LDB_CTRL_CH1_DI_SELECT BIT(3) > +#define LDB_CTRL_SPLIT_MODE BIT(4) > +#define LDB_CTRL_CH0_DATA_WIDTH BIT(5) > +#define LDB_CTRL_CH0_BIT_MAPPING BIT(6) > +#define LDB_CTRL_CH1_DATA_WIDTH BIT(7) > +#define LDB_CTRL_CH1_BIT_MAPPING BIT(8) > +#define LDB_CTRL_DI0_VSYNC_POLARITY BIT(9) > +#define LDB_CTRL_DI1_VSYNC_POLARITY BIT(10) > +#define LDB_CTRL_REG_CH0_FIFO_RESET BIT(11) > +#define LDB_CTRL_REG_CH1_FIFO_RESET BIT(12) > +#define LDB_CTRL_ASYNC_FIFO_ENABLE BIT(24) > +#define LDB_CTRL_ASYNC_FIFO_THRESHOLD_MASK GENMASK(27, 25) > + > +#define LVDS_CTRL0x128 > +#define LVDS_CTRL_CH0_EN BIT(0) > +#define LVDS_CTRL_CH1_EN BIT(1) > +#define LVDS_CTRL_VBG_EN BIT(2) > +#define LVDS_CTRL_HS_EN BIT(3) > +#define LVDS_CTRL_PRE_EMPH_ENBIT(4) > +#define LVDS_CTRL_PRE_EMPH_ADJ(n)(((n) & 0x7) << 5) > +#define LVDS_CTRL_PRE_EMPH_ADJ_MASK GENMASK(7, 5) > +#define LVDS_CTRL_CM_ADJ(n) (((n) & 0x7) << 8) > +#define LVDS_CTRL_CM_ADJ_MASKGENMASK(10, 8) > +#define LVDS_CTRL_CC_ADJ(n) (((n) & 0x7) << 11) > +#define LVDS_CTRL_CC_ADJ_MA
Re: [PATCH 1/2] dt-bindings: display: simple: Add Startek KD070WVFPA043-C069A panel
On Tue, Apr 19, 2022 at 09:56:24PM -0300, Fabio Estevam wrote: > From: Fabio Estevam > > Add Startek KD070WVFPA043-C069A 7" TFT LCD panel compatible string. > > Signed-off-by: Fabio Estevam Acked-by: Sam Ravnborg > --- > .../devicetree/bindings/display/panel/panel-simple.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > index 1eb9dd4f8f58..e190eef66872 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > @@ -294,6 +294,8 @@ properties: >- starry,kr070pe2t > # Starry 12.2" (1920x1200 pixels) TFT LCD panel >- starry,kr122ea0sra > +# Startek KD070WVFPA043-C069A 7" TFT LCD panel > + - startek,kd070wvfpa > # Team Source Display Technology TST043015CMHX 4.3" WQVGA TFT LCD > panel >- team-source-display,tst043015cmhx > # Tianma Micro-electronics TM070JDHG30 7.0" WXGA TFT LCD panel > -- > 2.25.1
Re: [PATCH v2 2/2] drm/panel: simple: Add DataImage FG040346DSSWBG04 panel support
On Fri, Apr 22, 2022 at 12:22:42PM +0200, Marek Vasut wrote: > Add DataImage FG040346DSSWBG04 4.3" 480x272 TFT LCD 24bit DPI panel > support. > > Acked-by: Thomas Zimmermann > Signed-off-by: Marek Vasut > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > To: dri-devel@lists.freedesktop.org Acked-by: Sam Ravnborg Sam
Re: [PATCH v2 1/2] dt-bindings: display: simple: Add DataImage FG040346DSSWBG04 compatible string
On Fri, Apr 22, 2022 at 12:22:41PM +0200, Marek Vasut wrote: > Add DataImage FG040346DSSWBG04 4.3" 480x272 TFT LCD 24bit DPI panel > compatible string. > > Acked-by: Thomas Zimmermann > Signed-off-by: Marek Vasut > Cc: Rob Herring > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Cc: devicet...@vger.kernel.org > To: dri-devel@lists.freedesktop.org Acked-by: Sam Ravnborg > --- > V2: Add AB from Thomas > --- > .../devicetree/bindings/display/panel/panel-simple.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > index 1eb9dd4f8f58..cfe7bb9f89de 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > @@ -105,6 +105,8 @@ properties: >- chunghwa,claa101wb01 > # Chunghwa Picture Tubes Ltd. 10.1" WXGA TFT LCD panel >- chunghwa,claa101wb03 > +# DataImage, Inc. 4.3" WQVGA (480x272) TFT LCD panel with 24-bit > parallel interface. > + - dataimage,fg040346dsswbg04 > # DataImage, Inc. 7" WVGA (800x480) TFT LCD panel with 24-bit > parallel interface. >- dataimage,scf0700c48ggu18 > # DLC Display Co. DLC1010GIG 10.1" WXGA TFT LCD Panel > -- > 2.35.1
Re: [PATCH v2 1/2] dt-bindings: display: bridge: ldb: Implement simple NXP i.MX8M LDB bridge
Hi Marek, I read the patch once more.
On Mon, Apr 18, 2022 at 04:51:04PM +0200, Marek Vasut wrote:
> The i.MX8MP contains two syscon registers which are responsible
Here it says i.MX8MP
> for configuring the on-SoC DPI-to-LVDS serializer. Add DT binding
> which represents this serializer as a bridge.
>
> Signed-off-by: Marek Vasut
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Maxime Ripard
> Cc: Peng Fan
> Cc: Rob Herring
> Cc: Robby Cai
> Cc: Robert Foss
> Cc: Sam Ravnborg
> Cc: Thomas Zimmermann
> Cc: devicet...@vger.kernel.org
> To: dri-devel@lists.freedesktop.org
> ---
> V2: - Consistently use fsl,imx8mp-ldb as compatible
> - Drop items: from compatible:
> - Replace minItems with maxItems in clocks:
> - Drop quotes from clock-names const: ldb
> - Rename syscon to fsl,syscon
> - Use generic name of ldb-lvds in example
> ---
> .../bindings/display/bridge/nxp,ldb.yaml | 96 +++
> 1 file changed, 96 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
> b/Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
> new file mode 100644
> index 0..f3182566eb316
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/nxp,ldb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX8M DPI to LVDS bridge chip
Here it says i.MX8M
> +
> +maintainers:
> + - Marek Vasut
> +
> +description: |
> + The i.MX8MP contains two syscon registers which are responsible
Here it says i.MX8MP
> + for configuring the on-SoC DPI-to-LVDS serializer. This describes
> + those registers as bridge within the DT.
> +
> +properties:
> + compatible:
> +const: fsl,imx8mp-ldb
Here it says fsl,imx8mp
It looks a little inconsistent, I guess the title needs a fix.
a-b stands with the title fixed.
Sam
> +
> + clocks:
> +maxItems: 1
> +
> + clock-names:
> +const: ldb
> +
> + fsl,syscon:
> +$ref: /schemas/types.yaml#/definitions/phandle
> +description: A phandle to media block controller.
> +
> + ports:
> +$ref: /schemas/graph.yaml#/properties/ports
> +
> +properties:
> + port@0:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for DPI input.
> +
> + port@1:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for LVDS Channel-A output (panel or bridge).
> +
> + port@2:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for LVDS Channel-B output (panel or bridge).
> +
> +required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - clocks
> + - fsl,syscon
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +
> +bridge {
> +compatible = "fsl,imx8mp-ldb";
> +clocks = < IMX8MP_CLK_MEDIA_LDB>;
> +clock-names = "ldb";
> +fsl,syscon = <_blk_ctrl>;
> +
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +port@0 {
> +reg = <0>;
> +
> +ldb_from_lcdif2: endpoint {
> +remote-endpoint = <_to_ldb>;
> +};
> +};
> +
> +port@1 {
> +reg = <1>;
> +
> +ldb_lvds_ch0: endpoint {
> +remote-endpoint = <_to_lvdsx4panel>;
> +};
> +};
> +
> +port@2 {
> +reg = <2>;
> +
> +ldb_lvds_ch1: endpoint {
> +};
> +};
> +};
> +};
> --
> 2.35.1
Re: [PATCH v2 1/2] dt-bindings: display: bridge: ldb: Implement simple NXP i.MX8M LDB bridge
On Mon, Apr 18, 2022 at 04:51:04PM +0200, Marek Vasut wrote:
> The i.MX8MP contains two syscon registers which are responsible
> for configuring the on-SoC DPI-to-LVDS serializer. Add DT binding
> which represents this serializer as a bridge.
>
> Signed-off-by: Marek Vasut
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Maxime Ripard
> Cc: Peng Fan
> Cc: Rob Herring
> Cc: Robby Cai
> Cc: Robert Foss
> Cc: Sam Ravnborg
> Cc: Thomas Zimmermann
> Cc: devicet...@vger.kernel.org
> To: dri-devel@lists.freedesktop.org
Acked-by: Sam Ravnborg but you need an ack from
someone else before you apply as I am not an expert here.
Sam
> ---
> V2: - Consistently use fsl,imx8mp-ldb as compatible
> - Drop items: from compatible:
> - Replace minItems with maxItems in clocks:
> - Drop quotes from clock-names const: ldb
> - Rename syscon to fsl,syscon
> - Use generic name of ldb-lvds in example
> ---
> .../bindings/display/bridge/nxp,ldb.yaml | 96 +++
> 1 file changed, 96 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
> b/Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
> new file mode 100644
> index 0..f3182566eb316
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/nxp,ldb.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/nxp,ldb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP i.MX8M DPI to LVDS bridge chip
> +
> +maintainers:
> + - Marek Vasut
> +
> +description: |
> + The i.MX8MP contains two syscon registers which are responsible
> + for configuring the on-SoC DPI-to-LVDS serializer. This describes
> + those registers as bridge within the DT.
> +
> +properties:
> + compatible:
> +const: fsl,imx8mp-ldb
> +
> + clocks:
> +maxItems: 1
> +
> + clock-names:
> +const: ldb
> +
> + fsl,syscon:
> +$ref: /schemas/types.yaml#/definitions/phandle
> +description: A phandle to media block controller.
> +
> + ports:
> +$ref: /schemas/graph.yaml#/properties/ports
> +
> +properties:
> + port@0:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for DPI input.
> +
> + port@1:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for LVDS Channel-A output (panel or bridge).
> +
> + port@2:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for LVDS Channel-B output (panel or bridge).
> +
> +required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - clocks
> + - fsl,syscon
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +
> +bridge {
> +compatible = "fsl,imx8mp-ldb";
> +clocks = < IMX8MP_CLK_MEDIA_LDB>;
> +clock-names = "ldb";
> +fsl,syscon = <_blk_ctrl>;
> +
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +port@0 {
> +reg = <0>;
> +
> +ldb_from_lcdif2: endpoint {
> +remote-endpoint = <_to_ldb>;
> +};
> +};
> +
> +port@1 {
> +reg = <1>;
> +
> +ldb_lvds_ch0: endpoint {
> +remote-endpoint = <_to_lvdsx4panel>;
> +};
> +};
> +
> +port@2 {
> +reg = <2>;
> +
> +ldb_lvds_ch1: endpoint {
> +};
> +};
> +};
> +};
> --
> 2.35.1
Re: [PATCH 2/2] drm/panel: simple: Add Startek KD070WVFPA043-C069A panel support
On Tue, Apr 19, 2022 at 09:56:25PM -0300, Fabio Estevam wrote:
> From: Heiko Schocher
>
> Add Startek KD070WVFPA043-C069A 7" TFT LCD panel support.
>
> Signed-off-by: Heiko Schocher
> Signed-off-by: Fabio Estevam
> ---
> drivers/gpu/drm/panel/panel-simple.c | 33
> 1 file changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index a34f4198a534..ca8cd017821d 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -3311,6 +3311,36 @@ static const struct panel_desc tsd_tst043015cmhx = {
> .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
> };
>
> +static const struct display_timing startek_kd070wvfpa_mode = {
> + .pixelclock = { 2520, 2720, 3050 },
> + .hactive = { 800, 800, 800 },
> + .hfront_porch = { 19, 44, 115 },
> + .hback_porch = { 5, 16, 101 },
> + .hsync_len = { 1, 2, 100 },
> + .vactive = { 480, 480, 480 },
> + .vfront_porch = { 5, 43, 67 },
> + .vback_porch = { 5, 5, 67 },
> + .vsync_len = { 1, 2, 66 },
> +};
> +
> +static const struct panel_desc startek_kd070wvfpa = {
> + .timings = _kd070wvfpa_mode,
> + .num_timings = 1,
> + .bpc = 8,
> + .size = {
> + .width = 152,
> + .height = 91,
> + },
> + .delay = {
> + .prepare = 20,
> + .enable = 200,
> + .disable = 200,
> + },
> + .bus_flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE,
> + .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> + .connector_type = DRM_MODE_CONNECTOR_DPI,
> +};
Please fix so order of startek_kd070wvfpa is the same order in
platform_of_match.
startek_kd070wvfpa is after tsd_tst043015cmhx above, but before
tsd_tst043015cmhx below.
Sam
> +
> static const struct drm_display_mode tfc_s9700rtwv43tr_01b_mode = {
> .clock = 3,
> .hdisplay = 800,
> @@ -3990,6 +4020,9 @@ static const struct of_device_id platform_of_match[] = {
> }, {
> .compatible = "starry,kr070pe2t",
> .data = _kr070pe2t,
> + }, {
> + .compatible = "startek,kd070wvfpa",
> + .data = _kd070wvfpa,
> }, {
> .compatible = "team-source-display,tst043015cmhx",
> .data = _tst043015cmhx,
> --
> 2.25.1
Re: [PATCH] drm/panel: newvision-nv3052c: Fix build
On Sat, Apr 09, 2022 at 11:00:16AM +0100, Paul Cercueil wrote:
> The driver was compile-tested then rebased on drm-misc-next, and not
> compile-tested after the rebase; unfortunately the driver didn't compile
> anymore when it hit drm-misc-next.
>
> Fixes: 49956b505c53 ("drm/panel: Add panel driver for NewVision NV3052C based
> LCDs")
> Cc: Christophe Branchereau
> Cc: kbuild-all
> Cc: Stephen Rothwell
> Reported-by: kernel test robot
> Signed-off-by: Paul Cercueil
Acked-by: Sam Ravnborg
> ---
> drivers/gpu/drm/panel/panel-newvision-nv3052c.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> index 127bcfdb59df..cf078f0d3cd3 100644
> --- a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> +++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> @@ -416,15 +416,13 @@ static int nv3052c_probe(struct spi_device *spi)
> return 0;
> }
>
> -static int nv3052c_remove(struct spi_device *spi)
> +static void nv3052c_remove(struct spi_device *spi)
> {
> struct nv3052c *priv = spi_get_drvdata(spi);
>
> drm_panel_remove(>panel);
> drm_panel_disable(>panel);
> drm_panel_unprepare(>panel);
> -
> - return 0;
> }
>
> static const struct drm_display_mode ltk035c5444t_modes[] = {
> --
> 2.35.1
Re: [PATCH] dt-bindings: display: panel-timing: Define a single type for properties
Hi Rob, On Wed, Apr 13, 2022 at 09:00:15AM -0500, Rob Herring wrote: > It's not good practice to define multiple types for the same property, so > factor out the type reference making the properties always an uint32-array > with a length of 1 or 3 items. > > Signed-off-by: Rob Herring Looks good, Reviewed-by: Sam Ravnborg Let me know if we shall take it in drm-misc. Sam
Re: [PATCH v5 3/3] drm/panel : innolux-ej030na and abt-y030xx067a : add .enable and .disable
Hi Christophe, On Mon, Mar 21, 2022 at 03:42:08PM +0100, Christophe Branchereau wrote: > Sorry I meant "sleep out" not "sleep in" obviously > > On Mon, Mar 21, 2022 at 3:39 PM Christophe Branchereau > wrote: > > > > Following the introduction of bridge_atomic_enable in the ingenic > > drm driver, the crtc is enabled between .prepare and .enable, if > > it exists. Add it so the backlight is only enabled after the crtc is, to > > avoid graphical issues. > > > > As we're moving the "sleep in" command out of the init sequence > > into .enable for the ABT, we need to switch the regmap cache > > to REGCACHE_FLAT to be able to use regmap_set_bits, given this > > panel registers are write-ony and read as 0. > > > > On Mon, Mar 21, 2022 at 3:21 PM Paul Cercueil wrote: > > > > > > Hi Christophe, > > > > > > Le lun., mars 21 2022 at 14:36:51 +0100, Christophe Branchereau > > > a écrit : > > > > Following the introduction of bridge_atomic_enable in the ingenic > > > > drm driver, the crtc is enabled between .prepare and .enable, if > > > > it exists. > > > > > > > > Add it so the backlight is only enabled after the crtc is, to avoid > > > > graphical issues. > > > > > > > > Signed-off-by: Christophe Branchereau > > > > > > Didn't Sam acked it? No, that was the new driver, already replied. For these changes - with the updated changelog: Acked-by: Sam Ravnborg
Re: [PATCH v5 2/3] drm/panel: Add panel driver for NewVision NV3052C based LCDs
Hi Christophe On Mon, Mar 21, 2022 at 02:36:50PM +0100, Christophe Branchereau wrote: > This driver supports the NewVision NV3052C based LCDs. Right now, it > only supports the LeadTek LTK035C5444T 2.4" 640x480 TFT LCD panel, which > can be found in the Anbernic RG-350M handheld console. > > Signed-off-by: Christophe Branchereau Please add: Reviewed-by: Sam Ravnborg from the previous review round. Sam
Re: [PATCH v4 2/4] drm/panel: Add panel driver for NewVision NV3052C based LCDs
Hi Christophe,
> > > + { 0x0d, 0x58 },
> > > + { 0x0e, 0x48 },
> > > + { 0x0f, 0x38 },
> > > + { 0x10, 0x2b },
> > > +
> > > + { 0xff, 0x30 },
> > > + { 0xff, 0x52 },
> > > + { 0xff, 0x00 },
> > > + { 0x36, 0x0a },
> > > +};
> > There are some random (?) empty lines.
> > If they have any significance then a short comment would be nice.
> > If not, then drop the empty lines.
> >
>
> The empty lines are not random no, to access a different page in the
> init, one must write i.e. { 0xff, 0x30 }, { 0xff, 0x52 }, { 0xff,
> 0x02 }, to access page 2, so they add a little bit of readability.
Then I suggest to just say what page is is like:
/* Page 1 */
{ 0xff, 0x30 },
...
Etc. then it is obvious this is page boundaries.
Sam
Re: [PATCH 1/4] drm/gma500: Remove unused declarations and other cruft
Hi Patrik, On Thu, Mar 17, 2022 at 10:25:52AM +0100, Patrik Jakobsson wrote: > Most of these are old leftovers from one of the driver merges. This is > all dead code. Nice cleanups. For all four patches: Acked-by: Sam Ravnborg
Re: [PATCH,v2] drm/panel: Fix return value check in nt35950_probe()
On Thu, Mar 17, 2022 at 04:37:07PM +0800, Lu Wei wrote:
> In function nt35950_probe(), mipi_dsi_device_register_full() is called
> to create a MIPI DSI device. If it fails, a pointer encoded with an error
> will be returned, so use IS_ERR() to check the return value. Besides, use
> PTR_ERR to return the actual errno.
>
> Fixes: 623a3531e9cf ("drm/panel: Add driver for Novatek NT35950 DSI DriverIC
> panels")
> Signed-off-by: Lu Wei
Acked-by: Sam Ravnborg
Re: [PATCH] drm: Don't make DRM_PANEL_BRIDGE dependent on DRM_KMS_HELPERS
On Tue, Mar 15, 2022 at 09:45:59AM +0100, Thomas Zimmermann wrote:
> Fix a number of undefined references to drm_kms_helper.ko in
> drm_dp_helper.ko:
>
> arm-suse-linux-gnueabi-ld: drivers/gpu/drm/dp/drm_dp_mst_topology.o: in
> function `drm_dp_mst_duplicate_state':
> drm_dp_mst_topology.c:(.text+0x2df0): undefined reference to
> `__drm_atomic_helper_private_obj_duplicate_state'
> arm-suse-linux-gnueabi-ld: drivers/gpu/drm/dp/drm_dp_mst_topology.o: in
> function `drm_dp_delayed_destroy_work':
> drm_dp_mst_topology.c:(.text+0x370c): undefined reference to
> `drm_kms_helper_hotplug_event'
> arm-suse-linux-gnueabi-ld: drivers/gpu/drm/dp/drm_dp_mst_topology.o: in
> function `drm_dp_mst_up_req_work':
> drm_dp_mst_topology.c:(.text+0x7938): undefined reference to
> `drm_kms_helper_hotplug_event'
> arm-suse-linux-gnueabi-ld: drivers/gpu/drm/dp/drm_dp_mst_topology.o: in
> function `drm_dp_mst_link_probe_work':
> drm_dp_mst_topology.c:(.text+0x82e0): undefined reference to
> `drm_kms_helper_hotplug_event'
>
> This happens if panel-edp.ko has been configured with
>
> DRM_PANEL_EDP=y
> DRM_DP_HELPER=y
> DRM_KMS_HELPER=m
>
> which builds DP helpers into the kernel and KMS helpers sa a module.
> Making DRM_PANEL_EDP select DRM_KMS_HELPER resolves this problem.
>
> To avoid a resulting cyclic dependency with DRM_PANEL_BRIDGE, don't
> make the latter depend on DRM_KMS_HELPER and fix the one DRM bridge
> drivers that doesn't already select DRM_KMS_HELPER. As KMS helpers
> cannot be selected directly by the user, config symbols should avoid
> depending on it anyway.
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 3755d35ee1d2 ("drm/panel: Select DRM_DP_HELPER for DRM_PANEL_EDP")
> Reported-by: kernel test robot
> Cc: Thomas Zimmermann
> Cc: Naresh Kamboju
> Cc: Linux Kernel Functional Testing
> Cc: Lyude Paul
> Cc: Sam Ravnborg
> Cc: Daniel Vetter
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: dri-devel@lists.freedesktop.org
> Cc: Dave Airlie
> Cc: Thierry Reding
Acked-by: Sam Ravnborg
Re: [PATCH v4 2/4] drm/panel: Add panel driver for NewVision NV3052C based LCDs
Hi Christophe,
On Fri, Mar 11, 2022 at 06:02:38PM +0100, Christophe Branchereau wrote:
> This driver supports the NewVision NV3052C based LCDs. Right now, it
> only supports the LeadTek LTK035C5444T 2.4" 640x480 TFT LCD panel, which
> can be found in the Anbernic RG-350M handheld console.
I had to get away from my day-time job and you were the lucky winner.
A couple of comments in the following that you can address now or later.
>
> Signed-off-by: Christophe Branchereau
Reviewed-by: Sam Ravnborg
> ---
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile| 1 +
> .../gpu/drm/panel/panel-newvision-nv3052c.c | 497 ++
> 3 files changed, 507 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-newvision-nv3052c.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index bb2e47229c68..40084f709789 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -283,6 +283,15 @@ config DRM_PANEL_NEC_NL8048HL11
> panel (found on the Zoom2/3/3630 SDP boards). To compile this driver
> as a module, choose M here.
>
> +config DRM_PANEL_NEWVISION_NV3052C
> + tristate "NewVision NV3052C RGB/SPI panel"
> + depends on OF && SPI
> + depends on BACKLIGHT_CLASS_DEVICE
> + select DRM_MIPI_DBI
> + help
> + Say Y here if you want to enable support for the panels built
> + around the NewVision NV3052C display controller.
> +
> config DRM_PANEL_NOVATEK_NT35510
> tristate "Novatek NT35510 RGB panel driver"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 5740911f637c..42a7ab54234b 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) +=
> panel-leadtek-ltk500hd1829.o
> obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
> obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
> obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o
> +obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3052C) += panel-newvision-nv3052c.o
> obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35510) += panel-novatek-nt35510.o
> obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35560) += panel-novatek-nt35560.o
> obj-$(CONFIG_DRM_PANEL_NOVATEK_NT35950) += panel-novatek-nt35950.o
> diff --git a/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> new file mode 100644
> index ..fc31df0dee12
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-newvision-nv3052c.c
> @@ -0,0 +1,497 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NevVision NV3052C IPS LCD panel driver
> + *
> + * Copyright (C) 2020, Paul Cercueil
> + * Copyright (C) 2022, Christophe Branchereau
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +
> +struct nv3052c_panel_info {
> + const struct drm_display_mode *display_modes;
> + unsigned int num_modes;
> + u16 width_mm, height_mm;
> + u32 bus_format, bus_flags;
> +};
> +
> +struct nv3052c {
> + struct device *dev;
> + struct drm_panel panel;
> + struct mipi_dbi dbi;
> +
> + const struct nv3052c_panel_info *panel_info;
> +
> + struct regulator *supply;
> + struct gpio_desc *reset_gpio;
> +};
> +
> +struct nv3052c_reg {
> + u8 cmd;
> + u8 val;
> +};
> +
> +static const struct nv3052c_reg nv3052c_panel_regs[] = {
> + { 0xff, 0x30 },
> + { 0xff, 0x52 },
> + { 0xff, 0x01 },
> + { 0xe3, 0x00 },
> + { 0x40, 0x00 },
> + { 0x03, 0x40 },
> + { 0x04, 0x00 },
> + { 0x05, 0x03 },
> + { 0x08, 0x00 },
> + { 0x09, 0x07 },
> + { 0x0a, 0x01 },
> + { 0x0b, 0x32 },
> + { 0x0c, 0x32 },
> + { 0x0d, 0x0b },
> + { 0x0e, 0x00 },
> + { 0x23, 0xa0 },
> +
> + { 0x24, 0x0c },
> + { 0x25, 0x06 },
> + { 0x26, 0x14 },
> + { 0x27, 0x14 },
> +
> + { 0x38, 0xcc },
> + { 0x39, 0xd7 },
> + { 0x3a, 0x4a },
> +
> + { 0x28, 0x40 },
> + { 0x29, 0x01 },
> + { 0x2a, 0xdf },
> + { 0x49, 0x3c },
> + { 0x91, 0x77 },
> + { 0x92, 0x77 },
> + { 0xa0, 0x55 },
> + { 0xa1, 0x50 },
> + { 0xa4, 0x9c },
> + { 0xa7, 0x02 },
> + { 0xa8, 0x01 },
> + { 0xa9, 0x01 },
> + { 0xaa, 0xfc },
> + { 0xab, 0x28 },
> + { 0xac, 0x06 },
>
Re: [PATCH v2] drm/bridge: anx7625: Fix not correct get property counts
Hi Xin.
On Fri, Mar 11, 2022 at 06:35:25PM +0800, Xin Ji wrote:
> The property length which returns from "of_get_property", it means array
> bytes count if the property has prefix as "/bits/ 8". The driver should
> call function "of_property_read_u8_array" to get correct array value.
>
> Fixes: fd0310b6fe7d ("drm/bridge: anx7625: add MIPI DPI input feature")
> Signed-off-by: Xin Ji
>
> ---
> V1 -> V2: Fix Sam comment, use of_property_read_u8_array to get array
> value
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 8
> drivers/gpu/drm/bridge/analogix/anx7625.h | 4 ++--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index c6a9a02ed762..628cbf769141 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1598,8 +1598,8 @@ static int anx7625_get_swing_setting(struct device *dev,
> num_regs = DP_TX_SWING_REG_CNT;
>
> pdata->dp_lane0_swing_reg_cnt = num_regs;
> - of_property_read_u32_array(dev->of_node, "analogix,lane0-swing",
> -pdata->lane0_reg_data, num_regs);
> + of_property_read_u8_array(dev->of_node, "analogix,lane0-swing",
> + pdata->lane0_reg_data, num_regs);
The current implementation do a two step approach. First is find the
number of elements and then read the elements.
The number of elements is only used to limit what is read.
I suggest to use:
of_property_read_u8_array(dev->of_node, "analogix,lane0-swing",
pdata->lane0_reg_data, DP_TX_SWING_REG_CNT);
Then you a guaranteed to read at maximum DP_TX_SWING_REG_CNT entries.
And as the number of elements is not stored anywhere that should be fine.
This looks simpler and matches what we for example do in
drivers/gpu/drm/arm/malidp_drv.c - the only user in gpu/ of
of_property_read_u8_array().
Sam
Re: [PATCH] drm/bridge: anx7625: Fix not correct get property counts
Hi Xin,
On Thu, Mar 10, 2022 at 05:16:53PM +0800, Xin Ji wrote:
> The property length which returns from "of_get_property", divided by
> sizeof(int) to get the total property counts.
>
> Fixes: fd0310b6fe7d ("drm/bridge: anx7625: add MIPI DPI input feature")
>
> Signed-off-by: Xin Ji
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index c6a9a02ed762..87081d5b408d 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1594,6 +1594,7 @@ static int anx7625_get_swing_setting(struct device *dev,
>
> if (of_get_property(dev->of_node,
> "analogix,lane0-swing", _regs)) {
> + num_regs /= sizeof(int);
Since the property is an array maybe use: of_property_read_u8_array()
Sam
Re: [PATCH 06/10] drm/gma500: Move GTT resume logic out of psb_gtt_init()
Hi Thomas,
One comment below.
On Sun, Mar 06, 2022 at 09:36:15PM +0100, Thomas Zimmermann wrote:
> The current implementation of psb_gtt_init() also does resume
> handling. Move the resume code into its own helper.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/gma500/gtt.c | 122 ++-
> drivers/gpu/drm/gma500/gtt.h | 2 +-
> drivers/gpu/drm/gma500/psb_drv.c | 2 +-
> 3 files changed, 104 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
> index acd50ee26b03..43ad3ec38c80 100644
> --- a/drivers/gpu/drm/gma500/gtt.c
> +++ b/drivers/gpu/drm/gma500/gtt.c
> @@ -209,7 +209,7 @@ static void psb_gtt_populate_resources(struct
> drm_psb_private *pdev)
> drm_dbg(dev, "Restored %u of %u gtt ranges (%u KB)", restored, total,
> (size / 1024));
> }
>
> -int psb_gtt_init(struct drm_device *dev, int resume)
> +int psb_gtt_init(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> struct pci_dev *pdev = to_pci_dev(dev->dev);
> @@ -218,10 +218,8 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> struct psb_gtt *pg;
> int ret = 0;
>
> - if (!resume) {
> - mutex_init(_priv->gtt_mutex);
> - mutex_init(_priv->mmap_mutex);
> - }
> + mutex_init(_priv->gtt_mutex);
> + mutex_init(_priv->mmap_mutex);
>
> pg = _priv->gtt;
>
> @@ -290,13 +288,6 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> dev_dbg(dev->dev, "Stolen memory base 0x%x, size %luK\n",
> dev_priv->stolen_base, vram_stolen_size / 1024);
>
> - if (resume && (gtt_pages != pg->gtt_pages) &&
> - (stolen_size != pg->stolen_size)) {
> - dev_err(dev->dev, "GTT resume error.\n");
> - ret = -EINVAL;
> - goto out_err;
> - }
> -
> pg->gtt_pages = gtt_pages;
> pg->stolen_size = stolen_size;
> dev_priv->vram_stolen_size = vram_stolen_size;
> @@ -304,19 +295,14 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> /*
>* Map the GTT and the stolen memory area
>*/
> - if (!resume)
> - dev_priv->gtt_map = ioremap(pg->gtt_phys_start,
> - gtt_pages << PAGE_SHIFT);
> + dev_priv->gtt_map = ioremap(pg->gtt_phys_start, gtt_pages <<
> PAGE_SHIFT);
> if (!dev_priv->gtt_map) {
> dev_err(dev->dev, "Failure to map gtt.\n");
> ret = -ENOMEM;
> goto out_err;
> }
>
> - if (!resume)
> - dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base,
> - stolen_size);
> -
> + dev_priv->vram_addr = ioremap_wc(dev_priv->stolen_base, stolen_size);
> if (!dev_priv->vram_addr) {
> dev_err(dev->dev, "Failure to map stolen base.\n");
> ret = -ENOMEM;
> @@ -333,11 +319,107 @@ int psb_gtt_init(struct drm_device *dev, int resume)
> return ret;
> }
>
The below is a lot of duplicated complex code.
Can you add one more helper for this?
> +static int psb_gtt_resume(struct drm_device *dev)
> +{
> + struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> + unsigned int gtt_pages;
> + unsigned long stolen_size, vram_stolen_size;
> + struct psb_gtt *pg;
> + int ret = 0;
> +
> + pg = _priv->gtt;
static void psb_enable_gtt(..)
{
> +
> + /* Enable the GTT */
> + pci_read_config_word(pdev, PSB_GMCH_CTRL, _priv->gmch_ctrl);
> + pci_write_config_word(pdev, PSB_GMCH_CTRL,
> + dev_priv->gmch_ctrl | _PSB_GMCH_ENABLED);
> +
> + dev_priv->pge_ctl = PSB_RVDC32(PSB_PGETBL_CTL);
> + PSB_WVDC32(dev_priv->pge_ctl | _PSB_PGETBL_ENABLED, PSB_PGETBL_CTL);
> + (void) PSB_RVDC32(PSB_PGETBL_CTL);
> +
> + /* The root resource we allocate address space from */
> + dev_priv->gtt_initialized = 1;
> +
> + pg->gtt_phys_start = dev_priv->pge_ctl & PAGE_MASK;
> +
> + /*
> + * The video mmu has a hw bug when accessing 0x0D000.
> + * Make gatt start at 0x0e000,. This doesn't actually
> + * matter for us but may do if the video acceleration ever
> + * gets opened up.
> + */
> + pg->mmu_gatt_start = 0xE000;
> +
> + pg->gtt_start = pci_resource_start(pdev, PSB_GTT_RESOURCE);
> + gtt_pages = pci_resource_len(pdev, PSB_GTT_RESOURCE) >> PAGE_SHIFT;
> + /* CDV doesn't report this. In which case the system has 64 gtt pages */
> + if (pg->gtt_start == 0 || gtt_pages == 0) {
> + dev_dbg(dev->dev, "GTT PCI BAR not initialized.\n");
> + gtt_pages = 64;
> + pg->gtt_start = dev_priv->pge_ctl;
> + }
> +
> + pg->gatt_start = pci_resource_start(pdev, PSB_GATT_RESOURCE);
> + pg->gatt_pages =
Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops
Hi Rob,
> > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > index 35e7f44c2a75..987e78b18244 100644
> > > --- a/include/drm/drm_gem.h
> > > +++ b/include/drm/drm_gem.h
> > > @@ -327,7 +327,7 @@ struct drm_gem_object {
> > > * non-static version of this you're probably doing it wrong and will
> > > break the
> > > * THIS_MODULE reference by accident.
> > > */
> > > -#define DEFINE_DRM_GEM_FOPS(name) \
> > > +#define DEFINE_DRM_GEM_FOPS(name, ...) \
> > > static const struct file_operations name = {\
> > > .owner = THIS_MODULE,\
> > > .open = drm_open,\
> > > @@ -338,6 +338,7 @@ struct drm_gem_object {
> > > .read = drm_read,\
> > > .llseek = noop_llseek,\
> > > .mmap = drm_gem_mmap,\
> > > + ##__VA_ARGS__\
> > > }
> >
> > Would it not be less convoluted to make the macro only provide
> > the initializers? So you'd get something like:
> >
> > static const struct file_operations foo = {
> > DRM_GEM_FOPS,
> > .my_stuff = whatever,
> > };
> >
>
> Hmm, I like my color of the bikeshed, but I guess it is a matter of opinion
> ;-)
Or less surprise. Most similar macros provides initializers only.
Try "git grep DRM_.*OPS | grep define" in include/drm
and take a look at the hits.
Sam
Re: [PATCH v5 5/5] drm/tiny: Add MIPI DBI compatible SPI driver
Hi Noralf,
On Thu, Feb 24, 2022 at 04:27:08PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
>
> Example (driver will load sainsmart18.bin):
>
> display@0 {
> compatible = "sainsmart18", "panel-mipi-dbi-spi";
> ...
> };
>
> v5:
> - kconfig: s/DRM_KMS_CMA_HELPER/DRM_GEM_CMA_HELPER/ (Sam)
> - kconfig: Add select VIDEOMODE_HELPERS (Sam)
> - kconfig: Add wiki url in the description (Sam)
> - Split out and use of_get_drm_panel_display_mode()(Sam)
> - Only use the first compatible to look for a firmware file since the
> binding mandates 2 compatibles.
> - Make having a firmware file mandatory so we can print an error
> message if it's missing to improve the user experience. It's very
> unlikely that a controller doesn't need to be initialized and if
> it doesn't, it's possible to have a firmware file containing only
> a DCS NOP.
>
> v4:
> - Move driver to drm/tiny where the other drivers of its kind are located.
> The driver module will not be shared with a future DPI driver after all.
>
> v3:
> - Move properties to DT (Maxime)
> - The MIPI DPI spec has optional support for DPI where the controller is
> configured over DBI. Rework the command functions so they can be moved
> to drm_mipi_dbi and shared with a future panel-mipi-dpi-spi driver
>
> v2:
> - Drop model property and use compatible instead (Rob)
> - Add wiki entry in MAINTAINERS
>
> Acked-by: Maxime Ripard
> Signed-off-by: Noralf Trønnes
Looks good.
Reviewed-by: Sam Ravnborg
Re: [PATCH v5 3/5] drm/modes: Add of_get_drm_panel_display_mode()
On Thu, Feb 24, 2022 at 04:27:06PM +0100, Noralf Trønnes wrote: > Add a function to get a drm_display_mode from a panel-timing > device tree subnode. Thanks for implementing this! > > Suggested-by: Sam Ravnborg > Signed-off-by: Noralf Trønnes Reviewed-by: Sam Ravnborg
Re: [PATCH v5 2/5] drm/modes: Remove trailing whitespace
On Thu, Feb 24, 2022 at 04:27:05PM +0100, Noralf Trønnes wrote: > Remove trailing whitespace from a comment. > > Signed-off-by: Noralf Trønnes Acked-by: Sam Ravnborg > --- > drivers/gpu/drm/drm_modes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 96b13e36293c..77a4c8dd0bb8 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -127,7 +127,7 @@ EXPORT_SYMBOL(drm_mode_probed_add); > * according to the hdisplay, vdisplay, vrefresh. > * It is based from the VESA(TM) Coordinated Video Timing Generator by > * Graham Loveridge April 9, 2003 available at > - * http://www.elo.utfsm.cl/~elo212/docs/CVTd6r1.xls > + * http://www.elo.utfsm.cl/~elo212/docs/CVTd6r1.xls > * > * And it is copied from xf86CVTmode in xserver/hw/xfree86/modes/xf86cvt.c. > * What I have done is to translate it by using integer calculation. > -- > 2.33.0
Re: [PATCH v3 4/5] fbdev: Improve performance of cfb_imageblit()
Hi Javier,
On Thu, Feb 24, 2022 at 10:02:59AM +0100, Javier Martinez Canillas wrote:
> Hello Sam,
>
> On 2/23/22 21:25, Sam Ravnborg wrote:
>
> [snip]
>
> >
> > Question: What is cfb an abbreviation for anyway?
> > Not related to the patch - but if I have known the memory is lost..
> >
>
> I was curious so I dug on this. It seems CFB stands for Color Frame Buffer.
> Doing a `git grep "(CFB)"` in the linux history repo [0], I get this:
>
> Documentation/isdn/README.diversion: (CFB).
> drivers/video/pmag-ba-fb.c: * PMAG-BA TURBOchannel Color Frame Buffer
> (CFB) card support,
> include/video/pmag-ba-fb.h: * TURBOchannel PMAG-BA Color Frame Buffer
> (CFB) card support,
>
> Probably the helpers are called like this because they were for any fbdev
> driver but assumed that the framebuffer was always in I/O memory. Later some
> drivers were allocating the framebuffer in system memory and still using the
> helpers, that were using I/O memory accessors and it's ilegal on some arches.
>
> So the sys_* variants where introduced by commit 68648ed1f58d ("fbdev: add
> drawing functions for framebuffers in system RAM") to fix this. The old
> ones just kept their name, but probably it should had been renamed to io_*
> for the naming to be consistent with the sys_* functions.
>
> [0]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
Interesting - thanks for the history lesson and thanks for taking your
time to share your findings too.
Sam
Re: [PATCH v3 5/5] drm: Add TODO item for optimizing format helpers
On Wed, Feb 23, 2022 at 08:38:04PM +0100, Thomas Zimmermann wrote: > Add a TODO item for optimizing blitting and format-conversion helpers > in DRM and fbdev. There's always demand for faster graphics output. > > Signed-off-by: Thomas Zimmermann > --- > Documentation/gpu/todo.rst | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 7bf7f2111696..7f113c6a02dd 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -241,6 +241,28 @@ Contact: Thomas Zimmermann , Daniel > Vetter > > Level: Advanced > > +Benchmark and optimize blitting and format-conversion function > +-- > + > +Drawing to dispay memory quickly is crucial for many applications' display > +performance. > + > +On at least x86-64, sys_imageblit() is significantly slower than On, at least x86-64, ... To me the extra comma makes sense, but grammar is not my strong side. > +cfb_imageblit(), even though both use the same blitting algorithm and > +the latter is written for I/O memory. It turns out that cfb_imageblit() > +uses movl instructions, while sys_imageblit apparently does not. This > +seems to be a problem with gcc's optimizer. DRM's format-conversion > +heleprs might be subject to similar issues. helpers > + > +Benchmark and optimize fbdev's sys_() helpers and DRM's format-conversion > +helpers. In cases that can be further optimized, maybe implement a different > +algorithm, For micro-optimizations, use movl/movq instructions explicitly. algorithm. (period, not comma) > +That might possibly require architecture specific helpers (e.g., storel() > +storeq()). > + > +Contact: Thomas Zimmermann > + > +Level: Intermediate With the small fixes above: Acked-by: Sam Ravnborg Another option would be to re-implement imageblit() to be drm specific. Maybe we can then throw out some legacy code and optimize only for the drm use. And then maybe only a small part of the code would differ if this is I/O memory or direct accessible memory. Sam
Re: [PATCH v3 4/5] fbdev: Improve performance of cfb_imageblit()
On Wed, Feb 23, 2022 at 08:38:03PM +0100, Thomas Zimmermann wrote: > Improve the performance of cfb_imageblit() by manually unrolling > the inner blitting loop and moving some invariants out. The compiler > failed to do this automatically. This change keeps cfb_imageblit() > in sync with sys_imagebit(). > > A microbenchmark measures the average number of CPU cycles > for cfb_imageblit() after a stabilizing period of a few minutes > (i7-4790, FullHD, simpledrm, kernel with debugging). > > cfb_imageblit(), new: 15724 cycles > cfb_imageblit(): old: 30566 cycles > > In the optimized case, cfb_imageblit() is now ~2x faster than before. > > v3: > * fix commit description (Pekka) > > Signed-off-by: Thomas Zimmermann Acked-by: Sam Ravnborg The code looks equally complicated now in the sys and cfb variants. Question: What is cfb an abbreviation for anyway? Not related to the patch - but if I have known the memory is lost.. Sam
Re: [PATCH v3 3/5] fbdev: Remove trailing whitespaces from cfbimgblt.c
On Wed, Feb 23, 2022 at 08:38:02PM +0100, Thomas Zimmermann wrote: > Fix coding style. No functional changes. > > Signed-off-by: Thomas Zimmermann Acked-by: Sam Ravnborg
Re: [PATCH] drm/omap: switch to drm_of_find_panel_or_bridge
Hi José, On Sun, Feb 20, 2022 at 08:52:12PM +0100, José Expósito wrote: > Use the "drm_of_find_panel_or_bridge" function instead of a custom > version of it to reduce the boilerplate. Thanks for looking into this. >From the documentation of drm_of_find_panel_or_bridge(): * This function is deprecated and should not be used in new drivers. Use * devm_drm_of_get_bridge() instead. Are you OK to give this a second try with the above referenced function? There is a good chance the deprecation happened after you looked into this first, sometimes things moves fast in the drm sub-system. Sam
Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver
Hi Noralf, On Sun, Feb 20, 2022 at 04:59:34PM +0100, Noralf Trønnes wrote: > > > Den 19.02.2022 23.10, skrev Sam Ravnborg: > > Hi Noralf, > > On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote: > >> Add a driver that will work with most MIPI DBI compatible SPI panels. > >> This avoids adding a driver for every new MIPI DBI compatible controller > >> that is to be used by Linux. The 'compatible' Device Tree property with > >> a '.bin' suffix will be used to load a firmware file that contains the > >> controller configuration. > > A solution where we have the command sequences as part of the DT-overlay > > is IMO a much better way to solve this: > > - The users needs to deal only with a single file, so there is less that > > goes wrong > > - The user need not reading special instructions how to handle a .bin > > file, if the overlay is present everything is fine > > - The file that contains the command sequences can be properly annotated > > with comments > > - The people that creates the command sequences has no need for a special > > script to create the file for a display - this is all readable ascii. > > - Using a DT-overlay the parsing of the DT-overlay can be done by > > well-tested functions, no need to invent something new to parse the > > file > > > > > > The idea with a common mipi DBI SPI driver is super, it is the detail > > with the .bin file that I am against. > > > > The fbtft drivers has an init= DT property that contains the command > sequence. Example for the MZ61581 display: > > init = <0x1b0 00 > 0x111 > 0x2ff > 0x1b3 0x02 0x00 0x00 0x00 > 0x1c0 0x13 0x3b 0x00 0x02 0x00 0x01 > 0x00 0x43 > 0x1c1 0x08 0x16 0x08 0x08 > 0x1c4 0x11 0x07 0x03 0x03 > 0x1c6 0x00 > 0x1c8 0x03 0x03 0x13 0x5c 0x03 0x07 > 0x14 0x08 0x00 0x21 0x08 > 0x14 0x07 0x53 0x0c 0x13 0x03 0x03 0x21 0x00 > 0x135 0x00 > 0x136 0xa0 > 0x13a 0x55 > 0x144 0x00 0x01 > 0x1d0 0x07 0x07 0x1d 0x03 > 0x1d1 0x03 0x30 0x10 > 0x1d2 0x03 0x14 0x04 > 0x129 > 0x12c>; > > Parsed here: > https://elixir.bootlin.com/linux/latest/C/ident/fbtft_init_display_from_property > > Before fbdev was closed for new drivers I looked at fixing up fbtft for > proper inclusion and asked on the Device Tree mailinglist about the init > property and how to handle the controller configuration generically. > I was politely told that this should be done in the driver, so when I > made my first DRM driver I made a driver specifically for a panel > (mi0283qt.c). > > Later I found another post that in clear words stated that setting > random registers from DT was not acceptable. Understood and thanks for the explanation. It is a shame that the users loose here :-( Sam
Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver
Hi Noralf,
> >>mode->flags) {
> >>dev_err(dev, "%pOF: panel-timing out of bounds\n",
> >> dev->of_node);
> >>return -EINVAL;
> >>}
> > With the display_timing => drm_display_mode I think the above is no
> > longer required.
> >
>
> I still need to verify the values to ensure that front_porch and
> sync_len are zero. Maybe I need a comment now to tell what I'm checking
> since I'm further away from the DT values:
>
> /*
> * Make sure width and height are set and that only back porch and
> * pixelclock are set in the other timing values. Also check that
> * width and height don't exceed the 16-bit value specified by MIPI DCS.
> */
Yes, that would be nice.
>
> >>
> >>/* The driver doesn't use the pixel clock but it is mandatory so fake
> >> one if not set */
> >>if (!mode->pixelclock)
> >>mode->pixelclock = mode->htotal * mode->vtotal * 60 / 1000;
> >>
> >>dbidev->top_offset = vback_porch;
> >>dbidev->left_offset = hback_porch;
> >>
> >>return 0;
> >> }
> >>
> >>
> >> int of_get_drm_panel_display_mode(struct device_node *np,
> >> struct drm_display_mode *dmode, u32
> >> *bus_flags)
> >> {
> > Not sure about the bus_flags argument here - seems misplaced.
> >
>
> I did the same as of_get_drm_display_mode(), don't panel drivers need
> the bus flags?
In my haste I missed the display_timing combines flags for the bus and
the mode - so yes it is needed.
>
> >>u32 width_mm = 0, height_mm = 0;
> >>struct display_timing timing;
> >>struct videomode vm;
> >>int ret;
> >>
> >>ret = of_get_display_timing(np, "panel-timing", );
> >>if (ret)
> >>return ret;
> >>
> >>videomode_from_timing(, vm);
> >>
> >>memset(dmode, 0, sizeof(*dmode));
> >>drm_display_mode_from_videomode(, dmode);
> >>if (bus_flags)
> >>drm_bus_flags_from_videomode(, bus_flags);
> >
> > This does a:
> > display_timing => video_mode => drm_display_display_mode
> >
> > We could do a:
> > display_timing => drm_display_mode.
> >
>
> I'll leave this to others to sort out. I want the function to look the
> same as of_get_drm_display_mode() and it uses videomode. If videomode
> goes away both can be fixed at the same time.
When I have dig myself out of the bridge hole I am in I may take a
look at this.
Sam
Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver
Hi Noralf.
On Sun, Feb 20, 2022 at 07:11:14PM +0100, Noralf Trønnes wrote:
> > Den 20.02.2022 11.04, skrev Sam Ravnborg:
> > > Hi Noralf,
> > >
> > >>> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev,
> struct drm_display_mode *mode)
> > >>> +{
> > >>> + struct device *dev = dbidev->drm.dev;
> > >>> + u32 width_mm = 0, height_mm = 0;
> > >>> + struct display_timing timing;
> > >>> + struct videomode vm;
> > >>> + int ret;
> > >>> +
> > >>> + ret = of_get_display_timing(dev->of_node, "panel-timing",
> > >>> );
> > >>> + if (ret) {
> > >>> + dev_err(dev, "%pOF: failed to get panel-timing
> > >>> (error=%d)\n",
> dev->of_node, ret);
> > >>> + return ret;
> > >>> + }
> > >>> +
> > >>> + videomode_from_timing(, );
> > >>> +
> > >>> + if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> > >>> + (vm.hback_porch + vm.hactive) > 0x ||
> > >>> + !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> > >>> + (vm.vback_porch + vm.vactive) > 0x ||
> > >>> + vm.flags) {
> > >>> + dev_err(dev, "%pOF: panel-timing out of bounds\n",
> > >>> dev->of_node);
> > >>> + return -EINVAL;
> > >>> + }
> > >> We should have a helper that implements this. Maybe the display_timing
> > >> => display_mode helper could do it.
> > >
> > > It would be nice with a drm_display_timing_to_mode() but that can come
> > > later - the comment above should not be understood that I consider it
> > > mandatory for this driver.
> > >
> >
> > I did consider adding an of_get_drm_panel_mode() fashioned after
> > of_get_drm_display_mode() but I didn't find any other driver that would
> > actually be able to use it and I would have to do some substraction to
> > get back the {h,v}front_porch values that I need and the optional pixel
> > clock calculation becomes more complex acting from a drm_display_mode so
> > I decided against it.
> >
> > Looking at it now, what I could do is add a function like what
> > of_get_videomode() does for "display-timings":
> >
> > /**
> > * of_get_panel_videomode - get the panel-timing videomode from devicetree
> > * @np: devicenode containing the panel-timing subnode
> > * @vm: returns the videomode
> > *
> > * Returns:
> > * Zero on success, negative error code on failure.
> > **/
> > int of_get_panel_videomode(struct device_node *np, struct videomode *vm)
> > {
> > struct display_timing timing;
> > int ret;
> >
> > ret = of_get_display_timing(np, "panel-timing", );
> > if (ret)
> > return ret;
> >
> > videomode_from_timing(, vm);
> >
> > return 0;
> > }
> >
> > This could also be used by panel-lvds and 2 fbdev drivers, the other
> > panel-timing users need/use the display_timing itself, some for bounds
> > checking.
>
> Scratch that, since videomode is to be avoided I tried adding a
> drm_display_mode function and it didn't complicate matter as I though it
> would so I'll do that instead:
I like, but would like to get rid of video_mode entirely.
>
> static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct
> drm_display_mode *mode)
> {
> struct device *dev = dbidev->drm.dev;
> u32 width_mm = 0, height_mm = 0;
> u16 hback_porch, vback_porch;
> struct videomode vm;
> int ret;
>
> ret = of_get_drm_panel_display_mode(dev->of_node, mode, NULL);
> if (ret) {
> dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
> dev->of_node, ret);
> return ret;
> }
>
> mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>
> hback_porch = mode->htotal - mode->hsync_end;
> vback_porch = mode->vtotal - mode->vsync_end;
The accesss functions I posed below could be used here - so we avoid
typing these (trivial) calculations in many places.
>
> if (mode->hsync_end > mode->hdisplay || (hback_porch + mode->hdisplay)
> > 0x ||
> mode->vsync_end > mo
Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver
Hi Noralf,
> > +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct
> > drm_display_mode *mode)
> > +{
> > + struct device *dev = dbidev->drm.dev;
> > + u32 width_mm = 0, height_mm = 0;
> > + struct display_timing timing;
> > + struct videomode vm;
> > + int ret;
> > +
> > + ret = of_get_display_timing(dev->of_node, "panel-timing", );
> > + if (ret) {
> > + dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
> > dev->of_node, ret);
> > + return ret;
> > + }
> > +
> > + videomode_from_timing(, );
> > +
> > + if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> > + (vm.hback_porch + vm.hactive) > 0x ||
> > + !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> > + (vm.vback_porch + vm.vactive) > 0x ||
> > + vm.flags) {
> > + dev_err(dev, "%pOF: panel-timing out of bounds\n",
> > dev->of_node);
> > + return -EINVAL;
> > + }
> We should have a helper that implements this. Maybe the display_timing
> => display_mode helper could do it.
It would be nice with a drm_display_timing_to_mode() but that can come
later - the comment above should not be understood that I consider it
mandatory for this driver.
Sam
Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver
Hi Noralf, On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote: > Add a driver that will work with most MIPI DBI compatible SPI panels. > This avoids adding a driver for every new MIPI DBI compatible controller > that is to be used by Linux. The 'compatible' Device Tree property with > a '.bin' suffix will be used to load a firmware file that contains the > controller configuration. A solution where we have the command sequences as part of the DT-overlay is IMO a much better way to solve this: - The users needs to deal only with a single file, so there is less that goes wrong - The user need not reading special instructions how to handle a .bin file, if the overlay is present everything is fine - The file that contains the command sequences can be properly annotated with comments - The people that creates the command sequences has no need for a special script to create the file for a display - this is all readable ascii. - Using a DT-overlay the parsing of the DT-overlay can be done by well-tested functions, no need to invent something new to parse the file The idea with a common mipi DBI SPI driver is super, it is the detail with the .bin file that I am against. With the above said, a few comments to the current implementation below. As we know it from you - a very well-written driver. Sam > Acked-by: Maxime Ripard > Signed-off-by: Noralf Trønnes > --- > MAINTAINERS | 8 + > drivers/gpu/drm/tiny/Kconfig | 13 + > drivers/gpu/drm/tiny/Makefile | 1 + > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 413 ++ > 4 files changed, 435 insertions(+) > create mode 100644 drivers/gpu/drm/tiny/panel-mipi-dbi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 8e6e892f99f0..3a0e57f23ad0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6107,6 +6107,14 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > F: Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt > F: drivers/gpu/drm/tiny/mi0283qt.c > > +DRM DRIVER FOR MIPI DBI compatible panels > +M: Noralf Trønnes > +S: Maintained > +W: https://github.com/notro/panel-mipi-dbi/wiki Nice with a wiki for this, I can see this will grow over time and be a place to find how to support more panels. > +T: git git://anongit.freedesktop.org/drm/drm-misc > +F: Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > +F: drivers/gpu/drm/tiny/panel-mipi-dbi.c > + > DRM DRIVER FOR MSM ADRENO GPU > M: Rob Clark > M: Sean Paul > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > index 712e0004e96e..d552e1618da7 100644 > --- a/drivers/gpu/drm/tiny/Kconfig > +++ b/drivers/gpu/drm/tiny/Kconfig > @@ -51,6 +51,19 @@ config DRM_GM12U320 >This is a KMS driver for projectors which use the GM12U320 chipset >for video transfer over USB2/3, such as the Acer C120 mini projector. > > +config DRM_PANEL_MIPI_DBI > + tristate "DRM support for MIPI DBI compatible panels" > + depends on DRM && SPI > + select DRM_KMS_HELPER > + select DRM_KMS_CMA_HELPER This symbol is not present in my drm-misc-next tree (which is a few weeks old, so it may be newer). > + select DRM_MIPI_DBI > + select BACKLIGHT_CLASS_DEVICE > + help > + Say Y here if you want to enable support for MIPI DBI compatible > + panels. The controller command setup can be provided using a > + firmware file. Consider adding a link to the wiki here - this may make it easier for the user to find it. > + To compile this driver as a module, choose M here. > + > config DRM_SIMPLEDRM > tristate "Simple framebuffer driver" > depends on DRM && MMU > diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile > index 5d5505d40e7b..1d9d6227e7ab 100644 > --- a/drivers/gpu/drm/tiny/Makefile > +++ b/drivers/gpu/drm/tiny/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o > obj-$(CONFIG_DRM_BOCHS) += bochs.o > obj-$(CONFIG_DRM_CIRRUS_QEMU)+= cirrus.o > obj-$(CONFIG_DRM_GM12U320) += gm12u320.o > +obj-$(CONFIG_DRM_PANEL_MIPI_DBI) += panel-mipi-dbi.o > obj-$(CONFIG_DRM_SIMPLEDRM) += simpledrm.o > obj-$(CONFIG_TINYDRM_HX8357D)+= hx8357d.o > obj-$(CONFIG_TINYDRM_ILI9163)+= ili9163.o > diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c > b/drivers/gpu/drm/tiny/panel-mipi-dbi.c > new file mode 100644 > index ..9240fdec38d6 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c > @@ -0,0 +1,413 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * DRM driver for MIPI DBI compatible display panels > + * > + * Copyright 2022 Noralf Trønnes > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include
Re: [PATCH v4 2/3] drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
Hi Noralf, On Fri, Feb 18, 2022 at 04:11:09PM +0100, Noralf Trønnes wrote: > devm_drm_dev_alloc() can't allocate structures that embed a structure > which then again embeds drm_device. Workaround this by adding a > driver_private pointer to struct mipi_dbi_dev which the driver can use for > its additional state. > > v3: > - Add documentation > > Acked-by: Maxime Ripard > Signed-off-by: Noralf Trønnes Acked-by: Sam Ravnborg
Re: [PATCH v4 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
Hi Noralf,
On Fri, Feb 18, 2022 at 04:11:08PM +0100, Noralf Trønnes wrote:
> Add binding for MIPI DBI compatible SPI panels.
>
> v4:
> - There should only be two compatible (Maxime)
> - s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible
>
> v3:
> - Move properties to Device Tree (Maxime)
> - Use contains for compatible (Maxime)
> - Add backlight property to example
> - Flesh out description
>
> v2:
> - Fix path for panel-common.yaml
> - Use unevaluatedProperties
> - Drop properties which are in the allOf section
> - Drop model property (Rob)
>
> Acked-by: Maxime Ripard
> Signed-off-by: Noralf Trønnes
> ---
> .../display/panel/panel-mipi-dbi-spi.yaml | 125 ++
> 1 file changed, 125 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> new file mode 100644
> index ..748c09113168
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPI DBI SPI Panel
> +
> +maintainers:
> + - Noralf Trønnes
> +
> +description: |
> + This binding is for display panels using a MIPI DBI compatible controller
> + in SPI mode.
> +
> + The MIPI Alliance Standard for Display Bus Interface defines the electrical
> + and logical interfaces for display controllers historically used in mobile
> + phones. The standard defines 4 display architecture types and this binding
> is
> + for type 1 which has full frame memory. There are 3 interface types in the
> + standard and type C is the serial interface.
> +
> + The standard defines the following interface signals for type C:
> + - Power:
> +- Vdd: Power supply for display module
> +- Vddi: Logic level supply for interface signals
> +Combined into one in this binding called: power-supply
> + - Interface:
> +- CSx: Chip select
> +- SCL: Serial clock
> +- Dout: Serial out
> +- Din: Serial in
> +- SDA: Bidrectional in/out
> +- D/CX: Data/command selection, high=data, low=command
> + Called dc-gpios in this binding.
> +- RESX: Reset when low
> + Called reset-gpios in this binding.
> +
> + The type C interface has 3 options:
> +
> +- Option 1: 9-bit mode and D/CX as the 9th bit
> + | Command | the next command or following
> data |
> +
> |<0>||
> +
> +- Option 2: 16-bit mode and D/CX as a 9th bit
> + | Command or data |
> + ||
> +
> +- Option 3: 8-bit mode and D/CX as a separate interface line
> + |Command or data |
> + ||
> +
> + The panel resolution is specified using the panel-timing node properties
> + hactive (width) and vactive (height). The other mandatory panel-timing
> + properties should be set to zero except clock-frequency which can be
> + optionally set to inform about the actual pixel clock frequency.
> +
> + If the panel is wired to the controller at an offset specify this using
> + hback-porch (x-offset) and vback-porch (y-offset).
Very informative description - well done.
> +
> +allOf:
> + - $ref: panel-common.yaml#
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> +items:
> + - {} # Panel Specific Compatible
> + - const: panel-mipi-dbi-spi
> +
> + write-only:
> +type: boolean
> + description:
> + Controller is not readable (ie. MISO is not wired up).
It would be easier to understand if this comment refers to one of the
pins on the display described above. So maybe something like
(ie. Din (MSIO on the SPI interface) is not wired up).
With the comment updated to include a reference to Din,
Acked-by: Sam Ravnborg
Re: [PATCH 3/3] drm/panel: nt35560: Support also ACX424AKM
Hi Linus, On Mon, Jan 03, 2022 at 12:38:22PM +0100, Linus Walleij wrote: > Add some code and config to also support the ACX424AKM used in > some Sony (Ericsson) Mobile phones. > > Signed-off-by: Linus Walleij Acked-by: Sam Ravnborg
Re: [PATCH 2/3] drm/panel: nt35560: Support more panel IDs
On Mon, Jan 03, 2022 at 12:38:21PM +0100, Linus Walleij wrote:
> These IDs were found in the wild in a Sony Xperia vendor tree.
>
> Signed-off-by: Linus Walleij
> ---
> drivers/gpu/drm/panel/panel-novatek-nt35560.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35560.c
> b/drivers/gpu/drm/panel/panel-novatek-nt35560.c
> index 620876225384..41dc278faf80 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35560.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35560.c
> @@ -32,13 +32,14 @@
> /*
> * Sony seems to use vendor ID 0x81
> */
> -#define DISPLAY_SONY_ACX424AKP_ID1 0x811b
> +#define DISPLAY_SONY_ACX424AKP_ID1 0x8103
> #define DISPLAY_SONY_ACX424AKP_ID2 0x811a
> +#define DISPLAY_SONY_ACX424AKP_ID3 0x811b
> /*
> * The third ID looks like a bug, vendor IDs begin at 0x80
> * and panel 00 ... seems like default values.
> */
This comment needs adjustment s/third/fourth/
With the comment fixed:
Acked-by: Sam Ravnborg
> -#define DISPLAY_SONY_ACX424AKP_ID3 0x8000
> +#define DISPLAY_SONY_ACX424AKP_ID4 0x8000
>
> struct nt35560 {
> struct drm_panel panel;
> @@ -225,6 +226,7 @@ static int nt35560_read_id(struct nt35560 *nt)
> case DISPLAY_SONY_ACX424AKP_ID1:
> case DISPLAY_SONY_ACX424AKP_ID2:
> case DISPLAY_SONY_ACX424AKP_ID3:
> + case DISPLAY_SONY_ACX424AKP_ID4:
> dev_info(nt->dev, "MTP vendor: %02x, version: %02x, panel:
> %02x\n",
>vendor, version, panel);
> break;
> --
> 2.31.1
Re: [PATCH 1/3] drm/panel: Rename Sony ACX424 to Novatek NT35560
On Mon, Jan 03, 2022 at 12:38:20PM +0100, Linus Walleij wrote: > A code drop from Sony Mobile reveals that the ACX424 panels are > built around the Novatek NT35560 panel controllers so just bite > the bullet and rename the driver and all basic symbols so that > we can modify this driver to cover any other panels also using > the Novatek NT35560 display controller. > > Signed-off-by: Linus Walleij Acked-by: Sam Ravnborg
Re: [PATCH 1/3] drm/panel: Rename Sony ACX424 to Novatek NT35560
Hi Linus, On Sat, Feb 19, 2022 at 02:40:33AM +0100, Linus Walleij wrote: > On Sat, Jan 29, 2022 at 2:26 AM Linus Walleij > wrote: > > On Mon, Jan 3, 2022 at 12:40 PM Linus Walleij > > wrote: > > > > > A code drop from Sony Mobile reveals that the ACX424 panels are > > > built around the Novatek NT35560 panel controllers so just bite > > > the bullet and rename the driver and all basic symbols so that > > > we can modify this driver to cover any other panels also using > > > the Novatek NT35560 display controller. > > > > > > Signed-off-by: Linus Walleij > > > > Could someone take mercy in reviewing this patch set? > > > > I can offer some patch review back! > > I can also offer coffee in person in Sweden, I will take you up on this on day - I hope :-) Greetings from Denmark. Sam
Re: [PATCH 20/22] drm/panel: Use drm_mode_duplicate()
Hi Ville,
On Fri, Feb 18, 2022 at 12:04:01PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Replace the hand rolled drm_mode_duplicate() with the
> real thing.
>
> @is_dup@
> @@
> drm_mode_duplicate(...)
> { ... }
>
> @depends on !is_dup@
> expression dev, oldmode;
> identifier newmode;
> @@
> - newmode = drm_mode_create(dev);
> + newmode = drm_mode_duplicate(dev, oldmode);
> ...
> - drm_mode_copy(newmode, oldmode);
>
> Signed-off-by: Ville Syrjälä
> Cc: Thierry Reding
> Cc: Sam Ravnborg
Looks good,
Acked-by: Sam Ravnborg
Re: [PATCH 2/2] fbdev: Improve performance of sys_imageblit()
Hi Thomas,
On Thu, Feb 17, 2022 at 11:34:05AM +0100, Thomas Zimmermann wrote:
> Improve the performance of sys_imageblit() by manually unrolling
> the inner blitting loop and moving some invariants out. The compiler
> failed to do this automatically. The resulting binary code was even
> slower than the cfb_imageblit() helper, which uses the same algorithm,
> but operates on I/O memory.
It would be super to have the same optimization done to cfb_imageblit(),
to prevent that the two codebases diverge more than necessary.
Also I think cfb_ version would also see a performance gain from this.
The actual implementation looks good.
So with or without the extra un-rolling the patch is:
Acked-by: Sam Ravnborg
One small nit belwo.
Sam
>
> A microbenchmark measures the average number of CPU cycles
> for sys_imageblit() after a stabilizing period of a few minutes
> (i7-4790, FullHD, simpledrm, kernel with debugging). The value
> for CFB is given as a reference.
>
> sys_imageblit(), new: 25934 cycles
> sys_imageblit(), old: 35944 cycles
> cfb_imageblit(): 30566 cycles
>
> In the optimized case, sys_imageblit() is now ~30% faster than before
> and ~20% faster than cfb_imageblit().
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/fbdev/core/sysimgblt.c | 51 +---
> 1 file changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/sysimgblt.c
> b/drivers/video/fbdev/core/sysimgblt.c
> index a4d05b1b17d7..d70d65af6fcb 100644
> --- a/drivers/video/fbdev/core/sysimgblt.c
> +++ b/drivers/video/fbdev/core/sysimgblt.c
> @@ -188,23 +188,32 @@ static void fast_imageblit(const struct fb_image
> *image, struct fb_info *p,
> {
> u32 fgx = fgcolor, bgx = bgcolor, bpp = p->var.bits_per_pixel;
> u32 ppw = 32/bpp, spitch = (image->width + 7)/8;
> - u32 bit_mask, end_mask, eorx, shift;
> + u32 bit_mask, eorx;
> const char *s = image->data, *src;
> u32 *dst;
> - const u32 *tab = NULL;
> - int i, j, k;
> + const u32 *tab;
> + size_t tablen;
> + u32 colortab[16];
> + int i, j, k, jdecr;
> +
> + if ((uintptr_t)dst1 % 8)
> + return;
This check is new - and should not trigger ever. Maybe add an unlikely
and a WARN_ON_ONCE()?
>
> switch (bpp) {
> case 8:
> tab = fb_be_math(p) ? cfb_tab8_be : cfb_tab8_le;
> + tablen = 16;
> break;
> case 16:
> tab = fb_be_math(p) ? cfb_tab16_be : cfb_tab16_le;
> + tablen = 4;
> break;
> case 32:
> - default:
> tab = cfb_tab32;
> + tablen = 2;
> break;
> + default:
> + return;
> }
>
> for (i = ppw-1; i--; ) {
> @@ -217,19 +226,37 @@ static void fast_imageblit(const struct fb_image
> *image, struct fb_info *p,
> bit_mask = (1 << ppw) - 1;
> eorx = fgx ^ bgx;
> k = image->width/ppw;
> + jdecr = 8 / ppw;
> +
> + for (i = 0; i < tablen; ++i)
> + colortab[i] = (tab[i] & eorx) ^ bgx;
This code could have been embedded with the switch (bpp) {
That would have made some sense I think.
But both ways works, so this was just a small observation.
Sam
Re: [PATCH 1/2] fbdev: Improve performance of sys_fillrect()
Hi Thomas,
On Thu, Feb 17, 2022 at 11:34:04AM +0100, Thomas Zimmermann wrote:
> Improve the performance of sys_fillrect() by using word-aligned
> 32/64-bit mov instructions. While the code tried to implement this,
> the compiler failed to create fast instructions. The resulting
> binary instructions were even slower than cfb_fillrect(), which
> uses the same algorithm, but operates on I/O memory.
>
> A microbenchmark measures the average number of CPU cycles
> for sys_fillrect() after a stabilizing period of a few minutes
> (i7-4790, FullHD, simpledrm, kernel with debugging). The value
> for CFB is given as a reference.
>
> sys_fillrect(), new: 26586 cycles
> sys_fillrect(), old: 166603 cycles
> cfb_fillrect(): 41012 cycles
>
> In the optimized case, sys_fillrect() is now ~6x faster than before
> and ~1.5x faster than the CFB implementation.
>
> Signed-off-by: Thomas Zimmermann
Nice optimization.
Reviewed-by: Sam Ravnborg
> ---
> drivers/video/fbdev/core/sysfillrect.c | 16 +++-
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/sysfillrect.c
> b/drivers/video/fbdev/core/sysfillrect.c
> index 33ee3d34f9d2..bcdcaeae6538 100644
> --- a/drivers/video/fbdev/core/sysfillrect.c
> +++ b/drivers/video/fbdev/core/sysfillrect.c
> @@ -50,19 +50,9 @@ bitfill_aligned(struct fb_info *p, unsigned long *dst, int
> dst_idx,
>
> /* Main chunk */
> n /= bits;
> - while (n >= 8) {
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - *dst++ = pat;
> - n -= 8;
> - }
> - while (n--)
> - *dst++ = pat;
> + memset_l(dst, pat, n);
> + dst += n;
> +
> /* Trailing bits */
> if (last)
> *dst = comp(pat, *dst, last);
> --
> 2.34.1
Re: [PATCH 0/8] drm: Add support for low-color frame buffer formats
Hi Geert, On Tue, Feb 15, 2022 at 05:52:18PM +0100, Geert Uytterhoeven wrote: > Hi all, > > A long outstanding issue with the DRM subsystem has been the lack of > support for low-color displays, as used typically on older desktop > systems and small embedded displays. This is one of the pieces missing for a long time - great to see something done here. Thanks Geert! Sam
Re: [PATCH 8/8] drm/fourcc: Add DRM_FORMAT_D1
Hi Geert, > > C1 is color-indexed, which can be any two colors. > R1 is light-on-dark. > D1 is dark-on-light. It would be nice to have this explained in the fourcc.h file, preferably with a little more verbosity than the current explanations. Sam
Re: [PATCH 2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]
Hi Geert,
> > > + switch (fb->format->depth) {
> >
> > The depth field is deprecated. It's probably better to use
> > fb->format->format and test against 4CC codes.
>
> The reason I checked for depth instead of a 4CC code is that the only
> thing that matters here is the number of bits per pixel. Hence this
> function won't need any changes to support R1, R2, R4, and D1 later.
> When we get here, we already know that we are using a format that
> is supported by the fbdev helper code, and thus passed the 4CC
> checks elsewhere.
>
> Alternatively, we could introduce drm_format_info_bpp() earlier in
> the series, and use that?
The drm_format_info_bpp() is very descriptive, so yes it would be good to use
it here also.
Sam
Re: [PATCH] drm/mode: Improve drm_mode_fb_cmd2 documentation
On Wed, Feb 16, 2022 at 09:41:36AM +0100, Geert Uytterhoeven wrote: > Fix various grammar mistakes in the kerneldoc comments documenting the > drm_mode_fb_cmd2 structure: > - s/is/are/, > - s/8 bit/8-bit/. > > Signed-off-by: Geert Uytterhoeven Acked-by: Sam Ravnborg
Re: [PATCH] drm/fb: Improve drm_framebuffer.offsets documentation
Hi Geert, On Wed, Feb 16, 2022 at 09:41:06AM +0100, Geert Uytterhoeven wrote: > Fix various spelling and grammar mistakes in the kerneldoc comments > documenting the offsets member in the drm_framebuffer structure: > - s/laytou/layout/, > - Add missing "is", > - s/it/its/. > > Signed-off-by: Geert Uytterhoeven Acked-by: Sam Ravnborg It would be great if you could get drm-misc commiter rights, then you can push this and all your other nice patches yourself. Especially since you are soon becoming a drm driver maintainer Sam
Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro
Hi Laurent, On Tue, Feb 08, 2022 at 02:30:42AM +0200, Laurent Pinchart wrote: > Hi Sam, > > Thank you for the patch. > > On Sun, Feb 06, 2022 at 04:43:57PM +0100, Sam Ravnborg wrote: > > The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that > > do not subclass drm_bridge_state to assign the default operations for > > reset, duplicate and destroy of the state. > > > > Signed-off-by: Sam Ravnborg > > Cc: Douglas Anderson > > Cc: Andrzej Hajda > > Cc: Neil Armstrong > > Cc: Robert Foss > > Cc: Laurent Pinchart > > Cc: Jonas Karlman > > Cc: Jernej Skrabec > > Cc: Maxime Ripard > > Cc: Thomas Zimmermann > > --- > > include/drm/drm_bridge.h | 12 > > 1 file changed, 12 insertions(+) > > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index 061d87313fac..fc00304be643 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -935,4 +935,16 @@ static inline struct drm_bridge > > *devm_drm_of_get_bridge(struct device *dev, > > } > > #endif > > > > +/** > > + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs > > I'd name the macro DRM_BRIDGE_STATE_DEFAULT_OPS. OK, done. > > > + * > > + * Bridge driver that do not subclass _bridge_state can use the helpers > > + * for reset, duplicate, and destroy. This macro provides a shortcut for > > + * setting the helpers in the _bridge_funcs structure. > > + */ > > +#define DRM_BRIDGE_STATE_OPS \ > > + .atomic_reset = drm_atomic_helper_bridge_reset, > > \ > > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > > \ > > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state > > I'm not a big fan of such macros for a small number of operations, as I > find that it obfuscates the code a bit, but that could change once I get > used to the new macro :-) The use of the macro is a nice signal that all the relevant default operations are specified - no need to look up if all are included. I have on my TODO to update all relevant bridge drivers to use it. > > Reviewed-by: Laurent Pinchart > > > + > > #endif > > -- > Regards, > > Laurent Pinchart
Re: [PATCH 2/2] fbdev: Don't sort deferred-I/O pages by default
u);
> + } else {
> + list_add_tail(>lru, >pagelist);
> }
Bikeshedding - my personal style is to have the likely part first.
This makes reading the code easier.
The following drivers uses deferred io but are not listed as
they need the page list sorted:
- hecubafb
- hyperv_fb
- sh_mobile_lcdcfb
- smscufx
- ssd1307fb
- xen-fbfront
It would be nice with some info in the commit log that they do not need
the pages sorted.
To make the list complete include the drm stuff too.
It did not jump to me why they did not need sorted pages,
so some sort of reassurance that they have been checked would be nice.
With the following addressed:
Acked-by: Sam Ravnborg
I hope someone else looks that can verify that the list of drivers
without sort_pagelist is correct so someone knowledgeable have looked
too.
Sam
Re: [PATCH 1/2] fbdev/defio: Early-out if page is already enlisted
Hi Thomas,
On Thu, Feb 10, 2022 at 03:11:11PM +0100, Thomas Zimmermann wrote:
> Return early if a page is already in the list of dirty pages for
> deferred I/O. This can be detected if the page's list head is not
> empty. Keep the list head initialized while the page is not enlisted
> to make this work reliably.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/fbdev/core/fb_defio.c | 21 -
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c
> b/drivers/video/fbdev/core/fb_defio.c
> index a591d291b231..3727b1ca87b1 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -59,6 +59,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> printk(KERN_ERR "no mapping available\n");
>
> BUG_ON(!page->mapping);
> + INIT_LIST_HEAD(>lru);
> page->index = vmf->pgoff;
>
> vmf->page = page;
> @@ -122,17 +123,19 @@ static vm_fault_t fb_deferred_io_mkwrite(struct
> vm_fault *vmf)
>*/
> lock_page(page);
>
> + /*
> + * This check is to catch the case where a new process could start
> + * writing to the same page through a new pte. this new access
> + * can cause the mkwrite even when the original ps's pte is marked
> + * writable.
> + */
When moving this comment it would be prudent to also fix the wording a
bit.
- Capital in start of sentence and after a period
- Spell out process and do not shorten ps
> + if (!list_empty(>lru))
> + goto page_already_added;
> +
This check says that if the page already has something in the parge->lru
then this is added by defio and thus is already added.
This matches your commit description - OK.
Maybe add something like:
* Pages added will have their lru set, and it is clered again in the
* deferred work handler.
> /* we loop through the pagelist before adding in order
> to keep the pagelist sorted */
> list_for_each_entry(cur, >pagelist, lru) {
> - /* this check is to catch the case where a new
> - process could start writing to the same page
> - through a new pte. this new access can cause the
> - mkwrite even when the original ps's pte is marked
> - writable */
> - if (unlikely(cur == page))
> - goto page_already_added;
> - else if (cur->index > page->index)
> + if (cur->index > page->index)
> break;
> }
>
> @@ -194,7 +197,7 @@ static void fb_deferred_io_work(struct work_struct *work)
>
> /* clear the list */
> list_for_each_safe(node, next, >pagelist) {
> - list_del(node);
> + list_del_init(node);
> }
> mutex_unlock(>lock);
> }
With the comment adjusted as you see fit
Acked-by: Sam Ravnborg
Re: [PATCH] fbcon: use min() to make code cleaner
On Wed, Feb 09, 2022 at 08:48:10AM +, cgel@gmail.com wrote:
> From: Changcheng Deng
>
> Use min() in order to make code cleaner.
>
> Reported-by: Zeal Robot
> Signed-off-by: Changcheng Deng
Reviewed-by: Sam Ravnborg
I had preferred in minmax.h was included, but it has an indirect include
so shrug.
I assume Daniel or Helge will pick it up.
Sam
> ---
> drivers/video/fbdev/core/fbcon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c
> b/drivers/video/fbdev/core/fbcon.c
> index f36829eeb5a9..61171159fee2 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -602,7 +602,7 @@ static void fbcon_prepare_logo(struct vc_data *vc, struct
> fb_info *info,
> save = kmalloc(array3_size(logo_lines, new_cols, 2),
> GFP_KERNEL);
> if (save) {
> - int i = cols < new_cols ? cols : new_cols;
> + int i = min(cols, new_cols);
> scr_memsetw(save, erase, array3_size(logo_lines,
> new_cols, 2));
> r = q - step;
> for (cnt = 0; cnt < logo_lines; cnt++, r += i)
> --
> 2.25.1
Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Hi Douglas,
On Mon, Feb 07, 2022 at 02:34:34PM -0800, Doug Anderson wrote:
> Hi,
>
> On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg wrote:
> >
> > From: Rob Clark
> >
> > Slightly awkward to fish out the display_info when we aren't creating
> > own connector. But I don't see an obvious better way.
> >
> > v3:
> > - Rebased and dropped the ti_sn_bridge_get_bpp() patch
> >as this was solved in a different way (Sam)
> >
> > v2:
> > - Remove error return with NO_CONNECTOR flag (Rob)
> >
> > Signed-off-by: Rob Clark
> > Signed-off-by: Sam Ravnborg
> > Cc: Rob Clark
> > Cc: Douglas Anderson
> > Cc: Andrzej Hajda
> > Cc: Neil Armstrong
> > Cc: Robert Foss
> > Cc: Laurent Pinchart
> > Cc: Jonas Karlman
> > Cc: Jernej Skrabec
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +---
> > 1 file changed, 9 insertions(+), 11 deletions(-)
>
> This is fine by me assuming we can fix the previous patches.
>
> Reviewed-by: Douglas Anderson
>
> NOTE: to me, this isn't something to do _instead_ of my patch [1] but
> _in addition_ to it. ${SUBJECT} patch transitions us to a more modern
> approach of having the connector created elsewhere but doesn't remove
> the old fallback code. Might as well clean the fallback code up unless
> you think it's going to simply be deleted right away or something?
Until we know all users have NO_CONNECTOR support we need the connector
creation code.
Please just go ahead with your patches as you already got r-b from
Laurent. For the debugfs patch I will look at it soonish unless someone
beats me.
Sam
>
> [1]
> https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid
Re: [PATCH v1 5/9] drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state
On Mon, Feb 07, 2022 at 02:34:10PM -0800, Doug Anderson wrote:
> Hi,
>
> On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg wrote:
> >
> > To prepare for DRM_BRIDGE_ATTACH_NO_CONNECTOR support,
> > fix so the bpc is found using the output format.
> >
> > This avoids the use of the connector stored in the private data.
> >
> > Signed-off-by: Sam Ravnborg
> > Cc: Douglas Anderson
> > Cc: Andrzej Hajda
> > Cc: Neil Armstrong
> > Cc: Robert Foss
> > Cc: Laurent Pinchart
> > Cc: Jonas Karlman
> > Cc: Jernej Skrabec
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++--
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index d681ab68205c..dc6ec40bc1ef 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -33,6 +33,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #define SN_DEVICE_REV_REG 0x08
> > #define SN_DPPLL_SRC_REG 0x0A
> > @@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct
> > ti_sn65dsi86 *pdata)
> > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > }
> >
> > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> > +static unsigned int ti_sn_bridge_get_bpp(struct drm_bridge_state
> > *bridge_state)
> > {
> > - if (pdata->connector.display_info.bpc <= 6)
> > + int bpc =
> > media_bus_format_to_bpc(bridge_state->output_bus_cfg.format);
> > +
> > + if (bpc <= 6)
> > return 18;
> > else
> > return 24;
>
> Unfortunately this doesn't work as hoped. :(
> `bridge_state->output_bus_cfg.format` is 0 in my testing...
>
> ...and then media_bus_format_to_bpc() returns an error, which is
> negative and <= 6. That's not super ideal...
>
> I guess this gets down to what the output bus format is _supposed_ to
> be for eDP. Based on my understanding of eDP there isn't really a
> concept of a fixed "bus format" that the panel ought to work at. There
> is a concept of the number of bits per component (6, 8, 10, 12) that a
> panel can run at but then after that I believe the format is standard,
> or at least it's something that's dynamic / negotiated. Also note that
> the format on the bus is more related to the current mode we're
> running the panel in than some inherent property of the panel. For
> instance, a 10 bpc panel can likely have data provided in 8 bpc and 6
> bpc. I've also seen 6 bpc panels that can accept 8 bpc data and can
> dither it. In any case, this type of stuff is really all dynamic for
> eDP. The old value we were looking at was really being interpreted as
> the "max" bpc that the panel could run in and we'd choose to run the
> panel in 8 bpc if the panel supported it and 6 bpc otherwise (this
> bridge chip only supports 6bpc or 8bpc).
>
> So I guess the question is: how do we move forward here?
I skipped a patch to find the connector - will try to give that a spin
again.
Thanks for the testing and the feedback!
Sam
>
> Do we need to somehow figure out how to get "output_bus_cfg.format"
> filled in? Any suggestions for how to do that? Do we just implement
> atomic_get_output_bus_fmts() and then pick one of
> MEDIA_BUS_FMT_RGB666_1X18 or MEDIA_BUS_FMT_RBG888_1X24 based on the
> bpc in the connector state? ...or do we just list both of them and
> something magically will pick the right one? Hrm, I tried that and it
> didn't magically work, but I didn't debug further...
>
> One thing I realized is that, at least in theory, we might not know
> whether we want to run in 6 bpc or 8 bpc until we've done link
> training. It's at least somewhat plausible that there could be edge
> cases where we'd want to fall back to the low bpc if link training
> failed at the higher bpc. The driver doesn't support that right now,
> but ideally the design wouldn't preclude it. At the moment link
> training happens in enable. Maybe that's a problem to worry about
> another day, though?
>
> -Doug
Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
On Tue, Feb 08, 2022 at 02:44:25AM +0200, Laurent Pinchart wrote:
> Hello,
>
> On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
> > On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg wrote:
> > >
> > > There is a number of bridge drivers that supports a single media bus
> > > format for DSI. Add a helper to avoid duplicating the code.
> > >
> > > Signed-off-by: Sam Ravnborg
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 41 +
> > > include/drm/drm_atomic_helper.h | 7 +
> > > 2 files changed, 48 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index a7a05e1e26bb..94f313dc196f 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct
> > > drm_bridge *bridge,
> > > return input_fmts;
> > > }
> > > EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
> > > +
> > > +/**
> > > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output
> > > format
> >
> > Is the description right? It's called "input" format but it defines an
> > output format?
> >
> >
> > > + * @bridge: bridge control structure
> > > + * @bridge_state: new bridge state
> > > + * @crtc_state: new CRTC state
> > > + * @conn_state: new connector state
> > > + * @output_fmt: tested output bus format
> > > + * @num_input_fmts: will contain the size of the returned array
> >
> > Maybe indicate that it's always 1 in the comments?
> >
> > > + *
> > > + * This helper is an implementation of the
> > > + * _bridge_funcs.atomic_get_input_bus_fmts operation for bridges
> > > that supports
> > > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
> > > + *
> > > + * RETURNS
> >
> > kernel-doc can't parse this return syntax and warns:
> >
> > warning: No description found for return value of
> > 'drm_atomic_helper_bridge_dsi_input_bus_fmt'
> >
> >
> > > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
> > > + * or NULL if the allocation failed
> > > + */
> > > +u32 *
> > > +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
> > > + struct drm_bridge_state
> > > *bridge_state,
> > > + struct drm_crtc_state
> > > *crtc_state,
> > > + struct drm_connector_state
> > > *conn_state,
> > > + u32 output_fmt,
> > > + unsigned int *num_input_fmts)
> > > +{
> > > + u32 *input_fmts;
> > > +
> > > + *num_input_fmts = 0;
> > > +
> > > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> > > GFP_KERNEL);
> >
> > I probably wouldn't have bothered with `kcalloc` for something that's
> > always just one value and you're setting it. Why not just
> >
> > input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
> >
> > ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you
> > said it didn't compile?
> >
> > Also: if it was common for others to want to provide fixed formats, I
> > wonder about adding a helper function that did most of the work here?
> > Dunno what it would be named since it's already a bit a of handful,
> > but I'd expect to call it like:
> >
> > static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 };
> > return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
> >
> > Then my_neat_helper() could do kmemdup() on the array passed and fill
> > in "num_output_formats" to be either the array size of 0 (if the
> > kmemdup failed).
>
> I quite like that approach. We could even have a wrapper macro that adds
> the ARRAY_SIZE() argument automatically.
Agree, will try to give that a spin.
And will then also process all the nice feedback from Douglas.
Sam
Re: [PATCH 21/21] fbdev: Make registered_fb[] private to fbmem.c
Hi Daniel,
On Mon, Jan 31, 2022 at 10:05:52PM +0100, Daniel Vetter wrote:
> Well except when the olpc dcon fbdev driver is enabled, that thing
> digs around in there in rather unfixable ways.
>
> Cc oldc_dcon maintainers as fyi.
>
> Cc: Jens Frederich
> Cc: Jon Nettleton
> Cc: Greg Kroah-Hartman
> Cc: linux-stag...@lists.linux.dev
> Signed-off-by: Daniel Vetter
> Cc: Daniel Vetter
> Cc: Helge Deller
> Cc: Matthew Wilcox
> Cc: Sam Ravnborg
> Cc: Tetsuo Handa
> Cc: Zhen Lei
> Cc: Alex Deucher
> Cc: Xiyu Yang
> Cc: linux-fb...@vger.kernel.org
> Cc: Zheyu Ma
> Cc: Guenter Roeck
with the build thingy fixed:
Acked-by: Sam Ravnborg
I do wonder if there is a more clean way to trigger a blank
in the main fbdev driver from the olpc driver.
The current hack is not nice and it would be good to see it gone.
Sam
> ---
> drivers/video/fbdev/core/fbmem.c | 8 ++--
> include/linux/fb.h | 7 +++
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 904ef1250677..dad6572942fa 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -48,10 +48,14 @@
> static DEFINE_MUTEX(registration_lock);
>
> struct fb_info *registered_fb[FB_MAX] __read_mostly;
> -EXPORT_SYMBOL(registered_fb);
> -
> int num_registered_fb __read_mostly;
> +#if IS_ENABLED(CONFIG_OLPC_DCON)
> +EXPORT_SYMBOL(registered_fb);
> EXPORT_SYMBOL(num_registered_fb);
> +#endif
> +#define for_each_registered_fb(i)\
> + for (i = 0; i < FB_MAX; i++)\
> + if (!registered_fb[i]) {} else
>
> bool fb_center_logo __read_mostly;
>
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index a8a00d2ba1f3..e236817502c2 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -622,16 +622,15 @@ extern int fb_get_color_depth(struct fb_var_screeninfo
> *var,
> extern int fb_get_options(const char *name, char **option);
> extern int fb_new_modelist(struct fb_info *info);
>
> +#if IS_ENABLED(CONFIG_OLPC_DCON)
> extern struct fb_info *registered_fb[FB_MAX];
> +
> extern int num_registered_fb;
> +#endif
> extern bool fb_center_logo;
> extern int fb_logo_count;
> extern struct class *fb_class;
>
> -#define for_each_registered_fb(i)\
> - for (i = 0; i < FB_MAX; i++)\
> - if (!registered_fb[i]) {} else
> -
> static inline void lock_fb_info(struct fb_info *info)
> {
> mutex_lock(>lock);
> --
> 2.33.0
Re: [PATCH 19/21] fbcon: Maintain a private array of fb_info
Hi Daniel, On Tue, Feb 08, 2022 at 03:03:28PM +0100, Daniel Vetter wrote: > On Fri, Feb 04, 2022 at 09:15:40PM +0100, Sam Ravnborg wrote: > > Hi Daniel, > > > > On Mon, Jan 31, 2022 at 10:05:50PM +0100, Daniel Vetter wrote: > > > Accessing the one in fbmem.c without taking the right locks is a bad > > > idea. Instead maintain our own private copy, which is fully protected > > > by console_lock() (like everything else in fbcon.c). That copy is > > > serialized through fbcon_fb_registered/unregistered() calls. > > > > I fail to see why we can make a private copy of registered_fb > > just like that - are they not somehow shared between fbcon and fbmem. > > So when fbmem updates it, then fbcon will use the entry or such? > > > > I guess I am just ignorant of how registered_fb is used - but please > > explain. > > The private copy is protected under console_lock, and hence safe to access > from fbcon.c code. > > The main registered_fb array is protected by a different mutex, so we > could indeed end up with hilarious corruption because the value is > inconsistent while we try to access it (e.g. we check for !NULL, but later > on gcc decides to reload the value and now it's suddenly become NULL and > we blow up). > > The two are synchronized by fbmem.c calling fbcon_register/unregister, so > aside from the different locks if there's no race going on, they will > always be identical. IT was this part that I missed, and it is already spelled out in the commit message. > > Other option would be to roll out get_fb_info() to fbcon.c, but since > fbcon.c is fully protected by console_lock that would add complexity in > the code flow that we don't really need. And we'd have to wire fb_info > through all call chains, since the right way to use get_fb_info is to look > it up once and then only drop it when your callback has finished. > > Since the current code just assume it's all protected by console_lock and > we never drop that during a callback this would mean major surgery and > essentially refactoring all of fbcon.c to only access the fbcon stuff > through fb_info, i.e. to get rid of _all_ the global arrays we have more > or less. I'm not volunteering for that (despite that really this would be > the right thing to do if we'd have infinite engineering time). > > Ack with that explainer added to the commit message? I consider the current commit message fine - it helps when you actually read it. Acked-by: Sam Ravnborg
Re: [PATCH 18/21] fbcon: untangle fbcon_exit
On Tue, Feb 08, 2022 at 02:58:21PM +0100, Daniel Vetter wrote:
> On Fri, Feb 04, 2022 at 09:06:38PM +0100, Sam Ravnborg wrote:
> > Hi Daniel,
> >
> > On Mon, Jan 31, 2022 at 10:05:49PM +0100, Daniel Vetter wrote:
> > > There's a bunch of confusions going on here:
> > > - The deferred fbcon setup notifier should only be cleaned up from
> > > fb_console_exit(), to be symmetric with fb_console_init()
> > > - We also need to make sure we don't race with the work, which means
> > > temporarily dropping the console lock (or we can deadlock)
> > > - That also means no point in clearing deferred_takeover, we are
> > > unloading everything anyway.
> > > - Finally rename fbcon_exit to fbcon_release_all and move it, since
> > > that's what's it doing when being called from consw->con_deinit
> > > through fbcon_deinit.
> > >
> > > Signed-off-by: Daniel Vetter
> > > Cc: Daniel Vetter
> > > Cc: Greg Kroah-Hartman
> > > Cc: Claudio Suarez
> > > Cc: Du Cheng
> > > Cc: Tetsuo Handa
> > > ---
> > > drivers/video/fbdev/core/fbcon.c | 63
> > > 1 file changed, 32 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/core/fbcon.c
> > > b/drivers/video/fbdev/core/fbcon.c
> > > index 5c14e24d14a1..22581952b4fd 100644
> > > --- a/drivers/video/fbdev/core/fbcon.c
> > > +++ b/drivers/video/fbdev/core/fbcon.c
> > > @@ -185,7 +185,6 @@ static void fbcon_set_disp(struct fb_info *info,
> > > struct fb_var_screeninfo *var,
> > > int unit);
> > > static void fbcon_modechanged(struct fb_info *info);
> > > static void fbcon_set_all_vcs(struct fb_info *info);
> > > -static void fbcon_exit(void);
> > >
> > > static struct device *fbcon_device;
> > >
> > > @@ -1149,6 +1148,27 @@ static void fbcon_free_font(struct fbcon_display
> > > *p, bool freefont)
> > >
> > > static void set_vc_hi_font(struct vc_data *vc, bool set);
> > >
> > > +static void fbcon_release_all(void)
> > > +{
> > > + struct fb_info *info;
> > > + int i, j, mapped;
> > > +
> > > + for_each_registered_fb(i) {
> > > + mapped = 0;
> > > + info = registered_fb[i];
> > > +
> > > + for (j = first_fb_vc; j <= last_fb_vc; j++) {
> > > + if (con2fb_map[j] == i) {
> > > + mapped = 1;
> > > + con2fb_map[j] = -1;
> > > + }
> > > + }
> > > +
> > > + if (mapped)
> > > + fbcon_release(info);
> > > + }
> > > +}
> > > +
> > > static void fbcon_deinit(struct vc_data *vc)
> > > {
> > > struct fbcon_display *p = _display[vc->vc_num];
> > > @@ -1188,7 +1208,7 @@ static void fbcon_deinit(struct vc_data *vc)
> > > set_vc_hi_font(vc, false);
> > >
> > > if (!con_is_bound(_con))
> > > - fbcon_exit();
> > > + fbcon_release_all();
> > >
> > > if (vc->vc_num == logo_shown)
> > > logo_shown = FBCON_LOGO_CANSHOW;
> > > @@ -3316,34 +3336,6 @@ static void fbcon_start(void)
> > > #endif
> > > }
> > >
> > > -static void fbcon_exit(void)
> > > -{
> > > - struct fb_info *info;
> > > - int i, j, mapped;
> > > -
> > > -#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> > > - if (deferred_takeover) {
> > > - dummycon_unregister_output_notifier(_output_nb);
> > > - deferred_takeover = false;
> > > - }
> > > -#endif
> > > -
> > > - for_each_registered_fb(i) {
> > > - mapped = 0;
> > > - info = registered_fb[i];
> > > -
> > > - for (j = first_fb_vc; j <= last_fb_vc; j++) {
> > > - if (con2fb_map[j] == i) {
> > > - mapped = 1;
> > > - con2fb_map[j] = -1;
> > > - }
> > > - }
> > > -
> > > - if (mapped)
> > > - fbcon_release(info);
> > > - }
> > > -}
> > > -
> > > void __init fb_console_init(void)
> > > {
> > > int i;
> > > @@ -3383,10 +3375,19 @@ static void __exit fbcon_
Re: [PATCH 13/21] fbcon: move more common code into fb_open()
On Tue, Feb 08, 2022 at 02:53:59PM +0100, Daniel Vetter wrote:
> On Fri, Feb 04, 2022 at 08:35:31PM +0100, Sam Ravnborg wrote:
> > On Mon, Jan 31, 2022 at 10:05:44PM +0100, Daniel Vetter wrote:
> > > No idea why con2fb_acquire_newinfo() initializes much less than
> > > fbcon_startup(), but so be it. From a quick look most of the
> > > un-initialized stuff should be fairly harmless, but who knows.
> > >
> > > Signed-off-by: Daniel Vetter
> > > Cc: Daniel Vetter
> > > Cc: Greg Kroah-Hartman
> > > Cc: Tetsuo Handa
> > > Cc: Thomas Zimmermann
> > > Cc: Claudio Suarez
> > > Cc: Du Cheng
> > > ---
> > > drivers/video/fbdev/core/fbcon.c | 74 +---
> > > 1 file changed, 31 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/core/fbcon.c
> > > b/drivers/video/fbdev/core/fbcon.c
> > > index b83a5a77d8a8..5a3391ff038d 100644
> > > --- a/drivers/video/fbdev/core/fbcon.c
> > > +++ b/drivers/video/fbdev/core/fbcon.c
> > > @@ -680,8 +680,18 @@ static int fbcon_invalid_charcount(struct fb_info
> > > *info, unsigned charcount)
> > >
> > > #endif /* CONFIG_MISC_TILEBLITTING */
> > >
> > > +static void fbcon_release(struct fb_info *info)
> > > +{
> > > + if (info->fbops->fb_release)
> > > + info->fbops->fb_release(info, 0);
> > > +
> > > + module_put(info->fbops->owner);
> > > +}
> > > +
> > > static int fbcon_open(struct fb_info *info)
> > > {
> > > + struct fbcon_ops *ops;
> > > +
> > > if (!try_module_get(info->fbops->owner))
> > > return -ENODEV;
> > >
> > > @@ -691,19 +701,22 @@ static int fbcon_open(struct fb_info *info)
> > > return -ENODEV;
> > > }
> > >
> > > - return 0;
> > > -}
> > > + ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> > > + if (!ops) {
> > > + fbcon_release(info);
> > > + return -ENOMEM;
> > > + }
> > >
> > > -static void fbcon_release(struct fb_info *info)
> > > -{
> > > - if (info->fbops->fb_release)
> > > - info->fbops->fb_release(info, 0);
> > > + INIT_DELAYED_WORK(>cursor_work, fb_flashcursor);
> > > + ops->info = info;
> > > + info->fbcon_par = ops;
> > > + ops->cur_blink_jiffies = HZ / 5;
> > >
> > > - module_put(info->fbops->owner);
> > > + return 0;
> > > }
> > >
> > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info
> > > *info,
> > > - int unit, int oldidx)
> > > + int unit)
> > > {
> > > struct fbcon_ops *ops = NULL;
> > > int err;
> > > @@ -712,27 +725,10 @@ static int con2fb_acquire_newinfo(struct vc_data
> > > *vc, struct fb_info *info,
> > > if (err)
> > > return err;
> > >
> > > - if (!err) {
> > > - ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> > > - if (!ops)
> > > - err = -ENOMEM;
> > > -
> > > - INIT_DELAYED_WORK(>cursor_work, fb_flashcursor);
> > > - }
> > > -
> > > - if (!err) {
> > > - ops->cur_blink_jiffies = HZ / 5;
> > > - ops->info = info;
> > > - info->fbcon_par = ops;
> > > -
> > > - if (vc)
> > > - set_blitting_type(vc, info);
> > > - }
> > > + ops = info->fbcon_par;
> > >
> > > - if (err) {
> > > - con2fb_map[unit] = oldidx;
> > > - fbcon_release(info);
> > > - }
> > > + if (vc)
> > > + set_blitting_type(vc, info);
> > >
> > > return err;
> > > }
> > > @@ -840,9 +836,11 @@ static int set_con2fb_map(int unit, int newidx, int
> > > user)
> > >
> > > found = search_fb_in_map(newidx);
> > >
> > > - con2fb_map[unit] = newidx;
> > > - if (!err && !found)
> > > - err = con2fb_acquire_newinfo(vc, info, unit, oldidx);
> > > + if (!err && !found) {
> > > + err = con2fb_acquire_newinfo(vc, info, unit);
> > > + if (!err)
> > > + con2fb_map[unit] = newidx;
> > > + }
> > This looks like an unintentional change of functionality as
> > con2fb_map[unit] is
> > only assigned when we do a con2fb_acquire_newinfo().
> >
> > Staring at the code I could not say it is wrong, but not nice to hide
> > the change in this patch.
>
> Nope, it's not an unintentional bugfix. The old con2fb_acquire_newinfo did
> reset con2fb_map to oldidx upon failure, which I've found to be a most
> bizarre calling convention. So this sorts this out.
>
> The reason I smashed this into the same patch is that I had to remove the
> fbcon_release call, and so the error handling in there looked even more
> funny. But I indeed failed to explain this all in the commit message.
>
> Ack with that explainer, or do you want me to split this out properly?
Please update the commit message, then this patch has my:
Acked-by: Sam Ravnborg
Re: [PATCH 11/21] fbcon: Extract fbcon_open/release helpers
Hi Daniel,
On Tue, Feb 08, 2022 at 02:48:29PM +0100, Daniel Vetter wrote:
> On Thu, Feb 03, 2022 at 10:15:14PM +0100, Sam Ravnborg wrote:
> > Hi Daniel,
> >
> > On Mon, Jan 31, 2022 at 10:05:42PM +0100, Daniel Vetter wrote:
> > > There's two minor behaviour changes in here:
> > > - in error paths we now consistently call fb_ops->fb_release
> > > - fb_release really can't fail (fbmem.c ignores it too) and there's no
> > > reasonable cleanup we can do anyway.
> > >
> > > Signed-off-by: Daniel Vetter
> > > Cc: Daniel Vetter
> > > Cc: Claudio Suarez
> > > Cc: Greg Kroah-Hartman
> > > Cc: Tetsuo Handa
> > > Cc: Du Cheng
> > > ---
> > > drivers/video/fbdev/core/fbcon.c | 107 +++
> > > 1 file changed, 53 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/drivers/video/fbdev/core/fbcon.c
> > > b/drivers/video/fbdev/core/fbcon.c
> > > index fa30e1909164..eea2ee14b64c 100644
> > > --- a/drivers/video/fbdev/core/fbcon.c
> > > +++ b/drivers/video/fbdev/core/fbcon.c
> > > @@ -680,19 +680,37 @@ static int fbcon_invalid_charcount(struct fb_info
> > > *info, unsigned charcount)
> > >
> > > #endif /* CONFIG_MISC_TILEBLITTING */
> > >
> > > +static int fbcon_open(struct fb_info *info)
> > > +{
> > > + if (!try_module_get(info->fbops->owner))
> > > + return -ENODEV;
> > > +
> > > + if (info->fbops->fb_open &&
> > > + info->fbops->fb_open(info, 0)) {
> > > + module_put(info->fbops->owner);
> > > + return -ENODEV;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void fbcon_release(struct fb_info *info)
> > > +{
> > > + if (info->fbops->fb_release)
> > > + info->fbops->fb_release(info, 0);
> > > +
> > > + module_put(info->fbops->owner);
> > > +}
> > >
> > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info
> > > *info,
> > > int unit, int oldidx)
> > > {
> > > struct fbcon_ops *ops = NULL;
> > > - int err = 0;
> > > -
> > > - if (!try_module_get(info->fbops->owner))
> > > - err = -ENODEV;
> > > + int err;
> > >
> > > - if (!err && info->fbops->fb_open &&
> > > - info->fbops->fb_open(info, 0))
> > > - err = -ENODEV;
> > > + err = fbcon_open(info);
> > > + if (err)
> > > + return err;
> > >
> > > if (!err) {
> > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL);
> > > @@ -713,7 +731,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc,
> > > struct fb_info *info,
> > >
> > > if (err) {
> > > con2fb_map[unit] = oldidx;
> > > - module_put(info->fbops->owner);
> > > + fbcon_release(info);
> > > }
> > >
> > > return err;
> > > @@ -724,45 +742,34 @@ static int con2fb_release_oldinfo(struct vc_data
> > > *vc, struct fb_info *oldinfo,
> > > int oldidx, int found)
> > > {
> > > struct fbcon_ops *ops = oldinfo->fbcon_par;
> > > - int err = 0, ret;
> > > + int ret;
> > >
> > > - if (oldinfo->fbops->fb_release &&
> > > - oldinfo->fbops->fb_release(oldinfo, 0)) {
> > > - con2fb_map[unit] = oldidx;
> > The old code assigns con2fb_map[unit] before is calls
> > newinfo->fbops->fb_release).
> > I wonder if there can be any callback to fbcon where the value
> > of con2fb_map[unit] matters?
>
> It's all protected by console_lock, so other threads cannot see the
> inconsistent state.
>
> Essentially everything in fbcon.c is protected by console_lock().
>
> Do you want me to hammer this in somewhere (maybe in the commit message),
> or good enough for your ack?
No need to spell it out more.
Add my a-b and apply it.
Sam
Re: [PATCH 10/21] fb: Delete fb_info->queue
Hi Daniel, On Tue, Feb 08, 2022 at 02:46:33PM +0100, Daniel Vetter wrote: > On Thu, Feb 03, 2022 at 10:31:00PM +0100, Sam Ravnborg wrote: > > On Mon, Jan 31, 2022 at 10:05:41PM +0100, Daniel Vetter wrote: > > > It was only used by fbcon, and that now switched to its own, > > > private work. > > > > > > Signed-off-by: Daniel Vetter > > > Cc: Helge Deller > > > Cc: linux-fb...@vger.kernel.org > > I would merge this with the patch that drops the usage > > Yeah, but I like to split these out so that if this does break something, > it's much easier to revert. In case I overlooked something somewhere. > > It's imo different if the cleanup is directly related to the preceeding > prep work, but this is a generic workqueue, and the cursor logic is rather > unrelated. And if I remember my history digging right, there were actually > other uses of this. So you basically say that because in the past there may have been other users, this deserves a dedicated removal commit. That works for me too. Acked-by: Sam Ravnborg Sam
Re: [PATCH 06/21] fbcon: delete delayed loading code
Hi Daniel, On Tue, Feb 08, 2022 at 02:42:25PM +0100, Daniel Vetter wrote: > On Thu, Feb 03, 2022 at 09:45:29PM +0100, Sam Ravnborg wrote: > > Hi Daniel, > > > > On Mon, Jan 31, 2022 at 10:05:37PM +0100, Daniel Vetter wrote: > > > Before > > > > > > commit 6104c37094e729f3d4ce65797002112735d49cd1 > > > Author: Daniel Vetter > > > Date: Tue Aug 1 17:32:07 2017 +0200 > > > > > > fbcon: Make fbcon a built-time depency for fbdev > > > > > > it was possible to load fbcon and fbdev drivers in any order, which > > > means that fbcon init had to handle the case where fbdev drivers where > > > already registered. > > > > > > This is no longer possible, hence delete that code. > > > > > > Note that the exit case is a bit more complex and will be done in a > > > separate patch. > > > > > > Signed-off-by: Daniel Vetter > > > Cc: Helge Deller > > > Cc: Daniel Vetter > > > Cc: Claudio Suarez > > > Cc: Greg Kroah-Hartman > > > Cc: Tetsuo Handa > > > Cc: Du Cheng > > > --- > > > drivers/video/fbdev/core/fbcon.c | 13 + > > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > > > diff --git a/drivers/video/fbdev/core/fbcon.c > > > b/drivers/video/fbdev/core/fbcon.c > > > index 8f971de35885..814b648e8f09 100644 > > > --- a/drivers/video/fbdev/core/fbcon.c > > > +++ b/drivers/video/fbdev/core/fbcon.c > > > @@ -942,7 +942,7 @@ static const char *fbcon_startup(void) > > > return display_desc; > > > /* > > >* Instead of blindly using registered_fb[0], we use info_idx, set by > > > - * fb_console_init(); > > > + * fbcon_fb_registered(); > > >*/ > > This comment change looks like it does not belong in this patch. > > Also, the comment is wrong as info_idx is set in several places. > > Like set_con2fb_map(), fbcon_remap_all(), and more. > > Yeah I can split this out into a separate patch, but I spotted this wrong > comment as part of reviewing this code change here - essentially you have > to check how fb_info for fbcon are registered and fbcon init happens to > validate that deleting the below code is correct. > > Ok if I put this explainer into the commit message, or do you want me to > split this out fully? Keep it and add my a-b Sam
Re: [PATCH v3] drm/bridge: dw-hdmi: use safe format when first in bridge chain
Hi Neail,
On Fri, Feb 04, 2022 at 03:33:37PM +0100, Neil Armstrong wrote:
> When the dw-hdmi bridge is in first place of the bridge chain, this
> means there is no way to select an input format of the dw-hdmi HW
> component.
>
> Since introduction of display-connector, negotiation was broken since
> the dw-hdmi negotiation code only worked when the dw-hdmi bridge was
> in last position of the bridge chain or behind another bridge also
> supporting input & output format negotiation.
>
> Commit 7cd70656d128 ("drm/bridge: display-connector: implement bus fmts
> callbacks")
> was introduced to make negotiation work again by making display-connector
> act as a pass-through concerning input & output format negotiation.
>
> But in the case where the dw-hdmi is single in the bridge chain, for
> example on Renesas SoCs, with the display-connector bridge the dw-hdmi
> is no more single, breaking output format.
I have forgotten all the details during my leave from drm, so I
may miss something obvious.
This fix looks like it papers over some general thingy with the
format negotiation.
We do not want to see the below in all display drivers, so
I assume the right fix is to stuff it in somewhere in the framework.
Or do I miss something?
Sam
>
> Reported-by: Biju Das
> Bisected-by: Kieran Bingham
> Tested-by: Kieran Bingham
> Fixes: 7cd70656d128 ("drm/bridge: display-connector: implement bus fmts
> callbacks").
> Signed-off-by: Neil Armstrong
> Reviewed-by: Robert Foss
> ---
> Changes since v2:
> - Add rob's r-b
> - Fix invalid Fixes commit hash
>
> Changes since v1:
> - Remove bad fix in dw_hdmi_bridge_atomic_get_input_bus_fmts
> - Fix typos in commit message
>
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 54d8fdad395f..97cdc61b57f6 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2551,8 +2551,9 @@ static u32
> *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
> if (!output_fmts)
> return NULL;
>
> - /* If dw-hdmi is the only bridge, avoid negociating with ourselves */
> - if (list_is_singular(>encoder->bridge_chain)) {
> + /* If dw-hdmi is the first or only bridge, avoid negociating with
> ourselves */
> + if (list_is_singular(>encoder->bridge_chain) ||
> + list_is_first(>chain_node, >encoder->bridge_chain))
> {
> *num_output_fmts = 1;
> output_fmts[0] = MEDIA_BUS_FMT_FIXED;
>
> --
> 2.25.1
Re: [PATCH] drm/sprd: remove allow_fb_modifiers setting
Hi Tomohito,
On Fri, Feb 04, 2022 at 11:36:35AM +0900, Tomohito Esaki wrote:
> Remove allow_fb_modifiers setting in this driver. The allow_fb_modifiers
> flag was removed.
>
> Signed-off-by: Tomohito Esaki
> Fixes: 3d082157a242 ("drm: remove allow_fb_modifiers")
> Reported-by: kernel test robot
> ---
> drivers/gpu/drm/sprd/sprd_drm.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
> index a077e2d4d721..54c851bccf5b 100644
> --- a/drivers/gpu/drm/sprd/sprd_drm.c
> +++ b/drivers/gpu/drm/sprd/sprd_drm.c
> @@ -43,7 +43,6 @@ static void sprd_drm_mode_config_init(struct drm_device
> *drm)
> drm->mode_config.min_height = 0;
> drm->mode_config.max_width = 8192;
> drm->mode_config.max_height = 8192;
> - drm->mode_config.allow_fb_modifiers = true;
>
> drm->mode_config.funcs = _drm_mode_config_funcs;
> drm->mode_config.helper_private = _drm_mode_config_helper;
Thanks, this partially fixed my arm build.
Pushed to drm-misc-next and should appear in -next within some days.
Sam
